From 8a7e8536d414bffa5b93ecc6ea53fc0ce2b4e904 Mon Sep 17 00:00:00 2001 From: Relayflow Date: Sun, 20 Sep 2026 10:59:30 +0000 Subject: [PATCH 1/3] feat: accept f.agent cwd in the kernel and contain it in the worker (#357) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `f.agent({ cwd })` was type-checked by the SDK, lowered into the step spec and passed `flows check`, then killed the run at dispatch with `invalid_spec: unknown field "cwd" at steps[0]`. Accepting a declaration and refusing it later is the worst place to refuse it: the author has already written the flow, and nothing before the run says otherwise. The kernel now accepts `cwd` on agent steps and the worker runs the CLI there, held inside the run's tree. `cwd` is run-root-relative — an absolute host path is not portable to another machine or a Cloud upload, and cannot be contained lexically, so allowing it would only move the late refusal. The contract is split because they are two different facts: the declaration is lexical, so `flows check`, the kernel's `validate()` and the worker answer identically for the same spec; the target is resolved only in the worker that shares the agent's filesystem, re-checked at every dispatch because the filesystem moves underneath a spec. A bad declaration is refused before a run exists; a bad target completes the step `worker_error` with the reason journaled, rather than spawning somewhere nobody established is inside the tree. `transport: 'relay'` refuses `cwd` instead of forwarding a string this contract cannot honour on another host. Both dialects are proven to agree rather than assumed to, against a shared corpus, including the Unicode whitespace edge where Rust's `is_whitespace` and JavaScript's `\s` disagree — the exact shape of the bug being fixed. The new live test is mutation-verified: reverting the kernel field reproduces the ticket's error character for character. Also repairs `cwd` inverse-mapping loss in `kernelStepToAuthoring`, and the communication worker's unresolved `spec.cwd ?? process.cwd()` spawn, which bypassed containment entirely. Co-Authored-By: Claude --- docs/SURFACE.md | 55 ++++ kernel/relayflowd-core/src/spec.rs | 68 ++++- kernel/relayflowd-core/tests/spec_parity.rs | 89 ++++++ packages/schema/flows.schema.json | 4 +- packages/sdk/src/agent-cwd.ts | 139 ++++++++++ packages/sdk/src/authored-worker-step.ts | 13 +- packages/sdk/src/communication/worker.ts | 18 +- packages/sdk/src/compile.ts | 6 +- packages/sdk/src/spec.ts | 18 +- packages/sdk/src/validate.ts | 19 ++ packages/sdk/src/worker.ts | 36 ++- packages/sdk/tests/agent-cwd-live.test.ts | 226 +++++++++++++++ packages/sdk/tests/agent-cwd.test.ts | 261 ++++++++++++++++++ .../tests/authored-agent-artifacts.test.ts | 8 +- .../tests/authored-agent-permissions.test.ts | 4 +- packages/sdk/tests/spec-parity.test.ts | 2 +- packages/sdk/tests/worker-transcript.test.ts | 6 +- packages/surface/src/context.ts | 12 +- summary.md | 170 ++++++++++++ testdata/agent-cwd-cases.json | 122 ++++++++ testdata/agent-cwd.flow.yaml | 27 ++ testdata/agent-cwd.spec.canonical.json | 1 + testdata/agent-cwd.spec.sha256 | 1 + 23 files changed, 1270 insertions(+), 35 deletions(-) create mode 100644 packages/sdk/src/agent-cwd.ts create mode 100644 packages/sdk/tests/agent-cwd-live.test.ts create mode 100644 packages/sdk/tests/agent-cwd.test.ts create mode 100644 summary.md create mode 100644 testdata/agent-cwd-cases.json create mode 100644 testdata/agent-cwd.flow.yaml create mode 100644 testdata/agent-cwd.spec.canonical.json create mode 100644 testdata/agent-cwd.spec.sha256 diff --git a/docs/SURFACE.md b/docs/SURFACE.md index 4620b56ea..ee0a52cda 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -321,6 +321,61 @@ flow-wide `FlowHeader.workspace` / `tools.fs` scopes. The chief harness above remains an aspirational example; this option does not make that entire harness executable today. +### Per-agent working directory + +`AgentOptions.cwd` — and the `cwd:` key on a declarative `type: agent` step — +names the directory the agent's CLI is spawned in. It is how one flow drives +agents in several checkouts: + +```ts +const api = await f.agent("api", { + task: "Apply the rename in the API checkout.", + cwd: "checkouts/service-a", +}); +const web = await f.agent("web", { + task: "Apply the matching rename in the web checkout.", + cwd: "checkouts/service-b", +}); +``` + +**The path is relative to the run root** — the working directory `flows run` +was invoked from, which is also the directory an agent runs in when `cwd` is +absent, and the uploaded tree on the Cloud path (docs/CLOUD.md, "Code sync"). +A relative declaration is the same declaration on every host; an absolute one +names a place a different machine does not have, so absolute paths are refused +rather than resolved. + +Two checks answer two different questions: + +- **The declaration** is checked lexically, with no filesystem access, by + `flows check`, by the authored runner, and by the kernel — the same rule in + all three, so a spec that passes the first is not refused by the last. A + `cwd` must be a nonempty string, must not be absolute or URI-like, must not + carry surrounding whitespace or a NUL, and must have no empty, `.` or `..` + components. `cwd: null` is a refusal, not "no directory". `cwd` is an agent + field: a `deterministic` or `llm` step that declares it is `invalid_spec`. +- **The target** is resolved at dispatch by the worker that spawns the CLI, + the one process that provably shares the agent's filesystem. It must be an + existing directory whose symlink-free path lies inside the symlink-free run + root. A symlink out of the tree, and a sibling whose name merely starts with + the run root's, are both outside it. A step whose directory does not resolve + completes `worker_error` with the reason journaled, and no CLI is spawned. + +`AgentResult.artifacts` are reported relative to this directory, so each agent +above reports paths within its own checkout. + +`cwd` is **not supported with `transport: "relay"`** and is refused when both +are declared: the relay agent runs on another host, where this process can +neither resolve the directory nor hold it inside the run root. + +This is a declaration of where an agent starts, not a sandbox. Nothing stops a +CLI from reading or writing outside the directory it was spawned in. Per-step +scoping is `permissions`, which is recorded and not enforced (gate 8 / #442). + +Workspace surfaces are a different thing and are not a directory selector: +`surfaces.workspace` names the revisions a step pins and writes back to, not a +path on the host running the CLI. + ### Supported TypeScript LLM calls The local authored executor supports these signatures: diff --git a/kernel/relayflowd-core/src/spec.rs b/kernel/relayflowd-core/src/spec.rs index bc5f4dae3..fc10df605 100644 --- a/kernel/relayflowd-core/src/spec.rs +++ b/kernel/relayflowd-core/src/spec.rs @@ -163,7 +163,15 @@ impl RunSpec { if cli.as_ref().is_some_and(|value| value.trim().is_empty()) { return Err(SpecError::EmptyStepCli(step.id.clone())); } - if let StepKind::Agent { surfaces, .. } = &step.kind { + if let StepKind::Agent { surfaces, cwd, .. } = &step.kind { + if let Some(cwd) = cwd + && !is_run_root_relative_path(cwd) + { + return Err(SpecError::InvalidAgentCwd { + step: step.id.clone(), + cwd: cwd.clone(), + }); + } for workspace in &surfaces.workspace { if path_surface_identity(&workspace.surface).is_none() { return Err(SpecError::InvalidWorkspaceSurface { @@ -264,6 +272,28 @@ pub(crate) fn path_surface_identity(path: &str) -> Option<(String, Vec)> .then_some((namespace, components)) } +/// Whether a declared agent `cwd` names a place inside the run's own tree. +/// +/// The same lexical canonical rule surfaces get — no whitespace aliases, +/// empty components, `.` or `..` — narrowed to relative paths only, because +/// the declaration is read against the run root the worker executes under. +/// An absolute path or a URI-like mount identity names a place the run root +/// does not contain, and a `..` component walks out of it; both are refused +/// here rather than discovered on the host. NUL cannot occur in a host path +/// component, so a declaration carrying one can only mislead a reader. +fn is_run_root_relative_path(path: &str) -> bool { + // U+FEFF is named explicitly because JavaScript's `\s` includes it and + // `char::is_whitespace` does not; the SDK names U+0085 for the mirror + // reason. Both sides then refuse `White_Space ∪ {U+FEFF}` at the edges, + // so a padded declaration cannot pass `flows check` and be refused here. + let padding = |c: char| c.is_whitespace() || c == '\u{feff}'; + !path.contains('\0') + && !path.starts_with(padding) + && !path.ends_with(padding) + && matches!(path_surface_identity(path), Some((namespace, components)) + if namespace.is_empty() && !components.is_empty()) +} + pub fn is_canonical_external_surface(path: &str) -> bool { path_surface_identity(path).is_some() } @@ -313,6 +343,7 @@ const STEP_AGENT_FIELDS: &[&str] = &[ "instruction", "cli", "model", + "cwd", "transport", "recovery_mode", "surfaces", @@ -330,7 +361,25 @@ fn reject_unknown_step_fields(value: &Value) -> Result<(), SpecError> { let kind_fields = match object.get("type").and_then(Value::as_str) { Some("deterministic") => STEP_DETERMINISTIC_FIELDS, Some("llm") => STEP_LLM_FIELDS, - Some("agent") => STEP_AGENT_FIELDS, + Some("agent") => { + // `cwd` deserializes into `Option`, where serde reads + // an explicit null as absence — the step would then run in the + // default directory under a spec that declared otherwise. The + // shape is checked before serde so a null, a number or an + // object is refused instead of silently defaulted. + if let Some(cwd) = object.get("cwd") + && !cwd.is_string() + { + return Err(SpecError::Malformed(format!( + "step {}: cwd must be a string", + object + .get("id") + .and_then(Value::as_str) + .unwrap_or("?") + ))); + } + STEP_AGENT_FIELDS + } // Missing/unknown type is rejected by serde's tagged-enum error. _ => continue, }; @@ -424,6 +473,17 @@ pub enum StepKind { /// then handed to the worker, which surfaces it to the CLI. #[serde(default, skip_serializing_if = "Option::is_none")] model: Option, + /// Directory the attached worker spawns the declared CLI in, relative + /// to the run root the worker executes under. The kernel performs no + /// I/O: it checks the declaration lexically (the same canonical-path + /// rule surfaces get, plus "relative, so it names a place inside the + /// run's tree") and carries it verbatim, so the choice is part of the + /// run's record rather than ambient host state. Resolving it against a + /// real filesystem — existence, symlinks, containment — belongs to the + /// worker that spawns the CLI, the one process that shares that + /// filesystem. Absent means the run root itself. + #[serde(default, skip_serializing_if = "Option::is_none")] + cwd: Option, /// How the attached worker invokes the declared CLI. The kernel does /// not implement either transport; it journals and dispatches the /// choice so the worker can honor it deterministically. @@ -739,6 +799,10 @@ pub enum SpecError { EmptyStepId, #[error("step {0} cli cannot be empty")] EmptyStepCli(String), + #[error( + "agent step {step} declares working directory {cwd:?}, which is not a run-root-relative path (no absolute paths, empty components, \".\" or \"..\")" + )] + InvalidAgentCwd { step: String, cwd: String }, #[error("agent step {step} declares non-canonical external surface {path:?}")] InvalidExternalSurface { step: String, path: String }, #[error("agent step {step} declares non-canonical workspace surface {surface:?}")] diff --git a/kernel/relayflowd-core/tests/spec_parity.rs b/kernel/relayflowd-core/tests/spec_parity.rs index 7e327879d..a0f525cb2 100644 --- a/kernel/relayflowd-core/tests/spec_parity.rs +++ b/kernel/relayflowd-core/tests/spec_parity.rs @@ -198,3 +198,92 @@ fn placement_declaration_acceptance_matches_the_sdk_corpus() { } } } + +/// flows#357: `cwd` reached the daemon as `unknown field "cwd" at steps[0]` +/// after `flows check` had already passed, because the SDK lowered a field the +/// kernel's closed agent schema did not name. The canonical bytes and hash of a +/// flow that declares it — on two steps and not on a third — are now pinned on +/// both sides of the boundary. +#[test] +fn agent_working_directories_have_identical_canonical_bytes_and_hash() { + assert_parity( + include_str!("../../../testdata/agent-cwd.spec.canonical.json"), + include_str!("../../../testdata/agent-cwd.spec.sha256"), + ); +} + +#[test] +fn agent_cwd_declaration_acceptance_matches_the_sdk_corpus() { + let cases: Vec = + serde_json::from_str(include_str!("../../../testdata/agent-cwd-cases.json")).unwrap(); + for case in cases { + let spec = serde_json::json!({"steps":[{ + "id":"s","type":"agent","instruction":"work","cwd":case["cwd"], + }]}); + let accepted = RunSpec::parse(&spec) + .and_then(|spec| spec.validate()) + .is_ok(); + assert_eq!( + accepted, + case["valid"].as_bool().unwrap(), + "{}", + case["name"] + ); + } +} + +/// An absent `cwd` is absent in the re-serialized spec, not `"cwd":null`: every +/// fixture committed before this field existed keeps its bytes and its hash. +#[test] +fn an_undeclared_agent_cwd_is_not_serialized() { + let value = serde_json::json!({ + "steps": [{"id": "agent", "type": "agent", "instruction": "work"}], + }); + let parsed = RunSpec::parse(&value).expect("an agent step without cwd must parse"); + parsed.validate().expect("and must validate"); + assert!( + serde_json::to_value(parsed).unwrap()["steps"][0] + .get("cwd") + .is_none() + ); +} + +/// `Option` reads an explicit null as absence, which would run the step +/// in the default directory under a spec that declared otherwise. The shape is +/// checked before serde so every non-string spelling fails closed. +#[test] +fn a_non_string_agent_cwd_fails_closed_rather_than_defaulting() { + for cwd in [ + serde_json::json!(null), + serde_json::json!(7), + serde_json::json!(["checkouts/service-a"]), + serde_json::json!({"path": "checkouts/service-a"}), + serde_json::json!(true), + ] { + let value = serde_json::json!({ + "steps": [{"id": "agent", "type": "agent", "instruction": "work", "cwd": cwd}], + }); + let error = RunSpec::parse(&value).expect_err("a non-string cwd must fail closed"); + assert!( + error.to_string().contains("cwd must be a string"), + "{cwd}: {error}" + ); + } +} + +/// `cwd` is agent-only, and the near-miss spelling is still an unknown field: +/// widening one verb's schema must not quietly widen the others or the name. +#[test] +fn agent_cwd_is_not_accepted_on_other_verbs_or_under_another_name() { + for step in [ + serde_json::json!({"id":"s","type":"deterministic","command":"true","cwd":"checkouts/a"}), + serde_json::json!({"id":"s","type":"llm","prompt":"work","cwd":"checkouts/a"}), + serde_json::json!({"id":"s","type":"agent","instruction":"work","cwdd":"checkouts/a"}), + serde_json::json!({"id":"s","type":"agent","instruction":"work","worker_cwd":"checkouts/a"}), + ] { + assert!( + RunSpec::parse(&serde_json::json!({"steps": [step.clone()]})).is_err(), + "{step} must fail closed" + ); + } +} diff --git a/packages/schema/flows.schema.json b/packages/schema/flows.schema.json index ff0742df4..520e506d1 100644 --- a/packages/schema/flows.schema.json +++ b/packages/schema/flows.schema.json @@ -1364,7 +1364,7 @@ }, "cwd": { "title": "cwd", - "description": "Working directory for the CLI subprocess; defaults to the flow-runner's cwd.", + "description": "Directory the declared CLI is spawned in, as a path relative to the run\nroot — the flow-runner's working directory, which is also where the CLI\nruns when this is absent. Absolute paths, `.`, `..` and empty components\nare refused lexically by `flows check` and by the kernel; the worker that\nspawns the CLI additionally requires the symlink-free directory to exist\ninside the symlink-free run root. A declaration, not a sandbox: nothing\nstops a CLI from writing outside it. Not supported with\n`transport: 'relay'`, where the agent runs on a host this process cannot\nresolve. See docs/SURFACE.md.", "type": "string" }, "transport": { @@ -2378,7 +2378,7 @@ }, "cwd": { "title": "cwd", - "description": "Working directory for the CLI subprocess; kernel passes through untouched.", + "description": "Run-root-relative directory the attached worker spawns the CLI in. The\nkernel checks the shape and does no I/O: existence and containment are\ndecided by the worker, on the host that shares the agent's filesystem.", "type": "string" }, "transport": { diff --git a/packages/sdk/src/agent-cwd.ts b/packages/sdk/src/agent-cwd.ts new file mode 100644 index 000000000..376743820 --- /dev/null +++ b/packages/sdk/src/agent-cwd.ts @@ -0,0 +1,139 @@ +import { realpathSync, statSync } from 'node:fs'; +import { resolve, sep } from 'node:path'; + +/** + * Where an agent step runs. + * + * A declared `cwd` is a path relative to the **run root** — the working + * directory of the process running the agent worker, which is the directory + * `flows run` was invoked from and, on the Cloud path, the uploaded tree + * (docs/CLOUD.md, "Code sync"). Absent, a step runs in the run root itself. + * + * Two checks, in two places, because they are two different facts: + * + * - The **declaration** is lexical and filesystem-free, so `flows check`, the + * kernel's `validate()` and this worker all answer identically for the same + * spec. `testdata/agent-cwd-cases.json` is the shared corpus both dialects + * are tested against. + * - The **target** is resolved here, in the worker that spawns the CLI — the + * one process provably sharing the agent's filesystem. A directory that + * exists, and whose symlink-free path is inside the symlink-free run root. + * Repeated at execution even when a preflight already passed, because the + * filesystem moves underneath a spec. + * + * This contains a declaration; it does not sandbox an agent. Nothing stops a + * CLI from writing outside the directory it was started in. Per-step scoping + * is `permissions`, which is recorded and not enforced (gate 8 / #442). + */ +export class AgentCwdError extends Error { + constructor(message: string) { + super(message); + this.name = 'AgentCwdError'; + } +} + +/** + * Why a declared `cwd` is not a run-root-relative path, or `undefined` when it + * is one. Pure and lexical: the exact rule + * `relayflowd_core::spec::is_run_root_relative_path` applies at the kernel + * boundary, so a declaration that passes `flows check` is not refused later by + * the daemon for its shape. + */ +export function agentCwdDeclarationError(cwd: unknown): string | undefined { + const expected = 'expected a run-root-relative path'; + if (typeof cwd !== 'string') return `${expected} (got ${cwd === null ? 'null' : typeof cwd})`; + if (cwd === '') return `${expected}, not an empty string`; + if (PADDING.test(cwd)) return `${expected} without surrounding whitespace`; + if (cwd.includes('\0')) return `${expected} without NUL`; + if (cwd.startsWith('/')) return `${expected}, not an absolute path`; + if (cwd.includes('://')) return `${expected}, not a URI-like mount identity`; + if (cwd.split('/').some((part) => part === '' || part === '.' || part === '..')) { + return `${expected} without empty, "." or ".." components`; + } + return undefined; +} + +/** + * Whitespace at either end, spelled so both dialects refuse the same set. + * JavaScript's `\s` is Unicode `White_Space` without U+0085 and with U+FEFF; + * Rust's `char::is_whitespace` is `White_Space` exactly. Naming U+0085 here + * and U+FEFF there makes both `White_Space ∪ {U+FEFF}`. Left to `trim()` + * alone, a path padded with either character would pass `flows check` and be + * refused by the kernel — the accept-then-refuse split this contract closes. + */ +const PADDING = /^[\s\u0085]|[\s\u0085]$/u; + +/** + * The refusal for a `cwd` declared on a step dispatched over the relay + * transport, or `undefined` when there is nothing to refuse. + * + * The relay agent runs on another host. This process can neither resolve that + * host's filesystem nor establish that the directory is inside that run's + * tree, and forwarding the string as `worker_cwd` would let a declaration this + * contract promises to contain go unchecked. Fail closed instead of pretending. + */ +export function agentCwdTransportError(cwd: unknown, transport: unknown): string | undefined { + if (cwd === undefined || transport !== 'relay') return undefined; + return 'cwd is not supported with transport "relay": the agent runs on another host, ' + + 'where this worker cannot resolve the directory or hold it inside the run root'; +} + +/** + * The directory to spawn the CLI in: `root` when nothing is declared, else the + * symlink-free directory `cwd` names beneath `root`. + * + * @throws AgentCwdError naming the declaration and what is wrong with it. + */ +export function resolveAgentCwd(root: string, cwd: unknown, transport?: unknown): string | undefined { + if (cwd === undefined) return undefined; + const refusal = (detail: string): AgentCwdError => + new AgentCwdError(`cwd ${JSON.stringify(cwd)}: ${detail}`); + const declaration = agentCwdDeclarationError(cwd); + if (declaration !== undefined) throw refusal(declaration); + const unsupported = agentCwdTransportError(cwd, transport); + if (unsupported !== undefined) throw new AgentCwdError(unsupported); + + let realRoot: string; + try { + realRoot = realpathSync(root); + } catch { + throw refusal(`the run root ${JSON.stringify(root)} cannot be resolved on this host`); + } + let target: string; + try { + target = realpathSync(resolve(realRoot, cwd as string)); + } catch { + throw refusal(`no such directory under the run root ${JSON.stringify(realRoot)}`); + } + // Component-aware, on the symlink-free forms of both: `checkout-b-old` is + // not inside `checkout-b`, and a symlink pointing out of the tree is not + // inside it either however it is spelled. + const prefix = realRoot.endsWith(sep) ? realRoot : realRoot + sep; + if (target !== realRoot && !target.startsWith(prefix)) { + throw refusal(`resolves to ${JSON.stringify(target)}, outside the run root ${JSON.stringify(realRoot)}`); + } + if (!statSync(target).isDirectory()) throw refusal(`${JSON.stringify(target)} is not a directory`); + return target; +} + +/** Either the directory an agent step runs in, or why the step is refused. */ +export type AgentCwdOutcome = { directory: string | undefined } | { refusal: string }; + +/** + * The dispatch-time form: resolve a dispatched agent step's `cwd` against the + * run root, or report the refusal for the worker to complete the step with. + * Reporting rather than throwing keeps the refusal on the step's completion — + * `worker_error`, with the reason journaled — instead of on the worker. + */ +export function agentStepCwd( + spec: { cwd?: unknown; transport?: unknown }, + stepId: string, + root: string = process.cwd(), +): AgentCwdOutcome { + try { + return { directory: resolveAgentCwd(root, spec.cwd, spec.transport) }; + } catch (error) { + if (!(error instanceof AgentCwdError)) throw error; + return { refusal: `agent step "${stepId}": ${error.message}` }; + } +} diff --git a/packages/sdk/src/authored-worker-step.ts b/packages/sdk/src/authored-worker-step.ts index bd85fac07..07b35db20 100644 --- a/packages/sdk/src/authored-worker-step.ts +++ b/packages/sdk/src/authored-worker-step.ts @@ -6,6 +6,7 @@ import { checkAuthoredFlow } from './cli/check.js'; import { classifyOutcome, type RunLifecycleOptions, type RunReport } from './cli/run.js'; import type { PreflightDiagnostic } from './preflight.js'; import { AuthoredFlowExecutionError } from './authored-flow-error.js'; +import { agentCwdDeclarationError, agentCwdTransportError } from './agent-cwd.js'; import type { JournalClient } from './journal-client.js'; import { SPEC_SCHEMA_VERSION, type FlowSpec, type PermissionsSpec, type StepSpec } from './spec.js'; import { isSurfaceCompletionReason, readCompletedStepOutput, readSuccessfulOutput, type AuthoredStepContext } from './authored-step-output.js'; @@ -145,10 +146,14 @@ export function authoredWorkerRunner( `f.agent options.model must be a string when set (got ${typeof options.model}).`, ); } - if (options.cwd !== undefined && typeof options.cwd !== 'string') { + // The same lexical rule the kernel and `flows check` apply, raised here + // so an authored body is refused before a run exists rather than at + // dispatch. Whether the directory is there is the worker's question. + const cwdProblem = options.cwd === undefined ? undefined : agentCwdDeclarationError(options.cwd); + if (cwdProblem !== undefined) { throw new AuthoredFlowExecutionError( 'agent_cli_unresolved', - `f.agent options.cwd must be a string when set (got ${typeof options.cwd}).`, + `f.agent options.cwd: ${cwdProblem}.`, ); } if (options.transport !== undefined && options.transport !== 'direct' && options.transport !== 'relay') { @@ -157,6 +162,10 @@ export function authoredWorkerRunner( `f.agent options.transport must be 'direct' or 'relay' (got ${JSON.stringify(options.transport)}).`, ); } + const cwdTransport = agentCwdTransportError(options.cwd, options.transport); + if (cwdTransport !== undefined) { + throw new AuthoredFlowExecutionError('agent_cli_unresolved', `f.agent options.${cwdTransport}.`); + } const permissions = options.permissions; const permissionsSnapshot = permissions === undefined ? undefined : snapshotJsonValue(permissions, 'f.agent options.permissions') as unknown as PermissionsSpec; diff --git a/packages/sdk/src/communication/worker.ts b/packages/sdk/src/communication/worker.ts index 0bb1a5839..2e56c5c9e 100644 --- a/packages/sdk/src/communication/worker.ts +++ b/packages/sdk/src/communication/worker.ts @@ -7,6 +7,7 @@ import type { KernelAgentStep } from '../spec.js'; import { withWorkerLease } from '../worker-lease.js'; import { workerInstruction } from '../worker-input.js'; import { resolveCliModel } from '../cli-adapter.js'; +import { resolveAgentCwd } from '../agent-cwd.js'; import { channelName, type CommunicationInstruction } from './spec.js'; import { acquireRelayRuntime, type RelayHandle } from './relay.js'; import { CommunicationSession } from './session.js'; @@ -17,13 +18,13 @@ export function requireCommunicationCli(cli: string | undefined): void { if (!cli?.trim()) throw new Error('Agent communication requires a declared CLI executable'); } export async function completeCommunicationDispatch(client: JournalClient, dispatch: StepDispatchEvent, - instruction: CommunicationInstruction, dataDir: string): Promise { + instruction: CommunicationInstruction, dataDir: string, runRoot?: string): Promise { const spec = dispatch.spec as KernelAgentStep; requireCommunicationCli(spec.cli); let output: unknown; let completionReason: 'success' | 'worker_error' = 'success'; try { - output = await withWorkerLease(client, dispatch, signal => run(client, dispatch, instruction, spec, dataDir, signal)); + output = await withWorkerLease(client, dispatch, signal => run(client, dispatch, instruction, spec, dataDir, signal, runRoot)); } catch (error) { completionReason = 'worker_error'; output = { error: error instanceof Error ? error.message : String(error) }; @@ -33,7 +34,14 @@ export async function completeCommunicationDispatch(client: JournalClient, dispa started_pins: dispatch.pins, end_pins: dispatch.pins }); } async function run(client: JournalClient, dispatch: StepDispatchEvent, instruction: CommunicationInstruction, - spec: KernelAgentStep, dataDir: string, lease: AbortSignal): Promise { + spec: KernelAgentStep, dataDir: string, lease: AbortSignal, runRoot?: string): Promise { + // Same contract as the CLI worker: a declared directory is resolved and held + // inside the same run root the CLI worker measures against, before anything + // is spawned. Thrown, not reported, because `completeCommunicationDispatch` + // already turns a throw here into a `worker_error` completion carrying the + // message. + const root = runRoot ?? process.cwd(); + const directory = resolveAgentCwd(root, spec.cwd) ?? root; const controller = new AbortController(); const signal = AbortSignal.any([lease, controller.signal, AbortSignal.timeout(instruction.timeoutMs)]); const relay = await acquireRelayRuntime(dataDir, dispatch.run_id); @@ -70,9 +78,9 @@ async function run(client: JournalClient, dispatch: StepDispatchEvent, instructi }); // Relay supplies each CLI's launch flags and injection behavior. handle = await relay.broker.spawnPty({ name, cli: basename(spec.cli!).replace(/\.exe$/i, ''), task: prompt, channels: [], skipRelayPrompt: true, - model: resolveCliModel(spec.cli!, spec.model), cwd: spec.cwd ?? process.cwd(), + model: resolveCliModel(spec.cli!, spec.model), cwd: directory, harnessConfig: { runtime: 'pty', command: quote(spec.cli!), args: [], - cwd: spec.cwd ?? process.cwd(), env: { ...agentEnvironment(spec.cli!), + cwd: directory, env: { ...agentEnvironment(spec.cli!), RELAYFLOW_COMMUNICATION_SOCKET: tools.path, RELAYFLOW_COMMUNICATION_TOKEN: tools.token }, delivery: { mode: 'pty-injection', format: 'relay-block' } } }); const ready = await handle.waitForReady(Math.min(instruction.timeoutMs, 90_000)); diff --git a/packages/sdk/src/compile.ts b/packages/sdk/src/compile.ts index 27368a108..30017ad6c 100644 --- a/packages/sdk/src/compile.ts +++ b/packages/sdk/src/compile.ts @@ -427,7 +427,7 @@ function kernelStepToAuthoring(value: unknown, at: string): unknown { const unionKeys = [ 'id', 'type', 'depends_on', 'max_iterations', 'retry', 'verification', 'memory', 'requirements', 'input', 'command', 'timeout_ms', 'lease_ms', 'prompt', 'model', 'cli', 'instruction', - 'recovery_mode', 'surfaces', 'permissions', + 'cwd', 'recovery_mode', 'surfaces', 'permissions', ] as const; const step = requireKernelObject(value, unionKeys, at); const type = step['type']; @@ -437,7 +437,7 @@ function kernelStepToAuthoring(value: unknown, at: string): unknown { : type === 'llm' ? ['prompt', 'model', 'cli'] as const : type === 'agent' - ? ['instruction', 'cli', 'model', 'recovery_mode', 'surfaces', 'permissions'] as const + ? ['instruction', 'cli', 'model', 'cwd', 'recovery_mode', 'surfaces', 'permissions'] as const : []; assertKernelKeys(step, [...commonKeys, ...typeKeys], at); if (step['retry'] !== undefined) validateAuthoringRetryDefaults(step['retry'], `${at}.retry`); @@ -470,7 +470,7 @@ function kernelStepToAuthoring(value: unknown, at: string): unknown { ...common, instruction: step['instruction'], ...(step['recovery_mode'] !== undefined ? { recoveryMode: step['recovery_mode'] } : {}), - ...copyDefined(step, ['cli', 'model', 'surfaces']), + ...copyDefined(step, ['cli', 'model', 'cwd', 'surfaces']), ...(step['permissions'] !== undefined ? { permissions: kernelPermissionsToAuthoring(step['permissions'], `${at}.permissions`) } : {}), diff --git a/packages/sdk/src/spec.ts b/packages/sdk/src/spec.ts index 6016a3806..62024408f 100644 --- a/packages/sdk/src/spec.ts +++ b/packages/sdk/src/spec.ts @@ -274,7 +274,17 @@ export interface AgentStepSpec extends BaseStepSpec { surfaces?: AgentSurfaces; recoveryMode?: RecoveryMode; permissions?: PermissionsSpec; - /** Working directory for the CLI subprocess; defaults to the flow-runner's cwd. */ + /** + * Directory the declared CLI is spawned in, as a path relative to the run + * root — the flow-runner's working directory, which is also where the CLI + * runs when this is absent. Absolute paths, `.`, `..` and empty components + * are refused lexically by `flows check` and by the kernel; the worker that + * spawns the CLI additionally requires the symlink-free directory to exist + * inside the symlink-free run root. A declaration, not a sandbox: nothing + * stops a CLI from writing outside it. Not supported with + * `transport: 'relay'`, where the agent runs on a host this process cannot + * resolve. See docs/SURFACE.md. + */ cwd?: string; /** * Dispatch transport (flows#385). `'direct'` (default) spawns the CLI as @@ -469,7 +479,11 @@ export interface KernelAgentStep extends KernelStepCommon { recovery_mode: RecoveryMode; surfaces?: KernelAgentSurfaces; permissions?: KernelPermissionsSpec; - /** Working directory for the CLI subprocess; kernel passes through untouched. */ + /** + * Run-root-relative directory the attached worker spawns the CLI in. The + * kernel checks the shape and does no I/O: existence and containment are + * decided by the worker, on the host that shares the agent's filesystem. + */ cwd?: string; /** * Dispatch transport (flows#385). Kernel passes through untouched; diff --git a/packages/sdk/src/validate.ts b/packages/sdk/src/validate.ts index 09b733c1a..cc130de89 100644 --- a/packages/sdk/src/validate.ts +++ b/packages/sdk/src/validate.ts @@ -19,6 +19,7 @@ import type { import { SPEC_SCHEMA_VERSION } from './spec.js'; import { validateOutputDeclaration } from './output-schema.js'; import { modelNameError } from './model-name.js'; +import { agentCwdDeclarationError, agentCwdTransportError } from './agent-cwd.js'; import { unknownKeyErrors } from './unknown-keys.js'; import { stepDependencyErrors } from './step-dependencies.js'; import { inputBindingErrors } from './input-binding.js'; @@ -486,6 +487,7 @@ class Validator { } this.validateCli(st.cli, at); this.validateModel(st.model, at); + this.validateCwd(st, at); if (st.surfaces !== undefined) this.validateSurfaces(st.surfaces, `${at}.surfaces`); if (st.permissions !== undefined) this.validatePermissions(st.permissions, `${at}.permissions`); } @@ -496,6 +498,23 @@ class Validator { } } + /** + * The declaration half of `cwd`: lexical, so an author sees the refusal from + * `flows check` rather than from a dispatch. Whether the directory exists and + * lies inside the run root is the worker's question (`agent-cwd.ts`), asked on + * the host that shares the agent's filesystem. + */ + private validateCwd(st: AgentStepSpec, at: string): void { + if (st.cwd === undefined) return; + const problem = agentCwdDeclarationError(st.cwd); + if (problem !== undefined) { + this.fail(`${at}.cwd: ${problem}`); + return; + } + const unsupported = agentCwdTransportError(st.cwd, st.transport); + if (unsupported !== undefined) this.fail(`${at}.${unsupported}`); + } + private validateModel(model: unknown, at: string, required = false): void { // Rejecting the empty string matters: it would reach the CLI as // RELAYFLOW_MODEL='', which reads as "declared, and declared as diff --git a/packages/sdk/src/worker.ts b/packages/sdk/src/worker.ts index 663809880..bd6b8c6f9 100644 --- a/packages/sdk/src/worker.ts +++ b/packages/sdk/src/worker.ts @@ -6,6 +6,7 @@ import type { JournalClient } from './journal-client.js'; import type { Pins, StepDispatchEvent } from './protocol.js'; import type { KernelAgentStep } from './spec.js'; import { runAgentCli } from './worker-cli.js'; +import { agentStepCwd } from './agent-cwd.js'; import { resolveCliModel } from './cli-adapter.js'; import { withWorkerLease } from './worker-lease.js'; import { workerInstruction } from './worker-input.js'; @@ -22,6 +23,14 @@ export interface AgentWorkerOptions { requiredStreams?: string[]; dataDir?: string; onPtyReady?: (path: string) => void; + /** + * The run root a step's declared `cwd` is resolved against and held inside. + * Defaults to this process's working directory, which is what the run root + * is: `flows run` attaches the worker from the directory it was invoked in, + * and that directory is the uploaded tree on the Cloud path. Named here so + * the root a dispatch is measured against is a value, not an assumption. + */ + runRoot?: string; } /** @@ -113,21 +122,28 @@ export class AgentWorker extends EventEmitter { if (communication) { if (!this.options.dataDir) throw new Error('Agent communication requires a worker data directory'); const { completeCommunicationDispatch } = await import('./communication/worker.js'); - await completeCommunicationDispatch(this.client, dispatch, communication, this.options.dataDir); + await completeCommunicationDispatch(this.client, dispatch, communication, this.options.dataDir, this.options.runRoot); return; } let humanIntervention = false; const effectiveModel = typeof spec.cli === 'string' ? resolveCliModel(spec.cli, spec.model) : spec.model; + // Resolved here, before the lease, because this is the process that shares + // the agent's filesystem. A refusal completes the step the way a missing + // CLI does — journaled as `worker_error` with the reason — rather than + // spawning into a directory nobody established is inside the run root. + const cwd = agentStepCwd(spec, dispatch.step_id, this.options.runRoot); const completed: WorkerCliResult = await withWorkerLease(this.client, dispatch, signal => - typeof spec.cli === 'string' && typeof spec.instruction === 'string' - ? runAgentCli(spec.cli, workerInstruction(spec.instruction, dispatch), dispatch.wake_context, effectiveModel, undefined, signal, 'agent', this.options.dataDir === undefined ? undefined : { - dataDir: this.options.dataDir, runId: dispatch.run_id, stepId: dispatch.step_id, attempt: dispatch.attempt, - onReady: this.options.onPtyReady, onDrive: () => { humanIntervention = true; }, - }, typeof spec.cwd === 'string' ? spec.cwd : undefined, - spec.transport === 'relay' ? 'relay' : 'direct', - { runId: dispatch.run_id, stepId: dispatch.step_id, idempotencyKey: dispatch.idempotency_key, - dataDir: this.options.dataDir, resultSchema: spec.verification?.json_schema }) - : Promise.resolve({ exit_code: null, stdout_tail: '', stderr_tail: 'agent step has no declared CLI' })); + 'refusal' in cwd + ? Promise.resolve({ exit_code: null, stdout_tail: '', stderr_tail: cwd.refusal }) + : typeof spec.cli === 'string' && typeof spec.instruction === 'string' + ? runAgentCli(spec.cli, workerInstruction(spec.instruction, dispatch), dispatch.wake_context, effectiveModel, undefined, signal, 'agent', this.options.dataDir === undefined ? undefined : { + dataDir: this.options.dataDir, runId: dispatch.run_id, stepId: dispatch.step_id, attempt: dispatch.attempt, + onReady: this.options.onPtyReady, onDrive: () => { humanIntervention = true; }, + }, cwd.directory, + spec.transport === 'relay' ? 'relay' : 'direct', + { runId: dispatch.run_id, stepId: dispatch.step_id, idempotencyKey: dispatch.idempotency_key, + dataDir: this.options.dataDir, resultSchema: spec.verification?.json_schema }) + : Promise.resolve({ exit_code: null, stdout_tail: '', stderr_tail: 'agent step has no declared CLI' })); const { result, usage } = workerSpend(completed, effectiveModel); const completionReason = result.exit_code === 0 ? 'success' : 'worker_error'; diff --git a/packages/sdk/tests/agent-cwd-live.test.ts b/packages/sdk/tests/agent-cwd-live.test.ts new file mode 100644 index 000000000..3d60c695e --- /dev/null +++ b/packages/sdk/tests/agent-cwd-live.test.ts @@ -0,0 +1,226 @@ +import { execFileSync, spawnSync } from 'node:child_process'; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { JournalClient } from '../src/journal-client.js'; +import { socketPathFor } from '../src/daemon-connection.js'; + +/** + * flows#357, end to end, through the argv the ticket used: `flows run --json + * --local-agent ` against a real daemon, with agents driving two + * checkouts under one run root. + * + * The bug was a split: the SDK accepted `cwd`, lowered it, and the kernel + * refused the run with `invalid_spec: unknown field "cwd" at steps[0]`. Only a + * live run proves the halves agree, because only a live run puts a real spec + * in front of a real kernel — the unit tests either side of the boundary both + * passed while the boundary was broken. + * + * So what is asserted here is where the CLI actually ran, read back from the + * journal and from the disk, and that a declaration this contract cannot + * honour is refused at the edge it belongs to. + */ + +const roots: string[] = []; +const sdk = resolve('.'); +const cli = process.env['FLOWS_TEST_CLI'] ?? join(sdk, 'dist/cli.js'); +const wrapperHelper = resolve('../../testdata/preflight/wrapper-session.mjs'); + +function resolveDaemon(): string { + if (process.env['RELAYFLOWD_BIN']) return process.env['RELAYFLOWD_BIN']; + try { + return join(JSON.parse(execFileSync('sh', [ + resolve('../../ops/cargo.sh'), 'metadata', '--format-version=1', '--no-deps', '--locked', '--offline', + ], { cwd: resolve('../../kernel'), encoding: 'utf8', + env: { ...process.env, RELAYFLOWS_NO_TOOLCHAIN_INSTALL: '1' }, + })).target_directory, 'debug', 'relayflowd'); + } catch (cause) { + throw new Error('Live CLI tests require npm run test:prep or an explicit RELAYFLOWD_BIN.', { cause }); + } +} + +afterEach(() => { + for (const root of roots.splice(0)) { + const connection = join(root, 'data/connection.json'); + if (existsSync(connection)) { + const { pid } = JSON.parse(readFileSync(connection, 'utf8')); + if (typeof pid === 'number') { + try { process.kill(pid, 'SIGTERM'); } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ESRCH') throw error; + } + } + } + rmSync(root, { recursive: true, force: true }); + } +}); + +/** + * A run root holding two checkouts. The wrapper CLI writes `` from its + * instruction into its own working directory and reports that directory, so + * the step's output says where it ran rather than where it was asked to run. + */ +function fixture(body: string) { + const relayflowd = resolveDaemon(); + const root = mkdtempSync(join(tmpdir(), 'flows-cwd-live-')); + roots.push(root); + symlinkSync(join(sdk, 'node_modules'), join(root, 'node_modules')); + mkdirSync(join(root, 'checkouts', 'service-a'), { recursive: true }); + mkdirSync(join(root, 'checkouts', 'service-b'), { recursive: true }); + const wrapper = join(root, 'agent.mjs'); + writeFileSync(wrapper, `#!/usr/bin/env node +import { receiveWrapperRequest } from ${JSON.stringify(wrapperHelper)}; +import { writeFileSync } from 'node:fs'; +if (process.argv[2] === 'auth') process.exit(0); +const request = await receiveWrapperRequest(); +if (request) { + const name = (request.instruction.match(/write:(\\S+)/) ?? [])[1]; + if (name) writeFileSync(name, 'written by ' + name + '\\n'); + console.log('ran in ' + process.cwd()); + process.exit(0); +} +`); + chmodSync(wrapper, 0o755); + writeFileSync(join(root, 'flows.json'), JSON.stringify({ cli: wrapper })); + writeFileSync(join(root, 'package.json'), '{"type":"module"}'); + writeFileSync(join(root, 'edit.flow.ts'), + `import { flow } from '@relayflows/surface';\nexport default flow('edit', async f => {\n${body}\n});\n`); + return { + root, + invoke: () => spawnSync(process.execPath, + [cli, 'run', '--json', '--data-dir', join(root, 'data'), '--local-agent', 'edit.flow.ts', '--input', '{}'], + { cwd: root, encoding: 'utf8', timeout: 120_000, env: { ...process.env, RELAYFLOWD_BIN: relayflowd } }), + }; +} + +interface Entry { entry_type: string; step_id?: string; payload: { completionReason?: string; output?: unknown } } + +/** Every `step.completed` across the child runs the root run lists, by step id. */ +async function readJournal(root: string, rootRunId: string, extraRunIds: string[] = []) { + const client = new JournalClient(socketPathFor(join(root, 'data')), { requestTimeoutMs: 5000 }); + await client.connect(); + await client.hello('agent-cwd-live-test'); + try { + const rootEntries = (await client.journalRead(rootRunId, 1, 1000)).entries as Entry[]; + const rootDone = rootEntries.find(e => e.entry_type === 'step.completed' && e.step_id === 'authored-root'); + const journalSteps = (rootDone?.payload.output as { journalSteps?: Array<{ id: string; runId: string }> } | undefined)?.journalSteps ?? []; + const completed = new Map(); + for (const runId of [...journalSteps.map(step => step.runId), ...extraRunIds]) { + const entries = (await client.journalRead(runId, 1, 1000)).entries as Entry[]; + for (const entry of entries) { + if (entry.entry_type === 'step.completed' && entry.step_id !== undefined) completed.set(entry.step_id, entry); + } + } + return completed; + } finally { + client.close(); + } +} + +function reportedRunIds(report: { runId?: string; diagnostics?: Array<{ message: string }> }): string[] { + const ids = new Set(); + if (report.runId) ids.add(report.runId); + for (const d of report.diagnostics ?? []) { + for (const m of d.message.matchAll(/\b(01[0-9A-HJKMNP-TV-Z]{24})\b/g)) ids.add(m[1]!); + } + return [...ids]; +} + +type Output = { stdout_tail?: string; artifacts?: string[] }; + +describe('agents drive two checkouts under one run root', () => { + it('runs each CLI in its declared directory, and the run is not refused as an unknown field', async () => { + const f = fixture(` + const a = await f.agent('edit-a', { task: 'edit write:a.txt', cwd: 'checkouts/service-a' }); + const b = await f.agent('edit-b', { task: 'edit write:b.txt', cwd: 'checkouts/service-b' }); + const root = await f.agent('note-root', { task: 'edit write:root.txt' }); + if (!a.artifacts.includes('a.txt')) throw new Error('a artifacts: ' + JSON.stringify(a.artifacts)); + if (!b.artifacts.includes('b.txt')) throw new Error('b artifacts: ' + JSON.stringify(b.artifacts)); + if (!root.artifacts.includes('root.txt')) throw new Error('root artifacts: ' + JSON.stringify(root.artifacts)); + f.done('success');`); + const result = f.invoke(); + // The ticket's symptom, pinned by its own words: the run reached the + // kernel and was accepted, rather than coming back a protocol error. + expect(result.stdout + result.stderr).not.toContain('unknown field "cwd"'); + expect(result.status, result.stderr + result.stdout).toBe(0); + const report = JSON.parse(result.stdout) as { runId: string; completionReason: string }; + expect(report.completionReason).toBe('success'); + + const completed = await readJournal(f.root, report.runId); + // Where each CLI actually ran, as the process itself reported it. + expect((completed.get('agent-1')!.payload.output as Output).stdout_tail) + .toContain(join(f.root, 'checkouts', 'service-a')); + expect((completed.get('agent-2')!.payload.output as Output).stdout_tail) + .toContain(join(f.root, 'checkouts', 'service-b')); + + // Artifacts are measured in the directory the agent ran in, so the paths a + // gate names are relative to THAT directory and not to the run root. + expect((completed.get('agent-1')!.payload.output as Output).artifacts).toEqual(['a.txt']); + expect((completed.get('agent-2')!.payload.output as Output).artifacts).toEqual(['b.txt']); + + // And the files landed only under the checkout that asked for them. + expect(existsSync(join(f.root, 'checkouts', 'service-a', 'a.txt'))).toBe(true); + expect(existsSync(join(f.root, 'checkouts', 'service-b', 'b.txt'))).toBe(true); + expect(existsSync(join(f.root, 'checkouts', 'service-a', 'b.txt'))).toBe(false); + expect(existsSync(join(f.root, 'checkouts', 'service-b', 'a.txt'))).toBe(false); + // A step that declares nothing still runs in the run root. + expect(existsSync(join(f.root, 'root.txt'))).toBe(true); + }, 120_000); + + it('gates on an artifact path relative to the agent directory', async () => { + const f = fixture(` + await f.agent('edit-a', { task: 'edit write:a.txt', cwd: 'checkouts/service-a' }) + .gate({ type: 'artifact_exists', path: 'a.txt' }); + f.done('success');`); + const result = f.invoke(); + expect(result.status, result.stderr + result.stdout).toBe(0); + }, 120_000); +}); + +describe('a declaration this contract cannot honour is refused at its own edge', () => { + it('refuses a path that climbs out of the run root without starting a run at all', async () => { + const f = fixture(` + await f.agent('escape', { task: 'edit write:a.txt', cwd: '../elsewhere' }); + f.done('success');`); + const result = f.invoke(); + const said = result.stdout + result.stderr; + expect(result.status, said).not.toBe(0); + const report = JSON.parse(result.stdout) as { runId?: string; diagnostics: Array<{ message: string }> }; + // The refusal is the SDK's own, in its own words, about the author's own + // option — not the kernel's `unknown field "cwd"` relayed back afterwards. + expect(report.diagnostics.map(d => d.message).join('\n')) + .toContain('f.agent options.cwd: expected a run-root-relative path'); + expect(said).not.toContain('unknown field "cwd"'); + // And no run exists to inspect: the declaration was refused before one was + // started, which is the half of the ticket that `flows check` also covers. + expect(report.runId).toBeUndefined(); + }, 120_000); + + it('fails the step with the reason when the directory is not there, rather than running somewhere else', async () => { + const f = fixture(` + await f.agent('missing', { task: 'edit write:a.txt', cwd: 'checkouts/service-c' }); + f.done('success');`); + const result = f.invoke(); + expect(result.status, result.stderr + result.stdout).toBe(1); + const report = JSON.parse(result.stdout) as { + runId: string; diagnostics: Array<{ kind: string; stderrTail?: string }>; + }; + // The kernel accepted the spec and dispatched it; the worker — the process + // that shares the agent's filesystem — is what refused, and said why. + const failure = report.diagnostics.find(d => d.kind === 'step_failed'); + expect(failure?.stderrTail).toContain('agent step "agent-1": cwd "checkouts/service-c"'); + expect(failure?.stderrTail).toContain('no such directory under the run root'); + + // On a `worker_error` the kernel nulls `output` and keeps the wrapper as + // the execution gate's detail, so that is where the reason is journaled. + const completed = await readJournal(f.root, report.runId, reportedRunIds(report)); + const payload = completed.get('agent-1')!.payload as { verification?: { detail?: string; verdict?: string } }; + expect(payload.verification?.verdict).toBe('fail'); + expect(payload.verification?.detail).toContain('no such directory under the run root'); + + // Nothing was invented to make the declaration true, and the agent did not + // quietly fall back to the run root. + expect(existsSync(join(f.root, 'checkouts', 'service-c'))).toBe(false); + expect(existsSync(join(f.root, 'a.txt'))).toBe(false); + }, 120_000); +}); diff --git a/packages/sdk/tests/agent-cwd.test.ts b/packages/sdk/tests/agent-cwd.test.ts new file mode 100644 index 000000000..f2d16127e --- /dev/null +++ b/packages/sdk/tests/agent-cwd.test.ts @@ -0,0 +1,261 @@ +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { realpathSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { + AgentCwdError, + agentCwdDeclarationError, + agentStepCwd, + resolveAgentCwd, +} from '../src/agent-cwd.js'; +import { compileSpec, kernelToAuthoring, toKernelSpec } from '../src/compile.js'; +import { validateSpec } from '../src/validate.js'; +import { runCli, type CheckReport, type CliIo } from '../src/cli.js'; + +// flows#357. `cwd` was accepted by the SDK, lowered into the step spec, and +// then refused by the kernel with `unknown field "cwd" at steps[0]` — after +// `flows check` had passed. These pin the declaration rule (shared with the +// kernel through testdata/agent-cwd-cases.json), the dialect round trip, and +// the containment the worker does on the host that runs the CLI. + +const cases: Array<{ name: string; cwd: string; valid: boolean }> = + JSON.parse(readFileSync(new URL('../../../testdata/agent-cwd-cases.json', import.meta.url), 'utf8')); + +const roots: string[] = []; +afterEach(() => { + for (const root of roots.splice(0)) rmSync(root, { recursive: true, force: true }); +}); + +/** A run root with two checkouts and a sibling name that merely shares a prefix. */ +function runRoot(): string { + const root = realpathSync(mkdtempSync(join(tmpdir(), 'flows-agent-cwd-'))); + roots.push(root); + mkdirSync(join(root, 'checkouts', 'service-a'), { recursive: true }); + mkdirSync(join(root, 'checkouts', 'service-b'), { recursive: true }); + writeFileSync(join(root, 'checkouts', 'service-a', 'README'), 'a'); + return root; +} + +const agentFlow = (cwd: unknown, extra: Record = {}): unknown => ({ + version: '0.1.0', + steps: [{ id: 'edit', type: 'agent', instruction: 'Edit.', cwd, ...extra }], +}); + +describe('the declaration rule is the kernel rule', () => { + for (const test of cases) { + it(`${test.valid ? 'accepts' : 'refuses'} ${test.name}`, () => { + expect(agentCwdDeclarationError(test.cwd) === undefined).toBe(test.valid); + expect(validateSpec(agentFlow(test.cwd)).ok).toBe(test.valid); + }); + } + + // `Option` in the kernel reads an explicit null as absence. The SDK + // must not let one through under a different name for the same silence. + it.each([ + ['null', null], + ['a number', 7], + ['an array', ['checkouts/service-a']], + ['an object', { path: 'checkouts/service-a' }], + ['a boolean', true], + ])('refuses %s rather than treating it as absence', (_label, cwd) => { + expect(agentCwdDeclarationError(cwd)).toContain('expected a run-root-relative path'); + const result = validateSpec(agentFlow(cwd)); + expect(result.ok).toBe(false); + expect(result.errors.join('\n')).toContain('steps[0].cwd'); + }); + + it('says nothing about a step that declares no directory', () => { + expect(agentCwdDeclarationError(undefined)).toContain('expected a run-root-relative path'); + expect(validateSpec({ version: '0.1.0', steps: [{ id: 'edit', type: 'agent', instruction: 'Edit.' }] }).ok) + .toBe(true); + }); +}); + +describe('the declaration survives the kernel dialect', () => { + it('round-trips through snake_case and back', () => { + const compiled = compileSpec(agentFlow('checkouts/service-a')); + const kernel = toKernelSpec(compiled); + expect(kernel.steps[0]).toMatchObject({ cwd: 'checkouts/service-a' }); + expect(kernelToAuthoring(kernel)).toEqual(compiled); + }); + + // The inverse mapping dropped `cwd` on the floor, so a kernel spec that + // declared a directory came back as one that did not — the same class of + // silence as the field the kernel refused outright. + it('does not lose the directory on the way back from the kernel', () => { + const kernel = toKernelSpec(compileSpec(agentFlow('checkouts/service-a'))); + const authoring = kernelToAuthoring(kernel) as { steps: Array<{ cwd?: string }> }; + expect(authoring.steps[0]?.cwd).toBe('checkouts/service-a'); + }); + + it('refuses a cwd the authoring dialect would have to invent a value for', () => { + const kernel = toKernelSpec(compileSpec(agentFlow('checkouts/service-a'))); + (kernel.steps[0] as Record)['cwd_hint'] = 'checkouts/service-b'; + expect(() => kernelToAuthoring(kernel)).toThrow('steps[0]'); + }); +}); + +describe('the worker resolves the declaration against the run root', () => { + it('runs in the run root itself when nothing is declared', () => { + expect(resolveAgentCwd(runRoot(), undefined)).toBeUndefined(); + }); + + it('resolves a nested checkout to its real path', () => { + const root = runRoot(); + expect(resolveAgentCwd(root, 'checkouts/service-a')).toBe(join(root, 'checkouts', 'service-a')); + }); + + it('resolves two sibling checkouts independently', () => { + const root = runRoot(); + expect(resolveAgentCwd(root, 'checkouts/service-a')).not.toBe(resolveAgentCwd(root, 'checkouts/service-b')); + }); + + it('refuses a directory that is not there', () => { + expect(() => resolveAgentCwd(runRoot(), 'checkouts/service-c')) + .toThrow(/no such directory under the run root/); + }); + + it('refuses a path that names a regular file', () => { + expect(() => resolveAgentCwd(runRoot(), 'checkouts/service-a/README')) + .toThrow(/is not a directory/); + }); + + // Containment is component-aware on the symlink-free form of both paths, so + // a name that merely shares the root's prefix is outside it. + it('refuses a sibling whose name starts with the run root', () => { + const root = runRoot(); + const sibling = `${root}-old`; + mkdirSync(sibling, { recursive: true }); + roots.push(sibling); + // Reached through a symlink, because the lexical rule refuses `..`. + symlinkSync(sibling, join(root, 'nearby'), 'dir'); + expect(() => resolveAgentCwd(root, 'nearby')).toThrow(/outside the run root/); + }); + + it('refuses a symlink pointing out of the run root', () => { + const root = runRoot(); + const outside = realpathSync(mkdtempSync(join(tmpdir(), 'flows-agent-cwd-outside-'))); + roots.push(outside); + symlinkSync(outside, join(root, 'escape'), 'dir'); + expect(() => resolveAgentCwd(root, 'escape')).toThrow(/outside the run root/); + }); + + it('follows a symlink that stays inside the run root', () => { + const root = runRoot(); + symlinkSync(join(root, 'checkouts', 'service-a'), join(root, 'current'), 'dir'); + expect(resolveAgentCwd(root, 'current')).toBe(join(root, 'checkouts', 'service-a')); + }); + + it('refuses a declaration the lexical rule already rejects, without touching the disk', () => { + expect(() => resolveAgentCwd(runRoot(), '../sibling')).toThrow(AgentCwdError); + expect(() => resolveAgentCwd(runRoot(), '/etc')).toThrow(/not an absolute path/); + }); + + // The relay agent runs on another host. Forwarding the string would let a + // declaration this contract promises to contain go unchecked. + it('refuses a directory declared on a relay-dispatched step', () => { + expect(() => resolveAgentCwd(runRoot(), 'checkouts/service-a', 'relay')) + .toThrow(/not supported with transport "relay"/); + expect(validateSpec(agentFlow('checkouts/service-a', { transport: 'relay' })).ok).toBe(false); + }); + + it('leaves a relay step that declares no directory alone', () => { + expect(resolveAgentCwd(runRoot(), undefined, 'relay')).toBeUndefined(); + expect(validateSpec({ + version: '0.1.0', + steps: [{ id: 'edit', type: 'agent', instruction: 'Edit.', transport: 'relay' }], + }).ok).toBe(true); + }); +}); + +describe('a refused dispatch is reported, not thrown at the worker', () => { + it('names the step and the declaration', () => { + const outcome = agentStepCwd({ cwd: '../sibling' }, 'edit', runRoot()); + expect(outcome).toEqual({ refusal: expect.stringContaining('agent step "edit": cwd "../sibling"') }); + }); + + it('reports a directory that is not there rather than spawning somewhere else', () => { + const outcome = agentStepCwd({ cwd: 'checkouts/service-c' }, 'edit', runRoot()); + expect('refusal' in outcome && outcome.refusal).toContain('no such directory under the run root'); + }); + + it('hands back the resolved directory when the declaration holds', () => { + const root = runRoot(); + expect(agentStepCwd({ cwd: 'checkouts/service-b' }, 'edit', root)) + .toEqual({ directory: join(root, 'checkouts', 'service-b') }); + }); + + it('hands back nothing to resolve when the step declares nothing', () => { + expect(agentStepCwd({}, 'edit', runRoot())).toEqual({ directory: undefined }); + }); +}); + +/** + * The ticket's headline: "`flows check` passes so nothing catches it before the + * run." The declaration is lexical, so `check` is exactly where an unusable one + * must be named — before a run exists, not at dispatch. + */ +describe('flows check names a bad declaration before a run exists', () => { + function project(cwd: string): string { + const directory = mkdtempSync(join(tmpdir(), 'flows-agent-cwd-check-')); + roots.push(directory); + writeFileSync(join(directory, 'flows.json'), JSON.stringify({ executors: [] })); + const cli = join(directory, 'agent-cli'); + writeFileSync(cli, `#!/bin/sh +if [ "\${1-}" = "--relayflows-adapter-v1" ]; then + printf '%s\\n' 'relayflows-agent-cli-v1' + exit 0 +fi +test "$1" = "auth" && test "$2" = "status" +`); + chmodSync(cli, 0o755); + const path = join(directory, 'edit.flow.yaml'); + writeFileSync(path, [ + "version: '0.1.0'", + 'steps:', + ' - id: edit', + ' type: agent', + ` cli: ${JSON.stringify(cli)}`, + ' instruction: Edit the checkout.', + ` cwd: ${JSON.stringify(cwd)}`, + '', + ].join('\n')); + return path; + } + + function capture(): { io: CliIo; stdout: string[]; stderr: string[] } { + const stdout: string[] = []; + const stderr: string[] = []; + return { io: { stdout: line => stdout.push(line), stderr: line => stderr.push(line) }, stdout, stderr }; + } + + it('passes a run-root-relative checkout', async () => { + const output = capture(); + expect(await runCli(['check', project('checkouts/service-a')], output.io), output.stderr.join('\n')).toBe(0); + expect(output.stdout.join('\n')).toContain('CHECK PASSED'); + }); + + it.each([ + ['an absolute path', '/srv/checkouts/service-a', 'not an absolute path'], + ['a path that climbs out', '../service-a', 'without empty, "." or ".." components'], + ['a padded path', ' checkouts/service-a', 'without surrounding whitespace'], + ])('refuses %s with a reason naming the step', async (_label, cwd, reason) => { + const output = capture(); + const code = await runCli(['check', project(cwd)], output.io); + expect(code).not.toBe(0); + const said = output.stderr.join('\n'); + expect(said).toContain('steps[0].cwd'); + expect(said).toContain(reason); + expect(output.stdout.join('\n')).not.toContain('CHECK PASSED'); + }); + + it('reports the refusal in --json without writing prose to stdout', async () => { + const output = capture(); + expect(await runCli(['check', '--json', project('/srv/checkouts')], output.io)).not.toBe(0); + expect(output.stdout).toHaveLength(1); + const report = JSON.parse(output.stdout[0]!) as CheckReport; + expect(report.ok).toBe(false); + expect(JSON.stringify(report.diagnostics)).toContain('steps[0].cwd'); + }); +}); diff --git a/packages/sdk/tests/authored-agent-artifacts.test.ts b/packages/sdk/tests/authored-agent-artifacts.test.ts index d23297124..c348fd7af 100644 --- a/packages/sdk/tests/authored-agent-artifacts.test.ts +++ b/packages/sdk/tests/authored-agent-artifacts.test.ts @@ -103,7 +103,7 @@ describe('f.agent artifacts (local-agent path)', () => { await client.hello('authored-artifacts-test'); try { const handle = flow('artifact-test', async (f) => { - const result = await f.agent('writer', { task: 'write research/notes.md', cwd: root! }); + const result = await f.agent('writer', { task: 'write research/notes.md' }); expect(result.artifacts).toEqual(['research/notes.md']); f.done('success'); }); @@ -130,7 +130,7 @@ describe('f.agent artifacts (local-agent path)', () => { // effect for any agent-type step, but a relay-transport step ran on // a remote host — this process's filesystem is not where it wrote, // so the local snapshot must not be trusted here. - const result = await f.agent('writer', { task: 'write research/notes.md', cwd: root!, transport: 'relay' }); + const result = await f.agent('writer', { task: 'write research/notes.md', transport: 'relay' }); expect(result.artifacts).toEqual([]); f.done('success'); }); @@ -156,9 +156,9 @@ describe('f.agent artifacts (local-agent path)', () => { await client.hello('authored-artifacts-test-2'); try { const handle = flow('artifact-test-no-local', async (f) => { - const first = await f.agent('writer', { task: 'write research/notes.md', cwd: root!, workspace: 'research' }); + const first = await f.agent('writer', { task: 'write research/notes.md', workspace: 'research' }); expect(first.artifacts).toEqual(['research/notes.md']); - const second = await f.agent('writer-again', { task: 'write nothing', cwd: root!, workspace: 'research' }); + const second = await f.agent('writer-again', { task: 'write nothing', workspace: 'research' }); expect(second.artifacts).toEqual([]); f.done('success'); }); diff --git a/packages/sdk/tests/authored-agent-permissions.test.ts b/packages/sdk/tests/authored-agent-permissions.test.ts index 70ca71c3f..e9383a104 100644 --- a/packages/sdk/tests/authored-agent-permissions.test.ts +++ b/packages/sdk/tests/authored-agent-permissions.test.ts @@ -97,7 +97,9 @@ process.stdout.write('unused'); }); it('accepts permissions without workspace on the local stream path', async () => { - const step = await capture({ task: 'x', cwd: root, permissions: { accessPreset: 'readonly' } }, true); + // `cwd` is run-root-relative (flows#357); it rides beside permissions here + // and is never resolved, because no worker is attached to this fake kernel. + const step = await capture({ task: 'x', cwd: 'checkout', permissions: { accessPreset: 'readonly' } }, true); expect(step.permissions).toEqual({ access_preset: 'readonly' }); }); diff --git a/packages/sdk/tests/spec-parity.test.ts b/packages/sdk/tests/spec-parity.test.ts index 48e25ac2d..572e2feb8 100644 --- a/packages/sdk/tests/spec-parity.test.ts +++ b/packages/sdk/tests/spec-parity.test.ts @@ -25,7 +25,7 @@ function fixture(name: string): string { } describe('spec parity: one dialect at the SDK<->kernel boundary', () => { - for (const name of ['hello-deterministic', 'hello-ladder', 'hello-llm', 'hello-agent', 'step-memory', 'step-placement']) { + for (const name of ['hello-deterministic', 'hello-ladder', 'hello-llm', 'hello-agent', 'step-memory', 'step-placement', 'agent-cwd']) { it(`compiles ${name} to the pinned canonical JSON`, () => { const yaml = fixture(`${name}.flow.yaml`); const canonical = compileYamlToCanonicalJson(yaml); diff --git a/packages/sdk/tests/worker-transcript.test.ts b/packages/sdk/tests/worker-transcript.test.ts index f745c69cc..ae60262a0 100644 --- a/packages/sdk/tests/worker-transcript.test.ts +++ b/packages/sdk/tests/worker-transcript.test.ts @@ -92,13 +92,15 @@ describe('the agent worker journals the digest in trajectory_tail on every compl // The data dir is outside the agent's cwd, as it is in Cloud's sandbox // (`join(stateDir, "journal")`): the transcript file is not an artifact. mkdirSync(join(root, 'workspace')); - const worker = new AgentWorker(client, { workerId: 'w', pins, dataDir: join(root, 'data') }); + // `cwd` is run-root-relative (flows#357), so the root the worker measures + // it against is named here rather than being this process's directory. + const worker = new AgentWorker(client, { workerId: 'w', pins, dataDir: join(root, 'data'), runRoot: root }); const errors: unknown[] = []; worker.on('error', error => errors.push(error)); await worker.attach(); (client as unknown as EventEmitter).emit('step.dispatch', { run_id: 'run-a', step_id: 'agent-1', attempt, step_type: 'agent', - spec: { cli: claude, instruction: 'probe', cwd: join(root, 'workspace') }, pins, + spec: { cli: claude, instruction: 'probe', cwd: 'workspace' }, pins, lease_id: 'lease', lease_deadline_ms: Date.now() + 30_000, idempotency_key: 'k', }); await worker.close(); diff --git a/packages/surface/src/context.ts b/packages/surface/src/context.ts index 38b48ce74..53371d9c9 100644 --- a/packages/surface/src/context.ts +++ b/packages/surface/src/context.ts @@ -32,7 +32,17 @@ export interface AgentOptions { permissions?: PermissionsSpec; cli?: string; model?: string; - /** Working directory for the CLI subprocess; defaults to the flow-runner's cwd. */ + /** + * Directory this agent's CLI is spawned in, relative to the run root — the + * flow-runner's working directory, and where the CLI runs when this is + * absent. One flow can therefore drive agents in sibling checkouts. The + * path must be relative and free of `.`, `..` and empty components, and + * must name an existing directory inside the run root at dispatch; + * anything else refuses the step instead of running it somewhere else. + * `artifacts` are reported relative to this directory. It is a declaration + * of where to start, not a sandbox. Not supported with + * `transport: 'relay'`. See docs/SURFACE.md. + */ cwd?: string; /** * Dispatch transport (flows#385). `'direct'` (default) spawns the CLI as a diff --git a/summary.md b/summary.md new file mode 100644 index 000000000..9f48fd2a8 --- /dev/null +++ b/summary.md @@ -0,0 +1,170 @@ +# `f.agent options.cwd`: accepted by the kernel, contained by the worker + +Closes the accept-then-refuse split in flows#357. `f.agent({ cwd })` was +type-checked by the SDK, lowered into the step spec, passed `flows check`, and +then killed the run at dispatch: + +``` +FAILED [protocol_error] relayflowd could not complete the run request: invalid_spec: unknown field "cwd" at steps[0] — refusing to guess (fail closed) +``` + +The ticket offered three resolutions and called this one — accepting, then +refusing at dispatch — "the worst of the three". This PR takes the first: the +kernel accepts `cwd` on agent steps and the worker runs the CLI in that +directory, held inside the run's tree, as the ticket asked. + +## The contract + +`cwd` is a **run-root-relative** path. The run root is the working directory of +the process running the agent worker: the directory `flows run` was invoked +from, and the uploaded tree on the Cloud path (docs/CLOUD.md, "Code sync"). + +Relative-only is the substantive decision. An absolute host path is not a +portable declaration — it means nothing on another machine or inside a Cloud +upload — and it cannot be contained lexically, so allowing it would just move +the late refusal somewhere else. The ticket's own phrase, "still inside the +run's tree", is the rule this implements. + +The contract is split in two, because they are two different facts: + +- **The declaration** is lexical and filesystem-free, so `flows check`, the + kernel's `validate()` and the worker all answer identically for the same + spec. A path must be relative, non-empty, unpadded, NUL-free, URI-free, and + free of empty / `.` / `..` components. +- **The target** is resolved only in the worker that spawns the CLI — the one + process provably sharing the agent's filesystem. It must exist, be a + directory, and have a symlink-free path inside the symlink-free run root. + Re-checked at every dispatch even when a preflight passed, because the + filesystem moves underneath a spec. + +Refusals land at the earliest edge that can see them. A bad *declaration* is +refused by `flows check` and by the authored body before any run exists +(`report.runId` is `undefined` — nothing is started). A bad *target* completes +the step `worker_error` with the reason journaled, the same way a missing CLI +does, instead of spawning somewhere nobody established is inside the tree. + +`cwd` is a declaration of where to start, not a sandbox — nothing stops a CLI +from writing outside it. Per-step scoping remains `permissions`, recorded and +not enforced (gate 8 / #442). This is stated in `docs/SURFACE.md`, as the +ticket requested, next to `AgentOptions`, along with why workspace surfaces are +not a directory selector. + +`cwd` is **not supported with `transport: 'relay'`** and is refused rather than +forwarded: the agent runs on another host, where this worker can neither +resolve the directory nor hold it inside the run root. Failing closed beats +forwarding a string the contract promises to contain. + +## Changes + +**Kernel** (`kernel/relayflowd-core/src/spec.rs`) +- `"cwd"` added to `STEP_AGENT_FIELDS`; `StepKind::Agent` gains + `cwd: Option` with `#[serde(default, skip_serializing_if = ...)]`, so + every pre-existing fixture stays byte-identical and the spec hash is unmoved. +- A **pre-serde shape check** refuses a non-string `cwd`. `Option` + reads an explicit `null` as absence, which would silently run the step in the + default directory under a spec that declared otherwise. +- `validate()` refuses a non-run-root-relative value with a new + `SpecError::InvalidAgentCwd { step, cwd }`, via `is_run_root_relative_path` + layered on the existing `path_surface_identity` rule. + +**SDK** +- `packages/sdk/src/agent-cwd.ts` (new) holds the whole policy: the lexical + declaration rule, the relay refusal, the resolve-and-contain step, and the + dispatch-time `agentStepCwd` that *reports* a refusal instead of throwing, so + it lands on the step's completion rather than on the worker. +- `worker.ts` resolves before the lease and before spawning or scanning + artifacts; `AgentWorkerOptions.runRoot` names the root a dispatch is measured + against instead of assuming it. +- `communication/worker.ts` previously passed `spec.cwd ?? process.cwd()` + straight to the PTY spawn — unresolved and unchecked. It now resolves through + the same module against the same `runRoot`, so the two worker paths cannot + disagree about where the run root is. +- `compile.ts`: `cwd` added to `kernelStepToAuthoring`, repairing inverse-mapping + data loss that silently dropped the field on the kernel→authoring round trip. +- `validate.ts` and `authored-worker-step.ts` raise the same refusal at + `flows check` and authoring time. + +### Cross-language whitespace parity + +Rust's `char::is_whitespace` is Unicode `White_Space` exactly; JavaScript's +`\s` is `White_Space` without U+0085 and with U+FEFF. Left alone, a path padded +with either character would pass `flows check` and be refused by the kernel — +exactly the split this PR closes. Each side names the other's missing +character, so both refuse `White_Space ∪ {U+FEFF}`. Both dialects are tested +against the shared corpus. + +## Tests + +- `testdata/agent-cwd-cases.json` — 24 shared declaration cases, consumed by + both `kernel/relayflowd-core/tests/spec_parity.rs` (5 new tests) and + `packages/sdk/tests/agent-cwd.test.ts` (53 tests), so the two dialects are + proven to answer identically rather than assumed to. +- `testdata/agent-cwd.flow.yaml` + canonical JSON + sha256 pin byte and hash + agreement across the boundary. +- `packages/sdk/tests/agent-cwd.test.ts` also covers `flows check` end to end + on a real project — the ticket's "`flows check` passes so nothing catches it + before the run" is now a test. +- `packages/sdk/tests/agent-cwd-live.test.ts` (new, 4 tests) drives the real + daemon through `flows run --local-agent` with **two agents in two nested + checkouts**, asserting each CLI's reported `process.cwd()`, that artifacts are + relative to the agent's directory, that an `artifact_exists` gate resolves + against it, and that neither refusal path runs anything anywhere. + +### Mutation-verified + +The live test was proven to catch the original bug, not merely to pass +alongside the fix. Deleting `"cwd",` from `STEP_AGENT_FIELDS` and rebuilding +the daemon turns it red, reproducing the ticket's error character for +character: + +``` +FAILED [protocol_error] relayflowd could not complete the run request: invalid_spec: unknown field "cwd" at steps[0] — refusing to guess (fail closed) +``` + +Under the same mutation Rust `spec_parity` goes `15 passed` → `13 passed; 2 failed`. +Restoring the line returns both to green. + +## Results + +``` +kernel: sh ops/cargo.sh test --workspace 265 passed; 0 failed +sdk: npx vitest run tests/agent-cwd.test.ts tests/agent-cwd-live.test.ts tests/communication-*.test.ts + Test Files 11 passed (11) Tests 110 passed (110) +sdk: npx vitest run (full) + Test Files 7 failed | 149 passed | 3 skipped (159) + Tests 40 failed | 2413 passed | 25 skipped (2478) +typecheck: SURFACE OK · SDK OK · SDK TESTS OK · SDK TYPE-TESTS OK +``` + +The 40 failures are **pre-existing and environmental**, not caused by this +change. Proven by stashing the entire working tree +(`git stash push --include-untracked`), rebuilding, and re-running: identical +files and identical per-file counts — stuck-run-triage 22, live-kernel 8, +webhook-live 6, provider-trigger-executor 3, mcp 1, +communication-mixed-resume 1, authored-node-runtime 1. Those files hard-code +`kernel/target/{debug,release}/relayflowd`, which this toolchain does not use; +the live tests added here resolve the binary through `ops/cargo.sh metadata` +and run. + +`packages/schema/flows.schema.json` was regenerated with +`node scripts/generate-json-schema.mjs`, never hand-edited; the diff is exactly +the two new `cwd` descriptions. + +## Deliberate behaviour change + +Absolute `cwd` values that SDK-only paths previously tolerated are now refused. +Two test files declared one; in both, the declaration was inert decoration that +nothing ever resolved (no worker is attached to those fake kernels), so both +were repaired to the run-root-relative form. No flow that ran before can stop +running: a spec carrying `cwd` could not reach a run at all. + +## Follow-up, deliberately not in this PR + +`transport` has the same inverse-mapping gap in `kernelStepToAuthoring` that +`cwd` had. It is a separate defect on a separate field and is left for its own +change rather than folded in here. + +## Not run + +`clippy` and `rustfmt` are not installed for this toolchain +(`error: 'cargo-clippy' is not installed`). diff --git a/testdata/agent-cwd-cases.json b/testdata/agent-cwd-cases.json new file mode 100644 index 000000000..f12ecbfc7 --- /dev/null +++ b/testdata/agent-cwd-cases.json @@ -0,0 +1,122 @@ +[ + { + "name": "nested checkout", + "cwd": "checkouts/service-a", + "valid": true + }, + { + "name": "single component", + "cwd": "worktree", + "valid": true + }, + { + "name": "hidden component", + "cwd": ".worktrees/service-a", + "valid": true + }, + { + "name": "a component that only looks like a traversal", + "cwd": "..data/checkout", + "valid": true + }, + { + "name": "a component containing a space", + "cwd": "checkouts/service a", + "valid": true + }, + { + "name": "deeply nested", + "cwd": "a/b/c/d/e", + "valid": true + }, + { + "name": "absolute", + "cwd": "/srv/checkouts/service-a", + "valid": false + }, + { + "name": "absolute root", + "cwd": "/", + "valid": false + }, + { + "name": "parent traversal", + "cwd": "../sibling", + "valid": false + }, + { + "name": "interior traversal", + "cwd": "checkouts/../../etc", + "valid": false + }, + { + "name": "current directory", + "cwd": ".", + "valid": false + }, + { + "name": "interior current directory", + "cwd": "checkouts/./service-a", + "valid": false + }, + { + "name": "empty", + "cwd": "", + "valid": false + }, + { + "name": "trailing separator", + "cwd": "checkouts/service-a/", + "valid": false + }, + { + "name": "doubled separator", + "cwd": "checkouts//service-a", + "valid": false + }, + { + "name": "leading space", + "cwd": " checkouts/service-a", + "valid": false + }, + { + "name": "trailing tab", + "cwd": "checkouts/service-a\t", + "valid": false + }, + { + "name": "leading no-break space", + "cwd": "\u00a0checkouts", + "valid": false + }, + { + "name": "trailing next line", + "cwd": "checkouts\u0085", + "valid": false + }, + { + "name": "leading byte order mark", + "cwd": "\ufeffcheckouts", + "valid": false + }, + { + "name": "embedded NUL", + "cwd": "checkouts/service\u0000a", + "valid": false + }, + { + "name": "URI-like mount identity", + "cwd": "s3://bucket/checkout", + "valid": false + }, + { + "name": "URI-like below a component", + "cwd": "checkouts/s3://bucket", + "valid": false + }, + { + "name": "scheme separator alone", + "cwd": "://checkouts", + "valid": false + } +] diff --git a/testdata/agent-cwd.flow.yaml b/testdata/agent-cwd.flow.yaml new file mode 100644 index 000000000..c5a70da34 --- /dev/null +++ b/testdata/agent-cwd.flow.yaml @@ -0,0 +1,27 @@ +# The multi-checkout shape flows#357 was filed from: one flow driving agents +# in two sibling directories of the run's own tree. `cwd` is declared on the +# agent steps only, and survives the whole boundary — SDK authoring -> +# compiler -> canonical JSON -> kernel parse -> kernel re-serialize -> the +# same sha256. A declaration that cannot make that trip is one the daemon +# refuses after `flows check` has already passed. +version: '0.1.0' +name: agent-cwd +description: Two agents run in two checkouts beneath the run root. +steps: + - id: prepare + type: deterministic + command: "printf ready" + - id: edit-service-a + type: agent + dependsOn: [prepare] + instruction: "Apply the change in service A." + cwd: checkouts/service-a + - id: edit-service-b + type: agent + dependsOn: [prepare] + instruction: "Apply the change in service B." + cwd: checkouts/service-b + - id: review-root + type: agent + dependsOn: [edit-service-a, edit-service-b] + instruction: "Review both checkouts from the run root." diff --git a/testdata/agent-cwd.spec.canonical.json b/testdata/agent-cwd.spec.canonical.json new file mode 100644 index 000000000..f014de743 --- /dev/null +++ b/testdata/agent-cwd.spec.canonical.json @@ -0,0 +1 @@ +{"description":"Two agents run in two checkouts beneath the run root.","name":"agent-cwd","steps":[{"command":"printf ready","depends_on":[],"id":"prepare","max_iterations":1,"retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"deterministic","verification":{}},{"cwd":"checkouts/service-a","depends_on":["prepare"],"id":"edit-service-a","instruction":"Apply the change in service A.","max_iterations":1,"recovery_mode":"reset","retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"agent","verification":{}},{"cwd":"checkouts/service-b","depends_on":["prepare"],"id":"edit-service-b","instruction":"Apply the change in service B.","max_iterations":1,"recovery_mode":"reset","retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"agent","verification":{}},{"depends_on":["edit-service-a","edit-service-b"],"id":"review-root","instruction":"Review both checkouts from the run root.","max_iterations":1,"recovery_mode":"reset","retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"agent","verification":{}}],"version":"0.1.0"} diff --git a/testdata/agent-cwd.spec.sha256 b/testdata/agent-cwd.spec.sha256 new file mode 100644 index 000000000..3637354ae --- /dev/null +++ b/testdata/agent-cwd.spec.sha256 @@ -0,0 +1 @@ +85d1ab87c0567ad17a5a3953c3f9315c81be4e0d8f249fefdd47545a19687b09 From 557745d262ee04e42fd7426724a96a4b44afcc96 Mon Sep 17 00:00:00 2001 From: khaliqgant Date: Tue, 22 Sep 2026 23:23:20 -0700 Subject: [PATCH 2/3] test(sdk): express the duration dispatch under the contained cwd contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Main's wrapper-duration test dispatched an absolute spec cwd, which the merged run-root contract refuses by design. Give the worker the temp directory as runRoot and omit cwd — the wrapper spawns there exactly as before. --- packages/sdk/tests/wrapper-execution-duration.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/sdk/tests/wrapper-execution-duration.test.ts b/packages/sdk/tests/wrapper-execution-duration.test.ts index 73b362272..31cba6188 100644 --- a/packages/sdk/tests/wrapper-execution-duration.test.ts +++ b/packages/sdk/tests/wrapper-execution-duration.test.ts @@ -220,14 +220,14 @@ it('completes an agent step exactly once when a wrapper outlives the former cap completions.push(args); return { ok: true }; }; - const worker = new AgentWorker(client as unknown as JournalClient, { workerId: 'w-long', pins: {} as Pins }); + const worker = new AgentWorker(client as unknown as JournalClient, { workerId: 'w-long', pins: {} as Pins, runRoot: directory }); const workerErrors: unknown[] = []; worker.on('error', (error: unknown) => { workerErrors.push(error); }); try { await worker.attach(); client.emit('step.dispatch', { run_id: 'run-long', step_id: 'step-long', attempt: 1, step_type: 'agent', - spec: { cli: held.wrapper, instruction: 'instruction', cwd: directory }, + spec: { cli: held.wrapper, instruction: 'instruction' }, lease_id: 'lease-long', lease_deadline_ms: Date.now() + 30_000, idempotency_key: 'idem-long', pins: {} as Pins, }); From 21fbe8f586e899eaba73c6f2c89cea907d2b0bd7 Mon Sep 17 00:00:00 2001 From: khaliqgant Date: Tue, 22 Sep 2026 23:49:28 -0700 Subject: [PATCH 3/3] fix(sdk): do not forward the local run root as worker_cwd over relay MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a relay-transport step omits cwd, the dispatch path fell back to this.options.runRoot and forwarded that host-local path as worker_cwd to the remote worker — a path it cannot resolve. Relay now receives no implicit cwd; the remote agent picks its own working directory. Regression: a relay dispatch on a runRoot-set worker asserts the invoke body carries no worker_cwd. Mutation-verified: reverting to the unconditional fallback forwards the run root and the test fails. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- packages/sdk/src/worker.ts | 2 +- packages/sdk/tests/agent-relay-transport.test.ts | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/sdk/src/worker.ts b/packages/sdk/src/worker.ts index bff566bbb..bdc9c6317 100644 --- a/packages/sdk/src/worker.ts +++ b/packages/sdk/src/worker.ts @@ -139,7 +139,7 @@ export class AgentWorker extends EventEmitter { ? runAgentCli(spec.cli, workerInstruction(spec.instruction, dispatch), dispatch.wake_context, effectiveModel, undefined, signal, 'agent', this.options.dataDir === undefined ? undefined : { dataDir: this.options.dataDir, runId: dispatch.run_id, stepId: dispatch.step_id, attempt: dispatch.attempt, onReady: this.options.onPtyReady, onDrive: () => { humanIntervention = true; }, - }, cwd.directory ?? this.options.runRoot, + }, cwd.directory ?? (spec.transport === 'relay' ? undefined : this.options.runRoot), spec.transport === 'relay' ? 'relay' : 'direct', { runId: dispatch.run_id, stepId: dispatch.step_id, idempotencyKey: dispatch.idempotency_key, dataDir: this.options.dataDir, resultSchema: spec.verification?.json_schema }) diff --git a/packages/sdk/tests/agent-relay-transport.test.ts b/packages/sdk/tests/agent-relay-transport.test.ts index d7ae5ba30..52c7a3469 100644 --- a/packages/sdk/tests/agent-relay-transport.test.ts +++ b/packages/sdk/tests/agent-relay-transport.test.ts @@ -315,7 +315,7 @@ import type { JournalClient } from "../src/journal-client.js"; import type { StepDispatchEvent } from "../src/protocol.js"; import { AgentWorker } from "../src/worker.js"; -async function workerFixture(f: Awaited>) { +async function workerFixture(f: Awaited>, runRoot?: string) { vi.stubEnv("RELAY_AGENT_TOKEN", "fixture-agent-token"); vi.stubEnv("RELAY_BASE_URL", "https://cast.agentrelay.com"); vi.stubGlobal("fetch", f.fetchMock); @@ -330,6 +330,7 @@ async function workerFixture(f: Awaited>) { workerId: "fixture-worker", pins: {}, dataDir: f.request.dataDir, + ...(runRoot === undefined ? {} : { runRoot }), }); const errors: unknown[] = []; worker.on("error", (error) => errors.push(error)); @@ -390,6 +391,18 @@ describe("Relay completion at the journal boundary", () => { expect(input.task).toContain("final=true"); expect(input.result_schema).toEqual({ type: "array" }); }); + it("does not forward the local run root as worker_cwd to a remote relay worker", async () => { + const f = await fixture(); + const runRoot = await mkdtemp(join(tmpdir(), "flows-relay-runroot-")); + directories.push(runRoot); + const w = await workerFixture(f, runRoot); + w.client.emit("step.dispatch", w.dispatch); + await vi.waitFor(() => expect(f.posts()).toHaveLength(1)); + await w.worker.close(); + expect(w.errors).toEqual([]); + const input = JSON.parse(String(f.posts()[0]!.init!.body)).input; + expect(input.worker_cwd).toBeUndefined(); + }); it("journals terminal task failure as worker_error with its explicit reason", async () => { const f = await fixture(); f.setStatus("failed");