fix(omp): limit thinking levels to model's reported efforts#2171
fix(omp): limit thinking levels to model's reported efforts#2171bendavid wants to merge 1 commit into
Conversation
The omp provider exposed all six thinking levels (off/minimal/low/ medium/high/xhigh) whenever a model had reasoning enabled, ignoring the per-model thinking config that omp reports via RPC. The OmpModelSchema didn't parse the thinking field at all, so the model's efforts subset and defaultLevel were discarded. Parse model.thinking (efforts/defaultLevel/effortMap) in OmpModelSchema and filter the thinking options in mapOmpModel to only the model's reported efforts. The default is the reported defaultLevel when it's in the filtered set, otherwise the first (lowest) effort. Older omp versions that don't report thinking.efforts keep getting the full set. Also fix createSession to not hardcode 'medium' as the thinking fallback when the client doesn't send a thinkingOptionId — pass undefined so omp uses its own model default instead of overriding it with a level that may not even be in the model's effort set.
2b599f4 to
25a3cd2
Compare
|
| Filename | Overview |
|---|---|
| packages/server/src/server/agent/providers/omp/agent.ts | Extracted resolveOmpThinkingConfig to compute thinking options from per-model config; inconsistent default ("off" vs "medium") in the all-unknown-efforts fallback branch; also adds mapThinkingOptionWithDefault which duplicates mapThinkingOption. |
| packages/server/src/server/agent/providers/omp/rpc-types.ts | Adds OmpModelThinkingSchema (with passthrough) and wires it into OmpModelSchema; straightforward schema addition, no issues. |
| packages/server/src/server/agent/providers/omp/map-omp-model.test.ts | New unit tests for mapOmpModel — good coverage of the happy path, backward compat, and edge cases, but the all-unknown-efforts test doesn't assert defaultThinkingOptionId (leaving the "off" default bug uncaught), and mapOmpModel was only exported to satisfy these tests. |
| packages/server/src/server/agent/providers/omp/agent.test.ts | Updated launch-config test to expect no --thinking flag by default; added test verifying --thinking xhigh is emitted when explicitly specified. Both changes are correct. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[mapOmpModel] --> B{model.reasoning?}
B -- false/absent --> C[thinkingOptions: undefined]
B -- true --> D{efforts exists and non-empty?}
D -- No --> E[Full option set, default = medium]
D -- Yes --> F[Filter by effort IDs]
F --> G{Any known efforts matched?}
G -- No, all unknown --> H[Full option set, default = off - differs from path E]
G -- Yes --> I[Filtered options]
I --> J{defaultLevel in filtered set?}
J -- Yes --> K[default = defaultLevel]
J -- No/absent --> L[default = first option id]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[mapOmpModel] --> B{model.reasoning?}
B -- false/absent --> C[thinkingOptions: undefined]
B -- true --> D{efforts exists and non-empty?}
D -- No --> E[Full option set, default = medium]
D -- Yes --> F[Filter by effort IDs]
F --> G{Any known efforts matched?}
G -- No, all unknown --> H[Full option set, default = off - differs from path E]
G -- Yes --> I[Filtered options]
I --> J{defaultLevel in filtered set?}
J -- Yes --> K[default = defaultLevel]
J -- No/absent --> L[default = first option id]
Reviews (1): Last reviewed commit: "fix(omp): limit thinking levels to model..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const effortSet = new Set(efforts); | ||
| const filtered = OMP_THINKING_OPTIONS.filter((option) => effortSet.has(option.id)); | ||
| const options = filtered.length > 0 ? filtered : [...OMP_THINKING_OPTIONS]; | ||
| const reportedDefault = model.thinking?.defaultLevel; | ||
| const defaultThinkingOptionId = | ||
| reportedDefault && options.some((option) => option.id === reportedDefault) | ||
| ? reportedDefault | ||
| : (options[0]?.id ?? DEFAULT_OMP_THINKING_LEVEL); |
There was a problem hiding this comment.
When efforts is non-empty but every ID is unrecognized (e.g. a future omp version uses "ultra"/"turbo"), options becomes the full set via the filtered.length > 0 ? filtered : [...OMP_THINKING_OPTIONS] fallback — identical to the early-return path for !efforts || efforts.length === 0. However, the early-return path returns defaultThinkingOptionId: DEFAULT_OMP_THINKING_LEVEL ("medium"), while this path resolves defaultThinkingOptionId to options[0]?.id which is "off". For a model with reasoning: true, silently defaulting to "off" (no thinking) is materially wrong, and the unit test for this branch ("falls back to the full set when every reported effort is unknown") does not assert defaultThinkingOptionId, so the divergence goes uncaught.
| function mapThinkingOptionWithDefault( | ||
| option: (typeof OMP_THINKING_OPTIONS)[number], | ||
| isDefault: boolean, | ||
| ): AgentSelectOption { | ||
| const mapped: AgentSelectOption = { | ||
| id: option.id, | ||
| label: option.label, | ||
| description: option.description, | ||
| }; | ||
| if (isDefault) { | ||
| mapped.isDefault = true; | ||
| } | ||
| return mapped; | ||
| } |
There was a problem hiding this comment.
mapThinkingOptionWithDefault does what mapThinkingOption already does, differing only in how isDefault is sourced. mapThinkingOption reads option.isDefault from the static array; mapThinkingOptionWithDefault accepts it as a parameter. Instead of a second function called from one place, mapThinkingOption could accept an optional override — or both callers could use the same function by computing isDefault at the call site. As written, any future change to the shape of AgentSelectOption must be applied in both places.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
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!
| import { describe, expect, test } from "vitest"; | ||
|
|
||
| import { mapOmpModel } from "./agent.js"; | ||
| import type { OmpModel } from "./rpc-types.js"; | ||
|
|
||
| function baseModel(overrides: Partial<OmpModel> = {}): OmpModel { | ||
| return { | ||
| provider: "pioneer", | ||
| id: "canada-quant/glm-5.2", | ||
| name: "GLM-5.2", |
There was a problem hiding this comment.
mapOmpModel was exported specifically so this test file could reach it (per the PR description). Per the project's test discipline, tests should cross the same module interface as callers — not import private implementation files directly. If the omp module has a designed public entry point, mapOmpModel should either be exposed from it or the tests should exercise the behavior through OmpAgentClient.getModels() / session creation paths. Exporting an internal function to satisfy a test is a sign the test is asserting the wrong thing.
Rule Used: # Code Review Pattern Reference: Slop, Tests, Feat... (source)
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!
1 participant
Problem
When running Paseo with an omp model configured with a limited subset of thinking levels (e.g. only
highandxhigh), the full set of six thinking levels (off/minimal/low/medium/high/xhigh) still appeared in the thinking dropdown menu.Root cause
The omp provider's
mapOmpModelexposed all six thinking levels whenever a model hadreasoning: true, ignoring the per-model thinking config that omp reports via RPC. TheOmpModelSchemadidn't parse thethinkingfield at all, so the model'seffortssubset anddefaultLevelwere discarded.Additionally,
createSessionhardcodedmediumas the thinking fallback when the client didn't send athinkingOptionId— overriding omp's own model default with a level that may not even be in the model's effort set.Fix
rpc-types.ts— AddedOmpModelThinkingSchema(mode/efforts/defaultLevel/effortMap, passthrough) and wired it intoOmpModelSchemaas thethinkingfield.agent.ts— RewrotemapOmpModelto delegate toresolveOmpThinkingConfig(model):reasoning: false/absent → no thinking options (unchanged).reasoning: truewith nothinking.efforts(older omp) → full set,mediumdefault (back-compat).reasoning: truewithefforts→ only the options whose id appears inefforts, in canonical order. Default isthinking.defaultLevelif it's in the filtered set, otherwise the first (lowest) effort. If none of the reported efforts are known ids, falls back to the full set.mapOmpModelfor unit testing.agent.ts(createSession) — Changed the thinking fallback fromDEFAULT_OMP_THINKING_LEVEL("medium") toundefined, sobuildOmpLaunchdoesn't emit--thinkingand omp uses its own model default when the client doesn't specify a thinking option.Verification
map-omp-model.test.tscovering: filtered subset, full set when no config, full set wheneffortsempty, omitted whenreasoningfalse/absent,defaultLevelnot in efforts → first option, nodefaultLevel→ first option, all-unknown efforts → full set, id/label/metadata preservation.agent.test.tslaunch-configuration test (no--thinkingflag when no thinking option specified) and added a test verifying--thinking xhighIS passed when explicitly specified.efforts: ["high","xhigh"],defaultLevel: "xhigh") now maps to exactly["high","xhigh"]withxhighas default, instead of all six options withmediumas default.