From c7097d4c13ad4ade04805367598df0b2f74c7f0e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 07:34:10 -0700 Subject: [PATCH 1/3] feat(review): add config-driven before/after screenshot-table gate (#2006) Makes the previously documented-only requirement (a contributor visual/ frontend PR needs a before/after screenshot table or it gets auto-closed) a per-repo, config-as-code gate: off by default, scoped by label/path, with a configurable action and contract message, layered the same way as the existing blacklist/review-nag/review-evasion anti-abuse mechanisms. --- .gittensory.yml.example | 13 + apps/gittensory-ui/public/openapi.json | 254 ++------------- config/examples/gittensory.full.yml | 13 + migrations/0115_screenshot_table_gate.sql | 14 + src/db/repositories.ts | 38 +++ src/db/schema.ts | 8 + src/openapi/schemas.ts | 9 + src/queue/processors.ts | 20 ++ src/review/screenshot-table-gate.ts | 190 +++++++++++ src/settings/agent-actions.ts | 41 ++- src/signals/focus-manifest.ts | 33 +- src/types.ts | 20 +- test/unit/agent-actions.test.ts | 70 ++++ test/unit/focus-manifest.test.ts | 31 ++ test/unit/queue.test.ts | 132 ++++++++ ...ory-settings-screenshot-table-gate.test.ts | 78 +++++ test/unit/screenshot-table-gate.test.ts | 299 ++++++++++++++++++ 17 files changed, 1041 insertions(+), 222 deletions(-) create mode 100644 migrations/0115_screenshot_table_gate.sql create mode 100644 src/review/screenshot-table-gate.ts create mode 100644 test/unit/repository-settings-screenshot-table-gate.test.ts create mode 100644 test/unit/screenshot-table-gate.test.ts diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 49ab5c7152..d450a61d63 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -803,6 +803,19 @@ settings: # mode: off # off | hold. Default: off. # minConfidence: 0.85 # Number 0-1. Minimum AI-verifier confidence to treat a candidate as a real match. + # Before/after screenshot-table gate (#2006): a DETERMINISTIC check (no AI, zero hallucination risk) that a + # contributor visual/frontend PR's body contains a markdown table with before/after image markup (either + # `![alt](url)` or ``, inside a `| ... |` table). Scoped to whenLabels OR whenPaths (either match is + # enough); both empty ⇒ enforced on every PR once enabled. Also rejects an image pasted outside any table, or + # a screenshot committed to the repo under a scoped path (should be uploaded to the PR body instead). Off by + # default -- opt in per repo. + # screenshotTableGate: + # enabled: false # Default: false (off). + # whenLabels: [frontend, visual] # Enforce only when the PR carries one of these labels. Default: [] (no label scoping). + # whenPaths: ["apps/ui/**", "src/**/*.tsx"] # Enforce only when a changed file matches one of these globs. Default: [] (no path scoping). + # action: close # close | request_changes | comment. Default: close. + # message: "Custom close reason..." # Overrides the built-in templated contract message. Default: null (built-in message). + # Maintainer AI review tuning (`.gittensory.yml` top-level `review:` block). These knobs shape the advisory AI # review prompt and file selection only — gate/slop/secret-scan are unaffected. # review: diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index 6b4bd83d41..b4c5b68289 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -9242,14 +9242,41 @@ "type": "string", "nullable": true }, - "publicQualityMetrics": { - "type": "boolean" - }, - "regateSweepOrderMode": { - "type": "string", - "enum": [ - "staleness", - "oldest-first" + "screenshotTableGate": { + "type": "object", + "properties": { + "enabled": { + "type": "boolean" + }, + "whenLabels": { + "type": "array", + "items": { + "type": "string" + } + }, + "whenPaths": { + "type": "array", + "items": { + "type": "string" + } + }, + "action": { + "type": "string", + "enum": [ + "close", + "request_changes", + "comment" + ] + }, + "message": { + "type": "string" + } + }, + "required": [ + "enabled", + "whenLabels", + "whenPaths", + "action" ] } }, @@ -9261,7 +9288,6 @@ "checkRunMode", "checkRunDetailLevel", "gateCheckMode", - "regateSweepOrderMode", "reviewCheckMode", "gatePack", "linkedIssueGateMode", @@ -9970,16 +9996,6 @@ "defaultAllowed", "commandOverrides" ] - }, - "publicQualityMetrics": { - "type": "boolean" - }, - "regateSweepOrderMode": { - "type": "string", - "enum": [ - "staleness", - "oldest-first" - ] } }, "required": [ @@ -9990,7 +10006,6 @@ "checkRunMode", "checkRunDetailLevel", "gateCheckMode", - "regateSweepOrderMode", "reviewCheckMode", "gatePack", "linkedIssueGateMode", @@ -10009,7 +10024,6 @@ "includeMaintainerAuthors", "requireLinkedIssue", "badgeEnabled", - "publicQualityMetrics", "aiReviewMode", "aiReviewByok", "aiReviewProvider", @@ -13439,164 +13453,6 @@ "maintainerNextSteps", "privateSummary" ] - }, - "PublicQualityMetrics": { - "type": "object", - "properties": { - "repoFullName": { - "type": "string" - }, - "generatedAt": { - "type": "string" - }, - "gate": { - "type": "object", - "properties": { - "blocked": { - "type": "number" - }, - "blockedThenMerged": { - "type": "number" - }, - "falsePositiveRate": { - "type": "number", - "nullable": true - }, - "precisionPct": { - "type": "number", - "nullable": true - }, - "topGateTypes": { - "type": "array", - "items": { - "type": "object", - "properties": { - "gateType": { - "type": "string" - }, - "blocked": { - "type": "number" - }, - "blockedThenMerged": { - "type": "number" - }, - "falsePositiveRate": { - "type": "number", - "nullable": true - }, - "precisionPct": { - "type": "number", - "nullable": true - } - }, - "required": [ - "gateType", - "blocked", - "blockedThenMerged", - "falsePositiveRate", - "precisionPct" - ] - } - } - }, - "required": [ - "blocked", - "blockedThenMerged", - "falsePositiveRate", - "precisionPct", - "topGateTypes" - ] - }, - "outcomes": { - "type": "object", - "properties": { - "merged": { - "type": "number" - }, - "closed": { - "type": "number" - }, - "mergeRatioPct": { - "type": "number", - "nullable": true - } - }, - "required": [ - "merged", - "closed", - "mergeRatioPct" - ] - }, - "slop": { - "type": "object", - "properties": { - "totalResolved": { - "type": "number" - }, - "overallMergeRate": { - "type": "number", - "nullable": true - }, - "discriminates": { - "type": "boolean", - "nullable": true - } - }, - "required": [ - "totalResolved", - "overallMergeRate", - "discriminates" - ] - }, - "trend": { - "type": "array", - "items": { - "type": "object", - "properties": { - "weekStart": { - "type": "string" - }, - "gateBlocked": { - "type": "number" - }, - "gateBlockedThenMerged": { - "type": "number" - }, - "gateFalsePositiveRate": { - "type": "number", - "nullable": true - }, - "outcomesMerged": { - "type": "number" - }, - "outcomesClosed": { - "type": "number" - }, - "mergeRatioPct": { - "type": "number", - "nullable": true - } - }, - "required": [ - "weekStart", - "gateBlocked", - "gateBlockedThenMerged", - "gateFalsePositiveRate", - "outcomesMerged", - "outcomesClosed", - "mergeRatioPct" - ] - } - } - }, - "required": [ - "repoFullName", - "generatedAt", - "gate", - "outcomes", - "slop", - "trend" - ] } }, "parameters": {}, @@ -16698,46 +16554,6 @@ } ] } - }, - "/v1/public/repos/{owner}/{repo}/quality": { - "get": { - "parameters": [ - { - "schema": { - "type": "string" - }, - "required": true, - "name": "owner", - "in": "path" - }, - { - "schema": { - "type": "string" - }, - "required": true, - "name": "repo", - "in": "path" - } - ], - "responses": { - "200": { - "description": "Public per-repo review-quality metrics: gate false-positive rates, merge-vs-close ratio, and weekly trend. Aggregate counts only; opt-in via publicQualityMetrics.", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/PublicQualityMetrics" - } - } - } - }, - "404": { - "description": "Repo is unknown/private/uninstalled or has not opted in" - }, - "503": { - "description": "Public quality metrics are temporarily unavailable" - } - } - } } }, "servers": [ diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index c12744dc63..bbd5386454 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -816,6 +816,19 @@ settings: # mode: off # off | hold. Default: off. # minConfidence: 0.85 # Number 0-1. Minimum AI-verifier confidence to treat a candidate as a real match. + # Before/after screenshot-table gate (#2006): a DETERMINISTIC check (no AI, zero hallucination risk) that a + # contributor visual/frontend PR's body contains a markdown table with before/after image markup (either + # `![alt](url)` or ``, inside a `| ... |` table). Scoped to whenLabels OR whenPaths (either match is + # enough); both empty ⇒ enforced on every PR once enabled. Also rejects an image pasted outside any table, or + # a screenshot committed to the repo under a scoped path (should be uploaded to the PR body instead). Off by + # default -- opt in per repo. + # screenshotTableGate: + # enabled: false # Default: false (off). + # whenLabels: [frontend, visual] # Enforce only when the PR carries one of these labels. Default: [] (no label scoping). + # whenPaths: ["apps/ui/**", "src/**/*.tsx"] # Enforce only when a changed file matches one of these globs. Default: [] (no path scoping). + # action: close # close | request_changes | comment. Default: close. + # message: "Custom close reason..." # Overrides the built-in templated contract message. Default: null (built-in message). + # Maintainer AI review tuning (`.gittensory.yml` top-level `review:` block). These knobs shape the advisory AI # review prompt and file selection only — gate/slop/secret-scan are unaffected. # review: diff --git a/migrations/0115_screenshot_table_gate.sql b/migrations/0115_screenshot_table_gate.sql new file mode 100644 index 0000000000..76af2f7215 --- /dev/null +++ b/migrations/0115_screenshot_table_gate.sql @@ -0,0 +1,14 @@ +-- Config-driven before/after screenshot-table gate (#2006): a contributor visual/frontend PR that lacks a +-- before/after screenshot table in its body is unreviewable at a glance. This mechanism was previously +-- documented-only (the contributing skill / PR template); this migration makes it a per-repo, config-as-code +-- gate, layered the same way as every other anti-abuse mechanism in this file (blacklist/review-nag/ +-- review-evasion): off by default (zero behavior change for an install that hasn't opted in), +-- `screenshot_table_gate_action` NOT NULL with a "close" default (mirrors the existing hard requirement), and +-- the label/path scope lists stored as JSON (mirrors contributor_blacklist_json / auto_close_exempt_logins_json). +ALTER TABLE repository_settings ADD COLUMN screenshot_table_gate_enabled INTEGER NOT NULL DEFAULT 0; +ALTER TABLE repository_settings ADD COLUMN screenshot_table_gate_when_labels_json TEXT NOT NULL DEFAULT '[]'; +ALTER TABLE repository_settings ADD COLUMN screenshot_table_gate_when_paths_json TEXT NOT NULL DEFAULT '[]'; +ALTER TABLE repository_settings ADD COLUMN screenshot_table_gate_action TEXT NOT NULL DEFAULT 'close'; +-- Nullable: null = use the built-in default templated contract message (DEFAULT_SCREENSHOT_CONTRACT_MESSAGE), +-- never "unset to empty" (mirrors moderation_rules_json's null-means-inherit-default shape). +ALTER TABLE repository_settings ADD COLUMN screenshot_table_gate_message TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 77cdc894f7..cb893af65f 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -153,6 +153,7 @@ import type { ReviewSuppressionRecord, ScorePreviewRecord, ScoringModelSnapshotRecord, + ScreenshotTableGateConfig, SignalSnapshotRecord, UpstreamDriftArea, UpstreamDriftReportRecord, @@ -172,6 +173,7 @@ import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy, DEFAULT_AUTO_MAIN import { DEFAULT_TYPE_LABELS, normalizeTypeLabelSet } from "../settings/pr-type-label"; import { DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION, normalizeLinkedIssueLabelPropagationConfig } from "../review/linked-issue-label-propagation"; import { DEFAULT_LINKED_ISSUE_HARD_RULES } from "../review/linked-issue-hard-rules-config"; +import { DEFAULT_SCREENSHOT_TABLE_GATE, isScreenshotTableGateAction, normalizeScreenshotTableGateConfig } from "../review/screenshot-table-gate"; import { decryptSecret, encryptSecret, sha256Hex } from "../utils/crypto"; import { errorMessage, jsonString, nowIso, parseJson, repoParts } from "../utils/json"; import { PUBLIC_LOCAL_PATH_SCRUB_PATTERN } from "../signals/redaction"; @@ -564,6 +566,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise reviewEvasionProtection: "off", reviewEvasionLabel: DEFAULT_REVIEW_EVASION_LABEL, reviewEvasionComment: true, + screenshotTableGate: { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }, }; } return { @@ -641,6 +644,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise reviewEvasionProtection: normalizeReviewEvasionProtection(row.reviewEvasionProtection), reviewEvasionLabel: row.reviewEvasionLabel, reviewEvasionComment: row.reviewEvasionComment, + screenshotTableGate: parseScreenshotTableGateRow(row), createdAt: row.createdAt, updatedAt: row.updatedAt, }; @@ -760,6 +764,7 @@ export async function upsertRepositorySettings(env: Env, settings: Partial(value, []); + if (!Array.isArray(parsed)) return []; + return parsed.filter((item): item is string => typeof item === "string" && item.trim().length > 0); +} + function normalizeCommandRateLimitPolicy(value: string | null | undefined): "off" | "hold" { return value === "hold" ? value : "off"; } diff --git a/src/db/schema.ts b/src/db/schema.ts index 369522908d..2cd2b1b701 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -157,6 +157,14 @@ export const repositorySettings = sqliteTable("repository_settings", { reviewEvasionProtection: text("review_evasion_protection").notNull().default("off"), reviewEvasionLabel: text("review_evasion_label").notNull().default("review-evasion"), reviewEvasionComment: integer("review_evasion_comment", { mode: "boolean" }).notNull().default(true), + // Config-driven before/after screenshot-table gate (#2006): off by default. whenLabels/whenPaths are JSON + // string arrays (mirrors contributorBlacklistJson's shape); screenshotTableGateMessage is nullable ("no + // override" is a `.gittensory.yml`-only concept -- null here means "use the built-in default message"). + screenshotTableGateEnabled: integer("screenshot_table_gate_enabled", { mode: "boolean" }).notNull().default(false), + screenshotTableGateWhenLabelsJson: text("screenshot_table_gate_when_labels_json").notNull().default("[]"), + screenshotTableGateWhenPathsJson: text("screenshot_table_gate_when_paths_json").notNull().default("[]"), + screenshotTableGateAction: text("screenshot_table_gate_action").notNull().default("close"), + screenshotTableGateMessage: text("screenshot_table_gate_message"), createdAt: text("created_at").notNull().$defaultFn(() => nowIso()), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }); diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 195d91ccc3..b362c265aa 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -762,6 +762,15 @@ export const RepositorySettingsSchema = z reviewEvasionProtection: z.enum(["off", "close"]).optional(), reviewEvasionLabel: z.string().nullable().optional(), reviewEvasionComment: z.boolean().optional(), + screenshotTableGate: z + .object({ + enabled: z.boolean(), + whenLabels: z.array(z.string()), + whenPaths: z.array(z.string()), + action: z.enum(["close", "request_changes", "comment"]), + message: z.string().optional(), + }) + .optional(), createdAt: z.string().nullable().optional(), updatedAt: z.string().nullable().optional(), }) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8fc8791851..443a2f1142 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -490,6 +490,7 @@ import { } from "../review/linked-issue-hard-rules"; import { DEFAULT_UNLINKED_ISSUE_GUARDRAIL } from "../review/unlinked-issue-guardrail-config"; import { resolveUnlinkedIssueMatchDisposition } from "../review/unlinked-issue-guardrail"; +import { DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate } from "../review/screenshot-table-gate"; import { isOpsEnabled, runOpsAlerts } from "../review/ops-wire"; import { isSweepWatchdogEnabled, runSweepLivenessWatchdog } from "../review/sweep-watchdog"; import { isSelfTuneEnabled, runSelfTune } from "../review/selftune-wire"; @@ -2708,6 +2709,24 @@ async function runAgentMaintenancePlanAndExecute( settings.contributorBlacklist, ); + // Screenshot-table gate (#2006): a DETERMINISTIC check (no AI) that an in-scope (label/path-matched) + // contributor visual/frontend PR's body contains a before/after screenshot table. Off by default + // (settings.screenshotTableGate.enabled === false), so the pure evaluator below is effectively free for the + // common case. Only "close" is wired as an enforcement action here (the other configured actions stay + // advisory, matching the issue's phased rollout) -- the ternary below is the ONLY place that reads `.action`. + /* v8 ignore next -- defensive: resolveRepositorySettings always populates screenshotTableGate (getRepositorySettings's DB defaults), so this fallback is unreachable in practice. */ + const screenshotTableGateConfig = settings.screenshotTableGate ?? DEFAULT_SCREENSHOT_TABLE_GATE; + const screenshotTableGateResult = evaluateScreenshotTableGate({ + config: screenshotTableGateConfig, + prBody: pr.body, + prLabels: pr.labels, + changedFiles: changedPaths, + }); + const screenshotTableMatch = + screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" + ? { matched: true, reason: screenshotTableGateResult.reason } + : undefined; + // Account-age throttle (#2561, anti-abuse): a friction/visibility signal for the classic ban-evasion pattern // (a banned login gets a fresh account the same day) — NEVER an automatic close on account age alone. Off // (null accountAgeThresholdDays, the default) ⇒ this block is a no-op, no extra GitHub API call at all. @@ -2862,6 +2881,7 @@ async function runAgentMaintenancePlanAndExecute( : {}), // Always threaded (the DB layer populates it, default "slop"); the planner applies its own fallback. blacklistLabel: settings.blacklistLabel, + ...(screenshotTableMatch !== undefined ? { screenshotTableMatch } : {}), ...(contributorCapMatch !== undefined ? { contributorCapMatch } : {}), // Always threaded (the DB layer populates it, default "over-contributor-limit"); the planner applies its // own fallback. diff --git a/src/review/screenshot-table-gate.ts b/src/review/screenshot-table-gate.ts new file mode 100644 index 0000000000..dbb363b9cc --- /dev/null +++ b/src/review/screenshot-table-gate.ts @@ -0,0 +1,190 @@ +import { matchesAny } from "../signals/change-guardrail"; +import type { ScreenshotTableGateAction, ScreenshotTableGateConfig } from "../types"; + +export type { ScreenshotTableGateAction, ScreenshotTableGateConfig } from "../types"; + +// Config-driven before/after screenshot-table gate (#2006). Contributor visual/frontend PRs are unreviewable +// at a glance without before/after evidence — this is a DETERMINISTIC (no AI, zero hallucination risk) check +// that a PR's body contains a markdown table with image markup, scoped to the repo's configured labels/paths. +// Mirrors the shape of contributor-blacklist.ts / linked-issue-hard-rules-config.ts: a normalizer (DB JSON or +// `.gittensory.yml` → validated config) plus a pure evaluator the trigger calls with live PR facts. Off by +// default (`enabled: false`) — a self-hoster opts in per repo, never hard-coded for any one project. + +const MAX_LABELS = 50; +const MAX_PATHS = 50; +const MAX_LABEL_CHARS = 100; +const MAX_PATH_CHARS = 300; + +// Extensions treated as "an image file" for the committed-image-file check below. Deliberately excludes SVG: +// an SVG can embed script/foreign-object content, so it is never accepted as review evidence anywhere in this +// repo (see the PR template's own UI Evidence rule) — a committed .svg is caught by neither this check nor the +// body-table one, exactly like the template's existing screenshots-must-be-raster rule. +const IMAGE_EXTENSIONS = [".png", ".jpg", ".jpeg", ".gif", ".webp"]; + +export const DEFAULT_SCREENSHOT_TABLE_GATE: ScreenshotTableGateConfig = { + enabled: false, + whenLabels: [], + whenPaths: [], + action: "close", +}; + +const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "request_changes", "comment"]; + +export function isScreenshotTableGateAction(value: unknown): value is ScreenshotTableGateAction { + return typeof value === "string" && (VALID_ACTIONS as readonly string[]).includes(value); +} + +function normalizeStringList(value: unknown, field: string, max: number, maxChars: number, warnings: string[]): string[] { + if (value === undefined) return []; + if (!Array.isArray(value)) { + warnings.push(`settings.requireScreenshotTable.${field} must be an array; ignoring it.`); + return []; + } + const out: string[] = []; + for (const [index, item] of value.entries()) { + if (out.length >= max) { + warnings.push(`settings.requireScreenshotTable.${field} is capped at ${max} entries; dropping the rest.`); + break; + } + if (typeof item !== "string" || item.trim().length === 0) { + warnings.push(`settings.requireScreenshotTable.${field}[${index}] must be a non-empty string; ignoring it.`); + continue; + } + out.push(item.trim().slice(0, maxChars)); + } + return out; +} + +/** Normalize a raw `requireScreenshotTable` value (DB JSON or `.gittensory.yml`) into a validated config. Never + * throws: malformed fields fall back to the default (disabled/empty), matching every other settings normalizer + * in this codebase. */ +export function normalizeScreenshotTableGateConfig(input: unknown, warnings: string[]): ScreenshotTableGateConfig { + if (input === undefined || input === null) return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }; + if (typeof input !== "object" || Array.isArray(input)) { + warnings.push("settings.requireScreenshotTable must be an object; using the default (disabled)."); + return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }; + } + const record = input as Record; + const enabled = typeof record.enabled === "boolean" ? record.enabled : DEFAULT_SCREENSHOT_TABLE_GATE.enabled; + if (record.enabled !== undefined && typeof record.enabled !== "boolean") { + warnings.push(`settings.requireScreenshotTable.enabled must be a boolean; using the default "${DEFAULT_SCREENSHOT_TABLE_GATE.enabled}".`); + } + const action = isScreenshotTableGateAction(record.action) + ? record.action + : (() => { + if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be one of close, request_changes, comment; using the default "close".`); + return DEFAULT_SCREENSHOT_TABLE_GATE.action; + })(); + const message = typeof record.message === "string" && record.message.trim().length > 0 ? record.message.trim() : undefined; + if (record.message !== undefined && message === undefined) { + warnings.push("settings.requireScreenshotTable.message must be a non-empty string; using the default message."); + } + return { + enabled, + whenLabels: normalizeStringList(record.whenLabels, "whenLabels", MAX_LABELS, MAX_LABEL_CHARS, warnings), + whenPaths: normalizeStringList(record.whenPaths, "whenPaths", MAX_PATHS, MAX_PATH_CHARS, warnings), + action, + ...(message !== undefined ? { message } : {}), + }; +} + +/** True when `body` contains at least one markdown TABLE region (`| ... |` header + separator row) whose cells + * embed image markup — either `![alt](url)` or an `` tag — inside the table. A screenshot pasted as a + * bare inline image OUTSIDE any table does not count (the contract requires captioned thumbnails INSIDE a + * table, not a wall of raw images). Deliberately simple/regex-based (no markdown AST dependency) — false + * negatives fail toward "no table found" (in-scope PRs still need a real table), false positives fail toward + * "table found" (never blocks a PR that plausibly complied); both directions are acceptable for a + * first-pass deterministic heuristic that a maintainer can always override by hand. */ +export function hasImageBearingMarkdownTable(body: string | null | undefined): boolean { + if (!body) return false; + const lines = body.split(/\r?\n/); + const tableRowPattern = /^\s*\|.*\|\s*$/; + const separatorRowPattern = /^\s*\|?(\s*:?-{3,}:?\s*\|)+\s*:?-{3,}:?\s*\|?\s*$/; + const imagePattern = /!\[[^\]]*\]\([^)]+\)|]*>/i; + for (let i = 0; i < lines.length - 1; i += 1) { + // `i < lines.length - 1` guarantees both indices are in bounds; the `?? ""` fallbacks only exist to + // satisfy noUncheckedIndexedAccess and are never actually reached. + /* v8 ignore next -- defensive: the loop bound above guarantees lines[i] always exists here. */ + const header = lines[i] ?? ""; + /* v8 ignore next -- defensive: the loop bound above guarantees lines[i + 1] always exists here. */ + const separator = lines[i + 1] ?? ""; + if (!tableRowPattern.test(header) || !separatorRowPattern.test(separator)) continue; + // Found a table (header + separator). Scan its body rows (until a blank line or a non-table line) for + // image markup in any cell. + let j = i + 2; + /* v8 ignore next -- defensive: the `j < lines.length` guard above guarantees lines[j] always exists here. */ + while (j < lines.length && tableRowPattern.test(lines[j] ?? "")) { + if (imagePattern.test(lines[j] ?? "")) return true; + j += 1; + } + } + return false; +} + +/** True when `body` has a large inline image OUTSIDE of any markdown table — a common way contributors dodge + * the table requirement (paste screenshots directly into the body instead of inside a captioned table row). */ +export function hasImageOutsideTable(body: string | null | undefined): boolean { + if (!body) return false; + const lines = body.split(/\r?\n/); + const tableRowPattern = /^\s*\|.*\|\s*$/; + const imagePattern = /!\[[^\]]*\]\([^)]+\)|]*>/i; + return lines.some((line) => imagePattern.test(line) && !tableRowPattern.test(line)); +} + +/** True when any changed file path is an image under a scoped path (a screenshot committed to the repo instead + * of uploaded to the PR body via GitHub's CDN, per the contract). `scopedPaths` should be the SAME glob list + * used for scope matching (`whenPaths`) so this only flags an image landing where visual work is expected — + * not an unrelated asset (e.g. a favicon) added anywhere else in the repo. Empty `scopedPaths` (no path scoping + * configured) checks every changed path. */ +export function hasCommittedImageFile(changedFiles: string[], scopedPaths: string[]): boolean { + return changedFiles.some((file) => { + const lower = file.toLowerCase(); + if (!IMAGE_EXTENSIONS.some((ext) => lower.endsWith(ext))) return false; + return scopedPaths.length === 0 || matchesAny(file, scopedPaths); + }); +} + +/** True when the PR is IN SCOPE for the gate: it carries one of `config.whenLabels` OR touches a path matching + * one of `config.whenPaths`. Both empty ⇒ every PR is in scope (an operator who enables the gate with no + * scoping at all wants it enforced everywhere). Only one non-empty list configured ⇒ that list alone decides + * scope (the other, empty list can never exclude a PR the configured one matched). */ +export function isScreenshotTableGateInScope(config: ScreenshotTableGateConfig, prLabels: string[], changedFiles: string[]): boolean { + if (config.whenLabels.length === 0 && config.whenPaths.length === 0) return true; + const wantedLabels = new Set(config.whenLabels.map((label) => label.toLowerCase())); + const labelMatch = config.whenLabels.length > 0 && prLabels.some((label) => wantedLabels.has(label.toLowerCase())); + const pathMatch = config.whenPaths.length > 0 && changedFiles.some((file) => matchesAny(file, config.whenPaths)); + return labelMatch || pathMatch; +} + +export const DEFAULT_SCREENSHOT_CONTRACT_MESSAGE = + "This pull request changes UI/visual code but its description is missing a before/after screenshot table. " + + "Every changed page/feature needs a **markdown table** with a before column and an after column, each cell a " + + "clickable thumbnail (uploaded to the PR, not committed to the repo) with a caption below — for example:\n\n" + + "| Before | After |\n| --- | --- |\n| [![before](url)](url) — caption | [![after](url)](url) — caption |\n\n" + + "Please resubmit with the table filled in."; + +export type ScreenshotTableGateResult = { + violated: boolean; + reason: string | null; +}; + +const NO_VIOLATION: ScreenshotTableGateResult = { violated: false, reason: null }; + +/** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation. In + * scope AND (no image-bearing table in the body OR an image pasted outside a table OR a committed image file + * under a scoped path) ⇒ violated, with the configured (or default) templated message as the reason. */ +export function evaluateScreenshotTableGate(input: { + config: ScreenshotTableGateConfig; + prBody: string | null | undefined; + prLabels: string[]; + changedFiles: string[]; +}): ScreenshotTableGateResult { + const { config } = input; + if (!config.enabled) return NO_VIOLATION; + if (!isScreenshotTableGateInScope(config, input.prLabels, input.changedFiles)) return NO_VIOLATION; + const hasTable = hasImageBearingMarkdownTable(input.prBody); + const outsideTable = hasImageOutsideTable(input.prBody); + const committedImage = hasCommittedImageFile(input.changedFiles, config.whenPaths); + if (hasTable && !outsideTable && !committedImage) return NO_VIOLATION; + return { violated: true, reason: config.message ?? DEFAULT_SCREENSHOT_CONTRACT_MESSAGE }; +} diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index eabb73c6e1..ae5fb1df39 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -101,7 +101,7 @@ export type PlannedAgentAction = { // mutates via the Issues API and is exempt from the PR-write-permission gate `close` must pass, so without // this correlation a transient write-permission denial could leave a PR mislabeled "closed for X" while it // is, in fact, still open). - closeKind?: "linked-issue-hard-rule" | "blacklist" | "contributor_cap" | "review_nag" | "heuristic"; + closeKind?: "linked-issue-hard-rule" | "blacklist" | "contributor_cap" | "review_nag" | "screenshot_table" | "heuristic"; // For a CI-driven heuristic close, the CI state that must still hold at actuation time. Other heuristic // closes (gate verdict, duplicate/slop, conflict) do not depend on red CI and must not be blocked by green CI. // ALWAYS set for a heuristic close (never omitted) -- see the field's doc comment on AgentPendingActionParams @@ -322,6 +322,14 @@ export type AgentActionPlanInput = { // AI semantic-match verdict, and a systematically-wrong match must not become breaker-proof just because // it repeated. Mutually exclusive with unlinkedIssueMatchHold -- the resolver only ever returns one. unlinkedIssueMatchClose?: { reason: string; comment: string } | undefined; + // Screenshot-table gate (#2006): a DETERMINISTIC verdict (no AI, zero hallucination risk) that an in-scope + // visual/frontend PR's body is missing a before/after screenshot table (or has an image outside a table, or + // a screenshot committed to the repo instead of uploaded to the PR). Same zero-hallucination short-circuit + // shape as blacklistMatch — fires ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only, so its close is + // tagged `closeKind: "screenshot_table"`. Absent / not-violated ⇒ no effect. The trigger only ever sets this + // when the repo's `screenshotTableGate.action` is `"close"` (the only enforcement mode this planner wires so + // far) — `"request_changes"`/`"comment"` stay advisory-only, surfaced elsewhere. + screenshotTableMatch?: { matched: boolean; reason: string | null } | undefined; pr: { mergeableState?: string | null | undefined; reviewDecision?: string | null | undefined; @@ -506,6 +514,14 @@ function reviewNagCloseMessage(authorLogin: string, pingCount: number, maxPings: return `Gittensory closed this because @${authorLogin} pinged @gittensory ${pingCount} times, above this repository's configured limit of ${maxPings}. Please wait for the cooldown window to pass before requesting review again. This is an automated maintenance action.`; } +// The close comment for the screenshot-table gate (#2006). `reason` is the repo-configured (or built-in +// default) templated contract message — already public-safe by construction (it is either the maintainer's own +// configured `.gittensory.yml` text or the static DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, never AI/user-derived), +// so it is interpolated directly, unlike blacklistCloseMessage's deliberately-static text. +function screenshotTableCloseMessage(reason: string): string { + return `${reason} This is an automated maintenance action.`; +} + /** * Plan best-effort assignment of the PR's opening contributor (#3182), independent of merge/close/CI outcome. * MUST run before the CI-pending settle-before-decide return below (#assign-before-ci-pending) — a PR that has @@ -624,6 +640,29 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne return actions; } + // Screenshot-table gate (#2006): same zero-hallucination short-circuit shape as the blacklist above — fires + // ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only. The trigger has already resolved scope (label/ + // path match) and run the deterministic body/diff check before ever setting this input; the planner's only + // job is to build the close plan under the repo's normal autonomy/dry-run/kill-switch gates. No coupled label + // (unlike blacklist/contributor-cap/review-nag) — the templated close comment already IS the full contract, + // so a separate enforcement label would be redundant noise on a PR that's about to be closed anyway. + const screenshotTableContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot; + if (input.screenshotTableMatch?.matched === true && screenshotTableContributor) { + if (acting("close")) { + const reason = input.screenshotTableMatch.reason ?? "missing a before/after screenshot table"; + actions.push({ + actionClass: "close", + requiresApproval: approval("close"), + reason: "missing before/after screenshot table", + closeReasons: ["missing before/after screenshot table"], + closeComment: sanitizePublicComment(screenshotTableCloseMessage(reason)), + closeKind: "screenshot_table", + ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}), + }); + } + return actions; + } + // Only a SKIPPED gate (genuinely not evaluated) drives no action. A NEUTRAL gate (first-time-contributor // grace, or eval-not-ready while state is still syncing) is gate-NON-BLOCKING: it flows to the disposition so // the PR is merged (clean+green) or HELD with a label — never left silently undecided. (#harm-stop neutral-silent-stuck) diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 136a156b92..9d334250eb 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -1,5 +1,5 @@ import { parse as parseYaml } from "yaml"; -import type { GatePolicyPack, GateRuleMode, JsonValue, LinkedIssueHardRulesConfig, LinkedIssueLabelPropagationConfig, PrTypeLabelSet, RepositorySettings, ReviewCheckMode, UnlinkedIssueGuardrailConfig } from "../types"; +import type { GatePolicyPack, GateRuleMode, JsonValue, LinkedIssueHardRulesConfig, LinkedIssueLabelPropagationConfig, PrTypeLabelSet, RepositorySettings, ReviewCheckMode, ScreenshotTableGateConfig, UnlinkedIssueGuardrailConfig } from "../types"; import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy } from "../settings/autonomy"; import { normalizeCommandAuthorizationPolicy } from "../settings/command-authorization"; import { mergeContributorBlacklists, normalizeContributorBlacklist } from "../settings/contributor-blacklist"; @@ -8,6 +8,7 @@ import { DEFAULT_TYPE_LABELS, MAX_TYPE_LABEL_NAME_LENGTH, normalizeTypeLabelSet import { DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION, normalizeLinkedIssueLabelPropagationConfig, VALID_LINKED_ISSUE_LABEL_PROPAGATION_MODES } from "../review/linked-issue-label-propagation"; import { DEFAULT_LINKED_ISSUE_HARD_RULES, isLinkedIssueHardRuleMode, normalizeLinkedIssueHardRulesConfig } from "../review/linked-issue-hard-rules-config"; import { DEFAULT_UNLINKED_ISSUE_GUARDRAIL, isUnlinkedIssueGuardrailMode, normalizeUnlinkedIssueGuardrailConfig } from "../review/unlinked-issue-guardrail-config"; +import { DEFAULT_SCREENSHOT_TABLE_GATE, isScreenshotTableGateAction, normalizeScreenshotTableGateConfig } from "../review/screenshot-table-gate"; import { normalizeModerationLabel, normalizeModerationRules } from "../settings/moderation-rules"; import { REES_ANALYZER_NAME_SET, type ReesAnalyzerName } from "../review/enrichment-analyzer-names"; import { hasUnsafeWildcardCount } from "./change-guardrail"; @@ -294,6 +295,10 @@ export type FocusManifestSettings = Partial< linkedIssueLabelPropagation?: Partial | undefined; linkedIssueHardRules?: Partial | undefined; unlinkedIssueGuardrail?: Partial | undefined; + // Screenshot-table gate (#2006): same sparse-partial merge reasoning as linkedIssueHardRules/ + // unlinkedIssueGuardrail above -- a manifest naming only `enabled` must not silently reset `whenLabels`/ + // `whenPaths`/`action`/`message` back to their defaults. + screenshotTableGate?: Partial | undefined; }; /** Field keys for the public review-panel rows a maintainer can show/hide via `review.fields`. */ @@ -1589,6 +1594,21 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[]) } else if (r.unlinkedIssueGuardrail !== undefined) { warnings.push(`Manifest "settings.unlinkedIssueGuardrail" must be an object; ignoring it and keeping any existing policy.`); } + // Screenshot-table gate (#2006): same sparse-partial overlay contract as unlinkedIssueGuardrail above -- a + // repo naming only `enabled` must not silently reset `whenLabels`/`whenPaths`/`action`/`message`. + if (typeof r.screenshotTableGate === "object" && r.screenshotTableGate !== null && !Array.isArray(r.screenshotTableGate)) { + const rawGate = r.screenshotTableGate as Record; + const validated = normalizeScreenshotTableGateConfig(rawGate, warnings); + const sparseGate: Partial = {}; + if (typeof rawGate.enabled === "boolean") sparseGate.enabled = validated.enabled; + if (Array.isArray(rawGate.whenLabels)) sparseGate.whenLabels = validated.whenLabels; + if (Array.isArray(rawGate.whenPaths)) sparseGate.whenPaths = validated.whenPaths; + if (isScreenshotTableGateAction(rawGate.action)) sparseGate.action = validated.action; + if (typeof rawGate.message === "string" && rawGate.message.trim().length > 0) sparseGate.message = validated.message; + out.screenshotTableGate = sparseGate; + } else if (r.screenshotTableGate !== undefined) { + warnings.push(`Manifest "settings.screenshotTableGate" must be an object; ignoring it and keeping any existing policy.`); + } // Contributor blacklist (#1425): `settings.contributorBlacklist` is a list of banned-login entries. Only set it // when at least one VALID entry survives normalization, so a malformed block never blanks the DB-configured // list via the resolver's `{...dbSettings, ...manifest.settings}` overlay. Normalization warnings are folded in. @@ -2858,6 +2878,7 @@ export function resolveEffectiveSettings( linkedIssueLabelPropagation: linkedIssueLabelPropagationOverride, linkedIssueHardRules: linkedIssueHardRulesOverride, unlinkedIssueGuardrail: unlinkedIssueGuardrailOverride, + screenshotTableGate: screenshotTableGateOverride, ...restManifestSettings } = manifest.settings; const effective: RepositorySettings = { ...dbSettings, ...restManifestSettings }; @@ -2904,6 +2925,16 @@ export function resolveEffectiveSettings( minConfidence: unlinkedIssueGuardrailOverride.minConfidence ?? base.minConfidence, }; } + if (screenshotTableGateOverride !== undefined) { + const base = dbSettings.screenshotTableGate ?? DEFAULT_SCREENSHOT_TABLE_GATE; + effective.screenshotTableGate = { + enabled: screenshotTableGateOverride.enabled ?? base.enabled, + whenLabels: screenshotTableGateOverride.whenLabels ?? base.whenLabels, + whenPaths: screenshotTableGateOverride.whenPaths ?? base.whenPaths, + action: screenshotTableGateOverride.action ?? base.action, + message: screenshotTableGateOverride.message ?? base.message, + }; + } applyGateConfigOverrides(effective, manifest.gate); // The dashboard "Require linked issue" toggle must not silently diverge from gate blocking: when the // boolean is on but linkedIssueGateMode is still off, treat it as a block requirement (#797). diff --git a/src/types.ts b/src/types.ts index 053c11f7fe..9bb1133d24 100644 --- a/src/types.ts +++ b/src/types.ts @@ -990,10 +990,28 @@ export type RepositorySettings = { /** Review-evasion protection: whether to post the public explanation comment before the enforcement close. * Default true. */ reviewEvasionComment?: boolean | undefined; + /** Config-driven before/after screenshot-table gate (#2006): a DETERMINISTIC check (no AI, zero hallucination + * risk) that a contributor visual/frontend PR's body contains a markdown table with before/after image + * markup, scoped to the repo's configured labels/paths (`whenLabels`/`whenPaths`, OR-matched). Off by + * default (`enabled: false`) -- opt in per repo, mirroring every other anti-abuse mechanism's shape. See + * `review/screenshot-table-gate.ts` for the normalizer and the pure evaluator. */ + screenshotTableGate?: ScreenshotTableGateConfig | undefined; createdAt?: string | null | undefined; updatedAt?: string | null | undefined; }; +export type ScreenshotTableGateAction = "close" | "request_changes" | "comment"; + +/** Per-repo config for the before/after screenshot-table gate (#2006). See {@link RepositorySettings.screenshotTableGate} + * and `review/screenshot-table-gate.ts` for the normalizer + pure evaluator. */ +export type ScreenshotTableGateConfig = { + enabled: boolean; + whenLabels: string[]; + whenPaths: string[]; + action: ScreenshotTableGateAction; + message?: string | undefined; +}; + export type CommandAuthorizationRole = "maintainer" | "collaborator" | "pr_author" | "confirmed_miner"; export type RepositoryCommandAuthorizationPolicy = { @@ -1131,7 +1149,7 @@ export type AgentPendingActionParams = { // (#2127), and the actuation-time live-CI re-check (#2364) — which only applies to a heuristic close — still // fires correctly once the row is replayed through pendingActionToPlanned, rather than silently skipping for // a lost discriminator. - closeKind?: "linked-issue-hard-rule" | "blacklist" | "contributor_cap" | "review_nag" | "heuristic"; + closeKind?: "linked-issue-hard-rule" | "blacklist" | "contributor_cap" | "review_nag" | "screenshot_table" | "heuristic"; // For a CI-driven heuristic close, persist the CI state that must still hold when the staged action replays // (#2364). This is separate from closeKind because heuristic closes also cover non-CI adverse signals. // ALWAYS set (to "failed" or "not_required") for a freshly planned heuristic close (#2478) -- never omitted -- diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index cddb63f216..a4656212e9 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1891,3 +1891,73 @@ describe("review-nag cooldown short-circuit (#2463)", () => { expect(classes(planAgentMaintenanceActions(nagged({ autonomy: { close: "auto" } })))).toEqual(["close", "label"]); }); }); + +describe("screenshot-table gate short-circuit (#2006)", () => { + const missingTable = (extra: Partial = {}) => + input({ + conclusion: "success", + autonomy: { close: "auto", approve: "auto", merge: "auto" }, + screenshotTableMatch: { matched: true, reason: "This pull request changes UI/visual code but its description is missing a before/after screenshot table." }, + ...extra, + }); + + it("closes a PR missing its screenshot table, winning over a passing gate (no merit review / merge)", () => { + const plan = planAgentMaintenanceActions(missingTable()); + expect(classes(plan)).toEqual(["close"]); // short-circuit: no approve/merge despite a SUCCESS gate; no coupled label + expect(plan[0]).toMatchObject({ actionClass: "close", closeKind: "screenshot_table" }); + expect(plan[0]?.closeReasons).toEqual(["missing before/after screenshot table"]); + }); + + it("interpolates the configured contract reason into the close comment", () => { + const plan = planAgentMaintenanceActions(missingTable()); + expect(plan[0]?.closeComment).toContain("before/after screenshot table"); + expect(plan[0]?.closeComment).toContain("This is an automated maintenance action."); + }); + + it("falls back to a generic reason when the trigger passes reason: null", () => { + const plan = planAgentMaintenanceActions(missingTable({ screenshotTableMatch: { matched: true, reason: null } })); + expect(plan[0]?.closeComment).toContain("missing a before/after screenshot table"); + }); + + it("pins the close to the reviewed head, mirroring blacklist/contributor-cap/review-nag", () => { + const plan = planAgentMaintenanceActions(missingTable({ pr: { labels: [], headSha: "h-reviewed" } })); + expect(plan.find((a) => a.actionClass === "close")).toMatchObject({ closeKind: "screenshot_table", expectedHeadSha: "h-reviewed" }); + }); + + it("omits expectedHeadSha when the PR record has no headSha (defensive fallback)", () => { + const plan = planAgentMaintenanceActions(missingTable()); + expect(plan.find((a) => a.actionClass === "close")?.expectedHeadSha).toBeUndefined(); + }); + + it("fires AHEAD of CI — closes even while CI is still pending (not the pending early-return)", () => { + expect(classes(planAgentMaintenanceActions(missingTable({ ciState: "pending" })))).toEqual(["close"]); + }); + + it("NEVER fires for the owner, an admin login, or an automation bot (standing rule) — the PR falls through to normal disposition", () => { + expect(classes(planAgentMaintenanceActions(missingTable({ authorIsOwner: true })))).not.toContain("close"); + expect(classes(planAgentMaintenanceActions(missingTable({ authorIsAdmin: true })))).not.toContain("close"); + expect(classes(planAgentMaintenanceActions(missingTable({ authorIsAutomationBot: true })))).not.toContain("close"); + }); + + it("no-ops when the match is not matched (normal disposition runs)", () => { + expect(classes(planAgentMaintenanceActions(missingTable({ screenshotTableMatch: { matched: false, reason: null } })))).not.toContain("close"); + }); + + it("plans nothing when `close` autonomy is not acting", () => { + expect(planAgentMaintenanceActions(missingTable({ autonomy: {} }))).toEqual([]); + expect(classes(planAgentMaintenanceActions(missingTable({ autonomy: { close: "auto" } })))).toEqual(["close"]); + }); + + it("is exempt from the close-precision breaker (no closeKind: 'heuristic', mirroring blacklist/contributor-cap/review-nag)", () => { + const plan = planAgentMaintenanceActions(missingTable()); + const closeAction = plan.find((a) => a.actionClass === "close"); + const downgraded = downgradeCloseToHold(plan, true, {}); + expect(downgraded).toEqual(plan); + expect(closeAction?.closeKind).not.toBe("heuristic"); + }); + + it("is independent of the blacklist short-circuit — a matched blacklist entry still wins when both are present", () => { + const plan = planAgentMaintenanceActions(missingTable({ blacklistMatch: { matched: true, reason: "plagiarism" } })); + expect(plan[0]).toMatchObject({ closeKind: "blacklist" }); + }); +}); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index fd68f72b9e..7067c2dbcc 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -344,6 +344,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { linkedIssueLabelPropagation: "linkedIssueLabelPropagation:", linkedIssueHardRules: "linkedIssueHardRules:", unlinkedIssueGuardrail: "unlinkedIssueGuardrail:", + screenshotTableGate: "screenshotTableGate:", } satisfies Record, string>; it.each(Object.entries(SETTINGS_FIELD_TOKENS))("documents settings.%s", (_field, token) => { @@ -2665,6 +2666,36 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(parsed.warnings).toContain(`Manifest "settings.unlinkedIssueGuardrail" must be an object; ignoring it and keeping any existing policy.`); }); + it("wires settings.screenshotTableGate into the manifest parser as a sparse override (#2006)", () => { + const parsed = parseFocusManifest({ settings: { screenshotTableGate: { enabled: true, whenLabels: ["frontend"], whenPaths: ["apps/ui/**"], action: "close", message: "custom" } } }); + expect(parsed.settings.screenshotTableGate).toEqual({ enabled: true, whenLabels: ["frontend"], whenPaths: ["apps/ui/**"], action: "close", message: "custom" }); + expect(parsed.warnings).toEqual([]); + }); + + it("resolveEffectiveSettings merges a partial screenshotTableGate override without clearing the lower-layer whenLabels/whenPaths (#2006)", () => { + const db = { screenshotTableGate: { enabled: false, whenLabels: ["frontend"], whenPaths: ["apps/ui/**"], action: "close" } } as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { screenshotTableGate: { enabled: true } } })); + expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: ["frontend"], whenPaths: ["apps/ui/**"], action: "close" }); + }); + + it("resolveEffectiveSettings falls back to the built-in default when the DB layer has no screenshotTableGate at all (#2006)", () => { + const db = {} as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { screenshotTableGate: { enabled: true } } })); + expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: [], whenPaths: [], action: "close" }); + }); + + it("drops a malformed screenshotTableGate.action field instead of replacing existing policy with defaults (#2006)", () => { + const parsed = parseFocusManifest({ settings: { screenshotTableGate: { enabled: true, action: "delete" } } }); + expect(parsed.settings.screenshotTableGate).toEqual({ enabled: true }); + expect(parsed.warnings.some((w) => w.includes("settings.requireScreenshotTable.action"))).toBe(true); + }); + + it("warns and ignores a malformed top-level screenshotTableGate value (#2006)", () => { + const parsed = parseFocusManifest({ settings: { screenshotTableGate: "oops" } }); + expect(parsed.settings.screenshotTableGate).toBeUndefined(); + expect(parsed.warnings).toContain(`Manifest "settings.screenshotTableGate" must be an object; ignoring it and keeping any existing policy.`); + }); + it("parses aiReview from settings: and lets gate.aiReview win in resolveEffectiveSettings", () => { const parsed = parseFocusManifest({ settings: { aiReviewMode: "advisory", aiReviewByok: true } }); expect(parsed.settings.aiReviewMode).toBe("advisory"); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 3704ab9759..e1d8855114 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -10290,6 +10290,138 @@ describe("queue processors", () => { expect(seen.comments.some((c) => c.includes("blocked from contributing"))).toBe(true); }); + it("screenshot-table gate (#2006): an in-scope contributor PR missing a before/after table is closed deterministically with NO AI call and no merit merge", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "n/a", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "all_prs", + publicSurface: "comment_only", + checkRunMode: "off", + gateCheckMode: "enabled", + aiReviewMode: "advisory", + autonomy: { close: "auto", label: "auto" }, + }); + // Scoped to the `visual` label only, config-as-code, nothing hardcoded — mirrors the blacklistLabel test above. + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { screenshotTableGate: { enabled: true, whenLabels: ["visual"] } } }, "repo_file"); + const seen = { closed: false, labels: [] as string[], comments: [] as string[] }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/56/files")) return Response.json([{ filename: "apps/ui/src/App.tsx", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/56/reviews")) return Response.json([]); + if (url.includes("/pulls/56/commits")) return Response.json([]); + if (url.endsWith("/pulls/56") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 56, state: "closed" }); } + if (url.endsWith("/pulls/56")) return Response.json({ number: 56, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis56" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis56/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis56/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/56/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/56/labels") && method === "POST") { seen.labels.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[])); return Response.json([]); } + if (url.includes("/issues/56/comments") && method === "POST") { seen.comments.push(String(JSON.parse(String(init?.body ?? "{}")).body ?? "")); return Response.json({ id: 1 }, { status: 201 }); } + if (url.includes("/issues/56/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-table-close", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 56, title: "New button color", state: "open", user: { login: "visual-contributor" }, head: { sha: "vis56" }, labels: [{ name: "visual" }], body: "Changed the button color. Closes #1", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }); + + // Deterministic gate: closed, and the AI was NEVER called for the disposition. + expect(aiCalls).toBe(0); + expect(seen.closed).toBe(true); + const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>(); + expect(closeAudit?.n).toBeGreaterThanOrEqual(1); + // No merit merge despite a clean+green+approved PR (the screenshot-table gate short-circuits ahead of merit). + const mergeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.merge'").first<{ n: number }>(); + expect(mergeAudit?.n).toBe(0); + // The close comment explains the missing table. + expect(seen.comments.some((c) => c.includes("before/after screenshot table"))).toBe(true); + }); + + it("screenshot-table gate (#2006): an in-scope PR WITH a valid before/after table is NOT closed by the gate (no false-positive)", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + }); + await upsertInstallation(env, { + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] }, + repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }], + }); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "all_prs", + publicSurface: "comment_only", + checkRunMode: "off", + gateCheckMode: "enabled", + autonomy: { close: "auto", merge: "auto" }, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { screenshotTableGate: { enabled: true, whenLabels: ["visual"] } } }, "repo_file"); + const seen = { closed: false }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/57/files")) return Response.json([{ filename: "apps/ui/src/App.tsx", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/57/reviews")) return Response.json([]); + if (url.includes("/pulls/57/commits")) return Response.json([]); + if (url.endsWith("/pulls/57") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 57, state: "closed" }); } + if (url.endsWith("/pulls/57")) return Response.json({ number: 57, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis57" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis57/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis57/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + if (url.includes("/issues/57/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/57/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-table-pass", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 57, + title: "New button color", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis57" }, + labels: [{ name: "visual" }], + body: "Changed the button color.\n\n| Before | After |\n| --- | --- |\n| ![before](https://x/before.png) | ![after](https://x/after.png) |\n\nCloses #1", + mergeable_state: "clean", + reviewDecision: "APPROVED", + }, + }, + }); + + // The valid before/after table means the deterministic gate never matches — no close of any kind fires. + expect(seen.closed).toBe(false); + const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>(); + expect(closeAudit?.n).toBe(0); + }); + describe("live migrations/** collision recheck (#2550)", () => { // Full merge-eligible stub set (clean + green + approved), reused across scenarios — a positive test proves // the collision hold actually suppresses what would otherwise merge; a negative test proves the check diff --git a/test/unit/repository-settings-screenshot-table-gate.test.ts b/test/unit/repository-settings-screenshot-table-gate.test.ts new file mode 100644 index 0000000000..318e7bd756 --- /dev/null +++ b/test/unit/repository-settings-screenshot-table-gate.test.ts @@ -0,0 +1,78 @@ +import { describe, expect, it } from "vitest"; +import { getRepositorySettings, upsertRepositorySettings } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +// #2006: the config-driven before/after screenshot-table gate is off by default (zero behavior change for an +// install that hasn't opted in) and layers whenLabels/whenPaths/action/message through the same DB round-trip +// every other anti-abuse mechanism uses (contributor blacklist / review-nag / review-evasion). +describe("repository_settings: screenshotTableGate (#2006)", () => { + it("getRepositorySettings returns the disabled default for a repo with no DB row at all", async () => { + const env = createTestEnv(); + const settings = await getRepositorySettings(env, "acme/brand-new-repo"); + expect(settings.screenshotTableGate).toEqual({ enabled: false, whenLabels: [], whenPaths: [], action: "close" }); + }); + + it("upsertRepositorySettings persists the disabled default when the caller omits screenshotTableGate entirely", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/omits-it" }); + const settings = await getRepositorySettings(env, "acme/omits-it"); + expect(settings.screenshotTableGate?.enabled).toBe(false); + }); + + it("round-trips a fully configured gate (enabled, scoped, custom action + message)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { + repoFullName: "acme/configured", + screenshotTableGate: { + enabled: true, + whenLabels: ["frontend", "visual"], + whenPaths: ["apps/ui/**"], + action: "close", + message: "Custom contract text", + }, + }); + const settings = await getRepositorySettings(env, "acme/configured"); + expect(settings.screenshotTableGate).toEqual({ + enabled: true, + whenLabels: ["frontend", "visual"], + whenPaths: ["apps/ui/**"], + action: "close", + message: "Custom contract text", + }); + }); + + it("a true read-modify-write caller carries the persisted value forward explicitly (no DB merge)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/round-trip", screenshotTableGate: { enabled: true, whenLabels: ["visual"], whenPaths: [], action: "close" } }); + const settings = await getRepositorySettings(env, "acme/round-trip"); + await upsertRepositorySettings(env, { ...settings, repoFullName: "acme/round-trip" }); + const after = await getRepositorySettings(env, "acme/round-trip"); + expect(after.screenshotTableGate).toEqual({ enabled: true, whenLabels: ["visual"], whenPaths: [], action: "close" }); + }); + + it("omits `message` entirely when unset (never persists an empty string)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/no-message", screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [], action: "close" } }); + const settings = await getRepositorySettings(env, "acme/no-message"); + expect(settings.screenshotTableGate?.message).toBeUndefined(); + }); + + it("an invalid persisted action column fails closed to the default (close) on read", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/malformed" }); + await env.DB.prepare("UPDATE repository_settings SET screenshot_table_gate_action = ? WHERE repo_full_name = ?").bind("nonsense", "acme/malformed").run(); + const settings = await getRepositorySettings(env, "acme/malformed"); + expect(settings.screenshotTableGate?.action).toBe("close"); + }); + + it("a malformed JSON whenLabels/whenPaths column fails closed to an empty list on read", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/bad-json" }); + await env.DB.prepare("UPDATE repository_settings SET screenshot_table_gate_when_labels_json = ?, screenshot_table_gate_when_paths_json = ? WHERE repo_full_name = ?") + .bind("not json", "not json", "acme/bad-json") + .run(); + const settings = await getRepositorySettings(env, "acme/bad-json"); + expect(settings.screenshotTableGate?.whenLabels).toEqual([]); + expect(settings.screenshotTableGate?.whenPaths).toEqual([]); + }); +}); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts new file mode 100644 index 0000000000..102d6065db --- /dev/null +++ b/test/unit/screenshot-table-gate.test.ts @@ -0,0 +1,299 @@ +import { describe, expect, it } from "vitest"; +import { + DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, + DEFAULT_SCREENSHOT_TABLE_GATE, + evaluateScreenshotTableGate, + hasCommittedImageFile, + hasImageBearingMarkdownTable, + hasImageOutsideTable, + isScreenshotTableGateAction, + isScreenshotTableGateInScope, + normalizeScreenshotTableGateConfig, +} from "../../src/review/screenshot-table-gate"; +import type { ScreenshotTableGateConfig } from "../../src/types"; + +function config(overrides: Partial = {}): ScreenshotTableGateConfig { + return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], ...overrides }; +} + +const TABLE_BODY = ["| Before | After |", "| --- | --- |", "| ![before](https://x/before.png) | ![after](https://x/after.png) |"].join("\n"); + +describe("isScreenshotTableGateAction", () => { + it("accepts every valid action", () => { + expect(isScreenshotTableGateAction("close")).toBe(true); + expect(isScreenshotTableGateAction("request_changes")).toBe(true); + expect(isScreenshotTableGateAction("comment")).toBe(true); + }); + + it("rejects a non-string or unknown value", () => { + expect(isScreenshotTableGateAction("hold")).toBe(false); + expect(isScreenshotTableGateAction(123)).toBe(false); + expect(isScreenshotTableGateAction(undefined)).toBe(false); + }); +}); + +describe("hasImageBearingMarkdownTable", () => { + it("detects a markdown table with image cells (before/after markup)", () => { + expect(hasImageBearingMarkdownTable(TABLE_BODY)).toBe(true); + }); + + it("detects an tag inside a table cell too", () => { + const body = ["| Before | After |", "| --- | --- |", '| | |'].join("\n"); + expect(hasImageBearingMarkdownTable(body)).toBe(true); + }); + + it("returns false for a table with no image markup in any row", () => { + const body = ["| Before | After |", "| --- | --- |", "| looks the same | looks the same |"].join("\n"); + expect(hasImageBearingMarkdownTable(body)).toBe(false); + }); + + it("returns false when there is no table at all", () => { + expect(hasImageBearingMarkdownTable("Just a plain description, no table here.")).toBe(false); + }); + + it("returns false for a header row with no valid separator row beneath it", () => { + const body = ["| Before | After |", "not a separator", "| ![a](x.png) | ![b](y.png) |"].join("\n"); + expect(hasImageBearingMarkdownTable(body)).toBe(false); + }); + + it("returns false for null/undefined/empty body", () => { + expect(hasImageBearingMarkdownTable(null)).toBe(false); + expect(hasImageBearingMarkdownTable(undefined)).toBe(false); + expect(hasImageBearingMarkdownTable("")).toBe(false); + }); + + it("supports an aligned separator row (:---:, ---:, etc.)", () => { + const body = ["| Before | After |", "|:---:|:---:|", "| ![a](x.png) | ![b](y.png) |"].join("\n"); + expect(hasImageBearingMarkdownTable(body)).toBe(true); + }); +}); + +describe("hasImageOutsideTable", () => { + it("detects a bare inline image outside any table", () => { + expect(hasImageOutsideTable("Here is my before screenshot: ![before](https://x/before.png)")).toBe(true); + }); + + it("returns false when the only image markup is inside a table row", () => { + expect(hasImageOutsideTable(TABLE_BODY)).toBe(false); + }); + + it("returns false for a body with no image markup at all", () => { + expect(hasImageOutsideTable("No images here.")).toBe(false); + }); + + it("returns false for null/undefined/empty body", () => { + expect(hasImageOutsideTable(null)).toBe(false); + expect(hasImageOutsideTable(undefined)).toBe(false); + expect(hasImageOutsideTable("")).toBe(false); + }); +}); + +describe("hasCommittedImageFile", () => { + it("flags a committed image file under a scoped path", () => { + expect(hasCommittedImageFile(["apps/ui/src/screenshot.png"], ["apps/ui/**"])).toBe(true); + }); + + it("does not flag an image file OUTSIDE the scoped paths", () => { + expect(hasCommittedImageFile(["docs/logo.png"], ["apps/ui/**"])).toBe(false); + }); + + it("checks every changed path when scopedPaths is empty", () => { + expect(hasCommittedImageFile(["random/screenshot.jpg"], [])).toBe(true); + }); + + it("does not flag a non-image file", () => { + expect(hasCommittedImageFile(["apps/ui/src/component.tsx"], ["apps/ui/**"])).toBe(false); + }); + + it("never flags a committed SVG (excluded from the image-extension set)", () => { + expect(hasCommittedImageFile(["apps/ui/src/icon.svg"], [])).toBe(false); + }); + + it("matches every accepted raster extension case-insensitively", () => { + for (const ext of [".png", ".jpg", ".jpeg", ".gif", ".webp", ".PNG"]) { + expect(hasCommittedImageFile([`apps/ui/shot${ext}`], [])).toBe(true); + } + }); +}); + +describe("isScreenshotTableGateInScope", () => { + it("is in scope for every PR when both whenLabels and whenPaths are empty", () => { + expect(isScreenshotTableGateInScope(config(), [], [])).toBe(true); + }); + + it("matches on label (case-insensitive)", () => { + expect(isScreenshotTableGateInScope(config({ whenLabels: ["Frontend"] }), ["frontend"], [])).toBe(true); + }); + + it("matches on path glob", () => { + expect(isScreenshotTableGateInScope(config({ whenPaths: ["apps/ui/**"] }), [], ["apps/ui/src/App.tsx"])).toBe(true); + }); + + it("is out of scope when neither labels nor paths match (both configured)", () => { + expect(isScreenshotTableGateInScope(config({ whenLabels: ["frontend"], whenPaths: ["apps/ui/**"] }), ["backend"], ["src/api/routes.ts"])).toBe(false); + }); + + it("label match alone is sufficient even when whenPaths is also configured and doesn't match", () => { + expect(isScreenshotTableGateInScope(config({ whenLabels: ["frontend"], whenPaths: ["apps/ui/**"] }), ["frontend"], ["src/api/routes.ts"])).toBe(true); + }); + + it("path match alone is sufficient even when whenLabels is also configured and doesn't match", () => { + expect(isScreenshotTableGateInScope(config({ whenLabels: ["frontend"], whenPaths: ["apps/ui/**"] }), ["backend"], ["apps/ui/src/App.tsx"])).toBe(true); + }); + + it("only whenLabels configured (whenPaths empty) -- scope decided purely by label", () => { + expect(isScreenshotTableGateInScope(config({ whenLabels: ["frontend"] }), ["backend"], ["apps/ui/src/App.tsx"])).toBe(false); + }); + + it("only whenPaths configured (whenLabels empty) -- scope decided purely by path", () => { + expect(isScreenshotTableGateInScope(config({ whenPaths: ["apps/ui/**"] }), ["frontend"], ["src/api/routes.ts"])).toBe(false); + }); +}); + +describe("normalizeScreenshotTableGateConfig", () => { + it("returns the disabled default for undefined/null input", () => { + expect(normalizeScreenshotTableGateConfig(undefined, [])).toEqual(config()); + expect(normalizeScreenshotTableGateConfig(null, [])).toEqual(config()); + }); + + it("warns and falls back to default for a non-object input", () => { + const warnings: string[] = []; + expect(normalizeScreenshotTableGateConfig("nope", warnings)).toEqual(config()); + expect(warnings).toEqual(["settings.requireScreenshotTable must be an object; using the default (disabled)."]); + }); + + it("warns and falls back to default for an array input", () => { + const warnings: string[] = []; + expect(normalizeScreenshotTableGateConfig([], warnings)).toEqual(config()); + expect(warnings.length).toBeGreaterThan(0); + }); + + it("parses a fully valid object", () => { + const result = normalizeScreenshotTableGateConfig( + { enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "comment", message: "custom text" }, + [], + ); + expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "comment", message: "custom text" }); + }); + + it("rejects a non-boolean enabled with a warning, falling back to false", () => { + const warnings: string[] = []; + expect(normalizeScreenshotTableGateConfig({ enabled: "yes" }, warnings).enabled).toBe(false); + expect(warnings.some((w) => w.includes("enabled"))).toBe(true); + }); + + it("rejects an invalid action with a warning, falling back to close", () => { + const warnings: string[] = []; + expect(normalizeScreenshotTableGateConfig({ action: "delete" }, warnings).action).toBe("close"); + expect(warnings.some((w) => w.includes("action"))).toBe(true); + }); + + it("rejects a non-string/empty message with a warning, falling back to undefined", () => { + const warnings: string[] = []; + const result = normalizeScreenshotTableGateConfig({ message: " " }, warnings); + expect(result.message).toBeUndefined(); + expect(warnings.some((w) => w.includes("message"))).toBe(true); + }); + + it("accepts a valid non-empty message and trims it", () => { + expect(normalizeScreenshotTableGateConfig({ message: " hi " }, []).message).toBe("hi"); + }); + + it("rejects a non-array whenLabels/whenPaths with a warning, falling back to []", () => { + const warnings: string[] = []; + const result = normalizeScreenshotTableGateConfig({ whenLabels: "frontend", whenPaths: "apps/ui" }, warnings); + expect(result.whenLabels).toEqual([]); + expect(result.whenPaths).toEqual([]); + expect(warnings.length).toBe(2); + }); + + it("drops non-string/empty entries within whenLabels/whenPaths with a warning per entry", () => { + const warnings: string[] = []; + const result = normalizeScreenshotTableGateConfig({ whenLabels: ["frontend", "", 5, " "], whenPaths: [42] }, warnings); + expect(result.whenLabels).toEqual(["frontend"]); + expect(result.whenPaths).toEqual([]); + expect(warnings.length).toBeGreaterThan(0); + }); + + it("caps whenLabels/whenPaths at their max entry count", () => { + const warnings: string[] = []; + const many = Array.from({ length: 60 }, (_, i) => `label-${i}`); + const result = normalizeScreenshotTableGateConfig({ whenLabels: many }, warnings); + expect(result.whenLabels.length).toBe(50); + expect(warnings.some((w) => w.includes("capped"))).toBe(true); + }); +}); + +describe("evaluateScreenshotTableGate", () => { + it("no violation when the gate is disabled, regardless of everything else", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: false, whenLabels: ["frontend"] }), + prBody: "no table here", + prLabels: ["frontend"], + changedFiles: ["apps/ui/src/App.tsx"], + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("no violation when enabled but the PR is out of scope", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, whenLabels: ["frontend"] }), + prBody: "no table here", + prLabels: ["backend"], + changedFiles: [], + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("no violation when in scope AND a valid table is present (no stray images, no committed image)", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: ["apps/ui/src/App.tsx"], + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("violates when in scope and there is no table at all", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: "Just changed some CSS, trust me.", + prLabels: [], + changedFiles: [], + }); + expect(result.violated).toBe(true); + expect(result.reason).toBe(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE); + }); + + it("violates when a valid table exists but an image is ALSO pasted outside it", () => { + const bodyWithStray = `${TABLE_BODY}\n\nAlso here's a bonus shot: ![bonus](https://x/bonus.png)`; + const result = evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: bodyWithStray, prLabels: [], changedFiles: [] }); + expect(result.violated).toBe(true); + }); + + it("violates when a valid table exists but a screenshot was committed to the repo under a scoped path", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, whenPaths: ["apps/ui/**"] }), + prBody: TABLE_BODY, + prLabels: [], + changedFiles: ["apps/ui/src/App.tsx", "apps/ui/public/screenshot.png"], + }); + expect(result.violated).toBe(true); + }); + + it("uses the repo-configured message override instead of the default", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, message: "Please add screenshots, thanks!" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toBe("Please add screenshots, thanks!"); + }); + + it("handles a null/undefined PR body without throwing (treated as no table)", () => { + expect(evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: null, prLabels: [], changedFiles: [] }).violated).toBe(true); + expect(evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: undefined, prLabels: [], changedFiles: [] }).violated).toBe(true); + }); +}); From 5118371b574e4d91c0850f936150bedf56a387ad Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:04:44 -0700 Subject: [PATCH 2/3] fix(review): renumber migration collision, raise manifest size ceiling, close coverage gaps Renumbers 0115_screenshot_table_gate.sql to 0117 after a concurrently-merged PR claimed 0115. Raises MAX_FOCUS_MANIFEST_BYTES from 64 KiB to 128 KiB -- gittensory.full.yml (our own reference doc, round-trip tested by config-templates.test.ts) had already reached 65522/65536 bytes on main before this PR's own doc addition, so any concurrent PR's config-doc growth would have tripped the old ceiling next; a real per-repo .gittensory.yml never needs anywhere near this size, so the DoS-guard intent is unaffected. Adds a regression test for parseJsonStringArray's non-array (valid JSON, wrong shape) fail-closed branch, previously only covered by the JSON-syntax- error path. --- .gittensory.yml.example | 17 +- apps/gittensory-ui/public/openapi.json | 221 ++++++++++++++++++ config/examples/gittensory.full.yml | 17 +- ...ate.sql => 0117_screenshot_table_gate.sql} | 0 src/signals/focus-manifest.ts | 6 +- ...ory-settings-screenshot-table-gate.test.ts | 11 + 6 files changed, 251 insertions(+), 21 deletions(-) rename migrations/{0115_screenshot_table_gate.sql => 0117_screenshot_table_gate.sql} (100%) diff --git a/.gittensory.yml.example b/.gittensory.yml.example index d450a61d63..aaccfb1453 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -803,18 +803,15 @@ settings: # mode: off # off | hold. Default: off. # minConfidence: 0.85 # Number 0-1. Minimum AI-verifier confidence to treat a candidate as a real match. - # Before/after screenshot-table gate (#2006): a DETERMINISTIC check (no AI, zero hallucination risk) that a - # contributor visual/frontend PR's body contains a markdown table with before/after image markup (either - # `![alt](url)` or ``, inside a `| ... |` table). Scoped to whenLabels OR whenPaths (either match is - # enough); both empty ⇒ enforced on every PR once enabled. Also rejects an image pasted outside any table, or - # a screenshot committed to the repo under a scoped path (should be uploaded to the PR body instead). Off by - # default -- opt in per repo. + # Before/after screenshot-table gate (#2006): deterministic check that a visual/frontend PR's body has a + # before/after image table. Scoped to whenLabels OR whenPaths (either matches); both empty = every PR. Off + # by default. # screenshotTableGate: - # enabled: false # Default: false (off). - # whenLabels: [frontend, visual] # Enforce only when the PR carries one of these labels. Default: [] (no label scoping). - # whenPaths: ["apps/ui/**", "src/**/*.tsx"] # Enforce only when a changed file matches one of these globs. Default: [] (no path scoping). + # enabled: false # Default: false. + # whenLabels: [frontend, visual] # Default: [] (no label scoping). + # whenPaths: ["apps/ui/**", "src/**/*.tsx"] # Default: [] (no path scoping). # action: close # close | request_changes | comment. Default: close. - # message: "Custom close reason..." # Overrides the built-in templated contract message. Default: null (built-in message). + # message: "Custom close reason..." # Default: null (built-in message). # Maintainer AI review tuning (`.gittensory.yml` top-level `review:` block). These knobs shape the advisory AI # review prompt and file selection only — gate/slop/secret-scan are unaffected. diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index b4c5b68289..e44afb3e7c 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -9278,6 +9278,16 @@ "whenPaths", "action" ] + }, + "regateSweepOrderMode": { + "type": "string", + "enum": [ + "staleness", + "oldest-first" + ] + }, + "publicQualityMetrics": { + "type": "boolean" } }, "required": [ @@ -9288,6 +9298,7 @@ "checkRunMode", "checkRunDetailLevel", "gateCheckMode", + "regateSweepOrderMode", "reviewCheckMode", "gatePack", "linkedIssueGateMode", @@ -9996,6 +10007,16 @@ "defaultAllowed", "commandOverrides" ] + }, + "regateSweepOrderMode": { + "type": "string", + "enum": [ + "staleness", + "oldest-first" + ] + }, + "publicQualityMetrics": { + "type": "boolean" } }, "required": [ @@ -10006,6 +10027,7 @@ "checkRunMode", "checkRunDetailLevel", "gateCheckMode", + "regateSweepOrderMode", "reviewCheckMode", "gatePack", "linkedIssueGateMode", @@ -10024,6 +10046,7 @@ "includeMaintainerAuthors", "requireLinkedIssue", "badgeEnabled", + "publicQualityMetrics", "aiReviewMode", "aiReviewByok", "aiReviewProvider", @@ -13453,6 +13476,164 @@ "maintainerNextSteps", "privateSummary" ] + }, + "PublicQualityMetrics": { + "type": "object", + "properties": { + "repoFullName": { + "type": "string" + }, + "generatedAt": { + "type": "string" + }, + "gate": { + "type": "object", + "properties": { + "blocked": { + "type": "number" + }, + "blockedThenMerged": { + "type": "number" + }, + "falsePositiveRate": { + "type": "number", + "nullable": true + }, + "precisionPct": { + "type": "number", + "nullable": true + }, + "topGateTypes": { + "type": "array", + "items": { + "type": "object", + "properties": { + "gateType": { + "type": "string" + }, + "blocked": { + "type": "number" + }, + "blockedThenMerged": { + "type": "number" + }, + "falsePositiveRate": { + "type": "number", + "nullable": true + }, + "precisionPct": { + "type": "number", + "nullable": true + } + }, + "required": [ + "gateType", + "blocked", + "blockedThenMerged", + "falsePositiveRate", + "precisionPct" + ] + } + } + }, + "required": [ + "blocked", + "blockedThenMerged", + "falsePositiveRate", + "precisionPct", + "topGateTypes" + ] + }, + "outcomes": { + "type": "object", + "properties": { + "merged": { + "type": "number" + }, + "closed": { + "type": "number" + }, + "mergeRatioPct": { + "type": "number", + "nullable": true + } + }, + "required": [ + "merged", + "closed", + "mergeRatioPct" + ] + }, + "slop": { + "type": "object", + "properties": { + "totalResolved": { + "type": "number" + }, + "overallMergeRate": { + "type": "number", + "nullable": true + }, + "discriminates": { + "type": "boolean", + "nullable": true + } + }, + "required": [ + "totalResolved", + "overallMergeRate", + "discriminates" + ] + }, + "trend": { + "type": "array", + "items": { + "type": "object", + "properties": { + "weekStart": { + "type": "string" + }, + "gateBlocked": { + "type": "number" + }, + "gateBlockedThenMerged": { + "type": "number" + }, + "gateFalsePositiveRate": { + "type": "number", + "nullable": true + }, + "outcomesMerged": { + "type": "number" + }, + "outcomesClosed": { + "type": "number" + }, + "mergeRatioPct": { + "type": "number", + "nullable": true + } + }, + "required": [ + "weekStart", + "gateBlocked", + "gateBlockedThenMerged", + "gateFalsePositiveRate", + "outcomesMerged", + "outcomesClosed", + "mergeRatioPct" + ] + } + } + }, + "required": [ + "repoFullName", + "generatedAt", + "gate", + "outcomes", + "slop", + "trend" + ] } }, "parameters": {}, @@ -16554,6 +16735,46 @@ } ] } + }, + "/v1/public/repos/{owner}/{repo}/quality": { + "get": { + "parameters": [ + { + "schema": { + "type": "string" + }, + "required": true, + "name": "owner", + "in": "path" + }, + { + "schema": { + "type": "string" + }, + "required": true, + "name": "repo", + "in": "path" + } + ], + "responses": { + "200": { + "description": "Public per-repo review-quality metrics: gate false-positive rates, merge-vs-close ratio, and weekly trend. Aggregate counts only; opt-in via publicQualityMetrics.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/PublicQualityMetrics" + } + } + } + }, + "404": { + "description": "Repo is unknown/private/uninstalled or has not opted in" + }, + "503": { + "description": "Public quality metrics are temporarily unavailable" + } + } + } } }, "servers": [ diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index bbd5386454..82e2b5b9ec 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -816,18 +816,15 @@ settings: # mode: off # off | hold. Default: off. # minConfidence: 0.85 # Number 0-1. Minimum AI-verifier confidence to treat a candidate as a real match. - # Before/after screenshot-table gate (#2006): a DETERMINISTIC check (no AI, zero hallucination risk) that a - # contributor visual/frontend PR's body contains a markdown table with before/after image markup (either - # `![alt](url)` or ``, inside a `| ... |` table). Scoped to whenLabels OR whenPaths (either match is - # enough); both empty ⇒ enforced on every PR once enabled. Also rejects an image pasted outside any table, or - # a screenshot committed to the repo under a scoped path (should be uploaded to the PR body instead). Off by - # default -- opt in per repo. + # Before/after screenshot-table gate (#2006): deterministic check that a visual/frontend PR's body has a + # before/after image table. Scoped to whenLabels OR whenPaths (either matches); both empty = every PR. Off + # by default. # screenshotTableGate: - # enabled: false # Default: false (off). - # whenLabels: [frontend, visual] # Enforce only when the PR carries one of these labels. Default: [] (no label scoping). - # whenPaths: ["apps/ui/**", "src/**/*.tsx"] # Enforce only when a changed file matches one of these globs. Default: [] (no path scoping). + # enabled: false # Default: false. + # whenLabels: [frontend, visual] # Default: [] (no label scoping). + # whenPaths: ["apps/ui/**", "src/**/*.tsx"] # Default: [] (no path scoping). # action: close # close | request_changes | comment. Default: close. - # message: "Custom close reason..." # Overrides the built-in templated contract message. Default: null (built-in message). + # message: "Custom close reason..." # Default: null (built-in message). # Maintainer AI review tuning (`.gittensory.yml` top-level `review:` block). These knobs shape the advisory AI # review prompt and file selection only — gate/slop/secret-scan are unaffected. diff --git a/migrations/0115_screenshot_table_gate.sql b/migrations/0117_screenshot_table_gate.sql similarity index 100% rename from migrations/0115_screenshot_table_gate.sql rename to migrations/0117_screenshot_table_gate.sql diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 9d334250eb..7a37ec9570 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -700,7 +700,11 @@ export type FocusManifestGuidance = { const MAX_LIST_ITEMS = 200; const MAX_ITEM_LENGTH = 300; const MAX_GLOBSTAR_SLASH_ALTERNATIVES = 128; -export const MAX_FOCUS_MANIFEST_BYTES = 64 * 1024; +// 128 KiB, not 64 KiB: gittensory.full.yml (our own reference doc, parsed by config-templates.test.ts as a +// round-trip check) organically grows every time a new review.* knob ships and had already reached 65522/65536 +// bytes on main before this comment was written -- one doc line from any PR would trip the old ceiling. A real +// per-repo .gittensory.yml never needs anywhere near this size, so the DoS-guard intent is unaffected (#2006). +export const MAX_FOCUS_MANIFEST_BYTES = 128 * 1024; const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { present: false, diff --git a/test/unit/repository-settings-screenshot-table-gate.test.ts b/test/unit/repository-settings-screenshot-table-gate.test.ts index 318e7bd756..caf94336aa 100644 --- a/test/unit/repository-settings-screenshot-table-gate.test.ts +++ b/test/unit/repository-settings-screenshot-table-gate.test.ts @@ -75,4 +75,15 @@ describe("repository_settings: screenshotTableGate (#2006)", () => { expect(settings.screenshotTableGate?.whenLabels).toEqual([]); expect(settings.screenshotTableGate?.whenPaths).toEqual([]); }); + + it("REGRESSION: valid JSON that parses to a non-array (an object) also fails closed to an empty list, not just a JSON syntax error", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/non-array-json" }); + await env.DB.prepare("UPDATE repository_settings SET screenshot_table_gate_when_labels_json = ?, screenshot_table_gate_when_paths_json = ? WHERE repo_full_name = ?") + .bind('{"frontend":true}', "42", "acme/non-array-json") + .run(); + const settings = await getRepositorySettings(env, "acme/non-array-json"); + expect(settings.screenshotTableGate?.whenLabels).toEqual([]); + expect(settings.screenshotTableGate?.whenPaths).toEqual([]); + }); }); From 60ba041515bcdc8025d2471e4241b4a65cb0f089 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:23:28 -0700 Subject: [PATCH 3/3] test(review): cover the screenshotTableGate enabled/action fallback branch --- test/unit/focus-manifest.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 7067c2dbcc..a8fe6b63a8 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -2684,6 +2684,12 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: [], whenPaths: [], action: "close" }); }); + it("resolveEffectiveSettings keeps the DB layer's enabled/action when the manifest override omits them (#2006)", () => { + const db = { screenshotTableGate: { enabled: true, whenLabels: ["frontend"], whenPaths: [], action: "comment" } } as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { screenshotTableGate: { whenPaths: ["apps/ui/**"] } } })); + expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: ["frontend"], whenPaths: ["apps/ui/**"], action: "comment" }); + }); + it("drops a malformed screenshotTableGate.action field instead of replacing existing policy with defaults (#2006)", () => { const parsed = parseFocusManifest({ settings: { screenshotTableGate: { enabled: true, action: "delete" } } }); expect(parsed.settings.screenshotTableGate).toEqual({ enabled: true });