diff --git a/.changeset/witty-birds-gather.md b/.changeset/witty-birds-gather.md deleted file mode 100644 index 688c84085..000000000 --- a/.changeset/witty-birds-gather.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -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 7430b66a3..3ca70d71d 100644 --- a/get-shit-done/bin/lib/surface.cjs +++ b/get-shit-done/bin/lib/surface.cjs @@ -34,52 +34,6 @@ 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 @@ -93,100 +47,42 @@ function mistypedOptionalFields(input) { * @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} + * @returns {SurfaceState|null} null if file missing or corrupt */ 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.`); + 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 { 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) { - 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'); + platformWriteSync(path.join(runtimeConfigDir, SURFACE_FILE_NAME), JSON.stringify(surfaceState, null, 2) + '\n'); } // --------------------------------------------------------------------------- diff --git a/tests/surface-state.test.cjs b/tests/surface-state.test.cjs index cb51451f6..09dcd7e1b 100644 --- a/tests/surface-state.test.cjs +++ b/tests/surface-state.test.cjs @@ -10,21 +10,9 @@ 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 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; - } + return fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-surface-state-')); } describe('readSurface / writeSurface', () => { @@ -41,7 +29,7 @@ describe('readSurface / writeSurface', () => { const read = readSurface(dir); assert.deepStrictEqual(read, state); } finally { - cleanup(dir); + fs.rmSync(dir, { recursive: true, force: true }); } }); @@ -57,7 +45,7 @@ describe('readSurface / writeSurface', () => { writeSurface(dir, state); assert.deepStrictEqual(readSurface(dir), state); } finally { - cleanup(dir); + fs.rmSync(dir, { recursive: true, force: true }); } }); @@ -65,7 +53,7 @@ describe('readSurface / writeSurface', () => { const dir = tmpDir(); try { const state = { - baseProfile: 'core,standard', + baseProfile: 'core,audit', disabledClusters: [], explicitAdds: [], explicitRemoves: ['health'], @@ -73,7 +61,7 @@ describe('readSurface / writeSurface', () => { writeSurface(dir, state); assert.deepStrictEqual(readSurface(dir), state); } finally { - cleanup(dir); + fs.rmSync(dir, { recursive: true, force: true }); } }); @@ -83,7 +71,7 @@ describe('readSurface / writeSurface', () => { const result = readSurface(dir); assert.strictEqual(result, null); } finally { - cleanup(dir); + fs.rmSync(dir, { recursive: true, force: true }); } }); @@ -93,38 +81,18 @@ describe('readSurface / writeSurface', () => { assert.strictEqual(result, 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 { - 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 { - cleanup(dir); - } - }); - - test('corrupt JSON returns null and warns', () => { + test('corrupt JSON returns null', () => { const dir = tmpDir(); try { fs.writeFileSync(path.join(dir, '.gsd-surface.json'), '{not valid json', 'utf8'); - const { result, warnings } = captureWarn(() => readSurface(dir)); + const result = readSurface(dir); assert.strictEqual(result, null); - assert.strictEqual(warnings.length, 1); - assert.match(warnings[0], /malformed JSON/); } finally { - cleanup(dir); + fs.rmSync(dir, { recursive: true, force: true }); } }); - test('JSON missing baseProfile field returns null and warns (#3662)', () => { + test('JSON missing baseProfile field returns null', () => { const dir = tmpDir(); try { fs.writeFileSync( @@ -132,275 +100,25 @@ describe('readSurface / writeSurface', () => { JSON.stringify({ disabledClusters: [], explicitAdds: [], explicitRemoves: [] }), 'utf8' ); - const { result, warnings } = captureWarn(() => readSurface(dir)); + const result = readSurface(dir); assert.strictEqual(result, null); - assert.strictEqual(warnings.length, 1); - assert.match(warnings[0], /baseProfile/); } finally { - cleanup(dir); + fs.rmSync(dir, { recursive: true, force: true }); } }); - test('JSON with non-string baseProfile returns null and warns', () => { + test('JSON with non-array disabledClusters returns null', () => { const dir = tmpDir(); try { fs.writeFileSync( path.join(dir, '.gsd-surface.json'), - JSON.stringify({ baseProfile: 42, disabledClusters: [], explicitAdds: [], explicitRemoves: [] }), + JSON.stringify({ baseProfile: 'standard', disabledClusters: 'utility', explicitAdds: [], explicitRemoves: [] }), 'utf8' ); - const { result, warnings } = captureWarn(() => readSurface(dir)); + const result = readSurface(dir); assert.strictEqual(result, null); - assert.match(warnings[0], /baseProfile/); } finally { - 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); + fs.rmSync(dir, { recursive: true, force: true }); } }); @@ -416,7 +134,7 @@ describe('readSurface / writeSurface', () => { // The canonical file exists assert.ok(files.includes('.gsd-surface.json')); } finally { - cleanup(dir); + fs.rmSync(dir, { recursive: true, force: true }); } }); @@ -429,7 +147,7 @@ describe('readSurface / writeSurface', () => { assert.strictEqual(read.baseProfile, 'standard'); assert.deepStrictEqual(read.disabledClusters, ['utility']); } finally { - cleanup(dir); + fs.rmSync(dir, { recursive: true, force: true }); } }); @@ -441,7 +159,7 @@ describe('readSurface / writeSurface', () => { assert.ok(fs.existsSync(nested)); assert.ok(readSurface(nested) !== null); } finally { - cleanup(base); + fs.rmSync(base, { recursive: true, force: true }); } }); });