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/README.md b/README.md index 90a0041..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: `: `. @@ -78,6 +94,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` | `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 | Raise `MAX_TICKETS_PER_RUN` before ever raising `MAX_CONCURRENT_TICKETS`. @@ -85,6 +104,33 @@ 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. + +> **⚠️ `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 `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. +- **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. 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 Jira is the only state store — no database, no files that outlive a run — so there is no @@ -106,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. @@ -124,7 +220,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 815ecc3..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 }} @@ -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 "sdk" }} + - 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 }} @@ -91,7 +106,7 @@ spec: {{- end }} {{- if .Values.extraEnvVars }} {{- toYaml .Values.extraEnvVars | nindent 12 }} - {{- end }} + {{- end }} envFrom: - configMapRef: name: {{ include "developer-agent-bot.fullname" . }} @@ -110,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 fa8e035..3889930 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -4,11 +4,28 @@ global: metrics: {} mclabels: - component: worker + # 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, 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 @@ -68,6 +85,19 @@ worker: pollIntervalMs: 300000 maxTicketsPerRun: 1 maxConcurrentTickets: 1 + # 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. + # + # `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: logLevel: info 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..b413e7a --- /dev/null +++ b/src/agent/credential.ts @@ -0,0 +1,154 @@ +/** + * 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. + * + * ## ⚠️ `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. 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: + * + * - 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, 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 `sdk`. */ +const AUTH_MODE_ENV = 'MODEL_AUTH'; + +type ModelAuthMode = 'api-key' | 'sdk'; + +const AUTH_MODES: readonly ModelAuthMode[] = ['api-key', 'sdk']; + +/** + * 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, + sdk: 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..6dea51c 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 `sdk` 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..23105dd --- /dev/null +++ b/tests/unit/agent/credential.spec.ts @@ -0,0 +1,87 @@ +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 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', + }); + }); + + it('should take the subscription token when that mode is configured.', () => { + 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: '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({ 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.', () => { + // 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: '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.', () => { + // 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.', () => { + // 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({ 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(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.', () => { + 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..55216bd 100644 --- a/tests/unit/agent/implementer.spec.ts +++ b/tests/unit/agent/implementer.spec.ts @@ -2,14 +2,14 @@ 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'; 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,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 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 */ + 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 + ); + }); + + 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/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..a898900 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: 'sdk', 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(); });