diff --git a/AGENTS.md b/AGENTS.md index 654d1f64..00f38f63 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -124,6 +124,8 @@ Every reproduced race requires a failing-before and passing-after regression. As - Never infer that agent-written content is unchanged from timestamps, sizes or an index's stat cache: not every write moves them (tmpfs, for one). Compare the content itself. - Hand agent-chosen names to tools as literal data: disable or sidestep pathspec magic (a leading `:`), quote them in the tool's own input format (a leading `"` in `--stdin-paths`), pass them as bytes, and keep empty fields when splitting NUL-separated output. Check a name's safety only where it is reported, never refusing unchanged input for a name the report never carries. - Check a trust root before anything reads it. When a tool reads config or rules from storage an agent could have reached (Git and the metadata volume), compare that storage with its trusted baseline first, and run nothing if it changed. +- When one run reports a result and then acts on the same data, serialize the report before the action reads it: a read that changes data (Perl autovivification, a lazy default) must not change what the caller checks. +- When the runner predicts what an external tool will produce (a commit ID from its inputs), refuse up front any input the tool would normalize (Git writes a `-0000` offset as `+0000` and re-encodes Unicode noncharacters), so the prediction cannot fail later on valid-looking input. ## Blinded experiments diff --git a/agents/container/changes.ts b/agents/container/changes.ts index 0b4246d2..24c1cc16 100644 --- a/agents/container/changes.ts +++ b/agents/container/changes.ts @@ -1,10 +1,11 @@ +import { constants } from 'node:buffer'; import { createHash } from 'node:crypto'; import { resolveMetadataBaseline, runStorageScript, type RecoveredTaskStorage, type StorageScriptOptions, type TaskFilesystems } from './storage.ts'; import { MAXIMUM_CHANGES, MAXIMUM_DECLARED_LINKS, MAXIMUM_NAME_BYTES, MAXIMUM_TARGET_ENTRIES, MAXIMUM_TREE_OUTPUT, - TREE_SCRIPT } from './tree-script.ts'; + TASK_COMMIT_REF, TREE_SCRIPT } from './tree-script.ts'; -export { MAXIMUM_CHANGES, MAXIMUM_DECLARED_LINKS, MAXIMUM_NAME_BYTES, MAXIMUM_TARGET_ENTRIES }; +export { MAXIMUM_CHANGES, MAXIMUM_DECLARED_LINKS, MAXIMUM_NAME_BYTES, MAXIMUM_TARGET_ENTRIES, TASK_COMMIT_REF }; // Walking a large checkout holds every path in memory: more than an export needs. const INSPECTION_MEMORY = '1g'; // The scratch index an inspection hashes into holds an entry for every file of base. @@ -262,20 +263,8 @@ interface InspectOutput { readonly targets: Readonly>; } -/** - * Compare the work tree with the tree of `base` after an invocation settled, without following links, and return the - * change manifest F2a audits (#66). Content IDs are what a commit would store (see `TaskChange`). Entries Git would - * skip are listed too: new ignored files, fifos, anything under a `.git` part, and anything in a gitlink directory - * (reported in `nestedGitlinkContent`); a new directory that base's rules ignore is one entry. Each declared link is - * resolved again and each target the snapshot recorded is compared with what is there now. Runs in a read-only - * container over the storage with no network. It fails, rather than returns part of the answer, on: more than - * `MAXIMUM_CHANGES` changes; a reported name or link target longer than `MAXIMUM_NAME_BYTES`, not strict UTF-8, or - * holding a Unicode control, format, separator or unassigned character (Cc, Cf, Zl, Zp, Cn); more - * than `MAXIMUM_TARGET_ENTRIES` entries beneath the recorded targets; anything it cannot read; a `base` that is not a - * commit there; and any Git failure. F treats every refusal as needs human. - */ -export async function inspectTaskChanges(storage: TaskFilesystems | RecoveredTaskStorage, - options: InspectOptions): Promise { +// Checks an inspection's inputs and returns the script arguments after the mode: baseline, base, then the links. +function inspectionInput(storage: TaskFilesystems | RecoveredTaskStorage, options: InspectOptions) { if (typeof options.base !== 'string' || !COMMIT_ID.test(options.base)) throw new Error('base must be a full commit ID.'); const baseline = resolveMetadataBaseline(storage, options.metadataBaseline); const links = options.linkSnapshot?.links, recorded = options.linkSnapshot?.targets; @@ -286,17 +275,18 @@ export async function inspectTaskChanges(storage: TaskFilesystems | RecoveredTas const targets = [...new Set(links.flatMap(link => link.target === undefined ? [] : [link.target]))]; for (const target of targets) assertDeclaredPath(target); assertArguments([...links.map(link => link.link), ...targets]); - const stdout = await runStorageScript(storage, { kind: 'inspect', operation: 'Task change inspection', - consequence: 'changes cannot be inspected', entrypoint: 'perl', maxOutputBytes: MAXIMUM_TREE_OUTPUT + OUTPUT_SLACK, - memory: INSPECTION_MEMORY, tmpBytes: INSPECTION_TMP, - args: ['-e', TREE_SCRIPT, 'inspect', baseline, options.base, String(links.length), ...links.map(link => link.link), ...targets] }, - { ...options, timeoutMs: options.timeoutMs ?? 120_000 }); - const output = JSON.parse(stdout) as InspectOutput & { readonly metadataOnly?: boolean }; + return { baseline, links, recorded, linkArgs: [String(links.length), ...links.map(link => link.link), ...targets] }; +} + +// The manifest from what the script printed: the same for an inspection and for a commit, so the commit's digest is +// the inspection's whenever the work tree is the same. +function manifestOf(base: string, { baseline, links, recorded }: ReturnType, + output: InspectOutput & { readonly metadataOnly?: boolean }): TaskChangeManifest { // The metadata changed: no Git command ran, so nothing else was read. That alone sends the task to needs human. if (output.metadataOnly === true) { if (!DIGEST.test(output.metadataDigest) || output.metadataDigest === baseline) throw new Error('The change inspection returned an unexpected result.'); - const manifest = { base: options.base, changes: [], agentCommits: [], metadataChanged: true, linkTargetChanges: [], + const manifest = { base, changes: [], agentCommits: [], metadataChanged: true, linkTargetChanges: [], nestedGitlinkContent: [] }; return deepFreeze({ ...manifest, digest: manifestDigest(manifest) }); } @@ -325,8 +315,190 @@ export async function inspectTaskChanges(storage: TaskFilesystems | RecoveredTas } return found; }); - const manifest = { base: options.base, changes: output.changes, agentCommits: output.agentCommits, + const manifest = { base, changes: output.changes, agentCommits: output.agentCommits, metadataChanged: output.metadataDigest !== baseline, linkTargetChanges, nestedGitlinkContent: output.nestedGitlinkContent }; return deepFreeze({ ...manifest, digest: manifestDigest(manifest) }); } + +/** + * Compare the work tree with the tree of `base` after an invocation settled, without following links, and return the + * change manifest F2a audits (#66). Content IDs are what a commit would store (see `TaskChange`). Entries Git would + * skip are listed too: new ignored files, fifos, anything under a `.git` part, and anything in a gitlink directory + * (reported in `nestedGitlinkContent`); a new directory that base's rules ignore is one entry. Each declared link is + * resolved again and each target the snapshot recorded is compared with what is there now. Runs in a read-only + * container over the storage with no network. It fails, rather than returns part of the answer, on: more than + * `MAXIMUM_CHANGES` changes; a reported name or link target longer than `MAXIMUM_NAME_BYTES`, not strict UTF-8, or + * holding a Unicode control, format, separator or unassigned character (Cc, Cf, Zl, Zp, Cn); more + * than `MAXIMUM_TARGET_ENTRIES` entries beneath the recorded targets; anything it cannot read; a `base` that is not a + * commit there; and any Git failure. F treats every refusal as needs human. + */ +export async function inspectTaskChanges(storage: TaskFilesystems | RecoveredTaskStorage, + options: InspectOptions): Promise { + const input = inspectionInput(storage, options); + const stdout = await runStorageScript(storage, { kind: 'inspect', operation: 'Task change inspection', + consequence: 'changes cannot be inspected', entrypoint: 'perl', maxOutputBytes: MAXIMUM_TREE_OUTPUT + OUTPUT_SLACK, + memory: INSPECTION_MEMORY, tmpBytes: INSPECTION_TMP, + args: ['-e', TREE_SCRIPT, 'inspect', input.baseline, options.base, ...input.linkArgs] }, + { ...options, timeoutMs: options.timeoutMs ?? 120_000 }); + return manifestOf(options.base, input, JSON.parse(stdout)); +} + +/** The most bundle bytes a runner commit returns; a larger one fails the commit. */ +export const MAXIMUM_BUNDLE_BYTES = 64 * 1024 * 1024; +const MAXIMUM_MESSAGE_BYTES = 64 * 1024; +// The script's JSON and the bundle in base64 come back as one string, which V8 caps. +if (MAXIMUM_TREE_OUTPUT + OUTPUT_SLACK + Math.ceil(MAXIMUM_BUNDLE_BYTES / 3) * 4 + 1 > constants.MAX_STRING_LENGTH) + throw new Error('The commit output limit is larger than one string can hold.'); + +/** + * Who a runner commit is by, from F: D takes nothing from the environment or the repository, so the commit ID follows + * from these, the message, base and the changes alone. `date` is Git's raw form: seconds since the epoch, a space, and + * the offset (`1700000000 +0000`). + */ +export interface CommitIdentity { + readonly name: string; + readonly email: string; + readonly date: string; +} +export interface CommitOptions extends InspectOptions { + /** The `digest` of the manifest the audit approved. The commit refuses a work tree whose manifest differs. */ + readonly digest: string; + /** The message. Trailing newlines are dropped; one ends it, and the trailers follow after a blank line. */ + readonly message: string; + /** Trailer lines, in this order, as `Key: value` (for example `Plan-Item`, `Plan-Revision`). */ + readonly trailers?: Readonly>; + readonly author: CommitIdentity; + readonly committer: CommitIdentity; + /** Default and maximum `MAXIMUM_BUNDLE_BYTES`. */ + readonly maxBundleBytes?: number; +} +export interface TaskCommit { + /** The runner commit, or `base` when the manifest has no changes ("planned but unchanged"). */ + readonly head: string; + readonly unchanged: boolean; + /** + * The commit as a `git bundle` whose one ref, `TASK_COMMIT_REF`, points at `head`, with `base` as its prerequisite. + * Empty when unchanged. F fetches it into a runner-owned repository and checks that the ref is `head`. + */ + readonly bundle: Buffer; +} +/** The commit refused the work tree: it changed after the audit, or holds something the runner never commits. */ +export class TaskCommitRefused extends Error {} + +// Git drops these from either end of a name or email (ident.c, crud), which would change the commit. +const CRUD = /^[\x00-\x20.,:;<>"\\']|[\x00-\x20.,:;<>"\\']$/; +function assertIdentity(identity: CommitIdentity | undefined, role: string): void { + const { name, email, date } = identity ?? {} as Partial; + for (const [field, value] of [['name', name], ['email', email]] as const) { + if (typeof value !== 'string' || !value || !value.isWellFormed() || Buffer.byteLength(value) > 256 || CRUD.test(value) + || /[<>\p{Cc}\p{Noncharacter_Code_Point}]/u.test(value) || (field === 'email' && /\s/.test(value))) + throw new Error(`The ${role}'s ${field} must be 1 to 256 bytes of text without <, >, control characters,` + + ' noncharacters, or punctuation or space at either end.'); + } + // Git writes a zero offset as +0000, so -0000 would make another commit than the one checked here. + if (typeof date !== 'string' || !/^(?:0|[1-9][0-9]{0,11}) [+-](?:0[0-9]|1[0-4])[0-5][0-9]$/.test(date) || date.endsWith(' -0000')) + throw new Error(`The ${role}'s date must be Git's raw form: seconds since the epoch and an offset, such as "1700000000 +0000".`); +} +// Git re-encodes a message it does not take for UTF-8 (commit.c, verify_utf8), and it counts noncharacters as not UTF-8. +function commitMessage(message: string, trailers: Readonly> = {}): string { + if (typeof message !== 'string' || !message.isWellFormed() || message.includes('\0') + || /\p{Noncharacter_Code_Point}/u.test(message)) + throw new Error('The commit message must be text without NUL or Unicode noncharacters.'); + const body = message.replace(/\n+$/, ''); + if (!body.trim()) throw new Error('The commit message must not be empty.'); + const lines = Object.entries(trailers ?? {}).map(([key, value]) => { + if (!/^[A-Za-z][A-Za-z0-9-]*$/.test(key) || typeof value !== 'string' || !value.trim() || value !== value.trim() + || !value.isWellFormed() || /[\p{Cc}\p{Noncharacter_Code_Point}]/u.test(value)) + throw new Error(`Trailer ${JSON.stringify(key)} must be a token with a one-line value.`); + return `${key}: ${value}`; + }); + const text = `${body}\n${lines.length ? `\n${lines.join('\n')}\n` : ''}`; + if (Buffer.byteLength(text) > MAXIMUM_MESSAGE_BYTES) throw new Error(`The commit message must be at most ${MAXIMUM_MESSAGE_BYTES} bytes.`); + return text; +} +// The ID Git gives the commit these inputs describe: nothing else (a signature, an encoding header) may be in it. +function commitId(base: string, tree: string, author: CommitIdentity, committer: CommitIdentity, message: string): string { + const body = Buffer.from(`tree ${tree}\nparent ${base}\nauthor ${author.name} <${author.email}> ${author.date}\n` + + `committer ${committer.name} <${committer.email}> ${committer.date}\n\n${message}`); + return createHash(base.length === 64 ? 'sha256' : 'sha1').update(`commit ${body.length}\0`).update(body).digest('hex'); +} +// Git refuses a symlink named .gitmodules, in any case, anywhere in a tree. +const gitmodulesLink = (change: TaskChange) => change.newType === 'symlink' + && change.path.split('/').at(-1)!.toLowerCase() === '.gitmodules'; +// What the runner never commits, whatever the audit said: each is needs human (the same list the script will not build). +function unstorable(manifest: TaskChangeManifest): string | undefined { + if (manifest.metadataChanged) return 'the Git metadata changed since the storage was seeded'; + if (manifest.agentCommits.length) return 'the agent made commits'; + // A retargeted declared link is the item's own edit of it, which the audit judges; every other kind is a write + // through the link. + if (manifest.linkTargetChanges.some(change => change.change !== 'retargeted')) return 'a declared link\'s target changed'; + if (manifest.nestedGitlinkContent.length) return 'a gitlink directory has content'; + const change = manifest.changes.find(entry => entry.underGit || gitmodulesLink(entry) + || [entry.oldType, entry.newType].some(type => type === 'directory' || type === 'other')); + if (change) return `${JSON.stringify(change.path)} is ${change.underGit ? 'under a .git part' : gitmodulesLink(change) + ? 'a symlink named .gitmodules' : 'a directory or special file'}, which a commit cannot hold`; + return undefined; +} + +/** + * Make the runner's commit (#66): `base` plus exactly the changes of the manifest the audit approved, each file stored + * as the inspection hashed it, with the message, trailers and identities F passes. It runs the inspection again in its + * own container (read-only over the storage, no network), and from the same reads writes the commit's objects to a + * scratch store in the container's /tmp and returns them as a bounded bundle: neither volume is ever written. The + * bundle is kept only if the manifest's digest is `digest`; otherwise it refuses (`TaskCommitRefused`), as it does + * for a manifest with agent commits, a metadata change, a declared link target change other than `retargeted` (the + * item's own edit of the link, which the audit judges), gitlink content, or a change a tree cannot hold (a directory, + * a fifo or other special file, a path under `.git`, a symlink named `.gitmodules`). An empty change set makes no commit + * and returns `base`. Hooks, signing and the repository's commit encoding never apply, so the same inputs give the + * same ID, which is checked here against the ID they describe. It fails on whatever fails an inspection, on a bundle + * over `maxBundleBytes`, on new content that does not fit the container's /tmp (512 MB, within its 1 GB of memory), + * and on any Git failure. + * Takes `signal`, `onProcessGroup` and `timeoutMs` (default 300 s), and settles only after its container is gone. + */ +export async function commitTaskChanges(storage: TaskFilesystems | RecoveredTaskStorage, + options: CommitOptions): Promise { + const input = inspectionInput(storage, options); + if (typeof options.digest !== 'string' || !DIGEST.test(options.digest)) throw new Error('digest must be the manifest\'s SHA-256.'); + const message = commitMessage(options.message, options.trailers); + assertIdentity(options.author, 'author'); + assertIdentity(options.committer, 'committer'); + const maxBundleBytes = options.maxBundleBytes ?? MAXIMUM_BUNDLE_BYTES; + if (!Number.isSafeInteger(maxBundleBytes) || maxBundleBytes < 1 || maxBundleBytes > MAXIMUM_BUNDLE_BYTES) + throw new Error(`maxBundleBytes must be a positive integer of at most ${MAXIMUM_BUNDLE_BYTES}.`); + const author = { name: options.author.name, email: options.author.email, date: options.author.date }; + const committer = { name: options.committer.name, email: options.committer.email, date: options.committer.date }; + const stdout = await runStorageScript(storage, { kind: 'commit', operation: 'Runner commit', + consequence: 'nothing can be committed', entrypoint: 'perl', + maxOutputBytes: MAXIMUM_TREE_OUTPUT + OUTPUT_SLACK + Math.ceil(maxBundleBytes / 3) * 4 + 1, + memory: INSPECTION_MEMORY, tmpBytes: INSPECTION_TMP, + input: Buffer.from(JSON.stringify({ message, author, committer })), + args: ['-e', TREE_SCRIPT, 'commit', input.baseline, options.base, String(maxBundleBytes), ...input.linkArgs] }, + { ...options, timeoutMs: options.timeoutMs ?? 300_000 }); + // The inspection's JSON (a metadata-only one has no newline after it), then the commit's, then the bundle. + const first = stdout.indexOf('\n'), second = first < 0 ? -1 : stdout.indexOf('\n', first + 1); + const manifest = manifestOf(options.base, input, JSON.parse(first < 0 ? stdout : stdout.slice(0, first))); + const built = second < 0 ? undefined : JSON.parse(stdout.slice(first + 1, second)) as { readonly tree?: unknown; readonly head?: unknown }; + const encoded = second < 0 ? '' : stdout.slice(second + 1); + if (manifest.digest !== options.digest) + throw new TaskCommitRefused('The work tree no longer matches the manifest the audit approved; nothing was committed.'); + const reason = unstorable(manifest); + if (reason) throw new TaskCommitRefused(`Nothing was committed: ${reason}.`); + if (!manifest.changes.length) { + if (built !== undefined || (first >= 0 && first !== stdout.length - 1)) throw new Error('The runner commit returned a commit for no changes.'); + return Object.freeze({ head: options.base, unchanged: true, bundle: Buffer.alloc(0) }); + } + const id = new RegExp(`^[0-9a-f]{${options.base.length}}$`), { tree, head } = built ?? {}; + // The script builds nothing for what it cannot store; its list mirrors `unstorable`, and a case only it knows is a + // refusal too. + if (built === undefined) throw new TaskCommitRefused('Nothing was committed: the storage container made no commit for the changes.'); + if (typeof tree !== 'string' || !id.test(tree) || typeof head !== 'string' || !id.test(head)) + throw new Error('The runner commit returned a malformed commit.'); + if (head !== commitId(options.base, tree, author, committer, message)) + throw new Error('The runner commit is not the commit its inputs describe; nothing was committed.'); + // Decoding skips what is not base64, so the bundle must encode back to exactly the text that came. + const bundle = Buffer.from(encoded, 'base64'); + if (!bundle.length || bundle.length > maxBundleBytes || bundle.toString('base64') !== encoded) + throw new Error('The runner commit returned a malformed bundle.'); + return Object.freeze({ head, unchanged: false, bundle }); +} diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 4279d3b5..0174ecd3 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -85,14 +85,14 @@ const createDeadline = (timeoutMs: number) => { }; /** One Docker call a storage step needs run, a wait, or a point where a long walk lets other work (and an abort) in. */ type StorageStep = { readonly args: readonly string[]; readonly timeoutMs: number; readonly cancellable: boolean; - readonly maxBuffer?: number } + readonly maxBuffer?: number; readonly input?: Buffer } | { readonly sleepMs: number } | typeof PAUSE; type Steps = Generator; const PAUSE = Symbol('pause'); /** Ask the driver to run one Docker call. Allocation calls are cancellable; cleanup calls never are. */ function* run(args: readonly string[], timeoutMs: number, cancellable: boolean, - maxBuffer?: number): Steps { - return (yield { args, timeoutMs, cancellable, maxBuffer })!; + maxBuffer?: number, input?: Buffer): Steps { + return (yield { args, timeoutMs, cancellable, maxBuffer, input })!; } /** Run one Docker call that must succeed; its failure throws a `DockerError` that `createOutcomeUnknown` reads. */ function* must(args: readonly string[], timeoutMs: number): Steps { @@ -111,7 +111,8 @@ function runSteps(steps: Steps): T { else if ('sleepMs' in step) { sleep(step.sleepMs); next = steps.next(); } else { const result = spawnSync('docker', [...step.args], { encoding: 'utf8', timeout: step.timeoutMs, killSignal: 'SIGKILL', - env: dockerEnvironment(), stdio: ['ignore', 'pipe', 'pipe'], maxBuffer: step.maxBuffer ?? 16 * 1024 * 1024 }); + env: dockerEnvironment(), stdio: [step.input ? 'pipe' : 'ignore', 'pipe', 'pipe'], input: step.input, + maxBuffer: step.maxBuffer ?? 16 * 1024 * 1024 }); const error = result.error ?? (result.status === null ? new Error(`docker ${step.args[0] ?? ''} was killed by ${result.signal}.`) : undefined); next = steps.next({ status: error ? null : result.status, stdout: String(result.stdout ?? ''), @@ -145,7 +146,7 @@ async function runStepsAsync(steps: Steps, options: PreparationOptions): P } else { next = steps.next(await runInProcessGroup('docker', step.args, { env: dockerEnvironment(), timeoutMs: step.timeoutMs, signal: step.cancellable ? options.signal : undefined, - onProcessGroup: options.onProcessGroup, maxBuffer: step.maxBuffer })); + onProcessGroup: options.onProcessGroup, maxBuffer: step.maxBuffer, input: step.input })); } } return next.value; @@ -223,8 +224,8 @@ export function taskMetadataBaseline(storage: TaskFilesystems | RecoveredTaskSto } /** * The baseline to check the metadata against: D's own for storage this process allocated (a given one must match it), - * or the one F recorded, which a recovery handle needs. Nothing but a runner commit (#66 part 2) may change the - * metadata after seeding, and that commit is the last step on a storage: no check runs after it. + * or the one F recorded, which a recovery handle needs. Nothing changes the metadata after seeding: agents mount it + * read-only, and so does every runner operation, the runner commit (#66 part 2) included. */ export function resolveMetadataBaseline(storage: TaskFilesystems | RecoveredTaskStorage, given: string | undefined): string { const known = taskMetadataBaseline(storage); @@ -729,7 +730,7 @@ export const EXPORT_SCRIPT = [ /** A container run over both volumes of task storage, read-only, as the storage user, with no network. */ interface StorageScript { /** The `io.codeboost.task-storage` kind the container carries; recovery removes a leftover one. */ - readonly kind: 'export' | 'inspect'; + readonly kind: 'export' | 'inspect' | 'commit'; /** Names the operation in messages, for example "Task diff export". */ readonly operation: string; /** Ends "Task work volume is missing; ..." when a volume is gone. */ @@ -742,6 +743,8 @@ interface StorageScript { readonly memory?: '256m' | '1g'; /** The size of its /tmp. Default 64m. */ readonly tmpBytes?: '64m' | '512m'; + /** Written to the script's stdin (`docker run -i`), which is then closed; without it stdin is not connected. */ + readonly input?: Buffer; } export interface StorageScriptOptions extends PreparationOptions { /** The immutable ID of the built agent image, whose tools run the script. */ @@ -765,14 +768,14 @@ function* storageScriptSteps(workVolume: string, metadataVolume: string, owner: const name = `codeboost-${script.kind}-${randomUUID()}`; let unsettled = false; try { - const args = ['run', '--rm', '--name', name, '--label', `io.codeboost.task-storage=${script.kind}`, ...ownerLabelArgs(owner), + const args = ['run', '--rm', ...script.input ? ['-i'] : [], '--name', name, '--label', `io.codeboost.task-storage=${script.kind}`, ...ownerLabelArgs(owner), '--read-only', '--user', '10001:10001', '--network=none', '--cap-drop=ALL', '--security-opt=no-new-privileges', '--security-opt=seccomp=builtin', '--runtime=runc', '--pids-limit=64', `--memory=${script.memory ?? '256m'}`, '--cpus=.5', '--tmpfs', `/tmp:rw,nosuid,nodev,noexec,size=${script.tmpBytes ?? '64m'}`, '--mount', `type=volume,source=${workVolume},target=/work,readonly`, '--mount', `type=volume,source=${metadataVolume},target=/work/.git,readonly`, '--entrypoint', script.entrypoint, imageId, ...script.args]; - const outcome = yield* run(args, remaining(), true, script.maxOutputBytes); + const outcome = yield* run(args, remaining(), true, script.maxOutputBytes, script.input); if (outcome.status !== 0) { const error = new DockerError(args, outcome); // A client stopped before the daemon answered may still have started the container. diff --git a/agents/container/tree-script.ts b/agents/container/tree-script.ts index 4c879aea..3528859a 100644 --- a/agents/container/tree-script.ts +++ b/agents/container/tree-script.ts @@ -1,5 +1,5 @@ /** - * The Perl program D runs inside task storage to read it without following links (#66). One program serves four modes, + * The Perl program D runs inside task storage to read it without following links (#66). One program serves five modes, * so the seeder's checks and a later inspection read storage the same way: * * - `links ` (run by the seeder, as root, over what it copied) refuses, with exit 11, any link in the @@ -13,16 +13,25 @@ * - `inspect ... ...` (in /work) compares the work tree with the tree of * `base`, hashing every file as a commit would store it, resolves each declared link again (without walking its * target), and reports the state now of each target the snapshot recorded, the metadata digest and HEAD. + * - `commit ... ...` (in /work) runs the inspection, then, + * from the same reads, builds the runner commit on top of `base` from base plus the changes it found. It reads the + * message and identities as JSON on stdin and writes every object to a scratch object store in /tmp: both volumes + * stay read-only. It prints the inspection's JSON on one line, encoded before the commit is built, then the commit's + * tree and ID as JSON on the next, then the commit as a base64 `git bundle` of at most `bundle limit` bytes. It + * builds nothing when there is nothing to commit, when HEAD is not `base`, a gitlink has content, or a change is one + * a commit cannot hold (a directory, a fifo or other special file, a path under a `.git` part, or a symlink named + * `.gitmodules`); the runner refuses those. * - * Both first compare the metadata digest with `baseline`, before any Git command: if it differs, a snapshot refuses + * Snapshot, inspect and commit first compare the metadata digest with `baseline`, before any Git command: if it differs, a snapshot refuses * (exit 10) and an inspection prints only the digest and `metadataOnly`. * - * Output is one JSON document on stdout, at most MAXIMUM_TREE_OUTPUT bytes. A reported path or link target that is not + * Output is one JSON document on stdout, at most MAXIMUM_TREE_OUTPUT bytes (a commit adds its own line and the bundle). A reported path or link target that is not * strict UTF-8, holds a control, format, separator or unassigned character, or is over MAXIMUM_NAME_BYTES cannot be * put in the manifest (exit 8); an unchanged one is never checked. More than MAXIMUM_CHANGES changes, more than * MAXIMUM_TARGET_ENTRIES target entries, or more output than the bound cannot be returned whole (exit 9). A directory * or file it cannot read fails it (exit 6), since what is inside is unknown. Exit 3 is a bad base, exit 4 a Git failure, - * exit 2 a bad argument, exit 5 an unexpected failure of the script itself. None of these returns part of the answer. + * exit 2 a bad argument, exit 5 an unexpected failure of the script itself. None of these returns part of the answer: a + * commit may have printed some of its output first, but the runner discards all of it when the exit status is not 0. */ export const MAXIMUM_CHANGES = 10_000; /** Entries recorded beneath all declared links' targets together, in one snapshot or inspection. */ @@ -31,6 +40,8 @@ export const MAXIMUM_TARGET_ENTRIES = 20_000; export const MAXIMUM_DECLARED_LINKS = 200; /** Bytes in one path or link target the manifest carries; a longer one fails the run (exit 8). */ export const MAXIMUM_NAME_BYTES = 1_024; +/** The one ref a runner commit's bundle carries, pointing at the commit. */ +export const TASK_COMMIT_REF = 'refs/heads/codeboost'; // JSON at most doubles a name (a quote or backslash is escaped; control characters are refused). A change carries at // most three names (a populated gitlink, which counts as a change, one), a target entry or a link record at most five, // each with under 1 KiB of other fields. @@ -43,7 +54,7 @@ export const MAXIMUM_TREE_OUTPUT = MAXIMUM_CHANGES * (3 * 2 * MAXIMUM_NAME_BYTES export const TREE_SCRIPT = String.raw` use strict; use warnings; use Time::HiRes qw(lstat); use Digest::SHA; use JSON::PP; use Encode (); use Fcntl qw(O_RDONLY O_NOFOLLOW); -use IPC::Open2 qw(open2); +use IPC::Open2 qw(open2); use MIME::Base64 (); $SIG{__WARN__} = sub { die @_ }; my $mode = shift @ARGV // ""; my ($MAXIMUM_CHANGES, $MAXIMUM_TARGET_ENTRIES, $MAXIMUM_NAME_BYTES, $MAXIMUM_OUTPUT) = @@ -153,7 +164,7 @@ if ($mode eq "links") { chdir "/work" or fail(6, "could not enter the work tree: $!"); # Before any Git command, the metadata must be as the seeder left it: Git reads its config, and config the agent could # have changed must never run (a filter driver, say). If it changed, no Git runs at all: a snapshot refuses, and an -# inspection reports only that. +# inspection or a commit reports only that. my $baseline = shift @ARGV // ""; fail(2, "bad metadata baseline") unless $baseline =~ /^[0-9a-f]{64}\z/; my $metadata_digest = metadata_digest("/work/.git"); @@ -366,9 +377,11 @@ if ($mode eq "snapshot") { emit({ links => \@links, targets => { map { (text($_, "target") => target_state($_)) } keys %resolved_targets } }); } -fail(2, "unknown mode") unless $mode eq "inspect"; +fail(2, "unknown mode") unless $mode eq "inspect" || $mode eq "commit"; my $base = shift @ARGV; fail(3, "base is not a full commit ID") unless $base =~ /^[0-9a-f]{40}([0-9a-f]{24})?$/; +my $bundle_limit = $mode eq "commit" ? shift @ARGV : 0; +fail(2, "bad bundle limit") unless $mode ne "commit" || (defined $bundle_limit && $bundle_limit =~ /^[1-9][0-9]{0,9}\z/); # --verify -q exits 1, silently, for a name that is not a commit here; anything else is Git failing. my $exists = do { my $pid = open(my $out, "-|", @GIT, "rev-parse", "--verify", "-q", "$base^{commit}") or fail(4, "could not run git: $!"); @@ -485,21 +498,24 @@ fail(4, "git check-ignore failed (status " . ($? >> 8) . ")") if ($? >> 8) > 1 | # commit step builds its tree from base plus the manifest's changes, storing each file as hashed here; what the # manifest does not list (an unchanged submodule, a symlinked .gitattributes) stays as base has it. my @commitable = grep { $work{$_}{type} eq "file" && !under_git_path($_) } sort keys %work; +# What is gone, or is no longer the same type, leaves the scratch index first: Git reads a deleted .gitattributes from +# the index, and nothing may be hashed with rules the work tree no longer has. (git add -A is not consistent here: it +# applies a deleted .gitattributes to paths it reaches before the deletion, so its result depends on path order.) +# Every .gitattributes leaves it too, a symlink included: where Git will not read the work tree's own (a symlink, one +# over 100 MB) it falls back to the index and parses what is there as rules, even a symlink's target text. So only +# attribute files the work tree has, and Git reads, decide; a regular one is added back below like any file. +my @gone = grep { !$work{$_} || $work{$_}{type} ne $base{$_}{type} || m{(?:\A|/)\.gitattributes\z} } sort keys %base; +# The index files are hashed against: base, without what is gone. The caller sets GIT_INDEX_FILE. +sub hashing_index { + git("read-tree", $base); + return unless @gone; + open(my $removals, ">", "/tmp/removed-paths") or fail(4, "could not write the removed paths: $!"); + print $removals map { "$_\0" } @gone; close $removals or fail(4, "could not write the removed paths: $!"); + git_in("/tmp/removed-paths", 1, "update-index", "--force-remove", "-z", "--stdin"); +} if (@commitable) { local $ENV{GIT_INDEX_FILE} = "/tmp/scratch-index"; - git("read-tree", $base); - # What is gone, or is no longer the same type, leaves the scratch index first: Git reads a deleted .gitattributes from - # the index, and nothing may be hashed with rules the work tree no longer has. (git add -A is not consistent here: it - # applies a deleted .gitattributes to paths it reaches before the deletion, so its result depends on path order.) - # Every .gitattributes leaves it too, a symlink included: where Git will not read the work tree's own (a symlink, one - # over 100 MB) it falls back to the index and parses what is there as rules, even a symlink's target text. So only - # attribute files the work tree has, and Git reads, decide; a regular one is added back below like any file. - my @gone = grep { !$work{$_} || $work{$_}{type} ne $base{$_}{type} || m{(?:\A|/)\.gitattributes\z} } sort keys %base; - if (@gone) { - open(my $removals, ">", "/tmp/removed-paths") or fail(4, "could not write the removed paths: $!"); - print $removals map { "$_\0" } @gone; close $removals or fail(4, "could not write the removed paths: $!"); - git_in("/tmp/removed-paths", 1, "update-index", "--force-remove", "-z", "--stdin"); - } + hashing_index(); open(my $list, ">", "/tmp/hash-paths") or fail(4, "could not write the paths to hash: $!"); print $list map { "$_\0" } @commitable; close $list or fail(4, "could not write the paths to hash: $!"); # A path that replaces a base file or directory (docs becoming docs/api/x) replaces its entries. @@ -598,6 +614,8 @@ push @changes, grep { !$paired_add{$_->{path}} } @added; # A populated gitlink is reported too, one name each, so it counts against the same limit. fail(9, "more than $MAXIMUM_CHANGES changes; the change set is too large to inspect") if @changes + @nested > $MAXIMUM_CHANGES; @changes = sort { $a->{path} cmp $b->{path} } @changes; +# The names as bytes, for the commit: the check below turns them into text for the JSON. +my @raw = map { +{ path => $_->{path}, oldPath => $_->{oldPath}, newLinkTarget => $_->{newLinkTarget} } } @changes; # Names are checked only here, as they go into the manifest: an unchanged path with any name is never refused. for my $change (@changes) { $change->{newLinkTarget} = text($change->{newLinkTarget}, "the link target of " . shown($change->{path})) @@ -614,6 +632,138 @@ if ($head ne $base) { } my @fresh = map { resolve($_, 0) } @links; my %targets = map { (text($_, "target") => target_state($_)) } @targets; -emit({ metadataDigest => $metadata_digest, head => $head, agentCommits => \@agent_commits, - changes => \@changes, nestedGitlinkContent => [map { text($_, "path") } sort @nested], links => \@fresh, targets => \%targets }); +my %result = (metadataDigest => $metadata_digest, head => $head, agentCommits => \@agent_commits, + changes => \@changes, nestedGitlinkContent => [map { text($_, "path") } sort @nested], links => \@fresh, targets => \%targets); +emit(\%result) if $mode eq "inspect"; +# A commit reports the inspection as encoded now, before anything below reads it: nothing that builds the commit can +# change what the runner checks. +# It goes out at once, on one line (JSON::PP escapes every newline inside a string), so the build below does not hold +# it in memory beside its own work. A later failure still fails the whole run, and the runner then discards it all. +{ + my $line = $json->encode(\%result); + fail(9, "the result is larger than $MAXIMUM_OUTPUT bytes") if length $line > $MAXIMUM_OUTPUT; + binmode STDOUT; print $line, "\n"; STDOUT->flush; +} +%result = (); + +# The runner commit: base plus exactly the changes above, each stored as hashed above. Whatever they do not list stays +# as base has it (an unchanged submodule, a symlinked .gitattributes, a file whose bytes are what checkout wrote). +# Nothing is built when the runner would refuse it anyway, or when a change is not something a tree can hold. +sub storable { + my $change = shift; return 0 if $change->{underGit}; + # Git refuses a symlink named .gitmodules in any tree. + return 0 if ($change->{newType} // "") eq "symlink" && lc((split m{/}, $change->{path})[-1]) eq ".gitmodules"; + for my $key (qw(oldType newType)) { my $type = $change->{$key}; return 0 if defined $type && ($type eq "directory" || $type eq "other") } + return 1; +} +my $built; +if (@changes && $head eq $base && !@nested && !grep { !storable($_) } @changes) { + binmode STDIN; my $request = do { local $/; } // ""; + $request = eval { $json->decode($request) }; + my @identity = map { my $who = ref $request eq "HASH" ? $request->{$_} : undef; ref $who eq "HASH" ? @$who{qw(name email date)} : (undef) x 3 } qw(author committer); + # The runner checked these; a NUL cannot reach Git through the environment, nor a newline into an identity line. + fail(2, "bad commit request") if ref $request ne "HASH" || (grep { !defined $_ || ref $_ || /[\0\n]/ } @identity) + || !defined $request->{message} || ref $request->{message} || $request->{message} =~ /\0/; + # Every object goes to /tmp: the metadata stays read-only, and objects base already has are read from it. + mkdir "/tmp/objects" or fail(4, "could not create the scratch object store: $!"); + local @ENV{qw(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES)} = ("/tmp/objects", "/work/.git/objects"); + my $null = "0" x length $base; + # New content is hashed again, writing this time, in an index built as the scratch index was: each file's rules (the + # work tree's attribute files, and base's own entry for it) are the same, so each must come out as the ID above. A + # rename or a mode change stores content base already has. + my @written = grep { $changes[$_]{kind} eq "add" || $changes[$_]{kind} eq "modify" } 0 .. $#changes; + my @files = grep { $changes[$_]{newType} eq "file" } @written; + if (@files) { + local $ENV{GIT_INDEX_FILE} = "/tmp/write-index"; + hashing_index(); + open(my $list, ">", "/tmp/write-paths") or fail(4, "could not write the paths to store: $!"); + print $list map { "$raw[$_]{path}\0" } @files; close $list or fail(4, "could not write the paths to store: $!"); + git_in("/tmp/write-paths", 1, "-c", "core.protectNTFS=false", "-c", "core.protectHFS=false", + "update-index", "--add", "--replace", "-z", "--stdin"); + my %stored; + for my $record (split /\0/, git("ls-files", "-s", "-z")) { my ($meta, $path) = split /\t/, $record, 2; $stored{$path} = (split / /, $meta)[1] } + for (@files) { fail(4, "git stored " . shown($raw[$_]{path}) . " differently from how it was hashed") if ($stored{$raw[$_]{path}} // "") ne $changes[$_]{newOid} } + } + # A link stores its target text as it is. + my @links_written = grep { $changes[$_]{newType} eq "symlink" } @written; + if (@links_written) { + mkdir "/tmp/link-text" or fail(4, "could not write the link targets: $!"); + open(my $list, ">", "/tmp/link-paths") or fail(4, "could not write the link targets: $!"); + for my $i (@links_written) { + open(my $text, ">", "/tmp/link-text/$i") or fail(4, "could not write the link targets: $!"); + binmode $text; print $text $raw[$i]{newLinkTarget}; close $text or fail(4, "could not write the link targets: $!"); + print $list "/tmp/link-text/$i\n"; + } + close $list or fail(4, "could not write the link targets: $!"); + my @ids = split /\n/, git_in("/tmp/link-paths", 1, "hash-object", "-w", "--no-filters", "--stdin-paths"); + for my $n (0 .. $#links_written) { + my $i = $links_written[$n]; + fail(4, "git stored the link " . shown($raw[$i]{path}) . " differently from how it was hashed") if ($ids[$n] // "") ne $changes[$i]{newOid}; + } + } + # The tree: base, without what was deleted or renamed away, with every other change set to its new entry. + my ($tree, $commit); + { + local $ENV{GIT_INDEX_FILE} = "/tmp/commit-index"; + git("read-tree", $base); + open(my $info, ">", "/tmp/index-info") or fail(4, "could not write the tree entries: $!"); + for my $i (0 .. $#changes) { + my $gone = $changes[$i]{kind} eq "delete" ? $raw[$i]{path} : $changes[$i]{kind} eq "rename" ? $raw[$i]{oldPath} : undef; + print $info "0 $null\t$gone\0" if defined $gone; + } + for my $i (grep { $changes[$_]{kind} ne "delete" } 0 .. $#changes) { print $info "$changes[$i]{newMode} $changes[$i]{newOid}\t$raw[$i]{path}\0" } + close $info or fail(4, "could not write the tree entries: $!"); + git_in("/tmp/index-info", 1, "-c", "core.protectNTFS=false", "-c", "core.protectHFS=false", + "update-index", "--add", "--replace", "-z", "--index-info"); + $tree = git("write-tree"); chomp $tree; + } + # Only what the runner passed decides the commit: identities and dates from the request, no signature, and UTF-8 + # with no encoding header, so its ID follows from the request, the tree and base alone. + open(my $message, ">", "/tmp/commit-message") or fail(4, "could not write the commit message: $!"); + binmode $message; print $message Encode::encode("UTF-8", $request->{message}); close $message or fail(4, "could not write the commit message: $!"); + { + my @values = map { Encode::encode("UTF-8", $_) } @identity; + local @ENV{qw(GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL GIT_COMMITTER_DATE)} = + (@values[0, 1], "\@$values[2]", @values[3, 4], "\@$values[5]"); + $commit = git("-c", "commit.gpgSign=false", "-c", "i18n.commitEncoding=UTF-8", "commit-tree", "--no-gpg-sign", $tree, + "-p", $base, "-F", "/tmp/commit-message"); + chomp $commit; + } + $built = { tree => $tree, head => $commit }; +} +exit 0 unless $built; +print $json->encode($built), "\n"; +# Then the commit as a bundle, from a scratch repository of the runner's own whose only ref is the commit, reading every +# object from /tmp and the metadata. Its prerequisite is base, which the runner's host repository already has. +{ + local @ENV{qw(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_INDEX_FILE)}; + delete @ENV{qw(GIT_OBJECT_DIRECTORY GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_INDEX_FILE)}; + system(@GIT, "-c", "init.defaultBranch=codeboost", "init", "-q", "--bare", "--template=", $format eq "sha256" ? ("--object-format=sha256") : (), "/tmp/bundle.git") == 0 + or fail(4, "git init failed (" . exit_reason($?) . ")"); + open(my $alternates, ">", "/tmp/bundle.git/objects/info/alternates") or fail(4, "could not write the bundle's alternates: $!"); + print $alternates "/tmp/objects\n/work/.git/objects\n"; close $alternates or fail(4, "could not write the bundle's alternates: $!"); + system(@GIT, "--git-dir=/tmp/bundle.git", "update-ref", "${TASK_COMMIT_REF}", $built->{head}) == 0 + or fail(4, "git update-ref failed (" . exit_reason($?) . ")"); + my $pid = open(my $bundle, "-|"); fail(4, "could not run git: $!") unless defined $pid; + if (!$pid) { + open(STDERR, ">", $stderr_file) or die "could not capture git errors: $!"; + # One thread: pack-objects otherwise starts one per host CPU, past the container's process limit. + exec(@GIT, "--git-dir=/tmp/bundle.git", "-c", "pack.threads=1", "bundle", "create", "-q", "-", "${TASK_COMMIT_REF}", + "^$base") or die "could not run git: $!"; + } + # Encoded as it is read, three bytes to four, so nothing holds the whole bundle; past the limit it stops at once. + binmode $bundle; my ($size, $pending) = (0, ""); + for (;;) { + my $read = sysread($bundle, my $chunk, 3 * 65536); fail(4, "could not read the bundle: $!") unless defined $read; + last unless $read; + $size += $read; + fail(9, "the commit's bundle is larger than $bundle_limit bytes") if $size > $bundle_limit; + $pending .= $chunk; + print MIME::Base64::encode_base64(substr($pending, 0, length($pending) - length($pending) % 3, ""), ""); + } + print MIME::Base64::encode_base64($pending, ""); + close $bundle; + fail(4, "git bundle failed (" . exit_reason($?) . ")") if $?; +} +exit 0; `; diff --git a/agents/process-group.ts b/agents/process-group.ts index db7bad60..d93da450 100644 --- a/agents/process-group.ts +++ b/agents/process-group.ts @@ -20,6 +20,11 @@ export interface ProcessGroupOptions { readonly graceMs?: number; /** Bytes kept of each of stdout and stderr; more stops the group. Default 16 MiB. */ readonly maxBuffer?: number; + /** + * Written to the leader's stdin, which is then closed; without it stdin is not connected. A leader that exits before + * reading it all is not an error: what it did is in its status and output. + */ + readonly input?: Buffer; } const DEFAULT_GRACE_MS = 5_000; @@ -75,7 +80,7 @@ export function runInProcessGroup(file: string, args: readonly string[], let child: ReturnType; try { child = spawn(file, [...args], { cwd: options.cwd, env: options.env, detached: true, - stdio: ['ignore', 'pipe', 'pipe'] }); + stdio: [options.input ? 'pipe' : 'ignore', 'pipe', 'pipe'] }); } catch (error) { resolve({ status: null, stdout: '', stderr: '', error: error as Error }); return; @@ -86,6 +91,11 @@ export function runInProcessGroup(file: string, args: readonly string[], child.once('error', error => resolve({ status: null, stdout: '', stderr: '', error })); return; } + if (options.input) { + // EPIPE when the leader exits without reading everything; the leader's exit decides the outcome. + child.stdin!.on('error', () => {}); + child.stdin!.end(options.input); + } const out: Buffer[] = [], err: Buffer[] = []; let outBytes = 0, errBytes = 0, stopped: 'cancelled' | 'timeout' | 'output-limit' | 'unrecorded' | undefined; let graceTimer: ReturnType | undefined; @@ -140,6 +150,7 @@ export function runInProcessGroup(file: string, args: readonly string[], // Timers run before I/O in each event-loop turn: after a long block the timer can win while the rest of the // output is already waiting. One more turn lets that I/O (and the end of the pipes) be read first. if (!pipesClosed) await new Promise(resolveTurn => setImmediate(resolveTurn)); + child.stdin?.destroy(); child.stdout!.destroy(); child.stderr!.destroy(); const stdout = Buffer.concat(out).toString('utf8'), stderr = Buffer.concat(err).toString('utf8'); diff --git a/agents/recovery.ts b/agents/recovery.ts index 5f5cffe1..4eb4819d 100644 --- a/agents/recovery.ts +++ b/agents/recovery.ts @@ -25,8 +25,8 @@ export interface UnownedResource extends RecoveredResource { } export interface RecoveryReport { /** - * Agent containers, egress proxies, seeders, export and inspection containers and networks of this runner, now - * confirmed gone. + * Agent containers, egress proxies, seeders, export, inspection and commit containers and networks of this runner, + * now confirmed gone. */ readonly removed: readonly RecoveredResource[]; /** One handle per task-storage allocation of this runner, kept whole for export and `removeTaskFilesystems`. */ @@ -122,8 +122,8 @@ const removeById = async (resource: RecoveredResource, remaining: () => number) /** * Crash recovery for one database (#51 item 4). Acts only on objects whose `io.codeboost.runner` label is - * `runnerOwner`: it removes agent containers, egress proxies, seeders, export and inspection containers and networks, - * and resolves only once they are gone. It keeps task storage whole (volumes and keeper) and returns a recovery handle per + * `runnerOwner`: it removes agent containers, egress proxies, seeders, export, inspection and commit containers and + * networks, and resolves only once they are gone. It keeps task storage whole (volumes and keeper) and returns a recovery handle per * allocation. Objects from older builds without a runner label, and anything it does not recognise, are reported and * never touched. * @@ -184,9 +184,10 @@ export async function recoverLeftovers(runnerOwner: string, timeoutMs = 120_000) for (const resource of owned.container) { const kind = kindOf(resource); if (kind === 'storage:keeper') keep(resource, 'keeper'); - // Seeders, export and inspection containers are transient: a leftover one is removed, never kept with the storage. - else if (kind === 'storage:seeder' || kind === 'storage:export' || kind === 'storage:inspect' || kind === 'agent' - || kind === 'egress') + // Seeders, export, inspection and commit containers are transient: a leftover one is removed, never kept with the + // storage. + else if (kind === 'storage:seeder' || kind === 'storage:export' || kind === 'storage:inspect' || kind === 'storage:commit' + || kind === 'agent' || kind === 'egress') remove.push(resource); else unknown(resource); } diff --git a/docs/implementation/agent-isolation.md b/docs/implementation/agent-isolation.md index a4aa431c..c4d98d87 100644 --- a/docs/implementation/agent-isolation.md +++ b/docs/implementation/agent-isolation.md @@ -118,9 +118,30 @@ Use only these entry points to run an agent: Treat a refusal as needs human. - **Both calls** run in a read-only container with no network, take `signal`, `onProcessGroup` and `timeoutMs` (default 120 s), and settle only after their container is gone. + - **To commit what the audit approved**, call `commitTaskChanges(storage, { base, linkSnapshot, digest, message, + trailers, author, committer, imageId })`, with the approved manifest's `digest`. Identities are F's (`name`, + `email`, and `date` in Git's raw form, such as `1700000000 +0000`); nothing comes from the environment or the + repository, so the same inputs always give the same commit ID. + - It runs the inspection again in its own container and builds the commit from those same reads: `base` plus + exactly the manifest's changes, each file stored as the inspection hashed it. Ignored files the manifest lists + are committed. What the manifest does not list stays as `base` has it. + - Both volumes stay read-only. The objects go to a scratch store in the container's `/tmp`, and the commit comes + back as a `git bundle` (`bundle`) whose one ref, `refs/heads/codeboost` (`TASK_COMMIT_REF`), points at `head`, + with `base` as its prerequisite. Fetch it into a runner-owned repository and check that the ref is `head`. + - It refuses with `TaskCommitRefused` when the manifest's digest is not `digest` (the work tree changed after the + audit). It also refuses, whatever the audit said, any agent commit, metadata change, change to a declared link's + target (a `retargeted` entry is the item's own edit of the link, which the audit judges), gitlink content, and any change a tree cannot hold: a directory, a fifo or other special file, a path + under a `.git` part, or a symlink named `.gitmodules`. + - An empty change set makes no commit: `head` is `base`, `unchanged` is true and `bundle` is empty. + - It fails on whatever fails an inspection, on a bundle over `maxBundleBytes` (64 MiB at most), on new content + that does not fit the container's 512 MB `/tmp` (within its 1 GB of memory), and on any Git failure. Identities, + the message and trailer values must not hold Unicode noncharacters, which Git would re-encode, and a date's + offset of zero is written `+0000`. The default deadline is 300 s. + - Hooks, signing and the repository's commit encoding never apply. Calling it again with the same inputs on the + same storage gives the same `head`. 7. After a crash or restart, call `recoverLeftovers(runnerOwner)` (`agents/recovery.ts`) while holding the database's single-runner lock and before admitting work. It touches only objects labelled with that runner - token. It removes agent containers, egress proxies, seeders, export and inspection containers and networks, and + token. It removes agent containers, egress proxies, seeders, export, inspection and commit containers and networks, and resolves once they are gone. It keeps task storage whole (both volumes and the keeper) and returns one recovery handle per allocation, carrying its attempt and allocation IDs. `removeTaskFilesystems` accepts a handle as it accepts the value `prepareTaskFilesystems` returned. D issues a handle only after checking every part's owner labels. Objects without diff --git a/docs/implementation/pull-request-opening.md b/docs/implementation/pull-request-opening.md index 8c729123..13bc8734 100644 --- a/docs/implementation/pull-request-opening.md +++ b/docs/implementation/pull-request-opening.md @@ -120,7 +120,7 @@ Some repositories do not support draft PRs (for example private repositories on - **Repository renames and transfers.** Every PR record is kept under the configured repository name. After a rename or transfer, publish fails closed (the task's PRs are refused or reported as unsettled) until the records are moved; nothing here moves them. -- **Push.** `BranchPusher` is injected. The real push needs D's commit export (#66) and a runner-owned host repository. +- **Push.** `BranchPusher` is injected. The real push needs the runner's commits in a runner-owned host repository, fetched from the bundles `commitTaskChanges` returns (#66, #87). - **Continue from possibly already fixed.** The Continue and Cancel actions are user actions for a later slice. - **Close the draft on cancel.** The design closes the draft PR when a person cancels a needs-human task. The PR record is kept for that. - **The pre-merge check.** `GhMergeGateway` keeps its own check for now. F6 moves it onto this module. The merge check treats a PR in another repository as `unknown`; this check treats it as a match. diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index 265e0971..f7c54587 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -211,10 +211,10 @@ The server computes `retryable` and sends it to the UI. The UI never works it ou 3. **If the job holds an in-memory first reason** (its earlier write failed), skip publication: settle the row through the first-reason mapping in rule 2, passing that reason. Otherwise, in one `Store` transaction: confirm that the attempt ID is the task's current attempt, the state is `running`, there is no first reason, the captured context is still current, the task's status is `running`, and `tasks.cancel_requested` is null (the same guard as the `running → completed` row). Do not compare the task's state version here; the attempt's own transitions have increased it. Then write `completed` and the result, and increase the state version. 4. If step 3 refuses, reread durable state and settle the row by the rules above. Keep the original diagnostic. 5. For a completed writable attempt, F2's result extraction is split in two: - - Step 2, before any `Store` write, makes the commit object inside the still-present task storage. It records nothing in the `Store`. + - Step 2, before any `Store` write, makes the commit while the task storage is still there: `commitTaskChanges` (#66) with the approved manifest's digest returns the commit as a bundle, and F fetches it into its runner-owned repository and checks the SHA. It records nothing in the `Store`. Removing the storage later loses nothing. - The step-3 transaction first runs every guard against the captured context. Then, in the same transaction, it writes `completed` and records the ledger entry (`recordHistory`), which increases the context generation. Because the guard runs before that increase inside one transaction, the attempt's own ledger write never makes it non-current. - If step 3 refuses, nothing reaches the `Store`, and the unpublished commit is discarded when the task storage is removed. Then, for every writable attempt, call `removeTaskFilesystems` after the terminal write, whatever the outcome. Free the slot only after it succeeds. If it fails, keep an unresolved marker, and startup recovery removes the storage. + If step 3 refuses, nothing reaches the `Store`. The unpublished commit is already in the runner-owned repository, so F deletes the ref it fetched it under; nothing else points at it. Then, for every writable attempt, call `removeTaskFilesystems` after the terminal write, whatever the outcome. Free the slot only after it succeeds. If it fails, keep an unresolved marker, and startup recovery removes the storage. **Irreversible actions.** Before each commit, push, PR open or merge, re-read the task's state version, the plan's `review_version` (which `saveReview` and `addReviewNote` advance) and the coordinator's `closing` flag **after the final await**. Stop if any of them changed. For a merge this means an approval or choice edit made during the final GitHub check blocks the merge. A check made before an await does not count. (This follows the AGENTS.md rules on guarded external actions.) diff --git a/runner/execution.ts b/runner/execution.ts index 41a9bfcc..98321a58 100644 --- a/runner/execution.ts +++ b/runner/execution.ts @@ -2,6 +2,7 @@ import { identityKey, type PlanIdentity } from '../core/identity.ts'; import type { PlanContext } from '../core/plan.ts'; import type { InvocationContext, InvocationHandle, InvocationInput, TaskClone } from '../agents/contract.ts'; import { prepareExecution } from '../core/execution-prompt.ts'; +import { neutralizeMentions, neutralizeReferences } from '../core/pull-request-body.ts'; import { auditRun, type ChangeManifest } from '../core/run-audit.ts'; import { FinishFailure, NEEDS_RESTART, PreparationFailure, type PreparedAttempt, type RunnerCoordinator, type RunnerDeps } from './coordinator.ts'; import type { AttemptRecord, Store } from './store.ts'; @@ -116,8 +117,9 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag try { head = await workspace.commit(data.workspace, { baseHead: data.baseHead, paths, digest: manifest.digest, // The title is plan text: on one line with no control or bidi/format characters, it cannot open a trailer block - // that forges Plan-Item or Plan-Revision, put terminal escapes into git log, or reorder how git log shows it. - message: `${item.id}: ${item.title.replace(/[\u0000-\u001f\u007f-\u009f\u200b-\u200f\u2028-\u202e\u2060-\u206f\ufeff]+/g, ' ').replace(/ {2,}/g, ' ').trim()}`, trailers: { 'Plan-Item': item.id, 'Plan-Revision': `r${plan.revision}` }, + // that forges Plan-Item or Plan-Revision, put terminal escapes into git log, or reorder how git log shows it. Its + // issue references and mentions are neutralised (AGENTS.md): on the default branch, "Fixes #12" would close #12. + message: `${item.id}: ${neutralizeMentions(neutralizeReferences(item.title.replace(/[\u0000-\u001f\u007f-\u009f\u200b-\u200f\u2028-\u202e\u2060-\u206f\ufeff]+/g, ' ').replace(/ {2,}/g, ' ').trim()))}`, trailers: { 'Plan-Item': item.id, 'Plan-Revision': `r${plan.revision}` }, }, signal); } catch (error) { // D's refusal text can name agent-chosen paths: quote it (AGENTS.md). A stop records its first reason before it diff --git a/test/agent-container.test.ts b/test/agent-container.test.ts index c28b987f..2694d5f7 100644 --- a/test/agent-container.test.ts +++ b/test/agent-container.test.ts @@ -15,7 +15,8 @@ import { createTaskClone } from '../git/clone.ts'; import { hasOwnerLabels } from '../agents/labels.ts'; import { exportTaskDiff, isRecoveredTaskStorage, prepareTaskFilesystemsAsync, taskFilesystemOwner, EXPORT_SCRIPT, UnusableRepositoryError } from '../agents/container/storage.ts'; -import { inspectTaskChanges, manifestDigest, MAXIMUM_CHANGES, MAXIMUM_NAME_BYTES, snapshotDeclaredLinks } from '../agents/container/changes.ts'; +import { commitTaskChanges, inspectTaskChanges, manifestDigest, MAXIMUM_CHANGES, MAXIMUM_NAME_BYTES, snapshotDeclaredLinks, + TASK_COMMIT_REF, TaskCommitRefused, type TaskChangeManifest } from '../agents/container/changes.ts'; import { TREE_SCRIPT } from '../agents/container/tree-script.ts'; import { recoverLeftovers } from '../agents/recovery.ts'; import { createVendorNetwork, removeVendorNetwork, VendorNetworkCreationCleanupError, @@ -37,11 +38,12 @@ const docker = (...args: string[]) => execFileSync('docker', args, { }).trim(); function fixture(options: { limits?: Parameters[1]; historyBytes?: number; - hostile?: (source: string, root: string) => void; beforeSeed?: (clone: string) => void } = {}) { + hostile?: (source: string, root: string) => void; beforeSeed?: (clone: string) => void; objectFormat?: 'sha256' } = {}) { const root = mkdtempSync(join(tmpdir(), 'agent-container-')); roots.push(root); const source = join(root, 'source'), staging = join(root, 'staging'), input = join(root, 'input'); mkdirSync(source); mkdirSync(staging); mkdirSync(input); - git(source, 'init'); git(source, 'config', 'user.name', 'Test'); git(source, 'config', 'user.email', 'test@example.com'); + git(source, 'init', ...options.objectFormat ? [`--object-format=${options.objectFormat}`] : []); + git(source, 'config', 'user.name', 'Test'); git(source, 'config', 'user.email', 'test@example.com'); if (options.historyBytes) { // Incompressible history that no longer exists in the checked-out worktree. writeFileSync(join(source, 'history.bin'), randomBytes(options.historyBytes)); @@ -1457,6 +1459,192 @@ describe('real Docker agent isolation', () => { }, 300_000); }); + describe('runner commit (#66)', () => { + const noLinks = { links: [], targets: {} }; + const runner = { name: 'codeboost', email: 'runner@codeboost.invalid', date: '1700000000 +0130' }; + const commit = (data: ReturnType, manifest: TaskChangeManifest, + options: Partial[1]> = {}) => commitTaskChanges(data.filesystems, { + base: manifest.base, linkSnapshot: noLinks, imageId, digest: manifest.digest, message: 'P1: Add things', + trailers: { 'Plan-Item': 'P1', 'Plan-Revision': 'r1' }, author: runner, committer: runner, ...options }); + // F's side: fetch the bundle into a runner-owned repository (the source stands in) and check its one ref. + const fetch = (data: ReturnType, bundle: Buffer) => { + const file = join(data.root, `${randomUUID()}.bundle`), ref = `refs/heads/fetched-${randomUUID()}`; + writeFileSync(file, bundle); + git(data.source, 'bundle', 'verify', '-q', file); + expect(git(data.source, 'bundle', 'list-heads', file).split('\n').map(line => line.split(' ')[1])).toEqual([TASK_COMMIT_REF]); + git(data.source, '-c', 'protocol.file.allow=always', 'fetch', '-q', file, `${TASK_COMMIT_REF}:${ref}`); + return git(data.source, 'rev-parse', ref); + }; + const tree = (repository: string, commit: string) => new Map(git(repository, 'ls-tree', '-r', '-z', '--full-tree', commit) + .split('\0').filter(Boolean).map(record => { + const [meta, path] = record.split('\t') as [string, string]; const [mode, , oid] = meta.split(' ') as [string, string, string]; + return [path, { mode, oid }]; + })); + const blob = (repository: string, oid: string) => execFileSync('git', ['cat-file', 'blob', oid], { cwd: repository, env: { PATH: process.env.PATH } }); + // Base plus exactly the manifest's changes: every change has its new entry, and nothing else differs from base. + const expectTreeOf = (data: ReturnType, manifest: TaskChangeManifest, head: string) => { + const expected = tree(data.source, manifest.base); + for (const change of manifest.changes) { + if (change.kind === 'delete') expected.delete(change.path); + if (change.oldPath) expected.delete(change.oldPath); + if (change.kind !== 'delete') { + // A path that replaces a base directory replaces what was under it. + for (const path of expected.keys()) if (path.startsWith(`${change.path}/`)) expected.delete(path); + expected.set(change.path, { mode: change.newMode!, oid: change.newOid! }); + } + } + expect(tree(data.source, head)).toEqual(expected); + }; + + it('commits base plus exactly the manifest\'s changes, as a bundle F can fetch, and the same inputs give the same commit', async () => { + const data = fixture({ hostile: source => { + mkdirSync(join(source, 'dir')); + for (const [name, text] of [['keep.txt', 'keep'], ['edit.txt', 'edit'], ['gone.txt', 'gone'], ['move-me.txt', 'move'], + ['script.sh', 'echo'], ['dir/a.txt', 'a']] as const) writeFileSync(join(source, name), `${text}\n`); + writeFileSync(join(source, '.gitignore'), '*.log\n'); symlinkSync('keep.txt', join(source, 'link')); + } }); + const base = data.clone.head; + asAgent(data.filesystems, ['printf "edited\\n" > edit.txt', 'rm gone.txt', 'mv move-me.txt moved.txt', 'chmod +x script.sh', + 'ln -sfn edit.txt link', 'mkdir -p new/deep && printf "new\\n" > new/deep/file.txt', 'printf "log\\n" > out.log', + 'rm -r dir && printf "now a file\\n" > dir', 'ln -s keep.txt newlink', `printf "q\\n" > '"quoted"'`].join(' && ')); + const manifest = await inspectTaskChanges(data.filesystems, { base, linkSnapshot: noLinks, imageId }); + expect(manifest.changes.map(change => [change.kind, change.path])).toEqual(expect.arrayContaining([['modify', 'edit.txt'], + ['delete', 'gone.txt'], ['rename', 'moved.txt'], ['mode', 'script.sh'], ['modify', 'link'], ['add', 'out.log'], + ['delete', 'dir/a.txt'], ['add', 'dir'], ['add', 'newlink'], ['add', '"quoted"']])); + const made = await commit(data, manifest); + expect(made.unchanged).toBe(false); + expect(fetch(data, made.bundle)).toBe(made.head); + expectTreeOf(data, manifest, made.head); + const committed = tree(data.source, made.head); + expect(blob(data.source, committed.get('edit.txt')!.oid).toString()).toBe('edited\n'); + expect(committed.get('script.sh')!.mode).toBe('100755'); + expect(committed.get('link')).toEqual({ mode: '120000', oid: expect.any(String) }); + expect(blob(data.source, committed.get('link')!.oid).toString()).toBe('edit.txt'); + expect(blob(data.source, committed.get('out.log')!.oid).toString()).toBe('log\n'); + // Exactly the commit the inputs describe: no signature, no encoding header, the identities and message as given. + expect(git(data.source, 'cat-file', 'commit', made.head)).toBe([`tree ${git(data.source, 'rev-parse', `${made.head}^{tree}`)}`, + `parent ${base}`, 'author codeboost 1700000000 +0130', + 'committer codeboost 1700000000 +0130', '', 'P1: Add things', '', 'Plan-Item: P1', 'Plan-Revision: r1'].join('\n')); + expect((await commit(data, manifest)).head).toBe(made.head); + // Neither volume was written: the work tree and the metadata are as the audit saw them. + const again = await inspectTaskChanges(data.filesystems, { base, linkSnapshot: noLinks, imageId }); + expect(again.digest).toBe(manifest.digest); + expect(again.metadataChanged).toBe(false); + expect(docker('ps', '-a', '-q', '--filter', `label=io.codeboost.allocation=${taskFilesystemOwner(data.filesystems).allocationId}`, + '--filter', 'label=io.codeboost.task-storage=commit')).toBe(''); + }, 300_000); + + it('commits type changes, gitlinks, names that look like options or pathspec magic, and a large incompressible file', async () => { + const data = fixture({ limits: { workBytes: 64 * 1024 * 1024, workInodes: 512, metadataBytes: 16 * 1024 * 1024, metadataInodes: 512 }, + hostile: source => { + for (const name of ['to-link', '-opt', ':(glob)x']) writeFileSync(join(source, name), `${name}\n`); + symlinkSync('to-link', join(source, 'to-file')); + for (const sm of ['sm-gone', 'sm-file']) { mkdirSync(join(source, sm)); git(source, 'update-index', '--add', '--cacheinfo', `160000,${'1'.repeat(40)},${sm}`); } + } }); + asAgent(data.filesystems, ['rm to-link && ln -s -- -opt to-link', 'rm to-file && printf "file\\n" > to-file', + 'rmdir sm-gone', 'rmdir sm-file && printf "was a submodule\\n" > sm-file', 'printf "edited\\n" > ./-opt', + `printf "edited\\n" > ':(glob)x' && mkdir -p ':' && printf "n\\n" > ':/new'`, + 'head -c 8388608 /dev/urandom > big.bin'].join(' && ')); + const manifest = await inspectTaskChanges(data.filesystems, { base: data.clone.head, linkSnapshot: noLinks, imageId }); + expect(manifest.changes.map(change => [change.path, change.oldType, change.newType])).toEqual(expect.arrayContaining([ + ['to-link', 'file', 'symlink'], ['to-file', 'symlink', 'file'], ['sm-gone', 'gitlink', undefined], ['sm-file', 'gitlink', 'file']])); + const made = await commit(data, manifest); + // Incompressible: the bundle is larger than the 8 MiB file, so it is decoded and checked whole. + expect(made.bundle.length).toBeGreaterThan(8 * 1024 * 1024); + expect(fetch(data, made.bundle)).toBe(made.head); + expectTreeOf(data, manifest, made.head); + const committed = tree(data.source, made.head); + expect(committed.get('to-link')!.mode).toBe('120000'); + expect(blob(data.source, committed.get('to-link')!.oid).toString()).toBe('-opt'); + expect(committed.get('sm-file')!.mode).toBe('100644'); + expect(committed.has('sm-gone')).toBe(false); + expect(blob(data.source, committed.get(':/new')!.oid).toString()).toBe('n\n'); + }, 300_000); + + it('stores each file as the inspection hashed it: line endings, attribute rules, and a symlinked .gitattributes', async () => { + const data = fixture({ hostile: source => { + // Committed with CRLF before text=auto: git add keeps such a file's CRLF. + writeFileSync(join(source, 'crlf.txt'), 'a\r\n'); git(source, 'add', 'crlf.txt'); git(source, 'commit', '-m', 'crlf'); + writeFileSync(join(source, '.gitattributes'), '* text=auto\nwin.txt eol=crlf\n'); + writeFileSync(join(source, 'win.txt'), 'one\ntwo\n'); + mkdirSync(join(source, 'sub')); writeFileSync(join(source, 'sub', 'x.txt'), 'x\n'); + // Git will not read a symlinked one: it stays as base has it, and decides nothing. + symlinkSync('../.gitattributes', join(source, 'sub', '.gitattributes')); + } }); + asAgent(data.filesystems, ['printf "a\\r\\nb\\r\\n" > crlf.txt', 'printf "one\\r\\nTWO\\r\\n" > win.txt', + 'printf "x\\r\\ny\\r\\n" > sub/x.txt'].join(' && ')); + const manifest = await inspectTaskChanges(data.filesystems, { base: data.clone.head, linkSnapshot: noLinks, imageId }); + expect(manifest.changes.map(change => change.path)).toEqual(['crlf.txt', 'sub/x.txt', 'win.txt']); + const made = await commit(data, manifest); + fetch(data, made.bundle); + expectTreeOf(data, manifest, made.head); + const committed = tree(data.source, made.head); + expect(blob(data.source, committed.get('crlf.txt')!.oid).toString()).toBe('a\r\nb\r\n'); + expect(blob(data.source, committed.get('win.txt')!.oid).toString()).toBe('one\nTWO\n'); + expect(blob(data.source, committed.get('sub/x.txt')!.oid).toString()).toBe('x\ny\n'); + expect(committed.get('sub/.gitattributes')!.mode).toBe('120000'); + }, 300_000); + + it('makes no commit for an empty change set, and commits in a SHA-256 repository', async () => { + const quiet = fixture(); + const none = await inspectTaskChanges(quiet.filesystems, { base: quiet.clone.head, linkSnapshot: noLinks, imageId }); + expect(await commit(quiet, none)).toEqual({ head: quiet.clone.head, unchanged: true, bundle: Buffer.alloc(0) }); + const data = fixture({ objectFormat: 'sha256', hostile: source => writeFileSync(join(source, 'old.txt'), 'old\n') }); + expect(data.clone.head).toMatch(/^[0-9a-f]{64}$/); + asAgent(data.filesystems, 'rm old.txt && printf "changed\\n" > file.txt && ln -s file.txt l'); + const manifest = await inspectTaskChanges(data.filesystems, { base: data.clone.head, linkSnapshot: noLinks, imageId }); + const made = await commit(data, manifest); + expect(made.head).toMatch(/^[0-9a-f]{64}$/); + expect(fetch(data, made.bundle)).toBe(made.head); + expectTreeOf(data, manifest, made.head); + }, 300_000); + + it('refuses a work tree that changed after the audit, and fails closed on a bundle over its limit', async () => { + const data = fixture(); + asAgent(data.filesystems, 'printf "audited\\n" > file.txt'); + const manifest = await inspectTaskChanges(data.filesystems, { base: data.clone.head, linkSnapshot: noLinks, imageId }); + // A write after the audit: the commit's own inspection sees it, and nothing the audit did not see is committed. + asAgent(data.filesystems, 'printf "later\\n" > file.txt'); + const refused = await commit(data, manifest).then(() => undefined, error => error); + expect(refused).toBeInstanceOf(TaskCommitRefused); + expect(refused.message).toContain('no longer matches the manifest the audit approved'); + const current = await inspectTaskChanges(data.filesystems, { base: data.clone.head, linkSnapshot: noLinks, imageId }); + await expect(commit(data, current, { maxBundleBytes: 64 })).rejects.toThrow('bundle is larger than 64 bytes'); + expect(docker('ps', '-a', '-q', '--filter', `label=io.codeboost.allocation=${taskFilesystemOwner(data.filesystems).allocationId}`, + '--filter', 'label=io.codeboost.task-storage=commit')).toBe(''); + }, 300_000); + + it('refuses what the runner never commits, even when the audit approved it: agent commits, gitlink content, link target, metadata, special files and .git parts', async () => { + const data = fixture({ hostile: source => { + writeFileSync(join(source, 'first.txt'), 'first\n'); git(source, 'add', 'first.txt'); git(source, 'commit', '-m', 'first'); + mkdirSync(join(source, 'sm')); git(source, 'update-index', '--add', '--cacheinfo', `160000,${'1'.repeat(40)},sm`); + writeFileSync(join(source, 'target.txt'), 't\n'); symlinkSync('target.txt', join(source, 'link')); + } }); + const refusal = async (reason: string, base = data.clone.head, linkSnapshot: Parameters[1]['linkSnapshot'] = noLinks) => { + const manifest = await inspectTaskChanges(data.filesystems, { base, linkSnapshot, imageId }); + const error = await commit(data, manifest, { linkSnapshot }).then(() => undefined, caught => caught); + expect(error).toBeInstanceOf(TaskCommitRefused); + expect(error.message).toContain(reason); + }; + // The storage's HEAD past base stands in for an agent commit, which the read-only metadata prevents. + await refusal('the agent made commits', git(data.source, 'rev-parse', 'HEAD~1')); + asAgent(data.filesystems, 'mkfifo pipe'); + await refusal('"pipe" is a directory or special file'); + asAgent(data.filesystems, 'rm pipe && mkdir -p out/.git && printf "p\\n" > out/.git/payload'); + await refusal('"out/.git/payload" is under a .git part'); + asAgent(data.filesystems, 'rm -r out && printf "work\\n" > sm/work.c'); + await refusal('a gitlink directory has content'); + asAgent(data.filesystems, 'rm sm/work.c'); + const linkSnapshot = await snapshotDeclaredLinks(data.filesystems, ['link'], { imageId }); + asAgent(data.filesystems, 'printf "through\\n" > link'); + await refusal('a declared link\'s target changed', data.clone.head, linkSnapshot); + docker('run', '--rm', '--network=none', '--user', '10001:10001', + '--mount', `type=volume,source=${data.filesystems.metadataVolume},target=/work/.git`, '--entrypoint', 'chmod', imageId, + '600', '/work/.git/config'); + await refusal('the Git metadata changed'); + }, 300_000); + }); + it('names what the diff cannot show: a submodule directory with content, and attributes that rewrite bytes', async () => { // The base commit has a submodule, which the non-recursive task clone leaves as an empty directory, and a tracked // file with an $Id$ keyword. diff --git a/test/agent-process-group.test.ts b/test/agent-process-group.test.ts index 844a2004..eb686c50 100644 --- a/test/agent-process-group.test.ts +++ b/test/agent-process-group.test.ts @@ -24,6 +24,30 @@ describe('runInProcessGroup', () => { expect(groupAlive(reported!.pgid)).toBe(false); }); + it('writes the input to the leader\'s stdin and closes it, and leaves stdin unconnected without one', async () => { + const input = Buffer.concat([Buffer.from('line\n\0'), Buffer.alloc(256 * 1024, 'x')]); + const read = await runInProcessGroup('sh', ['-c', 'wc -c'], { env, timeoutMs: 10_000, input }); + expect(read).toMatchObject({ status: 0 }); + expect(Number(read.stdout.trim())).toBe(input.length); + expect(await runInProcessGroup('sh', ['-c', 'wc -c'], { env, timeoutMs: 10_000 })).toMatchObject({ status: 0, stdout: expect.stringMatching(/^\s*0\n$/) }); + // A leader that never reads its input: the broken pipe is not an error, and the outcome is its own. + const ignored = await runInProcessGroup('sh', ['-c', 'exec 0<&-; exit 4'], { env, timeoutMs: 10_000, input: Buffer.alloc(1024 * 1024) }); + expect(ignored).toMatchObject({ status: 4 }); + expect(ignored.error).toBeUndefined(); + }); + + it('settles an abort while the input is still being written, with no error from the unread input', async () => { + const controller = new AbortController(); + let group: ProcessGroup | undefined; + setTimeout(() => controller.abort(), 200); + // More than a pipe holds, to a leader that never reads it: the write is still pending at the abort. + const outcome = await runInProcessGroup('sleep', ['60'], { env, timeoutMs: 30_000, signal: controller.signal, + input: Buffer.alloc(4 * 1024 * 1024), onProcessGroup: reported => { group = reported; } }); + expect(outcome.status).toBeNull(); + expect((outcome.error as NodeJS.ErrnoException).code).toBe('ABORT_ERR'); + expect(groupAlive(group!.pgid)).toBe(false); + }); + it('kills what the leader left in its group before settling, even after a normal exit', async () => { let group: ProcessGroup | undefined; const began = performance.now(); diff --git a/test/agent-recovery.test.ts b/test/agent-recovery.test.ts index 85eb0627..4533eb17 100644 --- a/test/agent-recovery.test.ts +++ b/test/agent-recovery.test.ts @@ -161,7 +161,7 @@ describe('recoverLeftovers', () => { expect(names()).toContain(agent!.name); }); - it.each(['export', 'inspect'])('removes a leftover %s container with the other transient containers, keeping the storage', async kind => { + it.each(['export', 'inspect', 'commit'])('removes a leftover %s container with the other transient containers, keeping the storage', async kind => { const mine = attempt(A, 'attempt-a'), storageLabels = mine.objects[0]!.labels; const transient: FakeObject = { kind: 'container', id: id(), name: `codeboost-${kind}-${randomUUID()}`, labels: { ...storageLabels, 'io.codeboost.task-storage': kind } }; diff --git a/test/agent-storage-async.test.ts b/test/agent-storage-async.test.ts index ec9e1b73..3076ad9e 100644 --- a/test/agent-storage-async.test.ts +++ b/test/agent-storage-async.test.ts @@ -1,5 +1,5 @@ import { fixtureGit } from './fixtures/git.ts'; -import { randomUUID } from 'node:crypto'; +import { createHash, randomUUID } from 'node:crypto'; import { existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -34,8 +34,8 @@ vi.mock('../agents/container/image.ts', async importOriginal => ({ const { adoptRecoveredTaskStorage, exportTaskDiff, hasLiveTaskStorage, prepareTaskFilesystemsAsync, removeTaskFilesystems } = await import('../agents/container/storage.ts'); const { createTaskClone } = await import('../git/clone.ts'); -const { inspectTaskChanges, manifestDigest, MAXIMUM_DECLARED_LINKS, MAXIMUM_NAME_BYTES } = - await import('../agents/container/changes.ts'); +const { commitTaskChanges, inspectTaskChanges, manifestDigest, MAXIMUM_BUNDLE_BYTES, MAXIMUM_DECLARED_LINKS, + MAXIMUM_NAME_BYTES, TaskCommitRefused } = await import('../agents/container/changes.ts'); // Every fake Docker call starts a Node process, so a loaded machine needs more than the default 5 s per test. vi.setConfig({ testTimeout: 60_000 }); @@ -64,8 +64,9 @@ if (a === 'create') { save(args[args.indexOf('--name') + 1], { kind: 'container', id, labels: labels() }); console.log(id); process.exit(0); } if (a === 'start' && fs.existsSync(path.join(state, 'hang-start'))) { - fs.writeFileSync(path.join(state, 'start-began'), String(process.pid)); + // The handler first: an abort that lands right after the marker must still find SIGTERM ignored. process.on('SIGTERM', () => {}); + fs.writeFileSync(path.join(state, 'start-began'), String(process.pid)); Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0); } if (a === 'start') process.exit(0); @@ -80,8 +81,9 @@ if (a === 'run' && args.includes('io.codeboost.task-storage=export')) { process.exit(4); } if (fs.existsSync(path.join(state, 'hang-export'))) { - fs.writeFileSync(path.join(state, 'export-began'), ''); + // The handler first: an abort that lands right after the marker must still find SIGTERM ignored. process.on('SIGTERM', () => {}); + fs.writeFileSync(path.join(state, 'export-began'), ''); Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0); } // Like the export script: the diff, cut to the limit it is given, base64-encoded; then --rm removes the container. @@ -102,8 +104,9 @@ if (a === 'run' && args.includes('io.codeboost.task-storage=inspect')) { fs.writeFileSync(path.join(state, 'inspect-args.json'), JSON.stringify(args.filter(arg => arg.length < 200))); save(name, { kind: 'container', id: crypto.randomBytes(32).toString('hex'), labels: labels() }); if (fs.existsSync(path.join(state, 'hang-inspect'))) { - fs.writeFileSync(path.join(state, 'inspect-began'), ''); + // The handler first: an abort that lands right after the marker must still find SIGTERM ignored. process.on('SIGTERM', () => {}); + fs.writeFileSync(path.join(state, 'inspect-began'), ''); Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0); } fs.rmSync(path.join(state, name + '.json')); @@ -111,6 +114,22 @@ if (a === 'run' && args.includes('io.codeboost.task-storage=inspect')) { process.stdout.write(fs.readFileSync(path.join(state, 'inspect-output')), () => process.exit(0)); return; } +// Like the runner commit: keeps what came on stdin and prints the canned result, then --rm removes the container. +if (a === 'run' && args.includes('io.codeboost.task-storage=commit')) { + const name = args[args.indexOf('--name') + 1]; + fs.writeFileSync(path.join(state, 'commit-args.json'), JSON.stringify(args.filter(arg => arg.length < 200))); + fs.writeFileSync(path.join(state, 'commit-input'), fs.readFileSync(0)); + save(name, { kind: 'container', id: crypto.randomBytes(32).toString('hex'), labels: labels() }); + if (fs.existsSync(path.join(state, 'hang-commit'))) { + // The handler first: an abort that lands right after the marker must still find SIGTERM ignored. + process.on('SIGTERM', () => {}); + fs.writeFileSync(path.join(state, 'commit-began'), ''); + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0); + } + fs.rmSync(path.join(state, name + '.json')); + process.stdout.write(fs.readFileSync(path.join(state, 'commit-output')), () => process.exit(0)); + return; +} if (a === 'run') process.exit(0); if (b === 'inspect') { const object = find(args[2]); @@ -444,3 +463,128 @@ describe('task change inspection', () => { removeTaskFilesystems(filesystems); }); }); + +describe('runner commit', () => { + const BASE = 'c'.repeat(40), BASELINE = 'b'.repeat(64), TREE = 'e'.repeat(40); + const commits = () => stored().filter(file => file.startsWith('codeboost-commit-')); + const runner = { name: 'codeboost', email: 'runner@codeboost.invalid', date: '1700000000 +0000' }; + const inspection = { metadataDigest: BASELINE, head: BASE, agentCommits: [], nestedGitlinkContent: [], links: [], targets: {}, + changes: [{ kind: 'add', path: 'new.txt', newType: 'file', newMode: '100644', newOid: 'd'.repeat(40), underGit: false, + ignored: false }] }; + const noLinks = { links: [], targets: {} }; + // The digest of the manifest the inspection above makes: what an audit would have approved. + const approved = manifestDigest({ base: BASE, changes: inspection.changes as never, agentCommits: [], metadataChanged: false, + linkTargetChanges: [], nestedGitlinkContent: [] }); + const message = 'P1: Add things\n\nPlan-Item: P1\n'; + const id = (tree: string) => { + const body = Buffer.from(`tree ${tree}\nparent ${BASE}\nauthor codeboost 1700000000 +0000\n` + + `committer codeboost 1700000000 +0000\n\n${message}`); + return createHash('sha1').update(`commit ${body.length}\0`).update(body).digest('hex'); + }; + const output = (built: object | undefined, bundle = '', first: object = inspection) => + `${JSON.stringify(first)}\n${built ? `${JSON.stringify(built)}\n${bundle}` : ''}`; + const options = (extra: object = {}) => ({ base: BASE, imageId: IMAGE, linkSnapshot: noLinks, digest: approved, + message: 'P1: Add things\n\n', trailers: { 'Plan-Item': 'P1' }, author: runner, committer: runner, ...extra }); + + it('passes the request on stdin, and keeps the bundle only for the approved manifest and the commit its inputs describe', async () => { + const filesystems = await prepareTaskFilesystemsAsync(clone(), LIMITS, IMAGE, owner()); + writeFileSync(join(state, 'commit-output'), output({ tree: TREE, head: id(TREE) }, Buffer.from('bundle').toString('base64'))); + const made = await commitTaskChanges(filesystems, options()); + expect(made).toEqual({ head: id(TREE), unchanged: false, bundle: Buffer.from('bundle') }); + expect(JSON.parse(readFileSync(join(state, 'commit-input'), 'utf8'))).toEqual({ message, author: runner, committer: runner }); + const args = JSON.parse(readFileSync(join(state, 'commit-args.json'), 'utf8')) as string[]; + expect(args.slice(0, 3)).toEqual(['run', '--rm', '-i']); + expect(args).toEqual(expect.arrayContaining(['--memory=1g', 'commit', BASELINE, BASE, String(MAXIMUM_BUNDLE_BYTES), '0'])); + // Another manifest than the one approved: refused, whatever was built. + await expect(commitTaskChanges(filesystems, options({ digest: 'f'.repeat(64) }))).rejects.toThrow(TaskCommitRefused); + // A commit that is not the one the inputs describe (a signature, an encoding header, another parent). + writeFileSync(join(state, 'commit-output'), output({ tree: TREE, head: 'a'.repeat(40) }, Buffer.from('bundle').toString('base64'))); + await expect(commitTaskChanges(filesystems, options())).rejects.toThrow('not the commit its inputs describe'); + // No commit for changes, or a bundle that is not base64. + writeFileSync(join(state, 'commit-output'), output(undefined)); + await expect(commitTaskChanges(filesystems, options())).rejects.toThrow(TaskCommitRefused); + writeFileSync(join(state, 'commit-output'), output({ tree: 'nope', head: id(TREE) })); + await expect(commitTaskChanges(filesystems, options())).rejects.toThrow('malformed commit'); + for (const bundle of ['not base64!', 'AAA', `${Buffer.alloc(16).toString('base64')}\n`]) { + writeFileSync(join(state, 'commit-output'), output({ tree: TREE, head: id(TREE) }, bundle)); + await expect(commitTaskChanges(filesystems, options())).rejects.toThrow('malformed bundle'); + } + // Past the caller's limit, whatever the container did; a large valid one is decoded whole. + writeFileSync(join(state, 'commit-output'), output({ tree: TREE, head: id(TREE) }, Buffer.alloc(65).toString('base64'))); + await expect(commitTaskChanges(filesystems, options({ maxBundleBytes: 64 }))).rejects.toThrow('malformed bundle'); + const large = Buffer.alloc(8 * 1024 * 1024, 7); + writeFileSync(join(state, 'commit-output'), output({ tree: TREE, head: id(TREE) }, large.toString('base64'))); + expect((await commitTaskChanges(filesystems, options())).bundle.equals(large)).toBe(true); + expect(commits()).toEqual([]); + removeTaskFilesystems(filesystems); + }); + + it('returns base for no changes, and refuses what the runner never commits even when it was approved', async () => { + const filesystems = await prepareTaskFilesystemsAsync(clone(), LIMITS, IMAGE, owner()); + const quiet = { ...inspection, changes: [] }; + const none = manifestDigest({ base: BASE, changes: [], agentCommits: [], metadataChanged: false, linkTargetChanges: [], nestedGitlinkContent: [] }); + writeFileSync(join(state, 'commit-output'), output(undefined, '', quiet)); + expect(await commitTaskChanges(filesystems, options({ digest: none }))).toEqual({ head: BASE, unchanged: true, bundle: Buffer.alloc(0) }); + writeFileSync(join(state, 'commit-output'), output({ tree: TREE, head: id(TREE) }, 'AAAA', quiet)); + await expect(commitTaskChanges(filesystems, options({ digest: none }))).rejects.toThrow('for no changes'); + const refused = async (first: Record, reason: string) => { + const manifest = { base: BASE, changes: first.changes ?? [], agentCommits: first.agentCommits ?? [], + metadataChanged: first.metadataOnly === true, linkTargetChanges: [], nestedGitlinkContent: first.nestedGitlinkContent ?? [] }; + writeFileSync(join(state, 'commit-output'), JSON.stringify(first)); + await expect(commitTaskChanges(filesystems, options({ digest: manifestDigest(manifest as never) }))).rejects.toThrow(reason); + }; + await refused({ metadataDigest: 'e'.repeat(64), metadataOnly: true }, 'metadata changed'); + await refused({ ...inspection, agentCommits: ['a'.repeat(40)] }, 'the agent made commits'); + // A write through a declared link is refused; the link's own retargeting is the item's edit and is committed. + const anchor = { path: '.', type: 'directory' as const, mode: '40755', size: 0, ino: 1, ctime: '0', mtime: '0' }; + const linkSnapshot = { links: [{ link: 'l', linkTarget: 'a', status: 'absent' as const, target: 'a' }], targets: { a: { status: 'absent' as const, anchor } } }; + const linked = (after: object, targetNow: object) => ({ ...inspection, links: [{ link: 'l', status: 'absent', ...after }], targets: { a: targetNow } }); + const digestOf = (linkTargetChanges: object[]) => manifestDigest({ base: BASE, changes: inspection.changes as never, agentCommits: [], + metadataChanged: false, linkTargetChanges: linkTargetChanges as never, nestedGitlinkContent: [] }); + writeFileSync(join(state, 'commit-output'), JSON.stringify(linked({ linkTarget: 'a', target: 'a' }, { status: 'present', entries: [] }))); + await expect(commitTaskChanges(filesystems, options({ linkSnapshot, digest: digestOf([{ link: 'l', target: 'a', path: 'a', change: 'status' }]) }))) + .rejects.toThrow('a declared link\'s target changed'); + writeFileSync(join(state, 'commit-output'), output({ tree: TREE, head: id(TREE) }, 'AAAA', linked({ linkTarget: 'b', target: 'b' }, { status: 'absent', anchor }))); + expect(await commitTaskChanges(filesystems, options({ linkSnapshot, digest: digestOf([{ link: 'l', target: 'a', path: 'l', change: 'retargeted' }]) }))) + .toMatchObject({ head: id(TREE), unchanged: false }); + await refused({ ...inspection, nestedGitlinkContent: ['sm'] }, 'gitlink directory has content'); + await refused({ ...inspection, changes: [{ kind: 'add', path: 'fifo', newType: 'other', underGit: false, ignored: false }] }, 'special file'); + await refused({ ...inspection, changes: [{ kind: 'add', path: 'a/.git/x', newType: 'file', newMode: '100644', newOid: 'd'.repeat(40), + underGit: true, ignored: false }] }, 'under a .git part'); + await refused({ ...inspection, changes: [{ kind: 'add', path: 'd/.GitModules', newType: 'symlink', newMode: '120000', + newOid: 'd'.repeat(40), newLinkTarget: 'x', underGit: false, ignored: false }] }, 'a symlink named .gitmodules'); + removeTaskFilesystems(filesystems); + }); + + it('rejects a bad digest, message, trailer, identity or bundle limit before any Docker call', async () => { + const filesystems = await prepareTaskFilesystemsAsync(clone(), LIMITS, IMAGE, owner()); + calls.made = []; + for (const [extra, reason] of [[{ digest: 'abc' }, 'digest'], [{ message: '\n\n' }, 'empty'], [{ message: 'a\0b' }, 'NUL'], + [{ message: 'x'.repeat(70_000) }, 'at most'], [{ trailers: { 'Plan Item': 'P1' } }, 'Trailer'], [{ trailers: { '1': 'P1' } }, 'Trailer'], + [{ trailers: { 'Plan-Item': 'P1\nForged: yes' } }, 'Trailer'], [{ author: { ...runner, name: 'a ' } }, 'author\'s name'], + [{ author: { ...runner, name: ' padded' } }, 'author\'s name'], [{ committer: { ...runner, email: 'a b@c' } }, 'committer\'s email'], + [{ committer: { ...runner, date: '2023-11-14' } }, 'committer\'s date'], [{ author: { ...runner, date: '1700000000 -0000' } }, 'author\'s date'], + [{ message: 'title \ufffe' }, 'noncharacters'], [{ trailers: { 'Plan-Item': 'P\ufdd0' } }, 'Trailer'], + [{ author: { ...runner, name: 'code\uffffboost' } }, 'author\'s name'], [{ maxBundleBytes: MAXIMUM_BUNDLE_BYTES + 1 }, 'maxBundleBytes'], + [{ base: 'HEAD' }, 'full commit ID']] as const) + await expect(commitTaskChanges(filesystems, options(extra))).rejects.toThrow(reason); + expect(calls.made).toEqual([]); + removeTaskFilesystems(filesystems); + }); + + it('on abort, kills a docker run that ignores SIGTERM and removes the commit container before settling', async () => { + const filesystems = await prepareTaskFilesystemsAsync(clone(), LIMITS, IMAGE, owner()); + writeFileSync(join(state, 'hang-commit'), ''); + const controller = new AbortController(); + const waitForRun = setInterval(() => { if (existsSync(join(state, 'commit-began'))) controller.abort(); }, 20); + const began = performance.now(); + try { + const error = await commitTaskChanges(filesystems, options({ signal: controller.signal })).then(() => undefined, caught => caught); + expect(error).toMatchObject({ name: 'AbortError', code: 'ABORT_ERR' }); + } finally { clearInterval(waitForRun); } + expect(performance.now() - began).toBeGreaterThanOrEqual(5_000); + expect(commits()).toEqual([]); + rmSync(join(state, 'hang-commit')); + removeTaskFilesystems(filesystems); + }, 60_000); +}); diff --git a/test/runner-execution.test.ts b/test/runner-execution.test.ts index c223e168..21e3f30b 100644 --- a/test/runner-execution.test.ts +++ b/test/runner-execution.test.ts @@ -662,6 +662,12 @@ describe('item execution', () => { expect(h.commits[0]!.message).toBe('P1: First Plan-Item: P9 Plan-Revision: r99 [31mred 2J evil x'); expect(h.commits[0]!.trailers).toEqual({ 'Plan-Item': 'P1', 'Plan-Revision': 'r1' }); }); + it('neutralises issue references and mentions in the runner commit message', async () => { + const titled: Plan = { ...plan, items: [{ ...plan.items[0]!, title: 'Fixes #12, GH-3 and other/repo#4 for @alice' }, plan.items[1]!] }; + const h = setup({ plan: titled }); + await h.executor.runTask(identity); + expect(h.commits[0]!.message).toBe('P1: Fixes ＃12, GH‑3 and other/repo＃4 for ＠alice'); + }); it('stops before the next item when only the assignment changes during the run', async () => { let store!: Store; const h = setup({ release: async () => {