Skip to content

fix(cli): scope model removal selectors to provider - #2821

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:fix/provider-scoped-model-removal
Aug 28, 2026
Merged

lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:fix/provider-scoped-model-removal

Conversation

@luvs01

@luvs01 luvs01 commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Parse the provider prefix in ocx models remove provider/model once and evaluate only custom-model rows owned by that provider.
  • Prevent a selector for one provider from deleting a different provider's row merely because that row's native model id resembles the full selector.
  • Preserve exact UUID removal, same-provider slash/dash equivalence, and fail-closed ambiguity handling introduced by fix(cli): route models remove through the shared slug relation #2596.

Verification

  • Stable Bun 1.4.0: bun test tests/cli-models.test.ts — 23 passed, 0 failed.
  • Stable Bun 1.4.0: bun run typecheck.
  • Stable Bun 1.4.0: bun run privacy:scan.
  • git diff --check.

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
    • Improved custom model removal for provider-qualified selectors.
    • Selectors such as provider/model now remove only the matching model from that provider.
    • Prevented models with the same ID under different providers from being removed accidentally.
    • Non-qualified selectors continue to match exact custom model IDs.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 28, 2026
@coderabbitai

coderabbitai Bot commented Aug 28, 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: 469d6a8c-3c9b-496f-a826-c11d7a141aa8

📥 Commits

Reviewing files that changed from the base of the PR and between f1d819b and d21ad61.

📒 Files selected for processing (2)
  • src/cli/models.ts
  • tests/cli-models.test.ts

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


📝 Walkthrough

Walkthrough

Custom model removal now parses provider-qualified selectors, restricts matches to that provider, preserves exact-ID matching for non-slash selectors, and adds tests for matching and not-found cases.

Changes

Custom model removal

Layer / File(s) Summary
Provider-scoped removal matching
src/cli/models.ts, tests/cli-models.test.ts
Slash-form selectors such as openai/gpt-5.5 now match only custom models from the selected provider. Non-slash selectors retain exact-ID matching. Tests verify both selective removal and the provider-mismatch not-found error.

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

Merge Risk: ⚪ Minimal · up to d21ad

The change restricts model removal selectors to the specified provider and adds coverage for cross-provider safety; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: ingwannu, lidge-jun

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. 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 main change: model removal selectors are now scoped to the specified provider.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 commented Aug 28, 2026 •

Copy link
Copy Markdown
Contributor

✅ 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

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 62 / 80

설명

이 PR은 ocx models remove provider/model이 다른 제공자 행을 같이 지우는 구멍을 막는다. 지금 dev HEAD는 d7a82a8fc이다. 방금 #2820(dest CI 정직)과 #2819(키로 빈 exec 안내 + 끝난 답을 다시 열지 않기)가 들어왔다. 둘 다 어댑터·턴 종료 축이다. 이 변경은 src/cli/models.ts의 커스텀 모델 삭제라서 축이 다르다. types.ts/config.ts 스플릿에 밀려 닫을 대상도 아니고, 이미 같은 일을 하는 열린 PR도 없다.

지금 HEAD의 handleCustomRemove는 선택 문자열에 슬래시가 있으면 customModels의 모든 행에 대해 resolveSlugSelection(그 행의 provider, target, [그 행의 modelId])를 돌린다. src/providers/slug-codec.ts의 이 함수는 선택이 provider/로 시작하지 않으면, 선택 문자열 전체를 그 행 제공자의 네이티브 아이디로 다시 붙인다. 그래서 openai/gpt-5.5를 지우라고 하면, provider가 openai이고 modelId가 gpt-5.5인 행뿐 아니라, provider가 test이고 modelId가 하필 openai/gpt-5.5인 행도 자기 아이디와 같은 키로 맞는다. 맞는 행이 둘이면 모호하다고 거절하고, 하나면 그 하나(다른 제공자 행)를 지운다. 파괴 명령인데 제공자 칸을 안 보는 셈이다.

고치는 방법은 단순하다. 슬래시가 있으면 그 앞을 제공자 이름으로 한 번만 읽고, model.provider가 그 이름인 행만 기존 해석기에 넘긴다. 슬래시가 없으면 예전처럼 커스텀 모델 UUID만 본다. 같은 제공자 안에서 슬래시/대시가 같은 아이디로 보이는 동치와, 모호하면 거절하는 규칙은 #2491 그대로다. 테스트 두 개가 그 회귀를 고정한다. 하나는 openai 행과 test 행이 같이 있을 때 openai/gpt-5.5가 openai 행만 지우는 경우고, 하나는 test 행만 있을 때 같은 선택이 not found로 끝나는 경우다.

베이스는 f1d819be8이다. #2820 직후, #2819 전이다. 만진 파일이 키로/턴 종료 디프와 안 겹쳐서 GitHub는 mergeable이다. hygiene과 enforce-target은 초록이다. 작성자가 말한 로컬 검증은 tests/cli-models.test.ts 23개, tsc, privacy:scan이다. 전체 bun run test는 안 돌렸다. 대상 브랜치는 dev라 enforce-target도 맞다.

라인 src/cli/models.ts const separator = target.indexOf("/") - 첫 슬래시만 제공자 경계로 본다. 제공자 이름에 슬래시가 들어가면 뒤가 잘린다. 지금 카탈로그 제공자 이름은 슬래시 없는 한 칸이라 실제 사고는 작다.
라인 src/cli/models.ts if (model.provider !== selectedProvider) return [] - 대소문자를 그대로 비교한다. 설정에 OpenAI로 저장된 행은 openai/gpt-5.5로 못 지운다. 예전에도 해석기가 provider/ 접두를 대소문자 그대로 봤으니 새로 생긴 구멍은 아니다.
경로 tests/cli-models.test.ts - 새 테스트가 #2491 describe 안에 들어 있다. 회귀는 막히지만, UUID로 지우는 길과 test/openai/gpt-5.5로 test 행을 지우는 길은 이 디프의 새 테스트가 아니다. 기존 테스트가 커버한다.
경로 CI - 아직 hygiene/enforce-target만 보인다. 맥 테스트 샤드 결과는 이 댓글을 쓰는 시점에 안 올라왔다.

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

  • #2819 위에 리베이스할지, 파일 안 겹치니 이대로 머지할지.
  • 제공자 이름 대소문자 정규화를 이 PR에서 할지, 후속으로 둘지.
  • 전체 테스트 샤드를 이 리뷰 봇이 보기 전에 기다릴지.

너의 추천
받아라. 파괴 명령이 다른 제공자 행을 지우는 구멍은 지금 dev의 키로 트레인과 무관하고, 고침도 한 함수 안에서 끝난다. types/config 스플릿 때문에 닫지 마라. 맥 테스트 샤드가 초록이면 리베이스 없이 머지해도 된다. 대소문자 정규화는 후속.

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

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head d21ad61. Provider-qualified destructive selectors are now scoped before shared slug resolution, so a native id owned by another provider cannot be removed. UUID removal and same-provider ambiguity behavior remain intact. The focused CLI models suite passes 23/23 under isolated HOME; the ten newer dev commits do not touch either changed file and the merge is conflict-free. No blocking issue found in this diff. Merge only after the repository-required exact-head CI is green.

lidge-jun added a commit to adtumk/opencodex that referenced this pull request Aug 28, 2026
dev's package.json said 2.35.0 while the repository had already published
v2.36.0-preview.20260829 (npm dist-tags: preview=2.36.0-preview.20260829,
latest=2.35.0). The preview bump was cut on the prerelease train and never came
back to dev, so tests/release-version-line.test.ts fails on every commit that
descends from dev:

  release version line > the in-tree version is never behind a released one
  package.json version 2.35.0 is BEHIND the highest release tag
  v2.36.0-preview.20260829

That is inherited red, not a defect in any of the pull requests hitting it. It
currently fails test 2/4, test 3/4, test 4/4, and macos on lidge-jun#2835, lidge-jun#2822, lidge-jun#2821,
lidge-jun#2796, lidge-jun#2797, and lidge-jun#2785 - six bug PRs whose own diffs are unrelated to release
tooling. Rebasing them onto an unrepaired dev cannot turn them green, which is
why this lands first.

2.36.0 rather than a preview suffix follows the precedent this repository set
twice: e4a85d1 moved dev to 2.34.0 when it trailed a published 2.33.0, and
076ad30 moved dev to 2.35.0 right after v2.34.0 shipped. dev carries the next
stable version; the preview train adds its own suffix at release time.

The value was chosen by running the repository's own comparator rather than by
reading it. Against the highest tag v2.36.0-preview.20260829, compareReleaseTags
returns -1 for 2.35.0 and 2.35.1, 0 for 2.36.0-preview.20260829 (legal only on
the commit that tag names, which a dev merge commit is not), and +1 for 2.36.0.
npm view @bitkyc08/opencodex@2.36.0 returns E404 and git tag --list v2.36.0 is
empty, so the string is unused.

Verification on this branch:
  bun test tests/release-version-line.test.ts   3 pass 0 fail (was 2 pass 1 fail)
  bun test tests/release-helper.test.ts         5 pass 0 fail
  bun test tests/compatibility-version.test.ts  1 pass 0 fail
@lidge-jun
lidge-jun merged commit 3058966 into lidge-jun:dev Aug 28, 2026
24 of 27 checks passed
tarunravi pushed a commit to tarunravi/opencodex that referenced this pull request Sep 14, 2026
dev's package.json said 2.35.0 while the repository had already published
v2.36.0-preview.20260829 (npm dist-tags: preview=2.36.0-preview.20260829,
latest=2.35.0). The preview bump was cut on the prerelease train and never came
back to dev, so tests/release-version-line.test.ts fails on every commit that
descends from dev:

  release version line > the in-tree version is never behind a released one
  package.json version 2.35.0 is BEHIND the highest release tag
  v2.36.0-preview.20260829

That is inherited red, not a defect in any of the pull requests hitting it. It
currently fails test 2/4, test 3/4, test 4/4, and macos on lidge-jun#2835, lidge-jun#2822, lidge-jun#2821,
lidge-jun#2796, lidge-jun#2797, and lidge-jun#2785 - six bug PRs whose own diffs are unrelated to release
tooling. Rebasing them onto an unrepaired dev cannot turn them green, which is
why this lands first.

2.36.0 rather than a preview suffix follows the precedent this repository set
twice: e4a85d1 moved dev to 2.34.0 when it trailed a published 2.33.0, and
076ad30 moved dev to 2.35.0 right after v2.34.0 shipped. dev carries the next
stable version; the preview train adds its own suffix at release time.

The value was chosen by running the repository's own comparator rather than by
reading it. Against the highest tag v2.36.0-preview.20260829, compareReleaseTags
returns -1 for 2.35.0 and 2.35.1, 0 for 2.36.0-preview.20260829 (legal only on
the commit that tag names, which a dev merge commit is not), and +1 for 2.36.0.
npm view @bitkyc08/opencodex@2.36.0 returns E404 and git tag --list v2.36.0 is
empty, so the string is unused.

Verification on this branch:
  bun test tests/release-version-line.test.ts   3 pass 0 fail (was 2 pass 1 fail)
  bun test tests/release-helper.test.ts         5 pass 0 fail
  bun test tests/compatibility-version.test.ts  1 pass 0 fail
tarunravi pushed a commit to tarunravi/opencodex that referenced this pull request Sep 14, 2026
agentHits pushed a commit to agentHits/opencodex that referenced this pull request Sep 17, 2026
dev's package.json said 2.35.0 while the repository had already published
v2.36.0-preview.20260829 (npm dist-tags: preview=2.36.0-preview.20260829,
latest=2.35.0). The preview bump was cut on the prerelease train and never came
back to dev, so tests/release-version-line.test.ts fails on every commit that
descends from dev:

  release version line > the in-tree version is never behind a released one
  package.json version 2.35.0 is BEHIND the highest release tag
  v2.36.0-preview.20260829

That is inherited red, not a defect in any of the pull requests hitting it. It
currently fails test 2/4, test 3/4, test 4/4, and macos on lidge-jun#2835, lidge-jun#2822, lidge-jun#2821,
lidge-jun#2796, lidge-jun#2797, and lidge-jun#2785 - six bug PRs whose own diffs are unrelated to release
tooling. Rebasing them onto an unrepaired dev cannot turn them green, which is
why this lands first.

2.36.0 rather than a preview suffix follows the precedent this repository set
twice: cfee468 moved dev to 2.34.0 when it trailed a published 2.33.0, and
2590f50 moved dev to 2.35.0 right after v2.34.0 shipped. dev carries the next
stable version; the preview train adds its own suffix at release time.

The value was chosen by running the repository's own comparator rather than by
reading it. Against the highest tag v2.36.0-preview.20260829, compareReleaseTags
returns -1 for 2.35.0 and 2.35.1, 0 for 2.36.0-preview.20260829 (legal only on
the commit that tag names, which a dev merge commit is not), and +1 for 2.36.0.
npm view @bitkyc08/opencodex@2.36.0 returns E404 and git tag --list v2.36.0 is
empty, so the string is unused.

Verification on this branch:
  bun test tests/release-version-line.test.ts   3 pass 0 fail (was 2 pass 1 fail)
  bun test tests/release-helper.test.ts         5 pass 0 fail
  bun test tests/compatibility-version.test.ts  1 pass 0 fail
agentHits pushed a commit to agentHits/opencodex that referenced this pull request Sep 17, 2026
@luvs01
luvs01 deleted the fix/provider-scoped-model-removal branch September 20, 2026 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants