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..4f5c083 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,19 @@ 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. +- **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 +103,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,13 +114,25 @@ 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 | +| `ANTHROPIC_API_KEY` | *required* | The model credential, from a Secret. The worker refuses to start without it, and never falls back to an interactive login | +| `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. @@ -106,17 +158,57 @@ 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. +- **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 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 @@ -124,22 +216,35 @@ unassigned and `In Progress` — which polls straight back in, forever. transition name as a fallback, which covers both shapes, and a `no-transition` refusal logs the `offered` names — so the first real run reports the vocabulary rather than refusing in silence. Confirm it from that log line before trusting a deployment. -- `helm lint` needs the private `mclabels` dependency and fails without registry access. +- **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. +- `helm lint` needs the private `mclabels` dependency (`az acr login --name acrarolibotnonprod`, + then `helm dependency build helm`) and fails without registry access. It also fails on + `mclabels.component: worker`, which is not a member of the subchart's enum; the fix lives in + the auth-modes branch rather than here, so lint this chart with + `--set mclabels.component=backend` until that lands. ## 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 + ANTHROPIC_API_KEY="$ANTHROPIC_API_KEY" \ + 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 +259,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..f92c5ef 100644 --- a/helm/templates/deployment.yaml +++ b/helm/templates/deployment.yaml @@ -58,6 +58,10 @@ spec: {{- 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,40 @@ 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 }} + {{- 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 readApiKey). + - name: ANTHROPIC_API_KEY + 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 }} @@ -103,6 +141,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: diff --git a/helm/values.yaml b/helm/values.yaml index fa8e035..5d2e11d 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -69,6 +69,42 @@ 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: '' + + # The Anthropic API key. **Required** — the worker refuses to start without it, which is the + # loudest thing a missing Secret can be. Never an interactive login: a run that billed a + # person's account would look exactly like a working one. + anthropicSecretName: '' + anthropicSecretKey: 'apiKey' + + # 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 +115,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/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..8472eb0 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,19 +11,25 @@ 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. + * + * 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. */ @@ -33,26 +39,47 @@ interface ImplementerOptions { 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. */ -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..2ed4049 100644 --- a/src/agent/sdkAgent.ts +++ b/src/agent/sdkAgent.ts @@ -65,10 +65,11 @@ class SdkAgent implements AgentPort { * for this to fall back to — `readApiKey` refuses to take one — and no branch here that could * grow one later. * - * 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 }); diff --git a/src/agent/sdkOptions.ts b/src/agent/sdkOptions.ts index 8e70649..1761ebf 100644 --- a/src/agent/sdkOptions.ts +++ b/src/agent/sdkOptions.ts @@ -275,6 +275,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 +346,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 +357,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/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..92029d6 --- /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 `readApiKey` 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..96afe3c --- /dev/null +++ b/src/worker.ts @@ -0,0 +1,126 @@ +import type { Logger } from '@map-colonies/js-logger'; +import { budgetOf, type WorkerConfig } from '@common/workerConfig'; +import { readApiKey } from './agent/apiKey'; +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 key is checked first because its refusal is the + // more specific ("never an interactive login"), 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. + readApiKey(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: '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/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('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/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(['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/workspace/some-service-0']); + expect(workspaces.disposed).toStrictEqual(['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/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/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..f473eec 100644 --- a/tests/unit/agent/implementer.spec.ts +++ b/tests/unit/agent/implementer.spec.ts @@ -3,7 +3,7 @@ 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 { 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'; @@ -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', () => { @@ -38,17 +43,42 @@ describe('createImplementer', () => { }); 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 +86,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 +95,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 = '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/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/sdkOptions.spec.ts b/tests/unit/agent/sdkOptions.spec.ts index 844694e..1f5173a 100644 --- a/tests/unit/agent/sdkOptions.spec.ts +++ b/tests/unit/agent/sdkOptions.spec.ts @@ -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, }; @@ -261,4 +262,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/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('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/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('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/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('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/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('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/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..7559e91 --- /dev/null +++ b/tests/unit/worker.spec.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from 'vitest'; +import { AgentConfigError } from '@src/agent/apiKey'; +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', 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. + /* eslint-disable-next-line @typescript-eslint/naming-convention -- an environment variable name */ + const withoutToken: NodeJS.ProcessEnv = { 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; +}