[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-go studio · code · diff
✅ stagehand-typescript studio · code · diff
✅ stagehand-ruby studio · code · diff
✅ stagehand-csharp studio · code · diff
✅ stagehand-java studio · code · diff
✅ stagehand-kotlin studio · code · diff
|
![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}
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.
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.
| ...requestModelClientOptions, | ||
| }; | ||
|
|
||
| if (ctx.modelApiKey && modelClientOptions.apiKey === undefined) { |
There was a problem hiding this comment.
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).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/server-v3/src/lib/InMemorySessionStore.ts, line 219:
<comment>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`).</comment>
<file context>
@@ -216,16 +216,7 @@ export class InMemorySessionStore implements SessionStore {
- modelClientOptions.apiKey === undefined &&
- !hasExplicitAwsCredentials
- ) {
+ if (ctx.modelApiKey && modelClientOptions.apiKey === undefined) {
modelClientOptions.apiKey = ctx.modelApiKey;
}
</file context>
| if (ctx.modelApiKey && modelClientOptions.apiKey === undefined) { | |
| const hasExplicitAwsCredentials = | |
| modelClientOptions.accessKeyId !== undefined || | |
| modelClientOptions.secretAccessKey !== undefined || | |
| modelClientOptions.sessionToken !== undefined; | |
| if ( | |
| ctx.modelApiKey && | |
| modelClientOptions.apiKey === undefined && | |
| !hasExplicitAwsCredentials | |
| ) { |
✅ Addressed in 93410e3
There was a problem hiding this comment.
This says it was addressed but maybe the fix was reverted? As is:
if (ctx.modelApiKey && modelClientOptions.apiKey === undefined) {
modelClientOptions.apiKey = ctx.modelApiKey;
}
a request using accessKeyId / secretAccessKey can still get a top-level apiKey added on top I believe
There was a problem hiding this comment.
Addressed in 2eca76af — added a hasBedrockCredentials guard so requests with accessKeyId/secretAccessKey skip the apiKey injection.
- 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.
There was a problem hiding this comment.
1 issue found across 6 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/test/integration/v3/agentExecute.test.ts">
<violation number="1" location="packages/server-v3/test/integration/v3/agentExecute.test.ts:335">
P2: Ensure browser harness cleanup still runs when `endSession` fails; the current `await` order can skip `browserHarness.close()` and leak test resources.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Resolve merge conflicts in: - packages/core/lib/v3/types/public/api.ts: keep providerConfig + main's updated headers description - packages/server-v3/openapi.v3.yaml: keep providerConfig schema entries + main's headers descriptions - packages/server-v3/src/routes/v1/sessions/_id/observe.ts: combine imports (ModelConfiguration from bedrock + ObserveOptions/Variables from main) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap endSession in nested try/finally so browserHarness.close() always runs even if endSession throws, preventing test resource leaks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use delete instead of underscore-prefixed destructuring to exclude properties from rest spreads, avoiding no-unused-vars lint errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| ...requestModelClientOptions, | ||
| }; | ||
|
|
||
| if (ctx.modelApiKey && modelClientOptions.apiKey === undefined) { |
There was a problem hiding this comment.
This says it was addressed but maybe the fix was reverted? As is:
if (ctx.modelApiKey && modelClientOptions.apiKey === undefined) {
modelClientOptions.apiKey = ctx.modelApiKey;
}
a request using accessKeyId / secretAccessKey can still get a top-level apiKey added on top I believe
|
For strictness, could add a superRefine that requires: This would better follow "parse, don't validate", but maybe falls under "taste". Otherwise, LGTM. |
Prevents Bedrock requests with accessKeyId/secretAccessKey from getting a spurious apiKey added via the x-model-api-key header fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shrey150
force-pushed
the
shrey/bedrock-auth-passthrough
branch
from
2eca76a to
e287c46
Compare
Extends the existing superRefine in addProviderConfigValidation to catch mutually exclusive auth modes at parse time. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed in 5f06f70c — extended the existing |
There was a problem hiding this comment.
2 issues found across 1 file (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:61">
P2: Mutual-exclusion validation for `apiKey` and Bedrock credentials is only enforced for `ModelConfig`, not for `SessionStartRequest.modelClientOptions`, so conflicting auth can still be accepted at session start.</violation>
<violation number="2" location="packages/core/lib/v3/types/public/api.ts:70">
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 3 violation: this new Bedrock auth mutual-exclusion validation changes public API request behavior, but there is no integration test covering the new rejection case. Add a server-v3 integration test that sends `model` with `apiKey` plus Bedrock `providerConfig.options.accessKeyId/secretAccessKey` and asserts a 400 with the validation error.
(Based on your team's feedback about checking providerConfig-related coverage in server-v3 integration tests first.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "accessKeyId" in providerConfig.options && | ||
| "secretAccessKey" in providerConfig.options; | ||
|
|
||
| if (hasApiKey && hasAwsCredentials) { |
There was a problem hiding this comment.
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 3 violation: this new Bedrock auth mutual-exclusion validation changes public API request behavior, but there is no integration test covering the new rejection case. Add a server-v3 integration test that sends model with apiKey plus Bedrock providerConfig.options.accessKeyId/secretAccessKey and asserts a 400 with the validation error.
(Based on your team's feedback about checking providerConfig-related coverage in server-v3 integration tests first.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/types/public/api.ts, line 70:
<comment>Rule 3 violation: this new Bedrock auth mutual-exclusion validation changes public API request behavior, but there is no integration test covering the new rejection case. Add a server-v3 integration test that sends `model` with `apiKey` plus Bedrock `providerConfig.options.accessKeyId/secretAccessKey` and asserts a 400 with the validation error.
(Based on your team's feedback about checking providerConfig-related coverage in server-v3 integration tests first.) </comment>
<file context>
@@ -57,6 +57,24 @@ function addProviderConfigValidation<
+ "accessKeyId" in providerConfig.options &&
+ "secretAccessKey" in providerConfig.options;
+
+ if (hasApiKey && hasAwsCredentials) {
+ ctx.addIssue({
+ code: z.ZodIssueCode.custom,
</file context>
There was a problem hiding this comment.
Integration tests exist in packages/server-v3/tests/integration/v3/start.test.ts covering session start with typed providerConfig (acceptance + rejection of mismatched config). Server-v4 doesn't have providerConfig plumbing, so no analogous tests needed there.
There was a problem hiding this comment.
Got it—I'll check the server-v3 integration tests at packages/server-v3/tests/integration/v3/start.test.ts and avoid asking for server-v4 providerConfig tests.
| }); | ||
| } | ||
|
|
||
| const hasApiKey = "apiKey" in value && typeof value.apiKey === "string"; |
There was a problem hiding this comment.
P2: Mutual-exclusion validation for apiKey and Bedrock credentials is only enforced for ModelConfig, not for SessionStartRequest.modelClientOptions, so conflicting auth can still be accepted at session start.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/types/public/api.ts, line 61:
<comment>Mutual-exclusion validation for `apiKey` and Bedrock credentials is only enforced for `ModelConfig`, not for `SessionStartRequest.modelClientOptions`, so conflicting auth can still be accepted at session start.</comment>
<file context>
@@ -57,6 +57,24 @@ function addProviderConfigValidation<
});
}
+
+ const hasApiKey = "apiKey" in value && typeof value.apiKey === "string";
+ const hasAwsCredentials =
+ providerConfig?.provider === "bedrock" &&
</file context>
There was a problem hiding this comment.
By design — SessionStartRequest.modelClientOptions is the raw transport bag and doesn't carry apiKey at the top level (the fallback API key comes from the x-model-api-key header, not the request body). The mutual-exclusion validation is enforced on ModelConfigSchema which is the canonical shape used in act/extract/observe/agent requests where both apiKey and providerConfig can appear together.
There was a problem hiding this comment.
Thanks for the feedback! I've updated an existing learning with this new information.
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