fix: prefer opencode.jsonc when loading OpenCode config#118
fix: prefer opencode.jsonc when loading OpenCode config#118kargnas merged 4 commits intoopgginc:mainfrom
Conversation
✅ AI Code Review Completed
Reviewed with |
![op-gg-ai-devops[bot]](https://avatars.githubusercontent.com/in/2680015?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Clean, focused PR — adds .jsonc preference ahead of .json when resolving OpenCode config paths, matching the copilothydra behavior. The zip + flatMap interleave is elegant and produces the correct [dir/jsonc, dir/json, dir/jsonc, dir/json, ...] ordering. Tests are thorough and cover ordering invariants, extension counts, directory coverage, and end-to-end JSONC parsing.
What's good
- The
zip(jsoncPaths, jsonPaths).flatMap { [$0, $1] }pattern is clever — it keeps the per-directory jsonc-before-json priority without duplicating the fallback directory list. Nice. - Test coverage is solid.
testOpenCodeConfigFilePathsReturnsJSONCBeforeJSONForEachLocationis exactly the right structural test for this kind of interleaving logic. - The
stripJSONCommentsvisibility change tointernalfor test access is the correct approach —@testable import+internalis idiomatic Swift testing. - Doc comments updated accurately with the new 8-path priority order.
One defensive note (not blocking)
The zip approach silently drops elements if the two arrays ever differ in length. Right now they can't — both calls to buildOpenCodeFilePaths use the same directory structure with only the filename differing, so dedup always removes/keeps the same indices. But if someone later adds a fallback directory to only one call, zip would silently swallow it. A debug assert could catch that:
assert(jsoncPaths.count == jsonPaths.count, "jsonc/json path arrays must be equal length for correct interleaving")Not a blocker at all — just a defensive thought for future-proofing.
CI Status
- Lint: ✅ passed
- Build / Test / CI: still running at time of review
Smoke tests (build, typecheck) could not be run locally — this is a macOS Swift project and the review runner is Linux.
🤖 This bot reviews every push to this branch automatically. You can also trigger a review with specific focus by mentioning @op-gg-ai-devops in a PR comment. Examples:
@op-gg-ai-devops review typos@op-gg-ai-devops focus on edge cases in the zip logic@op-gg-ai-devops check test coverage
✅ AI Code Review CompletedReview finished. Check the PR for inline comments. 📋 View Logs | 🤖 Model: |
✅ AI Code Review CompletedReview posted successfully — APPROVED with summary and notes.
Reviewed with |
There was a problem hiding this comment.
Review Summary
New commit since last review: 16a4080 adds openaiAPIKey fallback support for when openai in auth.json is stored as an API key object instead of OAuth. This is a clean extension of the existing lossy decoding pattern — the decoder tries OAuth first, falls back to APIKey, and everything flows through correctly.
What's good
- The dual-decode approach is elegant: try
.openaiasOAuth, and if that fails, try it asAPIKey.KeyedDecodingContainerallows re-reading the same key, so this works perfectly. - The encoder guard (
if openai == nil { try container.encodeIfPresent(openaiAPIKey, forKey: .openai) }) prevents duplicate JSON keys during round-trip encoding. Solid. getOpenAIAccounts()andgetOpenAIAccessToken()both gain API key fallback paths, maintaining the existing priority chain (OAuth → API Key → Codex native auth).- Test updates are well-placed — the
OpenCodeAuthDecodingTestsnow assert both the positive case (openaiAPIKey?.key == "sk-test-openai") and the mutual exclusion case (openaiAPIKeyis nil when OAuth succeeds). - Status/debug display updates are thorough — token status, debug info, and the ChatGPT section all account for the new API key variant.
- All CI checks passing: Lint ✅, Build ✅, CI ✅, Test ✅.
One thought (not blocking)
The getOpenAIAccounts() API key account creation path (lines 2833-2848) doesn't have a dedicated unit test. The decoding is covered, but there's no test verifying that getOpenAIAccounts() returns an account with source: .opencodeAuth and sourceLabels: ["OpenCode (API Key)"] when auth.json has an API key entry. Might be worth adding if you're in the neighborhood — but CI is green and the path is straightforward, so not blocking.
✅ AI Code Review CompletedReview has been posted — 1 inline comment, approval granted. all done.
Reviewed with |
There was a problem hiding this comment.
Review Summary
Solid follow-up — both suggestions from the previous review are implemented cleanly. The assert guard catches future interleave mismatches, and the new testGetOpenAIAccountsIncludesOpenCodeAPIKeyAccount test covers the end-to-end API key account discovery path that was missing.
What's good
clearOpenCodeAuthCacheForTesting()is the right approach — wraps all three cache fields in aqueue.syncblock, no shortcuts. Clean test helper.- The test itself is thorough: creates a temp directory, overrides
XDG_DATA_HOME, writes a minimal auth.json with API key format, verifies all account fields (accessToken,authSource,sourceLabels,source), and properly cleans up viadefer. nice. - The
assertguard on the zip interleaving is exactly the defensive check that prevents silent data loss if someone adds a directory to only one of the twobuildOpenCodeFilePathscalls later.
One nit (not blocking)
Lines 117–122 in testGetOpenAIAccountsIncludesOpenCodeAPIKeyAccount save the original XDG_DATA_HOME and immediately set/unset it back — this is a no-op. The actual restore happens in the defer block (lines 123–130), which is correct. The pre-defer block is dead code that just reads the value and writes it right back. Harmless but a bit confusing to read.
CI Status
- SwiftLint: ✅ passed
- GitHub Actions Lint: ✅ passed
- Build & Test: 🔄 still running at time of review
Smoke tests (build, typecheck) could not be run locally — this is a macOS Swift project and the review runner is Linux.
| let originalXDGDataHome = ProcessInfo.processInfo.environment["XDG_DATA_HOME"] | ||
| if let originalXDGDataHome { | ||
| setenv("XDG_DATA_HOME", originalXDGDataHome, 1) | ||
| } else { | ||
| unsetenv("XDG_DATA_HOME") | ||
| } |
There was a problem hiding this comment.
This block (lines 117–122) reads the current XDG_DATA_HOME value and immediately sets it back to the same value — it's a no-op. The real restore logic is in the defer block below, which is correct. You can safely remove this pre-defer block to reduce confusion:
| let originalXDGDataHome = ProcessInfo.processInfo.environment["XDG_DATA_HOME"] | |
| if let originalXDGDataHome { | |
| setenv("XDG_DATA_HOME", originalXDGDataHome, 1) | |
| } else { | |
| unsetenv("XDG_DATA_HOME") | |
| } | |
| let originalXDGDataHome = ProcessInfo.processInfo.environment["XDG_DATA_HOME"] | |
| defer { |
|
this is so cool |
2 participants
Summary
opencode.jsoncbeforeopencode.jsonwhen OpenCode Bar reads OpenCode config