feat(integrations): add OMP client support - #1109
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds OMP as a supported file integration and export client. It adds profile-aware paths, YAML serialization, loopback credentials, GUI routing, localized labels, API coverage, tests, and documentation updates. ChangesOMP integration
Estimated code review effort: 3 (Moderate) | ~25 minutes 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. |
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. This PR stays in draft until every box above is ticked. |
ca9183d to
6860b6c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs-site/src/content/docs/ja/reference/cli/agents.md (1)
136-148: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the localized CLI output contract.
Both pages list seven clients in the command heading but still restrict the
--clientflag toopencode|pi. Both pages also state that--jsonproduces JSON, although OMP, Hermes, and Gajae produce YAML, OpenClaw produces JSON5, and Kimi produces TOML.
docs-site/src/content/docs/ja/reference/cli/agents.md#L136-L148: List all seven client IDs. Replace JSON-only output wording with generated configuration bytes.docs-site/src/content/docs/ko/reference/cli/agents.md#L142-L154: List all seven client IDs. Replace JSON-only output wording with generated configuration bytes.As per path instructions, translated locale pages must not contradict the English source.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-site/src/content/docs/ja/reference/cli/agents.md` around lines 136 - 148, Update the CLI documentation in docs-site/src/content/docs/ja/reference/cli/agents.md lines 136-148 and docs-site/src/content/docs/ko/reference/cli/agents.md lines 142-154: change the --client option to list all seven supported client IDs, and revise --json wording to describe generated configuration bytes rather than JSON-only output, matching the English source and accounting for each client’s format.Source: Path instructions
docs-site/src/content/docs/ru/reference/cli/agents.md (1)
155-193: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe localized CLI references retain the same client-list and output-format drift.
Both pages add OMP to the heading and examples but leave the client option and destination tables incomplete. Both also describe
--jsonas JSON-only even though OMP emits YAML.
docs-site/src/content/docs/ru/reference/cli/agents.md#L155-L193: Update Line 167, add the four omitted client rows, and describe--jsonas native-format output.docs-site/src/content/docs/zh-cn/reference/cli/agents.md#L129-L155: Update Line 137, add the four omitted client rows, and describe--jsonas native-format output.As per path instructions, localized documentation must stay consistent with actual CLI and API behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-site/src/content/docs/ru/reference/cli/agents.md` around lines 155 - 193, Update docs-site/src/content/docs/ru/reference/cli/agents.md lines 155-193 and docs-site/src/content/docs/zh-cn/reference/cli/agents.md lines 129-155 consistently: revise the client option description to include all supported clients, add the four missing client rows to each destination table, and describe --json as native-format output so OMP’s YAML is covered rather than claiming JSON-only output.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/integrations.md`:
- Around line 19-22: Update docs-site/src/content/docs/guides/integrations.md
lines 19-22 to include PI_PROFILE and document ompAgentDir() precedence:
OMP_PROFILE overrides PI_PROFILE; with an active profile, PI_CODING_AGENT_DIR is
ignored and PI_CONFIG_DIR supplies the profile root; without a profile,
PI_CODING_AGENT_DIR takes precedence. Apply the same precedence rule in
docs-site/src/content/docs/reference/cli/agents.md line 180, replacing the flat
variable list.
In `@tests/management-client-config-route.test.ts`:
- Around line 162-176: Extend the OMP test around clientConfigApi and
PiGeneratedConfig to assert the complete routed model catalog: a/m1, a/m2, and
b/no-context. Also verify the exported models preserve their capability and
reasoning metadata, including the expected modelContextWindows values, while
retaining the existing provider and API key assertions.
- Around line 162-176: Update the OMP test around clientConfigApi and the
body.config assertions to verify that neither body.text nor the serialized
body.config contains REAL_LOOKING_KEY. Keep the existing provider model and
placeholder API-key assertions unchanged.
---
Outside diff comments:
In `@docs-site/src/content/docs/ja/reference/cli/agents.md`:
- Around line 136-148: Update the CLI documentation in
docs-site/src/content/docs/ja/reference/cli/agents.md lines 136-148 and
docs-site/src/content/docs/ko/reference/cli/agents.md lines 142-154: change the
--client option to list all seven supported client IDs, and revise --json
wording to describe generated configuration bytes rather than JSON-only output,
matching the English source and accounting for each client’s format.
In `@docs-site/src/content/docs/ru/reference/cli/agents.md`:
- Around line 155-193: Update
docs-site/src/content/docs/ru/reference/cli/agents.md lines 155-193 and
docs-site/src/content/docs/zh-cn/reference/cli/agents.md lines 129-155
consistently: revise the client option description to include all supported
clients, add the four missing client rows to each destination table, and
describe --json as native-format output so OMP’s YAML is covered rather than
claiming JSON-only output.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cfc8af1a-b446-49d9-b51a-426e76c08cf9
⛔ Files ignored due to path filters (1)
devlog/_plan/260806_omp_integration/evidence/omp-dashboard.pngis excluded by!**/*.png
📒 Files selected for processing (35)
docs-site/src/content/docs/guides/integrations.mddocs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/reference/cli/agents.mddocs-site/src/content/docs/ko/reference/management-api.mddocs-site/src/content/docs/reference/cli/agents.mddocs-site/src/content/docs/reference/management-api.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdgui/src/app-routing.tsgui/src/components/apikeys-workspace/client-config-clients.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Integrations.tsxgui/src/pages/integrations/FileIntegrationPage.tsxgui/src/pages/integrations/integration-api.tsgui/src/pages/integrations/overview-clients.tsgui/tests/client-config-panel.test.tsxgui/tests/integrations-overview-rows.test.tssrc/cli/export-command.tssrc/cli/help.tssrc/clients/config-export.tssrc/integrations/registry.tstests/cli-export-command.test.tstests/client-config-export-new-clients.test.tstests/client-config-export.test.tstests/integrations-invariants.test.tstests/integrations-state.test.tstests/management-client-config-route.test.ts
![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: 6860b6c4ac
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/ja/reference/cli/agents.md`:
- Line 154: Synchronize the OMP destination metadata rows in
docs-site/src/content/docs/ja/reference/cli/agents.md:154-154,
docs-site/src/content/docs/ko/reference/cli/agents.md:160-160,
docs-site/src/content/docs/ru/reference/cli/agents.md:187-187, and
docs-site/src/content/docs/zh-cn/reference/cli/agents.md:155-155: mark
~/.omp/agent/models.yml as the default path, document OMP_PROFILE/PI_PROFILE
precedence, and replace the generic placeholder with the literal
opencodex-loopback value in each localized table.
- Around line 141-144: Update the localized export-output descriptions to
reflect that non-JSON output is generated client configuration or the selected
native format, rather than always JSON. In
docs-site/src/content/docs/ja/reference/cli/agents.md lines 141-144, revise the
adjacent Line 148 wording; apply the equivalent correction in
docs-site/src/content/docs/ko/reference/cli/agents.md lines 147-150 for Line
154, docs-site/src/content/docs/ru/reference/cli/agents.md lines 172-175 for
Lines 179-181, and docs-site/src/content/docs/zh-cn/reference/cli/agents.md
lines 142-145 for Line 149.
In `@tests/client-config-export.test.ts`:
- Around line 479-488: Make the test around EXPORT_CLIENTS.omp.destination
deterministic by isolating homedir() or mocking the filesystem check used by
ompModelsConfigPath, so the real test account cannot affect the selected
filename. Preserve separate assertions covering both canonical models.yml and
legacy models.yaml resolution.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cc38f932-0454-4ec6-a12e-105f1d85597c
📒 Files selected for processing (10)
docs-site/src/content/docs/guides/integrations.mddocs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ko/reference/cli/agents.mddocs-site/src/content/docs/reference/cli/agents.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.mdsrc/cli/export-command.tssrc/cli/help.tssrc/clients/config-export.tstests/client-config-export.test.ts
e35bc9b to
1370270
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1370270476
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-site/src/content/docs/guides/integrations.md (1)
83-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not state that OMP has no header slot.
OMP supports provider-level headers. This integration still does not emit remote
x-opencodex-api-keywiring, so it remains loopback-only. Split OMP from clients whose schemas have no header field, and document the deferred remote-header limitation instead.As per path instructions, OMP documentation must distinguish provider-adapter header behavior from client export and preserve the loopback-only limitation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs-site/src/content/docs/guides/integrations.md` around lines 83 - 88, The integration guidance should distinguish OMP from Pi, Kimi Code, and Gajae Code: do not claim OMP lacks a header slot. Update the OMP-specific wording to acknowledge provider-level headers while stating that this integration does not emit remote x-opencodex-api-key wiring, so OMP remains loopback-only; keep the no-header-schema explanation only for the other clients.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/ja/reference/cli/agents.md`:
- Around line 128-130: Update the localized CLI introductions to describe the
actual native serialization contract: the selected client’s native format
contains a credential reference or the opencodex-loopback placeholder, as
applicable, rather than always an environment reference. Apply this wording to
docs-site/src/content/docs/ja/reference/cli/agents.md lines 128-130,
docs-site/src/content/docs/ko/reference/cli/agents.md lines 134-136, and
docs-site/src/content/docs/zh-cn/reference/cli/agents.md lines 129-131; update
docs-site/src/content/docs/ru/reference/cli/agents.md line 155 to mention native
format and loopback placeholders.
In `@src/clients/config-export.ts`:
- Around line 1006-1019: Update the filename value in EXPORT_CLIENTS.omp to
"models.yml" so downloaded OMP configurations match the canonical path selected
by ompModelsConfigPath. Keep the existing destination and loopback-only behavior
unchanged.
---
Outside diff comments:
In `@docs-site/src/content/docs/guides/integrations.md`:
- Around line 83-88: The integration guidance should distinguish OMP from Pi,
Kimi Code, and Gajae Code: do not claim OMP lacks a header slot. Update the
OMP-specific wording to acknowledge provider-level headers while stating that
this integration does not emit remote x-opencodex-api-key wiring, so OMP remains
loopback-only; keep the no-header-schema explanation only for the other clients.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b424375-3b85-44de-9851-84c63c7452d4
📒 Files selected for processing (16)
docs-site/src/content/docs/guides/integrations.mddocs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ko/reference/cli/agents.mddocs-site/src/content/docs/reference/cli/agents.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.mdgui/src/app-routing.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tssrc/clients/config-export.tstests/client-config-export.test.tstests/management-client-config-route.test.ts
1370270 to
9d67776
Compare
|
Is this ready? If yes I will please need some screenshots out of omp which shows that it is working. @n3wr1ch |
9d67776 to
f203792
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
f203792 to
195c885
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed current head 195c8853 against dev (243c3f4), including the full PR diff, OMP's current upstream path/model contracts, integration writer/rollback behaviour, credential handling, existing review threads, maintainer feedback, CI state, and the added live OMP evidence.
Blocking findings
P2 — PI_CONFIG_DIR resolution does not match OMP, so opencodex can manage the wrong file.
ompAgentDir() special-cases absolute and ~-prefixed PI_CONFIG_DIR values and treats them as absolute/expanded paths. OMP itself does not do that: upstream getConfigDirName() returns PI_CONFIG_DIR as the config-directory name and getBaseConfigRoot() always resolves it with path.join(os.homedir(), getConfigDirName()); OMP documents this value as relative to home. The PR therefore gives some environment values different filesystem meaning from the client it claims to follow. For example, an absolute or ~-prefixed value can make opencodex write/manage a different models.yml from the one OMP resolves. The new test currently locks this divergence in by using an absolute PI_CONFIG_DIR and expecting it to stay absolute.
Please mirror OMP's actual resolution semantics exactly, or explicitly reject forms opencodex will not mirror. Do not silently assign the same env value a different target path.
Relevant code: src/clients/config-export.ts (ompAgentDir) and tests/client-config-export.test.ts.
P2 — the non-loopback refusal message is factually wrong for OMP.
The shared applyIntegration() refusal says ${clientId} has nowhere to put the admission header ... and writing one by hand would not help either. That remains true for the genuinely headerless clients, but not for OMP. OMP supports provider-level headers; this PR intentionally defers managed remote x-opencodex-api-key wiring to keep the first integration loopback-only. The user-facing docs now describe that scope correctly, while the runtime error still tells OMP users that manual remote configuration is impossible.
Please make this message describe the generated integration policy, or make it client-specific. It must not claim OMP lacks a header capability it actually has.
Relevant code: src/integrations/writer.ts (applyIntegration).
CI / review gate
At the current head, both Cross-platform CI and React Doctor completed as action_required without running jobs. CodeRabbit is also processing the new head. Local validation and the live OMP screenshots are useful evidence, but they do not replace an executed green GitHub CI run. Please get the current-head checks to actually run and pass before merge.
The PR is also marked ready while the body still has My PR is ready for review unchecked. That is only hygiene, but it should be made consistent.
Security review
I did not find a new secret leak, auth bypass, path-traversal primitive, or unsafe overwrite path in the OMP changes. The generated OMP document uses the non-secret opencodex-loopback placeholder; the management regression checks that the real-looking upstream credential is absent from both text and structured config; integration writes retain the existing ownership/fingerprint/conflict protections. Existing-file snapshots can contain the user's own credentials, but they use the existing hardened 0600/Windows-ACL snapshot path and bounded retention design.
The deliberate loopback-only OMP scope is acceptable for this PR. The problem is the incorrect path contract and the misleading runtime explanation, not the decision to defer remote credential wiring.
Usefulness
The feature itself is worthwhile: OMP is wired through export, management API, managed integration state, GUI, localisation, docs, and focused tests, and the supplied OMP 17.2.11 catalog/prompt evidence materially improves confidence. After the two P2 issues are corrected and current-head CI is green, I would expect this to be in approvable shape.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/integrations.md`:
- Line 13: Update the OMP row in the integrations table to name the literal
loopback placeholder as opencodex-loopback instead of the generic “loopback
placeholder” description. Keep this value clearly separate from upstream
provider credentials.
In `@docs-site/src/content/docs/ja/reference/cli/agents.md`:
- Line 166: Update the remote credential guidance in
docs-site/src/content/docs/ja/reference/cli/agents.md:166-166,
docs-site/src/content/docs/ko/reference/cli/agents.md:172-172,
docs-site/src/content/docs/ru/reference/cli/agents.md:206-209, and
docs-site/src/content/docs/zh-cn/reference/cli/agents.md:167-167 to list
OPENCODEX_HERMES_API_KEY and OPENCODEX_OPENCLAW_API_KEY alongside the existing
opencode variable for non-loopback proxies. Preserve the loopback-only guidance
for OMP, Pi, Kimi, and Gajae, matching the export contract in config-export.ts.
In `@tests/client-config-export.test.ts`:
- Around line 468-497: The test covering ompModelsConfigPath and
EXPORT_CLIENTS.omp.destination must also assert that PI_PROFILE is used when
OMP_PROFILE is undefined, producing the expected profile path. Add assertions
that unsafe profile values such as ".." and Windows-reserved names throw
ClientPathError, using ompModelsConfigPath directly and preserving the existing
explicit OMP_PROFILE precedence cases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4253cc9e-d251-4303-9107-ccd5a1300893
⛔ Files ignored due to path filters (3)
devlog/_plan/260806_omp_integration/evidence/omp-dashboard.pngis excluded by!**/*.pngdevlog/_plan/260806_omp_integration/evidence/omp-live-prompt.pngis excluded by!**/*.pngdevlog/_plan/260806_omp_integration/evidence/omp-model-catalog.pngis excluded by!**/*.png
📒 Files selected for processing (35)
docs-site/src/content/docs/guides/integrations.mddocs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ja/reference/management-api.mddocs-site/src/content/docs/ko/reference/cli/agents.mddocs-site/src/content/docs/ko/reference/management-api.mddocs-site/src/content/docs/reference/cli/agents.mddocs-site/src/content/docs/reference/management-api.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/ru/reference/management-api.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/reference/management-api.mdgui/src/app-routing.tsgui/src/components/apikeys-workspace/client-config-clients.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Integrations.tsxgui/src/pages/integrations/FileIntegrationPage.tsxgui/src/pages/integrations/integration-api.tsgui/src/pages/integrations/overview-clients.tsgui/tests/client-config-panel.test.tsxgui/tests/integrations-overview-rows.test.tssrc/cli/export-command.tssrc/cli/help.tssrc/clients/config-export.tssrc/integrations/registry.tstests/cli-export-command.test.tstests/client-config-export-new-clients.test.tstests/client-config-export.test.tstests/integrations-invariants.test.tstests/integrations-state.test.tstests/management-client-config-route.test.ts
|
Addressed the current-head review blockers in
Final-head focused suites: 123 pass, typecheck and privacy scan pass, docs build passes. The fork workflow run is currently |
|
Final-head full suite is now also green: 10,101 pass, 7 skip, 0 fail. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/client-config-export.test.ts`:
- Around line 481-503: Extend the OMP path contract tests around
ompModelsConfigPath to cover PI_CONFIG_DIR values beginning with "/" and "~",
asserting they resolve via path.join(home, value) using the temporary home. Add
a profile-active case that sets PI_CODING_AGENT_DIR and verifies it is ignored,
with the profile-based models path unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0991914b-903d-4e9d-9626-9862f8ef66c1
📒 Files selected for processing (11)
docs-site/src/content/docs/guides/integrations.mddocs-site/src/content/docs/ja/reference/cli/agents.mddocs-site/src/content/docs/ko/reference/cli/agents.mddocs-site/src/content/docs/reference/cli/agents.mddocs-site/src/content/docs/ru/reference/cli/agents.mddocs-site/src/content/docs/zh-cn/reference/cli/agents.mdsrc/clients/config-export.tssrc/integrations/writer.tstests/client-config-export-new-clients.test.tstests/client-config-export.test.tstests/integrations-writer.test.ts
| const configDir = "custom-omp"; | ||
| const configRoot = join(home, configDir); | ||
| const profiled = { OMP_PROFILE: "work", PI_CONFIG_DIR: configDir } as NodeJS.ProcessEnv; | ||
| expect(ompModelsConfigPath(profiled, home)).toBe( | ||
| join(configRoot, "profiles", "work", "agent", "models.yml"), | ||
| ); | ||
| expect(ompModelsConfigPath({ PI_CONFIG_DIR: configDir } as NodeJS.ProcessEnv, home)).toBe( | ||
| join(configRoot, "agent", "models.yml"), | ||
| ); | ||
| expect(ompModelsConfigPath({ PI_PROFILE: "legacy", PI_CONFIG_DIR: configDir } as NodeJS.ProcessEnv, home)).toBe( | ||
| join(configRoot, "profiles", "legacy", "agent", "models.yml"), | ||
| ); | ||
| // OMP_PROFILE wins by presence, so an explicit blank selects the default | ||
| // profile instead of inheriting a legacy PI_PROFILE. | ||
| expect(ompModelsConfigPath({ | ||
| OMP_PROFILE: " ", | ||
| PI_PROFILE: "legacy", | ||
| PI_CONFIG_DIR: configDir, | ||
| } as NodeJS.ProcessEnv, home)).toBe( | ||
| join(configRoot, "agent", "models.yml"), | ||
| ); | ||
| expect(() => ompModelsConfigPath({ OMP_PROFILE: ".." } as NodeJS.ProcessEnv, home)).toThrow(ClientPathError); | ||
| expect(() => ompModelsConfigPath({ OMP_PROFILE: "NUL.txt" } as NodeJS.ProcessEnv, home)).toThrow(ClientPathError); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add regression cases for the full OMP path contract.
The current cases use only PI_CONFIG_DIR = "custom-omp". They do not protect the new home-relative behavior for values beginning with / or ~. They also do not prove that PI_CODING_AGENT_DIR is ignored when a profile is active.
Add both cases with the temporary home:
Suggested assertions
const profiled = {
OMP_PROFILE: "work",
PI_CONFIG_DIR: configDir,
+ PI_CODING_AGENT_DIR: join(home, "wrong-agent"),
} as NodeJS.ProcessEnv;
+ for (const value of ["/custom-omp", "~/.custom-omp"]) {
+ expect(ompModelsConfigPath({ PI_CONFIG_DIR: value } as NodeJS.ProcessEnv, home)).toBe(
+ join(home, value, "agent", "models.yml"),
+ );
+ }As per path instructions, tests/** should include focused regression coverage for behavior changes in src/**. The supplied OMP path contract requires these values to follow the path.join(home, value) behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const configDir = "custom-omp"; | |
| const configRoot = join(home, configDir); | |
| const profiled = { OMP_PROFILE: "work", PI_CONFIG_DIR: configDir } as NodeJS.ProcessEnv; | |
| expect(ompModelsConfigPath(profiled, home)).toBe( | |
| join(configRoot, "profiles", "work", "agent", "models.yml"), | |
| ); | |
| expect(ompModelsConfigPath({ PI_CONFIG_DIR: configDir } as NodeJS.ProcessEnv, home)).toBe( | |
| join(configRoot, "agent", "models.yml"), | |
| ); | |
| expect(ompModelsConfigPath({ PI_PROFILE: "legacy", PI_CONFIG_DIR: configDir } as NodeJS.ProcessEnv, home)).toBe( | |
| join(configRoot, "profiles", "legacy", "agent", "models.yml"), | |
| ); | |
| // OMP_PROFILE wins by presence, so an explicit blank selects the default | |
| // profile instead of inheriting a legacy PI_PROFILE. | |
| expect(ompModelsConfigPath({ | |
| OMP_PROFILE: " ", | |
| PI_PROFILE: "legacy", | |
| PI_CONFIG_DIR: configDir, | |
| } as NodeJS.ProcessEnv, home)).toBe( | |
| join(configRoot, "agent", "models.yml"), | |
| ); | |
| expect(() => ompModelsConfigPath({ OMP_PROFILE: ".." } as NodeJS.ProcessEnv, home)).toThrow(ClientPathError); | |
| expect(() => ompModelsConfigPath({ OMP_PROFILE: "NUL.txt" } as NodeJS.ProcessEnv, home)).toThrow(ClientPathError); | |
| const configDir = "custom-omp"; | |
| const configRoot = join(home, configDir); | |
| const profiled = { | |
| OMP_PROFILE: "work", | |
| PI_CONFIG_DIR: configDir, | |
| PI_CODING_AGENT_DIR: join(home, "wrong-agent"), | |
| } as NodeJS.ProcessEnv; | |
| expect(ompModelsConfigPath(profiled, home)).toBe( | |
| join(configRoot, "profiles", "work", "agent", "models.yml"), | |
| ); | |
| for (const value of ["/custom-omp", "~/.custom-omp"]) { | |
| expect(ompModelsConfigPath({ PI_CONFIG_DIR: value } as NodeJS.ProcessEnv, home)).toBe( | |
| join(home, value, "agent", "models.yml"), | |
| ); | |
| } | |
| expect(ompModelsConfigPath({ PI_CONFIG_DIR: configDir } as NodeJS.ProcessEnv, home)).toBe( | |
| join(configRoot, "agent", "models.yml"), | |
| ); | |
| expect(ompModelsConfigPath({ PI_PROFILE: "legacy", PI_CONFIG_DIR: configDir } as NodeJS.ProcessEnv, home)).toBe( | |
| join(configRoot, "profiles", "legacy", "agent", "models.yml"), | |
| ); | |
| // OMP_PROFILE wins by presence, so an explicit blank selects the default | |
| // profile instead of inheriting a legacy PI_PROFILE. | |
| expect(ompModelsConfigPath({ | |
| OMP_PROFILE: " ", | |
| PI_PROFILE: "legacy", | |
| PI_CONFIG_DIR: configDir, | |
| } as NodeJS.ProcessEnv, home)).toBe( | |
| join(configRoot, "agent", "models.yml"), | |
| ); | |
| expect(() => ompModelsConfigPath({ OMP_PROFILE: ".." } as NodeJS.ProcessEnv, home)).toThrow(ClientPathError); | |
| expect(() => ompModelsConfigPath({ OMP_PROFILE: "NUL.txt" } as NodeJS.ProcessEnv, home)).toThrow(ClientPathError); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/client-config-export.test.ts` around lines 481 - 503, Extend the OMP
path contract tests around ompModelsConfigPath to cover PI_CONFIG_DIR values
beginning with "/" and "~", asserting they resolve via path.join(home, value)
using the temporary home. Add a profile-active case that sets
PI_CODING_AGENT_DIR and verifies it is ignored, with the profile-based models
path unchanged.
Source: Path instructions
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review complete on current head 0865270b. The two prior P2 production blockers are fixed, the security review is clean, and the only remaining CodeRabbit item is a trivial regression-coverage gap. Because the GitHub integration cannot push into the contributor fork despite maintainer edits being enabled, I reproduced the exact #1109 head on maintainer PR #1323, added only the missing OMP path-contract regression cases, and ran the full Cross-platform CI + React Doctor matrix there. Both are green. Approving #1109; the test-only maintainer follow-up will be rebased onto post-merge dev and landed immediately after.
test(omp): close final #1109 path-contract review gap
|
Thanks @n3wr1ch for the OMP integration and for working through the review rounds. #1109 is merged, and the final maintainer-only OMP path regression coverage has now also landed via #1323 with a fresh green Cross-platform CI + React Doctor run. The live OMP verification, complete client/UI/docs wiring, and careful credential handling made this feature much easier to validate. Appreciate the thorough follow-through on the review feedback. 🙌 |
2 participants
Summary
~/.omp/agent/models.ymlopencodex-loopbackplaceholderDependency
The prerequisite Pi visibility fix has landed on
devvia #1151. This branch is now rebased onto currentdev; the inherited #1085 commits and the broader export-policy changes that were not part of OMP have been removed.Overlap
#1109 was opened first and intentionally remained draft while the Pi prerequisite was unresolved. The overlapping #1140 was closed as a duplicate of this earlier PR. This update keeps #1109 focused on its original OMP contract without the former stacked diff.
Security review
This adds a credential destination and requires explicit maintainer security review. No user or provider secret is serialized: OMP receives the non-secret
opencodex-loopbackplaceholder, and integration writes remain restricted to loopback proxy binds. Upstream provider credentials stay in opencodex's own configuration.Verification
Verified after rebasing onto the latest
devat final head0865270b:bun run typecheckbun run teston final head — 10,101 pass, 7 skip, 0 failbun run privacy:scancd gui && bun test tests— 696 pass, 0 failbun run lint:guicd gui && bun run lint:i18nbun run build:guicd docs-site && bun run buildopencodex/gpt-5.4-minireturned the requestedOMP_OPENCODEX_OKresponseScreenshots
The GUI exposes OMP as its own integration tab and reports the generated
models.ymldestination and restart guidance.OMP 17.2.11 loads the complete routed opencodex catalog from the generated configuration.
A live English prompt sent with
opencodex/gpt-5.4-minireceives the expected response through/v1/chat/completions.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