diff --git a/CHANGELOG.md b/CHANGELOG.md index 68a7859..ce0bdf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,67 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [8.2.0] — Unreleased +### LSP Status Entry-Point Unification (issue #33) + +The `codelens --lsp-status` top-level flag (intercepted in +`scripts/codelens.py`) and the `codelens lsp-status` subcommand +(`scripts/commands/lsp_status.py`) returned **structurally different +payloads** for what the documentation treats as the same operation: + +- `--lsp-status` called `hybrid_engine.get_lsp_status()` — the richer + payload with `available_count`, `total_servers`, per-server `path` + + `extensions`, and a `recommendation` field. +- `lsp-status` called `lsp_client.detect_available_servers()` directly + and rebuilt a smaller dict — no `available_count`/`total_servers`, + per-server entries missing `path`/`extensions`, and a `hint` field + instead of `recommendation`. + +The MCP server dynamically discovers subcommands, so MCP agents +(`codelens_lsp_status` tool) got the **smaller** payload while CLI +users got the **richer** one — two different "truths" for the same +question. + +**Fix (Option B from the issue):** both entry points now delegate to +`hybrid_engine.get_lsp_status()` — single source of truth. The +top-level `--lsp-status` flag is preserved as a backward-compatible +alias of the `lsp-status` subcommand. Option A (remove the flag +entirely) was rejected because the issue's DoD explicitly requires +both entry points to produce byte-identical output (repro diff exit +code 0), which is unsatisfiable if one entry point is removed. + +A pre-existing determinism bug was also fixed in +`lsp_client.detect_available_servers()`: `extensions` was returned as +`list(config["extensions"])` where `config["extensions"]` is a `set`, +so order varied across Python invocations (hash randomization). Now +sorted, so the repro diff is byte-identical, not just structurally +equal. + +### Changed (issue #33) + +- **`scripts/commands/lsp_status.py:execute()`** — Now delegates to + `hybrid_engine.get_lsp_status()` instead of rebuilding a smaller + dict from `lsp_client.detect_available_servers()`. The ImportError + fallback was updated to match the new (richer) shape so error + responses are still structurally consistent. +- **`scripts/lsp_client.py:detect_available_servers()`** — `extensions` + field is now `sorted(config["extensions"])` instead of + `list(config["extensions"])` for deterministic cross-invocation + output. + +### Documented (issue #33) + +- **`SKILL-QUICK.md`** — Trigger map now has an explicit + `"LSP servers available?"` → `lsp-status` entry, noting that + `--lsp-status` is an alias. The Setup & Lifecycle command list also + documents the alias relationship. + +### Tested (issue #33) + +- **`tests/test_cli.py:TestLspStatusEntryPointParity`** — New class + with 3 tests: top-level key parity, per-server field parity, and + full byte-identical payload equality. Guards against future + regressions of the dual-truth problem. + ### OSV Cache Staleness Flags + `cache_info` Output (issue #30) Phase 1 roadmap (#21) checklist item: "Fix vuln DB staleness (OSV.dev diff --git a/SKILL-QUICK.md b/SKILL-QUICK.md index 724ad8d..7d2ed69 100755 --- a/SKILL-QUICK.md +++ b/SKILL-QUICK.md @@ -93,6 +93,7 @@ $CLI list --limit 5 --offset 10 --format compact # → paginated + co | "MCP server" | `serve` | | "pre/post-write hook" | `guard --pre` / `guard --post` | | "don't know which command" | `ask "question"` | +| "LSP servers available?" | `lsp-status` (issue #33: `--lsp-status` top-level flag is an alias — both produce the identical payload, and the MCP `codelens_lsp_status` tool uses the same subcommand path) | ### Disambiguation @@ -116,7 +117,7 @@ $CLI list --limit 5 --offset 10 --format compact # → paginated + co ## All 64 Commands ### Setup & Lifecycle (8+) -`init` · `scan [--incremental] [--max-files N] [--full]` · `validate` · `detect` · `watch [--debounce SECS] [--git-mode] [--interval SECS]` · `git-status` · `migrate` · `serve` · `lsp-status` +`init` · `scan [--incremental] [--max-files N] [--full]` · `validate` · `detect` · `watch [--debounce SECS] [--git-mode] [--interval SECS]` · `git-status` · `migrate` · `serve` · `lsp-status` (issue #33: `codelens --lsp-status` top-level flag is an alias of `codelens lsp-status` — both delegate to `hybrid_engine.get_lsp_status()` and return the identical payload) ### Pre-Write Safety (5) `query "name" [--domain ...] [--fuzzy]` · `impact "name" [--action modify|delete]` · `refactor-safe "name" [--action rename|move]` · `guard (--pre|--post) --file PATH` · `check [--severity ...] [--max-findings N]` diff --git a/scripts/commands/lsp_status.py b/scripts/commands/lsp_status.py index 0738a51..f8449b4 100644 --- a/scripts/commands/lsp_status.py +++ b/scripts/commands/lsp_status.py @@ -1,4 +1,11 @@ -"""LSP status command — check which language servers are available.""" +"""LSP status command — check which language servers are available. + +Issue #33: this subcommand and the top-level ``--lsp-status`` flag (intercepted +in ``codelens.py``) MUST return the same payload. Both entry points therefore +delegate to :func:`hybrid_engine.get_lsp_status` — the single source of truth +for LSP availability. The MCP server dynamically discovers this subcommand, so +unifying here also fixes the CLI/MCP divergence described in the issue. +""" from commands import register_command @@ -9,38 +16,28 @@ def add_args(sub): def execute(args, workspace): - """Check which LSP servers are available on the system.""" + """Check which LSP servers are available on the system. + + Delegates to :func:`hybrid_engine.get_lsp_status` so that + ``codelens lsp-status``, ``codelens --lsp-status``, and the MCP + ``codelens_lsp_status`` tool all return the same payload. + """ try: - from lsp_client import detect_available_servers, LSP_SERVERS + from hybrid_engine import get_lsp_status except ImportError: return { "status": "ok", "lsp_available": False, + "available_count": 0, + "total_servers": 0, "servers": {}, - "hint": "lsp_client.py not found. Install hybrid analysis support.", - } - - available = detect_available_servers() - servers = {} - for name, info in available.items(): - servers[name] = { - "available": info.get("available", False), - "languages": info.get("languages", []), - "install_hint": info.get("install_hint", ""), + "recommendation": ( + "hybrid_engine.py not found. Install hybrid analysis support " + "to enable LSP status checks." + ), } - any_available = any(s["available"] for s in servers.values()) - - return { - "status": "ok", - "lsp_available": any_available, - "servers": servers, - "hint": ( - "LSP servers found! Use --deep flag for enhanced analysis." - if any_available - else "No LSP servers found. Install one for deep analysis (see install_hint)." - ), - } + return get_lsp_status() register_command( diff --git a/scripts/lsp_client.py b/scripts/lsp_client.py index 09554b9..e170bac 100644 --- a/scripts/lsp_client.py +++ b/scripts/lsp_client.py @@ -86,7 +86,14 @@ def _which(command: str) -> Optional[str]: def detect_available_servers() -> Dict[str, Dict[str, Any]]: - """Auto-detect which LSP servers are available on the system.""" + """Auto-detect which LSP servers are available on the system. + + ``extensions`` is sorted so the payload is deterministic across Python + invocations (``config["extensions"]`` is a set, and ``list(set)`` order + depends on hash randomization). Required by issue #33 so that + ``codelens --lsp-status`` and ``codelens lsp-status`` produce byte-identical + JSON, not just structurally-equal JSON. + """ results = {} for name, config in LSP_SERVERS.items(): cmd = config["command"][0] @@ -95,7 +102,7 @@ def detect_available_servers() -> Dict[str, Dict[str, Any]]: "available": path is not None, "path": path, "languages": config["languages"], - "extensions": list(config["extensions"]), + "extensions": sorted(config["extensions"]), "install_hint": config["install_hint"], } return results diff --git a/tests/test_cli.py b/tests/test_cli.py index 8aad8d5..59dae79 100755 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -775,3 +775,99 @@ def test_main_path_no_fallback_when_subprocess_succeeds(self, capsys): finally: import shutil shutil.rmtree(ws, ignore_errors=True) + + +class TestLspStatusEntryPointParity: + """Regression guard for issue #33. + + Before the fix, ``codelens --lsp-status`` (top-level flag, intercepted in + ``codelens.py``) called ``hybrid_engine.get_lsp_status()`` while + ``codelens lsp-status`` (subcommand, ``commands/lsp_status.py``) called + ``lsp_client.detect_available_servers()`` directly. The two payloads had + different top-level keys, different per-server fields, and different + hint/recommendation field names — so CLI users and MCP agents got + different answers to the same question. + + After the fix, both entry points delegate to ``hybrid_engine.get_lsp_status`` + (single source of truth). These tests assert structural parity: the set of + top-level keys must be identical, and the set of per-server keys must be + identical. The byte-identical check is covered by the repro diff in the + PR description; these tests guard against future regressions in the + test suite itself. + """ + + @staticmethod + def _run_codelens(extra_args): + """Run ``codelens --format json`` and return parsed JSON.""" + import subprocess + proc = subprocess.run( + [sys.executable, os.path.join(SCRIPT_DIR, "codelens.py"), + *extra_args, "--format", "json"], + capture_output=True, text=True, timeout=60, + env={**os.environ, "PYTHONPATH": "scripts"}, + ) + assert proc.returncode == 0, ( + f"codelens {' '.join(extra_args)} failed with rc={proc.returncode}:\n" + f"stdout: {proc.stdout}\nstderr: {proc.stderr}" + ) + # Strip any leading non-JSON lines (e.g. workspace auto-detect notices + # on stderr don't affect stdout, but be defensive). + idx = proc.stdout.find("{") + assert idx >= 0, f"No JSON in stdout: {proc.stdout!r}" + return json.loads(proc.stdout[idx:]) + + def test_top_level_keys_match(self): + """``--lsp-status`` and ``lsp-status`` must have identical top-level keys.""" + flag_payload = self._run_codelens(["--lsp-status"]) + sub_payload = self._run_codelens(["lsp-status"]) + + flag_keys = set(flag_payload.keys()) + sub_keys = set(sub_payload.keys()) + + assert flag_keys == sub_keys, ( + f"Top-level key sets differ between --lsp-status and lsp-status.\n" + f" --lsp-status only: {flag_keys - sub_keys}\n" + f" lsp-status only: {sub_keys - flag_keys}\n" + f" common : {flag_keys & sub_keys}" + ) + + def test_per_server_keys_match(self): + """Per-server field sets must be identical across both entry points.""" + flag_payload = self._run_codelens(["--lsp-status"]) + sub_payload = self._run_codelens(["lsp-status"]) + + flag_servers = flag_payload.get("servers", {}) + sub_servers = sub_payload.get("servers", {}) + + # Same set of server names + assert set(flag_servers.keys()) == set(sub_servers.keys()), ( + f"Server name sets differ:\n" + f" --lsp-status only: {set(flag_servers) - set(sub_servers)}\n" + f" lsp-status only: {set(sub_servers) - set(flag_servers)}" + ) + + # For each server, same set of field names + for name in flag_servers: + flag_fields = set(flag_servers[name].keys()) + sub_fields = set(sub_servers[name].keys()) + assert flag_fields == sub_fields, ( + f"Per-server field sets differ for server {name!r}:\n" + f" --lsp-status only: {flag_fields - sub_fields}\n" + f" lsp-status only: {sub_fields - flag_fields}" + ) + + def test_payloads_byte_identical(self): + """Full payload equality — the strongest possible parity guarantee. + + Both entry points must produce byte-identical JSON (after canonical + formatting), not just structural parity. This catches any future + regression that introduces a divergent field value. + """ + flag_payload = self._run_codelens(["--lsp-status"]) + sub_payload = self._run_codelens(["lsp-status"]) + + assert flag_payload == sub_payload, ( + "Payloads differ between --lsp-status and lsp-status:\n" + f" --lsp-status: {json.dumps(flag_payload, sort_keys=True, indent=2)}\n" + f" lsp-status : {json.dumps(sub_payload, sort_keys=True, indent=2)}" + )