Skip to content

feat(catalog): provider-level auto-review model selection - #4100

Draft
harryzhou2000 wants to merge 1 commit into
lidge-jun:devfrom
harryzhou2000:feat/provider-auto-review-model-override-v2
Draft

feat(catalog): provider-level auto-review model selection#4100
harryzhou2000 wants to merge 1 commit into
lidge-jun:devfrom
harryzhou2000:feat/provider-auto-review-model-override-v2

Conversation

@harryzhou2000

@harryzhou2000 harryzhou2000 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Codex chooses the model that reviews approval requests from the catalog field auto_review_model_override. OpenCodex already ships a root auto_review_model selector in $CODEX_HOME/config.toml, but that selector is global. This PR adds provider-scoped selection so routed providers can use different, usually cheaper, reviewers:

  • providers.<name>.autoReviewModel: provider-wide reviewer target.
  • providers.<name>.autoReviewModelOverrides: per-model reviewer targets, keyed by upstream model ids; entries win over the provider-wide value.

Values may be a bare model id of the same provider or a full public catalog slug such as opencode-go/deepseek-v4-flash. During catalog sync each selector is resolved against the final catalog and stamped as auto_review_model_override on routed rows. A provider stamp wins over the root selector on its own routed rows; the root selector remains the fallback for native rows and routed rows without a provider stamp. Removing the root selector never clears provider stamps, and removing a provider selector clears only that provider's stamps. Unknown or malformed targets fail closed for the override only, emit a diagnostic, and preserve normal upstream auto-review behavior.

The fields are carried through config load with degradation, provider management POST and PATCH, and the provider editor DTO. The canonical openai provider rejects them. Documentation is added to the provider configuration reference.

This is the focused follow-up invited when the earlier provider-scoped PR #2527 was closed as superseded by the global selector.

Verification

  • bun run typecheck: clean.
  • bun run privacy:scan: clean.
  • bun test tests/codex-integration/codex-catalog.test.ts: 319 pass / 0 fail.
  • bun test tests/codex-integration/codex-convergence-account-selectors.test.ts: 28 pass / 0 fail.
  • Focused provider management and config-load suites pass.
  • Full bun run test was run after rebasing onto the latest dev. The only failing case is tests/update/update-stop-first.test.ts > "npm launcher restarts the stopped runtime after a staged update failure", which also fails identically on a pristine upstream/dev snapshot in this environment; it is unrelated to this change.
  • An independent pre-push audit found no blockers; all substantive findings were fixed with regression tests.
  • git diff --check: clean.

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.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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 github-actions Bot added enhancement New feature or request intake: hygiene-blocked Deterministic PR hygiene checks failed labels Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/server/auth-cors.ts.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • hygiene: unsponsored_surface.

What to do

  • Fix unsponsored_surface — This changes an authentication, workflow, release-automation, or dependency surface. MAINTAINERS.md requires security review for these; ask a maintainer to apply maintainer-sponsored once they have reviewed it. Paths: src/server/auth-cors.ts.

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 was already a draft. Its draft status will be preserved after every issue above is resolved.

@harryzhou2000
harryzhou2000 force-pushed the feat/provider-auto-review-model-override-v2 branch from 8a0d94b to 1afdd77 Compare September 9, 2026 08:46
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 66 / 80

설명

이 PR(#4100, @harryzhou2000, Draft)은 제공자마다 다른 승인(auto-review) 모델을 고르게 하는 카탈로그/설정 확장이다. 지금 dev HEAD는 04808916c(#4098 design-dial fence MD040)이고 패키지는 2.49.0이다. 루트 $CODEX_HOME/config.tomlauto_review_model은 이미 전 카탈로그에 한 명의 리뷰어를 찍는다. 그런데 라우트된 제공자마다 더 싼 리뷰어를 쓰고 싶을 때는 전역 한 줄로는 부족하다. 그래서 이 PR은 config.jsonproviders.<name>.autoReviewModel(제공자 전체)과 providers.<name>.autoReviewModelOverrides(업스트림 모델 id별)를 추가하고, 카탈로그 sync 때 최종 카탈로그에 맞춰 auto_review_model_override를 스탬프한다.

왜 지금 dev와 맞닿는가. 예전에 #2527(같은 축, 제공자 스코프 auto-review)이 전역 셀렉터에 밀려 닫혔고, 그때 “나중에 제공자 스코프 follow-up”을 초대했던 바로 그 후속이다. 런타임 GUI 핫픽스(#3719/#3379/#3774 등)와는 직교하지만, 카탈로그·제공자 설정·관리 API 축이라 2.49 문서/설정 완성도와는 잘 맞는다. types.ts/config.ts 대형 분할에 걸려 무효화될 모양은 아니다. 중복 close 대상도 아니다(#2527은 이미 CLOSED, 이 브랜치는 v2 재작성).

무엇을 건드리는가. 규모는 약 +910/−17이다. 핵심은 src/codex/catalog/sync.tsapplyConfiguredAutoReviewModelOverride / finalizeAutoReviewModelOverride(config를 넘김), src/config/provider-validation.ts의 셀렉터 검증·정규화, src/config.ts load-time degrade(sanitizeAutoReviewForLoad), src/server/management/provider-routes.ts PATCH/POST 보존, src/server/auth-cors.tsproviderManagementConfigError와 editor field policy, src/types/provider.ts 타입, docs providers.md, 그리고 catalog/convergence/config-load/management 테스트다. 제공자 스탬프가 있으면 그 라우트 행에서는 루트보다 이기고, 루트를 지워도 제공자 스탬프는 남기며, 잘못된 타깃은 그 오버라이드만 fail-closed하고 진단만 남긴다. canonical openai 제공자는 이 필드를 거절한다. 설계 문장과 테스트 의도가 대체로 한 줄로 맞는다.

게이트 상태. 라벨에 enhancementintake: hygiene-blocked가 같이 있다. hygiene/enforce-target이 unsponsored_surface 로 실패했고, 경로는 src/server/auth-cors.ts다. 실제 diff는 CORS 정책이 아니라 제공자 관리 DTO 검증·editor 필드 등록이지만, 파일 자체가 보안 표면 목록에 있어 MAINTAINERS.mdmaintainer-sponsored가 필요하다. PR은 아직 Draft이고 CodeRabbit도 draft skip이다. 본문 checklist는 체크돼 있지만, 게이트가 hygiene에 막혀 ready 전환이 안 된 상태다. 로컬 검증 주장은 typecheck/privacy/catalog·convergence·management 테스트 통과, 전체 test에서 update-stop-first만 upstream dev와 동일 실패라고 적혀 있다.

라인 - 이게 무슨 문제다

src/server/auth-cors.ts providerManagementConfigError / PROVIDER_CONFIG_FIELD_POLICY - 기능상으로는 관리 API 검증과 editor 허용 필드 추가뿐이다. 그래도 hygiene가 unsponsored_surface로 막고 라벨 intake: hygiene-blocked가 붙어 있다. 머지 전에 메인테이너가 한 번 훑고 maintainer-sponsored를 달지 않으면 게이트가 통과하지 않는다.

src/codex/catalog/sync.ts opencodex_auto_review_root 마커 - 루트가 찍은 native 행에 내부 키를 붙여 카탈로그 JSON에 같이 쓴다. Codex가 모르는 필드는 보통 무시하지만, 유저 디스크 카탈로그에 OpenCodex 전용 메타가 남는 부작용이다. 제거 시 루트/제공자 스탬프를 구분하려고 넣은 장치라 이해는 된다. 다만 “카탈로그는 upstream shape만” 원칙과 충돌하면 마커를 sidecar/메모리로 빼는 편이 더 깨끗하다.

src/codex/catalog/sync.ts applyConfiguredAutoReviewModelOverride - if (selected || (perModel !== undefined)) providerStamped.add(entry) 에서 perModel만 있고 selected가 없는 경우는 selected = perModel ?? plan.wide 때문에 사실상 안 생긴다. 죽은 조건이라 읽기만 헷갈린다. if (selected) { ...; providerStamped.add(entry); } 로 줄여도 동작은 같다.

src/server/management/provider-routes.ts POST 보존 - 폼이 auto-review 필드를 안 보내면 기존 값을 살려 둔다. PATCH null로만 지운다. 좋다. 다만 WebUI에 실제 입력 칸이 아직 없다면 문서+API만 열린 설정이 된다. editor policy에만 넣고 UI 필드가 없으면 운영자가 Dashboard에서 못 고친다(파일/API로만 가능).

docs-site/.../providers.md - 예시에 opencode-go/deepseek-v4-flash, kimi-k3gpt-5.6-terra가 나온다. 문서는 명확하다. 루트 섹션에 “provider-scoped가 먼저” 한 줄도 추가됐다. docs-only 리스크는 낮다.

테스트 - catalog/convergence/config-load/management에 회귀가 꽤 있다(+약 380 테스트 줄). 셀렉터 해석·openai 거절·load degrade 축은 잠근 느낌이다. 전체 suite의 update-stop-first 실패는 이 PR과 무관하다고 본문이 주장하니, 머지 판단에서는 이 HEAD의 hygiene/test gates만 보면 된다.

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

  • src/server/auth-cors.ts 터치에 maintainer-sponsored를 줄지(실질은 management validation이지만 파일 경로가 게이트를 탄다).
  • opencodex_auto_review_root를 카탈로그 JSON에 남겨도 될지, 아니면 외부 메타로 뺄지.
  • Draft를 스폰서 후 Ready로 올려 2.49.x 설정 열차에 태울지, UI 입력 칸까지 붙인 뒤 받을지.
  • feat(catalog): provider-level auto-review model override #2527 닫힘 메모의 follow-up으로 이 PR을 공식 후속으로 인정할지.

너의 추천

방향은 받고, 지금은 스폰서+Draft 해제가 먼저다. 코드/문서/테스트는 전역 auto-review의 자연스러운 제공자 스코프 확장이고 #2527 초대를 이행한다. 다음 스텝: (1) 메인테이너가 auth-cors diff를 확인한 뒤 maintainer-sponsored 부여, (2) hygiene/enforce-target green 확인, (3) Ready for review로 전환, (4) CI green이면 dev 머지. UI 필드가 없다면 문서에 “PATCH/API 또는 config.json으로만”이라고 한 줄 보강하거나 후속 이슈로 남겨라. types/config 분할 무효화·중복 close 아님. 설정 완성도 대비 게이트 미완이라 66.

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

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

Labels

enhancement New feature or request intake: hygiene-blocked Deterministic PR hygiene checks failed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants