ufs-runner-mcp: fix path sandbox symlink regression, override warning, and output dedup (#101)

* ufs-runner-mcp: fix path sandbox, honest overrides, output dedup, async blocking

The path sandbox rejected every real RDHPCS mount (/scratch3, /work2/noaa,
...) since only the exact unnumbered prefix passed is_relative_to; fix by
allowing a digit suffix on the path component right after root while still
rejecting siblings like /work-attacker or /workfoo.

ufs_create_experiment documented flat overrides (dt_ocean, rnday, ibc, ...)
that have no matching {{placeholder}} in the schism_sandy_duck template, so
they were silently dropped while the tool still reported success; now
unmatched flat/namelist-group overrides and namelist write failures are
tracked and surfaced as an explicit warning section in the tool output.

collect_outputs globbed slurm-*.out separately even though the existing
"*.out" pattern already matches it, double-counting every Slurm log; dedupe
by resolved path instead.

All async tools called blocking UfsRunner methods (sbatch/sacct/scancel
subprocess calls, shutil.copytree, read_text/write_text, rglob) directly,
stalling the event loop for up to 30s per Slurm call; wrap those calls in
asyncio.to_thread at the tool layer.

* ufs-runner-mcp: fix review findings in path sandbox, overrides, dedup

Path sandbox: _prefix_matches resolved the input path but compared it
against the raw-string prefix, so a symlinked allowed root (common on
HPC sites, e.g. /work -> /lustre/work) got rejected — the same class of
"rejects real HPC mounts" bug the previous commit was meant to fix,
reintroduced via the digit-suffix matching logic. Now try two checks:
numbered-mount leniency on the raw prefix (unchanged, so /scratch3 style
siblings still work even if the base prefix is itself a symlink) and
symlink-transparent containment on the resolved prefix (fixes the
regression, and also resolves a relative env-supplied prefix against cwd
instead of silently no-op'ing). A configured prefix that resolves to just
"/" is now dropped with a warning instead of silently doing nothing or
becoming an accidental allow-all.

Override honesty: unmatched_flat_keys was computed against only the keys
that hit a literal {{placeholder}}, so overriding ocn_tasks/atm_tasks —
which drive OCN_petlist_bounds and total_tasks through
_compute_derived_vars without ever appearing as a placeholder themselves
— was falsely reported as having no effect, contradicting the feature's
own intent. Seed the matched-keys set with what _compute_derived_vars
actually consumes.

collect_outputs dedup: keying on the resolved path collapsed distinct,
both-meaningful directory entries that happen to point at the same file
(e.g. latest.nc -> history.nc), silently dropping one from the listing.
Key on the path relative to run_dir instead — this still catches the
original slurm-*.out double-count (same directory entry matched twice)
without merging legitimate symlinks.

ufs_list_templates still did blocking filesystem I/O directly in an
async function, unlike every other tool in this file. Move the logic
into a UfsRunner.list_templates method and call it via asyncio.to_thread,
matching the existing pattern.

Each fix was reproduced against the pre-fix code and reverified after;
6 new tests cover the symlink/relative/root-prefix path cases, the
ocn_tasks-alone override, and the symlinked-output listing.

Author: mansurjisan

servers/ufs-runner-mcp/src/ufs_runner_mcp/models.py

@@ -2,11 +2,14 @@
 
 from __future__ import annotations
 
+import logging
 import os
 import re
 from enum import Enum
 from pathlib import Path
 
+logger = logging.getLogger(__name__)
+
 
 class ModelType(str, Enum):
     """Supported UFS-Coastal model configurations."""
@@ -25,27 +28,115 @@ class ModelType(str, Enum):
 
 
 def get_allowed_prefixes() -> list[str]:
-    """Return allowed path prefixes, including any from UFS_RUNNER_ALLOWED_PATHS."""
+    """Return allowed path prefixes, including any from UFS_RUNNER_ALLOWED_PATHS.
+
+    A configured entry that resolves down to just the filesystem root (e.g.
+    a literal "/", or a relative value whose parent chain bottoms out there)
+    would match *every* absolute path via the symlink-transparent containment
+    check in _prefix_matches — silently disabling the sandbox instead of
+    adding a narrower allowance. That's essentially never the intent (no
+    admin means to allow the whole filesystem by setting this), so such an
+    entry is dropped with a loud warning rather than silently honored or
+    silently ignored.
+    """
     extra = os.environ.get("UFS_RUNNER_ALLOWED_PATHS", "")
     prefixes = list(_ALLOWED_PATH_PREFIXES)
     if extra.strip():
-        prefixes.extend(p.strip() for p in extra.split(":") if p.strip())
+        for raw in extra.split(":"):
+            candidate = raw.strip()
+            if not candidate:
+                continue
+            resolved_candidate = Path(candidate).resolve()
+            if len(resolved_candidate.parts) < 2:
+                logger.warning(
+                    "UFS_RUNNER_ALLOWED_PATHS entry %r resolves to the "
+                    "filesystem root (%s); ignoring it instead of allowing "
+                    "every path. Configure a specific subdirectory instead.",
+                    candidate,
+                    resolved_candidate,
+                )
+                continue
+            prefixes.append(candidate)
     return prefixes
 
 
+def _prefix_matches(resolved: Path, prefix: str) -> bool:
+    """Check whether *resolved* falls under *prefix*.
+
+    Two independent checks are tried; either is sufficient:
+
+    1. Numbered-mount leniency, on the RAW (unresolved) prefix. Real RDHPCS
+       mounts are numbered (/scratch3, /scratch4, /work2/noaa, ...), so a
+       plain is_relative_to(prefix) check (or the raw-string-prefix check it
+       replaced) rejects every genuine mount except the exact, un-numbered
+       name. Only the path *component immediately after the filesystem root*
+       is given digit-suffix leniency — e.g. prefix "/scratch" matches
+       resolved component "scratch5" — via re.fullmatch on that single
+       component, so "work-attacker" / "workshop" / "scratchpad-evil" still
+       fail (they are not the base name plus only digits). Any deeper
+       components of the prefix (e.g. the "noaa" in a hypothetical
+       "/work/noaa") must still match exactly, same as the original
+       is_relative_to check.
+
+       This is deliberately checked on the prefix's raw string components,
+       not resolved ones: numbered siblings (e.g. /work2) are separate
+       mounts, not reachable through whatever symlink /work itself might be
+       on a given site, so resolving the prefix first would compare against
+       the symlink's *target* name instead of "work" and silently break this
+       leniency.
+
+    2. Symlink-transparent containment, on the RESOLVED prefix. Many real
+       HPC sites symlink an allowed root itself (e.g. /work -> /lustre/work,
+       /scratch -> /gpfs/scratch, or a custom env-configured prefix pointing
+       at a symlink). The input path is already resolved by the caller, so
+       also resolve the prefix and check plain containment in that resolved
+       space — the same principle as the original pre-numbered-mount code
+       (is_relative_to), which resolved both sides. This also transparently
+       handles a *relative* env-supplied prefix, since Path.resolve() anchors
+       it against the current working directory instead of leaving it as a
+       silent no-op. A prefix that resolves to just the filesystem root is
+       never treated as a match here (see get_allowed_prefixes).
+    """
+    resolved_parts = resolved.parts
+
+    # 1. numbered-mount leniency, compared on the raw prefix
+    raw_prefix_parts = Path(prefix).parts
+    if len(raw_prefix_parts) >= 2 and len(resolved_parts) >= 2:
+        base_name = raw_prefix_parts[1]
+        pattern = re.compile(rf"^{re.escape(base_name)}\d*$")
+        if pattern.fullmatch(resolved_parts[1]):
+            remaining_prefix_parts = raw_prefix_parts[2:]
+            candidate = resolved_parts[2 : 2 + len(remaining_prefix_parts)]
+            if not remaining_prefix_parts or list(candidate) == list(
+                remaining_prefix_parts
+            ):
+                return True
+
+    # 2. symlink-transparent containment, compared on the resolved prefix
+    resolved_prefix_parts = Path(prefix).resolve().parts
+    if len(resolved_prefix_parts) >= 2 and len(resolved_parts) >= len(
+        resolved_prefix_parts
+    ):
+        if resolved_parts[: len(resolved_prefix_parts)] == resolved_prefix_parts:
+            return True
+
+    return False
+
+
 def validate_path(path: str, label: str = "path") -> str | None:
     """Validate that *path* is under an allowed path prefix.
 
     Returns None if valid, or an error message if not.
     """
     # Compare on path-component boundaries, not raw string prefix:
     # str.startswith("/work") wrongly accepted "/work-attacker", "/workshop",
-    # "/scratchpad-evil" — a full sandbox escape. is_relative_to() (which is
-    # true for an exact match too) only accepts genuine descendants.
+    # "/scratchpad-evil" — a full sandbox escape. _prefix_matches (like
+    # is_relative_to, which it replaces) only accepts genuine descendants,
+    # while still tolerating numbered HPC mounts (see its docstring).
     resolved = Path(path).resolve()
     prefixes = get_allowed_prefixes()
     for prefix in prefixes:
-        if resolved.is_relative_to(Path(prefix).resolve()):
+        if _prefix_matches(resolved, prefix):
             return None
     allowed = ", ".join(prefixes)
     return (