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

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

Browse Source 
* surface: default missing optional fields in readSurface, normalize writeSurface input

readSurface used to reject any .gsd-surface.json missing one of its four fields
and return null with no diagnostic, so the active surface silently degraded to
the 'full' profile. Optional array fields (disabledClusters, explicitAdds,
explicitRemoves) now default to [] when missing or wrong-typed; hard failures
(malformed JSON, non-object root, missing/non-string baseProfile) still return
null but emit a console.warn naming the file + reason.

writeSurface now normalizes its input to the full SurfaceState shape and
throws on missing baseProfile, so partial writes can no longer land on disk
and trip readSurface later. Tests extended to cover the new lenient and
warn-on-hard-fail behavior plus the writer guard.

Fixes #3662

* surface: reject whitespace-only baseProfile + migrate new tests to helpers.cjs

Applies CodeRabbit findings on PR #3666:

1. readSurface and writeSurface now reject baseProfile values that are blank
   after trim() (e.g. "   "), not only the empty string. Whitespace-only
   strings would split-by-comma to [''] downstream and silently produce an
   unresolvable profile mode. Both guards updated symmetrically; warn/error
   messages reworded to "missing, non-string, or blank".

2. tests/surface-state.test.cjs now uses createTempDir + cleanup from
   tests/helpers.cjs instead of local mkdtempSync + fs.rmSync, aligning with
   the repo coding guideline for root-level tests. The local tmpDir() helper
   delegates to createTempDir for backward-compat with the existing test
   bodies. Per-test cleanup calls swapped to cleanup(dir).

3. Added regression tests:
   - readSurface rejects whitespace-only baseProfile and warns
   - writeSurface rejects whitespace-only baseProfile, non-string baseProfile,
     and null surfaceState

Refs #3662

* surface: warn on unknown baseProfile mode names in read and write

Applies a Codex review finding on PR #3666:

readSurface and writeSurface used to accept any non-blank string as
baseProfile. A typo like {"baseProfile":"standrad"} would pass validation,
then resolveProfile() in install-profiles.cjs would silently fall back to
'full' with no diagnostic — the same silent-degradation symptom that #3662
was filed to fix, just through a different code path.

Both functions now split baseProfile by comma, validate each mode against
the registered PROFILES set ('core', 'standard', 'full'), and emit a single
[gsd] console.warn line that names the unknown modes and lists the valid
ones. The state is still parsed/written — resolveProfile() decides the
actual resolution fallback. Composed profiles where some modes are valid
and some are not warn only about the unknown subset.

Side note: the pre-existing 'round-trips composed base profile' test used
'core,audit' as a stand-in composed string. 'audit' is not a registered
profile (the three known profiles are 'core', 'standard', 'full'), so the
test was relying on the old lack of validation. Switched to 'core,standard'
to preserve the round-trip intent without producing diagnostic noise.

Refs #3662

* changeset: include blank/typo baseProfile in documented read failure cases

CodeRabbit minor finding on PR #3666 — the changeset wording only mentioned
"missing/non-string baseProfile" but the implementation also rejects blank
(including whitespace-only) baseProfile values, and warns on typo'd /
unknown profile mode names. Updated to match actual behavior.

* changeset: pr field should be PR number, not issue number

Codex review finding on PR #3666. The changeset's `pr: 3662` was the linked
issue (#3662), but the convention across other .changeset/*.md files is that
`pr:` carries the PR number. Verified by spot-checking other changesets
(2937 → pr: 3515, 3298 → pr: 3306, 3541 → pr: 3547 — all PR numbers).
Updated to `pr: 3666`. The body text still references the issue.

* surface: reject comma-only baseProfile + warn on wrong-typed optional fields

Three Gemini review findings on PR #3666 — all spirit-of-#3662 edge cases:

1. Comma-only baseProfile bypasses validation. readSurface used to accept
   baseProfile: ", ," because trim() returned "," (non-empty). Downstream
   resolveProfile() would split-and-filter to [] and silently fall back to
   'full'. Added effectiveProfileModes() helper that splits, trims, filters
   empty — both readSurface and writeSurface now reject when the result is
   empty. Same silent-degradation symptom as the original bug.

2. Wrong-typed optional fields silently coerced. readSurface used to coerce
   {disabledClusters: 42} to {disabledClusters: []} with no diagnostic.
   Now warns via mistypedOptionalFields() before normalizeSurfaceState()
   does the coercion, in both reader and writer.

3. Missing test coverage for the EACCES branch in readSurface. Added a
   chmod-000 unreadable-file test, skipped on Windows and root accounts
   (mode bits are ignored on those platforms).

48/48 surface tests pass. Final codex pass returned LGTM on the prior
state; these three additions strengthen the same lenient/loud contract.

Refs #3662
This commit is contained in:
Cristian Uibar
2026-05-18 07:14:07 +03:00
committed by GitHub
parent 7e6ba56985
commit c5657fcbfd
3 changed files with 428 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

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