-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(combos): carry forced default effort from PR #4054 #4714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b388a29
4dc3bcb
e38cd5a
62b745c
53267be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import type { OcxComboDefaultEffort, OcxComboReasoningEffortMode, OcxComboTarget, OcxConfig } from "../types"; | ||
| import { resolveEffortAtOrBelow } from "../reasoning-effort"; | ||
| import type { OcxComboDefaultEffort, OcxComboDefaultEffortMode, OcxComboReasoningEffortMode, OcxComboTarget, OcxConfig } from "../types"; | ||
| import { isCodexReasoningEffort, resolveEffortAtOrBelow } from "../reasoning-effort"; | ||
| import { resolveComboId } from "./types"; | ||
|
|
||
| const warnedUnsupportedDefaults = new Set<string>(); | ||
|
|
@@ -60,22 +60,29 @@ export function concreteComboRequestBody( | |
| defaultEffort: OcxComboDefaultEffort | null, | ||
| targetReasoningEfforts: readonly string[] | undefined, | ||
| reasoningEffortMode: OcxComboReasoningEffortMode = "strict", | ||
| defaultEffortMode: OcxComboDefaultEffortMode = "fallback", | ||
| ): Record<string, unknown> { | ||
| const clone = structuredClone(body) as Record<string, unknown>; | ||
| clone.model = `${target.provider}/${target.model}`; | ||
| if (defaultEffortMode === "force" && (!defaultEffort || !isCodexReasoningEffort(defaultEffort))) { | ||
| throw new Error("force combo default effort requires a valid defaultEffort"); | ||
| } | ||
| if (targetReasoningEfforts?.length === 0 | ||
| || (reasoningEffortMode === "adaptive" && targetReasoningEfforts === undefined)) { | ||
| stripUnsupportedReasoningControls(clone); | ||
| } | ||
| if (!defaultEffort) return clone; | ||
| if (!defaultEffort || !isCodexReasoningEffort(defaultEffort)) return clone; | ||
| const reasoning = clone.reasoning; | ||
| const needsDefault = reasoning === undefined || ( | ||
| reasoning | ||
| && typeof reasoning === "object" | ||
| && !Array.isArray(reasoning) | ||
| && !Object.prototype.hasOwnProperty.call(reasoning, "effort") | ||
| ); | ||
| if (!needsDefault) return clone; | ||
| const reasoningRecord = reasoning && typeof reasoning === "object" && !Array.isArray(reasoning) | ||
| ? reasoning as Record<string, unknown> | ||
| : undefined; | ||
| const hasEffort = reasoningRecord !== undefined | ||
| && Object.prototype.hasOwnProperty.call(reasoningRecord, "effort"); | ||
| const callerEffort = reasoningRecord?.effort; | ||
| const validCallerEffort = typeof callerEffort === "string" && isCodexReasoningEffort(callerEffort); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Override
Use Proposed fix-import { isCodexReasoningEffort, resolveEffortAtOrBelow } from "../reasoning-effort";
+import { isCodexReasoningEffort, isDeclaredReasoningEffort, resolveEffortAtOrBelow } from "../reasoning-effort";
...
- const validCallerEffort = typeof callerEffort === "string" && isCodexReasoningEffort(callerEffort);
+ const validCallerEffort = typeof callerEffort === "string" && isDeclaredReasoningEffort(callerEffort);🤖 Prompt for AI Agents |
||
| const needsDefault = reasoning === undefined || (reasoningRecord !== undefined && !hasEffort); | ||
| const shouldForce = defaultEffortMode === "force" && validCallerEffort; | ||
|
Comment on lines
+82
to
+84
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a client supplies the valid Useful? React with 👍 / 👎. |
||
| if (!needsDefault && !shouldForce) return clone; | ||
| // Picker availability treats an unknown ladder as a wildcard, but runtime | ||
| // injection stays fail-closed until this concrete target advertises support. | ||
| // | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,17 +80,20 @@ function sparseComboConfig<T extends { | |
| waitForCooldownMs?: number; | ||
| imageInput?: "auto" | "disabled"; | ||
| reasoningEffortMode?: "strict" | "adaptive"; | ||
| }>(combo: T): Omit<T, "cooldownMs" | "waitForCooldownMs" | "imageInput" | "reasoningEffortMode"> & { | ||
| defaultEffortMode?: "fallback" | "force"; | ||
| }>(combo: T): Omit<T, "cooldownMs" | "waitForCooldownMs" | "imageInput" | "reasoningEffortMode" | "defaultEffortMode"> & { | ||
| cooldownMs?: number; | ||
| waitForCooldownMs?: number; | ||
| imageInput?: "disabled"; | ||
| reasoningEffortMode?: "adaptive"; | ||
| defaultEffortMode?: "force"; | ||
| } { | ||
| const { | ||
| cooldownMs, | ||
| waitForCooldownMs, | ||
| imageInput, | ||
| reasoningEffortMode, | ||
| defaultEffortMode, | ||
| ...rest | ||
| } = combo; | ||
| return { | ||
|
|
@@ -101,6 +104,7 @@ function sparseComboConfig<T extends { | |
| : {}), | ||
| ...(imageInput === "disabled" ? { imageInput: "disabled" as const } : {}), | ||
| ...(reasoningEffortMode === "adaptive" ? { reasoningEffortMode: "adaptive" as const } : {}), | ||
| ...(defaultEffortMode === "force" ? { defaultEffortMode: "force" as const } : {}), | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -170,6 +174,11 @@ export async function handleComboRoutes(ctx: ManagementContext): Promise<Respons | |
| ...(!Object.hasOwn(requestedCombo, "waitForCooldownMs") && previous?.waitForCooldownMs !== undefined | ||
| ? { waitForCooldownMs: previous.waitForCooldownMs } | ||
| : {}), | ||
| // The dashboard does not expose this advanced CLI/API policy. Preserve it when | ||
| // a GUI round-trip omits the field instead of silently downgrading to fallback. | ||
| ...(!Object.hasOwn(requestedCombo, "defaultEffortMode") && previous?.defaultEffortMode !== undefined | ||
| ? { defaultEffortMode: previous.defaultEffortMode } | ||
| : {}), | ||
|
Comment on lines
+179
to
+181
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a combo previously saved in force mode, the dashboard sends Useful? React with 👍 / 👎. |
||
| }; | ||
| const error = comboConfigError(id, effectiveCombo, config.providers, { | ||
| requireEnabledTarget: true, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
ocx combo setupdates an existing forced combo's targets or strategy without--effort, this branch retainsdefaultEffortMode: "force", but the replacement payload contains nodefaultEffort. The management validator therefore rejects an otherwise ordinary update because force mode requires a default. Either carry the existing default along with the mode or downgrade to fallback whenever--effortis omitted.Useful? React with 👍 / 👎.