[STG-1573] Add providerOptions for extensible model auth#1822
[STG-1573] Add providerOptions for extensible model auth#1822
Conversation
|
✱ Stainless preview buildsThis PR will update the Edit this comment to update it. It will appear in the SDK's changelogs. ✅ stagehand-typescript studio · code · diff
✅ stagehand-ruby studio · code · diff
⚡ stagehand-python studio · conflict
✅ stagehand-kotlin studio · code · diff
✅ stagehand-go studio · code · diff
✅ stagehand-csharp studio · code · diff
✅ stagehand-java studio · code · diff
✅ stagehand-openapi studio · code · diff
✅ stagehand-php studio · code · diff
This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 21 files
Confidence score: 3/5
- There is some regression risk from configuration merge behavior changes, so this is not a no-risk merge despite looking contained to option-preparation paths (all flagged issues are medium severity at 6/10).
- Most severe impact is in
packages/server-v4/src/lib/header.ts:getRequestModelConfigcan ignorebody.modelNamewhenbody.options.modelis an object, which may select the wrong model/provider for clients using mixed old/new payload fields. packages/core/lib/v3/api.tsandpackages/server-v3/src/lib/InMemorySessionStore.tsboth have spread-order override hazards (apiKeyfallback being clobbered, andmodelClientOptions.modelNameoverriding explicitmodelName), which could cause auth/config mismatches at runtime.- Pay close attention to
packages/server-v4/src/lib/header.ts,packages/core/lib/v3/api.ts,packages/server-v3/src/lib/InMemorySessionStore.ts- merge-order and fallback precedence can silently change effective model configuration.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server-v4/src/lib/header.ts">
<violation number="1" location="packages/server-v4/src/lib/header.ts:49">
P2: `getRequestModelConfig` short-circuits on `body.options.model` existing as an object, dropping the old fallback to `body.modelName`. If a client sends `{ options: { model: { provider: "bedrock" } }, modelName: "some-model" }`, the old `getModelName` would find `"some-model"` via the `body.modelName` fallback, but the new code returns `undefined` because `getRequestModelConfig` returns the model object (which lacks `modelName`) without ever checking `body.modelName`.</violation>
</file>
<file name="packages/core/lib/v3/api.ts">
<violation number="1" location="packages/core/lib/v3/api.ts:660">
P2: Spread order in `prepareModelConfig` can clobber the resolved `apiKey` fallback. `resolveModelClientOptions` already merges `model` into its result and then adds a fallback `apiKey`—but `...model` is re-spread on top, overwriting that key if `model` carries an explicit `apiKey: undefined`. Swap the spread order so `resolveModelClientOptions` output takes precedence:
```ts
return {
...model,
...this.resolveModelClientOptions(provider, model),
};
This ensures the fallback apiKey (and any Bedrock credentials) aren't silently clobbered.
Architecture diagram
sequenceDiagram
participant SDK as Stagehand SDK (V3)
participant API as API Client
participant Srv as Server (API Route)
participant Store as Session Store
participant AWS as AWS Bedrock
Note over SDK, AWS: Session Initialization (Start)
SDK->>API: init(modelClientOptions)
API->>API: NEW: toSessionStartModelClientOptions()<br/>(Serializes Bedrock region/keys)
API->>Srv: POST /sessions/start (JSON body)
Note right of API: Body includes NEW: modelClientOptions<br/>(accessKeyId, secretAccessKey, etc.)
Srv->>Store: NEW: createSession(modelClientOptions)
Store->>Store: Persist provider settings for session
Store-->>Srv: sessionId
Srv-->>API: 200 OK
API-->>SDK: Session Ready
Note over SDK, AWS: Action Execution (e.g., act / extract)
SDK->>API: act({ input, options })
API->>API: NEW: prepareModelConfig()<br/>(Resolves default vs override creds)
API->>Srv: POST /act (JSON body)
Note right of API: Body contains options.model object<br/>with AWS credentials
Srv->>Srv: NEW: getRequestModelConfig()<br/>(Parses body for Bedrock keys)
alt NEW: Credentials in body
Srv->>Srv: Use body credentials
else CHANGED: Credentials missing in body
Srv->>Srv: Fallback to x-model-api-key header
end
Srv->>Store: getV3Options(requestContext)
Store->>Store: NEW: Merge Session settings + Request overrides
Store-->>Srv: V3Options (with resolved BedrockProviderSettings)
Srv->>AWS: Execute Model Call
Note right of Srv: Request authorized using passed-through<br/>AWS Access Key / Session Token
AWS-->>Srv: Model Response
Srv-->>API: Streamed Action Results
API-->>SDK: Result
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR adds AWS Bedrock as a first-class model provider by threading provider-specific credential objects ( Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant SDK as Stagehand SDK (v3.ts)
participant Client as StagehandAPIClient
participant Server as Server (start.ts)
participant Store as InMemorySessionStore
participant AI as AI Provider (Bedrock / OpenAI)
Note over SDK,AI: Session Initialization
SDK->>Client: init({ modelName, modelApiKey?, modelClientOptions? })
Client->>Client: merge modelApiKey into this.modelClientOptions
Client->>Server: POST /sessions/start<br/>body: { modelName, modelClientOptions }<br/>header: x-model-api-key (if apiKey present)
Server->>Store: createSession({ modelName, modelClientOptions })
Store-->>Server: sessionId
Server-->>Client: { sessionId, available }
Note over SDK,AI: Per-Request Tool Call (e.g. act)
SDK->>Client: act({ input, options? })
Client->>Client: getDefaultModelConfig()<br/>→ { modelName, ...modelClientOptions }
Client->>Server: POST /act<br/>body: { options: { model: { modelName, credentials... } } }<br/>header: x-model-api-key (if apiKey present)
Server->>Server: getRequestModelConfig(request)<br/>→ ctx.modelConfig
Server->>Store: getOrCreateStagehand(sessionId, ctx)
Store->>Store: buildV3Options:<br/>merge params.modelClientOptions + ctx.modelConfig<br/>apply ctx.modelApiKey if no explicit creds
Store->>AI: Stagehand call with merged credentials
AI-->>Store: result
Store-->>Server: result
Server-->>Client: streaming response
Last reviewed commit: a15158e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/core/lib/v3/api.ts
Outdated
| const requestBody: Api.SessionStartRequest = { | ||
| modelName, | ||
| modelClientOptions: | ||
| this.toSessionStartModelClientOptions(modelClientOptions), |
There was a problem hiding this comment.
Session body omits apiKey merged from modelApiKey
toSessionStartModelClientOptions is called with the raw modelClientOptions parameter rather than the fully-merged this.modelClientOptions. When a caller provides both modelApiKey and modelClientOptions: { region: "..." }, the apiKey that was merged into this.modelClientOptions is not included in the session start body under modelClientOptions.
As a result, params.modelClientOptions stored on the server side never contains the apiKey for this case — the server instead reconstructs it from the x-model-api-key header on each subsequent call via the ctx.modelApiKey → modelClientOptions.apiKey assignment in buildV3Options. This currently works because the header is still sent, but the two shapes diverge unexpectedly: the client's local this.modelClientOptions has apiKey, the server's stored params.modelClientOptions does not.
Consider calling toSessionStartModelClientOptions(this.modelClientOptions) to ensure the body is always consistent with the local merged state:
modelClientOptions:
this.toSessionStartModelClientOptions(this.modelClientOptions),
| delete requestOptions.provider; | ||
|
|
||
| const headers = requestOptions.headers; | ||
| if ( | ||
| headers !== undefined && | ||
| (headers === null || | ||
| typeof headers !== "object" || | ||
| Array.isArray(headers) || | ||
| "then" in headers || | ||
| Object.values(headers as Record<string, unknown>).some( | ||
| (value) => typeof value !== "string", | ||
| )) | ||
| ) { | ||
| delete requestOptions.headers; | ||
| } |
There was a problem hiding this comment.
Headers instance slips through the serialization filter
The guard correctly catches null, arrays, and Promises ("then" in headers), but a Headers instance from the Fetch API passes every check here:
typeof headers !== "object"→falseArray.isArray(headers)→false"then" in headers→falseObject.values(headers as Record<string, unknown>).some(v => typeof v !== "string")→ returns[]becauseHeadersproperties are not enumerable viaObject.values(), so.some(...)isfalse
JSON.stringify(new Headers({ "X-Custom": "value" })) then silently produces {}, discarding all headers without warning.
Since BedrockProviderSettings.headers is typed as HeadersInit (which includes Headers), this is a realistic input path. You can tighten the guard to also reject non-plain-object Headers instances:
typeof headers !== "object" ||
Array.isArray(headers) ||
headers instanceof Headers ||
"then" in headers ||
Object.values(headers as Record<string, unknown>).some(
(value) => typeof value !== "string",
)
The server already stores modelClientOptions from session start and merges them into subsequent requests. This removes the redundant client-side persistence (getDefaultModelConfig, resolveModelClientOptions, modelName/modelClientOptions fields) so that language SDKs don't each need to reimplement this logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/api.ts">
<violation number="1">
P2: Custom agent: **Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test**
These API client changes (removing default model injection on follow-up requests and simplifying `prepareModelConfig` to drop Bedrock credential merging) alter the wire format sent by act/extract/observe/agent calls. Per project rules, breaking changes to `packages/core/lib/v3/api.ts` must be covered by at least one integration test under `packages/server-v3/test/` or `packages/server-v4/test/`. Only a unit test (`packages/core/tests/unit/api-client-model-config.test.ts`) was added — consider adding an integration test that verifies end-to-end model config forwarding (e.g., session-start with `modelClientOptions` followed by an act/extract call that confirms the server receives the expected model configuration).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -29,7 +29,7 @@ import type { | |||
| SerializableResponse, | |||
There was a problem hiding this comment.
P2: Custom agent: Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test
These API client changes (removing default model injection on follow-up requests and simplifying prepareModelConfig to drop Bedrock credential merging) alter the wire format sent by act/extract/observe/agent calls. Per project rules, breaking changes to packages/core/lib/v3/api.ts must be covered by at least one integration test under packages/server-v3/test/ or packages/server-v4/test/. Only a unit test (packages/core/tests/unit/api-client-model-config.test.ts) was added — consider adding an integration test that verifies end-to-end model config forwarding (e.g., session-start with modelClientOptions followed by an act/extract call that confirms the server receives the expected model configuration).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/api.ts, line 431:
<comment>These API client changes (removing default model injection on follow-up requests and simplifying `prepareModelConfig` to drop Bedrock credential merging) alter the wire format sent by act/extract/observe/agent calls. Per project rules, breaking changes to `packages/core/lib/v3/api.ts` must be covered by at least one integration test under `packages/server-v3/test/` or `packages/server-v4/test/`. Only a unit test (`packages/core/tests/unit/api-client-model-config.test.ts`) was added — consider adding an integration test that verifies end-to-end model config forwarding (e.g., session-start with `modelClientOptions` followed by an act/extract call that confirms the server receives the expected model configuration).</comment>
<file context>
@@ -462,7 +428,7 @@ export class StagehandAPIClient {
model: agentConfig.model
? this.prepareModelConfig(agentConfig.model)
- : this.getDefaultModelConfig(),
+ : undefined,
executionModel: agentConfig.executionModel
? this.prepareModelConfig(agentConfig.executionModel)
</file context>
shrey150
changed the title
Instead of adding AWS-specific fields (region, accessKeyId, secretAccessKey, sessionToken) as top-level fields in the shared ModelConfigObjectSchema, move them into a providerOptions bag that is passed through to the AI SDK provider constructor. This keeps the shared schema generic (apiKey, baseURL, headers, providerOptions) and avoids vendor-specific logic in the server. TypeScript types provide autocomplete for known providers (Bedrock, Vertex) via a union type on ClientOptions.providerOptions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/types/public/api.ts">
<violation number="1" location="packages/core/lib/v3/types/public/api.ts:80">
P2: Custom agent: **Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test**
This schema change (removing `region`/`accessKeyId`/`secretAccessKey`/`sessionToken` in favor of `providerOptions`) is a breaking change to the public API surface. No integration test under `packages/server-v4/test/integration/` validates that the new `providerOptions` field (or the session-level `modelClientOptions` carrying it) round-trips correctly through the server routes. Add at least one integration test (e.g., in `start.test.ts` or a new file) that sends a session-start request with `modelClientOptions.providerOptions` and verifies it is preserved end-to-end.</violation>
</file>
<file name="packages/server-v3/src/lib/InMemorySessionStore.ts">
<violation number="1" location="packages/server-v3/src/lib/InMemorySessionStore.ts:219">
P2: Removing the `hasExplicitAwsCredentials` guard means `ctx.modelApiKey` will now be injected into `modelClientOptions` even when AWS credentials (`accessKeyId`, `secretAccessKey`, `sessionToken`) are present. If a hosted environment sets a default model API key via the `x-model-api-key` header and a request supplies Bedrock AWS credentials without an explicit `apiKey`, the injected key could confuse the model client's auth path. Consider either keeping the guard or adding a comment explaining why it's safe to remove (e.g., the downstream client now correctly prioritizes AWS credentials over `apiKey`).</violation>
</file>
<file name="packages/core/lib/v3/llm/LLMProvider.ts">
<violation number="1" location="packages/core/lib/v3/llm/LLMProvider.ts:121">
P2: Shallow spread of `providerOptions` over `restOptions` silently drops the top-level `headers` when both objects carry a `headers` key (e.g., `GoogleVertexProviderSettings` also has `headers`). Consider merging the `headers` fields explicitly to avoid silent data loss.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- LLMProvider: verify providerOptions is flattened into AI SDK constructor args (bedrock with region, empty providerOptions, no providerOptions) - API client: verify prepareModelConfig attaches apiKey for same-provider overrides and works without apiKey for Bedrock per-call overrides with providerOptions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Regenerate openapi.v3.yaml and openapi.v4.yaml to include modelClientOptions with providerOptions in SessionStartRequest - Remove Record<string, unknown> fallback from providerOptions union type so TypeScript autocomplete works for Bedrock and Vertex provider options Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shrey150
changed the title
Hosted deployments (core) don't persist modelClientOptions server-side. Store the serialized session model config in the SDK client and resend it as options.model on each act/observe/ extract/agentExecute call when no explicit model is provided. This is a simple passthrough — no vendor logic, no complex merging. The SDK just echoes back what the server needs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/api.ts">
<violation number="1" location="packages/core/lib/v3/api.ts:223">
P2: Redundant call to `toSessionStartModelClientOptions` — `serializedMco` computed three lines above is the same result and should be reused in the request body.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
3 issues found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server-v3/openapi.v3.yaml">
<violation number="1" location="packages/server-v3/openapi.v3.yaml:94">
P2: Copy-paste error: `baseURL` example in all four Bedrock-specific schemas shows `https://api.openai.com/v1` instead of a Bedrock endpoint. This will confuse consumers of the API docs / generated SDK examples.</violation>
<violation number="2" location="packages/server-v3/openapi.v3.yaml:202">
P2: `GenericModelConfigObject.provider` enum includes `bedrock`, which overlaps with the dedicated Bedrock schemas in the `anyOf`. A Bedrock request supplying only `modelName` validates against the generic branch, silently skipping the Bedrock-required `providerOptions`. Consider removing `bedrock` from the generic enum (it is covered by the two Bedrock-specific schemas) or adding a `discriminator` on the `provider` field.</violation>
</file>
<file name="packages/core/lib/v3/types/public/api.ts">
<violation number="1" location="packages/core/lib/v3/types/public/api.ts:339">
P1: Custom agent: **Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test**
Breaking API schema changes (`ModelConfigObjectSchema` → union, `ModelClientOptionsSchema` → union, `providerOptions` narrowed from open record to strict typed union) have no corresponding integration tests in `packages/server-v3/test/` or `packages/server-v4/test/`. Add integration tests to `start.test.ts` (or a new test file) in both server packages covering at least: (1) session start with Bedrock API-key auth, (2) session start with Bedrock AWS credentials, and (3) session start with generic/non-Bedrock provider options.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server-v3/src/lib/InMemorySessionStore.ts">
<violation number="1" location="packages/server-v3/src/lib/InMemorySessionStore.ts:223">
P2: This check hardcodes AWS credential field names in the vendor-agnostic session store, contradicting the PR's extensibility design. Consider a generic signal instead—e.g. a `skipApiKey` boolean in `providerOptions`—so future non-API-key providers don't require server-side changes.
Additionally, this non-obvious interaction between `providerOptions` and `apiKey` injection deserves an inline comment explaining why AWS credentials suppress the API key fallback.
(Based on your team's feedback about documenting complex logic and edge-case interactions inline.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/server-v3/openapi.v3.yaml">
<violation number="1">
P1: The provider enum is missing `vertex` and several other supported AI SDK providers, which will cause OpenAPI validation to incorrectly reject valid models.</violation>
<violation number="2" location="packages/server-v3/openapi.v3.yaml:83">
P2: The `ProviderOptions` schema will reject arbitrary fields for new providers, contradicting the PR's claim that future providers require "zero schema changes".</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
3 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/types/public/api.ts">
<violation number="1" location="packages/core/lib/v3/types/public/api.ts:262">
P2: Use a nested apiKey path when validating `SessionStartRequestSchema`; the current hardcoded `['apiKey']` points to the wrong field.</violation>
<violation number="2" location="packages/core/lib/v3/types/public/api.ts:289">
P1: Custom agent: **Any breaking changes to Stagehand REST API client / server implementation must be covered by an integration test under packages/server/test**
Rule 2 requires integration coverage for changed public API request-shape behavior, but these new Bedrock validation branches were added without corresponding server integration tests (especially the new rejection paths). Add `packages/server-*/test/integration/*` cases that exercise the new Bedrock validation outcomes.</violation>
</file>
<file name="packages/core/tests/unit/model-config-schema.test.ts">
<violation number="1" location="packages/core/tests/unit/model-config-schema.test.ts:114">
P3: This test is scoped to `SessionStartRequestSchema` but validates `ModelClientOptionsSchema`, so it doesn't actually verify session-start acceptance for generic provider options.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
3 participants
Summary
providerOptionsbag to model config schema for provider-specific auth (Bedrock, Vertex, future providers) without polluting the shared schema with vendor-specific fieldsmodelApiKeyoptional so providers with non-API-key auth (e.g. AWS SigV4) can work without itmodelClientOptionsfrom session start and reuses them on subsequent act/observe/extract/execute calls — SDKs don't need to reimplement this logicmodelClientOptions(includingproviderOptions) in the session start request body; server merges stored + per-request configWhat this enables
region,accessKeyId,secretAccessKey,sessionTokenviaproviderOptionsproject,location,googleAuthOptionsviaproviderOptionsproviderOptions— zero schema or server changes neededArchitecture
Linked PRs
Testing
pnpm run build:esm && npx vitest run dist/esm/tests/unit/api-client-model-config.test.js dist/esm/tests/unit/llm-provider.test.js dist/esm/tests/unit/public-api/llm-and-agents.test.jsscratchpad/smoke-test-provider-options.ts🤖 Generated with Claude Code