fix: code-review findings — registry index lock/temp collision, publish auth, CI

.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

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,
     )

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()

crates/fastskill-core/src/core/registry/index_manager.rs

@@ -1,11 +1,14 @@
 //! 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};
 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 +32,30 @@ struct IndexFileMetadata {
     size: u64,
 }
 
+/// Derive a per-call unique temp path next to the index file (same directory, so the
+/// final rename stays atomic on one filesystem).
+///
+/// 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);
+    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 {
     /// Create a new IndexManager instance
     ///
@@ -107,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 = index_path.with_extension("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()
@@ -261,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 = index_path.with_extension("tmp");
-        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)
         })?;
@@ -399,9 +381,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 +397,10 @@ mod tests {
         }
     }
 
+    fn entry(version: &str) -> VersionEntry {
+        entry_named("testorg/test-skill", version)
+    }
+
     fn read_entries(registry_path: PathBuf) -> Vec<VersionEntry> {
         let index_path = get_skill_index_path(&registry_path, "testorg/test-skill").unwrap();
         IndexManager::read_entries_from_path(&index_path).unwrap()
@@ -468,4 +454,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(&registry_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");
+    }
 }

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<Path
     Ok(registry_path.join(&org).join(&package))
 }
 
+/// Path to the sidecar lock file guarding a skill's index file.
+///
+/// Derived by appending `.lock` to the index path (not [`Path::with_extension`], which
+/// would collapse dotted package names like `org/web.scraper` and `org/web.crawler`
+/// onto a single `org/web.lock`). Kept next to [`get_skill_index_path`] so all of a
+/// skill's on-disk paths are derived in one place.
+pub fn get_skill_index_lock_path(
+    registry_path: &Path,
+    skill_id: &str,
+) -> Result<PathBuf, ServiceError> {
+    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(

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()