Skills host-root tests are polluted by the runner user environment

Open 💬 2 comments Opened Aug 16, 2026 by rebroad

Three skills extension tests fail on a checkout located beneath a user home that contains .agents content:

  • ext::skills::host_service_tests::snapshot_for_config_merges_extension_host_and_legacy_plugin_roots
  • ext::skills::host_service_tests::snapshot_for_config_preserves_host_precedence_for_symlinked_plugin_root
  • ext::skills::host_roots_tests::repo_ancestry_without_project_marker_does_not_walk_parents

The failures include skills from the runner user's global ~/.agents/skills and discover the workspace parent .agents root, changing the expected isolated fixture result. Reproduction on the current upstream main is possible with:

cargo nextest run -p codex-skills-extension -E "test(snapshot_for_config_merges_extension_host_and_legacy_plugin_roots)"
cargo nextest run -p codex-skills-extension -E "test(snapshot_for_config_preserves_host_precedence_for_symlinked_plugin_root)"
cargo nextest run -p codex-skills-extension -E "test(repo_ancestry_without_project_marker_does_not_walk_parents)"

Please make these tests hermetic by isolating or overriding global and parent skill discovery, then remove the temporary test quarantines.

View original on GitHub ↗

2 Comments

rebroad · 11 days ago

Confirmed on upstream/latest-alpha-cli at commit 1ed8da4365: all three tests reproduce the contamination. The hermetic fix is to use request-scoped home-directory injection for user .agents/skills discovery, place fixtures outside the checkout, and explicitly set project_root_markers = [] for the no-parent-walk case. The corresponding skills-extension test suite passes 165/165 with that approach.

jdcodes1 · 11 days ago

Reproduced all three failures deterministically on macOS against main @ 1f41cc5d92 — on a machine where they normally pass — which pins down the exact leak vectors and gives CI a way to regression-test hermeticity without special checkout placement:

fake=$(mktemp -d)/fakehome
mkdir -p "$fake/.agents/skills/leak-skill" "$fake/tmp" "$fake/.git"
printf -- '---\nname: leak-skill\ndescription: leaked\n---\n\n# Body\n' > "$fake/.agents/skills/leak-skill/SKILL.md"

cargo test -p codex-skills-extension --no-run
HOME="$fake" TMPDIR="$fake/tmp" <lib test binary> \
  snapshot_for_config_merges_extension_host_and_legacy_plugin_roots \
  snapshot_for_config_preserves_host_precedence_for_symlinked_plugin_root \
  repo_ancestry_without_project_marker_does_not_walk_parents
# 0 passed; 3 failed — "leak-skill" appears in both service snapshots,
# and the roots test collects $fake/.agents/skills plus outer/.agents/skills.

There are two distinct leak vectors, and the three tests split across them:

Vector 1 — real dirs::home_dir() (the two host_service_tests). snapshot_for_config → the public resolve_skill_roots, which resolves the actual home (host_roots.rs#L34-L36) and unconditionally registers ~/.agents/skills as a SkillScope::User root (host_roots.rs#L102-L107). The tests isolate codex_home in a tempdir but the user-scope root never goes through it. The injectable variant already exists — resolve_skill_roots_with_home_dir — and host_roots_tests.rs uses it with home_dir: None throughout; it just isn't reachable from HostSkillsService. Plumbing an optional home override through the service (or HostSkillsLoadInput) closes this for every service-level test at once.

Vector 2 — project-marker ancestry walk (the host_roots_tests case). That test already injects home_dir: None, so its failure is not the home leak. find_project_root (host_roots.rs#L201-L239) probes every ancestor of the fixture cwd for .git, and the fixture lives under TempDir$TMPDIR. Whenever any ancestor of the system temp dir contains .git (checkout under a dotfiles-tracked home, TMPDIR pointed into a workspace, …), the project root resolves above the fixture and dirs_between_project_root_and_cwd then sweeps every directory in between for .agents/skills — in my repro it collected both the fake home's and the sibling outer/ roots the test asserts are excluded.

One nuance on the project_root_markers = [] fix suggested above: an empty list takes the early return in find_project_root (L206-L208) and never exercises the ancestor-probe path at all — so the test would no longer cover the behavior its name describes ("no marker found anywhere ⇒ don't walk parents"). Configuring a unique non-existent marker name (e.g. a UUID) keeps the probe loop under test while making the result environment-independent. Alternatively an injected ExecutorFileSystem scoped to the fixture makes the whole family hermetic regardless of markers.

Since cargo nextest runs one process per test, a stopgap of setting HOME/TMPDIR per-test env is viable there, but it silently doesn't isolate under plain cargo test (shared process) — injection is the robust fix. The polluted-HOME recipe above also works as a cheap CI job to keep the suite hermetic going forward.