UAT: Actor compiler ignores node.lsp_binding field — per-node LSP bindings lost during compilation #1432

Closed
opened 2026-04-02 17:51:08 +00:00 by freemo · 16 comments
Owner

Bug Report

Feature Area: Actor Graphs (v3.3.0)
Tested by: UAT instance uat-worker-actor-graphs-1

What Was Tested

Per-node LSP binding extraction during actor graph compilation.

Expected Behavior (from spec)

The spec states: "Different nodes in an actor's graph can have different LSP configurations." The NodeDefinition schema has a dedicated lsp_binding: NodeLspBinding | None field for per-node LSP configuration. After compilation, CompilationMetadata.lsp_bindings should contain LspBinding records for nodes that have lsp_binding set.

Actual Behavior

The _extract_lsp_bindings function in src/cleveragents/actor/compiler.py reads LSP bindings from node.config.get("lsp_bindings", []) (the config dict) instead of node.lsp_binding (the dedicated field). Since lsp_binding is stored as a top-level field on NodeDefinition, the compiler always returns an empty list for nodes configured via the lsp_binding: YAML key.

Steps to Reproduce

from cleveragents.actor.schema import ActorConfigSchema
from cleveragents.actor.compiler import compile_actor
import yaml

config = ActorConfigSchema.model_validate(yaml.safe_load("""
name: test/per-node-lsp
type: graph
model: gpt-4
description: Actor with per-node LSP
route:
  nodes:
    - id: start
      type: agent
      name: Start
      description: Start with LSP
      config:
        model: gpt-4
      lsp_binding:
        server: local/pyright
        capabilities: [diagnostics, hover]
    - id: end
      type: agent
      name: End
      description: End node
      config:
        model: gpt-4
  edges:
    - from_node: start
      to_node: end
  entry_node: start
  exit_nodes: [end]
"""))
compiled = compile_actor(config)
print(compiled.metadata.lsp_bindings)  # [] — should contain binding for 'start' node

Code Location

src/cleveragents/actor/compiler.py, function _extract_lsp_bindings():

raw_bindings = node.config.get("lsp_bindings", [])

The function should also check node.lsp_binding (the NodeLspBinding field) and convert it to a LspBinding record.

Severity

Medium — per-node LSP bindings are silently ignored, meaning fine-grained LSP control described in the spec does not work.


Metadata

  • Branch: bugfix/actor-compiler-lsp-binding-field
  • Commit Message: fix(actor): read lsp_binding from NodeDefinition field instead of config dict in compiler
  • Milestone: v3.3.0
  • Parent Epic: (needs manual linking — Actor Graphs epic)

Subtasks

  • Fix _extract_lsp_bindings() in src/cleveragents/actor/compiler.py to read node.lsp_binding (the NodeLspBinding field) in addition to or instead of node.config.get("lsp_bindings", [])
  • Convert NodeLspBinding to LspBinding record correctly
  • Tests (Behave): Add scenario in features/ covering per-node LSP binding extraction with lsp_binding field
  • Tests (Robot): Add integration test verifying CompilationMetadata.lsp_bindings is populated for nodes with lsp_binding set
  • Verify coverage >= 97% via nox -s coverage_report
  • Run nox (all default sessions), fix any errors

Definition of Done

  • _extract_lsp_bindings() reads node.lsp_binding (the NodeLspBinding field)
  • CompilationMetadata.lsp_bindings is correctly populated for nodes with lsp_binding set
  • Per-node LSP bindings configured via the lsp_binding: YAML key are not silently dropped
  • All nox stages pass
  • Coverage >= 97%
## Bug Report **Feature Area:** Actor Graphs (v3.3.0) **Tested by:** UAT instance uat-worker-actor-graphs-1 ### What Was Tested Per-node LSP binding extraction during actor graph compilation. ### Expected Behavior (from spec) The spec states: "Different nodes in an actor's graph can have different LSP configurations." The `NodeDefinition` schema has a dedicated `lsp_binding: NodeLspBinding | None` field for per-node LSP configuration. After compilation, `CompilationMetadata.lsp_bindings` should contain `LspBinding` records for nodes that have `lsp_binding` set. ### Actual Behavior The `_extract_lsp_bindings` function in `src/cleveragents/actor/compiler.py` reads LSP bindings from `node.config.get("lsp_bindings", [])` (the config dict) instead of `node.lsp_binding` (the dedicated field). Since `lsp_binding` is stored as a top-level field on `NodeDefinition`, the compiler always returns an empty list for nodes configured via the `lsp_binding:` YAML key. ### Steps to Reproduce ```python from cleveragents.actor.schema import ActorConfigSchema from cleveragents.actor.compiler import compile_actor import yaml config = ActorConfigSchema.model_validate(yaml.safe_load(""" name: test/per-node-lsp type: graph model: gpt-4 description: Actor with per-node LSP route: nodes: - id: start type: agent name: Start description: Start with LSP config: model: gpt-4 lsp_binding: server: local/pyright capabilities: [diagnostics, hover] - id: end type: agent name: End description: End node config: model: gpt-4 edges: - from_node: start to_node: end entry_node: start exit_nodes: [end] """)) compiled = compile_actor(config) print(compiled.metadata.lsp_bindings) # [] — should contain binding for 'start' node ``` ### Code Location `src/cleveragents/actor/compiler.py`, function `_extract_lsp_bindings()`: ```python raw_bindings = node.config.get("lsp_bindings", []) ``` The function should also check `node.lsp_binding` (the `NodeLspBinding` field) and convert it to a `LspBinding` record. ### Severity Medium — per-node LSP bindings are silently ignored, meaning fine-grained LSP control described in the spec does not work. --- ## Metadata - **Branch**: `bugfix/actor-compiler-lsp-binding-field` - **Commit Message**: `fix(actor): read lsp_binding from NodeDefinition field instead of config dict in compiler` - **Milestone**: v3.3.0 - **Parent Epic**: *(needs manual linking — Actor Graphs epic)* ## Subtasks - [ ] Fix `_extract_lsp_bindings()` in `src/cleveragents/actor/compiler.py` to read `node.lsp_binding` (the `NodeLspBinding` field) in addition to or instead of `node.config.get("lsp_bindings", [])` - [ ] Convert `NodeLspBinding` to `LspBinding` record correctly - [ ] Tests (Behave): Add scenario in `features/` covering per-node LSP binding extraction with `lsp_binding` field - [ ] Tests (Robot): Add integration test verifying `CompilationMetadata.lsp_bindings` is populated for nodes with `lsp_binding` set - [ ] Verify coverage >= 97% via `nox -s coverage_report` - [ ] Run `nox` (all default sessions), fix any errors ## Definition of Done - [ ] `_extract_lsp_bindings()` reads `node.lsp_binding` (the `NodeLspBinding` field) - [ ] `CompilationMetadata.lsp_bindings` is correctly populated for nodes with `lsp_binding` set - [ ] Per-node LSP bindings configured via the `lsp_binding:` YAML key are not silently dropped - [ ] All nox stages pass - [ ] Coverage >= 97%
freemo self-assigned this 2026-04-02 18:45:11 +00:00
Author
Owner

PR #1488 Review: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. Key issues found:

  1. The core bug is not fixed_extract_lsp_bindings() still reads from the config dict (node.config.get(...)) instead of the dedicated node.lsp_binding field. The function needs to be updated to check node.lsp_binding and convert NodeLspBinding to LspBinding.

  2. Pervasive typolsp_bindingss (double 's') introduced throughout compiler.py and propagated to tests.

  3. Unnecessary renameNodeDefinition.lsp_binding was renamed to lsp_bindings, which conflicts with the spec docs (actor_hierarchy.md uses singular lsp_binding).

  4. Commit message format — scope should be actor, not v3.7.0.

  5. Missing PR metadata — no milestone, no Type/ label.

See the full review on PR #1488 for details and suggested fix approach.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. Key issues found: 1. **The core bug is not fixed** — `_extract_lsp_bindings()` still reads from the config dict (`node.config.get(...)`) instead of the dedicated `node.lsp_binding` field. The function needs to be updated to check `node.lsp_binding` and convert `NodeLspBinding` to `LspBinding`. 2. **Pervasive typo** — `lsp_bindingss` (double 's') introduced throughout `compiler.py` and propagated to tests. 3. **Unnecessary rename** — `NodeDefinition.lsp_binding` was renamed to `lsp_bindings`, which conflicts with the spec docs (`actor_hierarchy.md` uses singular `lsp_binding`). 4. **Commit message format** — scope should be `actor`, not `v3.7.0`. 5. **Missing PR metadata** — no milestone, no `Type/` label. See the full review on PR #1488 for details and suggested fix approach. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 Review Outcome: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. The PR does not fix the reported bug — the _extract_lsp_bindings() function still reads from the config dict (node.config.get(...)) instead of the dedicated node.lsp_binding field. Additionally, the PR introduces a pervasive typo (lsp_bindingss with double-s), breaks feature file / step definition alignment, and includes unrelated destructive changes to CHANGELOG.md, cli/commands/tool.py, and docs/timeline.md.

See the detailed review on PR #1488 for the full list of required changes.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review Outcome: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. The PR does not fix the reported bug — the `_extract_lsp_bindings()` function still reads from the config dict (`node.config.get(...)`) instead of the dedicated `node.lsp_binding` field. Additionally, the PR introduces a pervasive typo (`lsp_bindingss` with double-s), breaks feature file / step definition alignment, and includes unrelated destructive changes to CHANGELOG.md, cli/commands/tool.py, and docs/timeline.md. See the detailed review on [PR #1488](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488) for the full list of required changes. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 has been reviewed for the 3rd time. Changes still requested. None of the critical issues from the previous two reviews have been addressed:

  1. The core bug is still not fixed — _extract_lsp_bindings() still only reads from node.config dict, not from node.lsp_binding field
  2. Pervasive "lsp_bindingss" (double 's') typo throughout compiler.py
  3. Feature file / step definition mismatches that will cause Behave test failures
  4. Incorrect rename of NodeDefinition.lsp_bindinglsp_bindings (singular field shouldn't be plural)
  5. Commit message uses wrong scope (v3.7.0 instead of actor)
  6. No test verifying the actual bug fix

Detailed review comments posted on the PR.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

PR #1488 has been reviewed for the 3rd time. **Changes still requested.** None of the critical issues from the previous two reviews have been addressed: 1. The core bug is still not fixed — `_extract_lsp_bindings()` still only reads from `node.config` dict, not from `node.lsp_binding` field 2. Pervasive "lsp_bindingss" (double 's') typo throughout `compiler.py` 3. Feature file / step definition mismatches that will cause Behave test failures 4. Incorrect rename of `NodeDefinition.lsp_binding` → `lsp_bindings` (singular field shouldn't be plural) 5. Commit message uses wrong scope (`v3.7.0` instead of `actor`) 6. No test verifying the actual bug fix Detailed review comments posted on the PR. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 — 4th Review: Changes Requested

PR #1488 has been reviewed for the 4th time. None of the critical issues from the previous 3 reviews have been addressed. The PR:

  1. Does not fix the core bug_extract_lsp_bindings() still reads from the config dict, never from node.lsp_binding
  2. Contains a pervasive "lsp_bindingss" typo (double s) throughout compiler.py and tests
  3. Incorrectly renames NodeDefinition.lsp_bindinglsp_bindings (singular → plural, mismatching the type and spec)
  4. Has broken step definitions that don't match feature file text
  5. Contains 122 files of unrelated destructive changes (deleted features, tests, modules, docs)
  6. Has an incorrect commit message format
  7. Lacks a test for the actual bug fix

Detailed review comments are on the PR. The branch needs to be completely reworked — ideally starting fresh from master with only the targeted fix.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 — 4th Review: Changes Requested PR #1488 has been reviewed for the 4th time. **None of the critical issues from the previous 3 reviews have been addressed.** The PR: 1. **Does not fix the core bug** — `_extract_lsp_bindings()` still reads from the config dict, never from `node.lsp_binding` 2. **Contains a pervasive "lsp_bindingss" typo** (double s) throughout compiler.py and tests 3. **Incorrectly renames** `NodeDefinition.lsp_binding` → `lsp_bindings` (singular → plural, mismatching the type and spec) 4. **Has broken step definitions** that don't match feature file text 5. **Contains 122 files of unrelated destructive changes** (deleted features, tests, modules, docs) 6. **Has an incorrect commit message** format 7. **Lacks a test** for the actual bug fix Detailed review comments are on the PR. The branch needs to be completely reworked — ideally starting fresh from master with only the targeted fix. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 — 5th Review: Changes Still Requested

PR #1488 has been reviewed for the 5th time. None of the critical issues from the previous 4 reviews have been addressed. The PR remains in the same state as when first submitted.

Outstanding Critical Issues:

  1. Core bug not fixed_extract_lsp_bindings() still reads from config dict only, never from node.lsp_binding
  2. "lsp_bindingss" typo (double 's') throughout compiler.py and test files
  3. Feature file / step definition mismatch — step defs renamed but feature files not updated (tests will fail)
  4. Incorrect rename of NodeDefinition.lsp_bindinglsp_bindings (type is singular, not a list)
  5. 135 files of unrelated destructive changes — should be ~5-6 files for this bug fix
  6. Commit message format wrong (scope should be actor, not v3.7.0)
  7. No test for the actual bug fix

The PR needs a complete rework to address these issues.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 — 5th Review: Changes Still Requested PR #1488 has been reviewed for the 5th time. **None of the critical issues from the previous 4 reviews have been addressed.** The PR remains in the same state as when first submitted. ### Outstanding Critical Issues: 1. **Core bug not fixed** — `_extract_lsp_bindings()` still reads from config dict only, never from `node.lsp_binding` 2. **"lsp_bindingss" typo** (double 's') throughout `compiler.py` and test files 3. **Feature file / step definition mismatch** — step defs renamed but feature files not updated (tests will fail) 4. **Incorrect rename** of `NodeDefinition.lsp_binding` → `lsp_bindings` (type is singular, not a list) 5. **135 files of unrelated destructive changes** — should be ~5-6 files for this bug fix 6. **Commit message format** wrong (scope should be `actor`, not `v3.7.0`) 7. **No test** for the actual bug fix The PR needs a complete rework to address these issues. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 has been reviewed for the 6th time. Changes still requested.

Improvement: The unrelated destructive changes (122-135 files) have been removed. The PR now touches only 8 files.

Still outstanding:

  1. The core bug is NOT fixed — _extract_lsp_bindings() still only reads from the config dict, never from node.lsp_binding
  2. Pervasive "lsp_bindingss" (double 's') typo throughout compiler.py and test files
  3. Feature file / step definition mismatch — Behave tests will fail
  4. Incorrect rename of NodeDefinition.lsp_bindinglsp_bindings (type is singular NodeLspBinding | None)
  5. Commit message format violation (scope should be actor, not v3.7.0)
  6. No test for the actual bug fix

See PR #1488 comment for full details.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

PR #1488 has been reviewed for the 6th time. **Changes still requested.** **Improvement:** The unrelated destructive changes (122-135 files) have been removed. The PR now touches only 8 files. **Still outstanding:** 1. The core bug is NOT fixed — `_extract_lsp_bindings()` still only reads from the config dict, never from `node.lsp_binding` 2. Pervasive "lsp_bindingss" (double 's') typo throughout `compiler.py` and test files 3. Feature file / step definition mismatch — Behave tests will fail 4. Incorrect rename of `NodeDefinition.lsp_binding` → `lsp_bindings` (type is singular `NodeLspBinding | None`) 5. Commit message format violation (scope should be `actor`, not `v3.7.0`) 6. No test for the actual bug fix See [PR #1488 comment](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488#issuecomment-94720) for full details. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 has been reviewed for the 7th time. Changes still requested — none of the critical issues from the previous 6 reviews have been addressed:

  1. Core bug not fixed: _extract_lsp_bindings() still reads from config dict only, never from node.lsp_binding field
  2. "lsp_bindingss" typo (double 's') throughout compiler.py and test files
  3. Feature file / step definition mismatch: Step definitions renamed but feature files not updated — Behave tests will fail
  4. Incorrect rename: NodeDefinition.lsp_bindinglsp_bindings (singular → plural) doesn't match type, spec, or docs
  5. No test for the actual bug fix
  6. Commit message format doesn't follow CONTRIBUTING.md

See PR #1488 comment for full details.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

PR #1488 has been reviewed for the 7th time. **Changes still requested** — none of the critical issues from the previous 6 reviews have been addressed: 1. **Core bug not fixed**: `_extract_lsp_bindings()` still reads from config dict only, never from `node.lsp_binding` field 2. **"lsp_bindingss" typo** (double 's') throughout compiler.py and test files 3. **Feature file / step definition mismatch**: Step definitions renamed but feature files not updated — Behave tests will fail 4. **Incorrect rename**: `NodeDefinition.lsp_binding` → `lsp_bindings` (singular → plural) doesn't match type, spec, or docs 5. **No test for the actual bug fix** 6. **Commit message format** doesn't follow CONTRIBUTING.md See [PR #1488 comment](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488#issuecomment-94902) for full details. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 Review: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. The PR does not fix the core bug described in this issue — the compiler still reads LSP bindings from the config dict (node.config.get(...)) instead of from the dedicated node.lsp_binding field on NodeDefinition. Additionally, the PR introduces a pervasive lsp_bindingss typo (double 's') across the compiler, metadata model, tests, and benchmarks.

See the detailed review comment on PR #1488 for the full list of required changes.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. The PR does not fix the core bug described in this issue — the compiler still reads LSP bindings from the config dict (`node.config.get(...)`) instead of from the dedicated `node.lsp_binding` field on `NodeDefinition`. Additionally, the PR introduces a pervasive `lsp_bindingss` typo (double 's') across the compiler, metadata model, tests, and benchmarks. See the [detailed review comment on PR #1488](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488#issuecomment-99685) for the full list of required changes. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 Review Outcome: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. Key issues found:

  1. Core bug not fixed — The compiler still reads LSP bindings from the config dict (node.config.get(...)) instead of the dedicated node.lsp_binding field. The original bug persists.
  2. Pervasive typolsp_bindingss (double 's') introduced throughout compiler.py (field names, function names, config keys).
  3. Unjustified schema renameNodeDefinition.lsp_binding renamed to lsp_bindings without spec justification. The field is NodeLspBinding | None (singular).
  4. Feature files not updated — Step definitions renamed but Gherkin scenarios still reference old names, causing test failures.
  5. All CI checks failing — lint, typecheck, unit_tests, integration_tests, e2e_tests, security.

The PR needs to be reworked to actually read from node.lsp_binding in the compiler, without renaming existing fields.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review Outcome: Changes Requested ❌ PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. Key issues found: 1. **Core bug not fixed** — The compiler still reads LSP bindings from the config dict (`node.config.get(...)`) instead of the dedicated `node.lsp_binding` field. The original bug persists. 2. **Pervasive typo** — `lsp_bindingss` (double 's') introduced throughout `compiler.py` (field names, function names, config keys). 3. **Unjustified schema rename** — `NodeDefinition.lsp_binding` renamed to `lsp_bindings` without spec justification. The field is `NodeLspBinding | None` (singular). 4. **Feature files not updated** — Step definitions renamed but Gherkin scenarios still reference old names, causing test failures. 5. **All CI checks failing** — lint, typecheck, unit_tests, integration_tests, e2e_tests, security. The PR needs to be reworked to actually read from `node.lsp_binding` in the compiler, without renaming existing fields. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 Review Outcome: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. The PR does not fix the actual bug — the compiler still reads LSP bindings from node.config.get(...) instead of from the dedicated node.lsp_binding field. Additionally, lsp_bindingss (double-s) typos were introduced throughout the compiler and test files, and the NodeDefinition.lsp_binding field was unnecessarily renamed to plural form.

See the full review on PR #1488 for detailed inline comments and the expected fix approach.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review Outcome: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. The PR does not fix the actual bug — the compiler still reads LSP bindings from `node.config.get(...)` instead of from the dedicated `node.lsp_binding` field. Additionally, `lsp_bindingss` (double-s) typos were introduced throughout the compiler and test files, and the `NodeDefinition.lsp_binding` field was unnecessarily renamed to plural form. See the [full review on PR #1488](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488#issuecomment-105449) for detailed inline comments and the expected fix approach. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 Review: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. Key issues found:

  1. Core bug not fixed_extract_lsp_bindings() still reads from node.config.get(...) instead of node.lsp_binding (the dedicated field). The fundamental issue described in this ticket is not addressed.
  2. Systematic typolsp_bindingss (double 's') introduced throughout compiler and test files.
  3. Feature/step mismatch.feature files not updated to match renamed step definitions, breaking 15+ Behave scenarios.
  4. No new test for the actual fix (per-node LSP binding extraction via the lsp_binding field).
  5. CI failing across lint, typecheck, security, unit_tests, integration_tests, and e2e_tests.

Detailed review comments posted on the PR.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. Key issues found: 1. **Core bug not fixed** — `_extract_lsp_bindings()` still reads from `node.config.get(...)` instead of `node.lsp_binding` (the dedicated field). The fundamental issue described in this ticket is not addressed. 2. **Systematic typo** — `lsp_bindingss` (double 's') introduced throughout compiler and test files. 3. **Feature/step mismatch** — `.feature` files not updated to match renamed step definitions, breaking 15+ Behave scenarios. 4. **No new test** for the actual fix (per-node LSP binding extraction via the `lsp_binding` field). 5. **CI failing** across lint, typecheck, security, unit_tests, integration_tests, and e2e_tests. Detailed review comments posted on the PR. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 Review: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. The PR has four critical issues:

  1. The actual bug is not fixed — the compiler still reads from node.config.get(...) instead of from node.lsp_binding (the dedicated Pydantic field)
  2. Typo: lsp_bindingss (double s) throughout compiler.py
  3. Spec-breaking rename of NodeDefinition.lsp_bindinglsp_bindings (spec uses singular)
  4. Feature files not updated to match renamed step definitions

All CI checks are failing. See the review comment on PR #1488 for full details and required changes.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. The PR has four critical issues: 1. **The actual bug is not fixed** — the compiler still reads from `node.config.get(...)` instead of from `node.lsp_binding` (the dedicated Pydantic field) 2. **Typo: `lsp_bindingss`** (double s) throughout `compiler.py` 3. **Spec-breaking rename** of `NodeDefinition.lsp_binding` → `lsp_bindings` (spec uses singular) 4. **Feature files not updated** to match renamed step definitions All CI checks are failing. See the [review comment on PR #1488](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488#issuecomment-112183) for full details and required changes. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 Review: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. The PR does not fix the core bug described in this issue. Key problems:

  1. Core bug unfixed_extract_lsp_bindings() still reads from the config dict (node.config.get(...)) instead of the dedicated node.lsp_bindings Pydantic field
  2. Typo introducedlsp_bindingss (double 's') throughout compiler.py
  3. Test/feature file mismatch — step definitions were renamed but .feature files were not, causing Behave step-matching failures
  4. Tests encode wrong behavior — tests still put LSP bindings in the config dict rather than using the dedicated field

See the full review on PR #1488 for details and guidance on what the correct fix should look like.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. The PR does not fix the core bug described in this issue. Key problems: 1. **Core bug unfixed** — `_extract_lsp_bindings()` still reads from the config dict (`node.config.get(...)`) instead of the dedicated `node.lsp_bindings` Pydantic field 2. **Typo introduced** — `lsp_bindingss` (double 's') throughout `compiler.py` 3. **Test/feature file mismatch** — step definitions were renamed but `.feature` files were not, causing Behave step-matching failures 4. **Tests encode wrong behavior** — tests still put LSP bindings in the config dict rather than using the dedicated field See the [full review on PR #1488](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488#issuecomment-115010) for details and guidance on what the correct fix should look like. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Author
Owner

PR #1488 Review: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. The PR does not fix the core bug — the compiler still reads LSP bindings from node.config.get(...) instead of from the dedicated node.lsp_binding field. Key issues:

  1. Core bug unfixed_extract_lsp_bindingss() still reads from the config dict only, never from node.lsp_binding
  2. lsp_bindingss typo (double 's') throughout compiler.py, breaking the public API
  3. Unjustified rename of NodeDefinition.lsp_bindinglsp_bindings (singular field shouldn't be plural)
  4. Commit message uses wrong scope (v3.7.0 instead of actor)
  5. No test for the actual bug fix

The PR needs a complete rework — ideally starting fresh from master with only the targeted fix. See the detailed review on PR #1488 for the full list of required changes and a code example of the correct fix.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## PR #1488 Review: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. The PR does not fix the core bug — the compiler still reads LSP bindings from `node.config.get(...)` instead of from the dedicated `node.lsp_binding` field. Key issues: 1. **Core bug unfixed** — `_extract_lsp_bindingss()` still reads from the config dict only, never from `node.lsp_binding` 2. **`lsp_bindingss` typo** (double 's') throughout `compiler.py`, breaking the public API 3. **Unjustified rename** of `NodeDefinition.lsp_binding` → `lsp_bindings` (singular field shouldn't be plural) 4. **Commit message** uses wrong scope (`v3.7.0` instead of `actor`) 5. **No test** for the actual bug fix The PR needs a complete rework — ideally starting fresh from master with only the targeted fix. See the [detailed review on PR #1488](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488#issuecomment-128409) for the full list of required changes and a code example of the correct fix. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
Owner

PR #1488 Review: Changes Requested

PR #1488 (fix/1432-lsp) was reviewed and changes were requested. The PR's single commit (d0cd3cfb) does not touch compiler.py at all — the core bug remains unfixed. Instead, the commit performs an unrelated mass find-and-replace of "config dict" → "actor_ref" across 24 files, which:

  1. Does not fix the bug_extract_lsp_bindings() still reads from node.config.get(...) instead of node.lsp_binding
  2. Introduces broken step definitions — step decorator text was renamed but .feature files were not, breaking 9+ Behave scenarios
  3. Creates nonsensical text — e.g., "a actor_refionary" from "a config dictionary"
  4. References wrong issue — commit says #1431, PR says #1432
  5. Branch is 6 days behind master — needs rebase

The PR needs a complete rework starting fresh from master with only the targeted compiler fix.

See the full review on PR #1488 for detailed analysis and a code example of the correct fix.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-self-reviewer

## PR #1488 Review: Changes Requested PR #1488 (`fix/1432-lsp`) was reviewed and **changes were requested**. The PR's single commit (`d0cd3cfb`) does not touch `compiler.py` at all — the core bug remains unfixed. Instead, the commit performs an unrelated mass find-and-replace of "config dict" → "actor_ref" across 24 files, which: 1. **Does not fix the bug** — `_extract_lsp_bindings()` still reads from `node.config.get(...)` instead of `node.lsp_binding` 2. **Introduces broken step definitions** — step decorator text was renamed but `.feature` files were not, breaking 9+ Behave scenarios 3. **Creates nonsensical text** — e.g., `"a actor_refionary"` from `"a config dictionary"` 4. **References wrong issue** — commit says `#1431`, PR says `#1432` 5. **Branch is 6 days behind master** — needs rebase The PR needs a complete rework starting fresh from master with only the targeted compiler fix. See the [full review on PR #1488](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488) for detailed analysis and a code example of the correct fix. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: pr-self-reviewer
Owner

PR #1488 Review: Changes Requested (Stale Review)

PR #1488 (fix/1432-lsp) was reviewed with focus on specification-compliance, requirements-coverage, and behavior-correctness. Changes were requested. No new commits have been pushed since the last review — all previously identified issues persist.

Key Findings:

  1. Specification non-compliance — The compiler still reads from node.config.get(...) instead of the dedicated node.lsp_binding Pydantic field defined in NodeDefinition. The NodeLspBindingLspBinding conversion is not implemented.

  2. Requirements coverage: 0/6 subtasks — None of the issue's subtasks are addressed: no compiler fix, no NodeLspBinding conversion, no Behave test, no Robot test, no coverage verification, CI failing.

  3. Behavior regression — The branch introduces a lsp_bindingss (double 's') typo that breaks the existing config-dict fallback path, making the branch strictly worse than master.

  4. No TDD tags — Bug fix PR lacks @tdd_issue_1432 tests.

Recommendation: Complete rework starting fresh from master with a targeted ~10-line fix to _extract_lsp_bindings() plus new tests.

See the full review on PR #1488 for detailed analysis and code examples.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-self-reviewer

## PR #1488 Review: Changes Requested (Stale Review) PR #1488 (`fix/1432-lsp`) was reviewed with focus on **specification-compliance, requirements-coverage, and behavior-correctness**. **Changes were requested.** No new commits have been pushed since the last review — all previously identified issues persist. ### Key Findings: 1. **Specification non-compliance** — The compiler still reads from `node.config.get(...)` instead of the dedicated `node.lsp_binding` Pydantic field defined in `NodeDefinition`. The `NodeLspBinding` → `LspBinding` conversion is not implemented. 2. **Requirements coverage: 0/6 subtasks** — None of the issue's subtasks are addressed: no compiler fix, no `NodeLspBinding` conversion, no Behave test, no Robot test, no coverage verification, CI failing. 3. **Behavior regression** — The branch introduces a `lsp_bindingss` (double 's') typo that breaks the existing config-dict fallback path, making the branch strictly worse than master. 4. **No TDD tags** — Bug fix PR lacks `@tdd_issue_1432` tests. **Recommendation:** Complete rework starting fresh from master with a targeted ~10-line fix to `_extract_lsp_bindings()` plus new tests. See the [full review on PR #1488](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/1488#issuecomment-143636) for detailed analysis and code examples. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: pr-self-reviewer
Sign in to join this conversation.
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core#1432
No description provided.