diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 1668b4929f..691b02ed40 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -31,6 +31,12 @@ # WHO CAN BE BLOCKED: gate modes choose *which* deterministic checks run, never # *who* gets blocked. A hard block is always confirmed-contributor-gated. # +# MANIFEST VALIDATION: any parse warning from this file (an unrecognized field, a +# malformed value that got dropped) is surfaced as a single grouped "Manifest +# validation" section in the unified PR review comment, so a misconfigured value +# fails clearly instead of silently falling back to its default. A fully valid +# manifest shows nothing here (byte-identical). Always on; not a config knob. +# # NOTE: some capabilities (safety scanning, CI/full-file grounding, RAG, # reputation control, the unified comment) are switched on at the deployment # level by the operator's GITTENSORY_REVIEW_* feature flags AND a per-repo diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index f6061401c2..43ad3aa929 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -44,6 +44,12 @@ # WHO CAN BE BLOCKED: gate modes choose *which* deterministic checks run, never # *who* gets blocked. A hard block is always confirmed-contributor-gated. # +# MANIFEST VALIDATION: any parse warning from this file (an unrecognized field, a +# malformed value that got dropped) is surfaced as a single grouped "Manifest +# validation" section in the unified PR review comment, so a misconfigured value +# fails clearly instead of silently falling back to its default. A fully valid +# manifest shows nothing here (byte-identical). Always on; not a config knob. +# # NOTE: some capabilities (safety scanning, CI/full-file grounding, RAG, # reputation control, the unified comment) are switched on at the deployment # level by the operator's GITTENSORY_REVIEW_* feature flags AND a per-repo diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 1c6968eae6..a34e851d6b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -9445,8 +9445,8 @@ async function maybePublishPrPublicSurface( if (decision.willComment) { // Maintainer review-content overrides from `.gittensory.yml` (footer text, row toggles, intro note). // Cached, so this is a DB read after the settings resolution already loaded the manifest. - const reviewConfig = (await loadRepoFocusManifest(env, repoFullName)) - .review; + const repoFocusManifestForComment = await loadRepoFocusManifest(env, repoFullName); + const reviewConfig = repoFocusManifestForComment.review; // Duplicate-winner adjudication (#dup-winner): thread the flag into the public panel builders so the // winner's hard-duplicate block is suppressed (they recompute the winner from their own open-only sibling // list). Flag-OFF (default) ⇒ false ⇒ the panels are byte-identical to today. @@ -9718,6 +9718,10 @@ async function maybePublishPrPublicSurface( : {}), maxFindingsCaps: reviewConfig.maxFindings, commentVerbosity: reviewConfig.commentVerbosity, + // review-manifest validation (#2056): reuse the same manifest already loaded above for reviewConfig — + // unconditional (no manifest opt-in needed, a broken config should always fail clearly); no warnings + // ⇒ the bridge omits the section (byte-identical). + manifestWarnings: repoFocusManifestForComment.warnings, }); } else { deterministicBody = buildPublicPrIntelligenceComment(commentArgs); diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index b44d58780c..8838741856 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -21,6 +21,7 @@ import type { AdvisoryFinding } from "../types"; import type { GateCheckConclusion, GateCheckEvaluation } from "../rules/advisory"; import type { PublicPrPanelSignalRow } from "../signals/engine"; +import { formatManifestValidationNotice } from "../signals/focus-manifest"; import type { CaptureRoute } from "./visual/capture"; // Single-source the panel marker from its canonical home (the upsert reads it there); re-export so existing // importers of `PR_PANEL_COMMENT_MARKER` from this module keep working. The unified body MUST prepend this @@ -327,6 +328,11 @@ export type UnifiedCommentBridgeArgs = { * OFF (the processor passes this only when the manifest opts in — see `resolveReviewPromptOverrides`'s * `commentVerbosity`). */ commentVerbosity?: "quiet" | "normal" | "detailed" | null | undefined; + /** The manifest's parse `warnings[]` (#2056) — when non-empty, a "Manifest validation" collapsible listing + * each grouped, deduped warning is appended, so an invalid/malformed `.gittensory.yml` value fails clearly + * instead of silently falling back to a default. No AI, no network. Absent/empty ⇒ no section + * (byte-identical) — always safe to pass the manifest's raw warnings unconditionally. */ + manifestWarnings?: string[] | undefined; /** Line-anchored AI findings, one entry per inline finding (review.finding_categories port). When present + * non-empty, a "Finding categories" collapsible (a count per security/correctness/performance/maintainability/ * tests/style category) is appended. A finding missing its own `category` falls back to @@ -492,6 +498,18 @@ export function buildChangedFilesSummaryCollapsible(files: ChangedFileSummaryInp return { title: "Changed files", body }; } +/** + * Build the "Manifest validation" collapsible from a manifest's parse `warnings[]` (#2056) — grouped, + * deduped, so an invalid/malformed `.gittensory.yml` value fails clearly instead of silently falling back + * to a default. Returns null when there are no warnings, so the caller can unconditionally chain this + * alongside the other optional collapsibles (byte-identical when the manifest is fully valid). + */ +export function buildManifestValidationCollapsible(warnings: string[]): UnifiedCollapsible | null { + const notice = formatManifestValidationNotice(warnings); + if (notice === null) return null; + return { title: "Manifest validation", body: notice }; +} + /** One impact-map entry — everything `buildImpactMapCollapsible` needs to render a row. Deliberately narrower * than `ImpactMapEntry` (`src/review/impact-map.ts`) shape-wise (it IS that shape) so this bridge's import * surface stays limited to what rendering actually reads. */ @@ -635,6 +653,13 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string const visibleRows = args.panelRows.filter((row) => args.reviewFields?.[row.key] !== false); const signals = panelRowsToSignalRows(visibleRows); + // review-manifest validation (#2056): a broken/malformed .gittensory.yml value should fail clearly, so this + // is unconditional (no manifest opt-in) — prepended ahead of every content-shape summary since a config + // problem is more foundational than what changed. No warnings ⇒ extraCollapsibles is unchanged. + const manifestValidationCollapsible = + args.manifestWarnings && args.manifestWarnings.length > 0 ? buildManifestValidationCollapsible(args.manifestWarnings) : null; + const withManifestValidation = + manifestValidationCollapsible !== null ? [manifestValidationCollapsible, ...(args.extraCollapsibles ?? [])] : args.extraCollapsibles; // review.changed_files_summary port: when the manifest opts in, the processor hands us every changed file's // path + deltas here; append the grouped "Changed files" collapsible ahead of the visual preview (structure // before pixels). Flag-OFF (the processor passes undefined) ⇒ extraCollapsibles is unchanged. (#1957) @@ -643,7 +668,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string ? buildChangedFilesSummaryCollapsible(args.changedFilesSummary) : null; const withChangedFiles = - changedFilesCollapsible !== null ? [...(args.extraCollapsibles ?? []), changedFilesCollapsible] : args.extraCollapsibles; + changedFilesCollapsible !== null ? [...(withManifestValidation ?? []), changedFilesCollapsible] : withManifestValidation; // review.finding_categories port: when the manifest opts in, the processor hands us this review's line-anchored // AI findings here; append the "Finding categories" collapsible right after Changed files (both are structural // review-shape summaries, ahead of the visual preview). Flag-OFF (the processor passes undefined) ⇒ diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index b6f12c83b2..3e86629488 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -2956,6 +2956,25 @@ export function parseFocusManifestContent(content: string | null | undefined, so return parseFocusManifest(parsed, source); } +/** + * Format a manifest's parse `warnings[]` into one grouped, deduped, order-preserving notice for the review + * surface — an acceptance criterion of #1670: an invalid/malformed `.gittensory.yml` value should fail + * clearly instead of silently falling back to a default. Empty/no warnings ⇒ `null` (byte-identical, no + * notice). Pure; reuses the warnings every parser already accumulates rather than a parallel schema. (#2056) + */ +export function formatManifestValidationNotice(warnings: string[]): string | null { + const seen = new Set(); + const deduped: string[] = []; + for (const warning of warnings) { + const trimmed = warning.trim(); + if (!trimmed || seen.has(trimmed)) continue; + seen.add(trimmed); + deduped.push(trimmed); + } + if (deduped.length === 0) return null; + return deduped.map((warning) => `- ${warning}`).join("\n"); +} + function normalizePathForMatch(path: string): string { return String(path).replace(/\\/g, "/").replace(/^\.\//, "").replace(/^\/+/, "").toLowerCase(); } diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index e763bcc69e..f12e05b726 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -12,6 +12,7 @@ import { matchesManifestPath, parseFocusManifest, parseFocusManifestContent, + formatManifestValidationNotice, resolveEffectiveSettings, excludeReviewPaths, applyReviewPathFilters, @@ -4106,3 +4107,48 @@ describe("gate.expectedCiContexts (#selfhost-ci-verification)", () => { expect(eff.expectedCiContexts).toBeUndefined(); }); }); + +describe("formatManifestValidationNotice (#2056)", () => { + it("returns null for an empty warnings array", () => { + expect(formatManifestValidationNotice([])).toBeNull(); + }); + + it("returns null when every warning is blank/whitespace-only", () => { + expect(formatManifestValidationNotice(["", " ", "\n"])).toBeNull(); + }); + + it("formats a single warning as a bullet line", () => { + expect(formatManifestValidationNotice(["Manifest field \"review.tone\" must be a string; ignoring it."])).toBe( + "- Manifest field \"review.tone\" must be a string; ignoring it.", + ); + }); + + it("groups multiple distinct warnings, preserving order", () => { + const result = formatManifestValidationNotice(["first warning", "second warning", "third warning"]); + expect(result).toBe("- first warning\n- second warning\n- third warning"); + }); + + it("dedupes identical warnings (case-sensitive, exact-match), keeping the first occurrence's position", () => { + const result = formatManifestValidationNotice(["dup warning", "unique warning", "dup warning"]); + expect(result).toBe("- dup warning\n- unique warning"); + }); + + it("trims surrounding whitespace from each warning before formatting/deduping", () => { + const result = formatManifestValidationNotice([" padded warning ", "padded warning"]); + expect(result).toBe("- padded warning"); + }); + + it("round-trips through a real malformed manifest's warnings", () => { + const malformed = parseFocusManifest({ review: { tone: 42, profile: "loud" } }); + expect(malformed.warnings.length).toBeGreaterThan(0); + const notice = formatManifestValidationNotice(malformed.warnings); + expect(notice).not.toBeNull(); + expect(notice).toContain("- "); + }); + + it("returns null for a fully-valid manifest with zero warnings (byte-identical, no notice)", () => { + const valid = parseFocusManifest({ review: { profile: "chill" } }); + expect(valid.warnings).toEqual([]); + expect(formatManifestValidationNotice(valid.warnings)).toBeNull(); + }); +}); diff --git a/test/unit/manifest-validation-collapsible.test.ts b/test/unit/manifest-validation-collapsible.test.ts new file mode 100644 index 0000000000..139ebf596c --- /dev/null +++ b/test/unit/manifest-validation-collapsible.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it } from "vitest"; +import { buildManifestValidationCollapsible, buildUnifiedCommentBody } from "../../src/review/unified-comment-bridge"; +import type { GateCheckEvaluation } from "../../src/rules/advisory"; +import type { PublicPrPanelSignalRow } from "../../src/signals/engine"; + +function gate(over: Partial = {}): GateCheckEvaluation { + return { + enabled: true, + conclusion: "success", + title: "Gittensory Orb Review Agent passed", + summary: "No configured hard blocker was found.", + blockers: [], + warnings: [], + ...over, + }; +} + +const panelRows: PublicPrPanelSignalRow[] = [ + { key: "gateResult", cells: ["Gate result", "✅ Passing", "No configured blocker found.", "No action."] }, +]; +const footer = "💰 Earn for open-source contributions. Checked by Gittensory."; + +describe("buildManifestValidationCollapsible (#2056)", () => { + it("returns null for an empty warnings array", () => { + expect(buildManifestValidationCollapsible([])).toBeNull(); + }); + + it("returns null when every warning is blank", () => { + expect(buildManifestValidationCollapsible(["", " "])).toBeNull(); + }); + + it("builds a titled collapsible listing each grouped warning", () => { + const c = buildManifestValidationCollapsible(["bad review.tone value", "bad review.profile value"]); + expect(c).not.toBeNull(); + expect(c?.title).toBe("Manifest validation"); + expect(c?.body).toBe("- bad review.tone value\n- bad review.profile value"); + }); + + it("dedupes identical warnings", () => { + const c = buildManifestValidationCollapsible(["same warning", "same warning"]); + expect(c?.body).toBe("- same warning"); + }); +}); + +describe("buildUnifiedCommentBody: manifest validation wiring (#2056)", () => { + it("appends a Manifest validation collapsible when manifestWarnings is non-empty", () => { + const body = buildUnifiedCommentBody({ + gate: gate(), + panelRows, + readinessTotal: 88, + changedFiles: 1, + footerMarkdown: footer, + manifestWarnings: ["Manifest field \"review.tone\" must be a string; ignoring it."], + }); + expect(body).toContain("Manifest validation"); + expect(body).toContain("Manifest field \"review.tone\" must be a string; ignoring it."); + }); + + it("omits the section when manifestWarnings is empty or absent (byte-identical)", () => { + const withEmpty = buildUnifiedCommentBody({ + gate: gate(), + panelRows, + readinessTotal: 88, + changedFiles: 1, + footerMarkdown: footer, + manifestWarnings: [], + }); + const withoutField = buildUnifiedCommentBody({ + gate: gate(), + panelRows, + readinessTotal: 88, + changedFiles: 1, + footerMarkdown: footer, + }); + expect(withEmpty).toBe(withoutField); + expect(withEmpty).not.toContain("Manifest validation"); + }); + + it("renders the Manifest validation section ahead of the Changed files section", () => { + const body = buildUnifiedCommentBody({ + gate: gate(), + panelRows, + readinessTotal: 88, + changedFiles: 1, + footerMarkdown: footer, + manifestWarnings: ["a config warning"], + changedFilesSummary: [{ path: "src/a.ts", additions: 1, deletions: 0 }], + }); + expect(body.indexOf("Manifest validation")).toBeLessThan(body.indexOf("Changed files")); + }); + + it("renders Changed files with no Manifest validation section when only manifestWarnings is absent", () => { + const body = buildUnifiedCommentBody({ + gate: gate(), + panelRows, + readinessTotal: 88, + changedFiles: 1, + footerMarkdown: footer, + changedFilesSummary: [{ path: "src/a.ts", additions: 1, deletions: 0 }], + }); + expect(body).toContain("Changed files"); + expect(body).not.toContain("Manifest validation"); + }); +});