diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 341ae17be8..1ae642a52e 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -576,6 +576,8 @@ settings: # path_filters: # - "src/**" # - "!src/generated/**" +# # Public-safe voice brief complementing review.profile (e.g. concise, cite line numbers). null/unset ⇒ byte-identical prompt. +# tone: "Be concise and cite line numbers." # # Deterministic AI review eligibility filters — skipped PRs never fail the gate. (#1954) # auto_review: # skip_drafts: true diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c9e5c3db90..388025ec97 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -357,6 +357,7 @@ import { decidePublicSurface } from "../signals/settings-preview"; import { buildFocusManifestGuidance, composeRepoReviewContext, + composeManifestReviewInstructions, filterReviewFilesForAi, resolvePullRequestAutoReviewSkipReason, resolveRepoEnrichmentToggles, @@ -8136,6 +8137,7 @@ async function maybePublishPrPublicSurface( inlineComments: reviewInlineComments, pathInstructions: reviewPathInstructions, instructions: manifestReviewInstructions, + tone: reviewTone, excludePaths: reviewExcludePaths, pathFilters: reviewPathFilters, } = resolveReviewPromptOverrides(reviewManifest); @@ -8153,7 +8155,7 @@ async function maybePublishPrPublicSurface( // byte-identical prompt. getReviewFiles() is memoized, so the second call reuses the loaded diff. const reviewInstructions = [ - manifestReviewInstructions, + composeManifestReviewInstructions(manifestReviewInstructions, reviewTone), composeRepoReviewContext( await loadRepoReviewContext(repoFullName), changedPaths, diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index e212b825b0..ccbfbbaf37 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -301,6 +301,9 @@ export type FocusManifestReviewConfig = { enrichmentAnalyzers: Partial>; /** `review.profile`: chill / balanced / assertive. null (absent) = balanced = byte-identical reviewer prompt. */ profile: ReviewProfile | null; + /** `review.tone`: a bounded public-safe voice brief complementing `review.profile` (e.g. "concise, cite line numbers"). + * Folded into the review-instructions slot at runtime. null (default, absent) ⇒ byte-identical prompt. (#2044) */ + tone: string | null; /** `review.security_focus`: when true, the AI reviewer is told to prioritize a security-defect category * (injection, authn/authz bypass, secret handling, unsafe deserialization, SSRF, path traversal) with * elevated scrutiny, ON TOP OF whatever `profile` volume is set — an orthogonal "what to prioritize" axis, @@ -517,7 +520,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -547,7 +550,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, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1453,7 +1456,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, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }; 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.`); @@ -1487,6 +1490,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const footerText = footerRecord ? parsePublicSafeText(footerRecord.text, "review.footer.text", warnings) : null; const note = parsePublicSafeText(r.note, "review.note", warnings); const profile = parseReviewProfile(r.profile, warnings); + 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 pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); @@ -1500,6 +1504,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo footerText !== null || note !== null || profile !== null || + tone !== null || securityFocus !== null || inlineComments !== null || pathInstructions.length > 0 || @@ -1515,6 +1520,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo fields, enrichmentAnalyzers, profile, + tone, securityFocus, inlineComments, pathInstructions, @@ -1740,6 +1746,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.footerText !== null) out.footer = { text: review.footerText }; if (review.note !== null) out.note = review.note; if (review.profile !== null) out.profile = review.profile; + 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.instructions !== null) out.instructions = review.instructions; @@ -1836,16 +1843,27 @@ export function resolvePullRequestAutoReviewSkipReason(args: { }); } -/** Resolve the AI-reviewer overrides (`review.profile` + `review.security_focus` + `review.path_instructions` + +/** Fold `review.tone` into the repo-instructions slot alongside `review.instructions` so both inherit the same + * public-safe system append in the AI reviewer. Null/empty tone ⇒ instructions unchanged (byte-identical). (#2044) */ +export function composeManifestReviewInstructions(instructions: string | null, tone: string | null): string | null { + const toneText = tone?.trim() || null; + const instructionText = instructions?.trim() || null; + if (!toneText) return instructionText; + const toneSection = `Review tone (maintainer voice brief — complements review.profile): ${toneText}`; + if (!instructionText) return toneSection; + return `${toneSection}\n\n${instructionText}`; +} + +/** Resolve the AI-reviewer overrides (`review.profile` + `review.tone` + `review.security_focus` + `review.path_instructions` + * `review.exclude_paths` + `review.path_filters`) from a possibly-null manifest (null = load 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-security-focus / #review-path-instructions / #review-exclude-paths / #2043) */ -export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; securityFocus: boolean; inlineComments: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[] } { + * (#review-profile / #review-tone / #review-security-focus / #review-path-instructions / #review-exclude-paths / #2043) */ +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[] } { // 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. // securityFocus resolves the same way — true ONLY when the manifest explicitly set review.security_focus: true. - return { profile: manifest?.review.profile ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [] }; + return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [] }; } /** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 0aa3a68c7b..0cd1d2e990 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -21,6 +21,7 @@ import { evaluateAutoReviewSkipReason, resolveAutoReviewConfig, resolveReviewPromptOverrides, + composeManifestReviewInstructions, EMPTY_AUTO_REVIEW_CONFIG, repoDocGenerationConfigToJson, reviewConfigToJson, @@ -546,7 +547,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, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG } }, 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 }, @@ -2426,6 +2427,30 @@ describe("parseFocusManifest review config", () => { expect(m2.warnings.some((w) => /review\.profile.*must be a string/.test(w))).toBe(true); }); + it("parses review.tone, marks present, round-trips, and rejects non-public-safe values (#2044)", () => { + const m = parseFocusManifest({ review: { tone: " Be concise and cite line numbers. " } }); + expect(m.review.tone).toBe("Be concise and cite line numbers."); + expect(m.review.present).toBe(true); + expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.tone).toBe(m.review.tone); + const unsafe = parseFocusManifest({ review: { tone: "estimate the contributor reward payout" } }); + expect(unsafe.review.tone).toBeNull(); + expect(unsafe.warnings.some((w) => /review\.tone.*not public-safe/.test(w))).toBe(true); + const long = parseFocusManifest({ review: { tone: "x".repeat(400) } }); + expect(long.review.tone).toHaveLength(300); + }); + + it("composeManifestReviewInstructions: null tone is byte-identical; tone folds ahead of instructions (#2044)", () => { + expect(composeManifestReviewInstructions(null, null)).toBeNull(); + expect(composeManifestReviewInstructions("Follow our conventions.", null)).toBe("Follow our conventions."); + expect(composeManifestReviewInstructions(null, "Be concise.")).toBe( + "Review tone (maintainer voice brief — complements review.profile): Be concise.", + ); + expect(composeManifestReviewInstructions("Follow our conventions.", "Be concise.")).toBe( + "Review tone (maintainer voice brief — complements review.profile): Be concise.\n\nFollow our conventions.", + ); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { tone: "Be concise." } })).tone).toBe("Be concise."); + }); + it("parses review.path_instructions, drops invalid/unsafe entries, marks present, and round-trips (#review-path-instructions)", () => { const m = parseFocusManifest({ review: { @@ -2522,9 +2547,9 @@ 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, 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", securityFocus: true, inlineComments: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"] }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"] }); // A null manifest (load failure) yields the byte-identical defaults; inline comments + security focus default OFF. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [] }); + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [] }); // 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: { profile: "chill" } })).inlineComments).toBe(false); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 3496904c43..3c786deef4 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, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [] } }, + 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, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [] } }, 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