fix(cli-generator): fail fast when required credentials are missing

[devin-ai-integration]

Description

Stack 3/5 ("Benchling CLI fixes", split out of #17620). Stacked on #17622.

With no BENCHLING_API_KEY/client creds set, the CLI sent an unauthenticated request and then interpreted the server's 401. It now errors before dispatch when the OpenAPI security requirements say credentials are required.

Changes Made

• auth/provider.rs: EndpointAuthMetadata::requires_credentials() — Some(reqs) non-empty with no {} alternative → required; None → server decides; Some([]) → explicit anonymous; any {} alternative → optional.
• auth/error.rs: missing_credentials_error(provider) builds the "credentials are missing. Set BENCHLING_API_KEY, ..." message from credential_hints(); 401/403 handling reuses it when no credential was present (supplied-but-rejected creds keep the server-rejection message).
• executor.rs (build_http_request):

  let missing = auth_metadata.requires_credentials() && !auth_provider.has_credentials_for(auth_metadata);
  request = auth_provider.apply(request, auth_metadata)?;   // resolution errors (denied keychain, unreadable file) win
  if missing { return Err(missing_credentials_error(..)); }

• Changelog fix-fail-fast-missing-credentials.yml; regenerated seed/cli/cli-basic-auth.

Testing

• Unit tests added/updated: test_execute_method_fails_fast_when_required_credentials_missing (target 127.0.0.1:1, asserts auth error and no network attempt), test_execute_method_unreadable_credentials_not_reported_as_missing; cargo test --lib 1972 pass
• pnpm seed test --generator cli --fixture cli-basic-auth --skip-scripts --local 2/2
• Manual testing completed (benchling dna-sequence list with no env → exit 2, no request)

Link to Devin session: https://app.devin.ai/sessions/d3e6c0a2eab24903ae7765c8a4c54032
Open in Devin Desktop: https://app.devin.ai/desktop/session/d3e6c0a2eab24903ae7765c8a4c54032?variant=devin

---

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[nitpickybot]

AI Review Summary

Refactor extracts missing_credentials_error and adds a pre-flight requires_credentials() guard in build_http_request. Logic looks sound; a couple of concerns: the error message text is now reused for a pre-flight failure where "Access denied" is misleading, and the new tests appear to be placed outside the mod tests block (line numbers put them after its closing brace), which will pull tokio/test deps into non-test builds if not #[cfg(test)].

• 🟡 1 warning(s)
• 🔵 2 suggestion(s)

---

To request another review, comment /ai-review on this pull request.

[nitpickybot]

🟡 warning

These new #[tokio::test] fns land after the closing } of mod tests (line 12429), i.e. at module top level, not inside a #[cfg(test)] block. Unless the enclosing file is itself cfg-gated, this compiles test-only helpers (DeniedKeychain, execute_method test harness) into normal builds. Move them inside mod tests — and check the generator template so the seed outputs stay consistent.

[nitpickybot]

🔵 suggestion

Now that this message is also used pre-flight (no request sent), the "Access denied." prefix is inaccurate — nothing denied anything yet. Consider dropping the prefix here and letting the 401/403 path prepend it, so the fail-fast error reads e.g. "Authentication credentials are missing. Set BENCHLING_API_KEY."

[nitpickybot]

🔵 suggestion

Worth noting in the comment (or asserting in a test) that has_credentials_for must be side-effect free/cheap — it's now invoked on every request for every auth'd endpoint before apply. If any provider implementation triggers a keychain prompt or a token fetch in has_credentials_for, this doubles that work per request.

[devin-ai-integration]

Devin Review found 2 potential issues.

[devin-ai-integration]

🟡 Credential-store failures look like missing credentials

With an unreadable stored credential, RoutingAuthProvider::apply skips the failing child because its probe returned false. The CLI reports missing credentials instead of the storage failure.

Prompt for agents

The fail-fast path in generators/cli/sdk/src/openapi/executor.rs expects auth_provider.apply to surface credential resolution errors before returning the missing-credentials error. Composite providers in generators/cli/sdk/src/auth/compose.rs first call has_credentials or has_credentials_for and skip children that return false. Built-in keyring-backed providers intentionally collapse read failures to false during probing, so their fallible apply methods never run under AnyAuthProvider, AllAuthProvider, or RoutingAuthProvider. Preserve the distinction between an absent credential and a credential-resolution failure through composition. Add tests using the normal composed provider shape, including a denied keyring read on a required routed endpoint.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Correct that composite providers (Any/All/Routing) probe with has_credentials* and skip children that return false, and keyring providers collapse read errors to false during probing — so a denied keyring read behind a composite is reported as "missing". That collapse predates this PR (the old 401 path produced the same message), and fixing it means a fallible probe (try_has_credentials -> Result<bool>) through the trait and all composites. Flagging to the requester as a follow-up rather than expanding this PR.

[devin-ai-integration]

🟡 Changing credentials defeat the preflight check

If a credential file or closure changes between has_credentials_for and apply, the saved missing result no longer matches the request. Valid requests can fail, while credentialless requests can proceed.

Prompt for agents

The new preflight decision in generators/cli/sdk/src/openapi/executor.rs is calculated before apply, while AuthProvider implementations and composition wrappers resolve or probe the credential again. AuthCredentialSource explicitly permits file rereads and arbitrary closure reinvocation, so those observations need not agree. Redesign request authentication so the decision to fail is based on the same credential resolution that applies authentication, for example by returning an applied/missing outcome from a fallible provider operation or by resolving each credential once per request. Ensure composite providers preserve OR/AND/routing semantics without repeated mutable-source probes. Test false-then-present and present-then-false closure suppliers.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[devin-ai-integration]

Acknowledged, but this is inherent to AuthCredentialSource allowing re-reads and is not introduced here — before this PR the same probe/apply pair already disagreed in RoutingAuthProvider and in the 401 handler. Window is between two calls in the same request; a credential appearing/vanishing in that gap is a race the prior code had too. Making resolution single-shot per request means changing the AuthProvider trait; deferring to the requester as a follow-up rather than growing this PR.