From f33cdca342b80c9347a4126e7295a757a2b7f619 Mon Sep 17 00:00:00 2001 From: Anurag Goel Date: Wed, 23 Sep 2026 22:01:56 -0700 Subject: [PATCH] Use the config validators in doctor, and join service directories once - scripts/doctor.ts checked APPS_REPO, FACTORY_API_KEY, UI_USERNAME, and UI_PASSWORD with code of its own, with a third copy of the slug regex. Now it runs appsRepo(), apiKey(), and uiCredentials(), the validators that each process runs when it starts, and prints their errors. So doctor cannot pass a value that the gateway refuses. - config.ts checks UI_USERNAME with the slug schema of contracts.ts. The gateway gives that name to each run of the UI as its user, and the request schema checks the user with the same slug. Before, two copies of the regex had to agree. - verify-app joined the rootDir of a service onto the app directory with resolveServiceDir(), and the Blueprint with joinServiceDir(), with slightly different rules. Now both use joinServiceDir() in blueprint.ts, so they agree on each directory. It also accepts a path that is the full base, which gave a doubled path in verify-app. Tests: tests/blueprint.test.ts tests joinServiceDir() with relative and absolute bases. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 4 +-- app/blueprint.ts | 18 +++++----- app/config.ts | 5 +-- app/contracts.ts | 3 +- app/verify.ts | 17 ++------- scripts/doctor.ts | 76 ++++++++++++++++------------------------- tests/blueprint.test.ts | 25 ++++++++++++++ 7 files changed, 74 insertions(+), 74 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f127c59..6cdc655 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -111,8 +111,8 @@ and defines the MCP allowlist. `render` is imported by `claude` (for the MCP URL), by `teardown`, and by `deploy` and `delete`. `blueprint`, `git`, `images`, `teardown`, and `store` are used by `workflow` and the stages; `teardown` uses `render` and `blueprint`; `gateway` uses `store`, `policy`, -and `contracts`. `config` and `contracts` are leaves. Adding an edge that -points backwards is a design smell. +and `contracts`. `contracts` is a leaf, and `config` uses only its `slug` +schema. Adding an edge that points backwards is a design smell. ```text factory.config.ts Directories, branch, plans, asset hosts, model tiers diff --git a/app/blueprint.ts b/app/blueprint.ts index c2f7f27..c557b9e 100644 --- a/app/blueprint.ts +++ b/app/blueprint.ts @@ -365,15 +365,17 @@ function databaseBlocks(manifest: Manifest, names: ResourceNames): string[] { } /** - * Join a manifest-declared rootDir onto the app's repo-relative path. The - * manifest is agent-authored, so tolerate "." meaning the app directory - * itself and a rootDir that already repeats the app path. + * Join a directory of the manifest onto a base directory: a rootDir onto the + * app, or a staticPublishPath onto its service. The manifest is + * agent-authored, so tolerate the two things models get wrong: "." or "./" + * for the base itself, and a path that repeats the base or its end. The + * Blueprint and verify-app both use this, so they agree on each directory. */ -function joinServiceDir(root: string, rootDir: string): string { - const cleaned = rootDir.replace(/^\.\/+/, "").replace(/\/+$/, ""); - if (!cleaned || cleaned === ".") return root; - if (root.endsWith(`/${cleaned}`) || root === cleaned) return root; - return `${root}/${cleaned}`; +export function joinServiceDir(base: string, dir: string): string { + const cleaned = dir.replace(/^\.\/+/, "").replace(/\/+$/, ""); + if (!cleaned || cleaned === "." || cleaned === base) return base; + if (base.endsWith(`/${cleaned}`)) return base; + return `${base}/${cleaned}`; } /** diff --git a/app/config.ts b/app/config.ts index 2c74653..f43e7a3 100644 --- a/app/config.ts +++ b/app/config.ts @@ -1,8 +1,8 @@ /** Environment parsing and per-process validation. */ +import { slug } from "./contracts.js"; const REPO_PATTERN = /^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?\/[A-Za-z0-9._-]+$/; -const USER_SLUG_PATTERN = /^[a-z][a-z0-9-]{2,30}$/; export interface Repository { owner: string; @@ -52,7 +52,8 @@ export function apiKey(): string { export function uiCredentials(): { username: string; password: string } { const username = requireEnv("UI_USERNAME"); const password = requireEnv("UI_PASSWORD"); - if (!USER_SLUG_PATTERN.test(username)) { + // The gateway gives it to every run of the UI as the namespace `user`. + if (!slug.safeParse(username).success) { throw new Error("UI_USERNAME must be a lowercase slug"); } if (password.length < 16) { diff --git a/app/contracts.ts b/app/contracts.ts index bb6e140..e97d136 100644 --- a/app/contracts.ts +++ b/app/contracts.ts @@ -1,7 +1,8 @@ /** Zod schemas for external input, agent output, and the stored spec. */ import { z } from "zod"; -const slug = z +/** A user namespace or an app name. It goes into paths and resource names. */ +export const slug = z .string() .regex(/^[a-z][a-z0-9-]{2,30}$/, "must be a lowercase slug"); diff --git a/app/verify.ts b/app/verify.ts index cca99e1..32fb674 100644 --- a/app/verify.ts +++ b/app/verify.ts @@ -4,6 +4,7 @@ */ import { type TaskContext, task } from "@renderinc/sdk/workflows"; import { appPath } from "../factory.config.js"; +import { joinServiceDir } from "./blueprint.js"; import { type Manifest, type Service, @@ -117,7 +118,7 @@ async function verify( // This check needs no build, so a failed build cannot hide it. failures.push(...checkStaticSiteEnvVars(service)); - const serviceDir = resolveServiceDir(appDir, service.rootDir); + const serviceDir = joinServiceDir(appDir, service.rootDir); // Run the build command. const build = await runVerification(sandbox, serviceDir, [ @@ -131,7 +132,7 @@ async function verify( } if (service.kind === "static_site" && service.staticPublishPath) { - const publishDir = resolveServiceDir( + const publishDir = joinServiceDir( serviceDir, service.staticPublishPath, ); @@ -346,15 +347,3 @@ async function probeService( body: rest.join("\n"), }; } - -/** - * Join a manifest-declared subdirectory onto a base path. The manifest is - * agent-authored, so tolerate the two things models get wrong: "." or "./" - * meaning "this directory", and repeating the base path they were given. - */ -function resolveServiceDir(base: string, relative: string): string { - const cleaned = relative.replace(/^\.\/+/, "").replace(/\/+$/, ""); - if (!cleaned || cleaned === ".") return base; - if (base.endsWith(`/${cleaned}`)) return base; - return `${base}/${cleaned}`; -} diff --git a/scripts/doctor.ts b/scripts/doctor.ts index d0785d8..8a8789c 100644 --- a/scripts/doctor.ts +++ b/scripts/doctor.ts @@ -7,7 +7,12 @@ import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { factoryConfig } from "../factory.config.js"; -import { appsRepo, renderWorkspaceId } from "../app/config.js"; +import { + apiKey, + appsRepo, + renderWorkspaceId, + uiCredentials, +} from "../app/config.js"; import { githubToken, usingGitHubApp } from "../app/git.js"; import { RENDER_READ_ONLY_TOOLS } from "../app/policy.js"; import { findBlueprint, renderMcpUrl } from "../app/render.js"; @@ -52,60 +57,37 @@ async function github( } } -function checkConfiguration(): void { - heading("Configuration"); - +/** + * Run a validator of app/config.ts, the same one that a process runs when it + * starts, and record its message or its error. + */ +function validate(check: () => string, fix: string): void { try { - const repo = appsRepo(); - record({ level: "ok", message: `APPS_REPO is ${repo.fullName}` }); + record({ level: "ok", message: check() }); } catch (error) { record({ level: "fail", - message: "APPS_REPO is missing or malformed", - fix: error instanceof Error ? error.message : "Use owner/repo format.", + message: error instanceof Error ? error.message : String(error), + fix, }); } +} - const key = process.env.FACTORY_API_KEY?.trim() ?? ""; - if (!key) { - record({ - level: "fail", - message: "FACTORY_API_KEY is missing", - fix: "Generate one with: openssl rand -hex 32", - }); - } else if (key.length < 24) { - record({ - level: "fail", - message: `FACTORY_API_KEY is only ${key.length} characters`, - fix: "Use at least 24. Generate with: openssl rand -hex 32", - }); - } else { - record({ level: "ok", message: "FACTORY_API_KEY looks strong" }); - } +function checkConfiguration(): void { + heading("Configuration"); - const uiUsername = process.env.UI_USERNAME?.trim() ?? ""; - if (!/^[a-z][a-z0-9-]{2,30}$/.test(uiUsername)) { - record({ - level: "fail", - message: "UI_USERNAME is missing or is not a lowercase slug", - fix: "Use 3-31 lowercase letters, numbers, or hyphens; it also namespaces generated apps.", - }); - } else { - record({ - level: "ok", - message: `UI_USERNAME namespaces apps as ${uiUsername}`, - }); - } - const uiPassword = process.env.UI_PASSWORD?.trim() ?? ""; - if (uiPassword.length < 16) { - record({ - level: "fail", - message: "UI_PASSWORD is missing or shorter than 16 characters", - fix: "Set a strong password on the gateway.", - }); - } else { - record({ level: "ok", message: "UI_PASSWORD looks strong" }); - } + validate( + () => `APPS_REPO is ${appsRepo().fullName}`, + "Use owner/repo format.", + ); + validate(() => { + apiKey(); + return "FACTORY_API_KEY looks strong"; + }, "Generate one with: openssl rand -hex 32"); + validate( + () => `UI_USERNAME namespaces apps as ${uiCredentials().username}`, + "UI_USERNAME needs 3-31 lowercase letters, numbers, or hyphens, and UI_PASSWORD at least 16 characters.", + ); // The gateway needs this to dispatch, not just the workflows host. present( diff --git a/tests/blueprint.test.ts b/tests/blueprint.test.ts index 1a7ab03..885825e 100644 --- a/tests/blueprint.test.ts +++ b/tests/blueprint.test.ts @@ -8,6 +8,7 @@ import { parse } from "yaml"; import { appBlueprint, declaredResources, + joinServiceDir, resourceNames, rootBlueprint, } from "../app/blueprint.js"; @@ -129,6 +130,30 @@ function parsedServices(yaml: string) { return parse(yaml).projects[0].environments[0].services; } +/** + * The Blueprint and verify-app join the same agent-written directories, so + * they must agree on each one. + */ +describe("joinServiceDir", () => { + it.each([ + ["web", "apps/demo/shop/web"], + ["./web/", "apps/demo/shop/web"], + [".", "apps/demo/shop"], + ["./", "apps/demo/shop"], + // A model repeats the path that it was given, or its end. + ["shop", "apps/demo/shop"], + ["apps/demo/shop", "apps/demo/shop"], + ])("joins %s onto the app directory", (dir, joined) => { + expect(joinServiceDir("apps/demo/shop", dir)).toBe(joined); + }); + + it("joins onto an absolute directory, as verify-app does", () => { + const app = "/home/user/repo/apps/demo/shop"; + expect(joinServiceDir(app, app)).toBe(app); + expect(joinServiceDir(`${app}/web`, "dist")).toBe(`${app}/web/dist`); + }); +}); + describe("resourceNames", () => { // A new default prefix must not rename the resources of an existing app. it("takes the prefix from the spec, not from the factory configuration", () => {