Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gittensory.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ import { decidePublicSurface } from "../signals/settings-preview";
import {
buildFocusManifestGuidance,
composeRepoReviewContext,
composeManifestReviewInstructions,
filterReviewFilesForAi,
resolvePullRequestAutoReviewSkipReason,
resolveRepoEnrichmentToggles,
Expand Down Expand Up @@ -8136,6 +8137,7 @@ async function maybePublishPrPublicSurface(
inlineComments: reviewInlineComments,
pathInstructions: reviewPathInstructions,
instructions: manifestReviewInstructions,
tone: reviewTone,
excludePaths: reviewExcludePaths,
pathFilters: reviewPathFilters,
} = resolveReviewPromptOverrides(reviewManifest);
Expand All @@ -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,
Expand Down
32 changes: 25 additions & 7 deletions src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,9 @@ export type FocusManifestReviewConfig = {
enrichmentAnalyzers: Partial<Record<ReesAnalyzerName, boolean>>;
/** `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,
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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.`);
Expand Down Expand Up @@ -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);
Expand All @@ -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 ||
Expand All @@ -1515,6 +1520,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo
fields,
enrichmentAnalyzers,
profile,
tone,
securityFocus,
inlineComments,
pathInstructions,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
31 changes: 28 additions & 3 deletions test/unit/focus-manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
evaluateAutoReviewSkipReason,
resolveAutoReviewConfig,
resolveReviewPromptOverrides,
composeManifestReviewInstructions,
EMPTY_AUTO_REVIEW_CONFIG,
repoDocGenerationConfigToJson,
reviewConfigToJson,
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion test/unit/signals-coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 </details> edge case.\n- Keep the validator helper scoped." },
});
expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead
Expand Down