fix(actors): distinguish namespace/name from provider/model in actor name parsing #11255

Merged

CoreRasurae merged 5 commits from bugfix/m3-namespace-provider-name-collision into master 2026-05-23 13:31:18 +00:00

Conversation 3 Commits 5 Files Changed 21

+566 -64

CoreRasurae commented 2026-05-21 19:34:35 +00:00

Member

Copy Link

Copy Source

Description

Fixes #11254 — when action YAML references actors using namespace/name format (e.g. strategy_actor: local/my-strategist), _parse_actor_name() incorrectly treated the namespace prefix as a provider name, causing ValueError: Unknown provider type: local.

Changes

• _is_known_provider(): New utility function that checks whether a slash-separated first segment matches a known ProviderType value (openai, anthropic, etc.).

• _parse_actor_name(): When the first segment IS a known provider, preserves existing behaviour (provider/model). When it is NOT a known provider, treats the input as namespace/name and returns the full name as the model identifier with the default provider. Callers SHOULD pre-resolve namespace/name references via the actor registry before calling this function.

• PlanLifecycleService.resolve_actor_provider_model(): New method that resolves a namespaced actor name (e.g. local/my-strategist) to provider/model format by looking up the actor record and extracting its provider and model fields.

• Caller pre-resolution: StrategyActor, LLMStrategizeActor, and LLMExecuteActor now pre-resolve actor names through the lifecycle service before calling _parse_actor_name(), ensuring namespace/name references are correctly mapped to their underlying LLM providers.

Testing

• BDD scenarios added for namespace/name format parsing in both _parse_actor_name() implementations
• BDD scenarios added for _is_known_provider()
• All 15890+ existing scenarios continue to pass
• Lint and typecheck pass clean

Closes #11254

## Description Fixes #11254 — when action YAML references actors using namespace/name format (e.g. `strategy_actor: local/my-strategist`), `_parse_actor_name()` incorrectly treated the namespace prefix as a provider name, causing `ValueError: Unknown provider type: local`. ## Changes - **`_is_known_provider()`**: New utility function that checks whether a slash-separated first segment matches a known `ProviderType` value (`openai`, `anthropic`, etc.). - **`_parse_actor_name()`**: When the first segment IS a known provider, preserves existing behaviour (provider/model). When it is NOT a known provider, treats the input as namespace/name and returns the full name as the model identifier with the default provider. Callers SHOULD pre-resolve namespace/name references via the actor registry before calling this function. - **`PlanLifecycleService.resolve_actor_provider_model()`**: New method that resolves a namespaced actor name (e.g. `local/my-strategist`) to `provider/model` format by looking up the actor record and extracting its `provider` and `model` fields. - **Caller pre-resolution**: `StrategyActor`, `LLMStrategizeActor`, and `LLMExecuteActor` now pre-resolve actor names through the lifecycle service before calling `_parse_actor_name()`, ensuring namespace/name references are correctly mapped to their underlying LLM providers. ## Testing - BDD scenarios added for namespace/name format parsing in both `_parse_actor_name()` implementations - BDD scenarios added for `_is_known_provider()` - All 15890+ existing scenarios continue to pass - Lint and typecheck pass clean Closes #11254

CoreRasurae added this to the v3.2.0 milestone 2026-05-21 19:34:35 +00:00

CoreRasurae added the

Type

Bug

label 2026-05-21 19:34:54 +00:00

CoreRasurae added a new dependency 2026-05-21 20:45:22 +00:00

#11254 Bug: namespace prefix confused with provider name when executing a plan

CoreRasurae added the

Priority

Critical

label 2026-05-21 20:45:40 +00:00

CoreRasurae requested review from HAL9001 2026-05-21 20:45:46 +00:00

HAL9000 was assigned by CoreRasurae 2026-05-21 20:46:05 +00:00

brent.edwards requested changes 2026-05-21 22:01:50 +00:00

brent.edwards left a comment

Member

Copy Link

Copy Source

PR Review: Request Changes

Reviewer: Brent Edwards
Date: 2026-05-21
Review Type: First review
Outcome: REQUEST_CHANGES

---

CI Gate — BLOCKING

The coverage (pull_request) CI job is failing after 20+ minutes. All other required checks pass:

Job	Status
lint	✅ pass
typecheck	✅ pass
security	✅ pass
quality	✅ pass
unit_tests	✅ pass
integration_tests	✅ pass
build	✅ pass
docker	✅ pass
helm	✅ pass
push-validation	✅ pass
coverage	❌ FAIL
status-check	⏸ blocked

Per company policy, all 5 required-for-merge CI gates must pass (lint, typecheck, security, unit_tests, coverage) before the PR can be approved or merged. The failing coverage job is a hard blocker. Please fix the coverage failure and push a new commit.

---

Issue #11254 Scope — Incomplete Coverage

Issue #11254 lists 6 affected call sites for the ValueError: Unknown provider type: local bug:

Call Site	File	Status in this PR
StrategyActor._execute_with_llm()	strategy_actor.py:454-467	✅ Updated (pre-resolve)
LLMStrategizeActor.execute()	llm_actors.py:147-161	✅ Updated (pre-resolve)
LLMExecuteActor.execute()	llm_actors.py:376-393	✅ Updated (pre-resolve)
SessionWorkflow._resolve_llm()	session_workflow.py:383-391	❌ Not updated
validate_namespaced_actor()	cli/commands/plan.py:119-137	❌ Not updated
Action model from_config()	action.py:638-644	❌ Not updated

The PR correctly updates the three PlanLifecycleService-aware actors (StrategyActor, LLMStrategizeActor, LLMExecuteActor) with pre-resolution via resolve_actor_provider_model(). However, SessionWorkflow._resolve_llm() (line 365-391 in session_workflow.py) is explicitly listed in the issue as an affected call site and is not updated.

The _is_known_provider() check added to _parse_actor_name() will prevent the ValueError crash in SessionWorkflow (it now silently falls back to openai/<full_actor_name>), but this is incorrect behavior — the model field will contain the namespace/name string (e.g. "local/strategist") rather than the resolved model from the actor registry.

Required fix: Either:

• (a) Add the same pre-resolution pattern to SessionWorkflow._resolve_llm() (requires giving it lifecycle service access), or
• (b) If SessionWorkflow is intentionally out of scope for this PR, add an explicit advisory docstring to _resolve_llm() warning that namespace/name format is not supported there, and add a FIXME: #11254 reference.

The issue description does not carve out SessionWorkflow as out-of-scope, so option (a) is preferred.

---

Non-blocking Observations

1. # type: ignore[union-attr] workarounds — The hasattr(self._lifecycle, "resolve_actor_provider_model") guards followed by # type: ignore[union-attr] casts (e.g. llm_actors.py:171-172) are technically not # type: ignore suppressions, but they do serve the same purpose as a suppression. The contributing guide says "zero tolerance for # type: ignore" — while this pattern is not a literal # type: ignore, consider whether a protocol refinement or cast() from typing would be cleaner.

2. Coverage gap root cause — The failing coverage job is likely caused by uncovered branches in the new code: the logger.debug("…first segment is not a known provider…") path (strategy_resolution.py:157-161), the except Exception block in resolve_actor_provider_model() (plan_lifecycle_service.py:757-762), or the actor is None path (plan_lifecycle_service.py:765-767). Please add Behave scenarios to exercise these paths.

---

What Was Done Well

• The _is_known_provider() utility is a clean and reusable way to distinguish the two conventions — correctly implemented in both strategy_resolution.py and llm_actors.py.
• The resolve_actor_provider_model() method is a proper service-layer solution with appropriate null-checking and logging.
• The pre-resolution pattern in the three actor classes is consistent and well-isolated.
• The updated test scenarios for multi-slash and empty-model edge cases correctly reflect the new disambiguation behavior.
• Commit messages follow the Conventional Changelog format.

---

Summary

This PR is a solid fix for the core bug described in #11254 for the three PlanLifecycleService-aware actors. However, it cannot be approved because: (1) the coverage CI job is failing, which is a hard merge gate, and (2) SessionWorkflow._resolve_llm() — explicitly listed as an affected call site in the issue — is not addressed, leaving that code path silently producing incorrect LLM configurations instead of crashing.

Please address the coverage failure and either extend the fix to SessionWorkflow._resolve_llm() or explicitly document its limitation.

---

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## PR Review: Request Changes **Reviewer:** Brent Edwards **Date:** 2026-05-21 **Review Type:** First review **Outcome:** REQUEST_CHANGES --- ### CI Gate — BLOCKING The `coverage (pull_request)` CI job is **failing** after 20+ minutes. All other required checks pass: | Job | Status | |-----|--------| | lint | ✅ pass | | typecheck | ✅ pass | | security | ✅ pass | | quality | ✅ pass | | unit_tests | ✅ pass | | integration_tests | ✅ pass | | build | ✅ pass | | docker | ✅ pass | | helm | ✅ pass | | push-validation | ✅ pass | | **coverage** | ❌ **FAIL** | | status-check | ⏸ blocked | Per company policy, **all 5 required-for-merge CI gates must pass** (lint, typecheck, security, unit_tests, coverage) before the PR can be approved or merged. The failing coverage job is a hard blocker. Please fix the coverage failure and push a new commit. --- ### Issue #11254 Scope — Incomplete Coverage Issue #11254 lists **6 affected call sites** for the `ValueError: Unknown provider type: local` bug: | Call Site | File | Status in this PR | |-----------|------|-------------------| | `StrategyActor._execute_with_llm()` | `strategy_actor.py:454-467` | ✅ Updated (pre-resolve) | | `LLMStrategizeActor.execute()` | `llm_actors.py:147-161` | ✅ Updated (pre-resolve) | | `LLMExecuteActor.execute()` | `llm_actors.py:376-393` | ✅ Updated (pre-resolve) | | `SessionWorkflow._resolve_llm()` | `session_workflow.py:383-391` | ❌ **Not updated** | | `validate_namespaced_actor()` | `cli/commands/plan.py:119-137` | ❌ **Not updated** | | Action model `from_config()` | `action.py:638-644` | ❌ **Not updated** | The PR correctly updates the three `PlanLifecycleService`-aware actors (`StrategyActor`, `LLMStrategizeActor`, `LLMExecuteActor`) with pre-resolution via `resolve_actor_provider_model()`. However, **`SessionWorkflow._resolve_llm()`** (line 365-391 in `session_workflow.py`) is explicitly listed in the issue as an affected call site and is **not updated**. The `_is_known_provider()` check added to `_parse_actor_name()` will prevent the `ValueError` crash in `SessionWorkflow` (it now silently falls back to `openai/<full_actor_name>`), but this is **incorrect behavior** — the model field will contain the namespace/name string (e.g. `"local/strategist"`) rather than the resolved model from the actor registry. **Required fix:** Either: - (a) Add the same pre-resolution pattern to `SessionWorkflow._resolve_llm()` (requires giving it lifecycle service access), or - (b) If `SessionWorkflow` is intentionally out of scope for this PR, add an explicit advisory docstring to `_resolve_llm()` warning that namespace/name format is not supported there, and add a `FIXME: #11254` reference. The issue description does not carve out `SessionWorkflow` as out-of-scope, so option (a) is preferred. --- ### Non-blocking Observations 1. **`# type: ignore[union-attr]` workarounds** — The `hasattr(self._lifecycle, "resolve_actor_provider_model")` guards followed by `# type: ignore[union-attr]` casts (e.g. `llm_actors.py:171-172`) are technically not `# type: ignore` suppressions, but they do serve the same purpose as a suppression. The contributing guide says "zero tolerance for `# type: ignore`" — while this pattern is not a literal `# type: ignore`, consider whether a protocol refinement or `cast()` from `typing` would be cleaner. 2. **Coverage gap root cause** — The failing coverage job is likely caused by uncovered branches in the new code: the `logger.debug("…first segment is not a known provider…")` path (strategy_resolution.py:157-161), the `except Exception` block in `resolve_actor_provider_model()` (plan_lifecycle_service.py:757-762), or the `actor is None` path (plan_lifecycle_service.py:765-767). Please add Behave scenarios to exercise these paths. --- ### What Was Done Well - The `_is_known_provider()` utility is a clean and reusable way to distinguish the two conventions — correctly implemented in both `strategy_resolution.py` and `llm_actors.py`. - The `resolve_actor_provider_model()` method is a proper service-layer solution with appropriate null-checking and logging. - The pre-resolution pattern in the three actor classes is consistent and well-isolated. - The updated test scenarios for multi-slash and empty-model edge cases correctly reflect the new disambiguation behavior. - Commit messages follow the Conventional Changelog format. --- ### Summary This PR is a solid fix for the core bug described in #11254 for the three `PlanLifecycleService`-aware actors. However, it cannot be approved because: (1) the coverage CI job is failing, which is a hard merge gate, and (2) `SessionWorkflow._resolve_llm()` — explicitly listed as an affected call site in the issue — is not addressed, leaving that code path silently producing incorrect LLM configurations instead of crashing. Please address the coverage failure and either extend the fix to `SessionWorkflow._resolve_llm()` or explicitly document its limitation. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from 6fd4b9bce6 to de73ef9c3b 2026-05-22 10:52:00 +00:00 Compare

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from de73ef9c3b to 4427845446 2026-05-22 10:55:45 +00:00 Compare

hamza.khyari requested changes 2026-05-22 12:56:47 +00:00

Dismissed

hamza.khyari left a comment

Member

Copy Link

Copy Source

PR #11255 Review — fix(actors): distinguish namespace/name from provider/model in actor name parsing

---

Context & Prior Review

brent.edwards submitted REQUEST_CHANGES citing: (1) failing coverage CI job, and (2) SessionWorkflow._resolve_llm() not addressed. Both concerns are resolved in the current push — session_workflow.py and cli/commands/session.py are both updated, and CI now shows success. This is a fresh assessment of the current HEAD (44278454).

---

🔴 Blocking Issues

1. # type: ignore[union-attr] used 3 times — zero tolerance violated

Project policy: zero inline type suppressions, no exceptions. This PR adds three:

• strategy_actor.py — self._lifecycle.resolve_actor_provider_model( # type: ignore[union-attr]
• llm_actors.py (LLMStrategizeActor) — same pattern
• llm_actors.py (LLMExecuteActor) — same pattern

The suppression is needed because _lifecycle is typed as a protocol/abstract that doesn't declare resolve_actor_provider_model. The correct fix is to add this method to the relevant interface/protocol rather than suppressing the type error at the call site.

---

2. Multiple inline imports inside functions — project import rules violated

Project rules require all imports at the top of the file. The only permitted exception is if TYPE_CHECKING:. This PR adds inline imports in at least 5 locations:

• llm_actors.py — _is_known_provider() imports ProviderType inline
• strategy_resolution.py — _is_known_provider() imports ProviderType inline
• plan_lifecycle_service.py — resolve_actor_provider_model() imports _is_known_provider inline
• a2a/facade.py — _build_actor_resolver_for_session_workflow() imports get_container, _is_known_provider, NotFoundError inline inside a try
• cli/commands/session.py — _build_actor_resolver() imports get_container, _is_known_provider, NotFoundError inline inside a try

If circular imports prevent top-level placement, the solution is to restructure the dependency graph — not use inline imports.

---

3. _is_known_provider duplicated identically across two modules

An identical implementation exists in both llm_actors.py and strategy_resolution.py:

def _is_known_provider(provider: str) -> bool:
    from cleveragents.providers.registry import ProviderType
    return provider in ProviderType.__members__.values()

If the logic ever changes, both copies must be updated in sync. llm_actors.py should import _is_known_provider from strategy_resolution.py rather than defining its own copy.

---

4. Missing @tdd_issue @tdd_issue_11254 tags on all new BDD scenarios

This is a Type/Bug fix. No tdd/m3-* capture branch for issue #11254 exists on the remote and none of the new scenarios in llm_actors_coverage.feature, strategy_actor_llm.feature, or plan_lifecycle_service_coverage_boost_r4.feature carry @tdd_issue @tdd_issue_11254 tags. Per the TDD bug-fix workflow, the fix PR must include these two tags (without @tdd_expected_fail) as the permanent regression guard. CI will block on their absence.

---

🟠 Code Quality

5. _build_actor_resolver logic duplicated in facade.py and session.py

A2aLocalFacade._build_actor_resolver_for_session_workflow() and _build_actor_resolver() in cli/commands/session.py implement near-identical logic — both fetch actor_service from the container, build the same resolve() closure, and fall back on failure. The only difference is the fallback value (None vs _null_actor_resolver). This should be a single shared utility.

6. except (NotFoundError, Exception) is redundant

In both resolve() closures (facade.py and session.py):

except (NotFoundError, Exception):
    return None

Exception is a superclass of NotFoundError — listing both is redundant. Use except Exception alone, or narrow to the specific exceptions expected.

7. Outer except Exception swallows initialisation failures silently

In _build_actor_resolver() (session.py) and _build_actor_resolver_for_session_workflow() (facade.py), any failure during container/service setup is caught with no log entry:

except Exception:
    return _null_actor_resolver  # no warning logged

If the DI container is misconfigured, this degrades silently. A logger.warning(...) should be added to make startup failures observable.

---

🟡 Metadata

8. Issue #11254 still in State/Verified — should be State/In Review

The linked issue must be moved to State/In Review when a PR is submitted. Issue #11254 is still showing State/Verified.

9. docs/CHANGELOG.md not updated

CHANGELOG.md is updated but docs/CHANGELOG.md is not. The preceding PR (#11246) updated both. Please verify whether both files require entries for every change and update accordingly.

10. Commit body does not mention SessionWorkflow and session.py

The commit message body lists StrategyActor, LLMStrategizeActor, and LLMExecuteActor as updated callers but omits SessionWorkflow and cli/commands/session.py, which are also materially changed in this commit. The body should reflect all affected call sites.

---

✅ What Is Correct

• Branch name bugfix/m3-namespace-provider-name-collision ✅
• Milestone v3.2.0 matches linked issue ✅
• Labels Priority/Critical + Type/Bug ✅
• Forgejo dependency direction correct — issue #11254 depends on PR #11255 ✅
• Commit footer ISSUES CLOSED: #11254 ✅
• CHANGELOG updated ✅
• ProviderType is a StrEnum — __members__.values() comparison is correct ✅
• resolve_actor_provider_model() on PlanLifecycleService is well-structured with null-checking and logging ✅
• _null_actor_resolver applies Null Object pattern correctly ✅
• SessionWorkflow and cli/commands/session.py are addressed — brent.edwards' primary concern is resolved ✅
• Updated test expectations for multi-slash and empty-model edge cases correctly reflect new behavior ✅

---

Summary

#	Severity	Issue
1	🔴	# type: ignore[union-attr] — 3 instances, zero tolerance violated
2	🔴	Inline imports inside functions — 5+ locations, project rules violated
3	🔴	_is_known_provider duplicated identically in llm_actors.py and strategy_resolution.py
4	🔴	Missing @tdd_issue @tdd_issue_11254 tags on all new BDD scenarios
5	🟠	_build_actor_resolver logic duplicated in facade.py and session.py
6	🟠	except (NotFoundError, Exception) is redundant
7	🟠	Outer except Exception in resolver builders swallows errors with no log
8	🟡	Issue #11254 still State/Verified — should be State/In Review
9	🟡	docs/CHANGELOG.md not updated
10	🟡	Commit body omits SessionWorkflow and session.py from list of updated callers

Please address the 🔴 items and re-request review.

## PR #11255 Review — `fix(actors): distinguish namespace/name from provider/model in actor name parsing` --- ## Context & Prior Review brent.edwards submitted `REQUEST_CHANGES` citing: (1) failing `coverage` CI job, and (2) `SessionWorkflow._resolve_llm()` not addressed. **Both concerns are resolved in the current push** — `session_workflow.py` and `cli/commands/session.py` are both updated, and CI now shows success. This is a fresh assessment of the current HEAD (`44278454`). --- ## 🔴 Blocking Issues ### 1. `# type: ignore[union-attr]` used 3 times — zero tolerance violated Project policy: zero inline type suppressions, no exceptions. This PR adds three: - `strategy_actor.py` — `self._lifecycle.resolve_actor_provider_model( # type: ignore[union-attr]` - `llm_actors.py` (LLMStrategizeActor) — same pattern - `llm_actors.py` (LLMExecuteActor) — same pattern The suppression is needed because `_lifecycle` is typed as a protocol/abstract that doesn't declare `resolve_actor_provider_model`. The correct fix is to add this method to the relevant interface/protocol rather than suppressing the type error at the call site. --- ### 2. Multiple inline imports inside functions — project import rules violated Project rules require all imports at the top of the file. The only permitted exception is `if TYPE_CHECKING:`. This PR adds inline imports in at least 5 locations: - `llm_actors.py` — `_is_known_provider()` imports `ProviderType` inline - `strategy_resolution.py` — `_is_known_provider()` imports `ProviderType` inline - `plan_lifecycle_service.py` — `resolve_actor_provider_model()` imports `_is_known_provider` inline - `a2a/facade.py` — `_build_actor_resolver_for_session_workflow()` imports `get_container`, `_is_known_provider`, `NotFoundError` inline inside a `try` - `cli/commands/session.py` — `_build_actor_resolver()` imports `get_container`, `_is_known_provider`, `NotFoundError` inline inside a `try` If circular imports prevent top-level placement, the solution is to restructure the dependency graph — not use inline imports. --- ### 3. `_is_known_provider` duplicated identically across two modules An identical implementation exists in both `llm_actors.py` and `strategy_resolution.py`: ```python def _is_known_provider(provider: str) -> bool: from cleveragents.providers.registry import ProviderType return provider in ProviderType.__members__.values() ``` If the logic ever changes, both copies must be updated in sync. `llm_actors.py` should import `_is_known_provider` from `strategy_resolution.py` rather than defining its own copy. --- ### 4. Missing `@tdd_issue @tdd_issue_11254` tags on all new BDD scenarios This is a `Type/Bug` fix. No `tdd/m3-*` capture branch for issue #11254 exists on the remote and none of the new scenarios in `llm_actors_coverage.feature`, `strategy_actor_llm.feature`, or `plan_lifecycle_service_coverage_boost_r4.feature` carry `@tdd_issue @tdd_issue_11254` tags. Per the TDD bug-fix workflow, the fix PR must include these two tags (without `@tdd_expected_fail`) as the permanent regression guard. CI will block on their absence. --- ## 🟠 Code Quality ### 5. `_build_actor_resolver` logic duplicated in `facade.py` and `session.py` `A2aLocalFacade._build_actor_resolver_for_session_workflow()` and `_build_actor_resolver()` in `cli/commands/session.py` implement near-identical logic — both fetch `actor_service` from the container, build the same `resolve()` closure, and fall back on failure. The only difference is the fallback value (`None` vs `_null_actor_resolver`). This should be a single shared utility. ### 6. `except (NotFoundError, Exception)` is redundant In both `resolve()` closures (`facade.py` and `session.py`): ```python except (NotFoundError, Exception): return None ``` `Exception` is a superclass of `NotFoundError` — listing both is redundant. Use `except Exception` alone, or narrow to the specific exceptions expected. ### 7. Outer `except Exception` swallows initialisation failures silently In `_build_actor_resolver()` (`session.py`) and `_build_actor_resolver_for_session_workflow()` (`facade.py`), any failure during container/service setup is caught with no log entry: ```python except Exception: return _null_actor_resolver # no warning logged ``` If the DI container is misconfigured, this degrades silently. A `logger.warning(...)` should be added to make startup failures observable. --- ## 🟡 Metadata ### 8. Issue #11254 still in `State/Verified` — should be `State/In Review` The linked issue must be moved to `State/In Review` when a PR is submitted. Issue #11254 is still showing `State/Verified`. ### 9. `docs/CHANGELOG.md` not updated `CHANGELOG.md` is updated but `docs/CHANGELOG.md` is not. The preceding PR (#11246) updated both. Please verify whether both files require entries for every change and update accordingly. ### 10. Commit body does not mention `SessionWorkflow` and `session.py` The commit message body lists `StrategyActor`, `LLMStrategizeActor`, and `LLMExecuteActor` as updated callers but omits `SessionWorkflow` and `cli/commands/session.py`, which are also materially changed in this commit. The body should reflect all affected call sites. --- ## ✅ What Is Correct - Branch name `bugfix/m3-namespace-provider-name-collision` ✅ - Milestone `v3.2.0` matches linked issue ✅ - Labels `Priority/Critical` + `Type/Bug` ✅ - Forgejo dependency direction correct — issue #11254 depends on PR #11255 ✅ - Commit footer `ISSUES CLOSED: #11254` ✅ - CHANGELOG updated ✅ - `ProviderType` is a `StrEnum` — `__members__.values()` comparison is correct ✅ - `resolve_actor_provider_model()` on `PlanLifecycleService` is well-structured with null-checking and logging ✅ - `_null_actor_resolver` applies Null Object pattern correctly ✅ - `SessionWorkflow` and `cli/commands/session.py` are addressed — brent.edwards' primary concern is resolved ✅ - Updated test expectations for multi-slash and empty-model edge cases correctly reflect new behavior ✅ --- ## Summary | # | Severity | Issue | |---|----------|-------| | 1 | 🔴 | `# type: ignore[union-attr]` — 3 instances, zero tolerance violated | | 2 | 🔴 | Inline imports inside functions — 5+ locations, project rules violated | | 3 | 🔴 | `_is_known_provider` duplicated identically in `llm_actors.py` and `strategy_resolution.py` | | 4 | 🔴 | Missing `@tdd_issue @tdd_issue_11254` tags on all new BDD scenarios | | 5 | 🟠 | `_build_actor_resolver` logic duplicated in `facade.py` and `session.py` | | 6 | 🟠 | `except (NotFoundError, Exception)` is redundant | | 7 | 🟠 | Outer `except Exception` in resolver builders swallows errors with no log | | 8 | 🟡 | Issue #11254 still `State/Verified` — should be `State/In Review` | | 9 | 🟡 | `docs/CHANGELOG.md` not updated | | 10 | 🟡 | Commit body omits `SessionWorkflow` and `session.py` from list of updated callers | Please address the 🔴 items and re-request review.

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from 4427845446 to eaa84799d8 2026-05-22 15:54:37 +00:00 Compare

hamza.khyari approved these changes 2026-05-22 16:14:41 +00:00

hamza.khyari left a comment

Member

Copy Link

Copy Source

Re-Review Summary (Round 2) — PR #11255

Excellent progress — 8 of 10 previous findings fully resolved, 1 partially, 1 still open. The implementation is correct and the core quality issues are addressed. Approving with two remaining comments.

---

✅ Resolved from Previous Review

#	Finding	What was done
1	# type: ignore[union-attr] — 3 instances	✅ resolve_actor_provider_model added to LifecycleService protocol (strategy_resolution.py) and PlanLifecycleProtocol (llm_actors.py) — all suppressions removed
2	Inline imports — 5+ locations	✅ ProviderType now top-level in strategy_resolution.py; _is_known_provider top-level in llm_actors.py, plan_lifecycle_service.py, facade.py, session.py; _parse_actor_name top-level in session_workflow.py
3	_is_known_provider duplicated	✅ Consolidated in strategy_resolution.py; llm_actors.py imports from there
6	except (NotFoundError, Exception) redundant	✅ Both closures now use except Exception
7	Outer except Exception silent	✅ logger.warning("actor_resolver_unavailable", exc_info=True) added in both facade.py and session.py
8	Issue State/Verified	✅ Issue #11254 now State/In Review
9	docs/CHANGELOG.md not updated	✅ Entry added
10	Commit body omits SessionWorkflow	✅ Commit message now lists all affected call sites

---

⚠️ Partially Resolved

Finding 4 — 2 scenarios in plan_lifecycle_service_coverage_boost_r4.feature still missing @tdd_issue @tdd_issue_11254

The 4 core regression scenarios correctly carry the tags. The following 2 do not:

Scenario: resolve_actor_provider_model returns None when unit_of_work is absent
Scenario: resolve_actor_provider_model returns None for empty actor name

If these scenarios exercise code paths introduced specifically by this bug fix, they should carry the tags as part of the regression guard. If they are considered pure coverage scenarios unrelated to the bug, that should be made explicit via a comment. Please clarify and tag if applicable in a follow-up.

---

🟠 Still Open

Finding 5 — _build_actor_resolver closure logic duplicated in facade.py and session.py

The resolver closure (checking _is_known_provider, calling actor_service.get_actor, returning f"{actor.provider}/{actor.model}") is still identical in both _build_actor_resolver_for_session_workflow() and _build_actor_resolver(). The get_container inline import for lazy initialization is defensible, but the resolver closure itself should be extracted to a shared utility to prevent divergence. Please address in a follow-up issue.

---

Summary

#	Severity	Issue	Status
4	⚠️	2 scenarios missing @tdd_issue @tdd_issue_11254 in plan_lifecycle_service_coverage_boost_r4.feature	Partially resolved — follow-up
5	🟠	_build_actor_resolver closure duplicated in facade.py and session.py	Unresolved — follow-up
1–3, 6–10	✅	All other findings	Resolved

The fix is sound and production-ready. The two remaining items are non-blocking and can be addressed in follow-up issues.

## Re-Review Summary (Round 2) — PR #11255 Excellent progress — **8 of 10 previous findings fully resolved**, 1 partially, 1 still open. The implementation is correct and the core quality issues are addressed. Approving with two remaining comments. --- ## ✅ Resolved from Previous Review | # | Finding | What was done | |---|---------|---------------| | 1 | `# type: ignore[union-attr]` — 3 instances | ✅ `resolve_actor_provider_model` added to `LifecycleService` protocol (`strategy_resolution.py`) and `PlanLifecycleProtocol` (`llm_actors.py`) — all suppressions removed | | 2 | Inline imports — 5+ locations | ✅ `ProviderType` now top-level in `strategy_resolution.py`; `_is_known_provider` top-level in `llm_actors.py`, `plan_lifecycle_service.py`, `facade.py`, `session.py`; `_parse_actor_name` top-level in `session_workflow.py` | | 3 | `_is_known_provider` duplicated | ✅ Consolidated in `strategy_resolution.py`; `llm_actors.py` imports from there | | 6 | `except (NotFoundError, Exception)` redundant | ✅ Both closures now use `except Exception` | | 7 | Outer `except Exception` silent | ✅ `logger.warning("actor_resolver_unavailable", exc_info=True)` added in both `facade.py` and `session.py` | | 8 | Issue `State/Verified` | ✅ Issue #11254 now `State/In Review` | | 9 | `docs/CHANGELOG.md` not updated | ✅ Entry added | | 10 | Commit body omits `SessionWorkflow` | ✅ Commit message now lists all affected call sites | --- ## ⚠️ Partially Resolved ### Finding 4 — 2 scenarios in `plan_lifecycle_service_coverage_boost_r4.feature` still missing `@tdd_issue @tdd_issue_11254` The 4 core regression scenarios correctly carry the tags. The following 2 do not: ```gherkin Scenario: resolve_actor_provider_model returns None when unit_of_work is absent Scenario: resolve_actor_provider_model returns None for empty actor name ``` If these scenarios exercise code paths introduced specifically by this bug fix, they should carry the tags as part of the regression guard. If they are considered pure coverage scenarios unrelated to the bug, that should be made explicit via a comment. Please clarify and tag if applicable in a follow-up. --- ## 🟠 Still Open ### Finding 5 — `_build_actor_resolver` closure logic duplicated in `facade.py` and `session.py` The resolver closure (checking `_is_known_provider`, calling `actor_service.get_actor`, returning `f"{actor.provider}/{actor.model}"`) is still identical in both `_build_actor_resolver_for_session_workflow()` and `_build_actor_resolver()`. The `get_container` inline import for lazy initialization is defensible, but the resolver closure itself should be extracted to a shared utility to prevent divergence. Please address in a follow-up issue. --- ## Summary | # | Severity | Issue | Status | |---|----------|-------|--------| | 4 | ⚠️ | 2 scenarios missing `@tdd_issue @tdd_issue_11254` in `plan_lifecycle_service_coverage_boost_r4.feature` | Partially resolved — follow-up | | 5 | 🟠 | `_build_actor_resolver` closure duplicated in `facade.py` and `session.py` | Unresolved — follow-up | | 1–3, 6–10 | ✅ | All other findings | Resolved | The fix is sound and production-ready. The two remaining items are non-blocking and can be addressed in follow-up issues.

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from eaa84799d8 to a351d3726b 2026-05-22 17:48:35 +00:00 Compare

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from a351d3726b to fb1b6ffdae 2026-05-22 17:50:20 +00:00 Compare

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from fb1b6ffdae to 696426ec8d 2026-05-22 17:52:13 +00:00 Compare

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from 696426ec8d to 3e13411fcf 2026-05-22 19:43:10 +00:00 Compare

CoreRasurae scheduled this pull request to auto merge when all checks succeed 2026-05-22 19:45:58 +00:00

CoreRasurae closed this pull request 2026-05-22 23:04:33 +00:00

CoreRasurae reopened this pull request 2026-05-22 23:04:36 +00:00

CoreRasurae added 1 commit 2026-05-23 00:02:09 +00:00

test(persistence): remove stale @tdd_issue tags from EntityStore persistence tests ... 73d3bed2da

The _load_from_persistence() and _persist_if_needed() stubs have been
replaced with real SQLite persistence implementations, fixing bug #10455.
All five scenarios now pass as regular regression tests.

Refs: #10455

CoreRasurae closed this pull request 2026-05-23 10:50:50 +00:00

CoreRasurae reopened this pull request 2026-05-23 10:50:54 +00:00

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from 6b121bf64e to cdbe504b2c 2026-05-23 11:20:22 +00:00 Compare

CoreRasurae force-pushed bugfix/m3-namespace-provider-name-collision from 25430e5a9f to 190606d7e6 2026-05-23 12:23:08 +00:00 Compare

CoreRasurae added 1 commit 2026-05-23 13:14:23 +00:00

fix(memory): use **kwargs to avoid Pyright param name mismatch ... f1a4858a29

langchain-community renamed connection_string → connection, but
the locally installed stubs only know connection_string while CI
has the newer API. Build kwargs dict dynamically to satisfy both
environments without # type: ignore annotations.

CoreRasurae merged commit f1a4858a29 into master 2026-05-23 13:31:18 +00:00

CoreRasurae deleted branch bugfix/m3-namespace-provider-name-collision 2026-05-23 13:31:18 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

HAL9001

brent.edwards

hamza.khyari

Labels

Clear labels

auto/needs-reevaluation

Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

controller-managed

Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

overdue

auto/blocked-by-deps

PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

auto/ci-timeout

Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

auto/claimed-implementer

Currently being processed by an implementer worker.

auto/claimed-merge

Currently being processed by the merge driver.

auto/claimed-reviewer

Currently being processed by a reviewer worker.

auto/driver-down

Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

auto/invariant-violation

Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

auto/last-attempt-tier-0

In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

auto/last-attempt-tier-1

In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

auto/last-attempt-tier-2

In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

auto/last-attempt-tier-min

In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

Automation Tracking

Tracking issues used by the AI Automation system for agents to communicate and report.

auto/needs-conflict-resolution

Rebase conflict needs LLM conflict-resolver.

auto/needs-implementer

Failing CI needs implementer attention.

auto/postmortem

Documenting a driver incident or rollback.

auto/ready-to-merge

Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

auto/restart-throttled

Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

auto/revert

Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

auto/sentinel

Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

auto/stale-inactivity

No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

auto/unstable

Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

Blocked

A ticket in a blocked state and unable to complete until some other task is completed first.

Bounty

$100

A bounty of $100 for any open-source contributor who provides a MR that solves this issue

Bounty

$1000

A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

Bounty

$10000

A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

Bounty

$20

A bounty of $20 for any open-source contributor who provides a MR that solves this issue

Bounty

$2000

A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

Bounty

$250

A bounty of $250 for any open-source contributor who provides a MR that solves this issue

Bounty

$50

A bounty of $50 for any open-source contributor who provides a MR that solves this issue

Bounty

$500

A bounty of $500 for any open-source contributor who provides a MR that solves this issue

Bounty

$5000

A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

Bounty

$750

A bounty of $750 for any open-source contributor who provides a MR that solves this issue

MoSCoW

Could have

Could have feature in order to satisfy the epic/legendary.

MoSCoW

Must have

Must have feature in order to satisfy the epic/legendary.

MoSCoW

Should have

Should have feature in order to satisfy the epic/legendary.

Needs Feedback

There are questions in the ticket that can not be completed until the project owner provides clarity.

Points

1

1 man-hours worth of work for an expert with no learning curve.

Points

13

13 man-hours worth of work for an expert with no learning curve.

Points

2

2 man-hours worth of work for an expert with no learning curve.

Points

21

21 man-hours worth of work for an expert with no learning curve.

Points

3

3 man-hours worth of work for an expert with no learning curve.

Points

34

34 man-hours worth of work for an expert with no learning curve.

Points

5

5 man-hours worth of work for an expert with no learning curve.

Points

55

55 man-hours worth of work for an expert with no learning curve.

Points

8

8 man-hours worth of work for an expert with no learning curve.

Points

88

88 man-hours worth of work for an expert with no learning curve.

Priority

Backlog

This ticket has backlogged priority and is not to be worked on yet

Priority

CI Blocker

Critical priority issue that blocks CI/CD pipeline and prevents PR merges

Priority

Critical

The priority is critical

Priority

High

The priority is high

Priority

Low

The priority is low

Priority

Medium

The priority is medium

Signed-off: Owner

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

Signed-off: Scrum Master

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

Signed-off: Tech Lead

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

Spike

A ticket for learning a tool or technology that is needed to be able to do future planning and design.

State

Completed

The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

State

Duplicate

A ticket that represents the same content as an existing ticket.

State

In Progress

A ticket that is actively being developed.

State

In Review

A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

State

Paused

This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

State

Unverified

All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

State

Verified

The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

State

Wont Do

This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

Type

Automation

Any edits or discussion about the AI automated coding system.

Type

Bug

Something that doesnt work as intended.

Type

Discussion

Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

Type

Documentation

An error or improvement needed in the documentation.

Type

Epic

Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

Type

Feature

Some new functionality not present.

Type

Legendary

A type of Epic which will contain other Epics.

Type

Refactor

A code change that restructures existing code without changing its external behavior.

Type

Support

Someone needs help using the project.

Type

Task

A generic task that doesnt fit into the other type categories.

Type

Testing

Work exclusively focusing on fixing or expanding testing.

No Label

Priority

Critical

Type

Bug

Milestone

No items

No Milestone v3.2.0

Projects

Clear projects

No project

Assignees

Clear assignees

aditya (Aditya Chhabra) aleenaumair (Aleena Umair) brent.edwards (Brent Edwards) CoreRasurae (Luis Mendes) drew (Drew Morris) eugen.thaci (Eugen Thaci) freemo (Jeffrey Phillips Freeman) HAL9000 (HAL 9000) HAL9001 (HAL9001) hamza.khyari (Hamza Khyari) hurui200320 (Rui Hu) justin.morris khird (Kyle Hird) org.cleveragents

No Assignees HAL9000

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Blocks

#11254 Bug: namespace prefix confused with provider name when executing a plan

cleveragents/cleveragents-core

Reference: cleveragents/cleveragents-core#11255