Conversation
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 197951eeac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const userTemperature = this.clientOptions?.temperature; | ||
| const temperature = userTemperature ?? (isKimi ? 1 : undefined); |
There was a problem hiding this comment.
createChatCompletion still accepts options.temperature, and upstream callers depend on it (for example packages/core/lib/inference.ts sets temperature to 0.1/1 for extraction and metadata calls), but this change now derives temperature only from this.clientOptions?.temperature. In the common case where clientOptions.temperature is unset, all non-Kimi AISDK calls now send undefined and silently ignore the per-request sampling setting, which broadens this opus-specific tweak into a cross-model behavior regression that can change output quality and reproducibility.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 3/5
- There is a concrete regression risk in
packages/core/lib/v3/llm/aisdk.ts: per-requestoptions.temperaturefromChatCompletionOptionsappears to be ignored, changing runtime behavior for callers that rely on per-call overrides. - The issue is moderate severity (6/10) with high confidence (8/10), so this is more than a cosmetic concern and could be user-facing in generation quality/consistency.
- Pay close attention to
packages/core/lib/v3/llm/aisdk.ts- restore or verify per-request temperature precedence so call-level settings are not silently dropped.
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/llm/aisdk.ts">
<violation number="1" location="packages/core/lib/v3/llm/aisdk.ts:153">
P2: Per-request `options.temperature` is now silently ignored. The old code read from `options.temperature` (the per-call `ChatCompletionOptions`); the new code reads only from `this.clientOptions?.temperature` (the client-level config set at construction). If any caller passes a per-request temperature, the aisdk client will discard it — unlike every other LLM client in this codebase. Consider falling back through both: `userTemperature ?? options.temperature ?? (isKimi ? 1 : undefined)`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant App as Application / Example
participant Client as AISdkClient / Wrapped
participant SDK as AI SDK (generateObject)
Note over App,Client: Initialization with model "anthropic/claude-opus-4-7"
App->>Client: new Stagehand({ model, temperature? })
App->>Client: Request generation (e.g. generateObject)
Note over Client: CHANGED: Temperature Resolution Logic
Client->>Client: Check this.clientOptions.temperature
alt User temperature explicitly configured in clientOptions
Note right of Client: Use user-configured temperature
else Model ID includes "kimi"
Note right of Client: Use 1 (Provider requirement)
else Default Path
Note right of Client: NEW: Use undefined (Drops temperature from payload)
end
Client->>SDK: generateObject({ ..., temperature })
Note over SDK: If temperature is undefined, provider defaults apply
SDK-->>Client: Object Response
Client-->>App: Result
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| const isKimi = this.model.modelId.includes("kimi"); | ||
| const temperature = isKimi ? 1 : options.temperature; | ||
| const userTemperature = this.clientOptions?.temperature; | ||
| const temperature = userTemperature ?? (isKimi ? 1 : undefined); |
There was a problem hiding this comment.
P2: Per-request options.temperature is now silently ignored. The old code read from options.temperature (the per-call ChatCompletionOptions); the new code reads only from this.clientOptions?.temperature (the client-level config set at construction). If any caller passes a per-request temperature, the aisdk client will discard it — unlike every other LLM client in this codebase. Consider falling back through both: userTemperature ?? options.temperature ?? (isKimi ? 1 : undefined).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/llm/aisdk.ts, line 153:
<comment>Per-request `options.temperature` is now silently ignored. The old code read from `options.temperature` (the per-call `ChatCompletionOptions`); the new code reads only from `this.clientOptions?.temperature` (the client-level config set at construction). If any caller passes a per-request temperature, the aisdk client will discard it — unlike every other LLM client in this codebase. Consider falling back through both: `userTemperature ?? options.temperature ?? (isKimi ? 1 : undefined)`.</comment>
<file context>
@@ -147,9 +147,10 @@ export class AISdkClient extends LLMClient {
const isKimi = this.model.modelId.includes("kimi");
- const temperature = isKimi ? 1 : options.temperature;
+ const userTemperature = this.clientOptions?.temperature;
+ const temperature = userTemperature ?? (isKimi ? 1 : undefined);
// Resolve reasoning effort: user-configured > default "none" for GPT-5.x sub-models
</file context>
| const temperature = userTemperature ?? (isKimi ? 1 : undefined); | |
| const temperature = userTemperature ?? options.temperature ?? (isKimi ? 1 : undefined); |
There was a problem hiding this comment.
this is up to discussion - why would we need to pass temperature per call?
There was a problem hiding this comment.
Thanks for raising it—happy to align with your approach. Do you prefer enforcing temperature only at the client level with no per-call overrides?
There was a problem hiding this comment.
1 issue 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/understudy/context.ts">
<violation number="1" location="packages/core/lib/v3/understudy/context.ts:1070">
P1: This URL check breaks valid popup flows that remain on `about:blank`, causing `awaitActivePage()` to time out or return the previous page instead of the newly opened tab.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| const p = this.pagesByTarget.get(newestTid); | ||
| if (p && newestTs >= this._lastPopupSignalAt) return p; | ||
| if (p && newestTs >= this._lastPopupSignalAt) { | ||
| // Wait until the page has a real URL (not blank/initial state) |
There was a problem hiding this comment.
P1: This URL check breaks valid popup flows that remain on about:blank, causing awaitActivePage() to time out or return the previous page instead of the newly opened tab.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/understudy/context.ts, line 1070:
<comment>This URL check breaks valid popup flows that remain on `about:blank`, causing `awaitActivePage()` to time out or return the previous page instead of the newly opened tab.</comment>
<file context>
@@ -1066,7 +1066,11 @@ export class V3Context {
const p = this.pagesByTarget.get(newestTid);
- if (p && newestTs >= this._lastPopupSignalAt) return p;
+ if (p && newestTs >= this._lastPopupSignalAt) {
+ // Wait until the page has a real URL (not blank/initial state)
+ const url = p.url();
+ if (url && url !== "about:blank" && url !== ":") return p;
</file context>
There was a problem hiding this comment.
@miguelg719 it is not, but CI was failing and this fixed it. Should I nuke it? its not related.
2 participants
why
Claude Opus 4.7 rejects
temperatureas a deprecated parameter. Our AI SDK client could still forward a temperature value from Stagehand's default inference settings, which madeanthropic/claude-opus-4-7requests fail even when the user had not explicitly configured temperature.what changed
packages/core/lib/v3/llm/aisdk.tsto only sendtemperaturewhen the user explicitly configures it, while preserving the existingkimioverride to1.packages/evals/lib/AISdkClientWrapped.tsso eval behavior matches runtime behavior.packages/core/examples/example.tsto useanthropic/claude-opus-4-7so the affected path is easy to exercise.test plan
packages/core/examples/example.tswithanthropic/claude-opus-4-7and confirm the request succeeds without sending the deprecatedtemperatureparameter.