fix(surface): default missing optional fields in readSurface, normalize writeSurface input

.changeset/witty-birds-gather.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 3662
+---
+**`readSurface()` no longer silently degrades partial `.gsd-surface.json` to the `full` profile** — missing optional array fields (`disabledClusters`, `explicitAdds`, `explicitRemoves`) now default to `[]` so a hand-edited surface state with only some fields keeps working. Hard validation failures (malformed JSON, non-object root, missing/non-string `baseProfile`) still return `null` but now emit a `console.warn` diagnostic naming the file and reason instead of failing silently. `writeSurface()` normalizes its input symmetrically — partial inputs are completed before being written, and an empty/missing `baseProfile` throws — so the writer/reader asymmetry that produced the original bug cannot recur. (#3662)

get-shit-done/bin/lib/surface.cjs

@@ -46,42 +46,84 @@ const SURFACE_FILE_NAME = '.gsd-surface.json';
  * @property {string[]} explicitRemoves
  */
 
+/**
+ * Normalize a partial SurfaceState into the full four-field shape.
+ * Missing or non-array optional fields default to []; baseProfile must already
+ * be a non-empty string (callers gate on that before normalizing).
+ *
+ * @param {Object} input
+ * @returns {SurfaceState}
+ */
+function normalizeSurfaceState(input) {
+  return {
+    baseProfile: input.baseProfile,
+    disabledClusters: Array.isArray(input.disabledClusters) ? input.disabledClusters.slice() : [],
+    explicitAdds: Array.isArray(input.explicitAdds) ? input.explicitAdds.slice() : [],
+    explicitRemoves: Array.isArray(input.explicitRemoves) ? input.explicitRemoves.slice() : [],
+  };
+}
+
 /**
  * Read the surface state from a runtime config directory.
  *
+ * Returns `null` only when there is no usable surface state:
+ *   - file is absent (silent — expected when no profile has been pinned),
+ *   - file is unreadable, malformed JSON, non-object root, or missing/invalid
+ *     `baseProfile` (each of these emits a `console.warn` diagnostic so callers
+ *     don't silently fall back to `'full'` with no explanation).
+ *
+ * Missing or wrong-typed optional array fields (`disabledClusters`,
+ * `explicitAdds`, `explicitRemoves`) default to `[]` — they are meaningfully
+ * empty and the writer/reader stayed symmetric only by accident before #3662.
+ *
  * @param {string} runtimeConfigDir
- * @returns {SurfaceState|null} null if file missing or corrupt
+ * @returns {SurfaceState|null}
  */
 function readSurface(runtimeConfigDir) {
   const filePath = path.join(runtimeConfigDir, SURFACE_FILE_NAME);
+  let raw;
+  try {
+    raw = fs.readFileSync(filePath, 'utf8');
+  } catch (err) {
+    if (err && err.code === 'ENOENT') return null;
+    console.warn(`[gsd] readSurface(${filePath}): unreadable (${err && (err.code || err.message)}); falling back to no surface state.`);
+    return null;
+  }
+  let parsed;
   try {
-    const raw = fs.readFileSync(filePath, 'utf8');
-    const parsed = JSON.parse(raw);
-    // Structural validation — must have these fields with expected types
-    if (typeof parsed !== 'object' || parsed === null) return null;
-    if (typeof parsed.baseProfile !== 'string') return null;
-    if (!Array.isArray(parsed.disabledClusters)) return null;
-    if (!Array.isArray(parsed.explicitAdds)) return null;
-    if (!Array.isArray(parsed.explicitRemoves)) return null;
-    return {
-      baseProfile: parsed.baseProfile,
-      disabledClusters: parsed.disabledClusters,
-      explicitAdds: parsed.explicitAdds,
-      explicitRemoves: parsed.explicitRemoves,
-    };
-  } catch {
+    parsed = JSON.parse(raw);
+  } catch (err) {
+    console.warn(`[gsd] readSurface(${filePath}): malformed JSON (${err.message}); falling back to no surface state.`);
+    return null;
+  }
+  if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) {
+    console.warn(`[gsd] readSurface(${filePath}): expected JSON object root; falling back to no surface state.`);
     return null;
   }
+  if (typeof parsed.baseProfile !== 'string' || parsed.baseProfile === '') {
+    console.warn(`[gsd] readSurface(${filePath}): missing or non-string 'baseProfile'; falling back to no surface state.`);
+    return null;
+  }
+  return normalizeSurfaceState(parsed);
 }
 
 /**
  * Write the surface state atomically via the platform seam (mkdir + tmp+rename).
  *
+ * Input is normalized to the full four-field shape so partial / hand-rolled
+ * objects cannot land on disk and trip readSurface later (#3662 symmetry fix).
+ * `baseProfile` is the only load-bearing field — callers must supply it as a
+ * non-empty string.
+ *
  * @param {string} runtimeConfigDir
  * @param {SurfaceState} surfaceState
  */
 function writeSurface(runtimeConfigDir, surfaceState) {
-  platformWriteSync(path.join(runtimeConfigDir, SURFACE_FILE_NAME), JSON.stringify(surfaceState, null, 2) + '\n');
+  if (!surfaceState || typeof surfaceState.baseProfile !== 'string' || surfaceState.baseProfile === '') {
+    throw new TypeError("writeSurface: 'baseProfile' must be a non-empty string");
+  }
+  const normalized = normalizeSurfaceState(surfaceState);
+  platformWriteSync(path.join(runtimeConfigDir, SURFACE_FILE_NAME), JSON.stringify(normalized, null, 2) + '\n');
 }
 
 // ---------------------------------------------------------------------------

tests/surface-state.test.cjs

@@ -15,6 +15,17 @@ function tmpDir() {
   return fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-surface-state-'));
 }
 
+function captureWarn(fn) {
+  const original = console.warn;
+  const warnings = [];
+  console.warn = (...args) => warnings.push(args.join(' '));
+  try {
+    return { result: fn(), warnings };
+  } finally {
+    console.warn = original;
+  }
+}
+
 describe('readSurface / writeSurface', () => {
   test('round-trips a complete surface state', () => {
     const dir = tmpDir();
@@ -81,42 +92,150 @@ describe('readSurface / writeSurface', () => {
     assert.strictEqual(result, null);
   });
 
-  test('corrupt JSON returns null', () => {
+  test('corrupt JSON returns null and warns', () => {
     const dir = tmpDir();
     try {
       fs.writeFileSync(path.join(dir, '.gsd-surface.json'), '{not valid json', 'utf8');
-      const result = readSurface(dir);
+      const { result, warnings } = captureWarn(() => readSurface(dir));
       assert.strictEqual(result, null);
+      assert.strictEqual(warnings.length, 1);
+      assert.match(warnings[0], /malformed JSON/);
     } finally {
       fs.rmSync(dir, { recursive: true, force: true });
     }
   });
 
-  test('JSON missing baseProfile field returns null', () => {
+  test('JSON missing baseProfile field returns null and warns (#3662)', () => {
     const dir = tmpDir();
     try {
       fs.writeFileSync(
         path.join(dir, '.gsd-surface.json'),
         JSON.stringify({ disabledClusters: [], explicitAdds: [], explicitRemoves: [] }),
         'utf8'
       );
-      const result = readSurface(dir);
+      const { result, warnings } = captureWarn(() => readSurface(dir));
       assert.strictEqual(result, null);
+      assert.strictEqual(warnings.length, 1);
+      assert.match(warnings[0], /baseProfile/);
     } finally {
       fs.rmSync(dir, { recursive: true, force: true });
     }
   });
 
-  test('JSON with non-array disabledClusters returns null', () => {
+  test('JSON with non-string baseProfile returns null and warns', () => {
     const dir = tmpDir();
     try {
       fs.writeFileSync(
         path.join(dir, '.gsd-surface.json'),
-        JSON.stringify({ baseProfile: 'standard', disabledClusters: 'utility', explicitAdds: [], explicitRemoves: [] }),
+        JSON.stringify({ baseProfile: 42, disabledClusters: [], explicitAdds: [], explicitRemoves: [] }),
         'utf8'
       );
-      const result = readSurface(dir);
+      const { result, warnings } = captureWarn(() => readSurface(dir));
+      assert.strictEqual(result, null);
+      assert.match(warnings[0], /baseProfile/);
+    } finally {
+      fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  test('JSON with non-object root returns null and warns', () => {
+    const dir = tmpDir();
+    try {
+      fs.writeFileSync(path.join(dir, '.gsd-surface.json'), JSON.stringify(['not', 'an', 'object']), 'utf8');
+      const { result, warnings } = captureWarn(() => readSurface(dir));
       assert.strictEqual(result, null);
+      assert.match(warnings[0], /object root/);
+    } finally {
+      fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  test('missing optional array field defaults to [] (#3662)', () => {
+    const dir = tmpDir();
+    try {
+      fs.writeFileSync(
+        path.join(dir, '.gsd-surface.json'),
+        JSON.stringify({ baseProfile: 'standard', disabledClusters: [], explicitAdds: [] }),
+        'utf8'
+      );
+      const { result, warnings } = captureWarn(() => readSurface(dir));
+      assert.deepStrictEqual(result, {
+        baseProfile: 'standard',
+        disabledClusters: [],
+        explicitAdds: [],
+        explicitRemoves: [],
+      });
+      assert.deepStrictEqual(warnings, [], 'defaulting an optional field should not warn');
+    } finally {
+      fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  test('all optional arrays missing default to [] (#3662)', () => {
+    const dir = tmpDir();
+    try {
+      fs.writeFileSync(
+        path.join(dir, '.gsd-surface.json'),
+        JSON.stringify({ baseProfile: 'standard' }),
+        'utf8'
+      );
+      const { result, warnings } = captureWarn(() => readSurface(dir));
+      assert.deepStrictEqual(result, {
+        baseProfile: 'standard',
+        disabledClusters: [],
+        explicitAdds: [],
+        explicitRemoves: [],
+      });
+      assert.deepStrictEqual(warnings, []);
+    } finally {
+      fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  test('non-array optional field is coerced to [] (#3662)', () => {
+    const dir = tmpDir();
+    try {
+      fs.writeFileSync(
+        path.join(dir, '.gsd-surface.json'),
+        JSON.stringify({ baseProfile: 'standard', disabledClusters: 'utility', explicitAdds: [], explicitRemoves: [] }),
+        'utf8'
+      );
+      const { result } = captureWarn(() => readSurface(dir));
+      assert.deepStrictEqual(result, {
+        baseProfile: 'standard',
+        disabledClusters: [],
+        explicitAdds: [],
+        explicitRemoves: [],
+      });
+    } finally {
+      fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  test('writeSurface normalizes partial input — all four fields land on disk (#3662)', () => {
+    const dir = tmpDir();
+    try {
+      writeSurface(dir, { baseProfile: 'standard' });
+      const onDisk = JSON.parse(fs.readFileSync(path.join(dir, '.gsd-surface.json'), 'utf8'));
+      assert.deepStrictEqual(onDisk, {
+        baseProfile: 'standard',
+        disabledClusters: [],
+        explicitAdds: [],
+        explicitRemoves: [],
+      });
+    } finally {
+      fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  test('writeSurface rejects missing baseProfile (#3662 writer guard)', () => {
+    const dir = tmpDir();
+    try {
+      assert.throws(
+        () => writeSurface(dir, { disabledClusters: [], explicitAdds: [], explicitRemoves: [] }),
+        /baseProfile/
+      );
+      assert.throws(() => writeSurface(dir, { baseProfile: '' }), /baseProfile/);
     } finally {
       fs.rmSync(dir, { recursive: true, force: true });
     }