fix(lsp): Omit params field for shutdown/exit methods#5
Closed
ketema wants to merge 1 commit into
Closed
Conversation
WHY:
- HLS (Haskell) and rust-analyzer use Void/unit types for shutdown/exit
- These strict-typed servers reject ANY params field (even empty {})
- PR oraios#851 changed to send params:{} for Delphi/FPC compatibility
- This broke HLS/rust-analyzer with "Cannot parse Void" errors
EXPECTED:
- shutdown/exit methods: omit params field entirely (HLS/rust-analyzer compat)
- Other methods with None: send params:{} (Delphi/FPC compat)
- Other methods with params: include them unchanged (standard behavior)
- Three-way compatibility preserved per JSON-RPC 2.0 spec
Refs: oraios#870
ketema
added a commit
that referenced
this pull request
Feb 6, 2026
WHY: - Race conditions could cause duplicate server startups - Silent exception swallowing made debugging impossible - Missing async context manager risked resource leaks - Ignored task results hid startup failures - Linear search caused performance degradation EXPECTED: - FIX oraios#1 (CRITICAL): Race condition prevented with asyncio.Lock per language - FIX #2 (CRITICAL): All exceptions properly logged with exc_info=True - FIX #3 (MAJOR): Async context manager pattern implemented - FIX #4 (MAJOR): Task results from asyncio.gather() logged and handled - FIX #5 (MINOR): O(1) extension lookup via cached extension→language mapping - All tests pass IMPLEMENTATION DETAILS: FIX oraios#1 - Race Condition Prevention: - Added self._startup_locks: dict[Language, asyncio.Lock] - _start_language_server() uses 'async with lock' pattern - Double-check after acquiring lock (another task may have started it) FIX #2 - Proper Error Logging: - TimeoutError now logs with exc_info=True - All exceptions include full stack traces - Clear error messages for debugging FIX #3 - Async Context Manager: - Implemented __aenter__() and __aexit__() - shutdown_all() now async - _shutdown_single_lsp() helper for concurrent cleanup - Proper resource cleanup guaranteed FIX #4 - Task Result Handling: - start_all() iterates over asyncio.gather() results - Logs success/failure for each language - Tracks which languages failed and why FIX #5 - Performance Optimization: - _build_extension_cache() creates extension→language mapping - get_language_for_file() uses cache first (O(1) lookup) - Fallback to full pattern matching for complex patterns - Significant speedup for projects with many languages TESTING: - Added test_async_context_manager_cleanup() - All 13 tests pass - Verified race condition protection - Verified graceful degradation still works
ketema
added a commit
that referenced
this pull request
Feb 6, 2026
WHY: - 5 critical/major/minor issues in Phase 1.3 Step 2 - Issues ranged from resource leaks to code quality violations EXPECTED: - FIX oraios#1 (CRITICAL): Proper async/sync LSPManager shutdown (no resource leaks) - FIX #2 (ERROR): Comprehensive error handling with validation and rollback - FIX #3 (WARNING): Race condition prevention via cached references - FIX #4 (WARNING): DRY compliance via extracted timeout calculation - FIX #5 (MINOR): Proper TYPE_CHECKING import for LSPManager - All 20 tests pass (7 agent + 13 lsp_manager) IMPLEMENTATION: FIX oraios#1 - Async/Sync Mismatch (CRITICAL): - Added LSPManager.shutdown_all_sync() synchronous wrapper - Uses asyncio.run() with fallback for running event loops - SerenaAgent.reset_lsp_manager() now calls shutdown_all_sync() - Prevents resource leaks, zombie processes, hanging connections FIX #2 - Missing Error Handling (ERROR): - Added try/except around LSPManager creation in reset_lsp_manager() - Validates LSPManager creation succeeded (not None) - Rollback mechanism: restores old manager on failure - Comprehensive logging with exc_info=True for debugging FIX #3 - Race Conditions (WARNING): - Cache lsp_manager reference at start of methods - Prevents AttributeError if manager becomes None between check and access - Applied to: language_server property, is_language_server_running(), get_language_server_for_file(), reset_lsp_manager() FIX #4 - DRY Violation (WARNING): - Extracted _calculate_ls_timeout() method - Eliminates duplicated timeout logic in reset_lsp_manager() and reset_language_server() - Single source of truth for timeout calculation - Reduces maintenance burden and prevents inconsistency FIX #5 - Type Annotation (MINOR): - Added LSPManager to TYPE_CHECKING imports - Proper forward reference instead of string literal - Improves IDE support and type checking TESTING: - All 7 agent polyglot tests pass - All 13 LSPManager tests pass - No regressions in existing functionality - Backward compatibility maintained
ketema
added a commit
that referenced
this pull request
Feb 6, 2026
WHY: - add_workspace_root and remove_workspace_root were STUBS returning True without sending any LSP notification (CL3 violation) - Multi-root LSPs (rust-analyzer, pylsp, gopls) share instances across sessions but the second session's workspace was never registered - Tests were THEATER tests that only checked return value, not behavior (CL12-E, THEATER violation) EXPECTED: - add_workspace_root now sends workspace/didChangeWorkspaceFolders notification with the new folder in the 'added' list - remove_workspace_root now sends notification with folder in 'removed' list - LSP instances track workspace_roots list for path resolution - Tests now verify: return value, notification sent, correct params, tracking EVIDENCE: - T:test_lsp_capability_adapter::* = 36/36 PASS - POST-3 contract now enforced with behavioral assertions - CL12-E compliance: Tests cite "Enforces: POST-3" CONSTITUTIONAL FIX: Iteration 1 of ralph-loop for commit b4471d3 Fixes oraios#1-2 (stubs), #3-4 (missing clause IDs), #5-6 (theater tests)
ketema
added a commit
that referenced
this pull request
Feb 6, 2026
WHY: - Constitutional audit of GREEN phase found 5 documentation/contract inconsistencies between contracts and actual implementation EXPECTED: - Finding oraios#1 (HIGH): INV-05 in contract updated to reflect that 7 subclass files with method overrides need workspace_root, while _start_server implementations remain unchanged - Finding #2 (HIGH): INV-05 in requirements updated with same correction - Finding #3 (MEDIUM): retrieve_content_around_line contract signature now matches implementation (context_lines_before/after, workspace_root position, MatchedConsecutiveLines return type) - Finding #4 (MEDIUM): get_root_path() in symbol.py now documents the explicit-LSP edge case as acknowledged exception to CALLER-INV-1 - Finding #5 (MEDIUM): Log message at ls.py:1381 now references effective_root_str (workspace_root) instead of repository_root_path - 70/70 path resolution tests remain GREEN
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
params:{}for shutdown/exit methodsparamsfield entirely for shutdown/exit while maintaining{}for other methods (Delphi/FPC compatibility from PR Fix: Use {} instead of null in params field when None in JSON-RPC requests/notifications oraios/serena#851)Changes
_NO_PARAMS_METHODSconstant for methods requiring no params_build_params_field()helper for three-way compatibilitymake_notification()andmake_request()to use helperTest plan
References