From 001340ad8bca45d23d86ff7aaa833f1284532a03 Mon Sep 17 00:00:00 2001 From: razbroc Date: Sun, 23 Aug 2026 11:24:49 +0300 Subject: [PATCH] feat: implement a claimed ticket with the agent SDK and verify it locally MAPCO-11434. Hands a claimed ticket to the Claude Agent SDK inside the clone, runs the clone's own inferred test command, and retries up to a bound before giving up. The diff is left in the working tree; nothing is committed or pushed. The model's tool surface is the security control, not the prompt: `tools` names file and search tools only, so no Bash is built for the session, and Bash, PowerShell, the worktree tools, subagents, the network tools and every MCP tool are denied by name as well. Settings sources are off, so the clone cannot widen its own permissions. A write counts as a change only when its `tool_result` comes back without an error. Reading the attempted `tool_use` alone meant a failed Edit or a denied Write certified a pristine tree as a verified diff, which is the one thing this slice exists to guarantee. Not wired into runCycle yet, and `DescriptionPort` has no implementation: the poll does not fetch a description and `JiraTicket` carries none, so every ticket is handed back before the first model turn until that lands. The chart has no ANTHROPIC_API_KEY secretKeyRef yet either. --- package-lock.json | 207 +++++++++++ package.json | 1 + src/agent/apiKey.ts | 84 +++++ src/agent/implement.ts | 324 ++++++++++++++++ src/agent/implementer.ts | 58 +++ src/agent/prompt.ts | 72 ++++ src/agent/sdkAgent.ts | 78 ++++ src/agent/sdkOptions.ts | 353 ++++++++++++++++++ src/agent/types.ts | 165 +++++++++ src/agent/usage.ts | 21 ++ src/workspace/npmTestRunner.ts | 159 ++++++++ src/workspace/subprocess.ts | 128 +++++++ src/workspace/testCommand.ts | 196 ++++++++++ src/workspace/types.ts | 87 +++++ tests/unit/agent/apiKey.spec.ts | 49 +++ tests/unit/agent/implement.spec.ts | 411 +++++++++++++++++++++ tests/unit/agent/implementer.spec.ts | 72 ++++ tests/unit/agent/prompt.spec.ts | 69 ++++ tests/unit/agent/sdkAgent.spec.ts | 113 ++++++ tests/unit/agent/sdkOptions.spec.ts | 264 +++++++++++++ tests/unit/workspace/npmTestRunner.spec.ts | 293 +++++++++++++++ tests/unit/workspace/subprocess.spec.ts | 107 ++++++ tests/unit/workspace/testCommand.spec.ts | 182 +++++++++ 23 files changed, 3493 insertions(+) create mode 100644 src/agent/apiKey.ts create mode 100644 src/agent/implement.ts create mode 100644 src/agent/implementer.ts create mode 100644 src/agent/prompt.ts create mode 100644 src/agent/sdkAgent.ts create mode 100644 src/agent/sdkOptions.ts create mode 100644 src/agent/types.ts create mode 100644 src/agent/usage.ts create mode 100644 src/workspace/npmTestRunner.ts create mode 100644 src/workspace/subprocess.ts create mode 100644 src/workspace/testCommand.ts create mode 100644 src/workspace/types.ts create mode 100644 tests/unit/agent/apiKey.spec.ts create mode 100644 tests/unit/agent/implement.spec.ts create mode 100644 tests/unit/agent/implementer.spec.ts create mode 100644 tests/unit/agent/prompt.spec.ts create mode 100644 tests/unit/agent/sdkAgent.spec.ts create mode 100644 tests/unit/agent/sdkOptions.spec.ts create mode 100644 tests/unit/workspace/npmTestRunner.spec.ts create mode 100644 tests/unit/workspace/subprocess.spec.ts create mode 100644 tests/unit/workspace/testCommand.spec.ts diff --git a/package-lock.json b/package-lock.json index 6d757ad..59964c3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,6 +9,7 @@ "version": "0.1.0", "license": "ISC", "dependencies": { + "@anthropic-ai/claude-agent-sdk": "^0.3.237", "@map-colonies/config": "^4.0.1", "@map-colonies/js-logger": "^5.0.0", "@map-colonies/prometheus": "^1.0.0", @@ -44,6 +45,156 @@ "vitest": "^4.0.18" } }, + "node_modules/@anthropic-ai/claude-agent-sdk": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk/-/claude-agent-sdk-0.3.237.tgz", + "integrity": "sha512-MVjJ+13YP5uzA3WcCrtDmEirPcwdVsJjTGn/WbUtcXh0Y40KzcG5QocEo4x+QXl5eQ9kKIoDtcjIAfAAauXfeA==", + "license": "SEE LICENSE IN README.md", + "engines": { + "node": ">=18.0.0" + }, + "optionalDependencies": { + "@anthropic-ai/claude-agent-sdk-darwin-arm64": "0.3.237", + "@anthropic-ai/claude-agent-sdk-darwin-x64": "0.3.237", + "@anthropic-ai/claude-agent-sdk-linux-arm64": "0.3.237", + "@anthropic-ai/claude-agent-sdk-linux-arm64-musl": "0.3.237", + "@anthropic-ai/claude-agent-sdk-linux-x64": "0.3.237", + "@anthropic-ai/claude-agent-sdk-linux-x64-musl": "0.3.237", + "@anthropic-ai/claude-agent-sdk-win32-arm64": "0.3.237", + "@anthropic-ai/claude-agent-sdk-win32-x64": "0.3.237" + }, + "peerDependencies": { + "@anthropic-ai/sdk": ">=0.93.0", + "@modelcontextprotocol/sdk": "^1.29.0", + "zod": "^4.0.0" + } + }, + "node_modules/@anthropic-ai/claude-agent-sdk-darwin-arm64": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-darwin-arm64/-/claude-agent-sdk-darwin-arm64-0.3.237.tgz", + "integrity": "sha512-u9r73eYFatAT5h9ntX2Mx6v+4pe3+7mIQYnljf7MyJnitgnWBrexiNMyc2WxKQpkBNyen8m0dgnO0zCjGnfG1g==", + "cpu": [ + "arm64" + ], + "license": "SEE LICENSE IN LICENSE.md", + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/@anthropic-ai/claude-agent-sdk-darwin-x64": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-darwin-x64/-/claude-agent-sdk-darwin-x64-0.3.237.tgz", + "integrity": "sha512-M7gmrWLhTLS4p9jRTXktPwMendILa0zD3CeFa22dpYP35tHZJkwPbB2UbZrnYWHV2ws5pZi5lB1rLOsTInvC+Q==", + "cpu": [ + "x64" + ], + "license": "SEE LICENSE IN LICENSE.md", + "optional": true, + "os": [ + "darwin" + ] + }, + "node_modules/@anthropic-ai/claude-agent-sdk-linux-arm64": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-arm64/-/claude-agent-sdk-linux-arm64-0.3.237.tgz", + "integrity": "sha512-LTZ1cd1AKDJtNU6tdzYi2UUu8sC9rBfcuL1to1ET92rx8aVzHfF1/f36tkBPqc9PVPVpUhF8nr4Y7F4eild5HQ==", + "cpu": [ + "arm64" + ], + "license": "SEE LICENSE IN LICENSE.md", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/@anthropic-ai/claude-agent-sdk-linux-arm64-musl": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-arm64-musl/-/claude-agent-sdk-linux-arm64-musl-0.3.237.tgz", + "integrity": "sha512-gOe5H4SsL9KWPRn8YoJ0TcekLHU6XvGxPQ692lPq1ZGueYDsSE6LUeojYy1wQc3R0pwwmz2cJU5DtaM4qPa50A==", + "cpu": [ + "arm64" + ], + "license": "SEE LICENSE IN LICENSE.md", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/@anthropic-ai/claude-agent-sdk-linux-x64": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-x64/-/claude-agent-sdk-linux-x64-0.3.237.tgz", + "integrity": "sha512-CnJokzNI0TTLX75PjsrJM8vtfp5x1lReB0QLMZbSAjPRbPudeeZr5Gs2rwB9X63CXvHaTKOrt74TUPgMA3na0A==", + "cpu": [ + "x64" + ], + "license": "SEE LICENSE IN LICENSE.md", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/@anthropic-ai/claude-agent-sdk-linux-x64-musl": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-linux-x64-musl/-/claude-agent-sdk-linux-x64-musl-0.3.237.tgz", + "integrity": "sha512-EfI/AMf75UEDjIsYAYZj1RHXrFLvJXlCyx8gC4M1u1BG1+Qwdc/r3wz7cGYVknWE+8nu9Q+Ik9RMwPnXBPbqLA==", + "cpu": [ + "x64" + ], + "license": "SEE LICENSE IN LICENSE.md", + "optional": true, + "os": [ + "linux" + ] + }, + "node_modules/@anthropic-ai/claude-agent-sdk-win32-arm64": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-win32-arm64/-/claude-agent-sdk-win32-arm64-0.3.237.tgz", + "integrity": "sha512-5cKVKcjWSJ9iFDDj7UaiJ+N0GC1NGZC0Z3Z1K+wXW8uy4nWSe0bHpo5S95HypOwuHUENSjlmvv++xv9l6nPj3Q==", + "cpu": [ + "arm64" + ], + "license": "SEE LICENSE IN LICENSE.md", + "optional": true, + "os": [ + "win32" + ] + }, + "node_modules/@anthropic-ai/claude-agent-sdk-win32-x64": { + "version": "0.3.237", + "resolved": "https://registry.npmjs.org/@anthropic-ai/claude-agent-sdk-win32-x64/-/claude-agent-sdk-win32-x64-0.3.237.tgz", + "integrity": "sha512-DvHNDIFgx/jpRuGqKDedeK3zmaBzjmDz7dM1Vh/nzaJYuyLwK7uzh+Lc88o6wfRlRzvH1jOstetqKqol+BxuRw==", + "cpu": [ + "x64" + ], + "license": "SEE LICENSE IN LICENSE.md", + "optional": true, + "os": [ + "win32" + ] + }, + "node_modules/@anthropic-ai/sdk": { + "version": "0.120.0", + "resolved": "https://registry.npmjs.org/@anthropic-ai/sdk/-/sdk-0.120.0.tgz", + "integrity": "sha512-ZlvmNFT/iIF6JD13rxbbMWD8nvGR0RaUp6yMQnoc+4Af0YjVVe/bIdW1XSQQsoxXAtg1NaT6Vak0LKFlJ4d37Q==", + "license": "MIT", + "peer": true, + "dependencies": { + "json-schema-to-ts": "^3.1.1", + "standardwebhooks": "^1.0.0" + }, + "bin": { + "anthropic-ai-sdk": "bin/cli" + }, + "peerDependencies": { + "zod": "^3.25.0 || ^4.0.0" + }, + "peerDependenciesMeta": { + "zod": { + "optional": true + } + } + }, "node_modules/@apideck/better-ajv-errors": { "version": "0.3.7", "resolved": "https://registry.npmjs.org/@apideck/better-ajv-errors/-/better-ajv-errors-0.3.7.tgz", @@ -135,6 +286,16 @@ "node": ">=6.0.0" } }, + "node_modules/@babel/runtime": { + "version": "7.29.7", + "resolved": "https://registry.npmjs.org/@babel/runtime/-/runtime-7.29.7.tgz", + "integrity": "sha512-Nq8OhGWiZIZGV6hLHoyAKLLcJihP/xFeBMGJoUrxTX2psI8dCifzLhZISFb+VWS3wFMRDmCGw5R+dOySCqPLhw==", + "license": "MIT", + "peer": true, + "engines": { + "node": ">=6.9.0" + } + }, "node_modules/@babel/types": { "version": "7.29.8", "resolved": "https://registry.npmjs.org/@babel/types/-/types-7.29.8.tgz", @@ -4410,6 +4571,13 @@ "url": "https://ko-fi.com/dangreen" } }, + "node_modules/@stablelib/base64": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/@stablelib/base64/-/base64-1.0.1.tgz", + "integrity": "sha512-1bnPQqSxSuc3Ii6MhBysoWCg58j97aUjuCSZrGSmDxNqtytIi0k8utUenAwTZN4V5mXXYGsVUI9zeBqy+jBOSQ==", + "license": "MIT", + "peer": true + }, "node_modules/@standard-schema/spec": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/@standard-schema/spec/-/spec-1.1.0.tgz", @@ -7258,6 +7426,13 @@ "integrity": "sha512-W+KJc2dmILlPplD/H4K9l9LcAHAfPtP6BY84uVLXQ6Evcz9Lcg33Y2z1IVblT6xdY54PXYVHEv+0Wpq8Io6zkA==", "license": "MIT" }, + "node_modules/fast-sha256": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/fast-sha256/-/fast-sha256-1.3.0.tgz", + "integrity": "sha512-n11RGP/lrWEFI/bWdygLxhI+pVeo1ZYIVwvvPkW7azl/rOy+F3HYRZ2K5zeE9mmkhQppyv9sQFx0JM9UabnpPQ==", + "license": "Unlicense", + "peer": true + }, "node_modules/fast-uri": { "version": "3.1.5", "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.5.tgz", @@ -8246,6 +8421,20 @@ "dev": true, "license": "MIT" }, + "node_modules/json-schema-to-ts": { + "version": "3.1.1", + "resolved": "https://registry.npmjs.org/json-schema-to-ts/-/json-schema-to-ts-3.1.1.tgz", + "integrity": "sha512-+DWg8jCJG2TEnpy7kOm/7/AxaYoaRbjVB4LFZLySZlWn8exGs3A4OLJR966cVvU26N7X9TWxl+Jsw7dzAqKT6g==", + "license": "MIT", + "peer": true, + "dependencies": { + "@babel/runtime": "^7.18.3", + "ts-algebra": "^2.0.0" + }, + "engines": { + "node": ">=16" + } + }, "node_modules/json-schema-traverse": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-1.0.0.tgz", @@ -10523,6 +10712,17 @@ "dev": true, "license": "MIT" }, + "node_modules/standardwebhooks": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/standardwebhooks/-/standardwebhooks-1.0.0.tgz", + "integrity": "sha512-BbHGOQK9olHPMvQNHWul6MYlrRTAOKn03rOe4A8O3CLWhNf4YHBqq2HJKKC+sfqpxiBY52pNeesD6jIiLDz8jg==", + "license": "MIT", + "peer": true, + "dependencies": { + "@stablelib/base64": "^1.0.0", + "fast-sha256": "^1.3.0" + } + }, "node_modules/statuses": { "version": "2.0.2", "resolved": "https://registry.npmjs.org/statuses/-/statuses-2.0.2.tgz", @@ -10783,6 +10983,13 @@ "integrity": "sha512-N3WMsuqV66lT30CrXNbEjx4GEwlow3v6rr4mCcv6prnfwhS01rkgyFdjPNBYd9br7LpXV1+Emh01fHnq2Gdgrw==", "license": "MIT" }, + "node_modules/ts-algebra": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/ts-algebra/-/ts-algebra-2.0.0.tgz", + "integrity": "sha512-FPAhNPFMrkwz76P7cdjdmiShwMynZYN6SgOujD1urY4oNm80Ou9oMdmbR45LotcKOXoy7wSmHkRFE6Mxbrhefw==", + "license": "MIT", + "peer": true + }, "node_modules/ts-api-utils": { "version": "2.5.0", "resolved": "https://registry.npmjs.org/ts-api-utils/-/ts-api-utils-2.5.0.tgz", diff --git a/package.json b/package.json index 35158d8..2305a49 100644 --- a/package.json +++ b/package.json @@ -30,6 +30,7 @@ "author": "MapColonies", "license": "ISC", "dependencies": { + "@anthropic-ai/claude-agent-sdk": "^0.3.237", "@map-colonies/config": "^4.0.1", "@map-colonies/js-logger": "^5.0.0", "@map-colonies/prometheus": "^1.0.0", diff --git a/src/agent/apiKey.ts b/src/agent/apiKey.ts new file mode 100644 index 0000000..099a9e3 --- /dev/null +++ b/src/agent/apiKey.ts @@ -0,0 +1,84 @@ +/** + * How the worker gets the credential it talks to the model with. + * + * This lives here rather than in `WorkerConfig` for one reason that is not tidiness: every + * other field of `WorkerConfig` is safe to log, and this one is not. Keeping it out of that + * object means the config a cycle carries around — and that ends up in a log line the day + * someone logs it — never contains a key. It is read once, at the entry point, and handed + * straight to `AgentSettings`. + * + * The env var is the standard `ANTHROPIC_API_KEY`, which is what makes the deployment side of + * this a `secretKeyRef` in the pod spec and nothing more: + * + * ```yaml + * - name: ANTHROPIC_API_KEY + * valueFrom: + * secretKeyRef: + * name: {{ .Values.worker.anthropicSecretName }} + * key: apiKey + * ``` + * + * That block, and the README row that documents it, are the deployment half of MAPCO-11434's + * first acceptance criterion. They live in `helm/templates/deployment.yaml`, `helm/values.yaml` + * and `README.md`, none of which this slice owns — so the code half refuses to start without the + * variable, which is the loudest thing it can do about a Secret that never arrives. + */ + +/** The one variable the worker authenticates with. Set from a Secret in the cluster. */ +const API_KEY_ENV = 'ANTHROPIC_API_KEY'; + +/** + * Credentials that would let a run authenticate as a *person* rather than as the worker. + * + * Present on a developer's laptop, absent from the pod, and never a fallback. "Never an + * interactive login" is an acceptance criterion, and the way that criterion is usually broken + * is not by a decision but by a default: a library that quietly picks up whatever session + * token it can find. The worker refuses instead, and says which variable it saw, because a + * dry-run that silently billed a human's account would look exactly like a working one. + * + * These are stripped from the model's own subprocess environment as well — see + * `SECRET_ENV_NAMES` in src/workspace/subprocess.ts. + */ +const LOGIN_ENV_NAMES = ['CLAUDE_CODE_OAUTH_TOKEN', 'ANTHROPIC_AUTH_TOKEN'] as const; + +/** + * A missing or unusable model credential. + * + * Distinct class rather than a bare `Error` for the same reason as `ConfigError` in + * src/common/workerConfig.ts: this is a deployment fault, discovered at boot, and it must not + * read as a ticket that failed. + */ +class AgentConfigError extends Error { + public constructor(message: string) { + super(message); + this.name = 'AgentConfigError'; + } +} + +/** + * The API key, or a thrown `AgentConfigError`. + * + * Thrown rather than refused-as-a-value on purpose: a worker with no key cannot do the one + * thing it exists for, and every ticket it claimed in the meantime would be a claim burnt for + * nothing. Boot is the cheapest place to find out. + * + * The env map is a parameter so tests drive it with plain objects, exactly as + * `loadWorkerConfig` does — nothing in the suite mutates `process.env`. + */ +function readApiKey(env: NodeJS.ProcessEnv = process.env): string { + const key = env[API_KEY_ENV]?.trim() ?? ''; + + if (key !== '') { + return key; + } + + const login = LOGIN_ENV_NAMES.filter((name) => (env[name]?.trim() ?? '') !== ''); + const instead = + login.length > 0 + ? ` ${login.join(' and ')} ${login.length === 1 ? 'is' : 'are'} set, and will not be used instead: the worker authenticates as itself, never as whoever logged in.` + : ''; + + throw new AgentConfigError(`${API_KEY_ENV} must be set — the worker has no other way to reach the model.${instead}`); +} + +export { AgentConfigError, API_KEY_ENV, LOGIN_ENV_NAMES, readApiKey }; diff --git a/src/agent/implement.ts b/src/agent/implement.ts new file mode 100644 index 0000000..75adac4 --- /dev/null +++ b/src/agent/implement.ts @@ -0,0 +1,324 @@ +import type { Logger } from '@map-colonies/js-logger'; +import { tail } from '../workspace/subprocess'; +import type { TestRun, TestRunner } from '../workspace/types'; +import { buildFailureReport } from './prompt'; +import { addUsage, NO_USAGE } from './usage'; +import type { AgentLimits, AgentPort, AgentRun, AgentRunRequest, AgentTask, Assignment, DescriptionPort, ReleasePort, TokenUsage } from './types'; + +/** + * Three hand-offs: one to make the change, two to react to a failing suite. + * + * The number is a spending decision, not a belief about how many tries it takes. A model that + * has not got a suite passing on the third read of the same failure is not converging, and the + * ticket's own attempt cap (`ATTEMPT_CAP`, counted in Jira labels) is what gives it another go + * later with a fresh context rather than another go now with a tired one. + */ +const DEFAULT_MAX_ATTEMPTS = 3; +/** Turn budget per hand-off. Enough for a small ticket in a real repo, not enough to wander. */ +const DEFAULT_MAX_TURNS = 40; +const DEFAULT_AGENT_LIMITS: AgentLimits = { maxAttempts: DEFAULT_MAX_ATTEMPTS, maxTurns: DEFAULT_MAX_TURNS }; + +/** How much failing output goes on the ticket. The full run is in the pod logs. */ +const NOTE_OUTPUT_LIMIT = 2_000; + +/** + * Why the worker stopped without a verified change. + * + * All of them end the same way — hand the ticket back — but they are different things to read + * on a ticket six weeks later. `tests-failing` is the model not managing it; `no-description` is + * a ticket with nothing on it to work from, refused before a token is spent; `no-change` is the + * model reading a ticket it had prose for and finding nothing to do; `not-verifiable` is the + * repository, not the change; `verification-changed` is the change having edited the command + * that grades it, which is the one a person should actually look at; `agent-error` is the run + * itself falling over. + */ +type GiveUpReason = 'tests-failing' | 'no-description' | 'no-change' | 'not-verifiable' | 'verification-changed' | 'agent-error'; + +type ImplementResult = + | { readonly ok: true; readonly attempts: number; readonly usage: TokenUsage; readonly command: string } + | { readonly ok: false; readonly reason: GiveUpReason; readonly attempts: number; readonly usage: TokenUsage; readonly released: boolean }; + +interface ImplementDeps { + readonly agent: AgentPort; + readonly tests: TestRunner; + /** Where the ticket's prose comes from. See `DescriptionPort` — nothing implements it yet. */ + readonly description: DescriptionPort; + readonly release: ReleasePort; + readonly logger: Logger; + readonly limits: AgentLimits; +} + +/** + * What the worker says on a ticket it could not finish. + * + * Written for whoever finds the ticket back in Open: what was tried, how far it got, and the + * standing facts — nothing was pushed, no branch exists, and the clone the changes were made + * in is gone. A note that only said "failed" would send a person looking for a branch that was + * never created. + */ +function giveUpNote(reason: GiveUpReason, attempts: number, test: TestRun | undefined, summary: string): string { + const attemptWord = attempts === 1 ? 'attempt' : 'attempts'; + // Zero attempts is a real case and worth saying plainly: the repository was refused before + // any work was done, so there is nothing for a reader to go looking for. + const opening = + attempts === 0 + ? 'Picked this up automatically and did not start work on it.' + : `Picked this up automatically and could not finish it, after ${attempts} ${attemptWord}.`; + const lines = [opening, '']; + + if (reason === 'tests-failing' && test !== undefined) { + lines.push( + `The change was made, but \`${test.command ?? 'the test command'}\` did not pass. Its last output was:`, + '', + '{code}', + tail(test.output.trim(), NOTE_OUTPUT_LIMIT), + '{code}' + ); + } else if (reason === 'no-description') { + // Two ways to have no prose, and they ask different things of the reader: write a + // description, or fix the worker. Saying which costs one line and saves a wrong guess. + lines.push( + summary === '' + ? 'This ticket has no description, and its summary alone is not something to change code against without guessing. A few lines saying what should be different would be enough to pick it up again — no work was attempted and nothing was spent.' + : `The ticket's description could not be read, so no work was attempted: ${summary}` + ); + } else if (reason === 'no-change') { + lines.push( + 'Nothing was changed: there was not enough here to act on without guessing. A description saying what should be different would probably be enough.' + ); + } else if (reason === 'verification-changed') { + lines.push( + 'The change was thrown away because it edited the scripts the worker uses to verify it:', + '', + '{code}', + tail(test?.output.trim() ?? '', NOTE_OUTPUT_LIMIT), + '{code}', + '', + 'This is worth a look by a person. If the ticket really does need one of those scripts changed, it is not work this worker can grade itself on.' + ); + } else if (reason === 'not-verifiable') { + // Bounded like every other branch. The runner caps its own capture, but a note that + // depended on that would grow silently the day the capture limit is raised. + lines.push( + `The change could not be verified in this repository, so it was thrown away rather than offered. ${tail(test?.output.trim() ?? '', NOTE_OUTPUT_LIMIT)}`.trim(), + '', + 'A change nothing can test is a change a reviewer would be the first check on, which is the one thing this worker is not allowed to do.' + ); + } else { + lines.push(`The run did not finish: ${summary}`); + + // An earlier attempt can have changed the code and failed the suite before the run fell + // over. Both facts are worth having: one says what to fix, the other says why nobody got + // round to it. + if (test !== undefined) { + lines.push( + '', + `An earlier attempt had changed the code, and \`${test.command ?? 'the test command'}\` reported:`, + '', + '{code}', + tail(test.output.trim(), NOTE_OUTPUT_LIMIT), + '{code}' + ); + } + } + + lines.push( + '', + "Nothing was pushed and no branch was created — that only happens after the repository's own tests pass. This counts as an attempt; the ticket is available again." + ); + + return lines.join('\n'); +} + +/** + * One hand-off to the model, with a thrown error turned into an outcome. + * + * A transport failure is not exceptional here — it is just this attempt not working — and + * letting it escape would leave the ticket claimed with nothing said on it. Contained so the + * release path still runs. + */ +async function runAgent(request: AgentRunRequest, deps: ImplementDeps): Promise { + try { + return await deps.agent.run(request); + } 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: [] }; + } +} + +/** Everything a hand-back needs to know about the run that is being handed back. */ +interface GiveUp { + readonly reason: GiveUpReason; + readonly attempts: number; + readonly usage: TokenUsage; + readonly test: TestRun | undefined; + readonly summary: string; +} + +/** + * The one exit that touches Jira, shared by every way of not finishing. + * + * Every give-up goes through here so none of them can forget to comment, and so the + * attempt-count bump that `ReleasePort.handBack` owes cannot be skipped by a path that returns + * early — including the two that refuse before any model run at all. + */ +async function handBack(assignment: Assignment, gaveUp: GiveUp, deps: ImplementDeps): Promise { + const { reason, attempts, usage } = gaveUp; + const released = await deps.release.handBack(assignment.ticket, giveUpNote(reason, attempts, gaveUp.test, gaveUp.summary)); + + if (!released.ok) { + // Still held by the bot and still In Progress, on purpose — see `releaseTicket`. The boot + // orphan sweep (MAPCO-11432) is what gets it back. + deps.logger.error({ msg: 'gave up but could not release', key: assignment.ticket.key, reason, releaseReason: released.reason }); + } + + deps.logger.info({ msg: 'gave up', key: assignment.ticket.key, reason, attempts, released: released.ok, ...usage }); + + return { ok: false, reason, attempts, usage, released: released.ok }; +} + +/** The ticket's prose, or the reason there is none: empty (`''`) or the read having failed. */ +type Described = { readonly ok: true; readonly text: string } | { readonly ok: false; readonly failed: string }; + +/** + * The ticket's prose, or the reason there is none. + * + * A read that throws is contained rather than escaping: the ticket is claimed at this point, and + * an exception here would leave it held with nothing said on it. Either way the answer is the + * same shape and the caller hands the ticket back without paying for a model run. + */ +async function readDescription(assignment: Assignment, deps: ImplementDeps): Promise { + try { + const text = (await deps.description.read(assignment.ticket)).trim(); + + return text === '' ? { ok: false, failed: '' } : { ok: true, text }; + } catch (error) { + deps.logger.error({ msg: 'could not read the ticket description', key: assignment.ticket.key, err: error }); + + return { ok: false, failed: error instanceof Error ? error.message : String(error) }; + } +} + +/** + * Implement one claimed ticket in one clone, and prove it with the repository's own tests. + * + * The shape of this is the point of the ticket: the model changes the code, the *worker* runs + * the tests, and only a passing run counts as done. Nothing here talks to Jira, GitHub or git + * — the change is left in the working tree for the push slice to deal with, and the only Jira + * mutation this can cause is the hand-back on give-up, which goes through `ReleasePort` + * (MAPCO-11431) rather than being written here. + * + * How the change is graded is settled before the model is handed anything: `tests.plan` reads + * the test command off the clone as cloned, and every attempt is run against that plan. Doing + * it in this order is what stops the model from editing the command that judges it — the model + * has `Write` over the whole clone, package.json included, and re-reading the command after it + * has run would make `"test": "echo ok"` the cheapest way to a green run. + * + * Two things are settled before the model is handed anything, and failing either is a refusal + * rather than an attempt: a repository that states no test command, and a ticket that carries no + * prose. The second is the easy one to get wrong — handing over a one-line summary buys a + * hand-off whose only honest answer is "there is not enough here", at full price, on every + * ticket in the queue. + * + * Three loop exits are deliberately not retried. `no-change` is not, because the input to the + * next attempt would be identical — there is no failure to feed back, so the model would read + * the same ticket and reach the same conclusion, twice as expensively. `not-verifiable` is + * not, because a repository with no test command still has none a minute later. + * `verification-changed` is not, because asking again is not an answer to it. + */ +async function implementTicket(assignment: Assignment, deps: ImplementDeps): Promise { + const { ticket, workdir } = assignment; + const { tests, logger, limits } = deps; + + let usage = NO_USAGE; + let attempts = 0; + let previousFailure: string | undefined; + let lastTest: TestRun | undefined; + let lastSummary = ''; + let reason: GiveUpReason = 'tests-failing'; + + const planned = await tests.plan(workdir); + + if (!planned.ok) { + // Refused before the model is paid for. Nothing about the change, everything about the + // repository — and it would read the same way after three attempts as after none. + logger.warn({ msg: 'repository states no test command', key: ticket.key, reason: planned.reason }); + + const refusal: TestRun = { ok: false, reason: planned.reason, command: null, output: planned.output }; + + return handBack(assignment, { reason: 'not-verifiable', attempts, usage, test: refusal, summary: '' }, deps); + } + + const described = await readDescription(assignment, deps); + + if (!described.ok) { + // The second refusal that happens before the model is paid for. A ticket with no prose is + // one the model can only guess at, and a guess is what this worker exists not to offer — + // asking it anyway costs a hand-off to be told what the ticket already said. + logger.warn({ msg: 'ticket carries no description', key: ticket.key, reason: described.failed === '' ? 'empty' : described.failed }); + + return handBack(assignment, { reason: 'no-description', attempts, usage, test: undefined, summary: described.failed }, deps); + } + + const task: AgentTask = { key: ticket.key, summary: ticket.summary, description: described.text }; + + while (attempts < limits.maxAttempts) { + attempts += 1; + + const run = await runAgent({ task, workdir, maxTurns: limits.maxTurns, previousFailure }, deps); + usage = addUsage(usage, run.usage); + lastSummary = run.summary; + + // `deniedTools` is only ever noise when it is empty. A name in it means the model went + // looking for something it was not given — worth seeing, and evidence the deny worked. + logger.info({ msg: 'agent attempt done', key: ticket.key, attempt: attempts, outcome: run.outcome, denied: run.deniedTools, ...run.usage }); + + if (run.outcome !== 'changed') { + if (run.outcome === 'no-change' && lastTest !== undefined) { + // An earlier attempt did change the tree, and its suite failed — the loop only reaches + // here with a `lastTest` in hand if that happened. That failure is the truth about this + // ticket and the diff is still in the working tree, so reporting this run's "nothing to + // do" instead would put a note on the ticket saying it was too thin to act on while a + // real change sat next to it and the failure that stopped the run went unquoted. + reason = 'tests-failing'; + } else if (run.outcome === 'no-change') { + reason = 'no-change'; + } else { + // A run that fell over keeps its own reason — an API outage must not read as a failing + // suite — but the note quotes both, because both happened. + reason = 'agent-error'; + } + + break; + } + + // The plan, not the manifest as it now stands. `tests.run` refuses if the two have parted + // company; it does not quietly follow the new one. + const test = await tests.run(workdir, planned.plan); + lastTest = test; + + if (test.ok) { + logger.info({ msg: 'change verified', key: ticket.key, attempt: attempts, command: test.command, ...usage }); + + return { ok: true, attempts, usage, command: test.command }; + } + + logger.warn({ msg: 'tests did not pass', key: ticket.key, attempt: attempts, reason: test.reason, command: test.command }); + + if (test.reason !== 'failed') { + // A rewritten test script is reported as itself rather than folded into "not verifiable". + // The first is a person's problem and the second is a repository's, and a note that + // conflated them would hide the interesting one. + reason = test.reason === 'verification-changed' ? 'verification-changed' : 'not-verifiable'; + break; + } + + previousFailure = buildFailureReport(test.command, test.output); + } + + return handBack(assignment, { reason, attempts, usage, test: lastTest, summary: lastSummary }, deps); +} + +export { DEFAULT_AGENT_LIMITS, giveUpNote, implementTicket }; +export type { GiveUp, GiveUpReason, ImplementDeps, ImplementResult }; diff --git a/src/agent/implementer.ts b/src/agent/implementer.ts new file mode 100644 index 0000000..ef3958a --- /dev/null +++ b/src/agent/implementer.ts @@ -0,0 +1,58 @@ +import type { Logger } from '@map-colonies/js-logger'; +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'; + +/** + * The whole of this slice, assembled once. + * + * `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. + * + * 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. + */ +interface ImplementerOptions { + readonly logger: Logger; + /** Comment, release, and count the attempt. See `ReleasePort` — the third step is owed. */ + readonly release: ReleasePort; + /** Where the ticket's prose comes from. See `DescriptionPort`. */ + readonly description: DescriptionPort; + /** Overridden only to spend less. The defaults are the conservative ones. */ + readonly limits?: AgentLimits; + /** Read for the API key, and stripped of the worker's own secrets before the model sees it. */ + readonly env?: NodeJS.ProcessEnv; + readonly model?: string; +} + +/** + * Everything `implementTicket` needs, built from the environment the pod was given. + * + * Throws `AgentConfigError` if there is no `ANTHROPIC_API_KEY`, which is why this belongs at + * boot and not inside a cycle: a worker with no credential cannot do the one thing it exists + * for, and finding that out mid-cycle means a ticket claimed and handed straight back. Failing + * at start-up makes it a pod that will not come up — the loudest thing a missing Secret can be. + */ +function createImplementer(options: ImplementerOptions): ImplementDeps { + const { logger, release, description, limits = DEFAULT_AGENT_LIMITS, env = process.env, model } = options; + + return { + agent: createSdkAgent(env, model), + tests: new NpmTestRunner(spawnRunner({ env })), + description, + release, + logger, + limits, + }; +} + +export { createImplementer }; +export type { ImplementerOptions }; diff --git a/src/agent/prompt.ts b/src/agent/prompt.ts new file mode 100644 index 0000000..1491e8c --- /dev/null +++ b/src/agent/prompt.ts @@ -0,0 +1,72 @@ +import type { AgentRunRequest } from './types'; + +/** + * Appended to the SDK's own coding system prompt. + * + * None of this is a security control — the tool list is (see `buildAgentOptions`), and a + * sentence asking the model not to use git would be worth nothing next to it. What this is + * for is stopping the model from *planning around* tools it does not have: a model that + * believes it can commit spends its turns trying to, reports work it did not do, and asks a + * question no one is there to answer. + * + * The last rule is the one with teeth. The worker judges the change by the repository's own + * suite, so weakening the suite is the shortest path to a pass and the one thing the check + * cannot catch by itself. + */ +const AGENT_GUARDRAILS = [ + 'You are running inside an automated worker. There is no human in this session: nothing can answer a question, approve a plan, or unblock you.', + '', + 'You have file tools only — read, search, write, edit. You have no shell, no git, no GitHub and no network tools. They are absent from your tool list, not merely discouraged, so do not plan around them and do not report work that would need them.', + '', + "Do not commit, do not create a branch, and do not push. That is the worker's job after your change passes, and it is a later step you are not part of.", + '', + "You do not run the tests either. The worker runs the repository's own test suite after you stop, and if it fails you will be given the failure and asked again. Make the change you believe is right and stop; do not claim a suite passed.", + '', + 'Work only inside the working directory you were given, and follow the conventions already in the repository over your own preferences.', + '', + 'Never weaken, skip or delete a test to make the suite pass. If the ticket does not ask for a test change, a failing test means the change is wrong.', +].join('\n'); + +/** + * The failure a retry is given, as the model sees it. + * + * Verbatim output, not a summary: the worker has no idea which line of a suite's output + * matters, and paraphrasing it would throw away the stack trace that does. + */ +function buildFailureReport(command: string | null, output: string): string { + return [`The previous attempt did not pass. \`${command ?? 'the test command'}\` reported:`, '', '```', output.trim(), '```'].join('\n'); +} + +/** + * The ticket, as the task. + * + * Summary and description and nothing else invented on top: if a ticket is too thin for a + * person to act on, the honest outcome is a failed attempt and a comment saying so, not a + * worker that guesses what was meant. + * + * The empty-description branch is a floor, not a policy: `implementTicket` hands a ticket with + * no prose back before it ever gets here, precisely so that no one pays for a hand-off to be + * told what the ticket already said. It stays because this function is also called with + * whatever a future caller has, and a prompt that silently omitted the task would be worse than + * one that says there is none. + */ +function buildTaskPrompt(request: AgentRunRequest): string { + const { task, previousFailure } = request; + const parts = [ + `Implement Jira ticket ${task.key} in the repository checked out at ${request.workdir}.`, + '', + `## ${task.key}: ${task.summary}`, + '', + task.description.trim() === '' + ? '_The ticket has no description. Work from the summary alone, or change nothing if it is not enough to act on._' + : task.description.trim(), + ]; + + if (previousFailure !== undefined && previousFailure !== '') { + parts.push('', '## Previous attempt', '', previousFailure); + } + + return parts.join('\n'); +} + +export { AGENT_GUARDRAILS, buildFailureReport, buildTaskPrompt }; diff --git a/src/agent/sdkAgent.ts b/src/agent/sdkAgent.ts new file mode 100644 index 0000000..97b6c91 --- /dev/null +++ b/src/agent/sdkAgent.ts @@ -0,0 +1,78 @@ +import { query } from '@anthropic-ai/claude-agent-sdk'; +import { readApiKey } from './apiKey'; +import { buildTaskPrompt } from './prompt'; +import { buildAgentOptions, foldMessages, type AgentQueryOptions, type AgentSettings } from './sdkOptions'; +import type { AgentPort, AgentRun, AgentRunRequest } from './types'; + +/** + * The SDK entry point, as this worker uses it. + * + * Narrower than the SDK's own `query` — a string prompt, options that are always present — + * which is what lets a test drive `SdkAgent` with a recorder and prove that the options + * `buildAgentOptions` produced are the ones the SDK is actually given. `query` is assignable to + * it, and the compiler checks that at the one place it is bound, below. + */ +type RunQuery = (params: { prompt: string; options: AgentQueryOptions }) => AsyncIterable; + +/** + * The Claude Agent SDK behind `AgentPort`. + * + * Everything interesting lives in `sdkOptions.ts`: what the model is allowed to touch, and + * how its message stream reads as an outcome. What is left here is starting the run and + * draining the stream, which is why this file is the size it is. + * + * Note what this class does *not* do. It does not retry, does not decide whether the change + * is good, and does not touch Jira: an adapter that judged its own work would put the decision + * on the far side of the seam the tests exercise. + */ +class SdkAgent implements AgentPort { + public constructor( + private readonly settings: AgentSettings, + /** Injectable so the options-to-SDK seam is testable without the network. */ + private readonly runQuery: RunQuery = query + ) { + if (settings.apiKey.trim() === '') { + // Failing here rather than at the first ticket. An empty key means the deployment's + // Secret did not arrive, and the worker discovering that mid-cycle would burn a claim. + throw new Error( + 'an Anthropic API key is required — the worker authenticates with a key from its deployment Secret, never an interactive login' + ); + } + } + + public async run(request: AgentRunRequest): Promise { + const prompt = buildTaskPrompt(request); + const options = buildAgentOptions(request, this.settings); + + // The stream is bounded by `maxTurns`, so collecting it is bounded too. Kept whole rather + // than folded as it arrives because reading it is a pure function over the run, and that + // is the part worth having tests for. + const messages: unknown[] = []; + + for await (const message of this.runQuery({ prompt, options })) { + messages.push(message); + } + + return foldMessages(messages); + } +} + +/** + * An agent wired to the environment the pod was given. + * + * The one place the credential is read, so "the worker authenticates with a key from a Secret" + * is a single line someone can check rather than a claim. There is no interactive-login path + * for this to fall back to — `readApiKey` refuses to take one — and no branch here that could + * grow one later. + * + * 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. + */ +function createSdkAgent(env: NodeJS.ProcessEnv = process.env, model?: string): SdkAgent { + return new SdkAgent({ apiKey: readApiKey(env), model, env }); +} + +export { createSdkAgent, SdkAgent }; +export type { RunQuery }; diff --git a/src/agent/sdkOptions.ts b/src/agent/sdkOptions.ts new file mode 100644 index 0000000..8e70649 --- /dev/null +++ b/src/agent/sdkOptions.ts @@ -0,0 +1,353 @@ +import { tail, withoutSecrets } from '../workspace/subprocess'; +import { AGENT_GUARDRAILS } from './prompt'; +import { NO_USAGE } from './usage'; +import type { AgentOutcome, AgentRun, AgentRunRequest, TokenUsage } from './types'; + +/** + * Everything about the Agent SDK call except the call itself. + * + * Split out from `sdkAgent.ts` so the two things worth testing — the tool surface handed to + * the model, and how a message stream is read back — can be tested as values, with no + * network, no API key and no dependency on the SDK being installed. `sdkAgent.ts` is then + * thin enough to read in one sitting, and is the only file in the repository that imports + * `@anthropic-ai/claude-agent-sdk`. + * + * The types here restate the parts of the SDK's `Options` and message shapes that the worker + * uses. That is deliberate: `sdkAgent.ts` passes this object straight to `query`, so if a + * field name or a literal ever drifts, the compiler says so at that one seam instead of the + * options silently doing nothing. + */ + +/** The model this worker uses. */ +const DEFAULT_MODEL = 'claude-opus-5'; + +/** + * The model's entire tool surface: read the repository, search it, change files, keep its own + * plan. Nothing here reaches the network or a shell. + * + * This list is the security control for MAPCO-11434's "no git and no GitHub". `tools` is the + * SDK's own base-set option — the tools not named here are never built for the session, so + * there is no `Bash` for the model to reach for, no subagent it can delegate a `git push` to, + * and no MCP server it can be handed. A sentence in a prompt asking it not to use git would + * satisfy nothing; this is not a sentence. + */ +const MODEL_TOOLS = ['Read', 'Glob', 'Grep', 'Edit', 'Write', 'NotebookEdit', 'TodoWrite'] as const; + +/** + * The tools that can change the working tree. + * + * Naming one of these is not evidence that the tree changed — only that the model tried. The + * evidence is the `tool_result` that answers the call, which is why `foldMessages` pairs the + * two rather than trusting the attempt. + */ +const WRITE_TOOLS = ['Edit', 'Write', 'NotebookEdit'] as const; + +/** + * Denied by name as well as being absent from `MODEL_TOOLS`. + * + * Belt and braces, and the braces are the interesting half: a deny rule beats every other + * step of the SDK's permission evaluation, including a permission mode that would otherwise + * approve everything, so these stay off even if someone later widens the base set or turns on + * a mode that skips prompts. The list is not an attempt at completeness — `MODEL_TOOLS` is + * what makes it complete — it names the ways a model could otherwise get a shell, git, a + * subagent, the network, or a tool the worker did not choose. + */ +const DENIED_TOOLS = [ + 'Bash', + // The Windows shell. Absent from this image, but naming only `Bash` would read as though a + // shell were the problem rather than shells. + 'PowerShell', + // Git worktrees, by tool rather than by command. + 'EnterWorktree', + 'ExitWorktree', + // Subagents. A subagent gets its own tool list, so it is the one way a restriction here + // could be widened from inside the session. `Task` is the older name for the same thing. + 'Agent', + 'Task', + 'WebFetch', + 'WebSearch', + // Skills are prompts that arrive from disk — including from the clone, which is untrusted + // input. They cannot conjure a tool that does not exist, but they can spend the budget. + 'Skill', + // Every MCP tool from every server, whatever it turns out to be called. + 'mcp__*', +]; + +/** Longest model-written summary kept. It goes in a log line and, on a give-up, on the ticket. */ +const SUMMARY_LIMIT = 2_000; +/** `Array.prototype.at` index of the last element. */ +const LAST = -1; + +/** Restated from the SDK: which on-disk settings files a session loads. */ +type SettingSourceName = 'user' | 'project' | 'local'; + +/** + * The subset of the SDK's `Options` this worker sets. + * + * Arrays are mutable because the SDK's own `Options` declares them so, and a `readonly` + * array is not assignable to one. Everything is otherwise as narrow as the SDK allows, so a + * typo in a literal like `dontAsk` fails the build rather than the run. + */ +interface AgentQueryOptions { + readonly cwd: string; + readonly model: string; + readonly maxTurns: number; + readonly tools: string[]; + readonly allowedTools: string[]; + readonly disallowedTools: string[]; + readonly permissionMode: 'dontAsk'; + readonly settingSources: SettingSourceName[]; + readonly strictMcpConfig: true; + readonly mcpServers: Record; + readonly skills: string[]; + readonly persistSession: false; + readonly includePartialMessages: false; + readonly systemPrompt: { readonly type: 'preset'; readonly preset: 'claude_code'; readonly append: string }; + readonly env: Record; +} + +interface AgentSettings { + /** + * The Anthropic API key, passed in rather than read from here. + * + * The worker authenticates with a key it was given — from an OpenShift Secret in the + * cluster — and never with an interactive login. Handing it in as a value is what makes + * that checkable: this module has no other way to authenticate, and the ambient + * login credential is stripped out of the child environment below. + */ + readonly apiKey: string; + readonly model?: string; + /** The environment the model's process derives its own from. Injectable for tests. */ + readonly env?: NodeJS.ProcessEnv; +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +function readNumber(source: Record, key: string): number { + const value = source[key]; + + return typeof value === 'number' ? value : 0; +} + +function readString(source: Record, key: string): string { + const value = source[key]; + + return typeof value === 'string' ? value : ''; +} + +/** + * The environment the model's process runs with. + * + * The SDK replaces the child environment wholesale rather than merging, so `process.env` has + * to be spread in by hand or the child loses `PATH` and `HOME`. That is also the opportunity + * to take things away: the worker's own credentials go, including the interactive-login token + * that could otherwise authenticate the run as a person, and the API key goes back in + * explicitly as the one credential the model's process is meant to have. + */ +function modelEnv(settings: AgentSettings): Record { + return { + ...withoutSecrets(settings.env ?? process.env), + // eslint-disable-next-line @typescript-eslint/naming-convention -- an environment variable name + ANTHROPIC_API_KEY: settings.apiKey, + }; +} + +/** The options one run of the model is given. */ +function buildAgentOptions(request: AgentRunRequest, settings: AgentSettings): AgentQueryOptions { + return { + cwd: request.workdir, + model: settings.model ?? DEFAULT_MODEL, + maxTurns: request.maxTurns, + tools: [...MODEL_TOOLS], + // Auto-approve exactly the tools that exist, so nothing waits on an approval no one is + // there to give. + allowedTools: [...MODEL_TOOLS], + disallowedTools: [...DENIED_TOOLS], + // Anything not pre-approved is refused outright instead of prompting. In a pod a prompt + // is not a question, it is a hang. + permissionMode: 'dontAsk', + // No settings files, from anywhere. The clone is a repository off the internet: with the + // project source enabled, its own `.claude/settings.json` would be read as permission + // rules, letting the thing being worked on widen what may be done to it. The cost is that + // the clone's CLAUDE.md is not loaded either, which is a real loss of local convention and + // the right side of the trade. + settingSources: [], + // Same reasoning for MCP: ignore the clone's `.mcp.json` and every other configured + // server, and pass none. + strictMcpConfig: true, + mcpServers: {}, + skills: [], + // Nothing resumes these sessions, and a container filesystem is not where session + // transcripts should accumulate. + persistSession: false, + includePartialMessages: false, + systemPrompt: { type: 'preset', preset: 'claude_code', append: AGENT_GUARDRAILS }, + env: modelEnv(settings), + }; +} + +/** The content blocks of a message, or nothing if it does not carry any. */ +function contentOf(message: Record): Record[] { + const inner = message['message']; + + if (!isRecord(inner) || !Array.isArray(inner['content'])) { + return []; + } + + return (inner['content'] as unknown[]).filter(isRecord); +} + +/** The ids of the write-tool calls this assistant message attempted. */ +function writeToolUseIds(message: Record): string[] { + return contentOf(message) + .filter((block) => readString(block, 'type') === 'tool_use' && (WRITE_TOOLS as readonly string[]).includes(readString(block, 'name'))) + .map((block) => readString(block, 'id')) + .filter((id) => id !== ''); +} + +/** + * The ids of the tool calls this user message reports as having succeeded. + * + * The SDK answers every `tool_use` with a `tool_result` carrying the same id, and marks the + * ones that did not work with `is_error`. A missing `is_error` means success, so this tests for + * the failure explicitly rather than reading a falsy value as a pass. + * + * A call that was denied, or that the run never got back to, has no `tool_result` at all — so + * it is absent from here, which is the answer the worker wants. + */ +function settledToolUseIds(message: Record): string[] { + return contentOf(message) + .filter((block) => readString(block, 'type') === 'tool_result' && block['is_error'] !== true) + .map((block) => readString(block, 'tool_use_id')) + .filter((id) => id !== ''); +} + +/** + * Did the model actually change the tree? + * + * Only if a write it attempted came back without an error. Reading the attempt alone was a + * real bug and a quiet one: an `Edit` whose `old_string` did not match, or a `Write` the + * permission layer refused, produced a `tool_use` block and no change, and the worker went on + * to run the clone's suite against a pristine tree, watch it pass, and report a verified diff + * that did not exist. Since certifying the diff is the entire point of this slice, the write + * has to be observed landing, not requested. + */ +function wroteFiles(messages: readonly Record[]): boolean { + const attempted = new Set(messages.filter((message) => readString(message, 'type') === 'assistant').flatMap(writeToolUseIds)); + + if (attempted.size === 0) { + return false; + } + + return messages + .filter((message) => readString(message, 'type') === 'user') + .flatMap(settledToolUseIds) + .some((id) => attempted.has(id)); +} + +/** + * Tokens and cost for the whole run. + * + * `modelUsage` rather than `usage`: the SDK documents `usage` as the main agent loop only, + * while `modelUsage` covers every model call the run made, which is what the worker is billed + * for. It is a running total carried on each result, so this reads the last one rather than + * adding them up. + */ +function usageOf(result: Record): TokenUsage { + const perModel = result['modelUsage']; + + if (!isRecord(perModel)) { + return NO_USAGE; + } + + return Object.values(perModel) + .filter(isRecord) + .reduce( + (total, model) => ({ + input: total.input + readNumber(model, 'inputTokens'), + output: total.output + readNumber(model, 'outputTokens'), + cacheRead: total.cacheRead + readNumber(model, 'cacheReadInputTokens'), + costUsd: total.costUsd + readNumber(model, 'costUSD'), + }), + NO_USAGE + ); +} + +function deniedIn(result: Record): string[] { + const denials = result['permission_denials']; + + if (!Array.isArray(denials)) { + return []; + } + + const names = (denials as unknown[]).filter(isRecord).map((denial) => readString(denial, 'tool_name')); + + return [...new Set(names)].filter((name) => name !== ''); +} + +function outcomeOf(failed: boolean, wrote: boolean): AgentOutcome { + if (failed) { + return 'gave-up'; + } + + return wrote ? 'changed' : 'no-change'; +} + +function summaryOf(result: Record, failed: boolean): string { + const errors = result['errors']; + const reported = Array.isArray(errors) ? (errors as unknown[]).map(String).join('; ') : ''; + const text = failed + ? `${readString(result, 'subtype')}: ${reported === '' ? readString(result, 'result') : reported}` + : readString(result, 'result'); + + return tail(text.trim(), SUMMARY_LIMIT); +} + +/** + * Read a finished message stream as one observable outcome. + * + * Written against `unknown` and narrowed field by field rather than against the SDK's message + * union. The union is large and moves quickly, and this only cares about four things — did a + * turn complete, did it complete well, did a file change, what did it cost. Reading it + * defensively means a new message type or a renamed sibling field cannot stop the worker, and + * it is what lets the tests drive this with plain objects. + * + * "Did a file change" is answered across two messages, not one: the assistant's `tool_use` is + * the request, and the `tool_result` in the following user message is the outcome. See + * `wroteFiles` for why the attempt on its own is not good enough. + * + * A stream with no result message at all is a give-up: the SDK emits exactly one per turn, so + * its absence means the run did not finish, whatever else arrived. + * + * Note that `gave-up` wins over `changed`. A run cut off by its turn budget may well have left + * a half-applied edit behind, and testing a half-change is worse than reporting the truth. + */ +function foldMessages(messages: readonly unknown[]): AgentRun { + const records = messages.filter(isRecord); + const wrote = wroteFiles(records); + const results = records.filter((message) => readString(message, 'type') === 'result'); + const result = results.at(LAST); + + if (result === undefined) { + return { + outcome: 'gave-up', + usage: NO_USAGE, + summary: 'The model produced no result: the run ended before the turn completed.', + deniedTools: [], + }; + } + + const failed = readString(result, 'subtype') !== 'success' || result['is_error'] === true; + + return { + outcome: outcomeOf(failed, wrote), + usage: usageOf(result), + summary: summaryOf(result, failed), + deniedTools: deniedIn(result), + }; +} + +export { buildAgentOptions, DEFAULT_MODEL, DENIED_TOOLS, foldMessages, MODEL_TOOLS, WRITE_TOOLS }; +export type { AgentQueryOptions, AgentSettings }; diff --git a/src/agent/types.ts b/src/agent/types.ts new file mode 100644 index 0000000..a8bbf29 --- /dev/null +++ b/src/agent/types.ts @@ -0,0 +1,165 @@ +import type { JiraTicket } from '../jira/types'; + +/** + * The ticket, in the words a human wrote on it. This is the whole of what the model is told + * to do — there is no separate machine-readable instruction, on purpose: a ticket a person + * cannot act on is not one the model should be guessing at either. + * + * `description` is a separate field rather than part of `JiraTicket` because the poll does not + * fetch one — it is read per claimed ticket, through `DescriptionPort`. + */ +interface AgentTask { + readonly key: string; + readonly summary: string; + readonly description: string; +} + +/** + * What one run cost. Reported per run rather than accumulated inside the port so the caller + * can add up attempts and log one number, which is what the cycle line wants (MAPCO-11437). + */ +interface TokenUsage { + readonly input: number; + readonly output: number; + readonly cacheRead: number; + /** The SDK's own cost estimate, in USD. An estimate, not a billing statement. */ + readonly costUsd: number; +} + +/** + * What a run did, in terms an outside observer can check. + * + * `changed` means the model wrote at least one file — not that the change is any good, which + * is what the test run is for. `no-change` is a real outcome and not a failure to report: a + * model that read the repo and wrote nothing has nothing to verify, and retrying it with the + * same input would produce the same nothing. `gave-up` is the run itself ending badly — the + * turn budget ran out, the API errored — as opposed to the change being wrong. + */ +type AgentOutcome = 'changed' | 'no-change' | 'gave-up'; + +interface AgentRunRequest { + readonly task: AgentTask; + /** The clone the model works in. Nothing outside it is in scope. */ + readonly workdir: string; + /** + * Turn budget for this one run. This is not the attempt bound — the attempt bound belongs + * to the caller and is how many times the model is handed the ticket at all. This is how + * far the model may go inside a single hand-off before it is cut off. + */ + readonly maxTurns: number; + /** How the previous attempt's test run failed, when this run is a retry. Verbatim output. */ + readonly previousFailure?: string; +} + +interface AgentRun { + readonly outcome: AgentOutcome; + readonly usage: TokenUsage; + /** What the model said it did, or why it stopped. Goes in the log and the give-up comment. */ + readonly summary: string; + /** + * Tools the permission layer refused mid-run, by name. + * + * Kept because it is the only in-band evidence that the restriction did its job. A run + * that reports `Bash` here means the model went looking for a shell — the change is still + * safe, but the prompt is misleading it and that is worth seeing in the pod logs. + */ + readonly deniedTools: readonly string[]; +} + +/** + * One run of the model against one ticket. + * + * Deliberately one call and no session handle: a retry is a fresh run with the test failure + * in its prompt, not a conversation that stays open. That keeps the orchestration testable + * without the SDK and means a crash between attempts loses nothing but tokens. + */ +interface AgentPort { + run: (request: AgentRunRequest) => Promise; +} + +/** The two bounds on one ticket's worth of work. Both exist to stop an unbounded spend. */ +interface AgentLimits { + /** How many times the model may be handed the ticket before the run gives up. */ + readonly maxAttempts: number; + /** Turn budget for each of those hand-offs. */ + readonly maxTurns: number; +} + +/** + * 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. + */ +interface Assignment { + readonly ticket: JiraTicket; + readonly workdir: string; +} + +/** + * 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. + * + * 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. + * + * 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. + */ +interface DescriptionPort { + read: (ticket: JiraTicket) => Promise; +} + +/** + * Whether a release held. Structurally the same as `ReleaseOutcome` in src/tickets/claim.ts, + * restated here rather than imported so this slice does not pin down a type that MAPCO-11431 + * is actively reshaping — `ReleaseOutcome` is assignable to it either way. + */ +type ReleaseResult = { readonly ok: true } | { readonly ok: false; readonly reason: string }; + +/** + * The give-up path, as narrowly as this slice needs it. + * + * Named `handBack` rather than `release` deliberately, because it is **not** `releaseTicket`. + * A hand-back is four things: count that the worker has now tried this ticket, then comment, + * transition to Open and unassign. `releaseTicket` (src/tickets/claim.ts) does the last three + * and deliberately does not count — it is also the release used by paths that must not, like the + * hand-straight-back in `runCycle`, which does no work and spends nothing. + * + * The count is what ends a loop: it lives in a Jira label (`LABELS.attemptedPrefix`, + * src/common/constants.ts) and is what `ATTEMPT_CAP` filters the poll on, so without it a ticket + * the model cannot fix is picked up again on every tick, for ever, at full token price. It + * belongs *behind this one call* rather than beside it, so that no path can release a ticket + * without counting it. + * + * A binding that only calls `releaseTicket` therefore satisfies the type and not the contract. + * Bind to `handBackTicket` (src/tickets/handBack.ts), which does both in the order that is safe + * if it fails part-way. + */ +interface ReleasePort { + handBack: (ticket: JiraTicket, note: string) => Promise; +} + +export type { + AgentLimits, + AgentOutcome, + AgentPort, + AgentRun, + AgentRunRequest, + AgentTask, + Assignment, + DescriptionPort, + ReleasePort, + ReleaseResult, + TokenUsage, +}; diff --git a/src/agent/usage.ts b/src/agent/usage.ts new file mode 100644 index 0000000..2837ba6 --- /dev/null +++ b/src/agent/usage.ts @@ -0,0 +1,21 @@ +import type { TokenUsage } from './types'; + +/** What a run costs before it has cost anything. */ +const NO_USAGE: TokenUsage = { input: 0, output: 0, cacheRead: 0, costUsd: 0 }; + +/** + * Add two runs' usage together. + * + * Attempts are separate runs with separate bills, so a ticket's cost is the sum of them — the + * number a human wants when asking what the worker spent on a ticket it then handed back. + */ +function addUsage(left: TokenUsage, right: TokenUsage): TokenUsage { + return { + input: left.input + right.input, + output: left.output + right.output, + cacheRead: left.cacheRead + right.cacheRead, + costUsd: left.costUsd + right.costUsd, + }; +} + +export { addUsage, NO_USAGE }; diff --git a/src/workspace/npmTestRunner.ts b/src/workspace/npmTestRunner.ts new file mode 100644 index 0000000..8328aff --- /dev/null +++ b/src/workspace/npmTestRunner.ts @@ -0,0 +1,159 @@ +import { access, readFile } from 'node:fs/promises'; +import { join } from 'node:path'; +import { buildTestPlan, changedScripts, dependenciesChanged, TEST_SCRIPT_PRECEDENCE } from './testCommand'; +import type { CommandResult, CommandRunner, TestPlan, TestPlanResult, TestRun, TestRunner } from './types'; + +const PASSED = 0; +const MANIFEST = 'package.json'; +const INSTALLED = 'node_modules'; +const LOCKFILE = 'package-lock.json'; +/** + * npm's own switch for "resolve and unpack, but run nobody's install hooks". + * + * Used on exactly one path — the install of a manifest the model has edited — and the reason is + * whose code would otherwise run. The clone's own hooks are snapshotted and refused if they + * moved (`changedScripts`), but a *dependency's* `postinstall` is not in the clone's manifest at + * all: it arrives with whatever package the model named. Without this flag, adding one line to + * `dependencies` is a shell in the worker's pod, which is the one thing `MODEL_TOOLS` is there + * to prevent — and a deny list of tools would look silly next to it. + * + * The cost is real and accepted: a package that needs its install hook to be usable will not + * work, so the suite fails or the import does, and the ticket is handed back for a person to + * look at. A change that needs a package with a build step is a change worth a human anyway. + */ +const IGNORE_SCRIPTS = '--ignore-scripts'; + +/** What goes on the ticket when a clone states no test command. Names what was looked for. */ +const NO_COMMAND = `Could not find a test command in ${MANIFEST}. One of these scripts must exist: ${TEST_SCRIPT_PRECEDENCE.join(', ')}.`; + +/** What goes on the ticket when the change rewrote the thing that grades it. */ +function verificationChanged(names: readonly string[]): string { + return [ + `The change edited ${MANIFEST} scripts that the worker runs to verify it: ${names.join(', ')}.`, + '', + 'The suite was not run and the change was not offered. A change that redefines its own test command cannot be verified by it,', + 'and a ticket that genuinely needs one of these scripts changed needs a person to agree to it.', + ].join('\n'); +} + +/** + * Which npm invocation installs this clone. + * + * Three cases, and only the first is about the model. `stale` means the manifest no longer + * matches the lockfile, which in this worker means one thing: the model added a dependency. That + * install resolves a package nobody has vetted, so it runs no scripts. The other two installs + * are of the repository exactly as it was cloned — the same trust level as cloning it at all — + * so they run normally, because a repo whose devDependencies need a build step has to be + * testable. + */ +function installArgs(stale: boolean, clean: boolean): string[] { + if (stale) { + return ['install', IGNORE_SCRIPTS]; + } + + return clean ? ['ci'] : ['install']; +} + +async function exists(path: string): Promise { + try { + await access(path); + + return true; + } catch { + return false; + } +} + +/** + * The clone's package.json, or null if it has none or it does not parse. + * + * Both cases read the same way on purpose: this runner only knows how to verify a Node + * repository, and a clone it cannot read a manifest from is one it cannot judge. That is a + * refusal, never an assumed pass — see `TestRun`. + */ +async function readManifest(dir: string): Promise { + try { + return JSON.parse(await readFile(join(dir, MANIFEST), 'utf8')) as unknown; + } catch { + return null; + } +} + +/** + * Runs a cloned Node repository's own test suite. + * + * The command comes from the clone (see `inferTestCommand`), and the worker runs it rather + * than the model: the model cannot report a pass it did not get, cannot skip the run, and + * cannot decide the suite was not the important part. Reviewers being the first check is the + * thing this ticket exists to stop, so the check has to belong to the side that is not also + * writing the change. + * + * The command is also *fixed before the model starts* — `plan` is read from the pristine clone + * and handed back in to `run`. Re-reading it afterwards would hand the model the marking scheme + * along with the exam; see `TestPlan`. + */ +class NpmTestRunner implements TestRunner { + public constructor(private readonly commands: CommandRunner) {} + + /** Read how this clone verifies itself. Call before the model runs, once. */ + public async plan(dir: string): Promise { + const plan = buildTestPlan(await readManifest(dir)); + + return plan === null ? { ok: false, reason: 'no-command', output: NO_COMMAND } : { ok: true, plan }; + } + + public async run(dir: string, plan: TestPlan): Promise { + const manifest = await readManifest(dir); + const changed = changedScripts(plan, manifest); + + // Before the install, not after: `npm ci` and `npm install` execute the lifecycle scripts + // themselves, so checking them afterwards would be checking whether the thing that already + // ran was allowed to. + if (changed.length > 0) { + return { ok: false, reason: 'verification-changed', command: plan.command, output: verificationChanged(changed) }; + } + + const install = await this.install(dir, plan, manifest); + + if (install !== null) { + // The suite never ran, so this says nothing about the change. Kept apart from a real + // failure so a broken registry cannot read as a broken diff. + return { ok: false, reason: 'install-failed', command: plan.command, output: install.output }; + } + + const result = await this.commands.run('npm', ['run', plan.script], dir); + + return result.code === PASSED + ? { ok: true, command: plan.command, output: result.output } + : { ok: false, reason: 'failed', command: plan.command, output: result.output }; + } + + /** Returns the failed install, or null when the clone is ready to test. */ + private async install(dir: string, plan: TestPlan, manifest: unknown): Promise { + // A ticket whose fix needs a package is an ordinary ticket, and the model has no shell to + // install one with — so the worker has to notice. Two things follow from the manifest + // having moved: the tree in `node_modules` is stale even though it exists, and `npm ci` + // would refuse outright ("package.json and package-lock.json are not in sync"), throwing + // away a working change and blaming the repository for it. + const stale = dependenciesChanged(plan, manifest); + + // Attempt two runs in the same clone as attempt one, so unchanged dependencies are already + // there. Reinstalling between attempts would be minutes of pod time per retry for a tree + // that has not moved. + if (!stale && (await exists(join(dir, INSTALLED)))) { + return null; + } + + // `npm ci` only while the lockfile still describes the manifest. Otherwise `npm install`, + // which resolves the new range and rewrites the lockfile — and the rewritten lockfile is + // part of the change the push slice will offer, which is what a human would have committed + // too. Whether that install may run install hooks is a separate question — see + // `installArgs`. + const clean = !stale && (await exists(join(dir, LOCKFILE))); + const installed = await this.commands.run('npm', installArgs(stale, clean), dir); + + return installed.code === PASSED ? null : installed; + } +} + +export { IGNORE_SCRIPTS, installArgs, NO_COMMAND, NpmTestRunner, verificationChanged }; diff --git a/src/workspace/subprocess.ts b/src/workspace/subprocess.ts new file mode 100644 index 0000000..d83ade7 --- /dev/null +++ b/src/workspace/subprocess.ts @@ -0,0 +1,128 @@ +import { spawn } from 'node:child_process'; +import type { CommandResult, CommandRunner } from './types'; + +/** Fifteen minutes. Long enough for `npm ci` plus a real suite, short enough to lose a pod to. */ +const DEFAULT_TIMEOUT_MS = 900_000; +/** + * Kept small on purpose. Everything captured here ends up in a pod log line and, on a + * failure, inside the next attempt's prompt — so it is billed as well as stored. + */ +const DEFAULT_OUTPUT_LIMIT = 8_000; +/** What `timeout(1)` uses. A killed child reports no exit code of its own. */ +const TIMED_OUT = 124; +/** No exit code at all: the binary was not there, or the cwd was not. */ +const FAILED_TO_SPAWN = 127; + +/** + * Environment variables the worker holds that nothing it runs has any business reading. + * + * Both things this module starts — the model's own process and a cloned repository's test + * script — execute code the worker did not write. A test script is arbitrary code by + * definition: running a repo's tests *is* running its code. Stripping the credentials it does + * not need is the cheap half of containing that; the other half is the pod being + * outbound-only with no inbound surface at all (MAPCO-11430). + * + * A denylist can never be complete, which is why it is not the only control. It is here to + * make the obvious mistake — the model or a test script reading `GITHUB_TOKEN` and pushing + * with it — impossible rather than merely disallowed. + * + * `NPM_TOKEN` is deliberately absent: a clone with private dependencies cannot install + * without it, and an install that cannot run is a ticket that can never be verified. + */ +const SECRET_ENV_NAMES = [ + 'ANTHROPIC_API_KEY', + 'ANTHROPIC_AUTH_TOKEN', + // An interactive-login credential. Present locally on a developer's machine, and the one + // thing that could quietly turn "authenticate with an API key" into "authenticate as + // whoever ran this". + 'CLAUDE_CODE_OAUTH_TOKEN', + 'GITHUB_TOKEN', + 'GH_TOKEN', + 'GH_PAT', + // Not a credential itself, but the worker's private write path into Jira. Nothing inside a + // clone has a reason to know it exists. + 'MCP_ATLASSIAN_URL', + 'JIRA_BOT_ACCOUNT', +] as const; + +interface SpawnRunnerOptions { + readonly timeoutMs?: number; + readonly outputLimit?: number; + /** The environment to derive the child's from. Injectable so a test can drive it with a plain object. */ + readonly env?: NodeJS.ProcessEnv; +} + +/** + * Keep the end, not the beginning. + * + * A failing suite prints its summary last, so the tail is the part that says what broke. The + * marker matters: a silently shortened log reads as a suite that stopped mid-run. + */ +function tail(text: string, limit: number): string { + if (text.length <= limit) { + return text; + } + + return `[earlier output truncated]\n${text.slice(text.length - limit)}`; +} + +/** + * A copy of `env` without the worker's secrets. + * + * `keep` is for the one process that legitimately needs one of them — the model's own, which + * needs `ANTHROPIC_API_KEY` and nothing else on the list. + */ +function withoutSecrets(env: NodeJS.ProcessEnv, keep: readonly string[] = []): NodeJS.ProcessEnv { + const scrubbed: NodeJS.ProcessEnv = { ...env }; + + for (const name of SECRET_ENV_NAMES) { + if (!keep.includes(name)) { + delete scrubbed[name]; + } + } + + return scrubbed; +} + +/** + * The real `CommandRunner`: one child process, its output captured, never a shell. + * + * `shell: false` is the point. The command and its arguments are assembled from a cloned + * repository's package.json, which is untrusted input; handing that to a shell would make a + * script *name* enough to run something else. The script *body* still runs in npm's own + * shell, because that is what running a repo's tests means — but the worker does not add a + * second injection point of its own on top of it. + * + * Failures resolve rather than reject: a non-zero exit, a timeout and a missing binary are + * all just answers about the clone, and the caller turns them into a refusal. + */ +function spawnRunner(options: SpawnRunnerOptions = {}): CommandRunner { + const { timeoutMs = DEFAULT_TIMEOUT_MS, outputLimit = DEFAULT_OUTPUT_LIMIT, env = process.env } = options; + + return { + run: async (command: string, args: readonly string[], cwd: string): Promise => + new Promise((resolve) => { + const child = spawn(command, [...args], { cwd, env: withoutSecrets(env), shell: false, timeout: timeoutMs, killSignal: 'SIGKILL' }); + const chunks: string[] = []; + const collect = (data: Buffer): void => { + chunks.push(data.toString('utf8')); + }; + + child.stdout.on('data', collect); + child.stderr.on('data', collect); + + child.on('error', (error: Error) => { + resolve({ code: FAILED_TO_SPAWN, output: tail(`${chunks.join('')}${error.message}`, outputLimit) }); + }); + + // `code` is null when the child was killed — the timeout above, or the pod going away + // mid-run. Either way it did not pass, and it must not report as exit 0. + child.on('close', (code: number | null) => { + resolve({ code: code ?? TIMED_OUT, output: tail(chunks.join(''), outputLimit) }); + }); + }), + }; +} + +export { DEFAULT_OUTPUT_LIMIT, SECRET_ENV_NAMES, spawnRunner, tail, withoutSecrets }; +export type { SpawnRunnerOptions }; diff --git a/src/workspace/testCommand.ts b/src/workspace/testCommand.ts new file mode 100644 index 0000000..b9d1164 --- /dev/null +++ b/src/workspace/testCommand.ts @@ -0,0 +1,196 @@ +import type { TestPlan } from './types'; + +/** + * The npm scripts that count as "run this repository's tests", best first. + * + * The order is a judgement about intent, not a preference: + * + * - `test:ci` first, because a repo only writes one when plain `test` is *not* the thing to + * run unattended — it is the script whose author already thought about a machine. + * - `test` next: the npm convention, what CI runs for most repos, and what a maintainer means + * by "the tests". + * - `test:unit` last, for repos that leave `test` undefined, or leave it as a watcher, and + * keep the runnable suite here. This repository is one of them in spirit — its own `test` + * runs both projects with coverage while `test:unit` is the fast half. + * + * Deliberately no `test:integration` and no `test:e2e`: those need services the worker's pod + * does not have, and a suite that cannot pass in this container must not be what the work is + * judged on. + */ +const TEST_SCRIPT_PRECEDENCE = ['test:ci', 'test', 'test:unit'] as const; + +/** + * `npm init`'s placeholder. It is present in a lot of repos that have no tests at all, and it + * exits non-zero, so taking it at face value would fail every ticket in such a repo for a + * reason that has nothing to do with the change. + */ +const NO_TEST_PLACEHOLDER = /no test specified/iu; + +/** + * The scripts npm runs by itself when the worker installs the clone's dependencies. + * + * These matter more than they look. `npm ci` and `npm install` execute them in a shell, in the + * worker's pod, and they are read out of a manifest the model has write access to — so an + * added `postinstall` is a shell the model was never given as a tool. Snapshotting them and + * refusing when they differ is what keeps `MODEL_TOOLS` from being one file edit away from + * meaningless. + * + * Listed rather than pattern-matched because npm's set is fixed and short. `prepack`/`postpack` + * are in it because `prepare` implies them for local installs in some npm versions, and being + * wrong in the harmless direction here costs a refusal, not a shell. + */ +const INSTALL_LIFECYCLE_SCRIPTS = [ + 'preinstall', + 'install', + 'postinstall', + 'preprepare', + 'prepare', + 'postprepare', + 'prepublish', + 'prepack', + 'postpack', + 'dependencies', +] as const; + +/** + * The manifest keys that decide what gets installed. + * + * `overrides` is in the list because it rewrites the resolved tree without appearing in any + * dependency block, and a change to it invalidates the lockfile exactly as a new dependency + * does. + */ +const DEPENDENCY_BLOCKS = ['dependencies', 'devDependencies', 'optionalDependencies', 'peerDependencies', 'overrides'] as const; + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; +} + +/** + * The `scripts` block, from a manifest that is only claimed to be one. + * + * Typed as `unknown` all the way down because the input is `JSON.parse` of a file from a + * cloned repository: it can be a string, a number, or an object whose `scripts` is a boolean, + * and none of that may be allowed to throw. + */ +function scriptsOf(manifest: unknown): Record { + if (!isRecord(manifest)) { + return {}; + } + + const scripts = manifest['scripts']; + + return isRecord(scripts) ? scripts : {}; +} + +/** A script body as a comparable value. Anything that is not a string reads as absent. */ +function bodyOf(scripts: Record, name: string): string | null { + const body = scripts[name]; + + return typeof body === 'string' ? body : null; +} + +/** + * The npm script that runs a cloned repository's tests, or null if it has none. + * + * Inferred from the clone's own package.json rather than assumed, because "npm test" is not + * universal even inside one organisation, and a hardcoded command turns "this repo names its + * suite differently" into "the agent cannot work on this repo". + * + * Null is a normal answer and must stay one: it means the change cannot be verified here, + * which is a reason to hand the ticket back rather than to trust an unverified diff. + */ +function inferTestCommand(manifest: unknown): string | null { + const scripts = scriptsOf(manifest); + + return ( + TEST_SCRIPT_PRECEDENCE.find((name) => { + const body = scripts[name]; + + return typeof body === 'string' && body.trim() !== '' && !NO_TEST_PLACEHOLDER.test(body); + }) ?? null + ); +} + +/** + * Every script npm will execute on the worker's behalf for one test command. + * + * The suite, the `pre`/`post` hooks npm chains around it whether or not anyone asked, and the + * install lifecycle. A hook that does not exist yet is included on purpose — `pretest` being + * absent is part of the plan, so adding one later is a change like any other. + */ +function executedScriptNames(script: string): string[] { + return [...INSTALL_LIFECYCLE_SCRIPTS, `pre${script}`, script, `post${script}`]; +} + +/** + * The dependency blocks, as one opaque string to compare later. + * + * Not parsed, not sorted, and not meant to be read: the only question ever asked of it is + * "is this still what the lockfile was built from". Key order is part of the string, so + * alphabetising the block reads as a change — which costs one `npm install` and never a wrong + * answer, and that is the right direction to be wrong in. + */ +function dependencySnapshot(manifest: unknown): string { + const source = isRecord(manifest) ? manifest : {}; + + return JSON.stringify(DEPENDENCY_BLOCKS.map((block) => source[block] ?? null)); +} + +/** + * The verification plan for a manifest, or null if it states no test command. + * + * Must be called on the clone as cloned. Calling it on a tree the model has edited is the + * defect this whole shape exists to prevent — see `TestPlan`. + */ +function buildTestPlan(manifest: unknown): TestPlan | null { + const script = inferTestCommand(manifest); + + if (script === null) { + return null; + } + + const scripts = scriptsOf(manifest); + const executed: Record = {}; + + for (const name of executedScriptNames(script)) { + executed[name] = bodyOf(scripts, name); + } + + return { script, command: `npm run ${script}`, executed, dependencies: dependencySnapshot(manifest) }; +} + +/** + * The scripts in the plan that the working tree no longer agrees with, by name. + * + * Empty is the normal answer. A non-empty one means the change under test rewrote part of how + * it is graded, or added an install hook, and the honest response is to refuse the run rather + * than to report whatever the rewritten command says. + */ +function changedScripts(plan: TestPlan, manifest: unknown): string[] { + const scripts = scriptsOf(manifest); + + return Object.entries(plan.executed) + .filter(([name, shipped]) => bodyOf(scripts, name) !== shipped) + .map(([name]) => name); +} + +/** + * Did the manifest's dependencies move since it was cloned? + * + * True means the committed lockfile no longer describes the manifest — which is the normal + * outcome of a ticket that needs a package, not a fault. The runner reads it to decide how to + * install; nothing refuses a ticket over it. + */ +function dependenciesChanged(plan: TestPlan, manifest: unknown): boolean { + return dependencySnapshot(manifest) !== plan.dependencies; +} + +export { + buildTestPlan, + changedScripts, + dependenciesChanged, + executedScriptNames, + inferTestCommand, + INSTALL_LIFECYCLE_SCRIPTS, + TEST_SCRIPT_PRECEDENCE, +}; diff --git a/src/workspace/types.ts b/src/workspace/types.ts new file mode 100644 index 0000000..bd070ce --- /dev/null +++ b/src/workspace/types.ts @@ -0,0 +1,87 @@ +/** One finished child process: what it exited with and everything it printed. */ +interface CommandResult { + readonly code: number; + /** stdout and stderr, interleaved as they arrived and capped — see `tail`. */ + readonly output: string; +} + +/** + * Running a command in a directory, as a port. + * + * Exists so the test runner can be unit-tested without spawning anything: the interesting + * behaviour is which commands it decides to run and in what order, not `child_process`. + */ +interface CommandRunner { + run: (command: string, args: readonly string[], cwd: string) => Promise; +} + +/** + * How a clone verifies itself, read off the repository *before* the model touches it. + * + * This exists because of who edits what. The model is given `Write` and `Edit` over the whole + * clone, package.json included, and the worker judges the change by a command it reads out of + * that same file. Read it after the model has run and the model is defining its own exam: the + * shortest path to a green run stops being "fix the code" and becomes `"test": "echo ok"`. + * + * So the plan is taken first and carried through every attempt. `executed` is the bodies of + * every script npm will run on the worker's behalf — the suite, its `pre`/`post` hooks, and the + * install lifecycle — as the repository shipped them; anything else in `scripts` is the + * ticket's business and may change freely. `dependencies` is there for a different reason: if + * the model added a package, the committed lockfile no longer describes the manifest, and the + * runner has to install differently (see `NpmTestRunner.install`). + */ +interface TestPlan { + /** The npm script name, chosen by `inferTestCommand` from the pristine manifest. */ + readonly script: string; + /** `npm run