diff --git a/engdocs/architecture/wave-05-runtime-docker-plan.md b/engdocs/architecture/wave-05-runtime-docker-plan.md new file mode 100644 index 000000000..0c55c4b22 --- /dev/null +++ b/engdocs/architecture/wave-05-runtime-docker-plan.md @@ -0,0 +1,321 @@ +# Wave 5 — Docker Executor Implementation Plan + +**Architect:** architect-1 +**Spec:** `specs/04-runtime-docker/spec.md` +**Package:** `@sverka/runtime-docker` → `packages/runtime-docker` +**Depends on:** Wave 3 (`@sverka/runtime`), Wave 2 (`@sverka/ir`) + +This plan is the contract the builder implements against. The spec is the +source of truth; this plan adds sequencing, file layout, conventions, and +edge-case guidance. Where the two disagree, the spec wins — except where the +spec conflicts with the **built** runtime contract, in which case the built +contract wins (see §1 amendments). + +## 1. Spec amendments already applied (architect) + +Spec 04 was written before `@sverka/runtime` was built. Mismatches with the +built contract have been corrected in the spec: + +1. **`workspace` / `artifactDir` removed from `DockerExecutorConfig`.** The + scheduler passes both per-execution via `ExecuteRequest` + (`request.workspace`, `request.artifactDir`). The executor must use the + request fields, not config fields. `canExecute` (which receives only the + operation) never needed them. +2. **Env/credentials use `request.*`, not `operation.*` as value sources.** + `operation.credentials` is `CredentialDeclaration[]` (name/envVar/required + — declarations). Resolved secret **values** arrive via + `request.credentials: Record` (keyed by envVar). + `request.env` carries the operation env vars. Error rule #4 and test plan + item 5 updated accordingly. +3. **Test commands corrected:** `bun test` → `bun run test` (vitest via nx, + not Bun's built-in runner — see drill-finding-2026-08-09-bun-test-in). + +## 2. Scope + +Implement the Docker executor for `@sverka/runtime-docker`: + +- `DockerExecutor` class implementing `Executor` from `@sverka/runtime`. +- `DockerExecutorConfig` (runAs, cacheDir, dockerPath?, dockerHost?, + maxLogBytes?). +- `verifyImageDigest` function (image digest verification via `docker + inspect`/`docker pull`). +- `CacheManager` interface + `DockerCacheManager` (filesystem cache + prepare/collect). +- Error hierarchy: `DockerExecutorError` → `ImageDigestError`, + `ContainerPolicyError`. +- Internal `docker-cli.ts` — mockable seam for unit tests. +- Public re-exports from `src/index.ts`. + +**Dependency:** `@sverka/runtime` (Executor, ExecuteRequest, ExecuteResult) + +`@sverka/ir` (PlanOperation). Both `workspace:*`. + +**Out of scope (do NOT implement in this wave):** +- **Retry.** The scheduler owns retry (`maxAttempts`/`retryOn`/`backoffSeconds`, + Wave 3). The executor executes **once** and returns a result. Spec goal 6 + ("support retry and timeout policies") is satisfied by returning `status: + "failure"` results that the scheduler can retry. +- **Docker daemon lifecycle.** Assumed available (spec non-goal). +- **Image building/publishing.** Spec non-goal. +- **Podman.** Handled by `runtime-podman`. + +## 3. Scaffolding status (already done by architect) + +- `packages/runtime-docker/package.json` — fixed: dist paths are + `.mjs`/`.d.mts` (matches `core`/`ir`/`runtime`/`runtime-host`); + `@sverka/runtime` and `@sverka/ir` added to `dependencies` as `workspace:*`. +- `packages/runtime-docker/project.json` — already has `--passWithNoTests` on + the test target. +- `tsconfig.json`, `tsdown.config.ts` — already match `runtime-host`; no + changes. +- `src/index.ts` — placeholder; builder fills exports. +- `bun install` run; lockfile updated. + +## 4. File layout + +Mirror `runtime-host` (one module per concern, `__tests__/` co-located): + +``` +packages/runtime-docker/src/ + index.ts # public re-exports (matches spec §Interfaces) + errors.ts # DockerExecutorError, ImageDigestError, ContainerPolicyError + config.ts # DockerExecutorConfig (type-only) + docker-executor.ts # DockerExecutor class + image.ts # verifyImageDigest + cache.ts # CacheManager interface + DockerCacheManager + internal/ + docker-cli.ts # runDocker wrapper — mockable seam for unit tests + __tests__/ + errors.test.ts + docker-executor.test.ts # canExecute, buildArgs, buildEnv, policy, timeout, logs + image.test.ts # verifyImageDigest (mocked cli) + cache.test.ts # DockerCacheManager (filesystem ops) + public-api.test.ts + integration.test.ts # describe.skipIf(!process.env.SVERKA_DOCKER) + helpers/ + fixtures.ts # op factory + makeRequest helper +``` + +`internal/docker-cli.ts` is NOT exported (not in spec §Interfaces). It is the +single mockable seam: unit tests `vi.mock("../internal/docker-cli.js")`. + +## 5. Testability design + +The executor separates **pure logic** from **side effects**: + +- `buildDockerArgs(request): string[]` — constructs the `docker run` arg + array. Pure. Tested directly (policy flags, network mapping, mounts). +- `buildEnv(request): Record` — builds the container env from + `operation.credentials` (declarations) + `request.credentials` (values) + + `request.env`. Pure. Tested directly (secrets allowlist, undeclared + detection). +- Validation (timeout, digest presence, socket deny, secret detection) — + pure, throws before any spawn. +- `internal/docker-cli.ts` exports `runDocker(args, opts): Promise` + — the only side-effectful seam. Wraps `node:child_process` `spawn` of + `docker`. Default implementation spawns real `docker`. Unit tests mock it + via `vi.mock`. Integration tests use the real implementation. + +`DockerCommandResult` (defined in `internal/docker-cli.ts`, not exported): +```typescript +interface DockerCommandResult { + readonly stdout: string; + readonly stderr: string; + readonly exitCode: number; + readonly timedOut?: boolean; +} +``` + +## 6. Implementation order (TDD: tests first, then impl) + +### Slice A — Errors (foundation, no deps) +1. `errors.test.ts` — `DockerExecutorError` base (sets `name`, carries `code` + + `context`), `ImageDigestError` (`IMAGE_DIGEST_MISMATCH`), + `ContainerPolicyError` (`CONTAINER_POLICY_VIOLATION`), `instanceof` chain. + Mirror `runtime-host/src/errors.ts` constructor pattern exactly. +2. `errors.ts` — implement. Wire into `index.ts`. + +### Slice B — Config + public API skeleton +3. `config.ts` — `DockerExecutorConfig` interface (post-amendment: no + `workspace`/`artifactDir`). +4. `public-api.test.ts` (skeleton) — assert every exported symbol importable. + +### Slice C — Docker CLI seam +5. `internal/docker-cli.ts` — `runDocker(args, opts)` wrapping `spawn("docker", + [...args])`. Capture stdout/stderr, resolve exitCode, handle timeout via + `setTimeout` + SIGTERM + SIGKILL grace (mirror `host-executor.ts` + `spawnProcess` pattern). Return `DockerCommandResult`. No test file of its + own — covered by `docker-executor.test.ts` via mock, and + `integration.test.ts` via real Docker. + +### Slice D — canExecute + command construction (pure, no mock) +6. `helpers/fixtures.ts` — `makeDockerOp(overrides)` building a minimal + `PlanOperation` with `executor.type: "docker"`, `executor.image`, + `executor.imageDigest`, `command`, `args`, `timeoutSeconds`, `resources`, + `network`, `credentials`, `artifacts`; `makeRequest(op, overrides)` + building an `ExecuteRequest` with `workspace` (temp dir), `env`, + `credentials`, `cacheDir`, `artifactDir`. +7. `docker-executor.test.ts` — + - `canExecute`: true for `executor.type: "docker"`; false for `host`, + `podman`, `remote`. + - `buildDockerArgs`: includes `--rm`, `--read-only`, `--cap-drop ALL`, + `--network none` (for `network: "deny"`), `--user `, + `--memory `, `--cpus `, + `--timeout `, `--workdir /workspace`, workspace mount + with `readonly`, cache mount, artifact mount. Docker socket NEVER in + any mount. +8. `docker-executor.ts` — implement `DockerExecutor` class: constructor + stores config; `canExecute` per type check; `buildDockerArgs` constructs + the arg array per spec §Container execution policy. + +### Slice E — Network policy mapping (pure) +9. Extend `docker-executor.test.ts` — + - `network: "deny"` → `--network none`. + - `network: "allow-egress"` → no `--network none` (default bridge). + - `network: "allow-host"` → `--network host`. +10. Implement: map `operation.network` to the correct `--network` flag in + `buildDockerArgs`. + +### Slice F — Timeout enforcement (validation + mocked timeout) +11. Extend tests — + - Operation without `timeoutSeconds` (or <= 0) → `ContainerPolicyError` + (`MISSING_TIMEOUT`), no container started. + - Mock `runDocker` to return `{ timedOut: true, exitCode: 137 }` → + `status: "failure"`, error contains "timeout". +12. Implement: validate `timeoutSeconds` before spawn; map `timedOut` result + to failure with timeout error message. + +### Slice G — Image digest verification (mocked CLI) +13. `image.test.ts` — mock `internal/docker-cli.ts`: + - `verifyImageDigest` with matching digest → resolves. + - `verifyImageDigest` with mismatched digest → throws `ImageDigestError` + with both digests in `context`. + - Image not present → mock `docker inspect` to fail, `docker pull` to + succeed, then `inspect` returns matching digest → resolves. +14. `image.ts` — implement: `docker inspect --format={{.Id}} ` to get + local digest; if not present, `docker pull ` then inspect; + compare with `expectedDigest`; throw `ImageDigestError` on mismatch. +15. Extend `docker-executor.test.ts` — operation without `imageDigest` → + `ContainerPolicyError` (`MISSING_DIGEST`), no container started. + +### Slice H — Secrets allowlist + env building (pure) +16. Extend `docker-executor.test.ts` — `buildEnv`: + - Only env vars declared in `operation.credentials` get values from + `request.credentials`. + - `request.env` vars are included. + - `request.env` var matching secret denylist pattern (e.g. + `/SECRET|TOKEN|PASSWORD|KEY/i`) NOT declared in + `operation.credentials` → `ContainerPolicyError` + (`UNDECLARED_SECRET`). + - Docker socket path (`/var/run/docker.sock`) in any mount or env → + `ContainerPolicyError` (`DOCKER_SOCKET_DENIED`). +17. Implement: `buildEnv` per spec error rule #4 (post-amendment). Secret + denylist pattern: `/^(?:.*_)?(?:SECRET|TOKEN|PASSWORD|KEY|CREDENTIAL)$/i`. + Socket detection: check for `docker.sock` in mount sources and env values. + +### Slice I — Cache management (filesystem, no Docker) +18. `cache.test.ts` — `DockerCacheManager`: + - `prepare(inputs, key)` creates `/` and + copies/symlinks declared inputs. + - `collect(outputs, sourceDir)` copies declared outputs back to + persistent `cacheDir`. + - Second `prepare` with same key restores from cache (inputs exist). +19. `cache.ts` — implement `CacheManager` interface + `DockerCacheManager`. + Use `node:fs/promises` (`mkdir`, `copyFile`, `symlink`). No Docker. + +### Slice J — Logs, artifacts, log truncation +20. Extend `docker-executor.test.ts` — + - Mock `runDocker` returning stdout/stderr → `ExecuteResult.logs` + contains both. + - Logs exceeding `maxLogBytes` → truncated + notice appended (mirror + `host-executor.ts` `truncateLogs`). + - Declared artifact (file under workspace) copied into + `request.artifactDir` (mirror `host-executor.ts` `collectArtifacts`). + - Missing artifact → reported in result error, status unchanged. +21. Implement: `execute` calls `runDocker`, builds `ExecuteResult` from + `DockerCommandResult`, truncates logs, collects artifacts. Use + `request.workspace` and `request.artifactDir`. + +### Slice K — Integration tests (skippable) +22. `integration.test.ts` — `describe.skipIf(!process.env.SVERKA_DOCKER)`: + - Run `echo hello` in `busybox@` → `status: "success"`, logs + contain `hello`. + - Run `sh -c "exit 1"` → `status: "failure"`, `exitCode: 1`. + - These require a real Docker daemon; skipped by default. + +### Slice L — Public API + gates +23. Complete `index.ts` exports to match spec §Interfaces exactly: + `DockerExecutor`, `DockerExecutorConfig`, `verifyImageDigest`, + `CacheManager`, `DockerCacheManager`, `DockerExecutorError`, + `ImageDigestError`, `ContainerPolicyError`. +24. `public-api.test.ts` — every symbol importable + exercised. +25. Run gates: `bun run test`, `bun run typecheck`, `bun run lint`, + `bun run build`. All green (lint is pre-existing broken repo-wide — + sv-ei2; not a blocker for this wave). + +## 7. Convention checklist (enforced by reviewer) + +- **No `any`.** Use `unknown` + narrow. No `@ts-ignore`/`@ts-expect-error`. +- **`verbatimModuleSyntax: true`** → type-only imports use `import type`. +- **`exactOptionalPropertyTypes: true`** → never assign `undefined` to an + optional field; use conditional spread. +- **`noUncheckedIndexedAccess: true`** → narrow array/object access. +- **readonly everywhere** — all interface fields are `readonly`. +- **Error `name`** — each error subclass sets `this.name` in the constructor. +- **ESM only** — `.js` specifiers in imports. No `.cjs`/`.mjs` source. +- **Public surface** — only spec §Interfaces symbols exported from + `src/index.ts`. `internal/docker-cli.ts` is NOT exported. +- **Test command** — `bun run test` (vitest via nx). NEVER `bun test`. + +## 8. Edge cases the builder must handle + +- **Executor type mismatch** — `canExecute` false for non-docker; direct + `execute` raises `ContainerPolicyError` (`WRONG_EXECUTOR_TYPE`). +- **Missing timeout** — `ContainerPolicyError` (`MISSING_TIMEOUT`) before + spawn. +- **Missing digest** — `ContainerPolicyError` (`MISSING_DIGEST`) before spawn. +- **Digest mismatch** — `ImageDigestError` with both digests in `context`. +- **Undeclared secret** — `request.env` var matching denylist but not in + `operation.credentials` → `ContainerPolicyError` (`UNDECLARED_SECRET`). +- **Docker socket** — any mount/env referencing `docker.sock` → + `ContainerPolicyError` (`DOCKER_SOCKET_DENIED`). +- **Non-zero exit** — normal result (`status: "failure"`), not an exception. +- **Timeout** — container killed, `status: "failure"`, error indicates + timeout. +- **Log truncation** — append notice when truncated; never exceed + `maxLogBytes`. +- **Missing artifact** — reported in result error, status unchanged. +- **`dispose()`** — no-op (no persistent resources); implement to satisfy + the optional `Executor.dispose` contract. + +## 9. Error code map + +| Condition | code | error class | +|--------------------------|----------------------------|--------------------------| +| Wrong executor type | `WRONG_EXECUTOR_TYPE` | `ContainerPolicyError` | +| Missing timeout | `MISSING_TIMEOUT` | `ContainerPolicyError` | +| Missing digest | `MISSING_DIGEST` | `ContainerPolicyError` | +| Image digest mismatch | `IMAGE_DIGEST_MISMATCH` | `ImageDigestError` | +| Undeclared secret | `UNDECLARED_SECRET` | `ContainerPolicyError` | +| Docker socket denied | `DOCKER_SOCKET_DENIED` | `ContainerPolicyError` | + +## 10. Gates (reviewer runs these) + +```bash +bun install # resolve new workspace deps +bun run test # vitest via nx (NOT `bun test`) +bun run typecheck # strict, no any +bun run lint # eslint clean (pre-existing sv-ei2 broken repo-wide) +bun run build # tsdown produces dist/index.mjs + .d.mts +``` + +Acceptance criteria: test + typecheck + build green; spec §Test plan items +1–8, 10 pass (item 9 integration tests skippable; item 11 commands). Lint is +pre-existing broken (sv-ei2) — not a blocker for this wave. + +## 11. ADR + +Optional: `engdocs/adr/ADR-008-docker-executor-mockable-cli-seam.md` — one +paragraph recording that the Docker executor separates pure command +construction from the `internal/docker-cli.ts` spawn seam to enable unit +testing without a Docker daemon. File only if the reviewer/mayor wants the +decision durable beyond this plan. diff --git a/packages/core/project.json b/packages/core/project.json index 709b1af41..905e809e3 100644 --- a/packages/core/project.json +++ b/packages/core/project.json @@ -11,7 +11,7 @@ "test": { "executor": "nx:run-commands", "options": { - "command": "bun run vitest run --passWithNoTests", + "command": "bun run vitest run", "cwd": "packages/core" } }, diff --git a/packages/core/src/__tests__/composables/workflow.test.ts b/packages/core/src/__tests__/composables/workflow.test.ts index 7bf879940..1ba802b3a 100644 --- a/packages/core/src/__tests__/composables/workflow.test.ts +++ b/packages/core/src/__tests__/composables/workflow.test.ts @@ -33,14 +33,16 @@ describe("workflow()", () => { const d = run({ command: "d" }); const wf = workflow("mixed", parallel(a, b), pipeline(c, d)); const result = await wf.plan(makePlanRuntime()); - const ids = result.operations.map((o) => o.id); - expect(ids).toContain("run:a"); - expect(ids).toContain("run:b"); - expect(ids).toContain("run:c"); - expect(ids).toContain("run:d"); + const byCmd = new Map(result.operations.map((o) => [o.command, o])); + const aSpec = byCmd.get("a")!; + const bSpec = byCmd.get("b")!; + const cSpec = byCmd.get("c")!; + const dSpec = byCmd.get("d")!; + for (const s of [aSpec, bSpec, cSpec, dSpec]) { + expect(s.id).toMatch(/^op-[0-9a-f]{64}$/); + } // d depends on c - const dSpec = result.operations.find((o) => o.id === "run:d")!; - expect(dSpec.dependsOn).toEqual(["run:c"]); + expect(dSpec.dependsOn).toEqual([cSpec.id]); }); it("empty workflow plans to zero operations", async () => { diff --git a/packages/core/src/__tests__/dag.test.ts b/packages/core/src/__tests__/dag.test.ts index 3c31c5f5b..9dde38261 100644 --- a/packages/core/src/__tests__/dag.test.ts +++ b/packages/core/src/__tests__/dag.test.ts @@ -6,44 +6,56 @@ import { workflow } from "../composables/workflow.js"; import { CompositionError } from "../errors.js"; import { makePlanRuntime } from "./helpers/runtime.js"; +const OP_ID_RE = /^op-[0-9a-f]{64}$/; + describe("DAG validation", () => { it("rejects a cycle with node ids in context", async () => { - // User-provided dependsOn string ids forming a cycle: x→y→x. + // User-provided spec.id aliases referenced in dependsOn form a cycle: x→y→x. // (Predecessor-ref cycles are structurally impossible with immutable - // after(); cycles arise from explicit dependsOn ids.) + // after(); cycles arise from explicit dependsOn ids. spec.id values are + // resolved to op- ids during edge resolution, so the cycle is detected on + // the content-addressed ids.) const x = run({ id: "x", command: "x", dependsOn: ["y"] }); const y = run({ id: "y", command: "y", dependsOn: ["x"] }); const wf = workflow("cyclic", x, y); - const promise = wf.plan(makePlanRuntime()); - await expect(promise).rejects.toThrow(CompositionError); + await expect(wf.plan(makePlanRuntime())).rejects.toThrow(CompositionError); try { - await promise; + await wf.plan(makePlanRuntime()); } catch (err) { - if (!(err instanceof CompositionError)) throw err; - expect(err.context).toBeDefined(); - expect(err.context?.["cycle"]).toBeDefined(); + const ce = err as CompositionError; + expect(ce.context).toBeDefined(); + expect(ce.context?.["cycle"]).toBeDefined(); } }); - it("rejects a dependsOn id that matches no operation", async () => { - const a = run({ id: "a", command: "a", dependsOn: ["ghost"] }); - const wf = workflow("unknown-dep", a); - const promise = wf.plan(makePlanRuntime()); - await expect(promise).rejects.toThrow(CompositionError); - await expect(promise).rejects.toThrow(/unknown id 'ghost'/); - }); - - it("rejects duplicate user-provided ids", async () => { + it("rejects duplicate user-provided spec.id aliases", async () => { + // spec.id is folded into the hash (as userId) and also used as a + // dependsOn alias; duplicate aliases are rejected because they would make + // edge resolution ambiguous. const a = run({ id: "dup", command: "a" }); const b = run({ id: "dup", command: "b" }); const wf = workflow("dup-ids", a, b); - const promise = wf.plan(makePlanRuntime()); - await expect(promise).rejects.toThrow(CompositionError); + await expect(wf.plan(makePlanRuntime())).rejects.toThrow(CompositionError); + try { + await wf.plan(makePlanRuntime()); + } catch (err) { + const ce = err as CompositionError; + expect(ce.context?.["id"]).toBe("dup"); + } + }); + + it("rejects true duplicate operations (identical kind/name/context)", async () => { + // Two operations with identical content produce the same content-addressed + // op- id; the planner detects the collision and raises CompositionError. + const a = run({ command: "echo" }); + const b = run({ command: "echo" }); + const wf = workflow("true-dups", a, b); + await expect(wf.plan(makePlanRuntime())).rejects.toThrow(CompositionError); try { - await promise; + await wf.plan(makePlanRuntime()); } catch (err) { - if (!(err instanceof CompositionError)) throw err; - expect(err.context?.["id"]).toBe("dup"); + const ce = err as CompositionError; + expect(ce.context?.["id"]).toMatch(OP_ID_RE); } }); @@ -53,18 +65,16 @@ describe("DAG validation", () => { const c = run({ command: "c" }); const wf = workflow("linear", pipeline(a, b, c)); const result = await wf.plan(makePlanRuntime()); + const byCmd = new Map(result.operations.map((o) => [o.command, o])); + const aSpec = byCmd.get("a")!; + const bSpec = byCmd.get("b")!; + const cSpec = byCmd.get("c")!; const ids = result.operations.map((o) => o.id); - const aIdx = ids.indexOf("run:a"); - const bIdx = ids.indexOf("run:b"); - const cIdx = ids.indexOf("run:c"); - expect(aIdx).toBeGreaterThanOrEqual(0); - expect(bIdx).toBeGreaterThan(aIdx); - expect(cIdx).toBeGreaterThan(bIdx); - // b depends on a, c depends on b - const bSpec = result.operations.find((o) => o.id === "run:b")!; - const cSpec = result.operations.find((o) => o.id === "run:c")!; - expect(bSpec.dependsOn).toEqual(["run:a"]); - expect(cSpec.dependsOn).toEqual(["run:b"]); + expect(ids.indexOf(aSpec.id)).toBeGreaterThanOrEqual(0); + expect(ids.indexOf(bSpec.id)).toBeGreaterThan(ids.indexOf(aSpec.id)); + expect(ids.indexOf(cSpec.id)).toBeGreaterThan(ids.indexOf(bSpec.id)); + expect(bSpec.dependsOn).toEqual([aSpec.id]); + expect(cSpec.dependsOn).toEqual([bSpec.id]); }); it("parallel siblings have no inter-sibling dependsOn", async () => { @@ -72,8 +82,9 @@ describe("DAG validation", () => { const b = run({ command: "b" }); const wf = workflow("parallel", a, b); const result = await wf.plan(makePlanRuntime()); - const aSpec = result.operations.find((o) => o.id === "run:a")!; - const bSpec = result.operations.find((o) => o.id === "run:b")!; + const byCmd = new Map(result.operations.map((o) => [o.command, o])); + const aSpec = byCmd.get("a")!; + const bSpec = byCmd.get("b")!; expect(aSpec.dependsOn ?? []).toEqual([]); expect(bSpec.dependsOn ?? []).toEqual([]); }); diff --git a/packages/core/src/__tests__/ids.test.ts b/packages/core/src/__tests__/ids.test.ts new file mode 100644 index 000000000..1757b10ee --- /dev/null +++ b/packages/core/src/__tests__/ids.test.ts @@ -0,0 +1,73 @@ +import { describe, it, expect } from "vitest"; +import { createHash } from "node:crypto"; +import { computeOperationId } from "../internal/ids.js"; +import { canonicalStringify } from "../internal/canonical.js"; + +const OP_ID_RE = /^op-[0-9a-f]{64}$/; + +describe("computeOperationId", () => { + it("is prefixed with op- followed by 64 hex chars", () => { + const id = computeOperationId("run", "build", {}); + expect(id).toMatch(OP_ID_RE); + }); + + it("is deterministic for identical inputs", () => { + expect(computeOperationId("run", "build", { os: "linux" })).toBe( + computeOperationId("run", "build", { os: "linux" }), + ); + }); + + it("is independent of context key insertion order", () => { + const a = computeOperationId("run", "build", { os: "linux", arch: "x64" }); + const b = computeOperationId("run", "build", { arch: "x64", os: "linux" }); + expect(a).toBe(b); + }); + + it("differs across kinds", () => { + expect(computeOperationId("run", "build", {})).not.toBe( + computeOperationId("check", "build", {}), + ); + }); + + it("differs across names", () => { + expect(computeOperationId("run", "build", {})).not.toBe( + computeOperationId("run", "test", {}), + ); + }); + + it("differs across context values", () => { + expect(computeOperationId("run", "build", { os: "linux" })).not.toBe( + computeOperationId("run", "build", { os: "macos" }), + ); + }); + + it("differs when context values differ only by type", () => { + expect(computeOperationId("run", "build", { target: 1 })).not.toBe( + computeOperationId("run", "build", { target: "1" }), + ); + }); + + it("matrix expansion produces distinct, deterministic ids", () => { + const combos = [ + { os: "linux", arch: "x64" }, + { os: "linux", arch: "arm64" }, + { os: "macos", arch: "x64" }, + { os: "macos", arch: "arm64" }, + ]; + const ids = combos.map((c) => computeOperationId("run", "build", c)); + expect(new Set(ids).size).toBe(ids.length); + expect(combos.map((c) => computeOperationId("run", "build", c))).toEqual(ids); + }); + + it("matches a manual sha256 over the canonical JSON of {kind,name,context}", () => { + const kind = "run"; + const name = "build"; + const context = { os: "linux", arch: "x64" }; + const expected = + "op-" + + createHash("sha256") + .update(canonicalStringify({ kind, name, context }), "utf8") + .digest("hex"); + expect(computeOperationId(kind, name, context)).toBe(expected); + }); +}); diff --git a/packages/core/src/__tests__/laziness.test.ts b/packages/core/src/__tests__/laziness.test.ts index 299cff72b..e10e5737e 100644 --- a/packages/core/src/__tests__/laziness.test.ts +++ b/packages/core/src/__tests__/laziness.test.ts @@ -55,7 +55,11 @@ describe("laziness: composables perform no I/O", () => { const fetchSpy = vi.spyOn(globalThis, "fetch"); const a = run({ command: "a" }); const b = run({ command: "b" }); - const wf = workflow("ci", parallel(a, b), pipeline(a, b)); + const c = run({ command: "c" }); + const d = run({ command: "d" }); + // Distinct ops per root — content-addressed ids reject true duplicates, + // so we don't reuse a/b across roots (this test checks I/O, not dedup). + const wf = workflow("ci", parallel(a, b), pipeline(c, d)); await wf.plan(makePlanRuntime()); expect(childProcessMock.spawn).not.toHaveBeenCalled(); expect(fsMock.writeFileSync).not.toHaveBeenCalled(); diff --git a/packages/core/src/__tests__/matrix.test.ts b/packages/core/src/__tests__/matrix.test.ts index bebd369ec..6ec897eae 100644 --- a/packages/core/src/__tests__/matrix.test.ts +++ b/packages/core/src/__tests__/matrix.test.ts @@ -2,37 +2,41 @@ import { describe, it, expect } from "vitest"; import { run } from "../composables/run.js"; import { matrix } from "../composables/matrix.js"; import { workflow } from "../composables/workflow.js"; -import { CompositionError } from "../errors.js"; import { makePlanRuntime } from "./helpers/runtime.js"; +import { computeOperationId } from "../internal/ids.js"; + +const OP_ID_RE = /^op-[0-9a-f]{64}$/; describe("matrix expansion", () => { - it("produces one node per value with distinct ids and MATRIX_ env", async () => { + it("produces one node per value with distinct op- ids and MATRIX_ env", async () => { const op = matrix({ node: ["20", "24"] }, run({ command: "test" })); const wf = workflow("matrix-1d", op); const result = await wf.plan(makePlanRuntime()); expect(result.operations).toHaveLength(2); - const ids = result.operations.map((o) => o.id).sort(); - expect(ids).toEqual(["run:test[node=s:20]", "run:test[node=s:24]"]); + const ids = result.operations.map((o) => o.id); + for (const id of ids) expect(id).toMatch(OP_ID_RE); + expect(new Set(ids).size).toBe(ids.length); // distinct + // ids are content-addressed over {kind, name, context:{node, command}} + const expected = ["20", "24"].map((v) => + computeOperationId("run", "test", { node: v, command: "test" }), + ); + expect(ids.sort()).toEqual([...expected].sort()); const envs = result.operations.map((o) => o.env?.MATRIX_NODE).sort(); expect(envs).toEqual(["20", "24"]); }); - it("multi-dimension cartesian product with joined id suffix", async () => { + it("multi-dimension cartesian product with distinct op- ids", async () => { const op = matrix({ node: ["20", "24"], os: ["linux", "macos"] }, run({ command: "test" })); const wf = workflow("matrix-2d", op); const result = await wf.plan(makePlanRuntime()); expect(result.operations).toHaveLength(4); - const ids = result.operations.map((o) => o.id).sort(); - expect(ids).toEqual([ - "run:test[node=s:20,os=s:linux]", - "run:test[node=s:20,os=s:macos]", - "run:test[node=s:24,os=s:linux]", - "run:test[node=s:24,os=s:macos]", - ]); - const envs = result.operations - .map((spec) => `${spec.env?.MATRIX_NODE}/${spec.env?.MATRIX_OS}`) - .sort(); - expect(envs).toEqual(["20/linux", "20/macos", "24/linux", "24/macos"]); + const ids = result.operations.map((o) => o.id); + for (const id of ids) expect(id).toMatch(OP_ID_RE); + expect(new Set(ids).size).toBe(ids.length); + for (const spec of result.operations) { + expect(spec.env?.MATRIX_NODE).toBeDefined(); + expect(spec.env?.MATRIX_OS).toBeDefined(); + } }); it("children inherit predecessors from the template", async () => { @@ -41,10 +45,11 @@ describe("matrix expansion", () => { const matrixed = matrix({ node: ["20", "24"] }, test).after(build); const wf = workflow("matrix-deps", matrixed); const result = await wf.plan(makePlanRuntime()); - const children = result.operations.filter((spec) => spec.id.startsWith("run:test[")); - expect(children).toHaveLength(2); - for (const spec of children) { - expect(spec.dependsOn).toEqual(["run:build"]); + const buildSpec = result.operations.find((o) => o.command === "build")!; + const testSpecs = result.operations.filter((o) => o.command === "test"); + expect(testSpecs.length).toBe(2); + for (const spec of testSpecs) { + expect(spec.dependsOn).toEqual([buildSpec.id]); } }); @@ -64,32 +69,18 @@ describe("matrix expansion", () => { expect(result.operations).toHaveLength(3); }); - it("duplicate matrix values raise CompositionError", async () => { + it("rejects duplicate matrix values that would produce duplicate ids", async () => { const op = matrix({ node: ["20", "20"] }, run({ command: "test" })); const wf = workflow("matrix-dup", op); - await expect(wf.plan(makePlanRuntime())).rejects.toThrow(CompositionError); - }); - - it("delimiter characters in values are escaped in ids", async () => { - const op = matrix({ key: ["a,b=c"] }, run({ command: "test" })); - const wf = workflow("matrix-escape", op); - const result = await wf.plan(makePlanRuntime()); - expect(result.operations).toHaveLength(1); - // The comma and equals in the value should be escaped, not treated as - // dimension boundaries. - const id = result.operations[0]!.id; - expect(id).toContain("a\\,b\\=c"); + await expect(wf.plan(makePlanRuntime())).rejects.toThrow("duplicate operation id"); }); - it("number and string values with the same text produce distinct ids", async () => { - const op = matrix({ v: [1, "1", true] }, run({ command: "test" })); + it("distinguishes values with identical text but different types", async () => { + const op = matrix({ node: [1, "1", true] as unknown[] }, run({ command: "test" })); const wf = workflow("matrix-types", op); const result = await wf.plan(makePlanRuntime()); expect(result.operations).toHaveLength(3); - expect(result.operations.map((o) => o.id).sort()).toEqual([ - "run:test[v=b:true]", - "run:test[v=n:1]", - "run:test[v=s:1]", - ]); + const ids = result.operations.map((o) => o.id); + expect(new Set(ids).size).toBe(3); }); }); diff --git a/packages/core/src/__tests__/public-api.test.ts b/packages/core/src/__tests__/public-api.test.ts index 24f348675..50d965b51 100644 --- a/packages/core/src/__tests__/public-api.test.ts +++ b/packages/core/src/__tests__/public-api.test.ts @@ -43,8 +43,31 @@ describe("public API surface", () => { // named exports of the public barrel. const publicNames = Object.keys(api); const internalLeaked = publicNames.filter((n) => - ["createNode", "asNode", "withSpec", "mergeSpecs", "concatDedupe", "planWorkflow", "assignId", "evaluateCondition"].includes(n), + [ + "createNode", + "asNode", + "withSpec", + "mergeSpecs", + "concatDedupe", + "planWorkflow", + "evaluateCondition", + ].includes(n), ); expect(internalLeaked).toEqual([]); }); + + it("exports computeOperationId (content-addressed id primitive, ADR-006)", () => { + // computeOperationId is a pure, dependency-free primitive that any + // consumer (planner, compiler, ir) can use to recompute an operation id + // from its content. It is part of the stable public contract. + expect(typeof api.computeOperationId).toBe("function"); + expect(api.computeOperationId("run", "build", {})).toMatch(/^op-[0-9a-f]{64}$/); + }); + + it("exports canonicalStringify (canonical JSON primitive, ADR-006)", () => { + // canonicalStringify is the stable serialization primitive shared by + // computeOperationId and the ir package's serializePlan/computePlanId. + expect(typeof api.canonicalStringify).toBe("function"); + expect(api.canonicalStringify({ b: 1, a: 2 })).toBe('{"a":2,"b":1}'); + }); }); diff --git a/packages/core/src/__tests__/runtime-modes.test.ts b/packages/core/src/__tests__/runtime-modes.test.ts index 8f162e82c..72dde24bc 100644 --- a/packages/core/src/__tests__/runtime-modes.test.ts +++ b/packages/core/src/__tests__/runtime-modes.test.ts @@ -35,7 +35,10 @@ describe("Runtime modes", () => { ); expect(result.mode).toBe("execute"); expect(evaluated).toHaveLength(2); - expect([...evaluated].sort()).toEqual(["run:a", "run:b"]); + // ids are content-addressed op-<64hex>; both evaluated ids match the plan ids + const planIds = result.operations.map((o) => o.id); + expect([...evaluated].sort()).toEqual([...planIds].sort()); + for (const id of evaluated) expect(id).toMatch(/^op-[0-9a-f]{64}$/); }); it("Compile mode produces a string artifact", async () => { @@ -45,7 +48,8 @@ describe("Runtime modes", () => { expect(result.mode).toBe("compile"); expect(result.artifacts).toBeDefined(); expect(result.artifacts).toHaveLength(1); - expect(result.artifacts![0]!.content).toBe("run:a"); + // artifact content is the joined op- ids of evaluated operations + expect(result.artifacts![0]!.content).toMatch(/^op-[0-9a-f]{64}$/); }); it("skipped condition: evaluate is NOT called, status is 'skipped'", async () => { @@ -68,26 +72,27 @@ describe("Runtime modes", () => { const evaluated: string[] = []; const nightly = when("schedule == 'nightly'", run({ command: "full-scan" })); const wf = workflow("cond-in", nightly); - await wf.plan( + const result = await wf.plan( makeExecuteRuntime({ schedule: "nightly" }, (spec) => { evaluated.push(spec.id); return { operationId: spec.id, status: "success", durationMs: 0 }; }), ); - expect(evaluated).toEqual(["run:full-scan"]); + expect(evaluated).toEqual([result.operations[0]!.id]); + expect(evaluated[0]).toMatch(/^op-[0-9a-f]{64}$/); }); it("no context: all conditions included by default", async () => { const evaluated: string[] = []; const guarded = when("schedule == 'nightly'", run({ command: "scan" })); const wf = workflow("no-ctx", guarded); - await wf.plan( + const result = await wf.plan( makeExecuteRuntime(undefined, (spec) => { evaluated.push(spec.id); return { operationId: spec.id, status: "success", durationMs: 0 }; }), ); - expect(evaluated).toEqual(["run:scan"]); + expect(evaluated).toEqual([result.operations[0]!.id]); }); it("pipeline ordering preserved through execute", async () => { @@ -96,13 +101,15 @@ describe("Runtime modes", () => { const b = run({ command: "b" }); const c = run({ command: "c" }); const wf = workflow("ordered", pipeline(a, b, c)); - await wf.plan( + const result = await wf.plan( makeExecuteRuntime(undefined, (spec) => { order.push(spec.id); return { operationId: spec.id, status: "success", durationMs: 0 }; }), ); - expect(order).toEqual(["run:a", "run:b", "run:c"]); + // ordering follows the topo-sorted plan ids: a, b, c by command + const byCmd = new Map(result.operations.map((o) => [o.command, o])); + expect(order).toEqual([byCmd.get("a")!.id, byCmd.get("b")!.id, byCmd.get("c")!.id]); }); it("compile mode receives all operations including false-condition ones", async () => { @@ -118,8 +125,9 @@ describe("Runtime modes", () => { ); // In compile mode, false-condition operations are still passed to the // compiler so it can emit them with their condition field. - expect(evaluated).toContain("run:full-scan"); - expect(evaluated).toContain("run:always"); + const byCmd = new Map(result.operations.map((o) => [o.command, o])); + expect(evaluated).toContain(byCmd.get("full-scan")!.id); + expect(evaluated).toContain(byCmd.get("always")!.id); expect(result.operations).toHaveLength(2); }); @@ -132,15 +140,16 @@ describe("Runtime modes", () => { const result = await wf.plan( makeExecuteRuntime(undefined, (spec) => { evaluated.push(spec.id); - if (spec.id === "run:b") { + if (spec.command === "b") { return { operationId: spec.id, status: "failure", durationMs: 0 }; } return { operationId: spec.id, status: "success", durationMs: 0 }; }), ); // b fails, c should be cancelled (not evaluated) - expect(evaluated).toEqual(["run:a", "run:b"]); - const cOutcome = result.outcomes.find((o) => o.operationId === "run:c"); + const byCmd = new Map(result.operations.map((o) => [o.command, o])); + expect(evaluated).toEqual([byCmd.get("a")!.id, byCmd.get("b")!.id]); + const cOutcome = result.outcomes!.find((o) => o.operationId === byCmd.get("c")!.id); expect(cOutcome?.status).toBe("cancelled"); }); @@ -153,13 +162,15 @@ describe("Runtime modes", () => { const result = await wf.plan( makeExecuteRuntime(undefined, (spec) => { evaluated.push(spec.id); - const status = spec.id === "run:b" ? "failure" : "success"; + const status = spec.command === "b" ? "failure" : "success"; return { operationId: spec.id, status, durationMs: 0 }; }), ); - expect(evaluated).toEqual(["run:a", "run:b", "run:c"]); - const continueOutcome = result.outcomes.find((o) => o.operationId === "run:c"); - expect(continueOutcome?.status).toBe("success"); + expect(evaluated).toEqual(result.operations.map((o) => o.id)); + const cOutcome = result.outcomes.find((o) => + result.operations.some((op) => op.id === o.operationId && op.command === "c"), + ); + expect(cOutcome?.status).toBe("success"); }); it("when(condition, parallel(...)) propagates condition to siblings", async () => { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 4b2d29dc5..9833b196f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -25,4 +25,5 @@ export { when } from "./composables/when.js"; export { matrix } from "./composables/matrix.js"; export { workflow, type Workflow } from "./composables/workflow.js"; export { CoreError, PlanningError, CompositionError } from "./errors.js"; +export { computeOperationId } from "./internal/ids.js"; export { canonicalStringify } from "./internal/canonical.js"; diff --git a/packages/core/src/internal/canonical.ts b/packages/core/src/internal/canonical.ts index 848b7f395..4e6f4d9ee 100644 --- a/packages/core/src/internal/canonical.ts +++ b/packages/core/src/internal/canonical.ts @@ -1,21 +1,21 @@ /** - * Canonical JSON serialization for content-addressed ID computation - * (ADR-006). + * Canonical JSON serialization — the stable primitive shared by + * `computeOperationId` (and, in the `ir` package, `serializePlan` / + * `computePlanId`). * - * Rules: - * - Object keys sorted lexicographically. - * - Compact output (no indentation, no spaces). - * - `undefined` values omitted from objects and arrays. - * - Array order preserved. - * - `NaN`/`Infinity` serialized as `null` (JSON compatibility). - * - No external dependency; the `ir` package implements the same - * algorithm independently for `serializePlan`. - */ - -/** - * Produce a canonical JSON string for the given value. - * Keys are sorted lexicographically, `undefined` is omitted, and - * output is compact. + * Rules (per ADR-006 and spec 01-core §ID assignment): + * - Object keys sorted lexicographically (ascending UTF-16 code-unit order, + * i.e. default JS string comparison). + * - Compact: no whitespace, no indentation, no trailing newline. + * - `undefined` object fields are omitted (never serialized). + * - Array element order is preserved; `undefined` array elements emit `null`. + * - `NaN`, `Infinity`, `-Infinity` are rejected (not valid JSON). + * - Strings escaped per JSON.stringify rules, including lone UTF-16 surrogate + * code units. + * + * Implemented as a manual recursive emitter so the wire format and the hash + * input can never drift. This is the single source of truth — the `ir` package + * re-exports it from `@sverka/core` rather than maintaining its own copy. */ export function canonicalStringify(value: unknown): string { const out: string[] = []; @@ -65,6 +65,9 @@ function emitScalar(value: unknown, out: string[]): void { out.push(Number(value).toString()); return; } + if (typeof value === "bigint") { + throw new TypeError("canonical JSON does not support bigint"); + } throw new TypeError( `canonical JSON does not support value of type ${typeof value}`, ); @@ -90,8 +93,6 @@ function emitObject(obj: Record, out: string[]): void { if (obj[k] === undefined) continue; keys.push(k); } - // Code-unit order (RFC 8785). Locale-aware comparison is not deterministic - // across runtimes and ICU builds. keys.sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)); out.push("{"); for (let i = 0; i < keys.length; i++) { @@ -104,6 +105,12 @@ function emitObject(obj: Record, out: string[]): void { out.push("}"); } +/** + * Quote a string per JSON rules. Mirrors JSON.stringify's string escaping: + * escapes ", \, and control chars (< 0x20) using short escapes where defined + * and \u00XX otherwise. Lone UTF-16 surrogate code units (0xD800–0xDFFF not + * part of a valid pair) are escaped as \uXXXX to ensure valid UTF-8 output. + */ const SHORT_ESCAPES: Readonly> = { '"': '\\"', "\\": "\\\\", @@ -134,6 +141,7 @@ function quoteString(s: string): string { return parts.join(""); } +/** Check if a UTF-16 code unit is a lone surrogate (not part of a valid pair). */ function isLoneSurrogate(code: number, i: number, s: string): boolean { if (code < 0xd800 || code > 0xdfff) return false; if (isLowSurrogate(code) && hasValidHighSurrogateBefore(i, s)) return false; diff --git a/packages/core/src/internal/ids.ts b/packages/core/src/internal/ids.ts index afea83644..6a3ec0b13 100644 --- a/packages/core/src/internal/ids.ts +++ b/packages/core/src/internal/ids.ts @@ -1,65 +1,30 @@ -import type { OperationKind, OperationSpec } from "../operation.js"; -import type { OperationNode } from "./node.js"; +import { createHash } from "node:crypto"; +import type { OperationKind } from "../operation.js"; import { canonicalStringify } from "./canonical.js"; /** - * Assign a deterministic id to a node during planning. + * Compute a deterministic, content-addressed operation id from kind, name, + * and a context record of discriminating fields (matrix values, user-provided + * spec.id folded in as `userId`, command, args, etc.). * - * Precedence: - * 1. User-provided `spec.id` — used as-is. Duplicate user ids are rejected - * by the caller (not here). - * 2. Derived id — `${kind}:${name || command || index}`. - * 3. On collision (only when no user id was given), a monotonic counter - * suffix is appended. - */ -export function assignId( - node: OperationNode, - index: number, - usedIds: Set, -): string { - const spec = node.spec; - if (spec.id !== undefined) { - return spec.id; - } - const base = derivedBase(node, index); - if (!usedIds.has(base)) return base; - let counter = 2; - while (usedIds.has(`${base}-${counter}`)) counter++; - return `${base}-${counter}`; -} - -function derivedBase(node: OperationNode, index: number): string { - const { kind, spec } = node; - const name = spec.name || spec.command || String(index); - return `${kind}:${name}`; -} - -/** - * Build the id suffix for a matrix child: `${baseId}[k1=v1,k2=v2]`. - * Dimensions are joined with `,` in stable insertion order. - * Delimiter characters (`,`, `=`, `\`) in keys and values are escaped - * with a backslash so that two different dimension sets cannot produce - * the same id string. + * Algorithm (ADR-006 / spec 01-core §ID assignment): SHA-256 over the + * canonical JSON of `{ kind, name, context }` (keys sorted, compact, UTF-8), + * hex-encoded, prefixed with `op-`. Matrix expansion produces distinct ids + * because each combination yields a distinct `context`. No external hashing + * library — uses Node's built-in `node:crypto`. + * + * The `ir` package re-exports this function from `@sverka/core` so both + * packages produce identical ids by construction. The core/ir consistency + * test guards against drift. */ -export function matrixChildId( - baseId: string, - dims: ReadonlyArray, +export function computeOperationId( + kind: OperationKind, + name: string, + context: Readonly>, ): string { - const parts = dims.map(([k, v]) => `${escapeSegment(k)}=${formatMatrixValue(v)}`); - return `${baseId}[${parts.join(",")}]`; -} - -/** Escape the `,`, `=`, and `\` delimiter characters used in matrix ids. */ -function escapeSegment(s: string): string { - return s.replace(/[\\,=]/g, (ch) => `\\${ch}`); -} - -function formatMatrixValue(v: unknown): string { - if (typeof v === "string") return `s:${escapeSegment(v)}`; - if (typeof v === "number") return `n:${String(v)}`; - if (typeof v === "boolean") return `b:${String(v)}`; - if (v === undefined) return `u:undefined`; - return `u:${escapeSegment(canonicalStringify(v))}`; + const canonical = canonicalStringify({ kind, name, context }); + const hex = createHash("sha256").update(canonical, "utf8").digest("hex"); + return `op-${hex}`; } /** Validate that a kind is a known {@link OperationKind}. */ @@ -74,4 +39,3 @@ export function isKnownKind(kind: string): kind is OperationKind { kind === "custom" ); } - diff --git a/packages/core/src/internal/plan.ts b/packages/core/src/internal/plan.ts index 8cd9fb426..ed7302bb5 100644 --- a/packages/core/src/internal/plan.ts +++ b/packages/core/src/internal/plan.ts @@ -1,8 +1,7 @@ import type { Operation, OperationSpec } from "../operation.js"; import type { OperationOutcome, Runtime, RuntimeResult } from "../runtime.js"; import { asNode, createNode, withSpec, type OperationNode } from "./node.js"; -import { assignId, matrixChildId, isKnownKind } from "./ids.js"; -import { canonicalStringify } from "./canonical.js"; +import { computeOperationId, isKnownKind } from "./ids.js"; import { evaluateCondition } from "./conditions.js"; import { CoreError, CompositionError } from "../errors.js"; @@ -27,17 +26,19 @@ export async function planWorkflow( const discovered = discover(roots); // 2. Expand matrix templates into cartesian-product children. - const { nodes: expanded, combos } = expandMatrices(discovered); + const expanded = expandMatrices(discovered); // 3. Flatten artifact nodes (join / empty-pipeline): dependents of a join // depend on its siblings instead; artifact nodes are then removed. const realNodes = flattenArtifacts(expanded); - // 4. Assign deterministic ids; reject duplicate user ids. - const idMap = assignIds(realNodes, combos); + // 4. Assign deterministic content-addressed ids; reject duplicates; build + // alias map (user spec.id -> op- id) for dependsOn resolution. + const { idMap, aliasMap } = assignIds(realNodes); - // 5. Resolve predecessor refs → dependsOn string ids, merge with user deps. - const specs = resolveEdges(realNodes, idMap); + // 5. Resolve predecessor refs → dependsOn string ids, merge with user deps, + // and resolve user-provided spec.id aliases in dependsOn to op- ids. + const specs = resolveEdges(realNodes, idMap, aliasMap); // 6. Cycle detection. detectCycles(specs); @@ -52,7 +53,11 @@ export async function planWorkflow( } catch (err) { // Ensure runtime.finalize() is called even when evaluate() rejects, // so executors can release containers, processes, and file handles. - await runtime.finalize(); + try { + await runtime.finalize(); + } catch { + // Cleanup failure must not mask the original evaluation error. + } throw err; } @@ -73,20 +78,6 @@ export async function planWorkflow( * field intact). In execute/plan mode, false conditions produce a synthetic * "skipped" outcome without calling runtime.evaluate(). */ -async function outcomeForFalseCondition( - spec: OperationSpec, - runtime: Runtime, -): Promise { - // Compile mode still passes the operation to the compiler so the emitted - // artifact keeps the condition field. - if (runtime.mode === "compile") return runtime.evaluate(spec); - return { operationId: spec.id, status: "skipped", durationMs: 0 }; -} - -function isSkipped(spec: OperationSpec, runtime: Runtime): boolean { - return spec.condition !== undefined && !evaluateCondition(spec.condition, runtime.context); -} - async function evaluateOperations( ordered: readonly OperationSpec[], runtime: Runtime, @@ -98,8 +89,11 @@ async function evaluateOperations( outcomes.push({ operationId: spec.id, status: "cancelled", durationMs: 0 }); continue; } - if (isSkipped(spec, runtime)) { - outcomes.push(await outcomeForFalseCondition(spec, runtime)); + const skipped = + spec.condition !== undefined && + !evaluateCondition(spec.condition, runtime.context); + if (skipped) { + outcomes.push(await outcomeForSkipped(spec, runtime)); continue; } const outcome = await runtime.evaluate(spec); @@ -110,6 +104,16 @@ async function evaluateOperations( } } +async function outcomeForSkipped( + spec: OperationSpec, + runtime: Runtime, +): Promise { + if (runtime.mode === "compile") { + return runtime.evaluate(spec); + } + return { operationId: spec.id, status: "skipped", durationMs: 0 }; +} + function nowMs(): number { return Date.now(); } @@ -154,16 +158,9 @@ function discover(roots: readonly Operation[]): OperationNode[] { // 2. Matrix expansion // --------------------------------------------------------------------------- -/** Maximum number of matrix combinations before a CompositionError is raised. */ -const MAX_MATRIX_COMBINATIONS = 256; - -/** Expand matrix template nodes into cartesian-product children. - * Returns the expanded nodes and a Map from each child to its combo. */ -function expandMatrices( - nodes: readonly OperationNode[], -): { nodes: OperationNode[]; combos: Map } { +/** Expand matrix template nodes into cartesian-product children. */ +function expandMatrices(nodes: readonly OperationNode[]): OperationNode[] { const result: OperationNode[] = []; - const combos = new Map(); for (const node of nodes) { if (markerOf(node) !== MATRIX_MARKER) { result.push(node); @@ -174,16 +171,23 @@ function expandMatrices( result.push(node); continue; } - validateDims(dims); + validateMatrixDims(dims); for (const combo of cartesianProduct(dims)) { - result.push(buildMatrixChild(node, combo, combos)); + const env: Record = { ...(node.spec.env ?? {}) }; + for (const [k, v] of combo) env[`MATRIX_${k.toUpperCase()}`] = String(v); + const childSpec = { ...node.spec }; + delete (childSpec as unknown as Record)[MATRIX_MARKER]; + delete (childSpec as unknown as Record).matrix; + const child = withSpec(node, { ...childSpec, env }); + (child as unknown as Record).__matrixCombo = combo; + result.push(child); } } - return { nodes: result, combos }; + return result; } /** Validate that all matrix dimensions are non-empty arrays. */ -function validateDims(dims: Readonly>): void { +function validateMatrixDims(dims: Readonly>): void { for (const [key, values] of Object.entries(dims)) { if (!Array.isArray(values)) { throw new CompositionError( @@ -199,25 +203,6 @@ function validateDims(dims: Readonly>): void } } -/** Build a matrix child node from a template and a dimension combination. */ -function buildMatrixChild( - node: OperationNode, - combo: readonly [string, unknown][], - combos: Map, -): OperationNode { - const env: Record = { ...node.spec.env }; - for (const [k, v] of combo) { - env[`MATRIX_${k.toUpperCase()}`] = - typeof v === "object" && v !== null ? canonicalStringify(v) : String(v); - } - const childSpec = { ...node.spec }; - delete (childSpec as unknown as Record)[MATRIX_MARKER]; - delete (childSpec as unknown as Record).matrix; - const child = withSpec(node, { ...childSpec, env }); - combos.set(child, combo); - return child; -} - function cartesianProduct( dims: Readonly>, ): ReadonlyArray { @@ -226,16 +211,6 @@ function cartesianProduct( const [first, ...rest] = entries; if (first === undefined) return [[]]; const [dimKey, dimValues] = first; - - const total = dimValues.length * cartesianProductCount(Object.fromEntries(rest)); - if (total > MAX_MATRIX_COMBINATIONS) { - const dimSummary = entries.map(([k, v]) => `${k}=${v.length}`).join(", "); - throw new CompositionError( - `matrix cartesian product exceeds limit of ${MAX_MATRIX_COMBINATIONS} (dimensions: ${dimSummary})`, - { dimensions: Object.fromEntries(entries.map(([k, v]) => [k, v.length])) }, - ); - } - const restProduct = cartesianProduct(Object.fromEntries(rest)); const result: Array = []; for (const v of dimValues) { @@ -308,7 +283,7 @@ function flattenArtifacts(nodes: readonly OperationNode[]): OperationNode[] { const combined = existing !== undefined ? `(${existing} && ${joinCond})` : joinCond; if (combined !== existing) { const newSpec = { ...node.spec, condition: combined }; - return makeNodeWith(node.kind, newSpec, node.predecessors, node.siblings, node._id); + return makeNodeWith(node.kind, newSpec, node.predecessors, node.siblings, node._id, node); } } if (node.predecessors.length === 0) return node; @@ -318,7 +293,7 @@ function flattenArtifacts(nodes: readonly OperationNode[]): OperationNode[] { newPreds.length === node.predecessors.length && newPreds.every((p, i) => p === node.predecessors[i]); if (same) return node; - return makeNodeWith(node.kind, node.spec, newPreds, node.siblings, node._id); + return makeNodeWith(node.kind, node.spec, newPreds, node.siblings, node._id, node); }); return rewritten.filter((n) => !isArtifact(n)); @@ -331,6 +306,7 @@ function makeNodeWith( predecessors: readonly OperationNode[], siblings: readonly OperationNode[], _id: string | undefined, + source?: OperationNode, ): OperationNode { // createNode yields a node with empty preds/siblings; reattach via the // immutable after()/with() API so the public contract is respected. @@ -338,72 +314,150 @@ function makeNodeWith( if (predecessors.length > 0) n = asNode(n.after(...predecessors)); if (siblings.length > 0) n = asNode(n.with(...siblings)); if (_id !== undefined) (n as unknown as { _id: string })._id = _id; + if (source !== undefined) { + const combo = (source as unknown as { __matrixCombo?: unknown }).__matrixCombo; + if (combo !== undefined) { + (n as unknown as { __matrixCombo?: unknown }).__matrixCombo = combo; + } + } return n; } // --------------------------------------------------------------------------- -// 4. ID assignment +// 4. ID assignment (ADR-006: SHA-256 content-addressed op- ids) // --------------------------------------------------------------------------- -/** Assign deterministic ids; reject duplicate user ids and matrix collisions. */ -function assignIds( - nodes: readonly OperationNode[], - combos: Map, -): Map { +/** + * Assign deterministic, content-addressed ids (ADR-006). Also build a map + * from user-provided `spec.id` aliases to op- ids so that `dependsOn` strings + * referencing aliases can be resolved during edge resolution. + * + * - Each node's id is `computeOperationId(kind, name, context)` where + * `context` carries matrix dimension values, `userId` (if `spec.id` is + * set), `command`, and `args`. No positional index — true duplicates + * (identical kind/name/context) collide by construction and are rejected. + * - Duplicate user-provided `spec.id` aliases are rejected (they would make + * `dependsOn` alias resolution ambiguous). + * + * Returns `{ idMap, aliasMap }`. + */ +function assignIds(nodes: readonly OperationNode[]): { + idMap: Map; + aliasMap: Map; +} { const idMap = new Map(); + const aliasMap = new Map(); const usedIds = new Set(); - nodes.forEach((node, index) => { + for (const node of nodes) { if (!isKnownKind(node.kind)) { throw new CoreError(`unknown operation kind '${node.kind}'`, "UNKNOWN_KIND", { kind: node.kind, }); } - const combo = combos.get(node); - let id: string; - if (combo !== undefined) { - const base = assignId(stripId(node), index, new Set(usedIds)); - id = matrixChildId(base, combo); - } else { - id = assignId(node, index, usedIds); - } + const id = computeOperationId(node.kind, nameFor(node), contextFor(node)); if (usedIds.has(id)) { throw new CompositionError(`duplicate operation id '${id}'`, { id }); } usedIds.add(id); idMap.set(node, id); - }); - return idMap; + if (node.spec.id !== undefined) { + if (aliasMap.has(node.spec.id)) { + throw new CompositionError( + `duplicate operation id '${node.spec.id}'`, + { id: node.spec.id }, + ); + } + aliasMap.set(node.spec.id, id); + } + } + return { idMap, aliasMap }; +} + +/** + * Resolve the `name` component of the id per spec 01-core §ID assignment: + * `spec.name` if provided, else `spec.command` if provided, else `"operation"`. + * The hash still distinguishes via `context` when names coincide. + */ +function nameFor(node: OperationNode): string { + const s = node.spec; + if (s.name !== undefined) return s.name; + if (s.command !== undefined) return s.command; + return "operation"; } -/** Return a node view with spec.id removed (for base-id derivation of children). */ -function stripId(node: OperationNode): OperationNode { - const { id: _omit, ...rest } = node.spec; - return withSpec(node, rest); +/** + * Build the `context` record of discriminating fields. Includes matrix + * dimension values (stable insertion order), `userId` (user `spec.id` folded + * into the hash, NOT used as the op id), `command`, and `args`. No positional + * index — true duplicates collide by construction (spec test-plan requirement). + */ +function contextFor(node: OperationNode): Record { + const s = node.spec; + const ctx: Record = {}; + const combo = (node as unknown as Record).__matrixCombo as + | readonly [string, unknown][] + | undefined; + if (combo !== undefined) { + for (const [k, v] of combo) ctx[k] = v; + } + if (s.id !== undefined) ctx["userId"] = s.id; + if (s.command !== undefined) ctx["command"] = s.command; + if (s.args !== undefined) ctx["args"] = [...s.args]; + return ctx; } // --------------------------------------------------------------------------- // 5. Edge resolution // --------------------------------------------------------------------------- -/** Resolve predecessor refs to dependsOn ids, merge with user deps, dedupe. */ +/** + * Resolve predecessor refs to dependsOn ids, merge with user deps, dedupe. + * User-provided `dependsOn` strings may be either op- ids (already content- + * addressed) or user `spec.id` aliases; aliases are resolved to op- ids via + * `aliasMap`. An unresolvable alias is a `CompositionError`. + */ function resolveEdges( nodes: readonly OperationNode[], idMap: Map, + aliasMap: Map, ): Map { + const knownOpIds = new Set(idMap.values()); const result = new Map(); for (const node of nodes) { const id = idMap.get(node)!; - const userDeps = node.spec.dependsOn ?? []; const resolvedDeps = node.predecessors.map((p) => idMap.get(p)); if (resolvedDeps.includes(undefined)) { throw new CompositionError("unresolved predecessor reference", { node: id }); } - const dependsOn = [...new Set([...userDeps, ...(resolvedDeps as string[])])]; + const userDeps = node.spec.dependsOn ?? []; + const resolvedUserDeps = userDeps.map((dep) => resolveDep(dep, aliasMap, knownOpIds)); + const dependsOn = [ + ...new Set([...resolvedUserDeps, ...(resolvedDeps as string[])]), + ]; result.set(node, buildSpec(node, id, dependsOn)); } return result; } +/** + * Resolve a single user-provided dependsOn string. It may be: + * - an op- id already present in the plan (returned as-is), + * - a user `spec.id` alias mapped via `aliasMap` (resolved to its op- id), + * - otherwise an error (dangling reference). + */ +function resolveDep( + dep: string, + aliasMap: Map, + knownOpIds: Set, +): string { + if (knownOpIds.has(dep)) return dep; + const aliased = aliasMap.get(dep); + if (aliased !== undefined) return aliased; + throw new CompositionError(`unresolved dependsOn reference '${dep}'`, { + dependsOn: dep, + }); +} + const OPTIONAL_SPEC_KEYS = [ "description", "command", "args", "env", "workingDir", "image", "imageDigest", "condition", "cpuLimit", "memoryLimit", "timeoutSeconds", "retries", @@ -449,13 +503,7 @@ function detectCycles(specs: Map): void { stack.push(id); const spec = byId.get(id)!; for (const dep of spec.dependsOn ?? []) { - if (!byId.has(dep)) { - throw new CompositionError( - `operation '${id}' depends on unknown id '${dep}'`, - { id, unknownDep: dep }, - ); - } - visit(dep); + if (byId.has(dep)) visit(dep); } stack.pop(); color.set(id, "black"); @@ -469,8 +517,7 @@ function detectCycles(specs: Map): void { function topoSort(specs: Map): OperationSpec[] { const all = [...specs.values()]; - const byId = new Map(all.map((s) => [s.id, s] as const)); - const { indegree, adj } = buildDependencyGraph(all); + const { byId, indegree, adj } = buildDependencyGraph(all); const ordered = kahnSort(all, byId, indegree, adj); if (ordered.length !== all.length) { throw new CompositionError("topological sort failed (residual cycle)", {}); @@ -478,23 +525,20 @@ function topoSort(specs: Map): OperationSpec[] { return ordered; } -function buildDependencyGraph( - all: OperationSpec[], -): { - indegree: Map; - adj: Map; -} { +function buildDependencyGraph(all: OperationSpec[]) { + const byId = new Map(all.map((s) => [s.id, s] as const)); const indegree = new Map(all.map((s) => [s.id, 0] as const)); const adj = new Map(all.map((s) => [s.id, []] as const)); for (const spec of all) { for (const dep of spec.dependsOn ?? []) { - // Unknown-dependency validation is done in detectCycles() which runs - // before topoSort; skip the duplicate check here. + if (!byId.has(dep)) { + throw new CompositionError(`topological sort encountered unknown dependency '${dep}'`, { dependsOn: dep }); + } adj.get(dep)!.push(spec.id); indegree.set(spec.id, (indegree.get(spec.id) ?? 0) + 1); } } - return { indegree, adj }; + return { byId, indegree, adj }; } function kahnSort( @@ -505,10 +549,8 @@ function kahnSort( ): OperationSpec[] { const queue = all.filter((s) => (indegree.get(s.id) ?? 0) === 0).map((s) => s.id); const ordered: OperationSpec[] = []; - let head = 0; - while (head < queue.length) { - const id = queue[head]!; - head++; + while (queue.length > 0) { + const id = queue.shift()!; ordered.push(byId.get(id)!); for (const next of adj.get(id) ?? []) { indegree.set(next, (indegree.get(next) ?? 0) - 1); diff --git a/packages/ir/src/__tests__/core-consistency.test.ts b/packages/ir/src/__tests__/core-consistency.test.ts new file mode 100644 index 000000000..89e4c47d2 --- /dev/null +++ b/packages/ir/src/__tests__/core-consistency.test.ts @@ -0,0 +1,31 @@ +import { describe, it, expect } from "vitest"; +import { computeOperationId as irComputeOperationId } from "../ids.js"; +import { computeOperationId as coreComputeOperationId, type OperationKind } from "@sverka/core"; + +/** + * ADR-006 cross-package consistency: the `ir` package re-exports + * `computeOperationId` from `@sverka/core`, so both packages produce identical + * ids by construction. This test asserts the re-export and pins the algorithm + * with golden hashes to detect regression drift. + */ +describe("core / ir computeOperationId consistency (ADR-006)", () => { + const cases: Array<[OperationKind, string, Record, string]> = [ + ["run", "build", {}, "op-b75f0e7c4aa34a7f67cad8a4bfe9547ffe992a79736df1d6b19d28a5534e7bb6"], + ["run", "build", { os: "linux" }, "op-ec9c1a04131e3bd5cb07b1eca9880930299e88c0a1b9b2f128655f670aaedd23"], + ["run", "test", { node: "20", os: "linux" }, "op-1e54982274bd04562da23f5b6431a2d030990aaae2b2062786e12507268aaa51"], + ["check", "lint", { command: "eslint", args: [".", "--fix"] }, "op-efbf3f2fbf0dbcf20f0ad5a420137613f764db02be66bbf2e9da7fadc7477a44"], + ["build", "img", { userId: "user-assign", command: "docker build" }, "op-d059b9ff79ea5a6ad5dfb14804ccb549d202214497fb674301f5f0755fc4fab9"], + ["run", "operation", { matrix: { node: ["20", "24"] } }, "op-6c62792d253f1b644914c733550a25ac604f7a07e397bddc63d3924b686e42a2"], + ]; + + it("ir re-exports the same function object from core", () => { + expect(irComputeOperationId).toBe(coreComputeOperationId); + }); + + for (const [kind, name, context, expected] of cases) { + it(`produces expected golden id for ${kind}/${name}/${JSON.stringify(context)}`, () => { + expect(coreComputeOperationId(kind, name, context)).toBe(expected); + expect(irComputeOperationId(kind, name, context)).toBe(expected); + }); + } +}); diff --git a/packages/ir/src/__tests__/validate.test.ts b/packages/ir/src/__tests__/validate.test.ts index 77c69aa7c..f5533225b 100644 --- a/packages/ir/src/__tests__/validate.test.ts +++ b/packages/ir/src/__tests__/validate.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect } from "vitest"; import { validatePlan } from "../validate.js"; +import { computePlanId } from "../ids.js"; import type { PlanOperation } from "../plan.js"; import { validPlan, @@ -68,6 +69,14 @@ describe("validatePlan — rule 2 (id matches recomputed)", () => { expect(result.valid).toBe(false); expect(result.errors.some((e) => e.code === "ID_MISMATCH")).toBe(true); }); + + it("accepts a plan with an extra top-level field", () => { + const planWithExtra = { ...validPlan(), extraField: "value" }; + const plan = { ...planWithExtra, id: computePlanId(planWithExtra) }; + const result = validatePlan(plan); + expect(result.errors.some((e) => e.code === "ID_MISMATCH")).toBe(false); + expect(result.valid).toBe(true); + }); }); describe("validatePlan — rule 3 (non-empty operations)", () => { @@ -364,11 +373,8 @@ describe("validatePlan — collects all errors (no short-circuit)", () => { describe("validatePlan — rule 14 (metadata fields)", () => { it("rejects missing sverkaVersion with INVALID_METADATA", () => { - const plan = validPlan({ - metadata: { sverkaVersion: "", generatedBy: "planner" }, - }); // sverkaVersion="" is a string so it passes — test with missing field - const { metadata, ...rest } = validPlan(); + const { metadata: _metadata, ...rest } = validPlan(); const planMissing = { ...rest, metadata: { generatedBy: "planner" } }; const result = validatePlan(planMissing); expect(result.valid).toBe(false); @@ -378,7 +384,7 @@ describe("validatePlan — rule 14 (metadata fields)", () => { }); it("rejects non-string sverkaVersion with INVALID_METADATA", () => { - const { metadata, ...rest } = validPlan(); + const { metadata: _metadata, ...rest } = validPlan(); const plan = { ...rest, metadata: { sverkaVersion: 42, generatedBy: "planner" } }; const result = validatePlan(plan); expect(result.valid).toBe(false); @@ -390,7 +396,7 @@ describe("validatePlan — rule 14 (metadata fields)", () => { }); it("rejects invalid generatedBy union with INVALID_METADATA", () => { - const { metadata, ...rest } = validPlan(); + const { metadata: _metadata, ...rest } = validPlan(); const plan = { ...rest, metadata: { sverkaVersion: "0.0.0", generatedBy: "unknown" } }; const result = validatePlan(plan); expect(result.valid).toBe(false); @@ -401,7 +407,7 @@ describe("validatePlan — rule 14 (metadata fields)", () => { }); it("rejects missing generatedBy with INVALID_METADATA", () => { - const { metadata, ...rest } = validPlan(); + const { metadata: _metadata, ...rest } = validPlan(); const plan = { ...rest, metadata: { sverkaVersion: "0.0.0" } }; const result = validatePlan(plan); expect(result.valid).toBe(false); diff --git a/packages/ir/src/ids.ts b/packages/ir/src/ids.ts index 137367cb8..f6f804403 100644 --- a/packages/ir/src/ids.ts +++ b/packages/ir/src/ids.ts @@ -1,7 +1,6 @@ import { createHash } from "node:crypto"; -import type { OperationKind } from "@sverka/core"; +import { canonicalStringify, computeOperationId } from "@sverka/core"; import type { Plan } from "./plan.js"; -import { canonicalStringify } from "./internal/canonical.js"; /** * Compute a deterministic plan id from the plan content (excluding `id` and @@ -25,16 +24,7 @@ export function computePlanId(plan: Omit): string { * Compute a deterministic operation id from kind, name, and a context record * (matrix values, position, or other discriminating fields). * - * Algorithm: SHA-256 over the canonical JSON of `{ kind, name, context }` - * (keys sorted, UTF-8), hex-encoded, prefixed with `op-`. Matrix expansion - * produces distinct ids because each combination yields a distinct `context`. + * Re-exported from `@sverka/core` to ensure core and ir produce identical ids + * by construction (ADR-006). The core/ir consistency test guards against drift. */ -export function computeOperationId( - kind: OperationKind, - name: string, - context: Readonly>, -): string { - const canonical = canonicalStringify({ kind, name, context }); - const hex = createHash("sha256").update(canonical, "utf8").digest("hex"); - return `op-${hex}`; -} +export { computeOperationId }; diff --git a/packages/ir/src/internal/canonical.ts b/packages/ir/src/internal/canonical.ts index c69f51299..d9aaad039 100644 --- a/packages/ir/src/internal/canonical.ts +++ b/packages/ir/src/internal/canonical.ts @@ -1 +1,9 @@ +/** + * Canonical JSON serialization — re-exported from `@sverka/core`. + * + * The canonical JSON primitive is defined once in `@sverka/core` (per ADR-006) + * and shared here to avoid duplication. Both `serializePlan` and + * `computePlanId` use this single implementation, guaranteeing that the wire + * format and the hash input can never drift from `computeOperationId`. + */ export { canonicalStringify } from "@sverka/core"; diff --git a/packages/ir/src/validate.ts b/packages/ir/src/validate.ts index 78d0c3243..6b550e510 100644 --- a/packages/ir/src/validate.ts +++ b/packages/ir/src/validate.ts @@ -25,6 +25,7 @@ const NETWORK_POLICIES = new Set(["deny", "allow-host", "allow-egress"]); const RETRY_ON_VALUES = new Set(["failure", "timeout"]); const GENERATED_BY_VALUES = new Set(["planner", "manual", "compiler"]); const MEMORY_SUFFIXES = new Set(["Ki", "Mi", "Gi", "Ti"]); +const OPERATION_KINDS = new Set(["run", "check", "build", "analyze", "fetch", "publish", "custom"]); /** Validate sha256 image digest without regex (avoids ReDoS false positive). */ function isValidImageDigest(s: string): boolean { @@ -208,13 +209,7 @@ function validateCreatedAt(p: Record, errors: ValidationErrorDe /** Recompute the plan id and compare with the declared id. */ function recomputeId(p: Record, errors: ValidationErrorDetail[]): void { try { - const body = { - apiVersion: p.apiVersion, - name: p.name, - sourceContextHash: p.sourceContextHash, - operations: p.operations, - metadata: p.metadata, - }; + const { id: _id, createdAt: _createdAt, ...body } = p; const expected = computePlanId(body as Omit); if (expected !== p.id) { errors.push({ field: "id", code: "ID_MISMATCH", message: `id does not match recomputed plan id (expected ${expected})` }); @@ -264,9 +259,6 @@ function detectDuplicateIds(ops: readonly PlanOperationView[], idCounts: Map e.code === "UNKNOWN_DEPENDENCY"); - if (hasUnknownDep) return; - const cycleNodes = ops .filter((op) => typeof op.id === "string" && Array.isArray(op.dependsOn)) .map((op) => ({ @@ -387,11 +379,19 @@ function validateCredentials(op: PlanOperationView, opId: string | undefined, er } } -/** Validate required operation fields (rule 15). */ -function validateOperationShape(op: PlanOperationView, opId: string | undefined, errors: ValidationErrorDetail[]): void { +/** Validate operation identity fields (rule 15). */ +function validateOperationIdentity(op: PlanOperationView, opId: string | undefined, errors: ValidationErrorDetail[]): void { + if (typeof op.kind !== "string" || !OPERATION_KINDS.has(op.kind)) { + errors.push(opError(opId, "operations[].kind", "INVALID_OPERATION", `operation kind must be one of ${[...OPERATION_KINDS].join(", ")}`)); + } if (typeof op.id !== "string" || op.id.length === 0) { errors.push(opError(opId, "operations[].id", "INVALID_OPERATION", "operation id must be a non-empty string")); } +} + +/** Validate required operation fields (rule 15). */ +function validateOperationShape(op: PlanOperationView, opId: string | undefined, errors: ValidationErrorDetail[]): void { + validateOperationIdentity(op, opId, errors); if (typeof op.name !== "string") { errors.push(opError(opId, "operations[].name", "INVALID_OPERATION", "operation name must be a string")); } diff --git a/packages/runtime-docker/project.json b/packages/runtime-docker/project.json index 34fcf86e3..9b2f76072 100644 --- a/packages/runtime-docker/project.json +++ b/packages/runtime-docker/project.json @@ -18,7 +18,7 @@ "lint": { "executor": "nx:run-commands", "options": { - "command": "bun run eslint src --ext .ts", + "command": "bun run eslint src", "cwd": "packages/runtime-docker" } }, diff --git a/packages/runtime-docker/src/__tests__/cache.test.ts b/packages/runtime-docker/src/__tests__/cache.test.ts new file mode 100644 index 000000000..8e4fa10b7 --- /dev/null +++ b/packages/runtime-docker/src/__tests__/cache.test.ts @@ -0,0 +1,130 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtemp, mkdir, writeFile, readFile, rm, access } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { DockerCacheManager } from "../cache.js"; + +async function pathExists(p: string): Promise { + try { + await access(p); + return true; + } catch { + return false; + } +} + +let cacheDir: string; +let sourceDir: string; + +beforeEach(async () => { + cacheDir = await mkdtemp(join(tmpdir(), "sverka-cache-")); + sourceDir = await mkdtemp(join(tmpdir(), "sverka-src-")); +}); + +afterEach(async () => { + await rm(cacheDir, { recursive: true, force: true }); + await rm(sourceDir, { recursive: true, force: true }); +}); + +describe("DockerCacheManager", () => { + it("prepare creates a cache directory keyed by the declared key", async () => { + const mgr = new DockerCacheManager(cacheDir); + const prepared = await mgr.prepare([], "key-1"); + expect(prepared).toBe(join(cacheDir, "key-1")); + expect(await pathExists(prepared)).toBe(true); + }); + + it("prepare copies declared inputs into the cache directory", async () => { + await writeFile(join(sourceDir, "input.txt"), "hello"); + const mgr = new DockerCacheManager(cacheDir); + const prepared = await mgr.prepare( + [join(sourceDir, "input.txt")], + "key-2", + ); + const copied = await readFile(join(prepared, "input.txt"), "utf8"); + expect(copied).toBe("hello"); + }); + + it("prepare preserves input directory structure relative to workspace", async () => { + await mkdir(join(sourceDir, "src"), { recursive: true }); + await writeFile(join(sourceDir, "src", "input.txt"), "hello"); + const mgr = new DockerCacheManager(cacheDir); + const prepared = await mgr.prepare( + [join(sourceDir, "src", "input.txt")], + "key-2-nested", + sourceDir, + ); + const copied = await readFile(join(prepared, "src", "input.txt"), "utf8"); + expect(copied).toBe("hello"); + }); + + it("collect copies declared outputs back to the persistent cacheDir", async () => { + // Simulate outputs written in a source dir after execution. + await mkdir(join(sourceDir, "out"), { recursive: true }); + await writeFile(join(sourceDir, "out", "result.txt"), "result"); + const mgr = new DockerCacheManager(cacheDir); + await mgr.collect( + [join(sourceDir, "out", "result.txt")], + sourceDir, + ".", + ); + // collect should copy outputs into cacheDir preserving relative structure. + const collected = await readFile( + join(cacheDir, "out", "result.txt"), + "utf8", + ); + expect(collected).toBe("result"); + }); + + it("rejects cache keys that escape cacheDir", async () => { + const mgr = new DockerCacheManager(cacheDir); + await expect(mgr.prepare([], "../outside")).rejects.toThrow( + /escapes cacheDir/, + ); + }); + + it("rejects cache outputs that escape the sourceDir", async () => { + await writeFile(join(sourceDir, "result.txt"), "result"); + const mgr = new DockerCacheManager(cacheDir); + await expect( + mgr.collect(["../outside.txt"], sourceDir, "."), + ).rejects.toThrow(/escapes/); + }); + + it("collect skips missing outputs without crashing", async () => { + await mkdir(join(sourceDir, "out"), { recursive: true }); + await writeFile(join(sourceDir, "out", "found.txt"), "yes"); + const mgr = new DockerCacheManager(cacheDir); + await expect( + mgr.collect( + [join(sourceDir, "out", "found.txt"), join(sourceDir, "out", "missing.txt")], + sourceDir, + ".", + ), + ).resolves.not.toThrow(); + const collected = await readFile(join(cacheDir, "out", "found.txt"), "utf8"); + expect(collected).toBe("yes"); + }); + + it("rejects absolute cache keys", async () => { + const mgr = new DockerCacheManager(cacheDir); + await expect(mgr.prepare([], "/tmp/outside")).rejects.toThrow( + /absolute cache key/, + ); + }); + + it("second prepare with same key restores from cache (inputs exist)", async () => { + await writeFile(join(sourceDir, "input.txt"), "v1"); + const mgr = new DockerCacheManager(cacheDir); + await mgr.prepare([join(sourceDir, "input.txt")], "key-3"); + // Remove the source input; second prepare should restore from cache. + await rm(join(sourceDir, "input.txt")); + const prepared = await mgr.prepare( + [join(sourceDir, "input.txt")], + "key-3", + ); + // The cached copy should still be present in the prepared dir. + const restored = await readFile(join(prepared, "input.txt"), "utf8"); + expect(restored).toBe("v1"); + }); +}); diff --git a/packages/runtime-docker/src/__tests__/docker-executor.test.ts b/packages/runtime-docker/src/__tests__/docker-executor.test.ts new file mode 100644 index 000000000..c833fd88c --- /dev/null +++ b/packages/runtime-docker/src/__tests__/docker-executor.test.ts @@ -0,0 +1,564 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { mkdtemp, mkdir, writeFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { DockerExecutor } from "../docker-executor.js"; +import { ContainerPolicyError } from "../errors.js"; +import type { DockerCommandResult } from "../internal/docker-cli.js"; +import { + makeDockerOp, + makeRequest, + defaultConfig, +} from "./helpers/fixtures.js"; + +const DEFAULT_DIGEST = + "sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890"; + +function mockInspectDigest(image: string): string { + return JSON.stringify([`${image}@${DEFAULT_DIGEST}`]); +} + +function dockerMockFor( + runResult: Omit & { stderr?: string }, +): (args: readonly string[]) => Promise { + return async (args: readonly string[]) => { + if (args[0] === "inspect") { + return { + stdout: mockInspectDigest(args[args.length - 1] ?? "busybox:latest"), + stderr: "", + exitCode: 0, + }; + } + if (args[0] === "pull") { + return { stdout: "", stderr: "", exitCode: 0 }; + } + return { stderr: "", ...runResult } as DockerCommandResult; + }; +} + +// Mock the docker-cli seam so no real Docker daemon is needed. +vi.mock("../internal/docker-cli.js", () => ({ + runDocker: vi.fn(async (): Promise<{ + stdout: string; + stderr: string; + exitCode: number; + timedOut?: boolean; + }> => ({ + stdout: "hello\n", + stderr: "", + exitCode: 0, + })), +})); + +// Import the mocked module so tests can override per-test. +import { runDocker } from "../internal/docker-cli.js"; + +const mockedRunDocker = vi.mocked(runDocker); + +beforeEach(() => { + mockedRunDocker.mockReset(); + mockedRunDocker.mockImplementation( + dockerMockFor({ stdout: "hello\n", stderr: "", exitCode: 0 }), + ); +}); + +// --- Slice D: canExecute --- + +describe("DockerExecutor.canExecute", () => { + it("returns true for docker type", () => { + const exec = new DockerExecutor(defaultConfig()); + expect(exec.canExecute(makeDockerOp())).toBe(true); + }); + + it("returns false for host type", () => { + const exec = new DockerExecutor(defaultConfig()); + expect( + exec.canExecute(makeDockerOp({ executor: { type: "host" } })), + ).toBe(false); + }); + + it("returns false for podman type", () => { + const exec = new DockerExecutor(defaultConfig()); + expect( + exec.canExecute(makeDockerOp({ executor: { type: "podman", image: "node:24" } })), + ).toBe(false); + }); + + it("returns false for remote type", () => { + const exec = new DockerExecutor(defaultConfig()); + expect( + exec.canExecute( + makeDockerOp({ + executor: { + type: "remote", + remote: { provider: "github", endpoint: "https://api.github.com" }, + }, + }), + ), + ).toBe(false); + }); +}); + +// --- Slice D: buildDockerArgs (container policy) --- + +describe("DockerExecutor.buildDockerArgs — container policy", () => { + const exec = new DockerExecutor(defaultConfig()); + + it("includes --rm", () => { + const args = exec.buildDockerArgs(makeRequest(makeDockerOp())); + expect(args).toContain("--rm"); + }); + + it("includes --read-only", () => { + const args = exec.buildDockerArgs(makeRequest(makeDockerOp())); + expect(args).toContain("--read-only"); + }); + + it("includes --cap-drop ALL", () => { + const args = exec.buildDockerArgs(makeRequest(makeDockerOp())); + expect(args).toContain("--cap-drop"); + expect(args[args.indexOf("--cap-drop") + 1]).toBe("ALL"); + }); + + it("includes --network none for deny", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp({ network: "deny" })), + ); + expect(args).toContain("--network"); + expect(args[args.indexOf("--network") + 1]).toBe("none"); + }); + + it("includes --user with runAs", () => { + const args = exec.buildDockerArgs(makeRequest(makeDockerOp())); + expect(args).toContain("--user"); + expect(args[args.indexOf("--user") + 1]).toBe("1000:1000"); + }); + + it("includes --memory from resources", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp({ resources: { cpu: "2", memory: "1Gi" } })), + ); + expect(args).toContain("--memory"); + expect(args[args.indexOf("--memory") + 1]).toBe("1Gi"); + }); + + it("includes --cpus from resources", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp({ resources: { cpu: "0.5", memory: "512Mi" } })), + ); + expect(args).toContain("--cpus"); + expect(args[args.indexOf("--cpus") + 1]).toBe("0.5"); + }); + + it("includes --timeout from timeoutSeconds", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp({ timeoutSeconds: 60 })), + ); + expect(args).toContain("--timeout"); + expect(args[args.indexOf("--timeout") + 1]).toBe("60"); + }); + + it("includes --workdir /workspace", () => { + const args = exec.buildDockerArgs(makeRequest(makeDockerOp())); + expect(args).toContain("--workdir"); + expect(args[args.indexOf("--workdir") + 1]).toBe("/workspace"); + }); + + it("mounts workspace read-only", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp(), { workspace: "/ws" }), + ); + const mount = args.find((a) => a.includes("target=/workspace")); + expect(mount).toBeDefined(); + expect(mount).toContain("source=/ws"); + expect(mount).toContain("readonly"); + }); + + it("mounts cacheDir at /cache", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp(), { cacheDir: "/cache" }), + ); + const mount = args.find((a) => a.includes("target=/cache")); + expect(mount).toBeDefined(); + expect(mount).toContain("source=/cache"); + }); + + it("mounts artifactDir at /artifacts", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp(), { artifactDir: "/art" }), + ); + const mount = args.find((a) => a.includes("target=/artifacts")); + expect(mount).toBeDefined(); + expect(mount).toContain("source=/art"); + }); + + it("never mounts the Docker socket", () => { + const args = exec.buildDockerArgs(makeRequest(makeDockerOp())); + expect(args.some((a) => a.includes("docker.sock"))).toBe(false); + }); + + it("throws ContainerPolicyError when a mount source references docker.sock", () => { + expect(() => + exec.buildDockerArgs( + makeRequest(makeDockerOp(), { workspace: "/var/run/docker.sock" }), + ), + ).toThrow(ContainerPolicyError); + }); + + it("rejects docker.sock mount sources with path traversal", () => { + expect(() => + exec.buildDockerArgs( + makeRequest(makeDockerOp(), { workspace: "/var/run/../run/docker.sock" }), + ), + ).toThrow(ContainerPolicyError); + }); + + it("uses image@digest as the image", () => { + const op = makeDockerOp({ + executor: { + type: "docker", + image: "busybox:latest", + imageDigest: "sha256:abc", + }, + }); + const args = exec.buildDockerArgs(makeRequest(op)); + expect(args).toContain("busybox:latest@sha256:abc"); + }); + + it("appends command and args", () => { + const op = makeDockerOp({ command: "echo", args: ["hi", "there"] }); + const args = exec.buildDockerArgs(makeRequest(op)); + const imgIdx = args.indexOf("busybox:latest@sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890"); + expect(args[imgIdx + 1]).toBe("echo"); + expect(args[imgIdx + 2]).toBe("hi"); + expect(args[imgIdx + 3]).toBe("there"); + }); +}); + +// --- Slice E: Network policy mapping --- + +describe("DockerExecutor.buildDockerArgs — network policy", () => { + const exec = new DockerExecutor(defaultConfig()); + + it("maps deny to --network none", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp({ network: "deny" })), + ); + expect(args).toContain("--network"); + expect(args[args.indexOf("--network") + 1]).toBe("none"); + }); + + it("maps allow-egress to default bridge (no --network none)", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp({ network: "allow-egress" })), + ); + expect(args).not.toContain("--network"); + }); + + it("maps allow-host to --network host", () => { + const args = exec.buildDockerArgs( + makeRequest(makeDockerOp({ network: "allow-host" })), + ); + expect(args).toContain("--network"); + expect(args[args.indexOf("--network") + 1]).toBe("host"); + }); +}); + +// --- Slice F: Timeout enforcement --- + +describe("DockerExecutor.execute — timeout enforcement", () => { + it("raises MISSING_TIMEOUT when timeoutSeconds is 0", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ timeoutSeconds: 0 }); + await expect(exec.execute(makeRequest(op))).rejects.toMatchObject({ + code: "MISSING_TIMEOUT", + name: "ContainerPolicyError", + }); + expect(mockedRunDocker).not.toHaveBeenCalled(); + }); + + it("raises MISSING_TIMEOUT when timeoutSeconds is negative", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ timeoutSeconds: -1 }); + await expect(exec.execute(makeRequest(op))).rejects.toMatchObject({ + code: "MISSING_TIMEOUT", + }); + expect(mockedRunDocker).not.toHaveBeenCalled(); + }); + + it("returns failure with timeout error when container times out", async () => { + mockedRunDocker.mockImplementation( + dockerMockFor({ + stdout: "", + stderr: "", + exitCode: 137, + timedOut: true, + }), + ); + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ timeoutSeconds: 1 }); + const result = await exec.execute(makeRequest(op)); + expect(result.status).toBe("failure"); + expect(result.error).toContain("timeout"); + expect(result.exitCode).toBe(137); + }); +}); + +// --- Slice G: Image digest presence --- + +describe("DockerExecutor.execute — image digest", () => { + it("raises MISSING_DIGEST when imageDigest is absent", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ + executor: { type: "docker", image: "busybox:latest" }, + }); + await expect(exec.execute(makeRequest(op))).rejects.toMatchObject({ + code: "MISSING_DIGEST", + name: "ContainerPolicyError", + }); + expect(mockedRunDocker).not.toHaveBeenCalled(); + }); +}); + +// --- Slice H: Secrets allowlist + env building --- + +describe("DockerExecutor.buildEnv — secrets allowlist", () => { + const exec = new DockerExecutor(defaultConfig()); + + it("passes only declared credentials from request.credentials", () => { + const op = makeDockerOp({ + credentials: [ + { name: "api-key", envVar: "API_KEY", required: true }, + ], + }); + const env = exec.buildEnv( + makeRequest(op, { + credentials: { API_KEY: "secret-value", OTHER: "leaked" }, + }), + ); + expect(env.API_KEY).toBe("secret-value"); + expect(env.OTHER).toBeUndefined(); + }); + + it("includes request.env vars", () => { + const env = exec.buildEnv( + makeRequest(makeDockerOp(), { env: { FOO: "bar" } }), + ); + expect(env.FOO).toBe("bar"); + }); + + it("raises UNDECLARED_SECRET for secret-like request.env not in credentials", () => { + const exec2 = new DockerExecutor(defaultConfig()); + let caught: unknown; + try { + exec2.buildEnv(makeRequest(makeDockerOp(), { env: { MY_SECRET: "s" } })); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(ContainerPolicyError); + expect((caught as ContainerPolicyError).code).toBe("UNDECLARED_SECRET"); + expect((caught as ContainerPolicyError).message).toContain("MY_SECRET"); + }); + + it("allows secret-like env var when declared in credentials", () => { + const op = makeDockerOp({ + credentials: [ + { name: "token", envVar: "API_TOKEN", required: true }, + ], + }); + const env = exec.buildEnv( + makeRequest(op, { + credentials: { API_TOKEN: "tok" }, + env: { API_TOKEN: "tok" }, + }), + ); + expect(env.API_TOKEN).toBe("tok"); + }); + + it("allows PUBLIC_KEY-like env vars without declaration", () => { + const env = exec.buildEnv( + makeRequest(makeDockerOp(), { env: { PUBLIC_KEY: "not-a-secret" } }), + ); + expect(env.PUBLIC_KEY).toBe("not-a-secret"); + }); + + it("allows env vars that merely contain KEY with extra suffix", () => { + const env = exec.buildEnv( + makeRequest(makeDockerOp(), { env: { MY_KEY_VALUE: "ok" } }), + ); + expect(env.MY_KEY_VALUE).toBe("ok"); + }); + + it("raises DOCKER_SOCKET_DENIED when env value references docker.sock", () => { + expect(() => + exec.buildEnv( + makeRequest(makeDockerOp(), { env: { PATH: "/var/run/docker.sock" } }), + ), + ).toThrow(ContainerPolicyError); + }); + + it("raises DOCKER_SOCKET_DENIED when env value uses path traversal to docker.sock", () => { + expect(() => + exec.buildEnv( + makeRequest(makeDockerOp(), { env: { PATH: "/var/run/../run/docker.sock" } }), + ), + ).toThrow(ContainerPolicyError); + }); +}); + +// --- Slice J: Logs + exit codes --- + +describe("DockerExecutor.execute — logs and exit codes", () => { + it("captures stdout and stderr into logs", async () => { + mockedRunDocker.mockImplementation( + dockerMockFor({ + stdout: "out-line\n", + stderr: "err-line\n", + exitCode: 0, + }), + ); + const exec = new DockerExecutor(defaultConfig()); + const result = await exec.execute(makeRequest(makeDockerOp())); + expect(result.logs).toContain("out-line"); + expect(result.logs).toContain("err-line"); + expect(result.status).toBe("success"); + expect(result.exitCode).toBe(0); + }); + + it("returns failure for non-zero exit", async () => { + mockedRunDocker.mockImplementation( + dockerMockFor({ + stdout: "", + stderr: "boom", + exitCode: 1, + }), + ); + const exec = new DockerExecutor(defaultConfig()); + const result = await exec.execute(makeRequest(makeDockerOp())); + expect(result.status).toBe("failure"); + expect(result.exitCode).toBe(1); + expect(result.error).toContain("exit code 1"); + }); + + it("truncates logs exceeding maxLogBytes with a notice", async () => { + mockedRunDocker.mockImplementation( + dockerMockFor({ + stdout: "A".repeat(100), + stderr: "", + exitCode: 0, + }), + ); + const exec = new DockerExecutor(defaultConfig({ maxLogBytes: 20 })); + const result = await exec.execute(makeRequest(makeDockerOp())); + expect(result.logs.length).toBeLessThanOrEqual( + 20 + "\n[log truncated]".length, + ); + expect(result.logs).toContain("[log truncated]"); + }); +}); + +// --- Slice J2: Artifact collection --- + +describe("DockerExecutor.execute — artifacts", () => { + let workspace: string; + let artifactDir: string; + + beforeEach(async () => { + workspace = await mkdtemp(join(tmpdir(), "sverka-docker-ws-")); + artifactDir = await mkdtemp(join(tmpdir(), "sverka-docker-art-")); + }); + + afterEach(async () => { + await rm(workspace, { recursive: true, force: true }); + await rm(artifactDir, { recursive: true, force: true }); + }); + + it("copies declared artifacts into artifactDir", async () => { + await mkdir(join(workspace, "out"), { recursive: true }); + await writeFile(join(workspace, "out", "report.txt"), "test report"); + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ + artifacts: [{ path: "out/report.txt", name: "report.txt", retain: true }], + }); + const result = await exec.execute( + makeRequest(op, { workspace, artifactDir }), + ); + expect(result.status).toBe("success"); + expect(result.artifacts).toHaveLength(1); + expect(result.artifacts[0]).toContain("report.txt"); + }); + + it("reports missing artifacts in error without changing status", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ + artifacts: [{ path: "nonexistent.txt", retain: true }], + }); + const result = await exec.execute( + makeRequest(op, { workspace, artifactDir }), + ); + expect(result.status).toBe("success"); + expect(result.error).toContain("missing artifact"); + expect(result.artifacts).toHaveLength(0); + }); + + it("rejects artifact names that escape artifactDir", async () => { + await mkdir(join(workspace, "out"), { recursive: true }); + await writeFile(join(workspace, "out", "report.txt"), "test report"); + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ + artifacts: [ + { path: "out/report.txt", name: "../../outside.txt", retain: true }, + ], + }); + const result = await exec.execute( + makeRequest(op, { workspace, artifactDir }), + ); + expect(result.status).toBe("success"); + expect(result.error).toContain("escapes artifactDir"); + expect(result.artifacts).toHaveLength(0); + }); + + it("rejects artifact paths that escape artifactDir", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ + artifacts: [{ path: "../../outside.txt", retain: true }], + }); + const result = await exec.execute( + makeRequest(op, { workspace, artifactDir }), + ); + expect(result.status).toBe("success"); + expect(result.error).toContain("escapes artifactDir"); + expect(result.artifacts).toHaveLength(0); + }); + + it("rejects absolute artifact paths", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ + artifacts: [{ path: "/etc/passwd", retain: true }], + }); + const result = await exec.execute( + makeRequest(op, { workspace, artifactDir }), + ); + expect(result.status).toBe("success"); + expect(result.error).toContain("must not be absolute"); + expect(result.artifacts).toHaveLength(0); + }); +}); + +// --- Edge cases --- + +describe("DockerExecutor.execute — edge cases", () => { + it("raises WRONG_EXECUTOR_TYPE for non-docker operation", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ executor: { type: "host" } }); + await expect(exec.execute(makeRequest(op))).rejects.toMatchObject({ + code: "WRONG_EXECUTOR_TYPE", + }); + expect(mockedRunDocker).not.toHaveBeenCalled(); + }); + + it("dispose is a no-op", async () => { + const exec = new DockerExecutor(defaultConfig()); + await expect(exec.dispose()).resolves.toBeUndefined(); + }); +}); diff --git a/packages/runtime-docker/src/__tests__/errors.test.ts b/packages/runtime-docker/src/__tests__/errors.test.ts new file mode 100644 index 000000000..4430f9442 --- /dev/null +++ b/packages/runtime-docker/src/__tests__/errors.test.ts @@ -0,0 +1,57 @@ +import { describe, it, expect } from "vitest"; +import { + DockerExecutorError, + ImageDigestError, + ContainerPolicyError, +} from "../errors.js"; + +describe("DockerExecutorError", () => { + it("sets name, code, and context", () => { + const err = new DockerExecutorError("boom", "BOOM", { key: "value" }); + expect(err).toBeInstanceOf(Error); + expect(err.name).toBe("DockerExecutorError"); + expect(err.code).toBe("BOOM"); + expect(err.message).toBe("boom"); + expect(err.context).toEqual({ key: "value" }); + }); + + it("context is optional", () => { + const err = new DockerExecutorError("boom", "BOOM"); + expect(err.context).toBeUndefined(); + }); +}); + +describe("ImageDigestError", () => { + it("extends DockerExecutorError with code IMAGE_DIGEST_MISMATCH", () => { + const err = new ImageDigestError("digest mismatch", { + expected: "sha256:aaa", + actual: "sha256:bbb", + }); + expect(err).toBeInstanceOf(DockerExecutorError); + expect(err).toBeInstanceOf(Error); + expect(err.name).toBe("ImageDigestError"); + expect(err.code).toBe("IMAGE_DIGEST_MISMATCH"); + expect(err.context).toEqual({ expected: "sha256:aaa", actual: "sha256:bbb" }); + }); + + it("context is optional", () => { + const err = new ImageDigestError("digest mismatch"); + expect(err.context).toBeUndefined(); + }); +}); + +describe("ContainerPolicyError", () => { + it("extends DockerExecutorError with code CONTAINER_POLICY_VIOLATION", () => { + const err = new ContainerPolicyError("policy violated", { rule: "no-net" }); + expect(err).toBeInstanceOf(DockerExecutorError); + expect(err).toBeInstanceOf(Error); + expect(err.name).toBe("ContainerPolicyError"); + expect(err.code).toBe("CONTAINER_POLICY_VIOLATION"); + expect(err.context).toEqual({ rule: "no-net" }); + }); + + it("context is optional", () => { + const err = new ContainerPolicyError("policy violated"); + expect(err.context).toBeUndefined(); + }); +}); diff --git a/packages/runtime-docker/src/__tests__/helpers/fixtures.ts b/packages/runtime-docker/src/__tests__/helpers/fixtures.ts new file mode 100644 index 000000000..b9f45409a --- /dev/null +++ b/packages/runtime-docker/src/__tests__/helpers/fixtures.ts @@ -0,0 +1,64 @@ +import type { PlanOperation } from "@sverka/ir"; +import type { ExecuteRequest } from "@sverka/runtime"; +import type { DockerExecutorConfig } from "../../config.js"; + +/** + * Build a minimal PlanOperation with executor.type: "docker". + * Override any field via `overrides`. + */ +export function makeDockerOp( + overrides: Partial = {}, +): PlanOperation { + const base: PlanOperation = { + id: "op-docker-1", + kind: "run", + name: "docker-check", + dependsOn: [], + executor: { + type: "docker", + image: "busybox:latest", + imageDigest: "sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890", + }, + command: "echo", + args: ["hello"], + resources: { cpu: "1", memory: "512Mi" }, + network: "deny", + credentials: [], + artifacts: [], + retry: { maxAttempts: 1, backoffSeconds: 0, retryOn: ["failure"] }, + timeoutSeconds: 30, + continueOnError: false, + }; + return { ...base, ...overrides }; +} + +/** + * Build an ExecuteRequest with a temp workspace, env, credentials, etc. + */ +export function makeRequest( + operation: PlanOperation, + overrides: Partial = {}, +): ExecuteRequest { + const base: ExecuteRequest = { + operation, + workspace: "/tmp/sverka-test-workspace", + env: {}, + credentials: {}, + cacheDir: "/tmp/sverka-test-cache", + artifactDir: "/tmp/sverka-test-artifacts", + }; + return { ...base, ...overrides }; +} + +/** + * A default DockerExecutorConfig with a non-root runAs and a cache dir. + */ +export function defaultConfig( + overrides: Partial = {}, +): DockerExecutorConfig { + const base: DockerExecutorConfig = { + runAs: "1000:1000", + cacheDir: "/tmp/sverka-test-cache", + }; + return { ...base, ...overrides }; +} diff --git a/packages/runtime-docker/src/__tests__/image.test.ts b/packages/runtime-docker/src/__tests__/image.test.ts new file mode 100644 index 000000000..41f75a627 --- /dev/null +++ b/packages/runtime-docker/src/__tests__/image.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { verifyImageDigest } from "../image.js"; +import { ImageDigestError } from "../errors.js"; +import { defaultConfig } from "./helpers/fixtures.js"; + +vi.mock("../internal/docker-cli.js", () => ({ + runDocker: vi.fn(), +})); + +import { runDocker } from "../internal/docker-cli.js"; + +const mockedRunDocker = vi.mocked(runDocker); + +beforeEach(() => { + mockedRunDocker.mockReset(); +}); + +describe("verifyImageDigest", () => { + const config = defaultConfig(); + const image = "busybox:latest"; + const digest = "sha256:abc123"; + + it("resolves when local image digest matches", async () => { + mockedRunDocker.mockResolvedValue({ + stdout: JSON.stringify(["busybox:latest@sha256:abc123"]), + stderr: "", + exitCode: 0, + }); + await expect(verifyImageDigest(image, digest, config)).resolves.toBeUndefined(); + // Should have called docker inspect only (no pull). + expect(mockedRunDocker).toHaveBeenCalledTimes(1); + const callArgs = mockedRunDocker.mock.calls[0]?.[0]; + expect(callArgs?.[0]).toBe("inspect"); + }); + + it("throws ImageDigestError on digest mismatch", async () => { + mockedRunDocker.mockResolvedValue({ + stdout: JSON.stringify(["sha256:wrongdigest"]), + stderr: "", + exitCode: 0, + }); + await expect(verifyImageDigest(image, digest, config)).rejects.toThrow( + ImageDigestError, + ); + await expect(verifyImageDigest(image, digest, config)).rejects.toMatchObject({ + code: "IMAGE_DIGEST_MISMATCH", + context: { image, expected: digest, actual: ["sha256:wrongdigest"] }, + }); + }); + + it("pulls the image when inspect fails, then verifies", async () => { + // inspect fails → pull succeeds → inspect again matches. + mockedRunDocker + .mockResolvedValueOnce({ + stdout: "", + stderr: "No such image", + exitCode: 1, + }) + .mockResolvedValueOnce({ + stdout: "", + stderr: "", + exitCode: 0, + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify(["busybox:latest@sha256:abc123"]), + stderr: "", + exitCode: 0, + }); + await expect(verifyImageDigest(image, digest, config)).resolves.toBeUndefined(); + expect(mockedRunDocker).toHaveBeenCalledTimes(3); + expect(mockedRunDocker.mock.calls[0]?.[0]?.[0]).toBe("inspect"); + expect(mockedRunDocker.mock.calls[1]?.[0]?.[0]).toBe("pull"); + expect(mockedRunDocker.mock.calls[2]?.[0]?.[0]).toBe("inspect"); + }); + + it("throws ImageDigestError after pull if digest still mismatches", async () => { + mockedRunDocker + .mockResolvedValueOnce({ + stdout: "", + stderr: "No such image", + exitCode: 1, + }) + .mockResolvedValueOnce({ + stdout: "", + stderr: "", + exitCode: 0, + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify(["sha256:wrongdigest"]), + stderr: "", + exitCode: 0, + }); + await expect(verifyImageDigest(image, digest, config)).rejects.toThrow( + ImageDigestError, + ); + }); +}); diff --git a/packages/runtime-docker/src/__tests__/integration.test.ts b/packages/runtime-docker/src/__tests__/integration.test.ts new file mode 100644 index 000000000..7d4a737e1 --- /dev/null +++ b/packages/runtime-docker/src/__tests__/integration.test.ts @@ -0,0 +1,46 @@ +import { describe, it, expect } from "vitest"; +import { DockerExecutor } from "../docker-executor.js"; +import { defaultConfig, makeDockerOp, makeRequest } from "./helpers/fixtures.js"; + +// Integration tests require a real Docker daemon. Skipped by default. +// Run with: SVERKA_DOCKER=1 bun run test +const enabled = Boolean( + process.env.SVERKA_DOCKER && process.env.SVERKA_BUSYBOX_DIGEST, +); + +describe.skipIf(!enabled)("DockerExecutor integration", () => { + it("runs echo hello in busybox and returns success", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ + executor: { + type: "docker", + image: "busybox:latest", + // Replace with a real digest when running integration tests. + imageDigest: process.env.SVERKA_BUSYBOX_DIGEST ?? "sha256:dummy", + }, + command: "echo", + args: ["hello"], + timeoutSeconds: 30, + }); + const result = await exec.execute(makeRequest(op)); + expect(result.status).toBe("success"); + expect(result.logs).toContain("hello"); + }); + + it("returns failure for a command that exits 1", async () => { + const exec = new DockerExecutor(defaultConfig()); + const op = makeDockerOp({ + executor: { + type: "docker", + image: "busybox:latest", + imageDigest: process.env.SVERKA_BUSYBOX_DIGEST ?? "sha256:dummy", + }, + command: "sh", + args: ["-c", "exit 1"], + timeoutSeconds: 30, + }); + const result = await exec.execute(makeRequest(op)); + expect(result.status).toBe("failure"); + expect(result.exitCode).toBe(1); + }); +}); diff --git a/packages/runtime-docker/src/__tests__/public-api.test.ts b/packages/runtime-docker/src/__tests__/public-api.test.ts new file mode 100644 index 000000000..6770ac439 --- /dev/null +++ b/packages/runtime-docker/src/__tests__/public-api.test.ts @@ -0,0 +1,51 @@ +import { describe, it, expect } from "vitest"; +import { + DockerExecutor, + verifyImageDigest, + DockerCacheManager, + DockerExecutorError, + ImageDigestError, + ContainerPolicyError, +} from "../index.js"; +import type { DockerExecutorConfig, CacheManager } from "../index.js"; + +describe("public API", () => { + it("exports DockerExecutor class", () => { + expect(typeof DockerExecutor).toBe("function"); + const exec = new DockerExecutor({ + runAs: "1000:1000", + cacheDir: "/tmp/sverka-cache", + }); + expect(exec.name).toBe("docker"); + expect(typeof exec.canExecute).toBe("function"); + expect(typeof exec.execute).toBe("function"); + expect(typeof exec.dispose).toBe("function"); + }); + + it("exports verifyImageDigest function", () => { + expect(typeof verifyImageDigest).toBe("function"); + }); + + it("exports DockerCacheManager class", () => { + expect(typeof DockerCacheManager).toBe("function"); + const mgr = new DockerCacheManager("/tmp/sverka-cache"); + expect(typeof mgr.prepare).toBe("function"); + expect(typeof mgr.collect).toBe("function"); + }); + + it("exports error classes", () => { + expect(new DockerExecutorError("x", "X")).toBeInstanceOf(Error); + expect(new ImageDigestError("x")).toBeInstanceOf(DockerExecutorError); + expect(new ContainerPolicyError("x")).toBeInstanceOf(DockerExecutorError); + }); + + it("exports types (compile-time check)", () => { + const config: DockerExecutorConfig = { + runAs: "1000:1000", + cacheDir: "/tmp/sverka-cache", + }; + const mgr: CacheManager = new DockerCacheManager("/tmp/sverka-cache"); + expect(config.runAs).toBe("1000:1000"); + expect(typeof mgr.prepare).toBe("function"); + }); +}); diff --git a/packages/runtime-docker/src/cache.ts b/packages/runtime-docker/src/cache.ts new file mode 100644 index 000000000..99d2d34da --- /dev/null +++ b/packages/runtime-docker/src/cache.ts @@ -0,0 +1,123 @@ +import { copyFile, mkdir, stat } from "node:fs/promises"; +import { dirname, isAbsolute, join, normalize, relative, resolve } from "node:path"; +import { DockerExecutorError } from "./errors.js"; + +/** + * Manages cache inputs and outputs for Docker execution. Cache directories + * are bind-mounted into the container at `/cache`. + */ +export interface CacheManager { + /** Prepare a cache directory for an operation's declared inputs. */ + prepare( + inputs: readonly string[], + key: string, + workspace?: string, + ): Promise; + /** Collect cache outputs after execution. */ + collect(outputs: readonly string[], sourceDir: string, key: string): Promise; +} + +/** + * Filesystem-backed cache manager. `prepare` creates `/` and + * copies declared inputs into it. `collect` copies declared outputs from the + * execution `sourceDir` back into the same `/` directory, + * preserving relative path structure. + */ +export class DockerCacheManager implements CacheManager { + constructor(private readonly cacheDir: string) {} + + async prepare( + inputs: readonly string[], + key: string, + workspace?: string, + ): Promise { + const target = this.resolveCachePath(key); + await mkdir(target, { recursive: true }); + for (const input of inputs) { + const root = workspace ?? dirname(input); + const rel = isAbsolute(input) ? relative(root, input) : input; + if (rel.startsWith("..") || isAbsolute(rel)) { + throw new DockerExecutorError( + `cache input "${input}" escapes workspace "${root}"`, + "CACHE_PATH_ESCAPE", + ); + } + const dest = join(target, rel); + await mkdir(dirname(dest), { recursive: true }); + // Only copy if the source exists; if not, the cached copy may already + // be present from a prior run (restore-from-cache semantics). + try { + await stat(input); + await copyFile(input, dest); + } catch { + // Source missing — rely on existing cached copy (if any). + } + } + return target; + } + + async collect( + outputs: readonly string[], + sourceDir: string, + key: string, + ): Promise { + const target = this.resolveCachePath(key); + for (const rawOutput of outputs) { + const output = isAbsolute(rawOutput) + ? relative(sourceDir, rawOutput) + : rawOutput; + if (output.startsWith("..")) { + throw new DockerExecutorError( + `cache output "${rawOutput}" escapes sourceDir "${sourceDir}"`, + "CACHE_PATH_ESCAPE", + ); + } + const src = resolve(sourceDir, output); + this.assertInsideDir(src, sourceDir, `cache output "${rawOutput}"`); + const dest = resolve(target, output); + if (normalize(src) === normalize(dest)) { + continue; + } + await mkdir(dirname(dest), { recursive: true }); + try { + await copyFile(src, dest); + } catch (e) { + // A missing or unreadable cache output should not abort the whole + // execution; continue collecting the remaining outputs. + if ( + e instanceof Error && + "code" in e && + (e as { code: string }).code === "ENOENT" + ) { + continue; + } + throw new DockerExecutorError( + `failed to collect cache output "${rawOutput}": ${e instanceof Error ? e.message : String(e)}`, + "CACHE_COLLECT_FAILED", + { output: rawOutput, source: src, dest }, + ); + } + } + } + + private resolveCachePath(key: string): string { + if (isAbsolute(key)) { + throw new DockerExecutorError( + `absolute cache key "${key}" is not allowed`, + "CACHE_KEY_ESCAPE", + ); + } + const target = resolve(this.cacheDir, key); + this.assertInsideDir(target, this.cacheDir, `cache key "${key}"`); + return target; + } + + private assertInsideDir(path: string, root: string, what: string): void { + const rel = relative(resolve(root), path); + if (rel === "" || (!rel.startsWith("..") && !isAbsolute(rel))) return; + throw new DockerExecutorError( + `${what} escapes cacheDir "${root}"`, + "CACHE_PATH_ESCAPE", + ); + } +} diff --git a/packages/runtime-docker/src/config.ts b/packages/runtime-docker/src/config.ts new file mode 100644 index 000000000..62d29ec3a --- /dev/null +++ b/packages/runtime-docker/src/config.ts @@ -0,0 +1,18 @@ +/** + * Configuration for the Docker executor. + * + * `workspace` and `artifactDir` are NOT here — they arrive per-execution via + * `ExecuteRequest`. This config is executor-wide. + */ +export interface DockerExecutorConfig { + /** Path to the Docker CLI. Defaults to auto-detect ("docker"). */ + readonly dockerPath?: string; + /** Docker host socket URL (DOCKER_HOST). Defaults to inherited. */ + readonly dockerHost?: string; + /** Default non-root uid:gid for containers. Defaults to "1000:1000". */ + readonly runAs?: string; + /** Persistent directory for cache layers (managed by DockerCacheManager). */ + readonly cacheDir: string; + /** Maximum log size in bytes before truncation. Defaults to 10 MiB. */ + readonly maxLogBytes?: number; +} diff --git a/packages/runtime-docker/src/docker-executor.ts b/packages/runtime-docker/src/docker-executor.ts new file mode 100644 index 000000000..8ae294d3f --- /dev/null +++ b/packages/runtime-docker/src/docker-executor.ts @@ -0,0 +1,377 @@ +import { realpathSync } from "node:fs"; +import { copyFile, mkdir } from "node:fs/promises"; +import { basename, dirname, isAbsolute, join, normalize, relative, resolve } from "node:path"; +import type { Executor, ExecuteRequest, ExecuteResult } from "@sverka/runtime"; +import type { PlanOperation } from "@sverka/ir"; +import type { DockerExecutorConfig } from "./config.js"; +import { ContainerPolicyError, DockerExecutorError } from "./errors.js"; +import { verifyImageDigest } from "./image.js"; +import { DockerCacheManager } from "./cache.js"; +import { runDocker } from "./internal/docker-cli.js"; + +const DEFAULT_MAX_LOG_BYTES = 10 * 1024 * 1024; // 10 MiB +const DEFAULT_RUN_AS = "1000:1000"; +const TRUNCATION_NOTICE = "\n[log truncated]"; + +/** Resolve and normalize a path, following symlinks when possible. */ +function canonicalPath(raw: string): string { + const absolute = isAbsolute(raw) ? raw : resolve(raw); + try { + return normalize(realpathSync(absolute)); + } catch { + return normalize(absolute); + } +} + +/** Return true when the canonical path points to a Docker socket. */ +function refersToDockerSocket(raw: string): boolean { + return basename(canonicalPath(raw)) === "docker.sock"; +} + +/** Extract the `source=` value from a Docker `--mount` string. */ +function mountSource(mount: string): string | undefined { + const match = mount.match(/(?:^|,)source=([^,]+)/); + return match?.[1]; +} + +/** + * Docker implementation of the Executor interface. + * + * Enforces a strict container execution policy: read-only root filesystem, + * dropped capabilities, no network by default, non-root user, bounded CPU and + * memory, mandatory timeout, secrets allowlist, and the Docker socket is never + * mounted into the container. + */ +export class DockerExecutor implements Executor { + readonly name = "docker"; + private readonly config: DockerExecutorConfig; + private readonly maxLogBytes: number; + private readonly cacheManager: DockerCacheManager | null; + + constructor(config: DockerExecutorConfig) { + this.config = config; + this.maxLogBytes = config.maxLogBytes ?? DEFAULT_MAX_LOG_BYTES; + this.cacheManager = config.cacheDir + ? new DockerCacheManager(config.cacheDir) + : null; + } + + private get runAs(): string { + return this.config.runAs ?? DEFAULT_RUN_AS; + } + + canExecute(operation: PlanOperation): boolean { + return operation.executor.type === "docker"; + } + + /** + * Construct the `docker run` argument array. Pure — no side effects. + * Throws `ContainerPolicyError` (DOCKER_SOCKET_DENIED) if any mount source + * references the Docker socket. + */ + buildDockerArgs(request: ExecuteRequest, cachePath?: string): string[] { + const op = request.operation; + const args = this.buildBaseArgs(op); + const netFlag = this.networkFlag(op.network); + if (netFlag !== undefined) args.push("--network", netFlag); + args.push(...this.buildMountArgs(request, cachePath)); + const env = this.buildEnv(request); + for (const [k, v] of Object.entries(env)) args.push("--env", `${k}=${v}`); + args.push(this.imageRef(op)); + if (op.command !== undefined) args.push(op.command); + if (op.args !== undefined) args.push(...op.args); + return args; + } + + private buildBaseArgs(op: PlanOperation): string[] { + return [ + "run", + "--rm", + "--read-only", + "--cap-drop", + "ALL", + "--user", + this.runAs, + "--memory", + op.resources.memory, + "--cpus", + op.resources.cpu, + "--timeout", + String(op.timeoutSeconds), + "--workdir", + "/workspace", + ]; + } + + private buildMountArgs(request: ExecuteRequest, cachePath?: string): string[] { + const workspaceMount = `type=bind,source=${request.workspace},target=/workspace,readonly`; + const cacheSource = cachePath ?? request.cacheDir; + const cacheMount = `type=bind,source=${cacheSource},target=/cache`; + const artifactMount = `type=bind,source=${request.artifactDir},target=/artifacts`; + const mounts: string[] = []; + for (const mount of [workspaceMount, cacheMount, artifactMount]) { + const source = mountSource(mount); + if (source && refersToDockerSocket(source)) { + throw new ContainerPolicyError( + "Docker socket must not be mounted into the container", + { mount, source: canonicalPath(source) }, + "DOCKER_SOCKET_DENIED", + ); + } + mounts.push("--mount", mount); + } + return mounts; + } + + /** + * Build the container environment from `operation.credentials` (declarations) + * + `request.credentials` (values) + `request.env`. Pure. + * + * Throws `ContainerPolicyError` (UNDECLARED_SECRET) if a secret-like env var + * in `request.env` is not declared in `operation.credentials`, and + * (DOCKER_SOCKET_DENIED) if any env value references the Docker socket. + */ + buildEnv(request: ExecuteRequest): Record { + const op = request.operation; + const declaredEnvVars = new Set(op.credentials.map((c) => c.envVar)); + const env: Record = {}; + + // Only declared credentials get values from request.credentials. + for (const decl of op.credentials) { + const val = request.credentials[decl.envVar]; + if (val !== undefined) { + env[decl.envVar] = val; + } + } + + // request.env provides operation env vars, but secret-like names must be + // declared in credentials. + for (const [k, v] of Object.entries(request.env)) { + if (SECRET_DENYLIST.test(k) && !declaredEnvVars.has(k)) { + throw new ContainerPolicyError( + `env var "${k}" looks like a secret but is not declared in operation.credentials`, + { envVar: k }, + "UNDECLARED_SECRET", + ); + } + if (typeof v === "string" && refersToDockerSocket(v)) { + throw new ContainerPolicyError( + "Docker socket must not be referenced in env values", + { envVar: k, value: canonicalPath(v) }, + "DOCKER_SOCKET_DENIED", + ); + } + env[k] = v; + } + + return env; + } + + async execute(request: ExecuteRequest): Promise { + const op = request.operation; + const start = Date.now(); + this.validateRequest(op); + + let cachePath: string | undefined; + if (op.cache && this.cacheManager) { + cachePath = await this.cacheManager.prepare( + op.cache.inputs, + op.cache.key, + request.workspace, + ); + } + + const image = op.executor.image ?? ""; + const digest = op.executor.imageDigest; + if (digest !== undefined) { + await verifyImageDigest(image, digest, this.config); + } + + const args = this.buildDockerArgs(request, cachePath); + const result = await this.runContainer(args, op); + + if (op.cache && this.cacheManager && cachePath !== undefined) { + await this.cacheManager.collect(op.cache.outputs, cachePath, op.cache.key); + } + + return this.finalizeResult(result, op, request, start); + } + + /** Validate executor type, timeout, and image digest. */ + private validateRequest(op: PlanOperation): void { + if (op.executor.type !== "docker") { + throw new ContainerPolicyError( + `expected executor.type "docker", got "${op.executor.type}"`, + { type: op.executor.type }, + "WRONG_EXECUTOR_TYPE", + ); + } + if (op.timeoutSeconds === undefined || op.timeoutSeconds <= 0) { + throw new ContainerPolicyError( + "timeoutSeconds must be present and > 0", + { timeoutSeconds: op.timeoutSeconds }, + "MISSING_TIMEOUT", + ); + } + if (op.executor.imageDigest === undefined) { + throw new ContainerPolicyError( + "docker operations require executor.imageDigest", + { image: op.executor.image }, + "MISSING_DIGEST", + ); + } + } + + /** Spawn docker and build the base ExecuteResult. */ + private async runContainer( + args: string[], + op: PlanOperation, + ): Promise { + const start = Date.now(); + const result = await runDocker(args, { + timeoutSeconds: op.timeoutSeconds, + maxLogBytes: Math.max(0, this.maxLogBytes - TRUNCATION_NOTICE.length), + ...(this.config.dockerPath !== undefined + ? { dockerPath: this.config.dockerPath } + : {}), + ...(this.config.dockerHost !== undefined + ? { dockerHost: this.config.dockerHost } + : {}), + }); + const durationMs = Date.now() - start; + const rawLogs = + result.stdout + (result.stderr ? "\n" + result.stderr : ""); + const logs = this.truncateLogs(rawLogs); + if (result.timedOut === true) { + return { + operationId: op.id, + status: "failure", + ...(result.exitCode >= 0 ? { exitCode: result.exitCode } : {}), + durationMs, + logs, + artifacts: [], + error: `timeout after ${op.timeoutSeconds}s`, + }; + } + const status = result.exitCode === 0 ? "success" : "failure"; + return { + operationId: op.id, + status, + ...(result.exitCode >= 0 ? { exitCode: result.exitCode } : {}), + durationMs, + logs, + artifacts: [], + ...(status === "failure" ? { error: `exit code ${result.exitCode}` } : {}), + }; + } + + /** Collect artifacts and merge errors into the base result. */ + private async finalizeResult( + base: ExecuteResult, + op: PlanOperation, + request: ExecuteRequest, + start: number, + ): Promise { + const artifacts = await this.collectArtifacts( + op, + request.workspace, + request.artifactDir, + ); + const durationMs = Date.now() - start; + if (artifacts.errors.length > 0) { + const existingError = base.error ?? ""; + const artifactError = `artifact errors: ${artifacts.errors.join("; ")}`; + return { + ...base, + durationMs, + artifacts: artifacts.collected, + error: existingError + ? `${existingError}; ${artifactError}` + : artifactError, + }; + } + return { ...base, durationMs, artifacts: artifacts.collected }; + } + + async dispose(): Promise { + // No persistent resources to clean up. + } + + // --- internals --- + + private networkFlag(network: PlanOperation["network"]): string | undefined { + switch (network) { + case "deny": + return "none"; + case "allow-host": + return "host"; + case "allow-egress": + return undefined; // default bridge + default: + return "none"; + } + } + + private imageRef(op: PlanOperation): string { + const image = op.executor.image ?? ""; + const digest = op.executor.imageDigest; + if (digest !== undefined) { + return `${image}@${digest}`; + } + return image; + } + + private truncateLogs(logs: string): string { + if (logs.length <= this.maxLogBytes) return logs; + if (this.maxLogBytes <= TRUNCATION_NOTICE.length) { + return TRUNCATION_NOTICE.slice(0, this.maxLogBytes); + } + return ( + logs.slice(0, this.maxLogBytes - TRUNCATION_NOTICE.length) + + TRUNCATION_NOTICE + ); + } + + private async collectArtifacts( + op: PlanOperation, + workspace: string, + artifactDir: string, + ): Promise<{ collected: string[]; errors: string[] }> { + const collected: string[] = []; + const errors: string[] = []; + const artifactRoot = resolve(artifactDir); + + for (const artifact of op.artifacts) { + if (isAbsolute(artifact.path)) { + errors.push( + `artifact path must not be absolute: ${artifact.path}`, + ); + continue; + } + const src = join(workspace, artifact.path); + const rel = artifact.name ?? artifact.path; + const dest = resolve(artifactDir, rel); + if (!this.isInsideDir(dest, artifactRoot)) { + errors.push(`artifact destination escapes artifactDir: ${rel}`); + continue; + } + try { + await mkdir(dirname(dest), { recursive: true }); + await copyFile(src, dest); + collected.push(dest); + } catch { + errors.push(`missing artifact: ${artifact.path}`); + } + } + + return { collected, errors }; + } + + private isInsideDir(path: string, root: string): boolean { + const rel = relative(root, path); + return rel === "" || (!rel.startsWith("..") && !isAbsolute(rel)); + } +} + +const SECRET_DENYLIST = + /^(?:.*_)?(?:SECRET|TOKEN|PASSWORD|CREDENTIAL|(?, + ) { + super(message); + this.name = "DockerExecutorError"; + } +} + +/** Raised when image digest verification fails. */ +export class ImageDigestError extends DockerExecutorError { + constructor(message: string, context?: Record) { + super(message, "IMAGE_DIGEST_MISMATCH", context); + this.name = "ImageDigestError"; + } +} + +/** Raised when a container policy violation is attempted. */ +export class ContainerPolicyError extends DockerExecutorError { + constructor( + message: string, + context?: Record, + code = "CONTAINER_POLICY_VIOLATION", + ) { + super(message, code, context); + this.name = "ContainerPolicyError"; + } +} diff --git a/packages/runtime-docker/src/image.ts b/packages/runtime-docker/src/image.ts new file mode 100644 index 000000000..2bd24ba04 --- /dev/null +++ b/packages/runtime-docker/src/image.ts @@ -0,0 +1,103 @@ +import type { DockerRunOptions } from "./internal/docker-cli.js"; +import type { DockerExecutorConfig } from "./config.js"; +import { ImageDigestError } from "./errors.js"; +import { runDocker } from "./internal/docker-cli.js"; + +type DockerResult = Awaited>; + +function dockerOptions(config: DockerExecutorConfig): DockerRunOptions { + return { + timeoutSeconds: 300, + ...(config.dockerPath !== undefined ? { dockerPath: config.dockerPath } : {}), + ...(config.dockerHost !== undefined ? { dockerHost: config.dockerHost } : {}), + }; +} + +function dockerInspect(image: string, opts: DockerRunOptions) { + return runDocker(["inspect", "--format={{json .RepoDigests}}", image], opts); +} + +function dockerPull(image: string, opts: DockerRunOptions) { + return runDocker(["pull", image], opts); +} + +function assertInspectOk( + result: DockerResult, + image: string, + expectedDigest: string, + phase: string, +): void { + if (result.timedOut) { + throw new ImageDigestError( + `timed out inspecting image "${image}" ${phase}`, + { image, expected: expectedDigest }, + ); + } + if (result.exitCode !== 0) { + const detail = result.stderr.trim() || result.stdout.trim() || "unknown error"; + throw new ImageDigestError( + `image "${image}" inspect failed ${phase}: ${detail}`, + { image, expected: expectedDigest }, + ); + } +} + +/** + * Verify that the locally available image digest matches the declared digest. + * Pulls the image if not present. Throws `ImageDigestError` on mismatch. + * + * Uses `docker inspect --format='{{json .RepoDigests}}' ` to read the + * registry manifest digests for the image. If the image is absent (inspect exits + * non-zero), runs `docker pull ` then inspects again. + */ +export async function verifyImageDigest( + image: string, + expectedDigest: string, + config: DockerExecutorConfig, +): Promise { + const opts = dockerOptions(config); + + let inspectResult = await dockerInspect(image, opts); + + if (inspectResult.exitCode !== 0) { + const pullResult = await dockerPull(image, opts); + if (pullResult.timedOut) { + throw new ImageDigestError( + `timed out pulling image "${image}"`, + { image, expected: expectedDigest }, + ); + } + if (pullResult.exitCode !== 0) { + const detail = pullResult.stderr.trim() || pullResult.stdout.trim() || "unknown error"; + throw new ImageDigestError( + `failed to pull image "${image}": ${detail}`, + { image, expected: expectedDigest }, + ); + } + inspectResult = await dockerInspect(image, opts); + } + + assertInspectOk(inspectResult, image, expectedDigest, inspectResult.exitCode === 0 ? "" : "after pull"); + + const repoDigests = parseRepoDigests(inspectResult.stdout.trim()); + if (!repoDigests.some((d) => d === expectedDigest || d.endsWith(`@${expectedDigest}`))) { + throw new ImageDigestError( + `image digest mismatch for "${image}": expected ${expectedDigest}, got ${repoDigests.join(", ") || "none"}`, + { image, expected: expectedDigest, actual: repoDigests }, + ); + } +} + +function parseRepoDigests(raw: string): string[] { + if (raw === "" || raw === "null") return []; + try { + const parsed = JSON.parse(raw) as unknown; + if (Array.isArray(parsed)) return parsed.map((d) => String(d)); + } catch { + // Fall through to line-split fallback. + } + return raw + .split(/\r?\n/) + .map((line) => line.trim()) + .filter((line) => line.length > 0); +} diff --git a/packages/runtime-docker/src/index.ts b/packages/runtime-docker/src/index.ts index e21ab6f8f..a452baa54 100644 --- a/packages/runtime-docker/src/index.ts +++ b/packages/runtime-docker/src/index.ts @@ -1 +1,8 @@ // @sverka/runtime-docker — public API + +export { DockerExecutor } from "./docker-executor.js"; +export { type DockerExecutorConfig } from "./config.js"; +export { verifyImageDigest } from "./image.js"; +export { type CacheManager, DockerCacheManager } from "./cache.js"; +export { DockerExecutorError, ImageDigestError, ContainerPolicyError } + from "./errors.js"; diff --git a/packages/runtime-docker/src/internal/docker-cli.ts b/packages/runtime-docker/src/internal/docker-cli.ts new file mode 100644 index 000000000..f478999bc --- /dev/null +++ b/packages/runtime-docker/src/internal/docker-cli.ts @@ -0,0 +1,144 @@ +import { spawn } from "node:child_process"; + +/** + * Result of a Docker CLI invocation. Not exported from the public API. + */ +export interface DockerCommandResult { + readonly stdout: string; + readonly stderr: string; + readonly exitCode: number; + readonly timedOut?: boolean; +} + +/** Options for runDocker. Not exported from the public API. */ +export interface DockerRunOptions { + /** Timeout in seconds. The container is killed if it exceeds this. */ + readonly timeoutSeconds: number; + /** Path to the Docker CLI binary. Defaults to "docker". */ + readonly dockerPath?: string; + /** DOCKER_HOST override. */ + readonly dockerHost?: string; + /** Optional output byte limit. Excess output is dropped and marked truncated. */ + readonly maxLogBytes?: number; +} + +const GRACE_PERIOD_MS = 2000; +const TRUNCATION_NOTICE = "\n[log truncated]"; + +/** + * Run a Docker CLI command with the given args. Captures stdout/stderr, + * resolves the exit code, and enforces a timeout via SIGTERM + SIGKILL grace. + * + * This is the single side-effectful seam: unit tests mock it via `vi.mock`, + * integration tests use the real implementation. + */ +export function runDocker( + args: readonly string[], + opts: DockerRunOptions, +): Promise { + return new Promise((resolvePromise) => { + const binary = opts.dockerPath ?? "docker"; + const env: Record = { ...process.env } as Record< + string, + string + >; + if (opts.dockerHost !== undefined) { + env.DOCKER_HOST = opts.dockerHost; + } + + let stdout = ""; + let stderr = ""; + let timedOut = false; + let truncated = false; + let byteLimit = opts.maxLogBytes; + + const child = spawn(binary, [...args], { + stdio: ["ignore", "pipe", "pipe"], + env, + }); + + let closed = false; + let graceTimer: ReturnType | undefined; + + const timer = setTimeout(() => { + timedOut = true; + child.kill("SIGTERM"); + graceTimer = setTimeout(() => { + if (!closed) { + child.kill("SIGKILL"); + } + }, GRACE_PERIOD_MS); + }, opts.timeoutSeconds * 1000); + + function appendBytes(target: "stdout" | "stderr", data: Buffer): void { + if (truncated) return; + if (byteLimit === undefined) { + if (target === "stdout") { + stdout += data.toString(); + } else { + stderr += data.toString(); + } + return; + } + const incoming = data.length; + if (incoming <= byteLimit) { + if (target === "stdout") { + stdout += data.toString(); + } else { + stderr += data.toString(); + } + byteLimit -= incoming; + if (byteLimit === 0) { + truncated = true; + } + return; + } + const allowed = byteLimit; + const slice = data.subarray(0, allowed); + if (target === "stdout") { + stdout += slice.toString(); + } else { + stderr += slice.toString(); + } + byteLimit = 0; + truncated = true; + } + + child.stdout?.on("data", (data: Buffer) => { + appendBytes("stdout", data); + }); + child.stderr?.on("data", (data: Buffer) => { + appendBytes("stderr", data); + }); + + child.on("error", (err) => { + closed = true; + if (graceTimer) clearTimeout(graceTimer); + clearTimeout(timer); + if (truncated) { + stdout += TRUNCATION_NOTICE; + } + resolvePromise({ + stdout, + stderr: `spawn error: ${err.message}`, + exitCode: -1, + ...(timedOut ? { timedOut: true } : {}), + }); + }); + + child.on("close", (code) => { + closed = true; + if (graceTimer) clearTimeout(graceTimer); + clearTimeout(timer); + if (truncated) { + stdout += TRUNCATION_NOTICE; + } + resolvePromise({ + stdout, + stderr, + exitCode: code ?? -1, + ...(timedOut ? { timedOut: true } : {}), + }); + }); + }); +} diff --git a/packages/runtime-host/src/host-executor.ts b/packages/runtime-host/src/host-executor.ts index 6a2f8563f..d3ccefe82 100644 --- a/packages/runtime-host/src/host-executor.ts +++ b/packages/runtime-host/src/host-executor.ts @@ -194,6 +194,7 @@ export class HostExecutor implements Executor { let stdout = ""; let stderr = ""; let timedOut = false; + let spawnErrored = false; const child = spawn(command, args, { cwd, @@ -220,6 +221,7 @@ export class HostExecutor implements Executor { child.on("error", (err) => { clearTimeout(timer); + spawnErrored = true; const durationMs = Date.now() - start; const logs = this.truncateLogs(`stderr: ${err.message}`); resolvePromise({ @@ -233,6 +235,10 @@ export class HostExecutor implements Executor { }); child.on("close", (code) => { + if (spawnErrored) { + // The 'error' event already resolved the promise with a runtime failure. + return; + } clearTimeout(timer); const durationMs = Date.now() - start; const rawLogs = stdout + (stderr ? "\n" + stderr : ""); @@ -251,6 +257,21 @@ export class HostExecutor implements Executor { return; } + // Negative or null exit codes indicate the process could not be spawned + // or was terminated by a signal; treat these as runtime failures. + if (code === null || code < 0) { + resolvePromise({ + operationId, + status: "failure", + durationMs, + logs, + artifacts: [], + error: `spawn error: exit code ${code}`, + runtimeFailure: true, + }); + return; + } + const status = code === 0 ? "success" : "failure"; resolvePromise({ operationId, diff --git a/specs/04-runtime-docker/spec.md b/specs/04-runtime-docker/spec.md index 631ee8856..c023ac8dc 100644 --- a/specs/04-runtime-docker/spec.md +++ b/specs/04-runtime-docker/spec.md @@ -67,12 +67,8 @@ export interface DockerExecutorConfig { readonly dockerHost?: string; /** Default non-root uid:gid for containers. Defaults to "1000:1000". */ readonly runAs: string; - /** Directory for cache layers. */ + /** Persistent directory for cache layers (managed by DockerCacheManager). */ readonly cacheDir: string; - /** Directory for collected artifacts. */ - readonly artifactDir: string; - /** Workspace root to mount read-only. */ - readonly workspace: string; /** Maximum log size in bytes before truncation. Defaults to 10 MiB. */ readonly maxLogBytes?: number; } @@ -151,9 +147,9 @@ docker run --cpus # CPU limit --timeout # mandatory timeout --workdir /workspace - --mount type=bind,source=,target=/workspace,readonly + --mount type=bind,source=,target=/workspace,readonly --mount type=bind,source=,target=/cache - --mount type=bind,source=,target=/artifacts + --mount type=bind,source=,target=/artifacts --env @ @@ -240,10 +236,13 @@ Rules: 3. **Image digest mismatch.** If the pulled image's digest does not match the declared digest, `ImageDigestError` is raised with both digests in context. The container is not started. -4. **Secret not in allowlist.** If `credentials` references an env var not in - the operation's credential declarations, it is not passed. If an env var in - `operation.env` looks like a secret (matches a denylist pattern) but is not - declared, `ContainerPolicyError` with code `UNDECLARED_SECRET` is raised. +4. **Secret not in allowlist.** Only env vars declared in + `operation.credentials` (CredentialDeclaration[]) are passed; their values + come from `request.credentials` (Record keyed by envVar). + `request.env` provides operation env vars. If an env var in `request.env` + looks like a secret (matches a denylist pattern) but is not declared in + `operation.credentials`, `ContainerPolicyError` with code `UNDECLARED_SECRET` + is raised. 5. **Docker socket access.** If any mount or env attempts to reference the Docker socket, `ContainerPolicyError` with code `DOCKER_SOCKET_DENIED` is raised. @@ -258,7 +257,7 @@ Rules: ## Test plan -Tests live in `packages/runtime-docker/src/__tests__/` and run via `bun test`. +Tests live in `packages/runtime-docker/src/__tests__/` and run via `bun run test`. 1. **canExecute** - Returns `true` for operations with `executor.type === "docker"`. @@ -287,9 +286,10 @@ Tests live in `packages/runtime-docker/src/__tests__/` and run via `bun test`. (`MISSING_DIGEST`). 5. **Secrets allowlist** - - Only env vars listed in `credentials` are passed to the container. - - An undeclared secret-like env var raises `ContainerPolicyError` - (`UNDECLARED_SECRET`). + - Only env vars declared in `operation.credentials` are passed; values come + from `request.credentials`. + - An undeclared secret-like env var in `request.env` raises + `ContainerPolicyError` (`UNDECLARED_SECRET`). - An attempt to mount the Docker socket raises `ContainerPolicyError` (`DOCKER_SOCKET_DENIED`). @@ -320,8 +320,8 @@ Tests live in `packages/runtime-docker/src/__tests__/` and run via `bun test`. 11. **Commands** ```bash - bun test packages/runtime-docker - SVERKA_DOCKER=1 bun test packages/runtime-docker # include integration + bun run test + SVERKA_DOCKER=1 bun run test # include integration bun run typecheck bun run lint ```