Skip to content

Commit e063f55

Browse files
authored
fix(review): per-repo review.visual.production_url override for bot-capture (#4564)
PUBLIC_SITE_ORIGIN is a single global env var with no per-repo awareness, so on a multi-repo self-host instance it can be correct for at most one repo -- every other repo's "before" (production) shot silently degrades to a dash, which means the bot-capture screenshot pipeline never produces a real before/after pair for that repo (#3611). Add review.visual.production_url as a per-repo config-as-code override, mirroring the existing review.visual.preview.url_template precedence pattern. Also generalize the route-file regex (DEFAULT_ROUTE_FILE) and isVisualPath's app-folder pattern beyond apps/gittensory-ui/ -- metagraphed's apps/ui/** uses the identical TanStack flat-file routing convention, just under a different app folder name, so route inference was silently falling back to "/" for every metagraphed PR regardless of which page actually changed.
1 parent f5d5135 commit e063f55

8 files changed

Lines changed: 187 additions & 15 deletions

File tree

packages/gittensory-engine/src/focus-manifest.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -719,6 +719,13 @@ export const EMPTY_SELF_HOST_AI_MODEL_CONFIG: SelfHostAiModelConfig = {
719719
/** Per-repo before/after screenshot-capture config under `review.visual` (#3609 / #3610). Generic by design —
720720
* every self-hoster wires their OWN repo's preview-deploy setup and route shape with config, not code. */
721721
export type VisualConfig = {
722+
/** `review.visual.production_url`: the repo's "before" production URL — e.g. `https://metagraph.sh` for a
723+
* repo whose live site differs from the operator's own `PUBLIC_SITE_ORIGIN` env var (a single GLOBAL value
724+
* with no per-repo awareness, correct for at most one repo on a multi-repo self-host instance). ALWAYS wins
725+
* over `PUBLIC_SITE_ORIGIN` when set, mirroring `preview.url_template`'s precedence over GitHub-native
726+
* discovery. null (default) ⇒ byte-identical to today (falls back to `PUBLIC_SITE_ORIGIN`). Validated at
727+
* parse time against the same SSRF guard (`isSafeHttpUrl`) the renderer itself unconditionally applies. */
728+
productionUrl: string | null;
722729
preview: VisualPreviewConfig;
723730
routes: VisualRoutesConfig;
724731
themes: VisualTheme[];
@@ -787,6 +794,7 @@ export type VisualRoutesConfig = {
787794
};
788795

789796
export const EMPTY_VISUAL_CONFIG: VisualConfig = {
797+
productionUrl: null,
790798
preview: { urlTemplate: null },
791799
routes: { paths: [], maxRoutes: null },
792800
themes: [],
@@ -2301,6 +2309,7 @@ function overlaySelfHostAiModelConfig(base: SelfHostAiModelConfig, override: Sel
23012309

23022310
function overlayVisualConfig(base: VisualConfig, override: VisualConfig): VisualConfig {
23032311
return {
2312+
productionUrl: pickOverlayNullable(override.productionUrl, base.productionUrl),
23042313
preview: { urlTemplate: pickOverlayNullable(override.preview.urlTemplate, base.preview.urlTemplate) },
23052314
routes: {
23062315
paths: pickOverlayStringList(override.routes.paths, base.routes.paths),
@@ -2543,6 +2552,7 @@ function parseSelfHostAiModelConfig(value: JsonValue | undefined, warnings: stri
25432552

25442553
function visualConfigPresent(config: VisualConfig): boolean {
25452554
return (
2555+
config.productionUrl !== null ||
25462556
config.preview.urlTemplate !== null ||
25472557
config.routes.paths.length > 0 ||
25482558
config.routes.maxRoutes !== null ||
@@ -2588,6 +2598,20 @@ const VISUAL_URL_TEMPLATE_DUMMY_VARS: Record<string, string> = {
25882598
"{head_sha}": "0000000000000000000000000000000000000000",
25892599
};
25902600

2601+
/** Parse `review.visual.production_url` — validated at CONFIG-READ time against the exact same SSRF guard
2602+
* (`isSafeHttpUrl`) the renderer itself unconditionally applies to every URL it navigates to. Unlike
2603+
* `preview.url_template`, this is a plain static origin with no `{number}`/`{head_sha}` placeholders to
2604+
* substitute — the "before" shot is always the SAME production page, just at a different path per route. */
2605+
function parseVisualProductionUrl(value: JsonValue | undefined, warnings: string[]): string | null {
2606+
const url = parsePublicSafeText(value, "review.visual.production_url", warnings);
2607+
if (url === null) return null;
2608+
if (!isSafeHttpUrl(url)) {
2609+
warnings.push(`Manifest "review.visual.production_url" must be a valid HTTPS URL targeting a public host; ignoring it.`);
2610+
return null;
2611+
}
2612+
return url;
2613+
}
2614+
25912615
/** Parse `review.visual.preview.url_template` — validated at CONFIG-READ time against the exact same SSRF
25922616
* guard (`isSafeHttpUrl`) the renderer itself unconditionally applies to every URL it navigates to,
25932617
* regardless of source (`src/review/visual/shot.ts`). This is deliberately redundant with that runtime
@@ -2617,6 +2641,8 @@ function parseVisualConfig(value: JsonValue | undefined, warnings: string[]): Vi
26172641
}
26182642
const record = value as Record<string, JsonValue>;
26192643

2644+
const productionUrl = parseVisualProductionUrl(record.production_url, warnings);
2645+
26202646
const previewRecord = record.preview !== null && typeof record.preview === "object" && !Array.isArray(record.preview) ? (record.preview as Record<string, JsonValue>) : undefined;
26212647
if (record.preview !== undefined && record.preview !== null && previewRecord === undefined) {
26222648
warnings.push(`Manifest "review.visual.preview" must be a mapping; ignoring it.`);
@@ -2636,7 +2662,7 @@ function parseVisualConfig(value: JsonValue | undefined, warnings: string[]): Vi
26362662
const themeStorageKey = parsePublicSafeText(record.theme_storage_key, "review.visual.theme_storage_key", warnings);
26372663
const actionsFallback = normalizeOptionalBoolean(record.actions_fallback, "review.visual.actions_fallback", warnings) === true;
26382664

2639-
return { preview: { urlTemplate }, routes: { paths, maxRoutes }, themes, gif, enabled, themeStorageKey, actionsFallback };
2665+
return { productionUrl, preview: { urlTemplate }, routes: { paths, maxRoutes }, themes, gif, enabled, themeStorageKey, actionsFallback };
26402666
}
26412667

26422668
function parseAutoReviewTitleKeywords(value: JsonValue | undefined, warnings: string[]): string[] {
@@ -2943,6 +2969,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue
29432969
}
29442970
if (visualConfigPresent(review.visual)) {
29452971
const visual: Record<string, JsonValue> = {};
2972+
if (review.visual.productionUrl !== null) visual.production_url = review.visual.productionUrl;
29462973
if (review.visual.preview.urlTemplate !== null) visual.preview = { url_template: review.visual.preview.urlTemplate };
29472974
if (review.visual.routes.paths.length > 0 || review.visual.routes.maxRoutes !== null) {
29482975
const routes: Record<string, JsonValue> = {};

src/review/visual/capture.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Realtime visual capture (reviewbot→gittensory convergence — visual port). taopedia-style before/after.
22
//
3-
// before = production (PUBLIC_SITE_ORIGIN); after = the PR's preview-deploy URL, discovered the
3+
// before = production (review.visual.production_url, falling back to the global PUBLIC_SITE_ORIGIN env var);
4+
// after = the PR's preview-deploy URL, discovered the
45
// provider-agnostic way (Deployments API → commit checks → cloudflare-bot PR comment). Each page is
56
// rendered once here (in the queue consumer, which has the time budget), stored as a PNG in R2
67
// (env.REVIEW_AUDIT), and embedded either as <PUBLIC_API_ORIGIN>/gittensory/shot?key=<r2key> (this
@@ -30,7 +31,11 @@ import { encodeScrollGif, isScrollGifAvailable } from "./scroll-gif";
3031

3132
const NAMESPACE = "gittensory";
3233
const DEFAULT_ROUTES = ["/"];
33-
const DEFAULT_ROUTE_FILE = /apps\/gittensory-ui\/src\/routes\/(.+?)\.(?:tsx|jsx)$/i;
34+
// The app-folder segment is a wildcard, not hardcoded to gittensory-ui: metagraphed's UI (apps/ui/src/routes/)
35+
// uses the identical TanStack flat-file convention `routeForFile` below implements, just under a different app
36+
// folder name. Only ever matched against the CURRENT repo's own changed-file paths (see mapFilesToRoutes'
37+
// caller), so widening this carries no cross-repo ambiguity risk.
38+
const DEFAULT_ROUTE_FILE = /apps\/[^/]+\/src\/routes\/(.+?)\.(?:tsx|jsx)$/i;
3439
// Each route renders desktop + mobile for before + after (up to 4 PNGs). Cap routes to bound browser-render
3540
// wall-clock — Browser Rendering is the costliest binding.
3641
const MAX_ROUTES = 2;
@@ -368,6 +373,11 @@ async function captureScrollGif(
368373
* #3612 / #4109). Absent ⇒ byte-identical to today (GitHub-native discovery, automatic route inference,
369374
* single default-theme capture, built-in route cap, no scroll-GIF, no localStorage theme forcing). */
370375
export type VisualCaptureConfig = {
376+
/** `review.visual.production_url` (#3611 follow-up): overrides `env.PUBLIC_SITE_ORIGIN` (a single GLOBAL
377+
* value with no per-repo awareness) as the "before" base for THIS repo. ALWAYS wins when set, mirroring
378+
* `preview.urlTemplate`'s precedence over discovery. null/undefined ⇒ falls back to `env.PUBLIC_SITE_ORIGIN`,
379+
* byte-identical to today. */
380+
productionUrl?: string | null | undefined;
371381
preview?: VisualPreviewInput | null | undefined;
372382
routes?: VisualRoutesInput | null | undefined;
373383
themes?: readonly ShotTheme[] | null | undefined;
@@ -391,8 +401,10 @@ export type VisualCaptureConfig = {
391401
export async function buildCapture(env: Env, token: string, target: CaptureTarget, visualFiles: string[], rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined, visualConfig?: VisualCaptureConfig | null | undefined): Promise<CaptureResult> {
392402
const repo = parseRepo(target.repoFullName);
393403
const apiVersion = "2022-11-28";
394-
// before = production (PUBLIC_SITE_ORIGIN, e.g. https://gittensory.aethereal.dev).
395-
const prodBase = env.PUBLIC_SITE_ORIGIN ?? "";
404+
// before = production. review.visual.production_url (#3611 follow-up) ALWAYS wins when set -- PUBLIC_SITE_ORIGIN
405+
// is a single GLOBAL env var (e.g. https://gittensory.aethereal.dev) with no per-repo awareness, correct for at
406+
// most one repo on a multi-repo self-host instance; every other repo needs its own override here.
407+
const prodBase = visualConfig?.productionUrl ? visualConfig.productionUrl : (env.PUBLIC_SITE_ORIGIN ?? "");
396408

397409
// after = the PR's preview deploy. An explicit review.visual.preview.url_template (#3609) ALWAYS wins —
398410
// a maintainer-configured template is a stronger signal than inference, and is the only option for a

src/review/visual/paths.ts

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,17 @@
11
// Visual-path classifier (reviewbot→gittensory convergence — visual capture port).
22
//
33
// PORTED VERBATIM from reviewbot's src/agents/gittensory/capabilities.ts `isVisualPath` (the three
4-
// VISUAL_PATTERNS). This is the EMPHATIC gate: screenshots fire ONLY for WEB-VISIBLE changes — a
5-
// frontend page (apps/gittensory-ui/**), a public asset (public/**, e.g. an OG image), or a
6-
// front-of-house source extension (.tsx/.jsx/.css/.scss/.sass/.less/.html/.svg/.astro/.vue/.svelte/.mdx).
7-
// A backend change (.ts/.md/.json/.py/...) matches NONE of these, so capture never triggers for it.
4+
// VISUAL_PATTERNS), with the first pattern's app-folder segment widened to a wildcard (#3611 follow-up) so it
5+
// isn't gittensory-ui-only — see capture.ts's DEFAULT_ROUTE_FILE for the same generalization. This is the
6+
// EMPHATIC gate: screenshots fire ONLY for WEB-VISIBLE changes — any frontend app folder (apps/*/**, e.g.
7+
// apps/gittensory-ui/** or apps/ui/**), a public asset (public/**, e.g. an OG image), or a front-of-house
8+
// source extension (.tsx/.jsx/.css/.scss/.sass/.less/.html/.svg/.astro/.vue/.svelte/.mdx). A backend change
9+
// (.ts/.md/.json/.py/...) matches NONE of these, so capture never triggers for it.
810
//
911
// PURE — no imports, no I/O. Callers MUST filter changed files through this before any capture.
1012

1113
const VISUAL_PATTERNS: RegExp[] = [
12-
/^apps\/gittensory-ui\//i,
14+
/^apps\/[^/]+\//i,
1315
/(^|\/)public\//i,
1416
/\.(tsx|jsx|css|scss|sass|less|html|svg|astro|vue|svelte|mdx)$/i,
1517
];

test/unit/focus-manifest.test.ts

Lines changed: 66 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3988,6 +3988,7 @@ describe("review.visual (#3609 preview.url_template / #3610 routes)", () => {
39883988
},
39893989
});
39903990
expect(m.review.visual).toEqual({
3991+
productionUrl: null,
39913992
preview: { urlTemplate: "https://pr-{number}.preview.example.com" },
39923993
routes: { paths: ["/pricing", "/docs"], maxRoutes: 3 },
39933994
themes: [],
@@ -4090,7 +4091,68 @@ describe("review.visual (#3609 preview.url_template / #3610 routes)", () => {
40904091
it("resolveReviewVisualConfig: null manifest yields empty defaults; a set manifest passes through", () => {
40914092
expect(resolveReviewVisualConfig(null)).toEqual({ ...EMPTY_VISUAL_CONFIG });
40924093
const manifest = parseFocusManifest({ review: { visual: { routes: { paths: ["/app"] } } } });
4093-
expect(resolveReviewVisualConfig(manifest)).toEqual({ preview: { urlTemplate: null }, routes: { paths: ["/app"], maxRoutes: null }, themes: [], gif: false, enabled: null, themeStorageKey: null, actionsFallback: false });
4094+
expect(resolveReviewVisualConfig(manifest)).toEqual({ productionUrl: null, preview: { urlTemplate: null }, routes: { paths: ["/app"], maxRoutes: null }, themes: [], gif: false, enabled: null, themeStorageKey: null, actionsFallback: false });
4095+
});
4096+
});
4097+
4098+
describe("review.visual.production_url (#3611 follow-up — per-repo override of the global PUBLIC_SITE_ORIGIN env var)", () => {
4099+
it("parses a valid production_url, marks present, and round-trips", () => {
4100+
const m = parseFocusManifest({ review: { visual: { production_url: "https://metagraph.sh" } } });
4101+
expect(m.review.visual.productionUrl).toBe("https://metagraph.sh");
4102+
expect(m.review.present).toBe(true);
4103+
expect(reviewConfigToJson(m.review)).toEqual({ visual: { production_url: "https://metagraph.sh" } });
4104+
});
4105+
4106+
it("absent production_url stays null and does not mark review present on its own", () => {
4107+
expect(parseFocusManifest({}).review.visual.productionUrl).toBeNull();
4108+
expect(parseFocusManifest({ review: { visual: {} } }).review.present).toBe(false);
4109+
});
4110+
4111+
it("rejects a non-HTTPS production_url with a warning", () => {
4112+
const bad = parseFocusManifest({ review: { visual: { production_url: "http://metagraph.sh" } } });
4113+
expect(bad.review.visual.productionUrl).toBeNull();
4114+
expect(bad.warnings.some((w) => /review\.visual\.production_url.*valid HTTPS URL/.test(w))).toBe(true);
4115+
});
4116+
4117+
it("rejects a production_url resolving to a private/internal host with a warning", () => {
4118+
const bad = parseFocusManifest({ review: { visual: { production_url: "https://prod.internal" } } });
4119+
expect(bad.review.visual.productionUrl).toBeNull();
4120+
expect(bad.warnings.some((w) => /review\.visual\.production_url.*valid HTTPS URL/.test(w))).toBe(true);
4121+
});
4122+
4123+
it("rejects a malformed production_url with a warning", () => {
4124+
const bad = parseFocusManifest({ review: { visual: { production_url: "not-a-url-at-all" } } });
4125+
expect(bad.review.visual.productionUrl).toBeNull();
4126+
expect(bad.warnings.some((w) => /review\.visual\.production_url.*valid HTTPS URL/.test(w))).toBe(true);
4127+
});
4128+
4129+
it("composes with preview.url_template — both configured independently and both round-trip", () => {
4130+
const m = parseFocusManifest({
4131+
review: { visual: { production_url: "https://metagraph.sh", preview: { url_template: "https://pr-{number}.example.com" } } },
4132+
});
4133+
expect(m.review.visual.productionUrl).toBe("https://metagraph.sh");
4134+
expect(m.review.visual.preview.urlTemplate).toBe("https://pr-{number}.example.com");
4135+
expect(reviewConfigToJson(m.review)).toEqual({
4136+
visual: { production_url: "https://metagraph.sh", preview: { url_template: "https://pr-{number}.example.com" } },
4137+
});
4138+
});
4139+
4140+
it("resolveReviewVisualConfig passes a configured production_url through", () => {
4141+
const manifest = parseFocusManifest({ review: { visual: { production_url: "https://metagraph.sh" } } });
4142+
expect(resolveReviewVisualConfig(manifest).productionUrl).toBe("https://metagraph.sh");
4143+
});
4144+
4145+
it("overlay: a per-repo production_url wins over a global-default value", () => {
4146+
const globalDefault = parseReviewConfigMapping({ visual: { production_url: "https://gittensory.aethereal.dev" } }, []);
4147+
const perRepo = parseReviewConfigMapping({ visual: { production_url: "https://metagraph.sh" } }, []);
4148+
expect(overlayReviewConfig(globalDefault, perRepo).visual.productionUrl).toBe("https://metagraph.sh");
4149+
});
4150+
4151+
it("overlay: an unset per-repo production_url falls back to the global-default value", () => {
4152+
const globalDefault = parseReviewConfigMapping({ visual: { production_url: "https://gittensory.aethereal.dev" } }, []);
4153+
const perRepo = parseReviewConfigMapping({ visual: { routes: { paths: ["/app"] } } }, []);
4154+
expect(overlayReviewConfig(globalDefault, perRepo).visual.productionUrl).toBe("https://gittensory.aethereal.dev");
4155+
expect(overlayReviewConfig(globalDefault, perRepo).visual.routes.paths).toEqual(["/app"]);
40944156
});
40954157
});
40964158

@@ -4175,7 +4237,7 @@ describe("review.visual.gif (#3612 scroll-through GIF capture)", () => {
41754237

41764238
it("composes with themes — both configured independently and both round-trip", () => {
41774239
const m = parseFocusManifest({ review: { visual: { gif: true, themes: ["dark"] } } });
4178-
expect(m.review.visual).toEqual({ preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: ["dark"], gif: true, enabled: null, themeStorageKey: null, actionsFallback: false });
4240+
expect(m.review.visual).toEqual({ productionUrl: null, preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: ["dark"], gif: true, enabled: null, themeStorageKey: null, actionsFallback: false });
41794241
expect(reviewConfigToJson(m.review)).toEqual({ visual: { themes: ["dark"], gif: true } });
41804242
});
41814243

@@ -4278,7 +4340,7 @@ describe("review.visual.theme_storage_key (#4109 localStorage theme-forcing fall
42784340

42794341
it("composes with themes — both configured independently and both round-trip", () => {
42804342
const m = parseFocusManifest({ review: { visual: { themes: ["dark"], theme_storage_key: "theme" } } });
4281-
expect(m.review.visual).toEqual({ preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: ["dark"], gif: false, enabled: null, themeStorageKey: "theme", actionsFallback: false });
4343+
expect(m.review.visual).toEqual({ productionUrl: null, preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: ["dark"], gif: false, enabled: null, themeStorageKey: "theme", actionsFallback: false });
42824344
expect(reviewConfigToJson(m.review)).toEqual({ visual: { themes: ["dark"], theme_storage_key: "theme" } });
42834345
});
42844346

@@ -4333,7 +4395,7 @@ describe("review.visual.actions_fallback (#4112 GitHub-Actions build-and-serve f
43334395

43344396
it("composes with gif — both configured independently and both round-trip", () => {
43354397
const m = parseFocusManifest({ review: { visual: { actions_fallback: true, gif: true } } });
4336-
expect(m.review.visual).toEqual({ preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: true, enabled: null, themeStorageKey: null, actionsFallback: true });
4398+
expect(m.review.visual).toEqual({ productionUrl: null, preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: true, enabled: null, themeStorageKey: null, actionsFallback: true });
43374399
expect(reviewConfigToJson(m.review)).toEqual({ visual: { gif: true, actions_fallback: true } });
43384400
});
43394401

0 commit comments

Comments
 (0)