Skip to content

test(omp): close final #1109 path-contract review gap - #1323

Merged
Wibias merged 1 commit into
devfrom
maint/1109-omp-path-regression
Aug 9, 2026
Merged

Wibias merged 1 commit into
devfrom
maint/1109-omp-path-regression

Conversation

@Wibias

@Wibias Wibias commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Maintainer follow-up for #1109. #1109 is merged; this PR now contains only focused OMP path-contract regression coverage on top of current dev.

Current scope:

  • slash- and tilde-prefixed PI_CONFIG_DIR values stay home-relative under OMP's path.join(home, value) contract
  • named profiles ignore PI_CODING_AGENT_DIR
  • tests use unique temporary home directories so legacy models.yaml files or parallel runs cannot affect the result

No production behavior changes. Merge only after fresh current-head CI is green.

Summary by CodeRabbit

  • Tests
    • Added regression coverage for resolving configuration paths relative to the home directory.
    • Added coverage ensuring configured paths take priority when selecting named profiles.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Aug 9, 2026
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This change adds OMP regression tests for home-relative PI_CONFIG_DIR resolution and named-profile precedence over PI_CODING_AGENT_DIR. Tests create temporary home directories and clean them up after execution.

Changes

OMP path contract

Layer / File(s) Summary
OMP path resolution and profile tests
tests/omp-path-contract.test.ts
Adds isolated temporary-home setup and cleanup. Tests validate slash- and tilde-prefixed PI_CONFIG_DIR values and confirm that named profiles use PI_CONFIG_DIR for the models configuration path.

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

Suggested reviewers: ingwannu, lidge-jun

🚥 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%. 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 OMP regression tests that close the remaining path-contract review gap described in the changeset.
✨ 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/1109-omp-path-regression

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 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 the documentation around the named-profile
`PI_CONFIG_DIR` behavior to state that its value is passed to `path.join(home,
value)`: `/custom` and `~/custom` remain under the user’s home directory, and
`~` is not shell-expanded. Keep the existing distinction from
`PI_CODING_AGENT_DIR` unchanged.

In `@src/clients/config-export.ts`:
- Around line 1004-1017: The OMP export filename must match the resolved
destination basename. Update the OMP entry in EXPORT_CLIENTS and related
filename handling to derive the name from EXPORT_CLIENTS.omp.destination(env),
preserving models.yaml only when that is the resolved destination and otherwise
using models.yml; update the OMP-specific assertion in
tests/client-config-export.test.ts:450 accordingly, while the management route
assertion requires no direct change if it already validates the derived
filename.

In `@src/integrations/writer.ts`:
- Line 236: Update the integrations.error.nonLoopback translation in the
affected GUI locale catalogs, including ko.ts, ru.ts, and zh.ts, to remove the
stale claim that manual configuration cannot help and match the current
loopback/tunnel guidance shown by writer.ts. Search all remaining locale
catalogs for the same wording and synchronize them without changing the writer
logic or bypassing the i18n files.

In `@tests/management-client-config-route.test.ts`:
- Around line 180-181: Extend the credential-leakage assertions in the relevant
management-client configuration test to serialize and inspect the complete
response envelope via JSON.stringify(body), ensuring REAL_LOOKING_KEY is absent
from all response fields, including exportHint and destination.
- Around line 32-33: Update the local response envelope type in the management
client-config route test to include mediaType, then in the OMP assertion near
expect(body.format).toBe("yaml"), assert that body.mediaType equals the expected
YAML MIME type.

In `@tests/omp-path-contract.test.ts`:
- Around line 6-14: Create a unique temporary home directory with mkdtempSync()
for the assertion at tests/omp-path-contract.test.ts#L6-L14, wrap the
filesystem-dependent test in try/finally, and remove the directory in finally.
Apply the same isolated-directory and cleanup pattern to
tests/omp-path-contract.test.ts#L16-L27, reusing the generated directory for
each case.
🪄 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: 88ae266f-b34a-494d-8f1c-cec76e5a03b6

📥 Commits

Reviewing files that changed from the base of the PR and between 6327432 and 1006f73.

⛔ Files ignored due to path filters (3)
  • devlog/_plan/260806_omp_integration/evidence/omp-dashboard.png is excluded by !**/*.png
  • devlog/_plan/260806_omp_integration/evidence/omp-live-prompt.png is excluded by !**/*.png
  • devlog/_plan/260806_omp_integration/evidence/omp-model-catalog.png is excluded by !**/*.png
📒 Files selected for processing (38)
  • docs-site/src/content/docs/guides/integrations.md
  • docs-site/src/content/docs/ja/reference/cli/agents.md
  • docs-site/src/content/docs/ja/reference/management-api.md
  • docs-site/src/content/docs/ko/reference/cli/agents.md
  • docs-site/src/content/docs/ko/reference/management-api.md
  • docs-site/src/content/docs/reference/cli/agents.md
  • docs-site/src/content/docs/reference/management-api.md
  • docs-site/src/content/docs/ru/reference/cli/agents.md
  • docs-site/src/content/docs/ru/reference/management-api.md
  • docs-site/src/content/docs/zh-cn/reference/cli/agents.md
  • docs-site/src/content/docs/zh-cn/reference/management-api.md
  • gui/src/app-routing.ts
  • gui/src/components/apikeys-workspace/client-config-clients.ts
  • gui/src/i18n/de.ts
  • gui/src/i18n/en.ts
  • gui/src/i18n/ja.ts
  • gui/src/i18n/ko.ts
  • gui/src/i18n/ru.ts
  • gui/src/i18n/zh.ts
  • gui/src/pages/Integrations.tsx
  • gui/src/pages/integrations/FileIntegrationPage.tsx
  • gui/src/pages/integrations/integration-api.ts
  • gui/src/pages/integrations/overview-clients.ts
  • gui/tests/client-config-panel.test.tsx
  • gui/tests/integrations-overview-rows.test.ts
  • src/cli/export-command.ts
  • src/cli/help.ts
  • src/clients/config-export.ts
  • src/integrations/registry.ts
  • src/integrations/writer.ts
  • tests/cli-export-command.test.ts
  • tests/client-config-export-new-clients.test.ts
  • tests/client-config-export.test.ts
  • tests/integrations-invariants.test.ts
  • tests/integrations-state.test.ts
  • tests/integrations-writer.test.ts
  • tests/management-client-config-route.test.ts
  • tests/omp-path-contract.test.ts

Comment thread docs-site/src/content/docs/guides/integrations.md
Comment thread src/clients/config-export.ts
Comment thread src/integrations/writer.ts
Comment thread tests/management-client-config-route.test.ts
Comment thread tests/management-client-config-route.test.ts
Comment thread tests/omp-path-contract.test.ts
@Wibias Wibias closed this Aug 9, 2026
@Wibias
Wibias force-pushed the maint/1109-omp-path-regression branch from 1006f73 to 637711b Compare August 9, 2026 01:08
@Wibias Wibias reopened this Aug 9, 2026
@Wibias Wibias closed this Aug 9, 2026
@Wibias
Wibias force-pushed the maint/1109-omp-path-regression branch from 96989f2 to 2418291 Compare August 9, 2026 01:56
@Wibias Wibias reopened this Aug 9, 2026

Wibias commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Review-thread disposition after rebasing this PR onto current dev:

  • The current PR is now intentionally one file only: tests/omp-path-contract.test.ts.
  • CodeRabbit's current test-isolation finding was valid and is fixed in 53083223; CodeRabbit has auto-resolved that thread.
  • The omp-models.yaml filename finding is a duplicate of the finding already withdrawn on feat(integrations): add OMP client support #1109: filename is the qualified standalone download artifact, while destination is the managed client path. No production change is needed here.
  • The older docs/i18n/management-test comments refer to files that were part of the pre-reset inherited feat(integrations): add OMP client support #1109 tree and are no longer in this PR. Some are reasonable mainline hardening observations, but they are not findings against this current one-file follow-up and should not remain as merge blockers here.

Fresh CI is running on current head 53083223; this PR will merge only if that head is fully green.

@Wibias
Wibias merged commit 79831c9 into dev Aug 9, 2026
21 of 22 checks passed
@Wibias
Wibias deleted the maint/1109-omp-path-regression branch August 9, 2026 02:05
agentHits pushed a commit to agentHits/opencodex that referenced this pull request Sep 17, 2026
…regression

test(omp): close final lidge-jun#1109 path-contract review gap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant