Merge pull request #28 from alfredolopez80/fix/ci-pytest-fail-loud

fix(ci): make pytest tests/ fail-loud (remove || true)

Author: alfredolopez80

.claude/hooks/lib/worktree-utils.sh

@@ -207,12 +207,45 @@ checkWorktreeTTL() {
     return 1
   fi
 
-  # Get creation time from worktree directory metadata
-  local created_epoch
-  created_epoch=$(stat -f "%B" "$wt_dir" 2>/dev/null || stat -c "%W" "$wt_dir" 2>/dev/null || echo "0")
+  # Get creation time from worktree directory metadata.
+  #
+  # Cross-platform AND CI-safe:
+  #   * macOS/BSD stat uses `-f <fmt>`: %B = birth (create) time, %m = mtime.
+  #   * GNU/Linux stat uses `-c <fmt>`: %W = birth time, %Y = mtime.
+  #
+  # Two failures this guards against (both seen on Ubuntu CI, never on macOS):
+  #   1. The old `stat -f "%B"` was tried FIRST on Linux, where `-f` means
+  #      "filesystem mode" — GNU stat then printed multi-line, NON-NUMERIC
+  #      filesystem info into created_epoch, and `$(( now - created_epoch ))`
+  #      aborted the function under `set -uo pipefail` -> EMPTY stdout ->
+  #      JSONDecodeError in the tests.
+  #   2. Many Linux filesystems (ext4/overlayfs on CI) report birth time as 0
+  #      (`%W` == 0). created_epoch=0 made a FRESH worktree look ~29M minutes
+  #      old -> expired:true. We fall back to mtime when birth time is 0/empty.
+  local created_epoch=""
+  if stat -c '%W' / >/dev/null 2>&1; then
+    # GNU/Linux stat
+    created_epoch=$(stat -c '%W' "$wt_dir" 2>/dev/null || echo "")
+    if [[ -z "$created_epoch" || ! "$created_epoch" =~ ^[0-9]+$ || "$created_epoch" -eq 0 ]]; then
+      # Birth time unavailable on this fs -> use mtime (always populated).
+      created_epoch=$(stat -c '%Y' "$wt_dir" 2>/dev/null || echo "")
+    fi
+  else
+    # macOS/BSD stat
+    created_epoch=$(stat -f '%B' "$wt_dir" 2>/dev/null || echo "")
+    if [[ -z "$created_epoch" || ! "$created_epoch" =~ ^[0-9]+$ || "$created_epoch" -eq 0 ]]; then
+      created_epoch=$(stat -f '%m' "$wt_dir" 2>/dev/null || echo "")
+    fi
+  fi
+  # Final guard: never let a non-numeric value reach the arithmetic below
+  # (a non-numeric $(( )) aborts the function under set -e and yields empty stdout).
+  [[ "$created_epoch" =~ ^[0-9]+$ ]] || created_epoch=$(date +%s)
+
   local now_epoch
   now_epoch=$(date +%s)
   local elapsed_minutes=$(( (now_epoch - created_epoch) / 60 ))
+  # Clamp negative drift (clock skew between birth time and now) to 0.
+  (( elapsed_minutes < 0 )) && elapsed_minutes=0
   local expired="false"
 
   if [[ "$elapsed_minutes" -ge "$ttl_minutes" ]]; then

.claude/hooks/session-end-handoff.sh

@@ -304,7 +304,7 @@ gc_cleanup() {
             member_count=$(find "$team_dir" -type f -name "member-*.json" 2>/dev/null | wc -l | tr -d ' ')
             if [[ "$member_count" -eq 0 ]]; then
                 rm -rf "$team_dir" 2>/dev/null && {
-                    ((cleaned_teams++))
+                    cleaned_teams=$((cleaned_teams + 1))
                     log "GC: Removed empty team dir: $team_dir" >> "$gc_log"
                 }
             fi
@@ -321,7 +321,7 @@ gc_cleanup() {
             dir_date=$(basename "$task_dir" | grep -oE '[0-9]{8}' || echo "")
             if [[ -n "$dir_date" ]] && [[ "$dir_date" -lt "$cutoff_date" ]]; then
                 rm -rf "$task_dir" 2>/dev/null && {
-                    ((cleaned_tasks++))
+                    cleaned_tasks=$((cleaned_tasks + 1))
                     log "GC: Removed old task dir: $task_dir" >> "$gc_log"
                 }
             fi
@@ -338,7 +338,7 @@ gc_cleanup() {
                 sort -n | head -n "$to_delete" | cut -d' ' -f2- | \
                 while IFS= read -r file; do
                     rm -f "$file" 2>/dev/null && {
-                        ((cleaned_handoffs++))
+                        cleaned_handoffs=$((cleaned_handoffs + 1))
                         log "GC: Removed old handoff: $file" >> "$gc_log"
                     }
                 done

.claude/hooks/vault-writeback.sh

@@ -124,7 +124,13 @@ for i in $(seq 0 $((QUEUE_LENGTH - 1)) 2>/dev/null); do
     fi
 
     # Sanitize summary (no secrets, no absolute paths)
-    SAFE_SUMMARY=$(echo "$SUMMARY" | sed "s|${HOME}/|~/|g" | tr -cd ' a-zA-Z0-9_.:/-()[]{}?!,;' | head -c 2000)
+    # NOTE: the literal '-' MUST be last in the tr set. Inside '...:/-()...' the
+    # substring '/-(' is parsed by GNU tr (Linux/CI) as a range from '/' (0x2F)
+    # to '(' (0x28) — a reversed range — and GNU tr aborts with a non-zero exit
+    # ("range-endpoints ... in reverse collating sequence order"). Under
+    # `set -euo pipefail` that killed the whole hook (rc=1) on Ubuntu while BSD
+    # tr on macOS tolerated it. Trailing '-' is a literal dash on both.
+    SAFE_SUMMARY=$(echo "$SUMMARY" | sed "s|${HOME}/|~/|g" | tr -cd ' a-zA-Z0-9_.:/()[]{}?!,;-' | head -c 2000)
 
     # Create draft wiki article
     NOW=$(date -u +"%Y-%m-%dT%H:%M:%SZ")

.github/workflows/ci.yml

@@ -90,7 +90,7 @@ jobs:
       - name: Run pytest
         run: |
           if [ -d "tests" ]; then
-            python -m pytest tests/ -v --tb=short || true
+            python -m pytest tests/ -v --tb=short
           fi
 
       - name: Run harness tests

tests/conftest.py

@@ -6,6 +6,7 @@
 import sys
 import tempfile
 import shutil
+from pathlib import Path
 import pytest
 
 # Add project root to path
@@ -292,3 +293,79 @@ def _validate(skill_path: str) -> dict:
         return result
 
     return _validate
+
+
+# ============================================================
+# CI-safe isolation fixtures (PR #28: make tests/ fail-loud-able)
+# ============================================================
+
+@pytest.fixture
+def isolated_home(tmp_path, monkeypatch):
+    """Isolate $HOME to a temp dir seeded with the minimal Ralph layout.
+
+    Redirects BOTH ``$HOME`` (for shell hooks that read ``${HOME}``) and
+    ``Path.home()`` (for Python code), so a test never touches the developer's
+    real ~/.claude or ~/.ralph. The repo's hooks are symlinked in so they
+    resolve to the real, versioned scripts under the temp HOME.
+    """
+    import json
+
+    home = tmp_path / "home"
+    for sub in (
+        ".claude/state", ".claude/teams", ".claude/tasks", ".claude/skills", ".claude/scripts",
+        ".ralph/logs", ".ralph/state", ".ralph/handoffs", ".ralph/ledgers",
+        ".ralph/config", ".ralph/temp", ".ralph/markers",
+    ):
+        (home / sub).mkdir(parents=True, exist_ok=True)
+
+    # Point ~/.claude/hooks at the repo's real hooks (no duplication, no dev HOME).
+    repo_hooks = Path(PROJECT_ROOT) / ".claude" / "hooks"
+    hooks_link = home / ".claude" / "hooks"
+    if repo_hooks.is_dir() and not hooks_link.exists():
+        hooks_link.symlink_to(repo_hooks)
+
+    # Minimal valid config so settings/feature readers find a non-empty file.
+    (home / ".claude" / "settings.json").write_text(
+        json.dumps({"hooks": {}}), encoding="utf-8"
+    )
+    (home / ".ralph" / "config" / "features.json").write_text(
+        json.dumps({"RALPH_ENABLE_HANDOFF": True}), encoding="utf-8"
+    )
+
+    monkeypatch.setenv("HOME", str(home))
+    monkeypatch.setattr(Path, "home", lambda: home)
+    return home
+
+
+@pytest.fixture
+def requires_tool():
+    """Return a helper that skips the test when a named CLI tool is absent.
+
+    Usage: ``def test_x(requires_tool): requires_tool("rsync")``.
+    """
+    def _require(name):
+        if shutil.which(name) is None:
+            pytest.skip(f"required tool not available on PATH: {name}")
+    return _require
+
+
+@pytest.fixture
+def git_repo(tmp_path):
+    """Create a throwaway git repo for hook/worktree tests (skips if git absent)."""
+    import subprocess
+
+    if shutil.which("git") is None:
+        pytest.skip("git not available on PATH")
+
+    repo = tmp_path / "repo"
+    repo.mkdir()
+    env = {**os.environ, "GIT_CONFIG_GLOBAL": os.devnull, "GIT_CONFIG_SYSTEM": os.devnull}
+    run = lambda *a: subprocess.run(a, cwd=repo, check=True, env=env,
+                                    capture_output=True, text=True)
+    run("git", "init", "-q")
+    run("git", "config", "user.email", "test@example.com")
+    run("git", "config", "user.name", "ralph-test")
+    (repo / "README.md").write_text("test\n", encoding="utf-8")
+    run("git", "add", "-A")
+    run("git", "commit", "-qm", "init")
+    return repo

tests/layers/test_layer_stack.py

@@ -97,12 +97,20 @@ def sample_rules_json(tmp_path: Path) -> Path:
 class TestLayer0:
     """Tests for the identity layer (L0)."""
 
-    def test_exists_returns_true_for_real_file(self, real_l0_path: Path):
-        """L0.exists() returns True when the real identity file exists."""
-        layer = Layer0(path=real_l0_path)
+    def test_exists_returns_true_for_real_file(self, tmp_path: Path):
+        """L0.exists() returns True when an identity file is present.
+
+        CI-safe: build the artifact in tmp_path instead of depending on the
+        developer's ``~/.ralph/layers/L0_identity.md`` (absent on CI runners).
+        """
+        l0_path = tmp_path / "L0_identity.md"
+        l0_path.write_text(
+            "# Ralph Identity\n\n## Principles\nBuilt for the test.\n",
+            encoding="utf-8",
+        )
+        layer = Layer0(path=l0_path)
         assert layer.exists() is True, (
-            f"L0 identity file missing at {real_l0_path}. "
-            "Run W2.2 setup to create ~/.ralph/layers/L0_identity.md"
+            f"L0 identity file should exist after writing it at {l0_path}"
         )
 
     def test_exists_returns_false_for_missing_file(self, tmp_layers_dir: Path):
@@ -230,12 +238,21 @@ def test_load_raises_when_file_missing(self, tmp_layers_dir: Path):
         with pytest.raises(FileNotFoundError):
             layer.load()
 
-    def test_real_l1_exists(self, real_l1_path: Path):
-        """Real L1_essential.md was built by W2.2 setup."""
-        layer = Layer1(path=real_l1_path)
+    def test_real_l1_exists(self, tmp_path: Path, sample_rules_json: Path, monkeypatch):
+        """Layer1.build() produces an L1_essential.md that exists().
+
+        CI-safe: build the artifact in tmp_path from the sample rules fixture
+        (same pattern as test_build_creates_md_file) instead of asserting on the
+        developer's ``~/.ralph/layers/L1_essential.md`` (absent on CI runners).
+        """
+        l1_path = tmp_path / "L1_essential.md"
+        _patch_rules_json(monkeypatch, sample_rules_json)
+
+        layer = Layer1(path=l1_path, rule_count=15)
+        layer.build()
+
         assert layer.exists() is True, (
-            f"L1 file missing at {real_l1_path}. "
-            "Run: python3 .claude/lib/layers.py --build-l1"
+            f"L1 file should exist after build() at {l1_path}"
         )
 
     def test_real_l1_has_substantive_rules(self, real_l1_path: Path):