diff --git a/.env.example b/.env.example new file mode 100644 index 0000000..d6cf781 --- /dev/null +++ b/.env.example @@ -0,0 +1,28 @@ +# Copy to `.env` and fill in. `.env` is gitignored — never commit a filled-in copy. +# +# cp .env.example .env +# +# Used by the "Dry run (sdk auth)" VS Code launch config and `npm run dry-run:local`. +# The deployed worker reads these from the pod's env instead; see helm/values.yaml. + +# --- Jira, through the self-hosted atlassian-write MCP server. Needs the corporate VPN. --- +MCP_ATLASSIAN_URL=https://atlassian-mcp-write.mapcolonies.net/sse +JIRA_BOT_ACCOUNT=developer-agent@mapcolonies.net +JIRA_BOT_DISPLAY_NAME=AGENT DEVELOPER + +# --- Which account pays for the model. `sdk` reads CLAUDE_CODE_OAUTH_TOKEN, --- +# --- `api-key` reads ANTHROPIC_API_KEY. The worker refuses the other mode's credential. --- +MODEL_AUTH=sdk +# From `claude setup-token`. Bills, and is rate-limited as, the person whose token it is. +CLAUDE_CODE_OAUTH_TOKEN= +# Only for MODEL_AUTH=api-key. Leave empty in sdk mode. +ANTHROPIC_API_KEY= + +# --- Repo lookups, the clone, the push and the pull request. `gh auth token` prints one. --- +GITHUB_TOKEN= + +# --- Spend ceilings. Deliberately far below the defaults for a laptop run: --- +# --- MAX_TICKETS_PER_DAY is counted per process, so each dry run gets its own allowance. --- +MAX_TOKENS_PER_TICKET=150000 +MAX_TURNS_PER_TICKET=15 +MAX_TICKETS_PER_DAY=1 diff --git a/.vscode/launch.json b/.vscode/launch.json index 85986a3..e113cfe 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -4,6 +4,46 @@ // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387 "version": "0.2.0", "configurations": [ + { + // One real cycle, stepped through. This is the whole pipeline: it claims a real + // `agent-ready` ticket, clones the repo its title names, hands it to the model, runs that + // repo's suite, and on a pass pushes an `agent/` branch and opens a real pull request. + // + // Needs the corporate VPN — the MCP server is not reachable from outside it — and a + // subscription token from `claude setup-token`, exported as CLAUDE_CODE_OAUTH_TOKEN in the + // shell VS Code was launched from. The ceilings below are deliberately far under the + // defaults: MAX_TICKETS_PER_DAY is counted per process, so this run gets its own allowance. + "type": "node", + "request": "launch", + "name": "Dry run (sdk auth)", + "runtimeExecutable": "npx", + "runtimeArgs": ["tsx", "--inspect-brk"], + "program": "${workspaceFolder}/src/dryRun.ts", + "cwd": "${workspaceFolder}", + "console": "integratedTerminal", + "sourceMaps": true, + "smartStep": true, + "skipFiles": ["${workspaceFolder}/node_modules/**/*.js", "/**/*.js"], + // Everything the run needs lives in `.env` (gitignored). Copy `.env.example` to + // `.env` and fill in the two tokens. Nothing has to be exported into the shell, so + // VS Code no longer has to be launched from a particular terminal. + "envFile": "${workspaceFolder}/.env" + }, + { + // The same seam with every edge faked — no Jira, no GitHub, no model, no money. The place + // to find out where the breakpoints want to go before spending a real ticket on it. + "type": "node", + "request": "launch", + "name": "Integration: one cycle (fakes)", + "runtimeExecutable": "npx", + "runtimeArgs": ["vitest", "run", "--project", "integration", "--no-file-parallelism", "--coverage.enabled=false"], + "cwd": "${workspaceFolder}", + "console": "integratedTerminal", + "autoAttachChildProcesses": true, + "sourceMaps": true, + "smartStep": true, + "skipFiles": ["/**/*.js"] + }, { "type": "node", "request": "launch", @@ -27,7 +67,6 @@ "TELEMETRY_METRICS_ENABLED": "false", "TELEMETRY_TRACING_URL": "http://localhost:55681/v1/trace", "TELEMETRY_METRICS_URL": "http://localhost:55681/v1/metrics" - // "OPENAPI_FILE_PATH": "./openapi3.yaml" } } ] diff --git a/Dockerfile b/Dockerfile index 88f0ba1..d487f11 100644 --- a/Dockerfile +++ b/Dockerfile @@ -12,7 +12,10 @@ RUN npm run build FROM node:24.10.0-alpine3.22 AS production -RUN apk add dumb-init +# `git` because the worker clones the repository a ticket names and then commits and pushes to +# it — that is worker code, not model tooling: the Agent SDK is given file and test tools only +# and has no shell to reach a binary with (see MODEL_TOOLS in src/agent/sdkOptions.ts). +RUN apk add --no-cache dumb-init git ENV NODE_ENV=production diff --git a/README.md b/README.md index 90a0041..cf7a1b8 100644 --- a/README.md +++ b/README.md @@ -4,9 +4,11 @@ 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: the wiring.** Until now `runCycle` claimed a ticket and handed it straight +back, while a fully tested implement-and-verify core sat next to it that nothing called. The +two are now connected, so labelling a ticket `agent-ready` runs the whole thing: resolve the +repository, clone it, hand the ticket to the model inside that clone, run the repository's own +tests, and on a passing run push an `agent/` branch and open a pull request. ## Shape @@ -26,6 +28,27 @@ Jira is the sole source of truth. There is no database and nothing on disk outli pipeline is built and tested through. The scheduler calls it; so do the tests. Later slices add cases here rather than standing up harnesses of their own. +### What one ticket goes through + +1. **Poll.** `agent-ready`, unassigned, not finished, not at the attempt cap. +2. **Claim**, optimistically, and only if the day still has room for a ticket. A run that has + hit `MAX_TICKETS_PER_DAY` writes nothing at all. +3. **Resolve the repository** from the title's `: ` prefix, and refuse if it names + none or names one that does not exist. +4. **Clone** it at the branch GitHub calls default, shallow, into a directory of its own. +5. **Implement**: read the ticket's description, hand it to the Agent SDK inside the clone, up + to three hand-offs, feeding each test failure back into the next. +6. **Verify** with the repository's own test command — read off the clone *before* the model + touched it, so the model cannot rewrite the thing that grades it. +7. **Publish**, and only on a passing run: branch, commit, push, open a normal pull request, + comment the link on the ticket. +8. **Clean up** the clone, on every path. + +Anything that stops before step 7 is a **hand-back**: a comment saying what happened, the +attempt counter bumped, and the ticket returned to Open. A ticket whose pull request *is* open +stays assigned to the bot — the work is done and a human is reviewing it, and releasing it +would put it back in the poll's way. + ### Jira access Through the org's self-hosted `atlassian-write` MCP server, in-cluster — the worker carries @@ -38,7 +61,7 @@ master" enforces nothing. ## Things that look wrong but aren't -Verified against the live Jira instance in MAPCO-11427, and each one bit a first draft: +Each one bit a first draft. - **Finished work is excluded by status *name*, not `statusCategory`.** `Resolved` reports category `In Progress` in this instance, so a category filter hands the worker @@ -52,6 +75,26 @@ Verified against the live Jira instance in MAPCO-11427, and each one bit a first `() AND project = MAPCO`, so top-level `OR` is safe. - **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. +- **A transition is found by its name's second half, not by a target status.** This instance + reports no target status at all — `jira_get_transitions` answers with `id` and `name` — + and names its transitions `Open to In-Progress`. So the lookup reads the target out of a + ` to ` name, and compares statuses with case, hyphens and whitespace flattened, + because `In-Progress` and `In Progress` are the same status. Target status is still tried + first and still wins where a server reports one: an entry that says it lands in `Blocked` + is not matched for `In Progress` however it is named. +- **The poll does not fetch descriptions; the per-ticket read does.** A description is long, + the poll asks for one more ticket than it will work, and prose for a ticket nobody touches + would be paid for on every tick. So `POLL_FIELDS` omits it and `ISSUE_FIELDS` asks for it — + and an *absent* description is therefore not the same fact as an empty one. +- **`MAX_TURNS_PER_TICKET` is charged in model turns, not in hand-offs.** A hand-off that took + twelve turns charges twelve. Charging one per hand-off would make a ceiling of forty mean + forty hand-offs of up to forty turns each. +- **Cache-read tokens count against `MAX_TOKENS_PER_TICKET`.** They are billed prompt tokens, + and an agentic loop re-reads its whole context every turn — leaving them out would put most + of what a ticket costs outside the ceiling that exists to bound it. +- **A ticket is handed back at most once, by a per-ticket latch.** Two slices each own a + give-up path (the implement loop and the budget guard) and both can fire on one ticket. The + second would otherwise comment again on a ticket already back in Open. ## Ticket titles @@ -67,6 +110,10 @@ repo in the org is a **refusal**, never a guess: the worker comments what it loo releases the ticket and bumps the attempt count. Most existing MAPCO tickets have no prefix, so refusal is the common path until the convention spreads. +A ticket also needs a **description**. The summary alone is not something to change code +against without guessing, so a ticket with no prose is refused before the first model turn +rather than after paying for a hand-off whose only honest answer is "there is not enough here". + ## Configuration | Variable | Default | Meaning | @@ -74,17 +121,58 @@ so refusal is the common path until the convention spreads. | `MCP_ATLASSIAN_URL` | *required* | Address of the `atlassian-write` MCP server. Transport is picked from the path: `/sse` gets SSE, anything else Streamable HTTP | | `JIRA_BOT_ACCOUNT` | *required* | Identifier written to a ticket's assignee field — an email or accountId | | `JIRA_BOT_DISPLAY_NAME` | *required* | What `JIRA_BOT_ACCOUNT` reads back as, surname-first. The claim re-read compares against this | +| `MODEL_AUTH` | `sdk` | Which account model calls are billed to: `sdk` reads `CLAUDE_CODE_OAUTH_TOKEN`, `api-key` reads `ANTHROPIC_API_KEY`. Stated, never inferred — the worker refuses a credential its mode did not ask for | +| `CLAUDE_CODE_OAUTH_TOKEN` | *required in `sdk` mode* | A Claude subscription token from `claude setup-token`. Bills, and is rate-limited as, the person whose token it is | +| `ANTHROPIC_API_KEY` | *required in `api-key` mode* | The organisation's Anthropic key, from a Secret. The worker refuses to start without the credential its mode asks for | +| `GITHUB_TOKEN` | *required* | Repo lookups, cloning, pushing and opening the pull request. Checked at boot, like the model key: a public repo can be *cloned* without one but cannot be pushed, so a tokenless worker would fail after paying for the model rather than before. A PAT locally; interim until MAPCO-11428 mints App installation tokens per run | | `POLL_INTERVAL_MS` | `300000` | How often a cycle runs | | `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 | +| `MAX_CONCURRENT_TICKETS` | `1` | Tickets in flight at once. Each gets its own clone | +| `MAX_TOKENS_PER_TICKET` | `200000` | Per-ticket token ceiling, prompt and completion including cache reads. Going over aborts the ticket and comments what it cost | +| `MAX_TURNS_PER_TICKET` | `40` | Per-ticket model-turn ceiling, and also the turn bound on one hand-off | +| `MAX_TICKETS_PER_DAY` | `5` | Tickets the process may start in a day. Counted in memory, so it is per process-day | +| `WORKSPACE_ROOT` | `os.tmpdir()` | Where per-ticket clones are made. An `emptyDir` in the pod | +| `AGENT_GIT_NAME` | `mapcolonies-developer-agent[bot]` | Commit author and committer name | +| `AGENT_GIT_EMAIL` | `…[bot]@users.noreply.github.com` | Commit author and committer email | +| `AGENT_MODEL` | *unset* | Overrides the model. Unset means the worker's own default | Raise `MAX_TICKETS_PER_RUN` before ever raising `MAX_CONCURRENT_TICKETS`. +None of the three spend ceilings has a value meaning "unlimited", on purpose: anything below +`1` is rejected, so a ceiling cannot be switched off with an env var. One that can is one that +gets switched off during an incident and stays off. + The two bot-identity variables look redundant and are not: Jira takes an *identifier* on 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` 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,40 +194,111 @@ 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. +A hand-back is that release **plus the attempt counter**, as one act. The counter lives in a +Jira label and is the only thing that ends a loop: a ticket handed back without it matches the +poll on the very next tick and is paid for again. So the counter is written *first*, and a +label write that fails stops the hand-back entirely — held-and-counted-nowhere is recoverable, +available-and-uncounted is a re-burn loop. + +## What the model can and cannot do + +The model gets `Read`, `Glob`, `Grep`, `Edit`, `Write`, `NotebookEdit` and `TodoWrite`, inside +the clone, and nothing else. No shell, no git, no network, no subagents, no MCP servers, and no +settings or skills files — not even the clone's own, because the clone is a repository off the +internet and its `.claude/settings.json` would otherwise widen what may be done to it. + +Everything that touches the outside world is worker code: the branch name and commit title are +computed from the Jira issue, the push refuses any ref outside `agent/`, and there is no method +anywhere that merges, approves or force-pushes. The tests are run *by the worker*, against a +command read off the clone before the model started — so the model cannot report a pass it did +not get, skip the run, or make `"test": "echo ok"` the cheapest route to a green one. + ## Known gaps - 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. +- **The GitHub credential is a static token** (`GITHUB_TOKEN`) and is required rather than + optional, not the per-run App installation token MAPCO-11428 specifies and `TokenProvider` is + shaped for. `src/vcs/envToken.ts` + is the interim binding and exists to be deleted; everything around it already treats the + credential as short-lived — minted per call, never held, passed to git through the environment + rather than argv, and redacted out of error messages. +- **The commit does not know which files the model wrote.** `AgentRun` reports *whether* the + tree changed, not what changed, so the publish path commits every path `git status` reports + and says so in the pull-request body and in the log. That is bounded — `status --porcelain` + never reports an ignored file — but a repository that does not gitignore its build output can + get a noisier diff than a human would have committed. Forwarding the paths the SDK already + carries is what removes the branch. +- **Only Node repositories can be verified.** `NpmTestRunner` infers `test:ci`, `test` or + `test:unit` from `package.json`; MAPCO-11433 also asks for Python (`pytest`). `TestRunner` is + a port, so that is an implementation rather than a reshape, but it does not exist yet and a + Python ticket is refused as `not-verifiable`. - **The Jira identity is configured, not discovered.** The MCP server runs under a shared service account with no per-user attribution, so the worker cannot ask Jira who it is — hence `JIRA_BOT_ACCOUNT` / `JIRA_BOT_DISPLAY_NAME`. The claim re-read can therefore tell the bot apart from a *human*, but not from a second worker configured with the same account. A dedicated Jira account per deployment is still the recommendation, and it is what makes boot-time orphan release (MAPCO-11432) safe. -- **The real transition vocabulary is unverified.** `jira_get_transitions` and - `expand=transitions` are both rejected by the write-pilot MCP server, so the MAPCO - workflow's actual transition names and target statuses could not be read the way the poll - query was verified in MAPCO-11427. The lookup matches on target status with the - 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. +- **There is no boot-time orphan sweep yet** (MAPCO-11432). Several paths deliberately keep + hold of a ticket they cannot return to Open, on the argument that the sweep recovers it — + until it exists, such a ticket stays assigned to the bot. `runCycle` therefore hands a held + ticket back on an unexpected failure rather than dropping it, but a hand-back that itself + fails leaves the ticket held. +- **The *release* half of the transition vocabulary is still unverified.** The claim half is + now known: `jira_get_transitions` works against the write pilot after all, and on an `Open` + ticket it answers with `id` and `name` only — **no target status on any entry** — offering + `Canceled`, `Blocked`, `Open to In-Progress`, `Open to Reject`, `Open to Verified`. The + lookup reads the target out of that ` to ` shape, which is what makes a claim + work. What nobody has seen yet is the set offered on an `In Progress` ticket, which is what + a hand-back needs to get back to `Open` — so the first hand-back is still the thing that + reports it, through the `offered` names on a `no-transition` refusal. The consequence of a + miss there is worse than a refused claim: `releaseTicket` deliberately keeps hold of a + ticket it cannot return to Open, and the orphan sweep that would recover it does not exist + yet, so such a ticket stays assigned to the bot. +- **The Agent SDK on Alpine/musl is unverified.** The production image is + `node:24-alpine`, and the SDK ships vendored binaries (ripgrep, which backs `Grep`) that are + usually glibc-linked. A `Grep` that fails is a degraded session rather than a broken one — + the model still has `Glob` and `Read` — but it is worth checking in the first pod logs. +- **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.anthropicSecretName=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 One cycle against the real MCP server, from a laptop. Requires the corporate VPN — the server is not reachable from outside it. -**This writes to real tickets.** It claims the oldest `agent-ready` ticket and hands it -straight back, leaving a comment behind. That is the point: label a ticket `agent-ready` and -watch it get claimed and returned. +**This is the whole pipeline, and it costs money.** It claims the oldest `agent-ready` ticket, +clones the repository its title names, hands the ticket to the model, runs that repository's +tests, and on a passing run pushes an `agent/` branch and opens a real pull request. It writes +to real tickets and bills real tokens. The spend ceilings apply, and because the daily counter +is per process a dry run gets its own allowance — so set them low for a first run. ```sh MCP_ATLASSIAN_URL="https://atlassian-mcp-write.mapcolonies.net/sse" \ JIRA_BOT_ACCOUNT="developer-agent@mapcolonies.net" \ JIRA_BOT_DISPLAY_NAME="AGENT DEVELOPER" \ - GITHUB_TOKEN="$(gh auth token)" npm run dry-run + MODEL_AUTH=sdk CLAUDE_CODE_OAUTH_TOKEN="$CLAUDE_CODE_OAUTH_TOKEN" \ + GITHUB_TOKEN="$(gh auth token)" \ + MAX_TOKENS_PER_TICKET=50000 MAX_TURNS_PER_TICKET=15 MAX_TICKETS_PER_DAY=1 \ + npm run dry-run ``` It runs the same `runCycle` seam the deployed worker runs, so what it proves is about the @@ -154,3 +313,7 @@ npm test npm run lint npm run build ``` + +`git` must be on `PATH`: two specs drive the real binary against a real bare repository, because +what they assert — that a push to `master` fails, that hooks cannot kill a commit, that +`--branch` checks out the branch GitHub named — is only true of real git. diff --git a/helm/templates/deployment.yaml b/helm/templates/deployment.yaml index 815ecc3..f81a0cf 100644 --- a/helm/templates/deployment.yaml +++ b/helm/templates/deployment.yaml @@ -50,14 +50,18 @@ 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 }} volumeMounts: + # The per-ticket clones. Writable because the worker clones a repository and runs its + # own `npm ci` and test suite in here; nothing in it outlives a ticket. + - mountPath: {{ .Values.worker.workspaceRoot | quote }} + name: workspace {{- if .Values.caSecretName }} - mountPath: {{ printf "%s/%s" .Values.caPath .Values.caKey | quote }} name: root-ca @@ -83,6 +87,46 @@ spec: value: {{ .Values.worker.maxTicketsPerRun | quote }} - name: MAX_CONCURRENT_TICKETS value: {{ .Values.worker.maxConcurrentTickets | quote }} + - name: WORKSPACE_ROOT + value: {{ .Values.worker.workspaceRoot | quote }} + - name: MAX_TOKENS_PER_TICKET + value: {{ .Values.worker.maxTokensPerTicket | quote }} + - name: MAX_TURNS_PER_TICKET + value: {{ .Values.worker.maxTurnsPerTicket | quote }} + - name: MAX_TICKETS_PER_DAY + value: {{ .Values.worker.maxTicketsPerDay | quote }} + - name: AGENT_GIT_NAME + value: {{ .Values.worker.gitName | quote }} + - name: AGENT_GIT_EMAIL + value: {{ .Values.worker.gitEmail | quote }} + {{- if .Values.worker.model }} + - name: AGENT_MODEL + value: {{ .Values.worker.model | quote }} + {{- end }} + # Which account the model calls are billed to, stated rather than inferred. The + # worker refuses to start on a credential its mode did not ask for, so this value + # and the Secret's key below have to agree. + - name: MODEL_AUTH + value: {{ .Values.worker.modelAuth | quote }} + {{- if .Values.worker.anthropicSecretName }} + # The one credential the worker talks to the model with. Required: it refuses to + # start without it rather than claiming a ticket and finding out — see + # readModelCredential. The variable name is the mode's, never both at once. + - name: {{ if eq .Values.worker.modelAuth "sdk" }}CLAUDE_CODE_OAUTH_TOKEN{{ else }}ANTHROPIC_API_KEY{{ end }} + valueFrom: + secretKeyRef: + name: {{ .Values.worker.anthropicSecretName | quote }} + key: {{ .Values.worker.anthropicSecretKey | quote }} + {{- end }} + {{- if .Values.worker.githubSecretName }} + # Repo lookups, the clone, the push and the pull request. MAPCO-11428 replaces this + # with an App installation token minted per run. + - name: GITHUB_TOKEN + valueFrom: + secretKeyRef: + name: {{ .Values.worker.githubSecretName | quote }} + key: {{ .Values.worker.githubSecretKey | quote }} + {{- end }} {{- if .Values.caSecretName }} - name: REQUESTS_CA_BUNDLE value: {{ printf "%s/%s" .Values.caPath .Values.caKey | quote }} @@ -91,7 +135,7 @@ spec: {{- end }} {{- if .Values.extraEnvVars }} {{- toYaml .Values.extraEnvVars | nindent 12 }} - {{- end }} + {{- end }} envFrom: - configMapRef: name: {{ include "developer-agent-bot.fullname" . }} @@ -103,6 +147,9 @@ spec: {{ tpl (toYaml .Values.sidecars) . | nindent 8 }} {{- end }} volumes: + - name: workspace + emptyDir: + sizeLimit: {{ .Values.worker.workspaceSizeLimit | quote }} {{- if .Values.caSecretName }} - name: root-ca secret: @@ -110,5 +157,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..19d3d43 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: @@ -69,6 +72,55 @@ worker: maxTicketsPerRun: 1 maxConcurrentTickets: 1 + # Where the per-ticket clones are made. A writable emptyDir, because the worker now clones a + # repository and runs its `npm ci` and its suite, and nothing on disk outlives a ticket. + workspaceRoot: '/workspace' + # Room for one clone plus its node_modules, times maxConcurrentTickets, with headroom. A + # MapColonies service's node_modules is comfortably over 1Gi. + workspaceSizeLimit: 8Gi + + # Spend ceilings (MAPCO-11435). Conservative on purpose: a metered API key has no ceiling of + # its own, so these are the only thing between a stuck agent and an invoice. There is + # deliberately no value meaning "unlimited" — a ceiling that can be switched off gets switched + # off during an incident and stays off. + maxTokensPerTicket: 200000 + maxTurnsPerTicket: 40 + maxTicketsPerDay: 5 + + # Who the worker's commits are authored by. The real answer belongs to the GitHub App + # (MAPCO-11428): `{app-slug}[bot]` and `{app-id}+{app-slug}[bot]@users.noreply.github.com`. + # Until then this is a valid noreply address that is recognisably not a person's. + gitName: 'mapcolonies-developer-agent[bot]' + gitEmail: 'mapcolonies-developer-agent[bot]@users.noreply.github.com' + + # Model override. Empty means the worker's own default (agent/sdkOptions.ts). + model: '' + + # Which account the model calls are billed to. `sdk` reads CLAUDE_CODE_OAUTH_TOKEN — a Claude + # subscription token from `claude setup-token`; `api-key` reads ANTHROPIC_API_KEY instead. + # Explicit configuration, never inference: the two look alike to the SDK and bill completely + # differently, and a worker given the other mode's credential refuses to start rather than + # quietly billing an account nobody chose. + # + # `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 crash-loops when the token expires. + modelAuth: 'sdk' + + # The model credential. **Required** — the worker refuses to start without it, which is the + # loudest thing a missing Secret can be. The key must hold the credential the mode above asks + # for: a subscription token for `sdk`, an API key for `api-key`. + anthropicSecretName: '' + anthropicSecretKey: 'oauthToken' + + # The GitHub credential, used to look up repos, clone private ones, push and open the pull + # request. Interim: MAPCO-11428 replaces this with App installation tokens minted per run, so + # that there is no long-lived secret here at all. + githubSecretName: '' + githubSecretKey: 'token' + env: logLevel: info logPrettyPrintEnabled: false @@ -79,13 +131,16 @@ env: enabled: false url: http://localhost:55681/v1/metrics +# Sized for what the worker actually does now: clone a repository, run its `npm ci` and then its +# test suite, next to an Agent SDK session. The previous 100m/128Mi was right for a pod that only +# polled Jira and would OOM-kill on the first `npm ci`. resources: enabled: true value: limits: - cpu: 100m - memory: 128Mi + cpu: '2' + memory: 4Gi requests: - cpu: 100m - memory: 128Mi + cpu: 500m + memory: 1Gi diff --git a/package.json b/package.json index 2305a49..feb28f4 100644 --- a/package.json +++ b/package.json @@ -22,7 +22,9 @@ "assets:copy": "copyfiles -f ./config/* ./dist/config && copyfiles ./package.json dist", "clean": "rimraf dist", "prepare": "node .husky/install.mjs", - "dry-run": "tsx src/dryRun.ts" + "dry-run": "tsx src/dryRun.ts", + "dry-run:local": "tsx --env-file=.env src/dryRun.ts", + "preflight": "tsx --env-file=.env src/preflight.ts" }, "directories": { "test": "tests" 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/implement.ts b/src/agent/implement.ts index 75adac4..22e8d3c 100644 --- a/src/agent/implement.ts +++ b/src/agent/implement.ts @@ -143,7 +143,7 @@ async function runAgent(request: AgentRunRequest, deps: ImplementDeps): Promise< } catch (error) { deps.logger.error({ msg: 'agent run failed', key: request.task.key, err: error }); - return { outcome: 'gave-up', usage: NO_USAGE, summary: error instanceof Error ? error.message : String(error), deniedTools: [] }; + return { outcome: 'gave-up', usage: NO_USAGE, turns: 0, summary: error instanceof Error ? error.message : String(error), deniedTools: [] }; } } diff --git a/src/agent/implementer.ts b/src/agent/implementer.ts index ef3958a..4fbb960 100644 --- a/src/agent/implementer.ts +++ b/src/agent/implementer.ts @@ -3,7 +3,7 @@ import { NpmTestRunner } from '../workspace/npmTestRunner'; import { spawnRunner } from '../workspace/subprocess'; import { DEFAULT_AGENT_LIMITS, type ImplementDeps } from './implement'; import { createSdkAgent } from './sdkAgent'; -import type { AgentLimits, DescriptionPort, ReleasePort } from './types'; +import type { AgentLimits, AgentPort, DescriptionPort, ReleasePort } from './types'; /** * The whole of this slice, assembled once. @@ -11,48 +11,77 @@ import type { AgentLimits, DescriptionPort, ReleasePort } from './types'; * `implementTicket` takes its collaborators as values, which is what makes it testable and also * what makes it four constructions to wire up. This is those four in one place: the SDK agent * with the key from the pod's Secret, the npm runner over a real subprocess, the turn and - * attempt bounds, and the two ports the caller has to supply because this slice cannot — - * handing a ticket back (MAPCO-11431) and reading a ticket's description. + * attempt bounds, and the description read the caller has to supply because this slice cannot. * * 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. + * + * It is two calls rather than one because its collaborators have two different lifetimes, and + * collapsing them was a real defect: the agent must be built **once, at boot**, because that is + * where the model credential is read and a worker with no credential must fail to come up rather + * than claim a ticket and discover it — while the hand-back belongs to **one ticket**, because + * the guarantee that a ticket is handed back at most once is a per-ticket latch + * (`createHandBackOnce`) and a shared one would refuse the next legitimate attempt on the same + * ticket. So the expensive, credential-reading half happens here, and the ticket-scoped half + * arrives per ticket. */ interface ImplementerOptions { readonly logger: Logger; - /** Comment, release, and count the attempt. See `ReleasePort` — the third step is owed. */ - readonly release: ReleasePort; /** Where the ticket's prose comes from. See `DescriptionPort`. */ 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; } +/** What only the ticket in hand can supply. */ +interface TicketScope { + /** Comment, release, and count the attempt. See `ReleasePort` — the third step is owed. */ + readonly release: ReleasePort; + /** + * Decorates the boot-time agent for this one ticket. + * + * A function rather than a budget object so this module stays ignorant of the budget slice: the + * wiring passes `meterAgent` (src/agent/meteredAgent.ts), which charges every hand-off to the + * ticket's ledger and stops when it runs out. Omitted, the agent is handed over unmetered, + * which is only ever right in a test. + */ + readonly meter?: (agent: AgentPort) => AgentPort; +} + +/** Everything `implementTicket` needs for one ticket, from a worker already built. */ +type Implementer = (scope: TicketScope) => ImplementDeps; + /** - * Everything `implementTicket` needs, built from the environment the pod was given. + * Build the worker's implement step 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; +function createImplementer(options: ImplementerOptions): Implementer { + const { logger, description, limits = DEFAULT_AGENT_LIMITS, env = process.env, model } = options; + + const agent = createSdkAgent(env, model); + const tests = new NpmTestRunner(spawnRunner({ env })); - return { - agent: createSdkAgent(env, model), - tests: new NpmTestRunner(spawnRunner({ env })), + return (scope: TicketScope): ImplementDeps => ({ + agent: scope.meter === undefined ? agent : scope.meter(agent), + tests, description, - release, + release: scope.release, logger, limits, - }; + }); } export { createImplementer }; -export type { ImplementerOptions }; +export type { Implementer, ImplementerOptions, TicketScope }; diff --git a/src/agent/meteredAgent.ts b/src/agent/meteredAgent.ts new file mode 100644 index 0000000..a97d5f7 --- /dev/null +++ b/src/agent/meteredAgent.ts @@ -0,0 +1,144 @@ +import type { Logger } from '@map-colonies/js-logger'; +import type { TicketGuard } from '../budget/guard'; +import type { AttemptSummary, Spend } from '../budget/types'; +import type { HandBackOnce } from '../tickets/handBackOnce'; +import type { AgentOutcome, AgentPort, AgentRun, AgentRunRequest } from './types'; + +/** How much of a hand-off's summary goes into the attempt list on an aborted ticket's comment. */ +const STEP_SUMMARY_LIMIT = 120; + +/** + * What one hand-off cost, in the units the ledger charges. + * + * `cacheRead` is **in** the token count, and that is the one judgement here worth arguing. The + * SDK reports a prompt in two parts — the tokens it had to send and the tokens it read from + * cache — and `Spend.tokens` is specified as "prompt and completion together, the invoice's + * unit". A cache read is prompt: it is billed, at a discount, and an agentic loop re-reads its + * whole context every turn, so a forty-turn run on a 100k context can read millions of them. + * Leaving them out would mean most of what a ticket actually costs never reached the ceiling + * that exists to bound it, and `MAX_TOKENS_PER_TICKET` would be a number about the small half + * of the bill. + * + * `turns` comes off the run rather than being counted as one per hand-off, which is the mistake + * `TicketLedger.charge` warns about by name: a hand-off that took twelve turns charges twelve, + * because charging `1` would turn a ceiling of forty turns into forty hand-offs of forty turns. + */ +function spendOf(run: AgentRun): Spend { + return { tokens: run.usage.input + run.usage.output + run.usage.cacheRead, turns: run.turns }; +} + +/** One line for the attempt list on an aborted ticket's comment. Says what the hand-off did. */ +function describeStep(attempt: number, run: AgentRun): string { + if (run.outcome === 'changed') { + return `hand-off ${attempt}: changed files in the clone`; + } + + if (run.outcome === 'no-change') { + return `hand-off ${attempt}: read the ticket and changed nothing`; + } + + return `hand-off ${attempt}: the run did not finish (${run.summary.slice(0, STEP_SUMMARY_LIMIT)})`; +} + +interface MeteredAgentDeps { + /** The real agent. Built once at boot, because that is where the model credential is read. */ + readonly agent: AgentPort; + /** This ticket's meter, obtainable only from a `cycle.start` that claimed it. */ + readonly meter: TicketGuard; + /** + * This ticket's hand-back, told what the abort achieved. + * + * The seam that stops one ticket being handed back twice. `chargeSpend` performs the overspend + * hand-back itself, through the `AbortPort` the guard was built with; `implementTicket` then + * reports the stopped run as a give-up and hands the ticket back through `ReleasePort`. Passing + * the abort's result to `record` is what makes the second one a no-op — see `HandBackOnce`. + */ + readonly handBack: Pick; + readonly logger: Logger; +} + +/** + * An `AgentPort` that charges every hand-off to the ticket's budget and stops when it runs out + * (MAPCO-11435). + * + * Wrapping the agent is what makes the per-ticket ceiling bite *during* a ticket. The obvious + * alternative — charge once after `implementTicket` returns — cannot: by then the ticket has + * either been handed back already or has a verified change waiting to be published, so the + * ceiling could only ever be enforced in hindsight, and one ticket could spend ten times it + * before anything noticed. Charging per hand-off puts the check at the same granularity + * `implementTicket` already works at, and each hand-off is itself bounded by `maxTurns`. + * + * The hand-back on overspend is not performed here. `chargeSpend` calls `AbortPort.abort` on the + * charge that first runs the budget out, and that port is bound to `handBackTicket` — so the + * comment, the attempt count and the release have already happened by the time the charge + * returns. What this does with that fact is three things: tell the ticket's latch through + * `record`, so the give-up that follows writes nothing; refuse to spend anything more; and + * report the run as over, which takes `implementTicket` to its single give-up exit. One ticket, + * one comment, one attempt counted. + * + * A run that *throws* is left to throw: `runAgent` in implement.ts contains it, and a transport + * failure with no usage attached has nothing to charge. + */ +function meterAgent(deps: MeteredAgentDeps): AgentPort { + const { agent, meter, handBack, logger } = deps; + const tried: string[] = []; + let changedAnything = false; + let stopped: string | null = null; + + const attemptSummary = (): AttemptSummary => ({ + tried: [...tried], + reached: changedAnything ? 'a change in the clone, not yet verified' : null, + }); + + const over = (summary: string): AgentRun => ({ + outcome: 'gave-up' satisfies AgentOutcome, + usage: { input: 0, output: 0, cacheRead: 0, costUsd: 0 }, + turns: 0, + summary, + deniedTools: [], + }); + + return { + run: async (request: AgentRunRequest): Promise => { + if (stopped !== null) { + // Not reached by `implementTicket`, which breaks out of its loop on a give-up — but a + // port that would happily spend again after being told to stop is not one to rely on. + logger.warn({ msg: 'refusing a hand-off after the ticket ran out of budget', key: request.task.key }); + + return over(stopped); + } + + const run = await agent.run(request); + + tried.push(describeStep(tried.length + 1, run)); + changedAnything = changedAnything || run.outcome === 'changed'; + + const charged = await meter.charge(spendOf(run), attemptSummary()); + + if (charged.ok) { + return run; + } + + if (!charged.alreadyStopped) { + // What `chargeSpend` just did to the ticket, handed to the latch so the give-up that + // follows does not do it again. `alreadyStopped` means an earlier charge did the writing, + // and the latch was told then. + handBack.record({ released: charged.released, attemptCounted: charged.attemptCounted }); + } + + const { kind, limit, spend } = charged.overspend; + stopped = `the per-ticket budget ran out: ${spend.turns} turns and ${spend.tokens} tokens against a ceiling of ${limit} ${kind}`; + + // The ticket is already commented, counted and released by `AbortPort.abort` — this line + // is what ties the spend to the hand-off that spent it, next to what that hand-off did. + logger.warn({ msg: 'stopping the ticket for budget', key: request.task.key, kind, limit, outcome: run.outcome, ...spend }); + + // Reported as over even when this hand-off changed files: the change is unverified — the + // suite has not run — and publishing it would spend past the ceiling to do it. + return over(stopped); + }, + }; +} + +export { meterAgent, spendOf }; +export type { MeteredAgentDeps }; diff --git a/src/agent/sdkAgent.ts b/src/agent/sdkAgent.ts index 97b6c91..33dfedf 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,18 +59,20 @@ 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. - * That wiring is not part of this slice — the entry points also need the clone from MAPCO-11433 - * and the release path from MAPCO-11431 before there is anything to hand this. + * Composed once at boot by `createImplementer`, which `createWorker` (src/worker.ts) calls + * alongside `new McpJira(...)` — plain construction, no container, in the same style as every + * other collaborator in the worker path. Once per process and not once per ticket, because this + * is where the credential is read: a worker with no key must fail to come up rather than claim a + * ticket and discover it. */ 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..93d576f 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. */ @@ -275,6 +278,18 @@ function usageOf(result: Record): TokenUsage { ); } +/** + * How many model turns the run took, off the SDK's own count. + * + * `num_turns` rather than counting assistant messages: the SDK emits its own tally on the + * result, a turn is not one message, and a count derived from the stream would drift the day a + * new message type arrives. Absent or non-numeric reads as zero, which is the safe direction — + * an over-count would charge a ticket for turns nobody took. + */ +function turnsOf(result: Record): number { + return readNumber(result, 'num_turns'); +} + function deniedIn(result: Record): string[] { const denials = result['permission_denials']; @@ -334,6 +349,7 @@ function foldMessages(messages: readonly unknown[]): AgentRun { return { outcome: 'gave-up', usage: NO_USAGE, + turns: 0, summary: 'The model produced no result: the run ended before the turn completed.', deniedTools: [], }; @@ -344,6 +360,7 @@ function foldMessages(messages: readonly unknown[]): AgentRun { return { outcome: outcomeOf(failed, wrote), usage: usageOf(result), + turns: turnsOf(result), summary: summaryOf(result, failed), deniedTools: deniedIn(result), }; diff --git a/src/agent/types.ts b/src/agent/types.ts index a8bbf29..30efddb 100644 --- a/src/agent/types.ts +++ b/src/agent/types.ts @@ -54,6 +54,17 @@ interface AgentRunRequest { interface AgentRun { readonly outcome: AgentOutcome; readonly usage: TokenUsage; + /** + * Model turns this run took — one request and its response each. + * + * Reported separately from `TokenUsage` because it is not a bill, it is the other half of the + * per-ticket ceiling: `MAX_TURNS_PER_TICKET` is counted in turns, and the only thing that + * knows how many a hand-off took is the run itself. A caller that charged `1` per hand-off + * instead would turn a ceiling of forty turns into forty hand-offs of forty turns apiece — + * see `TicketLedger.charge` (src/budget/types.ts), which is why this is a number and not a + * boolean anybody has to guess at. + */ + readonly turns: number; /** What the model said it did, or why it stopped. Goes in the log and the give-up comment. */ readonly summary: string; /** @@ -89,8 +100,9 @@ interface AgentLimits { * A claimed ticket and the clone to work it in — everything the implement step needs and * nothing about how either was obtained. * - * The ticket arrives already claimed (MAPCO-11431) and the clone already made and classified - * (MAPCO-11433). Neither is this module's business, which is why they come in as values. + * The ticket arrives already claimed (MAPCO-11431) and the clone already made (MAPCO-11433): + * `handleTicket` (src/cycle.ts) claims it, `DeliveryPort.deliver` (src/deliver.ts) clones and + * then calls this. Neither is this module's business, which is why they come in as values. */ interface Assignment { readonly ticket: JiraTicket; @@ -100,21 +112,22 @@ interface Assignment { /** * The ticket's prose, read one claimed ticket at a time. * - * A port rather than a field on `Assignment` because there is nothing to pass yet and a field - * would have hidden that: `JiraTicket` (src/jira/types.ts) carries no description, the poll's - * `fields` list does not ask for one, and `JiraPort` has no call that returns one — so the only - * value a caller could have supplied was `''`, on every ticket, for ever. As an interface it is - * a thing someone has to implement instead of a default someone can accept by accident. + * A port rather than a field on `Assignment`, and that shape earned itself. When this slice + * landed there was nothing to pass: `JiraTicket` carried no description, the poll's `fields` list + * did not ask for one, and `JiraPort` had no call that returned one — so the only value a caller + * could have supplied was `''`, on every ticket, for ever. As an interface it was a thing + * somebody had to implement rather than a default they could accept by accident, and until + * somebody did, every `agent-ready` ticket was claimed, refused as `no-description` and handed + * back. `implementTicket` answers an empty description **before the first model turn** rather + * than paying for a hand-off whose answer it already knows. * - * Implementing it is three lines in files this slice does not own (MAPCO-11431's): a - * `description` field on `JiraTicket`, `description` in `POLL_FIELDS`, and one mapping line in - * `toTicket` (src/jira/mcpJira.ts) — the MCP server's `jira_get_issue` already returns it. Until - * then the honest implementation is one that returns `''`, and `implementTicket` answers that by - * handing the ticket back **before** the first model turn rather than paying for a hand-off it - * knows the answer to. + * It is implemented now, by `createDescriptionReader` (src/jira/description.ts), over + * `JiraPort.getIssue` and its own field list (`ISSUE_FIELDS` in src/jira/mcpJira.ts). * * Read per ticket rather than at poll time on purpose: descriptions are long, the poll asks for - * one more ticket than it will work, and prose the worker never uses is not worth carrying. + * one more ticket than it will work, and prose the worker never uses is not worth carrying — + * which is why `POLL_FIELDS` still does not ask for one, and why an *absent* description on a + * `JiraTicket` is a different fact from an empty one. */ interface DescriptionPort { read: (ticket: JiraTicket) => Promise; diff --git a/src/common/workerConfig.ts b/src/common/workerConfig.ts index b184707..101e6c8 100644 --- a/src/common/workerConfig.ts +++ b/src/common/workerConfig.ts @@ -8,8 +8,10 @@ * these knobs is follow-up work, not a blocker for the current slice. */ +import { tmpdir } from 'node:os'; import type { BudgetConfig } from '../budget/types'; import type { BotIdentity } from '../tickets/claim'; +import type { GitIdentity } from '../vcs/types'; interface WorkerConfig { /** How often the internal scheduler runs a cycle. */ @@ -35,6 +37,26 @@ interface WorkerConfig { * conservative defaults below, and never "no ceiling". */ readonly budget?: BudgetConfig; + /** + * Where the per-ticket clones are made. + * + * A mounted volume in the pod, `os.tmpdir()` on a laptop. A container's root filesystem is + * read-only under the org's OpenShift policy and a clone plus its `node_modules` is not small, + * so this is a `emptyDir` in the chart rather than wherever the process happens to start. + */ + readonly workspaceRoot: string; + /** + * Who the worker's commits are authored by. + * + * Configured rather than derived, because the real answer belongs to the GitHub App + * (MAPCO-11428): once it exists this is `{app-slug}[bot]` and + * `{app-id}+{app-slug}[bot]@users.noreply.github.com`, and the app id is not something this + * worker can know. The defaults are a valid `users.noreply.github.com` address in the + * meantime, so a commit is attributable and GitHub accepts it. + */ + readonly commitIdentity: GitIdentity; + /** Overrides the model the agent runs. Unset means `DEFAULT_MODEL` in agent/sdkOptions.ts. */ + readonly model?: string; } const DEFAULT_POLL_INTERVAL_MS = 300_000; @@ -59,6 +81,19 @@ const DEFAULT_BUDGET: BudgetConfig = { maxTicketsPerDay: DEFAULT_MAX_TICKETS_PER_DAY, }; +/** + * Who a commit is authored by until the GitHub App exists. + * + * A `users.noreply.github.com` address is the form GitHub accepts without asking whether the + * mailbox is real, and the `[bot]` suffix is the convention that makes a machine author + * recognisable in `git log` and in a review. It is deliberately not a person's address: an + * agent-authored commit must never look like somebody's work. + */ +const DEFAULT_COMMIT_IDENTITY: GitIdentity = { + name: 'mapcolonies-developer-agent[bot]', + email: 'mapcolonies-developer-agent[bot]@users.noreply.github.com', +}; + class ConfigError extends Error { public constructor(message: string) { super(message); @@ -80,6 +115,13 @@ function readInt(env: NodeJS.ProcessEnv, name: string, fallback: number): number return parsed; } +/** An optional string, with absent and empty treated the same — a helm value is often `""`. */ +function readText(env: NodeJS.ProcessEnv, name: string, fallback: string): string { + const raw = env[name]?.trim() ?? ''; + + return raw === '' ? fallback : raw; +} + function readRequired(env: NodeJS.ProcessEnv, name: string, why: string): string { const raw = env[name]; if (raw === undefined || raw === '') { @@ -119,6 +161,8 @@ function loadWorkerConfig(env: NodeJS.ProcessEnv = process.env): WorkerConfig { maxTicketsPerDay: readInt(env, 'MAX_TICKETS_PER_DAY', DEFAULT_BUDGET.maxTicketsPerDay), }; + const model = env['AGENT_MODEL']?.trim() ?? ''; + return { pollIntervalMs: readInt(env, 'POLL_INTERVAL_MS', DEFAULT_POLL_INTERVAL_MS), maxTicketsPerRun: readInt(env, 'MAX_TICKETS_PER_RUN', 1), @@ -126,8 +170,16 @@ function loadWorkerConfig(env: NodeJS.ProcessEnv = process.env): WorkerConfig { mcpUrl, bot, budget, + workspaceRoot: readText(env, 'WORKSPACE_ROOT', tmpdir()), + commitIdentity: { + name: readText(env, 'AGENT_GIT_NAME', DEFAULT_COMMIT_IDENTITY.name), + email: readText(env, 'AGENT_GIT_EMAIL', DEFAULT_COMMIT_IDENTITY.email), + }, + // Absent rather than empty, so `sdkOptions` falls back to its own default instead of asking + // the SDK for a model called "". + ...(model === '' ? {} : { model }), }; } -export { budgetOf, ConfigError, DEFAULT_BUDGET, loadWorkerConfig }; +export { budgetOf, ConfigError, DEFAULT_BUDGET, DEFAULT_COMMIT_IDENTITY, loadWorkerConfig }; export type { WorkerConfig }; diff --git a/src/cycle.ts b/src/cycle.ts index 21d2fa9..57b3784 100644 --- a/src/cycle.ts +++ b/src/cycle.ts @@ -1,30 +1,41 @@ import type { Logger } from '@map-colonies/js-logger'; import type { WorkerConfig } from '@common/workerConfig'; +import type { BudgetGuard, CycleGuard } from './budget/guard'; +import type { DeliveryPort } from './deliver'; import { buildPollQuery } from './jira/query'; import type { JiraPort, JiraTicket } from './jira/types'; -import { claimTicket, releaseTicket } from './tickets/claim'; +import { claimTicket } from './tickets/claim'; +import type { HandBackOnce } from './tickets/handBackOnce'; /** The attempt cap from MAPCO-11432. Enforced in the query so a capped ticket is never even seen. */ const ATTEMPT_CAP = 2; -/** - * What the worker says on a ticket it hands straight back. - * - * Written to be read by whoever finds the ticket back in Open and wonders what touched it. - * It names what was tried, which in this slice is nothing at all. - */ -const HANDED_BACK_NOTE = [ - 'Picked this up automatically and handed it straight back.', - '', - 'This build of the developer agent can claim a ticket and release it, but there is nothing in between yet (MAPCO-11431) — no branch, no changes, no pull request. Nothing was modified, and this does not count as an attempt.', - '', - 'The ticket is available again for whoever wants it next.', -].join('\n'); - interface CycleDeps { readonly jira: JiraPort; readonly logger: Logger; readonly config: WorkerConfig; + /** + * What happens to a claimed ticket: clone, implement, verify, open a pull request. + * + * The one call this seam was missing. Everything around it — the poll, the claim, the + * concurrency cap, the per-ticket containment — worked already and proved only itself. + */ + readonly delivery: DeliveryPort; + /** + * The process's spend ceilings. One cycle is opened per run; see `runCycle`. + * + * Built once per process because the daily counter is a property of the day and the process is + * what spans it. Passing it in rather than building it here is what makes that lifetime the + * caller's to get right, and `src/worker.ts` is where it is got right. + */ + readonly budget: BudgetGuard; + /** + * A fresh at-most-once hand-back. Called once per ticket, and never shared between two. + * + * The invariant it carries is that a ticket is handed back once however many give-up paths + * fire on it — see `createHandBackOnce`. + */ + readonly handBack: () => HandBackOnce; } interface CycleResult { @@ -35,6 +46,27 @@ interface CycleResult { readonly outcome: 'ok' | 'failed'; } +/** + * What the worker says on a ticket it was holding when something unexpected went wrong. + * + * The alternative is worse than it sounds. `releaseTicket` and `handBackTicket` deliberately keep + * hold of a ticket they cannot return to Open, on the argument that the boot-time orphan sweep + * recovers it — but that sweep is MAPCO-11432 and does not exist yet, so a ticket dropped here is + * a ticket assigned to a bot for ever, invisible to the poll and to whoever wrote it. Handing it + * back costs an attempt and says so. + */ +function failedNote(error: unknown): string { + return [ + 'Picked this up automatically and stopped part-way through with an error.', + '', + '{code}', + error instanceof Error ? error.message : String(error), + '{code}', + '', + 'This is the worker failing rather than the ticket being wrong, so it is worth a look. A branch under `agent/` may exist without a pull request; nothing was merged and nothing was approved. This counts as an attempt; the ticket is available again.', + ].join('\n'); +} + /** * Work through the run's tickets, no more than `maxConcurrentTickets` at a time. * @@ -42,13 +74,13 @@ interface CycleResult { * deployment that raises them, where the concurrency cap is what keeps the worker from * claiming a whole page of tickets at once. */ -async function handleTickets(tickets: JiraTicket[], deps: CycleDeps): Promise { +async function handleTickets(tickets: JiraTicket[], deps: CycleDeps, cycle: CycleGuard): Promise { const { maxConcurrentTickets } = deps.config; let started = 0; for (let index = 0; index < tickets.length; index += maxConcurrentTickets) { const batch = tickets.slice(index, index + maxConcurrentTickets); - const outcomes = await Promise.all(batch.map(async (ticket) => handleTicket(ticket, deps))); + const outcomes = await Promise.all(batch.map(async (ticket) => handleTicket(ticket, deps, cycle))); started += outcomes.filter((outcome) => outcome).length; } @@ -56,45 +88,81 @@ async function handleTickets(tickets: JiraTicket[], deps: CycleDeps): Promise { + try { + const released = await handBack.handBack(ticket, failedNote(error)); + + if (!released.ok) { + logger.error({ msg: 'failed ticket could not be handed back', key: ticket.key, reason: released.reason }); + } + } catch (err) { + // The second failure in a row. Nothing left to try, and a throw from in here would replace + // the original error in the log with a complaint about Jira. + logger.error({ msg: 'failed ticket could not be handed back', key: ticket.key, err }); + } +} + /** - * Claim one ticket and hand it back. Returns whether the worker actually held it. + * Claim one ticket, work it, and make sure it does not stay held. Returns whether the worker + * actually held it. * * Nothing in here is allowed to end the run: one ticket the worker cannot take says * nothing about the next one, and a run that dies on the first refusal would stall the * whole queue behind it. + * + * The claim goes through `cycle.start` rather than straight to `claimTicket`, and the indirection + * is load-bearing: it is what checks the day still has room for a ticket *before* anything is + * written, and what counts the ticket once the worker actually holds it. A run that has hit + * `MAX_TICKETS_PER_DAY` writes nothing at all — a ticket claimed and then dropped for a spend + * ceiling has already put a bot's name on a human's ticket and notified its watchers. */ -async function handleTicket(ticket: JiraTicket, deps: CycleDeps): Promise { - const { jira, logger, config } = deps; +async function handleTicket(ticket: JiraTicket, deps: CycleDeps, cycle: CycleGuard): Promise { + const { jira, logger, config, delivery } = deps; + const handBack = deps.handBack(); + let held = false; try { - const claim = await claimTicket(ticket, jira, config.bot); + const start = await cycle.start(ticket, async () => claimTicket(ticket, jira, config.bot)); + + if (!start.ok) { + if (start.reason === 'daily-cap') { + // Already reported by the guard, at info: this is a ceiling doing its job, not a fault. + return false; + } - if (!claim.ok) { // `saw` and `offered` are the diagnostic half of a refusal. A `lost-race` reporting // the bot's own name means JIRA_BOT_DISPLAY_NAME is wrong, not that a human raced; // `offered` names the transitions a workflow actually has. - logger.warn({ msg: 'not claimed', key: ticket.key, reason: claim.reason, saw: claim.saw, offered: claim.offered }); + logger.warn({ msg: 'not claimed', key: ticket.key, reason: start.claim.reason, saw: start.claim.saw, offered: start.claim.offered }); return false; } + held = true; logger.info({ msg: 'claimed', key: ticket.key }); - const release = await releaseTicket(ticket, HANDED_BACK_NOTE, jira); + const delivered = await delivery.deliver({ ticket, meter: start.ticket, handBack }); - if (!release.ok) { - // Still assigned to the bot and still In Progress, on purpose — see `releaseTicket`. - // The orphan sweep on boot (MAPCO-11432) is what gets it back. - logger.error({ msg: 'held ticket could not be released', key: ticket.key, reason: release.reason, offered: release.offered }); + if (delivered.ok) { + logger.info({ msg: 'pull request opened for ticket', key: ticket.key, branch: delivered.branch, url: delivered.url }); } else { - logger.info({ msg: 'released', key: ticket.key }); + // `handedBack: false` is the state worth finding in a log: the ticket is still assigned to + // the bot and still In Progress, which the poll query skips, so nothing will pick it up. + logger.warn({ msg: 'ticket not delivered', key: ticket.key, reason: delivered.reason, handedBack: delivered.handedBack }); } return true; } catch (error) { logger.error({ msg: 'ticket failed', key: ticket.key, err: error }); - return false; + if (held) { + await releaseAfterFailure(ticket, error, handBack, logger); + } + + // Truthfully: the worker did hold this one, whatever happened next. The alternative reports a + // ticket that was claimed, worked and commented on as one the run never touched. + return held; } } @@ -105,12 +173,17 @@ async function handleTicket(ticket: JiraTicket, deps: CycleDeps): Promise { const { jira, logger, config } = deps; const jql = buildPollQuery(ATTEMPT_CAP); + const cycle = deps.budget.cycle(); let tickets: JiraTicket[]; try { @@ -126,7 +199,7 @@ async function runCycle(deps: CycleDeps): Promise { const more = tickets.length > config.maxTicketsPerRun; const eligible = tickets.slice(0, config.maxTicketsPerRun); - const started = await handleTickets(eligible, deps); + const started = await handleTickets(eligible, deps, cycle); const result: CycleResult = { found: eligible.length, @@ -142,11 +215,11 @@ async function runCycle(deps: CycleDeps): Promise { msg: 'cycle complete', ...result, keys: eligible.map((ticket) => ticket.key), - tokensSpent: 0, + ...cycle.runLine(), }); return result; } -export { ATTEMPT_CAP, HANDED_BACK_NOTE, runCycle }; +export { ATTEMPT_CAP, failedNote, runCycle }; export type { CycleDeps, CycleResult }; diff --git a/src/deliver.ts b/src/deliver.ts new file mode 100644 index 0000000..371eae8 --- /dev/null +++ b/src/deliver.ts @@ -0,0 +1,209 @@ +import type { Logger } from '@map-colonies/js-logger'; +import { implementTicket } from './agent/implement'; +import { meterAgent } from './agent/meteredAgent'; +import type { Implementer } from './agent/implementer'; +import type { AgentPort } from './agent/types'; +import type { TicketGuard } from './budget/guard'; +import type { GitHubPort, Repo } from './github/types'; +import type { JiraTicket } from './jira/types'; +import { publishPullRequest, UNREPORTED_WRITES, type PublishRefusal } from './pr/publish'; +import type { PullRequestPort, TicketCommentPort, VerifiedCheck } from './pr/types'; +import type { HandBackOnce } from './tickets/handBackOnce'; +import { describeRefusal, resolveRepo } from './tickets/resolveRepo'; +import type { GitPort, Workspace, WorkspacePort } from './vcs/types'; + +/** + * What happens to a claimed ticket, which until now was nothing. + * + * `runCycle` claimed a ticket and handed it straight back; separately, a fully tested + * implement-and-verify core existed that nothing called. This is the sequence between the two: + * work out which repository the ticket is about, clone it, hand the ticket to the model inside + * that clone, run the repository's own tests, and — only on a passing run — commit, push and + * open a pull request. Every step is somebody else's slice; what lives here is the order, the + * refusals, and the guarantee that the clone goes away again. + * + * It is a port rather than inline code in `handleTicket` for the reason the rest of the worker is + * built that way: `runCycle` is the seam the pipeline is tested through, and the claim/release + * plumbing around this call has its own behaviour — concurrency caps, the poll's one-extra + * ticket, a broken ticket not taking the run down — that is worth being able to exercise without + * a whole pipeline standing behind it. + * + * Four things are refusals rather than errors, and each one hands the ticket back with an + * attempt counted, because each is a thing a human has to read and decide about: + * + * - `no-repo` — the title names no repository, or names one that does not exist (MAPCO-11433). + * - `no-workspace` — the repository could not be cloned. Often not the ticket's fault, which the + * note says; it is still counted, because the alternative is a ticket that is picked up and + * fails the same way on every tick. + * - `not-implemented` — `implementTicket` gave up. It has already commented the specifics and + * handed the ticket back through the same latch this module holds; nothing more is written. + * - `nothing-to-publish` — the suite passed over a working tree with nothing in it worth + * offering. An empty pull request is not a smaller result than a full one. + */ + +/** Why a claimed ticket did not end in a pull request, when nothing actually broke. */ +type DeliveryRefusal = 'no-repo' | 'no-workspace' | 'not-implemented' | 'nothing-to-publish'; + +type DeliveryOutcome = + | { readonly ok: true; readonly branch: string; readonly url: string } + | { + readonly ok: false; + readonly reason: DeliveryRefusal; + /** Whether the ticket is back in Open. False means the worker still holds it. */ + readonly handedBack: boolean; + }; + +interface DeliveryRequest { + readonly ticket: JiraTicket; + /** + * This ticket's meter, which only a `cycle.start` that actually claimed it can hand out. + * + * Taken as an argument rather than opened here so a ticket cannot be worked without the day's + * allowance having been checked and spent on it — the same structural argument the budget + * guard makes one level up. + */ + readonly meter: TicketGuard; + /** + * This ticket's at-most-once hand-back, created by `handleTicket` and shared with it. + * + * Passed in rather than made here so there is exactly **one** per ticket. Two objects each + * claiming to be the ticket's latch would each allow a hand-back, which is the invariant they + * exist to hold — and the caller needs the same one anyway, for the ticket it is still holding + * when something throws out of here. See `createHandBackOnce`. + */ + readonly handBack: HandBackOnce; +} + +interface DeliveryPort { + deliver: (request: DeliveryRequest) => Promise; +} + +interface DeliveryDeps { + readonly github: GitHubPort; + readonly workspaces: WorkspacePort; + /** The implement step, already built. Supplies the per-ticket half per ticket. */ + readonly implementer: Implementer; + readonly pullRequests: PullRequestPort; + /** The pull-request link comment. `JiraPort` satisfies this structurally. */ + readonly tickets: TicketCommentPort; + /** git inside one clone. A factory because the checkout does not exist until it is cloned. */ + readonly git: (options: { readonly cwd: string; readonly repo: Repo }) => GitPort; + readonly logger: Logger; +} + +/** + * What the worker says on a ticket whose repository it could not clone. + * + * Says plainly that this may not be the ticket's fault, because the attempt is counted either + * way and a reader who assumes the ticket is wrong will go looking in the wrong place. Two + * failures in a row retire the ticket from the queue (`ATTEMPT_CAP`), so the note has to be + * enough for somebody to decide whether to clear the counter and try again. + */ +function cloneFailedNote(repo: Repo, error: unknown): string { + return [ + 'Picked this up automatically and could not start work on it.', + '', + `\`${repo.fullName}\` could not be cloned at its default branch (\`${repo.defaultBranch}\`):`, + '', + '{code}', + error instanceof Error ? error.message : String(error), + '{code}', + '', + 'This may be the repository or the network rather than the ticket — a private repository the worker has no access to looks like this, and so does a bad five minutes on the way out of the cluster. Nothing was changed and nothing was pushed. This counts as an attempt; the ticket is available again.', + ].join('\n'); +} + +/** What the worker says when the suite passed but there is nothing worth offering a reviewer. */ +function unpublishableNote(reason: PublishRefusal): string { + const middle = + reason === 'nothing-to-commit' + ? 'The tests passed, but the working tree was identical to the branch they ran against — so there is no change to offer. Most often that means the model wrote a file back exactly as it found it.' + : 'The tests passed, but the only files that differed were ones the model never reported writing — build output or a regenerated lockfile, most likely. A pull request whose whole diff is generated is worse than no pull request.'; + + return [ + 'Picked this up automatically and did not open a pull request.', + '', + middle, + '', + 'Nothing was pushed and no branch was created. This counts as an attempt; the ticket is available again.', + ].join('\n'); +} + +/** What the pull-request body says was checked, from what the verify step actually ran. */ +function verifiedBy(command: string): readonly VerifiedCheck[] { + // `passed: true` is not an assumption: `implementTicket` only reports success on a test run + // that returned ok, against a command read off the clone *before* the model touched it. + return [{ command, passed: true }]; +} + +function createDelivery(deps: DeliveryDeps): DeliveryPort { + const { github, workspaces, implementer, pullRequests, tickets, logger } = deps; + + /** Clone, implement, publish. Separate so the workspace's `finally` has one thing to wrap. */ + const inWorkspace = async (repo: Repo, workspace: Workspace, request: DeliveryRequest): Promise => { + const { ticket, handBack } = request; + const meter = (agent: AgentPort): AgentPort => meterAgent({ agent, meter: request.meter, handBack, logger }); + const implemented = await implementTicket({ ticket, workdir: workspace.dir }, implementer({ release: handBack, meter })); + + if (!implemented.ok) { + // Already commented and handed back by `implementTicket`'s single give-up exit — through + // the same latch, so an overspend that also ended the run wrote one comment, not two. + return { ok: false, reason: 'not-implemented', handedBack: implemented.released }; + } + + const published = await publishPullRequest( + { ticket, repo, checks: verifiedBy(implemented.command), wrote: UNREPORTED_WRITES }, + { git: deps.git({ cwd: workspace.dir, repo }), pullRequests, tickets, logger } + ); + + if (!published.ok) { + const released = await handBack.handBack(ticket, unpublishableNote(published.reason)); + + return { ok: false, reason: 'nothing-to-publish', handedBack: released.ok }; + } + + return { ok: true, branch: published.branch, url: published.pullRequest.url }; + }; + + return { + deliver: async (request: DeliveryRequest): Promise => { + const { ticket, handBack } = request; + + const resolution = await resolveRepo(ticket, github); + + if (!resolution.ok) { + // The common path until the title convention spreads, and a refusal rather than a guess: + // the note names what was looked for so a human can fix the title. + logger.warn({ msg: 'refusing the ticket', key: ticket.key, reason: resolution.reason, looked: resolution.looked }); + const released = await handBack.handBack(ticket, describeRefusal(resolution)); + + return { ok: false, reason: 'no-repo', handedBack: released.ok }; + } + + const { repo } = resolution; + logger.info({ msg: 'repo resolved', key: ticket.key, repo: repo.fullName, base: repo.defaultBranch }); + + let workspace: Workspace; + try { + workspace = await workspaces.create(repo); + } catch (err) { + logger.error({ msg: 'could not clone the repository', key: ticket.key, repo: repo.fullName, err }); + const released = await handBack.handBack(ticket, cloneFailedNote(repo, err)); + + return { ok: false, reason: 'no-workspace', handedBack: released.ok }; + } + + try { + return await inWorkspace(repo, workspace, request); + } finally { + // MAPCO-11433's "cleaned up on every path", and it is the `finally` that makes it true: + // a publish failure throws by design, and a worker that leaked one clone per thrown + // ticket would fill the volume and then fail every ticket for an unrelated-looking reason. + await workspace.dispose(); + } + }, + }; +} + +export { cloneFailedNote, createDelivery, unpublishableNote }; +export type { DeliveryDeps, DeliveryOutcome, DeliveryPort, DeliveryRefusal, DeliveryRequest }; diff --git a/src/dryRun.ts b/src/dryRun.ts index 73756e7..842df2c 100644 --- a/src/dryRun.ts +++ b/src/dryRun.ts @@ -6,42 +6,40 @@ * It exercises the same `runCycle` seam the deployed worker runs, so what it proves is * about the worker and not about the harness. * - * Not read-only: this walks the same claim-and-release path the deployed worker walks, so - * it comments on, assigns and transitions a real ticket (MAPCO-11431). + * **This is the whole pipeline now, not a claim and a release.** It claims a real ticket, clones + * the repository its title names, hands the ticket to the model inside that clone, runs the + * repository's own tests, and on a passing run pushes an `agent/` branch and opens a real pull + * request. It costs real tokens against the configured key and it writes to real tickets. The + * spend ceilings apply — `MAX_TICKETS_PER_DAY` counts per process, so a dry run gets its own + * allowance — and the cheapest way to keep a first run small is to set them low on the command + * line. */ import 'reflect-metadata'; import { jsLogger } from '@map-colonies/js-logger'; -import { loadWorkerConfig } from '@common/workerConfig'; +import { budgetOf, loadWorkerConfig } from '@common/workerConfig'; import { ATTEMPT_CAP, runCycle } from './cycle'; import { McpJira } from './jira/mcpJira'; -import { RestGitHub } from './github/restGitHub'; -import { describeRefusal, resolveRepo } from './tickets/resolveRepo'; import { buildPollQuery } from './jira/query'; +import { createWorker } from './worker'; async function dryRun(): Promise { const config = loadWorkerConfig(); const logger = await jsLogger({ level: 'debug', prettyPrint: true }); - logger.info({ msg: 'dry run starting', mcpUrl: config.mcpUrl, jql: buildPollQuery(ATTEMPT_CAP) }); + logger.info({ + msg: 'dry run starting', + mcpUrl: config.mcpUrl, + jql: buildPollQuery(ATTEMPT_CAP), + workspaceRoot: config.workspaceRoot, + ...budgetOf(config).ticket, + maxTicketsPerDay: budgetOf(config).maxTicketsPerDay, + }); const jira = new McpJira(config.mcpUrl); - const github = new RestGitHub(process.env.GITHUB_TOKEN); try { - const result = await runCycle({ jira, logger, config }); + const result = await runCycle(createWorker({ jira, logger, config })); logger.info({ msg: 'dry run complete', ...result }); - - // Not part of the cycle yet — MAPCO-11433 wires it in. Shown here so a dry run - // reports what the next slice would decide about each ticket it found. - for (const ticket of await jira.search(buildPollQuery(ATTEMPT_CAP), config.maxTicketsPerRun)) { - const resolution = await resolveRepo(ticket, github); - - if (resolution.ok) { - logger.info({ msg: 'repo resolved', key: ticket.key, titleSaid: ticket.summary.split(':')[0], repo: resolution.repo }); - } else { - logger.warn({ msg: 'would refuse', key: ticket.key, reason: resolution.reason, comment: describeRefusal(resolution) }); - } - } } finally { await jira.close(); } diff --git a/src/index.ts b/src/index.ts index da946e5..12c6535 100644 --- a/src/index.ts +++ b/src/index.ts @@ -7,6 +7,7 @@ import { getTracing } from '@common/tracing'; import { registerExternalValues } from './containerConfig'; import { McpJira } from './jira/mcpJira'; import { createScheduler } from './scheduler'; +import { createWorker } from './worker'; async function main(): Promise { const container = await registerExternalValues(); @@ -14,7 +15,11 @@ async function main(): Promise { const config = loadWorkerConfig(); const jira = new McpJira(config.mcpUrl); - const scheduler = createScheduler({ jira, logger, config }, config.pollIntervalMs, logger); + // Everything the worker needs, built once — including the model credential, so a pod with no + // Secret fails to come up rather than claiming a ticket and discovering it. See `createWorker` + // for which of these lifetimes are load-bearing. + const worker = createWorker({ jira, logger, config }); + const scheduler = createScheduler(worker, config.pollIntervalMs, logger); const shutdown = (signal: string): void => { logger.info({ msg: 'shutting down', signal }); diff --git a/src/jira/description.ts b/src/jira/description.ts new file mode 100644 index 0000000..b4a5a67 --- /dev/null +++ b/src/jira/description.ts @@ -0,0 +1,29 @@ +import type { DescriptionPort } from '../agent/types'; +import type { JiraPort, JiraTicket } from './types'; + +/** + * The implementation `DescriptionPort` (src/agent/types.ts) was declared without. + * + * Until this existed the honest stub returned `''` on every ticket, and `implementTicket` answers + * an empty description by refusing the ticket *before the first model turn* — so every + * `agent-ready` ticket in the queue was claimed, refused as `no-description` and handed back, + * which is why labelling a ticket proved only that the claim and release plumbing worked. + * + * Read per claimed ticket rather than at poll time, which is the port's own argument and the + * reason `POLL_FIELDS` still does not ask for a description: the poll fetches one more ticket + * than it will work, and prose for a ticket nobody touches is paid for on every tick. The read + * is `JiraPort.getIssue`, whose field list (`ISSUE_FIELDS`) is the one that asks for it. + * + * A missing description and a missing *issue* both answer `''`, because they are the same fact + * from the model's side: there is nothing here to change code against. A read that *fails* is + * not flattened into that — it throws, and `implementTicket` catches it and says so, because + * "this ticket has no description" asks a human to write one while "the description could not be + * read" asks them to fix the worker. + */ +function createDescriptionReader(jira: JiraPort): DescriptionPort { + return { + read: async (ticket: JiraTicket): Promise => (await jira.getIssue(ticket.key))?.description ?? '', + }; +} + +export { createDescriptionReader }; diff --git a/src/jira/mcpJira.ts b/src/jira/mcpJira.ts index 06c14e8..5d20d94 100644 --- a/src/jira/mcpJira.ts +++ b/src/jira/mcpJira.ts @@ -8,6 +8,18 @@ import type { JiraPort, JiraTicket, JiraTransition } from './types'; const POLL_FIELDS = 'summary,status,labels,assignee,issuetype,created'; +/** + * The fields of a single issue read back by key, which is `POLL_FIELDS` plus the description. + * + * Separate from `POLL_FIELDS` rather than added to it, and the difference is the whole point. + * The poll asks for one more ticket than it will work and drops the rest, so a description in + * `POLL_FIELDS` would fetch prose for tickets nobody touches — on every tick, for the lifetime + * of the worker. A ticket read by key is one the worker has decided it cares about, so that is + * where the prose is worth paying for. See `DescriptionPort` (src/agent/types.ts), whose + * implementation is the only caller that needs it. + */ +const ISSUE_FIELDS = `${POLL_FIELDS},description`; + /* eslint-disable @typescript-eslint/naming-convention -- these mirror the MCP server's wire format */ interface McpTransition { id: number | string; @@ -18,6 +30,7 @@ interface McpTransition { interface McpTicket { key: string; summary?: string; + description?: string; labels?: string[]; status?: { name?: string }; issue_type?: { name?: string }; @@ -49,7 +62,7 @@ class McpJira implements JiraPort { public async getIssue(issueKey: string): Promise { // eslint-disable-next-line @typescript-eslint/naming-convention -- MCP wire format - const raw = await this.call('jira_get_issue', { issue_key: issueKey, fields: POLL_FIELDS, comment_limit: 0 }); + const raw = await this.call('jira_get_issue', { issue_key: issueKey, fields: ISSUE_FIELDS, comment_limit: 0 }); const parsed = JSON.parse(raw) as McpTicket | null; return parsed?.key === undefined ? null : toTicket(parsed); @@ -172,6 +185,10 @@ function toTicket(issue: McpTicket): JiraTicket { return { key: issue.key, summary: issue.summary ?? '', + // Left absent rather than defaulted to `''`: a ticket off the poll was never asked about its + // description, and reporting "no description" for a field nobody requested is the difference + // between a ticket the worker should refuse and one it has simply not read yet. + ...(typeof issue.description === 'string' ? { description: issue.description } : {}), issueType: issue.issue_type?.name ?? 'unknown', status: issue.status?.name ?? 'unknown', // The server omits `labels` entirely when a ticket has none. @@ -180,5 +197,5 @@ function toTicket(issue: McpTicket): JiraTicket { }; } -export { assigneeFields, labelsFields, McpJira, toTicket, toTransition }; +export { assigneeFields, ISSUE_FIELDS, labelsFields, McpJira, POLL_FIELDS, toTicket, toTransition }; export type { McpTicket, McpTransition }; diff --git a/src/jira/types.ts b/src/jira/types.ts index ac99e17..655765a 100644 --- a/src/jira/types.ts +++ b/src/jira/types.ts @@ -5,6 +5,19 @@ export interface JiraTicket { readonly status: string; readonly labels: readonly string[]; readonly assignee: string | null; + /** + * The ticket's prose, when it was read. + * + * Optional because it is deliberately absent from the poll. Descriptions are long, the poll + * asks for one more ticket than it will work (the server's `total` is always -1), and prose + * the worker never uses is not worth carrying — so `POLL_FIELDS` does not ask for one and a + * ticket off `search` never has one. It arrives on the per-ticket read instead, which is what + * `DescriptionPort` (src/agent/types.ts) is for; see `ISSUE_FIELDS` in mcpJira.ts. + * + * Absent and empty are therefore different facts and must not be conflated: absent means + * nobody asked, empty means the ticket has nothing on it. Only the second is a refusal. + */ + readonly description?: string; } export interface JiraTransition { diff --git a/src/pr/types.ts b/src/pr/types.ts index e357723..95daa6a 100644 --- a/src/pr/types.ts +++ b/src/pr/types.ts @@ -3,10 +3,14 @@ import type { Repo } from '../github/types'; /** * One thing the verify slice ran in the checkout before any of this was allowed to happen. * - * Declared here rather than imported because the verify slice (MAPCO-11434) does not exist - * yet: this is the narrowest shape a pull-request body needs in order to say what was checked, - * and it is the caller's job to fill it in honestly. `passed: false` is representable on - * purpose — a body that can only describe success is a body that will eventually lie. + * Declared here rather than imported from the verify slice: this is the narrowest shape a + * pull-request body needs in order to say what was checked, and it is the caller's job to fill it + * in honestly. `passed: false` is representable on purpose — a body that can only describe + * success is a body that will eventually lie. + * + * `verifiedBy` (src/deliver.ts) is what fills it in, from the command `implementTicket` reports + * having passed — which is the command read off the clone *before* the model touched it, not + * whatever the manifest says afterwards. */ interface VerifiedCheck { /** Exactly what was run, as a reviewer would run it themselves. */ diff --git a/src/preflight.ts b/src/preflight.ts new file mode 100644 index 0000000..dd2351e --- /dev/null +++ b/src/preflight.ts @@ -0,0 +1,25 @@ +/** Boot checks only — no Jira, no model, no writes. Safe to run as often as you like. */ +import { readModelCredential } from '@src/agent/credential'; +import { readGitHubToken } from '@src/vcs/envToken'; +import { budgetOf, loadWorkerConfig } from '@common/workerConfig'; + +try { + const cred = readModelCredential(process.env); + console.log(`model credential ok — mode '${cred.mode}', from ${cred.source} (${cred.value.length} chars)`); + + readGitHubToken(process.env); + console.log('github token ok'); + + const config = loadWorkerConfig(); + console.log('worker config ok —', { + mcpUrl: config.mcpUrl, + bot: config.bot, + workspaceRoot: config.workspaceRoot, + ...budgetOf(config).ticket, + maxTicketsPerDay: budgetOf(config).maxTicketsPerDay, + }); + console.log('\nready. nothing was claimed and nothing was spent.'); +} catch (error) { + console.error('NOT ready:', (error as Error).message); + process.exit(1); +} diff --git a/src/tickets/claim.ts b/src/tickets/claim.ts index 56cdb11..2a02349 100644 --- a/src/tickets/claim.ts +++ b/src/tickets/claim.ts @@ -42,16 +42,67 @@ interface Refused { type ClaimOutcome = { readonly ok: true } | Refused; type ReleaseOutcome = { readonly ok: true } | Refused; +/** + * Compare statuses the way a workflow author would, not the way a byte comparison does. + * + * MAPCO spells the status `In Progress` and the transition into it `Open to In-Progress`. The + * hyphen is the kind of difference nobody intends to be meaningful, so case, hyphens, + * underscores and runs of whitespace are all flattened before comparing. + */ +function normalise(value: string): string { + return value + .toLowerCase() + .replace(/[\s_-]+/gu, ' ') + .trim(); +} + +/** + * The target half of a transition named ` to `, if it has that shape. + * + * Non-greedy on the left so `Open to In-Progress` yields `In-Progress` rather than matching at + * the last `to`, which would matter for a name like `Ready to Deploy to Production`. + */ +function targetInName(name: string): string | undefined { + return /^.+?\s+to\s+(?.+)$/u.exec(name)?.groups?.['target']; +} + /** * Find the transition that lands a ticket in `status`. * - * Target status first, transition name second. Jira transition names are verbs on a real - * workflow — `Start Progress`, not `In Progress` — so matching on the name alone would find - * nothing and refuse every ticket. The name is kept only as a fallback for a server that - * reports no target status. + * Three strategies, narrowest first, because each is right for a different server and only the + * first is right in general: + * + * 1. **Target status**, when the server reports one. The only strategy that cannot be fooled. + * 2. **The transition's own name**, for a workflow whose transitions are named after where they + * land. + * 3. **The target half of a ` to ` name.** Verified against the live MAPCO workflow, + * which reports no target status at all — `jira_get_transitions` answers with `id` and `name` + * and nothing else — and names its transitions `Open to In-Progress`, `Open to Verified`, + * `Open to Reject`. Without this the first two both miss and every ticket is refused with + * `no-transition`, which is exactly what the first real run did. + * + * Order matters. A transition named plainly `Open` must win over `Back to Open` if a workflow + * somehow offers both, and strategy 3 must never outrank an exact name. */ function findTransition(transitions: readonly JiraTransition[], status: string): JiraTransition | undefined { - return transitions.find((candidate) => candidate.to === status) ?? transitions.find((candidate) => candidate.name === status); + const wanted = normalise(status); + + // Split first, because a reported target is authoritative *for that entry*: a transition + // that says it lands in Blocked does not land in In Progress however it is named, and + // letting a name-based guess override it would move a ticket somewhere nobody asked for. + // The name is consulted only where there is nothing better to go on. + const reported = transitions.filter((candidate): candidate is JiraTransition & { to: string } => candidate.to !== undefined); + const unreported = transitions.filter((candidate) => candidate.to === undefined); + + return ( + reported.find((candidate) => normalise(candidate.to) === wanted) ?? + unreported.find((candidate) => normalise(candidate.name) === wanted) ?? + unreported.find((candidate) => { + const target = targetInName(candidate.name); + + return target !== undefined && normalise(target) === wanted; + }) + ); } function refuseNoTransition(transitions: readonly JiraTransition[]): Refused { diff --git a/src/tickets/handBackOnce.ts b/src/tickets/handBackOnce.ts new file mode 100644 index 0000000..3873af5 --- /dev/null +++ b/src/tickets/handBackOnce.ts @@ -0,0 +1,90 @@ +import type { ReleaseResult } from '../agent/types'; +import type { AbortResult } from '../budget/types'; +import type { JiraTicket } from '../jira/types'; +import { handBackTicket, type HandBackDeps, type HandBackOutcome } from './handBack'; + +/** + * One ticket's hand-back, performed at most once. + * + * Two independent slices each own a give-up path, and after the wiring they can both fire on the + * same ticket in the same breath: + * + * - `implementTicket` (src/agent/implement.ts) funnels every give-up through one exit and calls + * `ReleasePort.handBack`. + * - `chargeSpend` (src/budget/enforce.ts) calls `AbortPort.abort` on the charge that first runs + * the per-ticket budget out. + * + * Both are correct on their own, both are specified as "comment, count the attempt, release", and + * both are bound to `handBackTicket` because their docstrings say in as many words not to write a + * second release path. The overlap is the ordinary overspend: the budget runs out mid-hand-off, + * the abort hands the ticket back, and the run that is now over is reported to `implementTicket` + * as a give-up — which hands it back again. That second pass would comment a second time and + * re-transition a ticket already in Open that a human may have picked up, which is exactly the + * hazard `BudgetExhausted.alreadyStopped` describes and cannot itself prevent: its latch stops + * repeated *charges*, not a different code path releasing. + * + * So the invariant lives here instead of in either slice, as one object per ticket in flight. The + * ordering is always budget-then-implement — a charge only happens inside a hand-off, and the + * give-up decision comes after it — so the overspend note is the one that lands, which is the + * right way round: `describeOverspend` says what the ticket cost and which ceiling it hit, where + * the give-up note would only say the run did not finish. + */ +interface HandBackOnce { + /** `ReleasePort.handBack`: comment, count the attempt, release — unless that already happened. */ + handBack: (ticket: JiraTicket, note: string) => Promise; + /** + * Record a hand-back the budget path already performed, so this one does not repeat it. + * + * Called by `meterAgent` with the `AbortResult` the charge reported. It latches **only if the + * attempt was counted**, because that is the write that happens first: a result with + * `attemptCounted: false` means `handBackTicket` refused before writing anything at all — no + * label, no comment, no transition — so nothing has been said on the ticket and the ordinary + * give-up path should still get its turn. Latching on that would leave a ticket held, silent + * and uncommented, with a worker that had decided it was done with it. + */ + record: (outcome: AbortResult) => void; + /** Whether this ticket has been handed back, for a caller deciding what to say about it. */ + handedBack: () => boolean; +} + +/** Whether a hand-back that has already happened counts as a release, in `ReleasePort`'s terms. */ +function asReleaseResult(outcome: HandBackOutcome): ReleaseResult { + return outcome.released ? { ok: true } : { ok: false, reason: outcome.reason ?? 'not-released' }; +} + +/** + * The hand-back for one ticket. Build one per ticket the worker takes, and throw it away with the + * ticket — a longer-lived one would refuse the *next* legitimate attempt on the same ticket. + */ +function createHandBackOnce(deps: HandBackDeps): HandBackOnce { + let pending: Promise | null = null; + + return { + handBack: async (ticket: JiraTicket, note: string): Promise => { + if (pending !== null) { + // Worth a line: it means two give-up paths fired on one ticket, which is normal for an + // overspend and would be worth knowing about if it ever happened for another reason. + deps.logger.info({ msg: 'ticket already handed back, not repeating it', key: ticket.key }); + + return asReleaseResult(await pending); + } + + // Latched before the first await, so two callers racing get one write: the second joins + // this promise rather than starting a hand-back of its own. + pending = handBackTicket(ticket, note, deps); + + return asReleaseResult(await pending); + }, + + record: (outcome: AbortResult): void => { + if (pending === null && outcome.attemptCounted) { + pending = Promise.resolve({ released: outcome.released, attemptCounted: true }); + } + }, + + handedBack: (): boolean => pending !== null, + }; +} + +export { createHandBackOnce }; +export type { HandBackOnce }; diff --git a/src/vcs/cliGit.ts b/src/vcs/cliGit.ts index cfd2b09..ee3645f 100644 --- a/src/vcs/cliGit.ts +++ b/src/vcs/cliGit.ts @@ -1,115 +1,24 @@ -import { execFile } from 'node:child_process'; -import { devNull } from 'node:os'; -import { promisify } from 'node:util'; import type { Repo } from '../github/types'; +import { CREDENTIAL_FROM_ENV, gitRunner, HOOKS_OFF, redact, TOKEN_ENV, type RunGit } from './gitInvoke'; import { AGENT_PREFIX } from './naming'; import type { GitIdentity, GitPort, TokenProvider } from './types'; -const run = promisify(execFile); - /** `git status --porcelain` prefixes every path with two status letters and a space. */ const STATUS_PREFIX_LENGTH = 3; /** Porcelain writes a rename as `old -> new`; the new path is the one that exists. */ const RENAME_ARROW = ' -> '; -/** - * Room for git's own output. The default 1 MiB is enough for a push, but `status --porcelain` - * on a large generated diff is not worth failing with `ENOBUFS` over. - */ -const MAX_OUTPUT_BYTES = 10_485_760; -/** What a redacted secret reads as in an error message. */ -const REDACTED = '***'; /** - * How long any one git invocation may take before it is killed. + * How long any one git invocation here may take before it is killed. * - * Every other subprocess boundary in the worker is bounded — the verify slice's runner gives a - * clone's own suite fifteen minutes (`src/workspace/subprocess.ts`) — and this one used to be - * the exception. The failure it prevents is specific: a push to a remote behind a proxy that - * completes the handshake and then answers nothing hangs `execFile` forever, and because - * `publishPullRequest` is awaited by `handleTicket` which is awaited by `runCycle`, one hung - * push stops the scheduler from ever ticking again. The pod stays alive and healthy — it is - * outbound-only, so no probe kills it (MAPCO-11430) — and the queue simply stops. - * - * Five minutes rather than fifteen: nothing here installs anything or runs a test suite. It is - * a status, a checkout, a commit and a push, and a push that has not finished in five minutes - * is not going to. + * Five minutes rather than the fifteen the verify slice gives a clone's own suite: nothing here + * installs anything or runs a test suite. It is a status, a checkout, a commit and a push, and a + * push that has not finished in five minutes is not going to. Why a bound exists at all — one + * hung push stops the scheduler for ever — is in `gitRunner`. */ const GIT_TIMEOUT_MS = 300_000; -/** - * Never ask a human anything. - * - * git's default is to prompt on a missing or rejected credential, and a prompt on a process - * with no terminal is a process that waits until the timeout above rather than failing with a - * usable message. `GIT_TERMINAL_PROMPT=0` turns the prompt into an immediate error; the askpass - * variables are emptied because a developer machine — where `npm run dry-run` runs — often has - * a graphical credential helper configured that would otherwise pop a window nobody sees. - */ -/* eslint-disable @typescript-eslint/naming-convention -- environment variable names, not identifiers */ -const NON_INTERACTIVE: NodeJS.ProcessEnv = { - GIT_TERMINAL_PROMPT: '0', - GIT_ASKPASS: '', - SSH_ASKPASS: '', - GCM_INTERACTIVE: 'never', -}; -/* eslint-enable @typescript-eslint/naming-convention */ - -/** - * The environment variable the push credential is handed over in, and the helper that reads it. - * - * The token used to be interpolated into the push URL, which put it in git's argv — and argv is - * world-readable through `/proc//cmdline`, so any process on the host could read a live - * installation token with org-wide write access straight out of the process table. It did not - * even need to be a process the worker started: the model has `Write`, the verify slice runs - * the clone's own `npm test`, and a rewritten test script therefore gets same-user execution in - * the same container minutes before the push happens (containing that is MAPCO-11430's job, but - * the token need not be reachable for it to matter). - * - * A credential helper is git's own answer to this. The helper is a shell snippet — git runs a - * `!`-prefixed value through `sh -c` with the operation appended — and the snippet contains no - * secret, only the *name* of a variable. The value travels in the child's environment, which - * `/proc//environ` exposes to the process owner alone rather than to everybody. The empty - * `credential.helper=` in front of it resets the helper list, so a helper inherited from a - * developer's global config cannot answer first with a stale credential of its own. - * - * `case` rather than `test "$1" = get &&`, so the snippet exits zero on the `store` and `erase` - * operations git also calls it with instead of looking like a failing helper. - */ -const TOKEN_ENV = 'GIT_AGENT_PUSH_TOKEN'; -/* Exported so that `tests/unit/vcs/scratchRepo.spec.ts` can hand the snippet to the real git and - * watch it answer, rather than asserting that one string equals another string. A shell snippet - * git runs through `sh -c` is not something a mock can tell you is correct. */ -const CREDENTIAL_FROM_ENV = [ - '-c', - 'credential.helper=', - '-c', - `credential.helper=!f() { case "$1" in get) printf 'username=x-access-token\\npassword=%s\\n' "$${TOKEN_ENV}" ;; esac; }; f`, -]; - -/** - * Every git invocation that could fire a hook carries this, and it is the reason the publish - * path cannot be killed by the repository it is working on. - * - * The clone is an arbitrary repository whose hooks the verify slice has just installed for us: - * `npm ci` runs `prepare`, which runs husky, which is how a `pre-commit` running `pretty-quick` - * and a `commit-msg` running `commitlint` end up live in the checkout. Those hooks are a - * contract between that repo and its humans; they are arbitrary code, and any one of them - * exiting non-zero loses the whole result — no commit, no branch, no pull request, no comment on - * the ticket. The repo's real gate on this change is the pull request's own CI plus a human - * review, and both still run. - * - * `--no-verify` was the first attempt and is **not** enough: it bypasses `pre-commit` and - * `commit-msg` only, so a `prepare-commit-msg` hook still runs — and still fails, and can also - * rewrite the header this worker computed. Observed in the scratch repo in - * `tests/unit/vcs/scratchRepo.spec.ts`, which is what a hooks-path of nothing fixes and - * `--no-verify` did not. Pointing `core.hooksPath` at `/dev/null` means git looks for every hook - * inside a path that is not a directory and finds none of them. - * - * None of this excuses writing a header the org's `commit-msg` hook would reject: `naming.ts` - * emits one commitlint accepts, because a squash merge puts that string on the default branch - * where the hook is not bypassed. - */ -const HOOKS_OFF = ['-c', `core.hooksPath=${devNull}`]; +const execGit = gitRunner(GIT_TIMEOUT_MS); /** * Refs the worker is allowed to write, matched in full. @@ -122,16 +31,6 @@ const WRITABLE_REF = /^agent\/[A-Za-z0-9][A-Za-z0-9._/-]*$/u; const REF_TRAVERSAL = '..'; const REF_LOCK_SUFFIX = '.lock'; -/** - * Credentials that reached an error message anyway, in `https://user:secret@host` form. - * - * Belt and braces next to redacting the minted token by value. This worker never builds such a - * URL any more — the credential goes to git through the environment — but git echoes back - * whatever remote it was given, a clone URL from GitHub's API could carry credentials, and a - * token in a log line outlives the token's own hour. - */ -const URL_CREDENTIALS = /\/\/[^@/\s]+@/gu; - /** * Ask git to read every pathspec as one literal path. * @@ -173,14 +72,6 @@ class GitGuardError extends Error { } } -/** - * Runs git and resolves its stdout. Injected so the guards can be tested without a checkout. - * - * `env` is additions to the child's environment, not a replacement for it, and exists for one - * reason: it is how the push credential reaches git without ever appearing in its argv. - */ -type RunGit = (args: readonly string[], cwd: string, env?: NodeJS.ProcessEnv) => Promise; - interface CliGitOptions { /** The checkout the verify slice left its diff in. */ readonly cwd: string; @@ -194,29 +85,6 @@ interface CliGitOptions { readonly run?: RunGit; } -async function execGit(args: readonly string[], cwd: string, env: NodeJS.ProcessEnv = {}): Promise { - // `execFile`, never `exec`: arguments are passed as an array, so there is no shell to quote - // for and a branch name or a commit message cannot become a second command. - const { stdout } = await run('git', [...args], { - cwd, - maxBuffer: MAX_OUTPUT_BYTES, - timeout: GIT_TIMEOUT_MS, - // A git that ignored the term signal would otherwise keep the promise pending past the - // timeout, which is the whole failure the timeout exists to prevent. - killSignal: 'SIGKILL', - env: { ...process.env, ...NON_INTERACTIVE, ...env }, - }); - - return stdout; -} - -/** Replace a token, and any credentials in a URL, wherever they appear in a message. */ -function redact(message: string, token: string): string { - const withoutToken = token === '' ? message : message.split(token).join(REDACTED); - - return withoutToken.replace(URL_CREDENTIALS, `//${REDACTED}@`); -} - /** * Refuse any ref outside `agent/`. * @@ -361,5 +229,7 @@ class CliGit implements GitPort { } } -export { CliGit, CREDENTIAL_FROM_ENV, GitGuardError, TOKEN_ENV }; -export type { CliGitOptions, RunGit }; +export { CliGit, GitGuardError }; +export { CREDENTIAL_FROM_ENV, TOKEN_ENV } from './gitInvoke'; +export type { CliGitOptions }; +export type { RunGit } from './gitInvoke'; diff --git a/src/vcs/clone.ts b/src/vcs/clone.ts new file mode 100644 index 0000000..892df86 --- /dev/null +++ b/src/vcs/clone.ts @@ -0,0 +1,205 @@ +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { Logger } from '@map-colonies/js-logger'; +import type { Repo } from '../github/types'; +import { CREDENTIAL_FROM_ENV, gitRunner, HOOKS_OFF, redact, TOKEN_ENV, type RunGit } from './gitInvoke'; +import type { TokenProvider, Workspace, WorkspacePort } from './types'; + +/** + * How long a clone may take before it is killed. + * + * Longer than the five minutes the publish path allows itself, because this one is a network + * transfer of somebody else's repository over whatever the cluster's egress is having today. It + * is bounded for the same reason everything else here is: `runCycle` awaits this, so a clone + * that hangs stops the scheduler ticking and the queue silently stops behind a pod that no probe + * will ever restart (MAPCO-11430). + */ +const CLONE_TIMEOUT_MS = 600_000; + +/** Every workspace directory starts with this, so a stray one is recognisable on a volume. */ +const WORKSPACE_PREFIX = 'agent-ticket-'; + +/** + * The only host the worker will hand a push or clone credential to. + * + * Hard-coded to match `RestGitHub`'s org, and checked rather than trusted even though the URL + * comes from GitHub's own API. The reason is specific to how the credential travels: the helper + * in `CREDENTIAL_FROM_ENV` answers with the token for *whatever host git asks it about*, so a + * remote nobody vetted is a live installation token handed to that remote. Checking the host + * before minting means there is nothing to hand over. + */ +const GITHUB_HOST = 'github.com'; + +/** + * Schemes the worker will clone from. + * + * `https:` is production, and only on `GITHUB_HOST`. `file:` is allowed and is deliberately + * given **no credential**: a local path cannot be an exfiltration target, and it is how a dry run + * can be pointed at a repository on disk and how `tests/unit/vcs/clone.spec.ts` exercises a real + * `git clone` against a real bare repository rather than against a fake that agrees with it. + * Everything else — `ssh:`, `git:`, a bare `host:path` scp-like remote — is refused: each one + * reaches a different credential path, none of which this worker has thought about. + */ +const HTTPS = 'https:'; +const FILE = 'file:'; + +/** + * A remote the worker will not clone from, as opposed to a clone that went wrong. + * + * A throw and not a refusal value, and the same argument as `GitGuardError`: a caller offering a + * remote outside the org is a bug in the caller, not a state the pipeline is expected to reach. + * It still lands on the ticket, because `DeliveryPort.deliver` (src/deliver.ts) turns a failed + * workspace into a hand-back rather than losing the ticket over it. + */ +class CloneRefusedError extends Error { + public constructor(message: string) { + super(message); + this.name = 'CloneRefusedError'; + } +} + +interface CloneWorkspaceOptions { + /** + * Directory the per-ticket workspaces are made under. + * + * A mounted volume in the pod and `os.tmpdir()` on a laptop. It is never deleted — only the + * per-ticket directories inside it are — because in the cluster it belongs to the Deployment. + */ + readonly root?: string; + /** Mints the clone credential. Called once per clone, never held. */ + readonly tokens: TokenProvider; + readonly logger: Logger; + /** Overridden in tests. */ + readonly run?: RunGit; +} + +/** + * Where the credential goes, if anywhere. + * + * Returned as a decision rather than taken as a flag so the two questions — may we clone this at + * all, and may we authenticate to it — are answered in one place and cannot drift apart. + */ +function credentialFor(cloneUrl: string, repo: Repo): 'github' | 'none' { + let parsed: URL; + + try { + parsed = new URL(cloneUrl); + } catch { + throw new CloneRefusedError(`refusing to clone ${repo.fullName}: \`${cloneUrl}\` is not a URL`); + } + + if (parsed.protocol === HTTPS && parsed.host === GITHUB_HOST) { + return 'github'; + } + + if (parsed.protocol === FILE) { + return 'none'; + } + + throw new CloneRefusedError( + `refusing to clone ${repo.fullName} from \`${parsed.protocol}//${parsed.host}\`: the worker only clones ${HTTPS}//${GITHUB_HOST}, because the push credential would otherwise be offered to it` + ); +} + +/** + * The ephemeral clone MAPCO-11433 asks for: the repo at its default branch, in a directory of + * its own, gone again afterwards. + * + * Three things are worth saying about the shape. + * + * **The branch is GitHub's `default_branch`, not the remote's HEAD.** The org has both `master` + * and `main` repositories and the two are not interchangeable — `repo.defaultBranch` is what the + * pull request is opened against, so the checkout has to be the same ref or the diff is against + * the wrong base. Asking git to pick means a repository whose remote HEAD disagrees with the API + * silently produces a pull request full of somebody else's commits. + * + * **Shallow and single-branch.** A ticket needs the tip, not the history: it is cheaper by orders + * of magnitude on a large repository, and pushing a new branch from a depth-1 clone is what + * `actions/checkout` does by default across the org, so it is not an exotic state for GitHub to + * receive. `--no-tags` for the same reason — nothing here reads a tag. + * + * **The directory is made before git runs, so every failure has to clean up after itself.** + * `create` removes it on the way out of a failed clone, and `dispose` removes it on the way out + * of a finished ticket. A worker that leaked one directory per failed ticket would fill the + * volume and then fail every ticket, which is the failure mode that looks like something else. + */ +class CloneWorkspace implements WorkspacePort { + private readonly root: string; + + private readonly git: RunGit; + + public constructor(private readonly options: CloneWorkspaceOptions) { + this.root = options.root ?? tmpdir(); + this.git = options.run ?? gitRunner(CLONE_TIMEOUT_MS); + } + + public async create(repo: Repo): Promise { + // Refused before the directory is made and before the token is minted: a remote the worker + // will not clone from should cost nothing and leave nothing. + const credential = credentialFor(repo.cloneUrl, repo); + + // Minted before the directory exists, so a credential this worker cannot produce leaves + // nothing behind either: `mint` throws on a missing `GITHUB_TOKEN`, and between `mkdtemp` + // and the `try` below there is no `catch` to remove what it had already made. + const token = credential === 'github' ? await this.options.tokens.mint() : ''; + + // `mkdtemp` is what makes two tickets unable to share a workspace — the name is chosen by + // the kernel, not by us, so there is no ticket key to collide on and no clean-up race + // between a retry and the attempt it is retrying. + const dir = await mkdtemp(join(this.root, WORKSPACE_PREFIX)); + + try { + await this.git( + [ + ...HOOKS_OFF, + ...(credential === 'github' ? CREDENTIAL_FROM_ENV : []), + 'clone', + '--depth', + '1', + '--single-branch', + '--no-tags', + '--branch', + repo.defaultBranch, + '--', + repo.cloneUrl, + dir, + ], + this.root, + credential === 'github' ? { [TOKEN_ENV]: token } : {} + ); + } catch (err) { + await this.remove(dir); + + // git echoes the remote URL back on failure, credentials included. + throw new Error(redact(err instanceof Error ? err.message : String(err), token)); + } + + this.options.logger.info({ msg: 'workspace cloned', repo: repo.fullName, branch: repo.defaultBranch, dir }); + + return { + dir, + dispose: async (): Promise => { + await this.remove(dir); + }, + }; + } + + /** + * Remove a directory and never reject. + * + * `force: true` is what makes a second `dispose` a no-op rather than an `ENOENT`, and the + * catch is what keeps a cleanup failure from replacing the ticket's real outcome — see + * `Workspace.dispose`. + */ + private async remove(dir: string): Promise { + try { + await rm(dir, { recursive: true, force: true }); + } catch (err) { + this.options.logger.warn({ msg: 'could not remove the workspace', dir, err }); + } + } +} + +export { CloneRefusedError, CloneWorkspace, GITHUB_HOST }; +export type { CloneWorkspaceOptions }; diff --git a/src/vcs/envToken.ts b/src/vcs/envToken.ts new file mode 100644 index 0000000..18a1248 --- /dev/null +++ b/src/vcs/envToken.ts @@ -0,0 +1,75 @@ +/** + * The interim `TokenProvider`, until the GitHub App exists (MAPCO-11428). + * + * `TokenProvider` is deliberately a *function* so that every call mints a fresh, short-lived + * installation token and there is nowhere for a long-lived secret to live. This implementation + * does not honour that — it reads one static token out of the environment — and it exists + * because the alternative is worse in a way that is easy to miss: without any implementation the + * clone, the push and the pull request cannot be constructed at all, so the whole pipeline is + * unreachable and the wiring cannot be shown to work end to end. + * + * So it is here, named for what it is, in a file whose only job is to be deleted. What it does + * buy is that the *shape* is already right everywhere else: the token is minted per call and + * never held, it reaches git through the environment rather than argv, and it is redacted out of + * error messages. Swapping this for the App is one line in `src/worker.ts`. + * + * `GITHUB_TOKEN` is the same variable `RestGitHub` already uses for repo lookups and the same + * one `SECRET_ENV_NAMES` (src/workspace/subprocess.ts) strips out of both the model's process + * and the clone's test run — so a PAT here is not reachable by the code the worker runs on a + * ticket's behalf. + */ + +/** The variable the interim credential is read from. A PAT locally, a token in the cluster. */ +const GITHUB_TOKEN_ENV = 'GITHUB_TOKEN'; + +/** + * A missing GitHub credential. + * + * Its own class for the same reason as `AgentConfigError` and `ConfigError`: it is a deployment + * fault and it must not read as a ticket that failed. + */ +class TokenError extends Error { + public constructor(message: string) { + super(message); + this.name = 'TokenError'; + } +} + +/** + * The GitHub credential, or a thrown `TokenError`. + * + * Called at boot by `createWorker` — where the value is discarded — and again on every mint. The + * boot call is the important one and it is there for the same reason `readModelCredential` is: without a + * credential the worker cannot finish a single ticket, and the failure would otherwise land + * *per ticket*, as a claim, a clone that refuses, a comment naming an environment variable, and + * an attempt counted against somebody's ticket. Two of those retire it from the queue. A pod + * that will not come up is a much cheaper way to learn the Secret did not arrive. + * + * Note that this makes the credential effectively required even for a public repository. That is + * the right trade: a clone can manage without one, but the push cannot, so a tokenless worker + * would fail *after* paying for the model rather than before. + */ +function readGitHubToken(env: NodeJS.ProcessEnv = process.env): string { + const token = env[GITHUB_TOKEN_ENV]?.trim() ?? ''; + + if (token === '') { + throw new TokenError( + `${GITHUB_TOKEN_ENV} must be set — the worker has no other way to clone a repository, push a branch or open a pull request until the GitHub App lands (MAPCO-11428).` + ); + } + + return token; +} + +/** + * Reads the environment on every call, rather than once at construction. + * + * Not laziness: it is the behaviour the App will have, so nothing downstream gets built around a + * credential that is fetched once and cached. The real provider hands out a token that expires + * in an hour, and a run that outlives one mints another. + */ +function envTokenProvider(env: NodeJS.ProcessEnv = process.env): { mint: () => Promise } { + return { mint: async (): Promise => Promise.resolve(readGitHubToken(env)) }; +} + +export { envTokenProvider, GITHUB_TOKEN_ENV, readGitHubToken, TokenError }; diff --git a/src/vcs/gitInvoke.ts b/src/vcs/gitInvoke.ts new file mode 100644 index 0000000..3701f4b --- /dev/null +++ b/src/vcs/gitInvoke.ts @@ -0,0 +1,155 @@ +import { execFile } from 'node:child_process'; +import { devNull } from 'node:os'; +import { promisify } from 'node:util'; + +/** + * How the worker invokes git, and how the push credential reaches it. + * + * Extracted so the two places that run git — cloning a repository in (src/vcs/clone.ts) and + * committing and pushing one out (src/vcs/cliGit.ts) — share one answer to the questions that + * are easy to get subtly wrong: never prompt a human, never put a token in argv, never let the + * target repository's own hooks decide whether the worker's git succeeds, and never echo a + * credential into an error message. Two copies of that would eventually disagree, and the copy + * that drifts is the one nobody is looking at. + */ + +const run = promisify(execFile); + +/** + * Room for git's own output. The default 1 MiB is enough for a push, but `status --porcelain` + * on a large generated diff is not worth failing with `ENOBUFS` over. + */ +const MAX_OUTPUT_BYTES = 10_485_760; +/** What a redacted secret reads as in an error message. */ +const REDACTED = '***'; + +/** + * Never ask a human anything. + * + * git's default is to prompt on a missing or rejected credential, and a prompt on a process + * with no terminal is a process that waits until the timeout rather than failing with a usable + * message. `GIT_TERMINAL_PROMPT=0` turns the prompt into an immediate error; the askpass + * variables are emptied because a developer machine — where `npm run dry-run` runs — often has + * a graphical credential helper configured that would otherwise pop a window nobody sees. + */ +/* eslint-disable @typescript-eslint/naming-convention -- environment variable names, not identifiers */ +const NON_INTERACTIVE: NodeJS.ProcessEnv = { + GIT_TERMINAL_PROMPT: '0', + GIT_ASKPASS: '', + SSH_ASKPASS: '', + GCM_INTERACTIVE: 'never', +}; +/* eslint-enable @typescript-eslint/naming-convention */ + +/** + * The environment variable the push credential is handed over in, and the helper that reads it. + * + * The token used to be interpolated into the push URL, which put it in git's argv — and argv is + * world-readable through `/proc//cmdline`, so any process on the host could read a live + * installation token with org-wide write access straight out of the process table. It did not + * even need to be a process the worker started: the model has `Write`, the verify slice runs + * the clone's own `npm test`, and a rewritten test script therefore gets same-user execution in + * the same container minutes before the push happens (containing that is MAPCO-11430's job, but + * the token need not be reachable for it to matter). + * + * A credential helper is git's own answer to this. The helper is a shell snippet — git runs a + * `!`-prefixed value through `sh -c` with the operation appended — and the snippet contains no + * secret, only the *name* of a variable. The value travels in the child's environment, which + * `/proc//environ` exposes to the process owner alone rather than to everybody. The empty + * `credential.helper=` in front of it resets the helper list, so a helper inherited from a + * developer's global config cannot answer first with a stale credential of its own. + * + * `case` rather than `test "$1" = get &&`, so the snippet exits zero on the `store` and `erase` + * operations git also calls it with instead of looking like a failing helper. + */ +const TOKEN_ENV = 'GIT_AGENT_PUSH_TOKEN'; +/* Exported so that `tests/unit/vcs/scratchRepo.spec.ts` can hand the snippet to the real git and + * watch it answer, rather than asserting that one string equals another string. A shell snippet + * git runs through `sh -c` is not something a mock can tell you is correct. */ +const CREDENTIAL_FROM_ENV = [ + '-c', + 'credential.helper=', + '-c', + `credential.helper=!f() { case "$1" in get) printf 'username=x-access-token\\npassword=%s\\n' "$${TOKEN_ENV}" ;; esac; }; f`, +]; + +/** + * Every git invocation that could fire a hook carries this, and it is the reason the publish + * path cannot be killed by the repository it is working on. + * + * The clone is an arbitrary repository whose hooks the verify slice has just installed for us: + * `npm ci` runs `prepare`, which runs husky, which is how a `pre-commit` running `pretty-quick` + * and a `commit-msg` running `commitlint` end up live in the checkout. Those hooks are a + * contract between that repo and its humans; they are arbitrary code, and any one of them + * exiting non-zero loses the whole result — no commit, no branch, no pull request, no comment on + * the ticket. The repo's real gate on this change is the pull request's own CI plus a human + * review, and both still run. + * + * `--no-verify` was the first attempt and is **not** enough: it bypasses `pre-commit` and + * `commit-msg` only, so a `prepare-commit-msg` hook still runs — and still fails, and can also + * rewrite the header this worker computed. Observed in the scratch repo in + * `tests/unit/vcs/scratchRepo.spec.ts`, which is what a hooks-path of nothing fixes and + * `--no-verify` did not. Pointing `core.hooksPath` at `/dev/null` means git looks for every hook + * inside a path that is not a directory and finds none of them. + * + * None of this excuses writing a header the org's `commit-msg` hook would reject: `naming.ts` + * emits one commitlint accepts, because a squash merge puts that string on the default branch + * where the hook is not bypassed. + */ +const HOOKS_OFF = ['-c', `core.hooksPath=${devNull}`]; + +/** + * Credentials that reached an error message anyway, in `https://user:secret@host` form. + * + * Belt and braces next to redacting the minted token by value. This worker never builds such a + * URL any more — the credential goes to git through the environment — but git echoes back + * whatever remote it was given, a clone URL from GitHub's API could carry credentials, and a + * token in a log line outlives the token's own hour. + */ +const URL_CREDENTIALS = /\/\/[^@/\s]+@/gu; + +/** + * Runs git and resolves its stdout. Injected so the guards can be tested without a checkout. + * + * `env` is additions to the child's environment, not a replacement for it, and exists for one + * reason: it is how the push credential reaches git without ever appearing in its argv. + */ +type RunGit = (args: readonly string[], cwd: string, env?: NodeJS.ProcessEnv) => Promise; + +/** + * A `RunGit` that kills any one invocation after `timeoutMs`. + * + * The bound is per caller because the two callers are not comparable: a commit is a local write + * and a clone of a large repository is a network transfer. What they share is that neither may + * hang forever — `publishPullRequest` is awaited by `handleTicket` which is awaited by + * `runCycle`, so one stuck git invocation stops the scheduler from ever ticking again, and the + * pod stays alive and healthy while the queue silently stops (it is outbound-only, so no probe + * kills it — MAPCO-11430). + */ +function gitRunner(timeoutMs: number): RunGit { + return async (args: readonly string[], cwd: string, env: NodeJS.ProcessEnv = {}): Promise => { + // `execFile`, never `exec`: arguments are passed as an array, so there is no shell to quote + // for and a branch name or a commit message cannot become a second command. + const { stdout } = await run('git', [...args], { + cwd, + maxBuffer: MAX_OUTPUT_BYTES, + timeout: timeoutMs, + // A git that ignored the term signal would otherwise keep the promise pending past the + // timeout, which is the whole failure the timeout exists to prevent. + killSignal: 'SIGKILL', + env: { ...process.env, ...NON_INTERACTIVE, ...env }, + }); + + return stdout; + }; +} + +/** Replace a token, and any credentials in a URL, wherever they appear in a message. */ +function redact(message: string, token: string): string { + const withoutToken = token === '' ? message : message.split(token).join(REDACTED); + + return withoutToken.replace(URL_CREDENTIALS, `//${REDACTED}@`); +} + +export { CREDENTIAL_FROM_ENV, gitRunner, HOOKS_OFF, MAX_OUTPUT_BYTES, NON_INTERACTIVE, redact, REDACTED, TOKEN_ENV }; +export type { RunGit }; diff --git a/src/vcs/types.ts b/src/vcs/types.ts index eb80b25..6c1c7a3 100644 --- a/src/vcs/types.ts +++ b/src/vcs/types.ts @@ -8,6 +8,8 @@ * property of code, not a sentence in an instruction file. */ +import type { Repo } from '../github/types'; + /** Who a commit is authored by. */ interface GitIdentity { /** Author and committer name. For the App this is `{app-slug}[bot]`. */ @@ -24,11 +26,15 @@ interface GitIdentity { * nothing to rotate. A GitHub App installation token lasts an hour; a run that outlives one * mints another rather than holding one open. * - * The implementation is the GitHub App itself, which is MAPCO-11428 and does not exist yet. + * The real implementation is the GitHub App itself, which is MAPCO-11428 and does not exist yet. * Nothing here signs an App JWT — this slice depends on the contract only, so that the * "never a static token" rule is expressed in the type rather than in a README paragraph. - * The same credential is what opens the pull request, which is why the port lives beside git - * rather than beside either caller. + * The same credential is what clones the repository and opens the pull request, which is why the + * port lives beside git rather than beside any one caller. + * + * What is bound to it today is `envTokenProvider` (src/vcs/envToken.ts), which reads one static + * `GITHUB_TOKEN` and therefore honours the shape but not the promise. It exists to be deleted; + * everything around it already treats the credential as short-lived. */ interface TokenProvider { mint: () => Promise; @@ -72,4 +78,39 @@ interface GitPort { push: (branch: string) => Promise; } -export type { GitIdentity, GitPort, TokenProvider }; +/** + * One ticket's clone, and the promise that it goes away again. + * + * `dispose` is separate from `create` rather than a scope-bounded callback because the two are + * not symmetric in the caller: the clone is made once and then handed to three different steps + * (the model, the test runner, the publish path), and a callback would have to wrap all of them + * in one function whose signature grew with every slice. What the caller owes is a `finally`. + */ +interface Workspace { + /** The clone, absolute. This is the `workdir` the model is confined to. */ + readonly dir: string; + /** + * Remove the workspace. Never rejects, and safe to call more than once. + * + * Both properties are there because the only correct place to call this is a `finally`, where + * a rejection would replace the outcome the caller was about to report with a complaint about + * a directory. A cleanup that failed is worth a log line and nothing more; MAPCO-11433's + * "cleaned up on every path" is about the disk, not about the ticket. + */ + dispose: () => Promise; +} + +/** + * Where the worker gets a checkout to work in. + * + * Deliberately not a member of `GitPort`: that port is what the worker does to a checkout it + * already has, and its whole argument is that a capability which does not exist cannot be + * mis-used. Cloning is the act that *creates* the checkout, it happens before there is a + * `GitPort` at all, and it is the one git operation the publish path has no business reaching. + */ +interface WorkspacePort { + /** Clone `repo` at its default branch into a directory nothing else is using. */ + create: (repo: Repo) => Promise; +} + +export type { GitIdentity, GitPort, TokenProvider, Workspace, WorkspacePort }; diff --git a/src/worker.ts b/src/worker.ts new file mode 100644 index 0000000..ebc8094 --- /dev/null +++ b/src/worker.ts @@ -0,0 +1,127 @@ +import type { Logger } from '@map-colonies/js-logger'; +import { budgetOf, type WorkerConfig } from '@common/workerConfig'; +import { readModelCredential } from './agent/credential'; +import { DEFAULT_AGENT_LIMITS } from './agent/implement'; +import { createImplementer } from './agent/implementer'; +import type { AgentLimits } from './agent/types'; +import { createBudgetGuard } from './budget/guard'; +import type { AbortPort, AbortResult } from './budget/types'; +import { ATTEMPT_CAP, type CycleDeps } from './cycle'; +import { createDelivery } from './deliver'; +import { RestGitHub } from './github/restGitHub'; +import type { Repo } from './github/types'; +import { createDescriptionReader } from './jira/description'; +import type { JiraPort, JiraTicket } from './jira/types'; +import { RestPullRequests } from './pr/restPullRequests'; +import { handBackTicket, type HandBackDeps } from './tickets/handBack'; +import { createHandBackOnce, type HandBackOnce } from './tickets/handBackOnce'; +import { CliGit } from './vcs/cliGit'; +import { CloneWorkspace } from './vcs/clone'; +import { envTokenProvider, readGitHubToken } from './vcs/envToken'; +import type { GitPort } from './vcs/types'; + +/** + * The worker, assembled once at boot. + * + * This is the only place in the tree that decides what lives how long, and that is the whole + * reason it exists as a file of its own rather than as a dozen `new`s in `index.ts` — three of + * the lifetimes are load-bearing and two of them have already been the subject of a defect: + * + * - **Per process.** The budget guard, because the daily ticket counter is a property of the day + * and the process is what spans one. The agent, because building it is where the model + * credential is read and a worker with no credential must fail to come up rather than claim a + * ticket and discover it. + * - **Per cycle.** The spend total on the "cycle complete" line. Reached through + * `budget.cycle()` inside `runCycle`, never here — holding it on the process's guard meant the + * figure never reset and a per-cycle field carried the lifetime total. + * - **Per ticket.** The hand-back latch, because "a ticket is handed back at most once" is a + * per-ticket fact, and the metered agent, because it charges to one ticket's ledger. + * + * Everything below is plain construction in the same style as the rest of the worker path: no + * container, no reflection, and the two entry points (`src/index.ts` for the deployment, + * `src/dryRun.ts` for a laptop) differ only in what they do with the result. + */ + +interface WorkerOptions { + /** Jira, which is also the pull-request comment port and the description read. */ + readonly jira: JiraPort; + readonly logger: Logger; + readonly config: WorkerConfig; + /** Read for credentials and scrubbed before anything the worker did not write sees it. */ + readonly env?: NodeJS.ProcessEnv; +} + +/** + * The turn and attempt bounds one hand-off gets, from the configured ceilings. + * + * `maxTurns` is the per-*ticket* ceiling deliberately, not a fraction of it: it is the bound the + * SDK applies to a single hand-off, and no hand-off may be allowed to exceed what the whole + * ticket is permitted. The cumulative total across hand-offs is the ledger's job — that is what + * `meterAgent` charges — so the two together mean `MAX_TURNS_PER_TICKET` bounds both the deepest + * a single hand-off can go and the sum of all of them, which is what the name says. + * + * `maxAttempts` is not configurable and stays at three. It is a judgement about convergence + * rather than a spend ceiling — a model that has not got a suite passing on the third read of the + * same failure is not converging — and the money is already bounded by the two ceilings above. + */ +function limitsFor(config: WorkerConfig): AgentLimits { + return { maxAttempts: DEFAULT_AGENT_LIMITS.maxAttempts, maxTurns: budgetOf(config).ticket.maxTurns }; +} + +function createWorker(options: WorkerOptions): CycleDeps { + const { jira, logger, config, env = process.env } = options; + + const handBackDeps: HandBackDeps = { jira, logger, attemptCap: ATTEMPT_CAP }; + + /** + * The overspend hand-back, bound to `handBackTicket` because both `AbortPort` and `ReleasePort` + * say in as many words to bind to that one rather than write a second release path. + * + * It is the process-wide function rather than a ticket's latch because `createBudgetGuard` is + * itself per process. What keeps the ticket from being handed back twice is the other + * direction: `meterAgent` passes this call's result to the ticket's latch (`record`), so the + * give-up that follows writes nothing. `HandBackOutcome` is already an `AbortResult`. + */ + const abort: AbortPort = { + abort: async (ticket: JiraTicket, note: string): Promise => handBackTicket(ticket, note, handBackDeps), + }; + + // Both credentials are read here, at boot, and both values are deliberately thrown away — + // nothing in this function holds one. The model credential is checked first because its refusal + // is the more specific one (it names the `MODEL_AUTH` mode and the variable that mode reads), + // and both are checked at all because the alternative is discovering a missing Secret *per + // ticket*: a claim, a comment naming an environment variable, and an attempt counted against + // somebody's ticket, twice over. + readModelCredential(env); + readGitHubToken(env); + + const tokens = envTokenProvider(env); + + const delivery = createDelivery({ + github: new RestGitHub(env['GITHUB_TOKEN']), + workspaces: new CloneWorkspace({ root: config.workspaceRoot, tokens, logger }), + implementer: createImplementer({ + logger, + description: createDescriptionReader(jira), + limits: limitsFor(config), + env, + ...(config.model === undefined ? {} : { model: config.model }), + }), + pullRequests: new RestPullRequests(tokens), + tickets: jira, + git: ({ cwd, repo }: { cwd: string; repo: Repo }): GitPort => new CliGit({ cwd, repo, identity: config.commitIdentity, tokens }), + logger, + }); + + return { + jira, + logger, + config, + delivery, + budget: createBudgetGuard({ budget: budgetOf(config), abort, logger }), + handBack: (): HandBackOnce => createHandBackOnce(handBackDeps), + }; +} + +export { createWorker, limitsFor }; +export type { WorkerOptions }; diff --git a/tests/helpers/fakePipeline.ts b/tests/helpers/fakePipeline.ts new file mode 100644 index 0000000..a790a21 --- /dev/null +++ b/tests/helpers/fakePipeline.ts @@ -0,0 +1,240 @@ +import type { Logger } from '@map-colonies/js-logger'; +import type { ImplementDeps } from '@src/agent/implement'; +import type { Implementer } from '@src/agent/implementer'; +import type { AgentLimits, AgentPort, AgentRun, AgentRunRequest, DescriptionPort } from '@src/agent/types'; +import type { GitHubPort, Repo } from '@src/github/types'; +import type { PullRequest, PullRequestDraft, PullRequestPort } from '@src/pr/types'; +import type { GitPort, Workspace, WorkspacePort } from '@src/vcs/types'; +import type { TestPlan, TestPlanResult, TestRun, TestRunner } from '@src/workspace/types'; + +/** + * Doubles for the edges of the pipeline, and nothing inside it. + * + * Everything faked here is a thing outside the process: GitHub's API, the model, the git binary, + * the clone on disk, a repository's own test suite. Everything between them — `runCycle`, the + * claim, the budget guard, the hand-back latch, `implementTicket`, `publishPullRequest` — is the + * real code in the integration suite, because those are what the wiring is about. A fake of any + * of them would be a fake that agrees with the wiring. + * + * Each double records what it was asked for, so a spec can assert on observable outcomes — which + * is the convention MAPCO-11434 asks for by name: Jira mutations, workspace state, claimed or + * released, never on an internal call count for its own sake. + */ + +const DEFAULT_TEST_COMMAND = 'npm run test:ci'; + +function repo(overrides: Partial = {}): Repo { + return { + name: 'some-service', + fullName: 'MapColonies/some-service', + defaultBranch: 'master', + cloneUrl: 'https://github.com/MapColonies/some-service.git', + ...overrides, + }; +} + +/** GitHub's repo lookup. Answers for the names it was given and `null` for anything else. */ +function fakeGitHub(repos: readonly Repo[] = [repo()]): GitHubPort & { looked: string[] } { + const looked: string[] = []; + const known = new Map(repos.map((known) => [known.name.toLowerCase(), known])); + + return { + looked, + findRepo: async (name: string): Promise => { + looked.push(name); + + return Promise.resolve(known.get(name.toLowerCase()) ?? null); + }, + }; +} + +interface FakeWorkspaces extends WorkspacePort { + /** Repos a clone was asked for, in order. */ + readonly cloned: Repo[]; + /** Directories that were disposed of. Every created one must end up here. */ + readonly disposed: string[]; + /** Directories created and not yet disposed. Non-empty at the end of a cycle is a leak. */ + readonly live: () => string[]; +} + +/** + * The clone, as a directory name and a promise to forget it. + * + * No real checkout: the agent and the test runner are doubles too, so nothing in the integration + * suite reads the directory. What the suite does assert is that one was made for the right repo, + * that the model was pointed at it, and that it was disposed of on every path — which is + * MAPCO-11433's acceptance criterion and needs only the bookkeeping. The real `git clone` has its + * own spec against a real bare repository (tests/unit/vcs/clone.spec.ts). + */ +function fakeWorkspaces(options: { failWith?: Error } = {}): FakeWorkspaces { + const cloned: Repo[] = []; + const disposed: string[] = []; + const made: string[] = []; + + return { + cloned, + disposed, + live: (): string[] => made.filter((dir) => !disposed.includes(dir)), + create: async (target: Repo): Promise => { + cloned.push(target); + + if (options.failWith) { + throw options.failWith; + } + + const dir = `/workspace/${target.name}-${made.length}`; + made.push(dir); + + return Promise.resolve({ + dir, + dispose: async (): Promise => { + disposed.push(dir); + + return Promise.resolve(); + }, + }); + }, + }; +} + +function agentRun(overrides: Partial = {}): AgentRun { + return { + outcome: 'changed', + usage: { input: 1_000, output: 200, cacheRead: 0, costUsd: 0.05 }, + turns: 6, + summary: 'added the retry', + deniedTools: [], + ...overrides, + }; +} + +/** + * The model. Hands out queued runs and repeats the last one rather than running dry, so a spec + * that only cares about the first hand-off need not queue three. + */ +function fakeAgent(runs: readonly AgentRun[] = [agentRun()]): AgentPort & { requests: AgentRunRequest[] } { + const requests: AgentRunRequest[] = []; + const queue = [...runs]; + + return { + requests, + run: async (request: AgentRunRequest): Promise => { + requests.push(request); + + return Promise.resolve((queue.length > 1 ? queue.shift() : queue[0]) ?? agentRun()); + }, + }; +} + +const plan: TestPlan = { + script: 'test:ci', + command: DEFAULT_TEST_COMMAND, + executed: { 'test:ci': 'vitest run' }, + dependencies: '[]', +}; + +/** A repository's own suite. Repeats its last answer, for the same reason `fakeAgent` does. */ +function fakeTests( + runs: readonly TestRun[] = [{ ok: true, command: DEFAULT_TEST_COMMAND, output: 'all green' }], + planned: TestPlanResult = { ok: true, plan } +): TestRunner & { dirs: string[] } { + const dirs: string[] = []; + const queue = [...runs]; + + return { + dirs, + plan: async (): Promise => Promise.resolve(planned), + run: async (dir: string): Promise => { + dirs.push(dir); + + return Promise.resolve((queue.length > 1 ? queue.shift() : queue[0]) ?? { ok: true, command: DEFAULT_TEST_COMMAND, output: '' }); + }, + }; +} + +interface FakeGit extends GitPort { + readonly branches: string[]; + readonly commits: { message: string; paths: readonly string[] }[]; + readonly pushes: string[]; +} + +/** git inside the clone, without a checkout. The real guards have their own specs. */ +function fakeGit(options: { cwd: string; changed?: readonly string[]; failWith?: Error }): FakeGit { + const branches: string[] = []; + const commits: { message: string; paths: readonly string[] }[] = []; + const pushes: string[] = []; + + return { + root: options.cwd, + branches, + commits, + pushes, + changedFiles: async (): Promise => Promise.resolve(options.changed ?? ['src/fetch.ts']), + createBranch: async (branch: string): Promise => { + branches.push(branch); + + return Promise.resolve(); + }, + commit: async (message: string, paths: readonly string[]): Promise => { + commits.push({ message, paths }); + + return Promise.resolve('c0ffee1'); + }, + push: async (branch: string): Promise => { + if (options.failWith) { + throw options.failWith; + } + + pushes.push(branch); + + return Promise.resolve(); + }, + }; +} + +/** GitHub's pull-request API. */ +function fakePullRequests(options: { failWith?: Error } = {}): PullRequestPort & { opened: { repo: Repo; draft: PullRequestDraft }[] } { + const opened: { repo: Repo; draft: PullRequestDraft }[] = []; + + return { + opened, + open: async (target: Repo, draft: PullRequestDraft): Promise => { + if (options.failWith) { + throw options.failWith; + } + + opened.push({ repo: target, draft }); + + return Promise.resolve({ number: 42, url: `https://github.com/${target.fullName}/pull/42` }); + }, + }; +} + +/** + * The implement step, assembled from doubles. + * + * Mirrors what `createImplementer` does — including applying the per-ticket `meter` — rather than + * calling it, because that function's whole job is to construct the *real* agent from the pod's + * model credential. That it wires the meter and the hand-back through correctly is asserted in + * `tests/unit/agent/implementer.spec.ts`; what the integration suite needs is the same shape with + * a model that costs nothing. + */ +function fakeImplementer(parts: { + agent: AgentPort; + tests: TestRunner; + description: DescriptionPort; + logger: Logger; + limits: AgentLimits; +}): Implementer { + return (scope): ImplementDeps => ({ + agent: scope.meter === undefined ? parts.agent : scope.meter(parts.agent), + tests: parts.tests, + description: parts.description, + release: scope.release, + logger: parts.logger, + limits: parts.limits, + }); +} + +export { agentRun, DEFAULT_TEST_COMMAND, fakeAgent, fakeGit, fakeGitHub, fakeImplementer, fakePullRequests, fakeTests, plan, repo, fakeWorkspaces }; +export type { FakeGit, FakeWorkspaces }; diff --git a/tests/integration/cycle.spec.ts b/tests/integration/cycle.spec.ts index cded6c7..3543781 100644 --- a/tests/integration/cycle.spec.ts +++ b/tests/integration/cycle.spec.ts @@ -1,12 +1,37 @@ import { describe, expect, it } from 'vitest'; +import { DEFAULT_AGENT_LIMITS } from '@src/agent/implement'; +import type { AgentRun } from '@src/agent/types'; +import { createBudgetGuard } from '@src/budget/guard'; +import type { AbortPort, AbortResult, BudgetConfig } from '@src/budget/types'; import { runCycle, type CycleDeps } from '@src/cycle'; +import { createDelivery } from '@src/deliver'; +import type { Repo } from '@src/github/types'; +import { createDescriptionReader } from '@src/jira/description'; +import type { JiraTicket, JiraTransition } from '@src/jira/types'; +import { handBackTicket } from '@src/tickets/handBack'; +import { createHandBackOnce, type HandBackOnce } from '@src/tickets/handBackOnce'; +import type { WorkerConfig } from '@src/common/workerConfig'; +import type { TestRun } from '@src/workspace/types'; import { FakeJira, ticket, type FakeWrite } from '@tests/helpers/fakeJira'; import { fakeLogger, type RecordedLine } from '@tests/helpers/fakeLogger'; -import type { WorkerConfig } from '@src/common/workerConfig'; -import type { JiraTransition } from '@src/jira/types'; +import { + agentRun, + DEFAULT_TEST_COMMAND, + fakeAgent, + fakeGit, + fakeGitHub, + fakeImplementer, + fakePullRequests, + fakeTests, + fakeWorkspaces, + repo, + type FakeGit, + type FakeWorkspaces, +} from '@tests/helpers/fakePipeline'; const BOT_ACCOUNT = 'developer-agent@mapcolonies.example'; const BOT_DISPLAY_NAME = 'AGENT DEVELOPER'; +const DESCRIPTION = 'The fetch helper gives up on the first 503. Make it retry three times.'; const baseConfig: WorkerConfig = { pollIntervalMs: 1000, @@ -14,6 +39,8 @@ const baseConfig: WorkerConfig = { maxConcurrentTickets: 1, mcpUrl: 'http://mcp.invalid', bot: { account: BOT_ACCOUNT, displayName: BOT_DISPLAY_NAME }, + workspaceRoot: '/workspace', + commitIdentity: { name: 'developer-agent[bot]', email: 'developer-agent[bot]@users.noreply.github.com' }, }; const displayNames = { [BOT_ACCOUNT]: BOT_DISPLAY_NAME }; @@ -31,180 +58,589 @@ function workflowFor(...keys: string[]): Record { ); } -function makeCycle(jira: FakeJira, overrides: Partial = {}): { deps: CycleDeps; lines: RecordedLine[] } { +/** A ticket the pipeline can actually work: a resolvable repo prefix and prose to work from. */ +function workableTicket(overrides: Partial = {}): JiraTicket { + return ticket({ summary: 'some-service: retry the fetch helper', description: DESCRIPTION, ...overrides }); +} + +interface Pipeline { + readonly deps: CycleDeps; + readonly lines: RecordedLine[]; + readonly jira: FakeJira; + readonly workspaces: FakeWorkspaces; + readonly github: ReturnType; + readonly agent: ReturnType; + readonly tests: ReturnType; + readonly pullRequests: ReturnType; + /** The git doubles handed out, one per clone. */ + readonly gits: FakeGit[]; +} + +interface PipelineOptions { + readonly config?: Partial; + readonly repos?: readonly Repo[]; + readonly runs?: readonly AgentRun[]; + readonly testRuns?: readonly TestRun[]; + readonly noTestCommand?: boolean; + readonly cloneFailsWith?: Error; + readonly pushFailsWith?: Error; + readonly openFailsWith?: Error; + readonly changed?: readonly string[]; + readonly budget?: BudgetConfig; +} + +/** + * The whole worker, with doubles only at the edges. + * + * `runCycle`, the claim, the budget guard, the hand-back latch, `implementTicket` and + * `publishPullRequest` are the real thing — this is the seam the repository's test strategy is + * built around, and the point of these cases is that the real modules compose. + */ +function pipeline(jira: FakeJira, options: PipelineOptions = {}): Pipeline { const { logger, lines } = fakeLogger(); + const config: WorkerConfig = { ...baseConfig, ...options.config }; + + const github = fakeGitHub(options.repos ?? [repo()]); + const workspaces = fakeWorkspaces(options.cloneFailsWith === undefined ? {} : { failWith: options.cloneFailsWith }); + const agent = fakeAgent(options.runs ?? [agentRun()]); + const tests = fakeTests( + options.testRuns ?? [{ ok: true, command: DEFAULT_TEST_COMMAND, output: 'all green' }], + options.noTestCommand === true ? { ok: false, reason: 'no-command', output: 'no test script' } : undefined + ); + const pullRequests = fakePullRequests(options.openFailsWith === undefined ? {} : { failWith: options.openFailsWith }); + const gits: FakeGit[] = []; + + const handBackDeps = { jira, logger, attemptCap: 2 }; + const abort: AbortPort = { + abort: async (target: JiraTicket, note: string): Promise => handBackTicket(target, note, handBackDeps), + }; + + const delivery = createDelivery({ + github, + workspaces, + implementer: fakeImplementer({ agent, tests, description: createDescriptionReader(jira), logger, limits: DEFAULT_AGENT_LIMITS }), + pullRequests, + tickets: jira, + git: ({ cwd }): FakeGit => { + const git = fakeGit({ + cwd, + ...(options.changed === undefined ? {} : { changed: options.changed }), + ...(options.pushFailsWith === undefined ? {} : { failWith: options.pushFailsWith }), + }); + gits.push(git); + + return git; + }, + logger, + }); - return { deps: { jira, logger, config: { ...baseConfig, ...overrides } }, lines }; + const deps: CycleDeps = { + jira, + logger, + config, + delivery, + budget: createBudgetGuard({ + budget: options.budget ?? { ticket: { maxTokens: 1_000_000, maxTurns: 1_000 }, maxTicketsPerDay: 5 }, + abort, + logger, + }), + handBack: (): HandBackOnce => createHandBackOnce(handBackDeps), + }; + + return { deps, lines, jira, workspaces, github, agent, tests, pullRequests, gits }; } function kindsOf(writes: FakeWrite[]): string[] { return writes.map((write) => write.kind); } +function commentsOn(writes: FakeWrite[]): string[] { + return writes.filter((write) => write.kind === 'comment').map((write) => write.body); +} + +function labelWrites(writes: FakeWrite[]): readonly string[][] { + return writes.filter((write) => write.kind === 'labels').map((write) => [...write.labels]); +} + /** The order tickets appear in the write log, collapsed — `[a, b]` means a finished before b started. */ function ticketRuns(writes: FakeWrite[]): string[] { return writes.map((write) => write.key).filter((key, index, keys) => key !== keys[index - 1]); } +function runLine(lines: RecordedLine[]): Record { + return lines.filter((line) => line.payload.msg === 'cycle complete').at(-1)?.payload ?? {}; +} + describe('runCycle', () => { - it('should claim the ticket it found and hand it straight back.', async () => { - const jira = new FakeJira({ tickets: [ticket({ key: 'MAPCO-100' })], transitions: workflowFor('MAPCO-100'), displayNames }); - const { deps, lines } = makeCycle(jira); - - const result = await runCycle(deps); - - expect(result).toMatchObject({ found: 1, started: 1, skipped: 0, outcome: 'ok' }); - expect(jira.writes).toEqual([ - { kind: 'assign', key: 'MAPCO-100', assignee: BOT_ACCOUNT }, - { kind: 'transition', key: 'MAPCO-100', transitionId: '21' }, - { kind: 'comment', key: 'MAPCO-100', body: expect.stringContaining('MAPCO-11431') as unknown as string }, - { kind: 'transition', key: 'MAPCO-100', transitionId: '11' }, - { kind: 'assign', key: 'MAPCO-100', assignee: null }, - ]); - expect(lines.at(-1)?.payload).toMatchObject({ found: 1, started: 1, keys: ['MAPCO-100'] }); - }); + describe('the whole pipeline', () => { + it('should turn an agent-ready ticket into a pull request linked on the ticket.', async () => { + // MAPCO-11436's Expected Result, through the seam: a ticket goes in and a reviewable pull + // request comes out, on an `agent/`-prefixed branch, linked from a comment. + const jira = new FakeJira({ tickets: [workableTicket({ key: 'MAPCO-100' })], transitions: workflowFor('MAPCO-100'), displayNames }); + const { deps, pullRequests, gits, lines } = pipeline(jira); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ found: 1, started: 1, skipped: 0, outcome: 'ok' }); + expect(gits[0]?.pushes).toStrictEqual(['agent/chore/MAPCO-100-retry-the-fetch-helper']); + expect(pullRequests.opened[0]?.draft).toMatchObject({ + head: 'agent/chore/MAPCO-100-retry-the-fetch-helper', + base: 'master', + title: 'chore: retry the fetch helper (MAPCO-100)', + }); + expect(commentsOn(jira.writes).at(-1)).toContain('https://github.com/MapColonies/some-service/pull/42'); + expect(lines.some((line) => line.payload.msg === 'pull request opened for ticket')).toBe(true); + }); - it('should leave the ticket unassigned again, so the next run can pick it up.', async () => { - const jira = new FakeJira({ tickets: [ticket()], transitions: workflowFor('MAPCO-1'), displayNames }); - const { deps } = makeCycle(jira); + it('should hand the ticket to the model inside the clone, with the prose a human wrote.', async () => { + // MAPCO-11434's "the ticket is handed to the Agent SDK as the task, working inside the + // clone" — which is the wiring itself, and which nothing could assert before. + const jira = new FakeJira({ tickets: [workableTicket({ key: 'MAPCO-101' })], transitions: workflowFor('MAPCO-101'), displayNames }); + const { deps, agent, workspaces } = pipeline(jira); - await runCycle(deps); + await runCycle(deps); - await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: null }); - }); + expect(agent.requests[0]?.task).toStrictEqual({ + key: 'MAPCO-101', + summary: 'some-service: retry the fetch helper', + description: DESCRIPTION, + }); + expect(agent.requests[0]?.workdir).toBe(workspaces.cloned.length > 0 ? `/workspace/some-service-0` : ''); + }); - it('should not touch a ticket that turns out to be assigned by the time it is reached.', async () => { - // The poll query filters `assignee is EMPTY`, so this is the snapshot-went-stale case - // rather than something the query would hand over — it exercises the guard before the write. - const jira = new FakeJira({ tickets: [ticket({ assignee: 'BROCHSTEIN RAZ' })], transitions: workflowFor('MAPCO-1'), displayNames }); - const { deps } = makeCycle(jira); + it('should clone the repository the title names, at the branch GitHub calls default.', async () => { + // MAPCO-11433: the repo comes from the `: ` prefix and is confirmed to exist. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, github, workspaces } = pipeline(jira, { repos: [repo({ defaultBranch: 'main' })] }); - const result = await runCycle(deps); + await runCycle(deps); - expect(result).toMatchObject({ found: 1, started: 0, skipped: 1 }); - expect(jira.writes).toEqual([]); - }); + expect(github.looked).toStrictEqual(['some-service']); + expect(workspaces.cloned).toStrictEqual([repo({ defaultBranch: 'main' })]); + }); + + it('should run the suite in the clone before anything is pushed.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, tests, workspaces } = pipeline(jira); + + await runCycle(deps); - it('should back off cleanly when a human claims the ticket mid-claim.', async () => { - const jira = new FakeJira({ - tickets: [ticket()], - transitions: workflowFor('MAPCO-1'), - displayNames, - stealOnAssign: 'BROCHSTEIN RAZ', + expect(tests.dirs).toStrictEqual(['/workspace/some-service-0']); + expect(workspaces.disposed).toStrictEqual(['/workspace/some-service-0']); }); - const { deps, lines } = makeCycle(jira); - const result = await runCycle(deps); + it('should say in the pull request what it actually ran.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, pullRequests } = pipeline(jira); - expect(result).toMatchObject({ started: 0, skipped: 1, outcome: 'ok' }); - // The assign is already out there; the point is that nothing follows it — no transition, - // no comment, and no attempt to take it back off the human. - expect(kindsOf(jira.writes)).toEqual(['assign']); - expect(lines.some((line) => line.level === 'warn' && line.payload.reason === 'lost-race' && line.payload.saw === 'BROCHSTEIN RAZ')).toBe(true); - }); + await runCycle(deps); + + expect(pullRequests.opened[0]?.draft.body).toContain(DEFAULT_TEST_COMMAND); + }); - it('should report the transitions a workflow did offer when it cannot claim.', async () => { - // The real transition vocabulary is unverified, so a refusal has to say what it saw - // rather than leaving a silent no-op to be discovered by a drained queue. - const jira = new FakeJira({ tickets: [ticket()], transitions: { 'MAPCO-1': [{ id: '31', name: 'Reject', to: 'Rejected' }] }, displayNames }); - const { deps, lines } = makeCycle(jira); + it('should keep hold of a ticket whose pull request is open, rather than releasing it.', async () => { + // The work is done and a human is reviewing it. Releasing would put the ticket back in the + // poll's way and let the worker do it all again. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira); - const result = await runCycle(deps); + await runCycle(deps); - expect(result).toMatchObject({ started: 0, skipped: 1 }); - expect(lines.some((line) => line.payload.reason === 'no-transition' && (line.payload.offered as string[])[0] === 'Reject')).toBe(true); - }); + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: BOT_DISPLAY_NAME }); + expect(labelWrites(jira.writes)).toStrictEqual([]); + }); - it('should honour the per-run ticket limit and say there is more waiting.', async () => { - const keys = ['MAPCO-1', 'MAPCO-2', 'MAPCO-3']; - const jira = new FakeJira({ tickets: keys.map((key) => ticket({ key })), transitions: workflowFor(...keys), displayNames }); - const { deps } = makeCycle(jira, { maxTicketsPerRun: 2, maxConcurrentTickets: 2 }); + it('should clean the workspace up on every path, including a failed push.', async () => { + // MAPCO-11433's "the workspace is cleaned up on every path". A publish failure throws by + // design, and a worker that leaked a clone per thrown ticket would fill the volume. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, workspaces } = pipeline(jira, { pushFailsWith: new Error('remote hung up') }); - const result = await runCycle(deps); + await runCycle(deps); - expect(result.found).toBe(2); - expect(result.started).toBe(2); - expect(result.more).toBe(true); - // One over the limit, because the server's `total` is always -1 and cannot be trusted. - expect(jira.queries[0]?.limit).toBe(3); - expect(jira.writes.some((write) => write.key === 'MAPCO-3')).toBe(false); + expect(workspaces.live()).toStrictEqual([]); + }); }); - it('should hold tickets to one at a time at the default concurrency.', async () => { - const keys = ['MAPCO-1', 'MAPCO-2']; - const jira = new FakeJira({ tickets: keys.map((key) => ticket({ key })), transitions: workflowFor(...keys), displayNames }); - const { deps } = makeCycle(jira, { maxTicketsPerRun: 2, maxConcurrentTickets: 1 }); + describe('refusing a ticket it cannot work', () => { + it('should refuse a title that names no repository, without cloning anything.', async () => { + const jira = new FakeJira({ tickets: [ticket({ summary: 'make the thing faster' })], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, workspaces } = pipeline(jira); - await runCycle(deps); + await runCycle(deps); - // Each ticket is finished with before the next is touched: the cap is what stops the - // worker holding a whole page of tickets at once. - expect(ticketRuns(jira.writes)).toEqual(['MAPCO-1', 'MAPCO-2']); - }); + expect(workspaces.cloned).toStrictEqual([]); + expect(commentsOn(jira.writes)[0]).toContain('Could not tell which repository this ticket is about'); + }); - it('should overlap tickets once concurrency is raised.', async () => { - const keys = ['MAPCO-1', 'MAPCO-2']; - const jira = new FakeJira({ tickets: keys.map((key) => ticket({ key })), transitions: workflowFor(...keys), displayNames }); - const { deps } = makeCycle(jira, { maxTicketsPerRun: 2, maxConcurrentTickets: 2 }); + it('should count the attempt when it refuses, so the same ticket is not re-burnt for ever.', async () => { + const jira = new FakeJira({ tickets: [ticket({ summary: 'no prefix here' })], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira); - await runCycle(deps); + await runCycle(deps); - // Interleaved rather than one-then-the-other, which is the difference the cap makes. - expect(ticketRuns(jira.writes).length).toBeGreaterThan(2); - }); + expect(labelWrites(jira.writes)).toStrictEqual([['agent-ready', 'agent-attempted-1']]); + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: null }); + }); - it('should not claim there is more waiting when the queue is exhausted.', async () => { - const jira = new FakeJira({ tickets: [ticket()], transitions: workflowFor('MAPCO-1'), displayNames }); - const { deps } = makeCycle(jira); + it('should refuse a repository that does not exist, naming what it looked for.', async () => { + const jira = new FakeJira({ + tickets: [workableTicket({ summary: 'ghost-service: do the thing' })], + transitions: workflowFor('MAPCO-1'), + displayNames, + }); + const { deps } = pipeline(jira); - expect((await runCycle(deps)).more).toBe(false); - }); + await runCycle(deps); + + expect(commentsOn(jira.writes)[0]).toContain('ghost-service'); + }); - it('should report an empty queue rather than treating it as a failure.', async () => { - const jira = new FakeJira({ tickets: [] }); - const { deps, lines } = makeCycle(jira); + it('should refuse a ticket with no description before the model is paid for.', async () => { + // The blocker this wiring had to close: `DescriptionPort` had no implementation, so the + // honest stub answered `''` for every ticket and `implementTicket` refused all of them. + const jira = new FakeJira({ tickets: [workableTicket({ description: '' })], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, agent } = pipeline(jira); - const result = await runCycle(deps); + await runCycle(deps); - expect(result).toMatchObject({ found: 0, started: 0, outcome: 'ok' }); - expect(lines[0]?.level).toBe('info'); - }); + expect(agent.requests).toStrictEqual([]); + expect(commentsOn(jira.writes)[0]).toContain('This ticket has no description'); + }); + + it('should work a ticket that does have a description, which is the case that used to fail.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, agent } = pipeline(jira); + + await runCycle(deps); + + expect(agent.requests).toHaveLength(1); + }); + + it('should refuse a repository that states no test command, rather than pushing unverified work.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, agent, pullRequests } = pipeline(jira, { noTestCommand: true }); + + await runCycle(deps); + + expect(agent.requests).toStrictEqual([]); + expect(pullRequests.opened).toStrictEqual([]); + expect(commentsOn(jira.writes)[0]).toContain('could not be verified in this repository'); + }); + + it('should hand a ticket back when the repository cannot be cloned, and say it may not be the ticket.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira, { cloneFailsWith: new Error('repository not found') }); - it('should survive a poll failure and report it, so the schedule keeps running.', async () => { - const jira = new FakeJira({ failWith: new Error('mcp unreachable') }); - const { deps, lines } = makeCycle(jira); + await runCycle(deps); - const result = await runCycle(deps); + expect(commentsOn(jira.writes)[0]).toContain('could not be cloned'); + expect(commentsOn(jira.writes)[0]).toContain('may be the repository or the network rather than the ticket'); + expect(labelWrites(jira.writes)).toStrictEqual([['agent-ready', 'agent-attempted-1']]); + }); + + it('should take the existing release path when the suite keeps failing.', async () => { + // MAPCO-11434's "repeated test failure, after a bounded number of attempts, takes the + // existing release path" — comment, release, bump the attempt count. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, agent, pullRequests } = pipeline(jira, { + testRuns: [{ ok: false, reason: 'failed', command: DEFAULT_TEST_COMMAND, output: '1 failed' }], + }); + + await runCycle(deps); + + expect(agent.requests).toHaveLength(DEFAULT_AGENT_LIMITS.maxAttempts); + expect(pullRequests.opened).toStrictEqual([]); + expect(commentsOn(jira.writes)[0]).toContain('did not pass'); + expect(labelWrites(jira.writes)).toStrictEqual([['agent-ready', 'agent-attempted-1']]); + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: null }); + }); + + it('should open no pull request when the verified tree has nothing in it.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, pullRequests } = pipeline(jira, { changed: [] }); + + await runCycle(deps); + + expect(pullRequests.opened).toStrictEqual([]); + expect(commentsOn(jira.writes)[0]).toContain('no change to offer'); + }); + + it('should hand a held ticket back when the publish throws, rather than holding it for ever.', async () => { + // There is no boot-time orphan sweep yet (MAPCO-11432), so a ticket dropped here would stay + // assigned to the bot and invisible to the poll — lost rather than contained. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, lines } = pipeline(jira, { openFailsWith: new Error('422 unprocessable') }); - expect(result.outcome).toBe('failed'); - expect(lines[0]?.level).toBe('error'); + const result = await runCycle(deps); + + expect(result).toMatchObject({ started: 1 }); + expect(lines.some((line) => line.level === 'error' && line.payload.msg === 'ticket failed')).toBe(true); + expect(commentsOn(jira.writes)[0]).toContain('stopped part-way through with an error'); + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: null }); + }); }); - it('should keep hold of a ticket it cannot return to Open, and say so loudly.', async () => { - const jira = new FakeJira({ - tickets: [ticket()], - transitions: { 'MAPCO-1': [{ id: '21', name: 'Start Progress', to: 'In Progress' }] }, - displayNames, + describe('spend ceilings', () => { + it('should stop a ticket that runs out of its token budget and say what it cost.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, pullRequests } = pipeline(jira, { + budget: { ticket: { maxTokens: 500, maxTurns: 1_000 }, maxTicketsPerDay: 5 }, + }); + + await runCycle(deps); + + expect(pullRequests.opened).toStrictEqual([]); + expect(commentsOn(jira.writes)[0]).toContain('ran out of the budget I am allowed to spend on one ticket'); + expect(commentsOn(jira.writes)[0]).toContain('1,200 tokens'); + }); + + it('should hand an overspent ticket back exactly once, however many give-up paths fire.', async () => { + // The invariant `createHandBackOnce` exists for: the budget aborts mid-hand-off and the + // stopped run is then reported as a give-up, which would otherwise comment a second time on + // a ticket already back in Open that a human may have picked up. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira, { budget: { ticket: { maxTokens: 500, maxTurns: 1_000 }, maxTicketsPerDay: 5 } }); + + await runCycle(deps); + + expect(commentsOn(jira.writes)).toHaveLength(1); + expect(labelWrites(jira.writes)).toHaveLength(1); + expect(jira.writes.filter((write) => write.kind === 'assign' && write.assignee === null)).toHaveLength(1); }); - const { deps, lines } = makeCycle(jira); - const result = await runCycle(deps); + it('should stop a ticket that runs out of its turn budget, counted in model turns.', async () => { + // Not one turn per hand-off: the run reports six, and a ceiling of five must stop it. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira, { budget: { ticket: { maxTokens: 1_000_000, maxTurns: 5 }, maxTicketsPerDay: 5 } }); - // It did hold the ticket, so it counts as started — but it is now stuck, and the run - // line has to make that findable. - expect(result).toMatchObject({ started: 1, outcome: 'ok' }); - await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: BOT_DISPLAY_NAME }); - expect(lines.some((line) => line.level === 'error' && line.payload.msg === 'held ticket could not be released')).toBe(true); + await runCycle(deps); + + expect(commentsOn(jira.writes)[0]).toContain('6 turns'); + }); + + it('should poll and start nothing once the day is used up.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, lines } = pipeline(jira, { budget: { ticket: { maxTokens: 1_000_000, maxTurns: 1_000 }, maxTicketsPerDay: 1 } }); + + await runCycle(deps); + const second = await runCycle(deps); + + expect(second).toMatchObject({ found: 1, started: 0, skipped: 1, outcome: 'ok' }); + expect(runLine(lines)).toMatchObject({ dailyCapReached: true, ticketsStartedToday: 1 }); + }); + + it('should write nothing at all on a run that has hit the daily cap.', async () => { + // A ticket claimed and then dropped for a spend ceiling has already put a bot's name on a + // human's ticket and notified its watchers. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira, { budget: { ticket: { maxTokens: 1_000_000, maxTurns: 1_000 }, maxTicketsPerDay: 1 } }); + + await runCycle(deps); + const before = jira.writes.length; + await runCycle(deps); + + expect(jira.writes).toHaveLength(before); + }); + + it("should report the cycle's own spend on the run line, not a hard-coded zero.", async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, lines } = pipeline(jira); + + await runCycle(deps); + + expect(runLine(lines)).toMatchObject({ tokensSpent: 1_200, turnsSpent: 6, ticketsStartedToday: 1, dailyCapLimit: 5 }); + }); + + it("should report each cycle's spend separately, not the process's running total.", async () => { + // The defect a single long-lived guard caused: the spend never reset, so a per-cycle field + // carried the lifetime total and any sum over those lines double-counted. The two counters + // on the same line have deliberately different lifetimes — the spend is the cycle's, the + // daily count is the process's — so a second cycle that starts nothing has to show one + // reset and the other not. + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps, lines } = pipeline(jira); + + await runCycle(deps); + + expect(runLine(lines)).toMatchObject({ tokensSpent: 1_200, ticketsStartedToday: 1 }); + + // The ticket is still held, with its pull request open, so this cycle claims nothing. + await runCycle(deps); + + expect(runLine(lines)).toMatchObject({ tokensSpent: 0, turnsSpent: 0, ticketsStartedToday: 1 }); + }); }); - it('should let one broken ticket fail without taking the run down with it.', async () => { - const keys = ['MAPCO-1', 'MAPCO-2']; - const jira = new FakeJira({ tickets: keys.map((key) => ticket({ key })), transitionsFailWith: new Error('jira exploded'), displayNames }); - const { deps, lines } = makeCycle(jira, { maxTicketsPerRun: 2 }); + describe('claiming and the poll', () => { + it('should leave a refused ticket unassigned again, so the next run can pick it up.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira); + + await runCycle(deps); + + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: null }); + }); + + it('should not touch a ticket that turns out to be assigned by the time it is reached.', async () => { + // The poll query filters `assignee is EMPTY`, so this is the snapshot-went-stale case + // rather than something the query would hand over — it exercises the guard before the write. + const jira = new FakeJira({ tickets: [workableTicket({ assignee: 'BROCHSTEIN RAZ' })], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ found: 1, started: 0, skipped: 1 }); + expect(jira.writes).toStrictEqual([]); + }); + + it('should back off cleanly when a human claims the ticket mid-claim.', async () => { + const jira = new FakeJira({ + tickets: [workableTicket()], + transitions: workflowFor('MAPCO-1'), + displayNames, + stealOnAssign: 'BROCHSTEIN RAZ', + }); + const { deps, lines, workspaces } = pipeline(jira); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ started: 0, skipped: 1, outcome: 'ok' }); + // The assign is already out there; the point is that nothing follows it — no transition, + // no comment, no clone, and no attempt to take it back off the human. + expect(kindsOf(jira.writes)).toStrictEqual(['assign']); + expect(workspaces.cloned).toStrictEqual([]); + expect(lines.some((line) => line.level === 'warn' && line.payload.reason === 'lost-race' && line.payload.saw === 'BROCHSTEIN RAZ')).toBe(true); + }); + + it('should report the transitions a workflow did offer when it cannot claim.', async () => { + // The real transition vocabulary is unverified, so a refusal has to say what it saw + // rather than leaving a silent no-op to be discovered by a drained queue. + const jira = new FakeJira({ + tickets: [workableTicket()], + transitions: { 'MAPCO-1': [{ id: '31', name: 'Reject', to: 'Rejected' }] }, + displayNames, + }); + const { deps, lines } = pipeline(jira); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ started: 0, skipped: 1 }); + expect(lines.some((line) => line.payload.reason === 'no-transition' && (line.payload.offered as string[])[0] === 'Reject')).toBe(true); + }); + + it('should honour the per-run ticket limit and say there is more waiting.', async () => { + const keys = ['MAPCO-1', 'MAPCO-2', 'MAPCO-3']; + const jira = new FakeJira({ tickets: keys.map((key) => workableTicket({ key })), transitions: workflowFor(...keys), displayNames }); + const { deps } = pipeline(jira, { config: { maxTicketsPerRun: 2, maxConcurrentTickets: 2 } }); + + const result = await runCycle(deps); + + expect(result.found).toBe(2); + expect(result.started).toBe(2); + expect(result.more).toBe(true); + // One over the limit, because the server's `total` is always -1 and cannot be trusted. + expect(jira.queries[0]?.limit).toBe(3); + expect(jira.writes.some((write) => write.key === 'MAPCO-3')).toBe(false); + }); + + it('should hold tickets to one at a time at the default concurrency.', async () => { + const keys = ['MAPCO-1', 'MAPCO-2']; + const jira = new FakeJira({ tickets: keys.map((key) => workableTicket({ key })), transitions: workflowFor(...keys), displayNames }); + const { deps } = pipeline(jira, { config: { maxTicketsPerRun: 2, maxConcurrentTickets: 1 } }); + + await runCycle(deps); + + // Each ticket is finished with before the next is touched: the cap is what stops the + // worker holding a whole page of tickets at once. + expect(ticketRuns(jira.writes)).toStrictEqual(['MAPCO-1', 'MAPCO-2']); + }); + + it('should give each concurrent ticket a workspace of its own.', async () => { + // Two tickets sharing a clone would have the model of one reading the half-finished change + // of the other, and the publish path committing both. + const keys = ['MAPCO-1', 'MAPCO-2']; + const jira = new FakeJira({ tickets: keys.map((key) => workableTicket({ key })), transitions: workflowFor(...keys), displayNames }); + const { deps, agent } = pipeline(jira, { config: { maxTicketsPerRun: 2, maxConcurrentTickets: 2 } }); + + await runCycle(deps); + + expect(new Set(agent.requests.map((request) => request.workdir)).size).toBe(2); + }); + + it('should not claim there is more waiting when the queue is exhausted.', async () => { + const jira = new FakeJira({ tickets: [workableTicket()], transitions: workflowFor('MAPCO-1'), displayNames }); + const { deps } = pipeline(jira); + + expect((await runCycle(deps)).more).toBe(false); + }); - const result = await runCycle(deps); + it('should report an empty queue rather than treating it as a failure.', async () => { + const jira = new FakeJira({ tickets: [] }); + const { deps, lines } = pipeline(jira); - expect(result).toMatchObject({ found: 2, started: 0, skipped: 2, outcome: 'ok' }); - expect(lines.filter((line) => line.payload.msg === 'ticket failed')).toHaveLength(2); + const result = await runCycle(deps); + + expect(result).toMatchObject({ found: 0, started: 0, outcome: 'ok' }); + expect(lines[0]?.level).toBe('info'); + }); + + it('should survive a poll failure and report it, so the schedule keeps running.', async () => { + const jira = new FakeJira({ failWith: new Error('mcp unreachable') }); + const { deps, lines } = pipeline(jira); + + const result = await runCycle(deps); + + expect(result.outcome).toBe('failed'); + expect(lines[0]?.level).toBe('error'); + }); + + it('should keep hold of a ticket it cannot return to Open, and say so loudly.', async () => { + const jira = new FakeJira({ + tickets: [ticket({ summary: 'no prefix, so it is refused' })], + transitions: { 'MAPCO-1': [{ id: '21', name: 'Start Progress', to: 'In Progress' }] }, + displayNames, + }); + const { deps, lines } = pipeline(jira); + + const result = await runCycle(deps); + + // It did hold the ticket, so it counts as started — but it is now stuck, and the run + // line has to make that findable. + expect(result).toMatchObject({ started: 1, outcome: 'ok' }); + await expect(jira.getIssue('MAPCO-1')).resolves.toMatchObject({ assignee: BOT_DISPLAY_NAME }); + expect(lines.some((line) => line.payload.msg === 'ticket not delivered' && line.payload.handedBack === false)).toBe(true); + }); + + it('should let one broken ticket fail without taking the run down with it.', async () => { + const keys = ['MAPCO-1', 'MAPCO-2']; + const jira = new FakeJira({ + tickets: keys.map((key) => workableTicket({ key })), + transitionsFailWith: new Error('jira exploded'), + displayNames, + }); + const { deps, lines } = pipeline(jira, { config: { maxTicketsPerRun: 2 } }); + + const result = await runCycle(deps); + + expect(result).toMatchObject({ found: 2, started: 0, skipped: 2, outcome: 'ok' }); + expect(lines.filter((line) => line.payload.msg === 'ticket failed')).toHaveLength(2); + }); + + it('should write nothing to a ticket it never managed to claim.', async () => { + // The claim threw before the worker held anything, so the failure path must not comment on + // or count a ticket that is still somebody else's to pick up. + const keys = ['MAPCO-1']; + const jira = new FakeJira({ + tickets: keys.map((key) => workableTicket({ key })), + transitionsFailWith: new Error('jira exploded'), + displayNames, + }); + const { deps } = pipeline(jira); + + await runCycle(deps); + + expect(jira.writes).toStrictEqual([]); + }); }); }); 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/implement.spec.ts b/tests/unit/agent/implement.spec.ts index a191922..abd8f03 100644 --- a/tests/unit/agent/implement.spec.ts +++ b/tests/unit/agent/implement.spec.ts @@ -47,8 +47,11 @@ function usage(input: number): TokenUsage { return { input, output: 1, cacheRead: 0, costUsd: 0.5 }; } +/** Turns one fake hand-off reports. Any number but 1 would do; 1 is the value that hides a bug. */ +const TURNS_PER_RUN = 7; + function changed(input = 100): AgentRun { - return { outcome: 'changed', usage: usage(input), summary: 'edited the helper', deniedTools: [] }; + return { outcome: 'changed', usage: usage(input), turns: TURNS_PER_RUN, summary: 'edited the helper', deniedTools: [] }; } /** Hands out queued runs and refuses to invent one, so an over-run of the bound fails loudly. */ @@ -154,7 +157,7 @@ describe('implementTicket', () => { }); it('should never verify a change the model did not make.', async () => { - const agent = fakeAgent([{ outcome: 'no-change', usage: usage(10), summary: 'nothing to do', deniedTools: [] }]); + const agent = fakeAgent([{ outcome: 'no-change', usage: usage(10), turns: TURNS_PER_RUN, summary: 'nothing to do', deniedTools: [] }]); const tests = fakeTests([]); const release = fakeRelease(); @@ -363,7 +366,10 @@ describe('implementTicket', () => { // the test is right and writes nothing. The diff from attempt one is still in the working // tree, so a note saying the ticket was too thin to act on would be false, and it would // throw away the only output a person could use. - const agent = fakeAgent([changed(), { outcome: 'no-change', usage: usage(10), summary: 'the test looks right to me', deniedTools: [] }]); + const agent = fakeAgent([ + changed(), + { outcome: 'no-change', usage: usage(10), turns: TURNS_PER_RUN, summary: 'the test looks right to me', deniedTools: [] }, + ]); const release = fakeRelease(); const result = await implementTicket(assignment, deps(agent, fakeTests([FAILED]), release)); @@ -401,7 +407,7 @@ describe('implementTicket', () => { }); it('should log which tools the model was refused, so a denial is visible in the pod.', async () => { - const agent = fakeAgent([{ outcome: 'changed', usage: usage(1), summary: 'edited', deniedTools: ['Bash'] }]); + const agent = fakeAgent([{ outcome: 'changed', usage: usage(1), turns: TURNS_PER_RUN, summary: 'edited', deniedTools: ['Bash'] }]); const wired = deps(agent, fakeTests([PASSED]), fakeRelease()); await implementTicket(assignment, wired); diff --git a/tests/unit/agent/implementer.spec.ts b/tests/unit/agent/implementer.spec.ts index 484b457..ec0ec69 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 { DEFAULT_AGENT_LIMITS } from '@src/agent/implement'; +import { AgentConfigError } from '@src/agent/credential'; +import { DEFAULT_AGENT_LIMITS, type ImplementDeps } 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 */ @@ -19,7 +19,12 @@ const description: DescriptionPort = { read: async (): Promise => Promis const dirs: string[] = []; function options(env: NodeJS.ProcessEnv): ImplementerOptions { - return { logger: fakeLogger().logger, release, description, env }; + return { logger: fakeLogger().logger, description, env }; +} + +/** The per-ticket half, which arrives after the boot-time half has already been built. */ +function forTicket(implementer: ReturnType): ImplementDeps { + return implementer({ release }); } describe('createImplementer', () => { @@ -33,22 +38,57 @@ 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.', () => { - const deps = createImplementer(options(WITH_KEY)); + const deps = forTicket(createImplementer(options(WITH_KEY))); expect(deps.limits).toStrictEqual(DEFAULT_AGENT_LIMITS); }); it('should carry a configured bound through instead of the default.', () => { - const deps = createImplementer({ ...options(WITH_KEY), limits: { maxAttempts: 1, maxTurns: 5 } }); + const deps = forTicket(createImplementer({ ...options(WITH_KEY), limits: { maxAttempts: 1, maxTurns: 5 } })); expect(deps.limits).toStrictEqual({ maxAttempts: 1, maxTurns: 5 }); }); + it('should read the model credential once, at boot, rather than once per ticket.', () => { + // The whole reason this is two calls: a worker with no credential must fail to come up, and + // a per-ticket read would instead claim a ticket and discover it. + const implementer = createImplementer(options(WITH_KEY)); + + expect(forTicket(implementer).agent).toBe(forTicket(implementer).agent); + }); + + it("should hand each ticket its own hand-back, because the at-most-once latch is the ticket's.", () => { + const implementer = createImplementer(options(WITH_KEY)); + const first: ReleasePort = { handBack: async (): Promise<{ ok: true }> => Promise.resolve({ ok: true }) }; + + expect(implementer({ release: first }).release).toBe(first); + expect(implementer({ release }).release).toBe(release); + }); + + it('should meter the agent for this ticket when the wiring asks it to.', () => { + // Unmetered is only ever right in a test: in production every hand-off is charged to the + // ticket's ledger, which is what stops one ticket eating the whole daily allowance. + const implementer = createImplementer(options(WITH_KEY)); + const sentinel = { run: async (): Promise => Promise.reject(new Error('not run')) }; + + expect(implementer({ release, meter: () => sentinel }).agent).toBe(sentinel); + }); + it('should wire a runner that reads the test command off a real clone.', async () => { // The point of building this in one place is that a caller cannot wire a runner that only // looks like one, so the test asks the assembled object about a directory on disk. @@ -56,7 +96,7 @@ describe('createImplementer', () => { dirs.push(dir); await writeFile(join(dir, 'package.json'), JSON.stringify({ name: 'some-service', scripts: { 'test:ci': 'vitest run' } })); - const deps = createImplementer(options(WITH_KEY)); + const deps = forTicket(createImplementer(options(WITH_KEY))); await expect(deps.tests.plan(dir)).resolves.toMatchObject({ ok: true, plan: { command: 'npm run test:ci' } }); }); @@ -65,7 +105,7 @@ describe('createImplementer', () => { const dir = await mkdtemp(join(tmpdir(), 'agent-bot-implementer-')); dirs.push(dir); - const deps = createImplementer(options(WITH_KEY)); + const deps = forTicket(createImplementer(options(WITH_KEY))); await expect(deps.tests.plan(dir)).resolves.toMatchObject({ ok: false, reason: 'no-command' }); }); diff --git a/tests/unit/agent/meteredAgent.spec.ts b/tests/unit/agent/meteredAgent.spec.ts new file mode 100644 index 0000000..5a33652 --- /dev/null +++ b/tests/unit/agent/meteredAgent.spec.ts @@ -0,0 +1,248 @@ +import { describe, expect, it } from 'vitest'; +import { meterAgent, spendOf } from '@src/agent/meteredAgent'; +import type { AgentPort, AgentRun, AgentRunRequest } from '@src/agent/types'; +import { createBudgetGuard, type TicketGuard } from '@src/budget/guard'; +import type { AbortPort, AbortResult, BudgetConfig } from '@src/budget/types'; +import type { JiraTicket } from '@src/jira/types'; +import type { HandBackOnce } from '@src/tickets/handBackOnce'; +import { ticket } from '@tests/helpers/fakeJira'; +import { fakeLogger, type RecordedLine } from '@tests/helpers/fakeLogger'; + +const WORKDIR = '/workspace/some-service'; +const TICKET = ticket({ key: 'MAPCO-77' }); + +const request: AgentRunRequest = { + task: { key: TICKET.key, summary: TICKET.summary, description: 'Make the retry actually retry.' }, + workdir: WORKDIR, + maxTurns: 40, +}; + +function run(overrides: Partial = {}): AgentRun { + return { + outcome: 'changed', + usage: { input: 100, output: 20, cacheRead: 5, costUsd: 0.5 }, + turns: 4, + summary: 'edited the helper', + deniedTools: [], + ...overrides, + }; +} + +/** Hands out queued runs and refuses to invent one, so a hand-off nobody expected fails loudly. */ +function fakeAgent(runs: AgentRun[]): AgentPort & { requests: AgentRunRequest[] } { + const requests: AgentRunRequest[] = []; + const queue = [...runs]; + + return { + requests, + run: async (asked: AgentRunRequest): Promise => { + requests.push(asked); + const next = queue.shift(); + + if (next === undefined) { + throw new Error(`the agent was run more times than the spec queued (${requests.length})`); + } + + return Promise.resolve(next); + }, + }; +} + +/** Records every abort, which is how the spec sees whether the hand-back path was taken. */ +function recordingAbort(): AbortPort & { notes: string[] } { + const notes: string[] = []; + + return { + notes, + abort: async (_: JiraTicket, note: string): Promise => { + notes.push(note); + + return Promise.resolve({ released: true, attemptCounted: true }); + }, + }; +} + +/** The ticket's latch, recording only. What it is told is the whole seam worth asserting on. */ +function recordingLatch(): Pick & { recorded: AbortResult[] } { + const recorded: AbortResult[] = []; + + return { recorded, record: (outcome: AbortResult): void => void recorded.push(outcome) }; +} + +function budget(overrides: Partial = {}, maxTicketsPerDay = 5): BudgetConfig { + return { ticket: { maxTokens: 1_000_000, maxTurns: 1_000, ...overrides }, maxTicketsPerDay }; +} + +interface Metered { + readonly agent: AgentPort; + readonly inner: AgentPort & { requests: AgentRunRequest[] }; + readonly abort: AbortPort & { notes: string[] }; + readonly handBack: Pick & { recorded: AbortResult[] }; + readonly meter: TicketGuard; + readonly lines: RecordedLine[]; +} + +async function metered(runs: AgentRun[], config: BudgetConfig = budget()): Promise { + const { logger, lines } = fakeLogger(); + const abort = recordingAbort(); + const guard = createBudgetGuard({ budget: config, abort, logger }); + const started = await guard.cycle().start(TICKET, async () => Promise.resolve({ ok: true })); + + if (!started.ok) { + throw new Error('the spec could not start a ticket'); + } + + const inner = fakeAgent(runs); + const handBack = recordingLatch(); + + return { + agent: meterAgent({ agent: inner, meter: started.ticket, handBack, logger }), + inner, + abort, + handBack, + meter: started.ticket, + lines, + }; +} + +describe('spendOf', () => { + it('should charge the whole prompt, cache reads included.', () => { + // A cache read is a billed prompt token, and an agentic loop re-reads its context every + // turn — leaving them out would put most of what a ticket costs outside the ceiling. + expect(spendOf(run({ usage: { input: 100, output: 20, cacheRead: 5_000, costUsd: 1 }, turns: 9 }))).toStrictEqual({ + tokens: 5_120, + turns: 9, + }); + }); + + it("should charge the hand-off's real turn count, not one per hand-off.", () => { + // Charging 1 would turn a ceiling of forty turns into forty hand-offs of forty turns. + expect(spendOf(run({ turns: 12 })).turns).toBe(12); + }); +}); + +describe('meterAgent', () => { + it('should pass a run that is within budget straight through.', async () => { + const { agent } = await metered([run()]); + + await expect(agent.run(request)).resolves.toMatchObject({ outcome: 'changed', summary: 'edited the helper' }); + }); + + it('should hand the request to the real agent unchanged.', async () => { + const { agent, inner } = await metered([run()]); + + await agent.run(request); + + expect(inner.requests).toStrictEqual([request]); + }); + + it('should charge every hand-off to the ticket, so the ceiling bites during it.', async () => { + const { agent, meter } = await metered([run({ turns: 3 }), run({ turns: 4 })]); + + await agent.run(request); + await agent.run(request); + + expect(meter.spend()).toStrictEqual({ tokens: 250, turns: 7 }); + }); + + it('should report the run as over once the token ceiling is reached.', async () => { + const { agent } = await metered([run()], budget({ maxTokens: 100 })); + + await expect(agent.run(request)).resolves.toMatchObject({ outcome: 'gave-up' }); + }); + + it('should report the run as over once the turn ceiling is reached.', async () => { + const { agent } = await metered([run({ turns: 50 })], budget({ maxTurns: 40 })); + + await expect(agent.run(request)).resolves.toMatchObject({ outcome: 'gave-up' }); + }); + + it('should say what it spent and which ceiling it hit.', async () => { + const { agent } = await metered([run({ turns: 50 })], budget({ maxTurns: 40 })); + + await expect(agent.run(request)).resolves.toMatchObject({ summary: expect.stringContaining('50 turns') as unknown as string }); + }); + + it('should let the budget guard hand the ticket back, rather than doing it here.', async () => { + // One release path, not two: the note comes from `describeOverspend` and the count from + // `handBackTicket`, both behind `AbortPort`. + const { agent, abort } = await metered([run()], budget({ maxTokens: 10 })); + + await agent.run(request); + + expect(abort.notes).toHaveLength(1); + expect(abort.notes[0]).toContain('ran out of the budget'); + }); + + it('should tell the ticket what the hand-offs managed, so the comment is worth reading.', async () => { + const { agent, abort } = await metered([run({ outcome: 'changed' })], budget({ maxTokens: 10 })); + + await agent.run(request); + + expect(abort.notes[0]).toContain('hand-off 1: changed files in the clone'); + expect(abort.notes[0]).toContain('How far it got: a change in the clone, not yet verified'); + }); + + it('should report a ticket that never got going as having reached nowhere.', async () => { + const { agent, abort } = await metered([run({ outcome: 'no-change' })], budget({ maxTokens: 10 })); + + await agent.run(request); + + expect(abort.notes[0]).toContain('read the ticket and changed nothing'); + expect(abort.notes[0]).not.toContain('How far it got'); + }); + + it('should throw away an unverified change rather than spend past the ceiling to publish it.', async () => { + // The suite has not run at this point, so "it changed files" is not "it works", and + // publishing would cost more of a budget that has already gone. + const { agent } = await metered([run({ outcome: 'changed' })], budget({ maxTokens: 10 })); + + await expect(agent.run(request)).resolves.toMatchObject({ outcome: 'gave-up' }); + }); + + it('should spend nothing more once the ticket has been stopped.', async () => { + // The queue holds one run, so a second call reaching the real agent fails the spec rather + // than quietly costing money. + const { agent, abort } = await metered([run()], budget({ maxTokens: 10 })); + + await agent.run(request); + + await expect(agent.run(request)).resolves.toMatchObject({ outcome: 'gave-up' }); + expect(abort.notes).toHaveLength(1); + }); + + it("should tell the ticket's latch what the overspend hand-back achieved.", async () => { + // The seam that stops one ticket being handed back twice: the abort has already commented, + // counted and released, and the give-up that follows must write nothing. + const { agent, handBack } = await metered([run()], budget({ maxTokens: 10 })); + + await agent.run(request); + + expect(handBack.recorded).toStrictEqual([{ released: true, attemptCounted: true }]); + }); + + it('should tell the latch nothing when the ticket was never stopped.', async () => { + const { agent, handBack } = await metered([run()]); + + await agent.run(request); + + expect(handBack.recorded).toStrictEqual([]); + }); + + it('should let a transport failure through, because there is nothing to charge for it.', async () => { + const { logger } = fakeLogger(); + const abort = recordingAbort(); + const guard = createBudgetGuard({ budget: budget(), abort, logger }); + const started = await guard.cycle().start(TICKET, async () => Promise.resolve({ ok: true })); + + if (!started.ok) { + throw new Error('the spec could not start a ticket'); + } + + const exploding: AgentPort = { run: async (): Promise => Promise.reject(new Error('the api fell over')) }; + const agent = meterAgent({ agent: exploding, meter: started.ticket, handBack: recordingLatch(), logger }); + + await expect(agent.run(request)).rejects.toThrow('the api fell over'); + expect(started.ticket.spend()).toStrictEqual({ tokens: 0, turns: 0 }); + }); +}); 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..6d51ab7 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', @@ -64,6 +64,7 @@ function result(overrides: Record = {}): unknown { is_error: false, result: 'Added a retry to the fetch helper.', modelUsage: { 'claude-opus-5': { inputTokens: 100, outputTokens: 20, cacheReadInputTokens: 7, costUSD: 0.5 } }, + num_turns: 3, permission_denials: [], ...overrides, }; @@ -118,7 +119,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(); }); @@ -261,4 +276,21 @@ describe('foldMessages', () => { it('should report no usage at all when the run produced no result.', () => { expect(foldMessages([]).usage).toStrictEqual({ input: 0, output: 0, cacheRead: 0, costUsd: 0 }); }); + + it("should report the run's own turn count, because the per-ticket ceiling is counted in turns.", () => { + // Not a count of messages and not `1`: `MAX_TURNS_PER_TICKET` is charged from this number, + // and a hand-off that reported one turn instead of twelve would raise the ceiling twelvefold. + /* eslint-disable-next-line @typescript-eslint/naming-convention -- the SDK's wire format */ + expect(foldMessages([...wrote('Edit'), result({ num_turns: 12 })]).turns).toBe(12); + }); + + it('should report no turns for a run that produced no result.', () => { + expect(foldMessages([]).turns).toBe(0); + }); + + it('should report no turns rather than guessing when the result states none.', () => { + // The safe direction: an invented count charges a ticket for turns nobody took. + /* eslint-disable-next-line @typescript-eslint/naming-convention -- the SDK's wire format */ + expect(foldMessages([...wrote('Edit'), result({ num_turns: undefined })]).turns).toBe(0); + }); }); diff --git a/tests/unit/jira/description.spec.ts b/tests/unit/jira/description.spec.ts new file mode 100644 index 0000000..74071bd --- /dev/null +++ b/tests/unit/jira/description.spec.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from 'vitest'; +import { createDescriptionReader } from '@src/jira/description'; +import type { JiraPort, JiraTicket } from '@src/jira/types'; +import { ticket } from '@tests/helpers/fakeJira'; + +/** A Jira that answers `getIssue` and refuses everything else, so the spec pins the one call used. */ +function readingJira(answer: JiraTicket | null | Error): JiraPort & { asked: string[] } { + const asked: string[] = []; + + return { + asked, + getIssue: async (issueKey: string): Promise => { + asked.push(issueKey); + + if (answer instanceof Error) { + throw answer; + } + + return Promise.resolve(answer); + }, + search: async (): Promise => Promise.reject(new Error('the description reader must not poll')), + getTransitions: async (): Promise => Promise.reject(new Error('the description reader must not read transitions')), + assign: async (): Promise => Promise.reject(new Error('the description reader must not write')), + transition: async (): Promise => Promise.reject(new Error('the description reader must not write')), + addComment: async (): Promise => Promise.reject(new Error('the description reader must not write')), + setLabels: async (): Promise => Promise.reject(new Error('the description reader must not write')), + }; +} + +describe('createDescriptionReader', () => { + it("should read the ticket's prose back by key.", async () => { + const jira = readingJira(ticket({ key: 'MAPCO-77', description: 'Make the retry actually retry.' })); + + await expect(createDescriptionReader(jira).read(ticket({ key: 'MAPCO-77' }))).resolves.toBe('Make the retry actually retry.'); + expect(jira.asked).toStrictEqual(['MAPCO-77']); + }); + + it('should answer with nothing when the ticket carries no description.', async () => { + // The refusal `implementTicket` is looking for: a ticket with no prose is one the model + // could only guess at, and it is refused before a token is spent rather than after. + const jira = readingJira(ticket({ key: 'MAPCO-77' })); + + await expect(createDescriptionReader(jira).read(ticket({ key: 'MAPCO-77' }))).resolves.toBe(''); + }); + + it('should answer with nothing when the issue has gone.', async () => { + // Deleted or moved between the poll and the read. Nothing to work from either way, and a + // null that read as a crash would leave the ticket claimed with nothing said on it. + const jira = readingJira(null); + + await expect(createDescriptionReader(jira).read(ticket())).resolves.toBe(''); + }); + + it('should let a failed read surface rather than reporting it as an empty description.', async () => { + // `implementTicket` contains this and tells the two apart on the ticket: "this ticket has no + // description" asks a human to write one, "the description could not be read" asks them to + // fix the worker. Swallowing the error here would put the wrong sentence on the ticket. + const jira = readingJira(new Error('mcp unreachable')); + + await expect(createDescriptionReader(jira).read(ticket())).rejects.toThrow('mcp unreachable'); + }); +}); diff --git a/tests/unit/jira/mcpJira.spec.ts b/tests/unit/jira/mcpJira.spec.ts index 3806f0c..ff5ecae 100644 --- a/tests/unit/jira/mcpJira.spec.ts +++ b/tests/unit/jira/mcpJira.spec.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/naming-convention -- these mirror the MCP server's wire format */ import { describe, expect, it } from 'vitest'; -import { assigneeFields, labelsFields, toTicket, toTransition } from '@src/jira/mcpJira'; +import { assigneeFields, ISSUE_FIELDS, labelsFields, POLL_FIELDS, toTicket, toTransition } from '@src/jira/mcpJira'; describe('toTicket', () => { it('should read an unassigned ticket as unclaimed, not as assigned to someone called Unassigned.', () => { @@ -43,6 +43,36 @@ describe('toTicket', () => { assignee: null, }); }); + + it('should leave the description absent when nobody asked for one.', () => { + // A ticket off the poll is never asked about its description, and an absent field must not + // read as "this ticket has no description" — that is the refusal `implementTicket` makes + // before spending anything, and it would then fire on every ticket in the queue. + expect(toTicket({ key: 'MAPCO-1', summary: 'a: b' })).not.toHaveProperty('description'); + }); + + it("should keep the ticket's prose when the read asked for it.", () => { + expect(toTicket({ key: 'MAPCO-1', description: 'Make the retry actually retry.' }).description).toBe('Make the retry actually retry.'); + }); + + it('should keep an explicitly empty description, which is a ticket with nothing on it.', () => { + expect(toTicket({ key: 'MAPCO-1', description: '' }).description).toBe(''); + }); +}); + +describe('the field lists', () => { + it('should keep the description out of the poll and in the per-issue read.', () => { + // The poll asks for one more ticket than it will work and drops the rest, so prose in + // `POLL_FIELDS` is paid for on every tick for tickets nobody touches. + expect(POLL_FIELDS).not.toContain('description'); + expect(ISSUE_FIELDS).toContain('description'); + }); + + it('should ask a per-issue read for everything the poll asks for as well.', () => { + // `getIssue` is also the claim's confirmation read, so it still has to answer the fields the + // claim compares — narrowing it to just the description would break the optimistic claim. + expect(ISSUE_FIELDS.split(',')).toEqual(expect.arrayContaining(POLL_FIELDS.split(','))); + }); }); describe('assigneeFields', () => { diff --git a/tests/unit/scheduler.spec.ts b/tests/unit/scheduler.spec.ts index 9e33c7b..7c0b6c8 100644 --- a/tests/unit/scheduler.spec.ts +++ b/tests/unit/scheduler.spec.ts @@ -1,5 +1,11 @@ +import type { Logger } from '@map-colonies/js-logger'; import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { createBudgetGuard } from '@src/budget/guard'; +import type { AbortResult } from '@src/budget/types'; +import type { CycleDeps } from '@src/cycle'; +import type { DeliveryOutcome, DeliveryPort } from '@src/deliver'; import { createScheduler } from '@src/scheduler'; +import { createHandBackOnce, type HandBackOnce } from '@src/tickets/handBackOnce'; import { FakeJira, ticket } from '@tests/helpers/fakeJira'; import { fakeLogger } from '@tests/helpers/fakeLogger'; import type { WorkerConfig } from '@src/common/workerConfig'; @@ -19,8 +25,39 @@ const config: WorkerConfig = { maxConcurrentTickets: 1, mcpUrl: 'http://mcp.invalid', bot: { account: 'developer-agent@mapcolonies.example', displayName: 'AGENT DEVELOPER' }, + workspaceRoot: '/workspace', + commitIdentity: { name: 'developer-agent[bot]', email: 'developer-agent[bot]@users.noreply.github.com' }, }; +/** + * Delivery that does nothing, because these cases are about the clock. + * + * What one claimed ticket costs is `runCycle`'s business and is covered through that seam + * (tests/integration/cycle.spec.ts); what is being asserted here is that a cycle runs, runs + * again after the interval, and stops when told — which a pipeline standing behind it would only + * make slower to read. + */ +const delivery: DeliveryPort = { + deliver: async (): Promise => Promise.resolve({ ok: false, reason: 'no-repo', handedBack: true }), +}; + +function cycleDeps(jira: FakeJira, logger: Logger): CycleDeps { + const handBackDeps = { jira, logger, attemptCap: 2 }; + + return { + jira, + logger, + config, + delivery, + budget: createBudgetGuard({ + budget: { ticket: { maxTokens: 1_000, maxTurns: 10 }, maxTicketsPerDay: 100 }, + abort: { abort: async (): Promise => Promise.resolve({ released: true, attemptCounted: true }) }, + logger, + }), + handBack: (): HandBackOnce => createHandBackOnce(handBackDeps), + }; +} + describe('createScheduler', () => { beforeEach(() => { vi.useFakeTimers(); @@ -33,7 +70,7 @@ describe('createScheduler', () => { it('should run a cycle immediately and again after the interval.', async () => { const jira = new FakeJira({ tickets: [ticket()], transitions: workflow, displayNames }); const { logger } = fakeLogger(); - const scheduler = createScheduler({ jira, logger, config }, 1000, logger); + const scheduler = createScheduler(cycleDeps(jira, logger), 1000, logger); scheduler.start(); await vi.advanceTimersByTimeAsync(0); @@ -50,7 +87,7 @@ describe('createScheduler', () => { it('should keep ticking after a failed poll rather than dying.', async () => { const jira = new FakeJira({ failWith: new Error('mcp unreachable') }); const { logger } = fakeLogger(); - const scheduler = createScheduler({ jira, logger, config }, 1000, logger); + const scheduler = createScheduler(cycleDeps(jira, logger), 1000, logger); scheduler.start(); await vi.advanceTimersByTimeAsync(0); @@ -64,7 +101,7 @@ describe('createScheduler', () => { it('should stop scheduling once stopped.', async () => { const jira = new FakeJira({ tickets: [] }); const { logger } = fakeLogger(); - const scheduler = createScheduler({ jira, logger, config }, 1000, logger); + const scheduler = createScheduler(cycleDeps(jira, logger), 1000, logger); scheduler.start(); await vi.advanceTimersByTimeAsync(0); diff --git a/tests/unit/tickets/claim.spec.ts b/tests/unit/tickets/claim.spec.ts index eb790d5..4bfacf3 100644 --- a/tests/unit/tickets/claim.spec.ts +++ b/tests/unit/tickets/claim.spec.ts @@ -54,6 +54,71 @@ describe('claimTicket', () => { await expect(claimTicket(ticket(), jira, bot)).resolves.toEqual({ ok: true }); }); + it('should claim against the real MAPCO workflow, which names transitions ` to `.', async () => { + // Verified against the live instance: `jira_get_transitions` answers with `id` and `name` + // only — no target status on any entry — and this is the exact vocabulary it returned. + // Both earlier strategies miss, and the first real run refused every ticket because of it. + const jira = new FakeJira({ + tickets: [ticket()], + transitions: { + 'MAPCO-1': [ + { id: '7', name: 'Canceled' }, + { id: '8', name: 'Blocked' }, + { id: '3', name: 'Open to In-Progress' }, + { id: '13', name: 'Open to Reject' }, + { id: '18', name: 'Open to Verified' }, + ], + }, + displayNames, + }); + + await expect(claimTicket(ticket(), jira, bot)).resolves.toEqual({ ok: true }); + expect(jira.writes).toContainEqual({ kind: 'transition', key: 'MAPCO-1', transitionId: '3' }); + }); + + it('should treat the hyphen in `In-Progress` as the same status as `In Progress`.', async () => { + // The difference nobody intends to be meaningful. A byte comparison refuses the ticket. + const jira = new FakeJira({ tickets: [ticket()], transitions: { 'MAPCO-1': [{ id: '5', name: 'In-Progress' }] }, displayNames }); + + await expect(claimTicket(ticket(), jira, bot)).resolves.toEqual({ ok: true }); + }); + + it('should not read a ` to ` name as landing in its *from* status.', async () => { + // `Open to Reject` lands in Reject. Matching it for a claim would move the ticket to a + // status nobody asked for — worse than refusing, because it writes. + const jira = new FakeJira({ + tickets: [ticket()], + transitions: { + 'MAPCO-1': [ + { id: '13', name: 'Open to Reject' }, + { id: '18', name: 'Open to Verified' }, + ], + }, + displayNames, + }); + + const outcome = await claimTicket(ticket(), jira, bot); + + expect(outcome).toEqual({ ok: false, reason: 'no-transition', offered: ['Open to Reject', 'Open to Verified'] }); + expect(jira.writes).toEqual([]); + }); + + it('should prefer a reported target status over a name that merely looks right.', async () => { + // The one strategy that cannot be fooled stays first: `Open to In-Progress` reports that it + // actually lands in Blocked, so the honest answer is to refuse rather than trust the name. + const jira = new FakeJira({ + tickets: [ticket()], + transitions: { 'MAPCO-1': [{ id: '3', name: 'Open to In-Progress', to: 'Blocked' }] }, + displayNames, + }); + + await expect(claimTicket(ticket(), jira, bot)).resolves.toEqual({ + ok: false, + reason: 'no-transition', + offered: ['Open to In-Progress'], + }); + }); + it('should never pick up a ticket someone already holds, and write nothing at all.', async () => { const held = ticket({ assignee: 'BROCHSTEIN RAZ' }); const jira = new FakeJira({ tickets: [held], transitions: workflow, displayNames }); diff --git a/tests/unit/tickets/handBackOnce.spec.ts b/tests/unit/tickets/handBackOnce.spec.ts new file mode 100644 index 0000000..7dc1be5 --- /dev/null +++ b/tests/unit/tickets/handBackOnce.spec.ts @@ -0,0 +1,112 @@ +import { describe, expect, it } from 'vitest'; +import { createHandBackOnce, type HandBackOnce } from '@src/tickets/handBackOnce'; +import { FakeJira, ticket, type FakeWrite } from '@tests/helpers/fakeJira'; +import { fakeLogger } from '@tests/helpers/fakeLogger'; + +const ATTEMPT_CAP = 2; +const GIVE_UP_NOTE = 'Picked this up automatically and could not finish it.'; + +const workflow = { + 'MAPCO-1': [ + { id: '21', name: 'Start Progress', to: 'In Progress' }, + { id: '11', name: 'Reopen', to: 'Open' }, + ], +}; + +/** A workflow with no route back to Open, which is what makes a hand-back keep hold of a ticket. */ +const stuck = { 'MAPCO-1': [{ id: '21', name: 'Start Progress', to: 'In Progress' }] }; + +function comments(writes: readonly FakeWrite[]): string[] { + return writes.filter((write) => write.kind === 'comment').map((write) => write.body); +} + +function handBackFor(jira: FakeJira): HandBackOnce { + return createHandBackOnce({ jira, logger: fakeLogger().logger, attemptCap: ATTEMPT_CAP }); +} + +describe('createHandBackOnce', () => { + it('should hand the ticket back the first time it is asked.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow }); + + await expect(handBackFor(jira).handBack(ticket(), GIVE_UP_NOTE)).resolves.toStrictEqual({ ok: true }); + + expect(jira.writes.map((write) => write.kind)).toStrictEqual(['labels', 'comment', 'transition', 'assign']); + }); + + it('should write nothing more once the budget path has handed the ticket back.', async () => { + // The ordinary overspend: the budget guard aborts mid-hand-off, and the run that is now over + // is reported to `implementTicket` as a give-up, which hands the ticket back as well. A + // second pass would comment again on a ticket already in Open that a human may have taken. + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow }); + const handBack = handBackFor(jira); + + handBack.record({ released: true, attemptCounted: true }); + + await expect(handBack.handBack(ticket(), GIVE_UP_NOTE)).resolves.toStrictEqual({ ok: true }); + expect(jira.writes).toStrictEqual([]); + }); + + it('should report what the budget path achieved rather than inventing a release.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow }); + const handBack = handBackFor(jira); + + handBack.record({ released: false, attemptCounted: true }); + + // Counted but still held: the ticket is not available, and a caller that read this as a + // release would report a ticket as back in Open when it is assigned to the bot. + await expect(handBack.handBack(ticket(), GIVE_UP_NOTE)).resolves.toMatchObject({ ok: false }); + }); + + it('should still hand the ticket back when the budget path wrote nothing at all.', async () => { + // `attemptCounted: false` means `handBackTicket` refused before the first write — no label, + // no comment, no transition. Latching on that would leave the ticket held and silent, with a + // worker that had decided it was finished with it. + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow }); + const handBack = handBackFor(jira); + + handBack.record({ released: false, attemptCounted: false }); + + await expect(handBack.handBack(ticket(), GIVE_UP_NOTE)).resolves.toStrictEqual({ ok: true }); + expect(comments(jira.writes)).toStrictEqual([GIVE_UP_NOTE]); + }); + + it('should collapse two callers racing into one write.', async () => { + // The latch is taken before the first await, so a second caller joins the first's promise + // rather than starting a hand-back of its own. + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow }); + const handBack = handBackFor(jira); + + await Promise.all([handBack.handBack(ticket(), GIVE_UP_NOTE), handBack.handBack(ticket(), GIVE_UP_NOTE)]); + + expect(comments(jira.writes)).toHaveLength(1); + }); + + it('should report a hand-back that could not release, without pretending it did.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: stuck }); + + await expect(handBackFor(jira).handBack(ticket(), GIVE_UP_NOTE)).resolves.toMatchObject({ ok: false, reason: 'no-transition' }); + }); + + it('should not retry a hand-back that failed, because the counter is already written.', async () => { + // The attempt counter landed and the release did not. Trying again would comment a second + // time on a ticket the worker still holds, and the boot-time orphan sweep is what recovers it. + const jira = new FakeJira({ tickets: [ticket()], transitions: stuck }); + const handBack = handBackFor(jira); + + await handBack.handBack(ticket(), GIVE_UP_NOTE); + await handBack.handBack(ticket(), GIVE_UP_NOTE); + + expect(comments(jira.writes)).toHaveLength(1); + }); + + it('should say whether the ticket has been handed back.', async () => { + const jira = new FakeJira({ tickets: [ticket()], transitions: workflow }); + const handBack = handBackFor(jira); + + expect(handBack.handedBack()).toBe(false); + + await handBack.handBack(ticket(), GIVE_UP_NOTE); + + expect(handBack.handedBack()).toBe(true); + }); +}); diff --git a/tests/unit/vcs/clone.spec.ts b/tests/unit/vcs/clone.spec.ts new file mode 100644 index 0000000..3cd0137 --- /dev/null +++ b/tests/unit/vcs/clone.spec.ts @@ -0,0 +1,238 @@ +import { execFile } from 'node:child_process'; +import { mkdtemp, readdir, rm, stat, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { promisify } from 'node:util'; +import { afterAll, describe, expect, it } from 'vitest'; +import type { Repo } from '@src/github/types'; +import { CloneRefusedError, CloneWorkspace } from '@src/vcs/clone'; +import { TOKEN_ENV, type RunGit } from '@src/vcs/gitInvoke'; +import type { TokenProvider } from '@src/vcs/types'; +import { fakeLogger } from '@tests/helpers/fakeLogger'; + +/** + * MAPCO-11433's "the repo is cloned at its default branch into an ephemeral workspace" and "the + * workspace is cleaned up on every path", run rather than argued. + * + * The clone half is real: a real `git init --bare` remote with two branches, the real `git` + * binary, the real filesystem. A fake that agreed with the worker about what `--branch` does + * would prove nothing, and the interesting assertion — that the checkout is on the branch + * GitHub named rather than the one git would have picked — is only true of real git. + * + * The credential half is driven through an injected runner instead, because what matters there + * is what git is *handed*: the token must be in the child's environment and never in its argv, + * and argv is the thing a spec can read. + */ + +const run = promisify(execFile); + +const TOKEN = 'ghs_atokenthatlookslikeatoken'; + +const roots: string[] = []; + +async function scratchRoot(): Promise { + const root = await mkdtemp(join(tmpdir(), 'agent-bot-clone-')); + roots.push(root); + + return root; +} + +/** A bare repository whose HEAD is `main`, with the real work on `release`. */ +async function remoteWithTwoBranches(): Promise<{ url: string; defaultBranch: string }> { + const root = await scratchRoot(); + const remote = join(root, 'remote.git'); + const work = join(root, 'work'); + + await run('git', ['init', '--bare', '--initial-branch=main', remote]); + await run('git', ['init', '--initial-branch=main', work]); + await writeFile(join(work, 'on-main.txt'), 'main\n'); + await run('git', ['-c', 'user.name=t', '-c', 'user.email=t@t', 'add', '.'], { cwd: work }); + await run('git', ['-c', 'user.name=t', '-c', 'user.email=t@t', 'commit', '-m', 'main'], { cwd: work }); + await run('git', ['checkout', '-b', 'release'], { cwd: work }); + await writeFile(join(work, 'on-release.txt'), 'release\n'); + await run('git', ['-c', 'user.name=t', '-c', 'user.email=t@t', 'add', '.'], { cwd: work }); + await run('git', ['-c', 'user.name=t', '-c', 'user.email=t@t', 'commit', '-m', 'release'], { cwd: work }); + await run('git', ['push', remote, 'main', 'release'], { cwd: work }); + + return { url: pathToFileURL(remote).href, defaultBranch: 'release' }; +} + +function repoAt(cloneUrl: string, defaultBranch = 'release'): Repo { + return { name: 'some-service', fullName: 'MapColonies/some-service', defaultBranch, cloneUrl }; +} + +const tokens: TokenProvider = { mint: async (): Promise => Promise.resolve(TOKEN) }; + +/** A provider that records whether it was asked, so a refusal can be shown to mint nothing. */ +function countingTokens(): TokenProvider & { minted: number } { + const counter = { minted: 0, mint: async (): Promise => Promise.resolve(TOKEN) }; + + return { + get minted(): number { + return counter.minted; + }, + mint: async (): Promise => { + counter.minted += 1; + + return counter.mint(); + }, + }; +} + +/** Records every git invocation, so a spec can read the argv and the environment it was given. */ +function recordingGit(fail?: Error): RunGit & { calls: { args: readonly string[]; cwd: string; env?: NodeJS.ProcessEnv }[] } { + const calls: { args: readonly string[]; cwd: string; env?: NodeJS.ProcessEnv }[] = []; + const recorder = async (args: readonly string[], cwd: string, env?: NodeJS.ProcessEnv): Promise => { + calls.push({ args, cwd, env }); + + if (fail) { + throw fail; + } + + return Promise.resolve(''); + }; + + return Object.assign(recorder, { calls }); +} + +function workspaceFor(options: { root: string; run?: RunGit; tokens?: TokenProvider }): CloneWorkspace { + return new CloneWorkspace({ + root: options.root, + tokens: options.tokens ?? tokens, + run: options.run, + logger: fakeLogger().logger, + }); +} + +async function exists(path: string): Promise { + try { + await stat(path); + + return true; + } catch { + return false; + } +} + +describe('CloneWorkspace', () => { + afterAll(async () => { + await Promise.all(roots.splice(0).map(async (root) => rm(root, { recursive: true, force: true }))); + }); + + it('should clone the branch GitHub called default, not the one git would have chosen.', async () => { + // The assertion the ticket actually asks for. The remote's own HEAD is `main`; a clone that + // simply took the remote's default would check out the wrong branch, and every pull request + // the worker opened would be based on it. + const { url, defaultBranch } = await remoteWithTwoBranches(); + const workspace = await workspaceFor({ root: await scratchRoot() }).create(repoAt(url, defaultBranch)); + + const { stdout } = await run('git', ['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: workspace.dir }); + + expect(stdout.trim()).toBe('release'); + await expect(exists(join(workspace.dir, 'on-release.txt'))).resolves.toBe(true); + }); + + it('should leave the clone in a directory of its own, so two tickets cannot share one.', async () => { + const { url } = await remoteWithTwoBranches(); + const root = await scratchRoot(); + const workspaces = workspaceFor({ root }); + + const [first, second] = await Promise.all([workspaces.create(repoAt(url)), workspaces.create(repoAt(url))]); + + expect(first.dir).not.toBe(second.dir); + }); + + it('should remove the whole workspace when it is disposed.', async () => { + const { url } = await remoteWithTwoBranches(); + const root = await scratchRoot(); + const workspace = await workspaceFor({ root }).create(repoAt(url)); + + await workspace.dispose(); + + await expect(exists(workspace.dir)).resolves.toBe(false); + // The root it was made under survives: in the pod that is a mounted volume, not ours to delete. + await expect(readdir(root)).resolves.toStrictEqual([]); + }); + + it('should survive being disposed twice, because the cleanup runs on every path.', async () => { + // `dispose` is called from a `finally`, and a second call is the ordinary consequence of a + // caller that tidies up early and then unwinds. + const { url } = await remoteWithTwoBranches(); + const workspace = await workspaceFor({ root: await scratchRoot() }).create(repoAt(url)); + + await workspace.dispose(); + + await expect(workspace.dispose()).resolves.toBeUndefined(); + }); + + it('should leave nothing behind when the clone itself fails.', async () => { + // The path that would otherwise fill the pod's disk one failed ticket at a time: the + // directory is made before git runs, so a failure has to clean up after itself. + const root = await scratchRoot(); + const workspaces = workspaceFor({ root }); + + await expect(workspaces.create(repoAt(pathToFileURL(join(root, 'no-such-repo.git')).href))).rejects.toThrow(); + + await expect(readdir(root)).resolves.toStrictEqual([]); + }); + + it('should leave nothing behind when the credential cannot be minted.', async () => { + // The same disk-filling path as a failed clone, one step earlier: `mint` throws on a missing + // `GITHUB_TOKEN`, so a directory made before it is one no `catch` in `create` would remove. + const root = await scratchRoot(); + const failing: TokenProvider = { + mint: async (): Promise => Promise.reject(new Error('GITHUB_TOKEN is not set')), + }; + const workspaces = workspaceFor({ root, run: recordingGit(), tokens: failing }); + + await expect(workspaces.create(repoAt('https://github.com/MapColonies/some-service.git'))).rejects.toThrow('GITHUB_TOKEN'); + + await expect(readdir(root)).resolves.toStrictEqual([]); + }); + + it('should hand the credential to git through the environment, never in its argv.', async () => { + // argv is world-readable through `/proc//cmdline`; the environment is readable by the + // process owner alone. A token in the clone's argv is one any process on the host can read. + const git = recordingGit(); + await workspaceFor({ root: await scratchRoot(), run: git }).create(repoAt('https://github.com/MapColonies/some-service.git')); + + const [call] = git.calls; + + expect(call?.args.join(' ')).not.toContain(TOKEN); + expect(call?.env?.[TOKEN_ENV]).toBe(TOKEN); + }); + + it('should ask for a shallow, single-branch clone, because a ticket needs no history.', async () => { + const git = recordingGit(); + await workspaceFor({ root: await scratchRoot(), run: git }).create(repoAt('https://github.com/MapColonies/some-service.git')); + + expect(git.calls[0]?.args).toEqual(expect.arrayContaining(['--depth', '1', '--single-branch', '--branch', 'release'])); + }); + + it('should refuse a remote that is not the configured GitHub host, without minting anything.', async () => { + // The credential helper answers for whatever host git asks about, so a remote nobody + // vetted is a token handed to it. Refused before the mint, so there is nothing to hand over. + const minting = countingTokens(); + const workspaces = workspaceFor({ root: await scratchRoot(), run: recordingGit(), tokens: minting }); + + await expect(workspaces.create(repoAt('https://evil.example/MapColonies/some-service.git'))).rejects.toThrow(CloneRefusedError); + expect(minting.minted).toBe(0); + }); + + it('should refuse a remote whose scheme could carry a credential somewhere else.', async () => { + const workspaces = workspaceFor({ root: await scratchRoot(), run: recordingGit() }); + + await expect(workspaces.create(repoAt('ssh://github.com/MapColonies/some-service.git'))).rejects.toThrow(CloneRefusedError); + }); + + it('should keep the token out of the message when the clone fails.', async () => { + // git echoes the remote back on failure, and a token in a log line outlives the token. + const git = recordingGit(new Error(`fatal: could not read Password for 'https://x-access-token:${TOKEN}@github.com'`)); + const workspaces = workspaceFor({ root: await scratchRoot(), run: git }); + + await expect(workspaces.create(repoAt('https://github.com/MapColonies/some-service.git'))).rejects.toThrow( + expect.objectContaining({ message: expect.not.stringContaining(TOKEN) as unknown as string }) + ); + }); +}); diff --git a/tests/unit/vcs/envToken.spec.ts b/tests/unit/vcs/envToken.spec.ts new file mode 100644 index 0000000..ed2901e --- /dev/null +++ b/tests/unit/vcs/envToken.spec.ts @@ -0,0 +1,45 @@ +import { describe, expect, it } from 'vitest'; +import { envTokenProvider, GITHUB_TOKEN_ENV, readGitHubToken, TokenError } from '@src/vcs/envToken'; + +/* eslint-disable @typescript-eslint/naming-convention -- environment variable names */ +describe('readGitHubToken', () => { + it('should refuse at boot rather than once per ticket.', () => { + // Discovered per ticket, a missing credential costs a claim, a comment naming an environment + // variable and an attempt on somebody's ticket — twice over, which retires it from the queue. + expect(() => readGitHubToken({})).toThrow(TokenError); + }); + + it('should name the variable and the ticket that replaces it.', () => { + expect(() => readGitHubToken({})).toThrow(new RegExp(`${GITHUB_TOKEN_ENV}.*MAPCO-11428`, 'su')); + }); + + it('should accept the token the deployment supplied.', () => { + expect(readGitHubToken({ GITHUB_TOKEN: 'ghp_atoken' })).toBe('ghp_atoken'); + }); +}); + +describe('envTokenProvider', () => { + it('should mint the token the deployment supplied.', async () => { + await expect(envTokenProvider({ GITHUB_TOKEN: 'ghp_atoken' }).mint()).resolves.toBe('ghp_atoken'); + }); + + it('should refuse rather than offer git an empty credential.', async () => { + // An empty token reaches git as an empty password, which fails with a message about + // authentication rather than about a Secret that never arrived. + await expect(envTokenProvider({ GITHUB_TOKEN: ' ' }).mint()).rejects.toThrow(TokenError); + }); + + it('should read the environment on every call, as the App will.', async () => { + // Nothing downstream may be built around a credential that is fetched once and cached: the + // real provider hands out a token that expires in an hour. + const env: NodeJS.ProcessEnv = { GITHUB_TOKEN: 'first' }; + const tokens = envTokenProvider(env); + + await expect(tokens.mint()).resolves.toBe('first'); + + env.GITHUB_TOKEN = 'second'; + + await expect(tokens.mint()).resolves.toBe('second'); + }); +}); +/* eslint-enable @typescript-eslint/naming-convention */ diff --git a/tests/unit/worker.spec.ts b/tests/unit/worker.spec.ts new file mode 100644 index 0000000..285e9ed --- /dev/null +++ b/tests/unit/worker.spec.ts @@ -0,0 +1,105 @@ +import { describe, expect, it } from 'vitest'; +import { AgentConfigError } from '@src/agent/credential'; +import { DEFAULT_AGENT_LIMITS } from '@src/agent/implement'; +import type { WorkerConfig } from '@src/common/workerConfig'; +import { TokenError } from '@src/vcs/envToken'; +import { createWorker, limitsFor } from '@src/worker'; +import { FakeJira } from '@tests/helpers/fakeJira'; +import { fakeLogger } from '@tests/helpers/fakeLogger'; + +/** + * The lifetimes, which are the only thing this file decides. + * + * Two of the three have already been the subject of a defect — a spend total that never reset, + * and a hand-back latch shared where it had to be per ticket — so they are asserted rather than + * argued in a comment. + */ + +/* eslint-disable @typescript-eslint/naming-convention -- environment variable names */ +const env: NodeJS.ProcessEnv = { PATH: '/usr/bin', MODEL_AUTH: 'api-key', ANTHROPIC_API_KEY: 'sk-from-the-secret', GITHUB_TOKEN: 'ghp_atoken' }; +/* eslint-enable @typescript-eslint/naming-convention */ + +const config: WorkerConfig = { + pollIntervalMs: 1000, + maxTicketsPerRun: 1, + maxConcurrentTickets: 1, + mcpUrl: 'http://mcp.invalid', + bot: { account: 'developer-agent@mapcolonies.example', displayName: 'AGENT DEVELOPER' }, + workspaceRoot: '/workspace', + commitIdentity: { name: 'developer-agent[bot]', email: 'developer-agent[bot]@users.noreply.github.com' }, +}; + +function worker(overrides: Partial = {}): ReturnType { + return createWorker({ jira: new FakeJira(), logger: fakeLogger().logger, config: { ...config, ...overrides }, env }); +} + +describe('limitsFor', () => { + it('should bound one hand-off by the per-ticket turn ceiling.', () => { + // No hand-off may go further than the whole ticket is permitted; the cumulative total across + // hand-offs is the ledger's job, which is what `meterAgent` charges. + const limits = limitsFor({ ...config, budget: { ticket: { maxTokens: 1_000, maxTurns: 12 }, maxTicketsPerDay: 5 } }); + + expect(limits).toStrictEqual({ maxAttempts: DEFAULT_AGENT_LIMITS.maxAttempts, maxTurns: 12 }); + }); + + it('should fall back to the conservative ceilings when a config carries none.', () => { + // `budget` is optional on the type only. Absence must read as the defaults, never as "no + // ceiling" — see `budgetOf`. + expect(limitsFor(config).maxTurns).toBe(DEFAULT_AGENT_LIMITS.maxTurns); + }); +}); + +describe('createWorker', () => { + it('should refuse to come up at all when the deployment supplied no model credential.', () => { + // At boot rather than mid-cycle: a worker that discovers this on its first ticket has already + // claimed one and has to hand it straight back. + /* eslint-disable-next-line @typescript-eslint/naming-convention -- an environment variable name */ + const withoutKey: NodeJS.ProcessEnv = { GITHUB_TOKEN: 'ghp_atoken' }; + + expect(() => createWorker({ jira: new FakeJira(), logger: fakeLogger().logger, config, env: withoutKey })).toThrow(AgentConfigError); + }); + + it('should refuse to come up when there is no GitHub credential either.', () => { + // Discovered per ticket instead, this would cost a claim, a comment and an attempt on every + // ticket in the queue — and two of those retire a ticket from it. + // `MODEL_AUTH` is explicit so the model credential passes and the GitHub one is what is + // missing. Left off, `sdk` is the default and this would refuse for the wrong reason. + /* eslint-disable-next-line @typescript-eslint/naming-convention -- environment variable names */ + const withoutToken: NodeJS.ProcessEnv = { MODEL_AUTH: 'api-key', ANTHROPIC_API_KEY: 'sk-from-the-secret' }; + + expect(() => createWorker({ jira: new FakeJira(), logger: fakeLogger().logger, config, env: withoutToken })).toThrow(TokenError); + }); + + it('should hand out a fresh hand-back latch per ticket.', () => { + // The latch is what stops a ticket being handed back twice; sharing one between tickets + // would refuse the *next* legitimate hand-back instead. + const deps = worker(); + + expect(deps.handBack()).not.toBe(deps.handBack()); + }); + + it('should hold one budget guard for the process, because the daily counter is the day’s.', () => { + const deps = worker(); + + expect(deps.budget).toBe(deps.budget); + }); + + it('should open a separate cycle each run, so a spend total cannot outlive the line it is on.', () => { + const deps = worker(); + + expect(deps.budget.cycle()).not.toBe(deps.budget.cycle()); + }); + + it('should report nothing spent on a cycle that has charged nothing.', () => { + expect(deps(worker()).tokensSpent).toBe(0); + }); + + it('should carry the configured ceilings onto the run line.', () => { + expect(deps(worker({ budget: { ticket: { maxTokens: 10, maxTurns: 2 }, maxTicketsPerDay: 3 } })).dailyCapLimit).toBe(3); + }); +}); + +/** The budget half of a fresh cycle's run line. */ +function deps(built: ReturnType): Record { + return built.budget.cycle().runLine() as unknown as Record; +}