diff --git a/docs/design/earendil-agent-harness-integration-adr/evidence/earendil-0991-stream-credential-errors.md b/docs/design/earendil-agent-harness-integration-adr/evidence/earendil-0991-stream-credential-errors.md new file mode 100644 index 000000000..cd849e189 --- /dev/null +++ b/docs/design/earendil-agent-harness-integration-adr/evidence/earendil-0991-stream-credential-errors.md @@ -0,0 +1,38 @@ +# Public runtime credential error isolation + +Piclaw's public `ModelRuntime.stream` and `streamSimple` emitted raw credential-refresh diagnostics before this fix. A synthetic expired OAuth credential produced a private sentinel in both terminal error events and `result()`, with zero provider inference calls. Earendil 0.99.1 `ModelsError` appends cause text and lazy stream setup turns that message into an assistant error. + +## Protected boundary + +`createRuntimeModelServices` now injects a public `CredentialStore` facade around the app-owned backing store. It forwards `read`, `list`, `modify`, `delete`, provider IDs, callback identity and operation options unchanged. The backing store completes its locking, retry and persistence behaviour before the facade handles a rejected operation; the original store remains available internally. + +The facade replaces diagnostics with a new error without the original message, cause, stack or custom properties: + +| Failure | Public message | Recovery category | +|---|---|---| +| Permanent or unclassified | `Provider login required. Model credentials could not be resolved.` | `auth_config` | +| Transient | `Model credential service temporarily unavailable (503).` | `network` | +| Cancellation | `Credential operation aborted.` (`AbortError`) | `aborted` | + +Transient classification uses the existing private OAuth/store classifier after its retry sequence completes. It never forwards raw diagnostics. Getter/proxy inspection failures fall back to the permanent generic message; secondary exceptions cannot escape. Error-like and `DOMException` cancellation names are recognised under that guarded inspection. + +## Synthetic qualification + +Public runtime tests execute both stream methods with a native synthetic provider and app-owned file store: + +- permanent refresh failure makes one refresh attempt per stream, exposes only the generic auth message and retains old credentials; +- exhausted transient refresh makes two real store-managed attempts per stream, preserves network recovery classification and old credentials; +- malformed storage exposes only the generic auth message; +- successful retry is tested through public `getAuth`: first refresh throws, second rotates, subsequent resolution reuses the committed credential. + +All cases forbid provider inference and network requests. Captured logs and stream event/results must omit the private sentinel. Unit tests cover every store operation, argument/callback/result forwarding, hostile diagnostic getters, cancellation with signals and `DOMException`, and public recovery classification. Existing store/lifecycle tests remain unchanged apart from the service-injection identity assertion. + +## Failures and scope + +Initial child tests reached their assertions but timed out because the process stayed alive after the result. No public runtime disposal API was found. The owned fixture now exits explicitly after assertions, fetch restoration and profile cleanup; natural runtime teardown is unqualified. A retry-success assertion initially used `AuthResult.apiKey` instead of public `AuthResult.auth.apiKey`; it failed and was corrected from the public declaration. Review also found diagnostic-getter and `DOMException` cancellation gaps; both were fixed and re-reviewed. + +The facade protects rejected app-owned credential-store operations entering Piclaw's runtime. Direct backing-store callers, provider login/`toAuth` failures outside store operations, unrelated provider transport errors, historical data, live providers, Delegate parity and native MCP acceptance are outside this receipt. No live credentials, inference, deployment or restart. + +## Validation + +Focused regression/lifecycle/store set: 43 tests / 243 assertions. Five typechecks, strict fixture typing, scoped Oxlint, silent-swallow, local-entrypoint and diff checks pass. Independent review has no remaining blockers. At baseline `53b9c529590e1d703633ea05cb5aeb5d80f278f0`, `make ci-fast` passed 5,988 runtime tests, eight existing/opt-in skips and no failures, plus 25 feature tests and nine web checks. Pack hygiene passed 24,743 files; final five typechecks passed with the unchanged 95-diagnostic compose baseline. Private Bun caches were used without shared-cache permission changes. diff --git a/runtime/src/agent-pool/model-services.ts b/runtime/src/agent-pool/model-services.ts index 71df723d4..51fc49f01 100644 --- a/runtime/src/agent-pool/model-services.ts +++ b/runtime/src/agent-pool/model-services.ts @@ -9,6 +9,7 @@ import type { ModelsRefreshOptions, ModelsRefreshResult } from "@earendil-works/ import { getPiclawAgentDir } from "../core/agent-dir.js"; import { FileCredentialStore, type PiclawCredentialStore } from "./credential-store.js"; +import { createRuntimeCredentialStore } from "./runtime-credential-store.js"; import { installOpenAICompletionsUsageCompatibility } from "./openai-completions-usage-compat.js"; export class PiclawModelRegistry extends ModelRegistry { @@ -50,7 +51,7 @@ export async function createRuntimeModelServices( const credentialStore = options.credentialStore ?? new FileCredentialStore(join(agentDir, "auth.json")); const createModelRuntime = options.createModelRuntime ?? ((runtimeOptions) => ModelRuntime.create(runtimeOptions)); const modelRuntime = await createModelRuntime({ - credentials: credentialStore, + credentials: createRuntimeCredentialStore(credentialStore), authPath: join(agentDir, "auth.json"), modelsPath: join(agentDir, "models.json"), modelsStorePath: join(agentDir, "models-store.json"), diff --git a/runtime/src/agent-pool/runtime-credential-store.ts b/runtime/src/agent-pool/runtime-credential-store.ts new file mode 100644 index 000000000..7959de9fd --- /dev/null +++ b/runtime/src/agent-pool/runtime-credential-store.ts @@ -0,0 +1,38 @@ +import type { CredentialStore } from "@earendil-works/pi-ai"; +import { isTransientOAuthRefreshError } from "./credential-store.js"; + +/** Keep credential/store diagnostics out of public runtime auth error events. */ +function publicCredentialError(error: unknown, signal?: AbortSignal): Error { + // Provider-controlled diagnostic getters can throw. A secondary exception + // must never bypass this boundary and expose its own message or cause. + try { + const abortException = error !== null && typeof error === "object" && "name" in error && error.name === "AbortError"; + if (signal?.aborted || abortException) { + const aborted = new Error("Credential operation aborted."); + aborted.name = "AbortError"; + return aborted; + } + // Classify privately once, without retaining raw diagnostic properties. + if (isTransientOAuthRefreshError(error)) return new Error("Model credential service temporarily unavailable (503)."); + } catch { + return new Error("Provider login required. Model credentials could not be resolved."); + } + return new Error("Provider login required. Model credentials could not be resolved."); +} + +/** + * Public CredentialStore facade for ModelRuntime. The backing store completes + * locking/retries first, so sanitization does not change its transaction logic. + */ +export function createRuntimeCredentialStore(store: CredentialStore): CredentialStore { + const protect = async (operation: () => Promise, signal?: AbortSignal): Promise => { + try { return await operation(); } + catch (error) { throw publicCredentialError(error, signal); } + }; + return { + read: (provider, options) => protect(() => store.read(provider, options), options?.signal), + list: options => protect(() => store.list(options), options?.signal), + modify: (provider, fn, options) => protect(() => store.modify(provider, fn, options), options?.signal), + delete: (provider, options) => protect(() => store.delete(provider, options), options?.signal), + }; +} diff --git a/runtime/test/agent-control/fixtures/provider-auth-stream-errors-0991.ts b/runtime/test/agent-control/fixtures/provider-auth-stream-errors-0991.ts new file mode 100644 index 000000000..5bf35285c --- /dev/null +++ b/runtime/test/agent-control/fixtures/provider-auth-stream-errors-0991.ts @@ -0,0 +1,81 @@ +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import type { Model, Provider } from "@earendil-works/pi-ai"; +import { createRuntimeModelServices } from "../../../src/agent-pool/model-services.js"; +import { FileCredentialStore } from "../../../src/agent-pool/credential-store.js"; +import { classifyOpaqueAgentFailure } from "../../../src/agent-pool/automatic-recovery.js"; +import { addLogSink, removeLogSink, type LogRecord } from "../../../src/utils/logger.js"; + +const mode = process.argv[2]; +assert.ok(["refresh-permanent", "refresh-transient", "malformed-storage", "refresh-retry-success"].includes(mode)); +const packageRoot = dirname(dirname(fileURLToPath(import.meta.resolve("@earendil-works/pi-coding-agent")))); +assert.equal(JSON.parse(readFileSync(join(packageRoot, "package.json"), "utf8")).version, "0.99.1"); +const root = mkdtempSync(join(tmpdir(), "piclaw-auth-stream-")); +const sentinel = "PRIVATE-STREAM-CREDENTIAL-SENTINEL"; +let requests = 0, inference = 0, refreshes = 0; +const originalFetch = globalThis.fetch; +const denyNetwork = () => { requests++; throw new Error("Unexpected fixture network."); }; +globalThis.fetch = Object.assign(denyNetwork, { preconnect: denyNetwork }) as typeof fetch; +const logs: LogRecord[] = []; +const sink = (record: LogRecord) => logs.push(record); +addLogSink(sink); +try { + const credentials = new FileCredentialStore(join(root, "agent", "auth.json"), { maxRetries: 1, baseDelayMs: 1, maxDelayMs: 1, random: () => 0 }); + const id = "synthetic-stream-auth"; + await credentials.modify(id, async () => ({ type: "oauth", access: "synthetic-expired", refresh: "synthetic-refresh", expires: 1 })); + const { modelRuntime } = await createRuntimeModelServices({ agentDir: join(root, "agent"), credentialStore: credentials }); + const model: Model<"openai-completions"> = { provider: id, id: "fixture", name: "Fixture", api: "openai-completions", baseUrl: "https://fixture.invalid", input: ["text"], reasoning: false, contextWindow: 10000, maxTokens: 1000, cost: { input: 0, output: 0, cacheRead: 0, cacheWrite: 0 } }; + const provider: Provider = { id, name: "Fixture", getModels: () => [model], + stream: () => { inference++; throw new Error("Inference forbidden."); }, streamSimple: () => { inference++; throw new Error("Inference forbidden."); }, + auth: { oauth: { name: "Fixture", login: async () => { throw new Error("Login forbidden."); }, refresh: async () => { + refreshes++; + if (mode === "refresh-retry-success" && refreshes === 2) return { type: "oauth", access: "synthetic-rotated", refresh: "synthetic-rotated-refresh", expires: Date.now() + 3_600_000 }; + throw new Error(`${mode === "refresh-transient" || mode === "refresh-retry-success" ? "503 temporarily unavailable" : "invalid_grant"} ${sentinel}`, { cause: new Error(`refresh=${sentinel}`) }); + }, toAuth: async current => ({ apiKey: current.access }) } }, + }; + modelRuntime.registerNativeProvider(provider); + const results = []; + if (mode === "malformed-storage") { + const { writeFileSync } = await import("node:fs"); + writeFileSync(credentials.authPath, `{ broken-json-${sentinel}`); + } + if (mode === "refresh-retry-success") { + // Exercise successful refresh retries through public runtime auth setup. + // Do not submit a completion merely to verify usable rotated credentials. + const result = await modelRuntime.getAuth(model); + assert.equal(refreshes, 2); assert.equal(result?.auth.apiKey, "synthetic-rotated"); + const stored = await credentials.read(id); + assert.equal(stored?.type, "oauth"); + if (stored?.type === "oauth") assert.equal(stored.refresh, "synthetic-rotated-refresh"); + assert.equal((await modelRuntime.getAuth(model))?.auth.apiKey, "synthetic-rotated"); + assert.equal(refreshes, 2); + results.push({ method: "getAuth", retryAttempts: refreshes, status: "pass" }); + } else for (const method of ["stream", "streamSimple"] as const) { + const before = refreshes; + const stream = modelRuntime[method](model, { messages: [] }); + const events = []; + for await (const event of stream) events.push(event); + const message = await stream.result(); + assert.equal(message.stopReason, "error"); assert.equal(events.length, 1); assert.equal(events[0].type, "error"); + const encoded = JSON.stringify({ events, message }); + assert.ok(!encoded.includes(sentinel)); + const transient = mode === "refresh-transient"; + assert.ok(message.errorMessage?.includes(transient ? "Model credential service temporarily unavailable (503)." : "Provider login required. Model credentials could not be resolved.")); + assert.equal(classifyOpaqueAgentFailure(message.errorMessage), transient ? "network" : "auth_config"); + assert.equal(refreshes - before, transient ? 2 : mode === "refresh-permanent" ? 1 : 0); + if (mode !== "malformed-storage") { + const stored = JSON.parse(readFileSync(credentials.authPath, "utf8"))[id]; + assert.equal(stored.access, "synthetic-expired"); assert.equal(stored.refresh, "synthetic-refresh"); + } + results.push({ method, status: "pass", failureCategory: transient ? "network" : "auth_config", refreshAttempts: refreshes - before }); + } + assert.ok(!JSON.stringify(logs).includes(sentinel)); assert.equal(requests, 0); assert.equal(inference, 0); + console.log(JSON.stringify({ version: "0.99.1", mode, results, networkRequests: requests, inference, logsRedacted: true })); +} finally { globalThis.fetch = originalFetch; removeLogSink(sink); rmSync(root, { recursive: true, force: true }); } +// This owned fixture has no public runtime disposal API. Explicitly terminate +// after assertions and filesystem cleanup; natural runtime teardown is outside +// this auth-error regression's scope. +process.exit(0); diff --git a/runtime/test/agent-control/provider-auth-stream-errors-0991.test.ts b/runtime/test/agent-control/provider-auth-stream-errors-0991.test.ts new file mode 100644 index 000000000..82dd9b585 --- /dev/null +++ b/runtime/test/agent-control/provider-auth-stream-errors-0991.test.ts @@ -0,0 +1,20 @@ +import { expect, test } from "bun:test"; +import { dirname, resolve } from "node:path"; +for (const mode of ["refresh-permanent", "refresh-transient", "malformed-storage", "refresh-retry-success"]) { + test(`public runtime credential error isolation: ${mode}`, async () => { + const child = Bun.spawn([process.execPath, "--no-env-file", resolve(import.meta.dir, "fixtures/provider-auth-stream-errors-0991.ts"), mode], { + env: { PATH: `${dirname(process.execPath)}:/usr/bin:/bin`, HOME: "/nonexistent", PI_OFFLINE: "1", PI_TELEMETRY: "0", OTEL_SDK_DISABLED: "true" }, stdout: "pipe", stderr: "pipe", + }); + const timer = setTimeout(() => child.kill("SIGKILL"), 15_000); + try { + const [exit, out, err] = await Promise.all([child.exited, new Response(child.stdout).text(), new Response(child.stderr).text()]); + expect(exit, err).toBe(0); + const result = JSON.parse(out); + expect(result).toMatchObject({ version: "0.99.1", mode, networkRequests: 0, inference: 0, logsRedacted: true }); + expect(result.results.every((row: { status: string }) => row.status === "pass")).toBe(true); + expect(result.results.map((row: { method: string }) => row.method)).toEqual(mode === "refresh-retry-success" ? ["getAuth"] : ["stream", "streamSimple"]); + expect(out).not.toContain("PRIVATE-STREAM-CREDENTIAL-SENTINEL"); + expect(err).not.toContain("PRIVATE-STREAM-CREDENTIAL-SENTINEL"); + } finally { clearTimeout(timer); if (child.exitCode === null) child.kill("SIGKILL"); await child.exited; } + }, 20_000); +} diff --git a/runtime/test/agent-pool/model-services.test.ts b/runtime/test/agent-pool/model-services.test.ts index 786897d9b..5dd1cf546 100644 --- a/runtime/test/agent-pool/model-services.test.ts +++ b/runtime/test/agent-pool/model-services.test.ts @@ -44,7 +44,10 @@ describe("runtime model services", () => { modelsStorePath: join(agentDir, "models-store.json"), allowModelNetwork: false, }); - expect(captured?.credentials).toBe(services.credentialStore); + const runtimeOptions = captured as CreateModelRuntimeOptions | null; + expect(runtimeOptions?.credentials).not.toBe(services.credentialStore); + await runtimeOptions!.credentials!.modify("fixture", async () => ({ type: "api_key", key: "synthetic-key" })); + expect(await services.credentialStore.read("fixture")).toEqual({ type: "api_key", key: "synthetic-key" }); expect(services.modelRuntime).toBe(fakeRuntime); expect(services.modelRegistry).toBeInstanceOf(PiclawModelRegistry); }); diff --git a/runtime/test/agent-pool/runtime-credential-store.test.ts b/runtime/test/agent-pool/runtime-credential-store.test.ts new file mode 100644 index 000000000..c24b7d384 --- /dev/null +++ b/runtime/test/agent-pool/runtime-credential-store.test.ts @@ -0,0 +1,71 @@ +import { expect, test } from "bun:test"; +import type { CredentialStore } from "@earendil-works/pi-ai"; +import { createRuntimeCredentialStore } from "../../src/agent-pool/runtime-credential-store.js"; +import { classifyOpaqueAgentFailure } from "../../src/agent-pool/automatic-recovery.js"; + +const sentinel = "PRIVATE-CREDENTIAL-ERROR-SENTINEL"; +for (const [name, thrown, expected, category] of [ + ["permanent", new Error(`invalid_grant ${sentinel}`, { cause: new Error(`refresh=${sentinel}`) }), "Provider login required. Model credentials could not be resolved.", "auth_config"], + ["transient", new Error(`503 refresh_token=${sentinel}`), "Model credential service temporarily unavailable (503).", "network"], + ["malformed-storage", new SyntaxError(`auth.json ${sentinel}`), "Provider login required. Model credentials could not be resolved.", "auth_config"], + ["non-error", `invalid_grant ${sentinel}`, "Provider login required. Model credentials could not be resolved.", "auth_config"], +] as const) { + test(`runtime credential facade sanitizes ${name} across every store operation`, async () => { + const fail = async () => { throw thrown; }; + const store = createRuntimeCredentialStore({ read: fail, list: fail, modify: fail, delete: fail }); + for (const operation of [() => store.read("fixture"), () => store.list(), () => store.modify("fixture", async current => current), () => store.delete("fixture")]) { + const error = await operation().catch(error => error); + expect(error).toBeInstanceOf(Error); expect(error.message).toBe(expected); + expect(error.cause).toBeUndefined(); expect(JSON.stringify(error)).not.toContain(sentinel); + expect(error.stack).not.toContain(sentinel); expect(classifyOpaqueAgentFailure(error.message)).toBe(category); + } + }); +} + +for (const property of ["name", "message", "cause", "status", "toString"]) { + test(`runtime facade fails closed when private diagnostic ${property} inspection throws`, async () => { + const thrown = property === "status" || property === "toString" ? {} : new Error("private"); + Object.defineProperty(thrown, property, { get: () => { throw new Error(sentinel); } }); + const fail = async () => { throw thrown; }; + const store = createRuntimeCredentialStore({ read: fail, list: fail, modify: fail, delete: fail }); + const error = await store.read("fixture").catch(error => error); + expect(error.message).toBe("Provider login required. Model credentials could not be resolved."); + expect(error.stack).not.toContain(sentinel); expect(error.cause).toBeUndefined(); + }); +} + +test("runtime facade forwards callback, options and successful credentials without changing store semantics", async () => { + const credential = { type: "api_key" as const, key: "synthetic-key" }; + const options = { signal: new AbortController().signal }; + const fn = async () => credential; + const calls: unknown[] = []; + const underlying: CredentialStore = { + read: async (...args) => { calls.push(args); return credential; }, + list: async (...args) => { calls.push(args); return [{ providerId: "fixture", type: "api_key" }]; }, + modify: async (...args) => { calls.push(args); return args[1](credential); }, + delete: async (...args) => { calls.push(args); }, + }; + const store = createRuntimeCredentialStore(underlying); + expect(await store.read("fixture", options)).toBe(credential); + expect(await store.list(options)).toEqual([{ providerId: "fixture", type: "api_key" }]); + expect(await store.modify("fixture", fn, options)).toBe(credential); + await store.delete("fixture", options); + expect(calls).toEqual([["fixture", options], [options], ["fixture", fn, options], ["fixture", options]]); +}); + +test("DOMException cancellation retains category without an explicitly aborted signal", async () => { + const fail = async () => { throw new DOMException(sentinel, "AbortError"); }; + const store = createRuntimeCredentialStore({ read: fail, list: fail, modify: fail, delete: fail }); + const error = await store.read("fixture").catch(error => error); + expect(error.name).toBe("AbortError"); expect(error.message).toBe("Credential operation aborted."); + expect(error.cause).toBeUndefined(); expect(error.stack).not.toContain(sentinel); + expect(classifyOpaqueAgentFailure(error.message)).toBe("aborted"); +}); + +test("aborted credential operations retain cancellation category without provider text", async () => { + const fail = async () => { throw new Error(`invalid_grant ${sentinel}`); }; + const store = createRuntimeCredentialStore({ read: fail, list: fail, modify: fail, delete: fail }); + const error = await store.read("fixture", { signal: AbortSignal.abort() }).catch(error => error); + expect(error.name).toBe("AbortError"); expect(error.message).toBe("Credential operation aborted."); + expect(error.cause).toBeUndefined(); expect(classifyOpaqueAgentFailure(error.message)).toBe("aborted"); +});