From 7586df453b922c8740c758eac61453d12ac0cf6f Mon Sep 17 00:00:00 2001 From: claude-hdb Date: Mon, 13 Jul 2026 14:52:38 +0000 Subject: [PATCH] fix: diff refuses an absent target instead of reporting it as empty (#11, #6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `diff` could not tell "this project does not exist" from "this project is empty" — both came back as [] from fetchLive. That is right for `apply` (a first apply legitimately creates the project and its environment) and quietly wrong for `diff`: computeDiff(desired, []) means "every desired resource is missing", rendered as a confident full-create plan. So a diff aimed at a name that does not exist reported a CLEAN-LOOKING plan that verified nothing. Same shape of lie as the wrong-team token #10 closed — an unverifiable read that answers "absent" and invites a create — reached through the project name instead of the team. It matters more now: with a single Root Team the team assert can never fire, so it is no longer guarding this class of bug at all. There are two roads to it, not one. The project name may be wrong, and so may the environment name: cast names environments after --env, but a project built by hand in the Coolify UI uses whatever someone typed (Coolify's own default is `production`, not `prod`). Both are gated. - fetchLive returns a LiveLookup union, so absence is its own answer rather than a value that happens to equal "empty". diff refuses (exit 2) and names what it looked for, where that name came from, and what exists instead; apply keeps today's tolerant behaviour, which is the whole point of the split. - --project overrides the repo-derived project name, for an instance that names it differently. It overrides ONLY that: secrets stay keyed by the repo, a state-repo convention we own. #6, same root cause — `repoShort` was doing four unrelated jobs. github_apps is now resolved by full / slug, falling back to a bare key so existing state files keep working. A short name is unique only *within* an org, so two orgs' same-named repos collapsed onto one entry and whichever App was bound there would clone both — silently, because a wrong-but-existing App still resolves to a real uuid and the create succeeds. Verified end-to-end against a fake Coolify, driving the real binary: an absent project refuses (exit 2), --project recovers it (exit 0), an absent environment refuses (exit 2). 12 new tests; 98 pass; check clean. Co-Authored-By: Claude Opus 4.8 --- src/bindings.ts | 36 ++++++++++ src/cli.ts | 151 ++++++++++++++++++++++++++++++++++----- test/bindings.test.ts | 62 ++++++++++++++++ test/live-lookup.test.ts | 136 +++++++++++++++++++++++++++++++++++ 4 files changed, 366 insertions(+), 19 deletions(-) create mode 100644 test/bindings.test.ts create mode 100644 test/live-lookup.test.ts 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/); + }); +}); -- 2.45.2