-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(devin): propagate catalog supportsImages to the advertised catalog #4556
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
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 |
|---|---|---|
|
|
@@ -139,7 +139,13 @@ export const DEVIN_MODEL_EFFORTS: Record<string, string[]> = { | |
| export const DEVIN_DEFAULT_EFFORTS = ["low", "medium", "high", "xhigh", "max"]; | ||
|
|
||
| export type DevinUsableModelsResult = | ||
| | { ok: true; models: string[]; contextWindows: Record<string, number>; efforts: Record<string, string[]> } | ||
| | { | ||
| ok: true; | ||
| models: string[]; | ||
| contextWindows: Record<string, number>; | ||
| efforts: Record<string, string[]>; | ||
| inputModalities: Record<string, string[]>; | ||
|
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.
This changes both AGENTS.md reference: structure/AGENTS.md:L49-L50 Useful? React with 👍 / 👎. |
||
| } | ||
| | { ok: false; error: "auth" | "http" | "empty" | "unknown"; detail?: string }; | ||
|
|
||
| /** | ||
|
|
@@ -160,6 +166,8 @@ export async function fetchDevinUsableModels(opts: { | |
| const contextWindows: Record<string, number> = {}; | ||
| // Effort rungs per base, recovered from the suffixes the collapse strips. | ||
| const rungs = new Map<string, Set<string>>(); | ||
| // supportsImages votes per base; only rows that asserted field #5 vote. | ||
| const imageVotes = new Map<string, { sawTrue: boolean; sawFalse: boolean }>(); | ||
| for (const entry of catalog.byUid.values()) { | ||
| if (entry.disabled) continue; | ||
| // Skip internal enum constants (e.g. MODEL_GPT_5_2_LOW, MODEL_PRIVATE_*). | ||
|
|
@@ -183,6 +191,15 @@ export async function fetchDevinUsableModels(opts: { | |
| const seen = contextWindows[base]; | ||
| contextWindows[base] = seen === undefined ? entry.contextWindow : Math.min(seen, entry.contextWindow); | ||
| } | ||
| if (entry.supportsImages !== undefined) { | ||
| let votes = imageVotes.get(base); | ||
| if (!votes) { | ||
| votes = { sawTrue: false, sawFalse: false }; | ||
| imageVotes.set(base, votes); | ||
| } | ||
| if (entry.supportsImages) votes.sawTrue = true; | ||
| else votes.sawFalse = true; | ||
| } | ||
| } | ||
| if (bases.size === 0) return { ok: false, error: "empty" }; | ||
| const efforts: Record<string, string[]> = {}; | ||
|
|
@@ -191,7 +208,21 @@ export async function fetchDevinUsableModels(opts: { | |
| // would draw a picker whose only option is the value already in effect. | ||
| if (set.size > 1) efforts[base] = sortDevinRungs(set); | ||
| } | ||
| return { ok: true, models: [...bases].sort(), contextWindows, efforts }; | ||
| // supportsImages arrives tri-state per catalog row, so the collapse votes: | ||
| // a row that never asserted field #5 abstains, which keeps an unsuffixed | ||
| // unknown row from poisoning a base whose effort variants were measured | ||
| // image-capable. Unanimous measured rows advertise; measured disagreement | ||
| // advertises nothing, because a single measured false is not outvoted by | ||
| // its siblings. One accepted mismatch: resolveWireModelUid prefers the | ||
| // plain UID when the catalog lists it, so a base advertised | ||
| // ["text","image"] on variant evidence can still route a no-effort request | ||
| // to a plain row that never asserted the field. | ||
| const inputModalities: Record<string, string[]> = {}; | ||
| for (const [base, votes] of imageVotes) { | ||
| if (votes.sawTrue && votes.sawFalse) continue; | ||
| inputModalities[base] = votes.sawTrue ? ["text", "image"] : ["text"]; | ||
| } | ||
| return { ok: true, models: [...bases].sort(), contextWindows, efforts, inputModalities }; | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| if (/unauth|401|invalid token|login/i.test(message)) return { ok: false, error: "auth", detail: message }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1745,6 +1745,12 @@ async function fetchProviderModelsWithAuth( | |
| // away, and every client that keys an effort control off this field — | ||
| // the Pi-shaped exports — renders no control at all. | ||
| ...(liveResult.efforts[id]?.length ? { reasoningEfforts: liveResult.efforts[id] } : {}), | ||
| // The account catalog's per-base supportsImages vote collapses to one | ||
| // modalities value. It spreads before the hints so exact | ||
| // modelCapabilities declarations, the legacy modelInputModalities | ||
| // record and the vision-sidecar rewrite keep winning — the live | ||
| // value survives only when none of them applies. | ||
| ...(liveResult.inputModalities[id]?.length ? { inputModalities: liveResult.inputModalities[id] } : {}), | ||
|
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 signed-in Devin users, this changes whether clients such as Codex offer image attachments, yet only internal structure documents were updated. Add the behavior to the existing public Devin sections in AGENTS.md reference: src/AGENTS.md:L29-L29 Useful? React with 👍 / 👎. |
||
| ...catalogHintsFromProviderConfig(name, prov, id, contextCap, metadataModelIdCaseFold, captured.effectiveAlias), | ||
| } as CatalogModel; | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,180 @@ | ||
| /** | ||
| * Devin live-discovery collapse and advertised-catalog propagation for | ||
| * ClientModelConfig field #5 (supportsImages). | ||
| * | ||
| * Catalogs are hand-encoded protobuf run through the real parser | ||
| * (parseCatalogBuffer) and installed through setCachedCatalogForTests, so the | ||
| * tests cover the collapse in fetchDevinUsableModels and the Devin branch of | ||
| * fetchProviderModels without touching the network. KEY is unique to this | ||
| * file and HOST is the stripped default host: getCachedCatalog hits only on | ||
| * an exact (apiKey, host) match with a fresh fetchedAt. | ||
| */ | ||
| import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; | ||
| import * as oauth from "../../src/oauth"; | ||
| import { fetchDevinUsableModels } from "../../src/adapters/devin/live-models"; | ||
| import { parseCatalogBuffer, setCachedCatalogForTests } from "../../src/adapters/devin/cloud-direct/catalog"; | ||
| import { encodeMessage, encodeString, encodeVarintField } from "../../src/adapters/devin/cloud-direct/wire"; | ||
| import { fetchProviderModels } from "../../src/codex/catalog/provider-fetch"; | ||
| import { clearModelCache, providerCacheGenerations } from "../../src/codex/model-cache"; | ||
| import type { OcxProviderConfig } from "../../src/types"; | ||
|
|
||
| const HOST = "https://server.codeium.com"; | ||
| const KEY = "devin-live-models-test-key"; | ||
|
|
||
| /** One ClientModelConfig body; field #5 stays absent unless opts asserts it. */ | ||
| function catalogEntry( | ||
| uid: string, | ||
| opts: { disabled?: boolean; supportsImages?: boolean; contextWindow?: number } = {}, | ||
| ): Buffer { | ||
| return Buffer.concat([ | ||
| encodeString(1, uid), | ||
| ...(opts.disabled === true ? [encodeVarintField(4, 1)] : []), | ||
| // encodeVarintField(5, 0) is a measured text-only vote — real bytes, not | ||
| // an omission — while leaving field #5 out keeps the row unknown. | ||
| ...(opts.supportsImages !== undefined ? [encodeVarintField(5, opts.supportsImages ? 1 : 0)] : []), | ||
| ...(opts.contextWindow !== undefined ? [encodeVarintField(18, opts.contextWindow)] : []), | ||
| encodeString(22, uid), | ||
| ]); | ||
| } | ||
|
|
||
| function seedCatalog(...entries: Buffer[]): void { | ||
| setCachedCatalogForTests(parseCatalogBuffer( | ||
| Buffer.concat(entries.map((entry) => encodeMessage(1, entry))), | ||
| KEY, | ||
| HOST, | ||
| )); | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| setCachedCatalogForTests(null); | ||
| clearModelCache("devin-test"); | ||
| providerCacheGenerations.delete("devin-test"); | ||
|
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 | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/codex/model-cache.ts --items all
rg -n -C 6 'providerCacheGenerations|clearModelCache|generation' src/codex/model-cache.tsRepository: lidge-jun/opencodex Length of output: 7676 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- test cleanup ---'
cat -n tests/providers/devin-live-models.test.ts | sed -n '35,65p'
printf '%s\n' '--- generation-aware callers ---'
rg -n -C 8 'captureModelCacheGeneration|setCached\(' src tests/providers/devin-live-models.test.tsRepository: lidge-jun/opencodex Length of output: 15433 Preserve the provider cache generation after clearing.
If a discovery started at generation Proposed fix-import { clearModelCache, providerCacheGenerations } from "../../src/codex/model-cache";
+import { clearModelCache } from "../../src/codex/model-cache";
...
setCachedCatalogForTests(null);
clearModelCache("devin-test");
- providerCacheGenerations.delete("devin-test");
});
...
setCachedCatalogForTests(null);
clearModelCache("devin-test");
- providerCacheGenerations.delete("devin-test");
});#!/bin/bash
set -euo pipefail
ast-grep outline src/codex/model-cache.ts --items all
rg -n -C 6 'providerCacheGenerations|clearModelCache|generation' src/codex/model-cache.tsAlso applies to: 56-56 🤖 Prompt for AI Agents |
||
| }); | ||
| afterEach(() => { | ||
| setCachedCatalogForTests(null); | ||
| clearModelCache("devin-test"); | ||
| providerCacheGenerations.delete("devin-test"); | ||
| }); | ||
|
|
||
| describe("devin live model discovery", () => { | ||
| test("collapses per-variant supportsImages votes into per-base input modalities", async () => { | ||
| seedCatalog( | ||
| // Unanimous measured rows advertise. | ||
| catalogEntry("vision-model", { supportsImages: true, contextWindow: 262_000 }), | ||
| catalogEntry("vision-model-high", { supportsImages: true, contextWindow: 1_000_000 }), | ||
| catalogEntry("text-model-low", { supportsImages: false }), | ||
| catalogEntry("text-model-high", { supportsImages: false }), | ||
| // An unsuffixed row that never asserted field #5 abstains instead of | ||
| // poisoning a measured image base. | ||
| catalogEntry("abstain-model"), | ||
| catalogEntry("abstain-model-high", { supportsImages: true }), | ||
| // Measured disagreement stays unadvertised — a single false is not | ||
| // outvoted by its siblings. | ||
| catalogEntry("split-model", { supportsImages: true }), | ||
| catalogEntry("split-model-low", { supportsImages: true }), | ||
| catalogEntry("split-model-high", { supportsImages: false }), | ||
| catalogEntry("mixed-model-low", { supportsImages: true }), | ||
| catalogEntry("mixed-model-high", { supportsImages: false }), | ||
| // Zero measured rows advertise nothing. | ||
| catalogEntry("mystery-model"), | ||
| catalogEntry("mystery-model-high"), | ||
| // Disabled and MODEL_* rows are skipped before they can vote: if the | ||
| // disabled true voted, text-off-model would read as disagreement. | ||
| catalogEntry("text-off-model", { supportsImages: false }), | ||
| catalogEntry("text-off-model-high", { disabled: true, supportsImages: true }), | ||
| catalogEntry("ghost-model-high", { disabled: true, supportsImages: true }), | ||
| catalogEntry("MODEL_INTERNAL_VISION", { supportsImages: true }), | ||
| ); | ||
| const result = await fetchDevinUsableModels({ apiKey: KEY, baseUrl: HOST }); | ||
| if (!result.ok) throw new Error(`expected ok, got ${result.error}`); | ||
| expect(result.models).toEqual([ | ||
| "abstain-model", | ||
| "mixed-model", | ||
| "mystery-model", | ||
| "split-model", | ||
| "text-model", | ||
| "text-off-model", | ||
| "vision-model", | ||
| ]); | ||
| expect(result.inputModalities).toEqual({ | ||
| "vision-model": ["text", "image"], | ||
| "text-model": ["text"], | ||
| "abstain-model": ["text", "image"], | ||
| "text-off-model": ["text"], | ||
| }); | ||
| // The collapse adds a field; the existing projections are unchanged. | ||
| expect(result.contextWindows["vision-model"]).toBe(262_000); | ||
| expect(result.efforts["text-model"]).toEqual(["low", "high"]); | ||
| }); | ||
|
|
||
| test("a catalog with no measured rows still carries an empty record", async () => { | ||
| seedCatalog(catalogEntry("plain-model"), catalogEntry("plain-model-high")); | ||
| const result = await fetchDevinUsableModels({ apiKey: KEY, baseUrl: HOST }); | ||
| if (!result.ok) throw new Error(`expected ok, got ${result.error}`); | ||
| expect(result.inputModalities).toEqual({}); | ||
| }); | ||
| }); | ||
|
|
||
| describe("devin advertised catalog input modalities", () => { | ||
| // Devin is an oauth provider, so discovery resolves its bearer through | ||
| // resolveModelsAuthToken; the tests lend it a token rather than an account | ||
| // store (the same seam the Copilot oauth cases use). | ||
| let authSpy: ReturnType<typeof spyOn> | undefined; | ||
| beforeEach(() => { | ||
| authSpy = spyOn(oauth, "resolveModelsAuthToken").mockResolvedValue(KEY); | ||
| }); | ||
| afterEach(() => { | ||
| authSpy?.mockRestore(); | ||
| authSpy = undefined; | ||
| }); | ||
|
|
||
| const devinProvider = (extra: Partial<OcxProviderConfig> = {}): OcxProviderConfig => ({ | ||
| adapter: "devin", | ||
| baseUrl: HOST, | ||
| apiKey: KEY, | ||
| authMode: "oauth", | ||
| liveModels: true, | ||
| ...extra, | ||
| } as OcxProviderConfig); | ||
|
|
||
| test("a measured image base advertises text and image", async () => { | ||
| seedCatalog(catalogEntry("img-model", { supportsImages: true })); | ||
| const models = await fetchProviderModels("devin-test", devinProvider(), 60_000); | ||
| expect(models.map((model) => model.id)).toEqual(["img-model"]); | ||
| expect(models[0]?.inputModalities).toEqual(["text", "image"]); | ||
| }); | ||
|
|
||
| test("an exact modelCapabilities declaration overwrites the live value", async () => { | ||
| seedCatalog(catalogEntry("img-model", { supportsImages: true })); | ||
| const models = await fetchProviderModels("devin-test", devinProvider({ | ||
| modelCapabilities: { "img-model": { inputModalities: ["audio"] } }, | ||
| }), 60_000); | ||
| expect(models[0]?.inputModalities).toEqual(["audio"]); | ||
| }); | ||
|
|
||
| test("an exact text-only declaration still takes the sidecar path", async () => { | ||
| // A text-only modelCapabilities entry makes the row a vision-sidecar | ||
| // consumer (src/vision/eligibility.ts): the declaration governs runtime | ||
| // eligibility while the catalog keeps attachments unblocked. | ||
| seedCatalog(catalogEntry("img-model", { supportsImages: true })); | ||
| const models = await fetchProviderModels("devin-test", devinProvider({ | ||
| modelCapabilities: { "img-model": { inputModalities: ["text"] } }, | ||
| }), 60_000); | ||
| expect(models[0]?.inputModalities).toEqual(["text", "image"]); | ||
| }); | ||
|
|
||
| test("a noVisionModels entry upgrades a live text-only row through the sidecar", async () => { | ||
| seedCatalog(catalogEntry("side-model", { supportsImages: false })); | ||
| const models = await fetchProviderModels("devin-test", devinProvider({ | ||
| noVisionModels: ["side-model"], | ||
| }), 60_000); | ||
| expect(models[0]?.inputModalities).toEqual(["text", "image"]); | ||
| }); | ||
|
|
||
| test("a measured text-only base is not upgraded without a sidecar consumer", async () => { | ||
| seedCatalog(catalogEntry("plain-model", { supportsImages: false })); | ||
| const models = await fetchProviderModels("devin-test", devinProvider(), 60_000); | ||
| expect(models.map((model) => model.id)).toEqual(["plain-model"]); | ||
| expect(models[0]?.inputModalities).toEqual(["text"]); | ||
| }); | ||
| }); | ||
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run the required type check.
The PR summary states that
bun run typecheckwas not run. Run it before merge because this change is underscripts/**.As per coding guidelines: “Run
bun run typecheck.”🤖 Prompt for AI Agents
Source: Coding guidelines