feat(config): follow XDG Base Directory spec for config location - #1503
Conversation
Resolve the config/data directory via the XDG Base Directory specification instead of always using `~/.sentry`. Precedence: 1. `SENTRY_CONFIG_DIR` override (unchanged, highest priority) 2. Legacy `~/.sentry` when it already exists (no breakage for existing installs) 3. `/sentry`, defaulting to `~/.config/sentry` A non-absolute `XDG_CONFIG_HOME` is ignored per the spec. This keeps credentials and caches out of the home directory root, which also unblocks environments (e.g. coding agents) that restrict writes to `/root`. Fixes #1502
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
CI green on the feature branch (check_suite success). Self-review clean. Marking ready and requesting review from the issue author. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…onfig.json A bare ~/.sentry/bin created by the curl installer should not prevent new XDG-based installs. The legacy check now requires the presence of the actual database or the old JSON config file. Fixes the Cursor Bugbot report on PR #1503.
|
Jared, we need to do the following:
|
|
ack — will update binary install paths to follow XDG (via resolveConfigDir) and add migration logic inside |
|
Jared, update the patch already? |
|
working on it — expanding to XDG-aware binary paths + migration inside setup. will push the update shortly. |
|
yep, expanding the patch now to also move binary install paths to XDG (via XDG_BIN_HOME / ~/.local/bin) and add migration inside setup. will push the update. |
|
expanding the patch now to cover XDG binary paths + migration in setup. pushing shortly. |
|
not yet — expanding determineInstallDir + setup migration for binary paths now. will push once the XDG-aware logic + migration is in. |
|
Jared, update the patch based on our earlier discussions |
…acy layout Extend the XDG work to the binary install directory and add automatic migration of the legacy ~/.sentry layout in `sentry cli setup`. - determineInstallDir now honors an absolute XDG_BIN_HOME (after SENTRY_INSTALL_DIR) and falls back to ~/.local/bin instead of ~/.sentry/bin. - setup migrates an existing ~/.sentry/bin binary and legacy config (cli.db, config.json) into the XDG locations on first run; skipped when the target already exists. - upgrade's known-curl-path detection and fallback install path track the same XDG-aware resolution. - install script and getCurlInstallPaths recognize XDG_BIN_HOME. - resolveXdgConfigDir exposes the XDG target (bypassing legacy detection) so migration doesn't no-op. - docs: document binary install location + migration. Addresses review feedback on #1503.
|
pushed in 90c9cb2 — expanded per your two points:
tests: new XDG cases in binary.test.ts + a migration suite in setup.test.ts; full run green except one pre-existing root-only failure ( |
Reevaluated after scope expanded to include binary install paths. - Close the SQLite DB before renaming cli.db/WAL sidecars. The DB is opened at startup (cleanup-old-binary), and an open file cannot be renamed on Windows — previously the whole migration threw and was swallowed. Split config and binary migration into separate functions with independent try/catch so one failing can't skip the other. - migrateLegacyBinary now returns the new path; setup adopts it as binaryPath/binaryDir so PATH modification and setInstallInfo point at the migrated binary instead of the deleted legacy path (a --method setInstallInfo no longer overwrites it with the old location). - docs: clarify that 'sentry upgrade' keeps the binary in place (pins SENTRY_INSTALL_DIR) and only 'sentry cli setup' relocates it; config data still migrates on upgrade. - test: record-new-path regression covering the stale-path fix. Addresses cursor[bot] review on #1503.
# Conflicts: # packages/cli/plugins/sentry-cli/skills/sentry-cli/references/dashboard.md
|
merged latest main into the branch in 4148c42 — the conflict (a generated skill reference doc) is resolved and generated files are regenerated against the merged sources. PR is mergeable again; tsc + setup/binary/config tests green locally. |
BYK
left a comment
BYK
left a comment
There was a problem hiding this comment.
I think we also need one e2e migration test to make sure the migration is actually working.
- Binary migration is no longer hardcoded to ~/.sentry/bin: findMigratableBinary scans getKnownInstallDirs() (derived from KNOWN_CURL_DIRS) for a binary in any known install dir other than the resolved target. - Deduplicate path logic: samePath and LEGACY_INSTALL_SUBDIR now live in binary.ts and are shared by setup and upgrade (removed the duplicate samePath in upgrade.ts). - Use async node:fs/promises (mkdir/copyFile/chmod/rename/unlink) in the migration paths. - Report migration failures to Sentry: both migrations now run through bestEffort(), which warns the user and captureException()s while never aborting setup. - Add an e2e migration test (test/e2e/migration.test.ts) that spawns the real CLI and verifies both the config DB and a legacy ~/.sentry/bin binary migrate to the XDG locations. Plus unit tests for samePath / getKnownInstallDirs. Addresses BYK's CHANGES_REQUESTED review on #1503.
|
Thanks — all five review comments are addressed in b03db3b, and threads resolved:
PR description updated to match. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Address BugBot + Seer findings on the latest revision. - BugBot (high): findMigratableBinary scanned all known install dirs, so a stray binary in ~/.local/bin or ~/bin — both valid *current* XDG targets — could be moved out and the original deleted, leaving the real ~/.sentry/bin install behind. Replace getKnownInstallDirs with getLegacyInstallDirs, which is limited to the genuinely pre-XDG ~/.sentry/bin. Add setup tests asserting binaries in ~/.local/bin and ~/bin are never relocated. - Seer (medium): isInPath used a case-sensitive membership check; on Windows/ macOS a PATH entry can differ only in casing from a computed dir. Compare case-insensitively on those platforms, matching samePath. Add isInPath tests. Unit + e2e migration tests green; tsc + biome clean.
BYK
left a comment
BYK
left a comment
There was a problem hiding this comment.
Would also be great if the tests too used node:fs/promises variants rather than the sync ones.
… list Address BYK review comments. - samePath: collapse to a single expression using a module-level CASE_INSENSITIVE_PLATFORMS Set instead of an if/branch. - isInPath (shell.ts): reuse samePath from binary.ts instead of duplicating the case-insensitive PATH comparison. - getLegacyInstallDirs: back it with a LEGACY_INSTALL_SUBDIRS array (currently the single pre-XDG ~/.sentry/bin) so the 'list' is real and extensible, matching the doc comment. No behavior change; tsc + biome clean, unit + e2e migration tests green.
The ~/.local/bin and ~/bin candidate check used pathDirs.includes(dir), a case-sensitive comparison. On Windows/macOS a PATH entry that differs only in casing from the computed dir would miss, falling back to ~/.local/bin and prompting a PATH edit when a valid dir was already present. Use samePath (case-insensitive on win32/darwin) for consistency with isInPath and the rest of the PR. Add a test covering the mixed-case PATH entry.
…paths getKnownCurlPaths appended `sep` to a raw XDG_BIN_HOME, so a value with a trailing slash (e.g. /custom/bin/) produced /custom/bin// — a double separator that made process.execPath.startsWith(dir) miss, breaking curl install-method detection and upgrade path resolution. Extract the logic into a pure, testable buildKnownCurlPaths(homeDir, env) and normalize the XDG entry with join(xdgBinHome, '.') + sep so exactly one trailing separator is emitted. Add unit tests covering the trailing-slash, absolute, and non-absolute cases.
…erant Address BYK's review plus an independent adversarial review pass. - BYK: process.platform never changes at runtime, so compute IS_CASE_INSENSITIVE_FS once at module load (dropping the Set) and pick the case-folding step statically — no per-call platform check. - Adversarial review: samePath now strips a trailing separator (never from a bare root) before comparing, so a PATH entry like ~/.local/bin/ matches the computed ~/.local/bin. This fixes false negatives across all samePath call sites (isInPath, determineInstallDir PATH matching, upgrade relocation). - install script: only honor XDG_BIN_HOME when absolute, mirroring the Node determineInstallDir logic. - Tests: trailing-separator and root-not-stripped cases for samePath. tsc + biome clean; unit + e2e migration tests green.
|
Status on the latest push (9a84530):
I've re-requested your review since all threads are now resolved. I'm holding off on merging: the PR is |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9a84530. Configure here.
…ariants - BugBot: the install-script absolute-path guard only accepted POSIX /… paths, dropping a Windows drive-letter XDG_BIN_HOME (C:\… or C:/…) while Node's determineInstallDir (isAbsolute) still installs there — so SENTRY_INIT=1 couldn't find the binary. Accept drive-letter absolute paths too. - Per review: drop the foldCase helper and select the whole samePath implementation statically from IS_CASE_INSENSITIVE_FS (toLowerCase variant vs plain comparison).
1 participant
Closes #1502
What
sentrystored its config/data (thecli.dbSQLite database with credentials and caches) and its installed binary under~/.sentry, cluttering the home directory and breaking in environments that block writes to$HOME(e.g. sandboxed coding agents).This makes both the config directory and the binary install directory follow the XDG Base Directory specification, matching how the CLI already resolves shell/completion paths, and migrates existing
~/.sentryinstalls into the new locations on firstsetup.Config directory
getConfigDir()resolves via a pureresolveConfigDir(env, home)helper:SENTRY_CONFIG_DIR— explicit override (highest priority)~/.sentry— used only when it actually holds config (cli.dborconfig.json), so a bare~/.sentry/binfrom the installer no longer blocks XDG$XDG_CONFIG_HOME/sentry— defaulting to~/.config/sentry. A non-absoluteXDG_CONFIG_HOMEis ignored per the spec.resolveXdgConfigDir()exposes the XDG target directly (bypassing legacy detection) so migration doesn't no-op whilecli.dbstill sits in~/.sentry.Binary install directory
determineInstallDir()now resolves:SENTRY_INSTALL_DIR— explicit override$XDG_BIN_HOME— when set to an absolute path, per the XDG spec~/.local/binor~/bin— when either exists and is already onPATH~/.local/bin— default fallback (previously~/.sentry/bin)upgrade's known-curl-path detection and its fallback install path track the same resolution, and the install script recognizesXDG_BIN_HOME.Migration (in
sentry cli setup)On first run,
setupmigrates the legacy~/.sentrylayout into the XDG locations. Config and binary migration are independent (a failure in one can't skip the other):cli.db+ WAL sidecars andconfig.jsoninto the XDG config dir. Skipped when a targetcli.dbalready exists.getLegacyInstallDirs(), currently just the pre-XDG~/.sentry/bin) and, when found, copies it into the resolved install target,chmods it executable, records the new path viasetInstallInfo, and removes the old copy.~/.local/binand~/binare valid current XDG targets, so they are never treated as migration sources.setupthen adopts the new path soPATHsetup and recorded install metadata point at the migrated binary, not the deleted legacy location. Skipped when a binary already exists at the target.Both migrations use async
node:fs/promisesand run through the sharedbestEffort()helper, so any failure is surfaced as a warning and reported to Sentry (captureException) without ever aborting setup.sentry upgraderunssetupon the new binary, so it migrates too — but conservatively, because upgrade never editsPATH(it runs--no-modify-path). A legacy~/.sentry/binbinary is relocated to the XDG install dir only when that dir is already onPATH(resolveUpgradeInstallDir), so the moved binary stays discoverable; setup's legacy-binary migration moves the old binary and removes it before--installwrites the new one. If the XDG dir isn't onPATH, upgrade leaves the binary in place — runsentry cli setupexplicitly to relocate it and updatePATH. Legacy config is migrated on upgrade regardless.Changes
packages/cli/src/lib/db/index.ts—resolveConfigDir+ newresolveXdgConfigDirhelper.packages/cli/src/lib/binary.ts— XDG-awaredetermineInstallDir(XDG_BIN_HOME,~/.local/binfallback).packages/cli/src/lib/upgrade.ts— known-curl-path detection + fallback install path track the XDG resolution.packages/cli/src/lib/binary.ts— sharedsamePath,LEGACY_INSTALL_SUBDIR, andgetLegacyInstallDirs()(used by both setup and upgrade).packages/cli/src/commands/cli/setup.ts—migrateLegacyConfig/migrateLegacyBinary(async,bestEffort-wrapped), adopted into the setup flow.packages/cli/install— install script recognizesXDG_BIN_HOME.test/lib/config.test.ts,test/lib/binary.test.ts(XDG cases +samePath/getLegacyInstallDirs),test/commands/cli/setup.test.ts(migration + records-new-path + executability),test/commands/cli/upgrade.test.ts(resolveUpgradeInstallDir), andtest/e2e/migration.test.ts(spawns the real CLI and verifies config-DB + legacy-binary migration end-to-end).apps/cli-docs/src/fragments/configuration.mddocuments config location, binary install location, and migration behavior.Testing
vitest runfor config / binary / setup suites is green (one pre-existing root-onlyacquireLockpermission test fails onmaintoo, since root ignoreschmod 0o000).tsc --noEmitclean; Biome clean on changed files.Review follow-ups addressed
copyFileSyncpermission concern verified (mode is preserved) and hardened with an explicitchmod+ test.