Skip to content

fix(codex): bootstrap reasoning metadata during catalog sync - #5293

Draft
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/reasoning-metadata-sync-bootstrap
Draft

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/reasoning-metadata-sync-bootstrap

Conversation

@luvs01

@luvs01 luvs01 commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • syncModelsToCodex and the management-triggered catalog refresh now bootstrap the optional models.dev reasoning-effort snapshot before gathering the catalog, but only when a configured provider is a destination the snapshot covers (providerUsesReasoningMetadata).
  • Request-time ladder reads in src/reasoning-effort.ts no longer kick off a background refresh: a missing or corrupt snapshot is now fetched by catalog sync instead of never being fetched at all, and no models.dev request can interleave with a routed turn.
  • Structure docs updated for the sync-owns-refresh contract.

Verification

  • bun test tests/codex-integration/codex-sync-api.test.ts — 17 pass, including a new ordering test asserting the metadata refresh runs before catalog gathering for a gated provider.
  • bun x tsc --noEmit — clean.
  • bun run structure:check — green.

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

Summary by CodeRabbit

  • New Features

    • Supported provider catalog synchronization now refreshes reasoning-effort metadata before gathering models.
    • Metadata refresh occurs only when configured providers require it.
    • Request-time model handling no longer performs this network refresh, providing more predictable request behavior.
  • Documentation

    • Updated provider, configuration, runtime, operations, and synchronization documentation to explain metadata refresh timing and supported destinations.

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.

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

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

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: 71a567cf-1bce-4ca7-b218-bef16c421750

📥 Commits

Reviewing files that changed from the base of the PR and between cb8a85a and fcca85a.

📒 Files selected for processing (8)
  • src/providers/reasoning-metadata.ts
  • src/reasoning-effort.ts
  • structure/catalog.md
  • structure/config.md
  • structure/gui-and-management-api.md
  • structure/ops/docs-and-release.md
  • structure/providers/openai-tiers.md
  • structure/runtime.md

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


📝 Walkthrough

Walkthrough

The change moves reasoning metadata snapshot refresh from request-time effort resolution into catalog synchronization. Sync checks configured provider destinations, refreshes metadata when supported, and runs the refresh before catalog gathering. Documentation and integration tests describe and verify this ordering.

Changes

Reasoning metadata synchronization

Layer / File(s) Summary
Provider gate and sync dependency
src/providers/reasoning-metadata.ts:140-144, src/codex/sync.ts:17, src/codex/sync.ts:71-85
Adds providerUsesReasoningMetadata. Adds an optional injectable refreshReasoningMetadata dependency and wires the default implementation.
Refresh ordering and request-path ownership
src/codex/sync.ts:246-248, src/codex/sync.ts:354, src/reasoning-effort.ts:3, src/reasoning-effort.ts:171-174, tests/codex-integration/codex-sync-api.test.ts:167-202
Refreshes metadata before catalog gathering in both sync paths. Removes request-time snapshot refresh. Tests assert that the reasoning callback runs before the catalog callback.
Documented synchronization contract
structure/catalog.md:174-177, structure/codex-home.md:316-318, structure/config.md:477-479, structure/gui-and-management-api.md:748, structure/ops/docs-and-release.md:490-493, structure/providers/openai-tiers.md:734-737, structure/providers/xai-grok.md:131-132, structure/runtime.md:473-476, structure/subagents.md:253-255, structure/transports/inventory.md:49-52
Documents destination-gated refresh during catalog synchronization and the absence of network refresh during routed requests.

Priority: ⬇️ Low

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant syncModelsToCodex
  participant reasoningMetadata
  participant catalog
  syncModelsToCodex->>reasoningMetadata: Check configured provider destinations
  reasoningMetadata-->>syncModelsToCodex: Return metadata support status
  syncModelsToCodex->>reasoningMetadata: Refresh supported metadata snapshot
  syncModelsToCodex->>catalog: Gather catalog after refresh
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files. (6 skipped: 6… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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 primary code change: bootstrapping reasoning metadata during Codex catalog synchronization.
Full details: Docstring Coverage

Explanation

Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 4 files. (6 skipped: 6 unsupported.)

  • Fix all pre-merge checks with AI
✨ 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

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@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

@github-actions
github-actions Bot marked this pull request as draft September 20, 2026 08:19
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 64 / 80

이 PR은 models.dev reasoning-effort 스냅샷을 “요청이 올 때”가 아니라 “카탈로그 sync 할 때” 받아 오게 바꿉니다. 예전에는 스냅샷이 이미 있을 때만 백그라운드로 갱신해서, 없거나 깨진 상태면 영원히 안 받아 왔습니다. 지금은 syncModelsToCodex와 management 쪽 카탈로그 갱신이, 설정된 제공자 중 providerUsesReasoningMetadata가 참인 대상이 있을 때만 refreshReasoningMetadata를 카탈로그 모으기 전에 호출합니다. 요청 경로(src/reasoning-effort.ts)에서는 네트워크 갱신을 빼서, 라우팅 중인 턴에 models.dev 요청이 끼어들지 않게 합니다. structure 문서 여러 곳에 같은 계약을 적었고, 게이트된 Zen Go 제공자로 “reasoning → catalog” 순서를 고정하는 테스트를 하나 추가했습니다. base는 dev이고, types/config 분할이나 미리보기 배포와는 무관합니다.

라인 - GitHub mergeable=CONFLICTING / dirty — tip cb8a85adev보다 1커밋 뒤이고, 그 dev 커밋(#5268)이 src/providers/reasoning-metadata.ts, src/reasoning-effort.ts, structure/catalog.md 등 이 PR이 만진 파일을 같이 바꿨습니다. credential에 묶인 refusal·스냅샷 version 2가 들어가 있어서, 충돌 해결 없이 머지할 수 없습니다.
라인 - src/providers/reasoning-metadata.ts ensureReasoningMetadataSnapshot — 요청 경로 호출은 제거됐는데 함수·주석은 그대로입니다. 주석은 여전히 “ladder read path에서 호출”이라고 적혀 dead API가 됩니다. rebase 후 호출처가 없으면 지우거나 주석을 sync-only로 고치는 편이 맞습니다.
라인 - tests/codex-integration/codex-sync-api.test.ts — 게이트된 제공자에서 순서만 봅니다. 게이트 없는 config면 refresh를 안 부르는 경우, refreshCatalogForSync(catalog-only) 경로에서도 같은 bootstrap이 도는지는 없습니다.
라인 - src/codex/sync.ts refreshReasoningMetadataForSync — refresh 결과(ok: false)를 버리고 sync는 계속합니다. 의도일 수 있으나, 실패해도 카탈로그는 갱신되고 스냅샷만 비는 상태가 됩니다.
라인 - 동작 변화 — 오래 떠 있는 프로세스에서 TTL이 지난 스냅샷을 요청이 더 이상 백그라운드로 안 고칩니다. sync/ocx sync/management 카탈로그 갱신 때만 고쳐집니다. 신선 설치 bootstrap 구멍은 막히고, “가끔 sync 안 하는 장기 서버의 오래된 사다리”는 sync 주기에 묶입니다.
라인 - 체크리스트 — draft, readiness 0/4. enforce-target·hygiene·label·CodeRabbit은 통과로 보이고, resolve-pr은 이 시점 pending이었습니다.

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

#5268이 이미 reasoning-metadata·effort를 credential 범위로 바꾼 뒤라, 이 PR을 dev에 rebase할 때 snapshot/refusal 계약을 어떻게 합칠지. TTL 만료 갱신을 sync-only로 둘지, sync 말고도 주기적/관리 API 한 곳을 남길지. refresh 실패를 sync warning으로 올릴지 조용히 둘지.

너의 추천

방향은 맞고, “없는 스냅샷을 요청 경로에서 안 받고 sync가 받는다”는 설명이 코드·테스트·문서와 맞습니다. 지금 머지하지 마세요. 먼저 dev(#5268) 위로 rebase해서 reasoning-metadata 충돌을 풀고, ensureReasoningMetadataSnapshot dead 경로를 정리한 뒤, 게이트 스킵·catalog-only 중 하나라도 테스트에 넣고 체크리스트를 채우세요. types/config 분할 중복으로 닫을 대상은 아닙니다.

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

@github-actions
github-actions Bot marked this pull request as ready for review September 20, 2026 10:51
@luvs01
luvs01 force-pushed the fix/reasoning-metadata-sync-bootstrap branch from cb8a85a to fcca85a Compare September 21, 2026 03:23
@github-actions
github-actions Bot marked this pull request as draft September 21, 2026 03:24
The models.dev effort snapshot was only ever refreshed after a snapshot had already answered a request-time ladder read, so a missing or corrupt snapshot was never bootstrapped. Catalog sync now refreshes the snapshot for configured gated destinations before gathering the catalog, keeping the models.dev fetch off the request path.
@luvs01
luvs01 force-pushed the fix/reasoning-metadata-sync-bootstrap branch from fcca85a to a58bee8 Compare September 21, 2026 17:16
@luvs01

luvs01 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Checklist note: codex-sync-api.test.ts shows two spawn-timeout failures (child.status === null) on this head, but the identical failure reproduces on a clean origin/dev worktree (e4ceeb3). The failures are environment-sensitive spawn timeouts unrelated to this change; the rebased head is ticked on that basis.

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