From 88dc1c3a01a54aa85ec90bb63bbb9a0ef1c617a8 Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Mon, 6 Jul 2026 08:06:16 -0500 Subject: [PATCH] feat(config): add review.fixHandoff toggle (default off) for fix-handoff blocks (#2176) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Config slice for #1962: a per-repo boolean under review that gates whether fix-handoff blocks are emitted, ANDed with a global env kill-switch + the convergence cutover allowlist. Default-OFF, byte-identical when unset. Follows the inline_comments precedent. - focus-manifest.ts: fixHandoff: boolean|null on FocusManifestReviewConfig (mirror inlineComments) — normalizeOptionalBoolean parse, default null; round-trip serialize (omitted when null ⇒ byte-identical) + present + EMPTY literals. - src/review/fix-handoff.ts: isFixHandoffEnabled(env) kill-switch (mirror isInlineCommentsEnabled) + pure shouldEmitFixHandoff() ANDing manifest toggle + env flag + cutover allowlist (mirror shouldRequestInlineFindings). - Documented review.fixHandoff in BOTH .gittensory.yml.example and config/examples/gittensory.full.yml (the config-templates sync test enforces parity). - Tests: config absent/true/false round-trip + non-boolean warn; env truthy/falsy; resolver three-way gate (manifest AND env AND cutover). Verified: full suite (npm run test) green — 10349 passed, 0 failed. --- .gittensory.yml.example | 4 +++ config/examples/gittensory.full.yml | 4 +++ src/review/fix-handoff.ts | 25 ++++++++++++++ src/signals/focus-manifest.ts | 14 ++++++-- test/unit/focus-manifest.test.ts | 3 +- test/unit/review-fix-handoff.test.ts | 51 ++++++++++++++++++++++++++++ test/unit/signals-coverage.test.ts | 2 +- 7 files changed, 98 insertions(+), 5 deletions(-) create mode 100644 src/review/fix-handoff.ts create mode 100644 test/unit/review-fix-handoff.test.ts diff --git a/.gittensory.yml.example b/.gittensory.yml.example index b941c24de7..75726c0545 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -436,6 +436,10 @@ review: # finding_categories: false # When true, inline findings are tagged with a category label (requires inline_comments). + # Fix-handoff blocks (#2176). Bool | null. Default: null/false — byte-identical. + # Requires operator flag GITTENSORY_REVIEW_FIX_HANDOFF + cutover allowlist AND this toggle. + # fixHandoff: false + # Boundary-safe test generation (#2189). Bool | null. Default: null/false — byte-identical. # Requires operator flag GITTENSORY_REVIEW_TEST_GENERATION AND this toggle. # test_generation: false diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index 33f778bb3a..5020b8a982 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -449,6 +449,10 @@ review: # finding_categories: false # When true, inline findings are tagged with a category label (requires inline_comments). + # Fix-handoff blocks (#2176). Bool | null. Default: null/false — byte-identical. + # Requires operator flag GITTENSORY_REVIEW_FIX_HANDOFF + cutover allowlist AND this toggle. + # fixHandoff: false + # Boundary-safe test generation (#2189). Bool | null. Default: null/false — byte-identical. # Requires operator flag GITTENSORY_REVIEW_TEST_GENERATION AND this toggle. # test_generation: false diff --git a/src/review/fix-handoff.ts b/src/review/fix-handoff.ts new file mode 100644 index 0000000000..3bf40ed4f9 --- /dev/null +++ b/src/review/fix-handoff.ts @@ -0,0 +1,25 @@ +// 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"; + +/** 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. */ +export function shouldEmitFixHandoff( + 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); +} diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index d6b4a23cae..f73c54f6fc 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -340,6 +340,10 @@ export type FocusManifestReviewConfig = { * comments = byte-identical behavior. Operator-gated too (GITTENSORY_REVIEW_INLINE_COMMENTS + allowlist). * (#inline-comments) */ inlineComments: boolean | null; + /** `review.fixHandoff`: when true, the reviewer emits fix-handoff blocks (copy-paste remediation guidance). null/ + * false (default, absent) = no fix-handoff blocks = byte-identical. Operator-gated too (GITTENSORY_REVIEW_FIX_HANDOFF + * + the convergence cutover allowlist) — the manifest toggle is only one of the ANDed gates. (#2176, for #1962) */ + fixHandoff: boolean | null; /** `review.suggestions`: when true, an inline finding whose AI-provided fix is precise enough to anchor to a * single line is ALSO rendered as a GitHub-native ` ```suggestion ` block a contributor can commit in one * click. Only takes effect when inline comments are already on (a suggestion has nothing to attach to @@ -751,7 +755,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: 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 }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: 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 }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -782,7 +786,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: 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 }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: 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 }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1757,7 +1761,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: 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 }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: 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 }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -1794,6 +1798,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const tone = parsePublicSafeText(r.tone, "review.tone", warnings); const securityFocus = normalizeOptionalBoolean(r.security_focus, "review.security_focus", warnings); const inlineComments = normalizeOptionalBoolean(r.inline_comments, "review.inline_comments", warnings); + const fixHandoff = normalizeOptionalBoolean(r.fixHandoff, "review.fixHandoff", warnings); const suggestions = normalizeOptionalBoolean(r.suggestions, "review.suggestions", warnings); const changedFilesSummary = normalizeOptionalBoolean(r.changed_files_summary, "review.changed_files_summary", warnings); const effortScore = normalizeOptionalBoolean(r.effort_score, "review.effort_score", warnings); @@ -1826,6 +1831,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo tone !== null || securityFocus !== null || inlineComments !== null || + fixHandoff !== null || suggestions !== null || changedFilesSummary !== null || effortScore !== null || @@ -1860,6 +1866,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo tone, securityFocus, inlineComments, + fixHandoff, suggestions, changedFilesSummary, effortScore, @@ -2321,6 +2328,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.tone !== null) out.tone = review.tone; if (review.securityFocus !== null) out.security_focus = review.securityFocus; if (review.inlineComments !== null) out.inline_comments = review.inlineComments; + if (review.fixHandoff !== null) out.fixHandoff = review.fixHandoff; if (review.suggestions !== null) out.suggestions = review.suggestions; if (review.changedFilesSummary !== null) out.changed_files_summary = review.changedFilesSummary; if (review.effortScore !== null) out.effort_score = review.effortScore; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 0168f28b22..22ea4732b3 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -355,6 +355,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { tone: "tone:", securityFocus: "security_focus:", inlineComments: "inline_comments:", + fixHandoff: "fixHandoff:", suggestions: "suggestions:", changedFilesSummary: "changed_files_summary:", effortScore: "effort_score:", @@ -783,7 +784,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], 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, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: 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 }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: 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 }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: 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 }, diff --git a/test/unit/review-fix-handoff.test.ts b/test/unit/review-fix-handoff.test.ts new file mode 100644 index 0000000000..fd6e87cbf9 --- /dev/null +++ b/test/unit/review-fix-handoff.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vitest"; +import { parseFocusManifest, reviewConfigToJson } from "../../src/signals/focus-manifest"; +import { isFixHandoffEnabled, shouldEmitFixHandoff } from "../../src/review/fix-handoff"; + +const reviewOf = (fixHandoff: unknown) => parseFocusManifest({ review: { fixHandoff } }); +const ON = "acme/widgets"; +const ALLOW = { GITTENSORY_REVIEW_FIX_HANDOFF: "1", GITTENSORY_REVIEW_REPOS: ON }; + +describe("review.fixHandoff config toggle (#2176)", () => { + it("absent ⇒ null and OMITTED on serialize (byte-identical)", () => { + const review = parseFocusManifest({ review: { note: "x" } }).review; + expect(review.fixHandoff).toBe(null); + expect("fixHandoff" in (reviewConfigToJson(review) as Record)).toBe(false); + }); + + it("true / false parse, mark present, and round-trip", () => { + for (const v of [true, false]) { + const review = reviewOf(v).review; + expect(review.fixHandoff).toBe(v); + expect(review.present).toBe(true); + const json = reviewConfigToJson(review) as Record; + expect(json.fixHandoff).toBe(v); + expect(parseFocusManifest({ review: json }).review.fixHandoff).toBe(v); + } + }); + + it("a non-boolean value warns and falls back to null", () => { + const m = reviewOf("maybe"); + expect(m.review.fixHandoff).toBe(null); + expect(m.warnings.some((w) => /review\.fixHandoff/.test(w))).toBe(true); + }); +}); + +describe("fix-handoff env kill-switch + resolver (#2176)", () => { + 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 + expect(shouldEmitFixHandoff(ALLOW, ON, true)).toBe(true); + // manifest toggle off / undefined + 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); + }); +}); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index d896efe172..a2cc892813 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, suggestions: null, changedFilesSummary: null, effortScore: null, testGeneration: null, impactMap: null, findingCategories: null, minFindingSeverity: null, maxFindings: { blockers: null, nits: null }, commentVerbosity: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: null }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead