diff --git a/.github/workflows/agent-isolation.yml b/.github/workflows/agent-isolation.yml index 3354ea6a..018b55ce 100644 --- a/.github/workflows/agent-isolation.yml +++ b/.github/workflows/agent-isolation.yml @@ -8,6 +8,14 @@ on: - 'test/agent-*.test.ts' - 'runner/question-*.ts' - 'runner/questions.ts' + - 'runner/workspace.ts' + - 'runner/runner-repository.ts' + - 'runner/execution.ts' + - 'core/run-audit.ts' + - 'runner/review.ts' + - 'runner/store.ts' + - 'git/history.ts' + - 'test/runner-workspace.test.ts' - '.github/workflows/agent-isolation.yml' pull_request: paths: @@ -16,6 +24,14 @@ on: - 'test/agent-*.test.ts' - 'runner/question-*.ts' - 'runner/questions.ts' + - 'runner/workspace.ts' + - 'runner/runner-repository.ts' + - 'runner/execution.ts' + - 'core/run-audit.ts' + - 'runner/review.ts' + - 'runner/store.ts' + - 'git/history.ts' + - 'test/runner-workspace.test.ts' - '.github/workflows/agent-isolation.yml' permissions: contents: read @@ -33,4 +49,4 @@ jobs: - run: npm ci --ignore-scripts - run: npm run typecheck # The Docker suites share one image tag and daemon, so run test files one at a time. - - run: npx vitest run --no-file-parallelism test/agent-contract.test.ts test/agent-clone.test.ts test/agent-container.test.ts test/agent-network.test.ts test/agent-policy.test.ts test/agent-proxy.test.ts test/agent-adapter.test.ts test/agent-supervisor.test.ts test/agent-output.test.ts test/agent-gate.test.ts test/agent-question.test.ts + - run: npx vitest run --no-file-parallelism test/agent-contract.test.ts test/agent-clone.test.ts test/agent-container.test.ts test/agent-network.test.ts test/agent-policy.test.ts test/agent-proxy.test.ts test/agent-adapter.test.ts test/agent-supervisor.test.ts test/agent-output.test.ts test/agent-gate.test.ts test/agent-question.test.ts test/runner-workspace.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 31efcb43..d991ab1f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,6 @@ jobs: - run: npm run typecheck # The Docker agent suites run one file at a time in the Agent isolation workflow; running them here # would put them in parallel against the same image tag and daemon. - - run: npm test -- --exclude test/agent-container.test.ts --exclude test/agent-network.test.ts --exclude test/agent-adapter.test.ts --exclude test/agent-supervisor.test.ts --exclude test/agent-gate.test.ts --exclude test/agent-question.test.ts + - run: npm test -- --exclude test/agent-container.test.ts --exclude test/agent-network.test.ts --exclude test/agent-adapter.test.ts --exclude test/agent-supervisor.test.ts --exclude test/agent-gate.test.ts --exclude test/agent-question.test.ts --exclude test/runner-workspace.test.ts - run: npx playwright install --with-deps chromium - run: npm run test:browser diff --git a/agents/container/changes.ts b/agents/container/changes.ts index 24c1cc16..43c9e15f 100644 --- a/agents/container/changes.ts +++ b/agents/container/changes.ts @@ -39,6 +39,12 @@ export interface TaskChange { readonly oldOid?: string; readonly newOid?: string; readonly newLinkTarget?: string; + /** + * For a change whose new entry is a symlink: whether its target, resolved one part at a time as the kernel would in + * an agent container, passes through another link on the way or is a link itself. A target outside the work tree or + * in the metadata counts as `true`, since D cannot check it. + */ + readonly linkTargetTraversesLink?: boolean; /** Whether a part of the path is named `.git` (in any case): Git will never commit it. */ readonly underGit: boolean; /** diff --git a/agents/container/storage.ts b/agents/container/storage.ts index 0174ecd3..f4e7aaea 100644 --- a/agents/container/storage.ts +++ b/agents/container/storage.ts @@ -494,6 +494,27 @@ export function removeTaskFilesystems(filesystems: TaskFilesystems | RecoveredTa liveAllocations.delete(owner.allocationId); } +/** + * `removeTaskFilesystems` without blocking the event loop: the same removal, each Docker call in its own process group. + * It is never cancelled: it runs to its own deadline, so nothing is dropped, and settles only after every call exited. + */ +export async function removeTaskFilesystemsAsync(filesystems: TaskFilesystems | RecoveredTaskStorage): Promise { + const recovered = recoveredStorage.get(filesystems as RecoveredTaskStorage); + if (recovered) { + const handle = filesystems as RecoveredTaskStorage; + await runStepsAsync(cleanup(recovered.keeperId ? [recovered.keeperId] : [], + [handle.metadataVolume, handle.workVolume].filter((name): name is string => name !== undefined), recovered.owner), {}); + recoveredStorage.delete(handle); + return; + } + const allocated = filesystems as TaskFilesystems; + assertTaskFilesystems(allocated); + const owner = taskFilesystemOwner(allocated); + await runStepsAsync(cleanup([allocated.keeper], [allocated.metadataVolume, allocated.workVolume], owner), {}); + allocations.delete(allocated); + liveAllocations.delete(owner.allocationId); +} + /** At most this much diff is returned; the caller saves it as a stopped attempt's partial output. */ export const MAXIMUM_EXPORT_BYTES = 1024 * 1024; export interface TaskDiff { diff --git a/agents/container/tree-script.ts b/agents/container/tree-script.ts index 3528859a..690ff004 100644 --- a/agents/container/tree-script.ts +++ b/agents/container/tree-script.ts @@ -304,8 +304,12 @@ sub anchored { } return { status => $status, anchor => target_entry($anchor eq "" ? "." : $anchor) }; } +# With $unchecked, names are kept as bytes and never checked: only the status is wanted. sub resolve { - my ($link, $walk) = @_; my %record = (link => text($link, "declared path")); + my ($link, $walk, $unchecked) = @_; my $name = $unchecked ? sub { $_[0] } : \&text; + # Unchecked, a link on the way is reported by status alone: its anchor entry would check the anchor's names. + my $through = sub { $unchecked ? { status => "through-link" } : anchored("through-link", $_[0]) }; + my %record = (link => $name->($link, "declared path")); my @parents = split m{/}, $link; pop @parents; my $prefix = ""; # A declared path that is not there, or not a link, is not-a-link whether or not its directory exists: adding or # deleting a declared file must not look like a change to a link. A directory on the way that is a link still makes @@ -313,12 +317,12 @@ sub resolve { for my $part (@parents) { $prefix = join_path($prefix, $part); my @stat = lstat $prefix; return { %record, status => "not-a-link" } unless @stat; - return { %record, %{ anchored("through-link", $prefix) } } if -l _; + return { %record, %{ $through->($prefix) } } if -l _; return { %record, status => "not-a-link" } unless -d _; } my @stat = lstat $link; return { %record, status => "not-a-link" } unless @stat && -l _; - my $raw = readlink $link; $record{linkTarget} = text($raw, "the link target of " . shown($link)); + my $raw = readlink $link; $record{linkTarget} = $name->($raw, "the link target of " . shown($link)); # From the filesystem root: ("work", ...) is inside the work tree. my @at = $raw =~ m{^/} ? () : ("work", @parents); my @parts = grep { $_ ne "" && $_ ne "." } split m{/}, $raw; @@ -333,7 +337,7 @@ sub resolve { return { %record, status => "metadata" } if @at > 1 && $at[1] eq ".git"; next if @at == 1; my $path = join "/", @at[1 .. $#at]; my @here = lstat $path; - return { %record, %{ anchored("through-link", $path) } } if @here && -l _; + return { %record, %{ $through->($path) } } if @here && -l _; } return { %record, status => "outside" } if @at < 2; my $target = join "/", @at[1 .. $#at]; @@ -341,7 +345,7 @@ sub resolve { # a change to it. The target's own state (its entries, or its anchor) is reported once per target, not per link. my @final = lstat $target; $resolved_targets{$target} = 1; my $status = $walk ? target_state($target)->{status} : @final ? "present" : "absent"; - return { %record, target => text($target, "path"), status => $status }; + return { %record, target => $name->($target, "path"), status => $status }; } # Each distinct target is walked once per run, so two declared links to one target count its entries once. my %target_states; @@ -623,6 +627,15 @@ for my $change (@changes) { $change->{path} = text($change->{path}, "path"); $change->{oldPath} = text($change->{oldPath}, "path") if defined $change->{oldPath}; } +# Where each changed link's new target leads, resolved one part at a time as the kernel would in an agent container: +# whether a part on the way, or the target itself, is another link. One that leaves the work tree or enters the +# metadata counts as traversing, since what is there cannot be checked from here. +for my $i (0 .. $#changes) { + next unless ($changes[$i]{newType} // "") eq "symlink"; + # The joined target can be longer than either name the manifest carries, so nothing here is checked as a name. + my $status = resolve($raw[$i]{path}, 0, 1)->{status}; + $changes[$i]{linkTargetTraversesLink} = $status eq "present" || $status eq "absent" ? JSON::PP::false : JSON::PP::true; +} my $head = git("rev-parse", "--verify", "HEAD"); chomp $head; my @agent_commits; diff --git a/core/run-audit.ts b/core/run-audit.ts index cff4869b..dbe20daa 100644 --- a/core/run-audit.ts +++ b/core/run-audit.ts @@ -17,13 +17,24 @@ export interface ManifestChange { /** D reports whether resolving the new target would traverse another symlink. */ linkTargetTraversesLink?: boolean; underGit: boolean; + /** Whether base's own ignore rules ignore this new path. Such a file is never committed (#87 decision 2). */ + ignored: boolean; } +/** A difference in a declared link's target since the pre-run snapshot (D's `LinkTargetChange`). */ +export interface LinkTargetChange { + link: string; + target: string; + path: string; + /** `retargeted`: the link now resolves elsewhere, which is an edit of the link itself; every other kind is a write through it. */ + change: 'content' | 'mode' | 'type' | 'created' | 'deleted' | 'identity' | 'status' | 'retargeted'; +} +/** D's `TaskChangeManifest`, as the audit reads it. */ export interface ChangeManifest { changes: readonly ManifestChange[]; agentCommits: readonly string[]; metadataChanged: boolean; /** Differences from the pre-run snapshot of declared symlink targets. */ - linkTargetChanges: readonly string[]; + linkTargetChanges: readonly LinkTargetChange[]; nestedGitlinkContent: readonly string[]; } export type AuditOutcome = @@ -51,6 +62,7 @@ export function isDotGit(part: string): boolean { const q = (text: string) => JSON.stringify(text.length > 300 ? `${text.slice(0, 300)}…` : text); const list = (items: readonly string[]) => items.slice(0, 5).map(q).join(', ') + (items.length > 5 ? ` and ${items.length - 5} more` : ''); const TYPES = new Set(['file', 'symlink', 'gitlink', 'directory', 'other']); +const LINK_CHANGES = new Set(['content', 'mode', 'type', 'created', 'deleted', 'identity', 'status', 'retargeted']); /** * Every field the audit reads, checked before it reads any (AGENTS.md: partial records fail closed). A missing boolean @@ -59,8 +71,12 @@ const TYPES = new Set(['file', 'symlink', 'gitlink', 'directory', 'other']); function malformed(manifest: ChangeManifest): string | null { if (!manifest || typeof manifest !== 'object') return 'The change report is missing.'; if (!Array.isArray(manifest.changes)) return 'The change report has no change list.'; - for (const field of ['agentCommits', 'linkTargetChanges', 'nestedGitlinkContent'] as const) + for (const field of ['agentCommits', 'nestedGitlinkContent'] as const) if (!Array.isArray(manifest[field]) || manifest[field].some(entry => typeof entry !== 'string')) return `The change report has no ${field} list.`; + if (!Array.isArray(manifest.linkTargetChanges)) return 'The change report has no linkTargetChanges list.'; + for (const entry of manifest.linkTargetChanges) + if (!entry || typeof entry !== 'object' || [entry.link, entry.target, entry.path].some(value => typeof value !== 'string') + || !LINK_CHANGES.has(entry.change)) return 'The change report has a malformed link target change.'; if (typeof manifest.metadataChanged !== 'boolean') return 'The change report does not say whether Git metadata changed.'; let bytes = 0; for (const change of manifest.changes) { @@ -69,6 +85,7 @@ function malformed(manifest: ChangeManifest): string | null { if (change.kind === 'rename' ? typeof change.oldPath !== 'string' || !change.oldPath : change.oldPath !== undefined) return `The change at ${q(change.path)} has an invalid old path.`; if (!KINDS.has(change.kind)) return `The change at ${q(change.path)} has an unknown kind.`; if (typeof change.underGit !== 'boolean') return `The change at ${q(change.path)} does not say whether it is under .git.`; + if (typeof change.ignored !== 'boolean') return `The change at ${q(change.path)} does not say whether base ignores it.`; // add has only a new entry, delete only an old one, every other kind both. const needsOld = change.kind !== 'add', needsNew = change.kind !== 'delete'; if ((needsOld && !TYPES.has(change.oldType as string)) || (!needsOld && change.oldType !== undefined)) return `The change at ${q(change.path)} has an invalid old entry type.`; @@ -113,7 +130,11 @@ export function auditRun(item: PlanItem, manifest: ChangeManifest, pathKey: (pat if (manifest.metadataChanged) violations.push('The agent changed Git metadata under .git.'); // Agents never commit: the metadata volume is read-only to them, so any agent commit is a violation, never undone (#66). if (manifest.agentCommits.length) violations.push(`The agent made its own commits: ${list(manifest.agentCommits)}.`); - if (manifest.linkTargetChanges.length) violations.push(`A declared symlink target changed: ${list(manifest.linkTargetChanges)}.`); + // A write through a declared link changes what it points at. A retargeted link is an edit of the link itself, judged + // below by the link rules, so the manifest must list that edit; without one, the way to its target changed. + const changedPaths = new Set(manifest.changes.flatMap(change => [change.path, ...(change.oldPath ? [change.oldPath] : [])]).map(pathKey)); + const targetChanges = manifest.linkTargetChanges.filter(entry => entry.change !== 'retargeted' || !changedPaths.has(pathKey(entry.link))); + if (targetChanges.length) violations.push(`A declared symlink target changed: ${list(targetChanges.map(entry => `${entry.link} -> ${entry.path} (${entry.change})`))}.`); if (manifest.nestedGitlinkContent.length) violations.push(`Content appeared under a gitlink: ${list(manifest.nestedGitlinkContent)}.`); const declared = new Set(item.files.flatMap(file => [file.path, ...(file.renamed_from ? [file.renamed_from] : [])]).map(pathKey)); // Each path appears once, under the trusted path identity: two entries for one path contradict each other. @@ -142,6 +163,8 @@ export function auditRun(item: PlanItem, manifest: ChangeManifest, pathKey: (pat if (paths.some(path => path.startsWith('/') || posix.normalize(path).startsWith('../') || ['.', '..'].includes(posix.normalize(path)) || path.includes('\0'))) { violations.push(`Invalid path in the change report: ${q(change.path)}.`); continue; } if (change.oldType === 'gitlink' || change.newType === 'gitlink') { violations.push(`Plan items cannot change gitlinks: ${q(change.path)}.`); continue; } + // A file base's own rules ignore (a build output, a .env) is never committed (#87 decision 2). + if (change.ignored) { violations.push(`The agent added ${q(change.path)}, which the repository ignores.`); continue; } // Only a declared pre-existing link may change at all: deleting it, turning it into a file, or renaming it from an // undeclared path is a link change too. if (change.oldType === 'symlink' && !declared.has(pathKey(change.oldPath ?? change.path))) diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 00000000..7d0ac55f --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,427 @@ +# codeboost architecture: a high-level overview + +Status: current as of `main` at `aafd153` (2026-10-01). Open work is listed in **Build status and roadmap**. +Writing standard: plain language, ISO 24495-1:2023 + +## About this document + +**Who it is for.** New contributors, reviewers, and anyone who needs to know how codeboost fits together before they read the code. + +**What it is for.** It explains the parts of codeboost, what each part owns, how data moves between them, and where the trust boundaries are. It also says clearly which parts are built and which are only designed. + +**What it is not.** It is not the product design and it does not repeat the detailed rules. When this document and a detailed document disagree, the detailed document wins: + +| Topic | Detailed document | +|---|---| +| Product design, decisions and roadmap | [designs/codeboost-plan-indexed-review.md](designs/codeboost-plan-indexed-review.md) | +| Plan file format | [plan-format.md](plan-format.md) | +| SQLite store | [implementation/persistent-review-store.md](implementation/persistent-review-store.md) | +| Review screen | [implementation/read-only-review.md](implementation/read-only-review.md) | +| Agent container and network | [implementation/agent-isolation.md](implementation/agent-isolation.md) | +| Runner lifecycle (attempts, retries, shutdown) | [implementation/runner-lifecycle.md](implementation/runner-lifecycle.md) | +| Already-fixed check and opening the pull request | [implementation/pull-request-opening.md](implementation/pull-request-opening.md) | +| Merge gate and merge queue | [implementation/guarded-merge.md](implementation/guarded-merge.md), [implementation/merge-queue.md](implementation/merge-queue.md) | +| Issue ranking | [implementation/issue-prioritization.md](implementation/issue-prioritization.md) | +| Planning with an agent | [implementation/planning-provider.md](implementation/planning-provider.md), [implementation/planning-suggestions.md](implementation/planning-suggestions.md) | +| Visual design | [../DESIGN.md](../DESIGN.md) | + +**How to read it.** Read the Summary first. It fits on one screen. Special words are in **Terms used**. Each later section stands on its own. + +## Summary + +- **What codeboost does.** It turns a GitHub issue into a plan, has an AI agent (Claude or Codex) carry out the plan, and lets you review the resulting pull request one plan item at a time. Then it merges the pull request, but only when every guard passes. +- **Shape of the system.** One local Node.js process. It serves a browser screen on `127.0.0.1`, keeps all state in one SQLite file, reads Git directly, talks to GitHub only through the `gh` command, and runs every agent inside a locked-down Docker container. +- **Five layers.** + 1. `web/` — the local HTTP server and browser screen. + 2. `runner/` — the trusted coordinator. It is the only code that writes the database and the only code that starts agents or merges. + 3. `core/` — pure logic with no input or output: plan validation, linking code to plan items, approvals, issue ranking, planning prompts. + 4. `git/` and `github/` — hardened adapters for Git and for the `gh` command. + 5. `agents/` — the isolation boundary: containers, the vendor-only network, and the Claude and Codex adapters. +- **The central idea.** A runner-owned **commit ledger** records which plan item each commit belongs to. The linking engine uses the ledger, not commit messages, to put every changed line on a plan item's row. Anything the ledger cannot explain goes to the Unplanned or Ambiguous row. +- **The central safety rule.** Everything that comes from outside is untrusted: issue text, agent output, the browser, and the repository's own files. Only the runner decides. Agents never see your credentials, your other files, or GitHub. +- **Build state.** Three levels: + - **In use:** plan and linking library, SQLite store, review screen, Ask (questions to an agent), guarded merge with merge-queue support, ranked Issues screen, agent isolation boundary, and the single-runner lock. + - **Built and tested, not yet connected:** the runner coordinator and `/api/runner`, the planning API, startup recovery, per-item execution with the post-run audit, the already-fixed check, and opening the pull request. They wait for the runner's commit in lane D (#66 part 2, PR #90) and the workspace follow-ups (#87). + - **Not built:** rebasing, running `cmd:` checks, review rounds, continuing after a scope pause (#88), and the Planning, Queue and Learning screens. + +## Terms used + +One word means one thing in this document. + +| Term | Meaning | +|---|---| +| Plan | The list of plan items for one GitHub issue. Each plan has revisions: r1, r2, and so on. | +| Plan item | One change in a plan, such as P1. It declares its files and its acceptance checks. | +| Declared files | The files a plan item says it will change. | +| Task | One issue's journey from plan to merge or cancel. Identified by repository ID, task ID and plan ID together. | +| Attempt | One agent run or one runner command for a task. A retry is a new attempt with a new ID. | +| Phase | The permission profile an attempt runs under: `planning`, `questions`, `review`, `execute` or `fix`. | +| Snapshot | A recorded base commit and head commit pair. Review decisions are bound to a snapshot. | +| Commit ledger | The runner's record of each commit it made: full SHA, owning plan item (or none), and origin (owned or foreign). | +| Segment | A run of changed lines, or one non-text file change, that has a single owner. The review screen shows segments. | +| Unplanned | A segment that no ledger entry explains. | +| Ambiguous | A segment that more than one plan item changed. | +| Approval | Your sign-off on one plan item, stored as a fingerprint of exactly what you saw. | +| Stale | An approval whose fingerprint no longer matches the current code, plan item or dependencies. | +| Runner | The trusted part of codeboost (`runner/`) that owns the database, starts attempts, and merges. | +| Agent container | The Docker container that runs one attempt with only the task clone and the agent's own sign-in. | +| Lane | A development workstream in the roadmap, such as lane D (agent isolation) or lane F (runner). | + +## System context + +codeboost runs on your machine for one person. It has no server on the internet. + +```mermaid +flowchart LR + you([You, in a browser]) -- "HTTP on 127.0.0.1 + private token" --> cb + subgraph host [Your machine] + cb["codeboost process
(Node 26.7+)"] + db[(SQLite file)] + repo[(Your Git repository
read only)] + docker[[Docker daemon]] + gh[gh CLI
signed in as you] + cb --> db + cb -- "hardened git reads" --> repo + cb -- "start / stop containers" --> docker + cb -- "issues, checks, merge" --> gh + end + gh --> github[(GitHub)] + docker --> agent["Agent container
(claude or codex)"] + agent -- "vendor API hosts only,
through a proxy" --> vendor[(Anthropic / OpenAI API)] +``` + +| External system | How codeboost uses it | Who holds the credential | +|---|---|---| +| Browser | Shows the screens and sends review commands. | A random token made at start-up. | +| Git repository | Read-only history and file reads. codeboost never writes your checkout. | None. | +| GitHub | Issues, pull request state, branch rules, required checks, merge. Always through `gh`. | Your `gh` sign-in. Only the runner uses it. | +| Docker | Runs agent containers, the internal network and the egress proxy. | Local Docker socket. Never mounted into a container. | +| Claude or Codex API | The agent inside the container calls its own vendor. | `CLAUDE_CODE_OAUTH_TOKEN` or the Codex `auth.json`, passed only into that container. | + +## Building blocks + +### Layer map + +```mermaid +flowchart TB + subgraph web ["web/ — local server and screen"] + cli[cli.ts] + server[server.ts] + ui[public/app.js] + issuesView[issues.ts] + end + subgraph runner ["runner/ — trusted coordinator"] + review[review.ts
ReviewService] + questions[questions.ts + question-*.ts
Ask] + merge[merge.ts
merge coordinator] + coord[coordinator.ts + lifecycle.ts
attempts, slots, shutdown] + exec[execution.ts
per-item execution] + publish[publish.ts
open the pull request] + recov[recovery.ts
lock, startup recovery] + store[store.ts
only SQL writer] + end + subgraph core ["core/ — pure logic"] + plan[plan.ts, parse-v1.ts] + linking[linking.ts] + approvals[approvals.ts] + ranking[issue-ranking.ts] + planning[planning-author.ts,
planning-suggestions.ts] + execcore[execution-prompt.ts,
run-audit.ts] + prbody[pull-request-body.ts] + end + subgraph adapters ["git/ and github/ — hardened adapters"] + history[git/history.ts] + clone[git/clone.ts] + ghissues[github/issues.ts] + ghmerge[github/merge.ts] + ghfixed[github/already-fixed.ts] + ghpr[github/pull-requests.ts] + end + subgraph agents ["agents/ — isolation boundary"] + contract[contract.ts, policy.ts] + container[container/*] + network[network/*] + vendors[adapters/claude.ts, codex.ts,
supervisor.ts] + recovery[recovery.ts] + end + web --> runner + runner --> core + runner --> adapters + runner --> agents + agents --> clone +``` + +Dependencies point downward only. `core/` imports nothing that does input or output. `web/` never imports `store.ts` directly except to check the Node version at start-up. + +### What each part owns + +"In use" means production code runs it. "Not connected" means it is merged and tested, but only tests construct it so far. + +| Part | Main files | Owns | State | +|---|---|---|---| +| Command line | `web/cli.ts` | Parses `--demo` or `--config`, takes the single-runner lock, starts the server, releases the lock on Ctrl+C. | In use | +| HTTP server | `web/server.ts` | Binds `127.0.0.1` only. Checks the token, Host and Origin. Bounded JSON bodies. Routes `/api/review`, `/api/action`, `/api/questions`, `/api/merge`, `/api/issues`, `/api/settings`, plus `/api/runner` and `/api/plan/*` when a runner or planning provider is supplied. Ordered shutdown. | In use; `/api/runner` and `/api/plan/*` not connected | +| Browser screen | `web/public/*` | Review screen and Issues screen. Plain JavaScript with IBM Plex fonts served locally. Sends item and segment IDs and a state token, never fingerprints or ownership. | Yes | +| Review service | `runner/review.ts` | Builds the review view: reads history, links it to the plan, computes approval states and merge blockers. Applies review commands through the store. | Yes | +| Ask | `runner/questions.ts`, `runner/question-*.ts` | Answers a question about one plan item with a read-only agent. Runs lane D's synchronous setup in a worker thread so the server stays responsive. Tracks leftover containers. | Yes | +| Merge coordinator | `runner/merge.ts` | Runs the full merge gate twice, re-reads the local generation, then merges the exact reviewed head. Tracks merge-queue attempts. | Yes | +| Runner coordinator | `runner/coordinator.ts`, `runner/lifecycle.ts` | Attempt admission, concurrency slots, compare-and-swap on results, retries, shutdown order. | Not connected | +| Execution | `runner/execution.ts` | Runs plan items in order: fresh workspace, prompt, agent, post-run audit, then the runner's own commit and ledger entry. Pauses in needs amendment on an out-of-scope edit; moves to needs human on a safety violation. | Not connected; uses a test workspace until #66 part 2 and #87 | +| Publishing | `runner/publish.ts` | Runs the already-fixed check, pushes the task head, then opens or reuses the task's pull request (a draft when the task needs a person). Records each opening before calling GitHub, so a lost outcome can be found again by a marker. | Not connected; the push step is injected until the runner's commit bundle (PR #90) exists | +| Lock and startup recovery | `runner/recovery.ts` | One runner per database, held as an OS lock on the database file's device and inode. Startup recovery finalizes interrupted attempts and removes leftover containers and storage. | Lock in use; recovery not connected | +| Store | `runner/store.ts` | The only database handle and all SQL. Revision numbers, snapshots, ledger, approvals, choices, notes, suggestion requests, tasks, attempts, user actions, feedback events, merge attempts, pull request openings, already-fixed results. | In use | +| Plan logic | `core/plan.ts`, `core/parse-v1.ts`, `schema/` | Schema validation, YAML and JSON import, meaning checks (IDs, dependencies, paths, projected file operations), literal command parsing, suggestion edits. | Yes | +| Linking engine | `core/linking.ts` | Replays each commit's line changes and assigns every final line to an owner from the ledger. Produces segments and the Unplanned and Ambiguous rows. | Yes | +| Approvals | `core/approvals.ts` | Approval fingerprints, dependency staleness, assignments, and accept-as-is choices. | Yes | +| Issue ranking | `core/issue-ranking.ts`, `github/issues.ts`, `web/issues.ts` | Deterministic, explainable issue score. Repository-scoped trust of issue authors. | Yes | +| Planning | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself. | Library tested; planning API not connected; no screen | +| Execution prompt and audit | `core/execution-prompt.ts`, `core/run-audit.ts`, `prompts/execute.md` | Builds the execute and fix prompt with untrusted text only in escaped data blocks. Audits the change manifest after a run: safety violations first (Git metadata, links that leave the repository, agent commits), then scope against the declared files. | Not connected | +| Pull request text | `core/pull-request-body.ts` | Title and description with the plan. Plan and agent text go in fenced blocks, and issue references and @-mentions are neutralized, so the text cannot close other issues or notify people. | Not connected | +| Git adapter | `git/history.ts`, `git/clone.ts` | Bounded, hardened history reads. Standalone task clones with no links back to your repository. Every Git command runs with an allowlisted environment. | In use | +| GitHub adapter | `github/merge.ts`, `github/issues.ts`, `github/already-fixed.ts`, `github/pull-requests.ts` | Reads rules, checks, cross-references and issues through `gh`. Runs `gh pr merge --match-head-commit`. Runs the bounded already-fixed check and opens pull requests. Fails closed on anything malformed. | Merge and issues in use; already-fixed and pull requests not connected | +| Agent boundary | `agents/**` | Container image and profile, task storage with byte and inode limits, vendor-only network and proxy, phase tool policy, output supervisor, crash recovery, a bounded diff export, and task-change inspection (`snapshotDeclaredLinks`, `inspectTaskChanges`). | In use by Ask. The runner's commit (#66 part 2) is in review in PR #90 | + +## Trust boundaries + +codeboost has four zones. Data may cross a boundary only in the direction and form shown. + +```mermaid +flowchart LR + subgraph U1 [Untrusted: outside world] + issue[Issue text and comments] + repofiles[Repository contents] + end + subgraph U2 [Semi-trusted: browser] + browser[Browser screen] + end + subgraph T [Trusted: runner] + runnerbox[runner/ + core/ + adapters] + sqlite[(SQLite)] + end + subgraph U3 [Untrusted: agent container] + agentbox[claude / codex] + end + browser -- "IDs + state token only" --> runnerbox + issue -- "as quoted data inside a prompt" --> runnerbox + runnerbox -- "clone + prompt + phase profile" --> agentbox + agentbox -- "bounded output; checked before use" --> runnerbox + repofiles -- "bounded, hardened reads" --> runnerbox + runnerbox --> sqlite +``` + +| Rule | Where it is enforced | +|---|---| +| Only the runner writes the database. | `runner/store.ts` keeps the handle private. | +| The browser can never supply ownership, fingerprints, SHAs or check results. The runner derives them. | `runner/review.ts`, `runner/merge.ts` | +| Commit messages never prove ownership. Only the ledger does. A commit missing from the ledger is foreign. | `core/linking.ts`, `runner/store.ts` | +| Issue text is data, not instructions. It enters a prompt only inside a marked, escaped data block. | `core/planning-author.ts`, `runner/questions.ts` | +| Agents never get your files, your `gh` sign-in, SSH keys, or the Docker socket. Each container gets one vendor's credential only. | `agents/container/profile.ts`, `agents/adapters/setup.ts` | +| Agent network traffic reaches only the vendor's API hosts. | `agents/network/network.ts`, `agents/network/proxy.mjs` | +| Read-only phases cannot write `/work` or run processes. Git metadata is read-only in every phase. | `agents/policy.ts`, `agents/container/storage.ts` | +| Agent output is bounded and fails closed on overflow, timeout or capture failure. | `agents/adapters/supervisor.ts`, `agents/container/run.ts` | +| Every subprocess gets an explicit allowlisted environment. Git runs with no user config, hooks or network protocols. | `git/*`, `agents/docker.ts`, `agents/process-group.ts` | +| After an execute or fix run, safety violations are checked before scope. An unsafe change is never committed. | `core/run-audit.ts`, `agents/container/changes.ts` | +| Plan and agent text in a pull request cannot close issues or notify people. | `core/pull-request-body.ts` | +| Nothing merges without the full gate passing twice against the same base and head, and a fresh local generation check. There is no "merge anyway". | `runner/merge.ts`, `github/merge.ts` | + +## Runtime flows + +### Flow 1: load the review (built) + +1. The browser calls `GET /api/review`. +2. `ReviewService` reads the stored plan revision and snapshot. +3. `git/history.ts` reads the base-to-head commit range with hard limits (500 commits, 32 MiB per Git response, 30-second deadline). +4. `core/linking.ts` replays each commit, using the ledger's ownership for the current plan revision, and produces segments. +5. Segments go onto rows by the placement table in the product design: owner row (in scope or out of scope), Ambiguous row, or Unplanned row. +6. `core/approvals.ts` compares stored approvals and choices with the current fingerprints and marks stale items. +7. The service adds merge blockers and returns the view. The browser renders it. + +### Flow 2: ask a question (built) + +```mermaid +sequenceDiagram + participant B as Browser + participant S as web/server.ts + participant Q as runner/questions.ts + participant W as Worker thread + participant D as agents/ (lane D) + participant C as Agent container + B->>S: POST /api/action (question on P2) + S->>Q: save note, start job + Q->>W: build prompt from current review view + W->>D: clone reviewed head, prepare bounded storage + D->>C: start in "questions" phase (read-only /work, no processes) + C-->>D: bounded answer + D-->>W: settled result; storage released after container stops + W-->>Q: answer + B->>S: GET /api/questions (poll) + S-->>B: answer, or "stale" if the snapshot or plan changed +``` + +### Flow 3: approve and merge (built) + +1. You approve each plan item. The store saves a fingerprint of the lines, file-change metadata, the item's definition and its function context. +2. You press **Approve & merge**. The browser sends only the state token. +3. `runner/merge.ts` reloads everything itself and checks local blockers: unapproved or stale items, out-of-scope files, Ambiguous or Unplanned segments, open change requests, missing `cmd:` results. +4. `github/merge.ts` reads the pull request, rulesets, branch protection, required checks and issue cross-references. Anything unreadable or malformed blocks the merge. +5. The coordinator runs the gate a second time, confirms the same base and head, and re-reads the local generation. +6. It runs `gh pr merge --match-head-commit `. With a merge queue, it records the queue attempt and reports "merged" only when GitHub confirms it. + +### Flow 4: carry out a plan and open the pull request (built, not connected) + +The code for this flow is merged, but it runs only against a test workspace. The steps marked **(pending)** need lane D's runner commit (#66 part 2, PR #90) and the workspace follow-ups (#87). + +```mermaid +sequenceDiagram + participant R as runner/execution.ts + participant S as Store + participant D as agents/ (lane D) + participant C as Agent container + participant P as runner/publish.ts + participant G as gh + R->>S: admit attempt (pending), capture context + R->>D: prepare task clone and bounded storage + R->>D: snapshotDeclaredLinks (refuse launch through a link) + R->>C: execute P1 with prompts/execute.md (writable /work) + C-->>D: settled (agent told not to commit) + R->>D: inspectTaskChanges → change manifest + R->>R: auditRun: safety first, then scope + R->>D: commitTaskChanges → bounded git bundle (pending, PR #90) + R->>S: ledger entry owned by P1 (compare-and-swap) + Note over R: repeat per plan item + P->>G: already-fixed check (12 s deadline, fails closed) + P->>G: push task head (pending), open or reuse the PR + P->>S: record opening, then outcome; task → in review +``` + +Key rules for this flow, from the product design and the runner contract: + +- The runner makes every commit. An agent commit or a Git metadata change makes the attempt fail closed and moves the task to **needs human**. +- An edit outside the declared files is committed but stays on the plan item's row, marked out of scope. The merge gate blocks it until the plan is amended. +- If a plan item needs another file, the task pauses in **needs amendment** until you approve a new plan revision. +- Changes to package manifests or scripts the runner will run pause in **needs approval**. +- Before opening a pull request, the already-fixed check runs. A match, or a check that cannot finish, opens nothing and moves the task to **possibly already fixed**. A task in needs human gets a draft pull request listing its open problems. +- Review rounds (a read-only review agent, then fixes, at most 3 rounds) are designed but not built. + +## State and lifecycles + +### Task status + +```mermaid +stateDiagram-v2 + [*] --> queued + queued --> running + running --> in_review: PR ready + running --> needs_human + running --> needs_amendment + running --> needs_approval + running --> possibly_already_fixed + needs_amendment --> queued: new plan revision approved + needs_approval --> queued: you approve + needs_human --> queued + possibly_already_fixed --> queued: continue + in_review --> queued: reject with feedback + in_review --> approved_but_merge_blocked + approved_but_merge_blocked --> in_review + in_review --> merged + approved_but_merge_blocked --> merged + queued --> cancelled + in_review --> cancelled + merged --> [*] + cancelled --> [*] +``` + +The diagram shows the designed transitions. The lane F code for running, needs human, needs amendment, possibly already fixed and in review is merged but not connected; queueing, needs approval and rejecting with feedback are not built yet. A task closes only on **merged** or **cancelled**. The source of truth for the status names is `TASK_STATUSES` in `runner/lifecycle.ts`. + +### Attempt state + +`pending → running → completed | failed | cancelled | stale` + +| Rule | Why | +|---|---| +| An attempt ID is a UUID v4 and is never reused. A retry is a new attempt. | Late results can be identified and discarded. | +| A result is saved only if the attempt is still current and its captured context (snapshot, plan revision, assignment, referenced code, context generation) still matches. | A slow agent cannot overwrite newer work. | +| For a writable attempt, the terminal state is written only after the container has stopped. | "Cancelled" never means "still running". | +| Cancel records the first reason and shows "Stopping". It does not free the slot. | The slot count stays true. | +| Shutdown order: reject new work, drain HTTP requests with a time limit, abort request-owned work, cancel and await attempts, write terminal states, close storage, release the runner lock. | No work outlives the state that authorized it. | + +### Stored data + +All state is in one SQLite file, opened with WAL and full synchronization. Each write takes an immediate write lock. The schema version is `PRAGMA user_version`, and migrations run in explicit steps; an unknown version fails. + +| Group | Tables | Notes | +|---|---|---| +| Plans | `plans`, `revisions`, `requests` | SQLite allocates revision numbers. Old revisions are never changed. Suggestion requests are bound to a revision and snapshot. | +| Code history | `snapshots`, `ledger`, `rewrites` | Ledger entries are immutable. Rebase mappings record which old commit became which new one; foreign stays foreign. | +| Review | `approvals`, `choices`, `review_notes`, `checkpoints`, `continuations` | Stored approvals are claims about a past snapshot. Freshness is recomputed every time. | +| Runner | `tasks`, `attempts`, `user_actions`, `feedback_events`, `merge_attempts`, `app_settings` | User actions carry idempotency keys. Feedback events are append-only and feed the future learning feature. | +| Publishing | `task_pull_requests`, `already_fixed_checks` | An opening is recorded before the GitHub call, so a lost outcome is recovered, not repeated. | + +## Deployment view + +| Item | Choice | +|---|---| +| Runtime | Node 26.7 or later. TypeScript source runs directly; there is no build step. | +| Runtime dependencies | `ajv`, `yaml`, `diff`, `image-size`, and two IBM Plex font packages. SQLite is Node's built-in `node:sqlite`. | +| Start | `npm run demo` (disposable repository and database under `.codeboost-local/demo`) or `npm start -- --config review.json`. | +| Agent image | Built on first use from `agents/container/Dockerfile`, on a digest-pinned Node base, with pinned `claude` and `codex` versions. | +| Required tools | Git, `gh` (for GitHub features), Docker (for Ask and future agent work). Without Docker, codeboost runs no agents and says what is missing. | + +## Cross-cutting concepts + +- **Fail closed.** A limit that is exceeded, a field that is missing, or an answer that is ambiguous blocks the action. codeboost never truncates evidence and reports "clear". +- **Bounded everything.** Git reads, linking, agent output, prompts, JSON bodies and task storage all have explicit byte, count and time limits. +- **Compare-and-swap.** Every write that depends on earlier state checks a revision, snapshot, state version or attempt ID in the same transaction. +- **Owned resources.** Every container, network and directory carries runner, attempt and allocation labels. If removal fails, the handle is kept and recorded until removal is confirmed. `agents/recovery.ts` removes leftovers after a crash. +- **Idempotent actions.** Replayable actions need an idempotency key. The saved outcome is returned before any other guard runs. +- **Async user interface.** A late response never erases newer input, and old results are marked stale rather than hidden. The rules are in [../AGENTS.md](../AGENTS.md). + +## Architecture decisions + +These are the decisions that shape the structure. The full list, with the reasoning and the people who approved it, is the Decision ledger in the product design. + +| Decision | Chosen | Main alternative rejected | +|---|---|---| +| How ownership is proven | Runner-owned commit ledger | Trusting `Plan-Item:` commit trailers | +| Where agents run | Docker container per attempt, vendor-only egress | Vendor CLI sandbox on the host | +| Agent workspace | Standalone `git clone --local --no-hardlinks` per task, Git metadata read-only | Git worktrees of your checkout | +| Storage | One SQLite file through `node:sqlite` | `sql.js` fallback or a separate database server | +| Who writes the database | Only `runner/store.ts` | Web handlers with direct access | +| GitHub access | `gh` on the host, runner only | A GitHub token inside agent containers | +| Merge binding | `--match-head-commit` after two full gate passes | Merge the pull request's current head | +| Where the plan lives | SQLite, copied into the pull request description when codeboost opens the PR | A plan file committed to the branch | +| User interface | Plain JavaScript page served locally | React bundle (proposed in the design; not needed so far) | + +## Build status and roadmap + +| Lane | Area | State on `main` | +|---|---|---| +| Foundation | Plan, linking, approvals, store, review screen | Done | +| C, K | Guarded merge, merge queue | Done | +| D | Agent isolation boundary | Done, plus the bounded diff export (#76) and task-change inspection (#66 part 1, PR #85). Next: the runner's commit (#66 part 2, PR #90). It builds the commit in one container with both task volumes read-only and returns it as a bounded `git bundle`; this replaces the separate `exportTaskCommit` first planned. Then gitlink mounts (#81). | +| E | Planning library and suggestions | Library done. E4 fixtures are a draft PR waiting for real vendor recordings (#45). | +| F | Runner | Merged: F1a–F1e (lifecycle store, coordinator, shutdown wiring and `/api/runner`, startup recovery and the lock, planning API and feedback events), F2a (execute prompts and post-run audit), F2b (per-item execution and commit step), F2d (already-fixed check and opening the PR). Next: connect the real workspace (#87), continue after a scope pause (#88), apply a stop only after its transaction commits (#79), then rebasing and `cmd:` checks (#22). In review: the pre-merge already-fixed check reusing the pre-PR rules (F6 part, PR #89). | +| Hardening | Allowlisted subprocess environments | Done: every Git command (#82, #83) and every `gh` runner (#84, #86). | +| G | Planning screen | Not started (after E4) | +| H | Issue ranking and Issues screen | Done except the "trust this issue" action | +| I, J | Queue and run windows; learning from feedback | Not started (after F) | + +## Known limits + +- History must be linear. Merge commits are refused with a request to rebase. +- Linking is line-based. It cannot see an unrelated edit inside a declared file; only the review agent and you can catch that. +- The container's guarantees are those of Docker. The agent can always reach its own vendor account. +- `cmd:` acceptance checks do not run yet, so any plan item with a `cmd:` check blocks merging. +- One user, one machine, one runner per database. The lock refuses a second runner, and it refuses network filesystems, where OS locks are unreliable. +- No production path runs an agent that writes code yet. Execution and publishing run only in tests until #66 part 2 and #87 land. + +## Test this document with a reader + +Before you rely on this document, ask someone new to the project to read only the Summary and the layer map, then answer: + +1. Which module is allowed to write the SQLite database? +2. What decides which plan item owns a commit, and what happens to a commit without that proof? +3. Name two things an agent container can never reach. +4. Why does the merge gate run twice? + +If they cannot answer from this document alone, fix the section that failed them. Recheck the "Build status" table and the status line at the top whenever a lane merges. diff --git a/docs/implementation/agent-isolation.md b/docs/implementation/agent-isolation.md index c4d98d87..d38bf470 100644 --- a/docs/implementation/agent-isolation.md +++ b/docs/implementation/agent-isolation.md @@ -67,7 +67,8 @@ Use only these entry points to run an agent: create are not atomic, so right after its first create D checks again that its object is the only one with the ID; if not, it removes what it made and refuses. Cleanup removes an object by the ID captured at its create, even if its labels are wrong; an object found only by name (including task storage) must carry all three owner labels. Call - `removeTaskFilesystems` when the task ends. It refuses a repository that has a + `removeTaskFilesystems` when the task ends (`removeTaskFilesystemsAsync` from a server, which does the same removal + without blocking the event loop). It refuses a repository that has a symbolic link with an absolute target or one that can lead outside the checkout (resolved in the container as its kernel would, even once the agent creates a missing directory on the way), or any link in its Git metadata; the seeder checks this and the allocation removes what it made, then throws an `UnusableRepositoryError`. Report @@ -103,7 +104,10 @@ Use only these entry points to run an agent: IDs as a commit would store them. New ignored files, fifos and entries under a `.git` part are listed; a new directory that `base`'s own ignore rules ignore, with no tracked entry beneath it, is one entry (`ignored: true`). It also returns `agentCommits`, `metadataChanged`, `linkTargetChanges`, `nestedGitlinkContent` and `digest`. If the metadata changed, no Git - command runs: the manifest has `metadataChanged: true` and every other list empty. + command runs: the manifest has `metadataChanged: true` and every other list empty. Every change whose new entry + is a symlink carries `linkTargetTraversesLink`: whether its target, resolved one part at a time as the kernel + would in an agent container, passes through another link or is one. A target outside the work tree or in the + metadata counts as `true`. - **Needs human.** The metadata is read-only to agents, so any agent commit or metadata change means a protection failed. Route it to needs human, as for link target changes and nested gitlink content. - **Refusals.** It refuses, and never returns part of the answer, when: diff --git a/docs/implementation/runner-lifecycle.md b/docs/implementation/runner-lifecycle.md index f7c54587..1522b100 100644 --- a/docs/implementation/runner-lifecycle.md +++ b/docs/implementation/runner-lifecycle.md @@ -207,7 +207,7 @@ The server computes `retryable` and sends it to the UI. The UI never works it ou ## Publishing a result 1. Wait for `settled`. -2. Validate the output (schema, size, file-scope audit for writable attempts). Do not await anything between the last check and the transaction. For a writable attempt, the audit reads D's change manifest (#66): F calls `snapshotDeclaredLinks` before launch and keeps the snapshot, and after `settled` calls `inspectTaskChanges` with it and the storage's `metadataBaseline`, which F recorded at allocation. Any agent commit, metadata change, change to a declared link's target, nested gitlink content, or an inspection that refuses, sends the task to needs human with no tests and no commit. A `retargeted` entry for a declared link the item itself edits is that edit, which plan-format.md permits at declared paths; F2a judges it by the plan's link rules, not as a target change. A declared link whose snapshot status is `through-link` is refused before launch. +2. Validate the output (schema, size, file-scope audit for writable attempts). Do not await anything between the last check and the transaction. For a writable attempt, the audit reads D's change manifest (#66): F calls `snapshotDeclaredLinks` before launch and keeps the snapshot, and after `settled` calls `inspectTaskChanges` with it and the storage's `metadataBaseline`, which F recorded at allocation. Any agent commit, metadata change, change to a declared link's target, nested gitlink content, new file that base's own ignore rules ignore (#87 decision 2), or an inspection that refuses, sends the task to needs human with no tests and no commit. A `retargeted` entry for a declared link is that link's own edit when the manifest lists it, which plan-format.md permits at declared paths; F2a judges it by the plan's link rules, not as a target change. Without that edit in the manifest it is a target change. A declared link whose snapshot status is `through-link` is refused before launch. 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: @@ -218,6 +218,18 @@ The server computes `retryable` and sends it to the UI. The UI never works it ou **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.) +## Runner-owned repository (#87) + +codeboost never writes the user's repository. The commits it makes live in a **runner-owned repository**: a bare repository per configured repository, at `//repositories/.git` (`runner/runner-repository.ts`), in directories only the current user can write. + +| Step | What happens | +|---|---| +| Materialize | `ensureCommit` makes the recorded head available: a base commit is fetched from the user's repository by ID (no refs, tags or `FETCH_HEAD`); a runner commit is there already. The task clone is made from the runner-owned repository (`runner/workspace.ts`). | +| Commit | `commitTaskChanges` returns a bundle. `fetchTaskCommit` takes it in under `refs/codeboost/attempts/` only if its one ref is the commit's ID and the commit is a single commit on its base; otherwise the ref is removed. | +| Release | After the terminal write, task storage is removed, then the ref of an attempt that did not complete is dropped. A completed attempt's ref keeps its commit. | +| Review | Once a task has runner commits (a completed execute or fix attempt that made a commit), the review screen and Ask read it from the runner-owned repository at the head the Store recorded. The user's HEAD is no longer observed for it: runner commits move the head only through the Store, with their ledger entries, and observing the user's older HEAD would roll the task back. A task without runner commits still observes the user's HEAD, owned ledger entries or not: a reviewed branch in the user's repository (the demo, a planted experiment) carries them too. | + + ## Shutdown `web/server.ts` `close()` and the runner coordinator follow this order. Part of step 1 and all of steps 2 and 3 already exist. F1 adds the coordinator barrier in step 1, steps 4 and 5, and step 8. diff --git a/git/clone.ts b/git/clone.ts index 39877c4d..6ff37997 100644 --- a/git/clone.ts +++ b/git/clone.ts @@ -33,10 +33,11 @@ const PAUSE = Symbol('pause'); interface GitOutcome { readonly status: number | null; readonly stdout: string; readonly stderr: string; readonly error?: Error } // Entries walked between pauses, so the asynchronous variant never holds the event loop for a whole object store. const PAUSE_EVERY = 1_000; -const GIT_OPTIONS = ['--no-pager', '--no-replace-objects', '-c', 'core.hooksPath=/dev/null', '-c', 'init.templateDir=', +/** The hardened Git invocation for repositories the runner owns: no hooks, templates, network, submodules or replacements. */ +export const GIT_OPTIONS = ['--no-pager', '--no-replace-objects', '-c', 'core.hooksPath=/dev/null', '-c', 'init.templateDir=', '-c', 'protocol.allow=never', '-c', 'submodule.recurse=false']; // Deliberately do not inherit Git variables or credential/config environment. -const gitEnvironment = () => ({ PATH: process.env.PATH, GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', +export const gitEnvironment = () => ({ PATH: process.env.PATH, GIT_CONFIG_NOSYSTEM: '1', GIT_CONFIG_GLOBAL: '/dev/null', GIT_TERMINAL_PROMPT: '0', GIT_NO_LAZY_FETCH: '1', GIT_GRAFT_FILE: '/dev/null' }); export interface CloneOptions { diff --git a/runner/execution.ts b/runner/execution.ts index 98321a58..8d482547 100644 --- a/runner/execution.ts +++ b/runner/execution.ts @@ -4,6 +4,7 @@ import type { InvocationContext, InvocationHandle, InvocationInput, TaskClone } 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 type { DeclaredLinkSnapshot } from '../agents/container/changes.ts'; import { FinishFailure, NEEDS_RESTART, PreparationFailure, type PreparedAttempt, type RunnerCoordinator, type RunnerDeps } from './coordinator.ts'; import type { AttemptRecord, Store } from './store.ts'; import { CLOSED_STATUSES, GuardRefusal, ShuttingDownError, bounded, sameContext, settleWith, type ShutdownCapability } from './lifecycle.ts'; @@ -20,15 +21,21 @@ export interface TaskWorkspace { * No-follow snapshot, taken before launch, of every declared path that is a symlink in this workspace. F passes every * path the item declares: an earlier item may have renamed or added links, so only D sees the actual entries. */ - snapshotDeclaredLinks(workspace: WorkspaceRef, paths: readonly string[], signal: AbortSignal): Promise; - /** The change manifest after the agent settled, plus a digest the commit step must match. */ - inspectChanges(workspace: WorkspaceRef, input: { baseHead: string; linkSnapshot: unknown }, signal: AbortSignal): Promise; + snapshotDeclaredLinks(workspace: WorkspaceRef, paths: readonly string[], signal: AbortSignal): Promise; + /** D's change manifest after the agent settled (`TaskChangeManifest`), with the digest the commit step must match. */ + inspectChanges(workspace: WorkspaceRef, input: { baseHead: string; linkSnapshot: DeclaredLinkSnapshot }, signal: AbortSignal): Promise; /** - * Commit exactly `paths` (both sides of every rename) on top of `baseHead` with hooks off; refuse if the tree no longer - * matches `digest`. Agent commits are never undone: a manifest with any is a safety violation and never gets here (#66). + * The runner's commit (#66, `commitTaskChanges`): `baseHead` plus exactly the changes of the manifest whose digest is + * `digest`, with hooks off, refused if the work tree no longer matches it. Returns the new head, which is then in the + * runner-owned repository (#87), so the next item's `materialize` and the push find it. Agent commits are never + * undone: a manifest with any is a safety violation and never gets here (#66). */ - commit(workspace: WorkspaceRef, input: { baseHead: string; paths: readonly string[]; message: string; trailers: Readonly>; digest: string }, signal: AbortSignal): Promise; + commit(workspace: WorkspaceRef, input: { baseHead: string; linkSnapshot: DeclaredLinkSnapshot; message: string; + trailers: Readonly>; digest: string }, signal: AbortSignal): Promise; + /** After the terminal write: remove task storage, and drop the commit of an attempt that did not complete. */ release(workspace: WorkspaceRef): Promise; + /** Remove host-side preparation files (the staging clone). Called before the terminal write when D never ran, after it otherwise. */ + cleanupPreparation?(attempt: AttemptRecord): Promise; } /** D's start call for an execute/fix phase with this prompt; returns at once (see #51). */ export type AgentLauncher = (input: InvocationInput, prompt: string, workspace: WorkspaceRef) => InvocationHandle; @@ -53,7 +60,7 @@ export class SafetyFindings { settle(attemptId: string): void { this.#found.delete(attemptId); } } export interface ExecutionResult { head: string; unchanged: boolean; inScope: string[]; outOfScope: string[] } -interface Private { workspace: WorkspaceRef; prompt: string; baseHead: string; linkSnapshot: unknown } +interface Private { workspace: WorkspaceRef; prompt: string; baseHead: string; linkSnapshot: DeclaredLinkSnapshot | undefined } /** * RunnerDeps for execute attempts: fresh workspace, prompt, agent, then audit and the runner's own commit. @@ -82,7 +89,8 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag catch (error) { throw new PreparationFailure(error, prepared); } return prepared; }, - async cleanupPreparation() { /* host-side files belong to D's materialize; task storage waits for release */ }, + // Host-side files only (the staging clone); task storage waits for release. + async cleanupPreparation(attempt) { await workspace.cleanupPreparation?.(attempt); }, start(input, prepared) { const data = prepared.private as Private; return launch(input, data.prompt, data.workspace); }, validate() { throw new Error('Execute attempts publish through finish().'); }, async finish(attempt, _result, prepared, signal) { @@ -94,7 +102,7 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag throw new FinishFailure(text); }; let manifest: ChangeManifest & { digest: string }; - try { manifest = await workspace.inspectChanges(data.workspace, { baseHead: data.baseHead, linkSnapshot: data.linkSnapshot }, signal); } + try { manifest = await workspace.inspectChanges(data.workspace, { baseHead: data.baseHead, linkSnapshot: data.linkSnapshot! }, signal); } catch (error) { // Only the stop's own abort error is the stop. Any other refusal is a finding, even if a stop is also pending. if (signal.aborted && (error === signal.reason || (error instanceof Error && error.name === 'AbortError'))) throw error; @@ -111,11 +119,10 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag // Last check before the commit, after the last await: a stop, shutdown or context change makes nothing. if (signal.aborted) throw signal.reason; if (!sameContext(attempt.context, store.currentContext(identity))) throw new FinishFailure('The plan, snapshot or assignment changed during the audit; nothing was committed.'); - // Every change is in or out of scope here; a rename stages both its old and its new path. - const paths = [...new Set(manifest.changes.flatMap(entry => [entry.path, ...(entry.oldPath ? [entry.oldPath] : [])]))]; let head: string; try { head = await workspace.commit(data.workspace, { - baseHead: data.baseHead, paths, digest: manifest.digest, + // Every change is in or out of scope here, and the commit holds all of them: exactly the manifest audited. + baseHead: data.baseHead, linkSnapshot: data.linkSnapshot!, 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. Its // issue references and mentions are neutralised (AGENTS.md): on the default branch, "Fixes #12" would close #12. @@ -142,7 +149,7 @@ export function executionDeps(store: Store, workspace: TaskWorkspace, launch: Ag }; } /** The plan identity that owns an attempt; attempts are stored per plan key. */ -function findIdentity(store: Store, attempt: AttemptRecord): PlanIdentity { +export function findIdentity(store: Store, attempt: AttemptRecord): PlanIdentity { const key = store.attemptOwner(attempt.id); if (!key) throw new Error('Unknown attempt.'); const [repositoryId, taskId, planId] = JSON.parse(key) as string[]; diff --git a/runner/questions.ts b/runner/questions.ts index 5860fd6b..41e4c59a 100644 --- a/runner/questions.ts +++ b/runner/questions.ts @@ -64,7 +64,7 @@ export class Questions { try { if(!agent) throw new Error('Choose a question agent in Settings, then retry.'); const aborted = new Promise((_,reject)=>controller.signal.addEventListener('abort',()=>reject(controller.signal.reason),{once:true})); - const scope={repository:this.service.config.repository,head:view.snapshot.head,snapshotId:view.snapshot.id,planId:this.service.config.identity.planId,planRevision:view.plan.revision,noteId:id,attemptId:attempt,contextId:note.contextId}; + const scope={repository:this.service.reviewRepository().path,head:view.snapshot.head,snapshotId:view.snapshot.id,planId:this.service.config.identity.planId,planRevision:view.plan.revision,noteId:id,attemptId:attempt,contextId:note.contextId}; invocation = agent(questionPrompt(view,note),controller.signal,scope,QUESTION_TIMEOUT_MS); const text=await Promise.race([invocation,aborted]); if(typeof text!=='string'||!text.trim()||text.length>24000) throw new Error('Agent returned an empty or oversized answer.'); diff --git a/runner/review.ts b/runner/review.ts index f5b67617..a61ebcaf 100644 --- a/runner/review.ts +++ b/runner/review.ts @@ -12,7 +12,10 @@ import { applyChoices, approvalStates, approveItem, choiceKeys, fingerprint, rev import type { PlanItem } from '../core/plan.ts'; import type { GhMergeConfig } from '../github/merge.ts'; -export interface ReviewConfig { database: string; repository: string; identity: PlanIdentity; pathIdentity: { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' }; demo?: boolean; github?: GhMergeConfig } +export interface ReviewConfig { database: string; repository: string; + /** The runner-owned repository (#87) holding the commits codeboost makes; required once the task has any. */ + runnerRepository?: string; + identity: PlanIdentity; pathIdentity: { caseSensitive: boolean; unicodeNormalization: 'none' | 'NFC' }; demo?: boolean; github?: GhMergeConfig } export class ReviewService { store: Store; config: ReviewConfig; @@ -21,13 +24,27 @@ export class ReviewService { this.config = config; this.store = new Store(config.database); } close() { this.store.close(); } + /** + * Where the task's reviewed commits are. Once the runner has committed for the task (a completed writable attempt that + * made a commit, #87), its branch is the runner's: the head is the one the Store recorded with that commit, in the + * runner-owned repository. Before that, the user's repository and its HEAD. Owned ledger entries alone do not decide it: + * a reviewed branch in the user's repository (the demo, a planted experiment) carries them too. + */ + reviewRepository(): { path: string; runnerOwned: boolean } { + if (!this.store.hasRunnerCommit(this.config.identity)) return { path: this.config.repository, runnerOwned: false }; + if (!this.config.runnerRepository) throw new Error('This task has runner commits, so its review needs the runner-owned repository, which is not configured.'); + return { path: this.config.runnerRepository, runnerOwned: true }; + } load() { - const { identity, repository, pathIdentity } = this.config; + const { identity, pathIdentity } = this.config; const reviewVersion = this.store.reviewVersion(identity); const plan = this.store.getPlan(identity); let snapshot = this.store.getSnapshot(identity); - // HEAD changes are observed; no Git mutation is performed by the review service. - const history = readHistory(repository, snapshot.base, 'HEAD'); + const reviewed = this.reviewRepository(); + // HEAD changes in the user's repository are observed; no Git mutation is performed by the review service. Runner + // commits move the head only through the Store, in the same transaction as their ledger entries, so there the + // recorded head is read as it is: observing the user's HEAD would record its older commit and roll the task back. + const history = readHistory(reviewed.path, snapshot.base, reviewed.runnerOwned ? snapshot.head : 'HEAD'); if (history.head !== snapshot.head) snapshot = this.store.recordHistory(identity, { revision: plan.revision, snapshotId: snapshot.id, reviewVersion }, history.base, history.head, []); const pathKey = (path: string) => { if (!pathIdentity.caseSensitive && /[^\x20-\x7e]/.test(path)) throw new Error('Non-ASCII case-insensitive paths require a filesystem-specific identity adapter.'); @@ -112,7 +129,7 @@ export class ReviewService { }; }); const token = createHash('sha256').update(JSON.stringify({ expected, saved, plan, segments })).digest('hex'); - return { repository: basename(repository), demo: this.config.demo ?? false, plan, snapshot, expected, token, items, segments, notes, approved: items.filter(item => item.state === 'approved').length }; + return { repository: basename(this.config.repository), demo: this.config.demo ?? false, plan, snapshot, expected, token, items, segments, notes, approved: items.filter(item => item.state === 'approved').length }; } /** The trusted plan context for import and Apply: base entries from the snapshot's base tree, and the configured path identity. */ planContext(): PlanContext { diff --git a/runner/runner-repository.ts b/runner/runner-repository.ts new file mode 100644 index 00000000..d1bb86f1 --- /dev/null +++ b/runner/runner-repository.ts @@ -0,0 +1,140 @@ +import { createHash, randomUUID } from 'node:crypto'; +import { lstatSync, mkdirSync, mkdtempSync, renameSync, rmSync, statSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { TASK_COMMIT_REF } from '../agents/container/changes.ts'; +import { runInProcessGroup, type ProcessGroup } from '../agents/process-group.ts'; +import { GIT_OPTIONS, gitEnvironment } from '../git/clone.ts'; +import { isUuidV4 } from './lifecycle.ts'; + +/** + * The runner-owned repository (#87 decision 1): a bare repository per configured repository, under the runner root, + * holding the commits codeboost makes. Task clones are made from it, the push reads from it, and the review screen + * reads a task's runner commits from it. codeboost never writes the user's repository: base commits are fetched from + * it, and each runner commit arrives as the bundle `commitTaskChanges` returned. + */ +export interface RunnerRepository { + /** The bare repository's directory. */ + readonly path: string; + /** The user's repository, where base commits come from. Only ever read. */ + readonly source: string; +} +export interface GitCallOptions { + readonly signal?: AbortSignal; + /** Called in the same turn as each Git spawn, so the caller can record the group. */ + readonly onProcessGroup?: (group: ProcessGroup) => void; + /** Deadline for each Git call. Default 120 s. */ + readonly timeoutMs?: number; +} +/** + * Create `/` as needed, then require the root and every directory below it on the way to be a real + * directory owned by the current user that no one else can write: whoever can write a directory can replace what is in it. + */ +export function ownerOnlyDirectory(root: string, ...parts: string[]): string { + const path = join(root, ...parts); + mkdirSync(path, { recursive: true, mode: 0o700 }); + for (let depth = 0; depth <= parts.length; depth += 1) { + const directory = join(root, ...parts.slice(0, depth)); + const stat = lstatSync(directory); + if (!stat.isDirectory() || (process.getuid && stat.uid !== process.getuid()) || (stat.mode & 0o022) !== 0) + throw new Error(`${directory} must be a directory owned by you and not writable by group or others.`); + } + return path; +} +/** The ref that keeps one attempt's runner commit; deleted when the attempt does not complete. */ +export const attemptRef = (attemptId: string): string => { + if (!isUuidV4(attemptId)) throw new Error('Attempt ID must be a UUID v4.'); + return `refs/codeboost/attempts/${attemptId}`; +}; +const COMMIT_ID = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/; + +async function git(repository: string, args: readonly string[], options: GitCallOptions & { allowFiles?: boolean } = {}): Promise { + const outcome = await runInProcessGroup('git', [...GIT_OPTIONS, '-c', 'gc.auto=0', '-c', 'maintenance.auto=false', + ...options.allowFiles ? ['-c', 'protocol.file.allow=always'] : [], ...args], + { cwd: repository, env: gitEnvironment(), timeoutMs: options.timeoutMs ?? 120_000, signal: options.signal, + onProcessGroup: options.onProcessGroup, maxBuffer: 1024 * 1024 }); + if ((outcome.error as NodeJS.ErrnoException | undefined)?.code === 'ABORT_ERR') + throw Object.assign(new Error(`git ${args[0]} was cancelled.`), { name: 'AbortError', code: 'ABORT_ERR' }); + // Git's message can quote a path; it stays one line of printable text. + if (outcome.status !== 0) throw new Error(`git ${args[0]} failed${outcome.status === null ? '' : ` (exit ${outcome.status})`}: ` + + JSON.stringify(outcome.stderr.trim().slice(0, 400) || outcome.error?.message || '')); + return outcome.stdout.trim(); +} +const exists = async (repository: string, object: string, options: GitCallOptions) => { + try { await git(repository, ['cat-file', '-e', object], options); return true; } catch (error) { + if ((error as Error).name === 'AbortError') throw error; + return false; + } +}; + +/** + * Open the runner-owned repository for `repositoryId`, creating it if needed. It lives at + * `//repositories/.git`, in directories only the current user can write. A new one is + * created under a temporary name and renamed into place, so a crash never leaves a half-made one under the real name. + */ +export async function openRunnerRepository(o: { runnerRoot: string; runnerOwner: string; repositoryId: string; source: string } + & GitCallOptions): Promise { + if (!/^[0-9a-f]{32}$/.test(o.runnerOwner)) throw new Error('Invalid runner owner token.'); + const parent = ownerOnlyDirectory(o.runnerRoot, o.runnerOwner, 'repositories'); + const path = join(parent, `${createHash('sha256').update(o.repositoryId).digest('hex').slice(0, 32)}.git`); + if (!lstatSync(path, { throwIfNoEntry: false })) { + const staging = mkdtempSync(join(parent, '.new-')); + try { + const format = await git(o.source, ['rev-parse', '--show-object-format'], o); + await git(staging, ['init', '-q', '--bare', '--template=', `--object-format=${format === 'sha256' ? 'sha256' : 'sha1'}`, '.'], o); + try { renameSync(staging, path); } catch (error) { + // Another opener made it first; theirs is used. + if (!['EEXIST', 'ENOTEMPTY'].includes((error as NodeJS.ErrnoException).code ?? '')) throw error; + } + } finally { rmSync(staging, { recursive: true, force: true }); } + } + const stat = lstatSync(path); + if (!stat.isDirectory() || stat.isSymbolicLink()) throw new Error(`${path} is not the runner repository.`); + if (await git(path, ['rev-parse', '--is-bare-repository'], o) !== 'true') throw new Error(`${path} is not a bare repository.`); + return Object.freeze({ path, source: o.source }); +} + +/** Make `head` available for a task clone: a runner commit is there already; a base commit is fetched from the source. */ +export async function ensureCommit(repository: RunnerRepository, head: string, options: GitCallOptions = {}): Promise { + if (!COMMIT_ID.test(head)) throw new Error('A full commit ID is required.'); + if (await exists(repository.path, `${head}^{commit}`, options)) return; + // Protocol v2 serves any commit the source has by ID. Nothing else is taken: no tags, no refs, no FETCH_HEAD. + await git(repository.path, ['fetch', '-q', '--no-tags', '--no-write-fetch-head', '--no-recurse-submodules', '--', repository.source, head], + { ...options, allowFiles: true }); + if (!(await exists(repository.path, `${head}^{commit}`, options))) throw new Error(`The source repository has no commit ${head}.`); +} + +/** + * Take a runner commit in from the bundle `commitTaskChanges` returned, under the attempt's ref, and check that it is + * the commit the runner made: the bundle's one ref is `head`, and `head` is a single commit on top of `base`. + */ +export async function fetchTaskCommit(repository: RunnerRepository, input: { bundle: Buffer; base: string; head: string; + attemptId: string }, options: GitCallOptions = {}): Promise { + if (!COMMIT_ID.test(input.base) || !COMMIT_ID.test(input.head)) throw new Error('A full commit ID is required.'); + const ref = attemptRef(input.attemptId); + // The bundle file is the runner's own, beside the repository, readable only by the current user. + const file = join(repository.path, `codeboost-${randomUUID()}.bundle`); + writeFileSync(file, input.bundle, { mode: 0o600, flag: 'wx' }); + try { + if (statSync(file).size !== input.bundle.length) throw new Error('The runner commit bundle was not written whole.'); + await git(repository.path, ['bundle', 'verify', '-q', file], options); + const heads = (await git(repository.path, ['bundle', 'list-heads', file], options)).split('\n'); + if (heads.length !== 1 || heads[0] !== `${input.head} ${TASK_COMMIT_REF}`) throw new Error('The runner commit bundle does not hold exactly the runner commit.'); + await git(repository.path, ['fetch', '-q', '--no-tags', '--no-write-fetch-head', '--', file, `+${TASK_COMMIT_REF}:${ref}`], + { ...options, allowFiles: true }); + } finally { rmSync(file, { force: true }); } + try { + if (await git(repository.path, ['rev-parse', '--verify', `${ref}^{commit}`], options) !== input.head) + throw new Error('The fetched runner commit is not the one the commit step made.'); + if (await git(repository.path, ['rev-list', '--parents', '-n', '1', input.head], options) !== `${input.head} ${input.base}`) + throw new Error('The runner commit is not a single commit on top of its base.'); + } catch (error) { + // Nothing keeps a commit that failed its checks. + await git(repository.path, ['update-ref', '-d', ref], { timeoutMs: options.timeoutMs }).catch(() => {}); + throw error; + } +} + +/** Drop an attempt's ref, for an attempt whose commit was not published. Its objects become unreachable. */ +export async function dropAttemptRef(repository: RunnerRepository, attemptId: string, options: GitCallOptions = {}): Promise { + await git(repository.path, ['update-ref', '-d', attemptRef(attemptId)], options); +} diff --git a/runner/store.ts b/runner/store.ts index 7425361a..1b5b181a 100644 --- a/runner/store.ts +++ b/runner/store.ts @@ -8,7 +8,7 @@ import type { InvocationContext, StopReason } from '../agents/contract.ts'; import type { AlreadyFixedResult } from '../github/already-fixed.ts'; import { ATTEMPT_PHASES, BadRequest, CLOSED_STATUSES, MERGEABLE_STATUSES, ShuttingDownError, type ShutdownCapability, DEFAULT_TASK_BUDGET_MS, FIRST_REASONS, GuardRefusal, ActionIdReused, RefusalWithEffect, MAX_RESULT_BYTES, TASK_STATUSES, TERMINAL_STATES, - assertUuidV4, bounded, classifySettlement, requestHash, sameContext, + WRITABLE_KINDS, assertUuidV4, bounded, classifySettlement, requestHash, sameContext, type AttemptKind, type AttemptState, type Classification, type FirstReason, type Settlement, type TaskStatus, } from './lifecycle.ts'; @@ -1241,6 +1241,14 @@ export class Store { return token; }); } + /** + * Whether the runner has committed for this task (#87): a completed writable attempt whose result made a commit. Only + * then do the task's commits live in the runner-owned repository. One indexed lookup, for every review load. + */ + hasRunnerCommit(identity: PlanIdentity): boolean { + return !!this.#get(`SELECT 1 AS found FROM attempts WHERE plan_key=? AND kind IN (${WRITABLE_KINDS.map(() => '?').join(',')}) AND state='completed' + AND json_valid(result) AND json_extract(result, '$.unchanged') = 0 LIMIT 1`, identityKey(identity), ...WRITABLE_KINDS); + } /** "Preparation starting": saved before any preparation subprocess is spawned. */ markPreparationStarting(identity: PlanIdentity, id: string, startedAt: number): void { const key = identityKey(identity); @@ -1251,10 +1259,16 @@ export class Store { cancelPreparationStart(identity: PlanIdentity, id: string): void { this.#run(`UPDATE attempts SET preparation_started_at=NULL WHERE plan_key=? AND id=? AND preparation_pgid IS NULL AND state='pending'`, identityKey(identity), id); } - /** Saved in the same synchronous turn as the spawn. */ - recordPreparationGroup(identity: PlanIdentity, id: string, pgid: number): void { + /** + * Saved in the same synchronous turn as the spawn. Preparation can run several subprocesses in turn: each one replaces + * the last, with its own start time when given, so startup recovery's start-time check (which guards against PID reuse) + * matches the group it finds. + */ + recordPreparationGroup(identity: PlanIdentity, id: string, pgid: number, startedAt?: number): void { if (!Number.isSafeInteger(pgid) || pgid < 2) throw new GuardRefusal('Invalid process group.'); - if (this.#run(`UPDATE attempts SET preparation_pgid=? WHERE plan_key=? AND id=? AND preparation_started_at IS NOT NULL`, pgid, identityKey(identity), id).changes !== 1) + if (startedAt !== undefined && !Number.isSafeInteger(startedAt)) throw new GuardRefusal('Invalid process start time.'); + if (this.#run(`UPDATE attempts SET preparation_pgid=?, preparation_started_at=COALESCE(?, preparation_started_at) WHERE plan_key=? AND id=? AND preparation_started_at IS NOT NULL`, + pgid, startedAt ?? null, identityKey(identity), id).changes !== 1) throw new GuardRefusal('Preparation was not marked as starting.'); } /** F chooses the allocation ID and saves it before the asynchronous allocation starts. */ diff --git a/runner/workspace.ts b/runner/workspace.ts new file mode 100644 index 00000000..38cb25fd --- /dev/null +++ b/runner/workspace.ts @@ -0,0 +1,106 @@ +import { randomUUID } from 'node:crypto'; +import { lstatSync, mkdirSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { commitTaskChanges, inspectTaskChanges, snapshotDeclaredLinks } from '../agents/container/changes.ts'; +import { prepareTaskFilesystemsAsync, removeTaskFilesystemsAsync, type TaskFilesystems, type TaskStorageLimits } from '../agents/container/storage.ts'; +import type { ProcessGroup } from '../agents/process-group.ts'; +import { identityKey, type PlanIdentity } from '../core/identity.ts'; +import { createTaskCloneAsync } from '../git/clone.ts'; +import { findIdentity, type TaskWorkspace, type WorkspaceRef } from './execution.ts'; +import { isUuidV4 } from './lifecycle.ts'; +import { dropAttemptRef, ensureCommit, fetchTaskCommit, ownerOnlyDirectory, type RunnerRepository } from './runner-repository.ts'; +import type { AttemptRecord, Store } from './store.ts'; + +export interface WorkspaceOptions { + readonly store: Store; + /** The directory startup recovery cleans: attempt directories live in `//attempts`. */ + readonly runnerRoot: string; + /** The database's runner token; task storage is allocated under it. */ + readonly runnerOwner: string; + readonly repository: RunnerRepository; + /** The immutable ID of the built agent image. */ + readonly imageId: string; + readonly limits: TaskStorageLimits; + /** Who runner commits are by; the date is the commit step's own time, in UTC. */ + readonly committer: { readonly name: string; readonly email: string }; + readonly now?: () => number; +} +/** What `materialize` keeps for the later steps, behind `WorkspaceRef.storage`. */ +interface Held { readonly filesystems: TaskFilesystems; readonly attemptId: string; readonly identity: PlanIdentity } +const held = (workspace: WorkspaceRef) => workspace.storage as Held; + +/** + * The real task workspace over lane D (#87): a fresh clone of the runner-owned repository at the recorded head, copied + * into bounded task storage; D's link snapshot, change inspection and runner commit; and the commit taken into the + * runner-owned repository under the attempt's ref. The user's repository is only ever read, for base commits. + */ +export function createTaskWorkspace(options: WorkspaceOptions): TaskWorkspace { + const { store, repository, imageId } = options; + if (!/^[0-9a-f]{32}$/.test(options.runnerOwner)) throw new Error('Invalid runner owner token.'); + const attemptDirectory = (attemptId: string) => { + if (!isUuidV4(attemptId)) throw new Error('Attempt ID must be a UUID v4.'); + return join(options.runnerRoot, options.runnerOwner, 'attempts', attemptId); + }; + return { + async materialize(attempt, head, signal) { + const identity = findIdentity(store, attempt), directory = attemptDirectory(attempt.id); + // Every preparation subprocess is recorded, with its own start time, so startup recovery can stop one a crash left + // running. + store.markPreparationStarting(identity, attempt.id, Date.now()); + let recorded = false; + const record = (group: ProcessGroup) => { store.recordPreparationGroup(identity, attempt.id, group.pgid, group.startedAt); recorded = true; }; + try { + await ensureCommit(repository, head, { signal, onProcessGroup: record }); + ownerOnlyDirectory(options.runnerRoot, options.runnerOwner, 'attempts'); + mkdirSync(directory, { mode: 0o700 }); + const clone = await createTaskCloneAsync({ source: repository.path, parent: directory, taskId: identityKey(identity), head, + timeoutMs: 120_000, signal, onProcessGroup: record }); + // Saved before the allocation starts, so a crash during it still leaves a row that matches what D made. + const allocationId = randomUUID(); + store.recordAllocation(identity, attempt.id, allocationId); + const filesystems = await prepareTaskFilesystemsAsync(clone, options.limits, imageId, + { runnerOwner: options.runnerOwner, attemptId: attempt.id, allocationId }, { signal, onProcessGroup: record, timeoutMs: 120_000 }); + return { clone, storage: { filesystems, attemptId: attempt.id, identity } satisfies Held }; + } catch (error) { + // No subprocess ever started (its spawn failed at once): the "starting" marker must not block the next startup. + if (!recorded) { try { store.cancelPreparationStart(identity, attempt.id); } catch { /* the marker stays; recovery asks a person */ } } + throw error; + } + }, + async snapshotDeclaredLinks(workspace, paths, signal) { + return snapshotDeclaredLinks(held(workspace).filesystems, paths, { imageId, signal }); + }, + async inspectChanges(workspace, input, signal) { + return inspectTaskChanges(held(workspace).filesystems, { base: input.baseHead, linkSnapshot: input.linkSnapshot, imageId, signal }); + }, + async commit(workspace, input, signal) { + const { filesystems, attemptId } = held(workspace); + const date = `${Math.floor((options.now?.() ?? Date.now()) / 1000)} +0000`; + const who = { name: options.committer.name, email: options.committer.email, date }; + const made = await commitTaskChanges(filesystems, { base: input.baseHead, linkSnapshot: input.linkSnapshot, digest: input.digest, + message: input.message, trailers: input.trailers, author: who, committer: who, imageId, signal }); + // Nothing to commit: the caller treats an unchanged head as no commit. + if (made.unchanged) return input.baseHead; + await fetchTaskCommit(repository, { bundle: made.bundle, base: input.baseHead, head: made.head, attemptId }, { signal }); + return made.head; + }, + async release(workspace) { + const { filesystems, attemptId, identity } = held(workspace); + // Storage first: a failure here holds the slot under a marker, and nothing below may stop the removal. + await removeTaskFilesystemsAsync(filesystems); + // Only a completed attempt's commit is published (its ledger entry is saved with `completed`); any other is dropped. + // An unreadable row, or a drop that fails, keeps the ref: an unpublished commit in the runner's own repository is + // never read, since nothing in the Store points at it. + let completed = true; + try { completed = store.getAttempt(identity, attemptId).state === 'completed'; } catch { /* keep it */ } + if (!completed) await dropAttemptRef(repository, attemptId) + .catch(error => console.error(`Runner job ${attemptId} could not drop its unpublished commit: ${JSON.stringify((error as Error).message)}`)); + }, + async cleanupPreparation(attempt: AttemptRecord) { + const directory = attemptDirectory(attempt.id), stat = lstatSync(directory, { throwIfNoEntry: false }); + if (!stat) return; + if (!stat.isDirectory() || stat.isSymbolicLink()) throw new Error('The attempt path is not a plain directory.'); + rmSync(directory, { recursive: true, force: true }); + }, + }; +} diff --git a/test/agent-container.test.ts b/test/agent-container.test.ts index 2694d5f7..825bcf5d 100644 --- a/test/agent-container.test.ts +++ b/test/agent-container.test.ts @@ -985,10 +985,30 @@ describe('real Docker agent isolation', () => { expect.objectContaining({ kind: 'modify', path: 'd/t.txt' }), expect.objectContaining({ kind: 'add', path: 'd/new.txt' }), expect.objectContaining({ kind: 'add', path: 'gone', newType: 'file' }), { kind: 'add', path: 'newlink', newType: 'symlink', newMode: '120000', newOid: expect.stringMatching(/^[0-9a-f]{40}$/), - newLinkTarget: '/etc/passwd', underGit: false, ignored: false }])); + newLinkTarget: '/etc/passwd', linkTargetTraversesLink: true, underGit: false, ignored: false }])); expect(manifest.changes.map(change => change.path)).not.toContain('r.txt'); }, 180_000); + it('says whether each changed link\'s new target passes through another link, as the kernel resolves it', async () => { + const data = fixture({ hostile: source => { + mkdirSync(join(source, 'd', 'e'), { recursive: true }); writeFileSync(join(source, 'd', 'e', 'f'), 'f\n'); + // A link the item does not change, whose target text no manifest could carry: never reported, so never checked. + symlinkSync('d\u0001', join(source, 'w')); + writeFileSync(join(source, 't'), 't\n'); symlinkSync('d/e', join(source, 'via')); symlinkSync('t', join(source, 'old')); + } }); + asAgent(data.filesystems, ['ln -sfn d/e/f old', 'ln -s via/f through', 'ln -s old chain', 'ln -s via/../t dotdot', + 'ln -s missing/x dangling', 'ln -s ../outside out', 'ln -s .git/config meta', 'ln -s d plain-dir', 'ln -s w/x via-odd', + // Each name fits the manifest, but the target joined to the link's directory is longer than any name it carries. + `mkdir -p ${'p'.repeat(250)}/${'q'.repeat(250)}/${'r'.repeat(250)} && ln -s ${'s'.repeat(250)}/${'t'.repeat(250)} ${'p'.repeat(250)}/${'q'.repeat(250)}/${'r'.repeat(250)}/deep`].join(' && ')); + const manifest = await inspectTaskChanges(data.filesystems, { base: data.clone.head, imageId, linkSnapshot: { links: [], targets: {} } }); + const traverses = Object.fromEntries(manifest.changes.filter(change => change.newType === 'symlink') + .map(change => [change.path, change.linkTargetTraversesLink])); + // A ".." after a link climbs from where that link leads, so dotdot goes through via too. + expect(traverses).toEqual({ old: false, through: true, chain: true, dotdot: true, dangling: false, out: true, meta: true, + 'plain-dir': false, 'via-odd': true, [`${'p'.repeat(250)}/${'q'.repeat(250)}/${'r'.repeat(250)}/deep`]: false }); + expect(manifest.changes.filter(change => change.newType !== 'symlink').every(change => !('linkTargetTraversesLink' in change))).toBe(true); + }, 180_000); + it('resolves a declared link one part at a time: a link before ".." stops it, as the kernel would', async () => { const data = fixture({ hostile: source => { mkdirSync(join(source, 'd', 'e'), { recursive: true }); diff --git a/test/agent-storage-async.test.ts b/test/agent-storage-async.test.ts index 3076ad9e..380accfb 100644 --- a/test/agent-storage-async.test.ts +++ b/test/agent-storage-async.test.ts @@ -32,7 +32,7 @@ vi.mock('../agents/container/image.ts', async importOriginal => ({ ...await importOriginal(), assertBuiltAgentImage: () => {}, })); const { adoptRecoveredTaskStorage, exportTaskDiff, hasLiveTaskStorage, prepareTaskFilesystemsAsync, - removeTaskFilesystems } = await import('../agents/container/storage.ts'); + removeTaskFilesystems, removeTaskFilesystemsAsync } = await import('../agents/container/storage.ts'); const { createTaskClone } = await import('../git/clone.ts'); const { commitTaskChanges, inspectTaskChanges, manifestDigest, MAXIMUM_BUNDLE_BYTES, MAXIMUM_DECLARED_LINKS, MAXIMUM_NAME_BYTES, TaskCommitRefused } = await import('../agents/container/changes.ts'); @@ -588,3 +588,27 @@ describe('runner commit', () => { removeTaskFilesystems(filesystems); }, 60_000); }); + +describe('asynchronous task storage removal', () => { + it('removes allocated storage and a recovery handle without blocking, each Docker call in its own process group', async () => { + const groups: ProcessGroup[] = []; + const filesystems = await prepareTaskFilesystemsAsync(clone(), LIMITS, IMAGE, owner(), { onProcessGroup: group => groups.push(group) }); + calls.made = []; + let ticks = 0; + const ticker = setInterval(() => { ticks += 1; }, 5); + try { await removeTaskFilesystemsAsync(filesystems); } finally { clearInterval(ticker); } + expect(stored()).toEqual([]); + expect(calls.made.length).toBeGreaterThan(0); + // The event loop ran while Docker did: timers fired in between. + expect(ticks).toBeGreaterThan(0); + // Removed for good: the value is no longer storage D knows. + await expect(removeTaskFilesystemsAsync(filesystems)).rejects.toThrow('trusted allocator'); + const owned = owner(), labels = (kind: string) => ({ 'io.codeboost.runner': owned.runnerOwner, + 'io.codeboost.attempt': owned.attemptId, 'io.codeboost.allocation': owned.allocationId, 'io.codeboost.task-storage': kind }); + const work = `codeboost-work-${randomUUID()}`; + writeFileSync(join(state, `${work}.json`), JSON.stringify({ kind: 'volume', labels: labels('work') })); + const handle = await adoptRecoveredTaskStorage(owned, { workVolume: work }); + await removeTaskFilesystemsAsync(handle); + expect(stored()).toEqual([]); + }); +}); diff --git a/test/review.test.ts b/test/review.test.ts index 631b9896..cc923d01 100644 --- a/test/review.test.ts +++ b/test/review.test.ts @@ -168,3 +168,10 @@ it('preserves removed-side snippets and marks their references outdated after HE fixtureGit(config.repository,'commit','--allow-empty','-m','new revision'); view=service.load();expect(view.notes[0]!.outdated).toBe(true);expect(view.notes[0]!.reference).toEqual(ref); }); +it('keeps observing the user\'s HEAD for a task the runner has not committed to, owned ledger entries or not (#87)', () => { + // The demo's ledger already owns its commits (a reviewed branch in the user's repository), but no attempt made them. + const {service,config}=fixture(); + expect(service.reviewRepository()).toEqual({path:config.repository,runnerOwned:false}); + fixtureGit(config.repository,'commit','--allow-empty','-m','User work'); + expect(service.load().snapshot.head).toBe(fixtureGit(config.repository,'rev-parse','HEAD')); +}); diff --git a/test/run-audit.test.ts b/test/run-audit.test.ts index 813b70c0..4a4bbbf7 100644 --- a/test/run-audit.test.ts +++ b/test/run-audit.test.ts @@ -9,7 +9,7 @@ const item: PlanItem = { id: 'P1', title: 'T', intent: 'I', depends_on: [], acce ] }; const manifest = (changes: ManifestChange[], over: Partial = {}): ChangeManifest => ({ changes, agentCommits: [], metadataChanged: false, linkTargetChanges: [], nestedGitlinkContent: [], ...over }); -const file = (path: string, over: Partial = {}): ManifestChange => ({ path, kind: 'modify', oldType: 'file', newType: 'file', underGit: false, ...over }); +const file = (path: string, over: Partial = {}): ManifestChange => ({ path, kind: 'modify', oldType: 'file', newType: 'file', underGit: false, ignored: false, ...over }); const exact = (p: string) => p, folded = (p: string) => p.toLowerCase(); describe('post-run audit', () => { @@ -34,11 +34,32 @@ describe('post-run audit', () => { for (const field of ['agentCommits', 'linkTargetChanges', 'nestedGitlinkContent'] as const) expect(auditRun(item, manifest([file('src/retry.ts')], { [field]: undefined as unknown as string[] }), exact)).toEqual({ kind: 'violation', violations: [`The change report has no ${field} list.`] }); }); + it('treats a file base ignores as a safety violation, never something to commit (#87)', () => { + expect(auditRun(item, manifest([file('src/retry.ts'), file('src/.env', { kind: 'add', oldType: undefined, ignored: true })]), exact)) + .toEqual({ kind: 'violation', violations: ['The agent added "src/.env", which the repository ignores.'] }); + // Declared or not: a declared path that base ignores is still never committed. + expect(auditRun(item, manifest([file('src/retry.ts', { ignored: true })]), exact)).toMatchObject({ kind: 'violation' }); + expect(auditRun(item, manifest([file('src/retry.ts', { ignored: undefined as unknown as boolean })]), exact)) + .toEqual({ kind: 'violation', violations: ['The change at "src/retry.ts" does not say whether base ignores it.'] }); + }); + it('lets a declared link be retargeted by its own edit, and refuses every write through it', () => { + const edit = file('link', { oldType: 'symlink', newType: 'symlink', newLinkTarget: 'src/retry.ts', linkTargetTraversesLink: false }); + const retargeted = { link: 'link', target: 'old.ts', path: 'link', change: 'retargeted' as const }; + expect(auditRun(item, manifest([edit], { linkTargetChanges: [retargeted] }), exact)).toMatchObject({ kind: 'commit', inScope: ['link'] }); + // Retargeted with no edit of the link in the manifest: something on the way to its target changed. + expect(auditRun(item, manifest([file('src/retry.ts')], { linkTargetChanges: [retargeted] }), exact)).toMatchObject({ kind: 'violation' }); + // The edit does not excuse a write through the link to its old target. + expect(auditRun(item, manifest([edit], { linkTargetChanges: [retargeted, { ...retargeted, path: 'old.ts', change: 'content' }] }), exact)) + .toEqual({ kind: 'violation', violations: ['A declared symlink target changed: "link -> old.ts (content)".'] }); + for (const entry of ['old.ts', { ...retargeted, change: 'moved' }, { ...retargeted, path: undefined }]) + expect(auditRun(item, manifest([edit], { linkTargetChanges: [entry as never] }), exact)) + .toEqual({ kind: 'violation', violations: ['The change report has a malformed link target change.'] }); + }); it('fails closed on a partial record: every field the audit reads must be present and well-formed', () => { const cases: [string, ChangeManifest, RegExp][] = [ ['no metadata flag', manifest([file('src/retry.ts')], { metadataChanged: undefined as unknown as boolean }), /whether Git metadata changed/], ['no underGit', manifest([{ path: 'src/retry.ts', kind: 'modify', oldType: 'file', newType: 'file' } as ManifestChange]), /under \.git/], - ['add without a new type', manifest([{ path: 'src/new.ts', kind: 'add', underGit: false } as ManifestChange]), /invalid new entry type/], + ['add without a new type', manifest([{ path: 'src/new.ts', kind: 'add', underGit: false, ignored: false } as ManifestChange]), /invalid new entry type/], ['modify without an old type', manifest([file('src/retry.ts', { oldType: undefined })]), /invalid old entry type/], ['unknown kind', manifest([file('src/retry.ts', { kind: 'chmod' as ManifestChange['kind'] })]), /unknown kind/], ['rename without an old path', manifest([file('docs/New.md', { kind: 'rename' })]), /invalid old path/], @@ -176,7 +197,7 @@ describe('post-run audit', () => { const cases: [string, ChangeManifest][] = [ ['metadata', manifest([file('src/retry.ts')], { metadataChanged: true })], ['under .git', manifest([file('.git/hooks/pre-commit', { underGit: true })])], - ['link target changed', manifest([], { linkTargetChanges: ['target/file'] })], + ['link target changed', manifest([], { linkTargetChanges: [{ link: 'link', target: 'target', path: 'target/file', change: 'content' }] })], ['gitlink content', manifest([], { nestedGitlinkContent: ['vendor/lib/x'] })], ['new gitlink', manifest([file('vendor/lib', { kind: 'add', oldType: undefined, newType: 'gitlink' })])], ['new symlink', manifest([file('src/retry.ts', { newType: 'symlink', newLinkTarget: 'other.ts', linkTargetTraversesLink: false })])], diff --git a/test/runner-execution.test.ts b/test/runner-execution.test.ts index 21e3f30b..7d331e78 100644 --- a/test/runner-execution.test.ts +++ b/test/runner-execution.test.ts @@ -22,8 +22,8 @@ const plan: Plan = { schema_version: 1, issue: 1, revision: 1, summary: 'Two ite const context: PlanContext = { identity, issue: 1, baseEntries: [{ path: 'a.ts', kind: 'file' }, { path: 'b.ts', kind: 'file' }], pathKey: p => p, allowedCommands: [['npm', 'test']] }; const dirs: string[] = [], cleanups: (() => Promise | void)[] = []; afterEach(async () => { for (const c of cleanups.splice(0).reverse()) await c(); for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); }); -const change = (path: string, over: Partial = {}): ManifestChange => ({ path, kind: 'modify', oldType: 'file', newType: 'file', underGit: false, ...over }); -const manifest = (changes: ManifestChange[], over: Partial = {}): ChangeManifest & { digest: string } => +const change = (path: string, over: Partial = {}): ManifestChange => ({ path, kind: 'modify', oldType: 'file', newType: 'file', underGit: false, ignored: false, ...over }); +const manifest = (changes: ManifestChange[], over: Partial = {}): ChangeManifest & { digest: string } => ({ changes, agentCommits: [], metadataChanged: false, linkTargetChanges: [], nestedGitlinkContent: [], digest: `digest-${changes.length}`, ...over }); function setup(options: { manifests?: Record; exit?: Record>; @@ -34,19 +34,19 @@ function setup(options: { manifests?: Record; digest: string; message: string }[] = []; + const log: string[] = [], commits: { item: string; baseHead: string; linkSnapshot: unknown; trailers: Record; digest: string; message: string }[] = []; let next = 100; const itemOf = (ws: WorkspaceRef) => (ws.storage as { item: string }).item; const workspace: TaskWorkspace = { async materialize(attempt, head) { log.push(`materialize ${attempt.item} @${head.slice(-3)}`); if (options.materializeError) throw options.materializeError; return { clone: { id: `c-${attempt.id}`, taskId: 'task', directory: '/tmp/x', head }, storage: { item: attempt.item, attemptId: attempt.id } }; }, - async snapshotDeclaredLinks(ws, paths) { log.push(`snapshot ${itemOf(ws)} [${paths.join(',')}]`); if (options.snapshotError) throw options.snapshotError; return { item: itemOf(ws) }; }, + async snapshotDeclaredLinks(ws, paths) { log.push(`snapshot ${itemOf(ws)} [${paths.join(',')}]`); if (options.snapshotError) throw options.snapshotError; return { item: itemOf(ws) } as never; }, async inspectChanges(ws, input, signal) { log.push(`inspect ${itemOf(ws)} @${input.baseHead.slice(-3)}`); await options.inspect?.(itemOf(ws), signal); return options.manifests?.[itemOf(ws)] ?? manifest([change(itemOf(ws) === 'P1' ? 'a.ts' : 'b.ts')]); }, async commit(ws, input) { await options.commit?.(itemOf(ws)); - const head = options.commitHead ?? oid(next++); commits.push({ item: itemOf(ws), baseHead: input.baseHead, paths: input.paths, trailers: { ...input.trailers }, digest: input.digest, message: input.message }); + const head = options.commitHead ?? oid(next++); commits.push({ item: itemOf(ws), baseHead: input.baseHead, linkSnapshot: input.linkSnapshot, trailers: { ...input.trailers }, digest: input.digest, message: input.message }); log.push(`commit ${itemOf(ws)} -> ${head.slice(-3)}`); return head; }, async release(ws) { @@ -74,9 +74,10 @@ describe('item execution', () => { it('runs items in order, commits each with trailers, and records owned ledger entries', async () => { const { store, executor, log, commits, prompts, argv, owners } = setup(); expect(await executor.runTask(identity)).toEqual({ kind: 'executed', items: ['P1', 'P2'], unchanged: [] }); - expect(commits.map(c => [c.item, c.baseHead.slice(-3), c.trailers, c.paths, c.message])).toEqual([ - ['P1', '002', { 'Plan-Item': 'P1', 'Plan-Revision': 'r1' }, ['a.ts'], 'P1: First'], - ['P2', '064', { 'Plan-Item': 'P2', 'Plan-Revision': 'r1' }, ['b.ts'], 'P2: Second']]); + // Each commit gets the link snapshot its own item took before launch. + expect(commits.map(c => [c.item, c.baseHead.slice(-3), c.trailers, c.linkSnapshot, c.message])).toEqual([ + ['P1', '002', { 'Plan-Item': 'P1', 'Plan-Revision': 'r1' }, { item: 'P1' }, 'P1: First'], + ['P2', '064', { 'Plan-Item': 'P2', 'Plan-Revision': 'r1' }, { item: 'P2' }, 'P2: Second']]); expect(store.getLedger(identity)).toEqual(expect.arrayContaining([ { sha: oid(100), owner: 'P1', origin: 'owned', sourceSha: null }, { sha: oid(101), owner: 'P2', origin: 'owned', sourceSha: null }])); expect(store.getSnapshot(identity).head).toBe(oid(101)); @@ -97,7 +98,8 @@ describe('item execution', () => { const { store, executor, commits, log } = setup({ manifests: { P1: manifest([change('a.ts'), change('extra.ts', { kind: 'add', oldType: undefined })]) } }); const outcome = await executor.runTask(identity); expect(outcome).toMatchObject({ kind: 'needs amendment', item: 'P1', outOfScope: ['extra.ts'] }); - expect(commits[0]!.paths).toEqual(['a.ts', 'extra.ts']); + // The commit is the whole audited manifest, the out-of-scope file included. + expect(commits[0]!.digest).toBe('digest-2'); expect(store.getCheckpoint(identity, (outcome as { checkpointId: string }).checkpointId)).toMatchObject({ item: 'P1', completedItems: ['P1'], outOfScopePaths: ['extra.ts'] }); expect(store.getTask(identity).status).toBe('needs amendment'); expect(log.some(line => line.includes('P2'))).toBe(false); @@ -234,10 +236,10 @@ describe('item execution', () => { expect(await h.executor.runTask(identity)).toMatchObject({ kind: 'needs amendment', item: 'P1' }); expect(store.getTask(identity).status).toBe('needs amendment'); }); - it('stages both paths of a rename in the runner commit', async () => { - const { executor, commits } = setup({ manifests: { P1: manifest([change('a.ts', { kind: 'rename', oldPath: 'old.ts' })]) } }); + it('commits a rename as the manifest audited it', async () => { + const { executor, commits } = setup({ manifests: { P1: manifest([change('a.ts', { kind: 'rename', oldPath: 'old.ts' })], { digest: 'renamed' }) } }); await executor.runTask(identity); - expect(commits[0]!.paths).toEqual(['a.ts', 'old.ts']); + expect(commits[0]!.digest).toBe('renamed'); }); it('treats an inspection aborted by a stop as that stop, not a finding', async () => { let runner!: RunnerCoordinator, store!: Store; @@ -269,7 +271,7 @@ describe('item execution', () => { expect(h.commits).toEqual([]); }); it('sends a malformed change report to needs human', async () => { - const { store, executor, commits } = setup({ manifests: { P1: manifest([change('a.ts')], { linkTargetChanges: undefined as unknown as string[] }) } }); + const { store, executor, commits } = setup({ manifests: { P1: manifest([change('a.ts')], { linkTargetChanges: undefined as unknown as [] }) } }); expect(await executor.runTask(identity)).toMatchObject({ kind: 'needs human', item: 'P1' }); expect(store.getTask(identity).status).toBe('needs human'); expect(commits).toEqual([]); diff --git a/test/runner-lifecycle-store.test.ts b/test/runner-lifecycle-store.test.ts index 6f9b8829..120c2f7f 100644 --- a/test/runner-lifecycle-store.test.ts +++ b/test/runner-lifecycle-store.test.ts @@ -503,3 +503,31 @@ describe('user actions', () => { expect(store.feedbackEvents(identity).map(event => event.kind)).toEqual(['task-closed']); }); }); + +describe('runner commits and preparation groups (#87)', () => { + it('knows a task has a runner commit only from a completed writable attempt whose result made one', () => { + const { store } = queued(); + expect(store.hasRunnerCommit(identity)).toBe(false); + const unchanged = admit(store); store.markRunning(identity, unchanged.id); + settle(store, unchanged.id, { result: { head: oid(2), unchanged: true, inScope: [], outOfScope: [] } }); + expect(store.hasRunnerCommit(identity)).toBe(false); + const failed = admit(store); store.markRunning(identity, failed.id); + settle(store, failed.id, { exitCode: 1, valid: false }); + expect(store.hasRunnerCommit(identity)).toBe(false); + const committed = admit(store); store.markRunning(identity, committed.id); + settle(store, committed.id, { result: { head: oid(3), unchanged: false, inScope: ['a'], outOfScope: [] }, + history: { base: oid(1), head: oid(3), entries: [{ sha: oid(3), owner: 'P1', origin: 'owned', sourceSha: null }] } }); + expect(store.hasRunnerCommit(identity)).toBe(true); + }); + + it('keeps each recorded preparation group paired with its own start time', () => { + const { store } = queued(); const attempt = admit(store); + store.markPreparationStarting(identity, attempt.id, 1_000); + store.recordPreparationGroup(identity, attempt.id, 4242, 41_000); + expect(store.interruptedAttempts()[0]).toMatchObject({ preparationPgid: 4242, preparationStartedAt: 41_000 }); + // Without a time, the marker's own stays. + store.recordPreparationGroup(identity, attempt.id, 4343); + expect(store.interruptedAttempts()[0]).toMatchObject({ preparationPgid: 4343, preparationStartedAt: 41_000 }); + expect(() => store.recordPreparationGroup(identity, attempt.id, 4444, 1.5)).toThrow('start time'); + }); +}); diff --git a/test/runner-repository.test.ts b/test/runner-repository.test.ts new file mode 100644 index 00000000..e86ef2b3 --- /dev/null +++ b/test/runner-repository.test.ts @@ -0,0 +1,89 @@ +import { randomUUID } from 'node:crypto'; +import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { fixtureGit as git } from './fixtures/git.ts'; +import { attemptRef, dropAttemptRef, ensureCommit, fetchTaskCommit, openRunnerRepository } from '../runner/runner-repository.ts'; + +const OWNER = '0123456789abcdef0123456789abcdef'; +const roots: string[] = []; +afterEach(() => { for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); }); + +function setup() { + const root = mkdtempSync(join(tmpdir(), 'runner-repo-')); roots.push(root); + const source = join(root, 'source'), runnerRoot = join(root, 'runner'); + git(root, 'init', '-q', source); git(source, 'config', 'user.name', 'T'); git(source, 'config', 'user.email', 't@e'); + writeFileSync(join(source, 'a.txt'), 'a\n'); git(source, 'add', '.'); git(source, 'commit', '-qm', 'base'); + const base = git(source, 'rev-parse', 'HEAD'); + return { root, source, runnerRoot, base }; +} +/** What `commitTaskChanges` returns: a bundle of one commit on `base`, its one ref `refs/heads/codeboost`. */ +function bundleOf(s: ReturnType, commits = 1, ref = 'refs/heads/codeboost') { + const work = join(s.root, `work-${randomUUID()}`); + git(s.root, '-c', 'protocol.file.allow=always', 'clone', '-q', s.source, work); + git(work, 'config', 'user.name', 'R'); git(work, 'config', 'user.email', 'r@e'); + for (let i = 0; i < commits; i += 1) { writeFileSync(join(work, `n${i}.txt`), `${i}\n`); git(work, 'add', '.'); git(work, 'commit', '-qm', `c${i}`); } + git(work, 'update-ref', ref, 'HEAD'); + const file = join(work, 'out.bundle'); + git(work, 'bundle', 'create', '-q', file, ref, `^${s.base}`); + return { bundle: readFileSync(file), head: git(work, 'rev-parse', 'HEAD') }; +} +const open = (s: ReturnType) => openRunnerRepository({ runnerRoot: s.runnerRoot, runnerOwner: OWNER, repositoryId: 'repo-1', source: s.source }); + +describe('runner-owned repository', () => { + it('is a bare repository of the runner\'s own, created once, and never writes the user\'s repository', async () => { + const s = setup(), refsBefore = git(s.source, 'for-each-ref'); + const repo = await open(s); + expect(repo.path.startsWith(join(s.runnerRoot, OWNER, 'repositories'))).toBe(true); + expect(git(repo.path, 'rev-parse', '--is-bare-repository')).toBe('true'); + expect((await open(s)).path).toBe(repo.path); + await ensureCommit(repo, s.base); + expect(git(repo.path, 'cat-file', '-t', s.base)).toBe('commit'); + // Only the commit came over: no refs, tags or FETCH_HEAD. + expect(git(repo.path, 'for-each-ref')).toBe(''); + expect(git(s.source, 'for-each-ref')).toBe(refsBefore); + await expect(ensureCommit(repo, 'f'.repeat(40))).rejects.toThrow(); + await expect(ensureCommit(repo, 'HEAD')).rejects.toThrow('full commit ID'); + }); + + it('refuses a repositories directory others can write', async () => { + const s = setup(); + await open(s); + // Each level, from the runner root down. + for (const path of [s.runnerRoot, join(s.runnerRoot, OWNER), join(s.runnerRoot, OWNER, 'repositories')]) { + chmodSync(path, 0o777); + await expect(open(s)).rejects.toThrow(`${path} must be a directory owned by you`); + chmodSync(path, 0o700); + } + expect((await open(s)).path).toContain(join(s.runnerRoot, OWNER, 'repositories')); + }); + + it('takes in exactly the runner commit, under the attempt\'s ref, and drops it when asked', async () => { + const s = setup(), repo = await open(s), attemptId = randomUUID(); + await ensureCommit(repo, s.base); + const { bundle, head } = bundleOf(s); + await fetchTaskCommit(repo, { bundle, base: s.base, head, attemptId }); + expect(git(repo.path, 'rev-parse', attemptRef(attemptId))).toBe(head); + // No bundle file is left beside the repository. + expect(git(repo.path, 'for-each-ref', '--format=%(refname)')).toBe(attemptRef(attemptId)); + await dropAttemptRef(repo, attemptId); + expect(git(repo.path, 'for-each-ref')).toBe(''); + }); + + it('refuses a bundle that is not the runner commit: another head, more than one commit, another ref', async () => { + const s = setup(), repo = await open(s); + await ensureCommit(repo, s.base); + const one = bundleOf(s), two = bundleOf(s, 2), named = bundleOf(s, 1, 'refs/heads/other'); + await expect(fetchTaskCommit(repo, { bundle: one.bundle, base: s.base, head: two.head, attemptId: randomUUID() })) + .rejects.toThrow('does not hold exactly the runner commit'); + await expect(fetchTaskCommit(repo, { bundle: two.bundle, base: s.base, head: two.head, attemptId: randomUUID() })) + .rejects.toThrow('not a single commit on top of its base'); + expect(git(repo.path, 'for-each-ref')).toBe(''); + await expect(fetchTaskCommit(repo, { bundle: named.bundle, base: s.base, head: named.head, attemptId: randomUUID() })) + .rejects.toThrow('does not hold exactly the runner commit'); + await expect(fetchTaskCommit(repo, { bundle: Buffer.from('not a bundle'), base: s.base, head: one.head, attemptId: randomUUID() })) + .rejects.toThrow('bundle'); + await expect(fetchTaskCommit(repo, { ...one, base: s.base, attemptId: 'not-a-uuid' })).rejects.toThrow('UUID'); + }); +}); diff --git a/test/runner-workspace-preparation.test.ts b/test/runner-workspace-preparation.test.ts new file mode 100644 index 00000000..12186578 --- /dev/null +++ b/test/runner-workspace-preparation.test.ts @@ -0,0 +1,39 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, expect, it, vi } from 'vitest'; +import type { Plan, PlanContext } from '../core/plan.ts'; + +// Every spawn throws synchronously, as Node does for ENOMEM: no child, so no process group is ever recorded. +vi.mock('node:child_process', async importOriginal => { + const actual = await importOriginal(); + return { ...actual, spawn: (() => { throw Object.assign(new Error('spawn ENOMEM'), { code: 'ENOMEM' }); }) as typeof actual.spawn }; +}); +const { Store } = await import('../runner/store.ts'); +const { createTaskWorkspace } = await import('../runner/workspace.ts'); + +const identity = { repositoryId: 'repo', taskId: 'task', planId: 'plan' }; +const context: PlanContext = { identity, issue: 1, baseEntries: [{ path: 'a', kind: 'file' }], pathKey: p => p, allowedCommands: [] }; +const plan: Plan = { schema_version: 1, revision: 1, issue: 1, summary: 'S', questions: [], items: [{ id: 'P1', title: 'T', intent: 'I', + files: [{ path: 'a', kind: 'edit', renamed_from: null, change: 'x' }], acceptance: [{ type: 'check', text: 'ok' }], depends_on: [] }] }; +const oid = (n: number) => n.toString(16).padStart(40, '0'); +const dirs: string[] = []; +afterEach(() => { for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); }); + +it('clears the preparation marker when no subprocess ever started, so the next startup is not blocked', async () => { + const dir = mkdtempSync(join(tmpdir(), 'workspace-prep-')); dirs.push(dir); + const store = new Store(join(dir, 'state.sqlite')); + try { + store.createPlan(JSON.stringify(plan), 'json', context, oid(1), oid(2)); + store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); + const attempt = store.admitAttempt(identity, { expectedStateVersion: store.getTask(identity).stateVersion, kind: 'execute', item: 'P1', + expectedContext: store.currentContext(identity), deadline: Date.now() + 60_000 }); + const workspace = createTaskWorkspace({ store, runnerRoot: join(dir, 'runner'), runnerOwner: '0123456789abcdef0123456789abcdef', + repository: { path: join(dir, 'repo.git'), source: join(dir, 'source') }, imageId: `sha256:${'a'.repeat(64)}`, + limits: { workBytes: 1, workInodes: 1, metadataBytes: 1, metadataInodes: 1 }, committer: { name: 'c', email: 'c@e' } }); + await expect(workspace.materialize(attempt, oid(2), new AbortController().signal)).rejects.toThrow(); + // Neither "starting" nor a group: startup recovery has nothing to ask a person about. + expect(store.unownedPreparations()).toEqual([]); + expect(store.interruptedAttempts()[0]).toMatchObject({ preparationStartedAt: null, preparationPgid: null }); + } finally { store.close(); } +}); diff --git a/test/runner-workspace.test.ts b/test/runner-workspace.test.ts new file mode 100644 index 00000000..e2d9e363 --- /dev/null +++ b/test/runner-workspace.test.ts @@ -0,0 +1,122 @@ +import { execFile, execFileSync } from 'node:child_process'; +import { existsSync, mkdtempSync, readdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { promisify } from 'node:util'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { buildAgentImage } from '../agents/container/image.ts'; +import type { TaskFilesystems } from '../agents/container/storage.ts'; +import type { Plan, PlanContext } from '../core/plan.ts'; +import { RunnerCoordinator } from '../runner/coordinator.ts'; +import { ItemExecutor, SafetyFindings, executionDeps, type ExecutionSources } from '../runner/execution.ts'; +import { attemptRef, openRunnerRepository } from '../runner/runner-repository.ts'; +import { ReviewService } from '../runner/review.ts'; +import { Store } from '../runner/store.ts'; +import { createTaskWorkspace } from '../runner/workspace.ts'; +import { fixtureGit as git } from './fixtures/git.ts'; + +// F2b against the real lane D (#87): each item runs in fresh task storage cloned from the runner-owned repository, an +// agent edits the work volume, and the runner's audited commit is taken into that repository for the next item. +const RUNNER_OWNER = '0123456789abcdef0123456789abcdef'; +const identity = { repositoryId: 'repo', taskId: 'task', planId: 'plan' }; +const plan: Plan = { schema_version: 1, issue: 1, revision: 1, summary: 'Two items', questions: [], items: [ + { id: 'P1', title: 'First', intent: 'Change a', files: [{ path: 'a.ts', kind: 'edit', renamed_from: null, change: 'x' }], acceptance: [{ type: 'check', text: 'a' }], depends_on: [] }, + { id: 'P2', title: 'Second', intent: 'Change b', files: [{ path: 'b.ts', kind: 'edit', renamed_from: null, change: 'y' }], acceptance: [{ type: 'check', text: 'b' }], depends_on: ['P1'] }, +] }; +const LIMITS = { workBytes: 16 * 1024 * 1024, workInodes: 512, metadataBytes: 16 * 1024 * 1024, metadataInodes: 512 }; +let imageId = ''; +const roots: string[] = [], stores: Store[] = []; +beforeAll(() => { imageId = buildAgentImage(); }, 10 * 60_000); +afterAll(() => { + for (const store of stores) store.close(); + for (const root of roots) rmSync(root, { recursive: true, force: true }); +}); +const docker = (...args: string[]) => execFileSync('docker', args, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }).trim(); + +async function setup(agent: Record) { + const root = mkdtempSync(join(tmpdir(), 'runner-workspace-')); roots.push(root); + const source = join(root, 'source'), runnerRoot = join(root, 'runner'); + git(root, 'init', '-q', source); git(source, 'config', 'user.name', 'T'); git(source, 'config', 'user.email', 't@e'); + writeFileSync(join(source, '.gitignore'), '*.log\n'); writeFileSync(join(source, 'a.ts'), 'a\n'); git(source, 'add', '.'); git(source, 'commit', '-qm', 'base'); + const base = git(source, 'rev-parse', 'HEAD'); + writeFileSync(join(source, 'b.ts'), 'b\n'); git(source, 'add', '.'); git(source, 'commit', '-qm', 'head'); + const head = git(source, 'rev-parse', 'HEAD'), sourceRefs = git(source, 'for-each-ref'); + const context: PlanContext = { identity, issue: 1, baseEntries: [{ path: 'a.ts', kind: 'file' }, { path: 'b.ts', kind: 'file' }], pathKey: p => p, allowedCommands: [] }; + const store = new Store(join(root, 'state.sqlite')); stores.push(store); + store.createPlan(JSON.stringify(plan), 'json', context, base, head); + store.transitionTask(identity, store.getTask(identity).stateVersion, 'queued'); + const repository = await openRunnerRepository({ runnerRoot, runnerOwner: RUNNER_OWNER, repositoryId: identity.repositoryId, source }); + const workspace = createTaskWorkspace({ store, runnerRoot, runnerOwner: RUNNER_OWNER, repository, imageId, limits: LIMITS, + committer: { name: 'codeboost', email: 'runner@codeboost.invalid' }, now: () => 1_700_000_000_000 }); + const sources: ExecutionSources = { planContext: () => context, issue: () => ({ number: 1, title: 'Issue', body: 'Fix', comments: [] }), + lessons: () => [], vendor: () => 'claude' }; + const findings = new SafetyFindings(); + // The agent: a container that edits the work volume, mounted as an agent container mounts it (metadata read-only). + const deps = executionDeps(store, workspace, (input, _prompt, ws) => { + const { workVolume, metadataVolume } = (ws.storage as { filesystems: TaskFilesystems }).filesystems; + const item = store.getAttempt(identity, input.attemptId).item!; + const settled = promisify(execFile)('docker', ['run', '--rm', '--network=none', '--user', '10001:10001', '--tmpfs', '/tmp', + '--mount', `type=volume,source=${workVolume},target=/work`, '--mount', `type=volume,source=${metadataVolume},target=/work/.git,readonly`, + '--entrypoint', 'bash', imageId, '-c', `set -e; cd /work; ${agent[item] ?? 'true'}`]) + .then(() => ({ attemptId: input.attemptId, context: input.context, exitCode: 0, signal: null, stdout: '', stderr: '' })); + return { attemptId: input.attemptId, settled, cancel: () => undefined }; + }, sources, RUNNER_OWNER, findings); + const runner = new RunnerCoordinator(store, deps); + return { root, source, runnerRoot, repository, store, base, head, sourceRefs, runner, + executor: new ItemExecutor(store, runner, sources, findings) }; +} +// Task storage volumes still labelled with one of this task's attempts. +const leftovers = (store: Store) => store.getAttempts(identity).flatMap(attempt => + docker('volume', 'ls', '-q', '--filter', `label=io.codeboost.attempt=${attempt.id}`).split('\n').filter(Boolean)); + +describe('real task workspace (#87)', () => { + it('runs each item in fresh storage from the runner-owned repository and chains the runner commits there', async () => { + const s = await setup({ P1: 'printf "a2\\n" > a.ts', P2: 'test "$(cat a.ts)" = a2 && printf "b2\\n" > b.ts' }); + expect(await s.executor.runTask(identity)).toEqual({ kind: 'executed', items: ['P1', 'P2'], unchanged: [] }); + const [first, second] = s.store.getAttempts(identity); + const one = (first!.result as { head: string }).head, two = (second!.result as { head: string }).head; + // Each commit is in the runner-owned repository under its attempt, P2's on top of P1's, P1's on the recorded head. + expect(git(s.repository.path, 'rev-parse', attemptRef(first!.id), attemptRef(second!.id))).toBe(`${one}\n${two}`); + expect(git(s.repository.path, 'rev-list', '--parents', '-n', '1', two)).toBe(`${two} ${one}`); + expect(git(s.repository.path, 'rev-list', '--parents', '-n', '1', one)).toBe(`${one} ${s.head}`); + expect(git(s.repository.path, 'show', `${two}:a.ts`)).toBe('a2'); + expect(git(s.repository.path, 'show', `${two}:b.ts`)).toBe('b2'); + expect(git(s.repository.path, 'log', '-1', '--format=%an <%ae> %at%n%B', one)).toBe('codeboost 1700000000\nP1: First\n\nPlan-Item: P1\nPlan-Revision: r1'); + expect(s.store.getSnapshot(identity).head).toBe(two); + // The user's repository is untouched: no refs, and no runner commit. + expect(git(s.source, 'for-each-ref')).toBe(s.sourceRefs); + expect(() => git(s.source, 'cat-file', '-e', one)).toThrow(); + // Storage and the staging clones are gone once each attempt settled. + expect(leftovers(s.store)).toEqual([]); + expect(readdirSync(join(s.runnerRoot, RUNNER_OWNER, 'attempts'))).toEqual([]); + // The review screen reads the runner commits from the runner-owned repository, each owned by its item, and keeps the + // recorded head: the user's HEAD (still the old commit) would roll the task back. + const review = new ReviewService({ database: join(s.root, 'state.sqlite'), repository: s.source, runnerRepository: s.repository.path, + identity, pathIdentity: { caseSensitive: true, unicodeNormalization: 'none' } }); + try { + const view = review.load(); + expect(view.snapshot.head).toBe(two); + // a.ts's lines are P1's own runner commit's. + expect(view.segments.filter(segment => segment.path === 'a.ts').map(segment => [segment.row, segment.operation])) + .toEqual([['P1', '-'], ['P1', '+']]); + expect(review.reviewRepository()).toEqual({ path: s.repository.path, runnerOwned: true }); + } finally { review.close(); } + // Without the runner-owned repository it fails rather than read the user's. + const unconfigured = new ReviewService({ database: join(s.root, 'state.sqlite'), repository: s.source, identity, + pathIdentity: { caseSensitive: true, unicodeNormalization: 'none' } }); + try { expect(() => unconfigured.load()).toThrow('needs the runner-owned repository'); } finally { unconfigured.close(); } + }, 600_000); + + it('sends a file the repository ignores to needs human, and keeps no commit of it', async () => { + const s = await setup({ P1: 'printf "a2\\n" > a.ts && printf "secret\\n" > debug.log' }); + const outcome = await s.executor.runTask(identity); + expect(outcome).toMatchObject({ kind: 'needs human', item: 'P1' }); + expect((outcome as { reason: string }).reason).toContain('"debug.log", which the repository ignores'); + expect(s.store.getTask(identity).status).toBe('needs human'); + // Nothing was committed: no ref in the runner-owned repository, and the snapshot is still the recorded head. + expect(git(s.repository.path, 'for-each-ref')).toBe(''); + expect(s.store.getSnapshot(identity).head).toBe(s.head); + expect(leftovers(s.store)).toEqual([]); + expect(existsSync(join(s.runnerRoot, RUNNER_OWNER, 'attempts', s.store.getAttempts(identity)[0]!.id))).toBe(false); + }, 600_000); +});