From a46f585fd9b5959fa1b5452ca3313442e4008a61 Mon Sep 17 00:00:00 2001 From: razbroc Date: Thu, 27 Aug 2026 11:01:32 +0300 Subject: [PATCH 1/6] 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(); }); From 2b486c90727d4b151dfe69e8e9493aee65686904 Mon Sep 17 00:00:00 2001 From: razbroc Date: Tue, 15 Sep 2026 10:52:52 +0300 Subject: [PATCH 2/6] refactor: rename the subscription auth mode to sdk and make it the default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MODEL_AUTH takes 'sdk' rather than 'subscription', and a deployment that sets nothing now gets it. This worker is meant to run on a Claude subscription, and a default of api-key meant every manifest had to remember to say so. The cost is that the safe direction is no longer the default. A deployment or a laptop carrying only ANTHROPIC_API_KEY used to work and now refuses to start, naming the credential it found — which is the loud half of the trade, and better than billing an account nobody chose. --- src/agent/credential.ts | 31 ++++++++++++------ src/agent/sdkAgent.ts | 2 +- tests/unit/agent/credential.spec.ts | 48 ++++++++++++++++++---------- tests/unit/agent/implementer.spec.ts | 22 ++++++------- tests/unit/agent/sdkOptions.spec.ts | 2 +- 5 files changed, 67 insertions(+), 38 deletions(-) diff --git a/src/agent/credential.ts b/src/agent/credential.ts index 479554b..b413e7a 100644 --- a/src/agent/credential.ts +++ b/src/agent/credential.ts @@ -15,12 +15,13 @@ * failure is silent — a run that quietly spends someone's personal quota looks exactly like a * working one. * - * ## ⚠️ `subscription` mode needs Anthropic's approval + * ## ⚠️ `sdk` mode needs Anthropic's approval, and is the default * * 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. + * the mode reachable because the operator asked for it; it cannot make it permitted. Running + * this worker at all — since `sdk` is now `DEFAULT_AUTH_MODE` — asserts that this deployment + * has that approval. * * Three operational consequences, none of which code can fix: * @@ -35,22 +36,34 @@ /** 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. */ +/** + * The credential for a person's Claude subscription, as `claude setup-token` issues it. + * Billed to, and rate-limited as, that person. This is what `sdk` mode reads. + */ const SUBSCRIPTION_ENV = 'CLAUDE_CODE_OAUTH_TOKEN'; -/** Selects which of the two the worker authenticates with. Defaults to the org-billed key. */ +/** Selects which of the two the worker authenticates with. Defaults to `sdk`. */ const AUTH_MODE_ENV = 'MODEL_AUTH'; -type ModelAuthMode = 'api-key' | 'subscription'; +type ModelAuthMode = 'api-key' | 'sdk'; -const AUTH_MODES: readonly ModelAuthMode[] = ['api-key', 'subscription']; +const AUTH_MODES: readonly ModelAuthMode[] = ['api-key', 'sdk']; -const DEFAULT_AUTH_MODE: ModelAuthMode = 'api-key'; +/** + * The mode a deployment gets when it says nothing. + * + * `sdk` by operator decision: this worker is meant to run on a Claude subscription, and a + * default of `api-key` meant every manifest had to remember to say so. The cost is that the + * safe direction is no longer the default — see the warning at the top of this file, and note + * that a deployment which sets only `ANTHROPIC_API_KEY` now refuses to start rather than + * quietly using it, which is the loud half of the trade. + */ +const DEFAULT_AUTH_MODE: ModelAuthMode = 'sdk'; /** 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, + sdk: SUBSCRIPTION_ENV, }; /** diff --git a/src/agent/sdkAgent.ts b/src/agent/sdkAgent.ts index 9290980..6dea51c 100644 --- a/src/agent/sdkAgent.ts +++ b/src/agent/sdkAgent.ts @@ -62,7 +62,7 @@ class SdkAgent implements AgentPort { * 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 + * module for why inference would be the wrong design, and for the approval `sdk` mode * requires. * * Composed at an entry point (src/index.ts, src/dryRun.ts) alongside `new McpJira(...)`, in the diff --git a/tests/unit/agent/credential.spec.ts b/tests/unit/agent/credential.spec.ts index c43f26c..23105dd 100644 --- a/tests/unit/agent/credential.spec.ts +++ b/tests/unit/agent/credential.spec.ts @@ -4,10 +4,23 @@ import { AgentConfigError, API_KEY_ENV, AUTH_MODE_ENV, readModelCredential, SUBS /* 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({ + it('should default to the subscription token, which is how this worker is meant to run.', () => { + expect(readModelCredential({ CLAUDE_CODE_OAUTH_TOKEN: 'oauth-of-a-person' })).toStrictEqual({ + mode: 'sdk', + source: SUBSCRIPTION_ENV, + value: 'oauth-of-a-person', + }); + }); + + it('should refuse a lone API key rather than silently using it, now that sdk is the default.', () => { + // The consequence of the default: a deployment or laptop carrying only ANTHROPIC_API_KEY + // used to work and now will not start. Refusing loudly is the point — the alternative is + // billing an account nobody chose, which reads as success. + expect(() => readModelCredential({ ANTHROPIC_API_KEY: 'sk-from-the-secret' })).toThrow(/ANTHROPIC_API_KEY is set/u); + }); + + it('should take the org-billed key when that mode is asked for.', () => { + expect(readModelCredential({ MODEL_AUTH: 'api-key', ANTHROPIC_API_KEY: 'sk-from-the-secret' })).toStrictEqual({ mode: 'api-key', source: API_KEY_ENV, value: 'sk-from-the-secret', @@ -15,19 +28,19 @@ describe('readModelCredential', () => { }); 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', + expect(readModelCredential({ MODEL_AUTH: 'sdk', CLAUDE_CODE_OAUTH_TOKEN: 'oauth-of-a-person' })).toStrictEqual({ + mode: 'sdk', 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'); + expect(readModelCredential({ MODEL_AUTH: 'Sdk', CLAUDE_CODE_OAUTH_TOKEN: 'oauth' }).mode).toBe('sdk'); }); 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'); + expect(readModelCredential({ MODEL_AUTH: 'api-key', 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.', () => { @@ -35,12 +48,15 @@ describe('readModelCredential', () => { // 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); + expect(() => readModelCredential({ MODEL_AUTH: 'sdk', 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); + // Naming the one it saw is the actionable half: a manifest that set a credential and the + // wrong mode is far more common than one that set nothing. + expect(() => readModelCredential({ MODEL_AUTH: 'api-key', 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.', () => { @@ -53,14 +69,14 @@ describe('readModelCredential', () => { }); 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); + expect(() => readModelCredential({ MODEL_AUTH: 'api-key', ANTHROPIC_API_KEY: ' ' })).toThrow(AgentConfigError); + expect(() => readModelCredential({ 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); + expect(() => readModelCredential({})).toThrow(SUBSCRIPTION_ENV); + expect(() => readModelCredential({ MODEL_AUTH: 'api-key' })).toThrow(API_KEY_ENV); + expect(() => readModelCredential({ MODEL_AUTH: 'api-key' })).toThrow(AUTH_MODE_ENV); }); it('should be a fault of its own kind, so a bad deployment cannot read as a failed ticket.', () => { diff --git a/tests/unit/agent/implementer.spec.ts b/tests/unit/agent/implementer.spec.ts index 8e8790c..55216bd 100644 --- a/tests/unit/agent/implementer.spec.ts +++ b/tests/unit/agent/implementer.spec.ts @@ -9,7 +9,7 @@ import type { DescriptionPort, ReleasePort } from '@src/agent/types'; import { fakeLogger } from '@tests/helpers/fakeLogger'; /* eslint-disable @typescript-eslint/naming-convention -- environment variable names */ -const WITH_KEY: NodeJS.ProcessEnv = { PATH: '/usr/bin', ANTHROPIC_API_KEY: 'sk-from-the-secret' }; +const WITH_KEY: NodeJS.ProcessEnv = { PATH: '/usr/bin', MODEL_AUTH: 'api-key', ANTHROPIC_API_KEY: 'sk-from-the-secret' }; const WITH_LOGIN: NodeJS.ProcessEnv = { PATH: '/usr/bin', CLAUDE_CODE_OAUTH_TOKEN: 'oauth-of-a-person' }; /* eslint-enable @typescript-eslint/naming-convention */ @@ -33,18 +33,18 @@ describe('createImplementer', () => { expect(() => createImplementer(options({}))).toThrow(AgentConfigError); }); - 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.', () => { + it('should refuse an API key that the configured mode did not ask for.', () => { + // The operator mistake worth catching, now that `sdk` is the default: a deployment carrying + // only ANTHROPIC_API_KEY meant to bill that account and never said so, and using it anyway + // would bill an account nobody chose. /* eslint-disable-next-line @typescript-eslint/naming-convention -- environment variable names */ - const env = { ...WITH_LOGIN, MODEL_AUTH: 'subscription' }; + expect(() => createImplementer(options({ PATH: '/usr/bin', ANTHROPIC_API_KEY: 'sk-from-the-secret' }))).toThrow( + /one mode's credential is never used for the other/u + ); + }); - expect(() => createImplementer(options(env))).not.toThrow(); + it('should build on a subscription token with no mode set, because sdk is the default.', () => { + expect(() => createImplementer(options(WITH_LOGIN))).not.toThrow(); }); it('should come up with the conservative bounds when none were configured.', () => { diff --git a/tests/unit/agent/sdkOptions.spec.ts b/tests/unit/agent/sdkOptions.spec.ts index 5ea8bec..a898900 100644 --- a/tests/unit/agent/sdkOptions.spec.ts +++ b/tests/unit/agent/sdkOptions.spec.ts @@ -123,7 +123,7 @@ describe('buildAgentOptions', () => { // 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' }, + credential: { mode: 'sdk', source: 'CLAUDE_CODE_OAUTH_TOKEN', value: 'oauth-of-a-person' }, }); expect(env['CLAUDE_CODE_OAUTH_TOKEN']).toBe('oauth-of-a-person'); From 02a12a0c771f34d9f7421b2d484d00f01fe5011d Mon Sep 17 00:00:00 2001 From: razbroc Date: Tue, 15 Sep 2026 10:53:52 +0300 Subject: [PATCH 3/6] fix: make the helm chart render, and wire the sdk credential into it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things, all of which had to happen before the MODEL_AUTH block could be trusted. **The chart could not render at all.** `helm/values.yaml` set `mclabels.component: worker`, and the subchart's values.schema.json restricts that field to frontend | backend | database | proxy-server | cache-server | infrastructure. Every `helm template` and `helm lint` failed on it, with or without the dependency present — this would have failed CI and blocked a deploy regardless of the auth work. Set to `backend`, with the allowed set named in a comment. That label feeds org-wide dashboards and ownership tooling, so change it if MapColonies conventionally tags pollers as `infrastructure`. **The MODEL_AUTH block is now verified rather than assumed.** Rendered in both modes against the real mclabels chart and parsed with a YAML parser: the expected variable is present at the right depth in containers[0].env and the other branch's variable is absent. `helm lint` passes. **Trailing whitespace in deployment.yaml** leaked into rendered output, landing on the new block's last line as `key: apiKey `. Stripped. The chart now defaults to `modelAuth: sdk` to match DEFAULT_AUTH_MODE, and the secretKeyRef key follows the mode — `oauthToken` for sdk, `apiKey` for api-key. README records how to fetch mclabels. Anonymous pull from the OCI registry is refused (401), so a developer needs `az acr login --name acrarolibotnonprod` or `helm registry login` first; that is documented rather than presented as solved. --- README.md | 36 ++++++++++++++++++++++++++-------- helm/templates/deployment.yaml | 10 +++++----- helm/values.yaml | 26 +++++++++++++----------- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index a153c66..a62663b 100644 --- a/README.md +++ b/README.md @@ -78,9 +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 | +| `MODEL_AUTH` | `sdk` | Which account model calls are billed to: `sdk` or `api-key`. An unrecognised value refuses to start rather than falling back | +| `CLAUDE_CODE_OAUTH_TOKEN` | required for `sdk` | A Claude subscription token from `claude setup-token`, from a Secret. Billed to, and rate-limited as, that person — see the warning below | | `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`. @@ -97,12 +97,12 @@ decision — and the failure is silent, because a run that quietly spends someon 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 +> **⚠️ `sdk` needs Anthropic's approval, and is the default.** 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. Running this worker asserts that this > deployment has that approval — the code cannot check it. -Three consequences of `subscription` mode that no code can fix: +Three consequences of `sdk` 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. @@ -111,7 +111,9 @@ Three consequences of `subscription` mode that no code can fix: - **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. +`api-key` mode has none of these. It is one `MODEL_AUTH` away if the trade stops being +worth it — and note the flip side of the default: a deployment carrying only +`ANTHROPIC_API_KEY` now refuses to start rather than quietly using it. ## Claiming and releasing @@ -152,7 +154,25 @@ unassigned and `In Progress` — which polls straight back in, forever. transition name as a fallback, which covers both shapes, and a `no-transition` refusal logs the `offered` names — so the first real run reports the vocabulary rather than refusing in silence. Confirm it from that log line before trusting a deployment. -- `helm lint` needs the private `mclabels` dependency and fails without registry access. +- **Rendering the chart needs a credential you must supply yourself.** The `mclabels` + dependency lives in a private ACR (`oci://acrarolibotnonprod.azurecr.io/helm/infra`) that + **rejects anonymous pull** — an unauthenticated `helm dependency build` fails with + `401 Unauthorized` from the registry's token endpoint. Once you are logged in, the rest is + repeatable: + + ```sh + # One-time, per machine. Use your own ACR credential — an AAD login: + az acr login --name acrarolibotnonprod + # ...or a registry token / service principal, if that is what you were issued: + helm registry login acrarolibotnonprod.azurecr.io + # Then, from the repo root: + helm dependency build helm + helm lint helm + helm template t helm --set worker.modelSecretName=test-secret + ``` + + `helm dependency build` writes `helm/charts/mclabels-.tgz`. That is a build + artifact: it is covered by the root `.gitignore` `*.tgz` rule and must never be committed. ## Dry run diff --git a/helm/templates/deployment.yaml b/helm/templates/deployment.yaml index a3b62b7..515ddb7 100644 --- a/helm/templates/deployment.yaml +++ b/helm/templates/deployment.yaml @@ -50,10 +50,10 @@ spec: imagePullPolicy: {{ .pullPolicy | default "IfNotPresent" }} {{- end }} {{- if .Values.command }} - command: + command: {{- toYaml .Values.command | nindent 12 }} {{- if .Values.args }} - args: + args: {{- toYaml .Values.args | nindent 12 }} {{- end }} {{- end }} @@ -85,7 +85,7 @@ spec: value: {{ .Values.worker.maxConcurrentTickets | quote }} - name: MODEL_AUTH value: {{ .Values.worker.modelAuth | quote }} - {{- if eq .Values.worker.modelAuth "subscription" }} + {{- if eq .Values.worker.modelAuth "sdk" }} - name: CLAUDE_CODE_OAUTH_TOKEN valueFrom: secretKeyRef: @@ -106,7 +106,7 @@ spec: {{- end }} {{- if .Values.extraEnvVars }} {{- toYaml .Values.extraEnvVars | nindent 12 }} - {{- end }} + {{- end }} envFrom: - configMapRef: name: {{ include "developer-agent-bot.fullname" . }} @@ -125,5 +125,5 @@ spec: {{- end }} {{- if .Values.extraVolumes -}} {{ tpl (toYaml .Values.extraVolumes) . | nindent 8 }} - {{- end }} + {{- end }} {{- end -}} diff --git a/helm/values.yaml b/helm/values.yaml index eb3dac1..1666449 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -4,7 +4,10 @@ global: metrics: {} mclabels: - component: worker + # Must be one of the values mclabels' values.schema.json allows: + # frontend | backend | database | proxy-server | cache-server | infrastructure. + # There is no "worker" member — this outbound-only poller is a backend process. + component: backend partOf: developer-agent-bot owner: common prometheus: @@ -68,17 +71,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. + # Which account the model calls are billed to. `sdk` reads CLAUDE_CODE_OAUTH_TOKEN from the + # Secret below — a Claude subscription token from `claude setup-token`; `api-key` reads + # ANTHROPIC_API_KEY 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. + # `sdk` 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 — running in this mode 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: 'sdk' + # Secret holding the model credential. Key name must be `oauthToken` for modelAuth: sdk, + # or `apiKey` for modelAuth: api-key. The pod will not start without it. modelSecretName: '' env: From b59ce4993fdf37a1061a011063ce9f5588cfda44 Mon Sep 17 00:00:00 2001 From: razbroc Date: Tue, 15 Sep 2026 11:09:40 +0300 Subject: [PATCH 4/6] fix: align the mclabels block with the org's service charts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compared against mapproxy-api, cleaner, exporter-trigger, geojson-viewer and the rest. Two things were wrong and one is now documented. **prometheus.enabled is false.** Almost every chart in the org sets it true, and this one must not: the worker is outbound-only — no Service, no Route, no container port — and although containerConfig.ts builds a prom-client Registry, nothing ever serves it over HTTP. Left true, mclabels stamped `prometheus.io/scrape: "true"` with port 8080 and /metrics onto the pod, giving Prometheus a target that could only ever fail. geojson-viewer, the other chart with nothing to scrape, does the same. **component: backend is confirmed, not guessed.** Every non-frontend chart in the org uses it, and `worker` appears in no schema. The earlier open question about `infrastructure` is settled: nothing uses it. **The environment label is documented.** Only `mclabels.environment` feeds `mapcolonies.io/environment` — `global.environment` and a top-level `environment` are both ignored by the subchart — and leaving it unset stamps the literal string `undefined`. Added the commented `#environment:` line the other charts carry, so a deploy-time values file supplies it. `gisDomain` stays absent: the raster-owned charts carry it, the common-owned tooling charts do not, and this is one of those. --- helm/values.yaml | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/helm/values.yaml b/helm/values.yaml index 1666449..3889930 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -4,14 +4,28 @@ global: metrics: {} mclabels: + # Set per environment at deploy time, as in mapproxy-api and the other service charts. Only + # `mclabels.environment` feeds the label — `global.environment` and a top-level `environment` + # are both ignored by the subchart — and leaving it unset stamps + # `mapcolonies.io/environment: undefined` on the pod rather than omitting the label. + #environment: development # Must be one of the values mclabels' values.schema.json allows: # frontend | backend | database | proxy-server | cache-server | infrastructure. - # There is no "worker" member — this outbound-only poller is a backend process. + # There is no "worker" member — this outbound-only poller is a backend process, which is + # what every other MapColonies service chart uses (mapproxy-api, cleaner, exporter-trigger). component: backend partOf: developer-agent-bot + # `gisDomain` is deliberately absent. The raster-owned charts carry it; the common-owned + # tooling charts (geojson-viewer) do not, and this is one of those. owner: common prometheus: - enabled: true + # False, unlike almost every other chart in the org, because this service has nothing to + # scrape. It is outbound-only — no Service, no Route, no container port — and while + # `containerConfig.ts` builds a prom-client Registry, nothing ever serves it over HTTP. + # Left true, mclabels stamps prometheus.io/scrape=true with port 8080 and /metrics onto + # the pod, and Prometheus gets a target that can only ever fail. Flip this the same day + # something starts listening, not before. + enabled: false enabled: true replicaCount: 1 From c05c7235cd67a499b4446fc896a55214f3c094cb Mon Sep 17 00:00:00 2001 From: razbroc Date: Tue, 15 Sep 2026 12:21:02 +0300 Subject: [PATCH 5/6] fix: stop committing the node_modules symlink A `node_modules` symlink pointing at the main checkout was committed with this branch. `.gitignore` listed `node_modules/` with a trailing slash, which matches a directory but not a symlink, so `git add -A` picked it up. On CI the symlink dangles, which broke two jobs: - eslint: `npm ci` could not populate it, so lint-action fell through to `npx eslint` and failed with 'npx canceled due to missing packages'. - build_docker_image: 'cannot replace to directory .../node_modules with file'. Drop the entry and drop the trailing slash so the ignore rule covers both. --- .gitignore | 2 +- node_modules | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) delete mode 120000 node_modules diff --git a/.gitignore b/.gitignore index 8ba7348..442b0a7 100644 --- a/.gitignore +++ b/.gitignore @@ -42,7 +42,7 @@ bower_components build/Release # Dependency directories -node_modules/ +node_modules jspm_packages/ # TypeScript v1 declaration files diff --git a/node_modules b/node_modules deleted file mode 120000 index 4daf7de..0000000 --- a/node_modules +++ /dev/null @@ -1 +0,0 @@ -/home/razbro/Repos/developer-agent-bot/node_modules \ No newline at end of file From 65732c3c46e940b38738541a65e41c4a5488f6e6 Mon Sep 17 00:00:00 2001 From: razbroc Date: Tue, 15 Sep 2026 12:35:40 +0300 Subject: [PATCH 6/6] docs: describe the implement-and-verify slice The README still said the current slice was MAPCO-11431 and described a worker that writes no code. It now covers what is actually on this branch. New section on implementing a ticket: why the model's tool surface is the security control rather than the prompt, why a write counts only when its tool_result comes back (reading the attempt alone certified diffs that did not exist), and why the test command is taken off the pristine clone before the first hand-off so the model cannot rewrite what grades it. Two chart decisions added to "Things that look wrong but aren't": this chart sets prometheus.enabled false where almost every other one sets it true, because there is nothing listening to scrape; and mclabels.environment is the only key that feeds the environment label, with global.environment and a top-level environment both ignored and the unset case stamping the literal string "undefined". Two known gaps recorded rather than left for a reader to discover: nothing is wired into runCycle, so this is a library with tests and not a behaviour the deployed worker has; and DescriptionPort has no implementation, so every ticket is refused before the first model turn. --- README.md | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index a62663b..49328dd 100644 --- a/README.md +++ b/README.md @@ -4,9 +4,13 @@ Pulls `agent-ready` Jira tickets from the MAPCO project, implements them, and op requests. Designed in [MAPCO-11374](https://mapcolonies.atlassian.net/browse/MAPCO-11374), substrate decided in [MAPCO-11377](https://mapcolonies.atlassian.net/browse/MAPCO-11377). -**Current slice: MAPCO-11431.** It walks the whole Jira state machine with nothing in the -middle — it claims a ticket and hands it straight back with a comment. It touches no -repository and writes no code yet. +**Current slice: MAPCO-11434.** On top of the claim/release cycle (MAPCO-11431) it can now +hand a claimed ticket to the Claude Agent SDK, let the model change a clone, and run that +clone's own tests before anyone sees the diff. Nothing is committed, nothing is pushed, and +no branch is created — that is MAPCO-11436. + +The pieces exist but are **not wired into `runCycle` yet**: labelling a ticket `agent-ready` +today still gets it claimed, commented and handed straight back. See *Known gaps*. ## Shape @@ -53,6 +57,18 @@ Verified against the live Jira instance in MAPCO-11427, and each one bit a first - **Transitions are resolved per issue, never cached.** Transition ids are not portable across issue types: id `4` starts work on a Tech Requirement and *ends* it on a Task. +- **The chart sets `prometheus.enabled: false`** while almost every other MapColonies chart + sets it true. This service has nothing to scrape: it is outbound-only, with no Service, no + Route and no container port, and although `containerConfig.ts` builds a prom-client + registry nothing ever serves it over HTTP. Left true, `mclabels` stamps + `prometheus.io/scrape: "true"` with port 8080 and `/metrics` onto the pod and Prometheus + gets a target that can only ever fail. Flip it the day something starts listening. +- **`mclabels.environment` is the only key that sets the environment label.** A + `global.environment` or a top-level `environment` is ignored by the subchart, and leaving + all three unset stamps the literal string `undefined` rather than omitting the label. The + commented `#environment:` line in `helm/values.yaml` is where a deploy-time values file + supplies it. + ## Ticket titles The repo a ticket is about comes from its title: `: `. @@ -136,8 +152,58 @@ part-way, the ticket is left held by the bot and `In Progress`, which the query boot-time orphan sweep (MAPCO-11432) recovers. Unassigning first risks leaving a ticket unassigned and `In Progress` — which polls straight back in, forever. +## Implementing a ticket + +A claimed ticket, a clone, and a bounded number of attempts. Each attempt hands the ticket to +the model, then runs the clone's own test command; an attempt counts as done only when that +command exits zero. Running out of attempts is a give-up, which already has a meaning — +comment, hand back, count the attempt. + +### The model's tool surface is the control + +`tools` is the SDK's base-set option, so the tools not named there are never built for the +session: there is no `Bash` to reach for, no subagent to delegate a `git push` to, and no MCP +server to be handed. `Bash`, `PowerShell`, the worktree tools, `Agent`/`Task`, the network +tools, `Skill` and `mcp__*` are denied by name as well, because a deny rule outranks every +other step of the permission evaluation. + +`settingSources: []` with `strictMcpConfig` matters more than it looks. The clone is a +repository off the internet, and with the project source enabled its own +`.claude/settings.json` would be read as permission rules — letting the thing being worked on +widen what may be done to it. The cost is that the clone's `CLAUDE.md` is not loaded either, +which is a real loss of local convention and the right side of the trade. + +There is a sentence in the prompt asking the model not to use git. It is documented as *not* +being the control; it is there so a model that goes looking is told why, and `deniedTools` +reports it in the run if one does. + +### A write counts only when its result comes back + +"The tree changed" is read from the `tool_result` that answers each write, paired by id — not +from the `tool_use` that requested it. Reading the attempt alone was a real bug: an `Edit` +whose `old_string` did not match, or a `Write` the permission layer refused, reported a +change, and the worker went on to test an unmodified clone, watch it pass, and certify a +verified diff that did not exist. A call that was denied, or that the run never got back to, +has no result at all and counts as no change. + +### The test plan is taken before the model runs + +The command is inferred from the pristine clone's `package.json` (`test:ci` → `test` → +`test:unit`, npm's placeholder script rejected) **before** the first hand-off, and every +attempt is graded against that snapshot — so a model cannot rewrite the command that grades +it. Install lifecycle scripts are compared before `npm ci` executes them, and a manifest the +model moved installs with `--ignore-scripts`. + ## Known gaps +- **Nothing is wired into `runCycle`.** `implementTicket` has no caller: `src/cycle.ts` still + claims a ticket and hands it straight back. The slice is a library with tests, not a + behaviour the deployed worker has. Wiring it needs the clone step (MAPCO-11433) to produce + a working directory and `ReleasePort` bound to `handBackTicket`. +- **`DescriptionPort` has no implementation.** The poll does not fetch a description and + `JiraTicket` carries none, so there is no way to hand the model the ticket's prose — every + ticket is refused before the first model turn. Closing it is `description` in `POLL_FIELDS`, + on `McpTicket`, in `toTicket` and on `JiraTicket`. - The worker knobs are read from the environment rather than `@map-colonies/config`, which needs a schema published in `@map-colonies/schemas`. Telemetry still goes through the library. Registering a real schema is follow-up work.