[adapter,hparams] fix: address Copilot review on PR X-GenGroup#160

Three fixes from the Copilot review on the upstream PR:

1. Path-traversal validation (utils/checkpoint.py):
   parse_hf_checkpoint_path now rejects '.', '..', and backslash
   segments with an informative ValueError that preserves the original
   spec. Validates at the parser front door rather than at the
   download site so error messages keep the user's exact input.
   Without this, a spec like 'owner/repo/..' would escape the snapshot
   directory via os.path.join.

2. Un-gate _resolve_checkpoint_path (models/abc.py):
   Remove the `if is_local_main_process:` gate and the post-barrier
   double-call pattern. All ranks now call snapshot_download directly;
   huggingface_hub's per-blob WeakFileLock serializes concurrent calls
   within each filesystem domain (cross-node on POSIX-locking shared
   FS, per-node on non-shared FS), so we still get exactly one
   download per filesystem domain.

   This eliminates the distributed-deadlock hazard where a download
   failure on the gated rank would raise before reaching
   wait_for_everyone(), leaving siblings blocked at the barrier until
   NCCL watchdog timeout. The trailing wait_for_everyone() is kept to
   maintain lockstep entry into the downstream loaders.

   Residual asymmetric-failure risk (one rank's network blip while
   others succeed) is documented in the docstring.

3. Skill-checklist alignment (.agents/skills/ff-review/SKILL.md):
   Replace the duplicated import-exception list with a reference to
   constraint X-GenGroup#22, where the full set of three sanctioned exceptions
   (optional deps, backend-gated runtime feature checks, circular
   imports) lives. Prevents future drift between the two documents.

Verified with 8 happy-path + 5 original-error + 6 path-traversal
parser test cases (all pass).

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: Jayce-Ping

.agents/skills/ff-review/SKILL.md

@@ -60,7 +60,7 @@ git status             # Modified files
 - [ ] English comments and docstrings
 - [ ] Apache 2.0 license header on new files
 - [ ] No unnecessary wildcard imports (except `hparams`)
-- [ ] **Top-level imports only** — no `import` / `from ... import ...` inside function or method bodies (constraint #22). Exceptions: documented optional deps via `try/except ImportError`, or genuine unresolvable circular imports.
+- [ ] **Top-level imports only** (constraint #22) — see that file for the three sanctioned exceptions (optional deps via `try/except ImportError`, backend-gated runtime feature checks like DeepSpeed/FSDP, unresolvable circular imports).
 
 ### Documentation
 - [ ] `guidance/` docs updated if behavior changed

src/flow_factory/models/abc.py

@@ -1470,11 +1470,17 @@ def _resolve_checkpoint_path(self, path: str) -> str:
             3. Otherwise, parse as ``owner/repo[/subfolder][@revision]`` and download
                via Hugging Face Hub.
 
-        Multi-node-safe: the download is gated on ``is_local_main_process`` (one
-        process per node), not ``is_main_process`` (one global). This populates the
-        per-node HF cache exactly once on non-shared filesystems; on shared
-        filesystems, ``huggingface_hub``'s per-blob ``WeakFileLock`` dedupes the
-        concurrent ``snapshot_download`` calls so only one node transfers bytes.
+        Multi-node-safe: all ranks call ``snapshot_download`` directly. Hugging
+        Face Hub's per-blob ``WeakFileLock`` serializes concurrent calls within
+        each filesystem domain (cross-node on POSIX-locking shared FS, per-node
+        on non-shared FS), so exactly one rank per filesystem domain actually
+        transfers bytes. Un-gated (rather than ``is_local_main_process`` plus a
+        barrier) so a failed download raises uniformly on every affected rank
+        instead of leaving siblings deadlocked at a barrier the failing rank
+        never reaches. Residual hazard: a rare single-rank transient failure
+        (e.g. one node's network blip) can produce asymmetric progress, in
+        which case the surviving ranks will eventually trip the NCCL watchdog
+        on the final barrier below.
 
         Args:
             path: Local filesystem path or HF spec (with or without ``hf://`` prefix).
@@ -1493,19 +1499,8 @@ def _resolve_checkpoint_path(self, path: str) -> str:
 
         repo_id, subfolder, revision = parse_hf_checkpoint_path(spec)
 
-        if self.accelerator.is_local_main_process:
-            local_path = download_hf_checkpoint(repo_id, subfolder, revision)
-            logger.info(
-                f"[local rank 0 / global rank {self.accelerator.process_index}] "
-                f"resolved checkpoint '{path}' -> {local_path}"
-            )
-        self.accelerator.wait_for_everyone()
-
-        # All ranks call again; on the populated cache this is a metadata-only
-        # path lookup. Narrow re-raise for the specific HF-Hub failure modes so
-        # users see a single actionable message instead of a raw HTTPError.
         try:
-            return download_hf_checkpoint(repo_id, subfolder, revision)
+            local_path = download_hf_checkpoint(repo_id, subfolder, revision)
         except (RepositoryNotFoundError, HfHubHTTPError) as e:
             raise FileNotFoundError(
                 f"Checkpoint {path!r} not found locally and could not be fetched "
@@ -1514,6 +1509,20 @@ def _resolve_checkpoint_path(self, path: str) -> str:
                 f"on ALL nodes."
             ) from e
 
+        # Sync after download so downstream loaders enter the lockstep dispatch
+        # together. On symmetric failure every rank raises above before this
+        # barrier is reached, so no deadlock; the residual asymmetric-failure
+        # case is documented in the docstring.
+        self.accelerator.wait_for_everyone()
+
+        if self.accelerator.is_local_main_process:
+            logger.info(
+                f"[local rank 0 / global rank {self.accelerator.process_index}] "
+                f"resolved checkpoint '{path}' -> {local_path}"
+            )
+
+        return local_path
+
     @staticmethod
     def load_sharded_checkpoint(checkpoint_dir: str, index_file: str) -> Dict[str, torch.Tensor]:
         """Load sharded safetensors checkpoint."""

src/flow_factory/utils/checkpoint.py

@@ -189,6 +189,17 @@ def parse_hf_checkpoint_path(path: str) -> Tuple[str, Optional[str], Optional[st
             f"(expected at least 'owner/repo', got {len(parts)} non-empty segments)"
         )
 
+    # Reject path-traversal segments. Without this, a spec like
+    # 'owner/repo/..' would resolve via os.path.join to a directory outside
+    # the snapshot root and let downstream loaders read from unintended
+    # locations. Backslashes are rejected to block Windows-style traversal.
+    for seg in parts:
+        if seg in (".", "..") or "\\" in seg:
+            raise ValueError(
+                f"invalid segment {seg!r} in HF checkpoint path: {path!r} "
+                f"('.', '..', and backslashes are not allowed)"
+            )
+
     repo_id = "/".join(parts[:2])
     subfolder = "/".join(parts[2:]) if len(parts) > 2 else None
     return repo_id, subfolder, revision