fix(installer): failed index build no longer reports install success (#351) (#365)

* fix(installer): failed index build no longer reports install success (#351)

run_install discarded run_init_if_needed's return value and unconditionally
returned 0, so a cocoindex/graph non-zero exit reported a successful install
while the most important step failed silently. Distinguish failure from skip:
run_init_if_needed now returns None when the index already exists (skip) and
False only when indexing ran and failed; run_install returns 1 on that failure,
mirroring run_update's `if not index_ok: return 1` check.

Co-Authored-By: Claude <noreply@anthropic.com>

* test(installer): cover skip→None→exit-0; document #351 deferred scope

- Add test_install_over_existing_index_skips_init_and_exits_zero: a re-run
  of install over an existing index skips init and exits 0. Guards the None
  branch of run_init_if_needed (the PR's central design: `if init_outcome is
  False`), so a future `if not init_outcome` simplification that collapses
  None into the failure branch would fail this test instead of silently
  breaking idempotent re-runs. Verified red->green.
- Document at the partial-failure classifier that issue #351's "treat
  skill/agent deploy failures as critical" is intentionally deferred: only
  MCP config failures are critical (a broken MCP config is fatal; missing
  skill/agent hints are recoverable), so the deferred scope reads as a
  deliberate product decision rather than an oversight.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: HumanBean17

java_codebase_rag/installer.py

@@ -847,8 +847,8 @@ def run_init_if_needed(
     non_interactive: bool,
     quiet: bool,
     verbose: bool = False,
-) -> bool:
-    """Run init if index directory has no artifacts. Return True if init was run.
+) -> bool | None:
+    """Run init if index directory has no artifacts.
 
     The indexing sub-step (CocoIndex update + AST graph build) renders the
     unified ``Vectors → Optimize → Graph`` progress on **stderr** in default
@@ -867,7 +867,10 @@ def run_init_if_needed(
         verbose: If True, raw-relay subprocess output (no Live region)
 
     Returns:
-        True if init was run, False if skipped
+        True if init ran and succeeded; False if it ran and failed (cocoindex or
+        graph build returned non-zero); None if skipped because the index already
+        exists. Callers must distinguish ``False`` (failure) from ``None`` (skip)
+        so a failed index does not report success (issue #351).
     """
     from java_codebase_rag.config import (
         index_dir_has_existing_artifacts,
@@ -878,7 +881,7 @@ def run_init_if_needed(
     has_existing, _ = index_dir_has_existing_artifacts(index_dir)
     if has_existing:
         print("Index already exists. Run `java-codebase-rag reprocess` to rebuild.")
-        return False
+        return None  # skipped, not failed
 
     cfg = resolve_operator_config(
         source_root=source_root,
@@ -1531,6 +1534,13 @@ def run_install(
         print("Warning: Some artifacts failed to deploy:")
         for r in partial_failures:
             print(f"  {r.path}: {r.error}")
+        # Severity model: only MCP config (.json/.yml/.yaml) deploy failures are
+        # critical (return 1) -- a broken MCP config means the server cannot start.
+        # Skill/agent (.md / dir) failures are downgraded to non-critical: the
+        # server still runs and the affected host simply lacks those hints. Issue
+        # #351's "treat skill/agent deploy failures as critical for the affected
+        # host" is intentionally DEFERRED here -- promoting them to critical is a
+        # product decision (recoverable vs. fatal) best made explicitly, not bundled.
         if all(
             r.success
             for r in results
@@ -1561,15 +1571,19 @@ def run_install(
     if not quiet:
         print("Configuration written to", config_path)
 
-    # Run init if index directory is empty
+    # Run init if index directory is empty. run_init_if_needed returns True (ran
+    # OK), False (ran and failed — cocoindex/graph non-zero exit), or None
+    # (skipped: index already exists). A failed index must NOT report success in
+    # CI/automation; a skip is not a failure (issue #351).
     index_dir = (source_root / ".java-codebase-rag").resolve()
-    run_init_if_needed(
+    init_outcome = run_init_if_needed(
         source_root,
         index_dir,
         resolved_model,
         non_interactive=non_interactive,
         quiet=quiet,
         verbose=verbose,
     )
-
+    if init_outcome is False:
+        return 1
     return 0

tests/test_installer.py

@@ -1693,3 +1693,87 @@ def boom(env, *, full_reprocess, quiet, verbose=True,
         # The footer rendered the failure marker (red cross), not the green check.
         assert cli_format.styled_cross() in err_text
         assert cli_format.styled_check() not in err_text
+
+    def test_install_indexing_failure_returns_nonzero(self, tmp_path, monkeypatch):
+        """A non-exception indexing failure (cocoindex exits non-zero) must NOT
+        report install success. Regression for issue #351: run_install discarded
+        run_init_if_needed's return value and unconditionally returned 0, so a
+        broken or empty index reported exit 0 in CI/automation while the most
+        important install step failed silently. (The exception path was already
+        covered; this covers the returncode != 0 path.)"""
+        import io
+        import contextlib
+        import subprocess
+        from java_codebase_rag.installer import run_install
+
+        cwd = self._setup_repo(tmp_path, monkeypatch)
+
+        def failing_coco(env, *, full_reprocess, quiet, verbose=True,
+                         lance_project_root=None, on_progress=None, on_progress_console=None):
+            return subprocess.CompletedProcess(args=["stub"], returncode=1, stdout="", stderr="boom")
+
+        monkeypatch.setattr("java_codebase_rag.pipeline.run_cocoindex_update", failing_coco)
+
+        out, err = io.StringIO(), io.StringIO()
+        with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err):
+            rc = run_install(
+                non_interactive=True,
+                agents=["claude-code"],
+                scope="project",
+                model="auto",
+                source_root=cwd,
+                quiet=False,
+            )
+        assert rc == 1, (
+            f"install reported success (exit {rc}) despite cocoindex failure (#351)"
+        )
+        # The failure was surfaced on stderr, not swallowed.
+        assert "CocoIndex update failed" in err.getvalue()
+
+    def test_install_over_existing_index_skips_init_and_exits_zero(self, tmp_path, monkeypatch):
+        """A re-run of install over an existing index skips init (the index build)
+        and still exits 0. Regression guard for the None branch of
+        run_init_if_needed (issue #351): run_install uses ``if init_outcome is
+        False: return 1`` precisely so a SKIP (None) stays exit 0 -- a future
+        ``if not init_outcome`` simplification would collapse None into the
+        failure branch and break idempotent re-runs in CI/automation."""
+        import io
+        import contextlib
+        import subprocess
+        from java_codebase_rag.installer import run_install
+
+        cwd = self._setup_repo(tmp_path, monkeypatch)
+
+        # Pre-create an existing index so index_dir_has_existing_artifacts is True
+        # -> run_init_if_needed returns None (skip), not True/False.
+        index_dir = cwd / ".java-codebase-rag"
+        index_dir.mkdir()
+        (index_dir / "code_graph.lbug").write_bytes(b"\x00" * 16)
+
+        coco_called = []
+
+        def coco_should_not_run(env, *, full_reprocess, quiet, verbose=True,
+                                lance_project_root=None, on_progress=None, on_progress_console=None):
+            coco_called.append(True)
+            return subprocess.CompletedProcess(args=["stub"], returncode=0, stdout="", stderr="")
+
+        monkeypatch.setattr("java_codebase_rag.pipeline.run_cocoindex_update", coco_should_not_run)
+
+        out, err = io.StringIO(), io.StringIO()
+        with contextlib.redirect_stdout(out), contextlib.redirect_stderr(err):
+            rc = run_install(
+                non_interactive=True,
+                agents=["claude-code"],
+                scope="project",
+                model="auto",
+                source_root=cwd,
+                quiet=False,
+            )
+        assert rc == 0, (
+            f"install over an existing index should skip init and exit 0, got exit {rc} "
+            "(#351 None branch: a skip must not be treated as failure)"
+        )
+        # init was genuinely skipped, not silently succeeded.
+        assert not coco_called, "init should have been SKIPPED (index exists) but cocoindex ran"
+        # run_init_if_needed prints the skip notice to stdout (no file=sys.stderr).
+        assert "Index already exists" in out.getvalue()