fix(services): merge environment on partial service update instead of replacing

apps/api/src/lib/secret-env.ts

@@ -62,6 +62,12 @@ function maskValue(value: string): string {
  * Keys absent from `incoming` are absent from the result: a full-map write still
  * deletes keys the client removed. Callers that only want partial semantics must
  * pre-merge (this is a whole-map replace with sentinel protection).
+ *
+ * This is the right helper when the caller genuinely owns the WHOLE set — create,
+ * compose sync, and the migration/deploy paths, which each rebuild the map from an
+ * upstream spec and must be able to drop a variable the spec no longer declares.
+ * The PATCH endpoint wants `mergeServiceEnv` below instead: a partial body there
+ * would otherwise delete every variable it merely failed to mention.
  */
 export function unmaskEnv(
   incoming: Record<string, string> | null | undefined,
@@ -81,6 +87,48 @@ export function unmaskEnv(
   return out;
 }
 
+/**
+ * Merge an incoming compose-service `environment` patch onto what's stored,
+ * preserving untouched variables and restoring masked secrets (#336, #619).
+ *
+ * `environment` is the only field on the PATCH endpoint that is masked on read,
+ * and reveal is deliberately off the automation surface — so a client cannot see
+ * what a whole-map replace is about to destroy, and cannot read it back. Omission
+ * therefore has to mean "keep", and removal has to be explicit. Same triad as
+ * `mergeAdvanced`, plus the sentinel arm that only a masked field needs:
+ *
+ *   - key absent from patch  → leave the stored value alone
+ *   - key value === null     → remove it
+ *   - key value === ENV_MASK → keep the stored value (dropped if not in stored)
+ *   - key value === string   → update/insert that key
+ *   - incoming === null      → clear the entire environment map
+ *   - incoming === undefined → leave the stored map alone
+ */
+export function mergeServiceEnv(
+  stored: Record<string, string> | null | undefined,
+  incoming: Record<string, string | null | undefined> | null | undefined,
+): Record<string, string> {
+  if (incoming === null) return {};
+  if (incoming === undefined || typeof incoming !== "object" || Array.isArray(incoming)) {
+    return { ...(stored ?? {}) };
+  }
+  const base: Record<string, string> = { ...(stored ?? {}) };
+  for (const [key, value] of Object.entries(incoming)) {
+    if (value === null) {
+      delete base[key];
+    } else if (isMaskedValue(value)) {
+      if (stored && Object.hasOwn(stored, key)) {
+        base[key] = stored[key];
+      } else {
+        delete base[key];
+      }
+    } else if (typeof value === "string") {
+      base[key] = value;
+    }
+  }
+  return base;
+}
+
 /** Whether an env map contains any mask sentinel (i.e. an un-revealed value). */
 export function hasMaskedValue(
   env: Record<string, string> | null | undefined,

apps/api/src/modules/services/service.routes.ts

@@ -141,7 +141,10 @@ r.patch(
   {
     tag: "project:service:write",
     body: UpdateServiceBody,
-    mcp: { description: "Update a service's configuration." },
+    mcp: {
+      description:
+        "Update a service's configuration. Partial: an omitted field is left alone. `environment` and `advanced` are MERGED onto the stored values rather than replacing them — omit a key to keep it, set a key to null to remove it, send null for the whole field to clear it. Env values read back masked as `••••••••`; echo the sentinel to keep one unchanged. Every other field (`ports`, `volumes`, `dependsOn`, `publicEndpoints`, …) REPLACES its stored value wholesale, so send the complete list.",
+    },
   },
   cloudProjectProxy,
   ctrl.update,

apps/api/src/modules/services/service.schema.ts

@@ -234,6 +234,25 @@ export const UpdateServiceBody = Type.Object(
     // spelling of absent.
     domain: Type.Optional(Type.Union([Type.String({ maxLength: 255 }), Type.Null()])),
     customDomain: Type.Optional(Type.Union([Type.String({ maxLength: 255 }), Type.Null()])),
+    // #619: same reasoning as the two above, and for the same reason as `advanced`
+    // — a keyed map on a PATCH has to be able to say "keep" and "clear" separately.
+    // It matters more here than anywhere else on this endpoint: `environment` is the
+    // only field MASKED on read, and reveal is off the automation surface, so a
+    // client that replaced the whole map could neither see nor recover what it
+    // destroyed. Nullable ONLY on update — Create/Sync own the whole set and keep
+    // `Record<string,string>` (the ratchet in test/modules/services/service-schema-env
+    // pins that asymmetry). The value type stays UNBOUNDED to match them: capping it
+    // here alone would make a long value (CA chain, base64 kubeconfig) creatable and
+    // then un-PATCH-able.
+    environment: Type.Optional(
+      Type.Union(
+        [Type.Record(Type.String(), Type.Union([Type.String(), Type.Null()])), Type.Null()],
+        {
+          description:
+            "MERGED onto the stored map, not replaced: omit a key to keep it, set a key to null to remove it, send null to clear every variable. An empty object changes nothing. Echo the •••••••• sentinel to keep a masked value unchanged.",
+        },
+      ),
+    ),
   },
   { additionalProperties: false },
 );

apps/api/src/modules/services/service.service.ts

@@ -15,7 +15,7 @@ import {
 import { scopedVolumeName, type CommandExecutor } from "@repo/adapters";
 import { execInContainer } from "../../lib/agent-exec";
 import { encrypt, decrypt } from "../../lib/encryption";
-import { ENV_MASK, hasMaskedValue, maskDriftChanges, maskServiceEnv, unmaskEnv } from "../../lib/secret-env";
+import { ENV_MASK, hasMaskedValue, maskDriftChanges, maskServiceEnv, mergeServiceEnv, unmaskEnv } from "../../lib/secret-env";
 import {
   assertNotControlPlane,
   assertNotControlPlaneById,
@@ -694,13 +694,17 @@ export async function updateService(
   // When domainType changes, clear the irrelevant domain field.
   const patch: Record<string, any> = { ...data };
 
-  // #336: env values are masked on read. Restore any sentinel the client echoed
-  // back to the stored value so editing an unrelated field never overwrites a
-  // secret with "••••••••". A real (revealed-and-changed) value passes through.
+  // #336/#619: env values are masked on read, so a client cannot see what it is
+  // about to overwrite. Merge onto what's stored rather than replacing: a partial
+  // body used to delete every variable it merely failed to mention, and because
+  // reveal is off the automation surface (see service.routes.ts) the caller could
+  // not read those values back. Sentinels the client echoed restore to the stored
+  // value, so editing an unrelated field never writes "••••••••" over a secret; a
+  // revealed-and-changed value passes through; an explicit null removes the key.
   if ("environment" in patch) {
-    patch.environment = unmaskEnv(
-      patch.environment,
+    patch.environment = mergeServiceEnv(
       svc.environment as Record<string, string> | null,
+      patch.environment,
     );
   }
 

apps/api/test/lib/secret-env.test.ts

@@ -10,6 +10,7 @@ import {
   maskScanService,
   maskServiceEnv,
   maskServicesEnv,
+  mergeServiceEnv,
   unmaskEnv,
 } from "../../src/lib/secret-env";
 
@@ -69,6 +70,73 @@ describe("unmaskEnv", () => {
   });
 });
 
+/**
+ * The PATCH counterpart (#619). `unmaskEnv` above deletes by omission, which is
+ * right for the whole-set writers but destroys a masked value the caller could
+ * never have read back. Here omission means "keep" and a delete is spelled null.
+ */
+describe("mergeServiceEnv", () => {
+  const stored = { API_TOKEN: "real-token", DB_PASSWORD: "hunter2", NODE_ENV: "production" };
+
+  test("a key absent from the patch keeps its stored value", () => {
+    expect(mergeServiceEnv(stored, { PORT: "8080" })).toEqual({ ...stored, PORT: "8080" });
+  });
+  test("a named key is overwritten, the rest survive", () => {
+    expect(mergeServiceEnv(stored, { NODE_ENV: "staging" })).toEqual({
+      ...stored,
+      NODE_ENV: "staging",
+    });
+  });
+  test("null removes just that key", () => {
+    expect(mergeServiceEnv(stored, { NODE_ENV: null, PORT: "3000" })).toEqual({
+      API_TOKEN: "real-token",
+      DB_PASSWORD: "hunter2",
+      PORT: "3000",
+    });
+  });
+  test("sentinel keeps the stored secret", () => {
+    expect(mergeServiceEnv(stored, { API_TOKEN: ENV_MASK, NEW: "v" })).toEqual({
+      ...stored,
+      NEW: "v",
+    });
+  });
+  test("sentinel with no stored counterpart is dropped (never persists dots)", () => {
+    expect(mergeServiceEnv(stored, { GHOST: ENV_MASK })).toEqual(stored);
+    expect(mergeServiceEnv(undefined, { A: ENV_MASK, B: "real" })).toEqual({ B: "real" });
+  });
+  // #472: maskValue leaves "" alone, so an unset variable reads back as empty and
+  // has to round-trip as empty — masking it would have the merge restore a value
+  // the operator had just cleared.
+  test("an empty value round-trips as empty rather than being treated as a sentinel", () => {
+    expect(mergeServiceEnv({ ...stored, EMPTY: "" }, { EMPTY: "" })).toEqual({
+      ...stored,
+      EMPTY: "",
+    });
+    expect(mergeServiceEnv(stored, { API_TOKEN: "" })).toEqual({ ...stored, API_TOKEN: "" });
+  });
+  test("null for the whole map clears it; undefined and {} leave it alone", () => {
+    expect(mergeServiceEnv(stored, null)).toEqual({});
+    expect(mergeServiceEnv(stored, undefined)).toEqual(stored);
+    expect(mergeServiceEnv(stored, {})).toEqual(stored);
+  });
+  test("a non-string value is dropped rather than written into the map", () => {
+    expect(mergeServiceEnv(stored, { N: 7 as unknown as string })).toEqual(stored);
+  });
+  // The map is attacker-influenced (any PATCH body key), and the sentinel arm asks
+  // whether a key exists in `stored` — a prototype-chain hit there would resurrect
+  // an inherited value under an attacker-chosen name.
+  test("prototype keys neither pollute nor resolve through the chain", () => {
+    const merged = mergeServiceEnv(stored, {
+      ["__proto__"]: "polluted",
+      constructor: ENV_MASK,
+      toString: ENV_MASK,
+    });
+    expect(merged).toEqual(stored);
+    expect(({} as Record<string, unknown>).polluted).toBeUndefined();
+    expect(Object.prototype.hasOwnProperty.call(merged, "toString")).toBe(false);
+  });
+});
+
 describe("helpers", () => {
   test("isMaskedValue / hasMaskedValue", () => {
     expect(isMaskedValue(ENV_MASK)).toBe(true);

apps/api/test/modules/services/service-schema-env.test.ts

@@ -0,0 +1,67 @@
+import { Value } from "@sinclair/typebox/value";
+import { describe, expect, it } from "vitest";
+import { ENV_MASK } from "@repo/core";
+import {
+  CreateServiceBody,
+  SyncServicesBody,
+  UpdateServiceBody,
+} from "../../../src/modules/services/service.schema";
+
+/**
+ * `environment` is nullable on UPDATE only (#619), and that asymmetry is load-bearing
+ * rather than incidental: on a partial patch, absent means "keep", so a delete needs
+ * its own spelling — while create and compose sync each rebuild the whole map from an
+ * upstream spec, where a null key would just be a second way to write "absent".
+ *
+ * Both halves are pinned here because both are easy to break by accident. Spreading
+ * the nullable field into `ComposeFieldsBlock` (the obvious "dedupe") would silently
+ * teach create and sync to accept nulls that `unmaskEnv` writes straight through as
+ * dropped keys. And the update override is only in effect because it is declared
+ * AFTER the spread — reordering the object literal reverts it with no other symptom.
+ */
+
+const checkUpdate = (env: unknown) => Value.Check(UpdateServiceBody, { environment: env });
+const checkCreate = (env: unknown) => Value.Check(CreateServiceBody, { name: "db", environment: env });
+const checkSync = (env: unknown) =>
+  Value.Check(SyncServicesBody, { services: [{ name: "db", environment: env }] });
+
+describe("UpdateServiceBody.environment", () => {
+  it("accepts a per-key null (remove that variable) and a null map (clear everything)", () => {
+    expect(checkUpdate({ KEEP: "1", DROP: null })).toBe(true);
+    expect(checkUpdate(null)).toBe(true);
+  });
+
+  it("accepts plain strings, the mask sentinel, and an empty map", () => {
+    expect(checkUpdate({ NODE_ENV: "production" })).toBe(true);
+    expect(checkUpdate({ API_TOKEN: ENV_MASK })).toBe(true);
+    expect(checkUpdate({})).toBe(true);
+  });
+
+  it("still rejects a non-string, non-null value", () => {
+    expect(checkUpdate({ PORT: 8080 })).toBe(false);
+    expect(checkUpdate({ NESTED: { a: 1 } })).toBe(false);
+  });
+
+  // The cap on the sibling env surfaces is 10000, but applying it here alone would
+  // make a long value (CA chain, base64 kubeconfig) creatable via POST or /sync and
+  // then permanently un-PATCH-able. Update stays as unbounded as they are.
+  it("does not cap value length more tightly than create and sync do", () => {
+    const long = "x".repeat(20_000);
+    expect(checkUpdate({ BUNDLE: long })).toBe(true);
+    expect(checkCreate({ BUNDLE: long })).toBe(true);
+  });
+});
+
+describe("create and sync stay non-nullable", () => {
+  it("CreateServiceBody rejects a null env value and a null map", () => {
+    expect(checkCreate({ DROP: null })).toBe(false);
+    expect(checkCreate(null)).toBe(false);
+    expect(checkCreate({ NODE_ENV: "production" })).toBe(true);
+  });
+
+  it("SyncServicesBody rejects a null env value and a null map", () => {
+    expect(checkSync({ DROP: null })).toBe(false);
+    expect(checkSync(null)).toBe(false);
+    expect(checkSync({ NODE_ENV: "production" })).toBe(true);
+  });
+});