diff --git a/.changeset/witty-birds-gather.md b/.changeset/witty-birds-gather.md new file mode 100644 index 000000000..688c84085 --- /dev/null +++ b/.changeset/witty-birds-gather.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3666 +--- +**`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 (unreadable file like `EACCES`, malformed JSON, non-object root, missing/non-string/blank/comma-only `baseProfile`) still return `null` but now emit a `console.warn` diagnostic naming the file and reason instead of failing silently. Unknown profile names in `baseProfile` (e.g. a typo like `"standrad"`) warn — they were previously swallowed by `resolveProfile()`'s fallback-to-`full`. Optional array fields with the wrong type (e.g. `disabledClusters: "utility"`) also warn before being coerced to `[]`. `writeSurface()` normalizes its input symmetrically — partial inputs are completed before being written, blank/non-string/comma-only `baseProfile` throws `TypeError`, and unknown profile names plus wrong-typed optional fields warn — so the writer/reader asymmetry that produced the original bug cannot recur. (#3662) diff --git a/get-shit-done/bin/lib/surface.cjs b/get-shit-done/bin/lib/surface.cjs index 3ca70d71d..7430b66a3 100644 --- a/get-shit-done/bin/lib/surface.cjs +++ b/get-shit-done/bin/lib/surface.cjs @@ -34,6 +34,52 @@ const { CLUSTERS, allClusteredSkills } = require('./clusters.cjs'); const { findInstallSourceRoot } = require('./runtime-artifact-layout.cjs'); const SURFACE_FILE_NAME = '.gsd-surface.json'; +const KNOWN_PROFILE_NAMES = new Set(Object.keys(PROFILES)); + +/** + * Split a `baseProfile` string (single name or comma-composed) into the + * non-empty trimmed mode list that resolveProfile() would see. + * + * @param {string} baseProfile + * @returns {string[]} effective modes after split/trim/empty-strip + */ +function effectiveProfileModes(baseProfile) { + return baseProfile + .split(',') + .map((m) => m.trim()) + .filter((m) => m.length > 0); +} + +/** + * Inspect a `baseProfile` string (single name or comma-composed) and return + * any modes that aren't registered in PROFILES. Callers keep the raw string — + * resolveProfile() decides the resolution fallback. + * + * @param {string} baseProfile + * @returns {string[]} unknown modes + */ +function unknownProfileModes(baseProfile) { + return effectiveProfileModes(baseProfile).filter((m) => !KNOWN_PROFILE_NAMES.has(m)); +} + +/** + * Collect optional array fields that are present but not an array, so the + * reader and writer can emit a single warn diagnostic before coercing them + * to `[]` in normalizeSurfaceState. A missing field is *not* flagged — it + * defaults to `[]` silently (that's the lenient #3662 behavior). + * + * @param {Object} input + * @returns {string[]} field names with wrong type + */ +function mistypedOptionalFields(input) { + const wrong = []; + for (const field of ['disabledClusters', 'explicitAdds', 'explicitRemoves']) { + if (Object.prototype.hasOwnProperty.call(input, field) && !Array.isArray(input[field])) { + wrong.push(field); + } + } + return wrong; +} // --------------------------------------------------------------------------- // State IO @@ -47,42 +93,100 @@ 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 { - 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 { + 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 { + 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' || effectiveProfileModes(parsed.baseProfile).length === 0) { + console.warn(`[gsd] readSurface(${filePath}): missing, non-string, blank, or comma-only 'baseProfile'; falling back to no surface state.`); + return null; + } + const unknownModes = unknownProfileModes(parsed.baseProfile); + if (unknownModes.length > 0) { + console.warn(`[gsd] readSurface(${filePath}): unknown profile mode(s) in 'baseProfile': ${unknownModes.join(', ')} (valid: ${[...KNOWN_PROFILE_NAMES].join(', ')}); resolveProfile() will skip unknowns and may fall back to 'full'.`); + } + const mistyped = mistypedOptionalFields(parsed); + if (mistyped.length > 0) { + console.warn(`[gsd] readSurface(${filePath}): optional field(s) with wrong type (expected array): ${mistyped.join(', ')}; coercing to [].`); + } + 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' || effectiveProfileModes(surfaceState.baseProfile).length === 0) { + throw new TypeError("writeSurface: 'baseProfile' must be a non-blank string with at least one mode"); + } + const unknownModes = unknownProfileModes(surfaceState.baseProfile); + if (unknownModes.length > 0) { + console.warn(`[gsd] writeSurface: unknown profile mode(s) in 'baseProfile': ${unknownModes.join(', ')} (valid: ${[...KNOWN_PROFILE_NAMES].join(', ')}); persisting anyway — resolveProfile() will skip unknowns.`); + } + const mistyped = mistypedOptionalFields(surfaceState); + if (mistyped.length > 0) { + console.warn(`[gsd] writeSurface: optional field(s) with wrong type (expected array): ${mistyped.join(', ')}; coercing to [].`); + } + const normalized = normalizeSurfaceState(surfaceState); + platformWriteSync(path.join(runtimeConfigDir, SURFACE_FILE_NAME), JSON.stringify(normalized, null, 2) + '\n'); } // --------------------------------------------------------------------------- diff --git a/tests/surface-state.test.cjs b/tests/surface-state.test.cjs index 09dcd7e1b..cb51451f6 100644 --- a/tests/surface-state.test.cjs +++ b/tests/surface-state.test.cjs @@ -10,9 +10,21 @@ const path = require('path'); const os = require('os'); const { readSurface, writeSurface } = require('../get-shit-done/bin/lib/surface.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); function tmpDir() { - return fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-surface-state-')); + return createTempDir('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', () => { @@ -29,7 +41,7 @@ describe('readSurface / writeSurface', () => { const read = readSurface(dir); assert.deepStrictEqual(read, state); } finally { - fs.rmSync(dir, { recursive: true, force: true }); + cleanup(dir); } }); @@ -45,7 +57,7 @@ describe('readSurface / writeSurface', () => { writeSurface(dir, state); assert.deepStrictEqual(readSurface(dir), state); } finally { - fs.rmSync(dir, { recursive: true, force: true }); + cleanup(dir); } }); @@ -53,7 +65,7 @@ describe('readSurface / writeSurface', () => { const dir = tmpDir(); try { const state = { - baseProfile: 'core,audit', + baseProfile: 'core,standard', disabledClusters: [], explicitAdds: [], explicitRemoves: ['health'], @@ -61,7 +73,7 @@ describe('readSurface / writeSurface', () => { writeSurface(dir, state); assert.deepStrictEqual(readSurface(dir), state); } finally { - fs.rmSync(dir, { recursive: true, force: true }); + cleanup(dir); } }); @@ -71,7 +83,7 @@ describe('readSurface / writeSurface', () => { const result = readSurface(dir); assert.strictEqual(result, null); } finally { - fs.rmSync(dir, { recursive: true, force: true }); + cleanup(dir); } }); @@ -81,18 +93,38 @@ describe('readSurface / writeSurface', () => { assert.strictEqual(result, null); }); - test('corrupt JSON returns null', () => { + // chmod-000 unreadable file — Linux-only because Windows and root accounts + // ignore mode bits. Covers the EACCES branch in readSurface (#3662 Gemini). + test('unreadable file (EACCES) returns null and warns', { skip: process.platform === 'win32' || process.getuid?.() === 0 }, () => { const dir = tmpDir(); try { - fs.writeFileSync(path.join(dir, '.gsd-surface.json'), '{not valid json', 'utf8'); - const result = readSurface(dir); + const filePath = path.join(dir, '.gsd-surface.json'); + fs.writeFileSync(filePath, '{"baseProfile":"standard"}', 'utf8'); + fs.chmodSync(filePath, 0o000); + const { result, warnings } = captureWarn(() => readSurface(dir)); assert.strictEqual(result, null); + assert.strictEqual(warnings.length, 1); + assert.match(warnings[0], /unreadable/); + fs.chmodSync(filePath, 0o644); // restore so cleanup can rm } finally { - fs.rmSync(dir, { recursive: true, force: true }); + cleanup(dir); } }); - test('JSON missing baseProfile field 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, warnings } = captureWarn(() => readSurface(dir)); + assert.strictEqual(result, null); + assert.strictEqual(warnings.length, 1); + assert.match(warnings[0], /malformed JSON/); + } finally { + cleanup(dir); + } + }); + + test('JSON missing baseProfile field returns null and warns (#3662)', () => { const dir = tmpDir(); try { fs.writeFileSync( @@ -100,25 +132,275 @@ describe('readSurface / writeSurface', () => { 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 }); + cleanup(dir); } }); - 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 }); + cleanup(dir); + } + }); + + test('typo in baseProfile warns about unknown mode but still returns state (#3662 Codex)', () => { + const dir = tmpDir(); + try { + fs.writeFileSync( + path.join(dir, '.gsd-surface.json'), + JSON.stringify({ baseProfile: 'standrad', disabledClusters: [], explicitAdds: [], explicitRemoves: [] }), + 'utf8' + ); + const { result, warnings } = captureWarn(() => readSurface(dir)); + assert.deepStrictEqual(result, { + baseProfile: 'standrad', + disabledClusters: [], + explicitAdds: [], + explicitRemoves: [], + }); + assert.strictEqual(warnings.length, 1); + assert.match(warnings[0], /unknown profile mode/); + assert.match(warnings[0], /standrad/); + } finally { + cleanup(dir); + } + }); + + test('composed baseProfile warns only about unknown member (#3662 Codex)', () => { + const dir = tmpDir(); + try { + fs.writeFileSync( + path.join(dir, '.gsd-surface.json'), + JSON.stringify({ baseProfile: 'core,bogus', disabledClusters: [], explicitAdds: [], explicitRemoves: [] }), + 'utf8' + ); + const { result, warnings } = captureWarn(() => readSurface(dir)); + assert.strictEqual(result.baseProfile, 'core,bogus'); + assert.strictEqual(warnings.length, 1); + // Warning must call out 'bogus' as unknown, but not list 'core' as unknown + // (it does appear once in the "(valid: core, standard, full)" hint — that's fine). + const unknownPart = warnings[0].split('(valid:')[0]; + assert.match(unknownPart, /bogus/); + assert.doesNotMatch(unknownPart, /\bcore\b/); + } finally { + cleanup(dir); + } + }); + + test('known composed baseProfile does not warn (#3662 Codex)', () => { + const dir = tmpDir(); + try { + fs.writeFileSync( + path.join(dir, '.gsd-surface.json'), + JSON.stringify({ baseProfile: 'core,standard', disabledClusters: [], explicitAdds: [], explicitRemoves: [] }), + 'utf8' + ); + const { warnings } = captureWarn(() => readSurface(dir)); + assert.deepStrictEqual(warnings, [], 'all-known modes should not warn'); + } finally { + cleanup(dir); + } + }); + + test('writeSurface warns on unknown profile mode but still writes (#3662 Codex)', () => { + const dir = tmpDir(); + try { + const { warnings } = captureWarn(() => writeSurface(dir, { baseProfile: 'standrad' })); + const onDisk = JSON.parse(fs.readFileSync(path.join(dir, '.gsd-surface.json'), 'utf8')); + assert.strictEqual(onDisk.baseProfile, 'standrad'); + assert.strictEqual(warnings.length, 1); + assert.match(warnings[0], /unknown profile mode/); + assert.match(warnings[0], /standrad/); + } finally { + cleanup(dir); + } + }); + + test('JSON with whitespace-only baseProfile returns null and warns (#3662 CodeRabbit)', () => { + const dir = tmpDir(); + try { + fs.writeFileSync( + path.join(dir, '.gsd-surface.json'), + JSON.stringify({ baseProfile: ' ', disabledClusters: [], explicitAdds: [], explicitRemoves: [] }), + 'utf8' + ); + const { result, warnings } = captureWarn(() => readSurface(dir)); + assert.strictEqual(result, null); + assert.match(warnings[0], /baseProfile/); + } finally { + cleanup(dir); + } + }); + + 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 { + cleanup(dir); + } + }); + + 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 { + cleanup(dir); + } + }); + + 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 { + cleanup(dir); + } + }); + + test('non-array optional field is coerced to [] and warns (#3662 Gemini)', () => { + const dir = tmpDir(); + try { + fs.writeFileSync( + path.join(dir, '.gsd-surface.json'), + JSON.stringify({ baseProfile: 'standard', disabledClusters: 'utility', explicitAdds: 42, explicitRemoves: [] }), + 'utf8' + ); + const { result, warnings } = captureWarn(() => readSurface(dir)); + assert.deepStrictEqual(result, { + baseProfile: 'standard', + disabledClusters: [], + explicitAdds: [], + explicitRemoves: [], + }); + assert.strictEqual(warnings.length, 1); + assert.match(warnings[0], /wrong type/); + assert.match(warnings[0], /disabledClusters/); + assert.match(warnings[0], /explicitAdds/); + } finally { + cleanup(dir); + } + }); + + test('comma-only baseProfile is rejected (#3662 Gemini)', () => { + const dir = tmpDir(); + try { + fs.writeFileSync( + path.join(dir, '.gsd-surface.json'), + JSON.stringify({ baseProfile: ', ,', disabledClusters: [], explicitAdds: [], explicitRemoves: [] }), + 'utf8' + ); + const { result, warnings } = captureWarn(() => readSurface(dir)); + assert.strictEqual(result, null); + assert.match(warnings[0], /comma-only/); + } finally { + cleanup(dir); + } + }); + + test('writeSurface rejects comma-only baseProfile (#3662 Gemini)', () => { + const dir = tmpDir(); + try { + assert.throws(() => writeSurface(dir, { baseProfile: ', ,' }), /baseProfile/); + assert.throws(() => writeSurface(dir, { baseProfile: ',' }), /baseProfile/); + } finally { + cleanup(dir); + } + }); + + test('writeSurface warns on wrong-typed optional field but still writes (#3662 Gemini)', () => { + const dir = tmpDir(); + try { + const { warnings } = captureWarn(() => writeSurface(dir, { + baseProfile: 'standard', + disabledClusters: 'utility', + })); + const onDisk = JSON.parse(fs.readFileSync(path.join(dir, '.gsd-surface.json'), 'utf8')); + assert.deepStrictEqual(onDisk, { + baseProfile: 'standard', + disabledClusters: [], + explicitAdds: [], + explicitRemoves: [], + }); + assert.strictEqual(warnings.length, 1); + assert.match(warnings[0], /wrong type/); + assert.match(warnings[0], /disabledClusters/); + } finally { + cleanup(dir); + } + }); + + 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 { + cleanup(dir); + } + }); + + test('writeSurface rejects missing, empty, or blank baseProfile (#3662 writer guard)', () => { + const dir = tmpDir(); + try { + assert.throws( + () => writeSurface(dir, { disabledClusters: [], explicitAdds: [], explicitRemoves: [] }), + /baseProfile/ + ); + assert.throws(() => writeSurface(dir, { baseProfile: '' }), /baseProfile/); + assert.throws(() => writeSurface(dir, { baseProfile: ' ' }), /baseProfile/); + assert.throws(() => writeSurface(dir, { baseProfile: 42 }), /baseProfile/); + assert.throws(() => writeSurface(dir, null), /baseProfile/); + } finally { + cleanup(dir); } }); @@ -134,7 +416,7 @@ describe('readSurface / writeSurface', () => { // The canonical file exists assert.ok(files.includes('.gsd-surface.json')); } finally { - fs.rmSync(dir, { recursive: true, force: true }); + cleanup(dir); } }); @@ -147,7 +429,7 @@ describe('readSurface / writeSurface', () => { assert.strictEqual(read.baseProfile, 'standard'); assert.deepStrictEqual(read.disabledClusters, ['utility']); } finally { - fs.rmSync(dir, { recursive: true, force: true }); + cleanup(dir); } }); @@ -159,7 +441,7 @@ describe('readSurface / writeSurface', () => { assert.ok(fs.existsSync(nested)); assert.ok(readSurface(nested) !== null); } finally { - fs.rmSync(base, { recursive: true, force: true }); + cleanup(base); } }); });