From cfdf577738c532eb8b171fb67781061edcc7b383 Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Mon, 29 Jun 2026 15:48:51 +0000 Subject: [PATCH] fix(lsp-status): unify --lsp-status flag and lsp-status subcommand payload (issue #33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both entry points now delegate to hybrid_engine.get_lsp_status() (single source of truth). Previously --lsp-status called get_lsp_status() while lsp-status called detect_available_servers() directly, producing structurally different payloads for what the docs treat as the same operation. The MCP server (which only exposes the subcommand) got the smaller payload, so CLI users and MCP agents saw different 'truths'. Also fixes a pre-existing determinism bug in detect_available_servers(): extensions was list(set) which varies across Python invocations due to hash randomization. Now sorted, so the repro diff is byte-identical. Chose Option B over Option A (issue suggested A is 'more recommended') because the DoD explicitly requires both entry points to produce byte-identical output (repro diff exit code 0) AND a test asserting 'both entry points have the same top-level keys' — both DoD items are unsatisfiable if the top-level flag is removed entirely. Option A's architectural concern (single source of truth) is preserved by routing both entry points through the same backend function. Files changed: - scripts/commands/lsp_status.py: delegate to hybrid_engine.get_lsp_status() - scripts/lsp_client.py: sort extensions for deterministic output - SKILL-QUICK.md: document both entry points in trigger map - CHANGELOG.md: add issue #33 entry - tests/test_cli.py: new TestLspStatusEntryPointParity class (3 tests) Closes #33. --- CHANGELOG.md | 61 +++++++++++++++++++++ SKILL-QUICK.md | 3 +- scripts/commands/lsp_status.py | 47 ++++++++-------- scripts/lsp_client.py | 11 +++- tests/test_cli.py | 99 ++++++++++++++++++++++++++++++++++ 5 files changed, 193 insertions(+), 28 deletions(-) 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 31b8f31..96b9e3f 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 56 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 c572507..5e29a2a 100755 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -331,3 +331,102 @@ def test_check_full_cli_invocation_with_positional(self): finally: import shutil shutil.rmtree(ws, ignore_errors=True) + + +# ─── Issue #33: --lsp-status flag and lsp-status subcommand must agree ──── + + +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)}" + )