test: config — ConfigPaths.parseText JSONC parsing and substitution

[anandgupta42]

What does this PR do?\n\n1. ConfigPaths.parseText — src/config/paths.ts (12 new tests)\n\nThis function is the core JSONC config parser used by both Config.get() and TUI config loading. It handles {env:VAR} and {file:path} token substitution, JSONC comment handling, and error reporting with line/column positions. Previously, it was only tested indirectly through integration tests in config.test.ts. Zero direct unit tests existed.\n\nNew coverage includes:\n\nValid JSONC parsing (2 tests)\n- Trailing comma support (verifies allowTrailingComma: true flag)\n- Line comment preservation during parsing\n\nJSONC error reporting (2 tests)\n- Missing comma throws ConfigJsonError with source path, line number, and column number\n- Error message includes the offending line content and a caret (^) marker pointing to the error position\n\n**{env:VAR} substitution (2 tests)\n- Present environment variable is substituted correctly\n- Missing environment variable resolves to empty string (not undefined/error)\n\n{file:} comment skipping (1 test)\n- {file:nonexistent.txt} inside a JSONC // comment is correctly skipped and does NOT trigger file I/O. This tests the prefix.startsWith(\"//\") guard in the substitute function — if broken, users with commented-out file references would get cryptic ENOENT errors.\n\n{file:} missing file error handling (2 tests)\n- Missing file throws ConfigInvalidError with "does not exist" message and source config path\n- missing='empty' mode gracefully returns empty string instead of throwing\n\n{file:} real file substitution (3 tests)**\n- Absolute path file content is substituted correctly\n- File content whitespace is trimmed (verifies the .trim() call)\n- Relative {file:} paths are resolved against the config file's directory (not CWD)\n\n### Type of change\n- [x] New feature (non-breaking change which adds functionality)\n\n### Issue for this PR\nN/A — proactive test coverage from /test-discovery\n\n### How did you verify your code works?\n\n\nbun test test/config/paths.test.ts # 12 pass, 0 fail\n\n\n### Checklist\n- [x] I have added tests that prove my fix is effective or that my feature works\n- [x] New and existing unit tests pass locally with my changes\n\nhttps://claude.ai/code/session_0153Zp8Yg62ibyUuu3vcxz3k

Summary by CodeRabbit

• Tests
  • Added comprehensive test suite for configuration file parsing functionality, covering JSONC (JSON with Comments) support including trailing commas and comments, error handling with detailed diagnostic information and position tracking, environment variable substitution with fallback behavior, and file inclusion features. Tests ensure robust configuration file processing and error reporting.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review.

_{Tip: disable this comment in your organization's Code Review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Added a new test file with comprehensive test coverage for ConfigPaths.parseText functionality, including JSONC parsing validation, error handling with metadata verification, environment variable substitution via {env:VAR} tokens, and file content reading with {file:...} token handling and relative path resolution.

Changes

Cohort / File(s)	Summary
Test Suite Addition packages/opencode/test/config/paths.test.ts	New test file (144 lines) covering ConfigPaths.parseText behavior with tests for JSONC parsing, error handling and metadata validation, environment variable substitution, file token substitution with missing file error handling, and file content reading with path resolution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

contributor

Poem

🐰 Hop, hop, hooray for tests so true,
ConfigPaths parsed through and through!
With env vars, files, and JSON's gleam,
This rabbit validates the testing dream! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding unit tests for ConfigPaths.parseText JSONC parsing and substitution functionality.
Description check	✅ Passed	The description is comprehensive and covers the required template sections. It includes a detailed summary of what changed and why, a clear test plan with verification results, and a completed checklist.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/test-config-parsetext-session_0153Zp8Yg62ibyUuu3vcxz3k

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/opencode/test/config/paths.test.ts (1)

90-108: Avoid hardcoded /tmp in missing-file tests.

Using a fixed absolute path is less portable and can become flaky if the file unexpectedly exists. Prefer deriving a guaranteed-missing path from tmpdir().

♻️ Suggested refactor

 test("throws InvalidError when referenced file does not exist", async () => {
-  const text = '{ "key": "{file:/tmp/definitely-nonexistent-parse-text-file-abc123.txt}" }'
+  await using tmp = await tmpdir()
+  const missingPath = path.join(tmp.path, "definitely-missing-file.txt")
+  const text = `{ "key": "{file:${missingPath}}" }`
   await expect(ConfigPaths.parseText(text, "/fake/config.json")).rejects.toMatchObject({
     name: "ConfigInvalidError",
     data: {
       path: "/fake/config.json",
     },
   })
   // Verify specific error details
   try {
     await ConfigPaths.parseText(text, "/fake/config.json")
   } catch (e: any) {
     expect(e.data.message).toContain("does not exist")
   }
 })

 test("returns empty string for missing file when missing='empty'", async () => {
-  const text = '{ "key": "{file:/tmp/definitely-nonexistent-parse-text-file-abc123.txt}" }'
+  await using tmp = await tmpdir()
+  const missingPath = path.join(tmp.path, "definitely-missing-file.txt")
+  const text = `{ "key": "{file:${missingPath}}" }`
   const result = await ConfigPaths.parseText(text, "/fake/config.json", "empty")
   expect(result.key).toBe("")
 })

As per coding guidelines, "Use the tmpdir function from fixture/fixture.ts to create temporary directories for tests with automatic cleanup in test files".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/config/paths.test.ts` around lines 90 - 108, The tests
use a hardcoded absolute /tmp path which is non-portable and can be flaky;
update both tests that call ConfigPaths.parseText to derive a guaranteed-missing
file path using the tmpdir() helper from fixture/fixture.ts (create a temp
directory via tmpdir(), then build a filename inside it that you never create)
and use that generated path instead of
"/tmp/definitely-nonexistent-parse-text-file-abc123.txt" so the assertions
around ConfigPaths.parseText (and the error message check) target a
reliably-missing file.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/opencode/test/config/paths.test.ts`:
- Around line 70-77: The test "replaces missing env var with empty string"
mutates process.env["DEFINITELY_MISSING_PARSE_TEXT_VAR_98765"] without restoring
it; update the test around ConfigPaths.parseText to save the original value
(e.g., const _orig = process.env["DEFINITELY_MISSING_PARSE_TEXT_VAR_98765"]),
then remove or unset it for the assertion, and finally restore the original
value in a finally block or after the assertion (or use afterEach) so the global
env is returned to its prior state regardless of test outcome.

---

Nitpick comments:
In `@packages/opencode/test/config/paths.test.ts`:
- Around line 90-108: The tests use a hardcoded absolute /tmp path which is
non-portable and can be flaky; update both tests that call ConfigPaths.parseText
to derive a guaranteed-missing file path using the tmpdir() helper from
fixture/fixture.ts (create a temp directory via tmpdir(), then build a filename
inside it that you never create) and use that generated path instead of
"/tmp/definitely-nonexistent-parse-text-file-abc123.txt" so the assertions
around ConfigPaths.parseText (and the error message check) target a
reliably-missing file.

🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e2723ebf-3827-49cd-80f7-5f6520b7146f

📥 Commits

Reviewing files that changed from the base of the PR and between 8eb13d4 and add2c78.

📒 Files selected for processing (1)

• packages/opencode/test/config/paths.test.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Restore env state after this test.

Line 71 mutates global process.env without restoring a prior value. If that variable is set in CI, this test can leak state into later tests.

🔧 Suggested fix

 test("replaces missing env var with empty string", async () => {
-  delete process.env["DEFINITELY_MISSING_PARSE_TEXT_VAR_98765"]
-  const result = await ConfigPaths.parseText(
-    '{ "key": "{env:DEFINITELY_MISSING_PARSE_TEXT_VAR_98765}" }',
-    "/fake/config.json",
-  )
-  expect(result.key).toBe("")
+  const key = "DEFINITELY_MISSING_PARSE_TEXT_VAR_98765"
+  const prev = process.env[key]
+  delete process.env[key]
+  try {
+    const result = await ConfigPaths.parseText('{ "key": "{env:DEFINITELY_MISSING_PARSE_TEXT_VAR_98765}" }', "/fake/config.json")
+    expect(result.key).toBe("")
+  } finally {
+    if (prev === undefined) delete process.env[key]
+    else process.env[key] = prev
+  }
 })

📝 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.

Suggested change

	test("replaces missing env var with empty string", async () => {
	delete process.env["DEFINITELY_MISSING_PARSE_TEXT_VAR_98765"]
	const result = await ConfigPaths.parseText(
	'{ "key": "{env:DEFINITELY_MISSING_PARSE_TEXT_VAR_98765}" }',
	"/fake/config.json",
	)
	expect(result.key).toBe("")
	})
	test("replaces missing env var with empty string", async () => {
	const key = "DEFINITELY_MISSING_PARSE_TEXT_VAR_98765"
	const prev = process.env[key]
	delete process.env[key]
	try {
	const result = await ConfigPaths.parseText('{ "key": "{env:DEFINITELY_MISSING_PARSE_TEXT_VAR_98765}" }', "/fake/config.json")
	expect(result.key).toBe("")
	} finally {
	if (prev === undefined) delete process.env[key]
	else process.env[key] = prev
	}
	})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/config/paths.test.ts` around lines 70 - 77, The test
"replaces missing env var with empty string" mutates
process.env["DEFINITELY_MISSING_PARSE_TEXT_VAR_98765"] without restoring it;
update the test around ConfigPaths.parseText to save the original value (e.g.,
const _orig = process.env["DEFINITELY_MISSING_PARSE_TEXT_VAR_98765"]), then
remove or unset it for the assertion, and finally restore the original value in
a finally block or after the assertion (or use afterEach) so the global env is
returned to its prior state regardless of test outcome.

[anandgupta42]

Consolidated into #403