Repository navigation
fix(docs,ci): correct stale test-count claim; wire StoryB00k/Vue into CI - #37
Merged
Merged
Conversation
Addresses PR #36 review, P2 findings. - docs/HANDOFF-2026-09-19-reqif-flexo.md: "cargo test -p kr0ki-core requirements # 5 passed" went stale in the same PR that made it true — M1 moved the semantic tests to ufo-types, leaving only the D2 adapter test local. Corrected to the actual current counts for every command listed, each re-run and verified before writing it down: 4 (ufo-types' own mbse::requirements tests), 5 (new consumer-level contract tests below), 2 (requirements_render's D2-adapter tests), 1 (the server HTTP test). - crates/kr0ki-core/tests/requirements_contract.rs (new): the "consumer-level contract tests for the re-exported model/view behavior" the review asked for — an integration test exercising kr0ki_core::requirements and requirements_render::to_d2 through kr0ki-core's public API only, proving the ufo-types re-export and the one D2 adapter kr0ki-core owns actually compose the way kr0ki-server's HTTP routes depend on. 5 tests: promotion isolation, bounded impact/cycles, verification coverage, validation errors, and view-to-D2 lowering. - .github/workflows/ci.yml, justfile: wire the StoryB00k Python suite (7 files, 43 tests) and the playbook Vue/Vite suite (test + build) into `check`/`just test`, alongside the existing Rust and kr0ki-mcp-Python coverage. Both needed real fixes to actually pass, not just a wiring change: - playbook: `pnpm --dir playbook test` failed on a clean install — @assistant-ui/tap's core module graph statically imports `react` even on the Vue-only path, though tap declares react as an *optional* peer dependency. Added `react` as a playbook devDependency (upstream packaging quirk in a transitive dependency, not something fixable from this repo or the vendored assistant-ui fork). Committed playbook/pnpm-lock.yaml, which had never been committed despite pnpm being the tool actually in use (only a stale npm package-lock.json existed) — CI needs a lockfile to install deterministically with --frozen-lockfile. - StoryB00k agent: a bare `pip install` hits PEP 668 "externally-managed-environment" on Debian/Ubuntu (including GitHub's ubuntu-latest runners) unless run inside a venv. Both the CI step and `just test` now create one. Verified end-to-end locally with a clean `pnpm install --frozen-lockfile` and a fresh venv: 43 Python tests, 15 Vitest tests, and the Vite production build all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJ5M1bGQJB1VS8Epr7AGLa
…-server Wiring this suite into CI (previous commit) surfaced a real, pre-existing test-isolation gap: PlanningFlowTest mocked the LLM client but not fetch_manifest(), the one real network call server._run() makes (to KR0KI_URL, default http://127.0.0.1:8787) before ever reaching the mocked LLM. It passed locally only because a real kr0ki-server happened to be listening on that port in dev — on a clean CI runner it hit "Connection refused" and failed 4 tests + 1 assertion mismatch. test_server.py already has this exact mock (`@patch("server. fetch_manifest", return_value=[])`) on its two tests that call _run(); this applies the same pattern in setUp so it covers every test in the class. Verified with KR0KI_URL pointed at a guaranteed-unreachable port (http://127.0.0.1:1), not just by having a server happen to be up: 7/7 in this file, 43/43 in the full containers/kr0ki-storyb00k-agent suite. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJ5M1bGQJB1VS8Epr7AGLa
…-gated test Per review discussion on PR #37: making `pnpm --dir playbook run build` pass in CI would require building the vendored @assistant-ui/vue package's dist/, which requires installing the ENTIRE elasticdotventures/assistant-ui monorepo -- 2,347 packages, ~2GB, 2m45s (Next.js, Nuxt, SvelteKit, docs tooling, none of which kr0ki needs) -- just to produce one small package's output. Confirmed by actually running the install. Decided against taking that on right now; dropped the build step from CI and `just test`. That also means `pnpm --dir playbook test` itself fails on a clean CI checkout: assistant-ui-vendor.test.js statically imports "@assistant-ui/vue", and Vite's import-analysis can't resolve it without dist/ present -- confirmed against the actual CI failure log, not assumed. describe.skipIf can't help here since the import fails at module-load time, before any test-level skip logic runs. Fixed via a CI-conditional `exclude` in playbook/vite.config.js instead (using vitest's own configDefaults.exclude as the base, not a hand-rolled duplicate of its defaults), so the file is never even collected when CI=true. It still runs for any developer who has built the vendored package locally. Verified all three states directly, including with vendor/assistant-ui/ packages/vue/dist/ actually absent (not just unset env), matching a real clean checkout: - dist/ present, no CI env: 3 files / 15 tests pass - dist/ absent, CI=true: 2 files / 13 tests pass (vendor file skipped) - dist/ absent, no CI env: correctly fails (accurate signal for a local dev who hasn't built the vendored package -- pre-existing behavior, now just correctly scoped to not affect CI) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CJ5M1bGQJB1VS8Epr7AGLa
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the two P2 findings from the #36 review (P1s were fixed in #36 itself before merge):
docs/HANDOFF-2026-09-19-reqif-flexo.mdclaimedcargo test -p kr0ki-core requirementsruns 5 tests; M1 (merged in ReqIF/Flexo requirements viewpoints (M1: vendor mbse::requirements to ufo-types) #36) moved the semantic tests upstream toufo-types, leaving only 1 local. Corrected every command/count in that section, each re-run and verified before writing it down.crates/kr0ki-core/tests/requirements_contract.rs(the "consumer-level contract tests for the re-exported model/view behavior" the review asked for — exerciseskr0ki_core::requirements+requirements_render::to_d2through the public API only), then wired the StoryB00k Python suite (7 files, 43 tests) and the playbook Vue/Vite suite (test + build) intocheck/just test.Wiring the Vue/Python suites into CI surfaced two real, previously-undetected gaps that had to be fixed for them to actually pass (not just be wired):
pnpm --dir playbook testfailed on a clean install:@assistant-ui/tap's core module graph statically importsreacteven on the Vue-only path, thoughreactis only an optional peer dependency. Addedreactas a playbook devDependency. Also committedplaybook/pnpm-lock.yaml, which had never been committed despite pnpm being the tool actually in use — CI needs it for--frozen-lockfile.pip installhits PEP 668 "externally-managed-environment" on Debian/Ubuntu, including GitHub'subuntu-latestrunners. Both the CI step andjust testnow create a venv first.Test plan
cargo build --workspace,cargo clippy --workspace -- -D warnings,cargo fmt --all -- --checkcargo test --workspace(all crates, 0 failures)cargo test -p kr0ki-core --test requirements_contract(5 new contract tests)just testend-to-end locally: 43 StoryB00k Python tests, 15 playbook Vitest tests, Vite production build — all clean from a freshpnpm install --frozen-lockfile+ fresh venv