UAT: agents actor add --config uses v2 ActorConfiguration parser — v3 ActorConfigSchema (type: llm|tool|graph, route:) not validated on registration #5869

Closed
opened 2026-04-09 11:07:24 +00:00 by HAL9000 · 7 comments
Owner

Bug Report

Feature Area: Actor System — v3 YAML schema validation on actor registration
Severity: Critical (v3 actor YAML files with type: graph, route:, context_view: etc. bypass Pydantic v3 schema validation when registered via CLI)
Discovered by: UAT Testing (uat-pool-1, worker: actor-system)


What Was Tested

Whether the agents actor add --config <FILE> command validates actor YAML files against the v3 ActorConfigSchema Pydantic model.

Expected Behavior (from spec)

Per v3.1.0 milestone acceptance criteria:

"Actor YAML files with version: "3", type: llm|tool|graph, and all schema fields parse and validate correctly via Pydantic models"

The v3 ActorConfigSchema (src/cleveragents/actor/schema.py) provides comprehensive validation:

  • type: llm|tool|graph enum validation
  • route: field required for GRAPH actors
  • model: field required for LLM/GRAPH actors
  • tools: required for TOOL actors
  • Graph topology validation (cycle detection, entry/exit validation, unreachable node detection)
  • Node ID format validation
  • Tool name namespace validation
  • context_view: enum validation (strategist/executor/reviewer/full)

Actual Behavior (from implementation)

The agents actor add CLI command (src/cleveragents/cli/commands/actor.py) uses _canonicalize_actor_config() which calls ActorConfiguration.from_blob() — the v2 parser (src/cleveragents/actor/config.py). This parser:

  1. Does NOT validate type: llm|tool|graph
  2. Does NOT validate route: structure
  3. Does NOT perform cycle detection
  4. Does NOT validate context_view: values
  5. Does NOT validate node IDs or tool namespacing
  6. Requires provider and model fields (v2 format) even for v3 actors

The v3 ActorConfigSchema is never invoked during the agents actor add flow. It is only used by ActorLoader.discover() (filesystem scanning) and in unit tests.

Code Location

CLI add command (src/cleveragents/cli/commands/actor.py):

# Line 573 — uses v2 ActorConfiguration, not v3 ActorConfigSchema
resolved, canonical_blob, requires_confirmation = _canonicalize_actor_config(
    name=name,
    config_blob=config_blob,
    ...
)

# Line 621 — bypasses registry.add() which also uses v2 parser
actor = registry.upsert_actor(
    name=name,
    config_blob=canonical_blob,
    ...
)

_canonicalize_actor_config (line 348):

resolved = ActorConfiguration.from_blob(  # v2 parser!
    blob=config_blob,
    ...
)

ActorRegistry.add() (line 208) — also uses v2 parser:

blob = ActorConfiguration.load_yaml_text(yaml_text)  # v2 parser!

Steps to Reproduce

Create a v3 GRAPH actor YAML with a cycle (should be rejected):

name: local/broken-graph
type: graph
description: Graph with cycle
model: gpt-4
route:
  nodes:
    - id: node_a
      type: agent
      name: A
      description: Node A
      config: {}
    - id: node_b
      type: agent
      name: B
      description: Node B
      config: {}
  edges:
    - from_node: node_a
      to_node: node_b
    - from_node: node_b
      to_node: node_a  # CYCLE!
  entry_node: node_a
  exit_nodes: [node_b]

Expected: agents actor add local/broken-graph --config broken.yaml should fail with cycle detection error.
Actual: The actor is registered successfully (cycle not detected because v3 schema is not invoked).

Impact

  • Invalid v3 actor YAML files (with cycles, invalid node types, missing required fields) are silently accepted and stored in the registry
  • The v3 ActorConfigSchema Pydantic model is effectively dead code for the CLI registration path
  • Actors registered via CLI may fail at runtime when the compiler tries to compile them
  • The v3.1.0 milestone acceptance criterion "Actor YAML files parse and validate correctly via Pydantic models" is not met for the CLI path

Fix

In the agents actor add CLI command:

  1. Parse the YAML file through ActorConfigSchema.from_yaml_file() first for v3 validation
  2. If validation passes, proceed with registration
  3. Alternatively, update ActorRegistry.add() to use ActorConfigSchema for v3 YAML files (detected by type: llm|tool|graph or version: "3.0")

Metadata

Field Value
Commit Message fix(actor): validate v3 YAML via ActorConfigSchema in agents actor add CLI
Branch fix/actor-add-v3-schema-validation
Milestone v3.1.0

Subtasks

  • Add ActorConfigSchema.from_yaml_file() call in agents actor add CLI command
  • Detect v3 YAML files (by type: llm|tool|graph or version: "3.0")
  • Return clear validation errors from ActorConfigSchema to the user
  • Update ActorRegistry.add() to use v3 schema for v3 YAML files
  • Add integration test: registering a GRAPH actor with a cycle should fail

Definition of Done

  • agents actor add --config graph-with-cycle.yaml fails with cycle detection error
  • agents actor add --config llm-without-model.yaml fails with "LLM actors require 'model' field"
  • agents actor add --config tool-without-tools.yaml fails with "TOOL actors require at least one tool"
  • All v3 schema validations (node IDs, tool namespacing, context_view enum) are enforced at registration time

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: uat-tester

## Bug Report **Feature Area**: Actor System — v3 YAML schema validation on actor registration **Severity**: Critical (v3 actor YAML files with `type: graph`, `route:`, `context_view:` etc. bypass Pydantic v3 schema validation when registered via CLI) **Discovered by**: UAT Testing (uat-pool-1, worker: actor-system) --- ## What Was Tested Whether the `agents actor add --config <FILE>` command validates actor YAML files against the v3 `ActorConfigSchema` Pydantic model. ## Expected Behavior (from spec) Per v3.1.0 milestone acceptance criteria: > "Actor YAML files with `version: "3"`, `type: llm|tool|graph`, and all schema fields parse and validate correctly via Pydantic models" The v3 `ActorConfigSchema` (`src/cleveragents/actor/schema.py`) provides comprehensive validation: - `type: llm|tool|graph` enum validation - `route:` field required for GRAPH actors - `model:` field required for LLM/GRAPH actors - `tools:` required for TOOL actors - Graph topology validation (cycle detection, entry/exit validation, unreachable node detection) - Node ID format validation - Tool name namespace validation - `context_view:` enum validation (strategist/executor/reviewer/full) ## Actual Behavior (from implementation) The `agents actor add` CLI command (`src/cleveragents/cli/commands/actor.py`) uses `_canonicalize_actor_config()` which calls `ActorConfiguration.from_blob()` — the **v2 parser** (`src/cleveragents/actor/config.py`). This parser: 1. Does NOT validate `type: llm|tool|graph` 2. Does NOT validate `route:` structure 3. Does NOT perform cycle detection 4. Does NOT validate `context_view:` values 5. Does NOT validate node IDs or tool namespacing 6. Requires `provider` and `model` fields (v2 format) even for v3 actors The v3 `ActorConfigSchema` is **never invoked** during the `agents actor add` flow. It is only used by `ActorLoader.discover()` (filesystem scanning) and in unit tests. ## Code Location **CLI add command** (`src/cleveragents/cli/commands/actor.py`): ```python # Line 573 — uses v2 ActorConfiguration, not v3 ActorConfigSchema resolved, canonical_blob, requires_confirmation = _canonicalize_actor_config( name=name, config_blob=config_blob, ... ) # Line 621 — bypasses registry.add() which also uses v2 parser actor = registry.upsert_actor( name=name, config_blob=canonical_blob, ... ) ``` **`_canonicalize_actor_config`** (line 348): ```python resolved = ActorConfiguration.from_blob( # v2 parser! blob=config_blob, ... ) ``` **`ActorRegistry.add()`** (line 208) — also uses v2 parser: ```python blob = ActorConfiguration.load_yaml_text(yaml_text) # v2 parser! ``` ## Steps to Reproduce Create a v3 GRAPH actor YAML with a cycle (should be rejected): ```yaml name: local/broken-graph type: graph description: Graph with cycle model: gpt-4 route: nodes: - id: node_a type: agent name: A description: Node A config: {} - id: node_b type: agent name: B description: Node B config: {} edges: - from_node: node_a to_node: node_b - from_node: node_b to_node: node_a # CYCLE! entry_node: node_a exit_nodes: [node_b] ``` Expected: `agents actor add local/broken-graph --config broken.yaml` should fail with cycle detection error. Actual: The actor is registered successfully (cycle not detected because v3 schema is not invoked). ## Impact - Invalid v3 actor YAML files (with cycles, invalid node types, missing required fields) are silently accepted and stored in the registry - The v3 `ActorConfigSchema` Pydantic model is effectively dead code for the CLI registration path - Actors registered via CLI may fail at runtime when the compiler tries to compile them - The v3.1.0 milestone acceptance criterion "Actor YAML files parse and validate correctly via Pydantic models" is not met for the CLI path ## Fix In the `agents actor add` CLI command: 1. Parse the YAML file through `ActorConfigSchema.from_yaml_file()` first for v3 validation 2. If validation passes, proceed with registration 3. Alternatively, update `ActorRegistry.add()` to use `ActorConfigSchema` for v3 YAML files (detected by `type: llm|tool|graph` or `version: "3.0"`) --- ### Metadata | Field | Value | |-------|-------| | Commit Message | `fix(actor): validate v3 YAML via ActorConfigSchema in agents actor add CLI` | | Branch | `fix/actor-add-v3-schema-validation` | | Milestone | v3.1.0 | ### Subtasks - [ ] Add `ActorConfigSchema.from_yaml_file()` call in `agents actor add` CLI command - [ ] Detect v3 YAML files (by `type: llm|tool|graph` or `version: "3.0"`) - [ ] Return clear validation errors from `ActorConfigSchema` to the user - [ ] Update `ActorRegistry.add()` to use v3 schema for v3 YAML files - [ ] Add integration test: registering a GRAPH actor with a cycle should fail ### Definition of Done - `agents actor add --config graph-with-cycle.yaml` fails with cycle detection error - `agents actor add --config llm-without-model.yaml` fails with "LLM actors require 'model' field" - `agents actor add --config tool-without-tools.yaml` fails with "TOOL actors require at least one tool" - All v3 schema validations (node IDs, tool namespacing, context_view enum) are enforced at registration time --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: uat-tester
HAL9000 added this to the v3.1.0 milestone 2026-04-09 11:15:47 +00:00
HAL9000 modified the milestone from v3.1.0 to v3.2.0 2026-04-09 11:37:13 +00:00
Author
Owner

Issue triaged by project owner:

  • State: Verified
  • Priority: High — Actor registration using the v2 parser means v3 actor YAML files with type: graph, route:, context_view: etc. bypass Pydantic v3 schema validation. This is a correctness issue — actors can be registered with invalid v3 YAML without any error. The v3.1.0 acceptance criteria explicitly requires v3 actor YAML to parse and validate correctly.
  • Milestone: v3.1.0 — This is a v3.1.0 acceptance criterion issue.
  • Story Points: 3 — M — Replacing the v2 parser with the v3 ActorConfigSchema in the agents actor add --config command requires careful migration, estimated 4-8 hours.
  • MoSCoW: MoSCoW/Must have — The v3.1.0 milestone acceptance criteria explicitly requires v3 actor YAML validation. Without this, the actor system cannot be considered complete for v3.1.0.
  • Parent Epic: Needs linking to Actor System Epic

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner

Issue triaged by project owner: - **State**: Verified - **Priority**: High — Actor registration using the v2 parser means v3 actor YAML files with `type: graph`, `route:`, `context_view:` etc. bypass Pydantic v3 schema validation. This is a correctness issue — actors can be registered with invalid v3 YAML without any error. The v3.1.0 acceptance criteria explicitly requires v3 actor YAML to parse and validate correctly. - **Milestone**: v3.1.0 — This is a v3.1.0 acceptance criterion issue. - **Story Points**: 3 — M — Replacing the v2 parser with the v3 `ActorConfigSchema` in the `agents actor add --config` command requires careful migration, estimated 4-8 hours. - **MoSCoW**: MoSCoW/Must have — The v3.1.0 milestone acceptance criteria explicitly requires v3 actor YAML validation. Without this, the actor system cannot be considered complete for v3.1.0. - **Parent Epic**: Needs linking to Actor System Epic --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner
Author
Owner

Label compliance fix applied:

  • Added missing labels and/or milestone to bring issue into compliance with CONTRIBUTING.md

Automated by CleverAgents Bot
Supervisor: Backlog Grooming | Agent: backlog-groomer

Label compliance fix applied: - Added missing labels and/or milestone to bring issue into compliance with CONTRIBUTING.md --- **Automated by CleverAgents Bot** Supervisor: Backlog Grooming | Agent: backlog-groomer
HAL9000 modified the milestone from v3.2.0 to v3.1.0 2026-04-09 11:59:35 +00:00
Author
Owner

PR #8636 has been opened to address this issue: fix(actor): validate v3 YAML via ActorConfigSchema in agents actor add CLI

This PR blocks issue #5869 and will close it upon merge.


Automated by CleverAgents Bot
Agent: pr-creator

PR #8636 has been opened to address this issue: [fix(actor): validate v3 YAML via ActorConfigSchema in agents actor add CLI](https://git.cleverthis.com/cleveragents/cleveragents-core/pulls/8636) This PR blocks issue #5869 and will close it upon merge. --- **Automated by CleverAgents Bot** Agent: pr-creator
Author
Owner

Implementation Attempt: Tier 1 - haiku

Status: Success

What Was Done

Implemented v3 YAML schema validation in agents actor add CLI command with comprehensive testing coverage.

Implementation Details

Core Changes:

  • Modified src/cleveragents/cli/commands/actor.py to detect and validate v3 YAML
  • Updated src/cleveragents/actor/registry.py add() and upsert_actor() methods
  • Added helper functions _is_v3_yaml() and _validate_v3_yaml()

V3 Detection Logic:

  • By type: llm, tool, or graph
  • By version: "3.0"
  • Integrated ActorConfigSchema validation

Test Coverage:

  • Created feature file: features/actor_add_v3_schema_validation.feature
  • Created step definitions: features/steps/actor_add_v3_schema_validation_steps.py
  • Created robot tests: robot/actor_add_v3_schema_validation.robot

Documentation:

  • Updated CHANGELOG.md with fix details
  • PR #8636 created and linked to this issue

Automated by CleverAgents Bot
Supervisor: Implementation Pool | Agent: implementation-worker

## Implementation Attempt: Tier 1 - haiku **Status:** ✅ Success ### What Was Done Implemented v3 YAML schema validation in agents actor add CLI command with comprehensive testing coverage. ### Implementation Details **Core Changes:** - Modified `src/cleveragents/cli/commands/actor.py` to detect and validate v3 YAML - Updated `src/cleveragents/actor/registry.py` `add()` and `upsert_actor()` methods - Added helper functions `_is_v3_yaml()` and `_validate_v3_yaml()` **V3 Detection Logic:** - By type: `llm`, `tool`, or `graph` - By version: `"3.0"` - Integrated ActorConfigSchema validation **Test Coverage:** - Created feature file: `features/actor_add_v3_schema_validation.feature` - Created step definitions: `features/steps/actor_add_v3_schema_validation_steps.py` - Created robot tests: `robot/actor_add_v3_schema_validation.robot` **Documentation:** - Updated `CHANGELOG.md` with fix details ### Related PR - PR #8636 created and linked to this issue --- **Automated by CleverAgents Bot** Supervisor: Implementation Pool | Agent: implementation-worker
HAL9000 modified the milestone from v3.1.0 to v3.2.0 2026-04-13 23:36:13 +00:00
Author
Owner

[GROOMED] Reassigned this issue to milestone v3.2.0 (M3) because v3.1.0 is closed. Verified required labels (State/In Review, Type/Bug, Priority/High) were already applied, so no label changes were needed. Updated linked PR #8636 to milestone v3.2.0 to keep the fix aligned with the issue.


Automated by CleverAgents Bot
Supervisor: Grooming | Agent: grooming-pool-supervisor

[GROOMED] Reassigned this issue to milestone v3.2.0 (M3) because v3.1.0 is closed. Verified required labels (State/In Review, Type/Bug, Priority/High) were already applied, so no label changes were needed. Updated linked PR #8636 to milestone v3.2.0 to keep the fix aligned with the issue. --- **Automated by CleverAgents Bot** Supervisor: Grooming | Agent: grooming-pool-supervisor
Member

Implementation Notes — PR Fix Round (commit 37aa421a)

All blocking review comments have been addressed. Here is a summary of what was changed and why:

1. provider field changed to Optional[str] with model validator

Review concern: PR had provider: str = Field(...) (always required), breaking TOOL actors which don't need a provider.

Fix: Changed to provider: str | None = Field(default=None, ...) with a new @model_validator(mode="after") validate_provider_required_for_llm_graph() that raises ValueError only for LLM and GRAPH actor types. TOOL actors pass without a provider.

Location: cleveragents.actor.schema.ActorConfigSchema

2. is_v3_yaml() — version string comparison tightened

Review concern: Using str(version).startswith("3") could match "30" or "300".

Fix: Now uses version_str == "3" or version_str.startswith("3.") — exact match for "3", prefix check for "3." to handle "3.0", "3.0.0", etc.

Location: cleveragents.actor.schema.is_v3_yaml

3. Tool namespace validation — strict 2-part split

Already correct: uses len(parts) != 2 or not parts[0] or not parts[1] to reject "/tool", "ns/", and "a/b/c".

Location: cleveragents.actor.schema.ActorConfigSchema.validate_tools

4. actor.pyraw_version pattern (no # type: ignore)

Review concern: schema_version: str | None = config_blob.get(...) # type: ignore[assignment] used a suppression comment.

Fix: Replaced with raw_version = config_blob.get("schema_version"); schema_version: str | None = str(raw_version) if raw_version is not None else None — clean typing, no suppression.

Location: cleveragents.cli.commands.actor.add

5. is_v3_yaml removed from actor/__init__.py

Review concern: is_v3_yaml was exposed as public API via __all__ and _LAZY_IMPORTS, but it is an internal schema utility.

Fix: Removed from both __all__ and _LAZY_IMPORTS. It remains importable from cleveragents.actor.schema directly for callers that need it.

Location: cleveragents.actor.__init__

6. Robot test fixtures — provider/model fields added

Review concern: Robot fixtures for "Reject v3 LLM Actor Without Model Field" and "Reject v3 TOOL Actor Without Tools Field" were missing provider (and model for TOOL), causing the wrong validation error.

Fix: Added provider: openai to the LLM fixture and provider: openai\nmodel: gpt-4 to the TOOL fixture so the tests exercise the correct validation paths.

Location: robot.actor_add_v3_schema_validation

7. Robot helper — except (TimeoutExpired, FileNotFoundError) unified

Review concern: Only subprocess.TimeoutExpired was caught, not FileNotFoundError (if the binary is missing).

Fix: Both add_actor() and update_actor() now catch (subprocess.TimeoutExpired, FileNotFoundError) with a single handler.

Location: robot.helper_actor_add_v3_schema_validation

8. Feature file — "Update" scenario now asserts schema validation called

Added And the actor should be validated via ActorConfigSchema to the "Update a v3 actor with valid YAML succeeds" scenario.

Error assertion strings tightened to exact Pydantic error messages:

  • "Input should be 'llm', 'tool' or 'graph'" (type enum)
  • "Node ID must be alphanumeric" (node ID)
  • "must be namespaced" (tool name)

Location: features.actor_add_v3_schema_validation

9. Steps file — step_run_actor_update now spies on ActorConfigSchema.model_validate

Added patch.object(ActorConfigSchema, "model_validate", wraps=...) to the update step so schema_validate_spy is set in context and And the actor should be validated via ActorConfigSchema can verify the spy was called.

Failure paths in both step_run_actor_add and step_run_actor_update now assert isinstance(result.exception, SystemExit) to distinguish clean CLI exits from unexpected exceptions.

Location: features.steps.actor_add_v3_schema_validation_steps


Quality Gates

All quality gates pass on commit 37aa421a:

  • nox -s lint ✓ (0 findings)
  • nox -s typecheck ✓ (0 errors, 3 pre-existing warnings about optional providers)
  • nox -s unit_tests ✓ (27 features passed, 550 scenarios passed, 0 failed)

Rui Hu (hurui200320)

## Implementation Notes — PR Fix Round (commit `37aa421a`) All blocking review comments have been addressed. Here is a summary of what was changed and why: ### 1. `provider` field changed to `Optional[str]` with model validator **Review concern**: PR had `provider: str = Field(...)` (always required), breaking TOOL actors which don't need a provider. **Fix**: Changed to `provider: str | None = Field(default=None, ...)` with a new `@model_validator(mode="after")` `validate_provider_required_for_llm_graph()` that raises `ValueError` only for `LLM` and `GRAPH` actor types. TOOL actors pass without a provider. **Location**: `cleveragents.actor.schema.ActorConfigSchema` ### 2. `is_v3_yaml()` — version string comparison tightened **Review concern**: Using `str(version).startswith("3")` could match `"30"` or `"300"`. **Fix**: Now uses `version_str == "3" or version_str.startswith("3.")` — exact match for `"3"`, prefix check for `"3."` to handle `"3.0"`, `"3.0.0"`, etc. **Location**: `cleveragents.actor.schema.is_v3_yaml` ### 3. Tool namespace validation — strict 2-part split Already correct: uses `len(parts) != 2 or not parts[0] or not parts[1]` to reject `"/tool"`, `"ns/"`, and `"a/b/c"`. **Location**: `cleveragents.actor.schema.ActorConfigSchema.validate_tools` ### 4. `actor.py` — `raw_version` pattern (no `# type: ignore`) **Review concern**: `schema_version: str | None = config_blob.get(...) # type: ignore[assignment]` used a suppression comment. **Fix**: Replaced with `raw_version = config_blob.get("schema_version"); schema_version: str | None = str(raw_version) if raw_version is not None else None` — clean typing, no suppression. **Location**: `cleveragents.cli.commands.actor.add` ### 5. `is_v3_yaml` removed from `actor/__init__.py` **Review concern**: `is_v3_yaml` was exposed as public API via `__all__` and `_LAZY_IMPORTS`, but it is an internal schema utility. **Fix**: Removed from both `__all__` and `_LAZY_IMPORTS`. It remains importable from `cleveragents.actor.schema` directly for callers that need it. **Location**: `cleveragents.actor.__init__` ### 6. Robot test fixtures — `provider`/`model` fields added **Review concern**: Robot fixtures for "Reject v3 LLM Actor Without Model Field" and "Reject v3 TOOL Actor Without Tools Field" were missing `provider` (and `model` for TOOL), causing the wrong validation error. **Fix**: Added `provider: openai` to the LLM fixture and `provider: openai\nmodel: gpt-4` to the TOOL fixture so the tests exercise the correct validation paths. **Location**: `robot.actor_add_v3_schema_validation` ### 7. Robot helper — `except (TimeoutExpired, FileNotFoundError)` unified **Review concern**: Only `subprocess.TimeoutExpired` was caught, not `FileNotFoundError` (if the binary is missing). **Fix**: Both `add_actor()` and `update_actor()` now catch `(subprocess.TimeoutExpired, FileNotFoundError)` with a single handler. **Location**: `robot.helper_actor_add_v3_schema_validation` ### 8. Feature file — "Update" scenario now asserts schema validation called Added `And the actor should be validated via ActorConfigSchema` to the "Update a v3 actor with valid YAML succeeds" scenario. Error assertion strings tightened to exact Pydantic error messages: - `"Input should be 'llm', 'tool' or 'graph'"` (type enum) - `"Node ID must be alphanumeric"` (node ID) - `"must be namespaced"` (tool name) **Location**: `features.actor_add_v3_schema_validation` ### 9. Steps file — `step_run_actor_update` now spies on `ActorConfigSchema.model_validate` Added `patch.object(ActorConfigSchema, "model_validate", wraps=...)` to the update step so `schema_validate_spy` is set in context and `And the actor should be validated via ActorConfigSchema` can verify the spy was called. Failure paths in both `step_run_actor_add` and `step_run_actor_update` now assert `isinstance(result.exception, SystemExit)` to distinguish clean CLI exits from unexpected exceptions. **Location**: `features.steps.actor_add_v3_schema_validation_steps` --- ### Quality Gates All quality gates pass on commit `37aa421a`: - `nox -s lint` ✓ (0 findings) - `nox -s typecheck` ✓ (0 errors, 3 pre-existing warnings about optional providers) - `nox -s unit_tests` ✓ (27 features passed, 550 scenarios passed, 0 failed) Rui Hu (hurui200320)
Member

Self-QA Implementation Notes (Cycles 1–4)

Self-QA loop completed on PR #8636. 4 review/fix cycles were run. Final verdict: Approved .


Cycle 1

Review findings: 2C/7M/8m/7n

  • C1: "Detect v3 YAML by version field" scenario fixture had both type: llm AND version: "3.0" — type check fires first, version branch never reached
  • C2: step_actor_not_validated_via_schema had silent if spy is not None: guard — v2 backward-compat test could pass vacuously
  • M1: CHANGELOG said "type: llm|tool|graph" but implementation catches ANY type field
  • M2: registry.add() legacy provider/model check blocked valid v3 TOOL actors without provider
  • M3: New validate_provider_required_for_llm_graph validator had zero test coverage
  • M4: ActorRegistry.add() registry-level defense-in-depth had no direct tests
  • M5: Invalid node ID fixture used id: "" (empty string) not actual non-alphanumeric chars
  • M6: is_v3_yaml() version branch edge cases ("3", "30", 3.0) untested
  • M7 + 8m/7n: Various nit-level issues (type: null guard, docstring, robot assertions, etc.)

Fixes applied (commit 2b6a3257):

  • C1: Changed "version field" scenario fixture to remove type field; added direct is_v3_yaml edge-case scenarios for "3", "30", null type
  • C2: Changed to hard assert — assert spy is not None, "..."; assert not spy.called, "..."
  • M1: CHANGELOG updated to say "ANY non-null type field"
  • M2: Registry legacy check gated with if not is_v3_yaml(blob):
  • M3: Added 3 scenarios — LLM without provider (fail), GRAPH without provider (fail), TOOL without provider (schema-level success)
  • M4: Added direct ActorRegistry.add() cycle-detection scenario bypassing CLI
  • M5: Fixture changed from id: "" to id: "bad!node"
  • M6: Covered by C1 fix (edge-case scenarios added)
  • Various m/n fixes: type: null guard, docstring, robot assertions tightened, 3 new Robot integration tests, if version is None:, robot helper prefix

Cycle 2

Review findings: 0C/2M/10m/7n

  • M1: Robot "cycle via update" test would fail at runtime — never pre-registers the actor before update attempt
  • M2: v3 TOOL actors without provider pass ActorConfigSchema but are rejected by legacy ActorConfiguration.from_blob() — acknowledged limitation

Fixes applied (commit df0d7afa):

  • M1: Added setup step to pre-register local/update-test-graph as valid actor before the cyclic update attempt
  • M2: Added # NOTE: comments at from_blob() call sites in actor.py and registry.py documenting limitation; added Gherkin comment explaining deferred CLI coverage; filed follow-up issue #9971
  • Various m fixes: removed bare except Exception from step; moved inline import pydantic to top-level; removed redundant MagicMock import; removed fragile node-name assertions from cycle test; cached is_v3_yaml() result as blob_is_v3; stale docstrings updated; scenario renamed from misleading name

Cycle 3

Review findings: 0C/6M/7m/6n

  • M1: Bare except Exception in registry.add() could silently swallow database errors (string-matching "already exists" is fragile)
  • M3: step_run_actor_update unconditionally set context.registered_actor_type = None — latent defect
  • M4: No CLI-level test for type: null YAML (only direct is_v3_yaml unit test)
  • M5: No test for update command without --config flag skipping schema validation
  • M6: No CLI-level test for version "3" without type field (hits version branch, then fails at schema for missing type)
  • Various m fixes requested for detect_cycles shared adjacency, str(None) guard, pydantic alias

Fixes applied (commit 991e12c1):

  • M1: Replaced fragile except Exception + string matching with proper try/except NotFoundError: pass / else: raise ValidationError(...)
  • M3: Mirrored mock_registry.upsert_actor.call_args extraction into step_run_actor_update success branch
  • M4: Added CLI scenario v3 actor with null type field falls back to v2 handling
  • M5: Added scenario Update a registered v3 actor without --config flag skips schema validation
  • M6: Added scenario Reject v3 actor YAML with version "3" but no type field
  • m1: Extracted _build_adjacency() as shared private method used by validate_references() (includes route_to targets); detect_cycles() intentionally keeps explicit-edges-only adjacency
  • m2: Added str(provider_raw) if provider_raw else "" guard
  • m3: Reworded error to "route: graph contains a cycle — back-edge detected at node 'X'" (singular)
  • m4: Removed import pydantic as _pydantic alias — changed to plain import pydantic
  • m5/m6: Added scenarios for duplicate node IDs and non-existent node edges
  • n1/n2: Added __all__ comment and validator ordering comment

Remaining Issues (non-blocking, deferred)

  • Branch name: fix/actor-add-v3-schema-validation should be bugfix/m3.2-... per CONTRIBUTING.md. Accepted as-is since renaming at this stage would be disruptive. The issue's Metadata field prescribed the wrong name.
  • TOOL actors without provider: Pass ActorConfigSchema but fail at legacy ActorConfiguration.from_blob(). Tracked in follow-up issue #9971.
  • ToolDefinition.validate_name: Weaker than string tool reference validation (pre-existing inconsistency). Should be aligned in a future cleanup.
  • Spec gap: provider field not documented in docs/specification.md as required for LLM/GRAPH. Should be added in a follow-up spec update.
  • Robot coverage gaps: Newer scenarios (duplicate node IDs, non-existent edges, provider rejection) have Behave coverage but no Robot integration-level equivalents.

Generated by Self-QA agent — 4 review/fix cycles on PR #8636.

## Self-QA Implementation Notes (Cycles 1–4) Self-QA loop completed on PR #8636. 4 review/fix cycles were run. Final verdict: **Approved** ✅. --- ### Cycle 1 **Review findings:** 2C/7M/8m/7n - **C1**: "Detect v3 YAML by version field" scenario fixture had both `type: llm` AND `version: "3.0"` — type check fires first, version branch never reached - **C2**: `step_actor_not_validated_via_schema` had silent `if spy is not None:` guard — v2 backward-compat test could pass vacuously - **M1**: CHANGELOG said "type: llm|tool|graph" but implementation catches ANY `type` field - **M2**: `registry.add()` legacy `provider`/`model` check blocked valid v3 TOOL actors without provider - **M3**: New `validate_provider_required_for_llm_graph` validator had zero test coverage - **M4**: `ActorRegistry.add()` registry-level defense-in-depth had no direct tests - **M5**: Invalid node ID fixture used `id: ""` (empty string) not actual non-alphanumeric chars - **M6**: `is_v3_yaml()` version branch edge cases (`"3"`, `"30"`, `3.0`) untested - M7 + 8m/7n: Various nit-level issues (type: null guard, docstring, robot assertions, etc.) **Fixes applied (commit `2b6a3257`):** - C1: Changed "version field" scenario fixture to remove `type` field; added direct is_v3_yaml edge-case scenarios for `"3"`, `"30"`, null type - C2: Changed to hard assert — `assert spy is not None, "..."`; `assert not spy.called, "..."` - M1: CHANGELOG updated to say "ANY non-null `type` field" - M2: Registry legacy check gated with `if not is_v3_yaml(blob):` - M3: Added 3 scenarios — LLM without provider (fail), GRAPH without provider (fail), TOOL without provider (schema-level success) - M4: Added direct `ActorRegistry.add()` cycle-detection scenario bypassing CLI - M5: Fixture changed from `id: ""` to `id: "bad!node"` - M6: Covered by C1 fix (edge-case scenarios added) - Various m/n fixes: `type: null` guard, docstring, robot assertions tightened, 3 new Robot integration tests, `if version is None:`, robot helper prefix --- ### Cycle 2 **Review findings:** 0C/2M/10m/7n - **M1**: Robot "cycle via update" test would fail at runtime — never pre-registers the actor before update attempt - **M2**: v3 TOOL actors without `provider` pass `ActorConfigSchema` but are rejected by legacy `ActorConfiguration.from_blob()` — acknowledged limitation **Fixes applied (commit `df0d7afa`):** - M1: Added setup step to pre-register `local/update-test-graph` as valid actor before the cyclic update attempt - M2: Added `# NOTE:` comments at `from_blob()` call sites in `actor.py` and `registry.py` documenting limitation; added Gherkin comment explaining deferred CLI coverage; filed follow-up issue **#9971** - Various m fixes: removed bare `except Exception` from step; moved inline `import pydantic` to top-level; removed redundant `MagicMock` import; removed fragile node-name assertions from cycle test; cached `is_v3_yaml()` result as `blob_is_v3`; stale docstrings updated; scenario renamed from misleading name --- ### Cycle 3 **Review findings:** 0C/6M/7m/6n - **M1**: Bare `except Exception` in `registry.add()` could silently swallow database errors (string-matching `"already exists"` is fragile) - **M3**: `step_run_actor_update` unconditionally set `context.registered_actor_type = None` — latent defect - **M4**: No CLI-level test for `type: null` YAML (only direct is_v3_yaml unit test) - **M5**: No test for `update` command without `--config` flag skipping schema validation - **M6**: No CLI-level test for version "3" without type field (hits version branch, then fails at schema for missing `type`) - Various m fixes requested for detect_cycles shared adjacency, `str(None)` guard, pydantic alias **Fixes applied (commit `991e12c1`):** - M1: Replaced fragile `except Exception` + string matching with proper `try/except NotFoundError: pass / else: raise ValidationError(...)` - M3: Mirrored `mock_registry.upsert_actor.call_args` extraction into `step_run_actor_update` success branch - M4: Added CLI scenario `v3 actor with null type field falls back to v2 handling` - M5: Added scenario `Update a registered v3 actor without --config flag skips schema validation` - M6: Added scenario `Reject v3 actor YAML with version "3" but no type field` - m1: Extracted `_build_adjacency()` as shared private method used by `validate_references()` (includes `route_to` targets); `detect_cycles()` intentionally keeps explicit-edges-only adjacency - m2: Added `str(provider_raw) if provider_raw else ""` guard - m3: Reworded error to `"route: graph contains a cycle — back-edge detected at node 'X'"` (singular) - m4: Removed `import pydantic as _pydantic` alias — changed to plain `import pydantic` - m5/m6: Added scenarios for duplicate node IDs and non-existent node edges - n1/n2: Added `__all__` comment and validator ordering comment --- ### Remaining Issues (non-blocking, deferred) - **Branch name**: `fix/actor-add-v3-schema-validation` should be `bugfix/m3.2-...` per CONTRIBUTING.md. Accepted as-is since renaming at this stage would be disruptive. The issue's Metadata field prescribed the wrong name. - **TOOL actors without provider**: Pass `ActorConfigSchema` but fail at legacy `ActorConfiguration.from_blob()`. Tracked in follow-up issue **#9971**. - **ToolDefinition.validate_name**: Weaker than string tool reference validation (pre-existing inconsistency). Should be aligned in a future cleanup. - **Spec gap**: `provider` field not documented in `docs/specification.md` as required for LLM/GRAPH. Should be added in a follow-up spec update. - **Robot coverage gaps**: Newer scenarios (duplicate node IDs, non-existent edges, provider rejection) have Behave coverage but no Robot integration-level equivalents. --- *Generated by Self-QA agent — 4 review/fix cycles on PR #8636.*
hurui200320 2026-04-17 08:58:35 +00:00
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.

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