refactor: use XDG base dirs for config and data#410
Open
JayYoung2021 wants to merge 1 commit intolarksuite:mainfrom
Open
refactor: use XDG base dirs for config and data#410JayYoung2021 wants to merge 1 commit intolarksuite:mainfrom
JayYoung2021 wants to merge 1 commit intolarksuite:mainfrom
Conversation
|
yi.yang08 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes CLI filesystem path logic into a new Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR extracts all application directory resolution into a new Confidence Score: 5/5Safe to merge — all P0/P1 concerns from the prior review thread have been addressed and no new critical issues were found. The two P1 findings from the previous thread (env vars accepted without validation in ConfigDir/CacheDir/StateDir; silent validation failure with no user feedback) are both fixed in this revision. The remaining open question (legacy data migration for non-Darwin platforms) was already discussed in the previous thread. The only new finding is a P2 style issue with a redundant control-character check in SafeServiceName. All path changes are backward-compatible for existing users thanks to the legacyConfigDir fallback preserving the ~/.lark-cli base. internal/appdir/appdir.go — Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Directory request e.g. ConfigDir] --> B{LARKSUITE_CLI_*_DIR set?}
B -- valid absolute path --> C[Use override dir]
B -- invalid / relative --> D[Print warning, fall through]
B -- not set --> E{XDG_*_HOME set?}
D --> E
E -- valid --> F[Use XDG base + appName]
E -- invalid --> G[Print warning, fall through]
E -- not set --> H{~/.lark-cli exists?}
G --> H
H -- yes --> I[Use legacy dir ~/.lark-cli]
H -- no --> J[Use platform default e.g. ~/.config/lark-cli]
Reviews (7): Last reviewed commit: "refactor: adopt XDG base directory speci..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (2)
internal/auth/app_registration.go (1)
43-43: Remove the ignorederrOutargument fromRequestAppRegistration.Line 43 keeps a writer parameter but discards it, while callers (e.g.,
cmd/config/init_interactive.go) still passf.IOStreams.ErrOut. This is a confusing API contract; either use it or remove it from signature and call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/app_registration.go` at line 43, The RequestAppRegistration function currently declares an unused writer parameter (errOut)—remove that parameter from RequestAppRegistration's signature and all call sites (e.g., where cmd/config/init_interactive.go passes f.IOStreams.ErrOut) so callers invoke RequestAppRegistration(httpClient, brand) instead; update any imports/usages accordingly and run tests/compile to find and fix any remaining references to the removed parameter name.internal/lockfile/lockfile.go (1)
31-33: Update the lock path docstring to reflectStateDir.The comment says
{configDir}/locks/..., but Line 42 now builds the directory fromappdir.StateDir().Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/lockfile/lockfile.go` around lines 31 - 33, The docstring for ForSubscribe is out of date: it references "{configDir}/locks/..." but the code constructs the path from appdir.StateDir(); update the comment for ForSubscribe to state the lock path uses StateDir (e.g., "{stateDir}/locks/subscribe_{appID}.lock") and any similar docstring lines so they match the actual implementation that calls appdir.StateDir().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/appdir/appdir.go`:
- Around line 21-23: The code currently returns raw
os.Getenv("LARKSUITE_CLI_CONFIG_DIR") in the config/cache/state path getters,
bypassing safe-path checks; change those getters to validate the environment
value before use by either reusing the existing validatedEnvDir helper (like
LogDir/DataDir do) or by passing the env value through validate.SafeInputPath
and returning the sanitized/fallback path when validation fails; update the
functions that read LARKSUITE_CLI_CONFIG_DIR (the config/cache/state path
getters in appdir.go) to perform this validation instead of returning the raw
env string.
In `@internal/registry/remote.go`:
- Line 239: The progress message is currently written via
fmt.Fprintln(log.Writer(), "Fetching API metadata...") which can send progress
to stdout if logs are redirected; replace that call to write explicitly to
stderr (e.g. fmt.Fprintln(os.Stderr, "Fetching API metadata...")) and add the
necessary import for os so progress/warning output is always on stderr rather
than the log writer.
---
Nitpick comments:
In `@internal/auth/app_registration.go`:
- Line 43: The RequestAppRegistration function currently declares an unused
writer parameter (errOut)—remove that parameter from RequestAppRegistration's
signature and all call sites (e.g., where cmd/config/init_interactive.go passes
f.IOStreams.ErrOut) so callers invoke RequestAppRegistration(httpClient, brand)
instead; update any imports/usages accordingly and run tests/compile to find and
fix any remaining references to the removed parameter name.
In `@internal/lockfile/lockfile.go`:
- Around line 31-33: The docstring for ForSubscribe is out of date: it
references "{configDir}/locks/..." but the code constructs the path from
appdir.StateDir(); update the comment for ForSubscribe to state the lock path
uses StateDir (e.g., "{stateDir}/locks/subscribe_{appID}.lock") and any similar
docstring lines so they match the actual implementation that calls
appdir.StateDir().
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e9dc249-4cc7-436d-90d7-8aa3a1b02325
📒 Files selected for processing (15)
cmd/auth/login_scope_cache.gointernal/appdir/appdir.gointernal/appdir/appdir_test.gointernal/auth/app_registration.gointernal/auth/device_flow.gointernal/auth/transport.gointernal/auth/uat_client.gointernal/auth/uat_client_options_test.gointernal/core/config.gointernal/keychain/auth_log.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_other.gointernal/lockfile/lockfile.gointernal/registry/remote.gointernal/update/update.go
JayYoung2021
force-pushed
the
refactor/adopt-xdg-for-config-and-data
branch
from
ecbb1fb to
3997c66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/update/update.go (1)
163-164: Use one computed path insaveState()to avoid split mkdir/write targets.Line 163 derives
dironce, but Line 171 recomputesstatePath(). Building the file path fromdirkeeps both operations consistent.♻️ Proposed refactor
func saveState(s *updateState) error { dir := appdir.StateDir() + path := filepath.Join(dir, stateFile) if err := vfs.MkdirAll(dir, 0o700); err != nil { return err } data, err := json.Marshal(s) if err != nil { return err } - return validate.AtomicWrite(statePath(), data, 0o644) + return validate.AtomicWrite(path, data, 0o644) }Also applies to: 171-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 163 - 164, In saveState(), avoid recomputing the path twice: call appdir.StateDir() once, derive the full state path via statePath(dir) (or build the file path from dir) and use that single computed path for the vfs.MkdirAll and subsequent write operations; update references to statePath() in saveState() so both the mkdir and file-write target the same path and remove the redundant recomputation.internal/registry/remote_test.go (1)
138-139: Fail fast on fixture setup errors in tests.
os.MkdirAll/os.WriteFilereturn values are ignored in several setup blocks. Please assert these errors to avoid opaque test failures.Also applies to: 158-163, 195-200, 413-417, 459-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/remote_test.go` around lines 138 - 139, The test fixture setup currently ignores errors from os.MkdirAll and os.WriteFile (e.g., the calls in internal/registry/remote_test.go that create cDir and write remote_meta.json); change each ignored call to capture the returned error and assert it (e.g., use t.Fatalf or require.NoError) so setup failures fail fast and reveal the underlying error; apply this fix to every occurrence of os.MkdirAll/os.WriteFile in the file including the blocks noted (around the shown call and at the ranges 158-163, 195-200, 413-417, 459-463).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/vfs/localfileio/path_test.go`:
- Line 111: The test fixture setup currently ignores errors from os.MkdirAll and
os.WriteFile which can hide setup failures; update the two offending calls that
create the "real" directory and write fixture files to check their returned
error and call t.Fatalf (or t.Fatal) with a clear message including the error
(e.g., t.Fatalf("os.MkdirAll(...): %v", err) / t.Fatalf("os.WriteFile(...): %v",
err)). Ensure both occurrences are updated so test setup fails fast with the
underlying error instead of proceeding.
---
Nitpick comments:
In `@internal/registry/remote_test.go`:
- Around line 138-139: The test fixture setup currently ignores errors from
os.MkdirAll and os.WriteFile (e.g., the calls in
internal/registry/remote_test.go that create cDir and write remote_meta.json);
change each ignored call to capture the returned error and assert it (e.g., use
t.Fatalf or require.NoError) so setup failures fail fast and reveal the
underlying error; apply this fix to every occurrence of os.MkdirAll/os.WriteFile
in the file including the blocks noted (around the shown call and at the ranges
158-163, 195-200, 413-417, 459-463).
In `@internal/update/update.go`:
- Around line 163-164: In saveState(), avoid recomputing the path twice: call
appdir.StateDir() once, derive the full state path via statePath(dir) (or build
the file path from dir) and use that single computed path for the vfs.MkdirAll
and subsequent write operations; update references to statePath() in saveState()
so both the mkdir and file-write target the same path and remove the redundant
recomputation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fe62927-9c2c-464b-bf79-41c3bd31e888
📒 Files selected for processing (22)
cmd/auth/login_scope_cache.gointernal/appdir/appdir.gointernal/appdir/appdir_test.gointernal/auth/app_registration.gointernal/auth/device_flow.gointernal/auth/transport.gointernal/auth/uat_client.gointernal/auth/uat_client_options_test.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_http_test.gointernal/core/config.gointernal/keychain/auth_log.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_other.gointernal/lockfile/lockfile.gointernal/registry/loader.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/update/update.gointernal/validate/path.gointernal/vfs/localfileio/path.gointernal/vfs/localfileio/path_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/lockfile/lockfile.go
- internal/auth/uat_client.go
- internal/auth/device_flow.go
🚧 Files skipped from review as they are similar to previous changes (9)
- cmd/auth/login_scope_cache.go
- internal/auth/uat_client_options_test.go
- internal/keychain/auth_log.go
- internal/keychain/keychain_other.go
- internal/core/config.go
- internal/auth/app_registration.go
- internal/keychain/keychain_darwin.go
- internal/appdir/appdir.go
- internal/appdir/appdir_test.go
JayYoung2021
force-pushed
the
refactor/adopt-xdg-for-config-and-data
branch
2 times, most recently
from
8b01e82 to
eb8a7ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/appdir/appdir.go`:
- Around line 114-120: xdgDataDir and defaultDataDir currently join paths as
<base>/<service>, causing XDG spec and collision issues; update both functions
to namespace under the appName constant by joining as <base>/<appName>/<service>
(or <base>/<appName> when service is empty) so paths match
ConfigDir/CacheDir/StateDir behavior and avoid collisions—locate the xdgDataDir
and defaultDataDir functions and change their filepath.Join logic to include
appName between base and service.
In `@internal/vfs/localfileio/path.go`:
- Around line 125-142: SafeServiceName currently relies on
charcheck.RejectControlChars which allows tab/newline characters; update
SafeServiceName to explicitly reject ASCII horizontal tab and newline (and
carriage return) so names cannot contain embedded tabs/newlines. Add a check in
SafeServiceName (after the empty/traversal/separator checks, before or after
calling charcheck.RejectControlChars) that detects any of '\t', '\n', '\r' (or
use strings.ContainsAny(name, "\t\n\r")) and returns a clear error like "service
name must not contain tabs or newlines". Keep the existing call to
charcheck.RejectControlChars for other control chars.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7ce3656-2dce-4d0f-bac8-a35f84422e88
📒 Files selected for processing (24)
cmd/auth/login_scope_cache.gointernal/appdir/appdir.gointernal/appdir/appdir_test.gointernal/auth/app_registration.gointernal/auth/device_flow.gointernal/auth/transport.gointernal/auth/uat_client.gointernal/auth/uat_client_options_test.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_http_test.gointernal/core/config.gointernal/keychain/auth_log.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_darwin_test.gointernal/keychain/keychain_other.gointernal/keychain/keychain_other_test.gointernal/lockfile/lockfile.gointernal/registry/loader.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/update/update.gointernal/validate/path.gointernal/vfs/localfileio/path.gointernal/vfs/localfileio/path_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/lockfile/lockfile.go
- internal/registry/remote.go
- internal/cmdutil/factory_default.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/auth/transport.go
- internal/auth/uat_client_options_test.go
- internal/cmdutil/factory_http_test.go
- internal/keychain/auth_log.go
- internal/auth/device_flow.go
- internal/registry/remote_test.go
- internal/keychain/keychain_other.go
- internal/auth/app_registration.go
- internal/appdir/appdir_test.go
JayYoung2021
force-pushed
the
refactor/adopt-xdg-for-config-and-data
branch
from
eb8a7ba to
782032b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/keychain/keychain_darwin.go (1)
280-295:⚠️ Potential issue | 🟠 MajorFail fast on storage-dir errors before touching the macOS keychain.
platformSetcan callgetMasterKey(service, true)before it proves thatStorageDir(service)is valid and the destination directory can be created. If either later step fails, the method returns an error after already creatingmaster.keyin the system keychain.Suggested fix
func platformSet(service, account, data string) error { + dir, err := StorageDir(service) + if err != nil { + return err + } + if err := vfs.MkdirAll(dir, 0o700); err != nil { + return err + } + key, err := getFileMasterKey(service, false) if err != nil { key, err = getMasterKey(service, true) if err != nil { key, err = getFileMasterKey(service, true) if err != nil { return err } } } - dir, err := StorageDir(service) - if err != nil { - return err - } - if err := vfs.MkdirAll(dir, 0o700); err != nil { - return err - } encrypted, err := encryptData(data, key) if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/keychain/keychain_darwin.go` around lines 280 - 295, platformSet currently may create a system keychain master key (via getMasterKey(service, true) / getFileMasterKey(service, true)) before validating the storage directory; move the StorageDir(service) & vfs.MkdirAll(dir, 0o700) checks to occur before any call that can create keys so failures fail fast. Concretely: in platformSet, after attempting getFileMasterKey(service, false) and seeing it missing, first call StorageDir(service) and ensure vfs.MkdirAll succeeds; only then call getMasterKey(service, true) and the subsequent getFileMasterKey(service, true) to create/write master.key. This prevents creating master.key in the macOS keychain when storage setup fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/login_scope_cache.go`:
- Around line 47-54: Before performing file I/O, validate the computed paths by
calling validate.SafeInputPath on the results of loginScopeCacheDir() and
loginScopeCachePath(deviceCode); if validation fails, return the error.
Specifically, call validate.SafeInputPath(loginScopeCacheDir()) before
vfs.MkdirAll(...) and call
validate.SafeInputPath(loginScopeCachePath(deviceCode)) before
validate.AtomicWrite(...), propagating any validation error instead of
proceeding with MkdirAll or AtomicWrite.
In `@internal/keychain/auth_log_test.go`:
- Around line 28-33: The test TestAuthLogDir_InvalidLogDirFallsBackToStateDir
ignores errors from filepath.EvalSymlinks and doesn't isolate config state;
update the test to check and handle the error returned by filepath.EvalSymlinks
instead of discarding it, and set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() via
t.Setenv to ensure deterministic config-state isolation; locate the logic in
TestAuthLogDir_InvalidLogDirFallsBackToStateDir and modify the call to
filepath.EvalSymlinks to capture and fail the test on error, and add
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before exercising the code.
In `@internal/keychain/auth_log.go`:
- Line 127: The panic warning currently writes to log.Writer(), which may not be
stderr; change the fmt.Fprintf call that prints "[lark-cli] [WARN] background
log cleanup panicked: %v\n" to explicitly write to os.Stderr (e.g.,
fmt.Fprintf(os.Stderr, ...)), and add the necessary os import; target the
fmt.Fprintf line in the background log cleanup panic handler so warnings are
guaranteed to go to stderr.
In `@internal/keychain/keychain_other_test.go`:
- Around line 35-41: The test fails to guarantee the HOME-based fallback because
XDG_DATA_HOME may be set in the environment; before calling StorageDir("svc") in
internal/keychain/keychain_other_test.go update the test to clear XDG_DATA_HOME
(e.g. use t.Setenv("XDG_DATA_HOME", "") or t.Unsetenv equivalent) so
StorageDir's HOME fallback branch (in internal/appdir/appdir.go) is exercised
and the expected `want` remains correct; keep the existing t.Setenv("HOME",
home) and StorageDir("svc") call as-is.
In `@internal/update/update.go`:
- Line 147: The computed state path produced by filepath.Join(appdir.StateDir(),
stateFile) is used directly in vfs.MkdirAll, vfs.ReadFile and
validate.AtomicWrite without validating it; before any of those calls (and
wherever the state path is constructed/returned), run validate.SafeInputPath on
the resulting path (or the directory part) and return/handle an error if
validation fails so you never call vfs.MkdirAll, vfs.ReadFile or
validate.AtomicWrite with an unsafe path; update the code paths around the state
path construction (the filepath.Join(appdir.StateDir(), stateFile) site and the
call sites that invoke vfs.MkdirAll, vfs.ReadFile, and validate.AtomicWrite) to
perform this validation first.
---
Outside diff comments:
In `@internal/keychain/keychain_darwin.go`:
- Around line 280-295: platformSet currently may create a system keychain master
key (via getMasterKey(service, true) / getFileMasterKey(service, true)) before
validating the storage directory; move the StorageDir(service) &
vfs.MkdirAll(dir, 0o700) checks to occur before any call that can create keys so
failures fail fast. Concretely: in platformSet, after attempting
getFileMasterKey(service, false) and seeing it missing, first call
StorageDir(service) and ensure vfs.MkdirAll succeeds; only then call
getMasterKey(service, true) and the subsequent getFileMasterKey(service, true)
to create/write master.key. This prevents creating master.key in the macOS
keychain when storage setup fails.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d4294b8-2b95-44d2-b93b-c976539193e8
📒 Files selected for processing (28)
cmd/auth/login_scope_cache.gocmd/config/init_interactive.gointernal/appdir/appdir.gointernal/appdir/appdir_test.gointernal/auth/app_registration.gointernal/auth/device_flow.gointernal/auth/transport.gointernal/auth/uat_client.gointernal/auth/uat_client_options_test.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_http_test.gointernal/core/config.gointernal/keychain/auth_log.gointernal/keychain/auth_log_test.gointernal/keychain/keychain_darwin.gointernal/keychain/keychain_darwin_test.gointernal/keychain/keychain_other.gointernal/keychain/keychain_other_test.gointernal/lockfile/lockfile.gointernal/lockfile/lockfile_test.gointernal/registry/loader.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/update/update.gointernal/update/update_test.gointernal/validate/path.gointernal/vfs/localfileio/path.gointernal/vfs/localfileio/path_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/validate/path.go
- internal/auth/transport.go
- internal/core/config.go
- internal/keychain/keychain_darwin_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/auth/uat_client_options_test.go
- internal/lockfile/lockfile.go
- internal/cmdutil/factory_http_test.go
- internal/vfs/localfileio/path.go
- internal/auth/app_registration.go
- internal/registry/remote_test.go
- internal/registry/remote.go
- internal/vfs/localfileio/path_test.go
- internal/cmdutil/factory_default.go
- internal/auth/uat_client.go
- internal/registry/loader.go
- internal/appdir/appdir.go
- internal/appdir/appdir_test.go
JayYoung2021
force-pushed
the
refactor/adopt-xdg-for-config-and-data
branch
from
782032b to
88a7272
Compare
Extract directory resolution into a new internal/appdir package that follows the XDG base directory specification. Config, cache, state, log, and per-service data directories now respect LARKSUITE_CLI_*_DIR environment variables and XDG_*_HOME fallbacks. Lock files are moved from the config directory to the state directory.
JayYoung2021
force-pushed
the
refactor/adopt-xdg-for-config-and-data
branch
from
88a7272 to
3153520
Compare
Author
|
@liangshuo-1 @kongenpei @infeng All review threads on this PR are resolved and the related tests are passing. Could you please take a look when you have a moment? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change centralizes application directory resolution behind a new internal/appdir package that
follows XDG base directory conventions for config, cache, state, data, and log storage. It replaces
several package-specific path implementations with shared helpers so directory handling is more
consistent, easier to maintain, and better aligned across Linux and macOS.
Changes
• Add internal/appdir with XDG-compliant directory helpers, support for environment variable
overrides, and unit coverage for directory resolution behavior.
• Refactor keychain, lockfile, registry, update, auth, and core config code paths to use
centralized appdir helpers instead of ad hoc config/data path logic.
• Update related auth and keychain handling to consume the new directory APIs while preserving
existing behavior for persisted state and cached artifacts.
Test Plan
Related Issues
• None
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests