fix(config): back up non-object configs instead of repairing them

[luvs01]

Summary

• A config.json containing a top-level JSON array (e.g. []) passed typeof === "object", so the schema-fail repair path spread it into defaults and produced a schema-valid config. The original file was then treated as "repaired" — no config.json.invalid-* backup was written, so the operator's actual file content was unrecoverable once something else overwrote it.
• The repair path now only merges object-shaped configs; any other top-level JSON value takes the existing invalid-file backup path (warnAndBackupInvalidConfig + defaults), matching what already happened for malformed JSON.

Verification

• bun test tests/server/config.test.ts — 224 pass, including 5 new cases asserting a top-level number / boolean / string / array / null is backed up verbatim and falls back to defaults.
• bun x tsc --noEmit — clean.

Checklist

• Scope stays focused and avoids unrelated cleanup.
• Docs or release notes were updated when needed.
• Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

• All CI tests are green on my local testing.

• I pushed my PR to the latest dev commit.

• I resolved all correct Codex and CodeRabbit findings.

• My PR is ready for review.

Summary by CodeRabbit

• Bug Fixes
  • Invalid configuration files containing top-level values other than an object are now rejected and backed up.
  • The application falls back to default configuration instead of attempting to repair unsupported values.
  • An error is logged when an invalid configuration is encountered.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

• ✅ All CI tests are green on my local testing.
• ✅ I pushed my PR to the latest dev commit.
• ✅ I resolved all correct Codex and CodeRabbit findings.
• ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e0912779-4c4a-4321-9de5-e5ab092a813c

📥 Commits

Reviewing files that changed from the base of the PR and between 447ac22 and 663e2a2.

📒 Files selected for processing (2)

• src/config.ts
• tests/server/config.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

---

📝 Walkthrough

Walkthrough

loadConfig now rejects top-level JSON values that are not objects. It backs up the invalid file, logs an error, and returns default configuration. Tests cover primitive values, arrays, and null.

Changes

Configuration validation

Layer / File(s)	Summary
Reject non-object configuration values src/config.ts:250-256, tests/server/config.test.ts:1921-1941	loadConfig backs up null, primitives, and arrays before repair and returns getDefaultConfig(). Parameterized tests verify the backup contents, error logging, and default result.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: non-object configuration values are backed up instead of repaired.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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

[lidge-jun]

리뷰 · 우선순위 67 / 80

이 PR은 config.json이 객체가 아닐 때(예: [], null, 숫자, 문자열, 불리언) 조용히 “고친 척”하다가 원본을 잃던 구멍을 막는 버그 수정이다. 베이스는 dev, 팁은 663e2a22e0b6, 지금은 draft다.

예전에는 JSON이 파싱만 되면 typeof === "object"를 통과했다. 배열도 객체로 보이기 때문이다. 스키마 검사에 실패하면 mergeConfigDefaults로 기본값과 합쳤고, 그 결과가 스키마를 통과하면 “수리됨”으로 취급했다. 이때 config.json.invalid-* 백업이 안 생겨서, 나중에 다른 경로가 파일을 덮어쓰면 운영자가 넣었던 내용(빈 배열이든 무엇이든)을 되돌릴 수 없었다.

이번 변경은 스키마 실패 직후, 수리(merge)로 들어가기 전에 한 줄을 더 본다. null이거나, 객체가 아니거나, 배열이면 warnAndBackupInvalidConfig로 원문을 백업하고 getDefaultConfig()로 돌아간다. 깨진 JSON(문법 오류)과 같은 실패 경로다. 객체인데 필드가 부족한 경우({ "port": 10100 } 같은 것)는 예전처럼 merge 수리를 탄다.

테스트는 tests/server/config.test.ts에 test.each로 숫자·불리언·문자열·배열·null 다섯 가지를 넣었다. 각각 기본 설정으로 떨어지고, 백업 파일이 하나 생기며, 백업 내용이 원문과 같고, 에러 로그에 “Could not load opencodex config”가 남는지를 본다. 작성자는 관련 bun 테스트 224통과·tsc 깨끗하다고 적었다. 리뷰 시점 호스티드 CI는 hygiene·label·enforce-target·CodeRabbit 정도만 보이고, draft라 본격 테스트 잡은 아직 안 돌거나 대기인 상태로 보인다. CodeRabbit은 actionable 코멘트 없이 통과했다. 중복·무효화된 형제 PR은 검색상 이 한 건이다.

라인 - src/config.ts loadConfig: 새 가드가 sanitize*ForLoad(parsed) 호출 이후에 있다. 지금 테스트가 비객체 다섯 종을 통과하니 sanitize가 비객체에서도 안 깨지는 상태로 보이지만, “수리 불가 값은 파싱 직후 바로 백업”이 의도면 가드를 sanitize 앞으로 옮기는 편이 읽기 쉽다. 필수는 아니다.

라인 - src/config.ts 실패 반환: 새 경로는 기존 깨진 파일 경로와 같이 getDefaultConfig()만 반환한다. 파일이 없을 때는 withRefreshedCostOverlays(getDefaultConfig())를 쓴다. 이번 PR이 새로 만든 차이는 아니고, “깨진 파일도 오버레이를 갱신할지”는 원래부터 있던 갈림길이다. 여기서는 기존 패턴과 맞춰 둔 것으로 보면 된다.

라인 - PR 상태: draft이고 readiness 체크리스트가 0/4다. tip 기준 핵심 테스트 CI도 아직 거의 안 보인다. 머지 판단은 ready 전환 + exact-head 초록 뒤에 하면 된다.

메인테이너의 판단이 필요한 지점

가드를 sanitize 앞으로 옮길지(가독성·방어 순서), 아니면 지금처럼 “스키마 실패 후 수리 직전”에 둘지. draft 체크리스트·CI를 머지 블로커로 볼지(보통은 ready 후에 보면 된다). 데이터 유실 버그라 우선순위는 낮지 않다.

너의 추천

원인(배열이 typeof object)과 고침(비객체는 백업 후 기본값)이 맞고, 범위도 src/config.ts 한 가드 + 대응 테스트로 작다. ready로 올린 뒤 tip CI가 초록이면 머지 후보로 두면 된다. sanitize 앞당김은 있으면 좋고 없어도 머지를 막을 정도는 아니다. preview deploy 이야기는 하지 않았다.

이 댓글은 grok-bot이 작성했습니다

[Ingwannu]

The exact-head change is correct and appropriately narrow: only plain object-shaped JSON enters repair/salvage, while primitives, arrays, and null take the existing verbatim backup path before defaults are returned. The five regression cases cover the data-loss boundary.

This head is now behind current dev, and I cannot use GitHub update-branch because the base includes workflow changes while the maintainer OAuth token lacks workflow scope. Please refresh onto current dev, re-complete the readiness checklist, and request exact-head CI; absent a new conflict, this remains an approval candidate.

[luvs01]

Refreshed onto current dev via update-branch (head 50f154d) — the change itself is untouched, only the dev merge was added. Re-completed the readiness checklist; exact-head CI is running.

[lidge-jun]

Superseded by #5609, which is open and not yet merged. The non-object config backup (663e2a2) is reimplemented there with you as co-author. To keep src/config.ts under its line cap, the persisted-mutation block moved unchanged into its own module, and the regression test now lives in tests/config/config-non-object-backup.test.ts. Thank you!