fix(omp): preserve rich native model capabilities - #1365
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds OMP-specific model export with native OpenAI Responses support, preserves unrelated YAML source during OMP updates, scopes conflict detection to owned fragments, and adds guarded replay serialization with structured size errors. ChangesOMP configuration export
Owned-fragment integration state
Chat Completions replay serialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModelCatalog
participant NativeModelRows
participant OmpConfigBuilder
participant OmpGeneratedConfig
ModelCatalog->>NativeModelRows: provide modalities and reasoning metadata
NativeModelRows->>OmpConfigBuilder: provide exportable model rows
OmpConfigBuilder->>OmpGeneratedConfig: select API dialect and serialize models
OmpGeneratedConfig-->>OmpConfigBuilder: return generated OMP configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Requesting changes for one blocking correctness/safety issue in the integration ownership change.
High — unrelated user-owned source text can now be silently destroyed
classifyIntegration() now intentionally ignores changes outside the recorded OpenCodex fragments, so a config can remain current when a user adds or edits an unrelated sibling such as another provider. That part is reasonable at the parsed-value level.
However, refresh/disable still parse the entire client config, remove/replace only the managed fragment in the parsed document, and then serialize the entire document back to disk. The YAML/TOML serializers are hand-renderers, and JSON/JSON5 are stringified, so comments and original formatting outside the managed fragment are not source-preserved.
Concrete example: after OpenCodex applies its provider block, the user adds an unrelated provider plus a YAML comment. With this PR the managed fragment still matches, so disable/refresh is allowed; the whole file is then re-rendered and the unrelated provider value survives, but the user's comment/formatting is silently lost.
That violates the writer's fail-closed safety contract: unrelated work performed after OpenCodex wrote the file must not be silently discarded merely because it sits outside the managed fragment.
Please either:
- mutate only the owned subtree using a source-preserving mechanism, or
- retain the whole-file conflict guard for formats where unrelated source text cannot be preserved.
Please also add an apply/refresh/disable regression that starts with an unrelated sibling plus comments/source formatting and proves those bytes survive the operation.
The OMP capability work itself looks sound: native OpenAI models use the model-level Responses API override, routed models remain on Chat Completions, image modalities are preserved, and the reasoning metadata matches OMP's accepted schema. The guarded Chat Completions replay-buffer 413 path also looks reasonable.
Merge gates after the blocker is fixed
- rebase onto current
dev(this head is behind) - run exact-head Cross-platform CI / React Doctor successfully
- complete the readiness checklist / leave draft when ready
Please address the source-preservation hole before merge.
85142f2 to
266e781
Compare
|
Addressed the blocking source-preservation finding in the latest-dev rebased head
The PR remains draft while the new-head CI runs. |
266e781 to
a328f43
Compare
Rate Limit Exceeded
|
Rate Limit Exceeded
|
Rate Limit Exceeded
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ne-comment fix(omp): follow up source-preservation hardening for #1365
|
Thanks @dbc-hbin for the work here. This is genuinely useful: OMP now preserves the richer native OpenAI model capabilities through the Responses API, including image input and reasoning-effort metadata, while routed providers keep their existing Chat Completions path. The source-preserving YAML work also avoids clobbering unrelated user comments and formatting, which makes the integration much safer to use on real shared config files. I'm closing this PR as superseded by #1392. #1392 contains the complete #1365 change set plus the follow-up source-preservation fix for nested inline comments inside Thanks again for the contribution and for pushing the OMP integration forward. 🙏 |
2 participants
Summary
providers.opencodex; unsupported or ambiguous YAML now fails closed.Verification
bun run typecheckbun run testbun run privacy:scanbun run buildindocs-site/ocx restartwas attempted for the new head and refused safely because the running proxy predates process-bound restart support; no stop/kill fallback was used.Checklist
Review readiness checklist
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