feat(core): implement global config directory with XDG support #377
Conversation
Add new global-config module following XDG Base Directory Specification with platform-specific fallbacks (Unix: ~/.config/openspec, Windows: %APPDATA%/openspec). Includes config loading with defaults, config saving with directory creation, and full test coverage. Archive add-global-config-dir change.
WalkthroughAdds a new global configuration module that resolves a platform-aware config directory (XDG/APPDATA fallbacks), exposes functions to get/load/save a JSON config, re-exports the API from core index, provides defaults and merge behavior, and includes comprehensive unit tests and a formal spec. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Nitpick comments (3)
openspec/specs/global-config/spec.md (1)
5-8: Minor formatting: missing blank line before Requirements heading.There's a missing blank line between the Purpose section (line 4) and the Requirements section (line 5), which affects readability.
🔎 Suggested fix
## Purpose TBD - created by archiving change add-global-config-dir. Update Purpose after archive. + ## Requirementstest/core/global-config.test.ts (1)
62-83: Platform-conditional tests provide limited cross-platform coverage.These tests are necessarily conditional on the running platform. The Unix fallback test (lines 68-70) only asserts on non-Windows, and the Windows test (lines 76-81) only runs on Windows. This means CI runs on a single platform won't verify cross-platform behavior.
Consider adding a note in the test file or relying on multi-platform CI matrix for full coverage.
src/core/global-config.ts (1)
79-85: Consider logging non-JSON errors for debugging.Non-
SyntaxErrorexceptions (e.g., permission denied, disk I/O errors) are silently swallowed, returning defaults without any indication of the underlying issue. This could make debugging difficult when a user's config file exists but can't be read.🔎 Suggested enhancement
} catch (error) { // Log warning for parse errors, but not for missing files if (error instanceof SyntaxError) { console.error(`Warning: Invalid JSON in ${configPath}, using defaults`); + } else if (error instanceof Error && error.code !== 'ENOENT') { + console.error(`Warning: Could not read ${configPath}: ${error.message}, using defaults`); } return { ...DEFAULT_CONFIG }; }Note: You'd need to handle the TypeScript type for
error.code(e.g., cast or type guard forNodeJS.ErrnoException).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openspec/changes/archive/2025-12-20-add-global-config-dir/tasks.md(1 hunks)openspec/specs/global-config/spec.md(1 hunks)src/core/global-config.ts(1 hunks)src/core/index.ts(1 hunks)test/core/global-config.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
openspec/specs/**/spec.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Use SHALL/MUST for normative requirements in spec files; avoid should/may unless intentionally non-normative
Files:
openspec/specs/global-config/spec.md
openspec/changes/**/*.md
📄 CodeRabbit inference engine (openspec/AGENTS.md)
Scaffold proposal using
proposal.md,tasks.md, optionaldesign.md, and delta specs underopenspec/changes/<id>/
Files:
openspec/changes/archive/2025-12-20-add-global-config-dir/tasks.md
🧬 Code graph analysis (2)
test/core/global-config.test.ts (2)
src/core/global-config.ts (6)
GLOBAL_CONFIG_DIR_NAME(6-6)GLOBAL_CONFIG_FILE_NAME(7-7)getGlobalConfigDir(24-44)getGlobalConfigPath(49-51)getGlobalConfig(58-86)saveGlobalConfig(92-102)src/core/index.ts (6)
GLOBAL_CONFIG_DIR_NAME(3-3)GLOBAL_CONFIG_FILE_NAME(4-4)getGlobalConfigDir(6-6)getGlobalConfigPath(7-7)getGlobalConfig(8-8)saveGlobalConfig(9-9)
src/core/global-config.ts (1)
src/core/index.ts (7)
GLOBAL_CONFIG_DIR_NAME(3-3)GLOBAL_CONFIG_FILE_NAME(4-4)GlobalConfig(5-5)getGlobalConfigDir(6-6)getGlobalConfigPath(7-7)getGlobalConfig(8-8)saveGlobalConfig(9-9)
🔇 Additional comments (8)
src/core/index.ts (1)
2-10: LGTM!Clean barrel export pattern correctly re-exporting all public API surface from the global-config module. The use of
typekeyword forGlobalConfigensures proper type-only imports when needed.openspec/changes/archive/2025-12-20-add-global-config-dir/tasks.md (1)
1-26: LGTM!Task tracking document properly reflects the completed implementation work. All items are marked complete and align with the files in this PR.
test/core/global-config.test.ts (3)
20-41: LGTM!Well-structured test setup with proper isolation: unique temp directories per test run, environment variable preservation, and console.error spying for warning verification.
96-191: LGTM!Comprehensive test coverage for
getGlobalConfig(): defaults when missing, no side effects on read, valid config loading, invalid JSON handling with warning verification, unknown field preservation for forward compatibility, and proper merging behavior.
193-254: LGTM!Thorough test coverage for
saveGlobalConfig(): directory creation, file writing, overwrite behavior, formatted JSON output with trailing newline, and round-trip correctness verification.src/core/global-config.ts (3)
1-16: LGTM!Clean module setup with node:-prefixed imports, well-named constants, and a minimal extensible interface. The default configuration provides a sensible baseline.
24-44: LGTM!Correct XDG Base Directory implementation with appropriate platform-specific fallbacks. Windows uses
%APPDATA%with a sensible fallback, and Unix/macOS respects$XDG_CONFIG_HOMEbefore falling back to~/.config.
92-102: LGTM!Clean implementation that creates the directory if needed and writes formatted JSON with a trailing newline. Errors propagate to callers, which is appropriate for a save operation.
One consideration for the future: non-atomic writes could leave a corrupted file if the process crashes mid-write. For a config file that's typically small and infrequently written, this is acceptable, but you might consider write-to-temp-then-rename for robustness in later iterations.
Replace placeholder text with a concise description of what the spec governs, its scope, and high-level objectives.
Summary
Implements XDG Base Directory Specification support for global configuration storage. Provides path resolution with platform-specific fallbacks, lazy config loading with defaults, and config saving functionality.
Changes
src/core/global-config.tsmodule with XDG-compliant path resolutiongetGlobalConfigDir()for resolving config directory (respects $XDG_CONFIG_HOME)getGlobalConfig()andsaveGlobalConfig()for reading/writing configopenspec/specs/global-config/spec.mdwith requirementsTest Plan
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.