diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 8d9b6da90e..a9b3aac662 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -144,6 +144,8 @@ export type PreMergeCheck = { // A hard cap so a hostile/huge manifest can't bloat the reviewer prompt (mirrors REVIEW_FIELD_KEYS discipline). const MAX_PATH_INSTRUCTIONS = 50; +const MAX_REVIEW_PATH_GUIDANCE_LENGTH = 4_000; +const CONTROL_CHARACTER_PATTERN = /[\u0000-\u001f\u007f]/; /** * Normalized maintainer focus manifest. Repo owners declare which work areas are wanted, @@ -662,8 +664,8 @@ function parseReviewExcludePaths(value: JsonValue | undefined, warnings: string[ } /** 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. */ + * bounded 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)) { @@ -681,15 +683,19 @@ function parseReviewPathInstructions(value: JsonValue | undefined, warnings: str continue; } const e = entry as Record; - const path = typeof e.path === "string" ? e.path.trim() : ""; + let 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 (path.length > MAX_ITEM_LENGTH) { - warnings.push(`Manifest "review.path_instructions[${index}].path" exceeds ${MAX_ITEM_LENGTH} chars; ignoring the entry.`); + if (CONTROL_CHARACTER_PATTERN.test(path)) { + warnings.push(`Manifest "review.path_instructions[${index}].path" must not contain control characters; ignoring the entry.`); continue; } + if (path.length > MAX_ITEM_LENGTH) { + warnings.push(`Manifest "review.path_instructions[${index}].path" truncated an over-long entry.`); + path = path.slice(0, MAX_ITEM_LENGTH); + } if (e.instructions === undefined || e.instructions === null) { warnings.push(`Manifest "review.path_instructions[${index}].instructions" is required; ignoring the entry.`); continue; @@ -749,10 +755,32 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue */ 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))); + const normalizedChangedPaths = changedPaths.map(normalizePathForMatch).filter(Boolean); + if (normalizedChangedPaths.length === 0) return ""; + const applicable = pathInstructions.filter((entry) => { + const matches = compileManifestPathMatcher(entry.path); + return normalizedChangedPaths.some((path) => matches(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")}`; + + const header = "\n\nPath-specific review instructions from the maintainer — apply these to the changed files that match each glob:"; + const lines: string[] = []; + let length = header.length; + let omitted = 0; + for (const entry of applicable) { + const line = `- \`${entry.path}\`: ${entry.instructions}`; + const nextLength = length + 1 + line.length; + if (nextLength > MAX_REVIEW_PATH_GUIDANCE_LENGTH) { + omitted += 1; + continue; + } + lines.push(line); + length = nextLength; + } + if (lines.length === 0) return ""; + const omittedLine = omitted > 0 ? `\n- (${omitted} additional matching path instruction(s) omitted to keep the reviewer prompt bounded.)` : ""; + const guidance = `${header}\n${lines.join("\n")}${omittedLine}`; + return guidance.length > MAX_REVIEW_PATH_GUIDANCE_LENGTH ? guidance.slice(0, MAX_REVIEW_PATH_GUIDANCE_LENGTH) : guidance; } /** Resolve the AI-reviewer overrides (`review.profile` + `review.path_instructions` + `review.exclude_paths`) from diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index b64ad7ce8d..405c13cc20 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1129,7 +1129,7 @@ describe("parseFocusManifest review config", () => { "nope", // non-mapping → dropped { path: "y/**" }, // missing instructions → dropped { path: 42, instructions: "non-string path" }, // path not a string → dropped - { path: `${"a".repeat(400)}/x`, instructions: "over-long path" }, // > MAX_ITEM_LENGTH → dropped (#review-audit) + { path: "bad\npath", instructions: "control path" }, // control character path → dropped ], }, }); @@ -1142,7 +1142,7 @@ describe("parseFocusManifest review config", () => { 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 - expect(m.warnings.some((w) => /path_instructions\[7\]\.path.*exceeds/.test(w))).toBe(true); // over-long path + expect(m.warnings.some((w) => /path_instructions\[7\]\.path.*control characters/.test(w))).toBe(true); // Round-trips through the cache serializer. expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.pathInstructions).toEqual(m.review.pathInstructions); }); @@ -1159,6 +1159,15 @@ describe("parseFocusManifest review config", () => { expect(m.review.pathInstructions).toHaveLength(50); expect(m.warnings.some((w) => /path_instructions.*capped/.test(w))).toBe(true); }); + + it("caps review.path_instructions paths at the manifest item length with a warning", () => { + const longGlob = `src/${"a".repeat(400)}/**`; + const m = parseFocusManifest({ review: { path_instructions: [{ path: longGlob, instructions: "keep it bounded" }] } }); + expect(m.review.pathInstructions).toHaveLength(1); + expect(m.review.pathInstructions[0]?.path).toHaveLength(300); + expect(m.review.pathInstructions[0]?.path).toBe(longGlob.slice(0, 300)); + expect(m.warnings.some((w) => /path_instructions\[0\]\.path.*over-long/.test(w))).toBe(true); + }); }); describe("resolveReviewPathInstructions (#review-path-instructions)", () => { @@ -1186,6 +1195,19 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(out).toContain("Cover both branches."); }); + it("bounds the resolved path guidance prompt section", () => { + const many = Array.from({ length: 50 }, (_, i) => ({ path: `src/${i}/**`, instructions: "x".repeat(300) })); + const changedPaths = many.map((entry) => `${entry.path.slice(0, -3)}/file.ts`); + const out = resolveReviewPathInstructions(many, changedPaths); + expect(out.length).toBeLessThanOrEqual(4_000); + expect(out).toContain("Path-specific review instructions"); + expect(out).toContain("omitted to keep the reviewer prompt bounded"); + }); + + it("returns an empty string when changed paths normalize to blanks", () => { + expect(resolveReviewPathInstructions(rules, ["", "///"])).toBe(""); + }); + it("resolveReviewPromptOverrides: non-null manifest passes the config through; null manifest → defaults", () => { const manifest = parseFocusManifest({ review: { profile: "chill", inline_comments: true, path_instructions: [{ path: "src/**", instructions: "be strict" }], exclude_paths: ["**/*.lock"] } }); expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", inlineComments: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], excludePaths: ["**/*.lock"] });