From 00061b2fd3d6147f11988b5df75693c56a076de7 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 6 Jan 2026 03:05:56 +0100 Subject: [PATCH] fix: harden config form --- CHANGELOG.md | 1 + src/agents/model-auth.test.ts | 10 +- src/wizard/onboarding.ts | 39 ++- ui/src/ui/config-form.browser.test.ts | 116 ++++++++- ui/src/ui/controllers/config.test.ts | 26 +- ui/src/ui/controllers/config.ts | 8 +- ui/src/ui/views/config-form.ts | 347 +++++++++++++++++++++++-- ui/src/ui/views/config.browser.test.ts | 44 ++++ ui/src/ui/views/config.ts | 24 +- 9 files changed, 577 insertions(+), 38 deletions(-) create mode 100644 ui/src/ui/views/config.browser.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ed9ccd79..8aad296b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - Onboarding: when OpenAI Codex OAuth is used, default to `openai-codex/gpt-5.2` and warn if the selected model lacks auth. - CLI: auto-migrate legacy config entries on command start (same behavior as gateway startup). - Auth: prioritize OAuth profiles but fall back to API keys when refresh fails; stored profiles now load without explicit auth order. +- Control UI: harden config Form view with schema normalization, map editing, and guardrails to prevent data loss on save. - Docs: add group chat participation guidance to the AGENTS template. - Gmail: stop restart loop when `gog gmail watch serve` fails to bind (address already in use). - Linux: auto-attempt lingering during onboarding (try without sudo, fallback to sudo) and prompt on install/restart to keep the gateway alive after logout/idle. Thanks @tobiasbischoff for PR #237. diff --git a/src/agents/model-auth.test.ts b/src/agents/model-auth.test.ts index ac37fad6a..49900389f 100644 --- a/src/agents/model-auth.test.ts +++ b/src/agents/model-auth.test.ts @@ -132,9 +132,13 @@ describe("getApiKeyForModel", () => { vi.resetModules(); const { resolveApiKeyForProvider } = await import("./model-auth.js"); - await expect( - resolveApiKeyForProvider({ provider: "openai" }), - ).rejects.toThrow(/openai-codex\/gpt-5\\.2/); + let error: unknown = null; + try { + await resolveApiKeyForProvider({ provider: "openai" }); + } catch (err) { + error = err; + } + expect(String(error)).toContain("openai-codex/gpt-5.2"); } finally { if (previousOpenAiKey === undefined) { delete process.env.OPENAI_API_KEY; diff --git a/src/wizard/onboarding.ts b/src/wizard/onboarding.ts index 1578db290..4077ac579 100644 --- a/src/wizard/onboarding.ts +++ b/src/wizard/onboarding.ts @@ -6,9 +6,15 @@ import { type OAuthCredentials, type OAuthProvider, } from "@mariozechner/pi-ai"; -import { ensureAuthProfileStore, listProfilesForProvider } from "../agents/auth-profiles.js"; +import { + ensureAuthProfileStore, + listProfilesForProvider, +} from "../agents/auth-profiles.js"; import { DEFAULT_MODEL, DEFAULT_PROVIDER } from "../agents/defaults.js"; -import { getCustomProviderApiKey, resolveEnvApiKey } from "../agents/model-auth.js"; +import { + getCustomProviderApiKey, + resolveEnvApiKey, +} from "../agents/model-auth.js"; import { loadModelCatalog } from "../agents/model-catalog.js"; import { resolveConfiguredModelRef } from "../agents/model-selection.js"; import { @@ -62,6 +68,7 @@ import type { RuntimeEnv } from "../runtime.js"; import { defaultRuntime } from "../runtime.js"; import { resolveUserPath, sleep } from "../utils.js"; import type { WizardPrompter } from "./prompts.js"; +import type { AgentModelListConfig } from "../config/types.js"; const OPENAI_CODEX_DEFAULT_MODEL = "openai-codex/gpt-5.2"; @@ -74,10 +81,22 @@ function shouldSetOpenAICodexModel(model?: string): boolean { return normalized === "gpt" || normalized === "gpt-mini"; } -function applyOpenAICodexModelDefault( - cfg: ClawdbotConfig, -): { next: ClawdbotConfig; changed: boolean } { - if (!shouldSetOpenAICodexModel(cfg.agent?.model)) { +function resolvePrimaryModel( + model?: AgentModelListConfig | string, +): string | undefined { + if (typeof model === "string") return model; + if (model && typeof model === "object" && typeof model.primary === "string") { + return model.primary; + } + return undefined; +} + +function applyOpenAICodexModelDefault(cfg: ClawdbotConfig): { + next: ClawdbotConfig; + changed: boolean; +} { + const current = resolvePrimaryModel(cfg.agent?.model); + if (!shouldSetOpenAICodexModel(current)) { return { next: cfg, changed: false }; } return { @@ -85,7 +104,10 @@ function applyOpenAICodexModelDefault( ...cfg, agent: { ...cfg.agent, - model: OPENAI_CODEX_DEFAULT_MODEL, + model: + cfg.agent?.model && typeof cfg.agent.model === "object" + ? { ...cfg.agent.model, primary: OPENAI_CODEX_DEFAULT_MODEL } + : { primary: OPENAI_CODEX_DEFAULT_MODEL }, }, }, changed: true, @@ -125,8 +147,7 @@ async function warnIfModelConfigLooksOff( } if (ref.provider === "openai") { - const hasCodex = - listProfilesForProvider(store, "openai-codex").length > 0; + const hasCodex = listProfilesForProvider(store, "openai-codex").length > 0; if (hasCodex) { warnings.push( `Detected OpenAI Codex OAuth. Consider setting agent.model to ${OPENAI_CODEX_DEFAULT_MODEL}.`, diff --git a/ui/src/ui/config-form.browser.test.ts b/ui/src/ui/config-form.browser.test.ts index 9ade22641..8de0b25ab 100644 --- a/ui/src/ui/config-form.browser.test.ts +++ b/ui/src/ui/config-form.browser.test.ts @@ -1,7 +1,7 @@ import { render } from "lit"; import { describe, expect, it, vi } from "vitest"; -import { renderConfigForm } from "./views/config-form"; +import { analyzeConfigSchema, renderConfigForm } from "./views/config-form"; const rootSchema = { type: "object", @@ -28,6 +28,14 @@ const rootSchema = { enabled: { type: "boolean", }, + bind: { + anyOf: [ + { const: "auto" }, + { const: "lan" }, + { const: "tailnet" }, + { const: "loopback" }, + ], + }, }, }; @@ -35,12 +43,14 @@ describe("config form renderer", () => { it("renders inputs and patches values", () => { const onPatch = vi.fn(); const container = document.createElement("div"); + const analysis = analyzeConfigSchema(rootSchema); render( renderConfigForm({ - schema: rootSchema, + schema: analysis.schema, uiHints: { "gateway.auth.token": { label: "Gateway Token", sensitive: true }, }, + unsupportedPaths: analysis.unsupportedPaths, value: {}, onPatch, }), @@ -79,10 +89,12 @@ describe("config form renderer", () => { it("adds and removes array entries", () => { const onPatch = vi.fn(); const container = document.createElement("div"); + const analysis = analyzeConfigSchema(rootSchema); render( renderConfigForm({ - schema: rootSchema, + schema: analysis.schema, uiHints: {}, + unsupportedPaths: analysis.unsupportedPaths, value: { allowFrom: ["+1"] }, onPatch, }), @@ -103,4 +115,102 @@ describe("config form renderer", () => { removeButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })); expect(onPatch).toHaveBeenCalledWith(["allowFrom"], []); }); + + it("renders union literals as select options", () => { + const onPatch = vi.fn(); + const container = document.createElement("div"); + const analysis = analyzeConfigSchema(rootSchema); + render( + renderConfigForm({ + schema: analysis.schema, + uiHints: {}, + unsupportedPaths: analysis.unsupportedPaths, + value: { bind: "auto" }, + onPatch, + }), + container, + ); + + const selects = Array.from(container.querySelectorAll("select")); + const bindSelect = selects.find((el) => + Array.from(el.options).some((opt) => opt.value === "tailnet"), + ) as HTMLSelectElement | undefined; + expect(bindSelect).not.toBeUndefined(); + if (!bindSelect) return; + bindSelect.value = "tailnet"; + bindSelect.dispatchEvent(new Event("change", { bubbles: true })); + expect(onPatch).toHaveBeenCalledWith(["bind"], "tailnet"); + }); + + it("renders map fields from additionalProperties", () => { + const onPatch = vi.fn(); + const container = document.createElement("div"); + const schema = { + type: "object", + properties: { + slack: { + type: "object", + additionalProperties: { + type: "string", + }, + }, + }, + }; + const analysis = analyzeConfigSchema(schema); + render( + renderConfigForm({ + schema: analysis.schema, + uiHints: {}, + unsupportedPaths: analysis.unsupportedPaths, + value: { slack: { channelA: "ok" } }, + onPatch, + }), + container, + ); + + const removeButton = Array.from(container.querySelectorAll("button")).find( + (btn) => btn.textContent?.trim() === "Remove", + ); + expect(removeButton).not.toBeUndefined(); + removeButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(onPatch).toHaveBeenCalledWith(["slack"], {}); + }); + + it("flags unsupported unions", () => { + const schema = { + type: "object", + properties: { + mixed: { + anyOf: [{ type: "string" }, { type: "number" }], + }, + }, + }; + const analysis = analyzeConfigSchema(schema); + expect(analysis.unsupportedPaths).toContain("mixed"); + }); + + it("supports nullable types", () => { + const schema = { + type: "object", + properties: { + note: { type: ["string", "null"] }, + }, + }; + const analysis = analyzeConfigSchema(schema); + expect(analysis.unsupportedPaths).not.toContain("note"); + }); + + it("flags additionalProperties true", () => { + const schema = { + type: "object", + properties: { + extra: { + type: "object", + additionalProperties: true, + }, + }, + }; + const analysis = analyzeConfigSchema(schema); + expect(analysis.unsupportedPaths).toContain("extra"); + }); }); diff --git a/ui/src/ui/controllers/config.test.ts b/ui/src/ui/controllers/config.test.ts index f8ee03dc0..87fda4ce1 100644 --- a/ui/src/ui/controllers/config.test.ts +++ b/ui/src/ui/controllers/config.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from "vitest"; -import { applyConfigSnapshot, type ConfigState } from "./config"; +import { + applyConfigSnapshot, + updateConfigFormValue, + type ConfigState, +} from "./config"; import { defaultDiscordActions, defaultSlackActions, @@ -137,3 +141,23 @@ describe("applyConfigSnapshot", () => { expect(state.slackForm.actions).toEqual(defaultSlackActions); }); }); + +describe("updateConfigFormValue", () => { + it("seeds from snapshot when form is null", () => { + const state = createState(); + state.configSnapshot = { + config: { telegram: { botToken: "t" }, gateway: { mode: "local" } }, + valid: true, + issues: [], + raw: "{}", + }; + + updateConfigFormValue(state, ["gateway", "port"], 18789); + + expect(state.configFormDirty).toBe(true); + expect(state.configForm).toEqual({ + telegram: { botToken: "t" }, + gateway: { mode: "local", port: 18789 }, + }); + }); +}); diff --git a/ui/src/ui/controllers/config.ts b/ui/src/ui/controllers/config.ts index 760b1d452..5844baad5 100644 --- a/ui/src/ui/controllers/config.ts +++ b/ui/src/ui/controllers/config.ts @@ -402,7 +402,9 @@ export function updateConfigFormValue( path: Array, value: unknown, ) { - const base = cloneConfigObject(state.configForm ?? {}); + const base = cloneConfigObject( + state.configForm ?? state.configSnapshot?.config ?? {}, + ); setPathValue(base, path, value); state.configForm = base; state.configFormDirty = true; @@ -412,7 +414,9 @@ export function removeConfigFormValue( state: ConfigState, path: Array, ) { - const base = cloneConfigObject(state.configForm ?? {}); + const base = cloneConfigObject( + state.configForm ?? state.configSnapshot?.config ?? {}, + ); removePathValue(base, path); state.configForm = base; state.configFormDirty = true; diff --git a/ui/src/ui/views/config-form.ts b/ui/src/ui/views/config-form.ts index 29bb0af99..dcf5d01f3 100644 --- a/ui/src/ui/views/config-form.ts +++ b/ui/src/ui/views/config-form.ts @@ -2,9 +2,11 @@ import { html, nothing } from "lit"; import type { ConfigUiHint, ConfigUiHints } from "../types"; export type ConfigFormProps = { - schema: unknown | null; + schema: JsonSchema | null; uiHints: ConfigUiHints; value: Record | null; + disabled?: boolean; + unsupportedPaths?: string[]; onPatch: (path: Array, value: unknown) => void; }; @@ -14,22 +16,26 @@ type JsonSchema = { description?: string; properties?: Record; items?: JsonSchema | JsonSchema[]; + additionalProperties?: JsonSchema | boolean; enum?: unknown[]; + const?: unknown; default?: unknown; anyOf?: JsonSchema[]; oneOf?: JsonSchema[]; allOf?: JsonSchema[]; + nullable?: boolean; }; export function renderConfigForm(props: ConfigFormProps) { if (!props.schema) { return html`
Schema unavailable.
`; } - const schema = props.schema as JsonSchema; + const schema = props.schema; const value = props.value ?? {}; if (schemaType(schema) !== "object" || !schema.properties) { return html`
Unsupported schema. Use Raw.
`; } + const unsupported = new Set(props.unsupportedPaths ?? []); const entries = Object.entries(schema.properties); const sorted = entries.sort((a, b) => { const orderA = hintForPath([a[0]], props.uiHints)?.order ?? 0; @@ -46,6 +52,8 @@ export function renderConfigForm(props: ConfigFormProps) { value: (value as Record)[key], path: [key], hints: props.uiHints, + unsupported, + disabled: props.disabled ?? false, onPatch: props.onPatch, }), )} @@ -58,13 +66,24 @@ function renderNode(params: { value: unknown; path: Array; hints: ConfigUiHints; + unsupported: Set; + disabled: boolean; + showLabel?: boolean; onPatch: (path: Array, value: unknown) => void; }) { - const { schema, value, path, hints, onPatch } = params; + const { schema, value, path, hints, unsupported, disabled, onPatch } = params; + const showLabel = params.showLabel ?? true; const type = schemaType(schema); const hint = hintForPath(path, hints); const label = hint?.label ?? schema.title ?? humanize(String(path.at(-1))); const help = hint?.help ?? schema.description; + const key = pathKey(path); + + if (unsupported.has(key)) { + return html`
+ ${label}: unsupported schema node. Use Raw. +
`; + } if (schema.anyOf || schema.oneOf || schema.allOf) { return html`
@@ -75,7 +94,11 @@ function renderNode(params: { if (type === "object") { const props = schema.properties ?? {}; const entries = Object.entries(props); - if (entries.length === 0) return nothing; + const hasMap = + schema.additionalProperties && + typeof schema.additionalProperties === "object"; + if (entries.length === 0 && !hasMap) return nothing; + const reservedKeys = new Set(entries.map(([key]) => key)); return html`
${label} @@ -86,9 +109,23 @@ function renderNode(params: { value: value && typeof value === "object" ? (value as any)[key] : undefined, path: [...path, key], hints, + unsupported, onPatch, + disabled, }), )} + ${hasMap + ? renderMapField({ + schema: schema.additionalProperties as JsonSchema, + value: value && typeof value === "object" ? (value as any) : {}, + path, + hints, + unsupported, + disabled, + reservedKeys, + onPatch, + }) + : nothing}
`; } @@ -101,14 +138,15 @@ function renderNode(params: { return html`
- ${label} -
@@ -121,11 +159,14 @@ function renderNode(params: { value: entry, path: [...path, index], hints, + unsupported, + disabled, onPatch, }) : nothing} +
+ ${entries.length === 0 + ? html`
No entries yet.
` + : entries.map(([key, entryValue]) => { + const valuePath = [...path, key]; + return html`
+ { + const nextKey = (e.target as HTMLInputElement).value.trim(); + if (!nextKey || nextKey === key) return; + const next = { ...(value ?? {}) }; + if (nextKey in next) return; + next[nextKey] = next[key]; + delete next[key]; + onPatch(path, next); + }} + /> +
+ ${renderNode({ + schema, + value: entryValue, + path: valuePath, + hints, + unsupported, + disabled, + showLabel: false, + onPatch, + })} +
+ +
`; + })} +
+ `; +} + +export type ConfigSchemaAnalysis = { + schema: JsonSchema | null; + unsupportedPaths: string[]; +}; + +export function analyzeConfigSchema(raw: unknown): ConfigSchemaAnalysis { + if (!raw || typeof raw !== "object") { + return { schema: null, unsupportedPaths: [""] }; + } + const result = normalizeSchemaNode(raw as JsonSchema, []); + return result; +} + +function normalizeSchemaNode( + schema: JsonSchema, + path: Array, +): ConfigSchemaAnalysis { + const unsupportedPaths: string[] = []; + const normalized = { ...schema }; + const pathLabel = pathKey(path) || ""; + + if (schema.anyOf || schema.oneOf || schema.allOf) { + const union = normalizeUnion(schema, path); + if (union) return union; + unsupportedPaths.push(pathLabel); + return { schema, unsupportedPaths }; + } + + const nullable = + Array.isArray(schema.type) && schema.type.includes("null"); + const type = + schemaType(schema) ?? + (schema.properties || schema.additionalProperties ? "object" : undefined); + normalized.type = type ?? schema.type; + normalized.nullable = nullable || schema.nullable; + + if (normalized.enum) { + const { enumValues, nullable: enumNullable } = normalizeEnumValues( + normalized.enum, + ); + normalized.enum = enumValues; + if (enumNullable) normalized.nullable = true; + if (enumValues.length === 0) { + unsupportedPaths.push(pathLabel); + } + } + + if (type === "object") { + const props = schema.properties ?? {}; + const normalizedProps: Record = {}; + for (const [key, child] of Object.entries(props)) { + const result = normalizeSchemaNode(child, [...path, key]); + if (result.schema) normalizedProps[key] = result.schema; + unsupportedPaths.push(...result.unsupportedPaths); + } + normalized.properties = normalizedProps; + + if (schema.additionalProperties === true) { + unsupportedPaths.push(pathLabel); + } else if (schema.additionalProperties === false) { + normalized.additionalProperties = false; + } else if (schema.additionalProperties) { + const result = normalizeSchemaNode( + schema.additionalProperties, + [...path, "*"], + ); + normalized.additionalProperties = result.schema ?? schema.additionalProperties; + if (result.unsupportedPaths.length > 0) { + unsupportedPaths.push(pathLabel); + } + } + } else if (type === "array") { + const itemSchema = Array.isArray(schema.items) + ? schema.items[0] + : schema.items; + if (!itemSchema) { + unsupportedPaths.push(pathLabel); + } else { + const result = normalizeSchemaNode(itemSchema, [...path, "*"]); + normalized.items = result.schema ?? itemSchema; + if (result.unsupportedPaths.length > 0) { + unsupportedPaths.push(pathLabel); + } + } + } else if ( + type === "string" || + type === "number" || + type === "integer" || + type === "boolean" + ) { + // ok + } else if (!normalized.enum) { + unsupportedPaths.push(pathLabel); + } + + return { + schema: normalized, + unsupportedPaths: Array.from(new Set(unsupportedPaths)), + }; +} + +function normalizeUnion( + schema: JsonSchema, + path: Array, +): ConfigSchemaAnalysis | null { + const variants = schema.anyOf ?? schema.oneOf ?? schema.allOf; + if (!variants) return null; + const values: unknown[] = []; + const nonLiteral: JsonSchema[] = []; + let nullable = false; + for (const variant of variants) { + if (!variant || typeof variant !== "object") return null; + if (Array.isArray(variant.enum)) { + const { enumValues, nullable: enumNullable } = normalizeEnumValues( + variant.enum, + ); + values.push(...enumValues); + if (enumNullable) nullable = true; + continue; + } + if ("const" in variant) { + if (variant.const === null || variant.const === undefined) { + nullable = true; + continue; + } + values.push(variant.const); + continue; + } + if (schemaType(variant) === "null") { + nullable = true; + continue; + } + nonLiteral.push(variant); + } + + if (values.length > 0 && nonLiteral.length === 0) { + const unique: unknown[] = []; + for (const value of values) { + if (!unique.some((entry) => Object.is(entry, value))) unique.push(value); + } + return { + schema: { + ...schema, + enum: unique, + nullable, + anyOf: undefined, + oneOf: undefined, + allOf: undefined, + }, + unsupportedPaths: [], + }; + } + + if (nonLiteral.length === 1) { + const result = normalizeSchemaNode(nonLiteral[0], path); + if (result.schema) { + result.schema.nullable = true; + } + return result; + } + + return null; +} + +function normalizeEnumValues(values: unknown[]) { + const filtered = values.filter((value) => value !== null && value !== undefined); + const nullable = filtered.length !== values.length; + const unique: unknown[] = []; + for (const value of filtered) { + if (!unique.some((entry) => Object.is(entry, value))) unique.push(value); + } + return { enumValues: unique, nullable }; +} diff --git a/ui/src/ui/views/config.browser.test.ts b/ui/src/ui/views/config.browser.test.ts new file mode 100644 index 000000000..c975c4631 --- /dev/null +++ b/ui/src/ui/views/config.browser.test.ts @@ -0,0 +1,44 @@ +import { render } from "lit"; +import { describe, expect, it, vi } from "vitest"; + +import { renderConfig } from "./config"; + +describe("config view", () => { + it("disables save when form is unsafe", () => { + const container = document.createElement("div"); + render( + renderConfig({ + raw: "{\n}\n", + valid: true, + issues: [], + loading: false, + saving: false, + connected: true, + schema: { + type: "object", + properties: { + mixed: { anyOf: [{ type: "string" }, { type: "number" }] }, + }, + }, + schemaLoading: false, + uiHints: {}, + formMode: "form", + formValue: { mixed: "x" }, + onRawChange: vi.fn(), + onFormModeChange: vi.fn(), + onFormPatch: vi.fn(), + onReload: vi.fn(), + onSave: vi.fn(), + }), + container, + ); + + const saveButton = Array.from( + container.querySelectorAll("button"), + ).find((btn) => btn.textContent?.trim() === "Save") as + | HTMLButtonElement + | undefined; + expect(saveButton).not.toBeUndefined(); + expect(saveButton?.disabled).toBe(true); + }); +}); diff --git a/ui/src/ui/views/config.ts b/ui/src/ui/views/config.ts index c11ee4204..27571d1ab 100644 --- a/ui/src/ui/views/config.ts +++ b/ui/src/ui/views/config.ts @@ -1,6 +1,6 @@ import { html, nothing } from "lit"; import type { ConfigUiHints } from "../types"; -import { renderConfigForm } from "./config-form"; +import { analyzeConfigSchema, renderConfigForm } from "./config-form"; export type ConfigProps = { raw: string; @@ -24,6 +24,16 @@ export type ConfigProps = { export function renderConfig(props: ConfigProps) { const validity = props.valid == null ? "unknown" : props.valid ? "valid" : "invalid"; + const analysis = analyzeConfigSchema(props.schema); + const formUnsafe = analysis.schema + ? analysis.unsupportedPaths.length > 0 + : false; + const canSaveForm = + Boolean(props.formValue) && !props.loading && !formUnsafe; + const canSave = + props.connected && + !props.saving && + (props.formMode === "raw" ? true : canSaveForm); return html`
@@ -52,7 +62,7 @@ export function renderConfig(props: ConfigProps) {
` : html`