fix(surface): default missing optional fields in readSurface, normalize writeSurface input#3666
Conversation
…iteSurface 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 gsd-build#3662
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughreadSurface now validates JSON root and baseProfile, warns on hard failures and returns null, and normalizes missing/mistyped optional arrays to []. writeSurface enforces non-blank baseProfile, warns on unknown/mistyped fields, normalizes partial input, and atomically writes the normalized four-field payload. Tests and a changeset were updated. ChangesSurface State Validation and Normalization
Sequence Diagram(s)sequenceDiagram
participant Caller
participant readSurface
participant Filesystem
participant JSONparse as JSON.parse
participant normalizeSurfaceState
Caller->>readSurface: readSurface(runtimeConfigDir)
readSurface->>Filesystem: platformReadSync(".gsd-surface.json")
Filesystem-->>readSurface: file contents / ENOENT / read error
readSurface->>JSONparse: JSON.parse(contents)
JSONparse-->>readSurface: parsed value / parse error
readSurface->>normalizeSurfaceState: normalizeSurfaceState(parsedObject)
normalizeSurfaceState-->>readSurface: normalized SurfaceState
readSurface-->>Caller: return normalized SurfaceState or null (with console.warn)
sequenceDiagram
participant Caller
participant writeSurface
participant normalizeSurfaceState
participant Filesystem
Caller->>writeSurface: writeSurface(runtimeConfigDir, surfaceState)
writeSurface->>normalizeSurfaceState: normalizeSurfaceState(surfaceState)
normalizeSurfaceState-->>writeSurface: normalized SurfaceState
writeSurface->>Filesystem: platformWriteSync(JSON.stringify(normalized))
Filesystem-->>writeSurface: write result
writeSurface-->>Caller: return / throw on invalid baseProfile
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@get-shit-done/bin/lib/surface.cjs`:
- Around line 103-104: The current guard allows whitespace-only baseProfile
values (e.g., " ") which later normalize to empty modes; update the validation
in readSurface() and writeSurface() to reject such values by checking typeof
parsed.baseProfile === 'string' && parsed.baseProfile.trim().length > 0 (and the
symmetric check in the writer) instead of just non-empty string, and use the
same trimmed check where baseProfile is written to ensure whitespace-only
profiles are not accepted or emitted.
In `@tests/surface-state.test.cjs`:
- Around line 95-106: The test is creating and cleaning a temp dir manually;
replace the local tmpDir() and fs.rmSync cleanup in the 'corrupt JSON returns
null and warns' test (and the other listed tests) with the shared temp/cleanup
helpers exported from tests/helpers.cjs: import and use the helper that creates
a temporary project and ensures teardown (instead of tmpDir() and manual
fs.rmSync), write the '.gsd-surface.json' into the helper-provided path, and
keep the assertions around readSurface and captureWarn unchanged so cleanup is
automatic via the helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82f717e7-322d-4ed9-bcec-9d1dd607d507
📒 Files selected for processing (3)
.changeset/witty-birds-gather.mdget-shit-done/bin/lib/surface.cjstests/surface-state.test.cjs
…lpers.cjs Applies CodeRabbit findings on PR gsd-build#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 gsd-build#3662
Addressed in 88d1ebd@coderabbitai both findings applied: 1. Whitespace-only 2. Migrate new tests to Re-running review pass please. |
|
Kicking off a fresh review pass now. 🐇 ✨ ✅ Actions performedFull review triggered. |
Applies a Codex review finding on PR gsd-build#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 gsd-build#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 gsd-build#3662
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/surface-state.test.cjs (1)
126-140: 💤 Low valueMissing
warnings.lengthassertion for consistency.The non-string baseProfile test (lines 126-140) asserts
warnings[0]matches/baseProfile/but doesn't verify exactly one warning was emitted, unlike the adjacent tests. Consider addingassert.strictEqual(warnings.length, 1)for consistency with other validation tests.Suggested fix
const { result, warnings } = captureWarn(() => readSurface(dir)); assert.strictEqual(result, null); + assert.strictEqual(warnings.length, 1); assert.match(warnings[0], /baseProfile/);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/surface-state.test.cjs` around lines 126 - 140, Add an assertion that exactly one warning was emitted in the "JSON with non-string baseProfile returns null and warns" test: after calling captureWarn(() => readSurface(dir)) and before cleanup, assert.strictEqual(warnings.length, 1). This ensures consistency with the adjacent validation tests and refers to the existing test block that calls readSurface and checks warnings[0] for /baseProfile/.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/witty-birds-gather.md:
- Line 5: Update the changelog sentence describing readSurface() failures to
include blank/whitespace-only baseProfile cases: change the phrase
“missing/non-string `baseProfile`” to “missing, non-string, or blank (including
whitespace-only) `baseProfile`” so it accurately matches the validation behavior
of readSurface() and writeSurface() regarding `.gsd-surface.json`.
---
Nitpick comments:
In `@tests/surface-state.test.cjs`:
- Around line 126-140: Add an assertion that exactly one warning was emitted in
the "JSON with non-string baseProfile returns null and warns" test: after
calling captureWarn(() => readSurface(dir)) and before cleanup,
assert.strictEqual(warnings.length, 1). This ensures consistency with the
adjacent validation tests and refers to the existing test block that calls
readSurface and checks warnings[0] for /baseProfile/.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 391ba657-decf-467f-9cd4-d46846dd996c
📒 Files selected for processing (3)
.changeset/witty-birds-gather.mdget-shit-done/bin/lib/surface.cjstests/surface-state.test.cjs
Codex review finding addressed in c6aef48Codex flagged a second silent-fall-back path adjacent to the original bug:
Same symptom shape as #3662 (silent profile reversion), different cause. Added a warn-on-unknown layer in both
Four new tests added:
Side note: the pre-existing 44/44 surface tests pass. CI is green on the previous push; awaiting CI on c6aef48. |
…cases CodeRabbit minor finding on PR gsd-build#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.
CodeRabbit minor finding addressed in 5813ed2Changeset wording updated to include blank/whitespace-only and unknown-name cases:
Now matches the implementation across all three commits in this PR. |
Codex review finding on PR gsd-build#3666. The changeset's `pr: 3662` was the linked issue (gsd-build#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.
|
Codex caught a third finding: changeset pr field — ' |
… fields Three Gemini review findings on PR gsd-build#3666 — all spirit-of-gsd-build#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 gsd-build#3662
Gemini review findings addressed in 93816c9Gemini caught three additional spirit-of-#3662 edge cases:
48/48 surface tests pass. Codex's previous LGTM pass covered the same surface; these three additions strengthen the same lenient/loud contract. Total state of this PR:
|
Convergence reached
48/48 surface tests pass locally. Ready for maintainer review. |
Review: NEEDS-CHANGESFix is correct in intent and scope is reasonable, but the test file violates two CONTRIBUTING.md rules that the project enforces specifically because they produce passing-but-useless tests. Critical — must fix before merge1.
The PR adds ~15 new 2. 17 occurrences in the new tests: Fix: surface a frozen reason enum from const READ_FAIL = Object.freeze({ MALFORMED_JSON: 'malformed_json', NON_OBJECT_ROOT: 'non_object_root', BAD_BASE_PROFILE: 'bad_base_profile', UNREADABLE: 'unreadable' });
const WARN = Object.freeze({ UNKNOWN_PROFILE_MODE: 'unknown_profile_mode', WRONG_TYPE_OPTIONAL: 'wrong_type_optional' });Then return / Important3. Scope creep on 4. Suggestions
CodeRabbit status
CodeRabbit did not flag either of the two CONTRIBUTING.md antipatterns above. VerdictConvert test cleanup to |
trek-e
added
area: core
2 participants
Fix PR
Linked Issue
Fixes #3662
The linked issue carries the
confirmed-buglabel (priority: low,area: installer,runtime: claude-code).What was broken
readSurface()inget-shit-done/bin/lib/surface.cjsrejected any.gsd-surface.jsonwhose four expected fields were not all present-and-typed, returningnull. Every caller treatednullas "no surface state," so a partial or hand-edited.gsd-surface.jsonsilently degraded the active profile tofullwith no log line and no error — debugging "mystandardprofile reverted" was effectively blind.What this fix does
disabledClusters,explicitAdds,explicitRemoves) now default to[]. They are meaningfully empty, so a hand-edited surface state with only some fields keeps working.null, but now emit aconsole.warnnaming the file and the reason. The four diagnostic paths: unreadable file (other than ENOENT), malformed JSON, non-object root, missing/non-stringbaseProfile. File-absent stays silent because it's the expected "no surface configured" path.writeSurface()normalizes its input to the full four-field shape before writing, and throwsTypeErrorifbaseProfileis missing or empty. The writer/reader asymmetry that produced the original bug cannot recur — you can't write a partial state from inside the SDK any more.Root cause
writeSurfacedid no shape validation and just stringified whatever object the caller handed it, whilereadSurfaceenforced strict structural validation and silently swallowed any mismatch asnull. Asymmetric writer/reader with a silent failure mode.Testing
How I verified the fix
node --test tests/surface-state.test.cjs— 17/17 pass (was 11).node --test tests/surface-state.test.cjs tests/surface-resolve.test.cjs tests/surface-apply.test.cjs tests/surface-list.test.cjs tests/surface-clusters.test.cjs tests/bug-3562-codex-install-skill-surface.test.cjs— 43/43 pass.npm test— 9975/9978 pass. The two failures (tests/bug-3491-nested-git-worktree.test.cjsandtests/enh-3170-graphify-commit-staleness.test.cjs) are pre-existing onmainbaseline; they fail identically without this change and are unrelated to surface-state IO.explicitRemovesfrom.gsd-surface.jsonnow produces a populatedSurfaceState(withexplicitRemoves: []) andresolveSurfacereturnssurface:standardinstead of falling back toprofile:full.Regression test added?
tests/surface-state.test.cjs:JSON missing baseProfile field returns null and warns (#3662)JSON with non-string baseProfile returns null and warnsJSON with non-object root returns null and warnsmissing optional array field defaults to [] (#3662)all optional arrays missing default to [] (#3662)non-array optional field is coerced to [] (#3662)writeSurface normalizes partial input — all four fields land on disk (#3662)writeSurface rejects missing baseProfile (#3662 writer guard)corrupt JSON returns nullextended to also assert the warn message.captureWarn(fn)helper silences/capturesconsole.warnso test output stays clean.Platforms tested
The fix is filesystem-agnostic; it operates on
path.join+fs.readFileSync+ JSON parsing, no platform-specific code paths.Runtimes tested
~/.claude/skills/.gsd-surface.json)Checklist
Fixes #3662confirmed-buglabelnpm test— pre-existing unrelated failures noted above).changeset/fragment added:.changeset/witty-birds-gather.md(type: Fixed, pr: 3662)Breaking changes
Minor behavioral change, no API break:
writeSurface()now throwsTypeErrorifbaseProfileis missing or empty. All in-tree callers (tests + production) already pass a non-empty string, so no production caller is affected. External callers passing completeSurfaceStateare unaffected. This is the symmetric writer guard the linked issue explicitly requested.readSurface()now emitsconsole.warnon hard validation failures. Previous behavior was silent. Callers depending on absolute silence on bad input will see new stderr output, but the returned value (null) is unchanged..gsd-surface.jsonfiles missing optional array fields used to returnnull; they now return a fully-populated record. This is the user-visible bug fix.No
CHANGELOG.mdbreaking-change entry needed — fields that defaulted tonullnow default to a usable record.Summary by CodeRabbit
Bug Fixes
Tests
Documentation