fix: clarify invalid config JSON errors

[nightcityblade]

Fixes #102

Summary

• Catch malformed JSON in loadConfig and wrap it with a clearer CLI-facing message
• Include the config file path and a hint to fix syntax or rerun init
• Add a regression test for malformed config JSON

Tests

• npm test
• node --test tests/*.test.mjs
• npm run build
• npx prettier --check src/config/store.ts
• git diff --check

Note: npm run format:check currently reports pre-existing formatting warnings in unrelated files (src/commands/init.ts, src/commands/suggest.ts, src/git/hook.ts, src/llm/client.ts); changed TypeScript file passes Prettier.

[404-Page-Found]

PR #118 Review: fix: clarify invalid config JSON errors

Overview

Author	@nightcityblade
Base → Head	main ← fix/issue-102
Size	+59/−1 across 2 files (1 commit)
Linked issue	#102 — "Improve invalid config JSON error message with file path"

Scope & purpose: Wraps JSON.parse in loadConfig() with a try/catch that rethrows SyntaxError as a user-friendly error including the config file path and a fix hint (commit-echo init). Adds one regression test. Clean, focused, single-concern PR.

---

✅ What's good

1. Meets every acceptance criterion from #102:

   • Catches JSON.parse failures and throws a user-friendly message
   • Includes the config file path in the error message
   • Non-JSON errors (e.g., permission errors) are rethrown unchanged
   • Regression test validates the full error message content

2. Preserves the original error via { cause: error } — the SyntaxError is still accessible in the error chain for debugging.

3. Correct error discrimination — instanceof SyntaxError guard ensures only JSON parse errors get wrapped; anything else rethrows cleanly.

4. Test is thorough — validates three distinct parts of the error message (prefix, config path, hint), uses escapeRegExp for the dynamic path, and cleans up environment variables in a finally block.

5. Prettier clean, builds successfully, all 83 tests pass (1 skipped — same as main minus streaming tests added after the branch point). No regressions.

6. Commit message follows conventional commits (fix: clarify invalid config JSON errors).

---

⚠️ Issues

Important — Test not hermetic on Windows

The test manipulates HOME and XDG_CONFIG_HOME to redirect getConfigDir() to a temp directory. This works on Linux/macOS, but on Windows the getConfigDir() function takes the win32 branch first and uses APPDATA, which the test never overrides:

// src/config/store.ts — getConfigDir()
if (os === 'win32') {
  const appData = process.env['APPDATA'];
  if (appData) return join(appData, 'commit-echo'); // ← always taken on Windows
}

Impact: On Windows, the test writes "{ invalid json" to the real user's %APPDATA%/commit-echo/config.json instead of a temp directory. The finally block restores env vars but never restores the original file contents, so running npm test on Windows would silently corrupt the developer's config.

Suggestion: Override APPDATA in the test (or mock getConfigPath directly). Something like:

const originalAppData = process.env.APPDATA;
process.env.APPDATA = home;
// ... run test ...
process.env.APPDATA = originalAppData;

Or, to avoid env var manipulation entirely, mock getConfigPath / getConfigDir to return the temp path directly. This would make the test platform-agnostic.

Note: CI runs on Linux, so this won't break CI — but it will break local npm test for any Windows contributor.

---

Nitpick — Minor observations

1. Double-quoted strings in the test — The test uses double quotes ("node:assert/strict") while the project convention (visible in .prettierrc with singleQuote: true and in all other test files) is single quotes. This doesn't fail Prettier because Prettier is not configured for the .mjs test files, but it's a minor style inconsistency.

---

Verdict: Approve ✅

The PR is well-scoped, correct for its target platform (Linux/macOS CI), and cleanly implemented. The Windows test hermeticity issue is real but does not block merge — it's a pre-existing platform gap that affects any test relying on HOME/XDG_CONFIG_HOME on Windows. A follow-up issue to harden tests for Windows would be the appropriate place to address it.

The core change in src/config/store.ts is solid and ready to ship.

[404-Page-Found]

PR #118 Review: fix: clarify invalid config JSON errors

Overview

Author	@nightcityblade
Base → Head	main ← fix/issue-102
Size	+59/−1 across 2 files (1 commit)
Linked issue	#102 — "Improve invalid config JSON error message with file path"

Scope & purpose: Wraps JSON.parse in loadConfig() with a try/catch that rethrows SyntaxError as a user-friendly error including the config file path and a fix hint (commit-echo init). Adds one regression test. Clean, focused, single-concern PR.

---

✅ What's good

1. Meets every acceptance criterion from #102:

   • Catches JSON.parse failures and throws a user-friendly message
   • Includes the config file path in the error message
   • Non-JSON errors (e.g., permission errors) are rethrown unchanged
   • Regression test validates the full error message content

2. Preserves the original error via { cause: error } — the SyntaxError is still accessible in the error chain for debugging.

3. Correct error discrimination — instanceof SyntaxError guard ensures only JSON parse errors get wrapped; anything else rethrows cleanly.

4. Test is thorough — validates three distinct parts of the error message (prefix, config path, hint), uses escapeRegExp for the dynamic path, and cleans up environment variables in a finally block.

5. Prettier clean, builds successfully, all 83 tests pass (1 skipped — same as main minus streaming tests added after the branch point). No regressions.

6. Commit message follows conventional commits (fix: clarify invalid config JSON errors).

---

🚫 Blocking — Test corrupts real config on Windows

The test manipulates HOME and XDG_CONFIG_HOME to redirect getConfigDir() to a temp directory. This works on Linux/macOS, but on Windows the getConfigDir() function takes the win32 branch first and uses APPDATA, which the test never overrides:

// src/config/store.ts — getConfigDir()
if (os === 'win32') {
  const appData = process.env['APPDATA'];
  if (appData) return join(appData, 'commit-echo'); // ← always taken on Windows
}

Impact: On Windows, the test writes "{ invalid json" to the real user's %APPDATA%/commit-echo/config.json instead of a temp directory. The finally block restores env vars but never restores the original file contents, so running npm test on Windows would silently corrupt the developer's config.

Fix: Add APPDATA override to the env-var backup/restore block so the test is hermetic on all platforms:

const originalHome = process.env.HOME;
const originalXdgConfigHome = process.env.XDG_CONFIG_HOME;
const originalAppData = process.env.APPDATA;
const home = await mkdtemp(`${tmpdir()}/commit-echo-config-`);

process.env.HOME = home;
process.env.APPDATA = home;       // ← needed on Windows
delete process.env.XDG_CONFIG_HOME;
// ... test body ...
} finally {
  if (originalHome === undefined) {
    delete process.env.HOME;
  } else {
    process.env.HOME = originalHome;
  }
  if (originalAppData === undefined) {
    delete process.env.APPDATA;
  } else {
    process.env.APPDATA = originalAppData;
  }
  if (originalXdgConfigHome === undefined) {
    delete process.env.XDG_CONFIG_HOME;
  } else {
    process.env.XDG_CONFIG_HOME = originalXdgConfigHome;
  }
}

CI runs on Linux so this won't break the pipeline, but it will silently destroy the local config of any Windows contributor who runs npm test.

---

Verdict: Request changes 🚫

The core loadConfig() change is solid and ready to ship. The test must be updated to also override APPDATA on Windows (or mock getConfigPath directly) to prevent corrupting the developer's real config file. Once that's addressed, this is good to merge.