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(); });