feat(lsp): add first-class LSP server support to install pipeline #1424
feat(lsp): add first-class LSP server support to install pipeline #1424stbenjam wants to merge 4 commits into
Conversation
Implements LSP server integration mirroring the MCP pattern: - LSPDependency model: Parse and validate LSP server configs from apm.yml and plugin.json - Plugin parser: Extract inline lspServers from plugin.json and .lsp.json files, convert to dependencies.lsp entries - LSPIntegrator: Orchestrates transitive collection, deduplication, installation, stale cleanup, and lockfile persistence - Lockfile: Add lsp_servers and lsp_configs fields for drift detection - Install pipeline: Wire LSP integration after MCP integration - InstallContext: Add direct_lsp_deps field for LSP deps tracking LSP config is written to .lsp.json at project root (Claude Code auto-discovers this file). Unlike MCP which targets multiple runtimes, LSP is Claude Code-only for now. This enables plugins that declare both MCP servers and LSP servers (like gopls for Go support) to have both integrated end-to-end. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add consumer guide, manifest schema section 4.3, lockfile spec fields, and apm-usage skill reference for the new dependencies.lsp support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sergio-sisternes-epam
added
the
panel-review
|
@stbenjam thank you for this contribution — LSP server support is a great addition to the install pipeline. We've confirmed that During review we identified a few quality gaps that we're fixing on your branch to help move this forward:
We'll push these directly to your branch to expedite things. Deferring the final merge decision to @danielmeppiel. Thanks again for picking this up — the MCP-mirroring approach and the docs are solid work. |
| startup_timeout=d.get("startupTimeout") or d.get("startup_timeout"), | ||
| shutdown_timeout=d.get("shutdownTimeout") or d.get("shutdown_timeout"), | ||
| restart_on_crash=d.get("restartOnCrash") | ||
| if d.get("restartOnCrash") is not None | ||
| else d.get("restart_on_crash"), | ||
| max_restarts=d.get("maxRestarts") or d.get("max_restarts"), |
| # Populate direct LSP deps from the manifest for LSP integration. | ||
| ctx.direct_lsp_deps = apm_package.get_lsp_dependencies() | ||
|
|
| def _extract_lsp_servers(plugin_path: Path, manifest: dict[str, Any]) -> dict[str, Any]: | ||
| """Extract LSP server definitions from a plugin manifest. | ||
|
|
||
| Resolves ``lspServers`` by type (per Claude Code spec): | ||
| - ``str`` -> read that file path relative to plugin root, parse JSON. | ||
| - ``dict`` -> use directly as inline server definitions. | ||
|
|
||
| When ``lspServers`` is absent and ``.lsp.json`` exists at plugin root, | ||
| read it as the default (matches Claude Code auto-discovery). | ||
|
|
||
| Security: symlinks are skipped, JSON parse errors are logged as warnings. | ||
|
|
||
| ``${CLAUDE_PLUGIN_ROOT}`` in string values is replaced with the absolute | ||
| plugin path. | ||
|
|
||
| Args: | ||
| plugin_path: Root of the plugin directory. | ||
| manifest: Parsed plugin.json dict. | ||
|
|
||
| Returns: | ||
| dict mapping server name -> server config. Empty on failure. | ||
| """ | ||
| logger = logging.getLogger("apm") | ||
| lsp_value = manifest.get("lspServers") | ||
|
|
||
| if lsp_value is not None: | ||
| if isinstance(lsp_value, dict): | ||
| servers = dict(lsp_value) | ||
| elif isinstance(lsp_value, str): | ||
| servers = _read_lsp_file(plugin_path, lsp_value, logger) | ||
| else: | ||
| logger.warning("Unsupported lspServers type %s; ignoring", type(lsp_value).__name__) | ||
| return {} | ||
| else: | ||
| # Fall back to auto-discovery: .lsp.json | ||
| servers = {} | ||
| candidate = plugin_path / ".lsp.json" | ||
| if candidate.exists() and candidate.is_file() and not candidate.is_symlink(): | ||
| servers = _read_lsp_json(candidate, logger) | ||
|
|
||
| # Substitute ${CLAUDE_PLUGIN_ROOT} in all string values | ||
| if servers: | ||
| abs_root = str(plugin_path.resolve()) | ||
| servers = _substitute_plugin_root(servers, abs_root, logger) | ||
|
|
||
| return servers |
| @dataclass | ||
| class LockFile: | ||
| """APM lock file for reproducible dependency resolution.""" | ||
|
|
||
| lockfile_version: str = "1" | ||
| generated_at: str = field(default_factory=lambda: datetime.now(timezone.utc).isoformat()) | ||
| apm_version: str | None = None | ||
| dependencies: dict[str, LockedDependency] = field(default_factory=dict) | ||
| mcp_servers: list[str] = field(default_factory=list) | ||
| mcp_configs: dict[str, dict] = field(default_factory=dict) | ||
| lsp_servers: list[str] = field(default_factory=list) | ||
| lsp_configs: dict[str, dict] = field(default_factory=dict) | ||
| local_deployed_files: list[str] = field(default_factory=list) | ||
| local_deployed_file_hashes: dict[str, str] = field(default_factory=dict) | ||
|
|
||
| def add_dependency(self, dep: LockedDependency) -> None: | ||
| """Add a dependency to the lock file.""" | ||
| self.dependencies[dep.get_unique_key()] = dep | ||
|
|
||
| def get_dependency(self, key: str) -> LockedDependency | None: | ||
| """Get a dependency by its unique key.""" | ||
| return self.dependencies.get(key) | ||
|
|
||
| def has_dependency(self, key: str) -> bool: | ||
| """Check if a dependency exists.""" | ||
| return key in self.dependencies | ||
|
|
||
| def get_all_dependencies(self) -> list[LockedDependency]: | ||
| """Get all dependencies sorted by depth then repo_url.""" | ||
| return sorted(self.dependencies.values(), key=lambda d: (d.depth, d.repo_url)) | ||
|
|
||
| def get_package_dependencies(self) -> list[LockedDependency]: | ||
| """Get all dependencies excluding the virtual self-entry.""" | ||
| return [d for d in self.get_all_dependencies() if d.local_path != "."] | ||
|
|
||
| def to_yaml(self) -> str: | ||
| """Serialize to YAML string.""" | ||
| # The synthesized self-entry (key ".") is an in-memory normalization | ||
| # of the flat local_deployed_files / local_deployed_file_hashes | ||
| # fields. It must not be written back into the dependencies list, | ||
| # since the flat fields remain the source of truth in YAML. | ||
| _self_dep = self.dependencies.pop(_SELF_KEY, None) | ||
| try: | ||
| data: dict[str, Any] = { | ||
| "lockfile_version": self.lockfile_version, | ||
| "generated_at": self.generated_at, | ||
| } | ||
| if self.apm_version: | ||
| data["apm_version"] = self.apm_version | ||
| data["dependencies"] = [dep.to_dict() for dep in self.get_all_dependencies()] | ||
| if self.mcp_servers: | ||
| data["mcp_servers"] = sorted(self.mcp_servers) | ||
| if self.mcp_configs: | ||
| data["mcp_configs"] = dict(sorted(self.mcp_configs.items())) | ||
| if self.lsp_servers: | ||
| data["lsp_servers"] = sorted(self.lsp_servers) | ||
| if self.lsp_configs: | ||
| data["lsp_configs"] = dict(sorted(self.lsp_configs.items())) | ||
| if self.local_deployed_files: | ||
| data["local_deployed_files"] = sorted(self.local_deployed_files) | ||
| if self.local_deployed_file_hashes: | ||
| data["local_deployed_file_hashes"] = dict( | ||
| sorted(self.local_deployed_file_hashes.items()) | ||
| ) | ||
| from ..utils.yaml_io import yaml_to_str | ||
|
|
||
| return yaml_to_str(data) | ||
| finally: | ||
| if _self_dep is not None: | ||
| self.dependencies[_SELF_KEY] = _self_dep | ||
|
|
||
| @classmethod | ||
| def from_yaml(cls, yaml_str: str) -> "LockFile": | ||
| """Deserialize from YAML string.""" | ||
| data = yaml.safe_load(yaml_str) | ||
| if not data: | ||
| return cls() | ||
| if not isinstance(data, dict): | ||
| return cls() | ||
| lock = cls( | ||
| lockfile_version=data.get("lockfile_version", "1"), | ||
| generated_at=data.get("generated_at", ""), | ||
| apm_version=data.get("apm_version"), | ||
| ) | ||
| for dep_data in data.get("dependencies", []): | ||
| lock.add_dependency(LockedDependency.from_dict(dep_data)) | ||
| lock.mcp_servers = list(data.get("mcp_servers", [])) | ||
| lock.mcp_configs = dict(data.get("mcp_configs") or {}) | ||
| lock.lsp_servers = list(data.get("lsp_servers", [])) | ||
| lock.lsp_configs = dict(data.get("lsp_configs") or {}) | ||
| lock.local_deployed_files = list(data.get("local_deployed_files", [])) |
| @staticmethod | ||
| def remove_stale( | ||
| stale_names: builtins.set, | ||
| project_root=None, | ||
| user_scope: bool = False, | ||
| logger=None, | ||
| ) -> None: | ||
| """Remove LSP server entries that are no longer required by any dependency. | ||
|
|
||
| Cleans up .lsp.json at project root or ~/.claude.json for user scope. | ||
|
|
||
| Args: | ||
| stale_names: Set of LSP server names to remove. | ||
| project_root: Project root directory. | ||
| user_scope: If True, clean user-level config (~/.claude.json). | ||
| logger: Optional logger instance. | ||
| """ | ||
| if logger is None: | ||
| logger = NullCommandLogger() | ||
| if not stale_names: | ||
| return | ||
|
|
||
| project_root_path = Path(project_root) if project_root is not None else Path.cwd() | ||
|
|
||
| # Clean project .lsp.json | ||
| if not user_scope: | ||
| lsp_json = project_root_path / ".lsp.json" | ||
| if lsp_json.exists(): | ||
| try: | ||
| config = json.loads(lsp_json.read_text(encoding="utf-8")) | ||
| if isinstance(config, dict): | ||
| removed = [n for n in stale_names if n in config] | ||
| for name in removed: | ||
| del config[name] | ||
| if removed: | ||
| lsp_json.write_text( | ||
| json.dumps(config, indent=2) + "\n", encoding="utf-8" | ||
| ) | ||
| for name in removed: | ||
| logger.progress(f"Removed stale LSP server '{name}' from .lsp.json") | ||
| except Exception: | ||
| _log.debug( | ||
| "Failed to clean stale LSP servers from .lsp.json", | ||
| exc_info=True, | ||
| ) | ||
|
|
||
| # Clean user ~/.claude.json (lspServers section) | ||
| if user_scope: | ||
| claude_user = Path.home() / ".claude.json" | ||
| if claude_user.exists(): | ||
| try: | ||
| config = json.loads(claude_user.read_text(encoding="utf-8")) | ||
| if isinstance(config, dict): | ||
| servers = config.get("lspServers", {}) | ||
| if isinstance(servers, dict): | ||
| removed = [n for n in stale_names if n in servers] | ||
| for name in removed: | ||
| del servers[name] | ||
| if removed: | ||
| claude_user.write_text( | ||
| json.dumps(config, indent=2) + "\n", encoding="utf-8" | ||
| ) | ||
| for name in removed: | ||
| logger.progress( | ||
| f"Removed stale LSP server '{name}' from ~/.claude.json" | ||
| ) | ||
| except Exception: | ||
| _log.debug( | ||
| "Failed to clean stale LSP servers from ~/.claude.json", | ||
| exc_info=True, | ||
| ) | ||
|
|
Add unit tests for LSPDependency model, plugin parser, LSP install integration, and LSPIntegrator. Add CHANGELOG entry. Export LSPIntegrator from integration __init__. Extract shared deduplicate and locked-path resolution helpers to fix R0801 duplication lint. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 3 | 2 | Solid MCP-mirror pattern; one correctness bug (falsy-zero timeout silenced by or), install duplication, and unnecessary builtins aliasing are the main issues. |
| CLI Logging Expert | 2 | 1 | 1 | Logger is never forwarded into LSPIntegrator.install() or remove_stale(), silently dropping all LSP progress messages; lsp_count is also discarded from the post-install summary. |
| DevX UX Expert | 0 | 3 | 2 | LSP manifest UX is clean and familiar; chief UX flaw is --only=apm silently skipping LSP with no user-visible indication, and string-only form semantics are under-signalled. |
| Supply Chain Security Expert | 0 | 5 | 1 | Transitive LSP deps are fully trusted and can write to ~/.claude.json; args/env/workspace_folder pass through unvalidated; no hash pinning in lockfile. |
| OSS Growth Hacker | 0 | 3 | 2 | LSP support completes the 'one manifest, all tools' story -- a genuine milestone, but the README hero and release beat don't yet reflect it, leaving the biggest conversion surface stale. |
| Auth Expert | 0 | 1 | 1 | LSP integration is auth-neutral: no AuthResolver, token manager, or HTTP calls touched. One credential-leakage surface worth documenting: env dict written verbatim to disk. |
| Doc Writer | 0 | 1 | 3 | Docs are structurally sound and accurate to the code; one parity gap in the lockfile drift-checks table (no lsp-configs-match row) and one minor voice inconsistency. |
| Test Coverage Expert | 0 | 5 | 0 | Zero tests exist for the new LSP dependency kind; all 11 behavioral surfaces (model validation, integrator, pipeline step, lockfile round-trip) are completely uncovered. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [CLI Logging Expert] (blocking-severity) Forward logger (and diagnostics) into LSPIntegrator.install() and remove_stale() so LSP progress is visible -- Users see nothing when LSP servers are written or removed. A silent install is indistinguishable from a broken one. This is an output correctness regression on a user-observable surface.
- [Test Coverage Expert] (blocking-severity) Add unit + integration tests for LSPDependency model, LSPIntegrator scope routing, run_lsp_integration pipeline, lockfile round-trip, and plugin_parser LSP extraction -- Zero automated coverage on all eleven new surfaces. Scope-routing logic that writes to ~/.claude.json with no regression guard is a liability the maintainer will pay on the next refactor.
- [Python Architect] (blocking-severity) Fix falsy-zero bug in LSPDependency.from_dict(): replace
orchains with explicitis not Noneguards for startup_timeout, shutdown_timeout, and max_restarts -- A timeout of 0 is semantically valid ('do not wait') and is silently dropped today. The fix is three lines and the correct pattern already exists in the same file for restart_on_crash. - [Supply Chain Security Expert] Add a trust gate for transitive LSP deps and emit an install-time warning when any LSP dep carries a non-empty env dict -- A compromised transitive package can inject a 'command' pointing to any binary; combined with user_scope writes to ~/.claude.json this is a cross-project persistence surface. The env-dict credential-leakage warning (auth-expert corroboration) is a one-liner.
- [OSS Growth Hacker] Update README hero to show all three dependency kinds, add a CHANGELOG milestone beat, and file good-first-issues for Cursor/OpenCode LSP adapters linked from the runtime support table -- The README hero is the top-of-funnel conversion surface for the 'one manifest' claim. A stale hero kills the claim before it lands.
Architecture
classDiagram
direction LR
class LSPDependency {
<<ValueObject>>
+name str
+command str
+transport str
+extension_to_language dict
+startup_timeout int
+from_string(s) LSPDependency
+from_dict(d) LSPDependency
+to_dict() dict
+to_lsp_json_entry() dict
+validate(strict) None
}
class MCPDependency {
<<ValueObject>>
+name str
+transport str
+from_string(s) MCPDependency
+from_dict(d) MCPDependency
+validate(strict) None
}
class LSPIntegrator {
<<StaticNamespace>>
+collect_transitive(apm_modules_dir) list
+deduplicate(deps) list
+get_server_names(lsp_deps) set
+get_server_configs(lsp_deps) dict
+remove_stale(stale_names, project_root) None
+update_lockfile(lsp_server_names, lock_path) None
+install(lsp_deps, project_root) int
}
class MCPIntegrator {
<<StaticNamespace>>
+collect_transitive() list
+deduplicate() list
+install() int
+remove_stale() None
+update_lockfile() None
}
class LockFile {
<<ValueObject>>
+lsp_servers list
+lsp_configs dict
+mcp_servers list
+mcp_configs dict
+read(path) LockFile
+save(path) None
}
class APMPackage {
+get_lsp_dependencies() list
+get_dev_lsp_dependencies() list
+get_mcp_dependencies() list
}
class run_lsp_integration {
<<PureProcedure>>
+run_lsp_integration(apm_package, lock_path, ...) int
}
class run_mcp_integration {
<<PureProcedure>>
+run_mcp_integration(apm_package, lock_path, ...) int
}
LSPIntegrator ..> LSPDependency : reads
LSPIntegrator ..> LockFile : updates
run_lsp_integration ..> LSPIntegrator : delegates
run_lsp_integration ..> APMPackage : reads
run_lsp_integration ..> LockFile : reads existing
run_mcp_integration ..> MCPIntegrator : delegates
run_mcp_integration ..> APMPackage : reads
APMPackage ..> LSPDependency : produces
APMPackage ..> MCPDependency : produces
class LSPDependency:::touched
class LSPIntegrator:::touched
class run_lsp_integration:::touched
class LockFile:::touched
class APMPackage:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A([apm install CLI]) --> B[commands/install.py]
B --> C{should_install_mcp\nreused as LSP gate}
C -->|True| D[run_lsp_integration\ninstall/lsp/integration.py]
C -->|False| E[restore old lsp_servers\nto lockfile only]
D --> F[APMPackage.get_lsp_dependencies\nmodels/apm_package.py]
F --> G[direct LSPDependency list]
D --> H[LSPIntegrator.collect_transitive\nintegration/lsp_integrator.py]
H --> I[I/O: scan apm_modules/*/apm.yml]
I --> J[transitive LSPDependency list]
G & J --> K[LSPIntegrator.deduplicate\nfirst-occurrence wins]
K --> L[deduplicated lsp_deps]
L --> M[LSPIntegrator.install]
M --> N{user_scope?}
N -->|No| O[FS: read+merge .lsp.json\nat project_root]
N -->|Yes| P[FS: read+merge ~/.claude.json\nlspServers section]
D --> Q[old_lsp_servers from LockFile]
L --> R[LSPIntegrator.get_server_names]
R --> S[new_lsp_servers set]
Q & S --> T[stale = old - new]
T --> U[LSPIntegrator.remove_stale]
U --> V[FS: delete entries from .lsp.json\nor ~/.claude.json]
S --> W[LSPIntegrator.update_lockfile]
W --> X[FS/LOCK: write lsp_servers+lsp_configs\nto apm.lock.yaml]
sequenceDiagram
participant User
participant Install as commands/install.py
participant Integration as run_lsp_integration
participant Integrator as LSPIntegrator
participant FS as Filesystem
User->>Install: apm install
Install->>Integration: run_lsp_integration(should_install=should_install_mcp)
Integration->>Integrator: collect_transitive(apm_modules_path)
Integrator->>FS: read apm_modules/*/apm.yml
FS-->>Integrator: LSPDependency list
Integrator-->>Integration: transitive deps
Integration->>Integrator: deduplicate(direct + transitive)
Integration->>Integrator: install(lsp_deps, project_root)
Integrator->>FS: read .lsp.json
Integrator->>FS: write .lsp.json (merged)
Integration->>Integrator: remove_stale(old - new)
Integrator->>FS: delete stale entries from .lsp.json
Integration->>Integrator: update_lockfile(new_lsp_servers)
Integrator->>FS: write apm.lock.yaml
Integration-->>Install: count of servers configured
Install-->>User: install summary
Recommendation
Hold for three required fixes before merge: (1) forward the logger into LSPIntegrator.install() and remove_stale() -- a feature that writes to disk with no user-visible output is broken by definition; (2) add baseline test coverage for the model, integrator, pipeline, lockfile, and plugin_parser surfaces -- scope-routing logic that touches ~/.claude.json with no regression guard is a liability the maintainer will pay on the next refactor; (3) fix the falsy-zero timeout bug in from_dict() -- it is three lines and the correct pattern is already in the file. Once those three are in, the PR is a clean ship. The supply-chain hardening (transitive trust gate, args/env validation, lockfile hash) and growth-surface work (README, CHANGELOG, contributor funnel) are important but can land as tracked follow-up issues in the same release cycle -- they do not gate the milestone.
Full per-persona findings
Python Architect
-
[recommended] from_dict uses
orto merge camelCase/snake_case for integer fields -- silences explicit zero values atsrc/apm_cli/models/dependency/lsp.py:71
startup_timeout=d.get('startupTimeout') or d.get('startup_timeout') returns the snake_case value when startupTimeout is 0. A timeout of 0 (meaning 'do not wait') is a semantically valid config that gets silently dropped. The same flaw applies to shutdown_timeout and max_restarts. restart_on_crash correctly uses an explicitis not Noneguard -- that pattern should be applied consistently to all nullable scalar fields.
Suggested: Replaced.get('startupTimeout') or d.get('startup_timeout')withd.get('startupTimeout') if d.get('startupTimeout') is not None else d.get('startup_timeout')(same pattern already used for restart_on_crash). Apply identically to shutdown_timeout and max_restarts. -
[recommended] install() duplicates the read-merge-write block for user_scope vs project-scope paths at
src/apm_cli/integration/lsp_integrator.py:309
Lines 310-364 repeat the same read-existing-JSON, merge-servers, write pattern twice (~30 lines each). When a third target surface is added (e.g. Cursor, Zed), this block gets copied a third time. Extract a _merge_and_write_json helper now while the duplication is still just 2x.
Suggested: def _merge_and_write_json(path: Path, servers: dict, nested_key: str | None, logger) -> int -
[recommended] LSP install is gated by should_install_mcp -- couples two independent feature flags at
src/apm_cli/commands/install.py
Passing should_install=should_install_mcp to run_lsp_integration couples LSP and MCP gates. Future --only flags will be painful to untangle.
Suggested: Derive should_install_lsp as a separate variable using the same --only logic. Today they produce the same value, so no behaviour changes; the separation makes future --only=lsp or --only=mcp diffs trivial. -
[nit] builtins.set / builtins.dict aliasing is cargo-culted from MCPIntegrator where it was needed at
src/apm_cli/integration/lsp_integrator.py:59
MCPIntegrator uses builtins.set because it has a local variable that shadows the name. LSPIntegrator has no such shadowing; using builtins.set and builtins.dict everywhere is noise that confuses readers.
Suggested: Replace builtins.set(...) and builtins.dict(...) with plain set() and dict() throughout LSPIntegrator and run_lsp_integration. -
[nit] _VALID_TRANSPORTS on LSPDependency dataclass lacks ClassVar annotation at
src/apm_cli/models/dependency/lsp.py:41
Unannotated class-level assignments in a@dataclassare not picked up as fields at runtime, but omitting ClassVar[frozenset] makes it ambiguous to static analysers and future maintainers.
Suggested: from typing import ClassVar; _VALID_TRANSPORTS: ClassVar[frozenset[str]] = frozenset({'stdio', 'socket'})
CLI Logging Expert
-
[blocking] logger not passed to LSPIntegrator.install() -- all 'Configured N LSP server(s)' messages are silently dropped at
src/apm_cli/install/lsp/integration.py:76
run_lsp_integration() receives a logger but calls LSPIntegrator.install() without forwarding it. The integrator falls back to NullCommandLogger, so the progress() call never emits output. Users see nothing when LSP servers are written.
Suggested: Pass logger=logger (and diagnostics=diagnostics) to LSPIntegrator.install(), mirroring MCPIntegrator call pattern. -
[blocking] logger not passed to LSPIntegrator.remove_stale() -- stale-removal progress messages are silently dropped at
src/apm_cli/install/lsp/integration.py:88
Both remove_stale() call sites in integration.py omit the logger kwarg. The integrator falls back to NullCommandLogger, so 'Removed stale LSP server' messages are never emitted.
Suggested: Add logger=logger to both remove_stale() calls (lines 88 and 100). -
[recommended] lsp_count return value from run_lsp_integration() is discarded -- LSP servers never appear in post-install summary at
src/apm_cli/commands/install.py:1895
install.py calls run_lsp_integration() without capturing its return value. render_post_install_summary() has no lsp_count parameter. Users see a summary that looks like nothing happened.
Suggested: Capture lsp_count = run_lsp_integration(...), thread it through _install_apm_packages return tuple, and include 'N LSP server(s) configured' in the summary line. -
[nit] Progress verb mismatch: 'Configured' for install vs 'Registered' for MCP at
src/apm_cli/integration/lsp_integrator.py:359
Suggested: Use 'Registered N LSP server(s) in .lsp.json' to mirror MCPIntegrator wording.
DevX UX Expert
-
[recommended] --only=apm silently skips LSP with no indication; InstallMode has no LSP value at
src/apm_cli/commands/install.py
LSP is a third peer kind but is invisibly bundled under the MCP gate. --only help text still says 'apm or mcp'. A user who wants only APM packages but does want LSP gets silent omission.
Suggested: Add InstallMode.LSP = 'lsp' and a separate gate, or update the --only help string to list 'apm, mcp, lsp' and document the LSP-under-MCP coupling explicitly. -
[recommended] String-only form (e.g. '- gopls') silently writes empty {} to .lsp.json when no transitive dep resolves it at
src/apm_cli/integration/lsp_integrator.py
No warning emitted at install time when a string-only LSP dep fails to resolve to a full definition.
Suggested: After deduplication, warn for any LSPDependency whose command is None: "LSP server 'gopls' has no command -- it may not be fully resolved from transitive packages." -
[recommended] Docs use camelCase field names (extensionToLanguage) but YAML convention expects snake_case at
docs/src/content/docs/consumer/install-lsp-servers.md
from_dict() accepts both but the docs only show camelCase. A user who writes snake_case will not know it works.
Suggested: Add a callout: 'Both camelCase (extensionToLanguage) and snake_case (extension_to_language) are accepted in apm.yml.' -
[nit] Error message references 'extensionToLanguage' (camelCase) not the key the user typed at
src/apm_cli/models/dependency/lsp.py
Suggested: Change error to: "requires 'extensionToLanguage' (or 'extension_to_language')" -
[nit] install.py help text still says '--only filters by type (apm or mcp)' -- does not mention lsp at
src/apm_cli/commands/install.py
Supply Chain Security Expert
-
[recommended] Transitive LSP deps from installed packages carry blanket executable trust with no user gate at
src/apm_cli/integration/lsp_integrator.py:47
collect_transitive() docs explicitly state 'All LSP servers from installed packages are trusted'. A compromised transitive package can inject a 'command' pointing to any binary. Combined with user_scope=True writes to ~/.claude.json, this creates a cross-project persistence attack surface.
Suggested: Add a trust gate: require transitive LSP servers to be opt-in via 'allowTransitiveLsp: true' or restrict user_scope writes to root-declared deps only. -
[recommended] 'args' list items are written to LSP config without any validation at
src/apm_cli/models/dependency/lsp.py:29
Unvalidated args are written verbatim into .lsp.json and become the argument vector for the LSP binary. Path traversal sequences in args bypass the command-level path guard.
Suggested: Apply validate_path_segments() or a type+len check to each arg item in validate(). -
[recommended] 'env' dict values pass through with no validation, enabling PATH/LD_PRELOAD injection at
src/apm_cli/models/dependency/lsp.py:32
A malicious plugin can set LD_PRELOAD or PATH in env, causing arbitrary code execution in the LSP server process.
Suggested: Apply a key blocklist (LD_PRELOAD, DYLD_INSERT_LIBRARIES, LD_LIBRARY_PATH, PATH) or surface a warning at install time when sensitive keys are present. -
[recommended] 'workspace_folder' written to LSP config without path containment check at
src/apm_cli/models/dependency/lsp.py:35
Suggested: Apply validate_path_segments() to workspace_folder in validate(), consistent with how 'command' is treated. -
[recommended] lsp_configs in lockfile has no content hash -- no integrity baseline for the binary that will execute at
src/apm_cli/integration/lsp_integrator.py:248
Suggested: Record a SHA-256 of the resolved 'command' binary at install time in lsp_configs. -
[nit] _read_lsp_file resolves target before symlink check but checks the unresolved candidate for is_symlink() at
src/apm_cli/deps/plugin_parser.py:472
OSS Growth Hacker
-
[recommended] README hero example still shows only apm: and mcp: -- LSP is invisible at the top of the funnel
Suggested: Add a third block under the README yaml example showing lsp: with the gopls one-liner. -
[recommended] No CHANGELOG / release narrative beat surfaced for the 'third dependency kind' milestone
Suggested: CHANGELOG entry should open with the user benefit headline: 'APM now manages your full AI dev toolchain: packages, MCP servers, and LSP servers in a single apm.yml.' -
[recommended] Runtime support table is a latent contributor magnet with no call-to-action
Suggested: Add 'Want to add support for your runtime? See [issue #XXXX]' below the runtime table. File a good-first-issue for each unimplemented runtime. -
[nit] Doc page title 'Install LSP servers' undersells lifecycle management
Suggested: Change frontmatter title to 'Manage LSP servers'. -
[nit] 'Claude Code only' framing buries the extensibility proof before readers reach the runtime table
Auth Expert
-
[recommended] env dict on LSPDependency is written verbatim to .lsp.json / ~/.claude.json with no redaction or warning at
src/apm_cli/models/dependency/lsp.py
If an apm.yml author populates env with real credential values, those values land in .lsp.json which is typically committed to version control. No warning at install time.
Suggested: Emit a one-line install-time warning when any LSP dep carries a non-empty env dict. Also add .lsp.json to recommended .gitignore entries in docs. -
[nit] ~/.claude.json read-modify-write is not file-locked at
src/apm_cli/integration/lsp_integrator.py
Two concurrent apm install runs could clobber each other. Same pattern exists in MCP integrator so this is not a regression.
Suggested: Use a filelock around the read-modify-write, consistent with any locking done for mcp_integrator.
Doc Writer
-
[recommended] lockfile-spec.md drift-checks table omits lsp-configs-match row at
docs/src/content/docs/reference/lockfile-spec.md:191
The 'Drift and integrity' table lists mcp-configs-match but has no equivalent row for lsp_servers/lsp_configs. The runtime equivalence check exists in lockfile.py but the doc does not surface it.
Suggested: Add a row: | lsp-configs-match | lsp_servers and lsp_configs | -- mirroring the mcp-configs-match row. -
[nit] install-lsp-servers.md 'One-line answer' leads with multi-line YAML -- heading creates false expectation of a CLI shortcut at
docs/src/content/docs/consumer/install-lsp-servers.md:18
Suggested: Rename the section to 'Quick start' or add a sentence noting that LSP servers are declared only in apm.yml (no --lsp CLI flag). -
[nit] install-lsp-servers.md missing sidebar.order -- page will sort by filename at
docs/src/content/docs/consumer/install-lsp-servers.md:1 -
[nit] lockfile-spec.md full Example block does not show lsp_servers/lsp_configs fields at
docs/src/content/docs/reference/lockfile-spec.md:222
Suggested: Append lsp_servers/lsp_configs fields to the Example block to mirror the top-level structure example.
Test Coverage Expert
-
[recommended] LSPDependency model validation has no unit tests at
src/apm_cli/models/dependency/lsp.py
from_string(), from_dict(), and validate() -- zero hits in tests/. If validation drifts silently, malformed entries reach the integrator with no assertion firing.
Proof (missing):tests/unit/models/test_lsp_dependency.py-- proves: LSPDependency rejects invalid names, path-traversal commands, and unknown transports before they reach the integrator [secure-by-default] -
[recommended] LSPIntegrator install/remove_stale/deduplicate have no integration tests at
src/apm_cli/integration/lsp_integrator.py
Scope-routing logic (project vs user) has no test that would catch a regression.
Proof (missing):tests/integration/test_lsp_integrator.py-- proves: apm install writes LSP server entries to .lsp.json for project-scope and ~/.claude.json for user-scope [devx] -
[recommended] run_lsp_integration pipeline step has no integration test at
src/apm_cli/install/lsp/integration.py
Proof (missing):tests/integration/test_lsp_install_pipeline.py-- proves: apm install with LSP entries in apm.yml results in LSP servers written to disk and recorded in the lockfile [portability-by-manifest] -
[recommended] LockFile lsp_servers/lsp_configs round-trip and is_semantically_equivalent with LSP fields have no tests at
src/apm_cli/deps/lockfile.py
Proof (missing):tests/unit/test_lockfile_lsp_roundtrip.py-- proves: A lockfile with lsp_servers and lsp_configs survives to_yaml/from_yaml round-trip and is_semantically_equivalent correctly detects when LSP fields differ [portability-by-manifest] -
[recommended] plugin_parser LSP extraction has no unit test at
src/apm_cli/deps/plugin_parser.py
Proof (missing):tests/unit/deps/test_plugin_parser_lsp.py-- proves: plugin_parser extracts LSP server entries from a plugin.json that declares them, returning LSPDependency objects [portability-by-manifest]
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Note
🔒 Integrity filter blocked 2 items
The following items were blocked because they don't meet the GitHub integrity level.
- #1424
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved". - feat(lsp): add first-class LSP server support to install pipeline #1424
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by PR Review Panel for issue #1424 · ● 4.9M · ◷
github-actions
Bot
removed
the
panel-review
Third controlled run, fixed-corpus rerun: - Architect C (v0.3.1, fixed binding-site corpus) produced a 339-line handoff with explicit per-element MODEL BINDING TABLE: 6 of 7 sites bound to non-default SKUs (5 lenses + arbiter, planner=Opus, trivial=Haiku, others=Sonnet). - Executor C honored the binding by passing model: to each task sub-agent dispatch. Telemetry confirms 3 distinct models in real billing: 15 turns on claude-haiku-4.5, 37 on claude-sonnet-4.6, 29 on claude-opus-4.7. - Output quality parity with Exec A and B: catches the LSP env-RCE CRITICAL plus 1 HIGH + 9 MEDIUM + 9 LOW. Key empirical finding: B12 MODEL ROUTER fires as designed at the sub-agent level (~10x cheaper per-turn on Haiku-bound trivial lenses) but the orchestrator's session-default model dominates total cost when it is more expensive than the routed sub-agents. In this run the executor session ran on Opus 4.7 by default; the 29 orchestrator turns alone cost $6.14, swamping the $2 saved by Haiku routing. Counterfactual with orchestrator bound to Sonnet (matching handoff intent): $2.73, which would be 10% cheaper than Exec B's $3.02. This generates a new corpus lesson for v0.3.2: B12 must include the orchestrator thread as a binding site. PR description (and REPORT.md mirror) updated with full 3-way FinOps analysis and the proposed v0.3.2 corpus addition. Artifacts: - architect-C-v0.3.1-handoff.md - executor-C-v0.3.1-review.md - executor-C-tokens.json - executor-C-process.log.gz (3MB, 19MB uncompressed) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lled orchestrator The earlier 3-cell A/B/C runs all had Opus session-default orchestrators that masked the corpus-level signal. This commit adds a clean 2-cell A/B with the only variable being the genesis corpus version: - Both architects: claude-opus-4.7 (design tier) - Both executor orchestrators: claude-sonnet-4.6 (pinned) - Same target PR: microsoft/apm#1424 - Same lens count: 5 (correctness, security, performance, style, test-coverage) Result: - Cell D (v0.1 baseline, no cost-aware corpus): $11.77 total, 52 findings, 6 BLOCKER - Cell E (v0.3.1 treatment, B12 fires at 7 sites): $14.68 total (+25%), 61 findings, 4 CRITICAL + 10 more above threshold Cell E catches 2 additional CRITICAL security findings Cell D missed (SEC-001 TOCTOU symlink race, SEC-002 validated-object discarded). Honest reframing: in Copilot CLI where task(explore) defaults to Haiku, B12 promotes lenses UP from Haiku to Sonnet, which is a quality-routing knob, NOT a cost-reduction knob. The v0.3.1 corpus over-applies B12 by encouraging explicit binding at every .agent.md primitive. v0.3.2 must add a B12 SELECTION RULE: bind explicitly only when stakes, portability, or operator economic preference justifies it; otherwise trust the harness default. Artifacts added: - architect-D-v0.1-handoff.md, architect-E-v0.3.1-handoff.md - executor-D-v0.1-review.md, executor-E-v0.3.1-review.md - executor-D-findings.json, executor-E-findings.json - architect-D-process.log.gz, executor-D-process.log.gz - architect-E-process.log.gz, executor-E-process.log.gz - tools/profile-per-model.py (new per-model attribution profiler) - REPORT.md updated to v4 with the clean 2-cell story PR #12 body updated to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
After Cell F (v0.3.2 SELECTION RULE) only got to +7% vs v0.1 baseline, RCA #2 named the residual cost driver: synth-heavy dispatched to Opus by default for cross-lens adjudication = HEAVY ADJUDICATOR anti-pattern. v0.3.2.1 corpus edit (architectural-patterns.md §A12): - HEAVY ADJUDICATOR anti-pattern: synthesis that reconciles already- produced lens findings is reviewer-class, not planner-class. - "WHERE THE HEAVY ROLE BELONGS" cure paragraph: bind planner only on rare, narrow triggers (>=2 BLOCKERs + contradictory + same diff hunk; expected firing rate ~2-4%). - Cell F named as the empirical canonical case. Cell G result on microsoft/apm#1424: - Architect G (Opus, 20 turns): $7.34 - Executor G (Sonnet orch, 179 turns): $2.85 -> 115 Haiku ($0.91) + 64 Sonnet ($1.93) + 0 Opus ($0) - TOTAL: $10.19 (-13.4% vs Cell D baseline $11.77) - Opus arbiter correctly stayed dark (narrow trigger not met) - Bug-finding parity with Cell D (same class of issues) - 2 false-positive BLOCKERs caught and downgraded via inline gh-api verification (Sonnet orchestrator), avoiding wasteful Opus dispatch Final cell table: | Cell | Corpus | Total | Delta vs v0.1 | | D | v0.1 | $11.77 | baseline | | E | v0.3.1 | $14.68 | +24.7% (FAIL)| | F | v0.3.2 | $12.63 | +7.3% (PARTIAL)| | G | v0.3.2.1| $10.19 | -13.4% (WIN) | This commit ships: - The v0.3.2.1 corpus edit (HEAVY ADJUDICATOR anti-pattern in A12) - Cell F + G architect handoffs, executor reviews, gzipped process logs, per-lens findings JSONs - REPORT.md rewritten to v5 with full D/E/F/G arc, iteration narrative, mermaid diagrams for Cell G (winner) and Cell E (over-bound failure), and named load-bearing corpus edits Corpus claim now empirically grounded: cost-aware genesis corpus produces designs neatly cheaper than the unconscious v0.1 baseline, with parity on bug-finding quality. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Implements Language Server Protocol (LSP) integration mirroring the MCP pattern:
LSP config is written to .lsp.json at project root (Claude Code auto-discovers this file). Unlike MCP which targets multiple runtimes, LSP is Claude Code-only for now.
This enables plugins that declare both MCP servers and LSP servers (like gopls for Go support) to have both integrated end-to-end.
Type of change
Testing