Skip to content

fix(management): validate complete provider POST candidates before adoption - #5013

Merged
lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:agent/pinsless-post-validate-20260918
Sep 18, 2026
Merged

lidge-jun merged 1 commit into
lidge-jun:devfrom
luvs01:agent/pinsless-post-validate-20260918

Conversation

@luvs01

@luvs01 luvs01 commented Sep 18, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A management POST /api/providers that carries no pin fields never runs validateConfigCandidate: the pinsOwned gate skips candidate validation entirely, so a provider field the management boundary does not check (for example an editor-owned enum such as apiKeyPoolStrategy) can persist a schema-invalid candidate before live adoption.

  • The registration draft is now staged for every new registration, not only pin-owning ones, so discovery/disabled-model side effects are validated with the candidate instead of mutating live state first.
  • validateConfigCandidate runs unconditionally on the completed POST draft; a failure returns 400 before any provider/default state is adopted.

Regression coverage: provider POST validates a pins-less candidate before live adoption posts a schema-invalid apiKeyPoolStrategy with no pin fields and asserts a 400 with nothing persisted.

Verification

Exact head: ec5f84aae714799971eeac3c2022e0ddeedb384a (tree 72a6ad8aea3dafdaf36edcdd36c94502221f2f4f), based on dev 444cf77012a6563d10768088546a43a07399a0d7.

  • bun x tsc --noEmit
  • bun run structure:check
  • bun run privacy:scan
  • bun test ./tests/server/management-provider-validation.test.ts

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. Local gates passed on this head (tsc, structure:check, privacy:scan, focused suite); fork CI dispatched.

  • I pushed my PR to the latest dev commit. The branch carries dev 444cf7701, 10 behind tip - inside the 10-commit window.

  • I resolved all correct Codex and CodeRabbit findings. No review rounds yet on this head; the branch is new.

  • My PR is ready for review.

Summary by CodeRabbit

  • Bug Fixes
    • Provider registration now validates the complete configuration before adoption, including submissions without pin settings.
    • Invalid provider configuration values are rejected with a clear validation error instead of being saved.
    • Prevented invalid providers from being persisted to the application configuration.

@coderabbitai

coderabbitai Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: d7a631c2-8116-4cfe-b50f-5929ab277fa7

📥 Commits

Reviewing files that changed from the base of the PR and between c9fd114 and ec5f84a.

📒 Files selected for processing (2)
  • src/server/management/provider-routes.ts
  • tests/server/management-provider-validation.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The provider POST route now validates every new-provider draft before adoption. The route no longer skips draft validation when pin fields are absent. Tests verify that an invalid apiKeyPoolStrategy returns HTTP 400 and does not persist the provider.

Changes

Provider validation

Layer / File(s) Summary
Draft construction and validation
src/server/management/provider-routes.ts
At lines 1260-1271, the route always stages registrationDraft, initializes model selection, removes registry-only static headers, builds a draft config, and runs validateConfigCandidate. It returns HTTP 400 when validation fails.
Regression coverage
tests/server/management-provider-validation.test.ts
The existing POST setup now stubs providerDestinationResolvedError to return null at line 592. The new test at lines 615-647 submits apiKeyPoolStrategy: "bogus" without pins and verifies HTTP 400 plus no providers.relay config entry.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to ec5f8

Invalid provider registrations are rejected before being saved, with no confirmed regression in successful registrations.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. 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 and concisely describes the main change: validating complete provider POST candidates before adoption, including requests without pin fields.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added bug Something isn't working review-ready labels Sep 18, 2026
@github-actions
github-actions Bot marked this pull request as ready for review September 18, 2026 03:06
@github-actions

github-actions Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ 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.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

@lidge-jun
lidge-jun merged commit 9269f94 into lidge-jun:dev Sep 18, 2026
9 checks passed
lzfxxx added a commit to lzfxxx/opencodex that referenced this pull request Sep 18, 2026
management-provider-validation.test.ts and codex-v2-gate.test.ts both sat at their file-size caps, so the cases added after the cap was set failed the ratchet. Move the lidge-jun#5013 pins-less POST candidate case and the three lidge-jun#4941 pristine-baseline pin cases into sibling files, register both in the layout maps, and leave the baselines unchanged. Cases are unchanged.
@luvs01
luvs01 deleted the agent/pinsless-post-validate-20260918 branch September 20, 2026 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants