refactor(codex): split inject and catalog sync behind facades - #4643
Conversation
Pure move. inject.ts 2342 -> 987 with five leaves, catalog/sync.ts 2698 -> 52 with seven leaves. INV-TOML-01 moves to inject/config-toml.ts and INV-AGENT-01 to catalog/subagent-roster.ts. Three source oracles that read these files as text are repointed in the same commit.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
added
the
chore
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c63e9ea676
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,539 @@ | |||
| import { loadConfig, shouldSyncCodexOnStart } from "../../config"; | |||
There was a problem hiding this comment.
When any caller imports src/codex/inject.ts, its new re-export instantiates this module, but src/config.ts does not export shouldSyncCodexOnStart; that function is defined in src/codex/desired-state.ts. Bun/TypeScript therefore rejects the module before CLI startup or native restoration can run. Import loadConfig from ../../config and shouldSyncCodexOnStart from ../desired-state.
AGENTS.md reference: AGENTS.md:L373-L375
Useful? React with 👍 / 👎.
| OCX_SECTION_MARKER, | ||
| REALTIME_WS_BASE_URL_KEY, | ||
| hasInjectedOpenaiBaseUrl, | ||
| readRootTomlString, |
There was a problem hiding this comment.
The inject.ts facade re-exports this module, but injected-marker.ts has no readRootTomlString export; the helper remains exported by src/codex/paths.ts. Consequently, importing the facade fails during ESM linking even for callers that do not invoke removal. Move this named import to ../paths.
AGENTS.md reference: AGENTS.md:L373-L375
Useful? React with 👍 / 👎.
| } from "./parsing"; | ||
| import type { CatalogModel, RawEntry } from "./parsing"; | ||
| import { | ||
| applyNativeOpenAiContextOverride, |
There was a problem hiding this comment.
Loading src/codex/catalog/sync.ts instantiates this extracted module, but metadata.ts does not export applyNativeOpenAiContextOverride; the function is still exported from catalog/parsing.ts. This invalid named import makes catalog sync fail at module linking before any catalog can be built, so source the helper from ./parsing.
AGENTS.md reference: AGENTS.md:L373-L375
Useful? React with 👍 / 👎.
| CODEX_NATIVE_ALIAS_CATALOG_KIND, | ||
| NATIVE_OPENAI_MODELS, | ||
| SUPPORTED_NATIVE_OPENAI_SLUGS, | ||
| applyNativeOpenAiContextOverride, |
There was a problem hiding this comment.
This second extracted catalog module also imports applyNativeOpenAiContextOverride from metadata.ts, which has no such export; its definition remains in parsing.ts. Even after correcting derive-entry.ts, the sync.ts facade will still fail to instantiate because it re-exports build-entries.ts, so move this import to ./parsing as well.
AGENTS.md reference: AGENTS.md:L373-L375
Useful? React with 👍 / 👎.
| import { loadConfig, websocketsEnabled } from "../../config"; | ||
| import { shouldSyncCodexOnStart } from "../desired-state"; | ||
| import { legacyCustomModelCatalogSlugs } from "../custom-model-catalog-migration"; | ||
| import { activeCodexModelsCachePath, getCodexHome, readCodexCatalogPath, readCodexCatalogPathForHome } from "../paths"; |
There was a problem hiding this comment.
For every retained catalog sync, this module requests activeCodexModelsCachePath, readCodexCatalogPath, and readCodexCatalogPathForHome from src/codex/paths.ts, but all three are exported by catalog/parsing.ts instead. The missing named exports prevent the catalog facade from loading; keep only getCodexHome on this import and source the three catalog helpers from ./parsing.
AGENTS.md reference: AGENTS.md:L373-L375
Useful? React with 👍 / 👎.
| observedReserveCatalogSource, | ||
| shouldIncludeAccountBoundNativeOpenAi, | ||
| shouldIncludeNativeOpenAi, | ||
| trustedAccountBoundNativeCatalogSlug, |
There was a problem hiding this comment.
metadata.ts imports trustedAccountBoundNativeCatalogSlug for its own use but does not re-export it; the public definition remains in account-models.ts. Thus this named import independently prevents retained-sync.ts and the catalog/sync.ts facade from linking. Add the helper to the existing ./account-models import instead.
AGENTS.md reference: AGENTS.md:L373-L375
Useful? React with 👍 / 👎.
| import { slugsEquivalent } from "../../providers/slug-codec"; | ||
| import { readCatalog, readCodexCatalogPath } from "./parsing"; | ||
| import type { RawEntry } from "./parsing"; | ||
| import { SUPPORTED_NATIVE_OPENAI_SLUGS, trustedAccountBoundNativeCatalogSlug } from "./metadata"; |
There was a problem hiding this comment.
The roster extraction requests trustedAccountBoundNativeCatalogSlug from metadata.ts, but that module does not export it; it is exported by account-models.ts. Because catalog/sync.ts re-exports this roster module, the invalid import breaks all consumers of the catalog facade, not only callers of effectiveSubagentRoster. Source the helper from ./account-models.
AGENTS.md reference: AGENTS.md:L373-L375
Useful? React with 👍 / 👎.
리뷰 · 우선순위 50 / 80이 PR은 제품 기능을 바꾸려는 것이 아니라, godfile round2 사이클 3에서 이미 약속한 대로 지금 나눈 모양은 030 계획과 맞다. inject 쪽은 불변식도 계획대로 옮겼다. INV-TOML-01 헤더는 형제 #4642와 비교하면 이 PR은 hygiene가 이미 통과했다(src+테스트 오라클이 한 커밋에 들어갔다). 다만 Cross-platform CI의 test/gates/keyring 등은 아직 pending이고 mergeStateStatus는 UNSTABLE이다. 작성자도 로컬 typecheck·full suite·install은 돌리지 않았고 hosted CI를 증거로 둔다고 했다. types.ts/config.ts 분할 캠페인과는 겹치지 않는다(여기는 inject/catalog sync 분해). close-don't-rebase 대상도 아니다. 다만 파일 크기 래칫 기준선( 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Pure move. routing.ts 3507 -> 1475 with six leaves, quota.ts 3313 -> 558 with five leaves. Dispatchers that would close a cycle stay on the facade. Retry budget scope is unchanged; no new attempt counter exists in any leaf.
The inject and catalog leaves imported six symbols from modules that never exported them. Each one is re-pointed at where it is actually defined: parsing, account-models, subagent-roster, paths, desired-state. Import paths only; no declaration moved.
…ta split Fifteen symbols lost their binding: leaves that defined a symbol never exported it, quota type imports pointed at src/types instead of providers/quota-types, and isModelDetourAffinityScope lost its definition entirely while its call site survived. Imports and exports only; no declaration was moved or rewritten.
lidge-jun
added
the
test-exception-approved
The leaf sits one directory deeper than routing.ts, so ../config resolved to src/codex/config, which does not exist. Every test shard that loaded the routing graph failed at import time.
refactor(codex,providers): split routing and quota behind facades
github-actions
Bot
removed
the
test-exception-approved
1 participant
Summary
src/codex/inject.ts와src/codex/catalog/sync.ts를 파사드로 남기고 리프로 나눈다. 순수 이동이며 로직·식별자·문자열을 바꾸지 않았다. 두 파일의 공개 export 표면은 이동 전과 같다.src/codex/inject.tsinject/routing-target.ts125,inject/routing-classify.ts109,inject/remove.ts194,inject/config-toml.ts563,inject/restore.ts538src/codex/catalog/sync.tscatalog/subagent-roster.ts175,catalog/derive-entry.ts229,catalog/gated-native-warn.ts63,catalog/restore.ts132,catalog/auto-review.ts506,catalog/retained-sync.ts703,catalog/build-entries.ts981sync.ts가 52줄 순수 파사드가 된 것은 계획보다 더 나눈 결과다.retained-sync가 build/merge 심볼 다섯을 쓰는데 파사드가retained-sync를 재수출하므로 build/merge를sync.ts에 남기면 순환이 확정된다. 그래서build-entries.ts를 함께 뽑았다.불변식 승계를 명시한다. INV-TOML-01은 루트키 선삽입 로직을 가져간
inject/config-toml.ts가, INV-AGENT-01은catalog/subagent-roster.ts가 승계하고 각 파일 헤더에 id 주석을 옮겼다.본문을 텍스트로 읽는 오라클 세 곳을 같이 고쳤다. 인라인
syncCodexHistoryProvider호출이 파사드에서inject/restore.ts로 옮겨갔기 때문에codex-history-reachability.test.ts의INLINE_ALLOWED를 바꾸지 않으면 그 테스트는 반드시 실패한다.codex-retained-root-serialization.test.ts의 슬라이스 대상도restore.ts로 옮겼고,codex-inject-history-wording.test.ts는 리터럴 여섯이 파사드와restore.ts로 갈라져 두 파일을 이어 읽게 했다.catalog/effort.ts의deriveEntryimport는 호출 사이트가 없는 죽은 줄이었고, 남겨두면effort → sync(파사드) → derive-entry순환이 되므로 지웠다.Verification
bun scripts/structure-ssot.ts→structure/ SSOT checks passed(subagents.md,catalog.md의 백틱 경로를 새 소유 모듈로 교체한 뒤)bun scripts/file-size-ratchet.ts→file-size ratchet passedBun.Transpiler구문 검사 통과origin/dev기준):inject.ts35 → 35,catalog/sync.ts31 → 31, 누락 0Checklist