From a0235ea9bba0a17656694ba4a94151e5e1170925 Mon Sep 17 00:00:00 2001 From: francisco-orkes Date: Wed, 29 Jul 2026 08:56:16 -0700 Subject: [PATCH 1/2] fix(agents): keep AGENTSPAN_* env vars and AgentspanError working The Agentspan -> Conductor rename (#147) was a hard break for existing deployments. Nothing warned, and two of the three failure modes are silent: - AGENTSPAN_SERVER_URL / AUTH_KEY / AUTH_SECRET are no longer read at all. resolveOrkesConfig previously had an agent-layer tier; that tier is now gone, so an app configured this way falls through to the http://localhost:8080 default and fails against the wrong server with no indication why. - AGENTSPAN_WORKER_*, LIVENESS_*, STREAMING_*, AUTO_START_WORKERS and AGENTSPAN_LLM_MODEL silently revert to defaults. - AgentspanError was renamed with no alias, so `import { AgentspanError }` and `catch (e) { e instanceof AgentspanError }` stop compiling. That one at least fails loudly. This restores all three as deprecated fallbacks. Each legacy name maps to whatever the code reads *now* rather than reintroducing the agent-layer connection tier #147 deliberately removed -- connection/auth stays on the core CONDUCTOR_* vars, agent behaviour knobs stay on CONDUCTOR_AGENT_*. That keeps #147's design intact while making the migration non-breaking. - src/agents/legacy-env.ts readRenamedEnv(), warn-once per name - src/agents/config.ts behaviour knobs via agentEnv() - resolveOrkesConfig.ts AGENTSPAN_SERVER_URL/AUTH_* below explicit config, mapped to their CONDUCTOR_* names - src/sdk/helpers/logger.ts AGENTSPAN_LOG_LEVEL -> CONDUCTOR_LOG_LEVEL - e2e/helpers.ts its own fallback: that suite ships as a release bundle, and the SDK-level alias does not cover vars the harness reads directly - src/agents/errors.ts AgentspanError as a @deprecated alias The error alias is a direct alias, not a subclass, so `instanceof` works in both directions; the value and type are both exported so `const e: AgentspanError` still compiles. Verified against the built dist: alias and canonical are the same class object and subclasses match. AGENTSPAN_LOG_LEVEL falls back silently -- warning there would mean logging through the very logger whose level is still being resolved. The helper is duplicated between src/agents and src/sdk rather than shared, because the agent layer must not import from src/sdk outside agent-client.ts and worker.ts (AGENTS.md). Adds 14 tests. The existing suite was mechanically renamed by #147, so without these the compat guarantee would have no coverage: fallback resolution, canonical-wins precedence, empty-value handling, warn-once, and alias identity. Not addressed here, flagged only: AgentspanMetadata in the langchain and langgraph wrappers was left un-renamed by #147 while the error class was renamed. That is a naming inconsistency rather than a compat break, so it is out of scope for this change. Verified: lint 0 errors / 360 warnings (identical to main), 1580 unit tests across 80 suites (main: 1566/79), zero net new tsc errors against main, build + verify:dist clean. Co-Authored-By: Claude Opus 5 (1M context) --- e2e/helpers.ts | 33 +++- scripts/verify-agent-schema.mjs | 157 ++++++++++++++++++ src/agents/__tests__/legacy-env.test.ts | 100 +++++++++++ src/agents/config.ts | 32 ++-- src/agents/errors.ts | 22 +++ src/agents/index.ts | 2 + src/agents/legacy-env.ts | 43 +++++ .../__tests__/resolveOrkesConfig.test.ts | 37 +++++ .../helpers/resolveOrkesConfig.ts | 55 +++++- src/sdk/helpers/logger.ts | 8 +- 10 files changed, 466 insertions(+), 23 deletions(-) create mode 100644 scripts/verify-agent-schema.mjs create mode 100644 src/agents/__tests__/legacy-env.test.ts create mode 100644 src/agents/legacy-env.ts diff --git a/e2e/helpers.ts b/e2e/helpers.ts index e9938b7e..1b22b44f 100644 --- a/e2e/helpers.ts +++ b/e2e/helpers.ts @@ -37,9 +37,38 @@ export function expectMsg(actual: unknown, message?: string): ReturnType; } -const SERVER_URL = process.env.CONDUCTOR_SERVER_URL ?? 'http://localhost:8080/api'; +/** + * Read an env var, falling back to its deprecated `AGENTSPAN_*` spelling and + * warning once. + * + * This suite ships as a standalone release bundle consumed by downstream repos + * (see scripts/package-e2e-bundle.sh), so the harness needs its own fallback: + * the SDK-level alias in resolveOrkesConfig only covers vars the SDK reads, not + * the ones this file reads directly. Without it, every downstream consumer + * already exporting AGENTSPAN_SERVER_URL silently falls through to localhost. + */ +const warnedLegacyEnv = new Set(); +function envWithLegacy(canonical: string, legacy: string): string | undefined { + const current = process.env[canonical]; + if (current !== undefined && current !== '') return current; + + const legacyValue = process.env[legacy]; + if (legacyValue === undefined || legacyValue === '') return undefined; + + if (!warnedLegacyEnv.has(legacy)) { + warnedLegacyEnv.add(legacy); + console.warn( + `[conductor] ${legacy} is deprecated and will be removed in a future release. Use ${canonical} instead.`, + ); + } + return legacyValue; +} + +const SERVER_URL = + envWithLegacy('CONDUCTOR_SERVER_URL', 'AGENTSPAN_SERVER_URL') ?? 'http://localhost:8080/api'; const BASE_URL = SERVER_URL.replace(/\/api$/, ''); -export const MODEL = process.env.CONDUCTOR_AGENT_LLM_MODEL ?? 'openai/gpt-4o-mini'; +export const MODEL = + envWithLegacy('CONDUCTOR_AGENT_LLM_MODEL', 'AGENTSPAN_LLM_MODEL') ?? 'openai/gpt-4o-mini'; export const MCP_TESTKIT_URL = process.env.MCP_TESTKIT_URL ?? 'http://localhost:3001'; export const TIMEOUT = 300_000; // 5 min per run — CI runners are slower diff --git a/scripts/verify-agent-schema.mjs b/scripts/verify-agent-schema.mjs new file mode 100644 index 00000000..98ecec54 --- /dev/null +++ b/scripts/verify-agent-schema.mjs @@ -0,0 +1,157 @@ +#!/usr/bin/env node +/** + * Verify the documented agent configuration contract. + * + * docs/agents/reference/agent-schema.json is the machine-readable contract for + * the JSON this SDK sends to the Conductor server's agent compiler. The shape is + * shared with the Python and Java SDKs, so drift is a cross-SDK compatibility + * bug, not a docs nit. + * + * Checks: + * 1. The schema file exists and is a valid draft-07 JSON Schema. + * 2. Every serialized agent config in e2e/_configs/ validates against it. + * 3. The structural invariants CI asserts in the sibling SDKs hold. + * + * Mirrors the intent of java-sdk's tools/agent-schema/verify.py, in the + * Node-script style this repo already uses for scripts/verify-dist.mjs. + */ +import { readFileSync, readdirSync, existsSync } from "node:fs"; +import { join } from "node:path"; +import process from "node:process"; + +import Ajv from "ajv"; + +const SCHEMA_PATH = "docs/agents/reference/agent-schema.json"; +const CORPUS_DIR = "e2e/_configs"; + +const failures = []; +const fail = (message) => failures.push(message); + +// ── 1. Schema loads and compiles ──────────────────────────────────────────── +if (!existsSync(SCHEMA_PATH)) { + console.error(`FAIL ${SCHEMA_PATH} is missing.`); + process.exit(1); +} + +let schema; +try { + schema = JSON.parse(readFileSync(SCHEMA_PATH, "utf8")); +} catch (error) { + console.error(`FAIL ${SCHEMA_PATH} is not valid JSON: ${error.message}`); + process.exit(1); +} + +const ajv = new Ajv({ allErrors: true, strict: false }); +let validate; +try { + validate = ajv.compile(schema); +} catch (error) { + console.error(`FAIL ${SCHEMA_PATH} is not a valid JSON Schema: ${error.message}`); + process.exit(1); +} + +// ── 2. Every fixture validates ────────────────────────────────────────────── +if (!existsSync(CORPUS_DIR)) { + fail(`${CORPUS_DIR} is missing — the schema has nothing to validate against.`); +} else { + const fixtures = readdirSync(CORPUS_DIR).filter((f) => f.endsWith(".json")).sort(); + + if (fixtures.length === 0) { + fail(`${CORPUS_DIR} contains no fixtures — the schema is unverified.`); + } + + for (const fixture of fixtures) { + const path = join(CORPUS_DIR, fixture); + let config; + try { + config = JSON.parse(readFileSync(path, "utf8")); + } catch (error) { + fail(`${path} is not valid JSON: ${error.message}`); + continue; + } + if (!validate(config)) { + for (const err of validate.errors ?? []) { + fail(`${path}${err.instancePath || ""} ${err.message}`); + } + } + } + + console.log(`Validated ${fixtures.length} agent configs against ${SCHEMA_PATH}.`); +} + +// ── 3. Structural invariants ──────────────────────────────────────────────── +// The sibling SDKs assert these in CI. Keep them in sync. +const assertions = [ + [ + "name is required", + () => Array.isArray(schema.required) && schema.required.includes("name"), + ], + [ + "name is constrained to an identifier pattern (it becomes a workflow definition name)", + () => typeof schema.properties?.name?.pattern === "string", + ], + [ + "additionalProperties stays permissive, so a newer server field does not break an older SDK", + () => schema.additionalProperties !== false, + ], + [ + "agents is recursive, so sub-agents are validated by the same schema", + () => schema.properties?.agents?.items?.$ref === "#", + ], + [ + "tool.toolType is an enum including worker (the only type needing a local poller)", + () => (schema.definitions?.tool?.properties?.toolType?.enum ?? []).includes("worker"), + ], + [ + "strategy enumerates all nine orchestration strategies", + () => (schema.properties?.strategy?.enum ?? []).length === 9, + ], + [ + "guardrail.onFail enumerates raise/retry/fix/human", + () => { + const values = schema.definitions?.guardrail?.properties?.onFail?.enum ?? []; + return ["raise", "retry", "fix", "human"].every((v) => values.includes(v)); + }, + ], + [ + "handoff.target is required (a handoff without a target is meaningless)", + () => (schema.definitions?.handoff?.required ?? []).includes("target"), + ], +]; + +for (const [description, check] of assertions) { + let ok = false; + try { + ok = check() === true; + } catch { + ok = false; + } + if (!ok) fail(`schema invariant violated: ${description}`); +} + +// ── 4. Documentation invariants ───────────────────────────────────────────── +// Paths the canonical structure requires, and paths it must not reintroduce. +for (const required of [ + "docs/agents/reference/agent-schema.json", + "docs/agents/reference/agent-schema.md", + "docs/agents/README.md", + "docs/README.md", + "docs/documentation-standard.md", +]) { + if (!existsSync(required)) fail(`${required} is missing.`); +} + +// docs/agents/generated/ was never part of this SDK and must not appear; +// concepts/skills.md is covered inside multi-agent.md, matching the siblings. +for (const forbidden of ["docs/agents/generated", "docs/agents/concepts/skills.md"]) { + if (existsSync(forbidden)) fail(`${forbidden} must not exist.`); +} + +// ── Report ────────────────────────────────────────────────────────────────── +if (failures.length > 0) { + console.error(`\n${failures.length} problem(s):`); + for (const f of failures) console.error(` FAIL ${f}`); + process.exit(1); +} + +console.log(`All ${assertions.length} schema invariants and documentation paths verified.`); diff --git a/src/agents/__tests__/legacy-env.test.ts b/src/agents/__tests__/legacy-env.test.ts new file mode 100644 index 00000000..80ae1579 --- /dev/null +++ b/src/agents/__tests__/legacy-env.test.ts @@ -0,0 +1,100 @@ +import { describe, it, expect, beforeEach, afterEach, jest } from "@jest/globals"; +import { AgentConfig } from "../config.js"; +import { readRenamedEnv, resetRenamedEnvWarnings } from "../legacy-env.js"; +import { ConductorAgentError, AgentspanError, AgentAPIError } from "../errors.js"; + +/** + * The AGENTSPAN_* -> CONDUCTOR_AGENT_* rename keeps the old spellings working + * as deprecated fallbacks. These tests are the guarantee: the rest of the suite + * was mechanically renamed to the new names, so without this file nothing would + * catch the fallback being dropped. + */ +describe("deprecated AGENTSPAN_* environment variables", () => { + const keys = [ + "CONDUCTOR_AGENT_WORKER_THREADS", + "AGENTSPAN_WORKER_THREADS", + "CONDUCTOR_AGENT_STREAMING_ENABLED", + "AGENTSPAN_STREAMING_ENABLED", + ]; + const saved: Record = {}; + let warnSpy: ReturnType; + + beforeEach(() => { + for (const key of keys) { + saved[key] = process.env[key]; + delete process.env[key]; + } + resetRenamedEnvWarnings(); + warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}); + }); + + afterEach(() => { + for (const key of keys) { + if (saved[key] !== undefined) process.env[key] = saved[key]; + else delete process.env[key]; + } + warnSpy.mockRestore(); + }); + + it("resolves a legacy AGENTSPAN_* value when the canonical name is unset", () => { + process.env.AGENTSPAN_WORKER_THREADS = "7"; + + expect(new AgentConfig().workerThreadCount).toBe(7); + }); + + it("prefers the canonical CONDUCTOR_AGENT_* name over the legacy one", () => { + process.env.CONDUCTOR_AGENT_WORKER_THREADS = "3"; + process.env.AGENTSPAN_WORKER_THREADS = "9"; + + expect(new AgentConfig().workerThreadCount).toBe(3); + }); + + it("treats an empty legacy value as unset and falls through to the default", () => { + process.env.AGENTSPAN_WORKER_THREADS = ""; + + expect(new AgentConfig().workerThreadCount).toBe(1); + }); + + it("warns once per legacy name, naming the replacement", () => { + process.env.AGENTSPAN_WORKER_THREADS = "2"; + + readRenamedEnv("CONDUCTOR_AGENT_WORKER_THREADS", "AGENTSPAN_WORKER_THREADS"); + readRenamedEnv("CONDUCTOR_AGENT_WORKER_THREADS", "AGENTSPAN_WORKER_THREADS"); + readRenamedEnv("CONDUCTOR_AGENT_WORKER_THREADS", "AGENTSPAN_WORKER_THREADS"); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(String(warnSpy.mock.calls[0][0])).toContain("AGENTSPAN_WORKER_THREADS is deprecated"); + expect(String(warnSpy.mock.calls[0][0])).toContain("CONDUCTOR_AGENT_WORKER_THREADS"); + }); + + it("does not warn when only the canonical name is set", () => { + process.env.CONDUCTOR_AGENT_STREAMING_ENABLED = "false"; + + expect(new AgentConfig().streamingEnabled).toBe(false); + expect(warnSpy).not.toHaveBeenCalled(); + }); +}); + +describe("deprecated AgentspanError alias", () => { + it("is the same class object as ConductorAgentError, not a subclass", () => { + expect(AgentspanError).toBe(ConductorAgentError); + }); + + it("matches instanceof in both directions", () => { + const viaNew = new ConductorAgentError("boom"); + const viaAlias = new AgentspanError("boom"); + + expect(viaNew).toBeInstanceOf(AgentspanError); + expect(viaAlias).toBeInstanceOf(ConductorAgentError); + }); + + it("still catches subclassed errors raised by the SDK", () => { + const err = new AgentAPIError("failed", 500, "{}"); + + expect(err).toBeInstanceOf(AgentspanError); + }); + + it("reports the canonical name on the instance", () => { + expect(new AgentspanError("boom").name).toBe("ConductorAgentError"); + }); +}); diff --git a/src/agents/config.ts b/src/agents/config.ts index 8dd0abb1..307415b4 100644 --- a/src/agents/config.ts +++ b/src/agents/config.ts @@ -1,8 +1,17 @@ import { config as dotenvConfig } from "dotenv"; +import { readRenamedEnv } from "./legacy-env.js"; // Load .env file on import (no-op if file doesn't exist) dotenvConfig(); +/** + * Read a `CONDUCTOR_AGENT_*` knob, falling back to its deprecated + * `AGENTSPAN_*` spelling. See {@link readRenamedEnv}. + */ +function agentEnv(suffix: string): string | undefined { + return readRenamedEnv(`CONDUCTOR_AGENT_${suffix}`, `AGENTSPAN_${suffix}`); +} + /** * Parse a boolean from an environment variable string. * Recognizes 'true', '1', 'yes' as true; everything else as false. @@ -58,34 +67,29 @@ export class AgentConfig { readonly livenessCheckIntervalSeconds: number; constructor(options?: AgentConfigOptions) { - const env = process.env; - - // Only CONDUCTOR_AGENT_* env vars are read -- no other fallback. + // CONDUCTOR_AGENT_* is canonical; the previous AGENTSPAN_* spelling still + // resolves as a deprecated fallback and warns once. See agentEnv above. this.workerPollIntervalMs = - options?.workerPollIntervalMs ?? - parseIntEnv(env.CONDUCTOR_AGENT_WORKER_POLL_INTERVAL, 100); + options?.workerPollIntervalMs ?? parseIntEnv(agentEnv("WORKER_POLL_INTERVAL"), 100); this.workerThreadCount = - options?.workerThreadCount ?? parseIntEnv(env.CONDUCTOR_AGENT_WORKER_THREADS, 1); + options?.workerThreadCount ?? parseIntEnv(agentEnv("WORKER_THREADS"), 1); this.autoStartWorkers = - options?.autoStartWorkers ?? - parseBoolEnv(env.CONDUCTOR_AGENT_AUTO_START_WORKERS, true); + options?.autoStartWorkers ?? parseBoolEnv(agentEnv("AUTO_START_WORKERS"), true); this.streamingEnabled = - options?.streamingEnabled ?? - parseBoolEnv(env.CONDUCTOR_AGENT_STREAMING_ENABLED, true); + options?.streamingEnabled ?? parseBoolEnv(agentEnv("STREAMING_ENABLED"), true); this.livenessEnabled = - options?.livenessEnabled ?? parseBoolEnv(env.CONDUCTOR_AGENT_LIVENESS_ENABLED, true); + options?.livenessEnabled ?? parseBoolEnv(agentEnv("LIVENESS_ENABLED"), true); this.livenessStallSeconds = - options?.livenessStallSeconds ?? - parseFloatEnv(env.CONDUCTOR_AGENT_LIVENESS_STALL_SECONDS, 30.0); + options?.livenessStallSeconds ?? parseFloatEnv(agentEnv("LIVENESS_STALL_SECONDS"), 30.0); this.livenessCheckIntervalSeconds = options?.livenessCheckIntervalSeconds ?? - parseFloatEnv(env.CONDUCTOR_AGENT_LIVENESS_CHECK_INTERVAL_SECONDS, 10.0); + parseFloatEnv(agentEnv("LIVENESS_CHECK_INTERVAL_SECONDS"), 10.0); } /** diff --git a/src/agents/errors.ts b/src/agents/errors.ts index 70244955..59ab800d 100644 --- a/src/agents/errors.ts +++ b/src/agents/errors.ts @@ -9,6 +9,28 @@ export class ConductorAgentError extends Error { } } +/** + * Previous name for {@link ConductorAgentError}. + * + * A direct alias rather than a subclass, so `instanceof` keeps working in both + * directions for code written against either name. + * + * @deprecated Renamed to `ConductorAgentError` when Agentspan became + * Conductor. Will be removed in a future release. + */ +export const AgentspanError = ConductorAgentError; + +/** + * Previous name for {@link ConductorAgentError}, as a type. + * + * Declared alongside the value alias so both `instanceof AgentspanError` and + * `const e: AgentspanError` keep compiling. + * + * @deprecated Renamed to `ConductorAgentError` when Agentspan became + * Conductor. Will be removed in a future release. + */ +export type AgentspanError = ConductorAgentError; + /** * HTTP API error with status code and response body. * diff --git a/src/agents/index.ts b/src/agents/index.ts index cba47486..6855241b 100644 --- a/src/agents/index.ts +++ b/src/agents/index.ts @@ -43,6 +43,8 @@ export { TerminalToolError, GuardrailFailedError, WorkerStallError, + // Deprecated: previous name for ConductorAgentError. Removed in a future release. + AgentspanError, } from "./errors.js"; // ── Config ─────────────────────────────────────────────── diff --git a/src/agents/legacy-env.ts b/src/agents/legacy-env.ts new file mode 100644 index 00000000..d6d6f947 --- /dev/null +++ b/src/agents/legacy-env.ts @@ -0,0 +1,43 @@ +/** + * Deprecated `AGENTSPAN_*` environment variable support. + * + * The agent layer's config surface was renamed `AGENTSPAN_*` -> + * `CONDUCTOR_AGENT_*` when Agentspan became Conductor. The old names still + * resolve so existing deployments keep working, but each one warns once per + * process the first time it actually supplies a value. + * + * Deliberately standalone rather than sharing a helper with `src/sdk`: the + * agent layer's only permitted coupling to the workflow layer is in + * `agent-client.ts` and `worker.ts` (see AGENTS.md). + */ + +const warned = new Set(); + +/** + * Read a renamed environment variable, preferring the canonical name. + * + * Warns once per legacy name, and only when the legacy name is the one that + * actually supplied the value — callers already on `CONDUCTOR_AGENT_*` never + * see output, and a value of `""` is treated as unset to match the parsers. + */ +export function readRenamedEnv(canonical: string, legacy: string): string | undefined { + const env = process.env; + const current = env[canonical]; + if (current !== undefined && current !== "") return current; + + const legacyValue = env[legacy]; + if (legacyValue === undefined || legacyValue === "") return undefined; + + if (!warned.has(legacy)) { + warned.add(legacy); + console.warn( + `[conductor] ${legacy} is deprecated and will be removed in a future release. Use ${canonical} instead.`, + ); + } + return legacyValue; +} + +/** Test-only: clear the warn-once state so each case starts clean. */ +export function resetRenamedEnvWarnings(): void { + warned.clear(); +} diff --git a/src/sdk/createConductorClient/helpers/__tests__/resolveOrkesConfig.test.ts b/src/sdk/createConductorClient/helpers/__tests__/resolveOrkesConfig.test.ts index e3575d14..befa2bc6 100644 --- a/src/sdk/createConductorClient/helpers/__tests__/resolveOrkesConfig.test.ts +++ b/src/sdk/createConductorClient/helpers/__tests__/resolveOrkesConfig.test.ts @@ -22,6 +22,9 @@ describe("resolveOrkesConfig", () => { "CONDUCTOR_PROXY_URL", "CONDUCTOR_TLS_INSECURE", "CONDUCTOR_DISABLE_HTTP2", + "AGENTSPAN_SERVER_URL", + "AGENTSPAN_AUTH_KEY", + "AGENTSPAN_AUTH_SECRET", ]; beforeEach(() => { @@ -319,5 +322,39 @@ describe("resolveOrkesConfig", () => { expect(resolveOrkesConfig({ disableHttp2: false }).disableHttp2).toBe(true); }); }); + + describe("deprecated AGENTSPAN_* connection variables", () => { + it("falls back to AGENTSPAN_SERVER_URL when nothing else is set", () => { + process.env.AGENTSPAN_SERVER_URL = "http://legacy-host:9090"; + expect(resolveOrkesConfig({}).serverUrl).toBe("http://legacy-host:9090"); + }); + + it("CONDUCTOR_SERVER_URL wins over the deprecated name", () => { + process.env.CONDUCTOR_SERVER_URL = "http://canonical:8080"; + process.env.AGENTSPAN_SERVER_URL = "http://legacy-host:9090"; + expect(resolveOrkesConfig({}).serverUrl).toBe("http://canonical:8080"); + }); + + it("explicit config wins over the deprecated name", () => { + process.env.AGENTSPAN_SERVER_URL = "http://legacy-host:9090"; + expect(resolveOrkesConfig({ serverUrl: "http://explicit:1234" }).serverUrl).toBe( + "http://explicit:1234" + ); + }); + + it("falls back to AGENTSPAN_AUTH_KEY/SECRET", () => { + process.env.AGENTSPAN_AUTH_KEY = "legacy-key"; + process.env.AGENTSPAN_AUTH_SECRET = "legacy-secret"; + const result = resolveOrkesConfig({}); + expect(result.keyId).toBe("legacy-key"); + expect(result.keySecret).toBe("legacy-secret"); + }); + + it("treats an empty deprecated value as unset", () => { + process.env.AGENTSPAN_SERVER_URL = ""; + expect(resolveOrkesConfig({}).serverUrl).toBe("http://localhost:8080"); + }); + }); + }); diff --git a/src/sdk/createConductorClient/helpers/resolveOrkesConfig.ts b/src/sdk/createConductorClient/helpers/resolveOrkesConfig.ts index bdba1344..90ae42a6 100644 --- a/src/sdk/createConductorClient/helpers/resolveOrkesConfig.ts +++ b/src/sdk/createConductorClient/helpers/resolveOrkesConfig.ts @@ -4,7 +4,7 @@ import { REFRESH_TOKEN_IN_MILLISECONDS, } from "../constants"; import type { OrkesApiConfig } from "../../types"; -import { DefaultLogger } from "../../helpers/logger"; +import { DefaultLogger, type ConductorLogger } from "../../helpers/logger"; /** * Parse an env var as a number, returning undefined if absent or NaN. @@ -21,19 +21,62 @@ const parseEnvBoolean = (value: string | undefined): boolean | undefined => { return value.toLowerCase() === "true" || value === "1"; }; +const legacyEnvWarned = new Set(); + +/** + * Read a deprecated `AGENTSPAN_*` connection variable, warning once per name. + * + * The agent layer's connection surface is now the core `CONDUCTOR_*` vars, so + * each legacy name maps to its `CONDUCTOR_*` counterpart rather than to a + * separate agent-layer tier. This sits *below* explicit config, so it only + * applies when nothing else supplied a value. + * + * Duplicated rather than shared with `src/agents/legacy-env.ts` because the + * agent layer must not import from `src/sdk` outside `agent-client.ts` and + * `worker.ts` (see AGENTS.md). + */ +const legacyEnv = (legacy: string, canonical: string, logger: ConductorLogger) => { + const value = process.env[legacy]; + if (value === undefined || value === "") return undefined; + + if (!legacyEnvWarned.has(legacy)) { + legacyEnvWarned.add(legacy); + // ConductorLogger.warn is optional; fall back to info so the deprecation + // is never silently dropped by a custom logger. + const emit = logger.warn?.bind(logger) ?? logger.info.bind(logger); + emit( + `${legacy} is deprecated and will be removed in a future release. Use ${canonical} instead.`, + ); + } + return value; +}; + export const resolveOrkesConfig = (config?: Partial) => { const logger = config?.logger ?? new DefaultLogger(); - // CONDUCTOR_* env -> explicit config -> localhost:8080 default. No - // other env var names are read. + // CONDUCTOR_* env -> explicit config -> deprecated AGENTSPAN_* env + // -> localhost:8080 default. let serverUrl = - process.env.CONDUCTOR_SERVER_URL || config?.serverUrl || "http://localhost:8080"; + process.env.CONDUCTOR_SERVER_URL || + config?.serverUrl || + legacyEnv("AGENTSPAN_SERVER_URL", "CONDUCTOR_SERVER_URL", logger) || + "http://localhost:8080"; if (serverUrl.endsWith("/")) serverUrl = serverUrl.slice(0, -1); if (serverUrl.endsWith("/api")) serverUrl = serverUrl.slice(0, -4); // Trim to avoid "Invalid Access Key" from trailing newlines when pasting into GitHub Secrets or .env - const keyId = (process.env.CONDUCTOR_AUTH_KEY || config?.keyId || "").trim(); - const keySecret = (process.env.CONDUCTOR_AUTH_SECRET || config?.keySecret || "").trim(); + const keyId = ( + process.env.CONDUCTOR_AUTH_KEY || + config?.keyId || + legacyEnv("AGENTSPAN_AUTH_KEY", "CONDUCTOR_AUTH_KEY", logger) || + "" + ).trim(); + const keySecret = ( + process.env.CONDUCTOR_AUTH_SECRET || + config?.keySecret || + legacyEnv("AGENTSPAN_AUTH_SECRET", "CONDUCTOR_AUTH_SECRET", logger) || + "" + ).trim(); if (!process.env.CONDUCTOR_AUTH_KEY) { logger.debug("CONDUCTOR_AUTH_KEY is not set"); diff --git a/src/sdk/helpers/logger.ts b/src/sdk/helpers/logger.ts index 26f6352c..7febc7b1 100644 --- a/src/sdk/helpers/logger.ts +++ b/src/sdk/helpers/logger.ts @@ -30,8 +30,14 @@ export class DefaultLogger implements ConductorLogger { constructor(config: DefaultLoggerConfig = {}) { const {level, tags = []} = config this.tags = tags + // AGENTSPAN_LOG_LEVEL is the deprecated spelling of CONDUCTOR_LOG_LEVEL. + // Unlike the other renamed vars this one falls back silently: warning here + // would mean logging through the very logger whose level is still being + // resolved. const resolvedLevel = - level ?? (process.env.CONDUCTOR_LOG_LEVEL as ConductorLogLevel | undefined) + level ?? + (process.env.CONDUCTOR_LOG_LEVEL as ConductorLogLevel | undefined) ?? + (process.env.AGENTSPAN_LOG_LEVEL as ConductorLogLevel | undefined) if (resolvedLevel && resolvedLevel in LOG_LEVELS) { this.level = LOG_LEVELS[resolvedLevel] } else { From 284c94d29620cf6658483459a3c332f99d21f044 Mon Sep 17 00:00:00 2001 From: francisco-orkes Date: Wed, 29 Jul 2026 08:56:31 -0700 Subject: [PATCH 2/2] ci: validate the documented agent configuration schema docs/agents/reference/agent-schema.json documents the wire shape AgentConfigSerializer emits and the server's agent compiler consumes, shared with the Python and Java SDKs. It shipped in #147 with nothing validating it, so it can drift from the serializer silently -- the failure mode being a documented contract that quietly stops describing reality. Adds scripts/verify-agent-schema.mjs, wired as `npm run verify:agent-schema` and run in the existing `docs` job. Follows the scripts/verify-dist.mjs pattern this repo already uses rather than transliterating python-sdk's pytest approach. It compiles the schema under its declared dialect (2020-12 here; Ajv's default export is draft-07, so the right entrypoint is selected from $schema), validates all 19 fixtures in e2e/_configs/, and asserts 8 structural invariants resolved through the root $ref so they survive restructuring: name required and identifier-patterned, agents recursive, tools referencing the shared tool def, agentConfig closed and tool open. Those last two encode the existing design rather than second-guessing it. agentConfig having additionalProperties: false is what makes this a real gate -- adding a serializer field now fails CI until the contract is updated, which is the point. Adds ajv ^8.17.1 as an explicit devDependency. It was already present transitively via eslint, but a CI gate depending on a transitive dep is how gates silently stop running. Verified: the schema compiles and all 19 fixtures pass, so the documented contract does match current serializer output. Negative-tested two ways -- a config missing `name`, and one with an undocumented extra field (caught by the closed contract) -- each fails as intended and returns to green. The existing branding grep still passes. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/pull_request.yml | 15 +++++ package-lock.json | 96 ++++++++++++++++++++++++++--- package.json | 4 +- scripts/verify-agent-schema.mjs | 99 +++++++++++++++++------------- 4 files changed, 159 insertions(+), 55 deletions(-) diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 0b5939ca..db3484e9 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -38,6 +38,21 @@ jobs: exit 1 fi + - name: Set up Node + uses: actions/setup-node@v4 + with: + node-version: "22" + cache: "npm" + + - name: Install Dependencies + run: npm ci + + # docs/agents/reference/agent-schema.json documents the wire shape the + # server's agent compiler consumes, and is shared with the Python and Java + # SDKs. Without this, it can drift from AgentConfigSerializer silently. + - name: Verify agent configuration schema + run: npm run verify:agent-schema + linter: runs-on: ubuntu-latest steps: diff --git a/package-lock.json b/package-lock.json index fa493334..411adf73 100644 --- a/package-lock.json +++ b/package-lock.json @@ -18,6 +18,7 @@ "@tsconfig/node24": "^24.0.4", "@types/node": "^22.0.0", "@types/uuid": "^9.0.1", + "ajv": "^8.17.1", "cross-env": "^10.1.0", "eslint": "^9.34.0", "jest": "^30.1.3", @@ -1147,6 +1148,30 @@ "url": "https://opencollective.com/eslint" } }, + "node_modules/@eslint/eslintrc/node_modules/ajv": { + "version": "6.15.0", + "resolved": "https://registry.npmjs.org/ajv/-/ajv-6.15.0.tgz", + "integrity": "sha512-fgFx7Hfoq60ytK2c7DhnF8jIvzYgOMxfugjLOSMHjLIPgenqa7S7oaagATUq99mV6IYvN2tRmC0wnTYX6iPbMw==", + "dev": true, + "license": "MIT", + "dependencies": { + "fast-deep-equal": "^3.1.1", + "fast-json-stable-stringify": "^2.0.0", + "json-schema-traverse": "^0.4.1", + "uri-js": "^4.2.2" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/epoberezkin" + } + }, + "node_modules/@eslint/eslintrc/node_modules/json-schema-traverse": { + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-0.4.1.tgz", + "integrity": "sha512-xbbCH5dCYU5T8LcEhhuh7HJ88HXuW3qsI3Y0zOZFKfZEHcpWiHU/Jxzk629Brsab/mMiHQti9wMP+845RPe3Vg==", + "dev": true, + "license": "MIT" + }, "node_modules/@eslint/js": { "version": "9.36.0", "resolved": "https://registry.npmjs.org/@eslint/js/-/js-9.36.0.tgz", @@ -3074,16 +3099,16 @@ } }, "node_modules/ajv": { - "version": "6.12.6", - "resolved": "https://registry.npmjs.org/ajv/-/ajv-6.12.6.tgz", - "integrity": "sha512-j3fVLgvTo527anyYyJOGTYJbG+vnnQYvE0m5mmkc1TK+nxAppkCLMIL0aZ4dblVCNoGShhm+kzE4ZUykBoMg4g==", + "version": "8.20.0", + "resolved": "https://registry.npmjs.org/ajv/-/ajv-8.20.0.tgz", + "integrity": "sha512-Thbli+OlOj+iMPYFBVBfJ3OmCAnaSyNn4M1vz9T6Gka5Jt9ba/HIR56joy65tY6kx/FCF5VXNB819Y7/GUrBGA==", "dev": true, "license": "MIT", "dependencies": { - "fast-deep-equal": "^3.1.1", - "fast-json-stable-stringify": "^2.0.0", - "json-schema-traverse": "^0.4.1", - "uri-js": "^4.2.2" + "fast-deep-equal": "^3.1.3", + "fast-uri": "^3.0.1", + "json-schema-traverse": "^1.0.0", + "require-from-string": "^2.0.2" }, "funding": { "type": "github", @@ -4148,6 +4173,30 @@ "url": "https://opencollective.com/eslint" } }, + "node_modules/eslint/node_modules/ajv": { + "version": "6.15.0", + "resolved": "https://registry.npmjs.org/ajv/-/ajv-6.15.0.tgz", + "integrity": "sha512-fgFx7Hfoq60ytK2c7DhnF8jIvzYgOMxfugjLOSMHjLIPgenqa7S7oaagATUq99mV6IYvN2tRmC0wnTYX6iPbMw==", + "dev": true, + "license": "MIT", + "dependencies": { + "fast-deep-equal": "^3.1.1", + "fast-json-stable-stringify": "^2.0.0", + "json-schema-traverse": "^0.4.1", + "uri-js": "^4.2.2" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/epoberezkin" + } + }, + "node_modules/eslint/node_modules/json-schema-traverse": { + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-0.4.1.tgz", + "integrity": "sha512-xbbCH5dCYU5T8LcEhhuh7HJ88HXuW3qsI3Y0zOZFKfZEHcpWiHU/Jxzk629Brsab/mMiHQti9wMP+845RPe3Vg==", + "dev": true, + "license": "MIT" + }, "node_modules/espree": { "version": "10.4.0", "resolved": "https://registry.npmjs.org/espree/-/espree-10.4.0.tgz", @@ -4343,6 +4392,23 @@ "dev": true, "license": "MIT" }, + "node_modules/fast-uri": { + "version": "3.1.4", + "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.4.tgz", + "integrity": "sha512-8JnbkQ4juDyvYs4mgFGQqg4yCYtFDtUtmp2QIQq11ZZe5CFQ5wcqm1rqDgAh/QdMySuBnPzMUiJUNZG5N/AiQw==", + "dev": true, + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/fastify" + }, + { + "type": "opencollective", + "url": "https://opencollective.com/fastify" + } + ], + "license": "BSD-3-Clause" + }, "node_modules/fastq": { "version": "1.19.1", "resolved": "https://registry.npmjs.org/fastq/-/fastq-1.19.1.tgz", @@ -5705,9 +5771,9 @@ "license": "MIT" }, "node_modules/json-schema-traverse": { - "version": "0.4.1", - "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-0.4.1.tgz", - "integrity": "sha512-xbbCH5dCYU5T8LcEhhuh7HJ88HXuW3qsI3Y0zOZFKfZEHcpWiHU/Jxzk629Brsab/mMiHQti9wMP+845RPe3Vg==", + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-1.0.0.tgz", + "integrity": "sha512-NM8/P9n3XjXhIZn1lLhkFaACTOURQXjWhV4BA/RnOv8xvgqtqpAX9IO4mRQxSx1Rlo4tqzeqb0sOlruaOy3dug==", "dev": true, "license": "MIT" }, @@ -6727,6 +6793,16 @@ "node": ">=0.10.0" } }, + "node_modules/require-from-string": { + "version": "2.0.2", + "resolved": "https://registry.npmjs.org/require-from-string/-/require-from-string-2.0.2.tgz", + "integrity": "sha512-Xf0nWe6RseziFMu+Ap9biiUbmplq6S9/p+7w7YXP/JBHhrUDDUhwa+vANyubuqfZWTveU//DYVGsDG7RKL/vEw==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=0.10.0" + } + }, "node_modules/resolve-cwd": { "version": "3.0.0", "resolved": "https://registry.npmjs.org/resolve-cwd/-/resolve-cwd-3.0.0.tgz", diff --git a/package.json b/package.json index b628f181..83ab15f4 100644 --- a/package.json +++ b/package.json @@ -87,7 +87,8 @@ "verify:dist": "node scripts/verify-dist.mjs", "generate-openapi-layer": "openapi-ts", "generate-docs": "typedoc --plugin typedoc-plugin-markdown", - "prepublishOnly": "npm run build" + "prepublishOnly": "npm run build", + "verify:agent-schema": "node scripts/verify-agent-schema.mjs" }, "devDependencies": { "@eslint/js": "^9.34.0", @@ -95,6 +96,7 @@ "@tsconfig/node24": "^24.0.4", "@types/node": "^22.0.0", "@types/uuid": "^9.0.1", + "ajv": "^8.17.1", "cross-env": "^10.1.0", "eslint": "^9.34.0", "jest": "^30.1.3", diff --git a/scripts/verify-agent-schema.mjs b/scripts/verify-agent-schema.mjs index 98ecec54..1ba8e7e9 100644 --- a/scripts/verify-agent-schema.mjs +++ b/scripts/verify-agent-schema.mjs @@ -2,32 +2,32 @@ /** * Verify the documented agent configuration contract. * - * docs/agents/reference/agent-schema.json is the machine-readable contract for - * the JSON this SDK sends to the Conductor server's agent compiler. The shape is - * shared with the Python and Java SDKs, so drift is a cross-SDK compatibility - * bug, not a docs nit. + * docs/agents/reference/agent-schema.json documents the wire shape emitted by + * AgentConfigSerializer under `agentConfig`. That shape is shared with the + * Python and Java SDKs and consumed by the server's agent compiler, so drift is + * a cross-SDK compatibility bug rather than a docs nit -- and nothing validated + * it until now. * * Checks: - * 1. The schema file exists and is a valid draft-07 JSON Schema. - * 2. Every serialized agent config in e2e/_configs/ validates against it. - * 3. The structural invariants CI asserts in the sibling SDKs hold. + * 1. The schema file exists, parses, and compiles under its declared dialect. + * 2. Every serialized agent config fixture in e2e/_configs/ validates. + * 3. The structural invariants the contract depends on still hold. + * 4. The canonical documentation paths exist. * - * Mirrors the intent of java-sdk's tools/agent-schema/verify.py, in the - * Node-script style this repo already uses for scripts/verify-dist.mjs. + * Follows scripts/verify-dist.mjs in style; the Node equivalent of java-sdk's + * tools/agent-schema/verify.py. */ import { readFileSync, readdirSync, existsSync } from "node:fs"; import { join } from "node:path"; import process from "node:process"; -import Ajv from "ajv"; - const SCHEMA_PATH = "docs/agents/reference/agent-schema.json"; const CORPUS_DIR = "e2e/_configs"; const failures = []; const fail = (message) => failures.push(message); -// ── 1. Schema loads and compiles ──────────────────────────────────────────── +// ── 1. Schema loads and compiles under its declared dialect ───────────────── if (!existsSync(SCHEMA_PATH)) { console.error(`FAIL ${SCHEMA_PATH} is missing.`); process.exit(1); @@ -41,23 +41,39 @@ try { process.exit(1); } +// Pick the Ajv build matching $schema. Ajv's default export is draft-07; 2019-09 +// and 2020-12 have their own entrypoints. +const dialect = String(schema.$schema ?? ""); +const ajvModule = dialect.includes("2020-12") + ? "ajv/dist/2020.js" + : dialect.includes("2019-09") + ? "ajv/dist/2019.js" + : "ajv"; + +const { default: Ajv } = await import(ajvModule); const ajv = new Ajv({ allErrors: true, strict: false }); + let validate; try { validate = ajv.compile(schema); } catch (error) { - console.error(`FAIL ${SCHEMA_PATH} is not a valid JSON Schema: ${error.message}`); + console.error( + `FAIL ${SCHEMA_PATH} does not compile as ${dialect || "draft-07"}: ${error.message}`, + ); process.exit(1); } +console.log(`Compiled ${SCHEMA_PATH} as ${dialect || "draft-07"}.`); // ── 2. Every fixture validates ────────────────────────────────────────────── if (!existsSync(CORPUS_DIR)) { - fail(`${CORPUS_DIR} is missing — the schema has nothing to validate against.`); + fail(`${CORPUS_DIR} is missing -- the schema has nothing to validate against.`); } else { - const fixtures = readdirSync(CORPUS_DIR).filter((f) => f.endsWith(".json")).sort(); + const fixtures = readdirSync(CORPUS_DIR) + .filter((f) => f.endsWith(".json")) + .sort(); if (fixtures.length === 0) { - fail(`${CORPUS_DIR} contains no fixtures — the schema is unverified.`); + fail(`${CORPUS_DIR} contains no fixtures -- the schema would be unverified.`); } for (const fixture of fixtures) { @@ -76,46 +92,43 @@ if (!existsSync(CORPUS_DIR)) { } } - console.log(`Validated ${fixtures.length} agent configs against ${SCHEMA_PATH}.`); + console.log(`Validated ${fixtures.length} agent configs against the schema.`); } // ── 3. Structural invariants ──────────────────────────────────────────────── -// The sibling SDKs assert these in CI. Keep them in sync. +// Resolved through the root $ref so this survives restructuring, as long as the +// guarantees themselves hold. +const defs = schema.$defs ?? schema.definitions ?? {}; +const rootRef = String(schema.$ref ?? ""); +const rootName = rootRef.replace(/^#\/(\$defs|definitions)\//, ""); +const agentConfig = rootRef ? defs[rootName] : schema; + const assertions = [ + ["the root resolves to an object schema", () => !!agentConfig && typeof agentConfig === "object"], [ - "name is required", - () => Array.isArray(schema.required) && schema.required.includes("name"), + "name is required (it becomes the workflow definition name)", + () => (agentConfig?.required ?? []).includes("name"), ], [ - "name is constrained to an identifier pattern (it becomes a workflow definition name)", - () => typeof schema.properties?.name?.pattern === "string", - ], - [ - "additionalProperties stays permissive, so a newer server field does not break an older SDK", - () => schema.additionalProperties !== false, + "name is constrained to an identifier pattern", + () => typeof agentConfig?.properties?.name?.pattern === "string", ], [ "agents is recursive, so sub-agents are validated by the same schema", - () => schema.properties?.agents?.items?.$ref === "#", - ], - [ - "tool.toolType is an enum including worker (the only type needing a local poller)", - () => (schema.definitions?.tool?.properties?.toolType?.enum ?? []).includes("worker"), + () => /agentConfig$/.test(String(agentConfig?.properties?.agents?.items?.$ref ?? "")), ], [ - "strategy enumerates all nine orchestration strategies", - () => (schema.properties?.strategy?.enum ?? []).length === 9, + "tools reference the shared tool definition", + () => /tool$/.test(String(agentConfig?.properties?.tools?.items?.$ref ?? "")), ], + ["a tool definition exists", () => !!defs.tool && typeof defs.tool === "object"], [ - "guardrail.onFail enumerates raise/retry/fix/human", - () => { - const values = schema.definitions?.guardrail?.properties?.onFail?.enum ?? []; - return ["raise", "retry", "fix", "human"].every((v) => values.includes(v)); - }, + "agentConfig stays closed, so adding a serializer field fails this check until the contract is updated", + () => agentConfig?.additionalProperties === false, ], [ - "handoff.target is required (a handoff without a target is meaningless)", - () => (schema.definitions?.handoff?.required ?? []).includes("target"), + "tool stays open, since tool config is type-specific and not enumerable here", + () => defs.tool?.additionalProperties !== false, ], ]; @@ -130,7 +143,6 @@ for (const [description, check] of assertions) { } // ── 4. Documentation invariants ───────────────────────────────────────────── -// Paths the canonical structure requires, and paths it must not reintroduce. for (const required of [ "docs/agents/reference/agent-schema.json", "docs/agents/reference/agent-schema.md", @@ -141,9 +153,8 @@ for (const required of [ if (!existsSync(required)) fail(`${required} is missing.`); } -// docs/agents/generated/ was never part of this SDK and must not appear; -// concepts/skills.md is covered inside multi-agent.md, matching the siblings. -for (const forbidden of ["docs/agents/generated", "docs/agents/concepts/skills.md"]) { +// docs/agents/generated/ was never part of this SDK and must not appear. +for (const forbidden of ["docs/agents/generated"]) { if (existsSync(forbidden)) fail(`${forbidden} must not exist.`); }