From 28d149d8dc9d0f584819ea24f396f8f746e76680 Mon Sep 17 00:00:00 2001 From: Relayflow Date: Sun, 20 Sep 2026 17:18:54 +0000 Subject: [PATCH 1/2] feat: spend the analysis flows check already does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `flows check` computed the facts that would have prevented five dead runs and said nothing about them. Two of those facts now become diagnostics, and the third turns out not to be a fact. `agent_worker_unresolved` (permanent, check-only): a spec with `agent` steps needs a worker attached for step type `agent`, and without one the run parks at the first such step. `check` already walks those steps to print `REQUIRES`, so it now says what has to be true for a worker to be attached, naming the steps and `--local-agent`. It warns rather than refuses because attachment is unknown to a daemon-free check, not absent; it is a property of the invocation, so it opts in through a new `CheckInvocation` seam and `preflight` stays a pure function of the spec. Scoped to `agent` steps — `--local-agent` attaches no `llm` worker — and counted from the compiled steps, so a YAML helper step is included where `requirements` omits it. Emitted once, in the `REQUIRES` position, because it is a footnote to that line. `gate_path_unreachable` (temporary, retires with #513): an `artifact_exists` gate reads the journaled artifact list and nothing else, so a path inside a prefix the bundled worker's scan skips can never match. That is unsatisfiable, not merely unproven, so preflight refuses it and names the excluded prefix. The exclusion rule moves into `artifact-scan-policy.ts` and the real scan consumes it, so the refusal cannot drift from the scan it describes; a parity test pins the two against a real filesystem walk. Collected before every early return, so a dead gate is not hidden behind an unresolved CLI for a pass or two. `gate_output_not_captured` is deliberately not added. The criterion was to warn for as long as a subprocess_gate's output is lost, and against a live daemon it is not: the kernel journals both tails on the lowered gate step and the CLI renders them. A live-kernel regression pins that capture instead, so the warning becomes warranted at the moment it becomes true. Fixes neither #511 nor #513, and does not round-trip the spec past the daemon's validator (#502). --- docs/SURFACE.md | 29 +++ packages/sdk/src/agent-artifacts.ts | 5 +- packages/sdk/src/artifact-scan-policy.ts | 39 ++++ packages/sdk/src/cli.ts | 24 ++- packages/sdk/src/cli/check-worker-surface.ts | 66 ++++++ packages/sdk/src/cli/check.ts | 39 +++- packages/sdk/src/failure-kinds.ts | 27 ++- packages/sdk/src/named-gate-preflight.ts | 42 ++++ packages/sdk/src/preflight.ts | 6 + packages/sdk/tests/artifact-gates.test.ts | 106 +++++++++- .../sdk/tests/check-worker-surface.test.ts | 190 ++++++++++++++++++ packages/sdk/tests/live-kernel.test.ts | 74 +++++++ packages/sdk/tests/preflight.test.ts | 4 + summary.md | 175 ++++++++++++++++ 14 files changed, 813 insertions(+), 13 deletions(-) create mode 100644 packages/sdk/src/artifact-scan-policy.ts create mode 100644 packages/sdk/src/cli/check-worker-surface.ts create mode 100644 packages/sdk/src/named-gate-preflight.ts create mode 100644 packages/sdk/tests/check-worker-surface.test.ts create mode 100644 summary.md diff --git a/docs/SURFACE.md b/docs/SURFACE.md index 4620b56ea..c69f1e3b7 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -848,6 +848,19 @@ existing run from its journal. The data directory defaults to `.relayflowd`. `--json` writes one report-shaped object to stdout while diagnostics remain on stderr. +`check`'s `REQUIRES` line names the harnesses a flow needs; a spec with `agent` +steps also needs a *worker attached for step type `agent`*, and without one the +run parks at the first such step rather than failing. `flows check` cannot see +whether one is attached — it is daemon-free by construction — so it warns +`agent_worker_unresolved` instead of guessing, naming the steps and pointing at +`flows run --local-agent`. The warning is emitted in the `REQUIRES` position, +because it is a footnote to that line. It never refuses, and it is scoped to +`agent` steps: `--local-agent` attaches no `llm` worker, so naming it for an +`llm` step would be false. Only `flows check` opts in. `run` knows the answer, +`build` and `deploy` check a spec that will run elsewhere, and an authored +`.flow.ts` is checked through its header without compiling step bodies, so it +has no agent steps to count. + `flows check --watch` checks once, then watches the target, its reachable relative `use:` imports, and the nearest `flows.json` walking up from the flow directory. Saves are debounced for 150 ms; a change during a check @@ -1153,6 +1166,22 @@ asserts on `AgentResult.artifacts` — which is how this was caught at all, by `artifacts.length !== 0` check. Dotfiles and dotdirs are skipped by the walk, so `.relayflowd` is already invisible; a data dir under any other name is not. +That exclusion cuts both ways. A gate reads the journaled list and nothing else, +so an `artifact_exists` path inside a prefix the walk skips — any segment +starting with `.`, or a segment named exactly `node_modules` — can never match, +however faithfully the agent writes the file. The gate is unsatisfiable rather +than merely unproven, so preflight **refuses** it as `gate_path_unreachable`, +naming the excluded prefix the artifact has to move out of. Segments are +compared exactly: `node_modules-copy/out.md` and `reports/v1.2/review.md` are +fine. The rule lives in `packages/sdk/src/artifact-scan-policy.ts` and the walk +itself consumes it, so the refusal cannot drift from the scan it describes. +The refusal is collected before any environment probe can return early, so a +dead gate is not hidden behind an unresolved CLI for a pass or two. It is a +property of the *bundled* worker, not of the protocol — `step.complete` accepts +any `output`, so a custom worker may journal a hidden path — and it retires +with the exclusion: when the scan stops skipping those prefixes, the kind and +`packages/sdk/src/named-gate-preflight.ts` are deleted together. + - Are YAML helper verbs (`slack:`, `mcp:`) core spec vocabulary or compile-time expansion into `run`/effect steps? Leaning: expansion — the kernel spec stays seven words; helpers stay a surface concern. - Helper generation cadence: generated from relayfile adapter manifests at build time vs published per-adapter packages. Leaning: generated, with hand-tuned verb names for the top providers. - `on` inside a running body (subscribe after start, buffer events while a step runs, end on `idle`/`deadline`). Proposed in [`docs/EVENT-AWAIT.md`](EVENT-AWAIT.md); motivating case is a flow that babysits the PR it opened. diff --git a/packages/sdk/src/agent-artifacts.ts b/packages/sdk/src/agent-artifacts.ts index c7f02b50a..dfddf7d73 100644 --- a/packages/sdk/src/agent-artifacts.ts +++ b/packages/sdk/src/agent-artifacts.ts @@ -1,13 +1,12 @@ import { createHash } from 'node:crypto'; import { readFile, readdir, stat } from 'node:fs/promises'; import { join, relative, sep } from 'node:path'; +import { isUnscannedEntryName } from './artifact-scan-policy.js'; function isEnoent(error: unknown): boolean { return error instanceof Error && (error as NodeJS.ErrnoException).code === 'ENOENT'; } -const SKIPPED_DIR_NAMES = new Set(['node_modules']); - /** * A recursive snapshot of every regular file under `dir`, keyed by its * `dir`-relative POSIX path, valued by a content signature (size + sha256). @@ -41,7 +40,7 @@ async function walk(root: string, current: string, out: Map): Pr throw error; } for (const entry of entries) { - if (entry.name.startsWith('.') || SKIPPED_DIR_NAMES.has(entry.name)) continue; + if (isUnscannedEntryName(entry.name)) continue; const path = join(current, entry.name); if (entry.isDirectory()) { await walk(root, path, out); diff --git a/packages/sdk/src/artifact-scan-policy.ts b/packages/sdk/src/artifact-scan-policy.ts new file mode 100644 index 000000000..ec98f74bf --- /dev/null +++ b/packages/sdk/src/artifact-scan-policy.ts @@ -0,0 +1,39 @@ +/** + * The bundled agent worker's artifact-scan policy, as a pure predicate. + * + * `agent-artifacts.ts` walks an agent step's cwd and records every regular + * file it finds, except entries whose name starts with `.` and entries named + * exactly `node_modules` — files as well as directories, in both cases. That + * walk decides what lands in the journaled `output.artifacts` list, which is + * the only thing an `artifact_exists` gate reads (`named-gate-lowering.ts` + * tests exact membership in that list and never touches disk). + * + * Static analysis needs the same rule without importing filesystem traversal, + * so the rule lives here and both consume it. This is a property of the + * bundled worker, not of the journal protocol: `JournalClient.stepComplete` + * accepts any `output`, so a custom worker is free to journal a hidden path. + */ + +/** Entry names the walk never descends into or records. */ +const SKIPPED_ENTRY_NAMES = new Set(['node_modules']); + +export function isUnscannedEntryName(name: string): boolean { + return name.startsWith('.') || SKIPPED_ENTRY_NAMES.has(name); +} + +/** + * The shortest `/`-prefix of a relative POSIX `path` the scan excludes, or + * `undefined` when every segment is scanned. + * + * Segments are compared exactly, with no normalization: the gate matches the + * author's literal string against the worker's literal list, so `node_modules` + * is excluded and `node_modules-copy` is not, and a backslash is an ordinary + * filename character rather than a separator. The whole prefix is returned, + * not the offending segment alone, because that prefix is the directory an + * author has to move the artifact out of. + */ +export function unscannedArtifactPrefix(path: string): string | undefined { + const segments = path.split('/'); + const index = segments.findIndex(isUnscannedEntryName); + return index === -1 ? undefined : segments.slice(0, index + 1).join('/'); +} diff --git a/packages/sdk/src/cli.ts b/packages/sdk/src/cli.ts index 75bfc1b1f..5a580943e 100644 --- a/packages/sdk/src/cli.ts +++ b/packages/sdk/src/cli.ts @@ -240,8 +240,13 @@ export async function runCli( // refuses `--data-dir` on `check`, so there is no data dir to attach to. // `flows check` keeps working with no daemon, no relayflowd binary and no // data directory at all -- a property worth keeping, not an omission. + // Only this invocation opts into `agent_worker_unresolved`: `flows check` + // attaches no worker and, being daemon-free, cannot see one attached + // elsewhere. The authored `.flow.ts` path checks header declarations + // without compiling steps, so it has no agent steps to count. const checked = /\.(?:[cm]?[jt]s)$/.test(parsed.value) - ? await checkAuthoredFlowComposed(parsed.value) : checkFlow(parsed.value); + ? await checkAuthoredFlowComposed(parsed.value) + : checkFlow(parsed.value, { warnUnresolvedAgentWorker: true }); emitCheckReport(checked.report, parsed.json, io); return checked.report.ok ? 0 : 2; } @@ -916,12 +921,26 @@ function parseTickArgs(rest: readonly string[]): ParsedArgs | undefined { }; } +/** + * `agent_worker_unresolved` reads as a footnote to `REQUIRES codex (step + * "implement"), …`: that line already names the steps that need an agent + * worker, and this says what has to be true for one to be attached. So the + * plain-text pass holds it back and emits it in that position, exactly once — + * the leading batch below skips it rather than printing it twice. JSON mode + * returns before any of this and keeps the single ordered diagnostics array. + */ +function isWorkerSurfaceWarning(diagnostic: CheckReport['diagnostics'][number]): boolean { + return diagnostic.kind === 'agent_worker_unresolved'; +} + function emitCheckReport(report: CheckReport, json: boolean, io: CliIo): void { - emitDiagnostics(report.diagnostics, io); if (json) { + emitDiagnostics(report.diagnostics, io); io.stdout(JSON.stringify(report)); return; } + const deferred = report.diagnostics.filter(isWorkerSurfaceWarning); + emitDiagnostics(report.diagnostics.filter((diagnostic) => !isWorkerSurfaceWarning(diagnostic)), io); for (const gate of report.gates) { // A gate that accepts every output is legal, but it must not read like a // gate that judges something. @@ -948,6 +967,7 @@ function emitCheckReport(report: CheckReport, json: boolean, io: CliIo): void { // the hosted verbs check the same list against Cloud before submitting. const requires = report.requirements === undefined ? '' : describeFlowRequirements(report.requirements); if (requires) io.stdout(`REQUIRES ${requires}`); + emitDiagnostics(deferred, io); if (report.ok) io.stdout(`CHECK PASSED ${report.path ?? ''}`.trimEnd()); } diff --git a/packages/sdk/src/cli/check-worker-surface.ts b/packages/sdk/src/cli/check-worker-surface.ts new file mode 100644 index 000000000..dbaec4559 --- /dev/null +++ b/packages/sdk/src/cli/check-worker-surface.ts @@ -0,0 +1,66 @@ +import { compileSpec } from '../compile.js'; +import type { FlowSpec } from '../spec.js'; +import type { CheckWarningDiagnostic } from './check.js'; + +/** Enough ids to recognise the flow, not enough to bury the remedy. */ +const NAMED_STEP_LIMIT = 3; + +/** + * `agent` steps need a worker attached for step type `agent`, and nothing in a + * spec says whether one will be. `flows run` attaches the SDK's `AgentWorker` + * only under `--local-agent` (`cli/run.ts`); with no worker the run parks at + * the first agent step with `no worker is attached for step type "agent"`. + * `flows check` already walks these steps to print its `REQUIRES` line, so the + * fact is in hand — this spends it. + * + * Warning, never a refusal, and worded as a requirement rather than a + * prediction: worker attachment is *unknown* here, not absent. A worker can be + * attached out of band (an SDK `AgentWorker` against the same daemon, Cloud's + * workers for a hosted run), and `flows check` is daemon-free by construction, + * so it cannot see one either way. + * + * Scoped to `agent` steps. `--local-agent` attaches no `llm` worker on the + * YAML path, so naming it for `llm` steps would be false; those steps are not + * counted and not mentioned. + * + * Blind spot, shared with `permissions_unenforced`: an authored `.flow.ts` + * flow is checked through `checkMcpHeader`, which preflights a synthetic + * one-step header spec without executing the body. There are no compiled + * `agent` steps to count, so this diagnostic is silent for `.flow.ts`. + */ +export function agentWorkerDiagnostics(authoring: FlowSpec): CheckWarningDiagnostic[] { + const steps = agentStepIds(authoring); + if (steps.length === 0) return []; + return [{ severity: 'warning', kind: 'agent_worker_unresolved', message: message(steps) }]; +} + +/** + * Compiled `agent` steps, by id. Compilation is the only walk that sees them + * all: a YAML helper step (`slack: { post: … }`) is an `agent` step carrying a + * helper envelope and needs the same SDK agent worker, and only the compiler + * lowers it into one. `requirements.harnesses` is the wrong source — it + * deduplicates by harness, counts `llm` use, and omits helper steps entirely. + * + * A spec that does not compile yields no diagnostic rather than a thrown + * check: the compile refusal is owned by the existing preflight path and is + * the whole report in that case. + */ +function agentStepIds(authoring: FlowSpec): string[] { + try { + return compileSpec(authoring).steps.filter((step) => step.type === 'agent').map((step) => step.id); + } catch { + return []; + } +} + +function message(steps: readonly string[]): string { + const named = steps.slice(0, NAMED_STEP_LIMIT).map((id) => `"${id}"`).join(', '); + const rest = steps.length - NAMED_STEP_LIMIT; + const list = rest > 0 ? `${named} and ${rest} more` : named; + const subject = steps.length === 1 + ? `1 agent step (${list}) requires` + : `${steps.length} agent steps (${list}) require`; + return `${subject} an attached worker. For a local run, use \`flows run --local-agent \`, ` + + 'unless you already attach an agent worker for this daemon. `flows check` does not verify worker ' + + 'attachment: with no worker attached the run parks at the first agent step.'; +} diff --git a/packages/sdk/src/cli/check.ts b/packages/sdk/src/cli/check.ts index 22c362c9c..ac072d783 100644 --- a/packages/sdk/src/cli/check.ts +++ b/packages/sdk/src/cli/check.ts @@ -6,6 +6,7 @@ import { dirname, isAbsolute, join, parse as parsePath, resolve } from 'node:pat import { spawnSync } from 'node:child_process'; import { parse as parseYaml } from 'yaml'; import { CompileError, compileSpec, kernelToAuthoring } from '../compile.js'; +import { agentWorkerDiagnostics } from './check-worker-surface.js'; import { helperReady } from '../yaml-helper-effect.js'; import { flowRequirements, type FlowRequirements } from '../flow-requirements.js'; import { @@ -94,6 +95,24 @@ export interface CheckExecution { flow?: FlowSpec; } +/** + * Facts about the *caller*, not about the spec, that change which diagnostics + * apply. Preflight stays a pure function of the spec plus environment probes; + * anything that depends on how the flow is about to be invoked opts in here. + */ +export interface CheckInvocation { + /** + * Report `agent_worker_unresolved` when the spec has `agent` steps. + * + * Only `flows check` sets this. `flows run` attaches its own worker under + * `--local-agent` and knows the answer, `flows build` and `flows deploy` + * check a spec that will run elsewhere, and an SDK caller that reaches + * `checkAuthoredFlow` directly is generally running a worker already — + * warning any of them would be noise about a question they have answered. + */ + warnUnresolvedAgentWorker?: boolean; +} + export class CheckFailure extends Error { constructor(readonly kind: CheckFailureKind, message: string) { super(message); @@ -101,7 +120,7 @@ export class CheckFailure extends Error { } /** Validate and preflight one working-tree spec without starting a run. */ -export function checkFlow(path: string): CheckExecution { +export function checkFlow(path: string, invocation: CheckInvocation = {}): CheckExecution { const absolutePath = resolve(path); try { const source = readFlowSource(absolutePath); @@ -111,7 +130,7 @@ export function checkFlow(path: string): CheckExecution { : []; let execution: CheckExecution; try { - execution = checkAuthoredFlow(readFlow(source, absolutePath), path); + execution = checkAuthoredFlow(readFlow(source, absolutePath), path, undefined, invocation); } catch (error) { if (!(error instanceof CheckFailure)) throw error; execution = { report: inputFailureReport(error, path) }; @@ -141,7 +160,12 @@ function safeRequirements(authoring: FlowSpec, projectCli: string | undefined): } /** Preflight a validated authored flow through the same path as YAML/JSON. */ -export function checkAuthoredFlow(authoring: FlowSpec, path: string, projectConfig?: ProjectConfig): CheckExecution { +export function checkAuthoredFlow( + authoring: FlowSpec, + path: string, + projectConfig?: ProjectConfig, + invocation: CheckInvocation = {}, +): CheckExecution { const absolutePath = resolve(path); try { const config = projectConfig ?? readProjectConfig(dirname(absolutePath)); @@ -172,6 +196,13 @@ export function checkAuthoredFlow(authoring: FlowSpec, path: string, projectConf result.diagnostics.push({ severity: 'warning', kind: 'budget_unmetered', message: 'Managed communication sessions do not report token or dollar usage. Budget ceilings cannot bound their spend; communication.timeoutMs bounds their duration.' }); } + // Reported whatever preflight concluded. An environment refusal (a missing + // CLI, an unknown model) is fixed and rerun; the worker question is still + // open on the next pass, and staying silent about it here is what made an + // author meet it one dead run at a time. + const workerSurface = invocation.warnUnresolvedAgentWorker === true + ? agentWorkerDiagnostics(authoring) + : []; return { report: { ok: result.ok, @@ -179,7 +210,7 @@ export function checkAuthoredFlow(authoring: FlowSpec, path: string, projectConf ...(config.path !== undefined ? { projectConfigPath: config.path } : {}), gates: result.gates, resolutions: result.resolutions, - diagnostics: result.diagnostics, + diagnostics: [...result.diagnostics, ...workerSurface], requirements: safeRequirements(authoring, config.cli), }, ...(result.ok && flow !== undefined ? { flow } : {}), diff --git a/packages/sdk/src/failure-kinds.ts b/packages/sdk/src/failure-kinds.ts index 46b704693..e60bced80 100644 --- a/packages/sdk/src/failure-kinds.ts +++ b/packages/sdk/src/failure-kinds.ts @@ -2,10 +2,22 @@ import { PLUGIN_FAILURE_KINDS } from './plugin-manifest.js'; import { NAMED_GATE_FAILURE_KINDS } from './named-gates.js'; const SHARED_SPEC_FAILURE_KINDS = ['invalid_spec'] as const; -/** Environment refusal kinds produced after spec validation succeeds. */ +/** + * Environment refusal kinds produced after spec validation succeeds. + * + * `gate_path_unreachable` is the one member decided from the compiled + * snapshot alone: an `artifact_exists` path inside a prefix the bundled agent + * worker's artifact scan excludes can never appear in the journaled + * `output.artifacts` the gate reads, so the gate is unsatisfiable rather than + * merely unproven. It lives in this list because it is the list both + * `PREFLIGHT_FAILURE_KINDS` and `CHECK_FAILURE_KINDS` draw from. Temporary: + * it describes #513, and retires with `named-gate-preflight.ts` when the scan + * stops excluding those paths. + */ const PREFLIGHT_ENVIRONMENT_FAILURE_KINDS = [ ...NAMED_GATE_FAILURE_KINDS, ...PLUGIN_FAILURE_KINDS, + 'gate_path_unreachable', 'helper_provider.mount_required', 'helper_provider.unsupported', 'helper_slack.credential_missing', @@ -136,8 +148,17 @@ export const RUN_WARNING_KINDS = [ 'connection_file_stale', ] as const; -/** File-level editor hints emitted by flows check, outside pure preflight. */ -export const CHECK_WARNING_KINDS = ['editor_schema_missing'] as const; +/** + * Warnings emitted by flows check, outside pure preflight. + * + * `editor_schema_missing` is a file-level editor hint. `agent_worker_unresolved` + * is a property of the *invocation*, not of the spec: a spec with `agent` + * steps needs a worker attached for step type `agent`, and only the caller + * knows whether it attaches one. `flows check` attaches none and, being + * daemon-free, can see none either — so it opts in, while `flows run`, `flows + * build` and SDK submissions do not (cli/check-worker-surface.ts). + */ +export const CHECK_WARNING_KINDS = ['editor_schema_missing', 'agent_worker_unresolved'] as const; export type CheckWarningKind = (typeof CHECK_WARNING_KINDS)[number]; export type PreflightFailureKind = (typeof PREFLIGHT_FAILURE_KINDS)[number]; diff --git a/packages/sdk/src/named-gate-preflight.ts b/packages/sdk/src/named-gate-preflight.ts new file mode 100644 index 000000000..d501827e8 --- /dev/null +++ b/packages/sdk/src/named-gate-preflight.ts @@ -0,0 +1,42 @@ +import { unscannedArtifactPrefix } from './artifact-scan-policy.js'; +import { isNamedGate } from './named-gates.js'; +import type { StepSpec } from './spec.js'; +import type { PreflightRefusal } from './preflight.js'; + +/** + * Static named-gate reachability, decided from the compiled snapshot alone. + * + * An `artifact_exists` gate lowers to a deterministic step that asks whether + * the producer's journaled `output.artifacts` list contains the literal path + * (`named-gate-lowering.ts`). The bundled agent worker builds that list by + * walking the step's cwd under the policy in `artifact-scan-policy.ts`, which + * records nothing under a dot-named or `node_modules` entry. A path inside one + * of those prefixes therefore cannot pass under that worker, on any host, in + * any environment — so it is refused here rather than after a run. + * + * Scoped to the bundled worker on purpose. The journal protocol lets a custom + * worker submit any `output`, and the gate does not reject a hidden path in + * such a list. The refusal names the policy it applies so the scope is legible + * in the message, and applies wherever public preflight runs: `flows check`, + * `flows run`, `flows build` and SDK submissions all share that path. + * + * Temporary: this describes #513. When the scan stops excluding these paths, + * delete this module and its call site. + */ +export function namedGateReachabilityDiagnostics(step: StepSpec): PreflightRefusal[] { + const gate = step.verification; + if (!isNamedGate(gate) || gate.type !== 'artifact_exists') return []; + // A malformed path is already `gate_path_invalid` from compilation; only + // validated relative POSIX paths reach here. + const prefix = unscannedArtifactPrefix(gate.path); + if (prefix === undefined) return []; + return [{ + severity: 'refusal', + kind: 'gate_path_unreachable', + stepId: step.id, + message: `Step "${step.id}" gates on artifact_exists path "${gate.path}", but the bundled agent worker's ` + + `artifact scan never records anything under "${prefix}": it skips entries whose name starts with "." ` + + 'and entries named "node_modules". The path can never appear in the journaled output.artifacts this ' + + 'gate reads, so the gate cannot pass. Write the artifact to a scanned path.', + }]; +} diff --git a/packages/sdk/src/preflight.ts b/packages/sdk/src/preflight.ts index 2ebad36dd..a7df582f8 100644 --- a/packages/sdk/src/preflight.ts +++ b/packages/sdk/src/preflight.ts @@ -12,6 +12,7 @@ import { compileSpec, CompileError } from './compile.js'; import { helperCall } from './yaml-helpers.js'; import { resolveCliModelSelection, type CliModelSource } from './cli-adapter.js'; import { isNamedGate, NAMED_GATE_FAILURE_KINDS, type NamedGateFailureKind } from './named-gates.js'; +import { namedGateReachabilityDiagnostics } from './named-gate-preflight.js'; import { compileScopes, type ScopeInput, type MountRegistry } from './scope-compiler.js'; import { readMountRegistry } from './mount-registry.js'; import type { @@ -231,6 +232,11 @@ function preflightSync(flow: unknown, options: PreflightOptions): PreflightResul // permissions they declared on the same file are not enforced. diagnostics.push(...permissionsDiagnostics(compiled)); diagnostics.push(...scopeDiagnostics(compiled, options)); + // Same rule, same reason: an `artifact_exists` path the bundled worker's + // scan can never journal is decidable from the snapshot, so it is collected + // here rather than beside `probeNamedGate` below — which CLI resolution, an + // unknown model, a bad scope or a budget refusal can all return before. + diagnostics.push(...compiled.steps.flatMap(namedGateReachabilityDiagnostics)); for (const server of new Set(options.mcpServers ?? [])) { if (options.mcp !== undefined && Object.hasOwn(options.mcp, server)) continue; diagnostics.push({ severity: 'refusal', kind: 'mcp_undeclared_server', server, diff --git a/packages/sdk/tests/artifact-gates.test.ts b/packages/sdk/tests/artifact-gates.test.ts index 4983684ad..9b1a4bc37 100644 --- a/packages/sdk/tests/artifact-gates.test.ts +++ b/packages/sdk/tests/artifact-gates.test.ts @@ -1,7 +1,7 @@ import { EventEmitter } from 'node:events'; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; import { afterEach, describe, expect, it, vi } from 'vitest'; // A fake `claude` whose "work" is whatever the test's `onSpawn` hook writes @@ -40,6 +40,9 @@ import { lowerNamedGates } from '../src/named-gate-lowering.js'; import { namedGateErrors, namedGateFailure } from '../src/named-gates.js'; import { validateSpec } from '../src/validate.js'; import { checkFlow } from '../src/cli/check.js'; +import { preflight } from '../src/preflight.js'; +import { unscannedArtifactPrefix } from '../src/artifact-scan-policy.js'; +import { snapshotWorkspaceFiles } from '../src/agent-artifacts.js'; import { execFileSync } from 'node:child_process'; const dirs: string[] = []; @@ -147,3 +150,104 @@ describe('artifact_exists named gate', () => { expect(report.gates).toContainEqual(expect.objectContaining({ stepId: 'review', checks: ['exit_code'], preflightable: true, replayable: true })); }); }); + +/** + * #513: the gate reads the worker's journaled `artifacts` list, and the + * bundled worker's scan never puts a dot-named or `node_modules` path in it. + * That makes such a gate statically unsatisfiable, which preflight now refuses + * rather than letting a run discover it. Retire this block with the module. + */ +describe('artifact_exists named gate: static reachability', () => { + const probes = () => ({ cli: () => ({ exists: true, authenticated: true }), executor: () => true, command: () => true }); + const gated = (path: string) => ({ + version: '0.1.0', name: 'x', + steps: [{ id: 'review', type: 'agent' as const, instruction: 'i', cli: 'x', + verification: { type: 'artifact_exists' as const, path } }], + }); + + it.each([ + ['.workflow-artifacts/rust/review.md', '.workflow-artifacts'], + ['.hidden.md', '.hidden.md'], + ['node_modules/pkg/out.md', 'node_modules'], + ['reports/.drafts/review.md', 'reports/.drafts'], + ['reports/node_modules/out.md', 'reports/node_modules'], + // The whole prefix, not the offending segment alone: that prefix is the + // directory the author has to move the artifact out of. + ['a/b/.c/d/e.md', 'a/b/.c'], + ])('refuses %s and names the excluded prefix %s', (path, prefix) => { + const result = preflight(gated(path) as never, { probes: probes() }); + expect(result.ok).toBe(false); + const refusal = result.diagnostics.find(d => d.kind === 'gate_path_unreachable'); + expect(refusal, JSON.stringify(result.diagnostics)).toMatchObject({ severity: 'refusal', stepId: 'review' }); + expect(refusal!.message).toContain(`"${prefix}"`); + expect(refusal!.message).toContain(path); + }); + + it.each([ + 'review/security.md', + // Exact segment comparison, so a similar name is not an exclusion. + 'node_modules-copy/out.md', + 'reports/node_modules.md', + // A dot inside a segment is not a dot-named entry. + 'review.md', + 'reports/v1.2/review.md', + // A backslash is an ordinary filename character in a POSIX path, never a + // separator: `readdir` reports one entry whose name starts with "d". + String.raw`dir\.hidden.md`, + ])('accepts %s, which the scan does record', (path) => { + const result = preflight(gated(path) as never, { probes: probes() }); + expect(result.diagnostics.filter(d => d.kind === 'gate_path_unreachable')).toEqual([]); + expect(result.ok).toBe(true); + }); + + it('agrees with what the real scan records, for every case above', async () => { + const cwd = tempDir(); + const paths = [ + '.workflow-artifacts/rust/review.md', '.hidden.md', 'node_modules/pkg/out.md', + 'reports/.drafts/review.md', 'reports/node_modules/out.md', 'a/b/.c/d/e.md', + 'review/security.md', 'node_modules-copy/out.md', 'reports/node_modules.md', + 'review.md', 'reports/v1.2/review.md', String.raw`dir\.hidden.md`, + ]; + for (const path of paths) { + const full = join(cwd, ...path.split('/')); + mkdirSync(dirname(full), { recursive: true }); + writeFileSync(full, 'x'); + } + const scanned = new Set((await snapshotWorkspaceFiles(cwd)).keys()); + // The predicate is the scan's rule, so "excluded by the predicate" and + // "absent from the scan" must be the same set. A drift in either + // direction would make the refusal lie. + for (const path of paths) { + expect(scanned.has(path), path).toBe(unscannedArtifactPrefix(path) === undefined); + } + }); + + it('is reported even when an unrelated environment refusal returns first', () => { + // `cli_unresolved` returns from preflight before any probe runs. The + // author fixes the CLI, reruns, and would otherwise meet the dead gate + // only on the pass after that — or at run time. + const result = preflight({ + version: '0.1.0', name: 'x', + steps: [{ id: 'review', type: 'agent', instruction: 'i', + verification: { type: 'artifact_exists', path: '.out/review.md' } }], + } as never, { probes: probes() }); + + expect(result.diagnostics.map(d => d.kind)).toEqual(['gate_path_unreachable', 'cli_unresolved']); + }); + + it('refuses through flows check, naming the step and the prefix', () => { + const dir = tempDir(); + writeFileSync(join(dir, 'flows.json'), JSON.stringify({ cli: 'true' })); + writeFileSync(join(dir, 'flow.yaml'), JSON.stringify({ version: '0.1.0', name: 'gated', steps: [ + { id: 'review', type: 'agent', instruction: 'i', cli: 'true', + verification: { type: 'artifact_exists', path: '.workflow-artifacts/rust/review.md' } }, + ] })); + + const report = checkFlow(join(dir, 'flow.yaml')).report; + + expect(report.ok).toBe(false); + expect(report.diagnostics).toContainEqual(expect.objectContaining({ + severity: 'refusal', kind: 'gate_path_unreachable', stepId: 'review', + })); + }); +}); diff --git a/packages/sdk/tests/check-worker-surface.test.ts b/packages/sdk/tests/check-worker-surface.test.ts new file mode 100644 index 000000000..5695424a1 --- /dev/null +++ b/packages/sdk/tests/check-worker-surface.test.ts @@ -0,0 +1,190 @@ +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { runCli, type CheckReport, type CliIo } from '../src/cli.js'; +import { checkFlow } from '../src/cli/check.js'; +import { CHECK_WARNING_KINDS } from '../src/failure-kinds.js'; + +const directories: string[] = []; + +afterEach(() => { + for (const directory of directories.splice(0)) rmSync(directory, { recursive: true, force: true }); +}); + +/** + * One ordered transcript, not two arrays. + * + * The claim under test is positional — the warning reads as a footnote to + * `REQUIRES`, so it has to land *between* that line and `CHECK PASSED`. + * Capturing stdout and stderr separately would discard exactly the fact being + * asserted, so both streams are recorded into a single list, tagged with the + * stream they were written to. + */ +function transcript(): { io: CliIo; lines: string[]; on: (stream: 'stdout' | 'stderr') => string[] } { + const lines: string[] = []; + return { + io: { stdout: (line) => lines.push(`out ${line}`), stderr: (line) => lines.push(`err ${line}`) }, + lines, + on: (stream) => lines.filter(line => line.startsWith(stream === 'stdout' ? 'out ' : 'err ')) + .map(line => line.slice(4)), + }; +} + +/** + * A project boundary, so an ambient /tmp/flows.json cannot reach the fixture. + * + * `name` decides whether the flow renders a `REQUIRES` line at all: + * `describeFlowRequirements` reports a harness only when the CLI's basename is + * a known one, so `claude` produces the line and `agent-cli` does not. Both + * shapes are real, and the warning has to be placed correctly in each. + */ +function project(flow: string, { config = {}, name = 'agent-cli' }: { + config?: Record; + name?: string; +} = {}): string { + const directory = mkdtempSync(join(tmpdir(), 'flows-worker-surface-')); + directories.push(directory); + writeFileSync(join(directory, 'flows.json'), JSON.stringify({ executors: [], ...config })); + const cli = join(directory, name); + // Answers all three preflight probes, so nothing unrelated to the worker + // question refuses: adapter handshake, `auth status`, and — for a CLI whose + // basename implies a default model — the model-scoped readiness probe. + writeFileSync(cli, [ + '#!/bin/sh', + 'case "${1-}" in', + " --relayflows-adapter-v1) printf '%s\\n' relayflows-agent-cli-v1 ;;", + ' auth) test "$2" = status ;;', + " -p) printf '%s\\n' RELAYFLOWS_MODEL_READY ;;", + ' *) exit 9 ;;', + 'esac', + '', + ].join('\n')); + chmodSync(cli, 0o755); + const path = join(directory, 'flow.flow.yaml'); + writeFileSync(path, `# yaml-language-server: $schema=x\n${flow.replaceAll('', `./${name}`)}`); + return path; +} + +const agentStep = (id: string) => ` - id: ${id}\n type: agent\n cli: \n instruction: Do the thing.\n`; +const flowOf = (steps: string) => `version: '0.1.0'\nname: surface\nsteps:\n${steps}`; + +function warning(report: CheckReport): CheckReport['diagnostics'][number] | undefined { + return report.diagnostics.find(diagnostic => diagnostic.kind === 'agent_worker_unresolved'); +} + +describe('agent_worker_unresolved', () => { + it('is a declared check warning kind, suppressible and deletable on its own', () => { + expect(CHECK_WARNING_KINDS).toContain('agent_worker_unresolved'); + }); + + it('names --local-agent, the steps, and what happens without a worker', () => { + const report = checkFlow(project(flowOf(agentStep('implement'))), { warnUnresolvedAgentWorker: true }).report; + + const diagnostic = warning(report); + expect(diagnostic).toMatchObject({ severity: 'warning', kind: 'agent_worker_unresolved' }); + expect(diagnostic!.message).toContain('--local-agent'); + expect(diagnostic!.message).toContain('"implement"'); + // Worded as a requirement, not a prediction: attachment is unknown here. + expect(diagnostic!.message).toContain('1 agent step'); + expect(diagnostic!.message).toContain('parks'); + }); + + it('agrees in number with the steps it counts, and stops naming them at three', () => { + const many = checkFlow(project(flowOf(['a', 'b', 'c', 'd'].map(agentStep).join(''))), + { warnUnresolvedAgentWorker: true }).report; + + expect(warning(many)!.message).toContain('4 agent steps ("a", "b", "c" and 1 more) require'); + }); + + it('warns without refusing: a flow whose only complaint is the worker still passes', async () => { + const output = transcript(); + + const code = await runCli(['check', project(flowOf(agentStep('implement')))], output.io); + + expect(code).toBe(0); + expect(output.on('stdout').some(line => line.startsWith('CHECK PASSED'))).toBe(true); + }); + + it('is emitted once, after REQUIRES and before CHECK PASSED', async () => { + const output = transcript(); + + await runCli(['check', project(flowOf(agentStep('implement') + agentStep('review')), { name: 'claude' })], output.io); + + const requires = output.lines.findIndex(line => line.startsWith('out REQUIRES')); + const warned = output.lines.filter(line => line.includes('[agent_worker_unresolved]')); + const passed = output.lines.findIndex(line => line.startsWith('out CHECK PASSED')); + // The line this annotates: it already names the steps needing a worker. + expect(requires, output.lines.join('\n')).toBeGreaterThanOrEqual(0); + expect(warned, output.lines.join('\n')).toHaveLength(1); + // Diagnostics stay on stderr; report lines stay on stdout. + expect(warned[0]!.startsWith('err ')).toBe(true); + expect(output.lines.indexOf(warned[0]!)).toBeGreaterThan(requires); + expect(output.lines.indexOf(warned[0]!)).toBeLessThan(passed); + }); + + it('still lands before CHECK PASSED when there is no REQUIRES line to follow', async () => { + // An unrecognised CLI basename is no harness, so nothing renders a + // `REQUIRES` line. The warning must not vanish with the line it annotates. + const output = transcript(); + + await runCli(['check', project(flowOf(agentStep('implement')))], output.io); + + const warned = output.lines.filter(line => line.includes('[agent_worker_unresolved]')); + const passed = output.lines.findIndex(line => line.startsWith('out CHECK PASSED')); + expect(output.lines.some(line => line.startsWith('out REQUIRES'))).toBe(false); + expect(warned, output.lines.join('\n')).toHaveLength(1); + expect(output.lines.indexOf(warned[0]!)).toBeLessThan(passed); + }); + + it('counts a YAML helper step, which compiles to an agent step needing the same worker', () => { + const report = checkFlow(project(flowOf( + ' - id: notify\n slack:\n post:\n channel: "#eng"\n text: done\n', + )), { warnUnresolvedAgentWorker: true }).report; + + // `requirements` is the wrong source for this count: it reports the helper + // as an integration, never as a step that needs an agent worker. + expect(report.requirements?.harnessUses ?? []).toEqual([]); + expect(warning(report)!.message).toContain('"notify"'); + }); + + it('is silent for flows with no agent steps', () => { + const deterministic = checkFlow(project(flowOf(' - id: build\n type: deterministic\n command: "true"\n')), + { warnUnresolvedAgentWorker: true }).report; + const llm = checkFlow(project(flowOf(' - id: draft\n type: llm\n model: test-model-v1\n instruction: Draft.\n'), + { config: { models: ['test-model-v1'] } }), { warnUnresolvedAgentWorker: true }).report; + + // `--local-agent` attaches no `llm` worker, so naming it for an `llm` step + // would be false. + expect(warning(deterministic)).toBeUndefined(); + expect(warning(llm)).toBeUndefined(); + }); + + it('survives a preflight refusal, because the worker question is open on the next pass', () => { + const report = checkFlow(project(flowOf(' - id: implement\n type: agent\n cli: ./absent-cli\n instruction: Do it.\n')), + { warnUnresolvedAgentWorker: true }).report; + + expect(report.ok).toBe(false); + expect(report.diagnostics.some(diagnostic => diagnostic.severity === 'refusal')).toBe(true); + expect(warning(report)).toBeDefined(); + }); + + it('is opt-in: a caller that did not ask is not told', () => { + const path = project(flowOf(agentStep('implement'))); + + // `flows run` (cli/run.ts) and `flows build` both reach check this way. + expect(warning(checkFlow(path).report)).toBeUndefined(); + expect(warning(checkFlow(path, {}).report)).toBeUndefined(); + expect(warning(checkFlow(path, { warnUnresolvedAgentWorker: false }).report)).toBeUndefined(); + }); + + it('appears once in --json, in the payload and on stderr', async () => { + const output = transcript(); + + await runCli(['check', '--json', project(flowOf(agentStep('implement')))], output.io); + + const payload = JSON.parse(output.on('stdout').at(-1)!) as CheckReport; + expect(payload.diagnostics.filter(d => d.kind === 'agent_worker_unresolved')).toHaveLength(1); + expect(output.on('stderr').filter(line => line.includes('[agent_worker_unresolved]'))).toHaveLength(1); + }); +}); diff --git a/packages/sdk/tests/live-kernel.test.ts b/packages/sdk/tests/live-kernel.test.ts index ff89a5c30..bb4dee569 100644 --- a/packages/sdk/tests/live-kernel.test.ts +++ b/packages/sdk/tests/live-kernel.test.ts @@ -1407,6 +1407,80 @@ steps: }, 60_000); }); +/** + * Pins a fact a diagnostic was almost written about. + * + * `subprocess_gate` lowers to a nested `.gate` deterministic step, and + * the proposal here was for `flows check` to warn that a failing gate's stdout + * and stderr are lost. Against a live daemon they are not: the kernel journals + * them into the gate step's `step.completed` output, and the CLI renders both + * tails. A warning saying otherwise would have been false, so none was added + * and this stands in its place. If capture ever regresses, this fails and the + * warning becomes warranted — which is the only condition under which it + * should exist. + */ +describe('subprocess_gate output capture against live relayflowd', () => { + it('journals the gate command output and reports both tails', async () => { + const dataDir = temporaryDirectory('flows-gate-output-'); + await startDaemon(dataDir); + const gated = (name: string, gate: string): string => { + const path = join(dataDir, `${name}.flow.yaml`); + writeFileSync(path, `version: '0.1.0' +steps: + - id: emit + type: deterministic + command: "printf 'body\\n'" + verification: + type: subprocess_gate + command: ${JSON.stringify(gate)} +`); + return path; + }; + + const noisy = invokeCli(['run', '--json', '--data-dir', dataDir, + gated('noisy', "printf 'GATE_STDOUT_MARKER\\n'; printf 'GATE_STDERR_MARKER\\n' 1>&2; exit 3")]); + + expect(noisy.status, noisy.stderr).toBe(1); + const report = JSON.parse(noisy.stdout) as { runId: string; diagnostics: Array> }; + const failure = report.diagnostics.find(diagnostic => diagnostic['kind'] === 'step_failed'); + expect(failure, noisy.stdout).toMatchObject({ + // The gate runs as its own journaled step, so the failure names it. + stepId: 'emit.gate', + stepType: 'deterministic', + stdoutTail: 'GATE_STDOUT_MARKER\n', + stderrTail: 'GATE_STDERR_MARKER\n', + }); + // Rendered, not merely carried as structured fields: this is the text an + // author reads when a gate fails. + expect(failure!['message']).toContain('GATE_STDOUT_MARKER'); + expect(failure!['message']).toContain('GATE_STDERR_MARKER'); + + // The journal is the durable record; the report above is one view of it. + const client = await connectClient(dataDir); + const { entries } = await client.journalRead(report.runId, 1, 1000); + const completed = (entries as Array>).find(entry => + journalType(entry) === 'step.completed' && entry['step_id'] === 'emit.gate'); + expect(completed).toMatchObject({ + payload: { + completionReason: 'retries_exhausted', + output: { stdout_tail: 'GATE_STDOUT_MARKER\n', stderr_tail: 'GATE_STDERR_MARKER\n' }, + }, + }); + + // A control, so the assertions above cannot pass on a fixed string: a gate + // that fails silently reports empty tails rather than someone else's. + const silent = invokeCli(['run', '--json', '--data-dir', dataDir, gated('silent', 'false')]); + expect(silent.status, silent.stderr).toBe(1); + const quiet = (JSON.parse(silent.stdout) as { diagnostics: Array> }) + .diagnostics.find(diagnostic => diagnostic['kind'] === 'step_failed'); + // An empty stdout tail is omitted rather than reported as "" (step-failure.ts), + // so absent and empty both mean "the gate said nothing" here. + expect(quiet).toMatchObject({ stepId: 'emit.gate', stderrTail: '' }); + expect(quiet!['stdoutTail'] ?? '').toBe(''); + expect(JSON.stringify(quiet)).not.toContain('GATE_STDOUT_MARKER'); + }, 30_000); +}); + describe('JournalClient wire conformance against live relayflowd', () => { it('exercises every protocol-v0 verb with the real server', async () => { const dataDir = temporaryDirectory('flows-live-wire-'); diff --git a/packages/sdk/tests/preflight.test.ts b/packages/sdk/tests/preflight.test.ts index 6c5596f9c..e15c6369c 100644 --- a/packages/sdk/tests/preflight.test.ts +++ b/packages/sdk/tests/preflight.test.ts @@ -582,6 +582,10 @@ describe('preflight: CLI resolution and refusal predicates', () => { { probes: probes({ command: (c) => c !== '/nonexistent-gate-binary-xxx' }) }), preflight(flow({ id: 'a', type: 'agent', instruction: 'i', cli: 'x', verification: { type: 'artifact_exists', path: '../escape.md' } }), { probes: probes() }), + // `gate_path_unreachable`: a well-formed path the bundled worker's + // artifact scan can never journal, so the gate is unsatisfiable. + preflight(flow({ id: 'a', type: 'agent', instruction: 'i', cli: 'x', + verification: { type: 'artifact_exists', path: '.workflow-artifacts/review.md' } }), { probes: probes() }), // `scope_syntax_invalid`: grant string doesn't match "mount/path: mode". preflight({ ...flow({ id: 'a', type: 'deterministic', command: 'x' }), workspace: 'not-a-grant' } as FlowSpec, { probes: probes() }), diff --git a/summary.md b/summary.md new file mode 100644 index 000000000..ecdcab6d8 --- /dev/null +++ b/summary.md @@ -0,0 +1,175 @@ +# Spend the analysis `flows check` already does + +`flows check` computed the facts that would have prevented five dead runs and +said nothing about them. This spends two of them, and establishes that the +third fact is not a fact. + +## What changed + +### `agent_worker_unresolved` — a check-only warning (permanent) + +A spec with `agent` steps needs a worker *attached for step type `agent`*. +Without one the run parks at the first such step. `flows check` already walks +those steps to print `REQUIRES codex (step "implement"), …` — the analysis +exists; it just stopped one sentence short of the remedy. + +``` +REQUIRES claude (step "implement") +WARNING [agent_worker_unresolved] 2 agent steps ("implement", "review") require an +attached worker. For a local run, use `flows run --local-agent `, unless you +already attach an agent worker for this daemon. `flows check` does not verify +worker attachment: with no worker attached the run parks at the first agent step. +``` + +Design decisions worth naming: + +- **Warning, never a refusal, worded as a requirement rather than a + prediction.** Worker attachment is *unknown* to `flows check`, not absent: + `check` is daemon-free by construction, so it cannot see a worker attached + elsewhere. Saying "this will park" would be a guess. +- **It is a property of the invocation, not of the spec,** so it opts in + through a new `CheckInvocation` seam on `checkFlow` / `checkAuthoredFlow` + rather than entering `preflight`, which stays a pure function of the spec + plus environment probes. Only `flows check` sets it. `flows run` knows the + answer (it attaches its own worker under `--local-agent`), `flows build` and + `flows deploy` check a spec that will run elsewhere, and an SDK caller + reaching `checkAuthoredFlow` directly is generally running a worker already. +- **Scoped to `agent` steps.** `--local-agent` attaches no `llm` worker on the + YAML path, so counting `llm` steps and naming that flag for them would be + false. They are not counted and not mentioned. +- **Counted from the compiled steps, not from `requirements`.** A YAML helper + step (`slack: { post: … }`) compiles to an `agent` step carrying a helper + envelope and needs the same SDK worker, but `requirements` reports it as an + integration, dedupes by harness, and includes `llm` use. Compilation is the + only walk that sees every step that needs the worker. +- **Emitted in the `REQUIRES` position,** because it reads as a footnote to + that line. `emitCheckReport` holds it back from the leading diagnostic batch + and emits it once, after `REQUIRES` and before `CHECK PASSED`; diagnostics + stay on stderr, report lines stay on stdout. `--json` returns before any of + that and keeps the single ordered `diagnostics` array. +- **Emitted alongside a preflight refusal.** An environment refusal is fixed + and rerun; the worker question is still open on the next pass, and staying + silent about it is what made an author meet it one dead run at a time. + +Known blind spot, documented rather than papered over: an authored `.flow.ts` +is checked through `checkMcpHeader`, which preflights a synthetic one-step +header spec without compiling the body, so there are no agent steps to count. +Same blind spot as `permissions_unenforced`. + +### `gate_path_unreachable` — a preflight refusal (temporary, retires with #513) + +An `artifact_exists` gate reads the journaled `output.artifacts` list and +nothing else. The bundled worker's scan skips any entry whose name starts with +`.` and any entry named exactly `node_modules`. A gate on a path inside one of +those prefixes can therefore never match, however faithfully the agent writes +the file. That is *unsatisfiable*, not merely unproven — so, as the issue +suggested, it refuses: + +``` +REFUSED [gate_path_unreachable] Step "review" gates on artifact_exists path +".workflow-artifacts/rust/review.md", but the bundled agent worker's artifact scan +never records anything under ".workflow-artifacts": it skips entries whose name +starts with "." and entries named "node_modules". The path can never appear in the +journaled output.artifacts this gate reads, so the gate cannot pass. Write the +artifact to a scanned path. +``` + +- **One rule, two consumers.** The exclusion predicate moved out of + `agent-artifacts.ts` into `src/artifact-scan-policy.ts`; the real scan now + consumes it, so the refusal cannot drift from the scan it describes. A + parity test writes all twelve case paths to a temp dir and asserts the real + `snapshotWorkspaceFiles` output is exactly the predicate's complement. +- **The whole excluded prefix is named,** not the offending segment alone — + that prefix is the directory the author has to move the artifact out of. +- **Segments are compared exactly, with no normalization.** The gate matches + the author's literal string against the worker's literal list, so + `node_modules-copy/out.md`, `reports/node_modules.md`, `review.md` and + `reports/v1.2/review.md` are all accepted, and a backslash is an ordinary + filename character rather than a separator. +- **Collected before preflight's early returns.** `probeNamedGate` runs after + CLI resolution, model governance, scope and budget can each return, so an + author with an unresolved CLI would not learn about the dead gate until a + later pass — or at run time. A test pins the diagnostic order as + `['gate_path_unreachable', 'cli_unresolved']`. +- **Scope consequence, recorded deliberately:** the kind lives in + `PREFLIGHT_ENVIRONMENT_FAILURE_KINDS`, so it also fires at `flows build` and + on SDK submissions. That is correct — the gate is unsatisfiable under the + bundled worker regardless of host — and it is a property of the *bundled* + worker, not of the journal protocol: `step.complete` accepts any `output`, + so a custom worker may journal a hidden path. + +### `gate_output_not_captured` — **not added**, because it is not true + +The acceptance criterion was conditional: warn that a `subprocess_gate`'s +output is not captured *"for as long as that is true"*. It is not true today. +Verified against a live `relayflowd` before writing anything: + +``` +"stepId": "emit.gate", "completionReason": "retries_exhausted", "exitCode": 1, +"stdoutTail": "GATE_STDOUT_MARKER\n", "stderrTail": "GATE_STDERR_MARKER\n" +``` + +and in the journal itself, on the lowered gate step's `step.completed`: + +```json +"output": { "exit_code": 1, "stdout_tail": "GATE_STDOUT_MARKER\n", + "stderr_tail": "GATE_STDERR_MARKER\n" } +``` + +A warning claiming otherwise would have been a false diagnostic added to a +change whose whole point is that `check` should only say what it knows. What +the criterion actually asks for is that the warning exist exactly while the +bug does — so instead of the warning, this adds a **live-kernel regression +that pins the capture**. If capture ever regresses, that test fails, and the +warning becomes warranted at the moment it becomes true. + +## Tests + +- `tests/artifact-gates.test.ts` — six parameterised refusals asserting the + full excluded prefix is named; six lookalike paths that must be accepted; + the scan/predicate parity test against a real `snapshotWorkspaceFiles` run; + the early-return ordering test; and an end-to-end `checkFlow` refusal. +- `tests/check-worker-surface.test.ts` (new) — message content, singular vs + plural and the "and N more" truncation, exit code 0, the YAML helper step + being counted where `requirements` omits it, silence for `llm`-only and + deterministic-only flows, survival alongside a preflight refusal, opt-in + discipline, `--json` emitting it once in both the payload and on stderr, and + an **ordered `CliIo` transcript** (both streams in one list) pinning the + position after `REQUIRES` and before `CHECK PASSED` — in both the + `REQUIRES`-present and `REQUIRES`-absent shapes. +- `tests/preflight.test.ts` — `gate_path_unreachable` added to the + refusal-reachability list, which asserts set equality against + `PREFLIGHT_FAILURE_KINDS`. +- `tests/live-kernel.test.ts` (new block) — the `subprocess_gate` capture + regression, asserting both the rendered message and the journal payload, with + a silent-gate control so the assertions cannot pass on a fixed string. + +`npm test` in `packages/sdk`: 2380 passed, 40 failed. All 40 failures are +pre-existing and environmental — they reproduce identically on a stashed tree +(same 40 failures, same 7 files): `relayflowd` looked up under +`kernel/target/{debug,release}/` while this toolchain builds outside the repo, +and flows needing real harness CLIs. `tsc --noEmit` is clean for `src`, the +type tests, and `tests`. + +## Files + +| File | Change | +| --- | --- | +| `src/artifact-scan-policy.ts` | new — the scan's exclusion rule as a pure predicate, shared | +| `src/named-gate-preflight.ts` | new — the `gate_path_unreachable` refusal (retires with #513) | +| `src/cli/check-worker-surface.ts` | new — the `agent_worker_unresolved` warning | +| `src/agent-artifacts.ts` | consumes the extracted predicate instead of its own copy | +| `src/preflight.ts` | collects gate reachability before every early return | +| `src/failure-kinds.ts` | the two new kinds, each with its retirement note | +| `src/cli/check.ts` | `CheckInvocation` opt-in seam | +| `src/cli.ts` | opts in at the `check` dispatch; defers the warning to the `REQUIRES` position | +| `docs/SURFACE.md` | documents both kinds where the facts they describe already live | + +Each diagnostic is a distinct kind, so it can be suppressed and later deleted +on its own. Both temporary kinds carry their retirement condition in a comment +at the definition site. + +## Out of scope, unchanged + +The underlying bugs (#511, #513) are not fixed, and the spec is not +round-tripped past the daemon's validator (#502). From 2732505a6cf09fde240b3facb8c58636faebef87 Mon Sep 17 00:00:00 2001 From: Relayflow Date: Sun, 20 Sep 2026 17:38:22 +0000 Subject: [PATCH 2/2] fix: warn rather than refuse when a gate path is unscanned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The refusal assumed the bundled worker's artifact scan is the only writer of `output.artifacts`. It is not: the same worker journals object-shaped JSON stdout and completed Relay task output verbatim, so an agent can report an excluded path itself and satisfy the gate. Refusing rejected those specs at `check`, `run`, `build` and every SDK submission. `gate_path_unreachable` becomes the warning `gate_path_unscanned` — the old name asserted the falsehood — and the message reports the scan's limitation and the routes that bypass it instead of declaring the gate impossible. A regression drives the real `AgentWorker` over an excluded path the fake CLI both writes and reports as JSON, then runs the real lowered gate command over exactly what the worker journaled: the scan records nothing, the gate exits 0. Co-Authored-By: Claude --- docs/SURFACE.md | 32 ++++--- packages/sdk/src/artifact-scan-policy.ts | 19 ++-- packages/sdk/src/failure-kinds.ts | 24 +++-- packages/sdk/src/named-gate-preflight.ts | 41 ++++---- packages/sdk/src/preflight.ts | 13 +-- packages/sdk/tests/artifact-gates.test.ts | 109 +++++++++++++++++----- packages/sdk/tests/preflight.test.ts | 9 +- summary.md | 83 +++++++++------- 8 files changed, 214 insertions(+), 116 deletions(-) diff --git a/docs/SURFACE.md b/docs/SURFACE.md index c69f1e3b7..cac1973ad 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -1168,19 +1168,25 @@ asserts on `AgentResult.artifacts` — which is how this was caught at all, by That exclusion cuts both ways. A gate reads the journaled list and nothing else, so an `artifact_exists` path inside a prefix the walk skips — any segment -starting with `.`, or a segment named exactly `node_modules` — can never match, -however faithfully the agent writes the file. The gate is unsatisfiable rather -than merely unproven, so preflight **refuses** it as `gate_path_unreachable`, -naming the excluded prefix the artifact has to move out of. Segments are -compared exactly: `node_modules-copy/out.md` and `reports/v1.2/review.md` are -fine. The rule lives in `packages/sdk/src/artifact-scan-policy.ts` and the walk -itself consumes it, so the refusal cannot drift from the scan it describes. -The refusal is collected before any environment probe can return early, so a -dead gate is not hidden behind an unresolved CLI for a pass or two. It is a -property of the *bundled* worker, not of the protocol — `step.complete` accepts -any `output`, so a custom worker may journal a hidden path — and it retires -with the exclusion: when the scan stops skipping those prefixes, the kind and -`packages/sdk/src/named-gate-preflight.ts` are deleted together. +starting with `.`, or a segment named exactly `node_modules` — is a path the +scan can never contribute, however faithfully the agent writes the file. +Preflight **warns** `gate_path_unscanned`, naming the excluded prefix. Segments +are compared exactly: `node_modules-copy/out.md` and `reports/v1.2/review.md` +are fine. The rule lives in `packages/sdk/src/artifact-scan-policy.ts` and the +walk itself consumes it, so the warning cannot drift from the scan it +describes, and it is collected before any environment probe can return early, +so it is not hidden behind an unresolved CLI for a pass or two. + +It warns rather than refuses because the scan is not the only writer of that +list, which makes such a gate unproven rather than unsatisfiable. The *bundled* +worker journals object-shaped JSON stdout — and a completed Relay task's output +— verbatim in place of the scanned wrapper, so an agent that answers with its +own `{"artifacts": [...]}` puts exactly what it names in the list, a hidden path +included, and the gate passes. `step.complete` accepts any `output`, so a custom +worker may do the same. Which route a step takes is a run-time fact, so +preflight reports the scan's limitation and leaves the verdict to the run. The +warning retires with the exclusion: when the scan stops skipping those prefixes, +the kind and `packages/sdk/src/named-gate-preflight.ts` are deleted together. - Are YAML helper verbs (`slack:`, `mcp:`) core spec vocabulary or compile-time expansion into `run`/effect steps? Leaning: expansion — the kernel spec stays seven words; helpers stay a surface concern. - Helper generation cadence: generated from relayfile adapter manifests at build time vs published per-adapter packages. Leaning: generated, with hand-tuned verb names for the top providers. diff --git a/packages/sdk/src/artifact-scan-policy.ts b/packages/sdk/src/artifact-scan-policy.ts index ec98f74bf..193a32bae 100644 --- a/packages/sdk/src/artifact-scan-policy.ts +++ b/packages/sdk/src/artifact-scan-policy.ts @@ -3,15 +3,20 @@ * * `agent-artifacts.ts` walks an agent step's cwd and records every regular * file it finds, except entries whose name starts with `.` and entries named - * exactly `node_modules` — files as well as directories, in both cases. That - * walk decides what lands in the journaled `output.artifacts` list, which is - * the only thing an `artifact_exists` gate reads (`named-gate-lowering.ts` - * tests exact membership in that list and never touches disk). + * exactly `node_modules` — files as well as directories, in both cases. What + * that walk records is one source of the journaled `output.artifacts` list, + * which is the only thing an `artifact_exists` gate reads + * (`named-gate-lowering.ts` tests exact membership in that list and never + * touches disk). * * Static analysis needs the same rule without importing filesystem traversal, - * so the rule lives here and both consume it. This is a property of the - * bundled worker, not of the journal protocol: `JournalClient.stepComplete` - * accepts any `output`, so a custom worker is free to journal a hidden path. + * so the rule lives here and both consume it. It bounds the *scan*, not the + * list: `worker.ts` journals object-shaped JSON stdout and completed Relay + * task output verbatim instead of the scanned wrapper, and + * `JournalClient.stepComplete` accepts any `output` from any worker — so a + * path this predicate excludes can still reach the list by those routes + * (`named-gate-preflight.ts` warns rather than refusing for exactly that + * reason). */ /** Entry names the walk never descends into or records. */ diff --git a/packages/sdk/src/failure-kinds.ts b/packages/sdk/src/failure-kinds.ts index e60bced80..1668f679a 100644 --- a/packages/sdk/src/failure-kinds.ts +++ b/packages/sdk/src/failure-kinds.ts @@ -2,22 +2,10 @@ import { PLUGIN_FAILURE_KINDS } from './plugin-manifest.js'; import { NAMED_GATE_FAILURE_KINDS } from './named-gates.js'; const SHARED_SPEC_FAILURE_KINDS = ['invalid_spec'] as const; -/** - * Environment refusal kinds produced after spec validation succeeds. - * - * `gate_path_unreachable` is the one member decided from the compiled - * snapshot alone: an `artifact_exists` path inside a prefix the bundled agent - * worker's artifact scan excludes can never appear in the journaled - * `output.artifacts` the gate reads, so the gate is unsatisfiable rather than - * merely unproven. It lives in this list because it is the list both - * `PREFLIGHT_FAILURE_KINDS` and `CHECK_FAILURE_KINDS` draw from. Temporary: - * it describes #513, and retires with `named-gate-preflight.ts` when the scan - * stops excluding those paths. - */ +/** Environment refusal kinds produced after spec validation succeeds. */ const PREFLIGHT_ENVIRONMENT_FAILURE_KINDS = [ ...NAMED_GATE_FAILURE_KINDS, ...PLUGIN_FAILURE_KINDS, - 'gate_path_unreachable', 'helper_provider.mount_required', 'helper_provider.unsupported', 'helper_slack.credential_missing', @@ -86,6 +74,15 @@ export const CHECK_FAILURE_KINDS = [ * (#442). Warning-only by design: the declaration stays legal and the flow * still runs, but an author who wrote one must not be left believing it * sandboxes the step. + * + * `gate_path_unscanned` names an `artifact_exists` path inside a prefix the + * bundled agent worker's artifact scan excludes, so the scan can never put it + * in the journaled `output.artifacts` the gate reads. It warns rather than + * refuses because the scan is not the only writer of that list — the same + * worker promotes JSON stdout and completed Relay task output verbatim, and a + * custom worker may journal anything — so such a gate is unproven, not + * unsatisfiable. Temporary: it describes #513, and retires with + * `named-gate-preflight.ts` when the scan stops excluding those paths. */ export const PREFLIGHT_WARNING_KINDS = [ 'unprovable_effects', @@ -95,6 +92,7 @@ export const PREFLIGHT_WARNING_KINDS = [ 'vacuous_gate', 'budget_unmetered', 'permissions_unenforced', + 'gate_path_unscanned', ] as const; /** diff --git a/packages/sdk/src/named-gate-preflight.ts b/packages/sdk/src/named-gate-preflight.ts index d501827e8..8d782d11f 100644 --- a/packages/sdk/src/named-gate-preflight.ts +++ b/packages/sdk/src/named-gate-preflight.ts @@ -1,29 +1,32 @@ import { unscannedArtifactPrefix } from './artifact-scan-policy.js'; import { isNamedGate } from './named-gates.js'; import type { StepSpec } from './spec.js'; -import type { PreflightRefusal } from './preflight.js'; +import type { PreflightWarning } from './preflight.js'; /** - * Static named-gate reachability, decided from the compiled snapshot alone. + * Static named-gate scan coverage, decided from the compiled snapshot alone. * * An `artifact_exists` gate lowers to a deterministic step that asks whether * the producer's journaled `output.artifacts` list contains the literal path - * (`named-gate-lowering.ts`). The bundled agent worker builds that list by - * walking the step's cwd under the policy in `artifact-scan-policy.ts`, which - * records nothing under a dot-named or `node_modules` entry. A path inside one - * of those prefixes therefore cannot pass under that worker, on any host, in - * any environment — so it is refused here rather than after a run. + * (`named-gate-lowering.ts`). One writer of that list is the bundled agent + * worker's filesystem scan, which walks the step's cwd under the policy in + * `artifact-scan-policy.ts` and records nothing under a dot-named or + * `node_modules` entry. A path inside one of those prefixes can never come + * from that scan, on any host, in any environment. * - * Scoped to the bundled worker on purpose. The journal protocol lets a custom - * worker submit any `output`, and the gate does not reject a hidden path in - * such a list. The refusal names the policy it applies so the scope is legible - * in the message, and applies wherever public preflight runs: `flows check`, - * `flows run`, `flows build` and SDK submissions all share that path. + * A warning rather than a refusal, because the scan is not the only writer. + * The same bundled worker promotes object-shaped JSON stdout — and a completed + * Relay task's output — straight to the journaled `output` (`worker.ts`), so + * an agent that reports its own `artifacts` array makes a hidden path pass; + * the journal protocol lets a custom worker do the same. Which of those an + * `agent` step will take is not decidable from the spec, so this reports the + * scan's limitation and leaves the verdict to the run rather than declaring a + * satisfiable gate impossible. * * Temporary: this describes #513. When the scan stops excluding these paths, * delete this module and its call site. */ -export function namedGateReachabilityDiagnostics(step: StepSpec): PreflightRefusal[] { +export function namedGateScanCoverageDiagnostics(step: StepSpec): PreflightWarning[] { const gate = step.verification; if (!isNamedGate(gate) || gate.type !== 'artifact_exists') return []; // A malformed path is already `gate_path_invalid` from compilation; only @@ -31,12 +34,14 @@ export function namedGateReachabilityDiagnostics(step: StepSpec): PreflightRefus const prefix = unscannedArtifactPrefix(gate.path); if (prefix === undefined) return []; return [{ - severity: 'refusal', - kind: 'gate_path_unreachable', + severity: 'warning', + kind: 'gate_path_unscanned', stepId: step.id, message: `Step "${step.id}" gates on artifact_exists path "${gate.path}", but the bundled agent worker's ` - + `artifact scan never records anything under "${prefix}": it skips entries whose name starts with "." ` - + 'and entries named "node_modules". The path can never appear in the journaled output.artifacts this ' - + 'gate reads, so the gate cannot pass. Write the artifact to a scanned path.', + + `artifact scan records nothing under "${prefix}": it skips entries whose name starts with "." and ` + + 'entries named "node_modules". The gate reads the journaled output.artifacts and never the disk, so ' + + 'writing the file is not enough: the step has to report the path itself — as object-shaped JSON stdout ' + + 'carrying its own "artifacts" array, as a completed Relay task output, or from a custom worker. If the ' + + 'gate is meant to rest on the scan, write the artifact to a path the scan records.', }]; } diff --git a/packages/sdk/src/preflight.ts b/packages/sdk/src/preflight.ts index a7df582f8..0df6b1a3d 100644 --- a/packages/sdk/src/preflight.ts +++ b/packages/sdk/src/preflight.ts @@ -12,7 +12,7 @@ import { compileSpec, CompileError } from './compile.js'; import { helperCall } from './yaml-helpers.js'; import { resolveCliModelSelection, type CliModelSource } from './cli-adapter.js'; import { isNamedGate, NAMED_GATE_FAILURE_KINDS, type NamedGateFailureKind } from './named-gates.js'; -import { namedGateReachabilityDiagnostics } from './named-gate-preflight.js'; +import { namedGateScanCoverageDiagnostics } from './named-gate-preflight.js'; import { compileScopes, type ScopeInput, type MountRegistry } from './scope-compiler.js'; import { readMountRegistry } from './mount-registry.js'; import type { @@ -232,11 +232,12 @@ function preflightSync(flow: unknown, options: PreflightOptions): PreflightResul // permissions they declared on the same file are not enforced. diagnostics.push(...permissionsDiagnostics(compiled)); diagnostics.push(...scopeDiagnostics(compiled, options)); - // Same rule, same reason: an `artifact_exists` path the bundled worker's - // scan can never journal is decidable from the snapshot, so it is collected - // here rather than beside `probeNamedGate` below — which CLI resolution, an - // unknown model, a bad scope or a budget refusal can all return before. - diagnostics.push(...compiled.steps.flatMap(namedGateReachabilityDiagnostics)); + // Same rule, same reason: whether the bundled worker's artifact scan can + // reach an `artifact_exists` path is decidable from the snapshot, so it is + // collected here rather than beside `probeNamedGate` below — which CLI + // resolution, an unknown model, a bad scope or a budget refusal all return + // before. + diagnostics.push(...compiled.steps.flatMap(namedGateScanCoverageDiagnostics)); for (const server of new Set(options.mcpServers ?? [])) { if (options.mcp !== undefined && Object.hasOwn(options.mcp, server)) continue; diagnostics.push({ severity: 'refusal', kind: 'mcp_undeclared_server', server, diff --git a/packages/sdk/tests/artifact-gates.test.ts b/packages/sdk/tests/artifact-gates.test.ts index 9b1a4bc37..4243ca249 100644 --- a/packages/sdk/tests/artifact-gates.test.ts +++ b/packages/sdk/tests/artifact-gates.test.ts @@ -6,8 +6,10 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; // A fake `claude` whose "work" is whatever the test's `onSpawn` hook writes // into the cwd it was spawned in, so artifact detection is exercised without -// a real CLI. +// a real CLI. `claudeResult` is the text of its final result frame; set it to +// JSON to take the worker's JSON-output path. let onSpawn: ((cwd: string | undefined) => void) | undefined; +let claudeResult = 'done'; vi.mock('node:child_process', async () => { const actual = await vi.importActual('node:child_process'); return { @@ -25,7 +27,7 @@ vi.mock('node:child_process', async () => { setImmediate(() => { onSpawn?.(options['cwd'] as string | undefined); stdout.emit('data', Buffer.from(cli === 'claude' - ? JSON.stringify({ type: 'result', result: 'done', usage: { input_tokens: 1, output_tokens: 1 }, total_cost_usd: 0 }) + ? JSON.stringify({ type: 'result', result: claudeResult, usage: { input_tokens: 1, output_tokens: 1 }, total_cost_usd: 0 }) : JSON.stringify({ type: 'usage', usage: { input_tokens: 1, output_tokens: 1, total_cost_usd: 0 } }))); child.emit('close', 0); }); @@ -35,6 +37,8 @@ vi.mock('node:child_process', async () => { }); import { runAgentCli } from '../src/worker-cli.js'; +import { AgentWorker } from '../src/worker.js'; +import type { JournalClient } from '../src/journal-client.js'; import { compileSpec } from '../src/compile.js'; import { lowerNamedGates } from '../src/named-gate-lowering.js'; import { namedGateErrors, namedGateFailure } from '../src/named-gates.js'; @@ -46,7 +50,7 @@ import { snapshotWorkspaceFiles } from '../src/agent-artifacts.js'; import { execFileSync } from 'node:child_process'; const dirs: string[] = []; -afterEach(() => { onSpawn = undefined; for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); }); +afterEach(() => { onSpawn = undefined; claudeResult = 'done'; for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); }); function tempDir(): string { const d = mkdtempSync(join(tmpdir(), 'artifact-gates-')); dirs.push(d); return d; } describe('worker-side artifacts', () => { @@ -153,11 +157,12 @@ describe('artifact_exists named gate', () => { /** * #513: the gate reads the worker's journaled `artifacts` list, and the - * bundled worker's scan never puts a dot-named or `node_modules` path in it. - * That makes such a gate statically unsatisfiable, which preflight now refuses - * rather than letting a run discover it. Retire this block with the module. + * bundled worker's *scan* never puts a dot-named or `node_modules` path in it. + * The scan is not the only writer of that list, so preflight warns instead of + * refusing — the last case here is the run that proves why. Retire this block + * with the module. */ -describe('artifact_exists named gate: static reachability', () => { +describe('artifact_exists named gate: static scan coverage', () => { const probes = () => ({ cli: () => ({ exists: true, authenticated: true }), executor: () => true, command: () => true }); const gated = (path: string) => ({ version: '0.1.0', name: 'x', @@ -174,13 +179,13 @@ describe('artifact_exists named gate: static reachability', () => { // The whole prefix, not the offending segment alone: that prefix is the // directory the author has to move the artifact out of. ['a/b/.c/d/e.md', 'a/b/.c'], - ])('refuses %s and names the excluded prefix %s', (path, prefix) => { + ])('warns on %s and names the excluded prefix %s, without refusing', (path, prefix) => { const result = preflight(gated(path) as never, { probes: probes() }); - expect(result.ok).toBe(false); - const refusal = result.diagnostics.find(d => d.kind === 'gate_path_unreachable'); - expect(refusal, JSON.stringify(result.diagnostics)).toMatchObject({ severity: 'refusal', stepId: 'review' }); - expect(refusal!.message).toContain(`"${prefix}"`); - expect(refusal!.message).toContain(path); + const warning = result.diagnostics.find(d => d.kind === 'gate_path_unscanned'); + expect(warning, JSON.stringify(result.diagnostics)).toMatchObject({ severity: 'warning', stepId: 'review' }); + expect(warning!.message).toContain(`"${prefix}"`); + expect(warning!.message).toContain(path); + expect(result.ok).toBe(true); }); it.each([ @@ -194,9 +199,9 @@ describe('artifact_exists named gate: static reachability', () => { // A backslash is an ordinary filename character in a POSIX path, never a // separator: `readdir` reports one entry whose name starts with "d". String.raw`dir\.hidden.md`, - ])('accepts %s, which the scan does record', (path) => { + ])('says nothing about %s, which the scan does record', (path) => { const result = preflight(gated(path) as never, { probes: probes() }); - expect(result.diagnostics.filter(d => d.kind === 'gate_path_unreachable')).toEqual([]); + expect(result.diagnostics.filter(d => d.kind === 'gate_path_unscanned')).toEqual([]); expect(result.ok).toBe(true); }); @@ -216,7 +221,7 @@ describe('artifact_exists named gate: static reachability', () => { const scanned = new Set((await snapshotWorkspaceFiles(cwd)).keys()); // The predicate is the scan's rule, so "excluded by the predicate" and // "absent from the scan" must be the same set. A drift in either - // direction would make the refusal lie. + // direction would make the warning lie. for (const path of paths) { expect(scanned.has(path), path).toBe(unscannedArtifactPrefix(path) === undefined); } @@ -224,18 +229,18 @@ describe('artifact_exists named gate: static reachability', () => { it('is reported even when an unrelated environment refusal returns first', () => { // `cli_unresolved` returns from preflight before any probe runs. The - // author fixes the CLI, reruns, and would otherwise meet the dead gate - // only on the pass after that — or at run time. + // author fixes the CLI, reruns, and would otherwise meet the unscanned + // path only on the pass after that — or at run time. const result = preflight({ version: '0.1.0', name: 'x', steps: [{ id: 'review', type: 'agent', instruction: 'i', verification: { type: 'artifact_exists', path: '.out/review.md' } }], } as never, { probes: probes() }); - expect(result.diagnostics.map(d => d.kind)).toEqual(['gate_path_unreachable', 'cli_unresolved']); + expect(result.diagnostics.map(d => d.kind)).toEqual(['gate_path_unscanned', 'cli_unresolved']); }); - it('refuses through flows check, naming the step and the prefix', () => { + it('warns through flows check, naming the step, and refuses nothing for it', () => { const dir = tempDir(); writeFileSync(join(dir, 'flows.json'), JSON.stringify({ cli: 'true' })); writeFileSync(join(dir, 'flow.yaml'), JSON.stringify({ version: '0.1.0', name: 'gated', steps: [ @@ -245,9 +250,69 @@ describe('artifact_exists named gate: static reachability', () => { const report = checkFlow(join(dir, 'flow.yaml')).report; - expect(report.ok).toBe(false); expect(report.diagnostics).toContainEqual(expect.objectContaining({ - severity: 'refusal', kind: 'gate_path_unreachable', stepId: 'review', + severity: 'warning', kind: 'gate_path_unscanned', stepId: 'review', })); + // `true` is a real binary that is not an agent CLI, so this report does + // refuse — for that, and only that. The gate adds no refusal of its own. + expect(report.diagnostics.filter(d => d.severity === 'refusal').map(d => d.kind)).toEqual(['cli_unsupported']); + }); + + /** + * Why it may not refuse. The scan is one writer of `output.artifacts`; the + * same bundled worker promotes an agent's object-shaped JSON stdout to + * `output` verbatim (`worker.ts`), hidden paths included. This runs the real + * `AgentWorker` over a dispatch whose fake `claude` writes the dot-directory + * file AND reports it, then runs the real lowered gate command over exactly + * what the worker journaled. A refusal would have rejected this spec before + * submission. + */ + it('does not refuse a gate the bundled worker itself can satisfy through JSON output', async () => { + const cwd = tempDir(); + const path = '.workflow-artifacts/review.md'; + onSpawn = (dir) => { + mkdirSync(join(dir!, '.workflow-artifacts'), { recursive: true }); + writeFileSync(join(dir!, '.workflow-artifacts/review.md'), 'findings'); + }; + claudeResult = JSON.stringify({ artifacts: [path] }); + const authored = { version: '0.1.0', name: 'x', steps: [{ id: 'review', type: 'agent', instruction: 'i', + cli: 'claude', cwd, verification: { type: 'artifact_exists', path } }] }; + + // `claude` carries a default model, so its probe has to answer the + // model-scoped readiness question too; nothing else about it matters here. + const checked = preflight(authored as never, { probes: { ...probes(), + cli: () => ({ exists: true, authenticated: true, modelAvailable: true }) } }); + expect(checked.diagnostics.map(d => d.kind)).toEqual(['gate_path_unscanned']); + expect(checked.ok).toBe(true); + + const lowered = lowerNamedGates(compileSpec(authored).steps); + const client = new EventEmitter() as EventEmitter & Record; + client.workerAttach = async () => ({}); + client.stepHeartbeat = async () => ({ lease_deadline_ms: Date.now() + 30_000 }); + const completed = new Promise>((resolve, reject) => { + client.stepComplete = async (...args: unknown[]) => { resolve(args[5] as Record); return {}; }; + client.once('worker-error', reject); + }); + const worker = new AgentWorker(client as unknown as JournalClient, { + workerId: 'w', pins: { workspace: [], streams: [] }, + }); + worker.on('error', (error: unknown) => client.emit('worker-error', error)); + await worker.attach(); + client.emit('step.dispatch', { + run_id: 'r', step_id: 'review', step_type: 'agent', attempt: 1, idempotency_key: 'k', + lease_id: 'l', lease_deadline_ms: Date.now() + 30_000, pins: { workspace: [], streams: [] }, + spec: lowered[0], + }); + const completion = await completed; + await worker.close(); + + // The scan recorded nothing (the file is under a dot-directory); the + // agent's own JSON is the journaled output, and the gate reads it. + expect(await snapshotWorkspaceFiles(cwd)).toEqual(new Map()); + expect(completion['output']).toEqual({ artifacts: [path] }); + const command = (lowered[1] as { command: string }).command; + expect(() => execFileSync('sh', ['-c', command], { + env: { ...process.env, FLOWS_INPUT: JSON.stringify({ output: completion['output'] }) }, stdio: 'pipe', + })).not.toThrow(); }); }); diff --git a/packages/sdk/tests/preflight.test.ts b/packages/sdk/tests/preflight.test.ts index e15c6369c..a6c1464cf 100644 --- a/packages/sdk/tests/preflight.test.ts +++ b/packages/sdk/tests/preflight.test.ts @@ -518,6 +518,11 @@ describe('preflight: CLI resolution and refusal predicates', () => { } as never), { probes: probes() }, ), + // An artifact_exists path the bundled worker's artifact scan never + // records. Another writer of `output.artifacts` may still produce it, + // so the gate is unproven rather than unsatisfiable: warn, never refuse. + preflight(flow({ id: 'a', type: 'agent', instruction: 'i', cli: 'x', + verification: { type: 'artifact_exists', path: '.workflow-artifacts/review.md' } }), { probes: probes() }), ]; const warningKinds = scenarios.flatMap((result) => result.diagnostics) .filter((diagnostic) => diagnostic.severity === 'warning') @@ -582,10 +587,6 @@ describe('preflight: CLI resolution and refusal predicates', () => { { probes: probes({ command: (c) => c !== '/nonexistent-gate-binary-xxx' }) }), preflight(flow({ id: 'a', type: 'agent', instruction: 'i', cli: 'x', verification: { type: 'artifact_exists', path: '../escape.md' } }), { probes: probes() }), - // `gate_path_unreachable`: a well-formed path the bundled worker's - // artifact scan can never journal, so the gate is unsatisfiable. - preflight(flow({ id: 'a', type: 'agent', instruction: 'i', cli: 'x', - verification: { type: 'artifact_exists', path: '.workflow-artifacts/review.md' } }), { probes: probes() }), // `scope_syntax_invalid`: grant string doesn't match "mount/path: mode". preflight({ ...flow({ id: 'a', type: 'deterministic', command: 'x' }), workspace: 'not-a-grant' } as FlowSpec, { probes: probes() }), diff --git a/summary.md b/summary.md index ecdcab6d8..adab0f127 100644 --- a/summary.md +++ b/summary.md @@ -56,47 +56,60 @@ is checked through `checkMcpHeader`, which preflights a synthetic one-step header spec without compiling the body, so there are no agent steps to count. Same blind spot as `permissions_unenforced`. -### `gate_path_unreachable` — a preflight refusal (temporary, retires with #513) +### `gate_path_unscanned` — a preflight warning (temporary, retires with #513) An `artifact_exists` gate reads the journaled `output.artifacts` list and -nothing else. The bundled worker's scan skips any entry whose name starts with -`.` and any entry named exactly `node_modules`. A gate on a path inside one of -those prefixes can therefore never match, however faithfully the agent writes -the file. That is *unsatisfiable*, not merely unproven — so, as the issue -suggested, it refuses: +nothing else. The bundled worker's *scan* skips any entry whose name starts +with `.` and any entry named exactly `node_modules`, so a gate on a path inside +one of those prefixes cannot rest on the scan, however faithfully the agent +writes the file: ``` -REFUSED [gate_path_unreachable] Step "review" gates on artifact_exists path +WARNING [gate_path_unscanned] Step "review" gates on artifact_exists path ".workflow-artifacts/rust/review.md", but the bundled agent worker's artifact scan -never records anything under ".workflow-artifacts": it skips entries whose name -starts with "." and entries named "node_modules". The path can never appear in the -journaled output.artifacts this gate reads, so the gate cannot pass. Write the -artifact to a scanned path. +records nothing under ".workflow-artifacts": it skips entries whose name starts with +"." and entries named "node_modules". The gate reads the journaled output.artifacts +and never the disk, so writing the file is not enough: the step has to report the +path itself — as object-shaped JSON stdout carrying its own "artifacts" array, as a +completed Relay task output, or from a custom worker. If the gate is meant to rest +on the scan, write the artifact to a path the scan records. ``` +The issue asked for a refusal, on the premise that such a gate is statically +unsatisfiable. That premise is false, and review F1 is right to reject it: the +scan is only one writer of `output.artifacts`. The same **bundled** worker +promotes object-shaped JSON stdout — and a completed Relay task's output — +verbatim into `output` (`worker.ts`), so an agent that answers with its own +`{"artifacts": [...]}` puts a hidden path in the list and the gate passes; and +`step.complete` accepts any `output` from any worker. Which route a step takes +is a run-time fact, so this reports the scan's limitation and leaves the verdict +to the run. A regression runs the real `AgentWorker` over a fake `claude` that +writes `.workflow-artifacts/review.md` *and* reports it as JSON, then runs the +real lowered gate command over exactly what the worker journaled: the scan +records nothing, the gate exits 0. A refusal would have rejected that spec at +`check`, `run`, `build` and every SDK submission. + - **One rule, two consumers.** The exclusion predicate moved out of `agent-artifacts.ts` into `src/artifact-scan-policy.ts`; the real scan now - consumes it, so the refusal cannot drift from the scan it describes. A + consumes it, so the warning cannot drift from the scan it describes. A parity test writes all twelve case paths to a temp dir and asserts the real `snapshotWorkspaceFiles` output is exactly the predicate's complement. - **The whole excluded prefix is named,** not the offending segment alone — - that prefix is the directory the author has to move the artifact out of. + that prefix is the directory an author relying on the scan has to move the + artifact out of. - **Segments are compared exactly, with no normalization.** The gate matches the author's literal string against the worker's literal list, so `node_modules-copy/out.md`, `reports/node_modules.md`, `review.md` and - `reports/v1.2/review.md` are all accepted, and a backslash is an ordinary + `reports/v1.2/review.md` say nothing, and a backslash is an ordinary filename character rather than a separator. - **Collected before preflight's early returns.** `probeNamedGate` runs after CLI resolution, model governance, scope and budget can each return, so an - author with an unresolved CLI would not learn about the dead gate until a - later pass — or at run time. A test pins the diagnostic order as - `['gate_path_unreachable', 'cli_unresolved']`. -- **Scope consequence, recorded deliberately:** the kind lives in - `PREFLIGHT_ENVIRONMENT_FAILURE_KINDS`, so it also fires at `flows build` and - on SDK submissions. That is correct — the gate is unsatisfiable under the - bundled worker regardless of host — and it is a property of the *bundled* - worker, not of the journal protocol: `step.complete` accepts any `output`, - so a custom worker may journal a hidden path. + author with an unresolved CLI would not learn about the unscanned path until + a later pass — or at run time. A test pins the diagnostic order as + `['gate_path_unscanned', 'cli_unresolved']`. +- **It lives in `PREFLIGHT_WARNING_KINDS`,** so it travels with every public + preflight consumer — `flows check`, `run`, `build` and SDK submissions — and + refuses none of them. ### `gate_output_not_captured` — **not added**, because it is not true @@ -125,10 +138,14 @@ warning becomes warranted at the moment it becomes true. ## Tests -- `tests/artifact-gates.test.ts` — six parameterised refusals asserting the - full excluded prefix is named; six lookalike paths that must be accepted; - the scan/predicate parity test against a real `snapshotWorkspaceFiles` run; - the early-return ordering test; and an end-to-end `checkFlow` refusal. +- `tests/artifact-gates.test.ts` — six parameterised warnings asserting the + full excluded prefix is named and that none of them refuses; six lookalike + paths that must say nothing; the scan/predicate parity test against a real + `snapshotWorkspaceFiles` run; the early-return ordering test; an end-to-end + `checkFlow` case pinning the warning and the unrelated refusal beside it; + and the F1 regression described above, which drives the real `AgentWorker` + and the real lowered gate command over an excluded path the agent reports + itself. - `tests/check-worker-surface.test.ts` (new) — message content, singular vs plural and the "and N more" truncation, exit code 0, the YAML helper step being counted where `requirements` omits it, silence for `llm`-only and @@ -137,14 +154,14 @@ warning becomes warranted at the moment it becomes true. an **ordered `CliIo` transcript** (both streams in one list) pinning the position after `REQUIRES` and before `CHECK PASSED` — in both the `REQUIRES`-present and `REQUIRES`-absent shapes. -- `tests/preflight.test.ts` — `gate_path_unreachable` added to the - refusal-reachability list, which asserts set equality against - `PREFLIGHT_FAILURE_KINDS`. +- `tests/preflight.test.ts` — `gate_path_unscanned` added to the + warning-reachability list, which asserts set equality against + `PREFLIGHT_WARNING_KINDS`. - `tests/live-kernel.test.ts` (new block) — the `subprocess_gate` capture regression, asserting both the rendered message and the journal payload, with a silent-gate control so the assertions cannot pass on a fixed string. -`npm test` in `packages/sdk`: 2380 passed, 40 failed. All 40 failures are +`npm test` in `packages/sdk`: 2381 passed, 40 failed. All 40 failures are pre-existing and environmental — they reproduce identically on a stashed tree (same 40 failures, same 7 files): `relayflowd` looked up under `kernel/target/{debug,release}/` while this toolchain builds outside the repo, @@ -156,10 +173,10 @@ type tests, and `tests`. | File | Change | | --- | --- | | `src/artifact-scan-policy.ts` | new — the scan's exclusion rule as a pure predicate, shared | -| `src/named-gate-preflight.ts` | new — the `gate_path_unreachable` refusal (retires with #513) | +| `src/named-gate-preflight.ts` | new — the `gate_path_unscanned` warning (retires with #513) | | `src/cli/check-worker-surface.ts` | new — the `agent_worker_unresolved` warning | | `src/agent-artifacts.ts` | consumes the extracted predicate instead of its own copy | -| `src/preflight.ts` | collects gate reachability before every early return | +| `src/preflight.ts` | collects gate scan coverage before every early return | | `src/failure-kinds.ts` | the two new kinds, each with its retirement note | | `src/cli/check.ts` | `CheckInvocation` opt-in seam | | `src/cli.ts` | opts in at the `check` dispatch; defers the warning to the `REQUIRES` position |