diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 7b5d331e01..02e5762afe 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -777,6 +777,28 @@ settings: # claude_effort: null # Overrides CLAUDE_AI_EFFORT for this repo. String or null. Default (env unset): medium. # codex_model: null # Overrides CODEX_AI_MODEL for this repo. String or null. # codex_effort: null # Overrides CODEX_AI_EFFORT for this repo. String or null. Default (env unset): medium. +# # Per-repo before/after screenshot-capture config (#3609 preview / #3610 routes). Only takes effect when +# # the operator has ALSO enabled GITTENSORY_REVIEW_SCREENSHOTS + this repo's cutover allowlist -- this +# # config narrows/redirects that feature, it never turns it on by itself. All-null/empty/default ⇒ +# # byte-identical to today (GitHub-native preview discovery, automatic file-to-route inference). +# visual: +# preview: +# # The repo's "after" preview URL, with {number}/{head_sha}/{head_sha_short} placeholders substituted +# # at capture time. ALWAYS wins over GitHub-native preview discovery (Deployments API / commit checks / +# # cloudflare-bot PR comment) when set -- the only option for a provider (e.g. Cloudflare Workers +# # Builds' non-production branch builds) that never surfaces a GitHub-visible deployment at all. Must +# # resolve to a valid HTTPS URL targeting a public host. String or null. Default: null (discovery unchanged). +# url_template: "https://pr-{number}.myapp.workers.dev" +# routes: +# # An explicit, always-screenshotted route list. When non-empty, REPLACES automatic file-to-route +# # inference entirely -- for a repo whose routing convention isn't gittensory-ui's TanStack file-based +# # one. Empty/default ⇒ automatic inference (falling back to "/" when nothing matches). +# paths: +# - "/pricing" +# - "/docs" +# # Overrides the built-in cap (2) on how many routes get screenshotted per PR, whether they come from +# # `paths` above or automatic inference. Positive integer or null. Default: null (built-in default). +# max_routes: 3 # # Maintainer overrides for the public review-panel CONTENT (not what gittensory measures). The # # Gittensor attribution + register link is always appended to the footer regardless; maintainer text # # failing the public-safe filter is dropped, never published. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 05dde72aef..d2ca9c7291 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -366,11 +366,13 @@ import { resolveReviewPathInstructions, resolveReviewPreMergeChecks, resolveReviewPromptOverrides, + resolveReviewVisualConfig, type FocusManifestFinding, type FocusManifest, type ReviewPathInstruction, type ReviewProfile, type SelfHostAiModelConfig, + type VisualConfig, } from "../signals/focus-manifest"; import { decideReviewEligibility } from "../review/review-eligibility"; import { @@ -6367,6 +6369,15 @@ export async function resolveReviewManifestForAiReview( return cachedManifest ?? (await loadRepoFocusManifest(env, repoFullName).catch(() => null)); } +/** Resolve `review.visual` (#3609 preview.url_template / #3610 routes) for the before/after capture pipeline — + * a deterministic, non-AI feature, so it's resolved independently rather than reusing the AI-review manifest + * cache above. Fail-safe: a manifest-load error yields the empty defaults (byte-identical to no config + * configured), matching every other `resolveReview*` accessor's null-manifest behavior. */ +export async function resolveVisualCaptureConfig(env: Env, repoFullName: string): Promise { + const manifest = await loadRepoFocusManifest(env, repoFullName).catch(() => null); + return resolveReviewVisualConfig(manifest); +} + async function resolveReviewEnrichmentGithubToken( env: Env, repoFullName: string, @@ -9225,6 +9236,10 @@ async function maybePublishPrPublicSurface( if (screenshotsAllowed(env, repoFullName) && visualFiles.length > 0) { try { const token = await createInstallationToken(env, installationId); + // review.visual (#3609 / #3610): an explicit per-repo preview-URL template / route list. Absent config + // (the default for every repo today) ⇒ EMPTY_VISUAL_CONFIG ⇒ buildCapture's discovery/inference + // behavior is byte-identical to pre-#3609. + const reviewVisualConfig = await resolveVisualCaptureConfig(env, repoFullName); const capture = await buildCapture( env, token, @@ -9237,6 +9252,7 @@ async function maybePublishPrPublicSurface( }, visualFiles, githubRateLimitAdmissionKeyForInstallation(installationId), + reviewVisualConfig, ); beforeAfter = capture.routes; // Visual self-poll: the FIRST capture returns a "loading" placeholder for the AFTER shot when the diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 92485f70c4..3c965f1800 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -61,6 +61,26 @@ function joinUrl(base: string, path: string): string { return `${base.replace(/\/+$/, "")}${path.startsWith("/") ? path : `/${path}`}`; } +/** Per-repo `review.visual.preview` config, as resolved by the caller from the manifest (#3609). */ +export type VisualPreviewInput = { urlTemplate?: string | null | undefined }; + +/** + * Substitute `{number}`/`{head_sha}`/`{head_sha_short}` in a `review.visual.preview.url_template` (#3609). + * Pure string substitution — `number` and `headSha` are GitHub-controlled facts about the PR, never + * attacker-supplied free text, so this carries no injection risk regardless of the template's own content + * (which is maintainer-authored and already validated at parse time — see parseVisualUrlTemplate). A missing + * headSha leaves the sha placeholders empty rather than throwing; the resolved URL still goes through the + * SAME isSafeHttpUrl check every other capture URL does (in captureShot), so an unresolved/malformed result + * degrades to a null render, never a crash. + */ +export function resolvePreviewUrlTemplate(template: string, vars: { number: number; headSha?: string | undefined }): string { + const headSha = vars.headSha ?? ""; + return template + .split("{number}").join(String(vars.number)) + .split("{head_sha_short}").join(headSha.slice(0, 7)) + .split("{head_sha}").join(headSha); +} + /** * Map changed UI files to navigable routes, honoring TanStack Router's file conventions (flat routing uses * `.` as the path separator; folders use `/`): @@ -69,14 +89,30 @@ function joinUrl(base: string, path: string): string { * posts.$id.tsx -> "/" (dynamic param has no concrete value to render) * Anything we can't resolve to a concrete path falls back to "/" so we never screenshot a 404. */ -export function mapFilesToRoutes(files: string[], pattern: RegExp = DEFAULT_ROUTE_FILE): string[] { +export function mapFilesToRoutes(files: string[], pattern: RegExp = DEFAULT_ROUTE_FILE, maxRoutes: number = MAX_ROUTES): string[] { const routes = new Set(); for (const file of files) { const match = file.match(pattern); if (match) routes.add(routeForFile(match[1] as string)); } if (routes.size === 0) for (const route of DEFAULT_ROUTES) routes.add(route); - return [...routes].slice(0, MAX_ROUTES); + return [...routes].slice(0, maxRoutes); +} + +/** Per-repo `review.visual.routes` config, as resolved by the caller from the manifest (#3610). */ +export type VisualRoutesInput = { paths?: readonly string[] | null | undefined; maxRoutes?: number | null | undefined }; + +/** + * Resolve which routes to screenshot for this PR: an explicit, always-screenshotted `paths` list from + * `review.visual.routes` REPLACES automatic file-to-route inference entirely when non-empty (simpler and + * more robust for a repo whose routing convention isn't gittensory-ui's TanStack file-based one); absent/ + * empty config falls through to `mapFilesToRoutes` unchanged, so this is byte-identical to today by default. + * `maxRoutes` applies to either path — an explicit list is capped too, not just inferred routes. + */ +export function resolveVisualRoutes(files: string[], config?: VisualRoutesInput | null): string[] { + const maxRoutes = config?.maxRoutes && config.maxRoutes > 0 ? config.maxRoutes : MAX_ROUTES; + if (config?.paths && config.paths.length > 0) return [...config.paths].slice(0, maxRoutes); + return mapFilesToRoutes(files, DEFAULT_ROUTE_FILE, maxRoutes); } /** Resolve one TanStack route-file name (extension already stripped) to a navigable path. */ @@ -133,42 +169,55 @@ async function capturePage( return { url: onDemand }; } +/** Per-repo `review.visual` config, as resolved by the caller from the manifest (#3609 / #3610). Absent ⇒ + * byte-identical to today (GitHub-native discovery, automatic route inference, built-in route cap). */ +export type VisualCaptureConfig = { preview?: VisualPreviewInput | null | undefined; routes?: VisualRoutesInput | null | undefined }; + /** * Build the before/after capture for a PR: resolve the preview URL, derive routes from the changed UI files, * render desktop + mobile before/after for each route, and return the route URL set (for the visual-preview * collapsible). Fully fail-safe — a missing preview / failed render degrades to placeholders or dashes; this * NEVER throws (the caller also wraps it in try/catch so a capture failure can't sink a review). */ -export async function buildCapture(env: Env, token: string, target: CaptureTarget, visualFiles: string[], rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined): Promise { +export async function buildCapture(env: Env, token: string, target: CaptureTarget, visualFiles: string[], rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined, visualConfig?: VisualCaptureConfig | null | undefined): Promise { const repo = parseRepo(target.repoFullName); const apiVersion = "2022-11-28"; // before = production (PUBLIC_SITE_ORIGIN, e.g. https://gittensory.aethereal.dev). const prodBase = env.PUBLIC_SITE_ORIGIN ?? ""; - // after = the PR's preview deploy. Prefer the URL carried on the target (a deployment_status webhook set - // it — no extra API call); otherwise look it up from Deployments, then commit checks, then the - // cloudflare-bot PR comment. The lookups also tell us when the latest deploy FAILED (vs is still building) - // so we can show a terminal "deploy failed" card instead of a spinner. - let previewBase = typeof target.previewUrl === "string" ? target.previewUrl : ""; + // after = the PR's preview deploy. An explicit review.visual.preview.url_template (#3609) ALWAYS wins — + // a maintainer-configured template is a stronger signal than inference, and is the only option for a + // provider (e.g. Cloudflare Workers Builds' non-production branch builds) that never surfaces a + // GitHub-visible deployment at all. Otherwise, prefer the URL carried on the target (a deployment_status + // webhook set it — no extra API call); otherwise look it up from Deployments, then commit checks, then + // the cloudflare-bot PR comment. The lookups also tell us when the latest deploy FAILED (vs is still + // building) so we can show a terminal "deploy failed" card instead of a spinner. + let previewBase = ""; let previewFailed = target.previewFailed === true; let previewPending = false; - if (!previewBase && !previewFailed) { - try { - const status = await getLatestDeploymentStatus({ token, repo, sha: target.headSha, ref: target.headRef, apiVersion, rateLimitAdmissionKey }); - previewBase = status.url ?? ""; - previewFailed = status.failed; - } catch { - previewBase = ""; - } - if (!previewBase && !previewFailed && target.previewFromChecks && target.headSha) { - previewBase = (await findPreviewUrlFromChecks({ token, repo, sha: target.headSha, apiVersion, rateLimitAdmissionKey })) ?? ""; - if (!previewBase && target.prNumber) { - previewBase = (await findPreviewUrlFromPrComments({ token, repo, prNumber: target.prNumber, apiVersion, rateLimitAdmissionKey })) ?? ""; + const urlTemplate = visualConfig?.preview?.urlTemplate; + if (urlTemplate) { + previewBase = resolvePreviewUrlTemplate(urlTemplate, { number: target.prNumber, headSha: target.headSha }); + } else { + previewBase = typeof target.previewUrl === "string" ? target.previewUrl : ""; + if (!previewBase && !previewFailed) { + try { + const status = await getLatestDeploymentStatus({ token, repo, sha: target.headSha, ref: target.headRef, apiVersion, rateLimitAdmissionKey }); + previewBase = status.url ?? ""; + previewFailed = status.failed; + } catch { + previewBase = ""; } - if (!previewBase && target.headSha) { - const buildState = await getPreviewBuildState({ token, repo, sha: target.headSha, apiVersion, rateLimitAdmissionKey }); - if (buildState === "failed") previewFailed = true; - else if (buildState === "building" || buildState === "succeeded") previewPending = true; + if (!previewBase && !previewFailed && target.previewFromChecks && target.headSha) { + previewBase = (await findPreviewUrlFromChecks({ token, repo, sha: target.headSha, apiVersion, rateLimitAdmissionKey })) ?? ""; + if (!previewBase && target.prNumber) { + previewBase = (await findPreviewUrlFromPrComments({ token, repo, prNumber: target.prNumber, apiVersion, rateLimitAdmissionKey })) ?? ""; + } + if (!previewBase && target.headSha) { + const buildState = await getPreviewBuildState({ token, repo, sha: target.headSha, apiVersion, rateLimitAdmissionKey }); + if (buildState === "failed") previewFailed = true; + else if (buildState === "building" || buildState === "succeeded") previewPending = true; + } } } } @@ -180,7 +229,7 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge const failedPlaceholder = shotBase ? `${shotBase}/${NAMESPACE}/shot?placeholder=failed` : undefined; const afterPlaceholder = previewFailed ? failedPlaceholder : loadingPlaceholder; - const routes = mapFilesToRoutes(visualFiles); + const routes = resolveVisualRoutes(visualFiles, visualConfig?.routes); const captureRoutes: CaptureRoute[] = []; for (const path of routes) { const beforePage = prodBase ? joinUrl(prodBase, path) : ""; diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index bcdef38e95..000eb84b5a 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -12,6 +12,7 @@ import { normalizeModerationLabel, normalizeModerationRules } from "../settings/ import { REES_ANALYZER_NAME_SET, type ReesAnalyzerName } from "../review/enrichment-analyzer-names"; import { hasUnsafeWildcardCount } from "./change-guardrail"; import { PUBLIC_LOCAL_PATH_INLINE } from "./redaction"; +import { isSafeHttpUrl } from "../review/content-lane/safe-url"; export type FocusManifestSource = "repo_file" | "api_record" | "none"; export type FocusManifestLinkedIssuePolicy = "required" | "preferred" | "optional"; @@ -371,6 +372,12 @@ export type FocusManifestReviewConfig = { * CLAUDE_AI_MODEL/CLAUDE_AI_EFFORT/CODEX_AI_MODEL/CODEX_AI_EFFORT env vars apply unchanged (byte-identical). * (#selfhost-ai-model-override) */ aiModel: SelfHostAiModelConfig; + /** `review.visual`: per-repo before/after screenshot-capture config (#3609 preview / #3610 routes). + * All-empty (default, absent) ⇒ byte-identical to today (GitHub-native preview discovery, automatic + * file-to-route inference, built-in route cap). Only takes effect when the operator has also enabled + * GITTENSORY_REVIEW_SCREENSHOTS + the repo cutover allowlist — this config narrows/redirects that + * feature, it never turns it on by itself. */ + visual: VisualConfig; }; /** One `review.labeling_rules[]` entry: a non-reserved `label` plus the deterministic `when` criteria that must ALL @@ -428,6 +435,45 @@ export const EMPTY_SELF_HOST_AI_MODEL_CONFIG: SelfHostAiModelConfig = { codexEffort: null, }; +/** Per-repo before/after screenshot-capture config under `review.visual` (#3609 / #3610). Generic by design — + * every self-hoster wires their OWN repo's preview-deploy setup and route shape with config, not code. */ +export type VisualConfig = { + preview: VisualPreviewConfig; + routes: VisualRoutesConfig; +}; + +export type VisualPreviewConfig = { + /** `review.visual.preview.url_template`: the repo's "after" preview URL, with `{number}` (PR number), + * `{head_sha}` (full commit SHA), and `{head_sha_short}` (first 7 chars) placeholders substituted at + * capture time — e.g. `https://pr-{number}.myapp.workers.dev`. ALWAYS wins over GitHub-native preview + * discovery (the Deployments API / commit checks / cloudflare-bot PR comment) when set — an explicit, + * maintainer-configured template is a stronger signal than inference, and is the only option for a + * provider (e.g. Cloudflare Workers Builds' non-production branch builds) that doesn't surface a + * GitHub-visible deployment at all. null (default) ⇒ byte-identical to today (discovery unchanged). + * Validated at parse time against the same SSRF guard the renderer itself applies (isSafeHttpUrl) with + * placeholders substituted for a dummy value, so a malformed template warns at config-read time instead + * of only failing silently at render time — this is redundant with (not a replacement for) the + * renderer's own unconditional isSafeHttpUrl check on every resolved URL, regardless of source. */ + urlTemplate: string | null; +}; + +export type VisualRoutesConfig = { + /** `review.visual.routes.paths`: an explicit, always-screenshotted route list. When non-empty, this + * REPLACES automatic file-to-route inference entirely — for repos whose routing convention isn't + * gittensory-ui's TanStack file-based one, an explicit list is simpler and more robust than trying to + * infer one. Empty (default) ⇒ automatic inference (falling back to "/" when nothing matches). */ + paths: string[]; + /** `review.visual.routes.max_routes`: overrides the built-in cap (2) on how many routes get screenshotted + * per PR. null (default) ⇒ built-in default. Applies whether routes come from `paths` above or from + * automatic inference. */ + maxRoutes: number | null; +}; + +export const EMPTY_VISUAL_CONFIG: VisualConfig = { + preview: { urlTemplate: null }, + routes: { paths: [], maxRoutes: null }, +}; + /** One `review.path_instructions[]` entry: a manifest path glob + the public-safe instructions to apply when a * changed file matches it. */ export type ReviewPathInstruction = { path: string; instructions: string }; @@ -587,7 +633,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG } }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -617,7 +663,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG } }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1548,7 +1594,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG } }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -1596,6 +1642,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const autoReview = parseAutoReviewConfig(r.auto_review, warnings); const labelingRules = parseReviewLabelingRules(r.labeling_rules, warnings); const aiModel = parseSelfHostAiModelConfig(r.ai_model, warnings); + const visual = parseVisualConfig(r.visual, warnings); return { present: footerText !== null || @@ -1615,6 +1662,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo autoReviewPresent(autoReview) || labelingRules.length > 0 || selfHostAiModelPresent(aiModel) || + visualConfigPresent(visual) || Object.keys(fields).length > 0 || Object.keys(enrichmentAnalyzers).length > 0, footerText, @@ -1622,6 +1670,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo fields, autoReview, aiModel, + visual, enrichmentAnalyzers, profile, tone, @@ -1746,6 +1795,64 @@ function parseSelfHostAiModelConfig(value: JsonValue | undefined, warnings: stri }; } +function visualConfigPresent(config: VisualConfig): boolean { + return config.preview.urlTemplate !== null || config.routes.paths.length > 0 || config.routes.maxRoutes !== null; +} + +// `{number}`/`{head_sha}`/`{head_sha_short}` are GitHub-controlled facts about the PR (never attacker-supplied +// free text), so substitution itself carries no injection risk. The dummy values here exist only to make the +// TEMPLATE STRING (which a maintainer authored, and could still typo) validate as a well-formed HTTPS URL +// before it's ever used — see parseVisualUrlTemplate below. +const VISUAL_URL_TEMPLATE_DUMMY_VARS: Record = { + "{number}": "1", + "{head_sha_short}": "0000000", + "{head_sha}": "0000000000000000000000000000000000000000", +}; + +/** Parse `review.visual.preview.url_template` — validated at CONFIG-READ time against the exact same SSRF + * guard (`isSafeHttpUrl`) the renderer itself unconditionally applies to every URL it navigates to, + * regardless of source (`src/review/visual/shot.ts`). This is deliberately redundant with that runtime + * check, not a replacement for it — it exists so a maintainer sees a warning immediately for a malformed + * template (e.g. a typo'd scheme, or an accidental internal host) instead of only discovering it later as + * a silently-blank "after" cell. Placeholders are substituted with dummy values before validation since the + * raw template (e.g. `https://pr-{number}.example.com`) is not itself a parseable URL. */ +function parseVisualUrlTemplate(value: JsonValue | undefined, warnings: string[]): string | null { + const template = parsePublicSafeText(value, "review.visual.preview.url_template", warnings); + if (template === null) return null; + let probe = template; + for (const [placeholder, dummy] of Object.entries(VISUAL_URL_TEMPLATE_DUMMY_VARS)) probe = probe.split(placeholder).join(dummy); + if (!isSafeHttpUrl(probe)) { + warnings.push(`Manifest "review.visual.preview.url_template" must be a valid HTTPS URL (with {number}/{head_sha}/{head_sha_short} placeholders substituted) targeting a public host; ignoring it.`); + return null; + } + return template; +} + +/** Parse `review.visual` — per-repo before/after screenshot-capture config (#3609 preview / #3610 routes). */ +function parseVisualConfig(value: JsonValue | undefined, warnings: string[]): VisualConfig { + if (value === undefined || value === null) return { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null } }; + if (typeof value !== "object" || Array.isArray(value)) { + warnings.push(`Manifest field "review.visual" must be a mapping; ignoring it.`); + return { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null } }; + } + const record = value as Record; + + const previewRecord = record.preview !== null && typeof record.preview === "object" && !Array.isArray(record.preview) ? (record.preview as Record) : undefined; + if (record.preview !== undefined && record.preview !== null && previewRecord === undefined) { + warnings.push(`Manifest "review.visual.preview" must be a mapping; ignoring it.`); + } + const urlTemplate = previewRecord ? parseVisualUrlTemplate(previewRecord.url_template, warnings) : null; + + const routesRecord = record.routes !== null && typeof record.routes === "object" && !Array.isArray(record.routes) ? (record.routes as Record) : undefined; + if (record.routes !== undefined && record.routes !== null && routesRecord === undefined) { + warnings.push(`Manifest "review.visual.routes" must be a mapping; ignoring it.`); + } + const paths = routesRecord ? parseManifestGlobList(routesRecord.paths, "review.visual.routes.paths", warnings) : []; + const maxRoutes = routesRecord ? normalizeOptionalPositiveInteger(routesRecord.max_routes, "review.visual.routes.max_routes", warnings) : null; + + return { preview: { urlTemplate }, routes: { paths, maxRoutes } }; +} + function parseAutoReviewTitleKeywords(value: JsonValue | undefined, warnings: string[]): string[] { if (value === undefined || value === null) return []; if (!Array.isArray(value)) { @@ -1995,6 +2102,17 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.aiModel.codexEffort !== null) aiModel.codex_effort = review.aiModel.codexEffort; out.ai_model = aiModel; } + if (visualConfigPresent(review.visual)) { + const visual: Record = {}; + if (review.visual.preview.urlTemplate !== null) visual.preview = { url_template: review.visual.preview.urlTemplate }; + if (review.visual.routes.paths.length > 0 || review.visual.routes.maxRoutes !== null) { + const routes: Record = {}; + if (review.visual.routes.paths.length > 0) routes.paths = [...review.visual.routes.paths]; + if (review.visual.routes.maxRoutes !== null) routes.max_routes = review.visual.routes.maxRoutes; + visual.routes = routes; + } + out.visual = visual; + } return out; } @@ -2126,6 +2244,14 @@ export function resolveReviewSelfHostAiModel(manifest: FocusManifest | null): Se return manifest?.review.aiModel ?? { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }; } +/** Resolve `review.visual` from a possibly-null manifest (null = load failure ⇒ no per-repo override). The + * capture pipeline then falls back to GitHub-native preview discovery + automatic route inference, same as + * an explicit all-empty config — a manifest read failure never blocks a review or a capture attempt, it + * just loses the per-repo override for that one pass. (#3609 / #3610) */ +export function resolveReviewVisualConfig(manifest: FocusManifest | null): VisualConfig { + return manifest?.review.visual ?? { ...EMPTY_VISUAL_CONFIG }; +} + export function resolveEnrichmentAnalyzerToggles(manifest: FocusManifest | null): Partial> { return manifest?.review.enrichmentAnalyzers ?? {}; } diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index c08468f2c2..f65c146a04 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -26,7 +26,9 @@ import { composeManifestReviewInstructions, EMPTY_AUTO_REVIEW_CONFIG, EMPTY_SELF_HOST_AI_MODEL_CONFIG, + EMPTY_VISUAL_CONFIG, resolveReviewSelfHostAiModel, + resolveReviewVisualConfig, repoDocGenerationConfigToJson, reviewConfigToJson, settingsOverrideToJson, @@ -358,6 +360,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { autoReview: "auto_review:", labelingRules: "labeling_rules:", aiModel: "ai_model:", + visual: "visual:", } satisfies Record, string>; it.each(Object.entries(REVIEW_FIELD_TOKENS))("documents review.%s", (_field, token) => { @@ -758,7 +761,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG } }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, @@ -3233,6 +3236,111 @@ describe("review.ai_model (#selfhost-ai-model-override)", () => { }); }); +describe("review.visual (#3609 preview.url_template / #3610 routes)", () => { + it("parses preview.url_template + routes, marks present, and round-trips", () => { + const m = parseFocusManifest({ + review: { + visual: { + preview: { url_template: "https://pr-{number}.preview.example.com" }, + routes: { paths: ["/pricing", "/docs"], max_routes: 3 }, + }, + }, + }); + expect(m.review.visual).toEqual({ + preview: { urlTemplate: "https://pr-{number}.preview.example.com" }, + routes: { paths: ["/pricing", "/docs"], maxRoutes: 3 }, + }); + expect(m.review.present).toBe(true); + expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.visual).toEqual(m.review.visual); + }); + + it("absent/null visual yields the empty defaults and does not mark review present on its own", () => { + expect(parseFocusManifest({}).review.visual).toEqual({ ...EMPTY_VISUAL_CONFIG }); + expect(parseFocusManifest({ review: { visual: null } }).review.visual).toEqual({ ...EMPTY_VISUAL_CONFIG }); + expect(parseFocusManifest({}).review.present).toBe(false); + }); + + it("ignores a non-mapping review.visual with a warning", () => { + const bad = parseFocusManifest({ review: { visual: "on" } }); + expect(bad.review.visual).toEqual({ ...EMPTY_VISUAL_CONFIG }); + expect(bad.warnings.some((w) => /review\.visual.*must be a mapping/.test(w))).toBe(true); + }); + + it("ignores a non-mapping review.visual array with a warning", () => { + const bad = parseFocusManifest({ review: { visual: ["preview"] } }); + expect(bad.review.visual).toEqual({ ...EMPTY_VISUAL_CONFIG }); + expect(bad.warnings.some((w) => /review\.visual.*must be a mapping/.test(w))).toBe(true); + }); + + it("ignores a non-mapping review.visual.preview with a warning but keeps routes", () => { + const bad = parseFocusManifest({ review: { visual: { preview: "https://pr.example.com", routes: { paths: ["/app"] } } } }); + expect(bad.review.visual.preview).toEqual({ urlTemplate: null }); + expect(bad.review.visual.routes.paths).toEqual(["/app"]); + expect(bad.warnings.some((w) => /review\.visual\.preview.*must be a mapping/.test(w))).toBe(true); + }); + + it("ignores a non-mapping review.visual.routes with a warning but keeps preview", () => { + const bad = parseFocusManifest({ review: { visual: { preview: { url_template: "https://pr.example.com" }, routes: "everything" } } }); + expect(bad.review.visual.routes).toEqual({ paths: [], maxRoutes: null }); + expect(bad.review.visual.preview.urlTemplate).toBe("https://pr.example.com"); + expect(bad.warnings.some((w) => /review\.visual\.routes.*must be a mapping/.test(w))).toBe(true); + }); + + it("rejects a non-HTTPS url_template with a warning", () => { + const bad = parseFocusManifest({ review: { visual: { preview: { url_template: "http://pr-{number}.example.com" } } } }); + expect(bad.review.visual.preview.urlTemplate).toBeNull(); + expect(bad.warnings.some((w) => /review\.visual\.preview\.url_template.*valid HTTPS URL/.test(w))).toBe(true); + }); + + it("rejects a url_template resolving to a private/internal host with a warning", () => { + const bad = parseFocusManifest({ review: { visual: { preview: { url_template: "https://pr-{number}.internal" } } } }); + expect(bad.review.visual.preview.urlTemplate).toBeNull(); + expect(bad.warnings.some((w) => /review\.visual\.preview\.url_template.*valid HTTPS URL/.test(w))).toBe(true); + }); + + it("rejects a malformed url_template (unparseable even with placeholders substituted) with a warning", () => { + const bad = parseFocusManifest({ review: { visual: { preview: { url_template: "not-a-url-at-all" } } } }); + expect(bad.review.visual.preview.urlTemplate).toBeNull(); + expect(bad.warnings.some((w) => /review\.visual\.preview\.url_template.*valid HTTPS URL/.test(w))).toBe(true); + }); + + it("accepts a url_template with no placeholders at all (a fixed preview host)", () => { + const m = parseFocusManifest({ review: { visual: { preview: { url_template: "https://staging.example.com" } } } }); + expect(m.review.visual.preview.urlTemplate).toBe("https://staging.example.com"); + }); + + it("rejects max_routes of zero or a negative number with a warning", () => { + const zero = parseFocusManifest({ review: { visual: { routes: { max_routes: 0 } } } }); + expect(zero.review.visual.routes.maxRoutes).toBeNull(); + expect(zero.warnings.some((w) => /review\.visual\.routes\.max_routes.*positive whole number/.test(w))).toBe(true); + const negative = parseFocusManifest({ review: { visual: { routes: { max_routes: -1 } } } }); + expect(negative.review.visual.routes.maxRoutes).toBeNull(); + }); + + it("marks present via routes.paths alone (preview + max_routes both empty)", () => { + const m = parseFocusManifest({ review: { visual: { routes: { paths: ["/app"] } } } }); + expect(m.review.present).toBe(true); + expect(reviewConfigToJson(m.review)).toEqual({ visual: { routes: { paths: ["/app"] } } }); + }); + + it("marks present via routes.max_routes alone (preview + paths both empty)", () => { + const m = parseFocusManifest({ review: { visual: { routes: { max_routes: 5 } } } }); + expect(m.review.present).toBe(true); + expect(reviewConfigToJson(m.review)).toEqual({ visual: { routes: { max_routes: 5 } } }); + }); + + it("round-trips a preview-only config through reviewConfigToJson without an empty routes block", () => { + const m = parseFocusManifest({ review: { visual: { preview: { url_template: "https://pr-{number}.example.com" } } } }); + expect(reviewConfigToJson(m.review)).toEqual({ visual: { preview: { url_template: "https://pr-{number}.example.com" } } }); + }); + + it("resolveReviewVisualConfig: null manifest yields empty defaults; a set manifest passes through", () => { + expect(resolveReviewVisualConfig(null)).toEqual({ ...EMPTY_VISUAL_CONFIG }); + const manifest = parseFocusManifest({ review: { visual: { routes: { paths: ["/app"] } } } }); + expect(resolveReviewVisualConfig(manifest)).toEqual({ preview: { urlTemplate: null }, routes: { paths: ["/app"], maxRoutes: null } }); + }); +}); + describe("review.pre_merge_checks (#review-pre-merge-checks)", () => { it("parses checks (name + assertions + when_paths + enforce), marks present, and round-trips", () => { const m = parseFocusManifest({ diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index dc144d48b8..36d54ca3dc 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -15506,6 +15506,155 @@ describe("queue processors", () => { } }); + // #3609/#3610: same fixture as the unified-comment test above (screenshotsAllowed needs both the global flag + // AND the repo cutover allowlist — createTestEnv already defaults GITTENSORY_REVIEW_REPOS to include this + // repo), but the changed file is WEB-VISIBLE (isVisualPath) so the capture pipeline actually fires, proving + // resolveVisualCaptureConfig / buildCapture's config-threading (review.visual) is reached end to end from the + // real webhook path, not just from the pure-function unit tests in visual-capture.test.ts. + it("threads review.visual config into the capture pipeline and renders a Visual preview section (#3609 / #3610)", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", + GITTENSORY_REVIEW_SCREENSHOTS: "true", + }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicAudienceMode: "gittensor_only", + publicSignalLevel: "standard", + publicSurface: "comment_and_label", + autoLabelEnabled: false, + checkRunMode: "off", + checkRunDetailLevel: "minimal", + gateCheckMode: "enabled", + backfillEnabled: true, + privateTrustEnabled: true, + autonomy: { update_branch: "auto" }, + }); + let postedBody = ""; + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + 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([ + { + uid: 7, + githubUsername: "oktofeesh1", + githubId: "123", + totalPrs: 4, + totalMergedPrs: 3, + totalOpenPrs: 1, + totalClosedPrs: 0, + totalOpenIssues: 0, + totalClosedIssues: 0, + totalSolvedIssues: 0, + totalValidSolvedIssues: 0, + isEligible: true, + credibility: 1, + eligibleRepoCount: 1, + hotkey: "must-not-leak", + }, + ]); + } + if (url === "https://api.gittensor.io/miners/123") { + return Response.json({ + repositories: [ + { + repositoryFullName: "JSONbored/gittensory", + totalPrs: "4", + totalMergedPrs: "3", + totalOpenPrs: "1", + totalClosedPrs: "0", + totalOpenIssues: "0", + totalClosedIssues: "0", + isEligible: true, + credibility: "1.000000", + }, + ], + }); + } + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1", public_repos: 2, followers: 1 }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([{ language: "TypeScript" }]); + if (url.includes("/access_tokens")) { + return Response.json({ token: "installation-token", expires_at: "2026-05-28T00:04:00.000Z" }); + } + // A web-visible route file (isVisualPath) — the ONLY difference from the sibling unified-comment fixture — + // so screenshotsAllowed's file-touch gate opens and buildCapture actually runs for this PR. + if (url.includes("/pulls/3/files")) { + return Response.json([{ filename: "apps/gittensory-ui/src/routes/app.index.tsx", additions: 5, deletions: 1, status: "modified" }]); + } + if (/\/pulls\/3(?:\?|$)/.test(url)) return Response.json({ number: 3, mergeable_state: "clean" }); + if (url.includes("/check-runs") && method === "GET") return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 901 }, { status: 201 }); + if (url.includes("/check-runs/901") && method === "PATCH") return Response.json({ id: 901 }); + if (url.includes("/issues/3/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/3/comments") && method === "POST") { + postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 1, html_url: "https://github.com/comment/1" }, { status: 201 }); + } + // Preview discovery (deployments / commit checks / PR comments): none configured for this fixture, so + // buildCapture's discovery chain finds nothing and falls back to placeholders — it's wrapped in its own + // try/catch, so a 404 here degrades to "no preview" rather than failing the capture or the review. + return new Response("not found", { status: 404 }); + }); + + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "pr-visual-config-wiring", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "read", issues: "write", checks: "write" }, + events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 3, + title: "Update the app index route", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "visualcfg123" }, + labels: [{ name: "bug" }], + body: "Fixes #1\n\nValidation: npm test", + }, + }, + }); + + // The capture pipeline ran (resolveVisualCaptureConfig -> buildCapture, both reached only through this + // webhook path) and produced at least a placeholder-backed route, so the collapsible renders. + expect(postedBody).toContain("Visual preview"); + expect(postedBody).toContain("`/app`"); + // Public-safe by construction — no internal trust/economics fields leak through the shot URLs either. + expect(postedBody).not.toMatch(/wallet|hotkey|reward|trust score/i); + } finally { + liveCiSpy.mockRestore(); + } + }); + // #1957: with the unified comment on AND `.gittensory.yml` opting into `review.changed_files_summary`, the // rendered comment gains the deterministic "Changed files" collapsible built from the SAME PR-files fetch the // unified branch already does for the readiness chip — no separate call, no AI. Mirrors the base unified-comment diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index b91fe6177b..2bf86dbf69 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null } }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null } } }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 9cdb46a797..e57e432583 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -4,7 +4,8 @@ import { githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation, } from "../../src/github/client"; -import { buildCapture } from "../../src/review/visual/capture"; +import { buildCapture, mapFilesToRoutes, resolvePreviewUrlTemplate, resolveVisualRoutes } from "../../src/review/visual/capture"; +import * as previewUrlModule from "../../src/review/visual/preview-url"; import { createTestEnv } from "../helpers/d1"; afterEach(() => { @@ -76,4 +77,248 @@ describe("visual capture preview discovery", () => { observedAtMs: Date.parse("2026-06-24T12:00:00.000Z"), }); }); + + it("an explicit preview.url_template wins over the target's own previewUrl and skips discovery entirely", async () => { + const seenUrls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + seenUrls.push(String(input)); + throw new Error("discovery must never be called when review.visual.preview.url_template is configured"); + }); + + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + "installation-token", + { + repoFullName: "owner/repo", + prNumber: 42, + headSha: "abc1234def5678900000000000000000000000a", + previewUrl: "https://should-be-ignored.example.com", + previewFromChecks: true, + }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { preview: { urlTemplate: "https://pr-{number}-{head_sha_short}.preview.example.com" } }, + ); + + expect(seenUrls).toEqual([]); + expect(result.previewPending).toBe(false); + expect(result.routes).toEqual([ + { + path: "/app", + beforeUrl: undefined, + beforeUrlMobile: undefined, + afterUrl: `https://worker.example/gittensory/shot?url=${encodeURIComponent("https://pr-42-abc1234.preview.example.com/app")}&w=1440&h=900`, + afterUrlMobile: `https://worker.example/gittensory/shot?url=${encodeURIComponent("https://pr-42-abc1234.preview.example.com/app")}&w=390&h=844`, + }, + ]); + }); + + it("uses target.previewUrl directly (no url_template configured) and skips discovery entirely", async () => { + const seenUrls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + seenUrls.push(String(input)); + throw new Error("discovery must not run when target.previewUrl is already set"); + }); + + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 9, previewUrl: "https://existing-preview.example.com", previewFromChecks: true }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(seenUrls).toEqual([]); + expect(result.routes[0]?.afterUrl).toContain(encodeURIComponent("https://existing-preview.example.com/app")); + }); + + it("degrades to no preview (never throws) when getLatestDeploymentStatus itself throws — defense-in-depth for a callee that never actually rejects in practice", async () => { + const statusSpy = vi.spyOn(previewUrlModule, "getLatestDeploymentStatus").mockRejectedValueOnce(new Error("transient failure")); + vi.stubGlobal("fetch", async () => new Response("not found", { status: 404 })); + + try { + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 10, headSha: "deadbeef" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + expect(result.routes[0]?.afterUrl).toContain("placeholder=loading"); + expect(result.previewPending).toBe(false); + } finally { + statusSpy.mockRestore(); + } + }); + + it("marks the capture pending when a matching check run is still running (buildState 'building')", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) { + return Response.json({ check_runs: [{ name: "Cloudflare Workers Builds", status: "in_progress" }] }); + } + if (url.includes("/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 11, headSha: "cafebabe", previewFromChecks: true }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(result.previewPending).toBe(true); + expect(result.routes[0]?.afterUrl).toContain("placeholder=loading"); + }); + + it("finds the preview URL from a commit check run, skipping the PR-comment fallback entirely", async () => { + const seenUrls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + seenUrls.push(url); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) { + return Response.json({ check_runs: [{ status: "completed", conclusion: "success", details_url: "https://pr-9.myapp.pages.dev/preview" }] }); + } + return new Response("not found", { status: 404 }); + }); + + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 9, headSha: "cafebabe", previewFromChecks: true }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(seenUrls.some((url) => url.includes("/issues/9/comments"))).toBe(false); + expect(result.routes[0]?.afterUrl).toContain(encodeURIComponent("https://pr-9.myapp.pages.dev/app")); + }); + + it("marks the capture pending when a matching check run already succeeded (buildState 'succeeded')", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) { + return Response.json({ check_runs: [{ name: "Cloudflare Workers Builds", status: "completed", conclusion: "success" }] }); + } + if (url.includes("/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 12, headSha: "cafebabe", previewFromChecks: true }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(result.previewPending).toBe(true); + }); + + it("leaves the capture non-pending when no matching preview check run exists at all (buildState 'absent')", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/status")) return Response.json({ statuses: [] }); + if (url.includes("/check-runs")) return Response.json({ check_runs: [{ name: "lint", status: "completed", conclusion: "success" }] }); + if (url.includes("/comments")) return Response.json([]); + return new Response("not found", { status: 404 }); + }); + + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 13, headSha: "cafebabe", previewFromChecks: true }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(result.previewPending).toBe(false); + }); + + it("an explicit routes.paths list replaces file-based route inference end to end", async () => { + vi.stubGlobal("fetch", async () => Response.json([], { status: 200 })); + + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 1, previewFromChecks: false }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { routes: { paths: ["/pricing"] } }, + ); + + expect(result.routes.map((route) => route.path)).toEqual(["/pricing"]); + }); +}); + +describe("resolvePreviewUrlTemplate (#3609)", () => { + it("substitutes {number}, {head_sha}, and {head_sha_short}", () => { + const url = resolvePreviewUrlTemplate("https://pr-{number}-{head_sha_short}.preview.example.com/{head_sha}", { + number: 42, + headSha: "abc1234def5678900000000000000000000000a", + }); + expect(url).toBe("https://pr-42-abc1234.preview.example.com/abc1234def5678900000000000000000000000a"); + }); + + it("leaves the sha placeholders empty when headSha is missing", () => { + expect(resolvePreviewUrlTemplate("https://pr-{number}-{head_sha_short}.example.com/{head_sha}", { number: 7 })).toBe( + "https://pr-7-.example.com/", + ); + }); + + it("is a no-op on a template with no placeholders", () => { + expect(resolvePreviewUrlTemplate("https://staging.example.com", { number: 1, headSha: "abc" })).toBe("https://staging.example.com"); + }); +}); + +describe("resolveVisualRoutes (#3610)", () => { + const files = ["apps/gittensory-ui/src/routes/app.index.tsx"]; + const manyFiles = [ + "apps/gittensory-ui/src/routes/app.index.tsx", + "apps/gittensory-ui/src/routes/app.analytics.tsx", + "apps/gittensory-ui/src/routes/app.billing.tsx", + ]; + + it("falls through to file-based inference when config is absent, null, or empty", () => { + expect(resolveVisualRoutes(files)).toEqual(["/app"]); + expect(resolveVisualRoutes(files, null)).toEqual(["/app"]); + expect(resolveVisualRoutes(files, {})).toEqual(["/app"]); + }); + + it("an explicit non-empty paths list replaces file-based inference entirely", () => { + expect(resolveVisualRoutes(files, { paths: ["/pricing", "/docs"] })).toEqual(["/pricing", "/docs"]); + }); + + it("an explicit but empty paths list still falls through to inference", () => { + expect(resolveVisualRoutes(files, { paths: [] })).toEqual(["/app"]); + }); + + it("maxRoutes caps an explicit paths list, not just inferred routes", () => { + expect(resolveVisualRoutes(manyFiles, { paths: ["/a", "/b", "/c"], maxRoutes: 2 })).toEqual(["/a", "/b"]); + }); + + it("a maxRoutes of zero or negative falls back to the built-in default cap", () => { + expect(resolveVisualRoutes(manyFiles, { maxRoutes: 0 })).toEqual(["/app", "/app/analytics"]); + expect(resolveVisualRoutes(manyFiles, { maxRoutes: -1 })).toEqual(["/app", "/app/analytics"]); + }); +}); + +describe("mapFilesToRoutes maxRoutes parameter", () => { + const manyFiles = [ + "apps/gittensory-ui/src/routes/app.index.tsx", + "apps/gittensory-ui/src/routes/app.analytics.tsx", + "apps/gittensory-ui/src/routes/app.billing.tsx", + ]; + + it("defaults to the built-in cap of 2", () => { + expect(mapFilesToRoutes(manyFiles)).toEqual(["/app", "/app/analytics"]); + }); + + it("honors an explicit maxRoutes override", () => { + expect(mapFilesToRoutes(manyFiles, undefined, 1)).toEqual(["/app"]); + expect(mapFilesToRoutes(manyFiles, undefined, 3)).toEqual(["/app", "/app/analytics", "/app/billing"]); + }); }); diff --git a/test/unit/visual-config-wiring.test.ts b/test/unit/visual-config-wiring.test.ts new file mode 100644 index 0000000000..b2b11ea223 --- /dev/null +++ b/test/unit/visual-config-wiring.test.ts @@ -0,0 +1,37 @@ +import { describe, expect, it, vi } from "vitest"; +import { resolveVisualCaptureConfig } from "../../src/queue/processors"; +import { EMPTY_VISUAL_CONFIG, parseFocusManifest } from "../../src/signals/focus-manifest"; +import * as focusManifestLoader from "../../src/signals/focus-manifest-loader"; + +describe("review.visual wiring (#3609 / #3610)", () => { + it("resolves review.visual from the repo's focus manifest", async () => { + const manifest = parseFocusManifest({ + review: { + visual: { + preview: { url_template: "https://pr-{number}.preview.example.com" }, + routes: { paths: ["/pricing"], max_routes: 3 }, + }, + }, + }); + const loadSpy = vi.spyOn(focusManifestLoader, "loadRepoFocusManifest").mockResolvedValue(manifest); + + await expect(resolveVisualCaptureConfig({} as Env, "acme/widgets")).resolves.toEqual({ + preview: { urlTemplate: "https://pr-{number}.preview.example.com" }, + routes: { paths: ["/pricing"], maxRoutes: 3 }, + }); + expect(loadSpy).toHaveBeenCalledWith(expect.anything(), "acme/widgets"); + loadSpy.mockRestore(); + }); + + it("yields the empty defaults when the manifest has no review.visual config", async () => { + const loadSpy = vi.spyOn(focusManifestLoader, "loadRepoFocusManifest").mockResolvedValue(parseFocusManifest({})); + await expect(resolveVisualCaptureConfig({} as Env, "acme/widgets")).resolves.toEqual({ ...EMPTY_VISUAL_CONFIG }); + loadSpy.mockRestore(); + }); + + it("fails open to the empty defaults when the manifest load rejects", async () => { + const loadSpy = vi.spyOn(focusManifestLoader, "loadRepoFocusManifest").mockRejectedValue(new Error("manifest unavailable")); + await expect(resolveVisualCaptureConfig({} as Env, "acme/widgets")).resolves.toEqual({ ...EMPTY_VISUAL_CONFIG }); + loadSpy.mockRestore(); + }); +});