diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index 0b04a70ed9..ecf9b9bfaa 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -9498,6 +9498,9 @@ "items": { "type": "string" } + }, + "skillFileUrl": { + "type": "string" } }, "required": [ diff --git a/migrations/0133_screenshot_table_gate_skill_link.sql b/migrations/0133_screenshot_table_gate_skill_link.sql new file mode 100644 index 0000000000..ad2dc93455 --- /dev/null +++ b/migrations/0133_screenshot_table_gate_skill_link.sql @@ -0,0 +1,8 @@ +-- Contributor skill-file link for the screenshot-table gate (#4540 follow-up). Appended to the +-- AUTO-GENERATED rejection message (both matrix and presence mode) so a closed contributor always gets +-- pointed at the exact evidence contract, not just told evidence is missing. Nullable, same "no override +-- configured" shape as screenshot_table_gate_message (migration 0117) -- deliberately a SEPARATE field +-- from that one: message is a full replacement a maintainer uses for total control over the wording, +-- while skill_file_url only ever appends to whichever message is already being shown (auto-generated, +-- specific-missing-pairs text included), so setting one doesn't cost the other. +ALTER TABLE repository_settings ADD COLUMN screenshot_table_gate_skill_file_url TEXT; diff --git a/packages/gittensory-engine/src/focus-manifest.ts b/packages/gittensory-engine/src/focus-manifest.ts index fbfca523a9..e07190bedd 100644 --- a/packages/gittensory-engine/src/focus-manifest.ts +++ b/packages/gittensory-engine/src/focus-manifest.ts @@ -1883,6 +1883,7 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[], if (typeof rawGate.message === "string" && rawGate.message.trim().length > 0) sparseGate.message = validated.message; if (Array.isArray(rawGate.requireViewports)) sparseGate.requireViewports = validated.requireViewports; if (Array.isArray(rawGate.requireThemes)) sparseGate.requireThemes = validated.requireThemes; + if (typeof rawGate.skillFileUrl === "string" && rawGate.skillFileUrl.trim().length > 0) sparseGate.skillFileUrl = validated.skillFileUrl; out.screenshotTableGate = sparseGate; } else if (r.screenshotTableGate !== undefined) { warnings.push(`Manifest "settings.screenshotTableGate" must be an object; ignoring it and keeping any existing policy.`); diff --git a/packages/gittensory-engine/src/review/screenshot-table-gate.ts b/packages/gittensory-engine/src/review/screenshot-table-gate.ts index 4d02e30957..aa90875ceb 100644 --- a/packages/gittensory-engine/src/review/screenshot-table-gate.ts +++ b/packages/gittensory-engine/src/review/screenshot-table-gate.ts @@ -16,6 +16,7 @@ const MAX_LABEL_CHARS = 100; const MAX_PATH_CHARS = 300; const MAX_MATRIX_DIMENSION = 12; const MAX_MATRIX_TOKEN_CHARS = 40; +const MAX_SKILL_FILE_URL_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 @@ -83,6 +84,7 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str if (record.message !== undefined && message === undefined) { warnings.push("settings.requireScreenshotTable.message must be a non-empty string; using the default message."); } + const skillFileUrl = normalizeSkillFileUrl(record.skillFileUrl, warnings); return { enabled, whenLabels: normalizeStringList(record.whenLabels, "whenLabels", MAX_LABELS, MAX_LABEL_CHARS, warnings), @@ -91,9 +93,23 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str requireViewports: normalizeStringList(record.requireViewports, "requireViewports", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), requireThemes: normalizeStringList(record.requireThemes, "requireThemes", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), ...(message !== undefined ? { message } : {}), + ...(skillFileUrl !== undefined ? { skillFileUrl } : {}), }; } +/** Validate a `skillFileUrl` override: same trust/validation level as `message` above (a trusted + * maintainer-authored config value, never fetched server-side -- it is only ever embedded as TEXT in a + * GitHub comment/close reason, so there is no SSRF surface here to guard against, unlike a URL the + * server would dereference). Malformed values are dropped with a warning, never silently coerced. */ +function normalizeSkillFileUrl(value: unknown, warnings: string[]): string | undefined { + if (value === undefined) return undefined; + if (typeof value !== "string" || value.trim().length === 0 || value.trim().length > MAX_SKILL_FILE_URL_CHARS) { + warnings.push(`settings.requireScreenshotTable.skillFileUrl must be a non-empty string no longer than ${MAX_SKILL_FILE_URL_CHARS} characters; ignoring it.`); + return undefined; + } + return value.trim(); +} + /** Linear-time markdown table separator check. The previous single-regex form nested unbounded `\\s*` inside a * repeated group and could catastrophically backtrack on attacker-controlled PR bodies; this splits on `|` and * validates each cell independently instead. */ @@ -255,6 +271,13 @@ export function buildScreenshotMatrixMessage(missing: ScreenshotMatrixPair[]): s ); } +/** Append a contributor skill-file link to an auto-generated rejection message (#4540 follow-up). A no-op + * when `skillFileUrl` is unset -- callers only reach this on the AUTO-GENERATED path (a `message` + * override already owns its entire text and is never passed through here). */ +function appendSkillLink(text: string, skillFileUrl: string | undefined): string { + return skillFileUrl ? `${text}\n\nSee ${skillFileUrl} for the exact format and examples.` : text; +} + /** 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 @@ -313,12 +336,12 @@ export function evaluateScreenshotTableGate(input: { if (matrixPairs.length > 0) { const missing = missingScreenshotMatrixPairs(input.prBody, matrixPairs); if (missing.length === 0) return NO_VIOLATION; - return { violated: true, reason: config.message ?? buildScreenshotMatrixMessage(missing) }; + return { violated: true, reason: config.message ?? appendSkillLink(buildScreenshotMatrixMessage(missing), config.skillFileUrl) }; } 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 }; + return { violated: true, reason: config.message ?? appendSkillLink(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, config.skillFileUrl) }; } diff --git a/packages/gittensory-engine/src/types/manifest-deps-types.ts b/packages/gittensory-engine/src/types/manifest-deps-types.ts index f0db2655ac..72fd36c668 100644 --- a/packages/gittensory-engine/src/types/manifest-deps-types.ts +++ b/packages/gittensory-engine/src/types/manifest-deps-types.ts @@ -27,11 +27,16 @@ export type ScreenshotTableGateConfig = { whenLabels: string[]; whenPaths: string[]; action: ScreenshotTableGateAction; + // Full replacement for the rejection reason -- see src/types.ts's mirror of this type for the full + // rationale (unset ⇒ auto-generated message + skillFileUrl; set ⇒ used verbatim, skillFileUrl ignored). message?: string | undefined; // Viewport x theme completeness matrix (#4535) -- see src/types.ts's mirror of this type for the full // rationale. requireViewports: string[]; requireThemes: string[]; + // Contributor skill-file link appended to the auto-generated message (#4540 follow-up) -- see + // src/types.ts's mirror of this type for the full rationale. + skillFileUrl?: string | undefined; }; export type CommandAuthorizationRole = "maintainer" | "collaborator" | "pr_author" | "confirmed_miner"; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 7b9d8d2ab6..b22e6cc95f 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -860,6 +860,7 @@ export async function upsertRepositorySettings(env: Env, settings: Partial nowIso()), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }); diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 65c86d0b00..b7c1032718 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -792,6 +792,7 @@ export const RepositorySettingsSchema = z requireViewports: z.array(z.string()), requireThemes: z.array(z.string()), message: z.string().optional(), + skillFileUrl: z.string().optional(), }) .optional(), createdAt: z.string().nullable().optional(), diff --git a/src/review/screenshot-table-gate.ts b/src/review/screenshot-table-gate.ts index b232d4c2f1..32dfb5afc1 100644 --- a/src/review/screenshot-table-gate.ts +++ b/src/review/screenshot-table-gate.ts @@ -16,6 +16,7 @@ const MAX_LABEL_CHARS = 100; const MAX_PATH_CHARS = 300; const MAX_MATRIX_DIMENSION = 12; const MAX_MATRIX_TOKEN_CHARS = 40; +const MAX_SKILL_FILE_URL_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 @@ -83,6 +84,7 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str if (record.message !== undefined && message === undefined) { warnings.push("settings.requireScreenshotTable.message must be a non-empty string; using the default message."); } + const skillFileUrl = normalizeSkillFileUrl(record.skillFileUrl, warnings); return { enabled, whenLabels: normalizeStringList(record.whenLabels, "whenLabels", MAX_LABELS, MAX_LABEL_CHARS, warnings), @@ -91,9 +93,23 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str requireViewports: normalizeStringList(record.requireViewports, "requireViewports", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), requireThemes: normalizeStringList(record.requireThemes, "requireThemes", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), ...(message !== undefined ? { message } : {}), + ...(skillFileUrl !== undefined ? { skillFileUrl } : {}), }; } +/** Validate a `skillFileUrl` override: same trust/validation level as `message` above (a trusted + * maintainer-authored config value, never fetched server-side -- it is only ever embedded as TEXT in a + * GitHub comment/close reason, so there is no SSRF surface here to guard against, unlike a URL the + * server would dereference). Malformed values are dropped with a warning, never silently coerced. */ +function normalizeSkillFileUrl(value: unknown, warnings: string[]): string | undefined { + if (value === undefined) return undefined; + if (typeof value !== "string" || value.trim().length === 0 || value.trim().length > MAX_SKILL_FILE_URL_CHARS) { + warnings.push(`settings.requireScreenshotTable.skillFileUrl must be a non-empty string no longer than ${MAX_SKILL_FILE_URL_CHARS} characters; ignoring it.`); + return undefined; + } + return value.trim(); +} + /** Linear-time markdown table separator check. The previous single-regex form nested unbounded `\\s*` inside a * repeated group and could catastrophically backtrack on attacker-controlled PR bodies; this splits on `|` and * validates each cell independently instead. */ @@ -255,6 +271,13 @@ export function buildScreenshotMatrixMessage(missing: ScreenshotMatrixPair[]): s ); } +/** Append a contributor skill-file link to an auto-generated rejection message (#4540 follow-up). A no-op + * when `skillFileUrl` is unset -- callers only reach this on the AUTO-GENERATED path (a `message` + * override already owns its entire text and is never passed through here). */ +function appendSkillLink(text: string, skillFileUrl: string | undefined): string { + return skillFileUrl ? `${text}\n\nSee ${skillFileUrl} for the exact format and examples.` : text; +} + /** 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 @@ -313,12 +336,12 @@ export function evaluateScreenshotTableGate(input: { if (matrixPairs.length > 0) { const missing = missingScreenshotMatrixPairs(input.prBody, matrixPairs); if (missing.length === 0) return NO_VIOLATION; - return { violated: true, reason: config.message ?? buildScreenshotMatrixMessage(missing) }; + return { violated: true, reason: config.message ?? appendSkillLink(buildScreenshotMatrixMessage(missing), config.skillFileUrl) }; } 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 }; + return { violated: true, reason: config.message ?? appendSkillLink(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, config.skillFileUrl) }; } diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index ca34682799..c63b93dd42 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -569,6 +569,7 @@ export function resolveEffectiveSettings( requireViewports: screenshotTableGateOverride.requireViewports ?? base.requireViewports, requireThemes: screenshotTableGateOverride.requireThemes ?? base.requireThemes, message: screenshotTableGateOverride.message ?? base.message, + skillFileUrl: screenshotTableGateOverride.skillFileUrl ?? base.skillFileUrl, }; } if (advisoryAiRoutingOverride !== undefined) { diff --git a/src/types.ts b/src/types.ts index 2e2106d690..6692e73cb8 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1117,6 +1117,11 @@ export type ScreenshotTableGateConfig = { whenLabels: string[]; whenPaths: string[]; action: ScreenshotTableGateAction; + /** Full replacement for the rejection reason -- when set, this is used verbatim and NEITHER the + * auto-generated matrix "still missing: ..." list NOR `skillFileUrl` appear (a maintainer who sets + * this owns the entire message). Leave unset to get the auto-generated, always-accurate message + * (naming the exact missing pairs in matrix mode) with `skillFileUrl` appended when configured -- + * that combination is usually what you want; only set `message` for total control over the wording. */ message?: string | undefined; /** Viewport x theme completeness matrix (#4535). Both empty (the default) ⇒ byte-identical to the original * presence-only check (some image-bearing table, anywhere). A non-empty `requireViewports` switches the @@ -1126,6 +1131,11 @@ export type ScreenshotTableGateConfig = { * empty) has no effect -- the viewport dimension is what turns matrix mode on. */ requireViewports: string[]; requireThemes: string[]; + /** A link to this repo's contributor skill file, appended to the AUTO-GENERATED rejection message + * (#4540 follow-up) so a closed contributor always gets pointed at the exact format/contract instead + * of just being told evidence is missing. Ignored when `message` is set (a full override already + * owns the entire text -- append the link into that string yourself if you want it there too). */ + skillFileUrl?: string | undefined; }; export type CommandAuthorizationRole = "maintainer" | "collaborator" | "pr_author" | "confirmed_miner"; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 0ba3417878..b1f38d5230 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -2948,6 +2948,29 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: ["Desktop"], requireThemes: ["Light"] }); }); + it("wires settings.screenshotTableGate.skillFileUrl into the manifest parser as a sparse override (#4540 follow-up)", () => { + const url = "https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md"; + const parsed = parseFocusManifest({ settings: { screenshotTableGate: { skillFileUrl: url } } }); + expect(parsed.settings.screenshotTableGate).toEqual({ skillFileUrl: url }); + }); + + it("omits skillFileUrl from the sparse override when the raw manifest doesn't name it (#4540 follow-up)", () => { + const parsed = parseFocusManifest({ settings: { screenshotTableGate: { enabled: true } } }); + expect(parsed.settings.screenshotTableGate).not.toHaveProperty("skillFileUrl"); + }); + + it("resolveEffectiveSettings merges skillFileUrl without clearing the DB layer's other fields (#4540 follow-up)", () => { + const db = { screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] } } as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { screenshotTableGate: { skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md" } } })); + expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [], skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md" }); + }); + + it("resolveEffectiveSettings keeps the DB layer's skillFileUrl when the manifest override omits it (#4540 follow-up)", () => { + const db = { screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [], skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md" } } as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { screenshotTableGate: { enabled: true } } })); + expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [], skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md" }); + }); + it("wires settings.advisoryAiRouting into the manifest parser as a sparse override (#4364)", () => { const parsed = parseFocusManifest({ settings: { advisoryAiRouting: { slop: true, summaries: true } } }); expect(parsed.settings.advisoryAiRouting).toEqual({ slop: true, summaries: true }); diff --git a/test/unit/repository-settings-screenshot-table-gate.test.ts b/test/unit/repository-settings-screenshot-table-gate.test.ts index 65c458f2cd..ea0637cf9d 100644 --- a/test/unit/repository-settings-screenshot-table-gate.test.ts +++ b/test/unit/repository-settings-screenshot-table-gate.test.ts @@ -45,6 +45,31 @@ describe("repository_settings: screenshotTableGate (#2006)", () => { }); }); + it("round-trips skillFileUrl alongside a custom message (#4540 follow-up)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { + repoFullName: "acme/skill-link", + screenshotTableGate: { + enabled: true, + whenLabels: [], + whenPaths: [], + action: "close", + requireViewports: [], + requireThemes: [], + skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md", + }, + }); + const settings = await getRepositorySettings(env, "acme/skill-link"); + expect(settings.screenshotTableGate?.skillFileUrl).toBe("https://github.com/acme/widget/blob/main/SKILL.md"); + }); + + it("omits `skillFileUrl` entirely when unset (never persists an empty string) (#4540 follow-up)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/no-skill-link", screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] } }); + const settings = await getRepositorySettings(env, "acme/no-skill-link"); + expect(settings.screenshotTableGate?.skillFileUrl).toBeUndefined(); + }); + 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", requireViewports: [], requireThemes: [] } }); diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index 559a62cf3d..05525f2265 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -279,6 +279,23 @@ describe("normalizeScreenshotTableGateConfig", () => { expect(result.requireViewports.length).toBe(12); expect(warnings.some((w) => w.includes("capped"))).toBe(true); }); + + it("accepts a valid skillFileUrl and trims it (#4540 follow-up)", () => { + const url = " https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md "; + expect(normalizeScreenshotTableGateConfig({ skillFileUrl: url }, []).skillFileUrl).toBe(url.trim()); + }); + + it("defaults skillFileUrl to undefined when unset", () => { + expect(normalizeScreenshotTableGateConfig({}, []).skillFileUrl).toBeUndefined(); + }); + + it("rejects a non-string/empty/overlong skillFileUrl with a warning, falling back to undefined", () => { + const warnings: string[] = []; + expect(normalizeScreenshotTableGateConfig({ skillFileUrl: " " }, warnings).skillFileUrl).toBeUndefined(); + expect(normalizeScreenshotTableGateConfig({ skillFileUrl: 42 }, []).skillFileUrl).toBeUndefined(); + expect(normalizeScreenshotTableGateConfig({ skillFileUrl: "x".repeat(301) }, []).skillFileUrl).toBeUndefined(); + expect(warnings.some((w) => w.includes("skillFileUrl"))).toBe(true); + }); }); describe("requiredScreenshotMatrixPairs (#4535)", () => { @@ -460,6 +477,34 @@ describe("evaluateScreenshotTableGate", () => { expect(result.reason).toBe("Please add screenshots, thanks!"); }); + describe("skillFileUrl (#4540 follow-up)", () => { + it("appends the skill-file link to the auto-generated presence-mode message", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toContain(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE); + expect(result.reason).toContain("https://github.com/acme/widget/blob/main/SKILL.md"); + }); + + it("does not append anything when skillFileUrl is unset (byte-identical to the plain default)", () => { + const result = evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: "no table", prLabels: [], changedFiles: [] }); + expect(result.reason).toBe(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE); + }); + + it("a custom message override wins entirely -- skillFileUrl is ignored, never appended", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, message: "Custom text", skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toBe("Custom text"); + }); + }); + 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); @@ -578,6 +623,28 @@ describe("evaluateScreenshotTableGate", () => { expect(result.reason).toBe("Custom matrix rejection text"); }); + it("appends the skill-file link to the auto-generated matrix message, keeping the specific missing-pairs list (#4540 follow-up)", () => { + const result = evaluateScreenshotTableGate({ + config: matrixConfig({ skillFileUrl: "https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toContain("Still missing:"); + expect(result.reason).toContain("Desktop · Light"); + expect(result.reason).toContain("https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md"); + }); + + it("a message override in matrix mode also ignores skillFileUrl entirely", () => { + const result = evaluateScreenshotTableGate({ + config: matrixConfig({ message: "Custom matrix rejection text", skillFileUrl: "https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toBe("Custom matrix rejection text"); + }); + it("botCaptureSatisfied short-circuits matrix mode too, even with zero rows", () => { const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: "no table at all", prLabels: [], changedFiles: [], botCaptureSatisfied: true }); expect(result).toEqual({ violated: false, reason: null }); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index c845e515fc..695c290db3 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -278,6 +278,23 @@ describe("normalizeScreenshotTableGateConfig", () => { expect(result.requireViewports.length).toBe(12); expect(warnings.some((w) => w.includes("capped"))).toBe(true); }); + + it("accepts a valid skillFileUrl and trims it (#4540 follow-up)", () => { + const url = " https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md "; + expect(normalizeScreenshotTableGateConfig({ skillFileUrl: url }, []).skillFileUrl).toBe(url.trim()); + }); + + it("defaults skillFileUrl to undefined when unset", () => { + expect(normalizeScreenshotTableGateConfig({}, []).skillFileUrl).toBeUndefined(); + }); + + it("rejects a non-string/empty/overlong skillFileUrl with a warning, falling back to undefined", () => { + const warnings: string[] = []; + expect(normalizeScreenshotTableGateConfig({ skillFileUrl: " " }, warnings).skillFileUrl).toBeUndefined(); + expect(normalizeScreenshotTableGateConfig({ skillFileUrl: 42 }, []).skillFileUrl).toBeUndefined(); + expect(normalizeScreenshotTableGateConfig({ skillFileUrl: "x".repeat(301) }, []).skillFileUrl).toBeUndefined(); + expect(warnings.some((w) => w.includes("skillFileUrl"))).toBe(true); + }); }); describe("requiredScreenshotMatrixPairs (#4535)", () => { @@ -459,6 +476,34 @@ describe("evaluateScreenshotTableGate", () => { expect(result.reason).toBe("Please add screenshots, thanks!"); }); + describe("skillFileUrl (#4540 follow-up)", () => { + it("appends the skill-file link to the auto-generated presence-mode message", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toContain(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE); + expect(result.reason).toContain("https://github.com/acme/widget/blob/main/SKILL.md"); + }); + + it("does not append anything when skillFileUrl is unset (byte-identical to the plain default)", () => { + const result = evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: "no table", prLabels: [], changedFiles: [] }); + expect(result.reason).toBe(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE); + }); + + it("a custom message override wins entirely -- skillFileUrl is ignored, never appended", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, message: "Custom text", skillFileUrl: "https://github.com/acme/widget/blob/main/SKILL.md" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toBe("Custom text"); + }); + }); + 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); @@ -577,6 +622,28 @@ describe("evaluateScreenshotTableGate", () => { expect(result.reason).toBe("Custom matrix rejection text"); }); + it("appends the skill-file link to the auto-generated matrix message, keeping the specific missing-pairs list (#4540 follow-up)", () => { + const result = evaluateScreenshotTableGate({ + config: matrixConfig({ skillFileUrl: "https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toContain("Still missing:"); + expect(result.reason).toContain("Desktop · Light"); + expect(result.reason).toContain("https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md"); + }); + + it("a message override in matrix mode also ignores skillFileUrl entirely", () => { + const result = evaluateScreenshotTableGate({ + config: matrixConfig({ message: "Custom matrix rejection text", skillFileUrl: "https://github.com/JSONbored/metagraphed/blob/main/.claude/skills/metagraphed/SKILL.md" }), + prBody: "no table", + prLabels: [], + changedFiles: [], + }); + expect(result.reason).toBe("Custom matrix rejection text"); + }); + it("botCaptureSatisfied short-circuits matrix mode too, even with zero rows", () => { const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: "no table at all", prLabels: [], changedFiles: [], botCaptureSatisfied: true }); expect(result).toEqual({ violated: false, reason: null });