fix(providers): harden discovery contract cleanup

[olddonkey]

Summary

• reject registry-owned model-discovery paths containing a .. segment
• centralize temporary registry discovery overrides in a shared test helper
• always restore registry state and clear the affected model cache after those tests

This is a focused follow-up to the contract merged in #652 and to review findings surfaced on #653.

Relates to #572. This PR does not close the umbrella issue.

Verification

• bun run typecheck
• focused discovery/connection tests: 34 pass, 0 fail
• bun run privacy:scan
• full suite: 6029 pass, 2 skip, 0 fail

Summary by CodeRabbit

• Bug Fixes

  • Strengthened model discovery path validation by rejecting unsafe traversal segments (including encoded variants) and disallowing backslashes, improving overall safety and preventing invalid specs from being accepted.

• Tests

  • Added assertions covering rejection of parent-directory traversal paths and verified cache clearing occurs before temporary discovery policies run.
  • Simplified discovery test setup/cleanup to improve reliability and reduce manual registry mutation.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 03c314ce-b849-400d-8367-44994137cbe6

📥 Commits

Reviewing files that changed from the base of the PR and between 3ebe0bd and 5356e23.

📒 Files selected for processing (4)

• src/providers/model-discovery.ts
• tests/helpers/provider-registry-discovery.ts
• tests/provider-connection-test.test.ts
• tests/provider-model-discovery-contract.test.ts

---

📝 Walkthrough

Walkthrough

Discovery validation rejects backslashes and normalized parent-directory traversal segments. Tests now share scoped registry discovery overrides that restore registry state and clear model caches before and after execution.

Changes

Discovery validation and test infrastructure

Layer / File(s)	Summary
Reject unsafe discovery paths src/providers/model-discovery.ts, tests/provider-model-discovery-contract.test.ts	Discovery paths containing backslashes or normalized .. segments now fail validation, with encoded and dot-variant traversal cases covered by contract tests.
Centralize scoped registry overrides tests/helpers/provider-registry-discovery.ts, tests/provider-connection-test.test.ts, tests/provider-model-discovery-contract.test.ts	Tests use a shared helper to apply temporary registry discovery settings, restore prior values, clear model caches, and verify cache clearing during scoped discovery.

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

Possibly related PRs

• lidge-jun/opencodex#652: Adds related provider model-discovery path and contract validation.
• lidge-jun/opencodex#653: Introduces related model-discovery contracts and registry-discovery test infrastructure.

Suggested labels: bug

Suggested reviewers: wibias, ingwannu, lidge-jun

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% 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
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: hardening provider discovery contract cleanup and related safeguards.
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.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ebe0bdb60

ℹ️ 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".

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@src/providers/model-discovery.ts`:
- Line 93: Update the discovery-path validation near the existing parent-segment
guard in resolveProviderModelDiscoveryUrl to reject URL-equivalent encoded dot
segments such as %2e%2e and backslash-based traversal before constructing the
URL. Add regression cases in tests/provider-model-discovery-contract.test.ts
lines 70-74 covering both encoded traversal and backslash traversal inputs.

In `@tests/helpers/provider-registry-discovery.ts`:
- Around line 17-30: Update the temporary override flow around
entry.modelDiscovery in the provider registry helper to call
clearModelCache(providerId) before applying the override, ensuring run() cannot
reuse a cached model result. Keep the existing clearModelCache(providerId) call
in finally for cleanup after restoring the original registry settings.

🪄 Autofix (Beta)

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: 180c8aaf-399e-4115-aa9e-9429e7a09728

📥 Commits

Reviewing files that changed from the base of the PR and between 0666b41 and 3ebe0bd.

📒 Files selected for processing (4)

• src/providers/model-discovery.ts
• tests/helpers/provider-registry-discovery.ts
• tests/provider-connection-test.test.ts
• tests/provider-model-discovery-contract.test.ts

[olddonkey]

Review follow-up is complete in 5356e233, and the branch is rebased onto current dev (e4495211). All three review threads are resolved.

Verification:

• focused discovery/connection tests: 35 pass / 0 fail
• bun run typecheck: pass
• bun run privacy:scan: pass
• full suite: 6058 pass / 2 skip / 3 fail

The three full-suite failures are existing failures on the new dev base, not this diff. I reproduced the same assertions unchanged in a detached, clean upstream/dev@e4495211 worktree:

• CLI subcommand help > invalid service and codex-shim usage include remove alias
• workspace account integration seam > keeps logout and delete failure states honest
• decodeSchtasksOutput > decodes UTF-16LE BOM XML that would fail as UTF-8

No unrelated baseline fixes were added to this provider-contract PR.

[lidge-jun]

LAND-AFTER + NEEDS-SECURITY-REVIEW

The production surface here is genuinely small — four lines at src/providers/model-discovery.ts:87-96 rejecting backslashes and both literal and percent-encoded .. segments in registry-owned discovery paths. The rest is test isolation: pulling the ad-hoc PROVIDER_REGISTRY mutation into withRegistryDiscovery() with a finally restore is a real improvement, since a test that throws mid-way no longer leaves the registry poisoned for whatever runs next.

The security review requirement is not about the size of the diff. MAINTAINERS.md:33-34 says "Authentication, credential handling, GitHub Actions, release automation, dependency installation, and other security-boundary changes require explicit security review." A registry-owned discovery path is the destination provider credentials get sent to, and this PR changes the validation that decides which paths are acceptable. New path-traversal defenses deserve someone actively trying to get around them — mixed encoding, overlong forms, and whether a name that legitimately contains .. still works are the obvious places to push.

To be clear about scope: this hardens the shared discovery contract from #572 but does not close that umbrella, and the PR body saying "Relates to #572" is accurate.

What happens next: no code change requested. The PR waits on explicit security review, then lands after the release blockers.

---

CI context (shared across the current review round). dev is now at 62e937614. The baseline breakage that made every required check red is fixed: run 30545575865 has macos-latest, ubuntu-latest, and all three npm-global jobs green. The one remaining red is windows-latest, and it is not a test failure — it is a Bun 1.3.14 runtime panic (heap.zig:deleteMin reached through spawnSync with a timeout, triggered by our Windows ACL hardening path). That is a runtime defect being tracked separately, so please do not treat a red windows-latest as a signal about your change. Rebase onto current dev before your next push so your checks run against the repaired baseline.