From 6d01d4eab4fe3fb96b5667374d23feab3422f51c Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 2 Sep 2026 20:34:08 +0200 Subject: [PATCH 1/5] fix(sdk): close timeout and dependency boundaries Session-Id: 01a062df-b92c-7a13-815f-147a82a4fc51 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- sdk/src/compile.ts | 19 +++---- sdk/src/spec.ts | 3 +- sdk/src/step-dependencies.ts | 40 ++++++++++----- sdk/src/step-fields.ts | 3 +- sdk/src/validate.ts | 13 ++--- sdk/tests/dependency-validation.test.ts | 67 +++++++++++++++++++++++++ sdk/tests/validate.test.ts | 2 +- sdk/tests/verb-field-lint.test.ts | 15 +++++- sdk/tsconfig.type-tests.json | 9 ++++ sdk/type-tests/step-fields.ts | 40 +++++++++++++++ 10 files changed, 172 insertions(+), 39 deletions(-) create mode 100644 sdk/tests/dependency-validation.test.ts create mode 100644 sdk/tsconfig.type-tests.json create mode 100644 sdk/type-tests/step-fields.ts diff --git a/sdk/src/compile.ts b/sdk/src/compile.ts index 35df7b388..f931359ea 100644 --- a/sdk/src/compile.ts +++ b/sdk/src/compile.ts @@ -97,7 +97,6 @@ function compileStep(step: StepSpec): StepSpec { ...(step.dependsOn !== undefined ? { dependsOn: step.dependsOn } : {}), ...(verification !== undefined ? { verification } : {}), maxIterations, - ...(step.timeoutMs !== undefined ? { timeoutMs: step.timeoutMs } : {}), }; switch (step.type as StepType) { @@ -105,7 +104,13 @@ function compileStep(step: StepSpec): StepSpec { const s = step as DeterministicStepSpec; // A deterministic step with no verification gets the implicit exit_code gate. const verification = s.verification ?? { type: 'exit_code' as const }; - return { ...base, type: 'deterministic', command: s.command, verification }; + return { + ...base, + type: 'deterministic', + command: s.command, + verification, + ...(s.timeoutMs !== undefined ? { timeoutMs: s.timeoutMs } : {}), + }; } case 'llm': { const s = step as LlmStepSpec; @@ -388,7 +393,6 @@ function toKernelStep(step: StepSpec): KernelStepSpec { ...(step.timeoutMs !== undefined ? { timeout_ms: step.timeoutMs } : {}), }; case 'llm': { - requireNoTimeout(step); return { ...common, type: 'llm', @@ -398,7 +402,6 @@ function toKernelStep(step: StepSpec): KernelStepSpec { }; } case 'agent': { - requireNoTimeout(step); const out: KernelAgentStep = { ...common, type: 'agent', @@ -425,14 +428,6 @@ function toKernelStep(step: StepSpec): KernelStepSpec { } } -function requireNoTimeout(step: StepSpec): void { - if (step.timeoutMs !== undefined) { - throw new CompileError([ - `step "${step.id}": only deterministic steps carry a timeout in spec v${SPEC_SCHEMA_VERSION}`, - ]); - } -} - function toKernelVerification(step: StepSpec): KernelVerificationSpec { const output = step.type === 'deterministic' ? undefined : step.output; if (output !== undefined) { diff --git a/sdk/src/spec.ts b/sdk/src/spec.ts index 76d0a9e83..e049822d1 100644 --- a/sdk/src/spec.ts +++ b/sdk/src/spec.ts @@ -104,7 +104,6 @@ export interface BaseStepSpec { verification?: VerificationSpec; /** Semantic retry bound (kernel DESIGN.md §1.2 `max_iterations`). Default 1. */ maxIterations?: number; - timeoutMs?: number; } /** @@ -115,6 +114,8 @@ export interface BaseStepSpec { export interface DeterministicStepSpec extends BaseStepSpec { type: 'deterministic'; command: string; + /** Wall-clock command timeout; worker-backed verbs own their dispatch timeout. */ + timeoutMs?: number; } /** diff --git a/sdk/src/step-dependencies.ts b/sdk/src/step-dependencies.ts index 4dbfb8258..2623b78a1 100644 --- a/sdk/src/step-dependencies.ts +++ b/sdk/src/step-dependencies.ts @@ -31,25 +31,41 @@ export function stepDependencyErrors( const WHITE = 0, GRAY = 1, BLACK = 2; const color = new Map(); for (const id of adjacency.keys()) color.set(id, WHITE); - const stack: string[] = []; - const visit = (id: string): void => { - color.set(id, GRAY); - stack.push(id); - for (const dependency of adjacency.get(id) ?? []) { + const path: string[] = []; + for (const start of adjacency.keys()) { + if (color.get(start) !== WHITE) continue; + + color.set(start, GRAY); + path.push(start); + const frames: Array<{ id: string; nextDependency: number }> = [ + { id: start, nextDependency: 0 }, + ]; + + while (frames.length > 0) { + const frame = frames[frames.length - 1]; + if (frame === undefined) break; + const dependencies = adjacency.get(frame.id) ?? []; + const dependency = dependencies[frame.nextDependency]; + + if (dependency === undefined) { + frames.pop(); + path.pop(); + color.set(frame.id, BLACK); + continue; + } + + frame.nextDependency += 1; const dependencyColor = color.get(dependency); if (dependencyColor === GRAY) { errors.push( - `spec.steps: dependency cycle detected at "${dependency}" (path: ${[...stack].join(' -> ')} -> ${dependency})`, + `spec.steps: dependency cycle detected at "${dependency}" (path: ${path.join(' -> ')} -> ${dependency})`, ); } else if (dependencyColor === WHITE) { - visit(dependency); + color.set(dependency, GRAY); + path.push(dependency); + frames.push({ id: dependency, nextDependency: 0 }); } } - stack.pop(); - color.set(id, BLACK); - }; - for (const id of adjacency.keys()) { - if (color.get(id) === WHITE) visit(id); } return errors; } diff --git a/sdk/src/step-fields.ts b/sdk/src/step-fields.ts index 69045a99a..60490fe98 100644 --- a/sdk/src/step-fields.ts +++ b/sdk/src/step-fields.ts @@ -22,7 +22,6 @@ export const STEP_COMMON_FIELDS = [ 'dependsOn', 'verification', 'maxIterations', - 'timeoutMs', ] as const; /** @@ -31,7 +30,7 @@ export const STEP_COMMON_FIELDS = [ * per-verb boundary through a second, drifting allowlist. */ export const STEP_FIELDS_BY_TYPE = { - deterministic: ['command'], + deterministic: ['command', 'timeoutMs'], llm: ['prompt', 'model', 'cli', 'output'], agent: ['instruction', 'agent', 'cli', 'model', 'surfaces', 'recoveryMode', 'permissions', 'output'], } as const satisfies Record; diff --git a/sdk/src/validate.ts b/sdk/src/validate.ts index 9358c9dcd..ec0b75b59 100644 --- a/sdk/src/validate.ts +++ b/sdk/src/validate.ts @@ -257,16 +257,6 @@ class Validator { this.fail(`${at}.maxIterations: expected a positive integer`); } - if (st['timeoutMs'] !== undefined && !isPosInt(st['timeoutMs'])) { - this.fail(`${at}.timeoutMs: expected a positive integer`); - } - if (st['timeoutMs'] !== undefined && type !== 'deterministic') { - // The v0.1.0 spec dialect carries timeout_ms on deterministic steps - // only; llm/agent timeouts land with worker dispatch. Fail closed - // rather than silently drop the field. - this.fail(`${at}.timeoutMs: only deterministic steps carry a timeout in spec v0.1.0`); - } - if (type === 'deterministic') { this.validateDeterministic(st as unknown as DeterministicStepSpec, at); } else if (type === 'llm') { @@ -311,6 +301,9 @@ class Validator { if (!isNonEmptyString(st.command)) { this.fail(`${at}.command: expected a non-empty string`); } + if (st.timeoutMs !== undefined && !isPosInt(st.timeoutMs)) { + this.fail(`${at}.timeoutMs: expected a positive integer`); + } } private validateLlm(st: LlmStepSpec, at: string): void { diff --git a/sdk/tests/dependency-validation.test.ts b/sdk/tests/dependency-validation.test.ts new file mode 100644 index 000000000..c560abf3a --- /dev/null +++ b/sdk/tests/dependency-validation.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from 'vitest'; +import { CompileError, toKernelSpec } from '../src/compile.js'; +import { preflight, type PreflightProbes } from '../src/preflight.js'; +import type { FlowSpec } from '../src/spec.js'; +import { validateSpec } from '../src/validate.js'; + +const CHAIN_LENGTH = 10_000; + +const probes: PreflightProbes = { + cli: () => ({ exists: true, authenticated: true }), + executor: () => true, + command: () => true, +}; + +function reverseDependencyChain(length: number): FlowSpec { + return { + version: '0.1.0', + name: `reverse-chain-${length}`, + steps: Array.from({ length }, (_, index) => ({ + id: `s${index}`, + type: 'deterministic' as const, + command: 'true', + ...(index + 1 < length ? { dependsOn: [`s${index + 1}`] } : {}), + })), + }; +} + +describe('dependency validation', () => { + it('accepts a valid 10,000-step reverse chain through every direct public boundary', () => { + const flow = reverseDependencyChain(CHAIN_LENGTH); + + expect(validateSpec(flow)).toEqual({ ok: true, errors: [] }); + const result = preflight(flow, { probes }); + expect(result.ok).toBe(true); + expect(result.resolutions).toEqual([]); + expect(result.diagnostics).toHaveLength(CHAIN_LENGTH); + expect(result.diagnostics.every((diagnostic) => diagnostic.severity === 'warning')).toBe(true); + expect(toKernelSpec(flow).steps).toHaveLength(CHAIN_LENGTH); + }); + + it('still rejects a dependency cycle fail-closed through every direct public boundary', () => { + const flow: FlowSpec = { + version: '0.1.0', + name: 'cycle', + steps: [ + { id: 'a', type: 'deterministic', command: 'true', dependsOn: ['b'] }, + { id: 'b', type: 'deterministic', command: 'true', dependsOn: ['c'] }, + { id: 'c', type: 'deterministic', command: 'true', dependsOn: ['a'] }, + ], + }; + const expected = 'spec.steps: dependency cycle detected at "a" (path: a -> b -> c -> a)'; + + expect(validateSpec(flow)).toEqual({ ok: false, errors: [expected] }); + expect(preflight(flow, { probes })).toEqual({ + ok: false, + resolutions: [], + diagnostics: [{ + severity: 'refusal', + kind: 'invalid_spec', + message: expect.stringContaining(expected), + errors: [expected], + }], + }); + expect(() => toKernelSpec(flow)).toThrow(CompileError); + expect(() => toKernelSpec(flow)).toThrow(expected); + }); +}); diff --git a/sdk/tests/validate.test.ts b/sdk/tests/validate.test.ts index 35e1d91b6..070093b84 100644 --- a/sdk/tests/validate.test.ts +++ b/sdk/tests/validate.test.ts @@ -89,7 +89,7 @@ describe('validate: rejects malformed specs', () => { it('rejects a timeout on a non-deterministic step (no dialect surface in v0.1.0)', () => { const r = validateSpec({ version: '0.1.0', name: 'x', steps: [{ id: 'a', type: 'llm', prompt: 'p', timeoutMs: 1000 }] }); expect(r.ok).toBe(false); - expect(r.errors.join(' ')).toContain('deterministic steps carry a timeout'); + expect(r.errors.join(' ')).toContain('unknown key "timeoutMs"'); }); it('rejects an output_contains gate with no value', () => { diff --git a/sdk/tests/verb-field-lint.test.ts b/sdk/tests/verb-field-lint.test.ts index 14ff48169..7275217f6 100644 --- a/sdk/tests/verb-field-lint.test.ts +++ b/sdk/tests/verb-field-lint.test.ts @@ -19,6 +19,7 @@ import type { FlowSpec, StepType } from '../src/spec.js'; import { AGENT_DECLARATION_FIELDS, FLOW_FIELDS, + STEP_COMMON_FIELDS, STEP_FIELDS_BY_TYPE, } from '../src/step-fields.js'; import { validateSpec } from '../src/validate.js'; @@ -64,6 +65,7 @@ const VALID_STEP_BY_TYPE: Record> = { const VERB_FIELD_VALUES: Record = { agent: 'reviewer', command: 'printf foreign', + timeoutMs: 1_000, prompt: 'foreign prompt', model: 'foreign-model', cli: 'foreign-cli', @@ -163,14 +165,24 @@ describe('closed per-verb step fields', () => { 'version', 'name', 'description', 'cli', 'agents', 'triggers', 'steps', 'budget', ]); expect(AGENT_DECLARATION_FIELDS).toEqual(['cli', 'model']); + // `timeoutMs` is deliberately absent: it is a deterministic-only authoring + // field, not a common one. Pinned so the move cannot be silently undone. + expect(STEP_COMMON_FIELDS).toEqual([ + 'id', + 'type', + 'dependsOn', + 'verification', + 'maxIterations', + ]); expect(STEP_FIELDS_BY_TYPE).toEqual({ - deterministic: ['command'], + deterministic: ['command', 'timeoutMs'], llm: ['prompt', 'model', 'cli', 'output'], agent: ['instruction', 'agent', 'cli', 'model', 'surfaces', 'recoveryMode', 'permissions', 'output'], }); expect(CROSS_VERB_STEP_FIELDS.map(({ label }) => label).sort()).toEqual([ 'agent foreign command', 'agent foreign prompt', + 'agent foreign timeoutMs', 'deterministic foreign agent', 'deterministic foreign cli', 'deterministic foreign instruction', @@ -186,6 +198,7 @@ describe('closed per-verb step fields', () => { 'llm foreign permissions', 'llm foreign recoveryMode', 'llm foreign surfaces', + 'llm foreign timeoutMs', ]); }); diff --git a/sdk/tsconfig.type-tests.json b/sdk/tsconfig.type-tests.json new file mode 100644 index 000000000..481c3e0da --- /dev/null +++ b/sdk/tsconfig.type-tests.json @@ -0,0 +1,9 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "rootDir": "." + }, + "include": ["src/**/*.ts", "type-tests/**/*.ts"], + "exclude": ["node_modules", "dist"] +} diff --git a/sdk/type-tests/step-fields.ts b/sdk/type-tests/step-fields.ts new file mode 100644 index 000000000..89c7a3c10 --- /dev/null +++ b/sdk/type-tests/step-fields.ts @@ -0,0 +1,40 @@ +import type { + AgentStepSpec, + BaseStepSpec, + DeterministicStepSpec, + LlmStepSpec, +} from '../src/index.js'; + +type Assert = T; +type Equal = + (() => Value extends Left ? 1 : 2) extends + (() => Value extends Right ? 1 : 2) ? true : false; + +type _TimeoutIsNotCommon = Assert, never>>; + +const deterministic = { + id: 'deterministic', + type: 'deterministic', + command: 'true', + timeoutMs: 1_000, +} satisfies DeterministicStepSpec; + +const llm = { + id: 'llm', + type: 'llm', + prompt: 'answer', + // @ts-expect-error timeoutMs is a deterministic-step-only authoring field. + timeoutMs: 1_000, +} satisfies LlmStepSpec; + +const agent = { + id: 'agent', + type: 'agent', + instruction: 'act', + // @ts-expect-error timeoutMs is a deterministic-step-only authoring field. + timeoutMs: 1_000, +} satisfies AgentStepSpec; + +void deterministic; +void llm; +void agent; From 56a09acd6e5e4ec5ad5d57b021269bfca310c36e Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 2 Sep 2026 21:18:20 +0200 Subject: [PATCH 2/5] fix(kernel): validate deep dependency graphs iteratively Session-Id: 01a062df-b92c-7a13-815f-147a82a4fc51 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- kernel/relayflowd-core/src/spec.rs | 30 ++--------- .../relayflowd-core/src/spec/dependencies.rs | 54 +++++++++++++++++++ kernel/relayflowd-core/src/spec/tests.rs | 41 +++++++++++++- sdk/package.json | 4 +- 4 files changed, 101 insertions(+), 28 deletions(-) create mode 100644 kernel/relayflowd-core/src/spec/dependencies.rs diff --git a/kernel/relayflowd-core/src/spec.rs b/kernel/relayflowd-core/src/spec.rs index c68b5aedc..97abb689d 100644 --- a/kernel/relayflowd-core/src/spec.rs +++ b/kernel/relayflowd-core/src/spec.rs @@ -17,6 +17,10 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use thiserror::Error; +mod dependencies; + +use dependencies::validate_dependency_cycles; + /// The spec schema version this kernel reads and writes (semver, RFC §7). pub const SPEC_VERSION: &str = "0.1.0"; @@ -156,11 +160,7 @@ impl RunSpec { .iter() .map(|step| (step.id.as_str(), step.depends_on.as_slice())) .collect::>(); - let mut visiting = BTreeSet::new(); - let mut visited = BTreeSet::new(); - for id in &ids { - visit(id, &dependencies, &mut visiting, &mut visited)?; - } + validate_dependency_cycles(&ids, &dependencies)?; Ok(()) } @@ -215,26 +215,6 @@ fn reject_unknown_step_fields(value: &Value) -> Result<(), SpecError> { Ok(()) } -fn visit<'a>( - id: &'a str, - dependencies: &BTreeMap<&'a str, &'a [String]>, - visiting: &mut BTreeSet<&'a str>, - visited: &mut BTreeSet<&'a str>, -) -> Result<(), SpecError> { - if visited.contains(id) { - return Ok(()); - } - if !visiting.insert(id) { - return Err(SpecError::DependencyCycle(id.to_owned())); - } - for dependency in dependencies.get(id).copied().unwrap_or_default() { - visit(dependency, dependencies, visiting, visited)?; - } - visiting.remove(id); - visited.insert(id); - Ok(()) -} - #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] pub struct StepSpec { pub id: String, diff --git a/kernel/relayflowd-core/src/spec/dependencies.rs b/kernel/relayflowd-core/src/spec/dependencies.rs new file mode 100644 index 000000000..b410eecd7 --- /dev/null +++ b/kernel/relayflowd-core/src/spec/dependencies.rs @@ -0,0 +1,54 @@ +use std::collections::{BTreeMap, BTreeSet}; + +use super::SpecError; + +/// Reject dependency cycles without consuming call-stack depth from the spec. +pub(super) fn validate_dependency_cycles<'a>( + ids: &'a BTreeSet, + dependencies: &BTreeMap<&'a str, &'a [String]>, +) -> Result<(), SpecError> { + struct Frame<'a> { + id: &'a str, + next_dependency: usize, + } + + let mut visiting = BTreeSet::new(); + let mut visited = BTreeSet::new(); + + for id in ids { + let id = id.as_str(); + if visited.contains(id) { + continue; + } + + visiting.insert(id); + let mut frames = vec![Frame { + id, + next_dependency: 0, + }]; + + while let Some(frame) = frames.last_mut() { + let step_dependencies = dependencies.get(frame.id).copied().unwrap_or_default(); + let Some(dependency) = step_dependencies.get(frame.next_dependency) else { + let completed = frames.pop().expect("the active frame exists"); + visiting.remove(completed.id); + visited.insert(completed.id); + continue; + }; + frame.next_dependency += 1; + let dependency = dependency.as_str(); + + if visited.contains(dependency) { + continue; + } + if !visiting.insert(dependency) { + return Err(SpecError::DependencyCycle(dependency.to_owned())); + } + frames.push(Frame { + id: dependency, + next_dependency: 0, + }); + } + } + Ok(()) +} diff --git a/kernel/relayflowd-core/src/spec/tests.rs b/kernel/relayflowd-core/src/spec/tests.rs index 8f02a74fa..6412b9c51 100644 --- a/kernel/relayflowd-core/src/spec/tests.rs +++ b/kernel/relayflowd-core/src/spec/tests.rs @@ -1,4 +1,4 @@ -use serde_json::json; +use serde_json::{Value, json}; use super::*; @@ -27,6 +27,45 @@ fn cycles_are_rejected() { )); } +const DEEP_DEPENDENCY_GRAPH_LENGTH: usize = 10_000; + +fn sdk_boundary_dependency_graph(cyclic: bool) -> RunSpec { + let steps = (0..DEEP_DEPENDENCY_GRAPH_LENGTH) + .map(|index| { + let mut step = json!({ + "id": format!("s{index}"), + "type": "deterministic", + "command": "true", + }); + if index + 1 < DEEP_DEPENDENCY_GRAPH_LENGTH { + step["depends_on"] = json!([format!("s{}", index + 1)]); + } else if cyclic { + step["depends_on"] = json!(["s0"]); + } + step + }) + .collect::>(); + + RunSpec::parse(&json!({ + "version": "0.1.0", + "steps": steps, + })) + .expect("the SDK-to-kernel boundary shape must parse") +} + +#[test] +fn sdk_boundary_accepts_a_valid_10_000_step_reverse_chain() { + assert_eq!(sdk_boundary_dependency_graph(false).validate(), Ok(())); +} + +#[test] +fn sdk_boundary_rejects_a_10_000_step_cycle_with_a_typed_error() { + assert_eq!( + sdk_boundary_dependency_graph(true).validate(), + Err(SpecError::DependencyCycle("s0".to_owned())) + ); +} + #[test] fn a_misspelled_verification_gate_key_is_a_parse_error_not_a_dropped_gate() { // The fail-open refutation case: "output_contain" (typo) must never diff --git a/sdk/package.json b/sdk/package.json index 05763cb7a..d905764ce 100644 --- a/sdk/package.json +++ b/sdk/package.json @@ -22,10 +22,10 @@ "build": "tsc && node scripts/make-cli-executable.mjs", "demo:hn": "npm run build && node dist/demo-hn-monitor.js", "prepare": "npm run build", - "typecheck": "tsc --noEmit", + "typecheck": "tsc --noEmit && tsc -p tsconfig.type-tests.json", "typecheck:tests": "tsc -p tsconfig.tests.json", "test:prep": "( cd ../kernel && sh ../ops/cargo.sh build ) && ( [ ! -d ../testdata/preflight ] || find ../testdata/preflight -name '*-cli' -type f -exec chmod +x {} + )", - "test": "npm run test:prep && npm run build && npm run typecheck:tests && vitest run", + "test": "npm run test:prep && npm run typecheck && npm run build && npm run typecheck:tests && vitest run", "test:watch": "vitest" }, "license": "UNLICENSED", From 6fd57b3884a4ba625cdac6b5a63c0ed3c5626d86 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Wed, 2 Sep 2026 22:14:29 +0200 Subject: [PATCH 3/5] fix(sdk): bound dependency cycle diagnostics Session-Id: 01a062df-b92c-7a13-815f-147a82a4fc51 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- sdk/src/step-dependencies.ts | 23 +++++++++- sdk/tests/dependency-validation.test.ts | 60 +++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/sdk/src/step-dependencies.ts b/sdk/src/step-dependencies.ts index 2623b78a1..e96e6cd07 100644 --- a/sdk/src/step-dependencies.ts +++ b/sdk/src/step-dependencies.ts @@ -1,4 +1,6 @@ /** Return author-facing dependency errors without assuming parsed step shapes. */ +const MAX_REPORTED_CYCLE_PATH_IDS = 16; + export function stepDependencyErrors( steps: readonly unknown[], knownIds: ReadonlySet, @@ -58,8 +60,12 @@ export function stepDependencyErrors( const dependencyColor = color.get(dependency); if (dependencyColor === GRAY) { errors.push( - `spec.steps: dependency cycle detected at "${dependency}" (path: ${path.join(' -> ')} -> ${dependency})`, + `spec.steps: dependency cycle detected at "${dependency}" (path: ${formatCyclePath(path, dependency)})`, ); + // One deterministic back edge proves the graph is invalid. Continuing + // would report every remaining gray edge and amplify diagnostics + // cubically for dense graphs, unlike the kernel's first-cycle refusal. + return errors; } else if (dependencyColor === WHITE) { color.set(dependency, GRAY); path.push(dependency); @@ -70,6 +76,21 @@ export function stepDependencyErrors( return errors; } +function formatCyclePath(path: readonly string[], dependency: string): string { + const cycleStart = path.lastIndexOf(dependency); + const cycle = [...path.slice(cycleStart), dependency]; + if (cycle.length <= MAX_REPORTED_CYCLE_PATH_IDS) return cycle.join(' -> '); + + const headSize = MAX_REPORTED_CYCLE_PATH_IDS / 2; + const tailSize = MAX_REPORTED_CYCLE_PATH_IDS - headSize; + const omitted = cycle.length - MAX_REPORTED_CYCLE_PATH_IDS; + return [ + ...cycle.slice(0, headSize), + `... (${omitted} steps omitted) ...`, + ...cycle.slice(-tailSize), + ].join(' -> '); +} + function isObject(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } diff --git a/sdk/tests/dependency-validation.test.ts b/sdk/tests/dependency-validation.test.ts index c560abf3a..8dea5e92e 100644 --- a/sdk/tests/dependency-validation.test.ts +++ b/sdk/tests/dependency-validation.test.ts @@ -25,6 +25,30 @@ function reverseDependencyChain(length: number): FlowSpec { }; } +function reverseDependencyCycle(length: number): FlowSpec { + const flow = reverseDependencyChain(length); + const last = flow.steps[length - 1]; + if (last === undefined) throw new Error('cycle fixture requires at least one step'); + last.dependsOn = ['s0']; + return flow; +} + +function denseBackEdgeGraph(length: number): FlowSpec { + return { + version: '0.1.0', + name: `dense-back-edges-${length}`, + steps: Array.from({ length }, (_, index) => ({ + id: `s${index}`, + type: 'deterministic' as const, + command: 'true', + dependsOn: [ + ...(index + 1 < length ? [`s${index + 1}`] : []), + ...Array.from({ length: index }, (__, dependency) => `s${dependency}`), + ], + })), + }; +} + describe('dependency validation', () => { it('accepts a valid 10,000-step reverse chain through every direct public boundary', () => { const flow = reverseDependencyChain(CHAIN_LENGTH); @@ -64,4 +88,40 @@ describe('dependency validation', () => { expect(() => toKernelSpec(flow)).toThrow(CompileError); expect(() => toKernelSpec(flow)).toThrow(expected); }); + + it.each([50, 100, 150])( + 'reports only the first deterministic cycle for %i nodes with dense active-path back edges', + (length) => { + const flow = denseBackEdgeGraph(length); + const result = validateSpec(flow); + + expect(result.ok).toBe(false); + expect(result.errors).toHaveLength(1); + expect(result.errors[0]).toContain('dependency cycle detected at "s0"'); + + const preflightResult = preflight(flow, { probes }); + expect(preflightResult.ok).toBe(false); + expect(preflightResult.diagnostics).toHaveLength(1); + expect(preflightResult.diagnostics[0]?.errors).toHaveLength(1); + expect(() => toKernelSpec(flow)).toThrow(CompileError); + }, + ); + + it('bounds the author-facing path for a 10,000-step cycle', () => { + const flow = reverseDependencyCycle(CHAIN_LENGTH); + const expected = 'spec.steps: dependency cycle detected at "s0" (path: ' + + 's0 -> s1 -> s2 -> s3 -> s4 -> s5 -> s6 -> s7 -> ' + + '... (9985 steps omitted) ... -> ' + + 's9993 -> s9994 -> s9995 -> s9996 -> s9997 -> s9998 -> s9999 -> s0)'; + const result = validateSpec(flow); + + expect(result.ok).toBe(false); + expect(result.errors).toEqual([expected]); + expect(result.errors[0]?.length).toBeLessThan(1_024); + + const preflightResult = preflight(flow, { probes }); + expect(preflightResult.ok).toBe(false); + expect(preflightResult.diagnostics[0]?.errors).toEqual([expected]); + expect(() => toKernelSpec(flow)).toThrow(expected); + }); }); From adea919c3f28eb82008c0455ab087a2109d72013 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 13:17:19 +0200 Subject: [PATCH 4/5] test(sdk): bind the per-verb descriptor to the step interfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `STEP_FIELDS_BY_TYPE` is the only allowlist `validateSpec` consults, but it is a plain string table. `satisfies Record` checks its *shape* — three keys, each an array of strings — and nothing more. It does not check that the table agrees with `DeterministicStepSpec`, `LlmStepSpec` or `AgentStepSpec`. A field added to one of those interfaces but not to the table is therefore refused at runtime as an unknown key, with every compile gate green. That is not hypothetical. It happened twice in one day. #133 added `output` to the interfaces and to the then-current allowlist; when this branch moved the allowlist into `step-fields.ts` the new file merged without a conflict and without `output`, and only a test caught it. #136 then added `agent` to `AgentStepSpec` on the same descriptor. So `type-tests/step-fields.ts` now asserts the correspondence in both directions — every authoring field an interface declares is described, and nothing is described that the interface does not declare. Dropping `output`, `agent` or `timeoutMs` from the table is now a `tsc` error (TS2344) instead of a silent runtime refusal. This is why a type-test file earns its place here: it is the only gate that can see this class of drift, because the drift is between a type and a value and no runtime test exercises the pairing directly. Also asserts the llm/agent `output` sugar end to end. The three paths diverge — `validateSpec` reads the in-memory object, `compileYaml` goes through a YAML round-trip that silently drops an `undefined` value, and `flows check` adds preflight and a process exit code — so each is asserted separately rather than assuming the first covers the others. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- sdk/tests/verb-field-lint.test.ts | 66 ++++++++++++++++++++++++++++++- sdk/type-tests/step-fields.ts | 33 ++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/sdk/tests/verb-field-lint.test.ts b/sdk/tests/verb-field-lint.test.ts index 7275217f6..470e88e90 100644 --- a/sdk/tests/verb-field-lint.test.ts +++ b/sdk/tests/verb-field-lint.test.ts @@ -1,6 +1,7 @@ import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { stringify as stringifyYaml } from 'yaml'; import { afterEach, describe, expect, it } from 'vitest'; import { @@ -35,6 +36,10 @@ interface InvalidFieldCase { suggestion?: string; } +const AUTHENTICATED_CLI = join( + dirname(fileURLToPath(import.meta.url)), '..', '..', 'testdata', 'preflight', 'authenticated-cli', +); + const TYPO_STEP_FIELDS = [ { label: 'deterministic typo', @@ -92,6 +97,7 @@ function foreignFieldValue(field: string): unknown { } const ALL_VERB_FIELDS = [...new Set(Object.values(STEP_FIELDS_BY_TYPE).flat())]; + const CROSS_VERB_STEP_FIELDS: InvalidFieldCase[] = ( Object.entries(STEP_FIELDS_BY_TYPE) as Array<[StepType, readonly string[]]> ).flatMap(([type, allowed]) => ALL_VERB_FIELDS @@ -393,4 +399,62 @@ describe('closed per-verb step fields', () => { { stepId: 'act', cli: 'agent-cli', source: 'step', model: 'project-model' }, ]); }); + + // The per-verb descriptor is the *only* allowlist the validator consults, and + // it is a plain string table that no type checks against LlmStepSpec / + // AgentStepSpec. A verb field added to the spec interfaces elsewhere — main's + // `output` sugar (#133) is the live example — is therefore refused as an + // unknown key until it is listed here, and nothing but this test says so. + // The three paths are asserted separately because they diverge: validateSpec + // reads the in-memory object, compileYaml goes through a YAML round-trip, and + // `flows check` adds preflight and the process exit code. + describe('carries the llm/agent `output` sugar through every path', () => { + const outputSchema = { + type: 'object', + required: ['answer'], + properties: { answer: { type: 'string' } }, + }; + const acceptedStep = (type: 'llm' | 'agent'): Record => ({ + ...VALID_STEP_BY_TYPE[type], + cli: AUTHENTICATED_CLI, + output: outputSchema, + }); + + it.each(['llm', 'agent'] as const)('validateSpec accepts output on %s', (type) => { + expect(validateSpec(specWith(acceptedStep(type)))).toEqual({ ok: true, errors: [] }); + }); + + it.each(['llm', 'agent'] as const)( + 'compileYaml accepts output on %s and lowers it to the json_schema gate', + (type) => { + const yaml = stringifyYaml(specWith(acceptedStep(type))); + // Guard the YAML half explicitly: an undefined value would vanish here + // and the assertion below would pass against a spec with no `output`. + expect(yaml).toContain('output:'); + const compiled = compileYaml(yaml); + expect(compiled.steps[0]).not.toHaveProperty('output'); + expect(compiled.steps[0]?.verification).toEqual({ + type: 'json_schema', + schema: outputSchema, + }); + }, + ); + + it.each(['llm', 'agent'] as const)('flows check accepts output on %s', async (type) => { + const directory = mkdtempSync(join(tmpdir(), 'flows-output-accepted-')); + temporaryDirectories.push(directory); + writeFileSync(join(directory, 'flows.json'), JSON.stringify({ executors: [] })); + const path = join(directory, 'typed.flow.yaml'); + writeFileSync(path, stringifyYaml(specWith(acceptedStep(type)))); + const stderr: string[] = []; + + const exitCode = await runCli(['check', path], { + stdout: () => {}, + stderr: (line) => stderr.push(line), + }); + + expect(stderr.join('\n')).not.toContain('unknown key "output"'); + expect(exitCode).toBe(0); + }); + }); }); diff --git a/sdk/type-tests/step-fields.ts b/sdk/type-tests/step-fields.ts index 89c7a3c10..f3588bcf6 100644 --- a/sdk/type-tests/step-fields.ts +++ b/sdk/type-tests/step-fields.ts @@ -3,7 +3,9 @@ import type { BaseStepSpec, DeterministicStepSpec, LlmStepSpec, + StepType, } from '../src/index.js'; +import { STEP_COMMON_FIELDS, STEP_FIELDS_BY_TYPE } from '../src/step-fields.js'; type Assert = T; type Equal = @@ -38,3 +40,34 @@ const agent = { void deterministic; void llm; void agent; + +// --- the descriptor must not drift from the spec interfaces ----------------- +// +// `STEP_FIELDS_BY_TYPE` is the only allowlist `validateSpec` consults, but it +// is a plain string table: `satisfies Record` +// checks the *shape*, not that it agrees with LlmStepSpec / AgentStepSpec. +// A field added to an interface but not to the table is therefore refused at +// runtime as an unknown key, with nothing failing at compile time — exactly +// how main's `output` sugar was nearly dropped when this branch moved the +// allowlist out of validate.ts. These assertions bind the two together. + +type CommonField = typeof STEP_COMMON_FIELDS[number] | 'type'; +/** Authoring fields an interface declares, minus the ones every verb shares. */ +type VerbFields = Exclude; +/** Fields the descriptor lists for a verb. */ +type DescribedFields = typeof STEP_FIELDS_BY_TYPE[T][number]; + +type _DeterministicIsDescribed = + Assert, DescribedFields<'deterministic'>>, never>>; +type _LlmIsDescribed = + Assert, DescribedFields<'llm'>>, never>>; +type _AgentIsDescribed = + Assert, DescribedFields<'agent'>>, never>>; + +// ...and nothing is described that the interface does not declare. +type _DeterministicDescribesNothingExtra = + Assert, VerbFields>, never>>; +type _LlmDescribesNothingExtra = + Assert, VerbFields>, never>>; +type _AgentDescribesNothingExtra = + Assert, VerbFields>, never>>; From 4d85603b9972ae129cc043ebc613141a2bad03d4 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Thu, 3 Sep 2026 13:48:23 +0200 Subject: [PATCH 5/5] =?UTF-8?q?docs(reviews):=20PR=20#138=20rebase=20onto?= =?UTF-8?q?=20990093b=20=E2=80=94=20evidence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the previous rebase report. #136 landed between the two rebases and carries much of what #138 built, so two of the branch's five commits were dropped as empty and the diff against main fell from 8,463 deletions to 68. Records: which commits survived and why; the resolution of every conflict and every audited auto-merge, including the duplicate `foreignFieldValue` the commit-6 auto-merge introduced; three independent proofs that #136 was not reverted; the re-derived hazard enumeration now that the shared files exist on both sides; three-path proofs for `output` and `timeoutMs` across all three verbs; every gate with literal output and per-file count accounting against a measured 990093b baseline; and the mutation verification of the type binding, including the harness fault that first reported two false "guard missed it" results. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- ops/reviews/20260903-pr138-rebase-0903.md | 553 ++++++++++++++++++++++ 1 file changed, 553 insertions(+) create mode 100644 ops/reviews/20260903-pr138-rebase-0903.md diff --git a/ops/reviews/20260903-pr138-rebase-0903.md b/ops/reviews/20260903-pr138-rebase-0903.md new file mode 100644 index 000000000..695f9b8f1 --- /dev/null +++ b/ops/reviews/20260903-pr138-rebase-0903.md @@ -0,0 +1,553 @@ +# PR #138 — rebase onto `origin/main` @ `990093b` + +Branch `rebase/pr138-0903` → pushes to `feat/v2-field-lint`. + +| | | +|---|---| +| Signoff SHA (`REVIEW_PASSED`) | `e164e4239b0aa9e8b2dd58ed126263d206ad29d0` | +| First rebase (onto `3da71e2`) | `441c679e2e937b8f9d7e5b69935539651a0c1a2c` | +| Rebased onto | `990093b86f34b09e757fccf798276facda9e20bb` (#136) | +| Merge base | `3da71e2a6ccbeaf11a576b42ab0aa2c52031e93b` | +| Post-rebase SHA | see §8 | + +This is the second rebase of this branch today. The first landed on `3da71e2`; +`origin/main` then moved to `990093b` when #136 merged. #136 carries a large +part of what #138 built, so this rebase is mostly a subtraction: two of the +branch's commits are now redundant and were dropped, and what remains is the +work #136 does *not* contain. + +**No product behaviour changed relative to the union of the two sides.** + +--- + +## 1. What #136 already landed, and what is left of #138 + +#136 independently introduced `sdk/src/step-fields.ts`, +`sdk/src/step-dependencies.ts`, `sdk/src/unknown-keys.ts` and +`sdk/tests/verb-field-lint.test.ts` — the same extraction #138 performed. It +also added named agent declarations (`FlowSpec.agents`, `NamedAgentSpec`, the +`agent` step field), fail-closed model checks, the CLI adapters and the wrapper +session runtime. + +Replaying #138's five commits onto `990093b`: + +| # | commit | outcome | +|---|---|---| +| 1 | `fix(sdk): refuse fields outside step verb schemas` | **dropped — empty.** Every hunk resolved to content byte-identical to `990093b` | +| 2 | `fix(sdk): harden malformed step validation` | **dropped — empty.** Same | +| 3 | `fix(sdk): close timeout and dependency boundaries` | kept — the `timeoutMs` boundary | +| 4 | `fix(kernel): validate deep dependency graphs iteratively` | kept, applied clean | +| 5 | `fix(sdk): bound dependency cycle diagnostics` | kept, applied clean | +| 6 | `fix(sdk): carry main's output …` → **reworded** `test(sdk): bind the per-verb descriptor to the step interfaces` | kept; see §5 | + +Commits 1 and 2 were confirmed empty by resolving every conflict and then +checking `git diff --cached origin/main` was empty — not by assuming. + +The surviving delta against `990093b`: + +``` + kernel/relayflowd-core/src/spec.rs | 30 +- + kernel/relayflowd-core/src/spec/dependencies.rs | 54 + + kernel/relayflowd-core/src/spec/tests.rs | 41 +- + sdk/package.json | 4 +- + sdk/src/compile.ts | 19 +- + sdk/src/spec.ts | 3 +- + sdk/src/step-dependencies.ts | 61 +- + sdk/src/step-fields.ts | 3 +- + sdk/src/validate.ts | 13 +- + sdk/tests/dependency-validation.test.ts | 127 + + sdk/tests/validate.test.ts | 2 +- + sdk/tests/verb-field-lint.test.ts | 81 +- + sdk/tsconfig.type-tests.json | 9 + + sdk/type-tests/step-fields.ts | 73 + + (+ this report) +``` + +--- + +## 2. The revert hazard, and the proof it did not happen + +The branch predates the #136 merge, so `git diff origin/main ` before +this rebase read **62 files, 1378 insertions, 8463 deletions** — those +deletions being #136's work. A resolution that took the branch's side on any +shared file would have silently reverted it. + +After the rebase the same diff is **15 files, 1072 insertions, 68 deletions**, +and every one of those 68 deletions is an intended #138 removal (`timeoutMs` +from `BaseStepSpec` and `STEP_COMMON_FIELDS`, `requireNoTimeout`, the recursive +DFS replaced by the iterative walk, the kernel's recursive spec validation). + +Three independent checks: + +**(a) No file deleted.** + +``` +$ git diff --diff-filter=D --name-only origin/main HEAD +(empty) +``` + +**(b) Every #136 artifact byte-identical.** Blob hashes for the 40 files #136 +added or modified outside the shared set — the wrapper runtime and session, the +CLI adapters, `model-name.ts`, `unknown-keys.ts`, `worker-cli.ts`, their tests, +`wrapper-session.mjs`, every `testdata/preflight/` fixture, `testdata/flows.json`, +`cli.ts`, `worker.ts`, `preflight.ts`, `cli/check.ts` — captured from +`origin/main` before the rebase and compared after: + +``` +$ diff pr136-blobs.txt head-blobs.txt +IDENTICAL — all 40 #136 artifacts unchanged (blob hashes match) +``` + +**(c) Presence check** on the files the brief named explicitly: + +``` +sdk/src/wrapper-runtime.ts PRESENT +sdk/src/wrapper-session.ts PRESENT +sdk/src/cli-adapter.ts PRESENT +sdk/src/model-name.ts PRESENT +sdk/src/unknown-keys.ts PRESENT +sdk/src/worker-cli.ts PRESENT +testdata/preflight/wrapper-session.mjs PRESENT +sdk/tests/cli-adapter.test.ts PRESENT +sdk/tests/model-selection.test.ts PRESENT +sdk/tests/real-cli-adapters.test.ts PRESENT +sdk/tests/worker-cli.test.ts PRESENT +``` + +**(d) #136's tests pass** — `worker-cli.test.ts` 12/12, `cli-adapter.test.ts` +3/3, `model-selection.test.ts` 10/10, `cli.test.ts` 63/63, `preflight.test.ts` +24/24. `real-cli-adapters.test.ts` self-skips its 3 tests (it needs real vendor +CLIs); it skips identically on `990093b`. See §6. + +--- + +## 3. Resolution of each conflict + +Ten files were in play. Rebase raised conflicts in three of them across three +commits; the rest auto-merged and were audited as if they had conflicted, per +the generalised rule. + +### 3.1 `sdk/tests/verb-field-lint.test.ts` — add/add, commits 1, 2, 3, 6 + +Both sides created this file independently. Resolved to **main's version** in +commits 1 and 2 (their content is wholly #136's), then folded #138's distinct +additions into main's file in commits 3 and 6: + +- main's `FLOW_FIELDS` and `AGENT_DECLARATION_FIELDS` pins — **kept**; +- main's `agent: 'reviewer'` sample value, `modelAvailable` probe, `models:` + preflight option and named-agent ladder step — **kept**; +- **added** the `STEP_COMMON_FIELDS` pin, because `timeoutMs` leaving that list + is exactly what #138 changes and nothing else pins it; +- descriptor pin reconciled to the union; +- generated cross-verb label list reconciled to the union (19 entries — main's + 17 plus `llm foreign timeoutMs` and `agent foreign timeoutMs`, which exist + only once `timeoutMs` becomes a verb field); +- **added** the six `output` acceptance cases. + +Main's `foreignFieldValue` was kept over the branch's. Both sides independently +closed the same generated-case hole, but main's guard is strictly better: it +tests `Object.hasOwn`, so a field whose legitimate sample value *is* `undefined` +is not mistaken for a missing one. The branch's variant, and the redundant +`ALL_VERB_FIELDS.filter(...)` assertion that went with it, were dropped. + +### 3.2 `sdk/src/step-fields.ts` — add/add, commits 2, 3, 6 + +Resolved to main's file in commit 2. In commit 3 the marked conflict was the +descriptor; `STEP_COMMON_FIELDS` had already auto-merged with the branch's +`timeoutMs` removal. Resolved to the union: + +```ts + deterministic: ['command', 'timeoutMs'], // branch: timeoutMs is verb-specific + llm: ['prompt', 'model', 'cli', 'output'], // main (#133) + agent: ['instruction', 'agent', 'cli', 'model', 'surfaces', 'recoveryMode', 'permissions', 'output'], + // ^ main (#136) +``` + +This is the previous rebase's trap with the sides swapped: last time the +branch-only file silently lacked main's `output`; this time main's `output` and +`agent` are on the marked side and the branch's `timeoutMs` on the other. +Neither side is takeable wholesale. + +### 3.3 `sdk/package.json` — carried forward + +Not conflicting this time (#136 did not touch it), but the union resolved in +the first rebase had to survive, because CI invokes `npm run typecheck:tests` +**by name**: + +```json +"typecheck": "tsc --noEmit && tsc -p tsconfig.type-tests.json", +"typecheck:tests": "tsc -p tsconfig.tests.json", +"test": "npm run test:prep && npm run typecheck && npm run build && npm run typecheck:tests && vitest run", +``` + +Verified present at HEAD, and CI's own step run below (§6.4). + +### 3.4 Auto-merged, audited + +| file | branch delta preserved | main content preserved | +|---|---|---| +| `sdk/src/compile.ts` | `timeoutMs` out of `base` into the deterministic arm; `requireNoTimeout` deleted | `typedOutputVerification`, the `output → json_schema` branch, `toKernelSpec` validation, #136's adapter plumbing | +| `sdk/src/spec.ts` | `timeoutMs` off `BaseStepSpec`, onto `DeterministicStepSpec` | `output` on llm/agent; `agent?`, `NamedAgentSpec`, `FlowSpec.agents` | +| `sdk/src/validate.ts` | `timeoutMs` checks moved into `validateDeterministic` | `validateAgents`, `modelNameError`, `unknownKeyErrors`, `FLOW_FIELDS` | +| `sdk/src/step-dependencies.ts` | recursive DFS → iterative walk, bounded cycle path | main's file was byte-identical to the branch's *base*, so nothing of main's was displaced | +| `sdk/tests/validate.test.ts` | timeout assertion now expects `unknown key "timeoutMs"` | — | +| `sdk/src/failure-kinds.ts`, `sdk/src/index.ts`, `sdk/src/preflight.ts`, `sdk/tests/preflight.test.ts` | (none — subsumed) | all of #136's | + +**One real defect was caught in this audit.** The commit-6 auto-merge left +`foreignFieldValue` **defined twice** — main's and the branch's — which is a +redeclaration. Found by reading the staged diff before continuing the rebase, +removed there. `tsc` would also have caught it, but it is exactly the shape +that survives a "the suite is green" review of a large diff. + +--- + +## 4. Hazard enumeration against the new base + +The trap changed shape, as anticipated: `step-fields.ts`, `step-dependencies.ts` +and `verb-field-lint.test.ts` are no longer branch-only files but two +independently evolved versions of the same file. Re-derived from +`git diff --name-only 3da71e2 origin/main` against `git diff --name-only 3da71e2 `. + +### 4.1 Files on both sides (the new hazard class) + +| file | reconciled how | verified | +|---|---|---| +| `sdk/src/step-fields.ts` | union — §3.2 | descriptor pin test; type binding, §5 | +| `sdk/src/step-dependencies.ts` | branch's iterative+bounded version; main's was the branch's own base | `dependency-validation.test.ts` 6/6; kernel parity tests | +| `sdk/tests/verb-field-lint.test.ts` | main's file + #138's additions — §3.1 | 78/78 | +| `sdk/src/validate.ts` | main's + the `timeoutMs` move | §7 recheck, byte-compared vs main | +| `sdk/src/compile.ts`, `sdk/src/spec.ts` | main's + the `timeoutMs` move | §6, §7 | +| `sdk/src/failure-kinds.ts`, `sdk/src/index.ts`, `sdk/src/preflight.ts`, `sdk/tests/preflight.test.ts` | main's (branch subsumed) | byte-identical to main | + +### 4.2 Branch-only, does main touch the concern? + +| branch-only | concern | main's counterpart | verdict | +|---|---|---|---| +| `kernel/relayflowd-core/src/spec/dependencies.rs` (+ `spec.rs`, `spec/tests.rs`) | kernel dependency-graph validation | #136 touches no kernel file at all (`git diff 3da71e2 origin/main -- kernel/` is empty) | clean | +| `sdk/tests/dependency-validation.test.ts` | dependency diagnostics | none | clean | +| `sdk/tsconfig.type-tests.json`, `sdk/type-tests/step-fields.ts` | type-level authoring gate | `sdk/tsconfig.tests.json` (from #133) — same concern, different file, both wired in `package.json` | both kept and both run, §6 | +| `sdk/package.json` | gate wiring | CI calls `typecheck:tests` by name | §3.3, §6.4 | + +### 4.3 Main-only, does the branch touch the concern? + +| main-only | concern | verdict | +|---|---|---| +| `sdk/src/unknown-keys.ts` | `nearestKey`/levenshtein extracted out of `validate.ts` — the branch also rewrote `validate.ts` | clean: the branch's `validate.ts` delta is confined to `timeoutMs`; suggestion behaviour re-verified unchanged, §7 | +| `sdk/src/model-name.ts`, `sdk/src/cli-adapter.ts`, `sdk/src/worker-cli.ts`, `sdk/src/wrapper-runtime.ts`, `sdk/src/wrapper-session.ts`, `sdk/src/worker.ts`, `sdk/src/cli.ts`, `sdk/src/cli/check.ts` | agent CLI/model declaration and dispatch | no branch concern; byte-identical, §2(b) | +| `testdata/preflight/*` (11 fixtures), `testdata/flows.json`, `testdata/preflight/wrapper-session.mjs` | preflight fixtures the branch's validator gates | byte-identical, and row-for-row `flows check` parity, §6.5 | +| `docs/SURFACE.md` | documents `agents:` and the model contract | branch adds no authoring field; `output` example still validates, §6.5 | +| `ops/reviews/*.md` (14) | documentation | clean | + +### 4.4 Also checked + +`sdk/tests/live-kernel.test.ts` — modified by #136, untouched by the branch, +byte-identical at HEAD. `examples/**` and `regressions/**` — untouched by +either side since the previous rebase. + +--- + +## 5. The type-level binding (kept, per the lead's decision) + +`STEP_FIELDS_BY_TYPE` is the only allowlist `validateSpec` consults, and +`satisfies Record` checks its shape but not its +agreement with the spec interfaces. That is precisely why the `output` drift +was silent in the first rebase, and #136 has since added `agent` to +`AgentStepSpec` on the same descriptor. + +`sdk/type-tests/step-fields.ts` now asserts the correspondence in both +directions. Mutation-verified — each mutation asserted to have actually changed +the file before running the gate, because a mutation that does not mutate +proves nothing: + +``` +MUTATION llm-output ('cli', 'output'] -> 'cli']) + mutation applied, file changed + RESULT: tsc FAILED as required: type-tests/step-fields.ts(63,10): error TS2344: Type 'false' does not satisfy the constraint 'true'. +MUTATION agent-output ('permissions', 'output'] -> 'permissions']) + mutation applied, file changed + RESULT: tsc FAILED as required: type-tests/step-fields.ts(65,10): error TS2344: Type 'false' does not satisfy the constraint 'true'. +MUTATION det-timeoutMs ('command', 'timeoutMs'] -> 'command']) + mutation applied, file changed + RESULT: tsc FAILED as required: type-tests/step-fields.ts(61,10): error TS2344: Type 'false' does not satisfy the constraint 'true'. +MUTATION agent-agent ('instruction', 'agent', -> 'instruction',) + mutation applied, file changed + RESULT: tsc FAILED as required: type-tests/step-fields.ts(65,10): error TS2344: Type 'false' does not satisfy the constraint 'true'. + +=== RESTORED byte-for-byte === +identical +tsc exit=0 +``` + +The guard covers #136's `agent` field as well as #133's `output`, which is the +point: it is not a fix for one field, it is a fix for the class. + +An earlier run of this harness reported two of these four as "guard missed it". +That was a fault in the harness, not the guard — the patterns carried a trailing +comma that does not exist for the last element of an array, so the file was +never modified and `tsc` passed on unchanged input. Recorded because a +mutation harness that silently no-ops is the same failure mode as the generated +test cases in §3.1. + +--- + +## 6. Gates + +`npm ci` and `npm run *` hang on this machine; binaries were invoked directly. +The kernel was rebuilt first (`test:prep` equivalent), which matters — see §6.3. + +### 6.1 Typecheck + +``` +$ cd sdk && ./node_modules/.bin/tsc --noEmit +exit=0 +$ cd sdk && ./node_modules/.bin/tsc -p tsconfig.tests.json +exit=0 +$ cd sdk && ./node_modules/.bin/tsc -p tsconfig.type-tests.json +exit=0 +``` + +**PASS ×3.** + +### 6.2 SDK suite + +``` +$ cd sdk && ./node_modules/.bin/vitest run + ✓ tests/validate.test.ts (36 tests) ✓ tests/spec-parity.test.ts (15 tests) + ✓ tests/preflight.test.ts (24 tests) ↓ tests/real-cli-adapters.test.ts (3 skipped) + ✓ tests/backlog-picker.test.ts (14 tests) ✓ tests/parse-json-output.test.ts (7 tests) + ✓ tests/journal-client.test.ts (14 tests) ✓ tests/cli-adapter.test.ts (3 tests) + ✓ tests/cli-hn-monitor.test.ts (16 tests) ✓ tests/bin.test.ts (7 tests) + ✓ tests/verb-field-lint.test.ts (78 tests) ✓ tests/cli.test.ts (63 tests) + ✓ tests/typed-output.test.ts (14 tests) ✓ tests/worker-cli.test.ts (12 tests) + ✓ tests/model-selection.test.ts (10 tests) ✓ tests/live-kernel.test.ts (21 tests) + ✓ tests/dependency-validation.test.ts (6 tests) + ✓ tests/work-package-consumer.test.ts (13 tests) + ✓ tests/deterministic-llm.test.ts (5 tests) ✓ tests/hn-poller.test.ts (6 tests) + ✓ tests/dir-watcher-poller.test.ts (6 tests) ✓ tests/hello-deterministic.test.ts (5 tests) + ✓ tests/work-package-validator.test.ts (7 tests) + ✓ tests/backlog-picker-flow.test.ts (6 tests) + + Test Files 23 passed | 1 skipped (24) + Tests 388 passed | 3 skipped (391) +``` + +**PASS — 388 passed, 3 skipped, 0 failed.** + +#### Accounting against `990093b` + +Baseline measured by building and running `990093b` in a scratch worktree, not +quoted from anywhere: + +| | files | tests | +|---|---|---| +| `origin/main` `990093b` | 24 | 373 | +| rebased HEAD | 24 | **391** | + +Per-file, only non-zero deltas: + +| file | main | head | delta | why | +|---|---|---|---|---| +| `tests/dependency-validation.test.ts` | 0 | 6 | **+6** | new file — #138's dependency diagnostics | +| `tests/verb-field-lint.test.ts` | 66 | 78 | **+12** | see below | + +The **+12** is arithmetic, not a guess. `ALL_VERB_FIELDS` is derived from the +descriptor. On main it holds 10 fields (`timeoutMs` is a *common* field there, +so it is never a foreign one); on HEAD it holds 11, because #138 makes +`timeoutMs` verb-specific. That adds `llm foreign timeoutMs` and +`agent foreign timeoutMs` — 2 generated cases × the 3 `it.each` blocks that +consume `INVALID_STEP_FIELDS` = **+6** — plus the **+6** `output` acceptance +cases (2 verbs × 3 paths). Cross-checked by recomputing both totals from the +descriptor: main 17 cross-verb + 3 typo = 20 × 3 = 60, +4 malformed, +1 pin, ++1 ladder = 66 ✓; HEAD 19 + 3 = 22 × 3 = 66, +4, +1, +1, +6 = 78 ✓. + +**Nothing lost:** set difference on full test names shows every test present on +`990093b` is present at HEAD. + +### 6.3 Kernel + +``` +$ cd kernel && PATH="$HOME/.cargo/bin:$PATH" RUSTUP_TOOLCHAIN=stable sh ../ops/cargo.sh test --workspace + relayflowd lib 22 passed; 0 failed + relayflowd main 0 passed; 0 failed + crash_resume.rs 22 passed; 0 failed + event_wake.rs 1 passed; 0 failed + hn_monitor_integration 1 passed; 0 failed + subscription_liveness 3 passed; 0 failed + relayflowd_core lib 31 passed; 0 failed + spec_parity.rs 5 passed; 0 failed + relayflowd_journal lib 17 passed; 0 failed + Doc-tests ×3 0 passed; 0 failed +``` + +**PASS — 102, 0 failed.** `990093b` measures **100** (#136 touches no kernel +file). The +2 are the branch's own, by name: + +- `spec::tests::sdk_boundary_accepts_a_valid_10_000_step_reverse_chain` +- `spec::tests::sdk_boundary_rejects_a_10_000_step_cycle_with_a_typed_error` + +Set difference confirms nothing lost. + +### 6.4 CI's own gate + +`.github/workflows/cloud-runtime-artifact.yml` is unchanged by #136 and still +runs `npm run typecheck:tests` by name plus a four-file vitest subset. Run +directly, since `npm run` hangs here: + +``` +$ ./node_modules/.bin/tsc -p tsconfig.tests.json # == npm run typecheck:tests +exit=0 +$ ./node_modules/.bin/vitest run tests/typed-output.test.ts tests/validate.test.ts \ + tests/spec-parity.test.ts tests/deterministic-llm.test.ts + Test Files 4 passed (4) + Tests 70 passed (70) +``` + +### 6.5 Fixture parity — zero false positives + +Every `testdata/**/*.flow.yaml` through the built `flows check` on `990093b` +and on HEAD, capturing exit code, stdout and stderr, then diffed: + +``` +fixtures: 18 +=== ROW-FOR-ROW DIFF (990093b vs rebased HEAD) === +IDENTICAL — zero false positives +``` + +Including the fixtures that are meant to refuse (`cli-missing`, `cli-signal`, +`cli-unauthenticated`, `cli-unresolved`, `empty-path`, `no-executor`, +`shared-cli`) — same kinds, same messages. + +--- + +## 7. Behaviour proofs + +### 7.1 `output` and `timeoutMs`, per verb, per path + +`timeoutMs` is #138's actual product change now that #136 carries the rest, so +it is proved on the same three paths as `output`. Each positive case asserts +the field survived YAML serialization before compiling, so a dropped field +cannot masquerade as a pass. + +| field | `deterministic` | `llm` | `agent` | +|---|---|---|---| +| `output` | refused ×3 | accepted ×3, lowered to `json_schema` | accepted ×3, lowered to `json_schema` | +| `timeoutMs` | accepted ×3, emitted on the step | refused ×3 | refused ×3 | + +``` +output/llm validateSpec {"ok":true,"errors":[]} +output/llm yaml-has-field true +output/llm compileYaml.verification {"type":"json_schema","schema":{...}} +output/llm flows check exit=0 out=RESOLVED step "w" cli ".../authenticated-cli" from step | CHECK PASSED +output/agent validateSpec {"ok":true,"errors":[]} +output/agent yaml-has-field true +output/agent compileYaml.verification {"type":"json_schema","schema":{...}} +output/agent flows check exit=0 ... CHECK PASSED +output/deterministic validateSpec {"ok":false,"errors":["spec.steps[0]: unknown key \"output\" (expected one of id | type | dependsOn | verification | maxIterations | command | timeoutMs)"]} +output/deterministic compileYaml spec compile failed: | - spec.steps[0]: unknown key "output" (...) +output/deterministic flows check exit=2 err=REFUSED [invalid_spec] ... unknown key "output" ... + +timeoutMs/deterministic validateSpec {"ok":true,"errors":[]} +timeoutMs/deterministic yaml-has-field true +timeoutMs/deterministic compileYaml.step {"id":"w","type":"deterministic","maxIterations":1,"command":"true","verification":{"type":"exit_code"},"timeoutMs":1000} +timeoutMs/deterministic flows check exit=0 out=CHECK PASSED +timeoutMs/llm validateSpec {"ok":false,"errors":["spec.steps[0]: unknown key \"timeoutMs\" (expected one of id | type | dependsOn | verification | maxIterations | prompt | model | cli | output)"]} +timeoutMs/llm compileYaml spec compile failed: | - spec.steps[0]: unknown key "timeoutMs" (...) +timeoutMs/llm flows check exit=2 err=REFUSED [invalid_spec] ... unknown key "timeoutMs" ... +timeoutMs/agent validateSpec {"ok":false,"errors":["spec.steps[0]: unknown key \"timeoutMs\" (expected one of id | type | dependsOn | verification | maxIterations | instruction | agent | cli | model | surfaces | recoveryMode | permissions | output)"]} +timeoutMs/agent compileYaml spec compile failed: | - spec.steps[0]: unknown key "timeoutMs" (...) +timeoutMs/agent flows check exit=2 err=REFUSED [invalid_spec] ... unknown key "timeoutMs" ... +``` + +Note the `agent` verb's expected-key list contains `agent` — #136's field, +surviving. + +### 7.2 Prototype safety, duplicate YAML keys, nearest-key naming + +Re-run against the built `dist` on both `990093b` and HEAD. + +``` +A1 __proto__ step refused {"ok":false,"errors":["spec.steps[0]: unknown key \"__proto__\" (...)"]} +A2 __proto__ root {"ok":false,"errors":["spec: unknown key \"__proto__\" (expected one of version | name | description | cli | agents | triggers | steps | budget)"]} +A3 Object.prototype.polluted after validate undefined +A4 literal keys constructor / prototype / __proto__ / toString all refused +A5 Object.prototype.polluted at end undefined +A6 Array.prototype.polluted at end undefined +B1 yaml __proto__ refused CompileError: ... unknown key "__proto__" ... +B2 Object.prototype.polluted after yaml undefined +C1 duplicate yaml key refused YAMLParseError: Map keys must be unique at line 6, column 5 +C2 duplicate root key refused YAMLParseError: Map keys must be unique at line 2, column 1 +D deterministic unknown key "commnad" — did you mean "command"? +D llm unknown key "promt" — did you mean "prompt"? +D agent unknown key "instructon" — did you mean "instruction"? +D dependsOn unknown key "depends_on" — did you mean "dependsOn"? +D maxIterations unknown key "max_iterations" — did you mean "maxIterations"? +D output-on-llm-typo unknown key "ouput" — did you mean "output"? +``` + +`A2` shows `agents` in the root allowlist — #136's, preserved. + +**Every difference from `990093b` is the position of `timeoutMs` in the +"expected one of" list**, which is #138's change and nothing else: + +``` +< ... maxIterations | timeoutMs | command) (main: timeoutMs is common) +> ... maxIterations | command | timeoutMs) (HEAD: timeoutMs is deterministic-only) +``` + +Nearest-key suggestions, prototype safety and duplicate-key errors are +byte-identical. + +--- + +## 8. Summary + +| item | result | +|---|---| +| Rebase onto `990093b` | 4 commits (2 dropped as subsumed by #136) | +| #136 reverted? | **no** — 40 artifacts byte-identical, 0 files deleted, its tests pass | +| `tsc --noEmit` | PASS 0 | +| `tsc -p tsconfig.tests.json` | PASS 0 | +| `tsc -p tsconfig.type-tests.json` | PASS 0 | +| `vitest run` | PASS — 388 passed, 3 skipped, 0 failed (main 373 → 391, +18 accounted) | +| `cargo test --workspace` | PASS — 102, 0 failed (main 100 → 102, +2 accounted) | +| `live-kernel.test.ts` | **21/21 PASS** — ran for the first time in this work | +| `output` × 3 paths × 3 verbs | PASS | +| `timeoutMs` × 3 paths × 3 verbs | PASS | +| Type binding | kept; mutation-verified on 4 fields incl. #136's `agent` | +| Prototype / duplicate keys / nearest-key | unchanged from main | +| False positives | zero — 18/18 fixtures identical row-for-row | +| Tests lost from either parent | none (set difference on test names) | + +### Notes + +- **`live-kernel.test.ts` now runs.** #136 fixed the missing `node:fs` import + that made it fail to collect, and it passes 21/21 here. + + It is not reliably self-hosting on a shared machine, though, and this bit + twice during these gates. `locateRelayflowd` picks the newest `relayflowd` by + mtime across *every* `~/.relayflows-toolchain/target//` tree — not this + worktree's. There are 20+ such trees on this machine and other agents build + into them concurrently. A run that was green at 13:41 failed at 13:48 with + `unsupported_verb: run.cancel` and `run_terminal: ... cannot accept + mutations` because an unrelated worktree produced a newer binary at 13:44 + whose kernel predates #142. Nothing in this branch changed between those runs. + + Rebuilding this worktree's kernel is therefore not a dependable fix — it only + wins until the next neighbour builds. Pinning is: + + ``` + RELAYFLOWD_BIN=~/.relayflows-toolchain/target//debug/relayflowd \ + ./node_modules/.bin/vitest run + ``` + + All `live-kernel` numbers in this report are from a pinned run, and the suite + logs the binary it chose (`LIVE_KERNEL relayflowd=...`) so the choice is + checkable rather than assumed. Worth fixing upstream — mtime across foreign + target trees is not a correct selector — but that is not this PR's change to + make. +- `real-cli-adapters.test.ts` self-skips its 3 tests without real vendor CLIs + present; it skips identically on `990093b`. +- **#138 needs a fresh signoff at this head.** Its `REVIEW_PASSED` was at + `e164e42`, which is now two rebases and a substantially reduced diff away. +- GitHub CI runs only `linux-x64-artifact` and `packed-consumer`; the local + results above are the only real signal.