Separate custom provider credentials from model configuration

docs/design/earendil-agent-harness-integration-adr/evidence/earendil-0991-custom-auth-storage.md

@@ -0,0 +1,33 @@
+# New custom-provider credential writes (#1458)
+
+New custom-provider keys use public `ModelRuntime.login()` and Piclaw's existing private credential store. Newly submitted keys are not written to `models.json` or new configuration backups. Provider logout does not create new snapshots of `auth.json`. This is evidence toward AUTH-04/05/06; the full criteria are not satisfied. Legacy keys and historical backups require a separate migration and retention decision.
+
+## Configuration and credential ownership
+
+Custom setup writes non-secret endpoint/model configuration, refreshes the public runtime offline, then invokes the composed provider's public API-key login. The submitted key answers only the provider-owned secret prompt. No private runtime member, alternate credential owner or SDK storage cast is used.
+
+A blank key retains an existing usable stored credential. Required-key custom providers reject absent or blank stored credentials. Keyless local configuration does not create a credential or invoke login. Availability for every external/local provider still needs separate qualification.
+
+Different providers share a path-scoped configuration queue so asynchronous credential checks cannot lose another provider's update. Each provider also uses the existing credential mutation queue. Custom setup retires the prior provider state only after admission to both queues; later setup takes effect in that order rather than superseding a credential already committing. Expected provider revisions fence model activation after setup.
+
+Configuration failures before credential commit restore the previous configuration bytes and refresh the runtime. Public `CredentialSynchronizationError` identifies a committed credential whose local snapshot could not refresh: the new configuration stays in place, the user gets an explicit saved-but-refresh-failed result, and no older credential is restored. Owner/revision changes after an admitted write also deny activation without restoring superseded credentials.
+
+Custom logout validates/parses and removes configuration before deleting stored credentials. Pre-delete failures restore configuration and preserve the credential. A post-delete synchronization failure does not resurrect a removed credential. Direct logout can remove keyless custom configuration as well as stored custom credentials.
+
+## New backups and compatibility
+
+New `models.json` backups omit each provider's `apiKey` field and are written with mode 0600. Active configuration writes also enforce 0600. The private credential store retains its existing 0600 file/0700 directory behavior. Backups preserve endpoint/model settings and are intentionally insufficient to recover credentials; reauthentication may be needed.
+
+An existing target `models.json` API key blocks reconfiguration before mutation. No silent migration, removal or duplication occurs. Existing backup files are not rewritten or deleted. Other providers' active configuration is preserved. This slice does not scan or redact arbitrary headers, URLs, keychain references or historical backup contents.
+
+## Offline tests
+
+Thirteen disposable child scenarios exercise the real public runtime and Piclaw file store: new key and logout; blank-key update; keyless setup/logout; legacy and malformed configuration refusal; missing required key; pre-commit write rollback; committed-credential snapshot failure; concurrent providers; blank stored key; logout-delete failure; ordered same-provider setup; and a blocked credential-commit window followed by blank-key setup.
+
+The credential-store test double subclasses the app-owned store to inject failures/barriers. It does not patch the public runtime. Each child inherits a minimal disposable profile, denies fetch/preconnect and emits only its scenario name. No inference method is called. Synthetic keys exist only in the private temporary credential file. Tests inspect active configuration/backups and verify unrelated credentials survive.
+
+The thirteen scenarios passed 39 parent assertions plus child assertions. Related handler/isolation/card-service tests passed 52 tests and 408 assertions; the combined twelve-scenario run passed 64 tests and 444 assertions before the final commit-window case was added. Five standard typechecks, explicit strict fixture typechecking, scoped lint and diff checks passed. Independent reviews found and corrected configuration races, rollback, logout and commit-window issues; final scoped review found no blocker in the tested paths. At merged baseline `2b0f5aaa3ae7d0eeb6931535e8513331fecab72c`, `make ci-fast` passed 5,942 runtime tests, seven existing skips and no failures, plus 25 feature tests and nine web checks. Pack hygiene passed 24,741 files; final five typechecks and diff checks passed with the unchanged 95-diagnostic compose baseline. Private Bun caches were used without shared-cache permission changes.
+
+## Remaining qualification
+
+The configuration queue is process-local; cross-process configuration writers and crash interruption are unqualified. Existing literal keys, credential-bearing backups, complete secret/trace scans, provider-specific external auth, full CLI/device/browser matrix, Delegate children and approved live accounts remain open. This change does not certify rollback by restoring old OAuth tokens. No live account, provider request, deployment or restart was used.

runtime/src/agent-control/handlers/login.ts

@@ -13,10 +13,10 @@
  * custom-provider models.json configuration, with backups and awaited reload.
  */
 
-import type { AgentSession, ModelRegistry, ModelRuntime } from "@earendil-works/pi-coding-agent";
+import { CredentialSynchronizationError, type AgentSession, type ModelRegistry, type ModelRuntime } from "@earendil-works/pi-coding-agent";
 import type { AuthEvent, AuthPrompt, AuthType, CredentialInfo } from "@earendil-works/pi-ai";
 import type { AgentControlCommand, AgentControlResult } from "../agent-control-types.js";
-import { writeFileSync, readFileSync, existsSync, copyFileSync } from "fs";
+import { writeFileSync, readFileSync, existsSync, chmodSync, unlinkSync } from "fs";
 import { join } from "path";
 import { randomUUID } from "node:crypto";
 import { getPiclawAgentDir } from "../../core/agent-dir.js";
@@ -56,18 +56,30 @@ interface ModelRegistryLike {
 
 // ── Config paths ────────────────────────────────────────────────
 
-function getAuthJsonPath(): string {
-  return join(getPiclawAgentDir(), "auth.json");
-}
-
 function getModelsJsonPath(): string {
   return join(getPiclawAgentDir(), "models.json");
 }
 
-function backupFile(path: string): void {
+// Different providers share one configuration file; provider locks alone do
+// not protect its read/modify/write across asynchronous auth operations.
+const modelConfigWrites = new Map<string, Promise<void>>();
+async function serializeModelsConfig<T>(path: string, run: () => Promise<T>): Promise<T> {
+  const prior = modelConfigWrites.get(path) ?? Promise.resolve();
+  const operation = prior.then(run);
+  const tail = operation.then(() => undefined, () => undefined);
+  modelConfigWrites.set(path, tail);
+  try { return await operation; }
+  finally { if (modelConfigWrites.get(path) === tail) modelConfigWrites.delete(path); }
+}
+
+function backupModelsConfig(path: string): void {
   if (!existsSync(path)) return;
+  const snapshot = JSON.parse(readFileSync(path, "utf-8")) as { providers?: Record<string, Record<string, unknown>> };
+  // New backups must not duplicate provider API keys. Existing backups are
+  // preserved for a separately reviewed migration/retention decision.
+  for (const provider of Object.values(snapshot.providers ?? {})) delete provider.apiKey;
   const ts = new Date().toISOString().replace(/[:.]/g, "-");
-  copyFileSync(path, `${path}.${ts}.bak`);
+  writeJsonFile(`${path}.${ts}.bak`, snapshot);
 }
 
 function readJsonFile(path: string): Record<string, unknown> {
@@ -76,7 +88,8 @@ function readJsonFile(path: string): Record<string, unknown> {
 }
 
 function writeJsonFile(path: string, data: unknown): void {
-  writeFileSync(path, JSON.stringify(data, null, 2) + "\n", "utf-8");
+  writeFileSync(path, JSON.stringify(data, null, 2) + "\n", { encoding: "utf-8", mode: 0o600 });
+  chmodSync(path, 0o600);
 }
 
 // ── Provider definitions ────────────────────────────────────────
@@ -193,7 +206,7 @@ function buildCard2Config(def: ProviderDef): Record<string, unknown> {
 
   const body: unknown[] = [
     { type: "TextBlock", text: `${def.name} — Configuration`, weight: "Bolder", size: "Medium" },
-    { type: "TextBlock", text: "Saved to `~/.pi/agent/models.json` (backup created first) and applied immediately.", wrap: true, isSubtle: true },
+    { type: "TextBlock", text: "Model configuration is applied immediately. Keys are saved through provider authentication, not in models.json or its new backups. Leave the key blank to retain a stored credential.", wrap: true, isSubtle: true },
   ];
 
   for (const field of def.customFields || []) {
@@ -210,7 +223,7 @@ function buildCard2Config(def: ProviderDef): Record<string, unknown> {
     if (field.key === "apiKey") currentValue = "";
     body.push({
       type: "Input.Text", id: field.key,
-      label: `${field.label}${field.required ? " *" : ""}`,
+      label: `${field.label}${field.required && field.key !== "apiKey" ? " *" : ""}`,
       placeholder: field.placeholder, value: currentValue,
       ...(field.key === "apiKey" ? { style: "password" } : {}),
     });
@@ -809,14 +822,60 @@ async function handleStep2(
       ...(def.customCompat ? { compat: def.customCompat } : {}),
     }));
 
-    backupFile(getModelsJsonPath());
-    const modelsJson = readJsonFile(getModelsJsonPath()) as { providers?: Record<string, unknown> };
-    if (!modelsJson.providers) modelsJson.providers = {};
-    modelsJson.providers[providerId] = { baseUrl, api: def.customApi || "openai-completions", ...(apiKey ? { apiKey } : {}), models };
-    writeJsonFile(getModelsJsonPath(), modelsJson);
-    await modelRuntime.refresh({ allowNetwork: false });
-
-    return await showCard3OrComplete(session, modelRegistry, def, providerId, name, registry);
+    const owner = getRuntimeAuthOwner(session);
+    let revision = -1;
+    const active = () => isRuntimeAuthOwnerActive(owner) && revision === providerAuthState(modelRuntime, providerId).revision;
+    try {
+      const path = getModelsJsonPath();
+      await serializeProviderAuth(modelRuntime, providerId, () => serializeModelsConfig(path, async () => {
+        // Custom setup admission is ordered with credential writes. A later
+        // setup cannot supersede a credential that is already committing.
+        retireProviderAuth(modelRuntime, providerId);
+        revision = providerAuthState(modelRuntime, providerId).revision;
+        if (!active()) throw new Error("Authentication owner replaced");
+        const original = existsSync(path) ? readFileSync(path, "utf-8") : null;
+        const modelsJson = (original === null ? {} : JSON.parse(original)) as { providers?: Record<string, Record<string, unknown>> };
+        // Never silently discard or migrate a legacy credential. That path
+        // needs an explicit compatibility and historical-backup decision.
+        if (modelsJson.providers?.[providerId]?.apiKey) throw new Error("Legacy key migration required");
+        const stored = (await modelRuntime.listCredentials()).some(entry => entry.providerId === providerId);
+        if (def.customFields?.some(field => field.key === "apiKey" && field.required) && !apiKey && (!stored || !(await modelRuntime.getAuth(providerId))?.auth.apiKey)) throw new Error("API key required");
+        if (!active()) throw new Error("Authentication owner replaced");
+        backupModelsConfig(path);
+        if (!modelsJson.providers) modelsJson.providers = {};
+        modelsJson.providers[providerId] = { baseUrl, api: def.customApi || "openai-completions", models };
+        let written = false;
+        try {
+          writeJsonFile(path, modelsJson);
+          written = true;
+          await modelRuntime.refresh({ allowNetwork: false });
+          if (!active()) throw new Error("Authentication owner replaced");
+          if (apiKey) await modelRuntime.login(providerId, "api_key", {
+            prompt: async prompt => {
+              if (!active() || prompt.type !== "secret") throw new Error("Unsupported custom credential prompt");
+              return apiKey;
+            },
+            notify: () => {},
+          });
+          if (!active()) throw new CredentialSynchronizationError(providerId, "login", undefined, { cause: new Error("Authentication owner changed after commit") });
+        } catch (error) {
+          // The public runtime distinguishes a committed credential from a
+          // post-write snapshot failure. Never restore a superseded credential.
+          if (error instanceof CredentialSynchronizationError) throw error;
+          if (written) {
+            if (original === null) unlinkSync(path);
+            else { writeFileSync(path, original, { encoding: "utf-8", mode: 0o600 }); chmodSync(path, 0o600); }
+            await modelRuntime.refresh({ allowNetwork: false });
+          }
+          throw error;
+        }
+      }));
+    } catch (error) {
+      return { status: "error", message: error instanceof CredentialSynchronizationError
+        ? "The credential was saved, but model availability could not be refreshed. Retry model refresh; do not restore an older credential."
+        : "Custom authentication configuration failed. Supply a required key or review legacy model configuration, then retry. The prior configuration and stored credential are retained." };
+    }
+    return await showCard3OrComplete(session, modelRegistry, def, providerId, name, registry, revision);
   }
 
   if (method === "logout") {
@@ -825,23 +884,45 @@ async function handleStep2(
     if (!confirmation || confirmation.provider !== providerId || confirmation.expiresAt <= Date.now() || confirmation.providerRevision !== providerAuthState(modelRuntime, providerId).revision) return { status: "error", message: "Stale or foreign logout confirmation. Use /logout or request a new confirmation." };
     owner.logouts.delete(id);
     retireProviderAuth(modelRuntime, providerId);
-    backupFile(getAuthJsonPath());
-    await serializeProviderAuth(modelRuntime, providerId, () => modelRuntime.logout(providerId));
-    if (def?.isCustom) {
-      const modelsJson = readJsonFile(getModelsJsonPath()) as { providers?: Record<string, unknown> };
-      if (modelsJson.providers?.[providerId]) {
-        backupFile(getModelsJsonPath());
-        delete modelsJson.providers[providerId];
-        writeJsonFile(getModelsJsonPath(), modelsJson);
-        await modelRuntime.refresh({ allowNetwork: false });
-      }
-    }
-    return { status: "success", message: `✓ **${name}** removed. Backups created.` };
+    await serializeProviderAuth(modelRuntime, providerId, async () => {
+      if (def?.isCustom) await removeCustomModelConfig(modelRuntime, providerId, true);
+      else await modelRuntime.logout(providerId);
+    });
+    return { status: "success", message: `✓ **${name}** removed. New configuration backups omit API keys; credentials are not snapshotted.` };
   }
 
   return { status: "error", message: `Unknown method: ${method}` };
 }
 
+async function removeCustomModelConfig(modelRuntime: ModelRuntime, providerId: string, removeCredential: boolean): Promise<boolean> {
+  const path = getModelsJsonPath();
+  return serializeModelsConfig(path, async () => {
+    const original = existsSync(path) ? readFileSync(path, "utf-8") : null;
+    const config = (original === null ? {} : JSON.parse(original)) as { providers?: Record<string, unknown> };
+    const configured = Boolean(config.providers?.[providerId]);
+    let written = false;
+    try {
+      // Configuration errors must not remove an otherwise working credential.
+      if (configured) {
+        backupModelsConfig(path);
+        delete config.providers![providerId];
+        writeJsonFile(path, config);
+        written = true;
+        await modelRuntime.refresh({ allowNetwork: false });
+      }
+      if (removeCredential) await modelRuntime.logout(providerId);
+    } catch (error) {
+      if (written && !(error instanceof CredentialSynchronizationError)) {
+        writeFileSync(path, original!, { encoding: "utf-8", mode: 0o600 });
+        chmodSync(path, 0o600);
+        await modelRuntime.refresh({ allowNetwork: false });
+      }
+      throw error;
+    }
+    return configured;
+  });
+}
+
 async function activateProviderModel(
   session: AgentSession,
   modelRegistry: ModelRegistry,
@@ -864,9 +945,10 @@ async function showCard3OrComplete(
   providerId: string,
   name: string,
   registry: ModelRegistryLike,
+  expectedRevision?: number,
 ): Promise<AgentControlResult> {
   const owner = getRuntimeAuthOwner(session);
-  const providerRevision = providerAuthState(owner.runtime, providerId).revision;
+  const providerRevision = expectedRevision ?? providerAuthState(owner.runtime, providerId).revision;
   await registry.refresh?.();
   if (!isRuntimeAuthOwnerActive(owner) || owner.chatJid !== getChatJid() || providerRevision !== providerAuthState(owner.runtime, providerId).revision) return { status: "error", message: "Authentication session changed. Use /login in the active session." };
   const models = registry.getAll().filter((m) => m.provider === providerId);
@@ -964,10 +1046,11 @@ export async function handleLogout(
     retireProviderAuth(modelRuntime, providerId);
     const removed = await serializeProviderAuth(modelRuntime, providerId, async () => {
       const credentials = await modelRuntime.listCredentials();
-      if (!credentials.some((entry) => entry.providerId === providerId)) return false;
-      backupFile(getAuthJsonPath());
-      await modelRuntime.logout(providerId);
-      return true;
+      const stored = credentials.some((entry) => entry.providerId === providerId);
+      const custom = getProviderDef(modelRuntime, registry, providerId)?.isCustom;
+      const configured = custom ? await removeCustomModelConfig(modelRuntime, providerId, stored) : false;
+      if (stored && !custom) await modelRuntime.logout(providerId);
+      return stored || configured;
     });
     if (!removed) return { status: "error", message: `**${providerId}** is not logged in.` };
     return { status: "success", message: `✓ Logged out from **${providerId}**.` };