Skip to content

feat(api): let the provider probe use a stored secret by id - #6195

Open
mmabrouk wants to merge 1 commit into
credits-starter-seedingfrom
fix/provider-probe-stored-key
Open

feat(api): let the provider probe use a stored secret by id#6195
mmabrouk wants to merge 1 commit into
credits-starter-seedingfrom
fix/provider-probe-stored-key

Conversation

@mmabrouk

@mmabrouk mmabrouk commented Aug 22, 2026

Copy link
Copy Markdown
Member

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/probe can 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

  • The focused provider-probe suite passed 93 tests during this slice.
  • The final combined API verification passed 233 tests.
  • The managed regression test proves a caller-supplied URL produces no outbound request.
  • Ruff formatting and checks passed.

What to QA

  • Probe an ordinary saved write-only connection without retyping the key. Credential and discovery statuses must return normally.
  • Probe a managed secret ID with an overridden URL. The API must return 409 and make no outbound request.
  • Probe with a missing, cross-project, or mismatched secret. Existing 404, 403, and 422 behavior must remain unchanged.

Depends on #6138 through the credits-starter-seeding base.

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. backend enhancement New feature or request labels Aug 22, 2026
@vercel

vercel Bot commented Aug 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agenta-documentation Ready Ready Preview Aug 22, 2026 10:54pm

Request Review

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • release/.*

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 77354540-fd43-4b8c-827b-d852bf967f07

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Provider connectivity checks can now use securely stored credentials.
    • You can override stored provider credentials or connection details when probing.
    • Stored credentials are scoped to the current project for improved isolation.
  • Bug Fixes

    • Added validation for missing provider information and invalid credential references.
    • Prevented switching provider types without supplying compatible replacement credentials.

Walkthrough

Provider 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.

Changes

Stored provider probing

Layer / File(s) Summary
Probe request contract
api/oss/src/apis/fastapi/providers/models.py
ProbeProviderRequest now supports optional kind, secret_id, and provider credential overrides. Validation rejects requests without kind and secret_id.
Stored credential resolution
api/oss/src/apis/fastapi/providers/router.py, api/entrypoints/routers.py
ProvidersRouter receives VaultService, loads project-scoped secrets, merges stored and supplied credentials, validates provider-kind changes, and probes with the effective credentials.
Stored probe validation
api/oss/tests/pytest/unit/providers/test_provider_probe.py
Tests configure vault data and cover stored credentials, overrides, project isolation, unknown secrets, provider-kind consistency, and missing probe identifiers.

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

Merge Risk: ⚪ Minimal · up to d5a4d

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
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.74% which is insufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 4 files. 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.
Title check ✅ Passed The title clearly describes the main change: provider probes can use a stored secret by ID.
Description check ✅ Passed The description explains stored-secret probing, managed-secret safeguards, overrides, API behavior, tests, and QA expectations.
✨ 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 fix/provider-probe-stored-key

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.

@mmabrouk

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@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.

🧹 Nitpick comments (1)
api/oss/tests/pytest/unit/providers/test_provider_probe.py (1)

918-933: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert 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.py lines 93-100 is that a kind change uses the caller's key and never the stored key. Use a Recorder and 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

📥 Commits

Reviewing files that changed from the base of the PR and between a84533a and d5a4d5f.

📒 Files selected for processing (4)
  • api/entrypoints/routers.py
  • api/oss/src/apis/fastapi/providers/models.py
  • api/oss/src/apis/fastapi/providers/router.py
  • api/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.

mmabrouk added a commit that referenced this pull request Aug 22, 2026
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.
@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Railway Preview Environment

Preview URL https://gateway-pr-6195.up.railway.app/w
Project agenta-oss-clone-spike
Image tag pr-6195-0a6e646
Status Deployed
Railway logs Open logs
Workflow logs View workflow run
Updated at 2026-08-22T11:19:11.491Z

@mmabrouk
mmabrouk force-pushed the fix/provider-probe-stored-key branch from f4e81a6 to 227e913 Compare August 22, 2026 11:14
@mmabrouk
mmabrouk force-pushed the credits-starter-seeding branch from a84533a to e5d0e74 Compare August 22, 2026 11:14
mmabrouk added a commit that referenced this pull request Aug 22, 2026
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.
@mmabrouk
mmabrouk force-pushed the credits-starter-seeding branch from e5d0e74 to a3fa961 Compare August 22, 2026 22:02
@mmabrouk
mmabrouk force-pushed the fix/provider-probe-stored-key branch from 227e913 to f6bcdbc Compare August 22, 2026 22:02
mmabrouk added a commit that referenced this pull request Aug 22, 2026
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.
@mmabrouk
mmabrouk force-pushed the credits-starter-seeding branch from a3fa961 to ac58713 Compare August 22, 2026 22:25
@mmabrouk
mmabrouk force-pushed the fix/provider-probe-stored-key branch from f6bcdbc to 6533af8 Compare August 22, 2026 22:25
mmabrouk added a commit that referenced this pull request Aug 22, 2026
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.
@mmabrouk
mmabrouk force-pushed the credits-starter-seeding branch from ac58713 to dfd33cc Compare August 22, 2026 22:29
@mmabrouk
mmabrouk force-pushed the fix/provider-probe-stored-key branch from 6533af8 to 9e3da5e Compare August 22, 2026 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant