Skip to content

fix(omp): follow up source-preservation hardening for #1365 - #1392

Merged
Wibias merged 5 commits into
devfrom
maint/pr-1365-coderabbit-inline-comment
Aug 10, 2026
Merged

fix(omp): follow up source-preservation hardening for #1365#1392
Wibias merged 5 commits into
devfrom
maint/pr-1365-coderabbit-inline-comment

Conversation

@Wibias

@Wibias Wibias commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up / maintainer superset for #1365.

This branch is based directly on #1365 head a328f438eca6810f068e4c30f47e3d5547b180fc and carries the full #1365 change set plus the remaining validated CodeRabbit source-preservation fix.

Additional fix over #1365

CodeRabbit correctly found that nested inline comments inside the managed providers.opencodex mapping were not rejected. YAML parsing drops those comments from the semantic fingerprint, so a later stale refresh or disable could replace/remove the managed source range and silently delete user-owned inline comment text.

The follow-up fix:

  • makes childEnd() fail closed when a deeper managed line contains an inline comment
  • preserves existing handling for blank lines, standalone comments, and non-comment managed fields
  • adds focused regressions for both refresh/upsert and disable/remove

Additional commits over #1365:

  • 57cc6915b886b1310433c6fe070ae58aa58c9d36 — reject nested inline comments before managed-range replacement/removal
  • a4ab7b97856f21bfd403b86f7dda59f79bf94dc6 — add refresh and disable regressions

Relationship to #1365

#1365 has maintainer_can_modify: false, so the validated fix could not be pushed to the contributor branch. This PR is therefore intentionally a superset, not an independent feature PR.

Review notes

A full review of the #1365 changes found no additional code-level blocker beyond the inline-comment preservation hole. The OMP model-level openai-responses override and canonical thinking.efforts / defaultLevel shape match current upstream OMP schema, routed models remain on Chat Completions, and the structured translator-budget 413 path is bounded and covered.

Validation / gates

  • Rebased onto current dev 83dbe5de3bca9f1011a48ce7899a6c2501ccdde2.
  • Verified 5 ahead, 0 behind with the merge base exactly matching current dev.
  • React Doctor completed successfully on exact head d6e9ecd0c3917012814d90801162756278498068.
  • All 16 Cross-platform CI check runs on the exact head completed with no failed, cancelled, queued, or in-progress check run.
  • No unresolved review threads remain on fix(omp): follow up source-preservation hardening for #1365 #1392.

Summary by CodeRabbit

  • New Features

    • Added Oh My Pi configuration support for native OpenAI Responses models, reasoning-effort settings, context limits, and input modalities.
    • Preserved formatting and comments when applying, refreshing, or disabling managed Oh My Pi YAML configuration.
    • Added safer handling for ambiguous or unsupported configuration layouts.
  • Bug Fixes

    • Large chat-completions requests now return structured errors instead of unhandled failures, including clear payload-size details.
    • Improved configuration conflict detection to protect user edits and managed settings.
  • Documentation

    • Clarified API behavior and YAML formatting guidance for native and routed models.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds OMP model capability export, native OpenAI Responses selection, source-preserving OMP YAML mutations, fragment-scoped integration drift checks, and structured chat-completions request-size error responses.

Changes

OMP export and model capabilities

Layer / File(s) Summary
Model capability metadata
src/clients/config-export.ts, src/codex/catalog.ts, src/server/management/model-rows.ts
ExportModel and native management rows now include reasoning efforts, default reasoning effort, and input modalities. OMP reasoning values are normalized against supported levels.
OMP configuration serialization
src/clients/config-export.ts, tests/client-config-export.test.ts, docs-site/src/content/docs/guides/integrations.md
OMP uses openai-responses for native OpenAI models and retains openai-completions for routed models. Valid reasoning metadata and modality settings are exported and documented.

OMP source-preserving integration lifecycle

Layer / File(s) Summary
Managed YAML source patching
src/integrations/omp-yaml-source.ts, src/integrations/writer.ts, tests/integrations-writer.test.ts, tests/omp-yaml-source-inline-comments.test.ts, docs-site/src/content/docs/guides/integrations.md
Apply and disable operations patch only the providers.opencodex block. The patcher preserves unrelated formatting and comments, removes empty containers when safe, and refuses ambiguous structures without writing.
Fragment-scoped ownership validation
src/integrations/ownership.ts, src/integrations/state.ts, tests/integrations-state.test.ts, tests/management-integration-routes.test.ts
OMP checks recorded managed fragments instead of whole-file fingerprints. Tests cover catalog drift, owned-fragment edits, unrelated additions, and conflict preservation.

Chat-completions request limits

Layer / File(s) Summary
Guarded request serialization
src/server/chat-completions.ts, tests/chat-completions-endpoint.test.ts
Serialization and replay-budget failures return structured 413 or 500 responses. Tests cover successful 25 MiB image requests and rejected 33 MiB requests.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ManagementRows
  participant OMPConfigBuilder
  participant IntegrationWriter
  participant OMPYamlFile
  ManagementRows->>OMPConfigBuilder: provide model modalities and reasoning metadata
  OMPConfigBuilder->>IntegrationWriter: build managed OMP provider fragment
  IntegrationWriter->>OMPYamlFile: apply source-preserving upsert or removal
  OMPYamlFile-->>IntegrationWriter: patched source or unsafe refusal
Loading

Possibly related PRs

Suggested reviewers: ingwannu, lidge-jun, n3wr1ch

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. 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 identifies the OMP source-preservation hardening addressed by the pull request and references the related issue.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch maint/pr-1365-coderabbit-inline-comment

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 Aug 10, 2026
@Wibias
Wibias marked this pull request as ready for review August 10, 2026 02:40
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@Wibias
Wibias force-pushed the maint/pr-1365-coderabbit-inline-comment branch from a4ab7b9 to d6e9ecd Compare August 10, 2026 02:48
@Wibias
Wibias merged commit 14c01e5 into dev Aug 10, 2026
23 of 24 checks passed
@Wibias
Wibias deleted the maint/pr-1365-coderabbit-inline-comment branch August 10, 2026 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants