From d083f74bece047579d816252c97de44a5c929d53 Mon Sep 17 00:00:00 2001 From: claude-hdb Date: Thu, 16 Jul 2026 16:37:32 +0000 Subject: [PATCH] fix(diff): ignore preview env rows so they cannot shadow production (#85) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /applications/{uuid}/envs` does not return one row per key: it merges the production vars with the PREVIEW ones into one flat array (`environment_variables->merge(environment_variables_preview)`, ApplicationsController@envs v4.1.2). The two relations are complements split on `is_preview`, with a unique index per (key, resource, is_preview) — so the same key legitimately arrives twice. `fetchEnv` keyed by `key` alone, and `Object.fromEntries` keeps the LAST, so cast diffed the manifest against whichever row Coolify happened to serialize last. Confirmed on prod: REPORTING_ENABLED came back as {value:"true", is_preview:false} AND {value:"false", is_preview:true}; cast read the "false" twin and re-proposed a `change` that could never clear. That is also why #78 looked like a stale read. Both rows are born equal (Coolify seeds a preview twin), and syncEnv only ever PATCHes the PRODUCTION row — so the two diverge for exactly the vars updated in place. Five prod flags flipped false->true re-proposed on every diff forever, while created-once vars stayed clean because their twins still agreed. Nothing was stale: cast was reading the other deployment's value. `real_value` tracked `value` on every row, exactly as the accessor predicts. cast declares PRODUCTION env and already says so on every write — syncEnv sends `is_preview: false` on each bulk upsert. This is the read finally saying the same thing; the asymmetry was the whole bug. Services and databases map a single set, so this is a no-op for them. Tests pin the exact prod shape, both serialization orders (the fix is "drop preview", not "take the first"), a preview-only key, and rows with no is_preview field at all. Co-Authored-By: Claude Opus 4.8 --- src/cli.ts | 38 ++++++++++++++++++-- test/live-lookup.test.ts | 75 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 3 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 2d7dc7b..a929cfc 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -875,13 +875,38 @@ export function aliasLive( // The diff picks between them per var (see LiveEnvVar, diffEnv); callers that // only need one flattened string use flattenEnv below. // +// PREVIEW ROWS ARE DROPPED, and that is load-bearing rather than tidy. +// +// GET /applications/{uuid}/envs does NOT return one row per key. It MERGES two +// parallel sets into one flat array — the production vars and the PREVIEW ones +// (`environment_variables->merge(environment_variables_preview)`, +// ApplicationsController@envs v4.1.2) — and the two relations are complements +// split on `is_preview`, with a unique index per (key, resource, is_preview). So +// the SAME KEY legitimately arrives TWICE, and keying by `key` alone kept +// whichever row Coolify happened to serialize last. +// +// That is #85, and it is why #78 looked like a "stale read": both rows are born +// equal (Coolify seeds a preview twin), and syncEnv below only ever PATCHes the +// PRODUCTION row — so the two diverge for exactly the vars that were updated in +// place. Five prod flags flipped false->true re-proposed as `change` on every +// diff forever, while created-once vars (NODE_ENV, …) stayed clean because their +// twins still agreed. Nothing was stale; cast was reading the other deployment's +// value. +// +// cast declares PRODUCTION env, and already says so on every WRITE — syncEnv +// sends `is_preview: false` on each bulk upsert. This is the read finally saying +// the same thing. A preview var is another deployment's value for the same name: +// not cast's to compare, and not cast's to write. Services and databases have no +// preview relation at all (their controllers map a single set), so this is a +// no-op for them. +// // A 404 (a resource we just listed no longer having an envs endpoint — not // expected in practice, but consistent with treating "gone" as "no env vars") // collapses to {}; anything else (401, 5xx, network) must surface. Swallowing // it would make a live resource's env look EMPTY, which turns every one of its // vars into a spurious create in a diff, and into a spurious "missing" in a // capture. -async function fetchEnv( +export async function fetchEnv( client: CoolifyClient, l: Pick, ): Promise> { @@ -889,9 +914,16 @@ async function fetchEnv( const envs = (await client.get(`/${base}/${l.uuid}/envs`).catch((err) => { if (err instanceof HttpError && err.status === 404) return []; throw err; - })) as Array<{ key: string; real_value?: string; value: string }>; + })) as Array<{ + key: string; + real_value?: string; + value: string; + is_preview?: boolean; + }>; return Object.fromEntries( - envs.map((e) => [e.key, { value: e.value, realValue: e.real_value }]), + envs + .filter((e) => e.is_preview !== true) + .map((e) => [e.key, { value: e.value, realValue: e.real_value }]), ); } diff --git a/test/live-lookup.test.ts b/test/live-lookup.test.ts index ce6872e..33c3311 100644 --- a/test/live-lookup.test.ts +++ b/test/live-lookup.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from "vitest"; import { attachBackup, attachServiceDomains, + fetchEnv, fetchLive, renderAbsentTarget, } from "../src/cli.js"; @@ -276,6 +277,80 @@ describe("attachBackup", () => { }); }); +describe("fetchEnv — preview rows never shadow production (cast#85)", () => { + const app = { kind: "application" as const, uuid: "a1" }; + const client = (body: unknown) => + new CoolifyClient( + "https://coolify.test", + "tok", + vi.fn( + async () => new Response(JSON.stringify(body), { status: 200 }), + ) as unknown as typeof fetch, + ); + + // The exact shape prod returned (#85): GET /applications/{uuid}/envs merges + // environment_variables with environment_variables_preview, so a key arrives + // TWICE — production first, its preview twin second. Keying by `key` alone + // kept the LAST, which is how five flipped prod flags re-proposed forever. + it("takes the production row even though the preview twin is serialized last", async () => { + const env = await fetchEnv( + client([ + { + key: "REPORTING_ENABLED", + value: "true", + real_value: "true", + is_preview: false, + }, + { + key: "REPORTING_ENABLED", + value: "false", + real_value: "false", + is_preview: true, + }, + ]), + app, + ); + expect(env.REPORTING_ENABLED).toEqual({ value: "true", realValue: "true" }); + }); + + // Order must not decide the answer: the fix is "drop preview", not "take the + // first" — a serialization order that put the twin first would otherwise just + // move the bug rather than remove it. + it("takes the production row even when the preview twin is serialized FIRST", async () => { + const env = await fetchEnv( + client([ + { key: "BRAIN_ENABLED", value: "false", is_preview: true }, + { key: "BRAIN_ENABLED", value: "true", is_preview: false }, + ]), + app, + ); + expect(env.BRAIN_ENABLED).toEqual({ value: "true", realValue: undefined }); + }); + + // A preview-only var is not production state at all. It must not surface as an + // orphan/remove-candidate either — cast neither compares nor writes it. + it("drops a key that exists ONLY as a preview var", async () => { + const env = await fetchEnv( + client([ + { key: "PORT", value: "3000", is_preview: false }, + { key: "PREVIEW_ONLY", value: "x", is_preview: true }, + ]), + app, + ); + expect(Object.keys(env)).toEqual(["PORT"]); + }); + + // Services and databases map a single set and their rows may carry no + // is_preview at all — absent must mean "keep", never "drop". + it("keeps rows with no is_preview field (services/databases)", async () => { + const env = await fetchEnv(client([{ key: "APP_SECRET", value: "s" }]), { + kind: "service", + uuid: "s1", + }); + expect(env.APP_SECRET).toEqual({ value: "s", realValue: undefined }); + }); +}); + describe("attachServiceDomains (cast#72)", () => { const svc = () => ({ kind: "service" as const,