fix(cli): keep the workspace color when settings are synced from git (#11144)

* fix(cli): keep the workspace color when settings are synced from git

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(cli): name the sync direction consistently in the identity-field comments

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(cli): apply the workspace color from settings.yaml only when the file sets one

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

cli/src/commands/sync/sync.ts

@@ -2542,6 +2542,21 @@ export function preservePendingScriptLocks(
   }
 }
 
+// `sync push` never applies the workspace's display name from settings.yaml and
+// applies its color only when the local file carries one (see
+// pushWorkspaceSettings), so on a push the fields it would not apply must
+// compare equal, or the row is listed on every run.
+const isWorkspaceSettingsFile = (p: string) =>
+  /^settings(\.[^./\\]+)?\.(yaml|json)$/.test(p);
+function stripUnappliedSettingsFields(local: any, remote: any) {
+  delete local?.name;
+  delete remote?.name;
+  if (local?.color == null) {
+    delete local?.color;
+    delete remote?.color;
+  }
+}
+
 export async function compareDynFSElement(
   els1: DynFSElement,
   els2: DynFSElement | undefined,
@@ -2757,6 +2772,9 @@ export async function compareDynFSElement(
           delete parsedV?.enabled;
           delete parsedM2?.enabled;
         }
+        if (isEls1Remote === false && isWorkspaceSettingsFile(k)) {
+          stripUnappliedSettingsFields(parsedV, parsedM2);
+        }
         if (deepEqual(parsedV, parsedM2)) {
           continue;
         }
@@ -2771,6 +2789,9 @@ export async function compareDynFSElement(
           delete before?.enabled;
           delete after?.enabled;
         }
+        if (isEls1Remote === false && isWorkspaceSettingsFile(k)) {
+          stripUnappliedSettingsFields(after, before);
+        }
         if (deepEqual(before, after)) {
           continue;
         }

cli/src/core/settings.ts

@@ -214,9 +214,15 @@ export async function pushWorkspaceSettings(
   }
 
   // Exclude fields that are never applied here: slack_team_id/slack_name are OAuth-only,
-  // and name is not applied on pull (see below), so a name-only diff stays a no-op.
+  // and name is never applied (see below), so a name-only diff stays a no-op. color is
+  // applied only when the file carries it, so an unset one leaves the comparison too.
   const { slack_team_id: _lst, slack_name: _lsn, name: _ln, ...comparableLocal } = localSettings;
   const { slack_team_id: _rst, slack_name: _rsn, name: _rn, ...comparableRemote } = settings;
+  const colorManaged = localSettings.color != null;
+  if (!colorManaged) {
+    delete comparableLocal.color;
+    delete comparableRemote.color;
+  }
   if (isSuperset(comparableLocal, comparableRemote)) {
     log.debug(`Workspace settings are up to date`);
     return;
@@ -351,10 +357,9 @@ export async function pushWorkspaceSettings(
     });
   }
 
-  // Workspace display name is intentionally not applied on pull: settings.yaml is shared
-  // across a repo's branches, so applying it would let one workspace's name overwrite
-  // another's when both sync the same repo. It stays in the file (written on push), but a
-  // live workspace is only renamed by its owner.
+  // Workspace display name is intentionally never applied by `sync push`: settings.yaml is
+  // shared across a repo's branches, so applying it would let one workspace's name overwrite
+  // another's when both sync the same repo. `sync pull` still records it.
 
   if (localSettings.mute_critical_alerts != settings.mute_critical_alerts) {
     log.debug(`Updating mute critical alerts...`);
@@ -366,7 +371,9 @@ export async function pushWorkspaceSettings(
     });
   }
 
-  if (localSettings.color != settings.color) {
+  // A color is applied only when the file carries one: `sync pull` omits the key for a
+  // workspace without a color, so an unset key means "not managed by git", never "clear".
+  if (colorManaged && localSettings.color != settings.color) {
     log.debug(`Updating workspace color...`);
     await wmill.changeWorkspaceColor({
       workspace,

cli/test/push_diff_convergence_unit.test.ts

@@ -64,6 +64,7 @@ async function diff(
   remoteEl: Mock,
   skips: Record<string, unknown>,
   parentOwnsScheduleEnabled?: (scheduleFilePath: string) => boolean,
+  isEls1Remote = false,
 ) {
   const { changes } = await compareDynFSElement(
     localEl as any,
@@ -76,7 +77,7 @@ async function diff(
     false,
     undefined,
     undefined,
-    false,
+    isEls1Remote,
     false,
     parentOwnsScheduleEnabled,
   );
@@ -296,3 +297,26 @@ test("push: checkout inline names stay inside the flow folder", async () => {
     await checkoutInlineNames(join(process.cwd(), "missing.yaml")),
   ).toEqual({});
 });
+
+// A push never applies the workspace's display name and applies its color only
+// when the local file carries one (see pushWorkspaceSettings), so a file that
+// differs only in what would not be applied is not a push change; a pull still
+// rewrites the file.
+test("push: settings.yaml differing only by name or an unset color is not a change", async () => {
+  const remote = local({
+    "settings.yaml": "name: prod\ncolor: '#ff0000'\nerror_handler: null\n",
+  });
+  const unsetColor = local({
+    "settings.yaml": "name: staging\nerror_handler: null\n",
+  });
+  const skips = { includeSettings: true };
+  expect(await diff(unsetColor, remote, skips)).toEqual([]);
+  expect(await diff(remote, unsetColor, skips, undefined, true)).toEqual([
+    "edited settings.yaml",
+  ]);
+
+  const otherColor = local({
+    "settings.yaml": "name: staging\ncolor: '#00ff00'\nerror_handler: null\n",
+  });
+  expect(await diff(otherColor, remote, skips)).toEqual(["edited settings.yaml"]);
+});

cli/test/push_workspace_settings_identity_unit.test.ts

@@ -1,23 +1,32 @@
 /**
- * Regression guard: a pull (pushWorkspaceSettings) must not apply the workspace
- * display name from settings.yaml. Rationale lives at the apply site in settings.ts.
+ * Regression guard: `sync push` (pushWorkspaceSettings) must never apply the
+ * workspace display name from settings.yaml, and must apply the color only when
+ * the file carries one. Rationale lives at the apply sites in settings.ts.
  */
 
 import { expect, test, describe, beforeEach, mock } from "bun:test";
 
 let changeWorkspaceNameCalls: unknown[] = [];
+let changeWorkspaceColorCalls: unknown[] = [];
 let editWebhookCalls: unknown[] = [];
 let remoteName = "";
+let remoteColor: string | undefined = undefined;
 let remoteWebhook: string | undefined = undefined;
 
 // Every wmill.* call reachable from pushWorkspaceSettings is stubbed so the
-// function runs without a backend; only the two we assert on record calls.
+// function runs without a backend; only the three we assert on record calls.
 mock.module("../gen/services.gen.ts", () => ({
-  getSettings: async (_a: { workspace: string }) => ({ webhook: remoteWebhook }),
+  getSettings: async (_a: { workspace: string }) => ({
+    webhook: remoteWebhook,
+    color: remoteColor,
+  }),
   getWorkspaceName: async (_a: { workspace: string }) => remoteName,
   changeWorkspaceName: async (a: unknown) => {
     changeWorkspaceNameCalls.push(a);
   },
+  changeWorkspaceColor: async (a: unknown) => {
+    changeWorkspaceColorCalls.push(a);
+  },
   editWebhook: async (a: unknown) => {
     editWebhookCalls.push(a);
   },
@@ -30,7 +39,6 @@ mock.module("../gen/services.gen.ts", () => ({
   editWorkspaceDefaultApp: async () => {},
   editDefaultScripts: async () => {},
   workspaceMuteCriticalAlertsUi: async () => {},
-  changeWorkspaceColor: async () => {},
   updateOperatorSettings: async () => {},
   editDataTableConfig: async () => {},
   editSlackCommand: async () => {},
@@ -40,13 +48,15 @@ mock.module("../gen/services.gen.ts", () => ({
 
 const { pushWorkspaceSettings } = await import("../src/core/settings.ts");
 
-describe("pushWorkspaceSettings workspace name", () => {
+describe("pushWorkspaceSettings workspace identity", () => {
   const ws = "phoenix";
 
   beforeEach(() => {
     changeWorkspaceNameCalls = [];
+    changeWorkspaceColorCalls = [];
     editWebhookCalls = [];
     remoteName = "phoenix";
+    remoteColor = undefined;
     remoteWebhook = undefined;
   });
 
@@ -69,4 +79,37 @@ describe("pushWorkspaceSettings workspace name", () => {
     expect(editWebhookCalls.length).toBe(0);
     expect(changeWorkspaceNameCalls.length).toBe(0);
   });
+
+  test("a settings.yaml without a color key does not clear the workspace color", async () => {
+    remoteColor = "#ff0000";
+    remoteWebhook = "https://old";
+    await pushWorkspaceSettings(ws, "settings", undefined, {
+      name: "phoenix",
+      webhook: "https://new",
+    });
+    expect(editWebhookCalls.length).toBe(1);
+    expect(changeWorkspaceColorCalls.length).toBe(0);
+  });
+
+  test("a color in settings.yaml is applied when it differs from the workspace", async () => {
+    remoteColor = "#ff0000";
+    await pushWorkspaceSettings(ws, "settings", undefined, {
+      name: "phoenix",
+      color: "#00ff00",
+    });
+    expect(editWebhookCalls.length).toBe(0);
+    expect(changeWorkspaceColorCalls).toEqual([
+      { workspace: ws, requestBody: { color: "#00ff00" } },
+    ]);
+  });
+
+  test("a color matching the workspace is a complete no-op", async () => {
+    remoteColor = "#ff0000";
+    await pushWorkspaceSettings(ws, "settings", undefined, {
+      name: "phoenix",
+      color: "#ff0000",
+    });
+    expect(editWebhookCalls.length).toBe(0);
+    expect(changeWorkspaceColorCalls.length).toBe(0);
+  });
 });