golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

revert: fix(surface) #3666 — CONTRIBUTING.md test-pattern violations (#3688)

Browse Source 
Reverts c5657fcbfd (squash of PR #3666).

The production change was correct in intent. The new test file
tests/surface-state.test.cjs violates two CONTRIBUTING.md rules
that the project enforces specifically because they produce
passing-but-useless tests:

  1. ~15 try { ... } finally { cleanup(dir); } blocks in test bodies
     — CONTRIBUTING.md "Never use try/finally inside test bodies."
     Approved forms are beforeEach/afterEach hooks or t.after(...).

  2. 17 assert.match calls on rendered console.warn prose
     — CONTRIBUTING.md "Prohibited: Raw Text Matching on Test
     Outputs." Tests must assert on a typed structured surface
     (frozen reason enum) instead of the rendered prose, otherwise
     they pass-but-rot the moment a warning string is reworded.

The .changeset/witty-birds-gather.md fragment is also reverted so
the v1.43 changelog does not advertise a fix that is not on main.

A reworked PR addressing items 1-4 from the re-review (linked on
the closed PR thread) is welcome — the intent of the fix is right.

Refs #3666 #3662

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-05-18 00:20:36 -04:00
committed by GitHub
parent c2dc9e2532
commit cb154569cf
3 changed files with 36 additions and 427 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/witty-birds-gather.md
View File

@@ -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)

138
get-shit-done/bin/lib/surface.cjs
View File

@@ -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');
}
// ---------------------------------------------------------------------------

320
tests/surface-state.test.cjs
View File

@@ -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 });
}
});
});