From 69c453b27470147bf6c6c0a6d189a15de5211d40 Mon Sep 17 00:00:00 2001 From: Konan69 Date: Thu, 5 Mar 2026 15:52:59 +0100 Subject: [PATCH] Address PR feedback for OpenCode integration --- .../opencode-local/src/server/execute.ts | 8 +++--- .../opencode-local/src/server/models.ts | 26 +++++++++++++++++-- .../opencode-local/src/server/test.ts | 25 +++++++++++------- .../opencode-local/src/ui/build-config.ts | 2 ++ server/src/adapters/registry.ts | 3 +-- server/src/routes/agents.ts | 7 ++++- ui/src/api/agents.ts | 4 ++- ui/src/components/AgentConfigForm.tsx | 14 +--------- ui/src/components/NewAgentDialog.tsx | 6 +++++ ui/src/components/NewIssueDialog.tsx | 13 +++------- ui/src/components/OnboardingWizard.tsx | 18 +++---------- ui/src/lib/model-utils.ts | 16 ++++++++++++ 12 files changed, 87 insertions(+), 55 deletions(-) create mode 100644 ui/src/lib/model-utils.ts diff --git a/packages/adapters/opencode-local/src/server/execute.ts b/packages/adapters/opencode-local/src/server/execute.ts index 0d454bfc..2b50c23f 100644 --- a/packages/adapters/opencode-local/src/server/execute.ts +++ b/packages/adapters/opencode-local/src/server/execute.ts @@ -115,7 +115,7 @@ export async function execute(ctx: AdapterExecutionContext): Promise(); +const VOLATILE_ENV_KEY_PREFIXES = ["PAPERCLIP_", "npm_", "NPM_"] as const; +const VOLATILE_ENV_KEY_EXACT = new Set(["PWD", "OLDPWD", "SHLVL", "_", "TERM_SESSION_ID"]); function dedupeModels(models: AdapterModel[]): AdapterModel[] { const seen = new Set(); @@ -61,14 +65,30 @@ function normalizeEnv(input: unknown): Record { return env; } +function isVolatileEnvKey(key: string): boolean { + if (VOLATILE_ENV_KEY_EXACT.has(key)) return true; + return VOLATILE_ENV_KEY_PREFIXES.some((prefix) => key.startsWith(prefix)); +} + +function hashValue(value: string): string { + return createHash("sha256").update(value).digest("hex"); +} + function discoveryCacheKey(command: string, cwd: string, env: Record) { const envKey = Object.entries(env) + .filter(([key]) => !isVolatileEnvKey(key)) .sort(([a], [b]) => a.localeCompare(b)) - .map(([key, value]) => `${key}=${value}`) + .map(([key, value]) => `${key}=${hashValue(value)}`) .join("\n"); return `${command}\n${cwd}\n${envKey}`; } +function pruneExpiredDiscoveryCache(now: number) { + for (const [key, value] of discoveryCache.entries()) { + if (value.expiresAt <= now) discoveryCache.delete(key); + } +} + export async function discoverOpenCodeModels(input: { command?: unknown; cwd?: unknown; @@ -83,6 +103,7 @@ export async function discoverOpenCodeModels(input: { ); const cwd = asString(input.cwd, process.cwd()); const env = normalizeEnv(input.env); + const runtimeEnv = normalizeEnv(ensurePathInEnv({ ...process.env, ...env })); const result = await runChildProcess( `opencode-models-${Date.now()}-${Math.random().toString(16).slice(2)}`, @@ -90,7 +111,7 @@ export async function discoverOpenCodeModels(input: { ["models"], { cwd, - env, + env: runtimeEnv, timeoutSec: 20, graceSec: 3, onLog: async () => {}, @@ -124,6 +145,7 @@ export async function discoverOpenCodeModelsCached(input: { const env = normalizeEnv(input.env); const key = discoveryCacheKey(command, cwd, env); const now = Date.now(); + pruneExpiredDiscoveryCache(now); const cached = discoveryCache.get(key); if (cached && cached.expiresAt > now) return cached.models; diff --git a/packages/adapters/opencode-local/src/server/test.ts b/packages/adapters/opencode-local/src/server/test.ts index f45dd6b1..6e1ac0a3 100644 --- a/packages/adapters/opencode-local/src/server/test.ts +++ b/packages/adapters/opencode-local/src/server/test.ts @@ -38,6 +38,15 @@ function summarizeProbeDetail(stdout: string, stderr: string, parsedError: strin return clean.length > max ? `${clean.slice(0, max - 1)}...` : clean; } +function normalizeEnv(input: unknown): Record { + if (typeof input !== "object" || input === null || Array.isArray(input)) return {}; + const env: Record = {}; + for (const [key, value] of Object.entries(input as Record)) { + if (typeof value === "string") env[key] = value; + } + return env; +} + const OPENCODE_AUTH_REQUIRED_RE = /(?:auth(?:entication)?\s+required|api\s*key|invalid\s*api\s*key|not\s+logged\s+in|opencode\s+auth\s+login|free\s+usage\s+exceeded)/i; @@ -50,7 +59,7 @@ export async function testEnvironment( const cwd = asString(config.cwd, process.cwd()); try { - await ensureAbsoluteDirectory(cwd, { createIfMissing: true }); + await ensureAbsoluteDirectory(cwd, { createIfMissing: false }); checks.push({ code: "opencode_cwd_valid", level: "info", @@ -70,7 +79,7 @@ export async function testEnvironment( for (const [key, value] of Object.entries(envConfig)) { if (typeof value === "string") env[key] = value; } - const runtimeEnv = ensurePathInEnv({ ...process.env, ...env }); + const runtimeEnv = normalizeEnv(ensurePathInEnv({ ...process.env, ...env })); try { await ensureCommandResolvable(command, cwd, runtimeEnv); @@ -91,17 +100,15 @@ export async function testEnvironment( const canRunProbe = checks.every((check) => check.code !== "opencode_cwd_invalid" && check.code !== "opencode_command_unresolvable"); - let discoveredModels: string[] = []; let modelValidationPassed = false; if (canRunProbe) { try { - const discovered = await discoverOpenCodeModels({ command, cwd, env }); - discoveredModels = discovered.map((item) => item.id); - if (discoveredModels.length > 0) { + const discovered = await discoverOpenCodeModels({ command, cwd, env: runtimeEnv }); + if (discovered.length > 0) { checks.push({ code: "opencode_models_discovered", level: "info", - message: `Discovered ${discoveredModels.length} model(s) from OpenCode providers.`, + message: `Discovered ${discovered.length} model(s) from OpenCode providers.`, }); } else { checks.push({ @@ -135,7 +142,7 @@ export async function testEnvironment( model: configuredModel, command, cwd, - env, + env: runtimeEnv, }); checks.push({ code: "opencode_model_configured", @@ -173,7 +180,7 @@ export async function testEnvironment( args, { cwd, - env, + env: runtimeEnv, timeoutSec: 60, graceSec: 5, stdin: "Respond with hello.", diff --git a/packages/adapters/opencode-local/src/ui/build-config.ts b/packages/adapters/opencode-local/src/ui/build-config.ts index f656585e..3abfd6cd 100644 --- a/packages/adapters/opencode-local/src/ui/build-config.ts +++ b/packages/adapters/opencode-local/src/ui/build-config.ts @@ -57,6 +57,8 @@ export function buildOpenCodeLocalConfig(v: CreateConfigValues): Record api.post(agentPath(id, companyId, "/runtime-state/reset-session"), { taskKey: taskKey ?? null }), adapterModels: (companyId: string, type: string) => - api.get(`/companies/${companyId}/adapters/${type}/models`), + api.get( + `/companies/${encodeURIComponent(companyId)}/adapters/${encodeURIComponent(type)}/models`, + ), testEnvironment: ( companyId: string, type: string, diff --git a/ui/src/components/AgentConfigForm.tsx b/ui/src/components/AgentConfigForm.tsx index 3d36febc..f9b08d20 100644 --- a/ui/src/components/AgentConfigForm.tsx +++ b/ui/src/components/AgentConfigForm.tsx @@ -23,6 +23,7 @@ import { import { Button } from "@/components/ui/button"; import { FolderOpen, Heart, ChevronDown, X } from "lucide-react"; import { cn } from "../lib/utils"; +import { extractModelName, extractProviderId } from "../lib/model-utils"; import { queryKeys } from "../lib/queryKeys"; import { useCompany } from "../context/CompanyContext"; import { @@ -123,19 +124,6 @@ function formatArgList(value: unknown): string { return typeof value === "string" ? value : ""; } -function extractProviderId(modelId: string): string | null { - const trimmed = modelId.trim(); - if (!trimmed.includes("/")) return null; - const provider = trimmed.slice(0, trimmed.indexOf("/")).trim(); - return provider || null; -} - -function extractModelName(modelId: string): string { - const trimmed = modelId.trim(); - if (!trimmed.includes("/")) return trimmed; - return trimmed.slice(trimmed.indexOf("/") + 1); -} - const codexThinkingEffortOptions = [ { id: "", label: "Auto" }, { id: "minimal", label: "Minimal" }, diff --git a/ui/src/components/NewAgentDialog.tsx b/ui/src/components/NewAgentDialog.tsx index 236eb1ec..a5392716 100644 --- a/ui/src/components/NewAgentDialog.tsx +++ b/ui/src/components/NewAgentDialog.tsx @@ -58,6 +58,8 @@ export function NewAgentDialog() { const { data: adapterModels, error: adapterModelsError, + isLoading: adapterModelsLoading, + isFetching: adapterModelsFetching, } = useQuery({ queryKey: selectedCompanyId @@ -126,6 +128,10 @@ export function NewAgentDialog() { ); return; } + if (adapterModelsLoading || adapterModelsFetching) { + setFormError("OpenCode models are still loading. Please wait and try again."); + return; + } const discovered = adapterModels ?? []; if (!discovered.some((entry) => entry.id === selectedModel)) { setFormError( diff --git a/ui/src/components/NewIssueDialog.tsx b/ui/src/components/NewIssueDialog.tsx index 811bf156..8653cb1b 100644 --- a/ui/src/components/NewIssueDialog.tsx +++ b/ui/src/components/NewIssueDialog.tsx @@ -36,6 +36,7 @@ import { Paperclip, } from "lucide-react"; import { cn } from "../lib/utils"; +import { extractProviderIdWithFallback } from "../lib/model-utils"; import { issueStatusText, issueStatusTextDefault, priorityColor, priorityColorDefault } from "../lib/status-colors"; import { MarkdownEditor, type MarkdownEditorRef, type MentionOption } from "./MarkdownEditor"; import { AgentIcon } from "./AgentIconPicker"; @@ -54,12 +55,6 @@ function getContrastTextColor(hexColor: string): string { return luminance > 0.5 ? "#000000" : "#ffffff"; } -function extractProviderId(modelId: string): string { - const trimmed = modelId.trim(); - if (!trimmed.includes("/")) return "other"; - return trimmed.slice(0, trimmed.indexOf("/")).trim() || "other"; -} - interface IssueDraft { title: string; description: string; @@ -505,8 +500,8 @@ export function NewIssueDialog() { () => { return [...(assigneeAdapterModels ?? [])] .sort((a, b) => { - const providerA = extractProviderId(a.id); - const providerB = extractProviderId(b.id); + const providerA = extractProviderIdWithFallback(a.id); + const providerB = extractProviderIdWithFallback(b.id); const byProvider = providerA.localeCompare(providerB); if (byProvider !== 0) return byProvider; return a.id.localeCompare(b.id); @@ -514,7 +509,7 @@ export function NewIssueDialog() { .map((model) => ({ id: model.id, label: model.label, - searchText: `${model.id} ${extractProviderId(model.id)}`, + searchText: `${model.id} ${extractProviderIdWithFallback(model.id)}`, })); }, [assigneeAdapterModels], diff --git a/ui/src/components/OnboardingWizard.tsx b/ui/src/components/OnboardingWizard.tsx index 1df783a2..50b7a6a3 100644 --- a/ui/src/components/OnboardingWizard.tsx +++ b/ui/src/components/OnboardingWizard.tsx @@ -17,6 +17,7 @@ import { } from "@/components/ui/popover"; import { Button } from "@/components/ui/button"; import { cn } from "../lib/utils"; +import { extractModelName, extractProviderIdWithFallback } from "../lib/model-utils"; import { getUIAdapter } from "../adapters"; import { defaultCreateValues } from "./agent-config-defaults"; import { @@ -61,19 +62,6 @@ Ensure you have a folder agents/ceo and then download this AGENTS.md as well as And after you've finished that, hire yourself a Founding Engineer agent`; -function extractProviderId(modelId: string): string | null { - const trimmed = modelId.trim(); - if (!trimmed.includes("/")) return null; - const provider = trimmed.slice(0, trimmed.indexOf("/")).trim(); - return provider || null; -} - -function extractModelName(modelId: string): string { - const trimmed = modelId.trim(); - if (!trimmed.includes("/")) return trimmed; - return trimmed.slice(trimmed.indexOf("/") + 1); -} - export function OnboardingWizard() { const { onboardingOpen, onboardingOptions, closeOnboarding } = useDialog(); const { selectedCompanyId, companies, setSelectedCompanyId } = useCompany(); @@ -185,7 +173,7 @@ export function OnboardingWizard() { const query = modelSearch.trim().toLowerCase(); return (adapterModels ?? []).filter((entry) => { if (!query) return true; - const provider = extractProviderId(entry.id) ?? ""; + const provider = extractProviderIdWithFallback(entry.id, ""); return ( entry.id.toLowerCase().includes(query) || entry.label.toLowerCase().includes(query) || @@ -204,7 +192,7 @@ export function OnboardingWizard() { } const groups = new Map>(); for (const entry of filteredModels) { - const provider = extractProviderId(entry.id) ?? "other"; + const provider = extractProviderIdWithFallback(entry.id); const bucket = groups.get(provider) ?? []; bucket.push(entry); groups.set(provider, bucket); diff --git a/ui/src/lib/model-utils.ts b/ui/src/lib/model-utils.ts new file mode 100644 index 00000000..b3753e61 --- /dev/null +++ b/ui/src/lib/model-utils.ts @@ -0,0 +1,16 @@ +export function extractProviderId(modelId: string): string | null { + const trimmed = modelId.trim(); + if (!trimmed.includes("/")) return null; + const provider = trimmed.slice(0, trimmed.indexOf("/")).trim(); + return provider || null; +} + +export function extractProviderIdWithFallback(modelId: string, fallback = "other"): string { + return extractProviderId(modelId) ?? fallback; +} + +export function extractModelName(modelId: string): string { + const trimmed = modelId.trim(); + if (!trimmed.includes("/")) return trimmed; + return trimmed.slice(trimmed.indexOf("/") + 1); +}