Add reasoning effort config - #44
Conversation
![proval-0e6d07[bot]](https://avatars.githubusercontent.com/in/4828992?s=60&v=4){kind=small}
|
|
||
| modelProviderId: integer().references(() => modelProviderTable.id), | ||
| modelName: text().notNull().default(""), | ||
| reasoningEffort: text(), |
There was a problem hiding this comment.
⚫️ [PROBLEM] Reasoning effort is accepted as free text end-to-end; a typo persists silently and breaks every later run for that repository
The column is unconstrained free text (reasoningEffort: text()), unlike the analogous prReviewOnPush enum right above it (packages/db/src/schema.ts:137), and nothing validates the value before it reaches the provider SDKs: the form field is a plain text input whose suggestion list is only advisory (apps/client/src/lib/components/organism/RepositoryForm.svelte:28, submit only trims at :291), the controller parses the request body with type-only casts and no runtime check (apps/api/src/api/repository/repository.controller.ts:24 and :70), and RepositoryService.normalizeUserPrompt (apps/api/src/api/repository/repository.service.ts:397) normalizes only userPrompt. The adapters then cast the stored string straight into the SDK unions (openai.ts:39, anthropic.ts:47), so a typo like "hig" or a provider-incompatible value is stored silently.
Since the agent loop treats provider 400s as retryable (apps/api/src/agent/llm/loop.ts:196), every later trigger for that repository retries 3x with backoff and produces no review or reply, with an error that points nowhere near this setting.
Fix: normalize and validate in RepositoryService.create/update mirroring normalizeUserPrompt — trim, lowercase, empty to null, reject values outside the allowed set — and extend the controller's existing error-prefix branch so this surfaces as a 400 with a clear message. Keep the client suggestion list as the single source of allowed values, and note the lists may differ per provider (the OpenAI and Anthropic params here use different keys, so a value one provider accepts may 400 the other).
const REASONING_EFFORTS = ["low", "medium", "high", "xhigh", "max"] as const;
private normalizeReasoningEffort(value: unknown): string | null {
const trimmed = typeof value === "string" ? value.trim().toLowerCase() : "";
if (!trimmed) {
return null;
}
if (!(REASONING_EFFORTS as readonly string[]).includes(trimmed)) {
throw new Error(`Reasoning effort must be one of: ${REASONING_EFFORTS.join(", ")}`);
}
return trimmed;
}
// call alongside data.userPrompt = this.normalizeUserPrompt(...) in create/update:
// cleanData.reasoningEffort = this.normalizeReasoningEffort(cleanData.reasoningEffort);
| const inputClass = twMerge( | ||
| "h-10 w-full rounded-lg border border-neutral-200 bg-neutral-50 px-4 text-sm outline-none dark:border-neutral-700 dark:bg-neutral-800", | ||
| disabled ? "cursor-not-allowed opacity-60" : "", | ||
| className, | ||
| )} | ||
| type={password ? "password" : "text"} | ||
| {onchange} | ||
| {name} | ||
| {onkeydown} /> | ||
| ); |
There was a problem hiding this comment.
⚫️ [PROBLEM] inputClass is a plain const, so reactive changes to disabled/class stop restyling this shared atom
In Svelte 5 runes a plain const referencing props is evaluated once at component init and never recomputes. The previous markup used an inline class={twMerge(...)}, which re-evaluated whenever disabled or the documented class prop changed; the refactor to a plain const drops that reactivity, while the surrounding conditional attributes (placeholder, id, bind) stay reactive — an asymmetry that is hard to debug when it bites.
No current consumer passes reactive disabled/class, so nothing is visibly broken today, but this atom is shared by ~30 forms and the first caller that toggles disabled (e.g. an input disabled while a request is in flight) will silently keep enabled styling. Declaring it with $derived restores parity with the original behavior.
const inputClass = $derived(
twMerge(
"h-10 w-full rounded-lg border border-neutral-200 bg-neutral-50 px-4 text-sm outline-none dark:border-neutral-700 dark:bg-neutral-800",
disabled ? "cursor-not-allowed opacity-60" : "",
className,
),
);
|
Thanks for this change — the reasoning effort knob is wired end-to-end with surprisingly few loose ends, and it was easy to trace from the form field all the way to both provider adapters. OverviewSummaryThis PR adds a per-repository reasoning effort setting: a new nullable Good points
Main Issues
Additional Notes
|
Deploying proval-demo with
|
| Latest commit: |
8e9b326
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://435f87be.proval-demo.pages.dev |
| Branch Preview URL: | https://feat-reasoning-effort.proval-demo.pages.dev |
Deploying proval with
|
| Latest commit: |
8e9b326
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3c3f8dad.proval.pages.dev |
| Branch Preview URL: | https://feat-reasoning-effort.proval.pages.dev |
| ...(openAiTools.length > 0 ? { tools: openAiTools, tool_choice: "auto" as const } : {}), | ||
| ...(config.reasoningEffort | ||
| ? { | ||
| reasoning_effort: config.reasoningEffort as OpenAI.Chat.ChatCompletionCreateParams["reasoning_effort"], |
There was a problem hiding this comment.
🚨 [CRITICAL] Shared enum includes values OpenAI rejects; the cast hides the mismatch
🚨 [CRITICAL] The new shared reasoningEffortValueList (packages/db/src/schema.ts:101) adds "xhigh" and "max", and validation at normalizeReasoningEffort happily persists them (repository.service.ts:416-421), but OpenAI's reasoning_effort union only accepts none/minimal/low/medium/high. This as cast silently funnels invalid values into the request body, so a user picking "max" in the new Select compiles fine and then every review/reply run for that repository fails with a provider 400 — exactly the failure mode the enum was meant to close.
Could be wrong if you intentionally rely on newer SDK/API versions accepting these values; if so, a comment would help.
Fix: instead of casting, narrow at the boundary — map/whitelist the permitted subset per provider and omit (or throw) for unsupported members, e.g. accept only "minimal"|"low"|"medium"|"high" here.
...(config.reasoningEffort
? { reasoning_effort: config.reasoningEffort satisfies OpenAiReasoningEffort }
: {}),
// with `satisfies`, adding an enum member OpenAI doesn't support becomes a compile error
| effort: config.reasoningEffort as NonNullable< | ||
| Anthropic.Messages.MessageCreateParams["output_config"] | ||
| >["effort"], | ||
| }, |
There was a problem hiding this comment.
🚨 [CRITICAL] Anthropic adapter also casts through an invalid shared-enum value
🚨 [CRITICAL] The same cast pattern applies here: the shared enum's "none", "minimal", "xhigh" and "max" members (packages/db/src/schema.ts:101) are not valid Anthropic output_config.effort values (only low/medium/high are). This cast hides the mismatch from the compiler, so a repository stored with reasoningEffort "none" or "max" plus an Anthropic provider sends an invalid effort value and fails at request time instead of at save time. Add a small per-provider whitelist/switch in the adapter so unsupported members are rejected or ignored explicitly.
|
|
||
| modelProviderId: integer().references(() => modelProviderTable.id), | ||
| modelName: text().notNull().default(""), | ||
| reasoningEffort: text({ enum: reasoningEffortValueList }), |
There was a problem hiding this comment.
⚫️ [PROBLEM] Enum column changed without a matching migration
⚫️ [PROBLEM] Making reasoningEffort an enum column changes what drizzle-kit emits for repository.reasoning_effort(CHECK constraint), but no new migration was added in this push. The last migration on record, packages/db/src/migration/0043_damp_jack_murdock.sql, is still a plain ALTER TABLE ... ADD reasoning_effort text;. The checked-in migration set now disagrees with the code schema, so fresh deployments won't have the constraint and the next drizzle-kit generate will surface an unexpected pending diff.
Fix: either generate a migration reflecting the CHECK, or add a comment explaining that the enum is intentionally TS-only (service-level normalizeReasoningEffort already validates writes).
|
Thanks for the quick follow-up — the free-text validation gap from the last round is now closed end-to-end, and the InputText reactivity regression is gone too. This push is easy to reason about; the remaining concerns are all about one shared list being broader than what the providers accept. OverviewSummaryThis push converts the free-text reasoning effort into a single source of truth: The main residual risk: the shared enum contains values the providers don't accept ("xhigh", "max" for OpenAI; "none", "minimal", "xhigh", "max" for Anthropic), and the adapters' casts hide that from the compiler, so those values pass save-time validation and later fail every run with a provider 400 — the same failure mode the previous round flagged, now at a narrower level. Good points
Main Issues
Additional Notes
|
|
@proval-0e6d07 I know that it's too broad options but some of open weight models with OpenAI/Anthropic API supports it. It's on purpose |
|
Understood — thanks for the explanation. If the intent is to support open-weight models served through OpenAI/Anthropic-compatible APIs (which often accept a wider effort range than the first-party vendors), then the "enum is broader than the provider union" concern is mostly moot, and the Two small suggestions to make that intent durable, so future reviewers don't re-flag it:
That would resolve the 🚨 item and the "Additional Notes" point in my previous review with documentation only — no code change needed. The two remaining ⚫️ items (missing drizzle migration for the enum column, and the unmapped |
1 participant
close #39