From 10e8167ef733701708d451ffe704dd0f8579db2a Mon Sep 17 00:00:00 2001 From: CleverThis Date: Mon, 15 Jun 2026 00:39:43 -0400 Subject: [PATCH] =?UTF-8?q?fix(lsp):=20address=20reviewer=20blockers=20?= =?UTF-8?q?=E2=80=94=20remove=20duplicate=20step,=20move=20LspError=20impo?= =?UTF-8?q?rt,=20fix=20metadata?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove duplicate @then("ltcov the transport should be alive") in features/steps/lsp_transport_coverage_steps.py; the same step is already defined at features/steps/lsp_transport_post_spawn_cleanup_steps.py:92 for issue #7044. The duplicate caused AmbiguousStep at behave load time, erroring every BDD feature in the suite (root cause of the CI unit_tests + coverage gate failures). - Remove redundant inline `from cleveragents.lsp.errors import LspError` inside StdioTransport.start(); the module-level import at line 30 is sufficient and inline imports violate the project import rule. - Update CONTRIBUTORS.md: correct the PR number from #11160 (the linked issue) to #11185 (this PR). - features/lsp_transport_subprocess_cleanup.feature: add the required `@tdd_issue @tdd_issue_11160` tags per the TDD bug-fix workflow, fix the `returnscode` typo in a scenario name, and rename the `dye_on_start` command label to `die_on_start`. ISSUES CLOSED: #11160 --- CONTRIBUTORS.md | 2 +- features/lsp_transport_subprocess_cleanup.feature | 5 +++-- features/steps/lsp_transport_coverage_steps.py | 5 ----- src/cleveragents/lsp/transport.py | 2 -- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 8bdd8baa9..44dd3e8bf 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -120,4 +120,4 @@ Below are some specific details of individual PR contributions. * Jeffrey Phillips Freeman has contributed the `--format`/`-f` flag to `agents session tell` (issue #10466): adds JSON envelope output for machine-readable workflows alongside existing Rich console output, with Behave BDD test coverage verifying all four non-rich format paths (JSON, YAML, plain, table) and the short `-f` flag alias. * HAL 9000 has contributed the Semgrep guard for broad exception suppression (PR #9185 / issue #9103): added two new Semgrep rules (`python-no-suppressed-exception` and `python-no-suppress-exception`) to automate enforcement of error propagation guidelines, integrated Semgrep into `nox -s lint` in audit mode with migration plan for ~337 existing violations, and comprehensive BDD test coverage across all rule patterns and escape hatch scenarios. * HAL 9000 has contributed the `ProviderRegistry.FALLBACK_ORDER` fix (#10906): added the missing `ProviderType.GEMINI` to the fallback provider order list so that when only a Gemini API key is configured, the registry correctly selects it as the default provider. Includes BDD regression scenarios in `features/fallback_gemini_provider.feature`. -* HAL 9000 has contributed the StdioTransport subprocess cleanup on failed initialization fix (PR #11160 / issue #11160): after ``start()`` spawns a process with ``subprocess.Popen()``, the method now verifies the process is still alive via ``poll()``. If it died immediately during initialization, ``stop()`` is called to release the handle and an ``LspError`` with exit code is raised, preventing zombie processes and resource leaks. Includes BDD test coverage in ``features/lsp_transport_subprocess_cleanup.feature``. +* HAL 9000 has contributed the StdioTransport subprocess cleanup on failed initialization fix (PR #11185 / issue #11160): after ``start()`` spawns a process with ``subprocess.Popen()``, the method now verifies the process is still alive via ``poll()``. If it died immediately during initialization, ``stop()`` is called to release the handle and an ``LspError`` with exit code is raised, preventing zombie processes and resource leaks. Includes BDD test coverage in ``features/lsp_transport_subprocess_cleanup.feature``. diff --git a/features/lsp_transport_subprocess_cleanup.feature b/features/lsp_transport_subprocess_cleanup.feature index 869102381..32cdb82fa 100644 --- a/features/lsp_transport_subprocess_cleanup.feature +++ b/features/lsp_transport_subprocess_cleanup.feature @@ -1,3 +1,4 @@ +@tdd_issue @tdd_issue_11160 Feature: StdioTransport subprocess cleanup on failed initialization As a developer maintaining the LSP transport layer I need proper cleanup of subprocess handles when the process dies during start() @@ -27,14 +28,14 @@ Feature: StdioTransport subprocess cleanup on failed initialization When ltcov I try to start the transport Then ltcov the error should be an LspError with message "died on spawn" - Scenario: start raises LspError when process returnscode is zero (clean exit) + Scenario: start raises LspError when process returncode is zero (clean exit) Given ltcov I create a StdioTransport for command "quick_exit" And ltcov Popen is mocked to return a process that exited with code 0 When ltcov I try to start the transport Then ltcov the error should be an LspError with message "died on spawn" Scenario: stop() handles already-exited process and cleans up _process - Given ltcov I create a StdioTransport for command "dye_on_start" + Given ltcov I create a StdioTransport for command "die_on_start" And ltcov Popen is mocked to return a process that exited with code 42 When ltcov I try to start the transport Then ltcov the stop handles already-exited processes correctly diff --git a/features/steps/lsp_transport_coverage_steps.py b/features/steps/lsp_transport_coverage_steps.py index 2b2c21cdc..5bb7fa672 100644 --- a/features/steps/lsp_transport_coverage_steps.py +++ b/features/steps/lsp_transport_coverage_steps.py @@ -543,11 +543,6 @@ def step_ltcov_process_none_after_failed_start(context: Context) -> None: # --------------------------------------------------------------------------- -@then("ltcov the transport should be alive") -def step_ltcov_alive(context: Context) -> None: - assert context.ltcov_transport.is_alive, "Expected transport to be alive" - - @then("ltcov no error should have been raised") def step_ltcov_no_error(context: Context) -> None: assert context.ltcov_error is None, ( diff --git a/src/cleveragents/lsp/transport.py b/src/cleveragents/lsp/transport.py index 31c17bbc1..c34219757 100644 --- a/src/cleveragents/lsp/transport.py +++ b/src/cleveragents/lsp/transport.py @@ -107,8 +107,6 @@ class StdioTransport: if self._process is not None and self.is_alive: raise RuntimeError("Transport already started") - from cleveragents.lsp.errors import LspError - merged_env = {**os.environ, **self._env} cmd = [self._command, *self._args]