diff --git a/CHANGELOG.md b/CHANGELOG.md index dcc82ec18..7f2335fcc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Fixed +- `kitaru setup` is now safe to re-run against Claude Code. It verifies the whole registered launch (command, `--server`, and `--mode`) when it reads the entry back, so a stale entry in another scope or a failed readback is reported instead of passing as success, and it restores the previous `kitaru` entry when the replacement cannot be added. A project whose uv environment is set through `UV_PROJECT_ENVIRONMENT` is now recognized as a project install, so Cursor gets the project file rather than the global one. A skill whose final rename fails is put back in place instead of being left in a hidden retired directory. - Analyzer tasks that return an empty list now complete successfully without creating insights, so an analysis with no eligible findings does not fail its import job. Insight editor copy containing Markdown formatting falls back to deterministic plain text. - Worker-only installations now include the packaging dependency, and `kitaru importer test` accepts importer objects exposing `parse` and `fetch`. - `kitaru doctor` no longer prints "Kitaru is needs attention", and its missing-skills hint points at `kitaru setup`. The one-line installer prints `uvx kitaru ...` for its next steps when the tool directory is not on the current shell's PATH yet, so they work without opening a new terminal. diff --git a/src/kitaru/cli/setup.py b/src/kitaru/cli/setup.py index 43698405d..35a5b765c 100644 --- a/src/kitaru/cli/setup.py +++ b/src/kitaru/cli/setup.py @@ -264,20 +264,22 @@ def _scope_only(cwd: Path) -> Literal["project", "user"]: def _find_project_dir(prefix: Path, cwd: Path) -> Path | None: - """Return the project a virtual environment belongs to, if any.""" - if prefix.name != ".venv": - return None - project = prefix.parent - if not (project / "pyproject.toml").is_file(): - return None - # The working directory must be inside the project, otherwise a setup - # run from elsewhere would register a project-scoped entry in the wrong - # place. - try: - cwd.relative_to(project) - except ValueError: - return None - return project + """Return the project whose uv environment is ``prefix``, if any. + + The environment is ``/.venv`` unless ``UV_PROJECT_ENVIRONMENT`` + points elsewhere; uv resolves a relative value against the project root. + """ + environment = os.environ.get("UV_PROJECT_ENVIRONMENT") or ".venv" + resolved_prefix = prefix.resolve() + # Walk up from the working directory rather than down from the prefix, so + # a setup run from outside the project never registers a project-scoped + # entry in the wrong place. + for candidate in (cwd, *cwd.parents): + if not (candidate / "pyproject.toml").is_file(): + continue + if (candidate / environment).resolve() == resolved_prefix: + return candidate + return None def _find_sibling_executable(name: str) -> Path | None: @@ -408,7 +410,13 @@ def _write_skills(destination: Path, skills: dict[str, dict[str, bytes]]) -> Non ) retired.rmdir() os.replace(target, retired) - os.replace(staging, target) + try: + os.replace(staging, target) + except BaseException: + # Put the previous version back so a failed swap never + # leaves the skill missing or hidden in the retired copy. + os.replace(retired, target) + raise shutil.rmtree(retired, ignore_errors=True) continue os.replace(staging, target) @@ -498,6 +506,42 @@ async def register(self, command: str, args: tuple[str, ...]) -> dict[str, Any]: raise NotImplementedError +@dataclass(frozen=True, slots=True) +class ClaudeEntry: + """An MCP entry as `claude mcp get` reports it.""" + + command: str + args: tuple[str, ...] + scope: str + + def matches(self, command: str, args: tuple[str, ...]) -> bool: + """Check that this entry launches exactly ``command`` with ``args``.""" + return self.command == command and self.args == args + + +def _parse_claude_entry(output: str) -> ClaudeEntry | None: + """Read the launch and scope out of `claude mcp get` output. + + The CLI prints one `Command:` line, one space-joined `Args:` line, and a + `Scope:` line starting with `User`, `Project`, or `Local`. A space-joined + argument list cannot recover an argument containing whitespace; entries + written by `kitaru setup` never contain one. + """ + fields: dict[str, str] = {} + for line in output.splitlines(): + key, separator, value = line.strip().partition(":") + if separator: + fields.setdefault(key, value.strip()) + command = fields.get("Command") + if not command: + return None + return ClaudeEntry( + command=command, + args=tuple(fields.get("Args", "").split()), + scope=fields.get("Scope", "").split(" ", 1)[0].lower(), + ) + + class ClaudeCodeClient(McpClient): """Claude Code, configured through its own `claude mcp` commands.""" @@ -512,43 +556,90 @@ async def register(self, command: str, args: tuple[str, ...]) -> dict[str, Any]: """Replace any existing `kitaru` entry and verify it is the one in use. `claude mcp get` is scope-agnostic and reports whichever entry wins. - After adding ours, read it back: if the winning entry does not carry - our command, an entry in another scope shadows it and the user has to - remove that one. + An entry in our scope that already matches is left untouched. Otherwise + the entry in our scope is removed and ours added; if the add fails, the + removed entry is put back so a failed run never leaves Claude Code + without the server it had. The result is read back and must carry + exactly our command and arguments: a stale entry in another scope, or a + readback that cannot be parsed, is reported as a failure. """ existing = await _run_command(self.executable, "mcp", "get", MCP_SERVER_NAME) + previous: ClaudeEntry | None = None + removed: ProcessResult | None = None if existing.returncode == 0: - await _run_command( + entry = _parse_claude_entry(existing.stdout) + if entry is not None and entry.scope == self.scope: + if entry.matches(command, args): + return self._done() + previous = entry + removed = await _run_command( self.executable, "mcp", "remove", "--scope", self.scope, MCP_SERVER_NAME ) - added = await _run_command( - self.executable, - "mcp", - "add", - "--scope", - self.scope, - MCP_SERVER_NAME, - "--", - command, - *args, - ) + added = await self._add(command, args) if added.returncode != 0: - return _step("mcp", self.name, "failed", _failure_detail(added)) + detail = _failure_detail(added) + if removed is not None and removed.returncode == 0: + detail += await self._restore(previous) + return _step("mcp", self.name, "failed", detail) current = await _run_command(self.executable, "mcp", "get", MCP_SERVER_NAME) - if current.returncode == 0 and command not in current.stdout: + if current.returncode != 0: return _step( "mcp", self.name, "failed", - f"registered in {self.scope} scope, but an entry named " + f"registered in {self.scope} scope, but reading it back failed " + f"({_failure_detail(current)}). Check `claude mcp get " + f"{MCP_SERVER_NAME}` and run `kitaru setup` again.", + ) + entry = _parse_claude_entry(current.stdout) + if entry is None or not entry.matches(command, args): + return _step( + "mcp", + self.name, + "failed", + f"registered in {self.scope} scope, but `claude mcp get` reports " + f"a different command or arguments: an entry named " f"'{MCP_SERVER_NAME}' in another scope still wins. Remove it " f"with `claude mcp remove {MCP_SERVER_NAME}` in that scope and " "run `kitaru setup` again.", ) + return self._done() + + def _done(self) -> dict[str, Any]: + """Build the successful registration step.""" return _step( "mcp", self.name, "done", f"server '{MCP_SERVER_NAME}', {self.scope} scope" ) + async def _add(self, command: str, args: tuple[str, ...]) -> ProcessResult: + """Add the `kitaru` entry in this client's scope.""" + return await _run_command( + self.executable, + "mcp", + "add", + "--scope", + self.scope, + MCP_SERVER_NAME, + "--", + command, + *args, + ) + + async def _restore(self, previous: ClaudeEntry | None) -> str: + """Re-add the entry that was removed and describe the outcome.""" + if previous is None: + return ( + f"; the previous '{MCP_SERVER_NAME}' entry was removed and could " + "not be read back to restore it, re-add it manually" + ) + restored = await self._add(previous.command, previous.args) + if restored.returncode == 0: + return f"; the previous '{MCP_SERVER_NAME}' entry was restored" + return ( + f"; the previous '{MCP_SERVER_NAME}' entry could not be restored " + f"({_failure_detail(restored)})" + ) + class CodexClient(McpClient): """Codex CLI, configured through `codex mcp add` (which overwrites).""" diff --git a/tests/cli/test_setup.py b/tests/cli/test_setup.py index a8561d585..5e037c3f1 100644 --- a/tests/cli/test_setup.py +++ b/tests/cli/test_setup.py @@ -104,17 +104,52 @@ def _clis(monkeypatch, **paths: str) -> None: monkeypatch.setattr(setup_cli.shutil, "which", lambda name: paths.get(name)) -def _fake_claude(calls: list[tuple[str, ...]], *, existing: bool, winner: str): - """A claude/codex CLI stub; `winner` is what `claude mcp get` reports.""" +_OLD_ENTRY = ("/old/kitaru-mcp", "--server", "http://old:8000", "--mode", "destructive") + + +def _claude_get_output(launch: tuple[str, ...], scope: str = "User") -> str: + """Render an entry the way `claude mcp get` prints it.""" + command, *args = launch + return ( + f"kitaru:\n Scope: {scope} config\n Status: ✓ Connected\n Type: stdio\n" + f" Command: {command}\n Args: {' '.join(args)}\n" + ) + + +def _fake_claude( + calls: list[tuple[str, ...]], + *, + existing: tuple[str, ...] | None, + winner: tuple[str, ...] | None = None, + existing_scope: str = "User", + failing_adds: int = 0, + readback_fails: bool = False, +): + """A claude/codex CLI stub. + + `existing` is the entry `claude mcp get` reports before our add, in + `existing_scope`; `winner` the one it reports afterwards (defaulting to + whatever was last added). The first `failing_adds` add calls fail; + `readback_fails` makes the get after the add fail. + """ async def run(executable: str, *arguments: str) -> ProcessResult: calls.append((executable, *arguments)) + adds = [c for c in calls if c[1:3] == ("mcp", "add")] if arguments[:2] == ("mcp", "get"): - # Before the add: only "existing" answers. After: the winner. - adds = [c for c in calls if c[1:3] == ("mcp", "add")] - if not adds and not existing: - return ProcessResult(returncode=1, stdout="", stderr="not found") - return ProcessResult(returncode=0, stdout=f"kitaru: {winner}", stderr="") + if not adds: + if existing is None: + return ProcessResult(returncode=1, stdout="", stderr="not found") + return ProcessResult( + 0, _claude_get_output(existing, existing_scope), "" + ) + if readback_fails: + return ProcessResult(returncode=1, stdout="", stderr="config broken") + last_add = adds[-1] + reported = winner or tuple(last_add[last_add.index("--") + 1 :]) + return ProcessResult(0, _claude_get_output(reported), "") + if arguments[:2] == ("mcp", "add") and len(adds) <= failing_adds: + return ProcessResult(returncode=1, stdout="", stderr="add refused") return ProcessResult(returncode=0, stdout="", stderr="") return run @@ -212,7 +247,7 @@ async def test_claude_and_codex_clients_register_through_their_clis( _clis(monkeypatch, claude="/bin/claude", codex="/bin/codex") calls: list[tuple[str, ...]] = [] monkeypatch.setattr( - setup_cli, "_run_command", _fake_claude(calls, existing=True, winner=MCP) + setup_cli, "_run_command", _fake_claude(calls, existing=_OLD_ENTRY) ) result = await _run(home, server="http://localhost:9000", mode="read-only") @@ -247,7 +282,7 @@ async def test_claude_entry_shadowed_by_another_scope_is_reported( monkeypatch.setattr( setup_cli, "_run_command", - _fake_claude(calls, existing=True, winner="/old/kitaru-mcp"), + _fake_claude(calls, existing=_OLD_ENTRY, winner=_OLD_ENTRY), ) result = await _run(home, install_skills=False) @@ -258,6 +293,124 @@ async def test_claude_entry_shadowed_by_another_scope_is_reported( assert result.exit_code == 1 +async def test_claude_readback_with_stale_arguments_is_rejected( + home: Path, monkeypatch +): + """A readback carrying our command but old --server/--mode is not success.""" + _clis(monkeypatch, claude="/bin/claude") + stale = (MCP, "--server", "http://old:8000", "--mode", "destructive") + monkeypatch.setattr( + setup_cli, "_run_command", _fake_claude([], existing=stale, winner=stale) + ) + + result = await _run(home, install_skills=False, mode="read-only") + + step = result.item["steps"][0] + assert step["status"] == "failed" + assert "different command or arguments" in step["detail"] + assert result.exit_code == 1 + + +async def test_claude_failed_readback_is_a_failure(home: Path, monkeypatch): + """When `claude mcp get` fails after the add, setup does not report done.""" + _clis(monkeypatch, claude="/bin/claude") + monkeypatch.setattr( + setup_cli, "_run_command", _fake_claude([], existing=None, readback_fails=True) + ) + + result = await _run(home, install_skills=False) + + step = result.item["steps"][0] + assert step["status"] == "failed" + assert "reading it back failed" in step["detail"] + assert "config broken" in step["detail"] + assert result.exit_code == 1 + + +async def test_claude_failed_add_restores_the_previous_entry(home: Path, monkeypatch): + """If the replacement cannot be added, the removed entry is put back.""" + _clis(monkeypatch, claude="/bin/claude") + calls: list[tuple[str, ...]] = [] + monkeypatch.setattr( + setup_cli, + "_run_command", + _fake_claude(calls, existing=_OLD_ENTRY, failing_adds=1), + ) + + result = await _run(home, install_skills=False) + + add_prefix = ("/bin/claude", "mcp", "add", "--scope", "user", "kitaru", "--") + assert calls == [ + ("/bin/claude", "mcp", "get", "kitaru"), + ("/bin/claude", "mcp", "remove", "--scope", "user", "kitaru"), + (*add_prefix, MCP, "--server", "http://localhost:8000", "--mode", "standard"), + (*add_prefix, *_OLD_ENTRY), + ] + step = result.item["steps"][0] + assert step["status"] == "failed" + assert step["detail"] == ( + "exit 1: add refused; the previous 'kitaru' entry was restored" + ) + assert result.exit_code == 1 + + +async def test_claude_matching_entry_in_our_scope_is_left_untouched( + home: Path, monkeypatch +): + """Re-running against an already correct entry never removes or re-adds it.""" + _clis(monkeypatch, claude="/bin/claude") + calls: list[tuple[str, ...]] = [] + current = (MCP, "--server", "http://localhost:8000", "--mode", "standard") + monkeypatch.setattr( + setup_cli, "_run_command", _fake_claude(calls, existing=current) + ) + + result = await _run(home, install_skills=False) + + assert calls == [("/bin/claude", "mcp", "get", "kitaru")] + assert result.item["steps"][0]["status"] == "done" + assert result.exit_code == 0 + + +async def test_claude_failed_add_does_not_restore_an_entry_from_another_scope( + home: Path, monkeypatch +): + """A shadowing entry from another scope is never re-added into ours.""" + _clis(monkeypatch, claude="/bin/claude") + calls: list[tuple[str, ...]] = [] + monkeypatch.setattr( + setup_cli, + "_run_command", + _fake_claude( + calls, existing=_OLD_ENTRY, existing_scope="Local", failing_adds=1 + ), + ) + + result = await _run(home, install_skills=False) + + adds = [c for c in calls if c[1:3] == ("mcp", "add")] + assert len(adds) == 1 + step = result.item["steps"][0] + assert step["status"] == "failed" + assert "could not be read back to restore it" in step["detail"] + + +async def test_claude_failed_add_reports_an_unrestorable_entry(home: Path, monkeypatch): + """An entry that was removed but cannot be restored is called out.""" + _clis(monkeypatch, claude="/bin/claude") + monkeypatch.setattr( + setup_cli, + "_run_command", + _fake_claude([], existing=_OLD_ENTRY, failing_adds=2), + ) + + result = await _run(home, install_skills=False) + + step = result.item["steps"][0] + assert step["status"] == "failed" + assert "could not be restored" in step["detail"] + + async def test_one_failed_client_among_several_is_a_warning(home: Path, monkeypatch): """One failing client does not stop the others or fail the command.""" _clis(monkeypatch, claude="/bin/claude", codex="/bin/codex") @@ -350,9 +503,7 @@ async def test_project_install_uses_uv_run_and_project_scope(home: Path, monkeyp ) _clis(monkeypatch, claude="/bin/claude") calls: list[tuple[str, ...]] = [] - monkeypatch.setattr( - setup_cli, "_run_command", _fake_claude(calls, existing=False, winner="/bin/uv") - ) + monkeypatch.setattr(setup_cli, "_run_command", _fake_claude(calls, existing=None)) result = await _run(home, install_skills=False, cwd=project) @@ -499,6 +650,32 @@ def failing_mkdtemp(*args, **kwargs): assert result.exit_code == 0 +async def test_failed_skill_swap_restores_the_previous_skill(home: Path, monkeypatch): + """When the final rename fails, the old skill is put back, not left retired.""" + _clis(monkeypatch, codex="/bin/codex") + await _run(home, register_mcp=False) + codex_skills = home / ".codex" / "skills" + codex_skill = codex_skills / "kitaru-investigation" + before = (codex_skill / "SKILL.md").read_bytes() + + real_replace = setup_cli.os.replace + + def failing_replace(src, dst): + if ".staging" in str(src) and Path(dst) == codex_skill: + raise OSError("rename refused") + return real_replace(src, dst) + + monkeypatch.setattr(setup_cli.os, "replace", failing_replace) + + result = await _run(home, register_mcp=False) + + statuses = {s["target"]: s["status"] for s in result.item["steps"]} + assert statuses[str(codex_skills)] == "failed" + assert statuses[str(home / ".agents" / "skills")] == "done" + assert (codex_skill / "SKILL.md").read_bytes() == before + assert [p.name for p in codex_skills.iterdir() if p.name.startswith(".")] == [] + + async def test_nothing_to_do_is_a_warning(home: Path): """--no-skills --no-mcp does nothing and says so.""" result = await _run(home, install_skills=False, register_mcp=False) @@ -534,6 +711,35 @@ def test_resolve_mcp_launch_project_mode(tmp_path: Path, monkeypatch): setup_cli.resolve_mcp_launch(tmp_path, tmp_path) +@pytest.mark.parametrize("environment", ["env", "{tmp}/envs/kitaru-dev"]) +def test_resolve_mcp_launch_honors_uv_project_environment( + tmp_path: Path, monkeypatch, environment: str +): + """A project whose uv environment is not `.venv` still gets project scope.""" + project = tmp_path / "repo" + (project / "src").mkdir(parents=True) + (project / "pyproject.toml").write_text("[project]\nname='x'\n", encoding="utf-8") + environment = environment.format(tmp=tmp_path) + prefix = project / environment + (prefix / "bin").mkdir(parents=True) + monkeypatch.setenv("UV_PROJECT_ENVIRONMENT", environment) + monkeypatch.setattr(setup_cli.sys, "prefix", str(prefix)) + monkeypatch.setattr(setup_cli.sys, "executable", str(prefix / "bin" / "python")) + monkeypatch.setattr( + setup_cli.shutil, "which", lambda name: "/bin/uv" if name == "uv" else None + ) + + launch = setup_cli.resolve_mcp_launch(project / "src", tmp_path) + + assert launch.scope == "project" + assert launch.project_dir == project + assert setup_cli._scope_only(project / "src") == "project" + with pytest.raises(CLIError): + # From outside the project the environment does not belong to any + # ancestor of the working directory, so there is no project scope. + setup_cli.resolve_mcp_launch(tmp_path, tmp_path) + + def test_resolve_mcp_launch_user_mode_uses_sibling_executable( tmp_path: Path, monkeypatch ):