fix: diff refuses an absent target instead of reporting it as empty (#11, #6) #12

Merged
dan-claude-bot merged 1 commit from fix/slug-keying into main 2026-07-13 14:57:13 +00:00
4 changed files with 366 additions and 19 deletions

View file

@ -46,6 +46,9 @@ const BindingsSchema = z
})
.strict(),
),
// Keyed by the repo the App clones for. Prefer the FULL `<org>/<repo>`
// slug; a bare `<repo>` 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<typeof BindingsSchema>;
// Resolve the Coolify GitHub App name for a repo, full slug first.
//
// The short name alone is not a key: `<repo>` 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-`<repo>` 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}: <the App's name in Coolify>`,
].join("\n"),
);
}
return name;
}
export function loadBindings(
path: string,
opts: { overrideText?: string } = {},

View file

@ -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 <org>/<repo> --env <env> [--path <dir>] [--hostname-overlay <file>]
cast diff <org>/<repo> --env <env> [--full]
const USAGE = `usage: cast apply <org>/<repo> --env <env> [--path <dir>] [--project <name>] [--hostname-overlay <file>]
cast diff <org>/<repo> --env <env> [--full] [--project <name>]
cast server add <name> --ip <ip> --key <file> --env <env> [--user root] [--port 22]
cast smoke --env <env>
cast team [--env <env>]
@ -32,7 +32,13 @@ const USAGE = `usage: cast apply <org>/<repo> --env <env> [--path <dir>] [--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 <name>
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<Live[]> {
// List resources inside <projectName>/<envName>; 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<LiveLookup> {
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<Record<string, unknown>>;
services?: Array<Record<string, unknown>>;
} | null;
if (!env) return [];
if (!env) {
return {
found: false,
missing: "environment",
project: projectName,
environment: envName,
};
}
const map = (
kind: ResourceKind,
items: Array<Record<string, unknown>> = [],
@ -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<LiveLookup, { found: false }>,
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 <name> if this instance names it differently."
: "Re-run with --env naming the environment as it exists here.",
].join("\n");
}
async function main(): Promise<number> {
@ -215,6 +305,7 @@ async function main(): Promise<number> {
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<number> {
}
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<number> {
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<number> {
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,

62
test/bindings.test.ts Normal file
View file

@ -0,0 +1,62 @@
import { describe, expect, it } from "vitest";
import { type Bindings, githubAppNameFor } from "../src/bindings.js";
function bindings(github_apps: Record<string, string>): Bindings {
return {
environments: {
prod: { server: "prod-box", team: { id: 0, name: "Root Team" } },
},
github_apps,
} as Bindings;
}
describe("githubAppNameFor", () => {
it("resolves the full <org>/<repo> 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 <repo> 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/);
});
});

136
test/live-lookup.test.ts Normal file
View file

@ -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<string, unknown> = {},
): 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 <name>/);
// 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/);
});
});