From 8dbbf8609f50ae50218ace0ee81889bc9e62b572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yuniel=20Acosta=20P=C3=A9rez?= <33158051+yacosta738@users.noreply.github.com> Date: Sun, 23 Aug 2026 10:35:07 +0200 Subject: [PATCH 1/3] test(catalog): add regression test for sibling skills repo env var contract --- tests/test_catalog_integration.rs | 129 ++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/tests/test_catalog_integration.rs b/tests/test_catalog_integration.rs index 25e72e76..5b50fe5c 100644 --- a/tests/test_catalog_integration.rs +++ b/tests/test_catalog_integration.rs @@ -7,10 +7,41 @@ use agentsync::skills::install::{blocking_fetch_and_install_skill, install_from_ use agentsync::skills::provider::{SkillsShProvider, resolve_catalog_install_source}; use agentsync::skills::registry::read_registry; use std::path::Path; +use std::sync::Mutex; use std::thread; use std::time::Duration; use tempfile::TempDir; +/// Serialises tests that mutate `AGENTSYNC_LOCAL_SKILLS_REPO` so they cannot race the focused +/// Phase 1 test (which silently inherits whatever env var the surrounding `cargo test` +/// invocation set). The same mutex pattern lives in `tests/unit/provider.rs`. +static LOCAL_SKILLS_REPO_ENV_LOCK: Mutex<()> = Mutex::new(()); + +/// RAII guard that snapshots `AGENTSYNC_LOCAL_SKILLS_REPO` on construction and restores it on +/// drop. Reuses the contract documented in `src/skills/provider.rs:206-208`: the env var is +/// the second priority in the resolver, falling through to the sibling `/agents-skills` +/// checkout only when the var is unset. +struct LocalSkillsRepoEnvGuard { + previous: Option, +} + +impl LocalSkillsRepoEnvGuard { + fn new() -> Self { + let previous = std::env::var_os("AGENTSYNC_LOCAL_SKILLS_REPO"); + unsafe { std::env::remove_var("AGENTSYNC_LOCAL_SKILLS_REPO") }; + Self { previous } + } +} + +impl Drop for LocalSkillsRepoEnvGuard { + fn drop(&mut self) { + match self.previous.take() { + Some(value) => unsafe { std::env::set_var("AGENTSYNC_LOCAL_SKILLS_REPO", value) }, + None => unsafe { std::env::remove_var("AGENTSYNC_LOCAL_SKILLS_REPO") }, + } + } +} + fn project_root() -> &'static Path { Path::new(env!("CARGO_MANIFEST_DIR")) } @@ -130,6 +161,104 @@ fn phase1_bobmatnyc_catalog_entries_install_offline_and_register_local_ids() { } } +/// Regression for REQ-SKILLREC-001 / REQ-SKILLREC-002 — codifies the contract that the +/// resolver MUST honour `AGENTSYNC_LOCAL_SKILLS_REPO` when set and MUST fall back to the +/// sibling `/agents-skills` checkout when the env var is unset. The +/// CI workflow in `.github/workflows/catalog-e2e.yml` is the canonical place where the env +/// var is set today; the sibling fallback is what a developer workstation relies on when +/// running this test outside CI. +#[test] +fn phase1_bobmatnyc_catalog_resolver_uses_env_var_or_sibling_fallback() { + let _lock = LOCAL_SKILLS_REPO_ENV_LOCK.lock().unwrap(); + let _env_guard = LocalSkillsRepoEnvGuard::new(); + + let catalog = EmbeddedSkillCatalog::default(); + let provider = SkillsShProvider; + let phase1_ids: [(&str, &str); 3] = [ + ("dallay/agents-skills/drizzle-orm", "drizzle-orm"), + ("dallay/agents-skills/pydantic", "pydantic"), + ("dallay/agents-skills/sqlalchemy", "sqlalchemy"), + ]; + + // ---- Case A: AGENTSYNC_LOCAL_SKILLS_REPO unset, project_root has the sibling. ---- + // The agentsync worktree sits one level below the agents-skills checkout, so the + // sibling fallback MUST resolve each Phase 1 directory. This is the contract that a + // developer workstation relies on when `cargo test` is run without CI env vars. + for (provider_skill_id, local_skill_id) in phase1_ids { + let resolved = resolve_catalog_install_source( + &catalog, + &provider, + provider_skill_id, + local_skill_id, + Some(project_root()), + ) + .unwrap_or_else(|err| { + panic!( + "sibling fallback must resolve {local_skill_id} when AGENTSYNC_LOCAL_SKILLS_REPO is unset: {err}" + ) + }); + let resolved_path = Path::new(&resolved); + assert!( + resolved_path.is_dir(), + "{local_skill_id}: sibling fallback returned non-directory {resolved}" + ); + let canonical = + std::fs::canonicalize(resolved_path).unwrap_or_else(|_| resolved_path.to_path_buf()); + let canonical_str = canonical.to_string_lossy(); + assert!( + canonical_str.contains("agents-skills"), + "{local_skill_id}: sibling fallback did not resolve under an agents-skills \ + directory (got {canonical:?})" + ); + assert!( + canonical.ends_with(format!("skills/{local_skill_id}").as_str()), + "{local_skill_id}: sibling fallback returned {canonical:?}, expected .../skills/{local_skill_id}" + ); + } + + // ---- Case B: AGENTSYNC_LOCAL_SKILLS_REPO set, project_root has NO sibling. ---- + // The resolver MUST use the env var path. We point project_root at an isolated temp + // directory so the sibling fallback cannot accidentally satisfy the assertion. + let temp_root = TempDir::new().unwrap(); + let skills_root = temp_root.path().join("skills"); + for (_, local_skill_id) in phase1_ids { + let skill_dir = skills_root.join(local_skill_id); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "# regression fixture — not a real skill\n", + ) + .unwrap(); + } + let isolated_root = TempDir::new().unwrap(); + unsafe { std::env::set_var("AGENTSYNC_LOCAL_SKILLS_REPO", temp_root.path()) }; + + for (provider_skill_id, local_skill_id) in phase1_ids { + let resolved = resolve_catalog_install_source( + &catalog, + &provider, + provider_skill_id, + local_skill_id, + Some(isolated_root.path()), + ) + .unwrap_or_else(|err| { + panic!("AGENTSYNC_LOCAL_SKILLS_REPO must drive resolution for {local_skill_id}: {err}") + }); + let expected = temp_root.path().join("skills").join(local_skill_id); + let resolved_path = Path::new(&resolved); + assert_eq!( + resolved_path, expected, + "{local_skill_id}: resolver must return exactly the env var path \ + (expected {expected:?}, got {resolved_path:?})" + ); + assert!( + resolved_path.starts_with(temp_root.path()), + "{local_skill_id}: resolver must not silently fall back to the sibling \ + checkout when AGENTSYNC_LOCAL_SKILLS_REPO is set (got {resolved_path:?})" + ); + } +} + #[test] #[ignore] #[allow(unreachable_code)] From fae5c64f4660fe48fb7d2ee5e5807c7715d75a82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yuniel=20Acosta=20P=C3=A9rez?= <33158051+yacosta738@users.noreply.github.com> Date: Sun, 23 Aug 2026 10:35:28 +0200 Subject: [PATCH 2/3] fix(ci): checkout sibling agents-skills in catalog-e2e offline jobs --- .github/workflows/catalog-e2e.yml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/.github/workflows/catalog-e2e.yml b/.github/workflows/catalog-e2e.yml index f3fec3f0..09d9f286 100644 --- a/.github/workflows/catalog-e2e.yml +++ b/.github/workflows/catalog-e2e.yml @@ -20,18 +20,29 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: persist-credentials: false + - name: Checkout committed agents-skills source + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + repository: dallay/agents-skills + ref: c2e79fbb72d146305f82a8e979270795557d24fd + path: agents-skills + persist-credentials: false - uses: dtolnay/rust-toolchain@e97e2d8cc328f1b50210efc529dca0028893a2d9 # v1 with: toolchain: stable - name: Fetch locked dependencies before offline test run: cargo fetch --locked - name: Run offline fixtures with network disabled + env: + AGENTSYNC_LOCAL_SKILLS_REPO: ${{ github.workspace }}/agents-skills run: cargo test --test test_catalog_integration --locked --offline -- --nocapture catalog-installation: name: Verify catalog skill installation runs-on: ubuntu-latest timeout-minutes: 60 + env: + AGENTSYNC_LOCAL_SKILLS_REPO: ${{ github.workspace }}/agents-skills steps: - name: Checkout repository @@ -39,6 +50,14 @@ jobs: with: persist-credentials: false + - name: Checkout committed agents-skills source + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + repository: dallay/agents-skills + ref: c2e79fbb72d146305f82a8e979270795557d24fd + path: agents-skills + persist-credentials: false + - name: Setup Rust toolchain uses: dtolnay/rust-toolchain@e97e2d8cc328f1b50210efc529dca0028893a2d9 # v1 with: From 0ff4fba959c1a8ae01bac330b8d9878410cf9c67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yuniel=20Acosta=20P=C3=A9rez?= <33158051+yacosta738@users.noreply.github.com> Date: Sun, 23 Aug 2026 14:15:23 +0200 Subject: [PATCH 3/3] fix(test): stabilize catalog source resolution --- tests/test_catalog_integration.rs | 45 +++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/tests/test_catalog_integration.rs b/tests/test_catalog_integration.rs index 5b50fe5c..30cda690 100644 --- a/tests/test_catalog_integration.rs +++ b/tests/test_catalog_integration.rs @@ -17,27 +17,34 @@ use tempfile::TempDir; /// invocation set). The same mutex pattern lives in `tests/unit/provider.rs`. static LOCAL_SKILLS_REPO_ENV_LOCK: Mutex<()> = Mutex::new(()); -/// RAII guard that snapshots `AGENTSYNC_LOCAL_SKILLS_REPO` on construction and restores it on -/// drop. Reuses the contract documented in `src/skills/provider.rs:206-208`: the env var is -/// the second priority in the resolver, falling through to the sibling `/agents-skills` -/// checkout only when the var is unset. +/// RAII guard that snapshots the source override variables on construction and restores them on +/// drop. Reuses the precedence contract documented in `src/skills/provider.rs:191-218`. +const SOURCE_OVERRIDE_ENV_VARS: [&str; 2] = [ + "AGENTSYNC_LOCAL_SKILLS_REPO", + "AGENTSYNC_TEST_SKILL_SOURCE_DIR", +]; + struct LocalSkillsRepoEnvGuard { - previous: Option, + previous: [(&'static str, Option); 2], } impl LocalSkillsRepoEnvGuard { fn new() -> Self { - let previous = std::env::var_os("AGENTSYNC_LOCAL_SKILLS_REPO"); - unsafe { std::env::remove_var("AGENTSYNC_LOCAL_SKILLS_REPO") }; + let previous = SOURCE_OVERRIDE_ENV_VARS.map(|name| (name, std::env::var_os(name))); + for name in SOURCE_OVERRIDE_ENV_VARS { + unsafe { std::env::remove_var(name) }; + } Self { previous } } } impl Drop for LocalSkillsRepoEnvGuard { fn drop(&mut self) { - match self.previous.take() { - Some(value) => unsafe { std::env::set_var("AGENTSYNC_LOCAL_SKILLS_REPO", value) }, - None => unsafe { std::env::remove_var("AGENTSYNC_LOCAL_SKILLS_REPO") }, + for (name, value) in &mut self.previous { + match value.take() { + Some(value) => unsafe { std::env::set_var(*name, value) }, + None => unsafe { std::env::remove_var(*name) }, + } } } } @@ -95,6 +102,7 @@ fn offline_catalog_e2e_is_reproducible() { #[test] fn phase1_bobmatnyc_catalog_entries_install_offline_and_register_local_ids() { + let _lock = LOCAL_SKILLS_REPO_ENV_LOCK.lock().unwrap(); let catalog = EmbeddedSkillCatalog::default(); let provider = SkillsShProvider; let expected = [ @@ -181,16 +189,25 @@ fn phase1_bobmatnyc_catalog_resolver_uses_env_var_or_sibling_fallback() { ]; // ---- Case A: AGENTSYNC_LOCAL_SKILLS_REPO unset, project_root has the sibling. ---- - // The agentsync worktree sits one level below the agents-skills checkout, so the - // sibling fallback MUST resolve each Phase 1 directory. This is the contract that a - // developer workstation relies on when `cargo test` is run without CI env vars. + // Build an isolated sibling checkout so this assertion does not depend on the caller's + // filesystem layout. This is the contract that a developer workstation relies on when + // `cargo test` is run without CI env vars. + let sibling_parent = TempDir::new().unwrap(); + let sibling_project_root = sibling_parent.path().join("agentsync"); + let sibling_skills_root = sibling_parent.path().join("agents-skills").join("skills"); + for (_, local_skill_id) in phase1_ids { + let skill_dir = sibling_skills_root.join(local_skill_id); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("SKILL.md"), "# sibling fallback fixture\n").unwrap(); + } + for (provider_skill_id, local_skill_id) in phase1_ids { let resolved = resolve_catalog_install_source( &catalog, &provider, provider_skill_id, local_skill_id, - Some(project_root()), + Some(&sibling_project_root), ) .unwrap_or_else(|err| { panic!(