From a46f585fd9b5959fa1b5452ca3313442e4008a61 Mon Sep 17 00:00:00 2001 From: razbroc Date: Thu, 27 Aug 2026 11:01:32 +0300 Subject: [PATCH] feat: let the worker bill a Claude subscription instead of an API key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MAPCO-11434. Adds a second authentication mode so a deployment can run against a Claude subscription token rather than a metered Anthropic API key, and wires the Secret and the docs that were the unmet half of the first acceptance criterion. Which mode is in use is explicit configuration, `MODEL_AUTH`, and is never inferred from whichever credential happens to be set. Both credentials look alike to the SDK and bill completely differently, so inferring would make the billed party a property of the pod's environment rather than of a decision — and the failure is silent, because a run that quietly spends someone's personal quota looks exactly like a working one. One mode's credential is never used for the other; the worker refuses to start and names the one it found, since setting a token and forgetting the mode is the mistake an operator actually makes. An unrecognised mode also refuses rather than falling back to the default. `modelEnv` now scrubs every credential and injects exactly one, the configured mode's. Previously it injected ANTHROPIC_API_KEY over a partially-scrubbed environment; with two modes reading different variables, leaving the unused one in place would let the SDK pick the other. Chart: MODEL_AUTH plus a secretKeyRef for whichever credential the mode needs, from worker.modelSecretName. README documents both variables and the mode. subscription mode is reachable, not blessed. Anthropic's Agent SDK documentation states that claude.ai login and its rate limits may not be used for products built on the Agent SDK unless previously approved, so setting the mode asserts this deployment has that approval — code cannot check it. Three consequences no code can fix are recorded in README.md and credential.ts: the quota is shared with that person's own interactive use, runs are attributed to them rather than to the worker, and the pod crash-loops when the token expires. api-key remains the default for those reasons. Renames apiKey.ts to credential.ts, since it no longer only reads a key. --- README.md | 28 ++++++ helm/templates/deployment.yaml | 15 +++ helm/values.yaml | 12 +++ node_modules | 1 + src/agent/apiKey.ts | 84 ---------------- src/agent/credential.ts | 141 +++++++++++++++++++++++++++ src/agent/implementer.ts | 16 +-- src/agent/sdkAgent.ts | 24 ++--- src/agent/sdkOptions.ts | 31 +++--- tests/unit/agent/apiKey.spec.ts | 49 ---------- tests/unit/agent/credential.spec.ts | 71 ++++++++++++++ tests/unit/agent/implementer.spec.ts | 16 ++- tests/unit/agent/sdkAgent.spec.ts | 9 +- tests/unit/agent/sdkOptions.spec.ts | 18 +++- 14 files changed, 342 insertions(+), 173 deletions(-) create mode 120000 node_modules delete mode 100644 src/agent/apiKey.ts create mode 100644 src/agent/credential.ts delete mode 100644 tests/unit/agent/apiKey.spec.ts create mode 100644 tests/unit/agent/credential.spec.ts diff --git a/README.md b/README.md index 90a0041..a153c66 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,9 @@ so refusal is the common path until the convention spreads. | `MAX_TICKETS_PER_RUN` | `1` | Tickets one cycle may start | | `MAX_CONCURRENT_TICKETS` | `1` | Tickets in flight at once | | `GITHUB_TOKEN` | *optional* | Bearer token for repo lookups. A PAT locally; a short-lived App installation token in the cluster once MAPCO-11428 lands. Unauthenticated works at a lower rate limit | +| `MODEL_AUTH` | `api-key` | Which account model calls are billed to: `api-key` or `subscription`. An unrecognised value refuses to start rather than falling back | +| `ANTHROPIC_API_KEY` | required for `api-key` | Anthropic API key, from a Secret. Billed to that Anthropic account | +| `CLAUDE_CODE_OAUTH_TOKEN` | required for `subscription` | A Claude subscription token from `claude setup-token`. Billed to, and rate-limited as, that person — see the warning below | Raise `MAX_TICKETS_PER_RUN` before ever raising `MAX_CONCURRENT_TICKETS`. @@ -85,6 +88,31 @@ The two bot-identity variables look redundant and are not: Jira takes an *identi write and hands back a *display name* on read, and neither is derivable from the other in this instance. Set them inconsistently and every claim reads as lost. +### Which account pays for the model + +`MODEL_AUTH` is explicit, and deliberately not inferred from whichever credential happens to +be present. Both credentials look alike to the SDK and bill completely differently, so letting +the environment decide would make the billed party a property of the pod rather than of a +decision — and the failure is silent, because a run that quietly spends someone's personal +quota looks exactly like a working one. One mode's credential is never used for the other; the +worker refuses to start and names the one it found. + +> **⚠️ `subscription` needs Anthropic's approval.** Anthropic's Agent SDK documentation states +> that, unless previously approved, claude.ai login and its rate limits may not be used for +> products built on the Agent SDK. Setting `MODEL_AUTH=subscription` asserts that this +> deployment has that approval — the code cannot check it. + +Three consequences of `subscription` mode that no code can fix: + +- **Shared quota.** Rate limits belong to the account, so the worker and that person's own + interactive Claude Code use starve each other. +- **Attribution.** Runs are that person's, not the worker's — the same problem this README + already records for the shared Jira service account, now for the model too. +- **Expiry.** Subscription tokens lapse, and when one does the pod crash-loops rather than + running on unclear credentials. That is the intended failure, not a bug. + +`api-key` mode has none of these, and is the default for that reason. + ## Claiming and releasing Jira is the only state store — no database, no files that outlive a run — so there is no diff --git a/helm/templates/deployment.yaml b/helm/templates/deployment.yaml index 815ecc3..a3b62b7 100644 --- a/helm/templates/deployment.yaml +++ b/helm/templates/deployment.yaml @@ -83,6 +83,21 @@ spec: value: {{ .Values.worker.maxTicketsPerRun | quote }} - name: MAX_CONCURRENT_TICKETS value: {{ .Values.worker.maxConcurrentTickets | quote }} + - name: MODEL_AUTH + value: {{ .Values.worker.modelAuth | quote }} + {{- if eq .Values.worker.modelAuth "subscription" }} + - name: CLAUDE_CODE_OAUTH_TOKEN + valueFrom: + secretKeyRef: + name: {{ .Values.worker.modelSecretName | quote }} + key: oauthToken + {{- else }} + - name: ANTHROPIC_API_KEY + valueFrom: + secretKeyRef: + name: {{ .Values.worker.modelSecretName | quote }} + key: apiKey + {{- end }} {{- if .Values.caSecretName }} - name: REQUESTS_CA_BUNDLE value: {{ printf "%s/%s" .Values.caPath .Values.caKey | quote }} diff --git a/helm/values.yaml b/helm/values.yaml index fa8e035..eb3dac1 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -68,6 +68,18 @@ worker: pollIntervalMs: 300000 maxTicketsPerRun: 1 maxConcurrentTickets: 1 + # Which account the model calls are billed to. `api-key` reads ANTHROPIC_API_KEY from the + # Secret below; `subscription` reads CLAUDE_CODE_OAUTH_TOKEN from it instead. + # + # `subscription` bills, and is rate-limited as, the person whose token it is. Anthropic's + # Agent SDK documentation states that claude.ai login and its rate limits may not be used + # for products built on the Agent SDK unless previously approved — setting this asserts + # that this deployment has that approval. The worker and that person's own interactive use + # also share one quota, and the pod will crash-loop when the token expires. + modelAuth: 'api-key' + # Secret holding the model credential. Key name must be `apiKey` for modelAuth: api-key, + # or `oauthToken` for modelAuth: subscription. The pod will not start without it. + modelSecretName: '' env: logLevel: info diff --git a/node_modules b/node_modules new file mode 120000 index 0000000..4daf7de --- /dev/null +++ b/node_modules @@ -0,0 +1 @@ +/home/razbro/Repos/developer-agent-bot/node_modules \ No newline at end of file diff --git a/src/agent/apiKey.ts b/src/agent/apiKey.ts deleted file mode 100644 index 099a9e3..0000000 --- a/src/agent/apiKey.ts +++ /dev/null @@ -1,84 +0,0 @@ -/** - * How the worker gets the credential it talks to the model with. - * - * This lives here rather than in `WorkerConfig` for one reason that is not tidiness: every - * other field of `WorkerConfig` is safe to log, and this one is not. Keeping it out of that - * object means the config a cycle carries around — and that ends up in a log line the day - * someone logs it — never contains a key. It is read once, at the entry point, and handed - * straight to `AgentSettings`. - * - * The env var is the standard `ANTHROPIC_API_KEY`, which is what makes the deployment side of - * this a `secretKeyRef` in the pod spec and nothing more: - * - * ```yaml - * - name: ANTHROPIC_API_KEY - * valueFrom: - * secretKeyRef: - * name: {{ .Values.worker.anthropicSecretName }} - * key: apiKey - * ``` - * - * That block, and the README row that documents it, are the deployment half of MAPCO-11434's - * first acceptance criterion. They live in `helm/templates/deployment.yaml`, `helm/values.yaml` - * and `README.md`, none of which this slice owns — so the code half refuses to start without the - * variable, which is the loudest thing it can do about a Secret that never arrives. - */ - -/** The one variable the worker authenticates with. Set from a Secret in the cluster. */ -const API_KEY_ENV = 'ANTHROPIC_API_KEY'; - -/** - * Credentials that would let a run authenticate as a *person* rather than as the worker. - * - * Present on a developer's laptop, absent from the pod, and never a fallback. "Never an - * interactive login" is an acceptance criterion, and the way that criterion is usually broken - * is not by a decision but by a default: a library that quietly picks up whatever session - * token it can find. The worker refuses instead, and says which variable it saw, because a - * dry-run that silently billed a human's account would look exactly like a working one. - * - * These are stripped from the model's own subprocess environment as well — see - * `SECRET_ENV_NAMES` in src/workspace/subprocess.ts. - */ -const LOGIN_ENV_NAMES = ['CLAUDE_CODE_OAUTH_TOKEN', 'ANTHROPIC_AUTH_TOKEN'] as const; - -/** - * A missing or unusable model credential. - * - * Distinct class rather than a bare `Error` for the same reason as `ConfigError` in - * src/common/workerConfig.ts: this is a deployment fault, discovered at boot, and it must not - * read as a ticket that failed. - */ -class AgentConfigError extends Error { - public constructor(message: string) { - super(message); - this.name = 'AgentConfigError'; - } -} - -/** - * The API key, or a thrown `AgentConfigError`. - * - * Thrown rather than refused-as-a-value on purpose: a worker with no key cannot do the one - * thing it exists for, and every ticket it claimed in the meantime would be a claim burnt for - * nothing. Boot is the cheapest place to find out. - * - * The env map is a parameter so tests drive it with plain objects, exactly as - * `loadWorkerConfig` does — nothing in the suite mutates `process.env`. - */ -function readApiKey(env: NodeJS.ProcessEnv = process.env): string { - const key = env[API_KEY_ENV]?.trim() ?? ''; - - if (key !== '') { - return key; - } - - const login = LOGIN_ENV_NAMES.filter((name) => (env[name]?.trim() ?? '') !== ''); - const instead = - login.length > 0 - ? ` ${login.join(' and ')} ${login.length === 1 ? 'is' : 'are'} set, and will not be used instead: the worker authenticates as itself, never as whoever logged in.` - : ''; - - throw new AgentConfigError(`${API_KEY_ENV} must be set — the worker has no other way to reach the model.${instead}`); -} - -export { AgentConfigError, API_KEY_ENV, LOGIN_ENV_NAMES, readApiKey }; diff --git a/src/agent/credential.ts b/src/agent/credential.ts new file mode 100644 index 0000000..479554b --- /dev/null +++ b/src/agent/credential.ts @@ -0,0 +1,141 @@ +/** + * How the worker gets the credential it talks to the model with. + * + * This lives here rather than in `WorkerConfig` for one reason that is not tidiness: every + * other field of `WorkerConfig` is safe to log, and this one is not. Keeping it out of that + * object means the config a cycle carries around — and that ends up in a log line the day + * someone logs it — never contains a credential. It is read once, at the entry point, and + * handed straight to `AgentSettings`. + * + * There are two modes, and which one is in use is **explicit configuration, never inference**. + * That is the whole design of this file. Both credentials look alike to the SDK and bill + * completely differently: one draws on an organisation's Anthropic account, the other on a + * person's Claude subscription. Picking whichever happened to be present in the environment + * would make the billed party a property of the pod's env rather than of a decision, and the + * failure is silent — a run that quietly spends someone's personal quota looks exactly like a + * working one. + * + * ## ⚠️ `subscription` mode needs Anthropic's approval + * + * Anthropic's Agent SDK documentation states that, unless previously approved, claude.ai login + * and its rate limits may not be used for products built on the Agent SDK. This module makes + * the mode reachable because the operator asked for it; it cannot make it permitted. Whoever + * sets `MODEL_AUTH=subscription` is asserting that this deployment has that approval. + * + * Three operational consequences, none of which code can fix: + * + * - Rate limits belong to the account, so the worker and that person's own interactive use + * share one quota and starve each other. + * - Attribution is that person, not the worker — the same problem the README records for the + * shared Jira service account, now for the model too. + * - Subscription tokens expire. When one lapses the pod crash-loops, by design (see below), + * rather than running on unclear credentials. + */ + +/** The credential for an organisation's Anthropic account. Billed to that account. */ +const API_KEY_ENV = 'ANTHROPIC_API_KEY'; + +/** The credential for a person's Claude subscription. Billed to, and rate-limited as, them. */ +const SUBSCRIPTION_ENV = 'CLAUDE_CODE_OAUTH_TOKEN'; + +/** Selects which of the two the worker authenticates with. Defaults to the org-billed key. */ +const AUTH_MODE_ENV = 'MODEL_AUTH'; + +type ModelAuthMode = 'api-key' | 'subscription'; + +const AUTH_MODES: readonly ModelAuthMode[] = ['api-key', 'subscription']; + +const DEFAULT_AUTH_MODE: ModelAuthMode = 'api-key'; + +/** Which environment variable each mode reads, and nothing else may be substituted for it. */ +const CREDENTIAL_ENV: Record = { + 'api-key': API_KEY_ENV, + subscription: SUBSCRIPTION_ENV, +}; + +/** + * The credential, carrying which kind it is. + * + * A tagged value rather than a bare string because the two are injected into the model's + * subprocess under *different* variable names, and getting that wrong does not fail loudly — + * the SDK simply finds no credential where it looked. See `modelEnv` in sdkOptions.ts. + */ +interface ModelCredential { + readonly mode: ModelAuthMode; + /** The variable it was read from. Reported in the boot log; the value never is. */ + readonly source: string; + readonly value: string; +} + +/** + * A missing or unusable model credential. + * + * Distinct class rather than a bare `Error` for the same reason as `ConfigError` in + * src/common/workerConfig.ts: this is a deployment fault, discovered at boot, and it must not + * read as a ticket that failed. + */ +class AgentConfigError extends Error { + public constructor(message: string) { + super(message); + this.name = 'AgentConfigError'; + } +} + +function readMode(env: NodeJS.ProcessEnv): ModelAuthMode { + const raw = env[AUTH_MODE_ENV]?.trim() ?? ''; + + if (raw === '') { + return DEFAULT_AUTH_MODE; + } + + const mode = AUTH_MODES.find((candidate) => candidate === raw.toLowerCase()); + + if (mode === undefined) { + // Not a fallback to the default: a typo in the mode would silently bill the wrong party, + // which is the exact failure this file exists to prevent. + throw new AgentConfigError(`${AUTH_MODE_ENV} must be one of ${AUTH_MODES.join(', ')} — got '${raw}'.`); + } + + return mode; +} + +/** + * The model credential, or a thrown `AgentConfigError`. + * + * Thrown rather than refused-as-a-value on purpose: a worker with no credential cannot do the + * one thing it exists for, and every ticket it claimed in the meantime would be a claim burnt + * for nothing. Boot is the cheapest place to find out. + * + * The mode's own variable is the *only* one consulted. The other mode's credential being + * present is never a fallback, and is reported when the expected one is missing — a deployment + * that set the token but not the mode is a likely mistake and worth naming, whereas quietly + * using it would be the silent mis-billing this module refuses to allow. + * + * The env map is a parameter so tests drive it with plain objects, exactly as + * `loadWorkerConfig` does — nothing in the suite mutates `process.env`. + */ +function readModelCredential(env: NodeJS.ProcessEnv = process.env): ModelCredential { + const mode = readMode(env); + const source = CREDENTIAL_ENV[mode]; + const value = env[source]?.trim() ?? ''; + + if (value !== '') { + return { mode, source, value }; + } + + // The other mode's credential, when that is the one that happens to be present. Naming it is + // the actionable half of the message: setting a token and forgetting the mode is the mistake + // an operator actually makes, and using it anyway is the silent mis-billing this refuses. + const found = AUTH_MODES.filter((candidate) => candidate !== mode) + .map((candidate) => CREDENTIAL_ENV[candidate]) + .filter((name) => (env[name]?.trim() ?? '') !== ''); + const hint = + found.length > 0 + ? ` ${found.join(' and ')} is set, but ${AUTH_MODE_ENV} is '${mode}', and one mode's credential is never used for the other.` + : ''; + + throw new AgentConfigError(`${source} must be set — ${AUTH_MODE_ENV} is '${mode}' and the worker has no other way to reach the model.${hint}`); +} + +export { AgentConfigError, API_KEY_ENV, AUTH_MODE_ENV, AUTH_MODES, CREDENTIAL_ENV, DEFAULT_AUTH_MODE, readModelCredential, SUBSCRIPTION_ENV }; +export type { ModelAuthMode, ModelCredential }; diff --git a/src/agent/implementer.ts b/src/agent/implementer.ts index ef3958a..337cf1a 100644 --- a/src/agent/implementer.ts +++ b/src/agent/implementer.ts @@ -17,8 +17,9 @@ import type { AgentLimits, DescriptionPort, ReleasePort } from './types'; * Kept beside the code it composes rather than in an entry point on purpose. `src/index.ts` and * `runCycle` belong to the wiring slice, and every collaborator they would otherwise construct * by hand is one more thing that can be wired subtly wrong — a `NpmTestRunner` built on a - * command runner with no environment scrubbing, say, or an agent constructed with a key read - * somewhere other than `readApiKey`. Calling this leaves them one line and no choices. + * command runner with no environment scrubbing, say, or an agent constructed with a credential + * read somewhere other than `readModelCredential`. Calling this leaves them one line and no + * choices. */ interface ImplementerOptions { readonly logger: Logger; @@ -28,7 +29,7 @@ interface ImplementerOptions { readonly description: DescriptionPort; /** Overridden only to spend less. The defaults are the conservative ones. */ readonly limits?: AgentLimits; - /** Read for the API key, and stripped of the worker's own secrets before the model sees it. */ + /** Read for the model credential, and stripped of every secret before the model sees it. */ readonly env?: NodeJS.ProcessEnv; readonly model?: string; } @@ -36,10 +37,11 @@ interface ImplementerOptions { /** * Everything `implementTicket` needs, built from the environment the pod was given. * - * Throws `AgentConfigError` if there is no `ANTHROPIC_API_KEY`, which is why this belongs at - * boot and not inside a cycle: a worker with no credential cannot do the one thing it exists - * for, and finding that out mid-cycle means a ticket claimed and handed straight back. Failing - * at start-up makes it a pod that will not come up — the loudest thing a missing Secret can be. + * Throws `AgentConfigError` if the credential for the configured `MODEL_AUTH` mode is missing, + * which is why this belongs at boot and not inside a cycle: a worker with no credential cannot + * do the one thing it exists for, and finding that out mid-cycle means a ticket claimed and + * handed straight back. Failing at start-up makes it a pod that will not come up — the loudest + * thing a missing Secret, or an expired subscription token, can be. */ function createImplementer(options: ImplementerOptions): ImplementDeps { const { logger, release, description, limits = DEFAULT_AGENT_LIMITS, env = process.env, model } = options; diff --git a/src/agent/sdkAgent.ts b/src/agent/sdkAgent.ts index 97b6c91..9290980 100644 --- a/src/agent/sdkAgent.ts +++ b/src/agent/sdkAgent.ts @@ -1,5 +1,5 @@ import { query } from '@anthropic-ai/claude-agent-sdk'; -import { readApiKey } from './apiKey'; +import { readModelCredential } from './credential'; import { buildTaskPrompt } from './prompt'; import { buildAgentOptions, foldMessages, type AgentQueryOptions, type AgentSettings } from './sdkOptions'; import type { AgentPort, AgentRun, AgentRunRequest } from './types'; @@ -31,12 +31,11 @@ class SdkAgent implements AgentPort { /** Injectable so the options-to-SDK seam is testable without the network. */ private readonly runQuery: RunQuery = query ) { - if (settings.apiKey.trim() === '') { - // Failing here rather than at the first ticket. An empty key means the deployment's - // Secret did not arrive, and the worker discovering that mid-cycle would burn a claim. - throw new Error( - 'an Anthropic API key is required — the worker authenticates with a key from its deployment Secret, never an interactive login' - ); + if (settings.credential.value.trim() === '') { + // Failing here rather than at the first ticket. An empty credential means the + // deployment's Secret did not arrive, and the worker discovering that mid-cycle would + // burn a claim. + throw new Error(`a model credential is required — ${settings.credential.source} was empty, and the worker has no other way to reach the model`); } } @@ -60,10 +59,11 @@ class SdkAgent implements AgentPort { /** * An agent wired to the environment the pod was given. * - * The one place the credential is read, so "the worker authenticates with a key from a Secret" - * is a single line someone can check rather than a claim. There is no interactive-login path - * for this to fall back to — `readApiKey` refuses to take one — and no branch here that could - * grow one later. + * The one place the credential is read, so which account a run bills is a single line someone + * can check rather than a claim. Which of the two credentials it reads is decided by + * `MODEL_AUTH` inside `readModelCredential`, never by what happens to be set here — see that + * module for why inference would be the wrong design, and for the approval `subscription` mode + * requires. * * Composed at an entry point (src/index.ts, src/dryRun.ts) alongside `new McpJira(...)`, in the * same style as every other collaborator in the worker path: plain construction, no container. @@ -71,7 +71,7 @@ class SdkAgent implements AgentPort { * and the release path from MAPCO-11431 before there is anything to hand this. */ function createSdkAgent(env: NodeJS.ProcessEnv = process.env, model?: string): SdkAgent { - return new SdkAgent({ apiKey: readApiKey(env), model, env }); + return new SdkAgent({ credential: readModelCredential(env), model, env }); } export { createSdkAgent, SdkAgent }; diff --git a/src/agent/sdkOptions.ts b/src/agent/sdkOptions.ts index 8e70649..0fd1c4a 100644 --- a/src/agent/sdkOptions.ts +++ b/src/agent/sdkOptions.ts @@ -1,4 +1,5 @@ import { tail, withoutSecrets } from '../workspace/subprocess'; +import type { ModelCredential } from './credential'; import { AGENT_GUARDRAILS } from './prompt'; import { NO_USAGE } from './usage'; import type { AgentOutcome, AgentRun, AgentRunRequest, TokenUsage } from './types'; @@ -108,14 +109,14 @@ interface AgentQueryOptions { interface AgentSettings { /** - * The Anthropic API key, passed in rather than read from here. + * The model credential, passed in rather than read from here. * - * The worker authenticates with a key it was given — from an OpenShift Secret in the - * cluster — and never with an interactive login. Handing it in as a value is what makes - * that checkable: this module has no other way to authenticate, and the ambient - * login credential is stripped out of the child environment below. + * Handing it in as a value is what makes the authentication path checkable: this module has + * no other way to authenticate, and every credential in the ambient environment — including + * the one for the mode *not* in use — is stripped out of the child environment below. So the + * model's process sees exactly one credential, the one `readModelCredential` chose. */ - readonly apiKey: string; + readonly credential: ModelCredential; readonly model?: string; /** The environment the model's process derives its own from. Injectable for tests. */ readonly env?: NodeJS.ProcessEnv; @@ -142,16 +143,18 @@ function readString(source: Record, key: string): string { * * The SDK replaces the child environment wholesale rather than merging, so `process.env` has * to be spread in by hand or the child loses `PATH` and `HOME`. That is also the opportunity - * to take things away: the worker's own credentials go, including the interactive-login token - * that could otherwise authenticate the run as a person, and the API key goes back in - * explicitly as the one credential the model's process is meant to have. + * to take things away: **every** credential the worker holds is scrubbed first, and exactly one + * goes back in — the one whose mode was configured. + * + * Scrub-then-inject rather than injecting over the top, because the two modes read different + * variables. Leaving the unused one in place would mean a pod that has both set could have the + * SDK pick the other, which is precisely the silent mis-billing `readModelCredential` refuses + * to allow. Scrubbing first makes the choice singular by construction. */ function modelEnv(settings: AgentSettings): Record { - return { - ...withoutSecrets(settings.env ?? process.env), - // eslint-disable-next-line @typescript-eslint/naming-convention -- an environment variable name - ANTHROPIC_API_KEY: settings.apiKey, - }; + const scrubbed = withoutSecrets(settings.env ?? process.env); + + return { ...scrubbed, [settings.credential.source]: settings.credential.value }; } /** The options one run of the model is given. */ diff --git a/tests/unit/agent/apiKey.spec.ts b/tests/unit/agent/apiKey.spec.ts deleted file mode 100644 index c0d2b13..0000000 --- a/tests/unit/agent/apiKey.spec.ts +++ /dev/null @@ -1,49 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import { AgentConfigError, API_KEY_ENV, readApiKey } from '@src/agent/apiKey'; - -/* eslint-disable @typescript-eslint/naming-convention -- environment variable names */ - -describe('readApiKey', () => { - it('should take the key the deployment put in the environment.', () => { - expect(readApiKey({ ANTHROPIC_API_KEY: 'sk-from-the-secret' })).toBe('sk-from-the-secret'); - }); - - it('should trim it, because a Secret mounted from a file usually ends in a newline.', () => { - expect(readApiKey({ ANTHROPIC_API_KEY: 'sk-from-the-secret\n' })).toBe('sk-from-the-secret'); - }); - - it('should refuse to start with no key rather than discovering it on the first ticket.', () => { - // A worker with no key cannot do the one thing it exists for, and every ticket it claimed - // in the meantime would be a claim burnt for nothing. - expect(() => readApiKey({})).toThrow(AgentConfigError); - }); - - it('should treat an empty value as unset, which is what a missing Secret key looks like.', () => { - expect(() => readApiKey({ ANTHROPIC_API_KEY: ' ' })).toThrow(AgentConfigError); - }); - - it('should name the variable that has to be set, so the message is actionable.', () => { - expect(() => readApiKey({})).toThrow(API_KEY_ENV); - }); - - it('should never fall back to an interactive-login credential.', () => { - // The acceptance criterion is "never an interactive login", and the way that gets broken - // is a default rather than a decision — a session token picked up because it was lying - // around. On a developer's laptop that would bill a person's account and look like success. - expect(() => readApiKey({ CLAUDE_CODE_OAUTH_TOKEN: 'oauth-of-a-person' })).toThrow(AgentConfigError); - }); - - it('should say that it saw a login token and would not use it.', () => { - expect(() => readApiKey({ CLAUDE_CODE_OAUTH_TOKEN: 'oauth-of-a-person' })).toThrow(/CLAUDE_CODE_OAUTH_TOKEN/u); - }); - - it('should not accept the auth-token variable as a key either.', () => { - expect(() => readApiKey({ ANTHROPIC_AUTH_TOKEN: 'bearer-of-something-else' })).toThrow(AgentConfigError); - }); - - it('should be a fault of its own kind, so a bad deployment cannot read as a failed ticket.', () => { - expect(() => readApiKey({})).toThrow(expect.objectContaining({ name: 'AgentConfigError' })); - }); -}); - -/* eslint-enable @typescript-eslint/naming-convention */ diff --git a/tests/unit/agent/credential.spec.ts b/tests/unit/agent/credential.spec.ts new file mode 100644 index 0000000..c43f26c --- /dev/null +++ b/tests/unit/agent/credential.spec.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from 'vitest'; +import { AgentConfigError, API_KEY_ENV, AUTH_MODE_ENV, readModelCredential, SUBSCRIPTION_ENV } from '@src/agent/credential'; + +/* eslint-disable @typescript-eslint/naming-convention -- environment variable names */ + +describe('readModelCredential', () => { + it('should default to the org-billed API key when no mode is configured.', () => { + // The default is the safe direction: a deployment that says nothing about billing gets the + // account it was provisioned with, not whatever personal token is lying around. + expect(readModelCredential({ ANTHROPIC_API_KEY: 'sk-from-the-secret' })).toStrictEqual({ + mode: 'api-key', + source: API_KEY_ENV, + value: 'sk-from-the-secret', + }); + }); + + it('should take the subscription token when that mode is configured.', () => { + expect(readModelCredential({ MODEL_AUTH: 'subscription', CLAUDE_CODE_OAUTH_TOKEN: 'oauth-of-a-person' })).toStrictEqual({ + mode: 'subscription', + source: SUBSCRIPTION_ENV, + value: 'oauth-of-a-person', + }); + }); + + it('should accept the mode however it was cased in the manifest.', () => { + expect(readModelCredential({ MODEL_AUTH: 'Subscription', CLAUDE_CODE_OAUTH_TOKEN: 'oauth' }).mode).toBe('subscription'); + }); + + it('should trim, because a Secret mounted from a file usually ends in a newline.', () => { + expect(readModelCredential({ ANTHROPIC_API_KEY: 'sk-from-the-secret\n' }).value).toBe('sk-from-the-secret'); + }); + + it('should never use one mode’s credential for the other.', () => { + // The whole point of the mode being explicit. Falling back would make the billed party a + // property of the pod's environment rather than of a decision, and a run that quietly spent + // a person's quota would look exactly like a working one. + expect(() => readModelCredential({ MODEL_AUTH: 'api-key', CLAUDE_CODE_OAUTH_TOKEN: 'oauth-of-a-person' })).toThrow(AgentConfigError); + expect(() => readModelCredential({ MODEL_AUTH: 'subscription', ANTHROPIC_API_KEY: 'sk-from-the-secret' })).toThrow(AgentConfigError); + }); + + it('should point at the credential it found but would not use, because that is the likely mistake.', () => { + // Setting the token and forgetting the mode is the mistake an operator actually makes. + expect(() => readModelCredential({ CLAUDE_CODE_OAUTH_TOKEN: 'oauth-of-a-person' })).toThrow(/CLAUDE_CODE_OAUTH_TOKEN is set/u); + }); + + it('should refuse an unrecognised mode rather than falling back to the default.', () => { + // A typo would otherwise bill the wrong account silently. + expect(() => readModelCredential({ MODEL_AUTH: 'subscribtion', CLAUDE_CODE_OAUTH_TOKEN: 'oauth' })).toThrow(/MODEL_AUTH must be one of/u); + }); + + it('should refuse to start with no credential rather than discovering it on the first ticket.', () => { + expect(() => readModelCredential({})).toThrow(AgentConfigError); + }); + + it('should treat an empty value as unset, which is what a missing Secret key looks like.', () => { + expect(() => readModelCredential({ ANTHROPIC_API_KEY: ' ' })).toThrow(AgentConfigError); + expect(() => readModelCredential({ MODEL_AUTH: 'subscription', CLAUDE_CODE_OAUTH_TOKEN: ' ' })).toThrow(AgentConfigError); + }); + + it('should name the variable the configured mode needs, so the message is actionable.', () => { + expect(() => readModelCredential({})).toThrow(API_KEY_ENV); + expect(() => readModelCredential({ MODEL_AUTH: 'subscription' })).toThrow(SUBSCRIPTION_ENV); + expect(() => readModelCredential({ MODEL_AUTH: 'subscription' })).toThrow(AUTH_MODE_ENV); + }); + + it('should be a fault of its own kind, so a bad deployment cannot read as a failed ticket.', () => { + expect(() => readModelCredential({})).toThrow(expect.objectContaining({ name: 'AgentConfigError' })); + }); +}); + +/* eslint-enable @typescript-eslint/naming-convention */ diff --git a/tests/unit/agent/implementer.spec.ts b/tests/unit/agent/implementer.spec.ts index 484b457..8e8790c 100644 --- a/tests/unit/agent/implementer.spec.ts +++ b/tests/unit/agent/implementer.spec.ts @@ -2,7 +2,7 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, expect, it } from 'vitest'; -import { AgentConfigError } from '@src/agent/apiKey'; +import { AgentConfigError } from '@src/agent/credential'; import { DEFAULT_AGENT_LIMITS } from '@src/agent/implement'; import { createImplementer, type ImplementerOptions } from '@src/agent/implementer'; import type { DescriptionPort, ReleasePort } from '@src/agent/types'; @@ -33,8 +33,18 @@ describe('createImplementer', () => { expect(() => createImplementer(options({}))).toThrow(AgentConfigError); }); - it('should refuse an interactive login rather than authenticating as a person.', () => { - expect(() => createImplementer(options(WITH_LOGIN))).toThrow(/never as whoever logged in/u); + it('should refuse a login token that the configured mode did not ask for.', () => { + // A login token present with no `MODEL_AUTH` is the operator mistake worth catching: the + // deployment meant to bill a subscription and forgot to say so, and using it anyway would + // bill a person silently. + expect(() => createImplementer(options(WITH_LOGIN))).toThrow(/one mode's credential is never used for the other/u); + }); + + it('should build on a subscription token when that mode is configured.', () => { + /* eslint-disable-next-line @typescript-eslint/naming-convention -- environment variable names */ + const env = { ...WITH_LOGIN, MODEL_AUTH: 'subscription' }; + + expect(() => createImplementer(options(env))).not.toThrow(); }); it('should come up with the conservative bounds when none were configured.', () => { diff --git a/tests/unit/agent/sdkAgent.spec.ts b/tests/unit/agent/sdkAgent.spec.ts index 4dae9e6..841088c 100644 --- a/tests/unit/agent/sdkAgent.spec.ts +++ b/tests/unit/agent/sdkAgent.spec.ts @@ -10,7 +10,10 @@ const request: AgentRunRequest = { }; /* eslint-disable @typescript-eslint/naming-convention -- environment variable names */ -const settings: AgentSettings = { apiKey: 'sk-from-the-secret', env: { PATH: '/usr/bin', GITHUB_TOKEN: 'ghp_pushable' } }; +const settings: AgentSettings = { + credential: { mode: 'api-key', source: 'ANTHROPIC_API_KEY', value: 'sk-from-the-secret' }, + env: { PATH: '/usr/bin', GITHUB_TOKEN: 'ghp_pushable' }, +}; /* eslint-enable @typescript-eslint/naming-convention */ interface Call { @@ -108,6 +111,8 @@ describe('SdkAgent', () => { }); it('should refuse to be built without a key rather than failing on the first ticket.', () => { - expect(() => new SdkAgent({ apiKey: ' ' }, fakeQuery([]))).toThrow(/API key/u); + expect(() => new SdkAgent({ credential: { mode: 'api-key', source: 'ANTHROPIC_API_KEY', value: ' ' } }, fakeQuery([]))).toThrow( + /ANTHROPIC_API_KEY/u + ); }); }); diff --git a/tests/unit/agent/sdkOptions.spec.ts b/tests/unit/agent/sdkOptions.spec.ts index 844694e..5ea8bec 100644 --- a/tests/unit/agent/sdkOptions.spec.ts +++ b/tests/unit/agent/sdkOptions.spec.ts @@ -12,7 +12,7 @@ const request: AgentRunRequest = { /* eslint-disable @typescript-eslint/naming-convention -- environment variable names */ const settings: AgentSettings = { - apiKey: 'sk-from-the-secret', + credential: { mode: 'api-key', source: 'ANTHROPIC_API_KEY', value: 'sk-from-the-secret' }, env: { PATH: '/usr/bin', HOME: '/home/node', @@ -118,7 +118,21 @@ describe('buildAgentOptions', () => { expect(buildAgentOptions(request, settings).env['ANTHROPIC_API_KEY']).toBe('sk-from-the-secret'); }); - it('should not pass the interactive-login credential to the model, so the run cannot authenticate as a person.', () => { + it('should inject the subscription token, and only it, when that is the configured mode.', () => { + // The two modes read different variables, so injecting the wrong one fails silently — the + // SDK simply finds no credential where it looked. + const { env } = buildAgentOptions(request, { + ...settings, + credential: { mode: 'subscription', source: 'CLAUDE_CODE_OAUTH_TOKEN', value: 'oauth-of-a-person' }, + }); + + expect(env['CLAUDE_CODE_OAUTH_TOKEN']).toBe('oauth-of-a-person'); + expect(env['ANTHROPIC_API_KEY']).toBeUndefined(); + }); + + it('should not pass a login credential to the model when it is authenticating as the worker.', () => { + // In api-key mode an ambient login token is scrubbed and never reaches the model, so a run + // cannot end up authenticated as whoever last logged in on this machine. expect(buildAgentOptions(request, settings).env['CLAUDE_CODE_OAUTH_TOKEN']).toBeUndefined(); });