From b0f5591b6a6ada39dc84e1c4885ae873c4d57bc6 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 24 Jun 2026 22:54:00 +0000 Subject: [PATCH 1/2] fix: address code-review findings on registry index, publish auth, CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attacks the bugs surfaced by the max-effort code review of PR #194. Finding #1 & #2 (data integrity, index_manager.rs atomic_update): Sidecar lock and temp paths were derived with Path::with_extension, which REPLACES the last dotted segment. Skill index files are named by the bare package (no fixed extension) and package names may contain dots, so: - org/web.scraper and org/web.crawler both mapped to org/web.lock, forcing unrelated publishes to share one lock (false contention, possible 30s lock-timeout failures); and - org/data's temp path org/data.tmp was the literal index file of skill org/data.tmp, so the atomic rename destroyed that skill's index. Fix: append_suffix() derives the lock path by appending (distinct per skill), and unique_temp_path() tags the temp name with pid + a monotonic counter so it can never equal another skill's index. Two regression tests added (both verified to fail on the pre-fix code). Finding #3 (cli/commands/publish.rs): Restore the no-token guard removed in #194 — without a token, publish now fails fast with the actionable "Run `fastskill auth login`" message instead of sending an unauthenticated request and surfacing a raw 401. Finding #4 (.github/workflows/test.yml): Clippy only linted --all-features. Add a default-feature clippy pass, since release binaries are built from the default (registry-publish OFF) config. Finding #5 (orphaned root tests/): Update stale /api/registry/... mocks/snapshots to the /api/v1/registry/... contract from #194. (These files live in the virtual-workspace-root tests/ tree and are not compiled; wiring-in vs deleting is left as a follow-up.) Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/test.yml | 3 + crates/fastskill-cli/src/commands/publish.rs | 8 +- .../src/core/registry/index_manager.rs | 110 +++++++++++++++++- tests/cli/registry_e2e_tests.rs | 2 +- ...hot_helpers__sources_test_unreachable.snap | 2 +- tests/integration/registry_list_test.rs | 18 +-- tests/integration_registry_list_test.rs | 30 ++--- tests/integration_registry_publish_test.rs | 4 +- 8 files changed, 143 insertions(+), 34 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5dcf86c4..3f045e91 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -28,6 +28,9 @@ jobs: - uses: actions/checkout@v4 - uses: Swatinem/rust-cache@v2 - run: cargo clippy --workspace --all-targets --all-features + # Also lint the default-feature config (registry-publish OFF) — this is what + # release binaries are built from, so it must stay clean too. + - run: cargo clippy --workspace --all-targets build: name: Build diff --git a/crates/fastskill-cli/src/commands/publish.rs b/crates/fastskill-cli/src/commands/publish.rs index d35fca9f..247cb559 100644 --- a/crates/fastskill-cli/src/commands/publish.rs +++ b/crates/fastskill-cli/src/commands/publish.rs @@ -296,11 +296,17 @@ pub async fn execute_publish(args: PublishArgs) -> CliResult<()> { /// Publish to API with authentication async fn publish_to_api_with_auth(context: &PublishContext) -> CliResult<()> { let token = crate::auth_config::get_token_with_refresh(&context.target).await?; + let token_str = token.ok_or_else(|| { + CliError::Validation(format!( + "No authentication token found for registry: {}. Run `fastskill auth login` to authenticate.", + context.target + )) + })?; publish_to_api( &context.target, &context.packages, - token.as_deref(), + Some(&token_str), context.wait, context.max_wait, ) diff --git a/crates/fastskill-core/src/core/registry/index_manager.rs b/crates/fastskill-core/src/core/registry/index_manager.rs index cc4fa6ba..e86d9bbe 100644 --- a/crates/fastskill-core/src/core/registry/index_manager.rs +++ b/crates/fastskill-core/src/core/registry/index_manager.rs @@ -5,7 +5,8 @@ use crate::core::service::ServiceError; use fs2::FileExt; use std::fs::{File, OpenOptions}; use std::io::Write; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::Mutex; use std::time::{Duration, Instant, SystemTime}; use tracing::{info, warn}; @@ -29,6 +30,35 @@ struct IndexFileMetadata { size: u64, } +/// Append a suffix to a path's full file name — e.g. `org/web.scraper` + `lock` +/// → `org/web.scraper.lock`. +/// +/// Unlike [`Path::with_extension`], this does NOT replace the last dotted segment. +/// Skill index files are named by the bare package (no fixed extension) and package +/// names may contain dots, so `with_extension("lock")` mapped both `org/web.scraper` +/// and `org/web.crawler` onto the same `org/web.lock`, making unrelated publishes +/// share a sidecar lock. Appending keeps each skill's sidecar path distinct. +fn append_suffix(path: &Path, suffix: &str) -> PathBuf { + let mut name = path.as_os_str().to_os_string(); + name.push("."); + name.push(suffix); + PathBuf::from(name) +} + +/// Derive a per-call unique temp path next to the index file (same directory, so the +/// final rename stays atomic on one filesystem). +/// +/// A deterministic `index_path.with_extension("tmp")` could equal another skill's real +/// index file (e.g. writing `org/data` produced temp `org/data.tmp`, which is the index +/// of skill `org/data.tmp`), so the rename would destroy that skill's index. Tagging the +/// temp name with the process id and a monotonic counter makes such a collision +/// impossible in practice. +fn unique_temp_path(index_path: &Path) -> PathBuf { + static SEQ: AtomicU64 = AtomicU64::new(0); + let seq = SEQ.fetch_add(1, Ordering::Relaxed); + append_suffix(index_path, &format!("{}.{}.tmp", std::process::id(), seq)) +} + impl IndexManager { /// Create a new IndexManager instance /// @@ -107,7 +137,7 @@ impl IndexManager { // Step 4: Acquire exclusive lock with timeout. Use a sidecar lock file // because the index file is replaced by atomic rename during update. - let lock_path = index_path.with_extension("lock"); + let lock_path = append_suffix(&index_path, "lock"); let lock_start = Instant::now(); let file = loop { let file = OpenOptions::new() @@ -265,7 +295,7 @@ impl IndexManager { existing_entries.push(entry.clone()); // Step 7: Write to temporary file - let temp_path = index_path.with_extension("tmp"); + let temp_path = unique_temp_path(&index_path); let mut temp_file = match File::create(&temp_path) { Ok(file) => file, Err(e) => { @@ -399,9 +429,9 @@ mod tests { use std::sync::Arc; use tempfile::TempDir; - fn entry(version: &str) -> VersionEntry { + fn entry_named(name: &str, version: &str) -> VersionEntry { VersionEntry { - name: "testorg/test-skill".to_string(), + name: name.to_string(), vers: version.to_string(), deps: Vec::new(), cksum: format!("checksum-{version}"), @@ -415,6 +445,10 @@ mod tests { } } + fn entry(version: &str) -> VersionEntry { + entry_named("testorg/test-skill", version) + } + fn read_entries(registry_path: PathBuf) -> Vec { let index_path = get_skill_index_path(®istry_path, "testorg/test-skill").unwrap(); IndexManager::read_entries_from_path(&index_path).unwrap() @@ -468,4 +502,70 @@ mod tests { let entries = read_entries(registry_path); assert_eq!(entries.len(), 10); } + + /// Regression: two skills whose package names differ only after a dot must NOT + /// share a sidecar lock file. Before the fix, `with_extension("lock")` mapped both + /// `org/web.scraper` and `org/web.crawler` onto `org/web.lock`. + #[test] + fn atomic_update_uses_distinct_lock_files_for_dotted_names() { + let temp_dir = TempDir::new().unwrap(); + let registry_path = temp_dir.path().to_path_buf(); + let manager = IndexManager::new(registry_path.clone()); + + manager + .atomic_update( + "org/web.scraper", + "1.0.0", + &entry_named("org/web.scraper", "1.0.0"), + ) + .unwrap(); + manager + .atomic_update( + "org/web.crawler", + "1.0.0", + &entry_named("org/web.crawler", "1.0.0"), + ) + .unwrap(); + + // Each skill keeps its own sidecar lock (append, not replace-extension). + assert!( + registry_path.join("org/web.scraper.lock").exists(), + "expected distinct lock for web.scraper" + ); + assert!( + registry_path.join("org/web.crawler.lock").exists(), + "expected distinct lock for web.crawler" + ); + } + + /// Regression: publishing a skill must not clobber another skill whose index file + /// happens to match the first skill's temp path. Before the fix, writing `org/data` + /// used temp `org/data.tmp` — the literal index file of skill `org/data.tmp` — and + /// the atomic rename destroyed it. + #[test] + fn atomic_update_does_not_clobber_skill_named_like_temp_path() { + let temp_dir = TempDir::new().unwrap(); + let registry_path = temp_dir.path().to_path_buf(); + let manager = IndexManager::new(registry_path.clone()); + + manager + .atomic_update( + "org/data.tmp", + "1.0.0", + &entry_named("org/data.tmp", "1.0.0"), + ) + .unwrap(); + manager + .atomic_update("org/data", "1.0.0", &entry_named("org/data", "1.0.0")) + .unwrap(); + + let victim_index = get_skill_index_path(®istry_path, "org/data.tmp").unwrap(); + let victim = IndexManager::read_entries_from_path(&victim_index).unwrap(); + assert_eq!( + victim.len(), + 1, + "publishing org/data must not destroy the org/data.tmp index" + ); + assert_eq!(victim[0].name, "org/data.tmp"); + } } diff --git a/tests/cli/registry_e2e_tests.rs b/tests/cli/registry_e2e_tests.rs index 409819b5..dd2eacd1 100644 --- a/tests/cli/registry_e2e_tests.rs +++ b/tests/cli/registry_e2e_tests.rs @@ -458,7 +458,7 @@ async fn test_sources_test_connectivity() { return; }; Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string("[]")) .mount(&mock_server) .await; diff --git a/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__sources_test_unreachable.snap b/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__sources_test_unreachable.snap index 3bcc083a..9177827c 100644 --- a/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__sources_test_unreachable.snap +++ b/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__sources_test_unreachable.snap @@ -15,4 +15,4 @@ FastSkill [VERSION] - 'sources refresh' → 'repos refresh' - 'sources create' → 'marketplace create' -Error: Configuration error: Repository 'unreachable-repo' test failed: Client error: HTTP request failed: error sending request for url (http://localhost:9999/api/registry/index/skills): error trying to connect: tcp connect error: [NETWORK_ERROR] +Error: Configuration error: Repository 'unreachable-repo' test failed: Client error: HTTP request failed: error sending request for url (http://localhost:9999/api/v1/registry/index/skills): error trying to connect: tcp connect error: [NETWORK_ERROR] diff --git a/tests/integration/registry_list_test.rs b/tests/integration/registry_list_test.rs index 1bc2d11e..69833089 100644 --- a/tests/integration/registry_list_test.rs +++ b/tests/integration/registry_list_test.rs @@ -107,7 +107,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { // Mock the endpoint Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -136,7 +136,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { let filtered_response = serde_json::to_string(&filtered_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("scope", "acme")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response)) .mount(&mock_server) @@ -155,7 +155,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { }; Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("all_versions", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response)) .mount(&mock_server) @@ -171,7 +171,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { }; Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("include_pre_release", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response)) .mount(&mock_server) @@ -195,7 +195,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -209,7 +209,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { // Test 401 Unauthorized (authentication required) Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(401)) .mount(&mock_server) .await; @@ -221,7 +221,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { // Test 403 Forbidden (access denied) Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(403)) .mount(&mock_server) .await; @@ -244,7 +244,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(wiremock::matchers::header("Authorization", "token test-token-123")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) @@ -330,7 +330,7 @@ async fn test_performance_benchmark_http_listing_1000_skills() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; diff --git a/tests/integration_registry_list_test.rs b/tests/integration_registry_list_test.rs index da1064f4..17a9db65 100644 --- a/tests/integration_registry_list_test.rs +++ b/tests/integration_registry_list_test.rs @@ -109,7 +109,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { // Mock the endpoint Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -136,7 +136,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { let filtered_response = serde_json::to_string(&filtered_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("scope", "acme")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server2) @@ -162,7 +162,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { }; Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("all_versions", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server) @@ -178,7 +178,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { }; Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("include_pre_release", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server) @@ -202,7 +202,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -219,7 +219,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { return; }; Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(401)) .mount(&mock_server_401) .await; @@ -236,7 +236,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { return; }; Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(403)) .mount(&mock_server_403) .await; @@ -261,7 +261,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(wiremock::matchers::header( "Authorization", "token test-token-123", @@ -354,7 +354,7 @@ async fn test_performance_benchmark_http_listing_1000_skills() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -431,7 +431,7 @@ async fn test_list_skills_scope_flag() { let filtered_response = serde_json::to_string(&filtered_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("scope", "acme")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server) @@ -631,7 +631,7 @@ async fn test_list_skills_json_flag() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -676,7 +676,7 @@ async fn test_list_skills_json_scope_flag() { let filtered_response = serde_json::to_string(&filtered_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("scope", "acme")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server) @@ -767,7 +767,7 @@ async fn test_list_skills_all_versions_flag() { let response_body = serde_json::to_string(&all_versions_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("all_versions", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) @@ -833,7 +833,7 @@ async fn test_list_skills_all_versions_include_pre_release_json() { let response_body = serde_json::to_string(&summaries_with_prerelease).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("all_versions", "true")) .and(query_param("include_pre_release", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) @@ -890,7 +890,7 @@ async fn test_list_skills_all_versions_without_pre_release() { let response_body = serde_json::to_string(&summaries_without_prerelease).unwrap(); Mock::given(method("GET")) - .and(path("/api/registry/index/skills")) + .and(path("/api/v1/registry/index/skills")) .and(query_param("all_versions", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) diff --git a/tests/integration_registry_publish_test.rs b/tests/integration_registry_publish_test.rs index 56797619..d70dac99 100644 --- a/tests/integration_registry_publish_test.rs +++ b/tests/integration_registry_publish_test.rs @@ -266,7 +266,7 @@ async fn test_verify_skill_in_registry() { // Check if skill is in registry via API let client = reqwest::Client::new(); - let url = format!("{}/api/registry/skills", PRODUCTION_REGISTRY_URL); + let url = format!("{}/api/v1/registry/skills", PRODUCTION_REGISTRY_URL); let response = client .get(&url) @@ -447,7 +447,7 @@ async fn test_complete_publish_workflow() { // Step 4: Verify via API println!("[4/4] Verifying skill in registry..."); let client = reqwest::Client::new(); - let url = format!("{}/api/registry/skills", PRODUCTION_REGISTRY_URL); + let url = format!("{}/api/v1/registry/skills", PRODUCTION_REGISTRY_URL); let response = client .get(&url) From 40560bf4ab55a1fadc0aa45cb63d140d0e6a8e07 Mon Sep 17 00:00:00 2001 From: Alex Oliveira <4482374+aroff@users.noreply.github.com> Date: Wed, 24 Jun 2026 23:12:49 +0000 Subject: [PATCH 2/2] refactor: consolidate atomic-write, fix collision at the root (review) Addresses the code-quality review of the first commit. - Bug CLASS, not two leaves: the collision lived in the canonical `utils::atomic_write` too (`path.with_extension("tmp")`). Move the collision-safe `append_suffix` into utils, route `atomic_write` through it, and derive the index lock path via a new `get_skill_index_lock_path` co-located with `get_skill_index_path`. Now every write path is hardened in one place instead of patching index_manager alone. - Kill duplication: `index_manager::atomic_update` re-implemented the write-tmp/fsync/rename tail with the ENOSPC "filesystem full" check copy-pasted three times. Serialize once into a buffer and classify ENOSPC once via `is_disk_full`. index_manager.rs shrinks 571 -> 523 lines. - index_manager keeps a UNIQUE temp name (it holds its own sidecar lock across the read-modify-write, and a deterministic temp could equal another skill's index); atomic_write keeps a DETERMINISTIC temp (it doubles as the advisory lock token). Tests in utils/lock updated to the new tmp derivation. - Drop the dead-test churn from the first commit (those root tests/ files are uncompiled); wiring-in vs deleting tracked separately. All four index_manager regression tests still pass (and still fail on the pre-fix code); utils/lock/registry_index tests green; clippy clean on both default and --all-features. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/fastskill-core/src/core/lock.rs | 2 +- .../src/core/registry/index_manager.rs | 142 ++++++------------ .../fastskill-core/src/core/registry_index.rs | 16 ++ crates/fastskill-core/src/utils.rs | 25 ++- tests/cli/registry_e2e_tests.rs | 2 +- ...hot_helpers__sources_test_unreachable.snap | 2 +- tests/integration/registry_list_test.rs | 18 +-- tests/integration_registry_list_test.rs | 30 ++-- tests/integration_registry_publish_test.rs | 4 +- 9 files changed, 112 insertions(+), 129 deletions(-) diff --git a/crates/fastskill-core/src/core/lock.rs b/crates/fastskill-core/src/core/lock.rs index f3914dd8..d1c6689d 100644 --- a/crates/fastskill-core/src/core/lock.rs +++ b/crates/fastskill-core/src/core/lock.rs @@ -691,7 +691,7 @@ depth = 0 let tmp = TempDir::new().unwrap(); let lock_path = tmp.path().join("skills.lock"); - let tmp_path = lock_path.with_extension("tmp"); + let tmp_path = crate::utils::append_suffix(&lock_path, "tmp"); // Acquire exclusive lock on the .tmp file before calling save_to_file let holder = std::fs::OpenOptions::new() diff --git a/crates/fastskill-core/src/core/registry/index_manager.rs b/crates/fastskill-core/src/core/registry/index_manager.rs index e86d9bbe..036ed78c 100644 --- a/crates/fastskill-core/src/core/registry/index_manager.rs +++ b/crates/fastskill-core/src/core/registry/index_manager.rs @@ -1,6 +1,8 @@ //! Atomic index update manager with file locking -use crate::core::registry_index::{get_skill_index_path, ScopedSkillName, VersionEntry}; +use crate::core::registry_index::{ + get_skill_index_lock_path, get_skill_index_path, ScopedSkillName, VersionEntry, +}; use crate::core::service::ServiceError; use fs2::FileExt; use std::fs::{File, OpenOptions}; @@ -30,33 +32,28 @@ struct IndexFileMetadata { size: u64, } -/// Append a suffix to a path's full file name — e.g. `org/web.scraper` + `lock` -/// → `org/web.scraper.lock`. -/// -/// Unlike [`Path::with_extension`], this does NOT replace the last dotted segment. -/// Skill index files are named by the bare package (no fixed extension) and package -/// names may contain dots, so `with_extension("lock")` mapped both `org/web.scraper` -/// and `org/web.crawler` onto the same `org/web.lock`, making unrelated publishes -/// share a sidecar lock. Appending keeps each skill's sidecar path distinct. -fn append_suffix(path: &Path, suffix: &str) -> PathBuf { - let mut name = path.as_os_str().to_os_string(); - name.push("."); - name.push(suffix); - PathBuf::from(name) -} - /// Derive a per-call unique temp path next to the index file (same directory, so the /// final rename stays atomic on one filesystem). /// -/// A deterministic `index_path.with_extension("tmp")` could equal another skill's real -/// index file (e.g. writing `org/data` produced temp `org/data.tmp`, which is the index -/// of skill `org/data.tmp`), so the rename would destroy that skill's index. Tagging the -/// temp name with the process id and a monotonic counter makes such a collision -/// impossible in practice. +/// Unlike `utils::atomic_write`, whose temp name doubles as its advisory lock token and +/// so must be deterministic, the index writer holds its own sidecar lock across the whole +/// read-modify-write — so its temp file need not lock, and it MUST be unique: a +/// deterministic `org/data.tmp` could equal the real index file of skill `org/data.tmp` +/// and be renamed away. Tagging the name with the process id and a monotonic counter +/// makes that collision impossible in practice. fn unique_temp_path(index_path: &Path) -> PathBuf { static SEQ: AtomicU64 = AtomicU64::new(0); let seq = SEQ.fetch_add(1, Ordering::Relaxed); - append_suffix(index_path, &format!("{}.{}.tmp", std::process::id(), seq)) + crate::utils::append_suffix(index_path, &format!("{}.{}.tmp", std::process::id(), seq)) +} + +/// Returns true if `e` is an out-of-disk-space error (ENOSPC). +fn is_disk_full(e: &std::io::Error) -> bool { + if e.raw_os_error() == Some(28) { + return true; + } + let msg = e.to_string().to_lowercase(); + msg.contains("no space") || msg.contains("filesystem full") } impl IndexManager { @@ -137,7 +134,7 @@ impl IndexManager { // Step 4: Acquire exclusive lock with timeout. Use a sidecar lock file // because the index file is replaced by atomic rename during update. - let lock_path = append_suffix(&index_path, "lock"); + let lock_path = get_skill_index_lock_path(&self.registry_path, &normalized_id)?; let lock_start = Instant::now(); let file = loop { let file = OpenOptions::new() @@ -291,89 +288,44 @@ impl IndexManager { existing_entries.len() ); - // Step 6: Append new entry + // Step 6: Append the new entry and serialize the whole index as + // newline-delimited JSON. existing_entries.push(entry.clone()); - - // Step 7: Write to temporary file - let temp_path = unique_temp_path(&index_path); - let mut temp_file = match File::create(&temp_path) { - Ok(file) => file, - Err(e) => { - // Check if error is due to filesystem being full - let error_msg = e.to_string().to_lowercase(); - if error_msg.contains("no space") - || error_msg.contains("filesystem full") - || e.raw_os_error() == Some(28) - { - warn!( - "Filesystem full: cannot write index file for {} v{}", - normalized_id, version - ); - return Err(ServiceError::Custom(format!( - "Filesystem full: cannot update index for {} v{}. Existing index preserved.", - normalized_id, version - ))); - } - return Err(ServiceError::Io(e)); - } - }; - - // Write all entries as newline-delimited JSON + let mut buffer = String::new(); for entry in &existing_entries { let line = serde_json::to_string(entry).map_err(|e| { ServiceError::Custom(format!("Failed to serialize index entry: {}", e)) })?; - if let Err(e) = writeln!(temp_file, "{}", line) { - // Check if error is due to filesystem being full - let error_msg = e.to_string().to_lowercase(); - if error_msg.contains("no space") - || error_msg.contains("filesystem full") - || e.raw_os_error() == Some(28) - { - warn!( - "Filesystem full: cannot write index entry for {} v{}", - normalized_id, version - ); - // Clean up temp file - let _ = std::fs::remove_file(&temp_path); - return Err(ServiceError::Custom(format!( - "Filesystem full: cannot update index for {} v{}. Existing index preserved.", - normalized_id, version - ))); - } - return Err(ServiceError::Io(e)); - } + buffer.push_str(&line); + buffer.push('\n'); } - if let Err(e) = temp_file.sync_all() { - // Check if error is due to filesystem being full - let error_msg = e.to_string().to_lowercase(); - if error_msg.contains("no space") - || error_msg.contains("filesystem full") - || e.raw_os_error() == Some(28) - { - warn!( - "Filesystem full: cannot sync index file for {} v{}", - normalized_id, version - ); - // Clean up temp file - let _ = std::fs::remove_file(&temp_path); - return Err(ServiceError::Custom(format!( - "Filesystem full: cannot update index for {} v{}. Existing index preserved.", - normalized_id, version - ))); + // Step 7: Durably replace the index file — write to a unique temp, fsync, then + // atomically rename. A full filesystem only ever truncates the temp file, so the + // existing index is preserved on ENOSPC. + let temp_path = unique_temp_path(&index_path); + let on_write_err = |e: std::io::Error| { + let _ = std::fs::remove_file(&temp_path); + if is_disk_full(&e) { + warn!("Filesystem full: cannot update index for {normalized_id} v{version}"); + ServiceError::Custom(format!( + "Filesystem full: cannot update index for {normalized_id} v{version}. Existing index preserved." + )) + } else { + ServiceError::Io(e) } - return Err(ServiceError::Io(e)); - } + }; + + let mut temp_file = File::create(&temp_path).map_err(&on_write_err)?; + temp_file + .write_all(buffer.as_bytes()) + .map_err(&on_write_err)?; + temp_file.sync_all().map_err(&on_write_err)?; drop(temp_file); - // Step 8: Atomically rename temporary file to target + // Step 8: Atomically rename the temp file into place. std::fs::rename(&temp_path, &index_path).map_err(|e| { - warn!( - "Failed to atomically rename temp file {:?} to {:?}: {}", - temp_path, index_path, e - ); - // If rename fails, try to clean up temp file + warn!("Failed to rename temp file {temp_path:?} to {index_path:?}: {e}"); let _ = std::fs::remove_file(&temp_path); ServiceError::Io(e) })?; diff --git a/crates/fastskill-core/src/core/registry_index.rs b/crates/fastskill-core/src/core/registry_index.rs index 033cfe0c..bf58cd79 100644 --- a/crates/fastskill-core/src/core/registry_index.rs +++ b/crates/fastskill-core/src/core/registry_index.rs @@ -65,6 +65,22 @@ pub fn get_skill_index_path(registry_path: &Path, skill_id: &str) -> Result Result { + Ok(crate::utils::append_suffix( + &get_skill_index_path(registry_path, skill_id)?, + "lock", + )) +} + /// Update registry index with a new skill version /// Appends newline-delimited JSON to single file per skill (crates.io format) pub fn update_skill_version( diff --git a/crates/fastskill-core/src/utils.rs b/crates/fastskill-core/src/utils.rs index 76516f93..f3fcd145 100644 --- a/crates/fastskill-core/src/utils.rs +++ b/crates/fastskill-core/src/utils.rs @@ -3,14 +3,29 @@ use fs2::FileExt; use std::fs; use std::io; -use std::path::Path; +use std::path::{Path, PathBuf}; + +/// Append a suffix to a path's full file name — e.g. `org/web.scraper` + `tmp` +/// → `org/web.scraper.tmp`. +/// +/// Unlike [`Path::with_extension`], this does NOT replace the last dotted segment. +/// When a file name is derived from user input that may contain dots (e.g. a scoped +/// skill package), `with_extension("tmp")` is not injective — `org/web.scraper` and +/// `org/web.crawler` both collapse onto `org/web.tmp`. Appending keeps each derived +/// sidecar path distinct. +pub(crate) fn append_suffix(path: &Path, suffix: &str) -> PathBuf { + let mut name = path.as_os_str().to_os_string(); + name.push("."); + name.push(suffix); + PathBuf::from(name) +} /// Write `bytes` to `path` atomically: write to `.tmp` sibling → sync → rename. /// Advisory-locks the tmp file via `fs2` to prevent concurrent writers. /// /// Returns `LockError::FileLocked` if another process holds the advisory lock. pub(crate) fn atomic_write(path: &Path, bytes: &[u8]) -> io::Result<()> { - let tmp_path = path.with_extension("tmp"); + let tmp_path = append_suffix(path, "tmp"); // Ensure parent directory exists if let Some(parent) = path.parent() { @@ -84,7 +99,7 @@ mod tests { fn test_atomic_write_no_tmp_file_remains_on_success() { let tmp = TempDir::new().unwrap(); let path = tmp.path().join("test.toml"); - let tmp_path = path.with_extension("tmp"); + let tmp_path = append_suffix(&path, "tmp"); atomic_write(&path, b"data").unwrap(); @@ -104,7 +119,7 @@ mod tests { fs::write(&path, b"original content").unwrap(); // Simulate an interrupted write: write to .tmp, then leave it without renaming - let tmp_path = path.with_extension("tmp"); + let tmp_path = append_suffix(&path, "tmp"); fs::write(&tmp_path, b"incomplete write").unwrap(); // The original file should still have its original content @@ -127,7 +142,7 @@ mod tests { let tmp = TempDir::new().unwrap(); let path = tmp.path().join("skills.lock"); - let tmp_path = path.with_extension("tmp"); + let tmp_path = append_suffix(&path, "tmp"); // Acquire exclusive lock on the tmp file from this thread let holder = OpenOptions::new() diff --git a/tests/cli/registry_e2e_tests.rs b/tests/cli/registry_e2e_tests.rs index dd2eacd1..409819b5 100644 --- a/tests/cli/registry_e2e_tests.rs +++ b/tests/cli/registry_e2e_tests.rs @@ -458,7 +458,7 @@ async fn test_sources_test_connectivity() { return; }; Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string("[]")) .mount(&mock_server) .await; diff --git a/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__sources_test_unreachable.snap b/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__sources_test_unreachable.snap index 9177827c..3bcc083a 100644 --- a/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__sources_test_unreachable.snap +++ b/tests/cli/snapshots/cli_tests__cli__snapshot_helpers__sources_test_unreachable.snap @@ -15,4 +15,4 @@ FastSkill [VERSION] - 'sources refresh' → 'repos refresh' - 'sources create' → 'marketplace create' -Error: Configuration error: Repository 'unreachable-repo' test failed: Client error: HTTP request failed: error sending request for url (http://localhost:9999/api/v1/registry/index/skills): error trying to connect: tcp connect error: [NETWORK_ERROR] +Error: Configuration error: Repository 'unreachable-repo' test failed: Client error: HTTP request failed: error sending request for url (http://localhost:9999/api/registry/index/skills): error trying to connect: tcp connect error: [NETWORK_ERROR] diff --git a/tests/integration/registry_list_test.rs b/tests/integration/registry_list_test.rs index 69833089..1bc2d11e 100644 --- a/tests/integration/registry_list_test.rs +++ b/tests/integration/registry_list_test.rs @@ -107,7 +107,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { // Mock the endpoint Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -136,7 +136,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { let filtered_response = serde_json::to_string(&filtered_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("scope", "acme")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response)) .mount(&mock_server) @@ -155,7 +155,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { }; Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("all_versions", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response)) .mount(&mock_server) @@ -171,7 +171,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { }; Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("include_pre_release", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response)) .mount(&mock_server) @@ -195,7 +195,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -209,7 +209,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { // Test 401 Unauthorized (authentication required) Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(401)) .mount(&mock_server) .await; @@ -221,7 +221,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { // Test 403 Forbidden (access denied) Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(403)) .mount(&mock_server) .await; @@ -244,7 +244,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(wiremock::matchers::header("Authorization", "token test-token-123")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) @@ -330,7 +330,7 @@ async fn test_performance_benchmark_http_listing_1000_skills() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; diff --git a/tests/integration_registry_list_test.rs b/tests/integration_registry_list_test.rs index 17a9db65..da1064f4 100644 --- a/tests/integration_registry_list_test.rs +++ b/tests/integration_registry_list_test.rs @@ -109,7 +109,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { // Mock the endpoint Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -136,7 +136,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { let filtered_response = serde_json::to_string(&filtered_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("scope", "acme")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server2) @@ -162,7 +162,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { }; Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("all_versions", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server) @@ -178,7 +178,7 @@ async fn test_list_skills_cli_calling_registry_http_endpoint() { }; Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("include_pre_release", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server) @@ -202,7 +202,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -219,7 +219,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { return; }; Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(401)) .mount(&mock_server_401) .await; @@ -236,7 +236,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { return; }; Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(403)) .mount(&mock_server_403) .await; @@ -261,7 +261,7 @@ async fn test_unauthenticated_vs_authenticated_registry_listing() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(wiremock::matchers::header( "Authorization", "token test-token-123", @@ -354,7 +354,7 @@ async fn test_performance_benchmark_http_listing_1000_skills() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -431,7 +431,7 @@ async fn test_list_skills_scope_flag() { let filtered_response = serde_json::to_string(&filtered_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("scope", "acme")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server) @@ -631,7 +631,7 @@ async fn test_list_skills_json_flag() { let response_body = serde_json::to_string(&summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) .await; @@ -676,7 +676,7 @@ async fn test_list_skills_json_scope_flag() { let filtered_response = serde_json::to_string(&filtered_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("scope", "acme")) .respond_with(ResponseTemplate::new(200).set_body_string(filtered_response.clone())) .mount(&mock_server) @@ -767,7 +767,7 @@ async fn test_list_skills_all_versions_flag() { let response_body = serde_json::to_string(&all_versions_summaries).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("all_versions", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) @@ -833,7 +833,7 @@ async fn test_list_skills_all_versions_include_pre_release_json() { let response_body = serde_json::to_string(&summaries_with_prerelease).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("all_versions", "true")) .and(query_param("include_pre_release", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) @@ -890,7 +890,7 @@ async fn test_list_skills_all_versions_without_pre_release() { let response_body = serde_json::to_string(&summaries_without_prerelease).unwrap(); Mock::given(method("GET")) - .and(path("/api/v1/registry/index/skills")) + .and(path("/api/registry/index/skills")) .and(query_param("all_versions", "true")) .respond_with(ResponseTemplate::new(200).set_body_string(response_body)) .mount(&mock_server) diff --git a/tests/integration_registry_publish_test.rs b/tests/integration_registry_publish_test.rs index d70dac99..56797619 100644 --- a/tests/integration_registry_publish_test.rs +++ b/tests/integration_registry_publish_test.rs @@ -266,7 +266,7 @@ async fn test_verify_skill_in_registry() { // Check if skill is in registry via API let client = reqwest::Client::new(); - let url = format!("{}/api/v1/registry/skills", PRODUCTION_REGISTRY_URL); + let url = format!("{}/api/registry/skills", PRODUCTION_REGISTRY_URL); let response = client .get(&url) @@ -447,7 +447,7 @@ async fn test_complete_publish_workflow() { // Step 4: Verify via API println!("[4/4] Verifying skill in registry..."); let client = reqwest::Client::new(); - let url = format!("{}/api/v1/registry/skills", PRODUCTION_REGISTRY_URL); + let url = format!("{}/api/registry/skills", PRODUCTION_REGISTRY_URL); let response = client .get(&url)