Skip to content

fix(catalog): accept parser sentinels none/minimal in effort compatibility - #5296

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/effort-parser-sentinels
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/effort-parser-sentinels

Conversation

@luvs01

@luvs01 luvs01 commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • catalogEffortCompatibility flagged the parser-valid effort sentinels none and minimal as unsupported whenever they were absent from the observed native ladder, even though the Codex catalog parser accepts both.
  • Add CODEX_PARSER_SENTINEL_EFFORTS so absence from an observed bundled catalog is not treated as evidence that the selected runtime rejects them.

Verification

  • bun test tests/clients/client-catalog-compatibility.test.ts (18 pass)
  • bun x tsc --noEmit

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.

Summary by CodeRabbit

  • Bug Fixes
    • Reasoning levels none and minimal are now recognized as supported even when they are not listed by the detected runtime.
    • Catalog entries using these levels, including minimal as the default, are no longer incorrectly marked incompatible.

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 20, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

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: dd06eaae-6eaf-4d5b-a72d-b4535f993dec

📥 Commits

Reviewing files that changed from the base of the PR and between 53725b0 and ceb8807.

📒 Files selected for processing (2)
  • src/codex/catalog/effort.ts
  • tests/clients/client-catalog-compatibility.test.ts

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


📝 Walkthrough

Walkthrough

The compatibility check now accepts "none" and "minimal" even when the observed runtime ladder omits them. Tests verify sentinel entries and a "minimal" default.

Changes

Effort Compatibility

Layer / File(s) Summary
Accept parser sentinel efforts
src/codex/catalog/effort.ts, tests/clients/client-catalog-compatibility.test.ts
catalogEffortCompatibility recognizes "none" and "minimal" through a shared acceptance check. Rejected-effort filtering and default_reasoning_level validation use this check. Tests cover a runtime ladder containing only "low", "medium", "high", and "xhigh".

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 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: accepting the parser sentinel efforts "none" and "minimal" during catalog effort compatibility checks.
✨ 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

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the bug Something isn't working label Sep 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 61 / 80

이 PR은 카탈로그를 받을 때 “이 Codex가 이 effort를 아는가?”를 검사하는 catalogEffortCompatibility가, 관찰된 네이티브 사다리에 none/minimal이 없다고 해서 둘을 거부하던 버그를 고칩니다. 원래 관측은 번들 카탈로그의 네이티브 모델 줄에서 effort를 모으는데, 그 줄에는 보통 low~xhigh(또는 max)만 있고 none/minimal은 안 나옵니다. 그런데 Codex 쪽 파서와 reasoning-effort.ts는 둘을 이미 유효한 sentinel로 인정합니다. 그래서 허브 카탈로그에 none/minimal이 있으면 런타임이 사실 파싱할 수 있는데도 호환 불가로 막힐 수 있었습니다. 이번 변경은 CODEX_PARSER_SENTINEL_EFFORTS로 두 값을 예외로 두고, 테스트로 “관측 사다리에 없어도 통과”를 고정했습니다. base는 dev이고 범위도 작습니다.

라인 - src/codex/catalog/effort.ts catalogEffortCompatibility: 수정 방향은 isCodexDeclaredEffort / sentinel 설명과 맞음. 다만 clampEntryToCodexSupportedEfforts는 여전히 UNCLAMPABLE(max/ultra)만 예외라, 같은 관측 집합에서 none/minimal은 호환 검사에서는 살고 clamp에서는 잘릴 수 있음
라인 - CODEX_PARSER_SENTINEL_EFFORTSreasoning-effort.tsnone/minimal 정의와 문자열이 중복됨. 나중에 한쪽만 바뀌면 다시 어긋날 여지

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

호환 게이트(허브 카탈로그 거부)만 고치면 충분한지, 아니면 clamp 경로에도 같은 sentinel 예외를 맞춰야 하는지. 이 PR 본문은 호환성 오탐만 다루므로 clamp 비대칭을 이번 범위 밖으로 두는 선택은 타당해 보이지만, 의도적으로 남긴 건지 한 줄 확인이 있으면 좋습니다.

너의 추천

머지해도 됩니다. 회귀 테스트가 핵심 케이스를 잡고 있고, 변경이 한 함수와 한 테스트로 좁습니다. 후속으로 sentinel 집합을 reasoning-effort에서 공유하거나 clamp와의 예외 정책을 문서/코드로 맞추면 더 안전합니다.

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

@github-actions

github-actions Bot commented Sep 20, 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.

Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@luvs01

luvs01 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator Author

Consolidated into #5529 in native Stack #5505.

Source head: ceb88071abb5d860fa0d24b17fd37e3b97062697. Replacement head: d11f35f56f1da8bcf38e9db788d14892a49c31ee.

Source commits match by stable Git patch ID. Follow-up d11f35f fixes case-folded cap handling and moves the unchanged exclusion regression into a registered sibling file. Focused combined-head tests: 44 passed; prepared catalog tests: 515 passed. Full CI, docs build and independent review remain pending.

Closing this duplicate standalone review entry at the author's request after verifying migration. This is not a merge or release claim; remaining integration checks and reviews are tracked on the draft replacement. Original branches are retained.

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.

2 participants