refactor: use XDG base dirs for config and data#410
refactor: use XDG base dirs for config and data#410JayYoung2021 wants to merge 1 commit intolarksuite:mainfrom
Conversation
Introduce internal/appdir package for XDG-compliant directory resolution (CONFIG, CACHE, STATE, DATA, LOG) with environment variable overrides. Migrate keychain, lockfile, registry, update, auth, and config packages to use the centralized appdir helpers instead of scattered implementations, improving consistency and Linux/macOS compatibility.
|
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. |
📝 WalkthroughWalkthroughThis PR introduces a centralized filesystem path resolution package ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 introduces a new Confidence Score: 4/5Safe to merge after fixing the unvalidated LARKSUITE_CLI_CONFIG_DIR env var; all other callers are clean refactors. One P1 defect: ConfigDir, CacheDir, and StateDir accept LARKSUITE_CLI_CONFIG_DIR without running it through validatedEnvDir, so a relative path in that variable creates inconsistent, CWD-dependent directory resolution — inconsistent with how the other two override env vars behave. All remaining callsite migrations are straightforward and correct. internal/appdir/appdir.go — the three raw os.Getenv calls for LARKSUITE_CLI_CONFIG_DIR should go through validatedEnvDir. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Directory Request] --> B{LARKSUITE_CLI_CONFIG_DIR set?}
B -- yes --> C["ConfigDir / StateDir: return raw value\nCacheDir: return raw + /cache"]
B -- no --> D{XDG_*_HOME set and valid?}
D -- yes --> E["return XDG_HOME/lark-cli"]
D -- no --> F{"Legacy ~/.lark-cli exists?"}
F -- yes --> G["return ~/.lark-cli"]
F -- no --> H["return OS default\nLinux: ~/.config/lark-cli\nmacOS: same\nWindows: ~/.lark-cli"]
subgraph LogDir
L1{LARKSUITE_CLI_LOG_DIR valid?} -- yes --> L2["return validated path"]
L1 -- no --> L3["return StateDir + /logs"]
end
subgraph DataDir
D1{LARKSUITE_CLI_DATA_DIR valid?} -- yes --> D2["return validated + /service"]
D1 -- no --> D3{XDG_DATA_HOME valid?}
D3 -- yes --> D4["return XDG_DATA_HOME/service"]
D3 -- no --> D5{"Legacy ~/Library/AppSupport/svc (darwin)?"}
D5 -- yes --> D6["return legacy path"]
D5 -- no --> D7["return ~/.local/share/service"]
end
style C fill:#ffcccc,stroke:#cc0000
style E fill:#ccffcc,stroke:#009900
Reviews (1): Last reviewed commit: "refactor: use XDG base dirs for config a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func ConfigDir() string { | ||
| if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" { | ||
| return dir | ||
| } | ||
| if dir, ok := xdgAppDir("XDG_CONFIG_HOME"); ok { | ||
| return dir | ||
| } | ||
| if dir, ok := legacyConfigDir(); ok { | ||
| return dir | ||
| } | ||
| return defaultConfigDir() | ||
| } | ||
|
|
||
| // ConfigPath returns the CLI config file path. | ||
| func ConfigPath() string { | ||
| return filepath.Join(ConfigDir(), "config.json") | ||
| } | ||
|
|
||
| // CacheDir returns the CLI cache directory. | ||
| func CacheDir() string { | ||
| if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" { | ||
| return filepath.Join(dir, "cache") | ||
| } | ||
| if dir, ok := xdgAppDir("XDG_CACHE_HOME"); ok { | ||
| return dir | ||
| } | ||
| if dir, ok := legacyConfigDir(); ok { | ||
| return filepath.Join(dir, "cache") | ||
| } | ||
| return defaultCacheDir() | ||
| } | ||
|
|
||
| // StateDir returns the CLI state directory. | ||
| func StateDir() string { | ||
| if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" { | ||
| return dir | ||
| } | ||
| if dir, ok := xdgAppDir("XDG_STATE_HOME"); ok { | ||
| return dir | ||
| } | ||
| if dir, ok := legacyConfigDir(); ok { | ||
| return dir | ||
| } | ||
| return defaultStateDir() | ||
| } | ||
|
|
There was a problem hiding this comment.
LARKSUITE_CLI_CONFIG_DIR accepted without validation
ConfigDir(), CacheDir(), and StateDir() read LARKSUITE_CLI_CONFIG_DIR via raw os.Getenv(), but LogDir() and DataDir() both go through validatedEnvDir() which calls SafeEnvDirPath to reject control characters and require an absolute path. If LARKSUITE_CLI_CONFIG_DIR is set to a relative path (e.g. myconfig), all three functions silently return that relative string, so ConfigPath(), lock directories, and log paths resolve against whatever the process's working directory happens to be at call time — a different location than the user likely intended, and different from the consistent absolute-path guarantee the validated env vars provide.
func ConfigDir() string {
- if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" {
- return dir
+ if dir, ok := validatedEnvDir("LARKSUITE_CLI_CONFIG_DIR"); ok {
+ return dir
}
...
}
func CacheDir() string {
- if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" {
- return filepath.Join(dir, "cache")
+ if dir, ok := validatedEnvDir("LARKSUITE_CLI_CONFIG_DIR"); ok {
+ return filepath.Join(dir, "cache")
}
...
}
func StateDir() string {
- if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" {
- return dir
+ if dir, ok := validatedEnvDir("LARKSUITE_CLI_CONFIG_DIR"); ok {
+ return dir
}
...
}| func validatedEnvDir(envName string) (string, bool) { | ||
| value := os.Getenv(envName) | ||
| if value == "" { | ||
| return "", false | ||
| } | ||
| dir, err := validate.SafeEnvDirPath(value, envName) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| return dir, true | ||
| } |
There was a problem hiding this comment.
When SafeEnvDirPath rejects an env var value (e.g. a relative path or a path with control characters), validatedEnvDir silently returns ("", false) and the caller falls back to defaults with no warning. A user who mistakenly sets LARKSUITE_CLI_LOG_DIR=./logs will see the env var ignored without any hint as to why. A single fmt.Fprintf(log.Writer(), "warning: %s=%q is invalid (%v), using default\n", envName, value, err) before the return "", false would make misconfiguration much easier to diagnose.
![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
| if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" { | ||
| return dir | ||
| } |
There was a problem hiding this comment.
Validate LARKSUITE_CLI_CONFIG_DIR before reusing it for config/cache/state paths.
Lines 21-23, 40-42, and 54-56 use raw env input, unlike LogDir/DataDir which use validatedEnvDir. This bypasses the module’s own safe-path checks for directories that are later used in file I/O.
Suggested fix
func ConfigDir() string {
- if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" {
+ if dir, ok := validatedEnvDir("LARKSUITE_CLI_CONFIG_DIR"); ok {
return dir
}
@@
func CacheDir() string {
- if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" {
+ if dir, ok := validatedEnvDir("LARKSUITE_CLI_CONFIG_DIR"); ok {
return filepath.Join(dir, "cache")
}
@@
func StateDir() string {
- if dir := os.Getenv("LARKSUITE_CLI_CONFIG_DIR"); dir != "" {
+ if dir, ok := validatedEnvDir("LARKSUITE_CLI_CONFIG_DIR"); ok {
return dir
}As per coding guidelines, "Validate paths using validate.SafeInputPath before any file I/O operations".
Also applies to: 40-42, 54-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/appdir/appdir.go` around lines 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.
| // doSyncFetch performs a blocking fetch for first-run without embedded data. | ||
| func doSyncFetch() { | ||
| fmt.Fprintf(os.Stderr, "Fetching API metadata...\n") | ||
| fmt.Fprintln(log.Writer(), "Fetching API metadata...") |
There was a problem hiding this comment.
Keep first-run progress output pinned to stderr
Using log.Writer() here can leak progress text to stdout if log output is redirected, which can corrupt machine-readable CLI output. Write directly to stderr (or the CLI error stream) instead.
Suggested fix
- fmt.Fprintln(log.Writer(), "Fetching API metadata...")
+ fmt.Fprintln(os.Stderr, "Fetching API metadata...")As per coding guidelines, "Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fmt.Fprintln(log.Writer(), "Fetching API metadata...") | |
| fmt.Fprintln(os.Stderr, "Fetching API metadata...") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/registry/remote.go` at 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.
2 participants
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
Bug Fixes