Skip to content

test(inference): add gRPC provider-create-path coverage for built-in profiles #1941

New issue
New issue
Closed
Task
Closed
test(inference): add gRPC provider-create-path coverage for built-in profiles#1941
Task
Labels
retired:area:inferenceRetired; do not apply. See #5151 for item-specific migration to the current taxonomy.retired:area:providersRetired; do not apply. See #5151 for item-specific migration to the current taxonomy.state:staleInactive item at risk of automatic closure.topic:testingUnit, integration, regression, conformance, E2E tests, coverage, flakes, and test infrastructure.

Description

@st-gr
st-gr
opened on Jun 17, 2026

Important

Corrections verified against main @ 83284129 — see this comment.
The entrypoint named in the Scope section below (ProviderService::create_provider) does not exist; the real functions are create_provider_record{,_with_catalog}.
Scope item 2 is also blocked on a visibility change: create_provider_record (pub(super) in crate::grpc) and upsert_inference_route (private in crate::inference) are not visible to any common module.

Context

Follow-up to PR #1704 (merged), per @johntmyers's request.

The new route-resolution tests added in PR #1704 (upsert_cluster_route_* in crates/openshell-server/src/inference.rs) insert Provider records directly via store.put_message. That covers the routing logic but bypasses the public CLI path operators actually use:

openshell provider create --type aws-bedrock --credential ... --config BEDROCK_BASE_URL=...

If a built-in profile (aws-bedrock, deepinfra, nvidia, openai, anthropic, google-vertex-ai) drifts from what the gRPC provider create handler accepts — required vs. optional credential validation, discovery behaviour, defaulting — the existing tests won't catch it.

Scope

Add coverage that goes through openshell_server::grpc::provider::ProviderService::create_provider (or the equivalent public entrypoint) for each built-in profile:

  1. Positive create path: the documented provider create --type <profile> ... command shape produces a stored Provider record matching what resolve_provider_route expects.
  2. Cluster route end-to-end: chain create_provider → upsert_cluster_inference_route (with verify: false for protocols not yet supported by the validation probe — currently aws_bedrock_invoke, but this list is short).
  3. Negative paths: for each profile, the gRPC create_provider rejects malformed inputs (empty credential map where required, unknown profile type, etc.) with the same error shape the CLI surfaces.

Why this matters

Documentation in docs/sandboxes/manage-providers.mdx and docs/sandboxes/inference-routing.mdx shows specific provider create commands. Today nothing pins those down to behaviour. A test layer one level up from the route-resolution unit tests would catch docs/profile drift before users do.

Related

Activity

  1. github-actions
    added
    state:triage-neededOpened without agent diagnostics and needs triage
    on Jun 17, 2026

  2. matthewgrossman commented on Jul 29, 2026

    @matthewgrossman
    matthewgrossman
    on Jul 29, 2026
    Member

    📋 triage-agent

    Triage Assessment

    Classification: feature-valid

    Summary

    The requested provider-create contract coverage is a concrete and still-relevant testing task.

    Investigation

    Route tests in crates/openshell-server/src/inference.rs still persist provider records directly before calling route logic. grpc/provider.rs has substantial handler validation coverage, but it does not table-drive the documented inference profiles through handle_create_provider and then upsert_inference_route. PR #1704 supplied the Bedrock routing behavior, not this end-to-end contract coverage.

    Recommendation

    Keep the issue open as a Task. Prefer a table-driven suite for inference-capable built-in profiles, covering positive creation, route compatibility, and representative invalid inputs.

  3. matthewgrossman
    added
    retired:area:providersRetired; do not apply. See #5151 for item-specific migration to the current taxonomy.
    retired:area:inferenceRetired; do not apply. See #5151 for item-specific migration to the current taxonomy.
    topic:testingUnit, integration, regression, conformance, E2E tests, coverage, flakes, and test infrastructure.
    and removed
    state:triage-neededOpened without agent diagnostics and needs triage
    on Jul 29, 2026
  4. matthewgrossman
    added the
    Task
    issue type on Jul 29, 2026

  5. st-gr commented on Aug 4, 2026

    @st-gr
    st-gr
    on Aug 4, 2026
    ContributorAuthor

    Re-checked this against main @ 83284129. The gap is still real, but the issue text has gone stale in a way that would stop an implementer, so recording corrections here.

    Still valid

    • Route tests in crates/openshell-server/src/inference.rs still insert Provider records via store.put_message, bypassing the create path — 6 sites (lines 1236, 1316, 1391, 1443, 1499, 1534).
    • create_provider_record is called ~71 times across tests, but all ad hoc. There is still no table-driven pass over the built-in inference profiles.
    • The existing *built_in* tests cover profile management (import / update / delete / list), not "create a provider of each built-in type, then resolve a route".
    • Profile list unchanged: anthropic, aws-bedrock, deepinfra, google-vertex-ai, nvidia, openai. Both cited docs files still exist at their original paths.

    Corrections to the scope above

    1. The entrypoint named in this issue does not exist. I wrote openshell_server::grpc::provider::ProviderService::create_provider. The actual functions are:

    // crates/openshell-server/src/grpc/provider.rs:74
pub(super) async fn create_provider_record(
    store: &Store, workspace: &str, provider: Provider,
) -> Result<Provider, Status>

// crates/openshell-server/src/grpc/provider.rs:85
pub(super) async fn create_provider_record_with_catalog(
    store: &Store, catalog: &EffectiveProviderProfileCatalog,
    workspace: &str, provider: Provider,
) -> Result<Provider, Status>

    The _with_catalog variant is the better target for a table-driven suite — it takes the catalog explicitly instead of rebuilding it per call.

    2. The route function signature. Scope item 2 should target:

    // crates/openshell-server/src/inference.rs:219
async fn upsert_inference_route(
    store: &Store, workspace: &str, route_name: &str,
    provider_name: &str, model_id: &str, timeout_secs: u64,
    verify: bool,
) -> Result<UpsertedInferenceRoute, Status>

    The verify: false note in item 2 does map correctly — verify is derived from !req.no_verify.

    3. Blocker: the two functions cannot currently be called from the same module.

    mod grpc and mod inference are sibling private modules under the crate root (lib.rs:33, lib.rs:35):

    • create_provider_record is pub(super) → visible only within crate::grpc
    • upsert_inference_route has no visibility modifier → visible only within crate::inference

    So nothing in the crate can see both, and scope item 2 (chaining create → route) cannot be written as specified without first widening one of them to pub(crate). Worth deciding which, since it's a small API-surface change reviewers will want to be deliberate about. Items 1 and 3 are unaffected and can be written against create_provider_record_with_catalog inside the existing mod tests in grpc/provider.rs.

    Scope note

    The provider-profile system grew considerably after this issue was filed (custom profile import/lint/update/delete, AWS STS credential refresh). This issue should be read as covering built-in profiles only; custom-profile create coverage is a separate concern.

  6. github-actions commented on Aug 28, 2026

    @github-actions
    github-actionsBot
    on Aug 28, 2026 – with GitHub Actions

    This issue has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. Comment or remove the state:stale label to keep it open.

  7. github-actions
    added
    state:staleInactive item at risk of automatic closure.
    on Aug 28, 2026

  8. st-gr commented on Aug 30, 2026

    @st-gr
    st-gr
    on Aug 30, 2026
    ContributorAuthor

    Still relevant — re-verified against main @ 69a05ebb. Refreshing the references, since the line numbers in the earlier comments have gone stale again.

    The gap has widened, not closed. Provider records inserted directly via store.put_message in crates/openshell-server/src/inference.rs:

    checked sites
    2026-08-04 (83284129) 6
    today (69a05ebb) 27

    So route tests are still bypassing the create path, and the pattern has spread as the file grew.

    Entrypoints unchanged in name, moved in position — crates/openshell-server/src/grpc/provider.rs:

    :104  pub(super) async fn create_provider_record(..)
:116  pub(super) async fn create_provider_record_with_catalog(..)

    (were :74 / :85.)

    The *built_in* tests still cover profile management only, not create-then-route. Current set:

    • update_provider_profile_rejects_built_in_and_missing_profiles
    • list_provider_profiles_returns_built_in_profile_categories
    • import_provider_profile_allows_legacy_provider_type_ids_without_built_in_profiles

    None of these creates a provider of each built-in type and then resolves a route, which is what this issue asks for and what the triage assessment recommended keeping open as a Task.

    Could a maintainer drop state:stale? I do not have permission to remove the label myself.

  9. github-actions
    removed
    state:staleInactive item at risk of automatic closure.
    on Sep 1, 2026

  10. github-actions commented on Sep 16, 2026

    @github-actions
    github-actionsBot
    on Sep 16, 2026 – with GitHub Actions

    This issue has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. Comment or remove the state:stale label to keep it open.

  11. github-actions
    added
    state:staleInactive item at risk of automatic closure.
    on Sep 16, 2026

  12. johntmyers commented on Sep 28, 2026

    @johntmyers
    johntmyers
    on Sep 28, 2026
    Collaborator

    Built-in providers have been removed in 0.1.x, closing, if still relevant to 0.1.x please open a new issue.

  13. johntmyers
    closed this as completedon Sep 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    retired:area:inferenceRetired; do not apply. See #5151 for item-specific migration to the current taxonomy.retired:area:providersRetired; do not apply. See #5151 for item-specific migration to the current taxonomy.state:staleInactive item at risk of automatic closure.topic:testingUnit, integration, regression, conformance, E2E tests, coverage, flakes, and test infrastructure.

    Type

    Task

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions