Skip to content
Merged
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
22 changes: 15 additions & 7 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, resolveReviewPathInstructions, resolveReviewPromptOverrides, type ReviewPathInstruction, type ReviewProfile } from "../signals/focus-manifest";
import { buildFocusManifestGuidance, excludeReviewPaths, 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";
Expand Down Expand Up @@ -2030,6 +2030,10 @@ export async function runAiReviewForAdvisory(
// 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;
// `.gittensory.yml` review.exclude_paths (#review-exclude-paths), resolved by the caller from the cached
// manifest. Globs whose files are dropped from the AI review (diff + grounding + RAG) — generated/lockfiles
// the maintainer doesn't want reviewed. Empty ⇒ every file is reviewed (byte-identical). The gate is unaffected.
reviewExcludePaths?: string[] | undefined;
},
): Promise<{ notes: string; reviewerCount: number } | undefined> {
const packAllowsAnyAuthorBlockingReview = args.settings.gatePack === "oss-anti-slop" && args.settings.aiReviewMode === "block";
Expand Down Expand Up @@ -2059,7 +2063,9 @@ export async function runAiReviewForAdvisory(
: null;
// FIX B: prefer the caller's pre-resolved files (real diff even on a pre-sync first review); fall back to
// the stored read when the caller didn't pass them (e.g. unit tests calling this function directly).
const files = args.files ?? (await listPullRequestFiles(env, args.repoFullName, args.pr.number));
// review.exclude_paths (#review-exclude-paths): drop maintainer-excluded files (generated/lockfiles) so the
// AI review (diff + grounding + RAG) ignores them; empty excludePaths ⇒ the same array (byte-identical).
const files = excludeReviewPaths(args.files ?? (await listPullRequestFiles(env, args.repoFullName, args.pr.number)), args.reviewExcludePaths ?? []);
// Grounding (convergence, flag-gated by GITTENSORY_REVIEW_GROUNDING). Build the FINISHED CI status + the full
// content of the changed files so the reviewer verifies its claims against reality instead of guessing.
// Flag-OFF (default) → we take no new branch at all: NO check/repo load, NO file fetch, and `grounding`
Expand Down Expand Up @@ -2528,11 +2534,12 @@ 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.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));
// `.gittensory.yml` review.profile + review.path_instructions + review.exclude_paths (#review-profile /
// #review-path-instructions / #review-exclude-paths): 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; exclude-paths drop files from review. Absent ⇒
// byte-identical prompt. Fail-safe to defaults on any read error (resolveReviewPromptOverrides).
const { profile: reviewProfile, pathInstructions: reviewPathInstructions, excludePaths: reviewExcludePaths } = resolveReviewPromptOverrides(await loadRepoFocusManifest(env, repoFullName).catch(() => null));
aiReview = await runAiReviewForAdvisory(env, {
settings,
advisory,
Expand All @@ -2543,6 +2550,7 @@ async function maybePublishPrPublicSurface(
files: await getReviewFiles(),
reviewProfile,
reviewPathInstructions,
reviewExcludePaths,
});
}

Expand Down
61 changes: 51 additions & 10 deletions src/signals/focus-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,11 @@ export type FocusManifestReviewConfig = {
/** `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[];
/** `review.exclude_paths`: globs whose matching files are EXCLUDED from the AI review (diff + grounding + RAG)
* — generated/vendored/lockfiles the maintainer doesn't want reviewed. Empty (default) ⇒ every file is
* reviewed (byte-identical). Gate/slop/secret-scan are UNAFFECTED — this only narrows the AI review.
* (#review-exclude-paths) */
excludePaths: string[];
};

/** One `review.path_instructions[]` entry: a manifest path glob + the public-safe instructions to apply when a
Expand Down Expand Up @@ -205,7 +210,7 @@ const EMPTY_MANIFEST: FocusManifest = {
publicNotes: [],
gate: { ...EMPTY_GATE_CONFIG },
settings: {},
review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [] },
review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] },
warnings: [],
};

Expand All @@ -218,7 +223,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, pathInstructions: [] } };
return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] } };
}

function normalizeStringList(value: JsonValue | undefined, field: string, warnings: string[]): string[] {
Expand Down Expand Up @@ -496,7 +501,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, pathInstructions: [] };
const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] };
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 All @@ -518,16 +523,43 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo
const note = parsePublicSafeText(r.note, "review.note", warnings);
const profile = parseReviewProfile(r.profile, warnings);
const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings);
const excludePaths = parseReviewExcludePaths(r.exclude_paths, warnings);
return {
present: footerText !== null || note !== null || profile !== null || pathInstructions.length > 0 || Object.keys(fields).length > 0,
present: footerText !== null || note !== null || profile !== null || pathInstructions.length > 0 || excludePaths.length > 0 || Object.keys(fields).length > 0,
footerText,
note,
fields,
profile,
pathInstructions,
excludePaths,
};
}

/** Parse `review.exclude_paths` — an array of manifest glob strings whose matching files are excluded from the AI
* review. Each must be a non-empty string; blanks/non-strings are dropped with a warning. Capped at
* MAX_PATH_INSTRUCTIONS so a hostile manifest can't bloat the matcher. (#review-exclude-paths) */
function parseReviewExcludePaths(value: JsonValue | undefined, warnings: string[]): string[] {
if (value === undefined || value === null) return [];
if (!Array.isArray(value)) {
warnings.push(`Manifest "review.exclude_paths" must be a list of path globs; ignoring it.`);
return [];
}
const out: string[] = [];
for (const [index, entry] of value.entries()) {
if (out.length >= MAX_PATH_INSTRUCTIONS) {
warnings.push(`Manifest "review.exclude_paths" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`);
break;
}
const glob = typeof entry === "string" ? entry.trim() : "";
if (!glob) {
warnings.push(`Manifest "review.exclude_paths[${index}]" must be a non-empty string; ignoring it.`);
continue;
}
out.push(glob);
}
return out;
}

/** 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. */
Expand Down Expand Up @@ -587,6 +619,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue
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 (review.excludePaths.length > 0) out.exclude_paths = [...review.excludePaths];
if (Object.keys(review.fields).length > 0) out.fields = { ...review.fields } as Record<string, JsonValue>;
return out;
}
Expand All @@ -605,12 +638,20 @@ export function resolveReviewPathInstructions(pathInstructions: ReviewPathInstru
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 AI-reviewer overrides (`review.profile` + `review.path_instructions` + `review.exclude_paths`) 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-path-instructions / #review-exclude-paths) */
export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; pathInstructions: ReviewPathInstruction[]; excludePaths: string[] } {
return { profile: manifest?.review.profile ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], excludePaths: manifest?.review.excludePaths ?? [] };
}

/** Filter a PR's changed files down to the set the AI review should see — dropping any whose path matches a
* `review.exclude_paths` glob (generated/vendored/lockfiles). Empty `excludePaths` ⇒ the same array (byte-identical
* review). Pure; the gate/slop/secret-scan operate on the unfiltered files. (#review-exclude-paths) */
export function excludeReviewPaths<T extends { path: string }>(files: T[], excludePaths: string[]): T[] {
if (excludePaths.length === 0) return files;
return files.filter((file) => !excludePaths.some((glob) => matchesManifestPath(file.path, glob)));
}

/**
Expand Down
37 changes: 33 additions & 4 deletions test/unit/focus-manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
parseFocusManifest,
parseFocusManifestContent,
resolveEffectiveSettings,
excludeReviewPaths,
resolveReviewPathInstructions,
resolveReviewPromptOverrides,
reviewConfigToJson,
Expand Down Expand Up @@ -430,7 +431,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, pathInstructions: [] },
review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] },
warnings: [],
});
expect(policy.publicSafe.entryGuidance).toContain("Keep PRs focused.");
Expand Down Expand Up @@ -1126,9 +1127,37 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => {
});

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" }] });
const manifest = parseFocusManifest({ review: { profile: "chill", path_instructions: [{ path: "src/**", instructions: "be strict" }], exclude_paths: ["**/*.lock"] } });
expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", pathInstructions: [{ path: "src/**", instructions: "be strict" }], excludePaths: ["**/*.lock"] });
// A null manifest (load failure) yields the byte-identical defaults.
expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, pathInstructions: [] });
expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, pathInstructions: [], excludePaths: [] });
});
});

describe("review.exclude_paths (#review-exclude-paths)", () => {
it("parses exclude_paths, trims, drops blanks/non-strings with warnings, marks present, and round-trips", () => {
const m = parseFocusManifest({ review: { exclude_paths: [" **/*.lock ", "dist/**", "", 42, " "] } });
expect(m.review.excludePaths).toEqual(["**/*.lock", "dist/**"]);
expect(m.review.present).toBe(true);
expect(m.warnings.some((w) => /exclude_paths\[2\]/.test(w))).toBe(true); // empty string
expect(m.warnings.some((w) => /exclude_paths\[3\]/.test(w))).toBe(true); // non-string
expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.excludePaths).toEqual(m.review.excludePaths);
});

it("ignores a non-array exclude_paths and caps the list", () => {
const bad = parseFocusManifest({ review: { exclude_paths: "dist/**" } });
expect(bad.review.excludePaths).toEqual([]);
expect(bad.warnings.some((w) => /exclude_paths.*must be a list/.test(w))).toBe(true);
const many = parseFocusManifest({ review: { exclude_paths: Array.from({ length: 60 }, (_, i) => `dir${i}/**`) } });
expect(many.review.excludePaths).toHaveLength(50);
expect(many.warnings.some((w) => /exclude_paths.*capped/.test(w))).toBe(true);
});

it("excludeReviewPaths filters matching files; empty globs return the same array (byte-identical)", () => {
const files = [{ path: "src/a.ts" }, { path: "pnpm-lock.yaml" }, { path: "dist/bundle.js" }];
// `*` collapses to `.*` (crosses slashes), so `*.yaml` matches a top-level lockfile; `dist/**` matches under dist/.
expect(excludeReviewPaths(files, ["*.yaml", "dist/**"])).toEqual([{ path: "src/a.ts" }]);
expect(excludeReviewPaths(files, ["docs/**"])).toEqual(files); // no match → unchanged
expect(excludeReviewPaths(files, [])).toBe(files); // empty → same reference (no-op)
});
});
2 changes: 1 addition & 1 deletion test/unit/signals-coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, pathInstructions: [] },
review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, profile: null, pathInstructions: [], excludePaths: [] },
aiReview: { notes: "The change is focused.\n\n**Suggestions**\n- Add a test for the </details> edge case." },
});
expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead
Expand Down
Loading