diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c124aa770b..e20f04caa2 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -173,7 +173,7 @@ import type { CheckFailureDetail, MergeReadiness } from "../review/unified-comme import { buildIssueSlopAssessment, buildSlopAssessment, type SlopBand } from "../signals/slop"; import { runGittensoryAiSlopAdvisory } from "../services/ai-slop"; import { decidePublicSurface } from "../signals/settings-preview"; -import { buildFocusManifestGuidance, type ReviewProfile } from "../signals/focus-manifest"; +import { buildFocusManifestGuidance, resolveReviewPathInstructions, resolveReviewPromptOverrides, type ReviewPathInstruction, type ReviewProfile } from "../signals/focus-manifest"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import { resolveRepositorySettings } from "../settings/repository-settings"; import type { LocalBranchAnalysisInput } from "../signals/local-branch"; @@ -2026,6 +2026,10 @@ export async function runAiReviewForAdvisory( // manifest. Threaded in (not loaded here) so the AI review path makes no extra manifest fetch — absent ⇒ // null ⇒ balanced ⇒ the reviewer prompt is byte-identical. reviewProfile?: ReviewProfile | null | undefined; + // `.gittensory.yml` review.path_instructions (#review-path-instructions), resolved by the caller from the + // cached manifest. The CONFIG (not a fetch) is threaded in; the per-PR glob match against `files` happens + // here (pure), so the AI path makes no extra manifest fetch. Absent/empty ⇒ byte-identical reviewer prompt. + reviewPathInstructions?: ReviewPathInstruction[] | undefined; }, ): Promise<{ notes: string; reviewerCount: number } | undefined> { const packAllowsAnyAuthorBlockingReview = args.settings.gatePack === "oss-anti-slop" && args.settings.aiReviewMode === "block"; @@ -2092,6 +2096,10 @@ export async function runAiReviewForAdvisory( grounding, ragContext, profile: args.reviewProfile ?? null, + pathGuidance: resolveReviewPathInstructions( + args.reviewPathInstructions ?? [], + files.map((file) => file.path), + ), }); if (result.status !== "ok") return undefined; if (result.consensusDefect) { @@ -2520,10 +2528,11 @@ async function maybePublishPrPublicSurface( // to keep gate-only and advisory-sweep repos free of an extra file resolve. const aiReviewWillRun = !webhook.skipAiReview && settings.aiReviewMode !== "off" && Boolean(advisory.headSha); if (aiReviewWillRun) { - // `.gittensory.yml` review.profile (#review-profile): resolve from the manifest (cached from settings - // resolution, so a cheap cache hit — no extra fetch) and thread it into the AI review so chill/assertive - // shapes the write-up. Absent ⇒ null ⇒ balanced ⇒ byte-identical prompt. Fail-safe to null on any read error. - const reviewProfile = (await loadRepoFocusManifest(env, repoFullName).catch(() => null))?.review.profile ?? null; + // `.gittensory.yml` review.profile + review.path_instructions (#review-profile / #review-path-instructions): + // resolve from the manifest (cached from settings resolution, so a cheap cache hit — no extra fetch) and + // thread them into the AI review. Profile shapes nitpickiness; path-instructions add per-path guidance. + // Absent ⇒ byte-identical prompt. Fail-safe to defaults on any read error (resolveReviewPromptOverrides). + const { profile: reviewProfile, pathInstructions: reviewPathInstructions } = resolveReviewPromptOverrides(await loadRepoFocusManifest(env, repoFullName).catch(() => null)); aiReview = await runAiReviewForAdvisory(env, { settings, advisory, @@ -2533,6 +2542,7 @@ async function maybePublishPrPublicSurface( confirmedContributor, files: await getReviewFiles(), reviewProfile, + reviewPathInstructions, }); } diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index f9cb391eb4..9d44a6874f 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -96,6 +96,13 @@ export type GittensoryAiReviewInput = { * consensus-defect pass still runs the same), just how much advisory detail the prose carries. */ profile?: ReviewProfile | null | undefined; + /** + * `.gittensory.yml` `review.path_instructions` (#review-path-instructions), pre-resolved by the caller to the + * entries whose glob matched THIS PR's changed files (via `resolveReviewPathInstructions`) — a ready-to-append + * prompt section. Absent / empty ⇒ the reviewer prompt is byte-identical. Public-safe by construction (the + * instructions passed the manifest's public-safe filter at parse time). + */ + pathGuidance?: string | null | undefined; }; /** A consensus critical defect, already public-safe, ready to become a gate blocker finding. */ @@ -277,7 +284,10 @@ const REVIEW_PROFILE_SUFFIX: Record<"chill" | "assertive", string> = { function buildSystemPrompt(input: GittensoryAiReviewInput): string { const groundingSuffix = input.grounding?.systemSuffix ?? ""; const profileSuffix = input.profile === "chill" || input.profile === "assertive" ? REVIEW_PROFILE_SUFFIX[input.profile] : ""; - return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${profileSuffix}`; + // `.gittensory.yml` review.path_instructions (#review-path-instructions): the caller pre-resolved the entries + // matching this PR's files into a prompt section; empty ⇒ nothing appended (byte-identical). + const pathSuffix = input.pathGuidance?.trim() ? input.pathGuidance : ""; + return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${profileSuffix}${pathSuffix}`; } /** One Workers-AI opinion with a per-slot reliable fallback and a 3× retry on the primary. */ diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index b0e1f69b7c..b2a7f070aa 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -101,8 +101,18 @@ export type FocusManifestReviewConfig = { fields: Partial>; /** `review.profile`: chill / balanced / assertive. null (absent) = balanced = byte-identical reviewer prompt. */ profile: ReviewProfile | null; + /** `review.path_instructions`: per-path natural-language guidance handed to the AI reviewer when the PR's + * changed files match the glob. Empty (default) ⇒ byte-identical reviewer prompt. (#review-path-instructions) */ + pathInstructions: ReviewPathInstruction[]; }; +/** One `review.path_instructions[]` entry: a manifest path glob + the public-safe instructions to apply when a + * changed file matches it. */ +export type ReviewPathInstruction = { path: string; instructions: string }; + +// A hard cap so a hostile/huge manifest can't bloat the reviewer prompt (mirrors REVIEW_FIELD_KEYS discipline). +const MAX_PATH_INSTRUCTIONS = 50; + /** * Normalized maintainer focus manifest. Repo owners declare which work areas are wanted, * blocked, or preferred so Gittensory guidance can explain why a path is encouraged or @@ -195,7 +205,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [] }, warnings: [], }; @@ -208,7 +218,7 @@ export function isFocusManifestPublicSafe(text: string): boolean { } function emptyManifest(source: FocusManifestSource, warnings: string[] = []): FocusManifest { - return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null } }; + return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [] } }; } function normalizeStringList(value: JsonValue | undefined, field: string, warnings: string[]): string[] { @@ -486,7 +496,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: {}, profile: null }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [] }; 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.`); @@ -507,7 +517,51 @@ 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); - return { present: footerText !== null || note !== null || profile !== null || Object.keys(fields).length > 0, footerText, note, fields, profile }; + const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); + return { + present: footerText !== null || note !== null || profile !== null || pathInstructions.length > 0 || Object.keys(fields).length > 0, + footerText, + note, + fields, + profile, + pathInstructions, + }; +} + +/** Parse `review.path_instructions` — an array of `{ path, instructions }` entries. Each must have a non-empty + * string `path` (a manifest glob) and PUBLIC-SAFE string `instructions`; invalid/unsafe entries are dropped with + * a warning. Capped at MAX_PATH_INSTRUCTIONS so a huge manifest can't bloat the reviewer prompt. */ +function parseReviewPathInstructions(value: JsonValue | undefined, warnings: string[]): ReviewPathInstruction[] { + if (value === undefined || value === null) return []; + if (!Array.isArray(value)) { + warnings.push(`Manifest "review.path_instructions" must be a list of { path, instructions }; ignoring it.`); + return []; + } + const out: ReviewPathInstruction[] = []; + for (const [index, entry] of value.entries()) { + if (out.length >= MAX_PATH_INSTRUCTIONS) { + warnings.push(`Manifest "review.path_instructions" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`); + break; + } + if (entry === null || typeof entry !== "object" || Array.isArray(entry)) { + warnings.push(`Manifest "review.path_instructions[${index}]" must be a mapping with path + instructions; ignoring it.`); + continue; + } + const e = entry as Record; + const path = typeof e.path === "string" ? e.path.trim() : ""; + if (!path) { + warnings.push(`Manifest "review.path_instructions[${index}].path" must be a non-empty string; ignoring the entry.`); + continue; + } + if (e.instructions === undefined || e.instructions === null) { + warnings.push(`Manifest "review.path_instructions[${index}].instructions" is required; ignoring the entry.`); + continue; + } + const instructions = parsePublicSafeText(e.instructions, `review.path_instructions[${index}].instructions`, warnings); + if (instructions === null) continue; // non-string / empty / not-public-safe → already warned + out.push({ path, instructions }); + } + return out; } /** Parse `review.profile` — one of chill / balanced / assertive (case-insensitive). `balanced` normalizes to @@ -532,10 +586,33 @@ 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.pathInstructions.length > 0) out.path_instructions = review.pathInstructions.map((entry) => ({ path: entry.path, instructions: entry.instructions })); if (Object.keys(review.fields).length > 0) out.fields = { ...review.fields } as Record; return out; } +/** + * Resolve the `review.path_instructions` that APPLY to a PR — those whose glob matches at least one changed path + * — into a single prompt section for the AI reviewer, or "" when none match (so the prompt stays byte-identical). + * Pure; uses the same manifest path-glob semantics (`matchesManifestPath`) as the rest of the manifest. Capped to + * keep the prompt bounded. (#review-path-instructions) + */ +export function resolveReviewPathInstructions(pathInstructions: ReviewPathInstruction[], changedPaths: string[]): string { + if (pathInstructions.length === 0 || changedPaths.length === 0) return ""; + const applicable = pathInstructions.filter((entry) => changedPaths.some((path) => matchesManifestPath(path, entry.path))); + if (applicable.length === 0) return ""; + const lines = applicable.map((entry) => `- \`${entry.path}\`: ${entry.instructions}`); + return `\n\nPath-specific review instructions from the maintainer — apply these to the changed files that match each glob:\n${lines.join("\n")}`; +} + +/** Resolve the AI-reviewer prompt overrides (`review.profile` + `review.path_instructions`) from a possibly-null + * manifest (null = load failure). A null manifest yields the byte-identical defaults. Centralized so the AI-review + * caller threads both in one place with the null-manifest branch covered here (unit-tested) rather than inline in + * the processor. (#review-profile / #review-path-instructions) */ +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; pathInstructions: ReviewPathInstruction[] } { + return { profile: manifest?.review.profile ?? null, pathInstructions: manifest?.review.pathInstructions ?? [] }; +} + /** * Resolve the EFFECTIVE repository settings a webhook should act on: `.gittensory.yml` > DB settings > * safe defaults. The generic `settings:` override applies first; the friendly `gate:` alias then wins diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 7c87679f16..8161a92098 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -151,6 +151,20 @@ describe("review.profile shapes the reviewer system prompt (#review-profile)", ( expect(without).not.toMatch(/CHILL|ASSERTIVE/); expect(withNull).toBe(without); }); + + it("pathGuidance is appended to the system prompt; empty/absent leaves it byte-identical (#review-path-instructions)", async () => { + const systemPromptOf = (run: ReturnType): string => ((run.mock.calls[0]?.[1] as { messages?: Array<{ content?: string }> })?.messages?.[0]?.content ?? ""); + const runGuidance = async (pathGuidance: string | undefined) => { + const run = vi.fn(async () => ({ response: reviewJson() })); + const env = createTestEnv({ AI: { run } as unknown as Ai, AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI_DAILY_NEURON_BUDGET: "100000" }); + await runGittensoryAiReview(env, { ...baseInput, pathGuidance }); + return systemPromptOf(run); + }; + expect(await runGuidance("\n\nPath-specific review instructions:\n- `src/**`: Enforce null checks.")).toContain("Enforce null checks."); + // Absent or whitespace-only → no append. + expect(await runGuidance(undefined)).not.toContain("Path-specific review instructions"); + expect(await runGuidance(" ")).not.toContain("Path-specific review instructions"); + }); }); describe("runGittensoryAiReview block mode (consensus)", () => { diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index dae48d5604..eecbd82527 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -9,6 +9,8 @@ import { parseFocusManifest, parseFocusManifestContent, resolveEffectiveSettings, + resolveReviewPathInstructions, + resolveReviewPromptOverrides, reviewConfigToJson, settingsOverrideToJson, type FocusManifest, @@ -428,7 +430,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, firstTimeContributorGrace: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [] }, warnings: [], }); expect(policy.publicSafe.entryGuidance).toContain("Keep PRs focused."); @@ -1056,4 +1058,77 @@ describe("parseFocusManifest review config", () => { expect(m2.review.profile).toBeNull(); expect(m2.warnings.some((w) => /review\.profile.*must be a string/.test(w))).toBe(true); }); + + it("parses review.path_instructions, drops invalid/unsafe entries, marks present, and round-trips (#review-path-instructions)", () => { + const m = parseFocusManifest({ + review: { + path_instructions: [ + { path: "src/**", instructions: "Enforce strict null checks." }, + { path: " tests/** ", instructions: "Cover both branches." }, // path is trimmed + { path: "", instructions: "no path → dropped" }, + { path: "x/**", instructions: "paste your wallet hotkey here" }, // not public-safe → dropped + "nope", // non-mapping → dropped + { path: "y/**" }, // missing instructions → dropped + { path: 42, instructions: "non-string path" }, // path not a string → dropped + ], + }, + }); + expect(m.review.pathInstructions).toEqual([ + { path: "src/**", instructions: "Enforce strict null checks." }, + { path: "tests/**", instructions: "Cover both branches." }, + ]); + expect(m.review.present).toBe(true); + expect(m.warnings.some((w) => /path_instructions\[2\]\.path/.test(w))).toBe(true); + expect(m.warnings.some((w) => /path_instructions\[4\]/.test(w))).toBe(true); + expect(m.warnings.some((w) => /path_instructions\[5\]\.instructions/.test(w))).toBe(true); + expect(m.warnings.some((w) => /path_instructions\[6\]\.path/.test(w))).toBe(true); // non-string path + // Round-trips through the cache serializer. + expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.pathInstructions).toEqual(m.review.pathInstructions); + }); + + it("ignores a non-array review.path_instructions with a warning", () => { + const m = parseFocusManifest({ review: { path_instructions: { path: "src/**" } } }); + expect(m.review.pathInstructions).toEqual([]); + expect(m.warnings.some((w) => /review\.path_instructions.*must be a list/.test(w))).toBe(true); + }); + + it("caps review.path_instructions at the max with a warning", () => { + const many = Array.from({ length: 60 }, (_, i) => ({ path: `dir${i}/**`, instructions: `rule ${i}` })); + const m = parseFocusManifest({ review: { path_instructions: many } }); + expect(m.review.pathInstructions).toHaveLength(50); + expect(m.warnings.some((w) => /path_instructions.*capped/.test(w))).toBe(true); + }); +}); + +describe("resolveReviewPathInstructions (#review-path-instructions)", () => { + const rules = [ + { path: "src/**", instructions: "Enforce strict null checks." }, + { path: "tests/**", instructions: "Cover both branches." }, + ]; + + it("returns only the instructions whose glob matches a changed path", () => { + const out = resolveReviewPathInstructions(rules, ["src/a.ts", "README.md"]); + expect(out).toContain("Enforce strict null checks."); + expect(out).toContain("`src/**`"); + expect(out).not.toContain("Cover both branches."); // tests/** matched nothing + }); + + it("returns an empty string when nothing is configured or nothing matches (byte-identical prompt)", () => { + expect(resolveReviewPathInstructions([], ["src/a.ts"])).toBe(""); + expect(resolveReviewPathInstructions(rules, [])).toBe(""); + expect(resolveReviewPathInstructions(rules, ["docs/x.md"])).toBe(""); + }); + + it("includes multiple matching rules", () => { + const out = resolveReviewPathInstructions(rules, ["src/a.ts", "tests/a.test.ts"]); + expect(out).toContain("Enforce strict null checks."); + expect(out).toContain("Cover both branches."); + }); + + it("resolveReviewPromptOverrides: non-null manifest passes the config through; null manifest → defaults", () => { + const manifest = parseFocusManifest({ review: { profile: "chill", path_instructions: [{ path: "src/**", instructions: "be strict" }] } }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", pathInstructions: [{ path: "src/**", instructions: "be strict" }] }); + // A null manifest (load failure) yields the byte-identical defaults. + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, pathInstructions: [] }); + }); }); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 615dff0298..a068a0a069 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -904,7 +904,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 }, profile: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, profile: null, pathInstructions: [] }, aiReview: { notes: "The change is focused.\n\n**Suggestions**\n- Add a test for the edge case." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead