Summary
cargo test -p bsk --lib skill_install fails intermittently on main (d04c752), always inside skill_install::harness::tests. hermes_skills_dir_honors_hermes_home_env sets and restores the process-wide HERMES_HOME while sibling tests read it from other threads.
Reproduction
cargo test --locked -p bsk --lib skill_install
30 consecutive runs on macOS 15.6 (aarch64, Rust 1.97.1, main at d04c752): 6 failures, every one of them detects_hermes_from_home_layout.
thread 'skill_install::harness::tests::detects_hermes_from_home_layout' panicked at
crates/bsk-cli/src/skill_install/harness.rs:552:9:
assertion `left == right` failed
left: "/var/folders/.../.tmpnVRcYf/custom-hermes/skills"
right: "/var/folders/.../.tmpaX6clB/.hermes/skills"
The left-hand path is the temp directory created by hermes_skills_dir_honors_hermes_home_env, so the assertion is reading that test's HERMES_HOME.
Running the whole lib suite (cargo test -p bsk --lib) shows the same race land on skills_dirs_match_harness_spec instead (harness.rs:470, the Hermes assertion), at roughly 1 run in 10. --test-threads=1 is always green.
Cause
HarnessId::skills_dir_for_home resolves Hermes through hermes_home_for_user_home, which reads HERMES_HOME (harness.rs:323). hermes_skills_dir_honors_hermes_home_env mutates that variable without taking any lock, and its SAFETY comment says "harness tests do not run in parallel with other env-mutating tests". The hazard is not only other writers: detects_hermes_from_home_layout and skills_dirs_match_harness_spec read the variable concurrently.
The Kimi tests have the same shape but do take kimi_env_lock() (harness.rs:402). That SAFETY comment states the lock covers every test that reads or writes KIMI_CODE_HOME, which is not the case either: skills_dirs_match_harness_spec reads it through kimi_code_home_for_user_home without holding the lock. The same failure is therefore latent for KimiCode; Hermes just loses the race more often because nothing serializes it at all.
Edition 2024 marks set_var unsafe precisely because a concurrent getenv on another thread is undefined behaviour, so this is a soundness problem in the test module, not only an unstable assertion.
Suggested fix
Either serialize the whole group — one lock (kimi_env_lock generalized to a harness-wide env lock) taken by every test that reads or writes HERMES_HOME / KIMI_CODE_HOME, including skills_dirs_match_harness_spec, detects_hermes_from_home_layout and detects_kimi_code_from_home_layout — or remove process env from the tests by giving the resolvers an explicit override argument and reading the variable at the call site.
Happy to send a PR for whichever shape you prefer.
Noticed while validating #291; it is unrelated to that change, which reproduces the failure at the same rate as unmodified main.
Summary
cargo test -p bsk --lib skill_installfails intermittently onmain(d04c752), always insideskill_install::harness::tests.hermes_skills_dir_honors_hermes_home_envsets and restores the process-wideHERMES_HOMEwhile sibling tests read it from other threads.Reproduction
cargo test --locked -p bsk --lib skill_install30 consecutive runs on macOS 15.6 (aarch64, Rust 1.97.1,
mainatd04c752): 6 failures, every one of themdetects_hermes_from_home_layout.The left-hand path is the temp directory created by
hermes_skills_dir_honors_hermes_home_env, so the assertion is reading that test'sHERMES_HOME.Running the whole lib suite (
cargo test -p bsk --lib) shows the same race land onskills_dirs_match_harness_specinstead (harness.rs:470, the Hermes assertion), at roughly 1 run in 10.--test-threads=1is always green.Cause
HarnessId::skills_dir_for_homeresolves Hermes throughhermes_home_for_user_home, which readsHERMES_HOME(harness.rs:323).hermes_skills_dir_honors_hermes_home_envmutates that variable without taking any lock, and its SAFETY comment says "harness tests do not run in parallel with other env-mutating tests". The hazard is not only other writers:detects_hermes_from_home_layoutandskills_dirs_match_harness_specread the variable concurrently.The Kimi tests have the same shape but do take
kimi_env_lock()(harness.rs:402). That SAFETY comment states the lock covers every test that reads or writesKIMI_CODE_HOME, which is not the case either:skills_dirs_match_harness_specreads it throughkimi_code_home_for_user_homewithout holding the lock. The same failure is therefore latent for KimiCode; Hermes just loses the race more often because nothing serializes it at all.Edition 2024 marks
set_varunsafe precisely because a concurrentgetenvon another thread is undefined behaviour, so this is a soundness problem in the test module, not only an unstable assertion.Suggested fix
Either serialize the whole group — one lock (
kimi_env_lockgeneralized to a harness-wide env lock) taken by every test that reads or writesHERMES_HOME/KIMI_CODE_HOME, includingskills_dirs_match_harness_spec,detects_hermes_from_home_layoutanddetects_kimi_code_from_home_layout— or remove process env from the tests by giving the resolvers an explicit override argument and reading the variable at the call site.Happy to send a PR for whichever shape you prefer.
Noticed while validating #291; it is unrelated to that change, which reproduces the failure at the same rate as unmodified
main.