diff --git a/src/bindings.ts b/src/bindings.ts index 0c51232..cc7c5e1 100644 --- a/src/bindings.ts +++ b/src/bindings.ts @@ -46,6 +46,9 @@ const BindingsSchema = z }) .strict(), ), + // Keyed by the repo the App clones for. Prefer the FULL `/` + // slug; a bare `` key still resolves (see githubAppNameFor) so + // existing state files keep working. github_apps: z.record(z.string()), smoke_target: z.string().optional(), }) @@ -53,6 +56,39 @@ const BindingsSchema = z export type Bindings = z.infer; +// Resolve the Coolify GitHub App name for a repo, full slug first. +// +// The short name alone is not a key: `` is only unique *within* an org, +// so `heavy-duty/incubator` and `acme/incubator` collapse onto one entry and +// whichever App is bound there gets used to clone BOTH — silently, because a +// wrong-but-existing App resolves to a real uuid and the create succeeds. The +// full slug is the thing that actually identifies a repo, so it wins. +// +// The bare-`` fallback is kept deliberately: it is what every state file +// written before this used, and dropping it would break them for no gain. A +// short key is unambiguous right up until a second org shows up, which is +// precisely when the full-slug key it falls back from starts winning instead. +export function githubAppNameFor(bindings: Bindings, orgRepo: string): string { + const repoShort = orgRepo.split("/")[1] ?? orgRepo; + const name = bindings.github_apps[orgRepo] ?? bindings.github_apps[repoShort]; + if (!name) { + throw new Error( + [ + `no GitHub App bound for ${orgRepo}`, + "", + ` looked for: github_apps["${orgRepo}"], then github_apps["${repoShort}"]`, + ` bound repos: ${Object.keys(bindings.github_apps).join(", ") || "(none)"}`, + "", + "Add it to environments.yaml, keyed by the full slug:", + "", + " github_apps:", + ` ${orgRepo}: `, + ].join("\n"), + ); + } + return name; +} + export function loadBindings( path: string, opts: { overrideText?: string } = {}, diff --git a/src/cli.ts b/src/cli.ts index df3d668..e516aaa 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -4,7 +4,7 @@ import { join } from "node:path"; import { parseArgs } from "node:util"; import { parse as parseYaml } from "yaml"; import { type Executor, applyHostnameOverlay, applyPlan } from "./apply.js"; -import { loadBindings } from "./bindings.js"; +import { githubAppNameFor, loadBindings } from "./bindings.js"; import { loadCoolifyEnv } from "./config.js"; import { CoolifyClient, HttpError } from "./coolify.js"; import { @@ -20,8 +20,8 @@ import { serverAdd } from "./server.js"; import { smoke } from "./smoke.js"; import { assertTeam, formatTeam } from "./team.js"; -const USAGE = `usage: cast apply / --env [--path ] [--hostname-overlay ] - cast diff / --env [--full] +const USAGE = `usage: cast apply / --env [--path ] [--project ] [--hostname-overlay ] + cast diff / --env [--full] [--project ] cast server add --ip --key --env [--user root] [--port 22] cast smoke --env cast team [--env ] @@ -32,7 +32,13 @@ const USAGE = `usage: cast apply / --env [--path ] [--host Coolify takes one, because every one of them first asserts the token belongs to that environment's declared team. \`cast team\` alone (no --env) reports the token's team - without needing a binding — use it to fill environments.yaml.`; + without needing a binding — use it to fill environments.yaml. + --project + the Coolify project to act on, when it is not named after the + repo (the default). A project built by hand in the UI is called + whatever someone typed; \`diff\` refuses rather than reporting an + absent project as an empty one, and this is how you point it at + the real name.`; // cast is stateless: every instance-scoped input is read from the state // directory it is pointed at, never from a location the tool itself knows. @@ -142,20 +148,53 @@ export function projectLiveFields( }; } -async function fetchLive( +// The live side of a diff/apply is either "here are the resources" or "the +// thing I was told to look at does not exist" — and those two must NOT collapse +// into the same value. +// +// They used to: both returned []. That is right for `apply` (a first apply +// legitimately creates the project and its environment) and quietly wrong for +// `diff`, because computeDiff(desired, []) means "every desired resource is +// missing" — rendered as a confident full-create plan. So a diff pointed at a +// project name that does not exist reports a CLEAN-LOOKING plan that verified +// nothing at all. Same shape of lie as the wrong-team token in team.ts: an +// unverifiable read that answers "absent" and invites a create. +// +// Keeping the distinction in the type is what lets each caller take its own +// (opposite, and both correct) position on absence. +export type LiveLookup = + | { found: true; live: Live[] } + | { + found: false; + missing: "project"; + project: string; + available: string[]; + } + | { + found: false; + missing: "environment"; + project: string; + environment: string; + }; + +export async function fetchLive( client: CoolifyClient, projectName: string, envName: string, -): Promise { - // List resources inside /; tolerate a missing - // project (first apply creates it) or a missing environment (first apply - // into a project that doesn't have this environment yet) by returning []. +): Promise { const projects = (await client.get("/projects")) as Array<{ uuid: string; name: string; }>; const project = projects.find((p) => p.name === projectName); - if (!project) return []; + if (!project) { + return { + found: false, + missing: "project", + project: projectName, + available: projects.map((p) => p.name), + }; + } // GET /projects/{uuid}/{environment_name_or_uuid} eager-loads exactly // these relations (app/Http/Controllers/Api/ProjectController.php // @environment_details, coollabsio/coolify v4.1.2): applications, @@ -181,7 +220,14 @@ async function fetchLive( redis?: Array>; services?: Array>; } | null; - if (!env) return []; + if (!env) { + return { + found: false, + missing: "environment", + project: projectName, + environment: envName, + }; + } const map = ( kind: ResourceKind, items: Array> = [], @@ -193,12 +239,56 @@ async function fetchLive( fields: projectLiveFields(kind, i), env: undefined, // populated per-resource below only in full mode by caller })); + return { + found: true, + live: [ + ...map("application", env.applications), + ...map("database", env.postgresqls), + ...map("database", env.redis), + ...map("service", env.services), + ], + }; +} + +// Why `diff` refuses instead of reporting an empty live side: see LiveLookup. +// The message has one job — make it impossible to read "absent" as "empty" — +// so it names what was looked for, where the name came from, and what actually +// exists next to it. +export function renderAbsentTarget( + lookup: Extract, + ctx: { orgRepo: string; overridden: boolean }, +): string { + const origin = ctx.overridden + ? "--project" + : `derived from the repo slug ${ctx.orgRepo}`; + const head = + lookup.missing === "project" + ? [ + `refusing to diff: no project named "${lookup.project}" exists in this team`, + "", + ` looked for: project "${lookup.project}" (${origin})`, + ` exists here: ${lookup.available.join(", ") || "(no projects at all)"}`, + ] + : [ + `refusing to diff: project "${lookup.project}" has no environment "${lookup.environment}"`, + "", + ` looked for: environment "${lookup.environment}" in project "${lookup.project}"`, + " note: cast names environments after --env, so a project built by", + " hand in the Coolify UI may well use a different name for the", + " same tier (Coolify's own default is `production`).", + ]; return [ - ...map("application", env.applications), - ...map("database", env.postgresqls), - ...map("database", env.redis), - ...map("service", env.services), - ]; + ...head, + "", + "An absent target reads back exactly like an empty one, so continuing would diff", + 'it as "nothing exists — create everything": a clean-looking report that verified', + "nothing. `apply` may create a target; `diff` may only ever describe one that is", + "already there.", + "", + lookup.missing === "project" + ? "Pass --project if this instance names it differently." + : "Re-run with --env naming the environment as it exists here.", + ].join("\n"); } async function main(): Promise { @@ -215,6 +305,7 @@ async function main(): Promise { env: { type: "string" }, path: { type: "string" }, state: { type: "string" }, + project: { type: "string" }, "hostname-overlay": { type: "string" }, full: { type: "boolean", default: false }, }, @@ -227,6 +318,12 @@ async function main(): Promise { } const stateDir = stateDirFrom(values.state); const repoShort = orgRepo.split("/")[1]; + // The Coolify project name and the secrets-file key are different things + // that happen to default to the same string. Only the former is a name + // some other system chose: a project built by hand in the UI is called + // whatever someone typed. --project overrides that one, and nothing else — + // secrets stay keyed by the repo (a state-repo convention we own). + const projectName = values.project ?? repoShort; const checkout = resolveCheckout(orgRepo, { env: envName, path: values.path, @@ -264,7 +361,23 @@ async function main(): Promise { const team = await assertTeam(client, binding.team, envName); console.log(`team ${formatTeam(team)} ✓`); const mode = command === "apply" || values.full ? "full" : "structural"; - const live = await fetchLive(client, repoShort, envName); + const lookup = await fetchLive(client, projectName, envName); + // apply and diff take opposite (and both correct) positions on absence: + // apply is *allowed* to be the thing that brings a project into existence, + // so [] is a legitimate starting point. diff is only ever a claim about + // something that already exists — for it, absence is not an empty diff, it + // is the absence of anything to diff against, and reporting a full-create + // plan would launder that into a pass. See LiveLookup. + if (!lookup.found && command === "diff") { + console.error( + renderAbsentTarget(lookup, { + orgRepo, + overridden: values.project !== undefined, + }), + ); + return 2; + } + const live = lookup.found ? lookup.live : []; if (mode === "full") { for (const l of live) { const envs = (await client @@ -296,10 +409,10 @@ async function main(): Promise { if (command === "diff") return report.clean ? 0 : 1; const serverUuid = await client.serverUuid(binding.server); const githubAppUuid = await client.githubAppUuid( - bindings.github_apps[repoShort], + githubAppNameFor(bindings, orgRepo), ); const exec = buildExecutor(client, { - projectName: repoShort, + projectName, envName, serverUuid, githubAppUuid, diff --git a/test/bindings.test.ts b/test/bindings.test.ts new file mode 100644 index 0000000..6938282 --- /dev/null +++ b/test/bindings.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from "vitest"; +import { type Bindings, githubAppNameFor } from "../src/bindings.js"; + +function bindings(github_apps: Record): Bindings { + return { + environments: { + prod: { server: "prod-box", team: { id: 0, name: "Root Team" } }, + }, + github_apps, + } as Bindings; +} + +describe("githubAppNameFor", () => { + it("resolves the full / slug", () => { + const b = bindings({ "heavy-duty/incubator": "hdb-coolify" }); + expect(githubAppNameFor(b, "heavy-duty/incubator")).toBe("hdb-coolify"); + }); + + // Every state file written before full-slug keying uses the bare repo name. + // Dropping that would break them for no gain, so it stays as a fallback. + it("still resolves a legacy bare key", () => { + const b = bindings({ incubator: "hdb-coolify" }); + expect(githubAppNameFor(b, "heavy-duty/incubator")).toBe("hdb-coolify"); + }); + + // The whole point of the issue. A short name is unique only *within* an org, + // so two orgs' same-named repos collapse onto one key — and the loser gets + // cloned by the winner's App, silently, because a wrong-but-existing App + // still resolves to a real uuid and the create succeeds. + it("keeps two orgs' same-named repos on separate Apps", () => { + const b = bindings({ + "heavy-duty/incubator": "hdb-coolify", + "acme/incubator": "acme-coolify", + }); + expect(githubAppNameFor(b, "heavy-duty/incubator")).toBe("hdb-coolify"); + expect(githubAppNameFor(b, "acme/incubator")).toBe("acme-coolify"); + }); + + // Precedence matters in exactly the case that motivated the fix: a state file + // mid-migration carries both a legacy short key and a new full-slug one. The + // slug is the thing that actually identifies a repo, so it must win. + it("prefers the full slug over a colliding bare key", () => { + const b = bindings({ + incubator: "legacy-app", + "heavy-duty/incubator": "hdb-coolify", + }); + expect(githubAppNameFor(b, "heavy-duty/incubator")).toBe("hdb-coolify"); + }); + + it("refuses an unbound repo, naming both keys it tried", () => { + const b = bindings({ "heavy-duty/other": "other-app" }); + const err = githubAppNameFor.bind( + null, + b, + "heavy-duty/incubator", + ) as () => string; + expect(err).toThrow(/no GitHub App bound for heavy-duty\/incubator/); + expect(err).toThrow(/github_apps\["heavy-duty\/incubator"\]/); + expect(err).toThrow(/github_apps\["incubator"\]/); + expect(err).toThrow(/heavy-duty\/other/); + }); +}); diff --git a/test/live-lookup.test.ts b/test/live-lookup.test.ts new file mode 100644 index 0000000..a7808f2 --- /dev/null +++ b/test/live-lookup.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it, vi } from "vitest"; +import { fetchLive, renderAbsentTarget } from "../src/cli.js"; +import { CoolifyClient } from "../src/coolify.js"; + +// A Coolify that answers GET /projects with `projects`, and +// GET /projects/{uuid}/{env} with whatever `envByProject` holds for it +// (undefined → 404, which is how Coolify says "no such environment"). +function coolify( + projects: Array<{ uuid: string; name: string }>, + envByProject: Record = {}, +): CoolifyClient { + const fetchImpl = vi.fn(async (url: string | URL) => { + const path = new URL(String(url)).pathname.replace("/api/v1", ""); + if (path === "/projects") { + return new Response(JSON.stringify(projects), { status: 200 }); + } + const m = path.match(/^\/projects\/([^/]+)\/(.+)$/); + if (m) { + const body = envByProject[`${m[1]}/${m[2]}`]; + return body === undefined + ? new Response(JSON.stringify({ message: "Not found." }), { + status: 404, + }) + : new Response(JSON.stringify(body), { status: 200 }); + } + return new Response("{}", { status: 500 }); + }) as unknown as typeof fetch; + return new CoolifyClient("https://coolify.test", "tok", fetchImpl); +} + +describe("fetchLive", () => { + it("returns the live resources when project and environment both exist", async () => { + const client = coolify([{ uuid: "p1", name: "incubator" }], { + "p1/prod": { + applications: [{ name: "core", uuid: "a1" }], + postgresqls: [{ name: "db", uuid: "d1" }], + }, + }); + const r = await fetchLive(client, "incubator", "prod"); + expect(r.found).toBe(true); + if (!r.found) throw new Error("unreachable"); + expect(r.live.map((l) => l.name).sort()).toEqual(["core", "db"]); + }); + + // The bug this whole change exists for: an absent project used to come back + // as [], which computeDiff reads as "every desired resource is missing" and + // renders as a confident full-create plan. Absence must be its own answer, + // distinguishable from an empty-but-real environment. + it("reports an ABSENT project as absent, not as empty", async () => { + const client = coolify([ + { uuid: "p1", name: "incubator-prod" }, + { uuid: "p2", name: "umami" }, + ]); + const r = await fetchLive(client, "incubator", "prod"); + expect(r).toEqual({ + found: false, + missing: "project", + project: "incubator", + available: ["incubator-prod", "umami"], + }); + }); + + // Same lie, a different road: the project is real but the environment name + // is not. A hand-built project is very often `production`, not `prod`. + it("reports an ABSENT environment as absent, not as empty", async () => { + const client = coolify([{ uuid: "p1", name: "incubator" }], { + "p1/production": { applications: [] }, + }); + const r = await fetchLive(client, "incubator", "prod"); + expect(r).toEqual({ + found: false, + missing: "environment", + project: "incubator", + environment: "prod", + }); + }); + + // The distinction has to be real in BOTH directions, or the gate would just + // trade a false pass for a false alarm: a project whose environment exists + // and is genuinely empty is `found`, with zero resources. + it("distinguishes a real-but-empty environment from an absent one", async () => { + const client = coolify([{ uuid: "p1", name: "incubator" }], { + "p1/prod": { applications: [], postgresqls: [], services: [] }, + }); + const r = await fetchLive(client, "incubator", "prod"); + expect(r).toEqual({ found: true, live: [] }); + }); +}); + +describe("renderAbsentTarget", () => { + it("names what it looked for, where the name came from, and what exists", () => { + const msg = renderAbsentTarget( + { + found: false, + missing: "project", + project: "incubator", + available: ["incubator-prod", "umami"], + }, + { orgRepo: "heavy-duty/incubator", overridden: false }, + ); + expect(msg).toMatch(/no project named "incubator"/); + expect(msg).toMatch(/derived from the repo slug heavy-duty\/incubator/); + expect(msg).toMatch(/incubator-prod, umami/); + expect(msg).toMatch(/--project /); + // The reader must not be able to walk away thinking a clean diff was a pass. + expect(msg).toMatch(/verified\s+nothing/); + }); + + it("says the name came from --project when it was overridden", () => { + const msg = renderAbsentTarget( + { + found: false, + missing: "project", + project: "typo", + available: ["incubator"], + }, + { orgRepo: "heavy-duty/incubator", overridden: true }, + ); + expect(msg).toMatch(/\(--project\)/); + }); + + it("points at the UI-naming gotcha when the environment is what is missing", () => { + const msg = renderAbsentTarget( + { + found: false, + missing: "environment", + project: "incubator", + environment: "prod", + }, + { orgRepo: "heavy-duty/incubator", overridden: false }, + ); + expect(msg).toMatch(/has no environment "prod"/); + expect(msg).toMatch(/production/); + expect(msg).toMatch(/--env/); + }); +});