diff --git a/java_codebase_rag/installer.py b/java_codebase_rag/installer.py index fc948e85..9bef0baf 100644 --- a/java_codebase_rag/installer.py +++ b/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,9 +1571,12 @@ 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, @@ -1571,5 +1584,6 @@ def run_install( quiet=quiet, verbose=verbose, ) - + if init_outcome is False: + return 1 return 0 diff --git a/tests/test_installer.py b/tests/test_installer.py index 15c3a426..de4296a0 100644 --- a/tests/test_installer.py +++ b/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()