From 1af5eeba0ab4c9a9ec1af57c20b7cd426f873a15 Mon Sep 17 00:00:00 2001 From: claude-hdb Date: Tue, 14 Jul 2026 17:25:29 +0000 Subject: [PATCH] fix: the first apply against a fresh multi-destination box (#40, #41) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both of these were found by the same run — the genuinely-from-nothing apply that #38 was also hiding in, against a box that shares its server with another project. Neither is a bug in what apply DOES; both are bugs in what it leaves behind and what it says. #40 — cast removes the default environment it made Coolify create. POST /projects hands a new project Coolify's OWN default environment, `production`. #39 taught apply to create the environment its resources actually name, so a project cast creates from nothing now ends up carrying two: ours, holding everything, and an empty `production` that nothing will ever use. That is precisely the shape that makes a box unreadable later, and we have the live example — on the box being migrated away from, `production` is empty and everything runs in `staging`, and "the obvious guess is the wrong one" is a note we had to write down for ourselves. Shipping more of those is not neutrality. This is the only delete cast performs, so it argues for itself against apply-never- deletes: what that rule protects is things cast did not make, and this is a byproduct of cast's own POST /projects seconds earlier, holding nothing and having never held anything. Three conditions, jointly, or nothing is touched — cast created the project in THIS run (never a project someone built by hand), the environment is EMPTY (asked of Coolify via the details route, the only one that eager-loads resources — not inferred from the first condition), and its name is NOT ours (an --environment production keeps its production, since that is where everything is about to live). Best-effort: a delete that fails is reported and never fails an apply that worked. #41 — the multi-destination 400 says what to do, and the plan says what it assumed. A create against a server with more than one destination that names none is rejected with "Server has multiple destinations and you do not set destination_uuid." — a message that names neither the remedy nor the file it goes in, arriving at the FIRST create, after apply has already made the project and the environment. cast cannot pre-flight it and that half is not fixable: 4.1.2 serves no destinations API at all, and GET /servers/{uuid} does not carry them either, so a server's destination COUNT is unknowable until a create has been attempted. The diagnosis is what is fixable. The 400 is now answered with the failing resource, the server by the name the operator wrote (not its UUID), the exact path the UUID goes in (environments..projects./.destination_uuid), the create-time warning — placement is repaired by delete + recreate, never by a later apply — and Coolify's own words kept verbatim, so the next person's search still works. And the assumption behind an undeclared destination is now on screen at the moment it is made: `placement: server's default destination (none declared)`. This reverses a judgment cast held explicitly ("a line on every diff that says nothing is how a report stops being read" — the test it replaces). The line does not say nothing; it says which network the next create lands on. It stays on a clean run that creates nothing, too, because the trap is set for projects that are already built: the day their server gains a second destination, every one of them that declared no destination stops being able to create, and nothing will have warned them. Co-Authored-By: Claude Opus 4.8 --- README.md | 51 +++++- docs/semantics.md | 42 ++++- src/cli.ts | 272 ++++++++++++++++++++++++-------- src/coolify.ts | 31 ++++ src/diff.ts | 23 +++ test/diff.test.ts | 35 ++++- test/placement-cli.test.ts | 17 +- test/wire.test.ts | 314 ++++++++++++++++++++++++++++++++++++- 8 files changed, 703 insertions(+), 82 deletions(-) diff --git a/README.md b/README.md index 4154af4..4fb46ae 100644 --- a/README.md +++ b/README.md @@ -77,9 +77,13 @@ cast team [--env ] - **`apply`** — idempotent create-or-update of every manifest resource, then redeploy what changed. One-way: it never deletes a resource that Coolify has - and the manifest doesn't. Clones the repo's default branch unless `--path` - points at a local checkout (refused with `--env prod` — prod always reads the - default branch). + and the manifest doesn't. It creates the **project** and its **environment** + when they are absent — the two things a resource create has to name — and then + removes the empty `production` that Coolify hands every new project, which is + the single delete cast performs and never touches a project built by hand + ([docs/semantics.md](docs/semantics.md)). Clones the repo's default branch + unless `--path` points at a local checkout (refused with `--env prod` — prod + always reads the default branch). - **`diff`** — reports drift, manifest → Coolify. Structural by default; `--full` also compares env vars. Exits non-zero when dirty, so CI can gate on it. - **`--all`** — on `apply`/`diff`, act on **every project the registry lists for @@ -677,12 +681,49 @@ split placement: these resources sit on 2 different destinations verify it. That is deliberate. A setting that reads back as *absent* rather than *wrong* is the failure this whole file keeps trying not to be. +When you declare **nothing**, every `diff` says that too: + +``` +placement: server's default destination (none declared) — cast sends no destination_uuid, + so Coolify picks; a server with more than one destination refuses the create outright. +``` + +Declaring nothing is not the absence of a placement decision. It is one, and it +used to be the only one cast made silently — the inference sat in a source comment +("the server's only destination, which is what Coolify picks anyway"), which is +exactly where an assumption is invisible until it is wrong. + One sharp edge worth knowing: on a server with exactly **one** destination, Coolify ignores the `destination_uuid` you send and never validates it — a typo there is invisible until a second destination exists. On a server with more than one, a create that omits it is a hard `400`, which is why cast could not deploy -onto a shared box at all until it could send this. Details, with citations: -[reference/README.md](reference/README.md). +onto a shared box at all until it could send this. The asymmetry hides itself: +the day a server gains its second destination, every project on it that declared +no destination stops being able to create. + +cast cannot warn you before that create — Coolify 4.1.2 serves no destinations +API, so a server's destination *count* is not knowable until a create has already +been attempted, and the 400 therefore lands **after** apply has made the project +and the environment. What cast does instead is answer it: + +``` +cannot create application core: prod-box has multiple destinations, so a create must say which one to use. + + Coolify said: POST /applications/private-github-app → 400: {"message":"Server has multiple destinations and you do not set destination_uuid."} + +Read the destination UUID from the Coolify UI (4.1.2 exposes no API for it) and +declare it as: + + environments.prod.projects.heavy-duty/incubator.destination_uuid + +Placement is create-time — a resource cannot be moved between networks later, so a +wrong or missing destination is repaired by delete + recreate, never by a later apply. + +Re-run this apply once the UUID is declared: anything it already created (the project, +its environment) is adopted, not made twice — apply reads before it writes. +``` + +Details, with citations: [reference/README.md](reference/README.md). ## Guarding an environment diff --git a/docs/semantics.md b/docs/semantics.md index 474528c..58cb803 100644 --- a/docs/semantics.md +++ b/docs/semantics.md @@ -152,9 +152,21 @@ softened by an implementation detail): and leaves the project behind, created and empty (#38). Read-before-write, so an environment that already exists is never written to: adoption keeps working exactly as it did, and this cannot regress an apply that works today. -- That default environment is **left alone**, per *apply never deletes*. An - empty `production` beside the environment everything lives in is reported - (the same courtesy an orphan gets) and removed by hand, or not at all. +- That default environment is then **removed** — the one delete cast performs, + and the only exception to *apply never deletes* (#40). What that rule protects + is things cast did not make; this is a byproduct of cast's own `POST /projects` + seconds earlier, holding nothing and having never held anything. Leaving it + meant every project cast created from nothing carried a permanently-empty + `production` beside the environment everything actually lives in — precisely + the shape that makes a box unreadable later (on the box being migrated away + from, `production` is empty and everything runs in `staging`, and *"the obvious + guess is the wrong one"* is a note we had to write for ourselves). All three + conditions hold jointly or nothing is touched: **cast created the project in + this run** (never a project someone built by hand, whatever it carries), the + environment is **empty** (asked of Coolify — the details route is the only one + that eager-loads resources — not assumed from the first condition), and its + name is **not ours** (an `--environment production` keeps its `production`). + Best-effort: a delete that fails is reported and never fails the apply. - On drift in a field the API cannot update in place (`build_pack`, a database's `type`/`version`, a service's `type`), apply **fails loudly naming the field** rather than recreating. @@ -325,6 +337,30 @@ anything else. Two consequences, both deliberate: - Whenever a destination is declared, `diff` says explicitly that it was *not* compared. Silence would make an unverified setting read as a verified one — the failure shape this document exists to avoid. +- Whenever one is **not** declared, `diff` says *that*, too: + `placement: server's default destination (none declared)`. Declaring nothing is + not the absence of a placement decision — it is one (cast sends no + `destination_uuid`, Coolify picks), and it was the only placement decision made + in silence until #41. It is reported even on a clean run that creates nothing, + because the trap is set precisely for projects that are already built: the day + their server gains a second destination, every project on it that declared no + destination stops being able to create at all. + +**The multi-destination 400 is translated, not passed through** (#41). A create +against a server with more than one destination that names none is rejected with +*"Server has multiple destinations and you do not set destination_uuid."* — a +message that names neither the remedy nor the file the remedy goes in, and that +arrives at the **first create**, after `apply` has already made the project and +the environment. cast **cannot** pre-flight the condition: 4.1.2 serves no +destinations API at all, and `GET /servers/{uuid}` does not carry them either, so +a server's destination count is unknowable until a create has been attempted. +What cast can do is answer the question the 400 raises, and it does: the failing +resource, the server by the name the operator wrote (not its UUID), the exact +state-file path the UUID goes in +(`environments..projects./.destination_uuid`), the create-time +warning, and Coolify's own words kept verbatim. A run interrupted this way is +safe to re-run once the UUID is declared — the project and environment it already +made are adopted, not remade. **Coolify's create-time behavior** (`ApplicationsController` ~L1003, `DatabasesController` ~L1700, `ServicesController` ~L378 @ v4.1.2): a server with diff --git a/src/cli.ts b/src/cli.ts index 2935d3a..04505d7 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -29,6 +29,7 @@ import { } from "./config.js"; import { CoolifyClient, HttpError } from "./coolify.js"; import { + type Change, type Live, type ResourceKind, computeDiff, @@ -811,6 +812,11 @@ async function runProject( envName: coolifyEnv, serverUuid, githubAppUuid, + // Not the wire names above (see buildExecutor): the names the operator wrote, + // and the state-file path a missing destination has to be declared at. + serverName: ctx.binding.server, + orgRepo, + bindingEnv: ctx.envName, destinationUuid: projectBinding?.destination_uuid, s3DestinationUuid: ctx.binding.s3_destination, backupSchedules, @@ -1704,11 +1710,35 @@ async function resolveOrCreateProject( // duplicate name), reached when something else wins the race between our read // and our write. // -// Coolify's own default environment is left exactly where it is: cast does not -// remove things (see renderDiff — an orphan is reported and NOT repaired, -// "removal is a manual runbook act"), and an empty `production` beside the -// environment everything lives in is the mildest possible case of that. It is -// reported for the same reason an orphan is: so the operator knows, and decides. +// Coolify's default environment is then REMOVED — the one delete cast performs, +// and the exception that has to argue for itself against *apply never deletes* +// (#40). +// +// What that rule protects is things cast did not make: a resource, an env var, a +// project someone built by hand. This is none of those. It is a byproduct of +// cast's own `POST /projects` seconds earlier, in this run, holding nothing and +// having never held anything — cast declining to leave litter behind itself. The +// alternative is what #39 shipped and #40 was filed against: every project cast +// creates from nothing carries a permanently-empty `production` beside the +// environment everything actually lives in, which is *precisely* the shape that +// makes a box unreadable later. We have the live example — on the box being +// migrated away from, `production` is empty and everything runs in `staging`, +// and "the obvious guess is the wrong one" is a note we had to write down for +// ourselves. Shipping more of those is not neutrality; it is a bug with a +// changelog entry. +// +// All three conditions are load-bearing, and removeDefaultEnvironment enforces +// them jointly: +// +// cast created the project, in THIS run — never touch a project someone built +// by hand, whatever it happens to carry. +// the environment is EMPTY — asked of Coolify, not assumed from the above. +// its name is NOT ours — a project whose --environment legitimately IS +// `production` keeps it (it is the one everything is about to live in). +// +// And it is best-effort: a delete that fails leaves the environment reported, +// exactly as #39 left it, and never fails an apply that has otherwise worked. +// Tidying is not worth a half-applied run. async function ensureEnvironment( client: CoolifyClient, projectUuid: string, @@ -1717,7 +1747,7 @@ async function ensureEnvironment( projectWasCreated: boolean, ): Promise { // The read that decides. On a project cast just created, it is also the list - // of environments Coolify gave it by itself — which is what `strays` reports. + // of environments Coolify gave it by itself — which is what `strays` holds. const existing = await client.environments(projectUuid); if (!existing.includes(envName)) { try { @@ -1728,10 +1758,39 @@ async function ensureEnvironment( if (!(err instanceof HttpError) || err.status !== 409) throw err; } } - const strays = projectWasCreated ? existing.filter((e) => e !== envName) : []; - if (strays.length > 0) { + if (!projectWasCreated) return; + for (const stray of existing.filter((e) => e !== envName)) { + await removeDefaultEnvironment(client, projectUuid, projectName, stray); + } +} + +// The delete itself, and the two ways it declines to happen. Nothing here throws: +// every path ends in a line of output, because the operator's project is either +// tidy or carrying an environment they now know about. +async function removeDefaultEnvironment( + client: CoolifyClient, + projectUuid: string, + projectName: string, + envName: string, +): Promise { + try { + // Asked, not inferred. It is empty by construction — Coolify made it a + // moment ago and only cast has written to this project since — but "it must + // be empty" is a belief, and this is a delete. The check costs one GET and + // is what makes the guarantee a fact rather than an argument. + if (!(await client.environmentIsEmpty(projectUuid, envName))) { + console.log( + `note: left Coolify's default environment ${envName} on new project ${projectName} — it is NOT empty (cast deletes nothing that holds anything)`, + ); + return; + } + await client.deleteEnvironment(projectUuid, envName); console.log( - `note: new project ${projectName} carries Coolify's default environment(s): ${strays.join(", ")} — empty, unused, and cast never removes (delete by hand if unwanted)`, + `removed Coolify's default environment ${envName} from new project ${projectName} (empty — created by Coolify's POST /projects, never by the manifest)`, + ); + } catch (err) { + console.log( + `note: new project ${projectName} carries Coolify's default environment ${envName} — empty and unused, and cast could not remove it (${err instanceof Error ? err.message : String(err)}). Delete it by hand, or leave it.`, ); } } @@ -1841,6 +1900,55 @@ export function serviceApiFields( return rest; } +// Coolify's answer when a server has more than one destination and the create did +// not say which one to use (all three controllers, identically, v4.1.2): +// +// POST /applications/private-github-app → 400: +// {"message":"Server has multiple destinations and you do not set destination_uuid."} +// +// It names neither the remedy nor the file the remedy goes in, and it arrives at +// the FIRST create — after apply has already made the project and the environment. +// So the operator is holding a half-applied run and a message about a field they +// may never have heard of. (#41) +// +// cast cannot pre-flight this and that part is not fixable here: 4.1.2 serves no +// destinations API at all — not list, not read, not create — and GET /servers/{uuid} +// does not carry them either, so a server's destination COUNT is not knowable until +// a create has already been attempted. What IS fixable is the diagnosis, and this is +// the whole of it: catch the one message, and answer the question it raises. +function isMultiDestination400(err: unknown): err is HttpError { + return ( + err instanceof HttpError && + err.status === 400 && + err.message.includes("Server has multiple destinations") + ); +} + +export function multiDestinationRemedy(where: { + server: string; + env: string; + project: string; + resource: string; + coolify: string; +}): string { + return [ + `cannot create ${where.resource}: ${where.server} has multiple destinations, so a create must say which one to use.`, + "", + ` Coolify said: ${where.coolify}`, + "", + "Read the destination UUID from the Coolify UI (4.1.2 exposes no API for it) and", + "declare it as:", + "", + ` environments.${where.env}.projects.${where.project}.destination_uuid`, + "", + "Placement is create-time — a resource cannot be moved between networks later, so a", + "wrong or missing destination is repaired by delete + recreate, never by a later apply.", + "", + "Re-run this apply once the UUID is declared: anything it already created (the project,", + "its environment) is adopted, not made twice — apply reads before it writes.", + ].join("\n"); +} + export function buildExecutor( client: CoolifyClient, ctx: { @@ -1848,6 +1956,16 @@ export function buildExecutor( envName: string; serverUuid: string; githubAppUuid: string; + // The three names the multi-destination 400 has to be able to say back, and + // the only reason they are here: none of them is on the wire. A create sends + // `serverUuid`, but the operator wrote a server NAME — and the UUID they now + // have to go and read lands at `environments..projects./`, a + // path keyed by cast's OWN env name and the repo, never by the Coolify + // project/environment names above (which `--project`/`--environment` are free + // to make something else entirely). + serverName: string; + orgRepo: string; + bindingEnv: string; // The Docker network to create resources on. A raw UUID from // environments.yaml for the same reason s3DestinationUuid is one: Coolify // 4.1.2 has no destinations API, so there is no name for cast to resolve. @@ -1888,70 +2006,96 @@ export function buildExecutor( ctx.projectName, ctx.envName, ); + // Wrapped around all three creates rather than around each one: Coolify runs + // the same destination logic in ApplicationsController, DatabasesController and + // ServicesController, so whichever kind happens to be created first is the one + // that 400s, and which one that is depends only on the order of the manifest. + const withDestinationDiagnosis = async ( + change: Change, + create: () => Promise, + ): Promise => { + try { + return await create(); + } catch (err) { + if (!isMultiDestination400(err)) throw err; + throw new Error( + multiDestinationRemedy({ + server: ctx.serverName, + env: ctx.bindingEnv, + project: ctx.orgRepo, + resource: `${change.kind} ${change.name}`, + coolify: err.message, + }), + { cause: err }, + ); + } + }; return { async createResource(change) { - // Field payloads assembled from change.fieldDiffs (desired values): - const fields = Object.fromEntries( - change.fieldDiffs.map((f) => [f.field, f.desired]), - ); - const projectUuid = await projectEnv(); - if (change.kind === "application") { - const res = (await client.post("/applications/private-github-app", { - project_uuid: projectUuid, - environment_name: ctx.envName, - server_uuid: ctx.serverUuid, - ...destination, - github_app_uuid: ctx.githubAppUuid, - name: change.name, - instant_deploy: false, - ...applicationApiFields(fields), - // A compose stack must reach the managed Postgres/Redis resources - // (the box-B lesson, DEPLOY.md §0/§3) — Coolify only wires that up - // when this flag is set on create. - ...(fields.build_pack === "dockercompose" - ? { connect_to_docker_network: true } - : {}), - })) as { uuid: string }; - return res.uuid; - } - if (change.kind === "database") { - const type = String(fields.type); - const res = (await client.post( - `/databases/${type === "postgresql" ? "postgresql" : "redis"}`, - { + return withDestinationDiagnosis(change, async () => { + // Field payloads assembled from change.fieldDiffs (desired values): + const fields = Object.fromEntries( + change.fieldDiffs.map((f) => [f.field, f.desired]), + ); + const projectUuid = await projectEnv(); + if (change.kind === "application") { + const res = (await client.post("/applications/private-github-app", { project_uuid: projectUuid, environment_name: ctx.envName, server_uuid: ctx.serverUuid, ...destination, + github_app_uuid: ctx.githubAppUuid, name: change.name, - ...databaseApiFields(fields), - }, - )) as { uuid: string }; - const schedule = ctx.backupSchedules[change.name]; - if (schedule) { - if (!ctx.s3DestinationUuid) { - throw new Error( - `database ${change.name} declares a backup schedule but environments.yaml has no s3_destination UUID for this environment`, - ); - } - await client.post(`/databases/${res.uuid}/backups`, { - frequency: schedule.frequency, - database_backup_retention_amount_locally: schedule.retention, - save_s3: true, - s3_storage_uuid: ctx.s3DestinationUuid, - }); + instant_deploy: false, + ...applicationApiFields(fields), + // A compose stack must reach the managed Postgres/Redis resources + // (the box-B lesson, DEPLOY.md §0/§3) — Coolify only wires that up + // when this flag is set on create. + ...(fields.build_pack === "dockercompose" + ? { connect_to_docker_network: true } + : {}), + })) as { uuid: string }; + return res.uuid; } + if (change.kind === "database") { + const type = String(fields.type); + const res = (await client.post( + `/databases/${type === "postgresql" ? "postgresql" : "redis"}`, + { + project_uuid: projectUuid, + environment_name: ctx.envName, + server_uuid: ctx.serverUuid, + ...destination, + name: change.name, + ...databaseApiFields(fields), + }, + )) as { uuid: string }; + const schedule = ctx.backupSchedules[change.name]; + if (schedule) { + if (!ctx.s3DestinationUuid) { + throw new Error( + `database ${change.name} declares a backup schedule but environments.yaml has no s3_destination UUID for this environment`, + ); + } + await client.post(`/databases/${res.uuid}/backups`, { + frequency: schedule.frequency, + database_backup_retention_amount_locally: schedule.retention, + save_s3: true, + s3_storage_uuid: ctx.s3DestinationUuid, + }); + } + return res.uuid; + } + const res = (await client.post("/services", { + project_uuid: projectUuid, + environment_name: ctx.envName, + server_uuid: ctx.serverUuid, + ...destination, + name: change.name, + ...serviceApiFields(fields), + })) as { uuid: string }; return res.uuid; - } - const res = (await client.post("/services", { - project_uuid: projectUuid, - environment_name: ctx.envName, - server_uuid: ctx.serverUuid, - ...destination, - name: change.name, - ...serviceApiFields(fields), - })) as { uuid: string }; - return res.uuid; + }); }, async updateFields(uuid, kind, fields) { const base = diff --git a/src/coolify.ts b/src/coolify.ts index 364d94b..cae1bf0 100644 --- a/src/coolify.ts +++ b/src/coolify.ts @@ -147,6 +147,37 @@ export class CoolifyClient { return names(project?.environments); } + // Does this environment hold anything at all? + // + // The LIST route above cannot answer it: an `Environment` carries id, name, + // project_id, description, timestamps — no relations, so an environment with + // five applications in it looks exactly like an empty one. This route is the + // one that eager-loads them (ProjectController@environment_details, v4.1.2: + // applications, postgresqls, redis, mongodbs, mysqls, mariadbs, services). + // + // The question is asked of the SHAPE rather than of those seven names: any + // non-empty array in the response is a resource list, because everything else + // there is a scalar. Naming the seven instead would mean a Coolify that grows + // an eighth database type could answer "empty" about an environment holding + // one — and this answer is the guard on a delete. + async environmentIsEmpty( + projectUuid: string, + envName: string, + ): Promise { + const env = (await this.get( + `/projects/${projectUuid}/${encodeURIComponent(envName)}`, + )) as Record | null; + // Not "empty" — unreadable. The caller must not delete on this answer. + if (!env) return false; + return !Object.values(env).some((v) => Array.isArray(v) && v.length > 0); + } + + async deleteEnvironment(projectUuid: string, envName: string): Promise { + await this.delete_( + `/projects/${projectUuid}/environments/${encodeURIComponent(envName)}`, + ); + } + async deploy(uuid: string): Promise { await this.post(`/deploy?uuid=${encodeURIComponent(uuid)}`); } diff --git a/src/diff.ts b/src/diff.ts index 04fee78..2555bb4 100644 --- a/src/diff.ts +++ b/src/diff.ts @@ -241,6 +241,29 @@ export function renderDiff(report: DiffReport): string { " destination_uuid on write and returns destination_id on read, and has no endpoint", " mapping one to the other. cast sends it on create; nothing can verify it after.", ); + } else { + // The other half of the same principle, and #41: declaring NOTHING is also a + // decision about placement — cast sends no destination_uuid and lets Coolify + // pick — and it was the one placement decision made in silence. The inference + // lived in a source comment ("the server's only destination, which is what + // Coolify picks anyway"), which is exactly where an assumption is invisible + // until it is wrong. + // + // This reverses a judgment cast used to hold explicitly ("a line on every diff + // that says nothing is how a report stops being read" — the test this replaces). + // The line does not say nothing: it says which network the next create lands on, + // which is a fact about this run and a wrong one to have to infer from a blank + // space. It stays on a run that creates nothing, too, because the trap is set + // precisely for projects that are already built and clean — the day their server + // gains a second destination, every one of them that declared no destination + // stops being able to create at all, and nothing will have warned them. + // + // Two lines, not three: the old judgment was not wrong about noise, only about + // which side of it silence was on. + lines.push( + "placement: server's default destination (none declared) — cast sends no destination_uuid,", + " so Coolify picks; a server with more than one destination refuses the create outright.", + ); } lines.push( report.clean diff --git a/test/diff.test.ts b/test/diff.test.ts index 5534626..d0bf930 100644 --- a/test/diff.test.ts +++ b/test/diff.test.ts @@ -186,15 +186,44 @@ describe("renderDiff placement", () => { // The whole reason placement is in the report at all: a destination that read // back as absent rather than wrong is the failure shape #12/#14/#17/#18 are - // about. Silence is the bug — but so is noise on the happy path. - it("stays silent about placement when nothing is declared and nothing is split", () => { + // about. + // + // This test used to assert the opposite — that an undeclared, unsplit box says + // NOTHING about placement, on the grounds that a line on every diff is how a + // report stops being read. #41 reversed it. Declaring nothing is not the absence + // of a placement decision, it is a placement decision: cast sends no + // destination_uuid and Coolify picks. Leaving that inference in a source comment + // is what made it invisible until the day it was wrong — a first apply against a + // multi-destination server, 400ing after the run had already created the project + // and the environment. It is a fact about what the next create will do, so it is + // on screen while it is still true. + it("says out loud that an undeclared placement is the server's default", () => { const out = renderDiff( computeDiff([want("a")], [got("a", 3)], "structural"), ); - expect(out).not.toMatch(/placement/); + expect(out).toMatch( + /placement: server's default destination \(none declared\)/, + ); + // ...and the consequence, which is the only part that can hurt: on a server + // with more than one destination this is not a default, it is a 400. + expect(out).toMatch(/refuses the create/); + // Still clean: an undeclared destination is an assumption, not drift. expect(out).toContain("clean"); }); + // The claim above is about the UNDECLARED case only. A declared destination + // makes the opposite statement (it was sent, and cannot be verified) and must + // never make both. + it("does not call a declared destination the server's default", () => { + const out = renderDiff( + computeDiff([want("a")], [got("a", 3)], "structural", { + declaredDestination: "dest-abc", + }), + ); + expect(out).toMatch(/NOT compared/); + expect(out).not.toMatch(/none declared/); + }); + it("names every resource on each side of a split", () => { const out = renderDiff( computeDiff( diff --git a/test/placement-cli.test.ts b/test/placement-cli.test.ts index f20d6b6..15f44af 100644 --- a/test/placement-cli.test.ts +++ b/test/placement-cli.test.ts @@ -219,14 +219,21 @@ describe("cast diff — placement (#21)", () => { expect(r.output).not.toMatch(/NOT compared/); }); - // The state of every box today: one project, one server, one network, nothing - // declared. Placement must be silent — a line on every diff that says nothing - // is how a report stops being read. - it("says nothing at all about placement on an undeclared, unsplit box", async () => { + // The state of most boxes today: one project, one server, one network, nothing + // declared. This used to assert silence. #41 made it speak: an undeclared + // destination is cast choosing to let Coolify pick, and a run says which network + // its creates land on rather than leaving it to be inferred from a blank space. + // The clean exit is the part that must not move — an assumption is not drift. + it("names the assumption on an undeclared, unsplit box, and stays clean", async () => { const f = fixture((await stubCoolify({ core: 5, landing: 5 })).url); const r = await run(base(f)); expect(r.code).toBe(0); - expect(r.output).not.toMatch(/placement/); + expect(r.output).toMatch( + /placement: server's default destination \(none declared\)/, + ); expect(r.output).toContain("clean"); + // The declared-destination warning is the other branch, and belongs to a run + // that declared one. Saying both would be saying nothing. + expect(r.output).not.toMatch(/NOT compared/); }); }); diff --git a/test/wire.test.ts b/test/wire.test.ts index e7283cf..92ad045 100644 --- a/test/wire.test.ts +++ b/test/wire.test.ts @@ -195,6 +195,9 @@ describe("buildExecutor createResource (application, dockercompose)", () => { envName: "prod", serverUuid: "srv-1", githubAppUuid: "gh-1", + serverName: "prod-box", + orgRepo: "acme/widget", + bindingEnv: "prod", backupSchedules: {}, }); const uuid = await exec.createResource({ @@ -249,6 +252,9 @@ describe("buildExecutor createResource (application, dockercompose)", () => { envName: "prod", serverUuid: "srv-1", githubAppUuid: "gh-1", + serverName: "prod-box", + orgRepo: "acme/widget", + bindingEnv: "prod", backupSchedules: {}, }); await exec.createResource({ @@ -351,6 +357,9 @@ describe("buildExecutor createResource (destination placement)", () => { envName: "prod", serverUuid: "srv-1", githubAppUuid: "gh-1", + serverName: "prod-box", + orgRepo: "acme/widget", + bindingEnv: "prod", destinationUuid: "dest-abc", backupSchedules: {}, }); @@ -378,6 +387,9 @@ describe("buildExecutor createResource (destination placement)", () => { envName: "prod", serverUuid: "srv-1", githubAppUuid: "gh-1", + serverName: "prod-box", + orgRepo: "acme/widget", + bindingEnv: "prod", backupSchedules: {}, }); await exec.createResource(change); @@ -401,9 +413,16 @@ describe("buildExecutor createResource (environment reconcile)", () => { projects?: Array<{ uuid: string; name: string }>; environments?: string[]; envCreateStatus?: number; + // What an environment HOLDS, by name — the guard on #40's delete. A project + // cast just created cannot really hold anything, which is exactly why the + // mock has to be able to say otherwise: the guard is worth only as much as + // the case it refuses. + holds?: Record; + envDeleteStatus?: number; }) { const projects = opts.projects ?? []; let environments = opts.environments ?? []; + const holds = opts.holds ?? {}; const calls: string[] = []; const fetchImpl = vi.fn(async (url: string | URL, init?: RequestInit) => { const path = new URL(String(url)).pathname; @@ -458,6 +477,46 @@ describe("buildExecutor createResource (environment reconcile)", () => { environments.push(name); return new Response(JSON.stringify({ uuid: "env-1" }), { status: 201 }); } + // DELETE /projects/{uuid}/environments/{name} — #40. Three segments, so the + // two-segment details route below cannot swallow it. + const envDelete = + /^\/api\/v1\/projects\/([^/]+)\/environments\/([^/]+)$/.exec(path); + if (envDelete && method === "DELETE") { + const status = opts.envDeleteStatus ?? 200; + if (status !== 200) + return new Response(JSON.stringify({ message: "boom" }), { status }); + environments = environments.filter((e) => e !== envDelete[2]); + return new Response( + JSON.stringify({ message: "Environment deleted." }), + { status: 200 }, + ); + } + // GET /projects/{uuid}/{environment} — the details route, the ONLY one that + // eager-loads an environment's resources, and so the only one that can + // answer "is it empty?". The list route's Environment objects carry no + // relations at all. Checked after the /environments routes above, which + // this pattern would otherwise match. + const envDetail = /^\/api\/v1\/projects\/([^/]+)\/([^/]+)$/.exec(path); + if (envDetail && method === "GET") { + const name = envDetail[2]; + if (!environments.includes(name)) + return new Response(JSON.stringify({ message: "Not found." }), { + status: 404, + }); + return new Response( + JSON.stringify({ + id: 1, + name, + project_id: 1, + description: "", + applications: holds[name] ?? [], + postgresqls: [], + redis: [], + services: [], + }), + { status: 200 }, + ); + } if (path === "/api/v1/applications/private-github-app") { // Coolify's actual rule, and the whole of #38: a create names an // environment, and an environment that is not there is a 404. Without @@ -488,14 +547,21 @@ describe("buildExecutor createResource (environment reconcile)", () => { envDiffs: [], }; - function exec(fetchImpl: typeof fetch) { + // envName is a parameter because #40's third guard is about it: an environment + // named `production` is Coolify's leftover default in every run EXCEPT the one + // that asked for `--environment production`, where it is the environment + // everything is about to live in. + function exec(fetchImpl: typeof fetch, envName = "prod") { return buildExecutor( new CoolifyClient("https://coolify.test", "tok", fetchImpl), { projectName: "widget", - envName: "prod", + envName, serverUuid: "srv-1", githubAppUuid: "gh-1", + serverName: "prod-box", + orgRepo: "acme/widget", + bindingEnv: "prod", backupSchedules: {}, }, ); @@ -575,6 +641,250 @@ describe("buildExecutor createResource (environment reconcile)", () => { coolify.calls.filter((c) => c === "POST /api/v1/projects").length, ).toBe(1); }); + + // #40: what #39 left behind. POST /projects hands the new project Coolify's own + // default environment, and #39 then created OURS beside it — so every project cast + // creates from nothing carried a permanently-empty `production` next to the + // environment everything actually lives in. That is the shape that makes a box + // unreadable later; the box being migrated away from has an empty `production` and + // runs everything in `staging`, and "the obvious guess is the wrong one" is a note + // we had to write for ourselves. + // + // This is also the ONE delete cast performs, so the guards are the test: it happens + // only to a project cast made in this run, only to an environment that is empty, and + // only when the name is not the one we asked for. + describe("buildExecutor createResource (default environment removal, #40)", () => { + const DELETE_DEFAULT = + "DELETE /api/v1/projects/proj-new/environments/production"; + + it("removes the empty default environment from a project it just created", async () => { + const coolify = fakeCoolify({ projects: [] }); + const notes = vi.spyOn(console, "log").mockImplementation(() => {}); + + const uuid = await exec(coolify.fetchImpl).createResource(app); + + expect(uuid).toBe("app-1"); + expect(coolify.calls).toContain(DELETE_DEFAULT); + // The point of the whole issue: what is left is ours, and only ours. + expect(coolify.environments()).toEqual(["prod"]); + expect(notes.mock.calls.flat().join("\n")).toMatch( + /removed Coolify's default environment production/, + ); + notes.mockRestore(); + }); + + // Guard 1. The rule cast does not get to break: a project someone built by hand + // is not cast's to tidy, whatever it happens to carry. `production` sitting empty + // next to `staging` on an ADOPTED project is exactly the live box we migrate from + // — and it stays untouched. + it("never removes an environment from a project it adopted", async () => { + const coolify = fakeCoolify({ + projects: [{ uuid: "proj-1", name: "widget" }], + environments: ["production", "prod"], + }); + + await exec(coolify.fetchImpl).createResource(app); + + expect(coolify.calls.some((c) => c.startsWith("DELETE"))).toBe(false); + expect(coolify.environments()).toContain("production"); + }); + + // Guard 2. Asked of Coolify, not inferred from "we just made this project". The + // mock is lying here — a fresh project cannot hold an application — and it must + // be able to, because a guard that is only ever handed the safe case is not a + // guard. cast declines, and says why. + it("leaves the default environment alone when it holds anything", async () => { + const coolify = fakeCoolify({ + projects: [], + holds: { production: [{ uuid: "app-x", name: "legacy" }] }, + }); + const notes = vi.spyOn(console, "log").mockImplementation(() => {}); + + await exec(coolify.fetchImpl).createResource(app); + + expect(coolify.calls.some((c) => c.startsWith("DELETE"))).toBe(false); + expect(coolify.environments()).toContain("production"); + expect(notes.mock.calls.flat().join("\n")).toMatch( + /left Coolify's default environment production .* NOT empty/, + ); + notes.mockRestore(); + }); + + // Guard 3. `production` is a leftover in every run except the one that asked for + // it, where it is the environment everything is about to live in. Deleting it + // there would delete the target of the very apply doing the deleting. + it("keeps the default environment when it is the one we asked for", async () => { + const coolify = fakeCoolify({ projects: [] }); + + await exec(coolify.fetchImpl, "production").createResource(app); + + expect(coolify.calls.some((c) => c.startsWith("DELETE"))).toBe(false); + expect(coolify.environments()).toEqual(["production"]); + // ...and it was never re-created either: it was already there. + expect(coolify.calls).not.toContain( + "POST /api/v1/projects/proj-new/environments", + ); + }); + + // Tidying is a courtesy, and a courtesy that can fail an apply is not one. The + // resource still gets created; the operator is told what was left behind. + it("does not fail the apply when the delete fails", async () => { + const coolify = fakeCoolify({ projects: [], envDeleteStatus: 500 }); + const notes = vi.spyOn(console, "log").mockImplementation(() => {}); + + const uuid = await exec(coolify.fetchImpl).createResource(app); + + expect(uuid).toBe("app-1"); + expect(coolify.environments()).toContain("production"); + expect(notes.mock.calls.flat().join("\n")).toMatch( + /could not remove it .*500/s, + ); + notes.mockRestore(); + }); + }); +}); + +// #41: a first apply against a server with more than one destination 400s on the +// first create — with the project and the environment already made. Coolify's own +// message names neither the remedy nor the file it goes in, and cast cannot +// pre-flight the condition (4.1.2 serves no destinations API at all, so a server's +// destination count is unknowable until a create has been attempted). The diagnosis +// is the whole of what is fixable, so the diagnosis is what is tested. +describe("buildExecutor createResource (multi-destination 400, #41)", () => { + const app = { + kind: "application" as const, + name: "core", + op: "create" as const, + fieldDiffs: [ + { field: "build_pack", desired: "nixpacks", updatable: false }, + ], + envDiffs: [], + }; + + // Coolify's real answer, verbatim, from all three create controllers. + function multiDestinationCoolify() { + return vi.fn(async (url: string | URL, init?: RequestInit) => { + const path = new URL(String(url)).pathname; + const method = init?.method ?? "GET"; + if (path === "/api/v1/projects" && method === "GET") + return new Response( + JSON.stringify([{ uuid: "proj-1", name: "widget" }]), + { status: 200 }, + ); + if (path === "/api/v1/projects/proj-1/environments" && method === "GET") + return new Response(JSON.stringify([{ name: "prod" }]), { + status: 200, + }); + return new Response( + JSON.stringify({ + message: + "Server has multiple destinations and you do not set destination_uuid.", + }), + { status: 400 }, + ); + }) as unknown as typeof fetch; + } + + const exec = (fetchImpl: typeof fetch) => + buildExecutor(new CoolifyClient("https://coolify.test", "tok", fetchImpl), { + projectName: "widget", + envName: "prod", + serverUuid: "srv-1", + githubAppUuid: "gh-1", + // The names the message has to be able to say back. None is on the wire: + // Coolify knows srv-1, the operator wrote prod-box. + serverName: "prod-box", + orgRepo: "heavy-duty/incubator", + bindingEnv: "prod", + backupSchedules: {}, + }); + + const kinds = [ + { label: "application", change: app }, + { + label: "database", + change: { + kind: "database" as const, + name: "postgres", + op: "create" as const, + fieldDiffs: [ + { field: "type", desired: "postgresql", updatable: false }, + ], + envDiffs: [], + }, + }, + { + label: "service", + change: { + kind: "service" as const, + name: "umami", + op: "create" as const, + fieldDiffs: [{ field: "type", desired: "umami", updatable: false }], + envDiffs: [], + }, + }, + ]; + + // Every kind, because which one 400s first depends only on the order of the + // manifest — Coolify runs the same destination logic in all three controllers. + it.each(kinds)( + "answers the $label 400 with the state-file path the UUID goes in", + async ({ change }) => { + const err = await exec(multiDestinationCoolify()) + .createResource(change) + .catch((e: Error) => e); + + expect(err).toBeInstanceOf(Error); + const message = (err as Error).message; + // The remedy: the exact key, in the exact place, with the repo and the env + // the operator actually named. + expect(message).toContain( + "environments.prod.projects.heavy-duty/incubator.destination_uuid", + ); + // The server, by the name the operator wrote — not srv-1. + expect(message).toContain("prod-box has multiple destinations"); + // Which resource it died on, so a half-applied run can be read. + expect(message).toContain(`cannot create ${change.kind} ${change.name}`); + // Create-time: the part that decides whether they can fix this with an + // apply (they cannot) or a delete + recreate (they must). + expect(message).toMatch(/cannot be moved between networks later/); + // Coolify's own words survive — a translation that hides the original + // makes the next person's search fail. + expect(message).toContain( + "Server has multiple destinations and you do not set destination_uuid.", + ); + }, + ); + + // The translation must be about THIS 400, not about 400s. A create rejected for + // any other reason has to arrive unmolested, or the next bug gets a confident + // answer about a destination it has nothing to do with. + it("leaves every other failure exactly as Coolify sent it", async () => { + const fetchImpl = vi.fn(async (url: string | URL, init?: RequestInit) => { + const path = new URL(String(url)).pathname; + const method = init?.method ?? "GET"; + if (path === "/api/v1/projects" && method === "GET") + return new Response( + JSON.stringify([{ uuid: "proj-1", name: "widget" }]), + { status: 200 }, + ); + if (path === "/api/v1/projects/proj-1/environments" && method === "GET") + return new Response(JSON.stringify([{ name: "prod" }]), { + status: 200, + }); + return new Response( + JSON.stringify({ message: "The name field is required." }), + { status: 422 }, + ); + }) as unknown as typeof fetch; + + const err = await exec(fetchImpl) + .createResource(app) + .catch((e: Error) => e); + + expect((err as Error).message).toContain("The name field is required."); + expect((err as Error).message).not.toMatch(/destination/); + }); }); describe("databaseVersionFromImage / defaultDatabaseImage", () => {