From 01c9c0a6d852e2acd401465f0e9b19a637c7b03a Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Mon, 6 Jul 2026 13:52:59 +0300 Subject: [PATCH 1/2] =?UTF-8?q?feat(install):=20recommend=20cli=20surface?= =?UTF-8?q?=20+=20`update=20--surface`=20mcp=E2=86=94cli=20migration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two setup-wizard fixes following the jrag CLI ship: 1. `update` can now switch surfaces. New `update --surface {mcp,cli}` flag; on a TTY with no flag it prompts (cursor on the current surface). A switch migrates each affected host — tears down the old surface's artifacts (`_remove_mcp_entry` pops only our server, preserving siblings; skill/agent files removed), deploys the new surface's, and rewrites the install marker so it persists. Fails fast (exit 1) if the target binary is missing; dry-run safe. Non-interactive `update` without `--surface` refreshes each host on its own recorded surface and migrates nothing (a mixed-surface marker is left untouched, not normalized to the first host's surface). 2. Install recommends `cli`. `select_surface` now lists `cli (Recommended)` first and defaults to `cli` everywhere (interactive cursor + non-interactive default). Fixed `prompt()` so `questionary.select(default=...)` positions the cursor: it validates `default` only against `Choice.value`, so dict choices are normalized to `Choice` before the call. Note: non-interactive `install`/`update` without `--surface` now yields `cli` (previously `mcp`) — a breaking change for CI that relied on the implicit MCP default. Co-Authored-By: Claude --- README.md | 2 +- docs/JAVA-CODEBASE-RAG-CLI.md | 19 +- java_codebase_rag/cli.py | 15 +- java_codebase_rag/installer.py | 328 ++++++++++++++++++-- tests/test_installer.py | 3 + tests/test_installer_surface.py | 446 +++++++++++++++++++++++++++- tests/test_java_codebase_rag_cli.py | 6 +- 7 files changed, 776 insertions(+), 43 deletions(-) diff --git a/README.md b/README.md index 3dcb333f..55759b46 100644 --- a/README.md +++ b/README.md @@ -77,7 +77,7 @@ If you prefer manual configuration, see [`docs/JAVA-CODEBASE-RAG-CLI.md`](./docs ## Tools & commands at a glance -Pick a surface once at install time — `java-codebase-rag install --surface mcp|cli` (default `mcp`). Both surfaces walk the same LanceDB vectors + LadybugDB graph. +Pick a surface at install time — `java-codebase-rag install --surface mcp|cli` (default `cli`, recommended). Both surfaces walk the same LanceDB vectors + LadybugDB graph. Switch an existing install later with `java-codebase-rag update --surface mcp|cli`. **MCP surface — five tools over stdio** diff --git a/docs/JAVA-CODEBASE-RAG-CLI.md b/docs/JAVA-CODEBASE-RAG-CLI.md index 0ee1e4fb..b19636d0 100644 --- a/docs/JAVA-CODEBASE-RAG-CLI.md +++ b/docs/JAVA-CODEBASE-RAG-CLI.md @@ -43,6 +43,7 @@ java-codebase-rag install --scope user - `--agent {claude-code,qwen-code,gigacode}` — Agent host to configure (can be passed multiple times). - `--scope {project,user}` — Installation scope (default: `project`). Project scope writes to `./` in the project repo; user scope writes to `~/./` (globally available). - `--model MODEL` — Embedding model path or `auto` (default: `auto`, downloads `sentence-transformers/all-MiniLM-L6-v2` on first run). +- `--surface {mcp,cli}` — Agent surface (default: `cli`, recommended). `cli` deploys the `jrag` console-script skill + `explorer-rag-cli` subagent (one command per intent, no MCP entry). `mcp` registers the `java-codebase-rag` stdio MCP server (five tools: `search`/`find`/`describe`/`neighbors`/`resolve`) plus the `explore-codebase` skill + `explorer-rag-enhanced` subagent. Omit to choose interactively. - `--quiet` / `-q` — Suppress the indexing progress stream on stderr (wizard prompts unchanged). - `--verbose` / `-v` — Raw-relay subprocess output during the indexing sub-step (no progress bar). @@ -56,14 +57,15 @@ java-codebase-rag install --scope user 2. Embedding model selection — auto-download or local path. 3. Agent host selection — Claude Code, Qwen Code, GigaCode (multi-select). 4. Install scope — project or user. -5. MCP entrypoint resolution + artifact deployment — config, skill, agent files. -6. Index + finish — YAML generation, `.gitignore` update, `init`. Stage 6's indexing sub-step renders the unified `Vectors → Optimize → Graph` progress on **stderr** (see [Indexing progress](#indexing-progress-stderr)); the wizard's conversational stdout is unchanged. +5. Surface selection — `cli` (recommended, `jrag` skill+subagent) or `mcp` (stdio server + skill + subagent). Re-runs pre-fill the prior surface. +6. Surface entrypoint resolution + artifact deployment — config (mcp only), skill, agent files. +7. Index + finish — YAML generation, `.gitignore` update, `init`. Stage 7's indexing sub-step renders the unified `Vectors → Optimize → Graph` progress on **stderr** (see [Indexing progress](#indexing-progress-stderr)); the wizard's conversational stdout is unchanged. **Re-running `install`:** If `.java-codebase-rag.yml` exists, the installer shows current values and offers "Update" (pre-filled) or "Start fresh". Existing MCP entries are updated in-place (merged, not duplicated). Skill/agent files trigger overwrite confirmation. ### `update` -Post-upgrade refresh: overwrites skill and agent files with the latest shipped versions and updates the MCP command path. If an index exists, also runs an incremental Lance + graph catch-up (same as `increment`). Requires a prior `install` run. +Post-upgrade refresh: overwrites skill and agent files with the latest shipped versions and updates the MCP command path. If an index exists, also runs an incremental Lance + graph catch-up (same as `increment`). Can also switch the agent surface (`mcp` ↔ `cli`) for an existing install. Requires a prior `install` run. ```bash # Refresh after pip upgrade @@ -75,18 +77,23 @@ java-codebase-rag update --dry-run # Force overwrite all artifacts java-codebase-rag update --force + +# Switch surface (migrate an existing install) +java-codebase-rag update --surface cli # mcp → cli +java-codebase-rag update --surface mcp # cli → mcp ``` **Flags:** - `--force` — Overwrite all artifacts even if content matches. - `--dry-run` — Print changes without writing files. +- `--surface {mcp,cli}` — Switch agent surface. Tears down the old surface's artifacts (removes just the `java-codebase-rag` MCP entry on `mcp`→`cli`; removes the `jrag` skill/subagent on `cli`→`mcp`), deploys the new surface's, and rewrites the install marker so the switch persists. Omit to keep the current surface; on a TTY you'll be prompted (cursor on the current surface). - `--quiet` / `-q` — Suppress the indexing progress stream on stderr (wizard stdout unchanged). - `--verbose` / `-v` — Raw-relay subprocess output during the indexing sub-step (no progress bar). **Behavior:** -- Detects previously configured agent hosts (scans both project-level and user-level config files). -- Refreshes skill and agent files (versioned assets from the package). -- Updates MCP entrypoint path if `java-codebase-rag-mcp` has moved. +- Detects previously configured agent hosts (reads the `.java-codebase-rag.hosts` marker; falls back to scanning project- and user-level MCP config files). +- Refreshes skill and agent files (versioned assets from the package). On the `mcp` surface, also updates the MCP entrypoint path if `java-codebase-rag-mcp` has moved. +- With `--surface` (or the interactive prompt), migrates each host whose recorded surface differs: tears down the old surface, deploys the new one, rewrites the marker. Non-interactive `update` without `--surface` keeps the current surface. - Runs an incremental index update (Lance + graph) if an index exists — same as `java-codebase-rag increment`. The indexing sub-step renders the unified `Vectors → Optimize → Graph` progress on **stderr** (see [Indexing progress](#indexing-progress-stderr)); it no longer runs silently. **Exit codes:** diff --git a/java_codebase_rag/cli.py b/java_codebase_rag/cli.py index 9d03b396..560326b3 100644 --- a/java_codebase_rag/cli.py +++ b/java_codebase_rag/cli.py @@ -648,6 +648,7 @@ def _cmd_update(args: argparse.Namespace) -> int: dry_run=bool(args.dry_run), quiet=bool(args.quiet), verbose=bool(args.verbose), + surface=args.surface, ) @@ -986,7 +987,9 @@ def build_parser() -> argparse.ArgumentParser: "Post-upgrade refresh: overwrites skill and agent files with the latest " "shipped versions and updates the MCP command path. If an index exists, " "also runs an incremental Lance + graph catch-up (same as `increment`). " - "Use --dry-run to preview changes without writing. Requires a prior `install` run." + "Use --dry-run to preview changes without writing. Pass --surface to " + "switch between the mcp and cli surfaces (migrates artifacts + marker). " + "Requires a prior `install` run." ), ) update.add_argument( @@ -999,6 +1002,16 @@ def build_parser() -> argparse.ArgumentParser: action="store_true", help="Print changes without writing files.", ) + update.add_argument( + "--surface", + choices=["mcp", "cli"], + default=None, + help=( + "Switch agent surface: 'mcp' or 'cli'. Tears down the old surface's " + "artifacts and deploys the new surface's (also rewrites the install " + "marker). Omit to keep the current surface; on a TTY you'll be prompted." + ), + ) _add_verbosity_flags(update) update.set_defaults(handler=_cmd_update) diff --git a/java_codebase_rag/installer.py b/java_codebase_rag/installer.py index 60c4913f..361f3929 100644 --- a/java_codebase_rag/installer.py +++ b/java_codebase_rag/installer.py @@ -190,7 +190,27 @@ def prompt( if prompt_type == "checkbox": return questionary.checkbox(message, choices=choices, style=no_color_style).ask() elif prompt_type == "select": - return questionary.select(message, choices=choices, style=no_color_style).ask() + # Normalize dict choices to questionary.Choice so ``default`` is + # matched by value. questionary.select forwards ``default`` as the + # cursor position (initial_choice), but its validation only matches + # ``default`` against ``Choice.value`` — a raw dict's value is + # invisible there, so passing ``default`` with dict choices raises. + # Plain-string choices pass through unchanged. + norm_choices = [] + for c in choices or []: + if isinstance(c, dict): + title = c.get("name", c.get("value")) + norm_choices.append( + questionary.Choice(title, value=c.get("value", title)) + ) + else: + norm_choices.append(c) + return questionary.select( + message, + choices=norm_choices, + default=default, + style=no_color_style, + ).ask() elif prompt_type == "text": return questionary.text(message, default=default, style=no_color_style).ask() elif prompt_type == "confirm": @@ -475,6 +495,21 @@ def select_scope(*, non_interactive: bool, cli_scope: str | None) -> Scope: return selected # type: ignore +def _surface_choices() -> list[dict]: + """Choice list for a surface select prompt. + + Single source of truth for the surface option labels and order: ``cli`` is + listed first and marked "(Recommended)" — the jrag CLI surface is the + recommended default for new installs. ``mcp`` remains available. The + returned dicts are normalized to ``questionary.Choice`` inside ``prompt`` + so a value-based ``default`` (cursor position) validates. + """ + return [ + {"name": "cli (Recommended)", "value": "cli"}, + {"name": "mcp", "value": "mcp"}, + ] + + def select_surface( *, non_interactive: bool, @@ -483,15 +518,17 @@ def select_surface( ) -> Surface: """Select 'mcp' or 'cli' surface (PR-JRAG-5). - The MCP surface registers the stdio MCP server (today's behavior). The CLI - surface ships the ``jrag`` console-script skill+subagent instead — no MCP - entry is registered. + The MCP surface registers the stdio MCP server. The CLI surface ships the + ``jrag`` console-script skill+subagent instead — no MCP entry is registered. + The CLI surface is the recommended default (listed first, marked + "(Recommended)"). Args: - non_interactive: If True, honor ``cli_surface`` (default ``"mcp"``). + non_interactive: If True, honor ``cli_surface`` (default ``"cli"``). cli_surface: Surface from the ``--surface`` CLI flag. prefill: On re-run, the surface recorded in the existing marker file. - When set and the user does not pick otherwise, this is preserved. + When set, the cursor defaults to it so the user can keep the prior + choice with Enter (``cli`` is still shown first + recommended). Returns: Selected surface (``"mcp"`` or ``"cli"``). @@ -506,25 +543,22 @@ def select_surface( return cli_surface # type: ignore if non_interactive: - # Default to MCP for back-comat when no flag is passed. - return "mcp" + # Default to the recommended CLI surface when no flag is passed. + return "cli" print( - "Note: 'mcp' surface registers the java-codebase-rag MCP server (5 tools: " - "search/find/describe/neighbors/resolve)." + "Note: 'cli' surface deploys the `jrag` console-script skill+subagent " + "(one command per intent, no MCP server) — recommended." ) print( - " 'cli' surface deploys the `jrag` console-script skill+subagent " - "(one command per intent, no MCP server)." + " 'mcp' surface registers the java-codebase-rag MCP server " + "(5 tools: search/find/describe/neighbors/resolve)." ) - choices = ["mcp", "cli"] - if prefill is not None: - # Surface the prior choice first so the user can keep it with Enter. - choices = [prefill] + [c for c in ("mcp", "cli") if c != prefill] - default = prefill - else: - default = "mcp" + # cli is always shown first + recommended; the cursor defaults to the prior + # choice (prefill) on re-run so the user can keep it with Enter. + choices = _surface_choices() + default = prefill if prefill is not None else "cli" selected = prompt( "select", @@ -1543,6 +1577,133 @@ def _refresh_mcp_config( return ArtifactResult(path=config_path, success=False, error=str(e)) +def _remove_mcp_entry(config_path: Path, *, dry_run: bool) -> ArtifactResult: + """Remove the java-codebase-rag entry from an MCP config (surface migration). + + Pops ONLY the ``java-codebase-rag`` key from ``mcpServers`` — other servers + and the file itself are preserved. No-op success when the file or our key is + absent. Atomic write (same tmp + ``os.replace`` pattern as ``merge_mcp_config``). + Used by ``_undeploy_surface`` when switching off the MCP surface. + """ + if not config_path.is_file(): + return ArtifactResult(path=config_path, success=True, error=None) + try: + with open(config_path, "r") as f: + config = json.load(f) + except (json.JSONDecodeError, IOError, OSError) as e: + return ArtifactResult( + path=config_path, success=False, error=f"Failed to parse {config_path}: {e}" + ) + + servers = config.get("mcpServers") + if not isinstance(servers, dict) or _MCP_SERVER_NAME not in servers: + return ArtifactResult(path=config_path, success=True, error=None) + + if dry_run: + print(f"Would remove MCP entry from {config_path}") + return ArtifactResult(path=config_path, success=True, error=None) + + del servers[_MCP_SERVER_NAME] + if not servers: + config.pop("mcpServers", None) + + tmp_name = None + try: + with tempfile.NamedTemporaryFile( + mode="w", + dir=config_path.parent, + prefix=f".{config_path.name}.", + delete=False, + ) as tmp: + json.dump(config, tmp, indent=2) + tmp.flush() + os.fsync(tmp.fileno()) + tmp_name = tmp.name + os.replace(tmp_name, config_path) + print(f"Removed MCP entry from {config_path}") + return ArtifactResult(path=config_path, success=True, error=None) + except (IOError, OSError) as e: + if tmp_name: + try: + os.unlink(tmp_name) + except OSError: + pass + return ArtifactResult( + path=config_path, success=False, error=f"Failed to write {config_path}: {e}" + ) + + +def _remove_artifact_file(dest_path: Path, *, dry_run: bool) -> ArtifactResult: + """Remove a deployed skill/agent file (surface migration teardown). + + Best-effort prunes the now-empty immediate parent dir (e.g. + ``skills/explore-codebase``); leaves it in place if other files remain. + No-op success when the file is absent. + """ + if not dest_path.is_file(): + return ArtifactResult(path=dest_path, success=True, error=None) + if dry_run: + print(f"Would remove {dest_path}") + return ArtifactResult(path=dest_path, success=True, error=None) + try: + dest_path.unlink() + try: + dest_path.parent.rmdir() + except OSError: + # Not empty or not removable — leave the directory in place. + pass + print(f"Removed {dest_path}") + return ArtifactResult(path=dest_path, success=True, error=None) + except OSError as e: + return ArtifactResult( + path=dest_path, success=False, error=f"Failed to remove {dest_path}: {e}" + ) + + +def _undeploy_surface( + host: HostConfig, scope: str, cwd: Path, *, surface: Surface, dry_run: bool +) -> list[ArtifactResult]: + """Tear down every artifact a surface shipped (migration off ``surface``). + + Iterates ``ARTIFACT_MANIFEST[surface]`` so adding/removing an artifact is + one manifest edit, not two — mirrors ``deploy_artifacts``/``refresh_artifacts``. + The ``mcp`` row removes just our server entry; ``skill``/``agent`` rows + remove the file. Returns one ``ArtifactResult`` per manifest row. + """ + results: list[ArtifactResult] = [] + for kind, _package_path, dest_relative in ARTIFACT_MANIFEST[surface]: + if kind == "mcp": + mcp_config_path = host.mcp_config_path(scope, cwd) + results.append(_remove_mcp_entry(mcp_config_path, dry_run=dry_run)) + else: + dest_path = host.scope_path(scope, cwd) / dest_relative + results.append(_remove_artifact_file(dest_path, dry_run=dry_run)) + return results + + +def _resolve_update_surface(*, surface: str | None, current: Surface) -> Surface | None: + """Decide which surface ``run_update`` should target. + + Returns a surface to migrate toward, or ``None`` when no global choice was + made (non-TTY, no flag) — in which case ``run_update`` refreshes each host + on its OWN recorded surface and migrates nothing. + + - ``surface`` set (``--surface`` flag): validate and use it (enables + migration; invalid value raises ``SystemExit(2)`` via ``select_surface``). + - TTY, no flag: interactive prompt — ``cli`` recommended, cursor on the + current surface so the user can keep it with Enter or switch. + - non-TTY, no flag: ``None`` (no migration). Preserves the behavior of + non-interactive ``run_update(...)`` callers and, crucially, leaves a + mixed-surface marker untouched rather than normalizing it to the first + host's surface. + """ + if surface is not None: + return select_surface(non_interactive=True, cli_surface=surface) + if sys.stdin.isatty(): + return select_surface(non_interactive=False, cli_surface=None, prefill=current) + return None + + def run_update( *, force: bool, @@ -1550,6 +1711,7 @@ def run_update( cwd: Path | None = None, quiet: bool = False, verbose: bool = False, + surface: str | None = None, ) -> int: """Run the update pipeline. Returns exit code. @@ -1561,12 +1723,20 @@ def run_update( the indexing chatter that used to print to stdout moves onto the stderr renderer framing. + Surface switching (mcp ↔ cli): a ``surface`` choice different from a host's + recorded surface migrates that host — tearing down the old surface's + artifacts (``_undeploy_surface``), deploying the new surface's + (``deploy_artifacts``), and rewriting the marker so the switch persists. + Args: force: If True, overwrite all artifacts even if matching dry_run: If True, print changes without writing cwd: Current working directory (defaults to Path.cwd()) quiet: If True, suppress progress output verbose: If True, raw-relay subprocess output (no Live region) + surface: Target surface (``"mcp"``/``"cli"``) from ``--surface``. When + ``None``: a TTY prompts (cursor on the current surface); a non-TTY + keeps each host's recorded surface (no migration). Returns: Exit code (0=success, 1=partial, 2=fatal) @@ -1585,19 +1755,115 @@ def run_update( print(f"Found {len(configured_hosts)} configured host(s).") - # Refresh artifacts for each host + # Resolve the target surface. ``--surface`` validates + overrides; a TTY + # with no flag prompts (cursor on the current surface); a non-TTY with no + # flag yields None -> no global choice, so each host refreshes on its OWN + # recorded surface and nothing migrates (preserves non-interactive callers + # and leaves mixed-surface markers untouched rather than normalizing them). + current_surfaces = {ch.surface for ch in configured_hosts} + current_surface = configured_hosts[0].surface + chosen_surface = _resolve_update_surface( + surface=surface, current=current_surface + ) + if len(current_surfaces) > 1: + if chosen_surface is None: + print( + f"Note: configured hosts span multiple surfaces " + f"({sorted(current_surfaces)}); refreshing each on its own " + f"recorded surface (pass --surface to normalize)." + ) + else: + print( + f"Note: configured hosts span multiple surfaces " + f"({sorted(current_surfaces)}); normalizing to '{chosen_surface}'." + ) + + # If any host needs to migrate, resolve the target surface's runtime binary + # up front so a missing binary fails fast with a clear message rather than a + # per-host partial. (For the cli surface ``deploy_artifacts`` ignores the + # command, but resolving ``jrag`` confirms the invoked CLI actually exists.) + # ``chosen_surface`` is guaranteed non-None here when migration_needed. + migration_needed = ( + chosen_surface is not None + and any(ch.surface != chosen_surface for ch in configured_hosts) + ) + deploy_command = "" + if migration_needed and not dry_run: + try: + deploy_command = resolve_mcp_command( + non_interactive=not sys.stdin.isatty(), surface=chosen_surface + ) + except SystemExit: + binary = "jrag" if chosen_surface == "cli" else "java-codebase-rag-mcp" + print( + f"Error: `{binary}` not found on PATH — cannot migrate to the " + f"'{chosen_surface}' surface." + ) + print( + "Ensure `java-codebase-rag` is installed, then re-run `update " + f"--surface {chosen_surface}`." + ) + return EXIT_PARTIAL + + # Refresh (or migrate) artifacts for each host. When chosen_surface is None + # (non-TTY, no flag) every host takes the refresh branch on its own surface. all_results = [] - for host_config, scope, surface in configured_hosts: - print(f"\nRefreshing {host_config.name} ({scope} scope, surface={surface})...") - results = refresh_artifacts( - host_config, - scope, - cwd, - force=force, - dry_run=dry_run, - surface=surface, - ) - all_results.extend(results) + updated_configured: list[ConfiguredHost] = [] + migrated = False + for host_config, scope, host_surface in configured_hosts: + if chosen_surface is not None and host_surface != chosen_surface: + migrated = True + print( + f"\nMigrating {host_config.name} ({scope} scope): " + f"{host_surface} → {chosen_surface}..." + ) + if dry_run: + print( + f" Would tear down {host_surface} artifacts and deploy " + f"{chosen_surface} artifacts." + ) + updated_configured.append( + ConfiguredHost(host_config, scope, chosen_surface) + ) + continue + teardown_results = _undeploy_surface( + host_config, scope, cwd, surface=host_surface, dry_run=False + ) + all_results.extend(teardown_results) + # deploy_command was resolved up front (migration_needed && not + # dry_run); chosen_surface is non-None on this branch by the guard. + deploy_results = deploy_artifacts( + [host_config], + scope, + cwd, + non_interactive=True, + mcp_command=deploy_command, + surface=chosen_surface, + ) + all_results.extend(deploy_results) + updated_configured.append( + ConfiguredHost(host_config, scope, chosen_surface) + ) + else: + print( + f"\nRefreshing {host_config.name} ({scope} scope, surface={host_surface})..." + ) + results = refresh_artifacts( + host_config, + scope, + cwd, + force=force, + dry_run=dry_run, + surface=host_surface, + ) + all_results.extend(results) + updated_configured.append( + ConfiguredHost(host_config, scope, host_surface) + ) + + # Persist the surface switch so a later ``update`` sees the new surface. + if migrated and not dry_run: + _write_hosts_marker(cwd, updated_configured) # Check for partial failures partial_failures = [r for r in all_results if not r.success] diff --git a/tests/test_installer.py b/tests/test_installer.py index f844d852..d3224d87 100644 --- a/tests/test_installer.py +++ b/tests/test_installer.py @@ -783,6 +783,7 @@ def mock_run_build_ast_graph(*args, **kwargs): agents=["claude-code"], scope="project", model="auto", + surface="mcp", source_root=cwd, quiet=True, ) @@ -867,6 +868,7 @@ def mock_run_build_ast_graph(*args, **kwargs): agents=["claude-code", "qwen-code"], scope="project", model="auto", + surface="mcp", source_root=cwd, quiet=True, ) @@ -1358,6 +1360,7 @@ def mock_run_incremental_graph(*args, **kwargs): agents=["claude-code"], scope="project", model="auto", + surface="mcp", source_root=cwd, quiet=True, ) diff --git a/tests/test_installer_surface.py b/tests/test_installer_surface.py index b2a8b8db..f6921b8b 100644 --- a/tests/test_installer_surface.py +++ b/tests/test_installer_surface.py @@ -445,7 +445,8 @@ def test_install_subparser_registers_surface_flag(): Default is ``None`` so the interactive ``select_surface`` wizard prompts when the flag is omitted (the proposal's CLI-vs-MCP choice); non-interactive - installs fall back to ``'mcp'`` inside ``select_surface`` for back-comat. + installs fall back to ``'cli'`` inside ``select_surface`` (the recommended + default). """ import argparse @@ -465,3 +466,446 @@ def test_install_subparser_registers_surface_flag(): assert surface_action.choices == ["mcp", "cli"] assert surface_action.default is None assert surface_action.dest == "surface" + + +# --------------------------------------------------------------------------- +# cli is the recommended surface: choice order/label + default flip +# --------------------------------------------------------------------------- + + +def test_surface_choices_cli_first_and_recommended(): + """_surface_choices lists cli first and marks it '(Recommended)'.""" + from java_codebase_rag.installer import _surface_choices + + choices = _surface_choices() + assert [c["value"] for c in choices] == ["cli", "mcp"] + assert "Recommended" in choices[0]["name"] + assert choices[1]["value"] == "mcp" + + +def test_select_surface_non_interactive_defaults_to_cli(): + """Non-interactive install without --surface now defaults to cli.""" + assert select_surface(non_interactive=True, cli_surface=None) == "cli" + + +def test_select_surface_prefill_is_preserved_non_tty(): + """Re-run prefill is honored (cursor/default = prefill) on non-TTY. + + cli is still the first/recommended choice, but the default returns the + prior surface so a re-run preserves it. + """ + assert select_surface(non_interactive=False, cli_surface=None, prefill="mcp") == "mcp" + assert select_surface(non_interactive=False, cli_surface=None, prefill="cli") == "cli" + + +def test_prompt_select_forwards_default_and_normalizes_dict_choices(monkeypatch): + """prompt('select') forwards default and normalizes dict choices to Choice. + + questionary.select validates default only against Choice.value (not dict + values), so dict choices must be normalized or default raises. Verified on a + faked TTY (prompt returns default without calling questionary when non-TTY). + """ + import sys + + import questionary + + from java_codebase_rag.installer import prompt + + monkeypatch.setattr(sys.stdin, "isatty", lambda: True) + seen: dict = {} + + class _FakeQuestion: + def __init__(self, message, choices=None, default=None, style=None, **kw): + seen["choices"] = choices + seen["default"] = default + + def ask(self): + return "mcp" + + monkeypatch.setattr(questionary, "select", _FakeQuestion) + + result = prompt( + "select", + "pick", + choices=[{"name": "cli (Recommended)", "value": "cli"}, {"name": "mcp", "value": "mcp"}], + default="mcp", + ) + assert result == "mcp" + assert seen["default"] == "mcp" + norm = seen["choices"] + assert all(isinstance(c, questionary.Choice) for c in norm) + assert [c.value for c in norm] == ["cli", "mcp"] + assert norm[0].title == "cli (Recommended)" + + +# --------------------------------------------------------------------------- +# update --surface: mcp <-> cli migration +# --------------------------------------------------------------------------- + + +def _stub_update_index_skip(monkeypatch): + """Stub the index-discovery + pipeline so run_update stops before indexing.""" + monkeypatch.setattr( + "java_codebase_rag.config.discover_project_root", lambda cwd: None + ) + + +def test_update_migrates_mcp_to_cli(tmp_path, monkeypatch): + """run_update(surface='cli') on an mcp install migrates: tears down mcp, + deploys cli, rewrites the marker. Sibling MCP servers are preserved.""" + import json + + import java_codebase_rag.installer as installer + + _write_hosts_marker(tmp_path, [ConfiguredHost(HOSTS["claude-code"], "project", "mcp")]) + + # Existing mcp-surface state: .mcp.json (us + a sibling server) + mcp skill/agent. + (tmp_path / ".mcp.json").write_text( + json.dumps( + { + "mcpServers": { + "other": {"command": "/other"}, + "java-codebase-rag": {"command": "/fake/bin/java-codebase-rag-mcp", "type": "stdio"}, + } + } + ), + encoding="utf-8", + ) + mcp_skill = tmp_path / ".claude" / "skills" / "explore-codebase" / "SKILL.md" + mcp_skill.parent.mkdir(parents=True) + mcp_skill.write_text("OLD MCP SKILL", encoding="utf-8") + mcp_agent = tmp_path / ".claude" / "agents" / "explorer-rag-enhanced.md" + mcp_agent.parent.mkdir(parents=True) + mcp_agent.write_text("OLD MCP AGENT", encoding="utf-8") + + monkeypatch.setattr(shutil, "which", lambda name: f"/fake/bin/{name}") + monkeypatch.setattr( + "java_codebase_rag.installer._read_package_artifact", + lambda rel: f"FRESH:{rel}", + ) + _stub_update_index_skip(monkeypatch) + + rc = run_update(force=False, dry_run=False, cwd=tmp_path, surface="cli") + assert rc == 0 + + # mcp entry removed, sibling preserved. + cfg = json.loads((tmp_path / ".mcp.json").read_text()) + assert "java-codebase-rag" not in cfg["mcpServers"] + assert "other" in cfg["mcpServers"] + + # mcp skill/agent torn down; cli skill/agent deployed. + assert not mcp_skill.is_file() + assert not mcp_agent.is_file() + cli_skill = tmp_path / ".claude" / "skills" / "explore-codebase-cli" / "SKILL.md" + cli_agent = tmp_path / ".claude" / "agents" / "explorer-rag-cli.md" + assert cli_skill.is_file() and cli_skill.read_text().startswith("FRESH:") + assert cli_agent.is_file() and cli_agent.read_text().startswith("FRESH:") + + # Marker rewritten to cli. + detected = installer._read_hosts_marker(tmp_path) + assert detected is not None and detected[0].surface == "cli" + + +def test_update_migrates_cli_to_mcp(tmp_path, monkeypatch): + """run_update(surface='mcp') on a cli install migrates the other way.""" + import json + + import java_codebase_rag.installer as installer + + _write_hosts_marker(tmp_path, [ConfiguredHost(HOSTS["claude-code"], "project", "cli")]) + + cli_skill = tmp_path / ".claude" / "skills" / "explore-codebase-cli" / "SKILL.md" + cli_skill.parent.mkdir(parents=True) + cli_skill.write_text("OLD CLI SKILL", encoding="utf-8") + cli_agent = tmp_path / ".claude" / "agents" / "explorer-rag-cli.md" + cli_agent.parent.mkdir(parents=True) + cli_agent.write_text("OLD CLI AGENT", encoding="utf-8") + + monkeypatch.setattr(shutil, "which", lambda name: f"/fake/bin/{name}") + monkeypatch.setattr( + "java_codebase_rag.installer._read_package_artifact", + lambda rel: f"FRESH:{rel}", + ) + _stub_update_index_skip(monkeypatch) + + rc = run_update(force=False, dry_run=False, cwd=tmp_path, surface="mcp") + assert rc == 0 + + # cli artifacts gone; mcp entry + mcp skill/agent deployed. + assert not cli_skill.is_file() + assert not cli_agent.is_file() + cfg = json.loads((tmp_path / ".mcp.json").read_text()) + assert "java-codebase-rag" in cfg["mcpServers"] + assert (tmp_path / ".claude" / "skills" / "explore-codebase" / "SKILL.md").is_file() + assert (tmp_path / ".claude" / "agents" / "explorer-rag-enhanced.md").is_file() + + detected = installer._read_hosts_marker(tmp_path) + assert detected is not None and detected[0].surface == "mcp" + + +def test_update_surface_missing_target_binary_returns_partial(tmp_path, monkeypatch): + """Migrating to mcp when java-codebase-rag-mcp is absent -> exit 1, no migration.""" + import java_codebase_rag.installer as installer + + _write_hosts_marker(tmp_path, [ConfiguredHost(HOSTS["claude-code"], "project", "cli")]) + cli_skill = tmp_path / ".claude" / "skills" / "explore-codebase-cli" / "SKILL.md" + cli_skill.parent.mkdir(parents=True) + cli_skill.write_text("OLD CLI SKILL", encoding="utf-8") + + monkeypatch.setattr(shutil, "which", lambda name: None) + _stub_update_index_skip(monkeypatch) + + rc = run_update(force=False, dry_run=False, cwd=tmp_path, surface="mcp") + assert rc == 1 + # Nothing torn down / deployed. + assert cli_skill.is_file() + assert not (tmp_path / ".mcp.json").is_file() + # Marker unchanged. + detected = installer._read_hosts_marker(tmp_path) + assert detected is not None and detected[0].surface == "cli" + + +def test_update_surface_same_as_current_does_not_migrate(tmp_path, monkeypatch): + """run_update(surface=) takes the refresh path; no teardown/marker write.""" + import java_codebase_rag.installer as installer + + _write_hosts_marker(tmp_path, [ConfiguredHost(HOSTS["claude-code"], "project", "mcp")]) + mcp_skill = tmp_path / ".claude" / "skills" / "explore-codebase" / "SKILL.md" + mcp_skill.parent.mkdir(parents=True) + mcp_skill.write_text("STALE", encoding="utf-8") + + monkeypatch.setattr(shutil, "which", lambda name: f"/fake/bin/{name}") + monkeypatch.setattr( + "java_codebase_rag.installer._read_package_artifact", lambda rel: "FRESH" + ) + _stub_update_index_skip(monkeypatch) + + called = {"undeploy": False} + real_undeploy = installer._undeploy_surface + + def spy(host, scope, cwd, *, surface, dry_run): + called["undeploy"] = True + return real_undeploy(host, scope, cwd, surface=surface, dry_run=dry_run) + + monkeypatch.setattr("java_codebase_rag.installer._undeploy_surface", spy) + + rc = run_update(force=False, dry_run=False, cwd=tmp_path, surface="mcp") + assert rc == 0 + assert called["undeploy"] is False, "same-surface update must not tear down" + # Refresh did run. + assert mcp_skill.read_text() == "FRESH" + # Marker still mcp. + detected = installer._read_hosts_marker(tmp_path) + assert detected is not None and detected[0].surface == "mcp" + + +def test_update_surface_dry_run_writes_nothing(tmp_path, monkeypatch): + """run_update(surface='cli', dry_run=True) prints intent, writes no files/marker.""" + import json + + import java_codebase_rag.installer as installer + + _write_hosts_marker(tmp_path, [ConfiguredHost(HOSTS["claude-code"], "project", "mcp")]) + (tmp_path / ".mcp.json").write_text( + json.dumps( + {"mcpServers": {"java-codebase-rag": {"command": "/x", "type": "stdio"}}} + ), + encoding="utf-8", + ) + mcp_skill = tmp_path / ".claude" / "skills" / "explore-codebase" / "SKILL.md" + mcp_skill.parent.mkdir(parents=True) + mcp_skill.write_text("KEEP", encoding="utf-8") + + monkeypatch.setattr(shutil, "which", lambda name: f"/fake/bin/{name}") + monkeypatch.setattr( + "java_codebase_rag.installer._read_package_artifact", lambda rel: "FRESH" + ) + _stub_update_index_skip(monkeypatch) + + rc = run_update(force=False, dry_run=True, cwd=tmp_path, surface="cli") + # Dry run performs no writes, so there can be no partial failures. + assert rc == 0 + # Nothing changed on disk. + cfg = json.loads((tmp_path / ".mcp.json").read_text()) + assert "java-codebase-rag" in cfg["mcpServers"] + assert mcp_skill.read_text() == "KEEP" + assert not (tmp_path / ".claude" / "skills" / "explore-codebase-cli").exists() + # Marker still mcp (not rewritten on dry-run). + detected = installer._read_hosts_marker(tmp_path) + assert detected is not None and detected[0].surface == "mcp" + + +def test_remove_mcp_entry_preserves_sibling_servers(tmp_path): + """_remove_mcp_entry pops only our key; other servers + file survive.""" + import json + + from java_codebase_rag.installer import _remove_mcp_entry + + config_path = tmp_path / ".mcp.json" + config_path.write_text( + json.dumps( + { + "numStartups": 42, + "mcpServers": { + "other": {"command": "/other"}, + "java-codebase-rag": {"command": "/x", "type": "stdio"}, + }, + } + ), + encoding="utf-8", + ) + + result = _remove_mcp_entry(config_path, dry_run=False) + assert result.success + cfg = json.loads(config_path.read_text()) + assert "java-codebase-rag" not in cfg["mcpServers"] + assert "other" in cfg["mcpServers"] + assert cfg["numStartups"] == 42 + + +# --------------------------------------------------------------------------- +# mixed-surface markers + user scope (per-host dispatch) +# --------------------------------------------------------------------------- + + +def test_update_no_flag_non_tty_mixed_marker_does_not_migrate(tmp_path, monkeypatch): + """Non-TTY update with NO --surface refreshes each host on its OWN surface. + + Regression: an earlier version returned the first host's surface as the + global target, so a mixed marker like [claude-code/mcp, qwen-code/cli] would + migrate qwen-code to mcp. The non-TTY no-flag path must migrate nothing. + """ + import java_codebase_rag.installer as installer + + _write_hosts_marker( + tmp_path, + [ + ConfiguredHost(HOSTS["claude-code"], "project", "mcp"), + ConfiguredHost(HOSTS["qwen-code"], "project", "cli"), + ], + ) + claude_mcp_skill = tmp_path / ".claude" / "skills" / "explore-codebase" / "SKILL.md" + claude_mcp_skill.parent.mkdir(parents=True) + claude_mcp_skill.write_text("STALE", encoding="utf-8") + qwen_cli_skill = tmp_path / ".qwen" / "skills" / "explore-codebase-cli" / "SKILL.md" + qwen_cli_skill.parent.mkdir(parents=True) + qwen_cli_skill.write_text("STALE", encoding="utf-8") + + monkeypatch.setattr(shutil, "which", lambda name: f"/fake/bin/{name}") + monkeypatch.setattr( + "java_codebase_rag.installer._read_package_artifact", lambda rel: "FRESH" + ) + _stub_update_index_skip(monkeypatch) + + # _undeploy_surface must NOT be called (no migration on this path). + called = {"undeploy": False} + real_undeploy = installer._undeploy_surface + + def spy(host, scope, cwd, *, surface, dry_run): + called["undeploy"] = True + return real_undeploy(host, scope, cwd, surface=surface, dry_run=dry_run) + + monkeypatch.setattr("java_codebase_rag.installer._undeploy_surface", spy) + + rc = run_update(force=False, dry_run=False, cwd=tmp_path) # no surface, non-TTY + assert rc == 0 + assert called["undeploy"] is False, "non-TTY no-flag update must not migrate" + # Each host refreshed on its OWN surface. + assert claude_mcp_skill.read_text() == "FRESH" + assert qwen_cli_skill.read_text() == "FRESH" + # No cross-surface artifacts appeared. + assert not (tmp_path / ".claude" / "skills" / "explore-codebase-cli").exists() + assert not (tmp_path / ".qwen" / "skills" / "explore-codebase").exists() + # Marker surfaces unchanged. + detected = installer._read_hosts_marker(tmp_path) + assert [d.surface for d in detected] == ["mcp", "cli"] + + +def test_update_surface_normalizes_mixed_marker(tmp_path, monkeypatch): + """--surface normalizes a mixed-surface marker: every host migrates to it.""" + import json + + import java_codebase_rag.installer as installer + + _write_hosts_marker( + tmp_path, + [ + ConfiguredHost(HOSTS["claude-code"], "project", "mcp"), + ConfiguredHost(HOSTS["qwen-code"], "project", "cli"), + ], + ) + # claude-code (mcp) has an MCP entry to tear down. + (tmp_path / ".mcp.json").write_text( + json.dumps( + {"mcpServers": {"java-codebase-rag": {"command": "/x", "type": "stdio"}}} + ), + encoding="utf-8", + ) + # qwen-code (cli) already has the cli skill. + qwen_cli = tmp_path / ".qwen" / "skills" / "explore-codebase-cli" / "SKILL.md" + qwen_cli.parent.mkdir(parents=True) + qwen_cli.write_text("OLD", encoding="utf-8") + + monkeypatch.setattr(shutil, "which", lambda name: f"/fake/bin/{name}") + monkeypatch.setattr( + "java_codebase_rag.installer._read_package_artifact", + lambda rel: f"FRESH:{rel}", + ) + _stub_update_index_skip(monkeypatch) + + rc = run_update(force=False, dry_run=False, cwd=tmp_path, surface="cli") + assert rc == 0 + + # claude-code migrated mcp -> cli: entry gone, cli skill deployed. + cfg = json.loads((tmp_path / ".mcp.json").read_text()) + assert "java-codebase-rag" not in cfg.get("mcpServers", {}) + assert ( + tmp_path / ".claude" / "skills" / "explore-codebase-cli" / "SKILL.md" + ).is_file() + # qwen-code was already cli -> refreshed in place (still present). + assert qwen_cli.is_file() and qwen_cli.read_text().startswith("FRESH:") + # Marker normalized: both cli. + detected = installer._read_hosts_marker(tmp_path) + assert [d.surface for d in detected] == ["cli", "cli"] + + +def test_update_migrates_user_scope_host(tmp_path, monkeypatch): + """Migration is scope-agnostic: a user-scope host migrates too. + + User-scope paths resolve under ``Path.home()``; home is redirected to + ``tmp_path`` to keep the test hermetic. + """ + import json + from pathlib import Path + + import java_codebase_rag.installer as installer + + monkeypatch.setattr(Path, "home", lambda: tmp_path) + _write_hosts_marker(tmp_path, [ConfiguredHost(HOSTS["claude-code"], "user", "mcp")]) + + # User-scope MCP config for claude-code lives at ~/.claude.json (== tmp_path). + (tmp_path / ".claude.json").write_text( + json.dumps( + {"mcpServers": {"java-codebase-rag": {"command": "/x", "type": "stdio"}}} + ), + encoding="utf-8", + ) + + monkeypatch.setattr(shutil, "which", lambda name: f"/fake/bin/{name}") + monkeypatch.setattr( + "java_codebase_rag.installer._read_package_artifact", + lambda rel: f"FRESH:{rel}", + ) + _stub_update_index_skip(monkeypatch) + + rc = run_update(force=False, dry_run=False, cwd=tmp_path, surface="cli") + assert rc == 0 + + # User-scope MCP entry removed; user-scope cli skill deployed. + cfg = json.loads((tmp_path / ".claude.json").read_text()) + assert "java-codebase-rag" not in cfg.get("mcpServers", {}) + assert ( + tmp_path / ".claude" / "skills" / "explore-codebase-cli" / "SKILL.md" + ).is_file() + detected = installer._read_hosts_marker(tmp_path) + assert detected[0].scope == "user" and detected[0].surface == "cli" diff --git a/tests/test_java_codebase_rag_cli.py b/tests/test_java_codebase_rag_cli.py index 817f73b8..ff6678cb 100644 --- a/tests/test_java_codebase_rag_cli.py +++ b/tests/test_java_codebase_rag_cli.py @@ -1528,7 +1528,7 @@ def test_cmd_update_forwards_quiet_flag( captured: dict = {} def _fake_run_update(*, force=False, dry_run=False, cwd=None, - quiet=False, verbose=False): + quiet=False, verbose=False, surface=None): captured["quiet"] = quiet captured["verbose"] = verbose captured["force"] = force @@ -1552,7 +1552,7 @@ def test_cmd_update_forwards_verbose_flag( captured: dict = {} def _fake_run_update(*, force=False, dry_run=False, cwd=None, - quiet=False, verbose=False): + quiet=False, verbose=False, surface=None): captured["quiet"] = quiet captured["verbose"] = verbose return 0 @@ -1598,7 +1598,7 @@ def _fake_run_install(*, non_interactive, agents, scope, model, assert rc == 0 assert captured["verbose"] is True # Omitting --surface forwards None so the interactive select_surface wizard - # prompts (non-interactive falls back to "mcp" inside select_surface). The + # prompts (non-interactive falls back to "cli" inside select_surface). The # operator never picking a surface implicitly is the bug-#1 contract. assert captured["surface"] is None # quiet still flows through too. From 6397cf04fc0b12e0d03eeea2185c1a68437b90b2 Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Mon, 6 Jul 2026 16:02:53 +0300 Subject: [PATCH 2/2] test(install): heavy integration tests need --surface mcp after default flip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two JAVA_CODEBASE_RAG_RUN_HEAVY-gated integration tests invoke `install --non-interactive` via subprocess (no --surface) and assert the MCP artifacts (.mcp.json, explore-codebase skill, explorer-rag-enhanced agent). They broke when the non-interactive default flipped to cli — but they were skipped locally (no heavy env var) so the miss only surfaced in CI. Add `--surface mcp` to preserve their intent (testing the MCP install path). Co-Authored-By: Claude --- tests/test_installer_integration.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/test_installer_integration.py b/tests/test_installer_integration.py index e9361e5c..6d50037e 100644 --- a/tests/test_installer_integration.py +++ b/tests/test_installer_integration.py @@ -42,6 +42,8 @@ def test_install_non_interactive_claude_code_bank_chat(self, tmp_path): "--non-interactive", "--agent", "claude-code", + "--surface", + "mcp", "--quiet", ], cwd=cwd, @@ -109,6 +111,8 @@ def test_install_non_interactive_multi_host_bank_chat(self, tmp_path): "claude-code", "--agent", "qwen-code", + "--surface", + "mcp", "--quiet", ], cwd=cwd,