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/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. 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", 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..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, @@ -31,29 +33,64 @@ 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: ${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) { - 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; } +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/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..8dea5e92e --- /dev/null +++ b/sdk/tests/dependency-validation.test.ts @@ -0,0 +1,127 @@ +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}`] } : {}), + })), + }; +} + +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); + + 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); + }); + + 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); + }); +}); 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..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 { @@ -19,6 +20,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'; @@ -34,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', @@ -64,6 +70,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', @@ -90,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 @@ -163,14 +171,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 +204,7 @@ describe('closed per-verb step fields', () => { 'llm foreign permissions', 'llm foreign recoveryMode', 'llm foreign surfaces', + 'llm foreign timeoutMs', ]); }); @@ -380,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/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..f3588bcf6 --- /dev/null +++ b/sdk/type-tests/step-fields.ts @@ -0,0 +1,73 @@ +import type { + AgentStepSpec, + 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 = + (() => 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; + +// --- 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>>;