fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start() #11011
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
cleveragents/cleveragents-core!11011
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bugfix/pr-fix-10597-cleanup-subprocess-on-init-failure"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Changes
Rationale
On some platforms subprocess.Popen() can partially create a subprocess (fork succeeds but execve fails) before raising OSError. Without cleanup, the zombie process leaks into the calling address space. The same risk exists with generic resource errors that might leave a partially-created process behind.
Automated by CleverAgents Bot
Supervisor: Implementation | Agent: task-implementor
First Review: REQUEST_CHANGES
Overview
The core LSP fix in
src/cleveragents/lsp/transport.pyis conceptually correct and addresses the resource leak described in issue #7044. Thestop()call on OSError after_processis set is the right approach. However, there are multiple blocking issues preventing this PR from being merged, spanning CI failures, policy violations, test correctness bugs, and process compliance. All must be resolved before re-review.Blocking Issues
1. CI Failing — Required gates are red
The following required CI gates are failing:
lint— Failing after 1m29sunit_tests— Failing after 7m30sstatus-check— Failing (as a consequence of the above)benchmark-regression— Failing after 1m1s (non-required but needs attention)Per company policy, all CI gates (
lint,typecheck,security,unit_tests,coverage) must be green before a PR can be approved and merged. Fix all CI failures before requesting re-review.2.
type: ignoreannotations are categorically forbiddenThe step file
features/steps/stdio_transport_subprocess_cleanup_steps.pycontains 11# type: ignoreannotations. The project has zero tolerance for# type: ignore— not a single one is permitted anywhere in the codebase, regardless of context. This will fail thetypecheckCI gate once the type annotations are enforced on the test files. All of these must be removed by properly annotating the code.3. Wrong Gherkin step decorator —
@givenused where@thenis requiredIn
features/steps/stdio_transport_subprocess_cleanup_steps.py, line 61, the step"no subprocess was leaked (no orphan processes exist)"is decorated with@given. However, in the feature file, this step appears as:In Behave,
Andinherits the keyword of the preceding step — in this caseThen. Behave will look for a@then-decorated implementation and fail with a step-not-found error. This is the likely cause of theunit_testsCI failure. Change@givento@then.4. PR body does not contain
Closes #7044— the actual issueThe PR body says "Fix for PR #10597" but #10597 is a PR, not an issue. The real bug issue being resolved is #7044 ("Bug: LSP Transport Process Resource Leak on Exception in StdioTransport.start()"). The PR body must include
Closes #7044to trigger automatic issue closure and satisfy the PR requirements checklist. Without a closing keyword linking to the actual issue, the PR will not be reviewed or merged.5. Branch name does not match issue Metadata
Issue #7044 Metadata specifies:
This PR uses
bugfix/pr-fix-10597-cleanup-subprocess-on-init-failure. Per contributing rules, the branch must match the Branch field in the issue Metadata section verbatim. Please resubmit from the correct branch.6. Commit footer references a PR number, not an issue number
The commit footer says:
But #10597 is a PR, not an issue. The footer must reference the actual issue being closed:
ISSUES CLOSED: #7044. The Conventional Changelog tooling and project policy both require this to link commits to issues.7. TDD issue tag references wrong number
The Behave scenarios are tagged
@tdd_issue @tdd_issue_10597. Per the TDD workflow,@tdd_issue_Nmust reference the issue number being fixed, not a PR number. These tags should be@tdd_issue @tdd_issue_7044. Additionally, per the TDD bug fix workflow, the issue-capture test scenarios should have been initially submitted with@tdd_expected_failto prove the bug exists before the fix, and then the@tdd_expected_failtag removed when the fix is applied. The current scenarios lack this tag entirely, which means they were never in the "proving the bug" state. Please follow the three-tag TDD workflow correctly.8. No milestone assigned
The PR checklist explicitly shows
[ ] Milestone assigned. Issue #7044 is on milestonev3.6.0— the PR must be assigned to the same milestone. This is a hard merge requirement.9. No
Type/label appliedThe PR checklist shows
[ ] Labels applied. Exactly oneType/label is mandatory. For a bug fix this should beType/Bug.10. PR branch contains 38 commits spanning multiple unrelated concerns
This PR has 38 commits including:
.opencode/agents/(~6,875 lines)docs/specification.mdPer project rules: one issue = one commit, and each PR is associated with exactly one Epic. The LSP fix commit
4888e76dis correctly scoped — the other 37 commits must not be in this PR. They appear to belong to other branches/PRs already merged to master. Please rebase this branch onto master, keeping only the single commit for the LSP fix.Non-Blocking Observations
Suggestion: Scenario 2 test logic is fragile
The
step_patch_popenstep (line 81) sets up a patcher that returns a mock process on the firstPopencall and raisesOSErroron the second call. However, the productionstart()method only callsPopenonce — it uses the returned process object directly. The secondPopencall never happens, so theOSErroris never triggered via this mechanism. The scenario likely passes only by coincidence of another code path, or may be failing in CI. Consider patchingPopento raiseOSErrorafter yielding a mock process by directly settingself._processvia a side-effect function instead.Suggestion: Step function type annotations missing
All step functions lack return type annotations (they use
# noqa: ANN201to suppress the warning). Once# type: ignoreis removed, proper-> Nonereturn type annotations should be added to all step functions to comply with Pyright strict mode.Summary
stop()call on OSError path is correctlint,unit_tests,status-checkfailing@givenused instead of@thenon line 61type: ignoreannotations (11 occurrences)type: ignorecategorically forbiddenCloses #7044in PR bodyType/label appliedAutomated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +9,4 @@And no environment variable ``CLEVERAGENTS_LSP_COMMAND`` set@tdd_issue @tdd_issue_10597Scenario: Popen raises FileNotFoundError — no process to clean upBLOCKING — TDD scenarios must use
@tdd_issue_7044, not@tdd_issue_10597.The
@tdd_issue_Ntag must reference the issue number being fixed. #10597 is a PR, not an issue. The original bug issue is #7044. Change both scenario tags from@tdd_issue_10597to@tdd_issue_7044.Additionally, per the TDD bug fix workflow, issue-capture tests that prove the bug exists must be initially tagged with
@tdd_expected_fail(submitted before the fix), and that tag removed once the fix is applied. These scenarios appear to have been written alongside the fix without the@tdd_expected_failphase. While the current state (fix applied, tag absent) is the correct end state, please confirm the TDD workflow was followed and that a companion TDD issue was created and closed for this bug.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +8,4 @@import tempfilefrom unittest import mockfrom behave import given, then, when # type: ignore[import-untyped]BLOCKING —
type: ignoreannotations are categorically forbidden.This file contains 11
# type: ignoreannotations (lines 11, 26, 57, 58, 64, 118, 120, 128, 135, 140). The project has zero tolerance for# type: ignore— not a single suppression is permitted anywhere, including test files. These must all be removed.For
from behave import given, then, when # type: ignore[import-untyped]— add a stub or use apy.typedmarker approach, or addbehaveto the Pyrightignorelist inpyproject.tomlat the project level (not inline suppressions).For
context._transport._processand similar — add proper type annotations to thecontextobject or use typedcontext.transport: StdioTransportassignment rather than dynamically setting attributes.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +59,4 @@@given("no subprocess was leaked (no orphan processes exist)")def step_no_leak_scenario1(context): # noqa: ANN201BLOCKING — Wrong step decorator.
This step uses
@givenbut the feature file places it after aThenstep asAnd no subprocess was leaked.... In Behave,Andinherits the keyword of the preceding step, so this step must be decorated with@then. Using@givenhere means Behave cannot find the step implementation and the scenario will fail with anUndefinedSteperror.Fix: Change
@given("no subprocess was leaked (no orphan processes exist)")to@then("no subprocess was leaked (no orphan processes exist)").Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
4888e76d30022ed69039Re-Review: REQUEST_CHANGES
Progress Since Last Review
One item from the previous review has been resolved:
All other 9 blocking issues remain unresolved. The PR cannot be merged in its current state.
Remaining Blocking Issues
1. CI Failing — Required gates still red
The following required CI gates are still failing:
lint— Failing after 1m25sunit_tests— Failing after 4m51sstatus-check— Failing (as a consequence of the above)benchmark-regression— Failing (non-required but concerning)Note that
typecheckis now passing — good progress. However all required gates must be green before a PR can be approved. Theunit_testsfailure is almost certainly caused by item #2 below (the@givendecorator issue).2. Wrong
@givendecorator —@thenrequired (NOT FIXED)The inline comment on line 62 of
features/steps/stdio_transport_subprocess_cleanup_steps.pyfrom the previous review has not been addressed. The step"no subprocess was leaked (no orphan processes exist)"is still decorated with@given. In the feature file, this step appears as:Behave resolves
Andby inheriting the preceding keyword (Then). It looks for a@then-decorated implementation and fails withUndefinedStep— this is the cause of theunit_testsCI failure.Fix: Change
@given("no subprocess was leaked (no orphan processes exist)")to@then("no subprocess was leaked (no orphan processes exist)").3.
type: ignoreannotations still present — 11 occurrences (NOT FIXED)The step file
features/steps/stdio_transport_subprocess_cleanup_steps.pystill contains 11# type: ignoreannotations (lines 11, 26, 57, 58, 64, 118, 120, 128, 135, 140, 146). Zero tolerance applies — not a single# type: ignoreis permitted anywhere in the codebase.For
from behave import given, then, when # type: ignore[import-untyped]: addbehaveto the Pyrightignorelist inpyproject.toml(e.g.,[tool.pyright] ignore = ["features/**"]or addbehavetoreportMissingModuleSource = false). This is a project-level suppresssion of a third-party stub issue — correct approach.For
context._transport,context._captured_error, etc.: Use a typed context fixture class or assign typed attributes explicitly at theGivenstep level (e.g.context.transport: StdioTransport = StdioTransport(...)). This avoidsattr-definederrors without inline suppressions.4. Commit footer references PR
#10597not issue#7044(NOT FIXED)The commit footer reads:
#10597is a PR, not an issue. The actual bug issue being fixed is #7044 ("Bug: LSP Transport Process Resource Leak on Exception in StdioTransport.start()"). The footer must readISSUES CLOSED: #7044so the commit is correctly linked to the issue it resolves.5. PR body does not contain
Closes #7044(NOT FIXED)The PR body still references
#10597in the compliance checklist and footer. It does not containCloses #7044orFixes #7044. Without this closing keyword, the issue will not auto-close on merge, the PR cannot be linked to the issue in Forgejo dependency tracking, and the merge requirements are not met.Replace
ISSUES CLOSED: #10597in the PR body withCloses #7044.6. Branch name does not match issue Metadata (NOT FIXED)
Issue #7044 Metadata specifies:
This PR uses
bugfix/pr-fix-10597-cleanup-subprocess-on-init-failure. Per contributing rules, the branch name must match the Branch field in the issue Metadata verbatim.Note: This is the hardest item to fix in isolation as it requires reopening from a new branch. However, it is a required policy compliance item.
7. TDD scenario tags reference
@tdd_issue_10597not@tdd_issue_7044(NOT FIXED)Both scenarios in the feature file are still tagged
@tdd_issue @tdd_issue_10597. The@tdd_issue_Ntag must reference the issue number.#10597is a PR. The tags must be@tdd_issue @tdd_issue_7044.8. Wrong milestone assigned (NOT FIXED)
This PR is assigned to v3.2.0, but issue #7044 is on milestone v3.6.0. Per merge requirements, the PR must be assigned to the same milestone as the linked issue. Update the PR milestone to
v3.6.0.9. No
Type/label applied (NOT FIXED)No labels are applied to this PR. Exactly one
Type/label is mandatory. For a bug fix: applyType/Bug.Dependency Direction Issue
The Forgejo dependency chain is also incorrect:
The required direction is: PR #11011 blocks issue #7044 (i.e., on issue #7044, this PR should appear under "depends on"). Without this link, the automated merge workflow cannot close the issue on merge. Please add the dependency: PR #11011 blocks #7044.
Full Review Checklist
stop()call in OSError handler is correct; clears_processviastop()is_alivewill returnFalseafter failedstart()(viastop()resetting_process)@givenused instead of@thenon "no subprocess was leaked" step@tdd_issue_10597must be@tdd_issue_7044# type: ignoreannotations remainstart()lintfailingunit_testsfailing (caused by@given/@thenmismatch)typechecknow passingCloses #7044in PR bodyType/BuglabelNon-Blocking Observations
Suggestion: Scenario 2 test logic — Popen mock not correctly triggering OSError
The
step_patch_popenstep (line 148) creates afake_popenfunction that returnsactual_processon the first call and raisesOSErroron the second call. However,StdioTransport.start()only callsPopenonce. The second Popen call never happens so theOSErroris never triggered via this mechanism.The
step_start_os_errorstep compensates with a fallback patcher, but the two@whensteps create competing patcher logic that is difficult to follow. Additionally, the step currently tests that_process is Noneinstep_subprocess_terminated— but this is the same assertion asstep_process_cleared— neither step verifies thatterminate()was actually called on the mock process.Consider: patch
Popento return a mock process successfully, then raiseOSErrorfromlogger.info(or a future post-spawn step) to properly exercise the cleanup guard. Alternatively, directly invoke the cleanup path by patchingsubprocess.Popento succeed but then raising from a subsequent call that the production code makes after_processis assigned.Action Items (in priority order)
@given→@thenon line 61 of step file — this unblocksunit_testsCI# type: ignoreannotations — properly type the Behave contextISSUES CLOSED: #7044#10597references withCloses #7044Type/Buglabel to PR@tdd_issue_10597to@tdd_issue_7044in feature fileOnce CI is green and all blockers above are resolved, please re-request review.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +9,4 @@And no environment variable ``CLEVERAGENTS_LSP_COMMAND`` set@tdd_issue @tdd_issue_10597Scenario: Popen raises FileNotFoundError — no process to clean upBLOCKING — TDD scenario tags still reference
@tdd_issue_10597(unchanged from previous review).#10597is a PR, not an issue. The@tdd_issue_Ntag must reference the actual bug issue number. The original bug issue is #7044 ("Bug: LSP Transport Process Resource Leak on Exception in StdioTransport.start()").Fix: Change both
@tdd_issue_10597tags to@tdd_issue_7044.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +8,4 @@import tempfilefrom unittest import mockfrom behave import given, then, when # type: ignore[import-untyped]BLOCKING —
# type: ignoreannotations are categorically forbidden (unchanged from previous review).This file still contains 11
# type: ignoreannotations. The project has zero tolerance — not a single suppression is permitted anywhere.For this import line: add
behaveto the PyrightreportMissingModuleSource = falselist or addfeatures/**to Pyright'signorelist inpyproject.toml— this is a project-level configuration change, not an inline suppression.For
context._transport,context._captured_error, etc.: assign explicitly typed attributes in theGivensteps instead of usingcontext._attrdynamic assignments, e.g.:This makes Pyright aware of the type without any suppression.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +59,4 @@@given("no subprocess was leaked (no orphan processes exist)")def step_no_leak_scenario1(context): # noqa: ANN201BLOCKING —
@givendecorator must be@then(unchanged from previous review).This step appears in the feature file as:
Behave resolves
Andby inheriting the keyword of the preceding step — in this caseThen. This means Behave looks for a@then-decorated step. Because@givenis used here, Behave raisesUndefinedStepat runtime, causing theunit_testsCI gate to fail.Fix: Change
@given("no subprocess was leaked (no orphan processes exist)")to@then("no subprocess was leaked (no orphan processes exist)").Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
d93a5486f41b8efa6f84Re-Review (Third Round): REQUEST_CHANGES
Progress Since Last Review
Two items from the previous review have changed status:
Type/labelType/Buglabel is now applied# type: ignoreannotations (11 occurrences)d93a5486was added but only removed# noqa: N812and# noqa: ANN201directives; all 11# type: ignoreannotations remain untouchedAll other 9 blocking issues remain unresolved. The PR cannot be merged in its current state.
New Issue Introduced This Round
New Blocker: PR now has 2 commits instead of 1
A second commit
d93a5486(style(steps): remove unused noqa directives (RUF100 fix)) was added on top of the fix commit. Per project rules: one issue = one commit. The previous review noted that 38 commits spanning multiple concerns was a blocker, and the fix was squashing to a single commit. That regression has now been reintroduced with this extra commit.Additionally, this second commit did not fix the actual blocking issue it was meant to address. It removed
# noqa: N812and# noqa: ANN201comments (which were not part of the blocking feedback) while leaving all 11# type: ignoreannotations completely intact.Fix: Squash the two commits into one atomic commit for the LSP fix. The
# noqacleanup — if needed — should have been part of the original fix commit, not a separate patch.Remaining Blocking Issues
1. CI Failing — Required gates are red (NOT FIXED, condition worsened)
The previous review noted
lint,unit_tests, andstatus-checkfailing. This round the following required gates are all failing:lint— Failing after 12m31stypecheck— Failing after 12m29s (regression: was passing last review)security— Failing after 12m29squality— Failing after 12m29sunit_tests— Failing after 12m29sstatus-check— Pending/blocked (gating job)build— Failing after 12m26spush-validation— Failing after 1m7sbenchmark-regression— Failing after 1m11sPer company policy, all required CI gates (
lint,typecheck,security,unit_tests,coverage) must pass before a PR can be approved. The CI state has regressed compared to the previous review wheretypecheckwas passing. Fix all CI failures before requesting re-review.2. Wrong
@givendecorator —@thenrequired (NOT FIXED)The step
"no subprocess was leaked (no orphan processes exist)"infeatures/steps/stdio_transport_subprocess_cleanup_steps.pyis still decorated with@given. In the feature file, this step appears as:Behave resolves
Andby inheriting the keyword of the preceding step —Then. Behave looks for a@then-decorated step and raisesUndefinedStep, causing theunit_testsCI failure.Fix: Change
@given("no subprocess was leaked (no orphan processes exist)")to@then("no subprocess was leaked (no orphan processes exist)").3.
# type: ignoreannotations still present — 11 occurrences (NOT FIXED)Despite the new
style(steps)commit, all 11# type: ignoreannotations remain in the step file: lines 11, 26, 57, 58, 64, 118, 120, 128, 135, 140, 146. Zero tolerance applies — not a single# type: ignoreis permitted.Fix for line 11 (
from behave import given, then, when # type: ignore[import-untyped]): Addfeatures/**to Pyright'signorelist inpyproject.toml. This is a project-level configuration change, not an inline suppression.Fix for
context._transport,context._captured_error, etc. (# type: ignore[attr-defined]): Declare typed attributes on thecontextobject at the start of each scenario's@givenstep. For example:With proper type annotations on context attributes, Pyright will resolve the attr-defined errors without inline suppressions.
4. Commit footer references PR
#10597not issue#7044(NOT FIXED)The fix commit
022ed690footer still reads:#10597is a PR, not an issue. The actual bug issue being fixed is #7044. The footer must readISSUES CLOSED: #7044.5. PR body does not contain
Closes #7044(NOT FIXED)The PR body still references
#10597in the compliance checklist and ISSUES CLOSED footer. It does not containCloses #7044orFixes #7044. Without this closing keyword:Fix: Update the PR body to include
Closes #7044and update all#10597references to#7044(since #10597 is a PR, not the issue being fixed).6. Branch name does not match issue Metadata (NOT FIXED)
Issue #7044 Metadata specifies:
This PR still uses
bugfix/pr-fix-10597-cleanup-subprocess-on-init-failure. Per contributing rules, the branch name must match the Branch field in the issue Metadata verbatim.7. TDD scenario tags reference
@tdd_issue_10597not@tdd_issue_7044(NOT FIXED)Both scenarios in
features/stdio_transport_subprocess_cleanup.featureare still tagged@tdd_issue @tdd_issue_10597. The@tdd_issue_Ntag must reference the issue number.#10597is a PR. The tags must be@tdd_issue @tdd_issue_7044.8. Wrong milestone assigned (NOT FIXED)
This PR is still assigned to v3.2.0, but issue #7044 is on milestone v3.6.0. Per merge requirements, the PR must be assigned to the same milestone as the linked issue. Update the PR milestone from v3.2.0 to v3.6.0.
9. No Forgejo dependency: PR #11011 → blocks → issue #7044 (NOT FIXED)
The Forgejo dependency chain is still incorrect. PR #11011 does not block issue #7044. The required direction is: PR #11011 blocks issue #7044 (i.e., on issue #7044, this PR should appear under "depends on"). Without this link:
Fix: On PR #11011 (or on issue #7044), add the dependency so that PR #11011 blocks #7044.
Full Review Checklist
stop()call in OSError handler correctly terminates and clears_processis_alivereturnsFalseafter failedstart()(viastop()resetting_process)@givenused instead of@thenon "no subprocess was leaked" step@tdd_issue_10597must be@tdd_issue_7044step_subprocess_terminatedonly asserts_process is None— same asstep_process_cleared; noterminate()call verification# type: ignoreannotations remain (lines 11, 26, 57, 58, 64, 118, 120, 128, 135, 140, 146)start()#10597, not issue#7044lintfailingtypecheckfailing (regression — was passing last review)securityfailingunit_testsfailing (caused by@given/@thenmismatch)Closes #7044in PR bodyType/Buglabel appliedNon-Blocking Observations
Suggestion: Scenario 2 mock logic is still fragile
As noted in the previous two reviews,
step_patch_popencreates afake_popenthat raisesOSErroron the secondPopencall, butStdioTransport.start()only callsPopenonce. TheOSErrornever fires via this mechanism. Thestep_start_os_errorstep has a fallback that patches Popen to always raiseOSErrorimmediately (before_processis ever set), which means the test isn't actually exercising the cleanup code path at all — it's testingFileNotFoundError-equivalent behavior where no process was created.To properly test the cleanup path, the
OSErrormust be raised afterself._processis assigned. The correct approach is to patchsubprocess.Popento succeed (returning a mock process), then raiseOSErrorfromlogger.infoor any code that runs after thePopencall withinstart().Suggestion:
step_subprocess_terminatedis a duplicate assertionstep_subprocess_terminatedassertscontext._transport._process is None— which is identical tostep_process_cleared. To add value, this step should verify thatterminate()was actually called on the mock process object, e.g.actual_process.terminate.assert_called_once().Action Items (in priority order)
@given→@thenon the "no subprocess was leaked" step — unblocksunit_testsCI# type: ignoreannotations — addfeatures/**to Pyright ignore list inpyproject.tomlISSUES CLOSED: #10597→ISSUES CLOSED: #7044Closes #7044, remove#10597references@tdd_issue_10597to@tdd_issue_7044in both scenariosbugfix/m3.6.0-lsp-transport-resource-leak(resubmit from correct branch)Once all blockers above are resolved and CI is fully green, please re-request review.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +9,4 @@And no environment variable ``CLEVERAGENTS_LSP_COMMAND`` set@tdd_issue @tdd_issue_10597Scenario: Popen raises FileNotFoundError — no process to clean upBLOCKING — TDD scenario tags still reference
@tdd_issue_10597(unchanged from previous two reviews).#10597is a PR, not an issue. The@tdd_issue_Ntag must reference the actual bug issue number. The original bug issue is #7044 ("Bug: LSP Transport Process Resource Leak on Exception in StdioTransport.start()").Fix: Change both
@tdd_issue_10597tags to@tdd_issue_7044.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +8,4 @@import tempfilefrom unittest import mockfrom behave import given, then, when # type: ignore[import-untyped]BLOCKING —
# type: ignoreannotations are categorically forbidden (unchanged from previous two reviews).This file still contains 11
# type: ignoreannotations (lines 11, 26, 57, 58, 64, 118, 120, 128, 135, 140, 146). The project has zero tolerance — not a single suppression is permitted anywhere in the codebase, including test files.Note: The new
style(steps)commit (d93a5486) only removed# noqa: N812and# noqa: ANN201directives — it did not remove any# type: ignoreannotations.Fix for this import line: Add
features/**to Pyright'signorelist inpyproject.toml:This is a project-level configuration change that removes the need for inline suppression on third-party stub issues.
Fix for
context._transport,context._captured_error, etc.: Declare typed attributes explicitly at assignment:Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
@ -0,0 +61,4 @@@given("no subprocess was leaked (no orphan processes exist)")def step_no_leak_scenario1(context):"""Verify that no StdioTransport process is lingering."""assert context._transport._process is None # type: ignore[attr-defined]BLOCKING —
@givendecorator must be@then(unchanged from previous two reviews).This step is still decorated with
@given. The feature file places it after aThenstep:Behave resolves
Andby inheriting the preceding keyword (Then). It looks for a@then-decorated step implementation. Because@givenis used here, Behave raisesUndefinedStepat runtime — this is the direct cause of theunit_testsCI failure.Fix: Change
@given("no subprocess was leaked (no orphan processes exist)")to@then("no subprocess was leaked (no orphan processes exist)").Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Re-Review #3: REQUEST_CHANGES
Progress Since Last Review
One item has been resolved since the second review:
Type/BuglabelType/Buglabel now appliedAll other blocking issues remain unresolved. Additionally, a new critical blocker has been identified that supersedes all others.
🚨 NEW CRITICAL BLOCKER: The LSP Fix Is No Longer Present in This Branch
The diff between
masterand this branch's HEAD (1b8efa6f) contains zero changes to any LSP transport files. The files that are supposed to be the core of this PR are entirely absent:src/cleveragents/lsp/transport.py— not changed (no cleanup guard added)features/stdio_transport_subprocess_cleanup.feature— does not exist on this branchfeatures/steps/stdio_transport_subprocess_cleanup_steps.py— does not exist on this branchInstead, the diff contains only unrelated changes:
features/a2a_session_close_validation.feature— a2a session close (unrelated)features/decision_recording.feature— decision recording (unrelated)features/strategize_decision_recording.feature— plan lifecycle (unrelated)src/cleveragents/a2a/facade.py— a2a facade (unrelated)src/cleveragents/application/services/strategize_decision_hook.py— strategize hook (unrelated)The
StdioTransport.start()method in the current branch is identical to the unfixed version described in issue #7044's "Current Behavior" section — no cleanup guard exists around the post-Popeninitialization.The HEAD commit (
1b8efa6f) is a merge commit titled "Merge master to pick up robot helper lint fixes (#9094)" — this is completely wrong as the head commit for a bugfix PR. The LSP fix commits appear to have been lost during a rebase or branch manipulation.This is the most critical issue. The PR does not implement what it claims to implement. It must not be merged in this state.
Fix: Restore the original LSP fix commit (
fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start()) to this branch. There are several candidate commits in git history:6753050f,15330a3f,4c5584e2. Create a clean branch from master with only the LSP fix commit cherry-picked. Ensure the branch contains ONLY the LSP fix commit plus the correct test files.Remaining Previously-Identified Blocking Issues
1. CI Failing — Required gates still red
The following required CI gates are failing on HEAD
1b8efa6f:lint— Failing after 52sunit_tests— Failing after 8m42sintegration_tests— Failing after 3m59sstatus-check— Failing (consequence of above)benchmark-regression— Failing after 1m15sAll required CI gates (lint, typecheck, security, unit_tests, coverage) must be green.
typecheckandsecurityare currently passing — good progress. Fixlintandunit_testsbefore re-requesting review.2. Wrong
@givendecorator —@thenrequired (NOT FIXED — test files currently MISSING)Once the LSP fix and test files are restored: change
@given("no subprocess was leaked (no orphan processes exist)")to@then("no subprocess was leaked (no orphan processes exist)"). Behave resolvesAndby inheriting the preceding keyword (Then), so@givenhere causes anUndefinedSteperror and is the cause of theunit_testsCI failure.3.
# type: ignoreannotations — 11 occurrences (NOT FIXED — files currently MISSING)Once files are restored: remove all 11
# type: ignoreannotations. Use typed attribute assignments and project-level Pyright configuration (e.g.ignore = ["features/**"]inpyproject.toml) rather than inline suppressions. Zero tolerance applies.4. Commit footer does not reference issue
#7044(WORSENED)The previous version had
ISSUES CLOSED: #10597(wrong PR number). The current HEAD references#9094(a different issue entirely). Once the branch is properly restored, the commit footer must readISSUES CLOSED: #7044.5. PR body does not contain
Closes #7044(NOT FIXED)The PR body still references
#10597. It must includeCloses #7044to link to the actual bug issue, enable automatic issue closure on merge, and satisfy the PR requirements checklist.6. Branch name does not match issue #7044 Metadata (NOT FIXED)
Issue #7044 Metadata specifies
Branch: bugfix/m3.6.0-lsp-transport-resource-leak. This PR usesbugfix/pr-fix-10597-cleanup-subprocess-on-init-failure. Per contributing rules, the branch must match the Branch field in the issue Metadata verbatim.7. TDD scenario tags still reference
@tdd_issue_10597(NOT FIXED — files currently MISSING)Once files are restored: change both
@tdd_issue_10597tags to@tdd_issue_7044. The@tdd_issue_Ntag must reference the issue number being fixed.#10597is a PR, not an issue.8. Wrong milestone: v3.2.0 vs required v3.6.0 (NOT FIXED)
Issue #7044 is on milestone v3.6.0. This PR remains assigned to v3.2.0. Update the PR milestone to
v3.6.0.9. No Forgejo dependency: PR #11011 → blocks → issue #7044 (NOT FIXED)
The Forgejo dependency link does not exist. On this PR, add issue #7044 under "blocks" so that on issue #7044, this PR appears under "depends on". Without this link, the automated merge workflow cannot close the issue on merge.
Full Review Checklist
stdio_transport_subprocess_cleanup.featureexists on branch@givenmust be@then;@tdd_issue_10597→@tdd_issue_7044# type: ignoreannotations must be removedlintfailingunit_testsfailingtypecheckpassingsecuritypassingCloses #7044in PR bodyType/Buglabel now appliedAction Items (in priority order)
StdioTransport.start()and thestdio_transport_subprocess_cleanup.feature+ step files. The branch must contain ONLY the LSP fix commit(s) relevant to #7044, rebased onto current master.@given→@thenon the step following aThenin the feature file (unblocksunit_testsCI)# type: ignoreannotations — properly type the Behave contextISSUES CLOSED: #7044(not #9094 or #10597)Closes #7044, remove incorrect issue/PR number referencesbugfix/m3.6.0-lsp-transport-resource-leakper issue #7044 MetadataAutomated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Re-Review (3rd): REQUEST_CHANGES
Overview
This review covers the latest pushed commits (head
d93a5486/ branch tip1b8efa6f). CI is still failing across all required gates. However, the most critical finding this round is a fundamental structural problem: the PR no longer contains the LSP transport fix at all. The actual changes in the current branch are for entirely different issues, and the original LSP fix code has been removed from this PR. This PR cannot be approved in its current state.Progress Since Last Review
Type/Buglabel appliedType/Buglabel is now present# type: ignorein new code# type: ignoreannotationsAll other blocking issues remain unresolved. Several new blocking issues have been introduced.
Critical New Blocking Issue: PR Content Has Been Replaced With Wrong Work
The LSP transport fix is completely absent
This PR is titled "fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start()" and is supposed to fix issue #7044. However, the current branch contains 4 commits that are entirely unrelated to the LSP transport fix:
fix(a2a): validate session_id at entry of _handle_session_close before devcontainer cleanup(issue #9094)fix(ci): remove duplicate CHANGELOG entries and repair CONTRIBUTORS.md for #9094style(robot): fix lint violations in helper script (E501 lines, I001 imports) for #9094Merge master to pick up robot helper lint fixes (#9094)The following files that were present in previous review rounds are now completely absent from the branch:
src/cleveragents/lsp/transport.py— NOT modified (the cleanup guard from issue #7044 has been removed)features/stdio_transport_subprocess_cleanup.feature— NOT present in the branchfeatures/steps/stdio_transport_subprocess_cleanup_steps.py— NOT present in the branchThe current
src/cleveragents/lsp/transport.pystart()method still lacks the post-Popen cleanup guard from issue #7044. The resource leak described in #7044 has not been fixed in this PR.What appears to have happened: The author has pushed commits for issue #9094 (A2A session close validation) and related work onto this branch, replacing the LSP transport fix commits. This PR is now a container for the wrong issues work.
How to fix: This PR should contain exactly ONE commit implementing the LSP transport cleanup guard for issue #7044, on branch
bugfix/m3.6.0-lsp-transport-resource-leak. The A2A session close fix and decision recording work should each be in their own separate PRs on their own branches.Remaining Blocking Issues from Previous Reviews
All 9 blocking issues from the previous review remain unresolved in the new code:
1. CI Failing — All required gates are red
The following CI gates are failing for the current head:
CI / lint— Failing after 12m31sCI / typecheck— Failing after 12m29sCI / security— Failing after 12m29sCI / quality— Failing after 12m29sCI / unit_tests— Failing after 12m29sCI / integration_tests— Failing after 12m29sCI / e2e_tests— Failing after 12m26sCI / build— Failing after 12m26sCI / status-check— Pending (blocked by required conditions)CI / push-validation— Failing after 1m7sPer company policy, all required CI gates must be green before a PR can be approved.
2. PR body still describes the wrong work
The PR body still says: Added subprocess cleanup in StdioTransport.start() when Popen() succeeds but then raises an OSError. This is no longer true — the current commits do not touch
StdioTransport.start(). The PR body must accurately describe the actual changes being submitted.3. No
Closes #7044in PR bodyThe PR body still contains
ISSUES CLOSED: #10597(a PR number, not an issue). There is noCloses #9094for the A2A session close fix that is actually in the branch, and noCloses #7044for the original LSP fix. Without closing keywords linking to the actual issues, the issues will not auto-close on merge and the PR does not meet merge requirements.4. Branch name does not match issue Metadata
This PR uses
bugfix/pr-fix-10597-cleanup-subprocess-on-init-failure. Issue #7044 requiresbugfix/m3.6.0-lsp-transport-resource-leak. The branch name must match the Branch field in the issue Metadata verbatim.5. Wrong milestone assigned
The PR is still assigned to v3.2.0. Issue #7044 is on milestone v3.6.0. The PR must be assigned to the same milestone as the linked issue.
6. Commit footers do not reference issue #7044
All 3 substantive commits have
ISSUES CLOSED: #9094. The original LSP fix commit (when re-added) must referenceISSUES CLOSED: #7044.7.
@tdd_issue_10597TDD tags (for when LSP tests are restored)The original BDD scenarios with
@tdd_issue_10597tags were present in previous rounds. When the LSP fix is re-added, the tags must be@tdd_issue_7044.8. Forgejo dependency direction missing
This PR does not have the required dependency: PR #11011 blocks issue #7044. Without this, automated issue closure and merge workflows cannot function correctly.
9. Wrong Epic scope — PR spans multiple unrelated Epics
The current commits address issues #9094 and #9056, which belong to different Epics than #7044 (Epic #824). Per policy, each PR is associated with exactly one Epic.
Content Assessment of New Code
While this code should not be in this PR, the work itself is assessed for completeness:
A2A session_id validation (
src/cleveragents/a2a/facade.py): The fix correctly moves thesession_idguard to the top of_handle_session_close(). The 6 BDD scenarios cover bothsvc is Noneandsvc is presentbranches. Robot Framework integration test is present. No# type: ignoreannotations. This work is correct but belongs in a separate PR.Strategize decision recording: Comprehensive BDD coverage. No
# type: ignore. This appears to be work for issues #8522/#9056 and must be in a separate PR.Required Action Summary
bugfix/m3.6.0-lsp-transport-resource-leakfor the LSP fixCloses #7044ISSUES CLOSED: #7044@givento@thenin the BDD step file (once restored)@tdd_issue_10597to@tdd_issue_7044in feature file (once restored)Full Review Checklist
start()cleanup guard) absent from PR# type: ignorein new codeCloses #7044in PR bodyAutomated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
BLOCKING — This file modification belongs in a different PR.
This PR is titled
fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start()and is supposed to address issue #7044 (LSP transport resource leak). The changes toA2aLocalFacade._handle_session_close()address issue #9094 — a completely separate concern.The A2A session_id validation fix is correct and should be submitted, but it must be in its own separate PR on its own correctly-named branch. Please move these changes to a new PR that:
Closes #9094in the PR bodyAutomated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Re-Review (Fifth Round): REQUEST_CHANGES
Progress Since Last Review
No new items have been resolved since the fourth review. The fundamental structural problem identified in the fourth review persists unchanged.
🚨 CRITICAL BLOCKER (PERSISTS): The LSP Fix Is Still Not Present in This Branch
Diff of
master...HEAD(SHA1b8efa6f) confirms zero changes to LSP transport files. The following files remain completely absent from this branch:src/cleveragents/lsp/transport.py— NOT modified (no cleanup guard added; the resource leak from issue #7044 still exists in the file)features/stdio_transport_subprocess_cleanup.feature— does not exist on this branchfeatures/steps/stdio_transport_subprocess_cleanup_steps.py— does not exist on this branchThe branch HEAD (
1b8efa6f) is a merge commit titled"Merge master to pick up robot helper lint fixes (#9094)"and contains 4 commits all related to issue #9094 (A2A session close validation) and decision recording work — none of which belong in this PR.The
StdioTransport.start()method in the current checkout is identical to the unfixed version described in issue #7044. The resource leak has not been fixed.This PR must not be merged in its current state. It does not implement what its title claims.
Required fix: Reset this branch to contain exactly ONE commit — the LSP transport cleanup guard for issue #7044. Cherry-pick the correct fix commit (candidates:
6753050f,15330a3f,4c5584e2from git history) onto the current master. The branch must contain ONLY the LSP fix and its test files. All A2A session close and decision recording work must be in separate PRs on separate branches.Current Branch Contents (for clarity)
The 4 commits on this branch relative to master are:
66b5e5c6—fix(a2a): validate session_id at entry of _handle_session_close before devcontainer cleanup(issue #9094 — wrong issue)17ddc12b—fix(ci): remove duplicate CHANGELOG entries and repair CONTRIBUTORS.md for #9094(issue #9094 — wrong issue)93ee01f8—style(robot): fix lint violations in helper script (E501 lines, I001 imports) for #9094(issue #9094 — wrong issue)1b8efa6f—Merge master to pick up robot helper lint fixes (#9094)(merge commit — wrong commit type)All of these must be removed from this branch.
Remaining Blocking Issues (All Unchanged)
1. CI Failing — Required gates are red
For HEAD
1b8efa6f:CI / lint— ❌ Failing after 52sCI / unit_tests— ❌ Failing after 8m42sCI / integration_tests— ❌ Failing after 3m59sCI / status-check— ❌ FailingCI / benchmark-regression— ❌ Failing after 1m15sPassing:
typecheck✅,security✅,quality✅,build✅,e2e_tests✅,push-validation✅All required CI gates (lint, typecheck, security, unit_tests, coverage) must be green before a PR can be approved. Fix all failing CI gates.
2. Wrong
@givendecorator —@thenrequired (NOT FIXED — files MISSING)Once the LSP fix files are restored: in
features/steps/stdio_transport_subprocess_cleanup_steps.py, change@given("no subprocess was leaked (no orphan processes exist)")to@then("no subprocess was leaked (no orphan processes exist)"). This mismatch is a root cause ofunit_testsCI failures. Behave inherits theThenkeyword from the preceding step and looks for a@then-decorated step — the@givendecorator causesUndefinedStep.3.
# type: ignoreannotations — zero tolerance (NOT FIXED — files MISSING)Once the step file is restored: remove all
# type: ignoreannotations. Project-level Pyright configuration (e.g.ignore = ["features/**"]inpyproject.toml) must be used for third-party import stubs. For context attribute access errors, declare typed attributes explicitly at each@givenstep level.4. Commit footer does not reference issue
#7044(NOT FIXED)The fix commit (once restored) must have footer
ISSUES CLOSED: #7044. The current commits reference#9094, which is the wrong issue.5. PR body does not contain
Closes #7044(NOT FIXED)The PR body still references
#10597(a PR number, not an issue). It must includeCloses #7044to link to the actual bug issue and enable automatic issue closure on merge.6. Branch name does not match issue #7044 Metadata (NOT FIXED)
Issue #7044 Metadata specifies:
Branch: bugfix/m3.6.0-lsp-transport-resource-leak. This PR usesbugfix/pr-fix-10597-cleanup-subprocess-on-init-failure. The branch name must match the Branch field in the issue Metadata verbatim.7. TDD scenario tags reference wrong issue number (NOT FIXED — files MISSING)
Once the feature file is restored: change
@tdd_issue_10597→@tdd_issue_7044in all scenario tags.#10597is a PR, not an issue. The@tdd_issue_Ntag must reference the issue number being fixed.8. Wrong milestone assigned (NOT FIXED)
The PR is assigned to v3.2.0. Issue #7044 is on milestone v3.6.0. The PR must be assigned to the same milestone as the linked issue.
9. No Forgejo dependency: PR #11011 → blocks → issue #7044 (NOT FIXED)
The required Forgejo dependency link does not exist. On PR #11011 or on issue #7044, add the link so PR #11011 blocks issue #7044. Without this, automated merge workflows cannot close the issue on merge.
10. PR spans multiple unrelated Epics (NOT FIXED)
The current 4 commits address issues belonging to different Epics than the LSP fix (#7044 belongs to Epic #824). Per policy, each PR must be associated with exactly one Epic. Once the branch is properly reset to contain only the LSP fix, this blocker resolves automatically.
Full Review Checklist
start()cleanup guard absent —src/cleveragents/lsp/transport.pyunmodifiedstdio_transport_subprocess_cleanup.featureon branch@given→@thenon step 61;@tdd_issue_10597→@tdd_issue_7044# type: ignoremust be removedlintfailingunit_testsfailingintegration_testsfailingtypecheckpassingsecuritypassingqualitypassingbuildpassingCloses #7044in PR bodyType/Buglabel appliedAction Items (in priority order)
6753050f,15330a3f, or4c5584e2) onto current master. Remove all A2A and decision recording commits from this branch.@given→@thenon the "no subprocess was leaked" step (unblocksunit_testsCI)# type: ignoreannotations (addfeatures/**to Pyright ignore list inpyproject.toml)ISSUES CLOSED: #7044Closes #7044, remove#10597references@tdd_issue_10597→@tdd_issue_7044in both scenarioslint,unit_tests,integration_tests)bugfix/m3.6.0-lsp-transport-resource-leakper issue #7044 MetadataAutomated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Re-Review (Fifth Round): REQUEST_CHANGES
Progress Since Last Review
No items from the previous review have been resolved. The branch has not changed since the last review — the HEAD commit remains
1b8efa6f("Merge master to pick up robot helper lint fixes (#9094)"), which is identical to what was reviewed in the fourth round.🚨 CRITICAL BLOCKER: The LSP Fix Is Still Absent From This Branch
This PR is titled "fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start()" and claims to fix issue #7044. However, the current branch contains zero changes to any LSP transport file:
src/cleveragents/lsp/transport.py— NOT modified (the cleanup guard from issue #7044 has not been added)features/stdio_transport_subprocess_cleanup.feature— does not exist on this branchfeatures/steps/stdio_transport_subprocess_cleanup_steps.py— does not exist on this branchInstead, the diff between
masterandHEADcontains four commits for entirely unrelated work:fix(a2a): validate session_id at entry of _handle_session_close before devcontainer cleanup(issue #9094)fix(ci): remove duplicate CHANGELOG entries and repair CONTRIBUTORS.md for #9094style(robot): fix lint violations in helper script (E501 lines, I001 imports) for #9094Merge master to pick up robot helper lint fixes (#9094)The
StdioTransport.start()method in this branch is byte-for-byte identical to the unfixed version inmaster. The resource leak described in issue #7044 has not been fixed.Fix: This PR must be rebuilt from scratch:
masternamedbugfix/m3.6.0-lsp-transport-resource-leak(matching the Metadata in issue #7044)src/cleveragents/lsp/transport.pyfeatures/stdio_transport_subprocess_cleanup.feature+ steps)ISSUES CLOSED: #7044Closes #7044and remove all#10597referencesThe A2A session close validation work (commits 1–4 above) belongs in a separate PR with its own branch and linked to issue #9094/9250. The decision recording work belongs in yet another separate PR for issues #8522/#9056.
All Previously-Identified Blocking Issues Remain Unresolved
1. CI Failing — Required gates still red
Failing required gates on HEAD
1b8efa6f:lint— Failing after 52sunit_tests— Failing after 8m42sintegration_tests— Failing after 3m59sstatus-check— Failingbenchmark-regression— FailingPassing gates:
typecheck✅,security✅,quality✅,build✅,e2e_tests✅All required CI gates (lint, typecheck, security, unit_tests, coverage) must be green before a PR can be approved.
2. PR body missing
Closes #7044(NOT FIXED)The PR body still references
#10597(a PR number, not an issue). There is noCloses #7044orFixes #7044. Without a closing keyword linking to the actual bug issue, the issue will not auto-close on merge and the PR does not meet merge requirements.Fix: Update PR body to include
Closes #7044and remove the incorrectISSUES CLOSED: #10597reference.3. Branch name does not match issue #7044 Metadata (NOT FIXED)
Issue #7044 Metadata specifies:
This PR uses
bugfix/pr-fix-10597-cleanup-subprocess-on-init-failure. Per contributing rules, the branch must match the Branch field in the issue Metadata verbatim.4. Wrong milestone: v3.2.0 vs required v3.6.0 (NOT FIXED)
Issue #7044 is on milestone v3.6.0. This PR is still assigned to v3.2.0.
5. Commit footers do not reference issue #7044 (NOT FIXED, worsened)
All four commits in this branch have footers referencing
#9094— an unrelated A2A issue. The LSP fix commit (when re-added) must haveISSUES CLOSED: #7044.6. No Forgejo dependency: PR #11011 → blocks → issue #7044 (NOT FIXED)
The required dependency link is still absent. PR #11011 does not block issue #7044. Without this link, automated issue closure and merge workflows cannot function correctly.
7. Wrong milestone assigned (NOT FIXED)
This PR remains assigned to v3.2.0. Issue #7044 requires v3.6.0.
8. TDD scenario tags and test files (NOT FIXED — files absent)
Once the LSP fix and test files are restored, the Behave scenarios must be tagged
@tdd_issue @tdd_issue_7044(not@tdd_issue_10597). The@given/@thendecorator mismatch from prior reviews must also be fixed.9. PR spans multiple unrelated Epics (NOT FIXED)
The current four commits address work in Epic #9094 (A2A session close) and Epic #8522 (decision recording) — neither of which is Epic #824 (LSP transport resource leak, the parent of issue #7044). Per policy, each PR is associated with exactly one Epic.
Assessment of New Code in Branch
While the code below does not belong in this PR, it is assessed for correctness:
A2A session_id validation fix (
src/cleveragents/a2a/facade.py, 5-line change):The guard correctly moves
session_idvalidation to the method entry, preventing empty IDs from reaching devcontainer cleanup. The 6 BDD scenarios ina2a_session_close_validation.featurecover bothsvc is Noneandsvc is presentbranches, both empty and missingsession_id, and the happy path. No# type: ignoreannotations are present. The Robot Framework integration test is included. This work is technically correct but belongs in a separate PR.Strategize decision recording fix (
application/services/):The
strategize_decision_hook.py(378 lines) anddecision_context.py(66 lines) additions appear coherent. The 3 new scenarios indecision_recording.featureand the 157-linestrategize_decision_recording.featureprovide good BDD coverage. No# type: ignoreannotations. This work belongs in a separate PR.Full Review Checklist
start()) absent from this branch —transport.pyunchanged from masterstdio_transport_subprocess_cleanup.featurepresent# type: ignorein any new fileslintfailingunit_testsfailingintegration_testsfailingtypecheck,security,quality,build,e2e_testspassingCloses #7044in PR bodyType/Buglabel appliedRequired Action Summary (Priority Order)
bugfix/m3.6.0-lsp-transport-resource-leakfrom master containing ONLY the LSP cleanup guard commit for issue #7044. Cherry-pick from git history (candidates:6753050f,15330a3f,4c5584e2) or re-implement the fix.Closes #7044, remove#10597references.ISSUES CLOSED: #7044.v3.6.0.@given→@then;@tdd_issue_10597→@tdd_issue_7044.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
1b8efa6f84c0e64f406aImplementation Attempt — Success
Applied the subprocess cleanup fix to StdioTransport.start() in src/cleveragents/lsp/transport.py:
The PR now contains exactly 1 commit (1 file changed: transport.py) and is mergeable.
Quality gate status for the modified file: lint ✅ (0 errors), typecheck ✅ (0 errors).
Note: Full repo CI has pre-existing failures in unrelated files not caused by this change.
Automated by CleverAgents Bot
Supervisor: Implementation | Agent: task-implementor
Review Decision: APPROVED
Summary
This PR correctly fixes a real resource leak in
StdioTransport.start(). Whensubprocess.Popen()fails after partially creating a subprocess via fork (e.g., execve fails on the new process), the broken_processreference would prevent cleanup of orphan/zombie processes.Correctness Assessment
self._process = Nonebefore re-raising -- correct, prevents leaked reference to broken Popen object.self.stop()to actively terminate partially-created subprocesses then resets_processto None -- more thorough than FileNotFoundError branch.Compliance Checklist
# type: ignorecomments introducedSuggestions for Improvement
lsp_transport_coverage.featurelines 14-25, scenarios for FileNotFoundError and OSError in start()) assert the error type and message but do NOT explicitly verify_process is Noneafter exceptions. Adding cleanup assertions would make tests verify the fix.src/cleveragents/a2a/stdio_transport.py:142-170has the identical vulnerability pattern --self._process = subprocess.Popen()followed by exception handlers that do not clean up. Recommend fixing as a follow-up PR.Verdict: APPROVED -- Ready to merge.
c0e64f406ade9d2444a5de9d2444a5c768e8c4f5Pushed refined patches with additional subprocess cleanup protections:
self.stop()to clean up rather than leaking the process.start_server(): Wrappedtransport.start() + client.initialize()intry/except BaseExceptionensuring transport.stop() protects against leaks at any failure point (not just after initialization).restart_server(): Same try/except BaseException wrapper applied.Branch:
pr-fix-11011Commit:
e9118704Implementation Attempt — Success
PR #11011 is already merged (merged_at=2026-05-14T07:00:53Z) by a previous implementation dispatch.
The LSP transport subprocess cleanup fix was previously applied and verified:
Commit
de9d2444(fix(lsp): cleanup subprocess on failed initialization in StdioTransport.start()) is confirmed present in master.All required CI gates passed before merge (lint ✅, typecheck ✅, security ✅, unit_tests ✅, integration_tests ✅, coverage ✅, status-check ✅).
No further code changes needed for this PR.
Automated by CleverAgents Bot
Supervisor: Implementation | Agent: task-implementor