Repository navigation
Conversation
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#5037): refuse or write through a symlinked settings.json
Summary
Root cause identified correctly: writeSettings' temp-file rename replaced a symlinked settings.json. The fix is placed in writeSettings, so all six call sites are covered, and the #338 migration pre-check avoids a half-migration. Tests assert the persisted state, and the refusal cases pin the exit code and the stderr message.
Classification & gate compliance
Bug-Fix track (#5037 is confirmed-bug). Fixes keyword, conventional title, changeset (Fixed, pr 5300), CONTEXT.md and docs/CONFIGURATION.md are all present.
Root-cause verdict
Root-cause fix.
Blast radius
writeSettings is called at bin/install.js:9204, 9660, 13242, 13264 and 13661. Users who already set GSD_ALLOW_SYMLINKED_DEST=1 now write through to the target, and a read-only target will fail. The PR documents this.
Test quality
Good. No source-grep, timing or vacuous assertions.
Findings
- Minor, bin/install.js:1513: this duplicates the policy of reconcileCodexHooksJsonEvent. Extract a shared guarded-write helper next to hasExistingSymlinkBetween (needed by #5301 and #5315 too), or add a parity test.
- Minor, bin/install.js:13238: add a comment or a named wrapper for the pre-check whose result is discarded.
- Nit: tests/settings-jsonc.test.cjs, dangling test, defers cleanup inside a loop.
- Needs the maintainer to decide on the lstat-vs-existsSync dangling-link difference from the hooks.json writer (see PR body).
- CI: 8 fork runs are action_required (Tests, Install Smoke, Docs Required, Changeset Required, Validate Branch Name, Default Flip Documentation x2, Dependabot Auto-Merge). The matrix has not executed.
- The branch is 40 commits behind next. It merges cleanly; please rebase.
Verdict
Substance clean, awaiting CI.
Memtrace Evidence
The Memtrace MCP server was unavailable for this review (it failed to connect: memtrace not found in PATH), so no graph-backed check ran. None of the four checks below is a clean result.
get_impact: not run. Blast radius comes from reading the diff and grepping callers.get_symbol_context: not run. Callers and callees of the changed symbols were checked by grep.recall_decision: not run. The recorded-decision gate did not execute, so this does not show that no decision governs the change. ADRs andCONTEXT.mdpredicates were checked by grep instead.find_code_review_issues: not run. The deterministic review pass did not run. A manual diff review stood in for it, so the findings above are judgment, not graph facts.
Both installer writes of settings.json, the primary write and the open-gsd#338 local-install migration, replace a symlinked settings.json with a plain file today, with or without GSD_ALLOW_SYMLINKED_DEST. These tests pin the policy the Codex hooks.json writer already follows: refuse the link without the opt-in and name it, write through to the target with it, and leave plain settings files as they were. They fail on next.
writeSettings saved through atomicWriteFileSync, whose final rename onto a symlink replaces the link with a plain file. A dotfiles-managed settings.json was swapped for a plain copy on every install and update, the link's target stopped receiving changes, and nothing was printed, with or without GSD_ALLOW_SYMLINKED_DEST. writeSettings now applies the policy the Codex hooks.json writer already uses. Without the opt-in, hasExistingSymlinkBetween refuses a symlinked settings file and the error names GSD_ALLOW_SYMLINKED_DEST. With it, the write goes to the link's real target and the link stays in place. A dangling link is refused rather than replaced, as the guard treats one elsewhere. Plain and missing files are written exactly as before. The open-gsd#338 local-install migration writes settings.local.json before it strips the shared settings.json, so it now checks the shared file first: a refused link leaves both files untouched instead of half-migrated.
The fragment used a colon after the bold lead and five sentences of mechanism. .changeset/README.md asks for a bold lead, an em-dash separator and the user-visible change in one sentence.
…taller CONTEXT entry writeSettings now refuses or writes through a symlinked settings file, a durable installer invariant the Installer Module entry should name, along with the open-gsd#338 migration's pre-check.
…he hooks.json writer Review asked for one policy instead of two copies. resolveSymlinkedLeafWritePath now sits next to hasExistingSymlinkBetween in install-engine and serves writeSettings and the Codex hooks.json writer; open-gsd#5301 and open-gsd#5315 can reuse it. The hooks.json writer keeps its existsSync gate, so its dangling-link handling is unchanged pending the maintainer's call. The open-gsd#338 migration's pre-check becomes a named assertSettingsWriteAllowed instead of a call whose result is discarded.
…n-file test The looped tests built a layout and registered a cleanup per iteration inside one test. One test per state keeps each to a single layout and cleanup.
5993686 to
532d938
Compare
|
Thanks for the review. Rebased onto
Checks: |
Fix PR
Linked Issue
Fixes #5037
Related: #5179 (see "Interaction with #5179" below; this PR does not change the ordering that issue is about).
What was broken
Every install or update replaced a symlinked
settings.json(a dotfiles layout such as~/.claude/settings.json -> ~/dotfiles/settings.json) with a plain file. GSD's hooks went into the new plain file, the link's target was left as it was, and nothing was printed.GSD_ALLOW_SYMLINKED_DEST=1made no difference.What this fix does
writeSettingsnow applies the symlink policy the Codexhooks.jsonwriter already uses (reconcileCodexHooksJsonEvent). WithoutGSD_ALLOW_SYMLINKED_DEST, a symlinked settings file is refused and the error names the opt-in. With it, the write goes to the link's real target and the link stays a link. Plain and missing settings files are written exactly as before.Root cause
writeSettingssaves throughatomicWriteFileSync, whose last step renames a temp file onto the target path, and a rename onto a symlink replaces the link. None of its call sites ran thehasExistingSymlinkBetween/isSymlinkedDestOptIncheck that the other install writers run first.Details
writeSettingsitself (throughresolveSettingsWritePath, which since the review delegates to install-engine'sresolveSymlinkedLeafWritePath, shared with the Codexhooks.jsonwriter), so every call site is covered: the primary write infinishInstall, both writes of the [--local] installer writes hook wiring to .claude/settings.json instead of .claude/settings.local.json #338 local-settings migration, the uninstall cleanup, andconfigureAntigravityPermissions.settings.local.jsonbefore it strips the sharedsettings.json. It now checks the shared file first, so a refused link leaves both files untouched instead of half-migrated.lstat, so a dangling settings link is refused (with or without the opt-in) instead of being replaced. That is howhasExistingSymlinkBetweentreats a dangling link everywhere else. It differs from thehooks.jsonwriter, whoseexistsSyncgate lets a dangling link through to be replaced by the rename. Happy to align it the other way if you prefer.hasExistingSymlinkBetweenis the settings file's own directory, which is what thehooks.jsonwriter passes (targetDir, the parent ofhooks.json).docs/CONFIGURATION.md: theGSD_ALLOW_SYMLINKED_DESTrow now says a symlinked settings file follows the same rule, since the refusal is new user-visible behavior.CONTEXT.md: the Install Engine and Installer Module entries name the shared helper and the [--local] installer writes hook wiring to .claude/settings.json instead of .claude/settings.local.json #338 migration's pre-check.Interaction with #5179
Making settings.json refusals reachable does not hit the #5179 failure mode. Both settings writes run after
gsd-core/is copied and afterVERSION,.gsd-runtimeandgsd-file-manifest.jsonare written, so a refused run leaves a complete file install whose settings were not updated. Checked against a local checkout in a temp HOME: install, setVERSIONto0.0.1, swapsettings.jsonfor a symlink, rerun without the opt-in (exit 1;VERSIONback at 1.16.0,.gsd-runtimeand the manifest present; link and target unchanged), then rerun withGSD_ALLOW_SYMLINKED_DEST=1(exit 0, no newgsd-local-patchesentries; the only backup is the hand-editedVERSION, which a plain-file control run backs up the same way).What a refused install does leave: new files on disk but hooks not re-registered, until the rerun with the opt-in. Moving this check ahead of the first destructive write belongs with the #5179 preflight, whose brief keeps the settings.json path out of scope, so this PR does not do it. One related ordering note: uninstall cleans
settings.jsonafter it removes GSD's files, so a refused uninstall leaves settings entries that point at removed hooks until it is rerun with the opt-in (today the same run silently replaces the link instead).Same bug elsewhere, not in this PR
Other writers end in the same temp-file rename and are tracked separately, so this PR stays scoped to
writeSettings:GSD_ALLOW_SYMLINKED_DEST=1, a symlinked Codexconfig.tomlis replaced (mergeCodexConfigand the validated write ininstall()). Without the opt-in it is refused correctly.gsd capability install/removewith--shared-filereplace a symlinkedsettings.json, with or without the opt-in (applyCapabilitySharedEdits/stripCapabilitySharedEdits).hooks.jsonwriters and the Kimi hooks writer have no symlink check (noted in bug(install): with GSD_ALLOW_SYMLINKED_DEST=1, a symlinked Codex config.toml is replaced with a plain file #5301).Testing
How I verified the fix
Reproduced on
next(651bd2f) with the issue's steps against the local checkout, in a temp HOME: the installer exits 0,settings.jsonbecomes a regular file and the dotfiles copy is unchanged, both with and withoutGSD_ALLOW_SYMLINKED_DEST=1.With the fix, the same steps without the opt-in exit 1 with:
and the link and its target are unchanged. With
GSD_ALLOW_SYMLINKED_DEST=1the installer exits 0,settings.jsonis still a symlink, and the dotfiles copy holds the GSD hooks.Regression tests:
tests/settings-jsonc.test.cjs,writeSettings on a symlinked settings file (#5037): refused without the opt-in (the error namesGSD_ALLOW_SYMLINKED_DEST=1, link and target unchanged, no temp residue in either directory); written through with the opt-in; a dangling link refused in both modes; a plain file written in place in both modes.tests/install-regressions.test.cjs,#5037: the installer never replaces a symlinked settings.json: realbin/install.jsruns for the primary write (global install) and the [--local] installer writes hook wiring to .claude/settings.json instead of .claude/settings.local.json #338 migration write (local install), each with and without the opt-in. The refusal cases require exit code 1 and thewriteSettingsrefusal message on stderr, so an install that fails earlier for another reason (for example a checkout withouthooks/dist, which fails entrypoint validation first) cannot pass them. That stderr match is deliberate: the installer exits through an uncaught error and has no typed error surface for a spawned run to assert on, and without it an unrelated early failure satisfied these cases during development. The unit tests insettings-jsonc.test.cjsassert the refusal throughassert.throwsinstead.On
next, 7 of the 8 new tests fail (settings.json must still be a symlink,Missing expected exception); the plain-file test passes there too, as it should. With the fix all 8 pass. Taking out only the migration pre-check failssettings.local.json must not be half-migrated when the shared write is refused.Checks run locally (Node 24.15.0, Linux):
npm run lintclean;npm run lint:ciexit 0;GITHUB_BASE_REF=next node scripts/changeset/lint.cjsandscripts/lint-docs-required.cjspass once the fragment's PR number is filled in;npm test(all 1062 files): 44388 tests, 44349 pass, 0 fail, 38 skipped, 1 todo, exit 0.Commits:
test(#5037)adds the tests alone (7 of 8 fail on that commit),fix(#5037)makes them pass,chore(#5037)adds the changeset and a second one backfills its PR number. Follow-ups: the changeset rewritten to the.changeset/README.mdformat, the Installer Module entry inCONTEXT.md, and after review arefactor(#5037)that shares the policy with thehooks.jsonwriter plus atest(#5037)that splits the looped tests. Rebased ontonextafter review. The gsd-test-runner bench was not run.Rerun after the review changes:
npm run lint:ci, the changeset and docs-required lints andnpm run lint:generated-syncpass.npm testwithHOMEpointed at an empty directory: 44716 tests, 44613 pass, 0 fail, 102 skipped. Run against my real home directory instead, 41 tests in 12 files fail because they read local config (a GSD skill-surface profile and my global git hooks); none of them touch this change.Regression test added?
Platforms tested
Not run on macOS or Windows. The new tests skip when the account cannot create symlinks (unprivileged Windows), as the existing #2393 tests do.
Runtimes tested
The uninstall and Antigravity call sites go through the same
writeSettingsbut were not run end to end.Checklist
Fixes #NNNconfirmed-buglabelnpm test).changeset/fragment addedStandards followed: the
GSD_ALLOW_SYMLINKED_DESTopt-in contract from #2393 as documented onisSymlinkedDestOptIn/hasExistingSymlinkBetween(src/install-engine.cts), and the CONTRIBUTING "Filesystem writes and installers" matrix for the tests (broken symlink, symlink escaping the root, no temp-file residue on the refusal and success paths; a write-through that fails partway is not separately tested).Breaking changes
A user whose
settings.jsonis a symlink and who has not setGSD_ALLOW_SYMLINKED_DESTnow sees the install or update stop with an error that names the opt-in, instead of finishing with the link silently replaced. SettingGSD_ALLOW_SYMLINKED_DEST=1keeps the link and writes through it.Users who already set
GSD_ALLOW_SYMLINKED_DEST=1(for example for #2393) also change behavior: their settings writes now land on the link's target instead of replacing the link, so a target in a read-only location (such as a link into/nix/store) now fails the install with the filesystem's ownEACCES/EROFSerror instead of being silently replaced.