Skip to content

fix(inbox): validate persisted model before one-click cloud tasks #2728

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.

Sign up for GitHub

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

Open
andrewm4894 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(inbox): validate persisted model before one-click cloud tasks #2728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/code/src/renderer/desktop-services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,14 @@ container.bind<ReportModelResolver>(REPORT_MODEL_RESOLVER).toConstantValue({
async resolveDefaultModel(
apiHost: string,
adapter: "claude" | "codex",
preferredModel?: string | null,
): Promise<string | undefined> {
try {
const options = await hostTrpcClient.agent.getPreviewConfigOptions.query({
apiHost,
adapter,
});
return selectModelFromOptions(options);
return selectModelFromOptions(options, preferredModel);
} catch (error) {
reportModelResolverLog.warn("Failed to resolve default model", {
error,
Expand Down
6 changes: 6 additions & 0 deletions packages/core/src/inbox/identifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,15 @@ export const LINEAR_OAUTH_FLOW = Symbol.for(
);

export interface ReportModelResolver {
/**
* Resolve the model id to use for a cloud task. `preferredModel` (e.g. the
* persisted last-used model) is honoured only if the gateway still offers it;
* otherwise the adapter's server default is returned.
*/
resolveDefaultModel(
apiHost: string,
adapter: "claude" | "codex",
preferredModel?: string | null,
): Promise<string | undefined>;
}

Expand Down
56 changes: 56 additions & 0 deletions packages/core/src/inbox/reportTaskCreation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { describe, expect, it } from "vitest";
import {
type PreviewConfigOption,
selectModelFromOptions,
} from "./reportTaskCreation";

function modelOption(
currentValue: string,
available: string[],
): PreviewConfigOption {
return {
id: "model",
category: "model",
type: "select",
currentValue,
options: available.map((value) => ({ value })),
};
}

describe("selectModelFromOptions", () => {
it("returns the server default when no preferred model is given", () => {
const options = [modelOption("claude-opus-4-8", ["claude-opus-4-8"])];
expect(selectModelFromOptions(options)).toBe("claude-opus-4-8");
});

it("honours the preferred model when the gateway still offers it", () => {
const options = [
modelOption("claude-opus-4-8", ["claude-opus-4-8", "claude-sonnet-4-6"]),
];
expect(selectModelFromOptions(options, "claude-sonnet-4-6")).toBe(
"claude-sonnet-4-6",
);
});

it("falls back to the server default when the preferred model is no longer offered", () => {
// The persisted model (e.g. a de-listed fable) is not in the available
// options, so it must not be returned — otherwise the run 403s.
const options = [modelOption("claude-opus-4-8", ["claude-opus-4-8"])];
expect(selectModelFromOptions(options, "claude-fable-5")).toBe(
"claude-opus-4-8",
);
});

it("ignores an empty preferred model", () => {
const options = [modelOption("claude-opus-4-8", ["claude-opus-4-8"])];
expect(selectModelFromOptions(options, "")).toBe("claude-opus-4-8");
expect(selectModelFromOptions(options, null)).toBe("claude-opus-4-8");
});

it("returns undefined when there is no model option", () => {
const options: PreviewConfigOption[] = [
{ id: "mode", category: "mode", type: "select", currentValue: "plan" },
];
expect(selectModelFromOptions(options, "claude-opus-4-8")).toBeUndefined();
});
Comment on lines +20 to +55

@greptile-apps greptile-apps Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Tests could be parameterised

The five it(...) cases for selectModelFromOptions all follow the pattern (options, preferredModel?) → expectedResult and differ only in input/output values. Per the team's preference, these should use it.each (or equivalent) so a new model-availability variant only adds a table row rather than a full it block.

Context Used: Do not attempt to comment on incorrect alphabetica... (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/core/src/inbox/reportTaskCreation.test.ts
Line: 20-55

Comment:
**Tests could be parameterised**

The five `it(...)` cases for `selectModelFromOptions` all follow the pattern `(options, preferredModel?) → expectedResult` and differ only in input/output values. Per the team's preference, these should use `it.each` (or equivalent) so a new model-availability variant only adds a table row rather than a full `it` block.

**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

How can I resolve this? If you propose a fix, please make it concise.

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!

});
29 changes: 27 additions & 2 deletions packages/core/src/inbox/reportTaskCreation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,47 @@
import type { TaskCreationInput } from "@posthog/shared";

/** A selectable choice, either flat or wrapped in a labelled group. */
export interface PreviewConfigChoice {
value?: string;
options?: PreviewConfigChoice[];
}

/** Minimal shape of a preview-config option we scan for the default model. */
export interface PreviewConfigOption {
id?: string;
category?: string;
type?: string;
currentValue?: string | boolean | null;
options?: PreviewConfigChoice[];
}

/** Pick the default model id out of the agent's preview-config options, if present. */
/**
* Pick the model id out of the agent's preview-config options.
*
* When `preferredModel` is supplied (e.g. the user's persisted last-used model)
* it is honoured only if it is still one of the gateway's available models;
* otherwise we fall back to the server default (`currentValue`). One-click cloud
* flows pass their persisted model here so a stale id the gateway no longer
* offers can't slip through — without the check the run fails with a gateway 403
* (e.g. a previously-selected model that was later de-listed for the org).
*/
export function selectModelFromOptions(
options: PreviewConfigOption[],
preferredModel?: string | null,
): string | undefined {
const modelOption = options.find(
(o) => o.id === "model" || o.category === "model",
);
if (modelOption?.type !== "select") {
return undefined;
}
if (
preferredModel &&
modelOption.options?.some((o) => o.value === preferredModel)

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Flatten grouped model options before validating

When the preview-config model list is returned as grouped select options, this check only inspects the top-level group objects, so a valid preferred or pinned model inside a group is treated as unavailable and the one-click task silently falls back to the server default. The schema and existing TaskInput guard support grouped options via flattening, so this no longer mirrors the picker for grouped model lists; please flatten or recurse before comparing the preferred model.

Useful? React with 👍 / 👎.

) {
return preferredModel;
}
Comment on lines +39 to +43

@greptile-apps greptile-apps Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Shallow option lookup misses nested model groups

PreviewConfigChoice is declared to support recursive nesting (options?: PreviewConfigChoice[]), and the comment in the interface says "either flat or wrapped in a labelled group." However the lookup modelOption.options?.some((o) => o.value === preferredModel) only inspects the top level. For a grouped structure like [{ options: [{ value: "claude-opus-4-8" }] }], o.value is undefined for the outer entry and the preferred model would not be found, causing an unnecessary fallback to the server default. If the gateway can return grouped options, this check should recurse or flatten first.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/core/src/inbox/reportTaskCreation.ts
Line: 39-43

Comment:
**Shallow option lookup misses nested model groups**

`PreviewConfigChoice` is declared to support recursive nesting (`options?: PreviewConfigChoice[]`), and the comment in the interface says "either flat or wrapped in a labelled group." However the lookup `modelOption.options?.some((o) => o.value === preferredModel)` only inspects the top level. For a grouped structure like `[{ options: [{ value: "claude-opus-4-8" }] }]`, `o.value` is `undefined` for the outer entry and the preferred model would not be found, causing an unnecessary fallback to the server default. If the gateway can return grouped options, this check should recurse or flatten first.

How can I resolve this? If you propose a fix, please make it concise.

if (
modelOption?.type === "select" &&
typeof modelOption.currentValue === "string" &&
modelOption.currentValue
) {
Expand Down
15 changes: 15 additions & 0 deletions packages/core/src/inbox/signalReportTaskService.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,21 @@ describe("SignalReportTaskService", () => {
expect(result.status).toBe("created");
});

it("forwards the override to the resolver as a preference, not a hard selection", async () => {
// The resolver validates the override against the gateway's available
// models, so it must receive it rather than the service short-circuiting.
const { service, modelResolver } = makeService();
await service.createSignalReportTask(
makeInput({ modelOverride: "claude-sonnet" }),
vi.fn(),
);
expect(modelResolver.resolveDefaultModel).toHaveBeenCalledWith(
expect.any(String),
"claude",
"claude-sonnet",
);
});

it("aborts with missing-model when no model can be resolved", async () => {
const { service, createTask } = makeService(
{},
Expand Down
10 changes: 7 additions & 3 deletions packages/core/src/inbox/signalReportTaskService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,13 @@ export class SignalReportTaskService {
}

const apiHost = getCloudUrlFromRegion(input.cloudRegion);
const model =
input.modelOverride ??
(await this.modelResolver.resolveDefaultModel(apiHost, input.adapter));
// The override is a preference: the resolver keeps it only if the gateway
// still offers it, otherwise it falls back to the server default.
const model = await this.modelResolver.resolveDefaultModel(
apiHost,
input.adapter,
input.modelOverride,
);
if (!model) {
return { status: "missing-model" };
}
Expand Down
10 changes: 7 additions & 3 deletions packages/ui/src/features/home/hooks/useRunWorkstreamAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,17 @@ export function useRunWorkstreamAction() {
void (async () => {
try {
// The cloud runtime requires a model: action-pinned, then last-used,
// then the adapter's server default.
// then the adapter's server default. The preferred candidate is only
// honoured if the gateway still offers it (the resolver validates it),
// so a stale persisted/pinned id can't reach the run and 403.
const adapter = action.adapter ?? lastUsedAdapter;
let model = action.model ?? lastUsedModel ?? undefined;
if (!model && cloudRegion) {
const preferredModel = action.model ?? lastUsedModel ?? undefined;
let model = preferredModel;
if (cloudRegion) {
model = await modelResolver.resolveDefaultModel(
getCloudUrlFromRegion(cloudRegion),
adapter,
preferredModel,
);
}
if (!model) {
Comment on lines +107 to 115

@greptile-apps greptile-apps Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Transient resolver failure now hard-blocks runs that previously succeeded

Before this PR, resolveDefaultModel was only called when !model, so a transient network error on getPreviewConfigOptions only blocked users with no persisted model — users with a lastUsedModel were unaffected. Now the resolver is always called when cloudRegion is set, but both desktop-services.ts and resolveDefaultModel.ts swallow all exceptions and return undefined. A single getPreviewConfigOptions timeout therefore turns model = undefined, surfaces "Couldn't start task", and forces a fallback to the task-input screen — even when preferredModel was a currently-valid id.

The same regression exists in useInboxCloudTaskRunner.ts via the resolveDefaultModel wrapper. Consider model = resolvedModel ?? preferredModel at both call sites so a gateway outage degrades gracefully (same risk profile as the old code) rather than failing hard.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/ui/src/features/home/hooks/useRunWorkstreamAction.ts
Line: 107-115

Comment:
**Transient resolver failure now hard-blocks runs that previously succeeded**

Before this PR, `resolveDefaultModel` was only called when `!model`, so a transient network error on `getPreviewConfigOptions` only blocked users with no persisted model — users with a `lastUsedModel` were unaffected. Now the resolver is always called when `cloudRegion` is set, but both `desktop-services.ts` and `resolveDefaultModel.ts` swallow all exceptions and return `undefined`. A single `getPreviewConfigOptions` timeout therefore turns `model = undefined`, surfaces "Couldn't start task", and forces a fallback to the task-input screen — even when `preferredModel` was a currently-valid id.

The same regression exists in `useInboxCloudTaskRunner.ts` via the `resolveDefaultModel` wrapper. Consider `model = resolvedModel ?? preferredModel` at both call sites so a gateway outage degrades gracefully (same risk profile as the old code) rather than failing hard.

How can I resolve this? If you propose a fix, please make it concise.

Expand Down
15 changes: 7 additions & 8 deletions packages/ui/src/features/inbox/components/ConfigureAgentsSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,14 +301,13 @@ function SetupTaskSection() {
const settings = useSettingsStore.getState();
const adapter = settings.lastUsedAdapter ?? "claude";
const apiHost = getCloudUrlFromRegion(cloudRegion);
const model =
settings.lastUsedModel ??
(await resolveDefaultModel(
queryClient,
apiHost,
adapter,
modelResolver,
));
const model = await resolveDefaultModel(
queryClient,
apiHost,
adapter,
modelResolver,
settings.lastUsedModel,
);

if (!model) {
sonnerToast.dismiss(toastId);
Expand Down
22 changes: 15 additions & 7 deletions packages/ui/src/features/inbox/hooks/resolveDefaultModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,31 @@ import type { QueryClient } from "@tanstack/react-query";
const log = logger.scope("resolve-default-model");

/**
* Resolve the default model for the given adapter via the preview-config
* tRPC query. Returns the server's `currentValue` for the `model` option, or
* undefined if the call fails or the option is missing.
* Resolve the model for the given adapter via the preview-config tRPC query.
*
* Used by inbox flows that create cloud tasks directly (Discuss, Create PR)
* without going through TaskInput – they need a model to pass to the saga
* and the user hasn't necessarily picked one yet.
* `preferredModel` (e.g. the persisted last-used model) is honoured only if the
* gateway still offers it; otherwise the server default (`currentValue`) is
* returned. Returns undefined if the call fails or the option is missing.
*
* Used by one-click flows that create cloud tasks directly (Discuss, Create PR,
* scout chat) without going through TaskInput – they need a model to pass to the
* saga. Validating the preferred model here is what keeps a stale persisted id
* the gateway no longer offers from reaching the run and 403-ing.
*/
export async function resolveDefaultModel(
queryClient: QueryClient,
apiHost: string,
adapter: "claude" | "codex",
modelResolver: ReportModelResolver,
preferredModel?: string | null,
): Promise<string | undefined> {
void queryClient;
try {
return await modelResolver.resolveDefaultModel(apiHost, adapter);
return await modelResolver.resolveDefaultModel(
apiHost,
adapter,
preferredModel,
);
} catch (error) {
log.warn("Failed to resolve default model", { error, adapter });
}
Expand Down
14 changes: 11 additions & 3 deletions packages/ui/src/features/inbox/hooks/useInboxCloudTaskRunner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,17 @@ export function useInboxCloudTaskRunner({
const adapter = settings.lastUsedAdapter ?? "claude";
const apiHost = getCloudUrlFromRegion(cloudRegion);

const model =
settings.lastUsedModel ??
(await resolveDefaultModel(queryClient, apiHost, adapter, modelResolver));
// Pass the persisted model as a *preference*, not a hard selection: the
// resolver keeps it only if the gateway still offers it, otherwise it falls
// back to the server default. A stale id (e.g. one later de-listed for the
// org) would otherwise be sent here and fail the run with a gateway 403.
const model = await resolveDefaultModel(
queryClient,
apiHost,
adapter,
modelResolver,
settings.lastUsedModel,

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clamp effort after resolving a fallback model

When this call replaces a stale lastUsedModel with a fallback default, the task input below still sends settings.lastUsedReasoningEffort unchanged. If the old model supported a higher tier than the fallback (for example gpt-5.5 with xhigh falling back to a non-5.5 Codex model, or a Claude model with max falling back to Sonnet), the cloud runtime rejects the unsupported effort/model pair in packages/agent/src/server/bin.ts, so the one-click run still fails after the model is repaired; derive or clamp the effort from the resolved model before creating the task.

Useful? React with 👍 / 👎.

Comment on lines +142 to +147

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Update hook mocks for the new resolver dependency

This now calls the model resolver even when lastUsedModel is set, but the existing useDiscussReport hook test mock for useService only returns { createTask }. In that test path modelResolver.resolveDefaultModel is missing, resolveDefaultModel catches the TypeError and returns undefined, so the valid create flow exits as missingModel and createTask is never called; update the test DI mock to return distinct TaskService and ReportModelResolver instances.

Useful? React with 👍 / 👎.

);

if (!model) {
sonnerToast.dismiss(toastId);
Expand Down
Loading