feat(api): let the provider probe use a stored secret by id - #6195
feat(api): let the provider probe use a stored secret by id#6195mmabrouk wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughProvider probing now accepts either provider kind or a stored secret ID. The router loads project-scoped credentials, applies request overrides, validates provider changes, and invokes the probe service. Tests cover stored credentials, isolation, errors, and validation. ChangesStored provider probing
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ⚪ Minimal · up to The change enables stored-secret provider probes while preserving project scoping and credential override behavior. No actionable merge-blocking risk remains; the outbound-credential assertion can be added as routine follow-up. Sequence Diagram(s)sequenceDiagram
participant Client
participant ProvidersRouter
participant VaultService
participant ProviderProbeService
Client->>ProvidersRouter: Submit probe request
ProvidersRouter->>VaultService: Load project-scoped secret
VaultService-->>ProvidersRouter: Return stored credentials
ProvidersRouter->>ProviderProbeService: Probe with merged credentials
ProviderProbeService-->>Client: Return probe result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/oss/tests/pytest/unit/providers/test_provider_probe.py (1)
918-933: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert which credential is sent when the caller changes the provider kind.
This test only checks the status code. The security-relevant claim of the guard at
api/oss/src/apis/fastapi/providers/router.pylines 93-100 is that a kind change uses the caller's key and never the stored key. Use aRecorderand assert the outbound authorization header, as the neighbouring tests do.💚 Proposed assertion on the outbound credential
def test_a_kind_change_is_allowed_when_the_caller_brings_the_credential(monkeypatch): vault = _StubVault() secret_id = uuid4() vault.store(secret_id, PROJECT_ID, _stored_provider_key(kind="openai")) - client = build_client(monkeypatch, json_response({"data": []}), vault=vault) + recorder = Recorder(json_response({"data": []})) + client = build_client(monkeypatch, recorder, vault=vault) response = client.post( "/providers/probe", json={ "secret_id": str(secret_id), "kind": "anthropic", "provider": {"key": CANARY}, }, ) assert response.status_code == 200 + (sent,) = recorder.requests + assert STORED_KEY not in str(sent.headers)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: dea7d63e-f742-4978-b79a-b8e911aa3349
📒 Files selected for processing (4)
api/entrypoints/routers.pyapi/oss/src/apis/fastapi/providers/models.pyapi/oss/src/apis/fastapi/providers/router.pyapi/oss/tests/pytest/unit/providers/test_provider_probe.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
removal-batch published the real contract (PR #6195). `secret_id` and the field-by-field override were as assumed; two things were not. **`kind` is now omitted whenever the request names a row.** It is optional alongside `secret_id`, and sending one that disagrees with the stored kind is a 422 unless a key rides along — which this request deliberately has none of. The stored kind is authoritative, so the card's own canonical spelling, which can differ from the vault's for a legacy slug, is simply not sent. That removes the 422 class from this surface rather than handling it. **A failed probe no longer blames the provider for everything.** A probe outcome is a 200 with a status inside, so a throw here is the request failing — and "could not reach the provider" is false for a 404, which means the stored connection is gone. `probeFailureMessage` answers that case on its own terms, speaks the server's own words for any other 4xx that carried a message, and keeps the reach-the-provider line for a transport failure or a 5xx. It lives in core beside the other probe helpers so it is testable without the card's tree. Copy is placeholder, marked TODO(copy: owner) like the rest of this PR.
Railway Preview Environment
|
f4e81a6 to
227e913
Compare
a84533a to
e5d0e74
Compare
removal-batch published the real contract (PR #6195). `secret_id` and the field-by-field override were as assumed; two things were not. **`kind` is now omitted whenever the request names a row.** It is optional alongside `secret_id`, and sending one that disagrees with the stored kind is a 422 unless a key rides along — which this request deliberately has none of. The stored kind is authoritative, so the card's own canonical spelling, which can differ from the vault's for a legacy slug, is simply not sent. That removes the 422 class from this surface rather than handling it. **A failed probe no longer blames the provider for everything.** A probe outcome is a 200 with a status inside, so a throw here is the request failing — and "could not reach the provider" is false for a 404, which means the stored connection is gone. `probeFailureMessage` answers that case on its own terms, speaks the server's own words for any other 4xx that carried a message, and keeps the reach-the-provider line for a transport failure or a 5xx. It lives in core beside the other probe helpers so it is testable without the card's tree. Copy is placeholder, marked TODO(copy: owner) like the rest of this PR.
e5d0e74 to
a3fa961
Compare
227e913 to
f6bcdbc
Compare
removal-batch published the real contract (PR #6195). `secret_id` and the field-by-field override were as assumed; two things were not. **`kind` is now omitted whenever the request names a row.** It is optional alongside `secret_id`, and sending one that disagrees with the stored kind is a 422 unless a key rides along — which this request deliberately has none of. The stored kind is authoritative, so the card's own canonical spelling, which can differ from the vault's for a legacy slug, is simply not sent. That removes the 422 class from this surface rather than handling it. **A failed probe no longer blames the provider for everything.** A probe outcome is a 200 with a status inside, so a throw here is the request failing — and "could not reach the provider" is false for a 404, which means the stored connection is gone. `probeFailureMessage` answers that case on its own terms, speaks the server's own words for any other 4xx that carried a message, and keeps the reach-the-provider line for a transport failure or a 5xx. It lives in core beside the other probe helpers so it is testable without the card's tree. Copy is placeholder, marked TODO(copy: owner) like the rest of this PR.
a3fa961 to
ac58713
Compare
f6bcdbc to
6533af8
Compare
removal-batch published the real contract (PR #6195). `secret_id` and the field-by-field override were as assumed; two things were not. **`kind` is now omitted whenever the request names a row.** It is optional alongside `secret_id`, and sending one that disagrees with the stored kind is a 422 unless a key rides along — which this request deliberately has none of. The stored kind is authoritative, so the card's own canonical spelling, which can differ from the vault's for a legacy slug, is simply not sent. That removes the 422 class from this surface rather than handling it. **A failed probe no longer blames the provider for everything.** A probe outcome is a 200 with a status inside, so a throw here is the request failing — and "could not reach the provider" is false for a 404, which means the stored connection is gone. `probeFailureMessage` answers that case on its own terms, speaks the server's own words for any other 4xx that carried a message, and keeps the reach-the-provider line for a transport failure or a 5xx. It lives in core beside the other probe helpers so it is testable without the card's tree. Copy is placeholder, marked TODO(copy: owner) like the rest of this PR.
ac58713 to
dfd33cc
Compare
6533af8 to
9e3da5e
Compare
dfd33cc to
ab97118
Compare
9e3da5e to
e2a625c
Compare
Context
A saved write-only connection needs to be testable without asking the user to type its key again. The probe therefore accepts a Vault
secret_id. A managed credential must not use that path because caller-supplied overrides could redirect the credential to another endpoint.Changes
For ordinary user-owned rows,
POST /providers/probecan resolve the stored credential and apply non-secret field overrides as before.For any row with internal management metadata, the route now returns HTTP 409 before it inspects credentials, merges overrides, or makes an outbound request. The check is about managed lifecycle, not the manager name, so future managers receive the same protection.
The OpenAPI contract includes the probe request and response. The frontend Fern client is regenerated in #6174 and uses this endpoint directly.
Tests / notes
What to QA
Depends on #6138 through the
credits-starter-seedingbase.