diff --git a/packages/core/src/bootstrap/attachments.test.ts b/packages/core/src/bootstrap/attachments.test.ts new file mode 100644 index 0000000..8a28318 --- /dev/null +++ b/packages/core/src/bootstrap/attachments.test.ts @@ -0,0 +1,67 @@ +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { downloadAttachments } from "./attachments.js"; + +const okResponse = (data: string) => + new Response(new Blob([data]), { status: 200 }); + +describe("downloadAttachments", () => { + afterEach(() => vi.unstubAllGlobals()); + + it("dispatches all downloads in parallel, not one at a time", async () => { + const dir = mkdtempSync(join(tmpdir(), "att-")); + // fetch never settles on its own — we control it. A sequential loop would + // only have issued the FIRST fetch while waiting; parallel issues all three. + const releases: Array<(r: Response) => void> = []; + const fetchMock = vi.fn( + (url: string) => new Promise((resolve) => releases.push(() => resolve(okResponse(`body:${url}`)))), + ); + vi.stubGlobal("fetch", fetchMock); + + try { + const pending = downloadAttachments( + [ + { filename: "a.txt", content_type: "text/plain", url: "https://s3/a" }, + { filename: "b.txt", content_type: "text/plain", url: "https://s3/b" }, + { filename: "c.txt", content_type: "text/plain", url: "https://s3/c" }, + ], + dir, + ); + + // Let mkdir + the synchronous fan-out of fetch() calls flush. + await vi.waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(3)); + + releases.forEach((release) => release()); + await pending; + + expect(readFileSync(join(dir, "a.txt"), "utf8")).toBe("body:https://s3/a"); + expect(readFileSync(join(dir, "c.txt"), "utf8")).toBe("body:https://s3/c"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("rejects (fails boot) when any single download fails", async () => { + const dir = mkdtempSync(join(tmpdir(), "att-")); + const fetchMock = vi.fn((url: string) => + Promise.resolve(url.endsWith("bad") ? new Response(null, { status: 500 }) : okResponse("ok")), + ); + vi.stubGlobal("fetch", fetchMock); + + try { + await expect( + downloadAttachments( + [ + { filename: "good.txt", content_type: "text/plain", url: "https://s3/good" }, + { filename: "boom.txt", content_type: "text/plain", url: "https://s3/bad" }, + ], + dir, + ), + ).rejects.toThrow(/download failed/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/core/src/bootstrap/attachments.ts b/packages/core/src/bootstrap/attachments.ts new file mode 100644 index 0000000..6fe64fb --- /dev/null +++ b/packages/core/src/bootstrap/attachments.ts @@ -0,0 +1,37 @@ +import { createWriteStream } from "node:fs"; +import { mkdir } from "node:fs/promises"; +import { basename, join } from "node:path"; +import { Readable } from "node:stream"; +import { pipeline } from "node:stream/promises"; +import { log } from "../log.js"; +import type { AttachmentSpec } from "../manifest/types.js"; + +/** Download one attachment to `dest`. Uses the Node global fetch (Node >= 18). */ +export async function downloadAttachment(url: string, dest: string): Promise { + const res = await fetch(url); + if (!res.ok || !res.body) { + throw new Error(`download failed (${res.status})`); + } + await pipeline(Readable.fromWeb(res.body as any), createWriteStream(dest)); +} + +/** Download every attachment into `dir`, in parallel, sanitising filenames to + * basenames so a crafted name cannot escape the attachments directory. + * + * Concurrent rather than sequential: the set is small (throngx caps a task at + * 10 attachments) so unbounded fan-out needs no pool, and wall-clock is the + * slowest single download instead of their sum. `Promise.all` still fails loud + * — the first rejection rejects the whole call, which boot turns into a + * `StepError("attachments")`. */ +export async function downloadAttachments(attachments: AttachmentSpec[], dir: string): Promise { + if (attachments.length === 0) return; + await mkdir(dir, { recursive: true }); + await Promise.all( + attachments.map((att) => { + const name = basename(att.filename); + const dest = join(dir, name); + log.info("downloading attachment", { name, dest }); + return downloadAttachment(att.url, dest); + }), + ); +} diff --git a/packages/core/src/control/server.ts b/packages/core/src/control/server.ts index 5335039..6fdb7da 100644 --- a/packages/core/src/control/server.ts +++ b/packages/core/src/control/server.ts @@ -2,6 +2,7 @@ import { mkdirSync } from "node:fs"; import type { Server } from "node:http"; import { join } from "node:path"; import express, { type Express, type NextFunction, type Request, type Response } from "express"; +import { downloadAttachments } from "../bootstrap/attachments.js"; import { syncOrClone } from "../bootstrap/git.js"; import { runSetupCommands } from "../bootstrap/setup.js"; import { injectGitIdentity } from "../bootstrap/git-identity.js"; @@ -101,6 +102,7 @@ export function defaultBootDeps(): BootDeps { deleteCredentialConfig: () => deleteCredentialConfig(), injectGitIdentity, ensureWorkspace: (dir) => mkdirSync(dir, { recursive: true }), + downloadAttachments, // `||` rather than `??` so a blank WORKSPACE_DIR falls back instead of // resolving every clone destination against "". workspaceRoot: process.env.WORKSPACE_DIR || join(homeDir(), "workspace"), diff --git a/packages/core/src/engine/adapter.ts b/packages/core/src/engine/adapter.ts index 58a002d..269a085 100644 --- a/packages/core/src/engine/adapter.ts +++ b/packages/core/src/engine/adapter.ts @@ -35,8 +35,15 @@ export interface EngineAdapter { * * `workingDirectory` is the primary repo's destination, or the workspace root * when the manifest carries no repos — so it is not necessarily a repository. - * See resolveWorkingDirectory. */ - buildAgentConfig(manifest: Manifest, workingDirectory: string): TConfig; + * See resolveWorkingDirectory. + * + * `additionalDirectories` grants the engine access to directories outside + * `workingDirectory` — today, the sibling `attachments/` dir task-run.ts + * populates at boot when the manifest carries attachments. Optional, and + * defaulted by core to `[]`, so an adapter that has no notion of additional + * directories yet (see the codex adapter) stays a valid implementation of + * this interface unchanged. */ + buildAgentConfig(manifest: Manifest, workingDirectory: string, additionalDirectories?: string[]): TConfig; /** Start the engine's A2A server. */ createA2AServer(config: TConfig): Promise; diff --git a/packages/core/src/manifest/types.ts b/packages/core/src/manifest/types.ts index 14cb277..b35e865 100644 --- a/packages/core/src/manifest/types.ts +++ b/packages/core/src/manifest/types.ts @@ -7,6 +7,13 @@ export interface RepoSpec { primary: boolean; } +export interface AttachmentSpec { + filename: string; + content_type: string; + /** Short-lived presigned GET URL the sandbox downloads at boot. */ + url: string; +} + /** * Where `throng-creds` fetches GitHub tokens from, and the identity it presents * when it does. @@ -55,6 +62,9 @@ export interface WorkspaceManifest { /** A literal token. Takes precedence over `credentials` when both are set. */ github_token: string | null; setup_commands: string[]; + /** Task file attachments, downloaded to a sibling `attachments/` dir at boot. + * Optional on the wire; defaults to [] when absent. */ + attachments: AttachmentSpec[]; } /** Engine-agnostic manifest skeleton owned by core: a workspace plus the commit diff --git a/packages/core/src/manifest/validate.test.ts b/packages/core/src/manifest/validate.test.ts index ef76358..7b8f41b 100644 --- a/packages/core/src/manifest/validate.test.ts +++ b/packages/core/src/manifest/validate.test.ts @@ -51,6 +51,31 @@ describe("validate (registry routing)", () => { }); }); +describe("validate (attachments)", () => { + it("accepts a manifest with no attachments (defaults to [])", () => { + const r = validate(okInput, registry); + expect(r.ok).toBe(true); + if (r.ok) expect(r.manifest.attachments).toEqual([]); + }); + + it("carries valid attachments through", () => { + const input = { + ...okInput, + attachments: [{ filename: "a.txt", content_type: "text/plain", url: "https://s3/x" }], + }; + const r = validate(input, registry); + expect(r.ok).toBe(true); + if (r.ok) expect(r.manifest.attachments).toEqual(input.attachments); + }); + + it("rejects a malformed attachment entry", () => { + const input = { ...okInput, attachments: [{ filename: "a.txt" }] }; + const r = validate(input, registry); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.errors.some((e) => e.field.startsWith("attachments"))).toBe(true); + }); +}); + describe("validate (empty repo list)", () => { it("accepts repos: [] with a valid agent block", () => { const r = validate({ repos: [], agent: { platform: "test", model: "m" } }, registry); diff --git a/packages/core/src/manifest/validate.ts b/packages/core/src/manifest/validate.ts index af0f502..d4209c7 100644 --- a/packages/core/src/manifest/validate.ts +++ b/packages/core/src/manifest/validate.ts @@ -2,6 +2,7 @@ import type { Env } from "../env.js"; import type { AdapterRegistry, EngineAdapter } from "../engine/adapter.js"; import { log } from "../log.js"; import type { + AttachmentSpec, BaseManifest, CredentialsConfig, FieldError, @@ -182,6 +183,24 @@ function validateWorkspace(input: Record, errors: FieldError[]) errors.push({ field: "setup_commands", reason: "each entry must be a non-empty string" }); } } + + if ("attachments" in input) { + const list = input.attachments; + if (!Array.isArray(list)) { + errors.push({ field: "attachments", reason: "must be a list" }); + } else { + list.forEach((a, i) => { + if ( + !isObject(a) || + typeof a.filename !== "string" || + typeof a.content_type !== "string" || + typeof a.url !== "string" + ) { + errors.push({ field: `attachments[${i}]`, reason: "must be {filename, content_type, url}" }); + } + }); + } + } } /** Rules that need every repo at once; run only after the per-field ones pass. */ @@ -338,6 +357,13 @@ function buildWorkspaceManifest( credentials, github_token: blankToNil(input.github_token) ?? blankToNil(env.GITHUB_TOKEN), setup_commands: (input.setup_commands as string[] | undefined) ?? [], + attachments: Array.isArray(input.attachments) + ? (input.attachments as AttachmentSpec[]).map((a) => ({ + filename: a.filename, + content_type: a.content_type, + url: a.url, + })) + : [], }; } diff --git a/packages/core/src/task-run.test.ts b/packages/core/src/task-run.test.ts index 11a49f8..6bf0571 100644 --- a/packages/core/src/task-run.test.ts +++ b/packages/core/src/task-run.test.ts @@ -17,6 +17,7 @@ function deps(over: Partial = {}): BootDeps { deleteCredentialConfig: vi.fn(() => {}), injectGitIdentity: vi.fn(() => {}), ensureWorkspace: vi.fn(() => {}), + downloadAttachments: vi.fn(async () => {}), workspaceRoot: "/home/user/workspace", ...over, }; @@ -156,7 +157,7 @@ describe("TaskRun", () => { expect(tr.lifecycle.status().state).toBe("ready"); expect(d.syncOrClone).not.toHaveBeenCalled(); expect(d.ensureWorkspace).toHaveBeenCalledWith("/home/user/workspace"); - expect(claude.buildAgentConfig).toHaveBeenCalledWith(expect.anything(), "/home/user/workspace"); + expect(claude.buildAgentConfig).toHaveBeenCalledWith(expect.anything(), "/home/user/workspace", []); }); // Separate from the test above because it pins a different property: not that @@ -191,6 +192,39 @@ describe("TaskRun", () => { expect(d.runSetupCommands).toHaveBeenCalledWith("/home/user/workspace", ["mise install"]); expect(d.deleteCredentialConfig).toHaveBeenCalled(); }); + + it("downloads attachments into a sibling dir and grants the agent access", async () => { + const downloadAttachments = vi.fn(async () => {}); + const claude = adapter(); + const tr = new TaskRun(deps({ downloadAttachments }), { claude }); + + await tr.initialise({ + ...okPayload, + attachments: [{ filename: "a.txt", content_type: "text/plain", url: "https://s3/x" }], + }); + await settle(); + + expect(downloadAttachments).toHaveBeenCalledOnce(); + const [atts, dir] = downloadAttachments.mock.calls[0]; + expect(atts).toHaveLength(1); + // sibling of workingDirectory (workspaceRoot/y -> workspaceRoot/attachments) + expect(dir).toBe("/home/user/workspace/attachments"); + + // buildAgentConfig received the attachments dir as an additional directory. + expect(claude.buildAgentConfig).toHaveBeenCalledWith( + expect.anything(), + "/home/user/workspace/y", + ["/home/user/workspace/attachments"], + ); + }); + + it("passes no extra dirs when there are no attachments", async () => { + const claude = adapter(); + const tr = new TaskRun(deps(), { claude }); + await tr.initialise(okPayload); + await settle(); + expect(claude.buildAgentConfig).toHaveBeenCalledWith(expect.anything(), "/home/user/workspace/y", []); + }); }); describe("TaskRun credential ordering", () => { diff --git a/packages/core/src/task-run.ts b/packages/core/src/task-run.ts index 19f7c27..6dd9136 100644 --- a/packages/core/src/task-run.ts +++ b/packages/core/src/task-run.ts @@ -1,12 +1,17 @@ -import { join } from "node:path"; +import { dirname, join } from "node:path"; import type { GitResult } from "./bootstrap/git.js"; import { describeSetupFailure, redactTokens, type SetupResult } from "./bootstrap/setup.js"; import type { AdapterRegistry, EngineAdapter, ServerHandle } from "./engine/adapter.js"; import { Lifecycle } from "./lifecycle.js"; import { log } from "./log.js"; -import type { FieldError, Manifest, UserIdentity, WorkspaceManifest } from "./manifest/types.js"; +import type { AttachmentSpec, FieldError, Manifest, UserIdentity, WorkspaceManifest } from "./manifest/types.js"; import { validate, validatePrepare } from "./manifest/validate.js"; +/** Name of the directory attachments are downloaded into, a sibling of the + * working directory — so the system-prompt convention `../attachments/` + * resolves correctly regardless of the working directory's own name. */ +const ATTACHMENTS_DIRNAME = "attachments"; + /** Engine-agnostic boot dependencies. */ export interface BootDeps { /** Clone-or-resync, because a sandbox restored from a project snapshot already @@ -28,6 +33,8 @@ export interface BootDeps { * machine — stays inert. */ ensureWorkspace: (dir: string) => void; workspaceRoot: string; + /** Downloads task attachments into `dir` (a sibling of the working directory). */ + downloadAttachments: (attachments: AttachmentSpec[], dir: string) => Promise; } /** What a control-server route does with an accepted, rejected or duplicate POST. */ @@ -134,9 +141,11 @@ export class TaskRun { // second copy of these methods: the shared call path is what keeps a // snapshot build and the task boot that restores from it from drifting. // - // The returned working directory is discarded here on purpose: a prepare - // builds a filesystem image and never starts an agent, so nothing after - // this point needs to know where one would have run. + // The returned working directory and attachments directory are discarded + // here on purpose: a prepare builds a filesystem image and never starts an + // agent, so nothing after this point needs to know where one would have + // run. A `WorkspaceManifest` also carries no attachments, so nothing + // downloads on this path regardless. this.writeCredentials(manifest, "prepare"); await this.materialiseWorkspace(manifest, "prepare"); @@ -199,14 +208,18 @@ export class TaskRun { private async boot(manifest: Manifest, adapter: EngineAdapter): Promise { try { this.writeCredentials(manifest); - const workingDirectory = await this.materialiseWorkspace(manifest); + const { workingDirectory, attachmentsDir } = await this.materialiseWorkspace(manifest); log.info("boot step: injecting engine credentials and commit identity"); adapter.injectCredentials(manifest); // Commit identity only. GitHub auth is no longer environment-based. this.deps.injectGitIdentity(manifest.user_identity); - const config = adapter.buildAgentConfig(manifest, workingDirectory); + // Only granted when something was actually downloaded there — an + // attachments-less task must not hand the agent a directory that + // doesn't exist. + const extraDirs = (manifest.attachments ?? []).length > 0 ? [attachmentsDir] : []; + const config = adapter.buildAgentConfig(manifest, workingDirectory, extraDirs); log.info("boot step: starting A2A server", { workingDirectory }); try { this.serverHandle = await adapter.createA2AServer(config); @@ -257,14 +270,43 @@ export class TaskRun { * * `phase` only labels the logs — a prepare run that never boots an agent must * not emit "boot step: cloning repos" at an operator hunting a failure. + * + * Also resolves and populates the attachments directory, a sibling of the + * working directory: `workspaceRoot/` -> `workspaceRoot/attachments`. + * A sibling, not a subdirectory of the working directory, because the working + * directory is a git checkout the agent's own commands operate on, and the + * system-prompt convention pointing the agent at attachments is the fixed + * relative path `../attachments/` — that only resolves correctly if the + * two directories are siblings. Downloading happens here, alongside cloning + * and setup, for the same reason those live in one method: order is a + * correctness property (attachments must exist before the agent that reads + * them starts), and this is the one place trusted to keep it. A prepare run + * passes no attachments (see `WorkspaceManifest` vs `Manifest`), so the + * download is unconditionally skipped there. */ - private async materialiseWorkspace(manifest: WorkspaceManifest, phase: Phase = "boot"): Promise { + private async materialiseWorkspace( + manifest: WorkspaceManifest, + phase: Phase = "boot", + ): Promise<{ workingDirectory: string; attachmentsDir: string }> { // Resolved before the clone, so a manifest with no primary repo fails having // done nothing rather than after pulling every repo over the network. const workingDirectory = resolveWorkingDirectory(manifest, this.deps.workspaceRoot); + const attachmentsDir = join(dirname(workingDirectory), ATTACHMENTS_DIRNAME); await this.syncRepos(manifest, phase); await this.runSetup(manifest, workingDirectory, phase); - return workingDirectory; + + const attachments = manifest.attachments ?? []; + if (attachments.length > 0) { + this.lifecycle.set("setup"); + log.info(`${phase} step: downloading attachments`, { count: attachments.length, dir: attachmentsDir }); + try { + await this.deps.downloadAttachments(attachments, attachmentsDir); + } catch (err) { + throw new StepError("attachments", err instanceof Error ? err.message : String(err)); + } + } + + return { workingDirectory, attachmentsDir }; } /** Clones or resyncs every repo in the manifest. The working directory is diff --git a/throng-agent-claude/src/adapter.ts b/throng-agent-claude/src/adapter.ts index b4dbc02..106fd03 100644 --- a/throng-agent-claude/src/adapter.ts +++ b/throng-agent-claude/src/adapter.ts @@ -43,8 +43,12 @@ export class ClaudeEngineAdapter } } - buildAgentConfig(manifest: Manifest, workingDirectory: string): Required { - return buildAgentConfig(manifest, workingDirectory); + buildAgentConfig( + manifest: Manifest, + workingDirectory: string, + additionalDirectories?: string[], + ): Required { + return buildAgentConfig(manifest, workingDirectory, additionalDirectories); } async createA2AServer(config: Required): Promise { diff --git a/throng-agent-claude/src/config/build.test.ts b/throng-agent-claude/src/config/build.test.ts index b103813..096f750 100644 --- a/throng-agent-claude/src/config/build.test.ts +++ b/throng-agent-claude/src/config/build.test.ts @@ -215,4 +215,14 @@ describe("buildAgentConfig", () => { const cfg = buildAgentConfig(manifest({}), "/work/app"); expect(cfg.claude.outputFormat).toBeUndefined(); }); + + it("sets additionalDirectories when attachment dirs are provided", () => { + const cfg = buildAgentConfig(manifest({}), "/work/app", ["/work/attachments"]); + expect(cfg.claude.additionalDirectories).toEqual(["/work/attachments"]); + }); + + it("leaves additionalDirectories unset when none are provided", () => { + const cfg = buildAgentConfig(manifest({}), "/work/app"); + expect(cfg.claude.additionalDirectories ?? []).toEqual([]); + }); }); diff --git a/throng-agent-claude/src/config/build.ts b/throng-agent-claude/src/config/build.ts index fa62889..e71c79e 100644 --- a/throng-agent-claude/src/config/build.ts +++ b/throng-agent-claude/src/config/build.ts @@ -20,6 +20,7 @@ import type { ResolvedClaudeAgent } from "../manifest/claude-agent.js"; export function buildAgentConfig( manifest: Manifest, workingDirectory: string, + additionalDirectories: string[] = [], ): Required { const a = manifest.agent.keys; const claude: NonNullable = { @@ -73,6 +74,7 @@ export function buildAgentConfig( if (Array.isArray(a.allowed_tools)) claude.allowedTools = a.allowed_tools as string[]; if (Array.isArray(a.disallowed_tools)) claude.disallowedTools = a.disallowed_tools as string[]; if (typeof a.max_turns === "number") claude.maxTurns = a.max_turns; + if (additionalDirectories.length > 0) claude.additionalDirectories = additionalDirectories; // Structured output. Only the outer key is renamed — the schema body is JSON // Schema's own vocabulary and is forwarded verbatim. Left unset when absent so