Skip to content

ci: run the test suites on PRs, and fix the two tests that block them - #62

Merged
knihit merged 6 commits into
mainfrom
ci/run-test-suites
Oct 22, 2026
Merged

knihit merged 6 commits into
mainfrom
ci/run-test-suites

Conversation

@knihit

@knihit knihit commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Description

PR CI ran only the Kiro Power tests. The rest of tests/*.py, all of tests/skills/**/*.sh and the Rust tests for cli, mcp and shared never ran before merge. That's how a broken mcp telemetry test shipped in 1.0.3 and 1.1.0. This PR runs those suites and fixes the two tests that would otherwise block every PR.

CI (Makefile, .github/workflows/validate-pr.yml)

  • Added make test-py, which runs python3 -m pytest -q tests/. CI runs it after make synth, so the tests that need build/cli/konductor run instead of skipping. The pip step now also installs pyyaml, which two tests import.
  • Added make test-skills-sh, which runs every tests/skills/*/*.sh by glob, so new tests are picked up. SKILLS_SH_EXCLUDE lists tests known to fail, each with a reason.
  • CI runs the Rust tests: cargo test in cli/konductor-rs, then make -C mcp test-rust and make -C shared test-rust. It reuses the existing toolchain step.

Fix: mcp mcp_tool_call events missed parentAgentName

  • docs/telemetry-schema.json requires parentAgentName on every event. The skill-lookup MCP server's mcp_tool_call body left it out, so 1.0.3 and 1.1.0 sent events without it.
  • It's now sent as an explicit null. This fixes mcp_tool_call_body_satisfies_schemas_required_list.

Fix: konductor update --cli could fail with "Text file busy"

  • The self-update smoke test runs the binary it just wrote. A fork elsewhere in the process can briefly hold a write handle on it, so exec fails with ETXTBSY (os error 26).
  • cli_self_update.rs now retries that exec up to 10 times, 100 ms apart. Unit tests drive the retry with a fake exec.
  • real_smoke_test_succeeds_and_returns_stdout therefore runs in CI instead of being skipped.

Still excluded, each failing on main today

  • tests/skills/persistent-memory/test-nonbullet-injection.sh: 2 of 8 cases fail. The test writes its URL allowlist to a different directory from the one memory-validator.sh reads (.konductor/), so the allowlist is never applied.
  • cli::update::tests::update_finalize_index_reflects_a_concurrently_installed_other_strategy: skipped. On 4 CPUs it failed 5 of 10 full-suite runs. update_one_target sometimes returns 64.
  • npm test: not added. 17 tests fail because scripts/brand-config/lib/constants.js isn't in the repo.
  • make guide-check: not added. It fails on a stale config entry in WITHDRAWN_COMMANDS.

CHANGELOG.md has [Unreleased] / Fixed entries for both fixes.

Supersedes #47 and #61, both closed.

Type of Change

  • CLI (cli/) change: Rust
  • Infrastructure / pipeline

Testing

  • This PR's own CI: every Status Checks step passes, including the new pytest, skill shell and Rust steps.
  • make -C mcp test-rust: all pass. The schema test fails without the fix.
  • ETXTBSY retry, on 4 CPUs (taskset -c 0-3):
    • 19 cli_self_update tests pass.
    • The smoke test passed 20 of 20 runs on its own.
    • The full cli suite passed 5 of 5 runs, 1734 unit tests each, with only the update_finalize_index... skip.
  • make test-py and make test-skills-sh pass on main with the exclusions above. With SKILLS_SH_EXCLUDE= emptied, the target fails and names the file.
  • The workflow parses (PyYAML).
  • npm test: fails on main, see above
  • cd cli && make test: covered by the cli cargo test runs above
  • Changed code files carry the SPDX header

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own changes
  • I have commented my code where necessary
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added or updated tests that prove my change works

Notes for Reviewers

validate-pr.yml ran only the Kiro Power tests, so most of tests/*.py,
tests/skills/**/*.sh and the cli/mcp/shared Rust tests never ran before
merge. A broken mcp telemetry schema test shipped this way.

- Add `make test-py` (pytest over tests/) and `make test-skills-sh`
  (globs tests/skills/*/*.sh, minus SKILLS_SH_EXCLUDE).
- Run both in CI, with test-py after synth so the tests that need
  build/cli/konductor run.
- Run the Rust tests for cli, mcp and shared, skipping two cli tests
  that fail intermittently on a 4-vCPU runner.
- Exclude persistent-memory/test-nonbullet-injection.sh, which writes
  its config to a path the validator no longer reads.
- Install PyYAML alongside pytest; two tests import it.
update --cli's smoke test spawns the freshly downloaded binary with
Command::output(). On Linux, fork() duplicates every file descriptor
open anywhere in the process, so an unrelated Command::spawn() running
concurrently elsewhere in the same process can fork a child that
inherits a transient write handle on the just-written binary and hold
it open until that child execs or exits. Until then, exec of the
binary fails with ETXTBSY (os error 26) even though the smoke test's
own write handle closed well before the exec was attempted.

Retry the spawn up to 10 times with a 100ms delay between attempts
(about 1 second total) when the failure is ExecutableFileBusy, giving
the other process time to exec or exit and release its inherited
handle. The retry loop is extracted into its own function with an
injectable spawn/sleep seam, so a unit test can drive it
deterministically with a fake exec that fails N times then succeeds,
and a separate case that exhausts all retries, without depending on
reproducing the real race.
docs/telemetry-schema.json requires parentAgentName on every event, but
the skill-lookup MCP server's mcp_tool_call body omitted it. It's now sent
as an explicit null, which also fixes the existing schema-drift test.
@knihit knihit changed the title ci: run the pytest, skill shell and Rust test suites on PRs ci: run the test suites on PRs, and fix the two tests that block them Oct 16, 2026
@knihit
knihit marked this pull request as ready for review October 16, 2026 20:05
@knihit
knihit merged commit d7f7dc3 into main Oct 22, 2026
8 checks passed
@knihit
knihit deleted the ci/run-test-suites branch October 22, 2026 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants