From ecfce54449454649e0a539135f0545c54ad2ae67 Mon Sep 17 00:00:00 2001 From: kjgbot Date: Fri, 4 Sep 2026 00:54:56 +0200 Subject: [PATCH] ci: run the kernel and full SDK suites The only workflow in the repo built the kernel but never tested it, and ran four of the SDK suite's 25 test files. Every kernel-side defect found on 2026-09-03 was invisible to CI by construction, including an exactly-once double-fire and two tests in the tree that encoded opposite contracts and both passed because neither ever ran. Add `cargo test --workspace` through ops/cargo.sh, which is the wrapper `npm test`'s test:prep already uses, so both steps share one toolchain home rather than populating two registries. Replace the four named vitest files with `npm test`, the repo's own entry point and a strict superset of what the step did before -- it builds sdk/dist, without which several files fail at collection, and the relayflowd binary that live-kernel.test.ts execs. Both suites verified locally at this base: kernel 130 passed, SDK 464 passed with 30 skipped, 0 failed. Evidence in ops/reviews/20260904-ci-coverage-gap.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR Session-Id: c228933d-4f94-4d83-9a9a-daf3c83b94f1 --- .github/workflows/cloud-runtime-artifact.yml | 32 ++++-- ops/reviews/20260904-ci-coverage-gap.md | 113 +++++++++++++++++++ 2 files changed, 138 insertions(+), 7 deletions(-) create mode 100644 ops/reviews/20260904-ci-coverage-gap.md diff --git a/.github/workflows/cloud-runtime-artifact.yml b/.github/workflows/cloud-runtime-artifact.yml index eb213bb3d..8b3f8701f 100644 --- a/.github/workflows/cloud-runtime-artifact.yml +++ b/.github/workflows/cloud-runtime-artifact.yml @@ -42,19 +42,37 @@ jobs: working-directory: kernel run: cargo build --locked --release -p relayflowd + # The kernel suite had never run in CI. The only cargo invocation was the + # release build above, so every kernel-side defect was invisible here by + # construction -- including, on 2026-09-03, an exactly-once double-fire + # where one effect fired twice, a `$ref` cycle that aborted the daemon and + # re-ran the effect on every resume, and two tests in the tree encoding + # opposite contracts that both passed because neither was ever executed. + # + # The toolchain and the build are already paid for above; this adds the + # test run and nothing else. Invoked through ops/cargo.sh, as the repo + # does everywhere else, so the target directory stays outside the tree. + - name: Test kernel + working-directory: kernel + run: sh ../ops/cargo.sh test --workspace + - name: Install SDK dependencies run: npm ci --prefix sdk - name: Test SDK and type-level authoring contracts working-directory: sdk run: | - npm run build - npm run typecheck:tests - ./node_modules/.bin/vitest run \ - tests/typed-output.test.ts \ - tests/validate.test.ts \ - tests/spec-parity.test.ts \ - tests/deterministic-llm.test.ts + # The whole suite, not four named files. Naming files means a test + # added to any other file never runs, which is how ~22 of ~26 SDK + # test files were uncovered. + # + # `npm test` is the repo's own entry point and a strict superset of + # what this step used to do: + # test:prep && typecheck && build && typecheck:tests && vitest run + # test:prep builds the relayflowd binary that tests/live-kernel.test.ts + # execs, and the build it runs is why a bare `vitest run` is wrong + # here -- several files fail at collection without sdk/dist. + npm test - name: Build standalone flows CLI run: | diff --git a/ops/reviews/20260904-ci-coverage-gap.md b/ops/reviews/20260904-ci-coverage-gap.md new file mode 100644 index 000000000..f2694673f --- /dev/null +++ b/ops/reviews/20260904-ci-coverage-gap.md @@ -0,0 +1,113 @@ +# CI coverage gap — make CI run the suites the repo already has + +Scope: `.github/` only. No `kernel/`, `sdk/`, or `testdata/` change. + +## What was wrong + +`.github/workflows/cloud-runtime-artifact.yml` is the repository's only +workflow. Before this change: + +- the only cargo invocation was `cargo build --locked --release -p relayflowd`. + **`cargo test` appeared nowhere** — the entire kernel suite never ran in CI; +- vitest ran exactly four named files (`typed-output`, `validate`, + `spec-parity`, `deterministic-llm`). The suite has 25 collected files. + +Every kernel-side defect found on 2026-09-03 was invisible to CI by +construction: an exactly-once double-fire where one effect fired twice; a +`$ref` cycle that aborted the daemon and re-ran the effect on each resume; and +two tests in the tree encoding opposite contracts that both "passed" because +neither was ever executed. + +## The change + +Two steps in one workflow file: + +```yaml + - name: Test kernel + working-directory: kernel + run: sh ../ops/cargo.sh test --workspace +``` + +and the SDK step's four named files replaced by `npm test`. + +Why `ops/cargo.sh` rather than bare `cargo`: `npm test`'s `test:prep` already +shells out to the same wrapper, which redirects `CARGO_HOME`/`RUSTUP_HOME` to +`$HOME/.relayflows-toolchain`. Using it here means the two steps share one +toolchain home and target dir instead of populating two registries. The +wrapper's bootstrap branch is not taken on a runner: `dtolnay/rust-toolchain` +has already put cargo on `PATH`, so it takes the first branch and execs. + +Why `npm test` rather than a longer `vitest run` list: it is the repo's own +entry point and a strict superset of what the step previously did — +`test:prep && typecheck && build && typecheck:tests && vitest run`. `test:prep` +builds the `relayflowd` binary that `tests/live-kernel.test.ts` execs, and the +build is why a bare `vitest run` is the wrong invocation here: several files +fail at collection without `sdk/dist`. + +## Local verification + +Kernel, the exact command the workflow now runs: + +```text +$ cd kernel && sh ../ops/cargo.sh test --workspace +test result: ok. 22 passed; 0 failed; 0 ignored +test result: ok. 34 passed; 0 failed; 0 ignored +test result: ok. 42 passed; 0 failed; 0 ignored +test result: ok. 18 passed; 0 failed; 0 ignored +test result: ok. 5 passed; 0 failed; 0 ignored +test result: ok. 4 passed; 0 failed; 0 ignored +test result: ok. 3 passed; 0 failed; 0 ignored +test result: ok. 1 passed; 0 failed; 0 ignored +test result: ok. 1 passed; 0 failed; 0 ignored +(+ 4 empty targets) +``` + +130 passed, 0 failed. + +SDK, full suite: + +```text +$ cd sdk && ./node_modules/.bin/vitest run +Test Files 1 failed | 23 passed | 1 skipped (25) + Tests 437 passed | 30 skipped (467) +``` + +The one failed file was `tests/live-kernel.test.ts`, and the cause was in the +harness, not the tree: that run had `RELAYFLOWD_BIN` exported to a path built +from a failed `cargo metadata` parse (`/debug/relayflowd`). Re-run correctly, +and then re-run again with the variable unset — which is the condition CI +actually runs under, where `test:prep` builds the binary and +`locateRelayflowd()` finds it: + +```text +$ RELAYFLOWD_BIN= ./node_modules/.bin/vitest run tests/live-kernel.test.ts +Test Files 1 passed (1) + Tests 27 passed (27) + +$ env -u RELAYFLOWD_BIN ./node_modules/.bin/vitest run tests/live-kernel.test.ts +Test Files 1 passed (1) + Tests 27 passed (27) +``` + +So the SDK suite is 464 passed, 30 skipped, 0 failed, and `live-kernel` self- +locates its binary without an environment variable. + +## `--locked` was considered and not used + +The test step omits `--locked` while the neighbouring build step has it. This +is deliberate and is the command that was actually verified above. Lockfile +drift is still caught, by that adjacent `cargo build --locked --release`. +`cargo test --workspace` also left `Cargo.lock` untouched (`git status +--porcelain` reported only the workflow file), which is the evidence that +`--locked` would have passed — but the step ships as the command with test +counts behind it rather than one inferred to be equivalent. + +## Expected effect on CI + +Both suites are green locally at this base, so this should not turn CI red. If +it does, the failure is real and pre-existing — it was simply never executed — +and per the work package the correct response is to report it, not to weaken +the workflow or fix the test in this PR. + +The marginal cost is the test run itself; the toolchain install, the kernel +build, and `npm ci` were already paid for.