fix(lsp): address reviewer blockers — remove duplicate step, move LspError import, fix metadata
CI / lint (pull_request) Successful in 53s
CI / typecheck (pull_request) Successful in 1m2s
CI / security (pull_request) Successful in 1m5s
CI / quality (pull_request) Successful in 1m1s
CI / build (pull_request) Successful in 50s
CI / helm (pull_request) Successful in 51s
CI / push-validation (pull_request) Successful in 35s
CI / unit_tests (pull_request) Successful in 5m11s
CI / docker (pull_request) Successful in 1m37s
CI / integration_tests (pull_request) Successful in 8m57s
CI / coverage (pull_request) Failing after 19m24s
CI / status-check (pull_request) Has been cancelled
CI / lint (pull_request) Successful in 53s
CI / typecheck (pull_request) Successful in 1m2s
CI / security (pull_request) Successful in 1m5s
CI / quality (pull_request) Successful in 1m1s
CI / build (pull_request) Successful in 50s
CI / helm (pull_request) Successful in 51s
CI / push-validation (pull_request) Successful in 35s
CI / unit_tests (pull_request) Successful in 5m11s
CI / docker (pull_request) Successful in 1m37s
CI / integration_tests (pull_request) Successful in 8m57s
CI / coverage (pull_request) Failing after 19m24s
CI / status-check (pull_request) Has been cancelled
- 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
This commit is contained in:
+1
-1
@@ -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.
|
* 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 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 `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``.
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
@tdd_issue @tdd_issue_11160
|
||||||
Feature: StdioTransport subprocess cleanup on failed initialization
|
Feature: StdioTransport subprocess cleanup on failed initialization
|
||||||
As a developer maintaining the LSP transport layer
|
As a developer maintaining the LSP transport layer
|
||||||
I need proper cleanup of subprocess handles when the process dies during start()
|
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
|
When ltcov I try to start the transport
|
||||||
Then ltcov the error should be an LspError with message "died on spawn"
|
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"
|
Given ltcov I create a StdioTransport for command "quick_exit"
|
||||||
And ltcov Popen is mocked to return a process that exited with code 0
|
And ltcov Popen is mocked to return a process that exited with code 0
|
||||||
When ltcov I try to start the transport
|
When ltcov I try to start the transport
|
||||||
Then ltcov the error should be an LspError with message "died on spawn"
|
Then ltcov the error should be an LspError with message "died on spawn"
|
||||||
|
|
||||||
Scenario: stop() handles already-exited process and cleans up _process
|
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
|
And ltcov Popen is mocked to return a process that exited with code 42
|
||||||
When ltcov I try to start the transport
|
When ltcov I try to start the transport
|
||||||
Then ltcov the stop handles already-exited processes correctly
|
Then ltcov the stop handles already-exited processes correctly
|
||||||
|
|||||||
@@ -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")
|
@then("ltcov no error should have been raised")
|
||||||
def step_ltcov_no_error(context: Context) -> None:
|
def step_ltcov_no_error(context: Context) -> None:
|
||||||
assert context.ltcov_error is None, (
|
assert context.ltcov_error is None, (
|
||||||
|
|||||||
@@ -107,8 +107,6 @@ class StdioTransport:
|
|||||||
if self._process is not None and self.is_alive:
|
if self._process is not None and self.is_alive:
|
||||||
raise RuntimeError("Transport already started")
|
raise RuntimeError("Transport already started")
|
||||||
|
|
||||||
from cleveragents.lsp.errors import LspError
|
|
||||||
|
|
||||||
merged_env = {**os.environ, **self._env}
|
merged_env = {**os.environ, **self._env}
|
||||||
cmd = [self._command, *self._args]
|
cmd = [self._command, *self._args]
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user