test(e2e): cover credential-backed /v1/messages request - #33863
Conversation
Co-Authored-By: Ishaan Jaffer <155045088+ishaan-berri@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
|
Greptile SummaryThis PR adds a new end-to-end test that verifies stored credentials correctly resolve into a live
Confidence Score: 4/5Safe to merge; changes are confined to the e2e test harness and add no production code paths The test logic, teardown ordering, and client wiring are all correct. The only concerns are the overly narrow dict[str, str] types on CredentialCreateBody and a minor registry-metadata inconsistency — neither affects correctness of the new test or production behaviour. tests/e2e/models.py — the dict[str, str] type constraint on credential_values and credential_info could silently reject valid future harness usage
|
| Filename | Overview |
|---|---|
| tests/e2e/llm_translation/test_credential_messages_e2e.py | New e2e test covering credential-backed /v1/messages; teardown order, assertion logic, and lifecycle usage are all correct |
| tests/e2e/models.py | Adds CredentialCreateBody/CredentialCreateResponse and litellm_credential_name to LiteLLMParamsBody; credential_values and credential_info are typed dict[str, str] which is narrower than the actual API's plain dict |
| tests/e2e/proxy_client.py | Adds create_credential (unwrap on failure) and delete_credential (warn-only on failure) methods; consistent with existing harness patterns |
| tests/e2e/coverage_registry/mgmt.yaml | New mgmt.credential.new.serves_request registry row; source field references production endpoint file rather than the test file, unlike all other entries |
Reviews (1): Last reviewed commit: "test(e2e): cover credential-backed /v1/m..." | Re-trigger Greptile
| class CredentialCreateBody(BaseModel): | ||
| credential_name: str | ||
| credential_values: dict[str, str] | ||
| credential_info: dict[str, str] = {} |
There was a problem hiding this comment.
credential_values and credential_info typed too narrowly
Both fields are typed as dict[str, str], but the actual API model (CreateCredentialItem) uses plain dict — values can be any JSON type. Pydantic will reject a harness call the moment a credential value is non-string (e.g. a boolean or numeric timeout), even though the server would accept it fine. The repo-preferred wide type is dict[str, object].
| - {id: mgmt.config_override.hashicorp_vault.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "config_override_endpoints.py", rationale: "Vault integration (smoke)"} | ||
| - {id: mgmt.workflow.list.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "workflow_management_endpoints.py", rationale: "Workflow tracking (smoke)"} | ||
| - {id: mgmt.credential_migration.check.happy_path, module: mgmt, tier: P2, surface: api, assertions: [happy_path], source: "key_management_endpoints.py:4252", rationale: "Encryption migration (smoke)"} | ||
| - {id: mgmt.credential.new.serves_request, module: mgmt, tier: P1, surface: api, assertions: [serves_request], source: "credential_endpoints/endpoints.py:42", rationale: "Stored credential resolves into a deployment and serves a live /messages request"} |
There was a problem hiding this comment.
source field references production code, not the test file
Every other entry in this file sets source to a test-harness or test file (e.g. key_management_endpoints.py:4252, workflow_management_endpoints.py). This new entry sets source: "credential_endpoints/endpoints.py:42", which is the production endpoint. The test that covers this entry lives in llm_translation/test_credential_messages_e2e.py; pointing there would keep the registry consistent and make it easy to jump from the registry to the actual test.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Ishaan Jaffer <155045088+ishaan-berri@users.noreply.github.com>
Relevant issues
Linear ticket
Pre-Submission checklist
Screenshots / Proof of Fix
Captured against a local proxy at commit
eb36a94a5ec435fcba86613f9994caa894cfd476with Postgres + Redis, hitting the real Anthropic API (real $)Live test run
The same flow by hand (the request bodies reference
$ANTHROPIC_API_KEYso the key is never printed)The deployment carries no
api_keyof its own, so the 200 with assistant text proves the stored credential resolved into the requestType
✅ Test
Changes
We had no e2e coverage for the credentials feature actually serving traffic: creating a stored credential, attaching a model to it, and having a request resolve that credential at call time. This adds that as a single natural user scenario against
/v1/messagestests/e2e/llm_translation/test_credential_messages_e2e.pywalks it end to end: POST/credentialsstoring the real Anthropic key (read from the environment at runtime, never hardcoded), register a deployment whose only auth islitellm_credential_namepointing at that credential (the model carries noapi_keyof its own), then drive a real/v1/messagesrequest and assert an assistant message with non-empty text came back. Because the deployment has no key of its own, the call only succeeds if the stored credential resolved into the request, so the assertion actually exercises credential resolution rather than passing vacuouslySupporting harness additions
Teardown deletes the model before the credential so the deployment is never left referencing a deleted credential
Coverage registry gains one row,
mgmt.credential.new.serves_request, which the new test declares via@pytest.mark.coversOne thing surfaced while writing this: stored credential values are injected verbatim and do not go through the proxy's
os.environ/NAMEsecret resolution, even thoughlitellm_paramsvalues on a model config do. So a credential stored as{"api_key": "os.environ/ANTHROPIC_API_KEY"}reaches the provider literally and 401s, whereas the same value in a model'slitellm_paramsresolves. The test stores the real value (the realistic user path), so it does not depend on that behavior; flagging it separately in case the divergence between the two paths is unintendedQA runbook
curl -X POST http://localhost:4000/credentials -H "Authorization: Bearer sk-1234" -H "Content-Type: application/json" --data "{\"credential_name\":\"e2e-cred-demo\",\"credential_values\":{\"api_key\":\"$ANTHROPIC_API_KEY\"},\"credential_info\":{}}"curl -X POST http://localhost:4000/model/new -H "Authorization: Bearer sk-1234" -H "Content-Type: application/json" -d '{"model_name": "e2e-cred-messages", "litellm_params": {"model": "anthropic/claude-haiku-4-5", "litellm_credential_name": "e2e-cred-demo"}, "model_info": {}}'e2e-cred-messagesand expect a 200 with an assistant message:curl -X POST http://localhost:4000/v1/messages -H "Authorization: Bearer sk-1234" -H "anthropic-version: 2023-06-01" -H "Content-Type: application/json" -d '{"model": "e2e-cred-messages", "max_tokens": 16, "messages": [{"role": "user", "content": "reply with one word"}]}'Final Attestation
Link to Devin session: https://app.devin.ai/sessions/2b483d3d20c3428f852162b995ecfcd2
Requested by: @ishaan-berri