diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 95d3ac7a16..8cda9e21ae 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5124,6 +5124,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.security_focus (#review-security-focus), resolved by the caller from the + // (already-cached) manifest. Orthogonal to reviewProfile — composes with it rather than replacing it. + // Absent/false ⇒ the reviewer prompt is byte-identical. + reviewSecurityFocus?: boolean | 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. @@ -5359,6 +5363,7 @@ export async function runAiReviewForAdvisory( observability: { rag: ragTelemetry }, enrichment, profile: args.reviewProfile ?? null, + securityFocus: args.reviewSecurityFocus === true, // Inline comments (#inline-comments): ask the model for line-anchored findings only when the operator flag, // the cutover allowlist, AND the per-repo manifest toggle all pass. Otherwise the prompt is byte-identical. inlineFindings: shouldRequestInlineFindings( @@ -6478,13 +6483,16 @@ async function maybePublishPrPublicSurface( }, async () => { const reviewManifest = 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). + // `.gittensory.yml` review.profile + review.security_focus + review.path_instructions + + // review.exclude_paths (#review-profile / #review-security-focus / #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; security-focus + // adds elevated scrutiny for a security-defect category (orthogonal to profile); 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, + securityFocus: reviewSecurityFocus, inlineComments: reviewInlineComments, pathInstructions: reviewPathInstructions, instructions: manifestReviewInstructions, @@ -6565,6 +6573,7 @@ async function maybePublishPrPublicSurface( } : null, profile: reviewProfile, + securityFocus: reviewSecurityFocus, inlineComments: inlineCommentsEnabledForReview, pathInstructions: reviewPathInstructions, pathGuidance: resolveReviewPathInstructions( @@ -6615,6 +6624,7 @@ async function maybePublishPrPublicSurface( confirmedContributor, files: reviewFilesForAi, reviewProfile, + reviewSecurityFocus, reviewPathInstructions, reviewInstructions, reviewExcludePaths, diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index 1d2176d831..e5ba50b5c1 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -55,6 +55,7 @@ export type AiReviewCacheInput = { | null | undefined; profile: ReviewProfile | null | undefined; + securityFocus: boolean; inlineComments: boolean; pathInstructions: readonly ReviewPathInstruction[]; pathGuidance: string; @@ -122,6 +123,7 @@ export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): } : null, profile: input.profile ?? null, + securityFocus: input.securityFocus, inlineComments: input.inlineComments, pathInstructions: input.pathInstructions.map((instruction) => ({ path: instruction.path, diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 2af8cd8950..f5dedee730 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -161,6 +161,14 @@ 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.security_focus` (#review-security-focus): when true, instructs the reviewer to + * prioritize a security-defect category — injection, authn/authz bypass, secret handling, unsafe + * deserialization, SSRF, and path traversal — with elevated scrutiny. ORTHOGONAL to `profile`: it composes + * with (never replaces) the chill/balanced/assertive volume tuning above — a "what to prioritize" axis, not a + * fourth profile level. Absent/false (the default) ⇒ the reviewer prompt is byte-identical to today. + */ + securityFocus?: boolean | 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 @@ -533,6 +541,13 @@ const REVIEW_PROFILE_SUFFIX: Record<"chill" | "assertive", string> = { "\n\nReview profile: ASSERTIVE. Beyond blocking defects, also surface minor improvements, style/consistency suggestions, and nitpicks — be thorough and exacting, clearly marking each non-blocking item as a nit.", }; +// `.gittensory.yml` review.security_focus → an appended security-prioritization instruction (#review-security-focus). +// ORTHOGONAL to REVIEW_PROFILE_SUFFIX above — it composes with (never replaces) the chill/balanced/assertive volume +// tuning: profile controls HOW MANY findings surface, this controls WHAT KIND the reviewer hunts for with elevated +// scrutiny. False/absent (default) appends nothing (byte-identical). +const SECURITY_FOCUS_SUFFIX = + "\n\nSECURITY FOCUS: Beyond the usual review, prioritize hunting for security defects with elevated scrutiny — injection (SQL/command/template/log), authentication/authorization bypass, unsafe secret handling (hardcoded credentials, logged/leaked tokens), unsafe deserialization, server-side request forgery (SSRF), and path traversal. Treat a credible finding in any of these categories as a blocker even if it would otherwise read as a nit."; + // `.gittensory.yml` review.inline_comments → an appended instruction to ALSO emit line-anchored findings for // quiet inline PR comments (#inline-comments). Absent/off appends nothing (byte-identical). The model keeps the // existing 4-field shape and simply ADDS an `inlineFindings` array. @@ -540,8 +555,9 @@ const INLINE_FINDINGS_SUFFIX = '\n\nINLINE FINDINGS: ALSO include an additional top-level field "inlineFindings" in the SAME JSON object — an array (possibly empty) of your most important findings, each anchored to a specific changed line, for inline PR comments. Each item: {"path": the changed file path EXACTLY as shown in the diff, "line": the 1-based line number in the NEW file (count forward from the "+" start in the nearest "@@ -old +new @@" hunk header) of an ADDED ("+") line you are commenting on, "severity": "blocker" or "nit", "body": the one-sentence finding, "suggestion": optional replacement text for that line}. Include ONLY findings you can place on a specific added line; OMIT any you cannot anchor precisely (a wrong line is worse than none). If a suggestion is blank or you are not confident in an exact replacement, omit the suggestion field and keep the finding. At most ~10 items.'; /** The effective reviewer SYSTEM prompt. Appends the grounding-discipline suffix when the caller supplied one - * (flag GITTENSORY_REVIEW_GROUNDING on), the `review.profile` tone suffix when set, then the inline-findings - * instruction when the caller asked for them; all absent (default) → the base prompt, byte-identical to today. */ + * (flag GITTENSORY_REVIEW_GROUNDING on), the `review.profile` tone suffix when set, the `review.security_focus` + * prioritization suffix when on, then the inline-findings instruction when the caller asked for them; all absent + * (default) → the base prompt, byte-identical to today. */ function buildSystemPrompt(input: GittensoryAiReviewInput): string { const groundingSuffix = input.grounding?.systemSuffix ?? ""; // Review-enrichment brief (#1472): the REES supplies a one-line discipline suffix ("treat a listed CVE/secret as @@ -551,6 +567,7 @@ function buildSystemPrompt(input: GittensoryAiReviewInput): string { input.profile === "chill" || input.profile === "assertive" ? REVIEW_PROFILE_SUFFIX[input.profile] : ""; + const securityFocusSuffix = input.securityFocus === true ? SECURITY_FOCUS_SUFFIX : ""; // `.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 : ""; @@ -560,7 +577,7 @@ function buildSystemPrompt(input: GittensoryAiReviewInput): string { ? ` REPOSITORY REVIEW INSTRUCTIONS (maintainer conventions for this repo — honor them unless they conflict with a real defect): ${input.repoInstructions.trim()}` : ""; const inlineSuffix = input.inlineFindings ? INLINE_FINDINGS_SUFFIX : ""; - return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${enrichmentSuffix}${profileSuffix}${pathSuffix}${repoInstructionsSuffix}${inlineSuffix}`; + return `${REVIEW_SYSTEM_PROMPT}${groundingSuffix}${enrichmentSuffix}${profileSuffix}${securityFocusSuffix}${pathSuffix}${repoInstructionsSuffix}${inlineSuffix}`; } /** 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 549d533093..d152059ad7 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -184,6 +184,11 @@ export type FocusManifestReviewConfig = { fields: Partial>; /** `review.profile`: chill / balanced / assertive. null (absent) = balanced = byte-identical reviewer prompt. */ profile: ReviewProfile | 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, + * not a fourth profile level. null/false (default, absent) = byte-identical reviewer prompt. (#review-security-focus) */ + securityFocus: boolean | null; /** `review.inline_comments`: when true, the AI reviewer ALSO leaves quiet, non-blocking inline PR comments on * specific changed lines (in addition to the decision summary). null/false (default, absent) = no inline * comments = byte-identical behavior. Operator-gated too (GITTENSORY_REVIEW_INLINE_COMMENTS + allowlist). @@ -350,7 +355,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, warnings: [], @@ -379,7 +384,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, }; @@ -929,7 +934,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, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, profile: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }; 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.`); @@ -950,6 +955,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 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); const instructions = parsePublicSafeText(r.instructions, "review.instructions", warnings); @@ -960,6 +966,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo footerText !== null || note !== null || profile !== null || + securityFocus !== null || inlineComments !== null || pathInstructions.length > 0 || instructions !== null || @@ -970,6 +977,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo note, fields, profile, + securityFocus, inlineComments, pathInstructions, instructions, @@ -1116,6 +1124,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.securityFocus !== null) out.security_focus = review.securityFocus; if (review.inlineComments !== null) out.inline_comments = review.inlineComments; if (review.instructions !== null) out.instructions = review.instructions; if (review.pathInstructions.length > 0) out.path_instructions = review.pathInstructions.map((entry) => ({ path: entry.path, instructions: entry.instructions })); @@ -1149,14 +1158,16 @@ 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 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; inlineComments: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[] } { +/** Resolve the AI-reviewer overrides (`review.profile` + `review.security_focus` + `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-security-focus / #review-path-instructions / #review-exclude-paths) */ +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; securityFocus: boolean; inlineComments: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: 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. - return { profile: manifest?.review.profile ?? null, inlineComments: manifest?.review.inlineComments === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [] }; + // 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 ?? [] }; } /** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index 76d6de2f2c..9c1189615d 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -18,6 +18,7 @@ const baseInput = (): AiReviewCacheInput => ({ baseSha: null, reviewFiles: [], profile: null, + securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", @@ -241,4 +242,17 @@ describe("aiReviewCacheInputFingerprint", () => { expect(gatePackChanged).not.toBe(original); expect(repeated).toBe(original); }); + + it("changes when securityFocus toggles, independently of profile (#review-security-focus)", async () => { + const original = await aiReviewCacheInputFingerprint(baseInput()); + const securityFocusOn = await aiReviewCacheInputFingerprint({ ...baseInput(), securityFocus: true }); + const profileAndSecurityFocus = await aiReviewCacheInputFingerprint({ ...baseInput(), profile: "chill", securityFocus: true }); + const profileOnly = await aiReviewCacheInputFingerprint({ ...baseInput(), profile: "chill" }); + const repeated = await aiReviewCacheInputFingerprint(baseInput()); + + expect(securityFocusOn).not.toBe(original); + expect(profileAndSecurityFocus).not.toBe(profileOnly); + expect(profileAndSecurityFocus).not.toBe(securityFocusOn); + expect(repeated).toBe(original); + }); }); diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index f507ee6cc5..5ca8674b0d 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -17,6 +17,7 @@ const baseFingerprintInput = (): AiReviewCacheInput => ({ baseSha: null, reviewFiles: [], profile: null, + securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index fcf5f78eae..7adba6ef8a 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -389,6 +389,60 @@ describe("review.profile shapes the reviewer system prompt (#review-profile)", ( }); }); +describe("review.security_focus shapes the reviewer system prompt (#review-security-focus)", () => { + const systemPromptOf = (run: ReturnType): string => + (run.mock.calls[0]?.[1] as { messages?: Array<{ content?: string }> }) + ?.messages?.[0]?.content ?? ""; + const runSecurityFocus = async ( + securityFocus: GittensoryAiReviewInput["securityFocus"], + profile?: GittensoryAiReviewInput["profile"], + ) => { + 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, securityFocus, profile }); + return systemPromptOf(run); + }; + + it("true appends the SECURITY FOCUS instruction naming the prioritized defect categories", async () => { + const system = await runSecurityFocus(true); + expect(system).toContain("SECURITY FOCUS"); + expect(system).toContain("injection"); + expect(system).toContain("authentication/authorization bypass"); + expect(system).toContain("secret handling"); + expect(system).toContain("unsafe deserialization"); + expect(system).toContain("SSRF"); + expect(system).toContain("path traversal"); + }); + + it("absent / false leaves the prompt byte-identical (no security-focus suffix)", async () => { + const withFalse = await runSecurityFocus(false); + const withUndefined = await runSecurityFocus(undefined); + expect(withFalse).not.toContain("SECURITY FOCUS"); + expect(withUndefined).not.toContain("SECURITY FOCUS"); + expect(withFalse).toBe(withUndefined); + }); + + it("composes with (does not replace) the chill/assertive profile suffix — both appear together", async () => { + const chillPlusSecurity = await runSecurityFocus(true, "chill"); + expect(chillPlusSecurity).toContain("CHILL"); + expect(chillPlusSecurity).toContain("SECURITY FOCUS"); + + const assertivePlusSecurity = await runSecurityFocus(true, "assertive"); + expect(assertivePlusSecurity).toContain("ASSERTIVE"); + expect(assertivePlusSecurity).toContain("SECURITY FOCUS"); + + // security_focus alone (no profile) still appends only its own suffix. + const securityOnly = await runSecurityFocus(true, null); + expect(securityOnly).toContain("SECURITY FOCUS"); + expect(securityOnly).not.toMatch(/CHILL|ASSERTIVE/); + }); +}); + describe("runGittensoryAiReview block mode (consensus)", () => { function envWith(run: (model: string) => Promise) { return createTestEnv({ diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 3d1c568472..51a0b3a205 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -496,7 +496,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, sizeMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, 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 }, warnings: [], @@ -1654,6 +1654,32 @@ describe("parseFocusManifest review config", () => { expect(m.review.pathInstructions).toHaveLength(50); expect(m.warnings.some((w) => /path_instructions.*capped/.test(w))).toBe(true); }); + + it("parses review.security_focus (default OFF), marks present, round-trips, and warns on a non-boolean (#review-security-focus)", () => { + expect(parseFocusManifest({ review: { security_focus: true } }).review.securityFocus).toBe(true); + const on = parseFocusManifest({ review: { security_focus: true } }); + expect(on.review.present).toBe(true); // a security-focus-only manifest IS present + expect(parseFocusManifest({ review: reviewConfigToJson(on.review) }).review).toEqual(on.review); // survives round-trip + // Explicit false is retained (and marks present, since the maintainer set it). + const off = parseFocusManifest({ review: { security_focus: false } }); + expect(off.review.securityFocus).toBe(false); + expect(off.review.present).toBe(true); + // Absent ⇒ null (the byte-identical default), config not present. + expect(parseFocusManifest({ review: {} }).review.securityFocus).toBeNull(); + expect(parseFocusManifest({ review: {} }).review.present).toBe(false); + // A non-boolean is ignored with a warning. + const bad = parseFocusManifest({ review: { security_focus: "yes" } }); + expect(bad.review.securityFocus).toBeNull(); + expect(bad.warnings.some((w) => /review\.security_focus.*must be a boolean/.test(w))).toBe(true); + }); + + it("composes review.security_focus with review.profile independently — both persist together", () => { + const m = parseFocusManifest({ review: { profile: "chill", security_focus: true } }); + expect(m.review.profile).toBe("chill"); + expect(m.review.securityFocus).toBe(true); + expect(m.review.present).toBe(true); + expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review).toEqual(m.review); + }); }); describe("resolveReviewPathInstructions (#review-path-instructions)", () => { @@ -1682,13 +1708,15 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { }); 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" }], instructions: "Follow our async-error conventions.", exclude_paths: ["**/*.lock"] } }); - expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", inlineComments: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"] }); - // A null manifest (load failure) yields the byte-identical defaults; inline comments default OFF. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, inlineComments: false, pathInstructions: [], instructions: null, excludePaths: [] }); + 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"] } }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", securityFocus: true, inlineComments: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"] }); + // 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: [] }); // 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); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { security_focus: false } })).securityFocus).toBe(false); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).securityFocus).toBe(false); }); it("parses review.inline_comments (default OFF), marks present, round-trips, and warns on a non-boolean (#inline-comments)", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 19df52a122..62ea32471f 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -2276,6 +2276,7 @@ describe("queue processors", () => { baseSha: null, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = value.length;", additions: 1, deletions: 0 }], profile: null, + securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index be44013728..ea03222b59 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1066,7 +1066,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, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, profile: null, securityFocus: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, 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