Skip to content

refactor(config): extract provider validation boundary - #2380

Open
Ingwannu wants to merge 1 commit into
devfrom
ingw/refactor-provider-validation
Open

refactor(config): extract provider validation boundary#2380
Ingwannu wants to merge 1 commit into
devfrom
ingw/refactor-provider-validation

Conversation

@Ingwannu

@Ingwannu Ingwannu commented Aug 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • Extract the pure provider payload validators from the config persistence module into src/config/provider-validation.ts.
  • Keep every existing src/config.ts export as a compatibility facade while moving direct CLI and management-only consumers to the leaf module.
  • Add focused characterization coverage for URL, header, transport, numeric-map, model-list, reasoning-summary, and model-adapter validation contracts.
  • Document the ownership boundary and decision rationale in structure/02_config-and-codex-home.md.
  • Preserve validation order, error strings, persisted schema, management response shapes, and provider runtime behavior.

Closes #2379

Verification

  • Exact head: bun test tests/provider-config-validation.test.ts tests/management-provider-validation.test.ts tests/management-origin-tls.test.ts tests/server-auth.test.ts — 174 pass, 0 fail.
  • Exact head: bun run typecheck — passed.
  • Exact head: git diff --check origin/dev...HEAD — passed.
  • Exact head repository runner under a two-core CPU cap: 14,323 pass, 16 skip, 2 fail across 900 files. Both failures are outside this diff in tests/cursor-desktop-exec.test.ts and report Bun child-process EPIPE; the failing record-screen case reproduces with the same pinned Bun 1.4.0 command in a clean origin/dev worktree. The changed provider-validation and management/auth test surfaces are green.
  • bun run privacy:scan was not run because security scanning was explicitly excluded from this task. No request, credential, logging, or persistence behavior is changed; the auth-cors.ts edit is import-only and still requires another maintainer's review under repository policy.

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. Another maintainer must review the import-only auth-cors.ts change before merge.

Summary by CodeRabbit

  • Bug Fixes

    • Improved provider configuration validation for URLs, headers, API-key transport, HTTP versions, model adapters, capabilities, and reasoning settings.
    • Added checks for incompatible model overrides, unsupported hosted-tool configurations, invalid values, and unsafe headers.
    • Improved handling of provider-specific model and authentication settings.
  • Documentation

    • Documented provider validation rules and their consistent application across configuration interfaces.
  • Tests

    • Added comprehensive coverage for provider configuration validation scenarios.

@Ingwannu
Ingwannu requested a review from lidge-jun August 22, 2026 11:59
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d781bb3a-98da-4aab-8292-8f7e97aa52ca

📥 Commits

Reviewing files that changed from the base of the PR and between 5e50590 and 7b07f11.

📒 Files selected for processing (7)
  • src/cli/provider.ts
  • src/config.ts
  • src/config/provider-validation.ts
  • src/server/auth-cors.ts
  • src/server/management/provider-capability-config.ts
  • structure/02_config-and-codex-home.md
  • tests/provider-config-validation.test.ts

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


📝 Walkthrough

Walkthrough

Provider validation was extracted into src/config/provider-validation.ts. src/config.ts keeps compatibility re-exports. CLI and server imports use the new module. Documentation records the ownership boundary. Focused tests cover validation and normalization behavior.

Changes

Provider validation boundary

Layer / File(s) Summary
Shared provider validators
src/config/provider-validation.ts
Adds validation for provider URLs, headers, API-key transport, HTTP versions, numeric records, model lists, capability maps, reasoning summaries, and model adapter overrides.
Compatibility exports and caller imports
src/config.ts, src/cli/provider.ts, src/server/auth-cors.ts, src/server/management/provider-capability-config.ts
src/config.ts re-exports the shared helpers. CLI and server callers import provider-specific validation from the dedicated module.
Validation characterization tests
tests/provider-config-validation.test.ts
Adds coverage for valid and invalid provider payloads, normalization, cross-field conflicts, wire-pinned models, and canonical-forward providers.
Validation ownership documentation
structure/02_config-and-codex-home.md
Documents the shared validation module and compatibility re-exports.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 7b07f

This refactor preserves validation and runtime contracts, with focused tests and typechecking passing. No actionable merge-blocking risk remains.

Suggested reviewers: lidge-j

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 6 files. (1 skipped: 1 unsupported.) Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: extracting the provider validation boundary.
Linked Issues check ✅ Passed The PR satisfies issue #2379 by extracting shared validators, preserving compatibility and behavior, adding tests, and documenting the ownership boundary.
Out of Scope Changes check ✅ Passed The changes are limited to the requested validation extraction, compatibility imports, characterization tests, and ownership documentation.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ingw/refactor-provider-validation

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.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Aug 22, 2026
@Ingwannu
Ingwannu force-pushed the ingw/refactor-provider-validation branch from c020858 to 0664a7b Compare August 22, 2026 12:33
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 49 / 80

설명: 이 PR은 제공자 값이 맞는지 보는 일을 src/config.ts 에서 꺼내 src/config/provider-validation.ts 잎으로 옮긴다. 지금 CURRENT dev HEAD 는 e1d197565 이다. 이번 시간에 origin/dev 가 b268d1814 에서 여기로 옮겼다. 착지한 코드는 2381 WP2 기록(021_wp2_execution_record.md. 2310 은 이미 머지. 2350 빈 도구 출력, 2351 설정 변경 감사, 2355 설정 어긋남 경고는 재현된 결함으로 열어 둠), 2382 윈도 데스크톱 앱 재시작, 2384 WP4 기록(문서만. 2292 는 Refs 라 이슈는 열어 둠)이다. package.json 은 2.27.0 이다. 지금 HEAD 의 src/config.ts 는 3975줄이다. 이슈 2379 가 말한 1780줄은 지금 파일이 아니다. 검사 함수는 759줄 근처다. 이미 있는 잎은 src/config/provider-name.ts 이고 config.ts 757줄이 그 이름을 다시 보낸다. 이 PR 은 그 본보기를 따른다. Closes 2379 다. 옮긴 것은 providerBaseUrlConfigError, providerHeadersConfigError, apiKeyTransportConfigError, upstreamHttpVersionConfigError, 양의 정수 맵, 빈칸 없는 모델 목록, 불리언 맵, 추론 요약 전달, modelAdapterRecordConfigError 다. 에러 글자와 거절 순서는 그대로다. 민감 헤더 집합과 URL 자격 증명 거절도 그대로다. modelAdapterRecordConfigError 는 isWirePinnedModel 과 isCanonicalOpenAiForwardProvider 를 본다. 이건 레지스트리 전체가 아니라 타입과 오픈에이아이 티어라서 잎으로 가도 된다. 반대로 modelPreferHostedToolsConfigError 는 getProviderRegistryEntry 와 providerMatchesRegistryTransport 를 보므로 지금 HEAD 980줄에 그대로 남긴다. 그게 맞다. providerModelCostsConfigError, sanitizeModelCostsForDisplay, requestPacingConfigError, retryOn429PolicyConfigError, providerResponsesPathConfigError 도 config.ts 에 남는다. src/server/auth-cors.ts 는 잎에서 순수 검사를 가져가고, 레지스트리를 보는 것과 비용 검사는 여전히 config 에서 가져간다. src/cli/provider.ts 는 apiKeyTransportConfigError 만 잎에서 가져간다. 드래프트다. 보안 칸은 auth-cors 임포트 때문에 비어 있다. 베이스는 425e8bda7 이라 지금 HEAD 보다 2382 와 2384 문서만 뒤다. 파일은 안 겹친다. 사용자 길이 버그가 아니라 모듈 경계 일이라서 49. 카탈로그 팁은 Ox Alpha x-preview-f-free + deepseek-v4-flash-vision-exp. Cursor 정적 카탈로그는 opus-4-8-fast / opus-5-fast. 2334 CursorCredentialRouter 는 여전히 src/providers/cursor-pool.ts 모듈+테스트만 있고 어댑터에 연결되지 않았다. 2332 H2 는 discovery 전용. 2320 overflow + 2342 는 이미 dev. 2188 사이드카는 이미 dev. src/runtime 폴더는 지금 HEAD 에 없다.

src/config.ts 라인 759 - 지금 HEAD 의 providerBaseUrlConfigError. PR 은 이 함수를 src/config/provider-validation.ts 로 옮기고 config.ts 가 다시 보낸다
src/config.ts 라인 866 - apiKeyTransportConfigError. 앤트로픽 키 인증에서만 x-api-key 또는 bearer. CLI 가 잎에서 직접 가져가게 된다
src/config.ts 라인 980 - modelPreferHostedToolsConfigError. 레지스트리를 본다. 이 PR 은 옮기지 않았다. 순수 잎이라고 우기지 않은 것이 맞다
src/config.ts 라인 1081 - modelAdapterRecordConfigError. 와이어 핀과 캐논 포워드 제한을 잎으로 가져간다. 레지스트리 전체가 아니다
src/server/auth-cors.ts 라인 3 - 지금 HEAD 는 검사 이름을 전부 config 에서 가져온다. PR 은 순수 검사만 잎으로, 비용과 호스티드 도구는 config 에 남긴다

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

  • 레지스트리를 보는 modelPreferHostedToolsConfigError 를 다음 잎에 둘지, 지금처럼 config.ts 에 둘지
  • auth-cors.ts 의 임포트 나눔을 이 PR 에서 보안 리뷰할지. 동작은 안 바뀌지만 관리 API 검사 입구다
  • 2378 프로세스 상태 추출과 순서를 어떻게 할지. 둘 다 config.ts 를 만진다

너의 추천
체크리스트와 보안 칸을 채운 뒤에 머지한다. 지금 HEAD 의 2382/2384 는 다른 파일이라 이 잎과 안 싸운다. 425e8bda7 위에 그대로 둬도 된다. 2378 과 같은 PR 에 넣지 않는다. 2379 는 Closes 가 있으므로 머지되면 이슈가 닫힌다. 남겨 둔 비용 검사와 호스티드 도구 검사는 다음 PR 로 둔다. types.ts 스플릿과 겹치면 닫고 리베이스하지 않는다. 지금은 그 정도 아님. 라벨은 그대로 둔다. 프리뷰 배포가 아니다.

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

@Ingwannu

Copy link
Copy Markdown
Owner Author

Rebased this focused extraction onto current dev (5e5059044) without conflicts.

Exact new head verification:

  • provider/config/management/auth characterization: 174/174
  • strict typecheck: passed
  • git diff --check origin/dev...HEAD: passed

The import-only auth-cors.ts boundary still needs another maintainer's review. I am leaving the PR draft until exact-head CI completes; I will not self-approve or self-merge it.

@Ingwannu
Ingwannu force-pushed the ingw/refactor-provider-validation branch from 0664a7b to 7b07f11 Compare August 22, 2026 13:24
@Ingwannu
Ingwannu marked this pull request as ready for review August 22, 2026 13:34
@Ingwannu

Copy link
Copy Markdown
Owner Author

Exact-head GitHub CI is now fully green on 7b07f1114, including Linux, macOS, Windows/npm packaging, all four test shards, typecheck, GUI gates, storage/API checks, and repository hygiene. I have marked the PR ready for review and am leaving it unmerged for @lidge-jun's required review of the auth-cors.ts import boundary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants