Skip to content

Conversation

@TabishB
Copy link
Contributor

@TabishB TabishB commented Dec 20, 2025

Prioritize XDG_CONFIG_HOME on Windows to fix test environment overrides. Previously, Windows would always use APPDATA regardless of XDG_CONFIG_HOME, causing tests to fail. Now XDG_CONFIG_HOME is checked first on all platforms before falling back to platform-specific defaults. The Windows APPDATA test is updated to explicitly clear XDG_CONFIG_HOME when testing the fallback behavior.

Fixes failing Windows tests in test/core/global-config.test.ts.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed configuration directory resolution to consistently prioritize XDG_CONFIG_HOME environment variable across all platforms with appropriate platform-specific fallbacks.
  • Documentation

    • Updated documentation to clarify cross-platform configuration directory precedence and fallback behavior.
  • Tests

    • Updated tests to validate configuration directory behavior when environment variables are unset.

✏️ Tip: You can customize this high-level summary in your review settings.

Prioritize XDG_CONFIG_HOME on Windows to fix test environment overrides.
Previously, Windows would always use APPDATA regardless of XDG_CONFIG_HOME,
causing tests to fail. Now XDG_CONFIG_HOME is checked first on all platforms
before falling back to platform-specific defaults.

Also update the Windows APPDATA test to explicitly clear XDG_CONFIG_HOME
when testing the fallback behavior.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 20, 2025

Walkthrough

The pull request consolidates XDG_CONFIG_HOME environment variable handling into a single cross-platform check in getGlobalConfigDir, replacing platform-specific duplicate checks. When unset, it falls back to platform-specific defaults. Test updates align with the new precedence behavior.

Changes

Cohort / File(s) Summary
Configuration directory resolution
src/core/global-config.ts
Modified getGlobalConfigDir to prioritize XDG_CONFIG_HOME on all platforms with early return when set; simplified fallback logic for Windows (APPDATA + fallback) and Unix/macOS (~/.config/openspec); removed duplicate XDG_CONFIG_HOME check for Unix/macOS.
Config directory tests
test/core/global-config.test.ts
Updated Windows APPDATA test to reflect new behavior (only applies when XDG_CONFIG_HOME is unset); deleted XDG_CONFIG_HOME from test setup; renamed test description for clarity.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–15 minutes

  • Verify cross-platform precedence logic (XDG_CONFIG_HOME → platform fallback chain)
  • Confirm Windows APPDATA fallback chain remains correct
  • Check test coverage for edge cases (XDG_CONFIG_HOME set vs. unset on all platforms)

Possibly related PRs

Poem

🐰 A config path, once split three ways,
Now kneels to XDG's graceful gaze,
One home to rule them all with care,
Cross-platform harmony in the air! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: making XDG_CONFIG_HOME take priority across all platforms rather than only on Unix/macOS.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch TabishB/fix-win-xdg-test

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5129a8c and 6f3e351.

📒 Files selected for processing (2)
  • src/core/global-config.ts (2 hunks)
  • test/core/global-config.test.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/global-config.ts (1)
src/core/index.ts (2)
  • getGlobalConfigDir (6-6)
  • GLOBAL_CONFIG_DIR_NAME (3-3)
🔇 Additional comments (5)
src/core/global-config.ts (3)

21-24: LGTM! Clear documentation of cross-platform XDG precedence.

The updated documentation accurately describes the new behavior where XDG_CONFIG_HOME is prioritized on all platforms, improving alignment with the XDG Base Directory Specification.


26-30: Excellent consolidation of XDG_CONFIG_HOME handling.

The early return when XDG_CONFIG_HOME is set properly unifies cross-platform behavior and eliminates the previous Windows exception. The condition correctly treats empty strings as unset, falling back to platform defaults.


44-44: LGTM! Comment accurately reflects fallback behavior.

The updated comment correctly indicates this is a fallback used only when XDG_CONFIG_HOME is unset.

test/core/global-config.test.ts (2)

73-73: LGTM! Test description now accurately reflects the test scenario.

The updated description clarifies that this test verifies Windows APPDATA fallback behavior when XDG_CONFIG_HOME is explicitly not set, aligning with the production code changes.


77-77: Good addition to ensure clean test state.

Explicitly deleting XDG_CONFIG_HOME ensures the test verifies the Windows APPDATA fallback path rather than being affected by any XDG_CONFIG_HOME value that might be present in the test environment. This makes the test more deterministic and correctly validates the new cross-platform precedence logic.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@TabishB TabishB merged commit f39cc5c into main Dec 20, 2025
7 checks passed
appboypov added a commit to appboypov/OpenSplx that referenced this pull request Dec 21, 2025
* fix(global-config): respect XDG_CONFIG_HOME on all platforms (Fission-AI#378)

Prioritize XDG_CONFIG_HOME on Windows to fix test environment overrides.
Previously, Windows would always use APPDATA regardless of XDG_CONFIG_HOME,
causing tests to fail. Now XDG_CONFIG_HOME is checked first on all platforms
before falling back to platform-specific defaults.

Also update the Windows APPDATA test to explicitly clear XDG_CONFIG_HOME
when testing the fallback behavior.

* fix(cli): prevent hang in pre-commit hooks by using dynamic imports (Fission-AI#380)

Fixes Fission-AI#367

The CLI was hanging when run as a pre-commit hook because @inquirer/prompts
was statically imported at module load time. Even when prompts were never
called (e.g., `openspec validate --specs --no-interactive`), the import
itself could set up stdin references that prevented clean process exit
when stdin was piped.

Changes:
- Convert all static `@inquirer/prompts` imports to dynamic imports
- Dynamically import `InitCommand` (which uses `@inquirer/core`)
- Update `isInteractive()` to accept options object with both
  `noInteractive` and Commander's negated `interactive` property
- Handle empty validation queue with proper exit code

Now when running in non-interactive mode, the inquirer modules are never
loaded, allowing the process to exit cleanly after completion.

* feat(cli): add plx command alias and rebrand as OpenSplx (OpenSplx-#1)

- Add `plx` as an alias command alongside `openspec`
- Create OpenSplx pixel art logo assets (light/dark themes)
- Update README with fork notice and Quick Start section
- Make CLI command name dynamic based on invocation
- Update completion system to support both command names
- Add command-name utility for detecting invoked command

* feat(cli): add external issue tracking support

- Add YAML frontmatter parsing for tracked issues in proposal.md
- Display issue identifiers in `openspec list` output
- Include trackedIssues in `openspec show --json` output
- Report tracked issues when archiving changes
- Add External Issue Tracking section to AGENTS.md template
- Add TrackedIssue schema and type exports

* fix(cli): address PR review feedback and archive external issue tracking

- Fix list.ts alignment to include issue display in width calculation
- Fix command-name.ts to handle Windows extensions and cross-platform paths
- Fix postinstall.js to install completions for both openspec and plx
- Fix change.ts issue display format (after title in long format)
- Add comprehensive unit tests for all fixes
- Archive add-external-issue-tracking change with spec updates

---------

Co-authored-by: Tabish Bidiwale <30385142+TabishB@users.noreply.github.com>
appboypov added a commit to appboypov/OpenSplx that referenced this pull request Dec 21, 2025
* fix(global-config): respect XDG_CONFIG_HOME on all platforms (Fission-AI#378)

Prioritize XDG_CONFIG_HOME on Windows to fix test environment overrides.
Previously, Windows would always use APPDATA regardless of XDG_CONFIG_HOME,
causing tests to fail. Now XDG_CONFIG_HOME is checked first on all platforms
before falling back to platform-specific defaults.

Also update the Windows APPDATA test to explicitly clear XDG_CONFIG_HOME
when testing the fallback behavior.

* fix(cli): prevent hang in pre-commit hooks by using dynamic imports (Fission-AI#380)

Fixes Fission-AI#367

The CLI was hanging when run as a pre-commit hook because @inquirer/prompts
was statically imported at module load time. Even when prompts were never
called (e.g., `openspec validate --specs --no-interactive`), the import
itself could set up stdin references that prevented clean process exit
when stdin was piped.

Changes:
- Convert all static `@inquirer/prompts` imports to dynamic imports
- Dynamically import `InitCommand` (which uses `@inquirer/core`)
- Update `isInteractive()` to accept options object with both
  `noInteractive` and Commander's negated `interactive` property
- Handle empty validation queue with proper exit code

Now when running in non-interactive mode, the inquirer modules are never
loaded, allowing the process to exit cleanly after completion.

* feat(cli): add plx command alias and rebrand as OpenSplx (OpenSplx-#1)

- Add `plx` as an alias command alongside `openspec`
- Create OpenSplx pixel art logo assets (light/dark themes)
- Update README with fork notice and Quick Start section
- Make CLI command name dynamic based on invocation
- Update completion system to support both command names
- Add command-name utility for detecting invoked command

* feat(cli): add external issue tracking support

- Add YAML frontmatter parsing for tracked issues in proposal.md
- Display issue identifiers in `openspec list` output
- Include trackedIssues in `openspec show --json` output
- Report tracked issues when archiving changes
- Add External Issue Tracking section to AGENTS.md template
- Add TrackedIssue schema and type exports

* fix(cli): address PR review feedback and archive external issue tracking

- Fix list.ts alignment to include issue display in width calculation
- Fix command-name.ts to handle Windows extensions and cross-platform paths
- Fix postinstall.js to install completions for both openspec and plx
- Fix change.ts issue display format (after title in long format)
- Add comprehensive unit tests for all fixes
- Archive add-external-issue-tracking change with spec updates

---------

Co-authored-by: Tabish Bidiwale <30385142+TabishB@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants