cleveragents/cleveractors-core
0
0
Fork 0
Code Issues 17 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

feat(validate_dict): expose public validate_dict(config_dict, platform_limits) API #10

New Issue
Closed
opened 2026-06-03 05:58:41 +00:00 by hurui200320 · 8 comments
hurui200320 commented 2026-06-03 05:58:41 +00:00
Member
Copy Link
Copy Source

Background

cleveractors-core has no public validation function. The internal SchemaValidator.validate() only checks for the presence of top-level keys (agents, routes, default_router). It does not validate agent types, provider values, route structure, or enforce platform-level structural limits.

The CleverThis router must validate actor YAML at upload time — before storing it in the database — using a self-contained library call. The function must be callable without reading files, touching environment variables, or constructing ReactiveCleverAgentsApp.

Spec references: ADR-2024 (Router-Facing API), ADR-2025 (Format Validation), ADR-2029 (Upload-time Validation)

What Is Currently Missing

  • No validate_dict function exists anywhere in the package.
  • SchemaValidator.validate() only checks for top-level key presence — no structural or semantic validation.
  • No provider allowlist validation (caller-configurable via platform_limits["allowed_providers"]).
  • No route/edge field name validation.
  • No structural limit enforcement (max_graph_depth, max_subgraph_depth, max_total_nodes).

Acceptance Criteria

Implement validate_dict(config_dict: dict[str, Any], platform_limits: dict[str, Any]) -> dict[str, Any] and export it from cleveractors/__init__.py:

  1. Raises ConfigurationError if agents or routes keys are absent from config_dict.
  2. Validates all agent entries: type must be one of llm, tool, composite, chain, template_instance; unknown types raise ConfigurationError.
  3. Validates LLM agent provider values: if platform_limits["allowed_providers"] is present, the declared provider must be in that list; if absent, defaults to {"openai", "anthropic", "google"}. Unknown providers raise ConfigurationError.
  4. Validates route definitions: edge field names must use source/target; node and operator types must be within the spec-defined vocabulary.
  5. Enforces structural limits when present in platform_limits: max_graph_depth, max_subgraph_depth, max_total_nodes.
  6. Returns the input dict unchanged when valid (no transformation, no side effects).
  7. Callable without reading files, env vars, or constructing any app object.
  8. Exported from cleveractors/__init__.py and listed in __all__.

Subtasks

  • Implement validate_dict(config_dict, platform_limits) in cleveractors/validation.py (or equivalent module)
  • Add agent type validation
  • Add LLM provider validation using platform_limits["allowed_providers"] (default {openai, anthropic, google} if key absent)
  • Add route/edge structure validation (source/target fields, node types, operator types)
  • Add structural limit enforcement from platform_limits
  • Export from cleveractors/__init__.py and __all__
  • Write BDD/unit tests covering valid input, each error path, and platform_limits variants
  • Verify project coverage threshold is maintained

Definition of Done

  • All subtasks checked off.
  • from cleveractors import validate_dict works without error.
  • All existing and new tests pass.
  • Coverage at or above project threshold.
## Background `cleveractors-core` has no public validation function. The internal `SchemaValidator.validate()` only checks for the presence of top-level keys (`agents`, `routes`, `default_router`). It does not validate agent types, provider values, route structure, or enforce platform-level structural limits. The CleverThis router must validate actor YAML at upload time — before storing it in the database — using a self-contained library call. The function must be callable without reading files, touching environment variables, or constructing `ReactiveCleverAgentsApp`. **Spec references:** ADR-2024 (Router-Facing API), ADR-2025 (Format Validation), ADR-2029 (Upload-time Validation) ## What Is Currently Missing - No `validate_dict` function exists anywhere in the package. - `SchemaValidator.validate()` only checks for top-level key presence — no structural or semantic validation. - No provider allowlist validation (caller-configurable via `platform_limits["allowed_providers"]`). - No route/edge field name validation. - No structural limit enforcement (`max_graph_depth`, `max_subgraph_depth`, `max_total_nodes`). ## Acceptance Criteria Implement `validate_dict(config_dict: dict[str, Any], platform_limits: dict[str, Any]) -> dict[str, Any]` and export it from `cleveractors/__init__.py`: 1. Raises `ConfigurationError` if `agents` or `routes` keys are absent from `config_dict`. 2. Validates all agent entries: type must be one of `llm`, `tool`, `composite`, `chain`, `template_instance`; unknown types raise `ConfigurationError`. 3. Validates LLM agent `provider` values: if `platform_limits["allowed_providers"]` is present, the declared provider must be in that list; if absent, defaults to `{"openai", "anthropic", "google"}`. Unknown providers raise `ConfigurationError`. 4. Validates route definitions: edge field names must use `source`/`target`; node and operator types must be within the spec-defined vocabulary. 5. Enforces structural limits when present in `platform_limits`: `max_graph_depth`, `max_subgraph_depth`, `max_total_nodes`. 6. Returns the input dict unchanged when valid (no transformation, no side effects). 7. Callable without reading files, env vars, or constructing any app object. 8. Exported from `cleveractors/__init__.py` and listed in `__all__`. ## Subtasks - [x] Implement `validate_dict(config_dict, platform_limits)` in `cleveractors/validation.py` (or equivalent module) - [x] Add agent type validation - [x] Add LLM `provider` validation using `platform_limits["allowed_providers"]` (default `{openai, anthropic, google}` if key absent) - [x] Add route/edge structure validation (`source`/`target` fields, node types, operator types) - [x] Add structural limit enforcement from `platform_limits` - [x] Export from `cleveractors/__init__.py` and `__all__` - [x] Write BDD/unit tests covering valid input, each error path, and `platform_limits` variants - [x] Verify project coverage threshold is maintained ## Definition of Done - All subtasks checked off. - `from cleveractors import validate_dict` works without error. - All existing and new tests pass. - Coverage at or above project threshold.
hurui200320 added the
Points
8
Priority
High
State
Unverified
Type
Feature
 labels 2026-06-03 06:00:55 +00:00
hurui200320 added
State
Verified
 and removed
State
Unverified
 labels 2026-06-03 06:34:11 +00:00
hurui200320 self-assigned this 2026-06-03 06:37:52 +00:00
hurui200320 added
State
In Progress
 and removed
State
Verified
 labels 2026-06-03 06:48:00 +00:00
hurui200320 commented 2026-06-03 07:09:35 +00:00
Author
Member
Copy Link
Copy Source

Implementation Notes — Pre-Implementation Analysis

Branch and Commit Message

Since the issue body has no Metadata section, I derived these from context:

  • Branch: feature/validate-dict-api
  • Commit Message: feat(validate_dict): expose public validate_dict(config_dict, platform_limits) API (from issue title)

Reference

  • Epic cleveragents/cleveragents-webapp#275
  • Design docs: repo=cleveragents/cleveragents-webapp, branch=develop, path=docs/
  • ADR docs: repo=cleveragents/cleveragents-webapp, branch=develop, path=docs/adr/

Architecture Context (from ADR-2024, ADR-2025, ADR-2029)

The validate_dict() function is the first of four router-facing APIs established by ADR-2024. The router calls it at actor-upload time to validate the spec-conformant Actor Configuration Standard format before storing the dict as JSON.

ADR-2025 confirms that only the spec-conformant format is accepted (not the legacy flat format). Key field differences to enforce:

  • Edges use source/target (not from/to)
  • Graph entry is entry_point: scalar (not entry_node:)
  • Nodes are mapping-keyed (not list with id: fields)
  • Both agents and routes top-level sections are required

ADR-2029 defines the structural limits passed via platform_limits:

Key MVP constant
max_graph_depth 5 (binding platform constraint)
max_subgraph_depth 3
max_total_nodes 50

The library's own internal ceiling for each is very large (implementation-defined). The platform constants passed by the router are the binding limits.

Spec-Conformant Vocabulary (from actor-standard.md v1.0.0)

Agent types (§4.3):

  • llm, tool, composite, chain, template_instance

Default allowed providers (§4.4.1):

  • openai, anthropic, google

Route types (§5.1):

  • stream, graph, bridge

Node types (§6.2) — case-insensitive:

  • start, end, agent, function, tool, conditional, subgraph, message_router

Graph edge fields: source, target — edges using from/to must be rejected.

Operator types (§5.3.2):

  • map, filter, transform, debounce, throttle, delay, buffer, scan, reduce, switch, conditional_route, catch, retry, distinct, take, skip, sample, graph_execute, state_update, state_checkpoint, graph_node

Implementation Design

Module: cleveractors/validation.py (new file)

Function signature:

def validate_dict(
    config_dict: dict[str, Any],
    platform_limits: dict[str, Any],
) -> dict[str, Any]:
    ...

Validation order:

  1. Top-level key presence (agents, routes)
  2. Agent type validation for each agent entry
  3. LLM provider validation using platform_limits["allowed_providers"] (default {"openai", "anthropic", "google"})
  4. Route validation: edge source/target field names, node types, operator types
  5. Structural limit enforcement from platform_limits (max_graph_depth, max_subgraph_depth, max_total_nodes)
  6. Return config_dict unchanged

Graph depth calculation: DFS from entry_point, tracking visited nodes to avoid cycles, finding the longest path to end node.

No side effects: The function does NOT read files, env vars, or instantiate any app objects. It is a pure static validator.

Testing Plan

BDD (Behave) scenarios in features/validate_dict.feature:

  • Valid minimal config → returns dict unchanged
  • Missing agents key → ConfigurationError
  • Missing routes key → ConfigurationError
  • Unknown agent type → ConfigurationError
  • Valid LLM provider in custom allowlist
  • Invalid LLM provider not in custom allowlist → ConfigurationError
  • Valid LLM provider in default allowlist (openai/anthropic/google)
  • Invalid provider with no allowed_providers in platform_limits → ConfigurationError
  • Edge with source/target → valid
  • Edge with from/to → ConfigurationError
  • Invalid node type → ConfigurationError
  • Invalid operator type → ConfigurationError
  • Graph exceeds max_graph_depth → ConfigurationError
  • Graph exceeds max_total_nodes → ConfigurationError
  • Subgraph exceeds max_subgraph_depth → ConfigurationError

Robot Framework integration tests in robot/ — verify the public API is importable and callable.

## Implementation Notes — Pre-Implementation Analysis ### Branch and Commit Message Since the issue body has no Metadata section, I derived these from context: - **Branch**: `feature/validate-dict-api` - **Commit Message**: `feat(validate_dict): expose public validate_dict(config_dict, platform_limits) API` (from issue title) ### Reference + Epic cleveragents/cleveragents-webapp#275 + Design docs: repo=cleveragents/cleveragents-webapp, branch=develop, path=docs/ + ADR docs: repo=cleveragents/cleveragents-webapp, branch=develop, path=docs/adr/ ### Architecture Context (from ADR-2024, ADR-2025, ADR-2029) The `validate_dict()` function is the first of four router-facing APIs established by ADR-2024. The router calls it at actor-upload time to validate the spec-conformant Actor Configuration Standard format before storing the dict as JSON. ADR-2025 confirms that **only the spec-conformant format** is accepted (not the legacy flat format). Key field differences to enforce: - Edges use `source`/`target` (not `from`/`to`) - Graph entry is `entry_point:` scalar (not `entry_node:`) - Nodes are mapping-keyed (not list with `id:` fields) - Both `agents` and `routes` top-level sections are required ADR-2029 defines the structural limits passed via `platform_limits`: | Key | MVP constant | |-----|-------------| | `max_graph_depth` | 5 (binding platform constraint) | | `max_subgraph_depth` | 3 | | `max_total_nodes` | 50 | The library's own internal ceiling for each is very large (implementation-defined). The platform constants passed by the router are the binding limits. ### Spec-Conformant Vocabulary (from actor-standard.md v1.0.0) **Agent types** (§4.3): - `llm`, `tool`, `composite`, `chain`, `template_instance` **Default allowed providers** (§4.4.1): - `openai`, `anthropic`, `google` **Route types** (§5.1): - `stream`, `graph`, `bridge` **Node types** (§6.2) — case-insensitive: - `start`, `end`, `agent`, `function`, `tool`, `conditional`, `subgraph`, `message_router` **Graph edge fields**: `source`, `target` — edges using `from`/`to` must be rejected. **Operator types** (§5.3.2): - `map`, `filter`, `transform`, `debounce`, `throttle`, `delay`, `buffer`, `scan`, `reduce`, `switch`, `conditional_route`, `catch`, `retry`, `distinct`, `take`, `skip`, `sample`, `graph_execute`, `state_update`, `state_checkpoint`, `graph_node` ### Implementation Design **Module**: `cleveractors/validation.py` (new file) **Function signature**: ```python def validate_dict( config_dict: dict[str, Any], platform_limits: dict[str, Any], ) -> dict[str, Any]: ... ``` **Validation order**: 1. Top-level key presence (`agents`, `routes`) 2. Agent type validation for each agent entry 3. LLM provider validation using `platform_limits["allowed_providers"]` (default `{"openai", "anthropic", "google"}`) 4. Route validation: edge `source`/`target` field names, node types, operator types 5. Structural limit enforcement from `platform_limits` (`max_graph_depth`, `max_subgraph_depth`, `max_total_nodes`) 6. Return `config_dict` unchanged **Graph depth calculation**: DFS from `entry_point`, tracking visited nodes to avoid cycles, finding the longest path to `end` node. **No side effects**: The function does NOT read files, env vars, or instantiate any app objects. It is a pure static validator. ### Testing Plan BDD (Behave) scenarios in `features/validate_dict.feature`: - Valid minimal config → returns dict unchanged - Missing `agents` key → `ConfigurationError` - Missing `routes` key → `ConfigurationError` - Unknown agent type → `ConfigurationError` - Valid LLM provider in custom allowlist - Invalid LLM provider not in custom allowlist → `ConfigurationError` - Valid LLM provider in default allowlist (`openai`/`anthropic`/`google`) - Invalid provider with no `allowed_providers` in platform_limits → `ConfigurationError` - Edge with `source`/`target` → valid - Edge with `from`/`to` → `ConfigurationError` - Invalid node type → `ConfigurationError` - Invalid operator type → `ConfigurationError` - Graph exceeds `max_graph_depth` → `ConfigurationError` - Graph exceeds `max_total_nodes` → `ConfigurationError` - Subgraph exceeds `max_subgraph_depth` → `ConfigurationError` Robot Framework integration tests in `robot/` — verify the public API is importable and callable.
hurui200320 commented 2026-06-03 08:27:24 +00:00
Author
Member
Copy Link
Copy Source

Implementation Results

What was implemented

cleveractors/validation.py (new module, 100% covered):

  • validate_dict(config_dict, platform_limits) — the public entry point
  • _validate_top_level_keys() — checks for required agents and routes keys
  • _validate_agents() — validates each agent's type; delegates to _validate_llm_provider() for llm agents
  • _validate_llm_provider() — checks provider against allowlist (platform-configurable or default {openai, anthropic, google})
  • _validate_routes() — validates route types; delegates to _validate_graph_route() and _validate_stream_route()
  • _validate_graph_route() — rejects legacy from/to edge fields; validates node types
  • _validate_stream_route() — validates operator types against the 20 spec-defined operator names
  • _validate_structural_limits() — enforces max_graph_depth, max_subgraph_depth, max_total_nodes
  • _compute_graph_depth() — DFS longest-path traversal with cycle guard (frozenset visited set)
  • _compute_subgraph_depth() — recursive subgraph-reference depth with cycle guard

cleveractors/__init__.py (updated):

  • Added from cleveractors.validation import validate_dict
  • Added "validate_dict" to __all__

robot/CleverActorsLib.py (updated):

  • Added 4 keyword methods: validate_dict_succeeds, validate_dict_raises_configuration_error, validate_dict_returns_same_object, validate_dict_is_exported

Test results

Suite Count Result
BDD unit tests (features/validate_dict.feature) 56 scenarios ✅ All pass
Robot integration tests (robot/validate_dict.robot) 12 tests ✅ All pass
Total project unit tests 1717 scenarios ✅ All pass
Total project integration tests 61 tests ✅ All pass

Quality gates

Gate Result
nox -s lint ✅ Pass
nox -s typecheck ✅ Pass (0 errors, 1 pre-existing warning)
nox -s unit_tests ✅ Pass
nox -s integration_tests ✅ Pass
nox -s coverage_report ✅ COVERAGE OK: 97.0% (threshold: 96.5%)
validation.py coverage ✅ 100%

Design decisions

  1. No side effects: validate_dict is a pure static validator. It does not read files, touch env vars, or construct any app object. The function can be imported and called from the router in isolation.

  2. Identity return contract: The function returns config_dict itself (the same object, not a copy) when valid. This matches the ADR-2024 contract and allows the router to chain calls without extra dict copies.

  3. Removed dead defensive guards: Two guards (if not isinstance(route_def, dict): continue in _validate_structural_limits and _count_total_nodes) were removed because _validate_routes() already ensures all route values are dicts before structural limits are checked. Removing them improves code clarity and coverage.

  4. Provider comparison is case-insensitive: provider.lower() is compared against lowercase allowlist entries, following the spec's §4.4.1 note that "provider name comparison is case-insensitive."

  5. LLM agent with agent_template/template key: Agents using template references may omit the type field. These are silently skipped for type validation (treated as template_instance implicitly), consistent with §4.8.

  6. Cyclic graph handling: _compute_graph_depth uses a frozenset visited set during DFS to avoid infinite recursion on cyclic edge graphs. Cycles are broken by returning 0 when a node is revisited.

Branch and commit

  • Branch: feature/validate-dict-api
  • Commit: b9f61ba — feat(validate_dict): expose public validate_dict(config_dict, platform_limits) API
## Implementation Results ### What was implemented **`cleveractors/validation.py`** (new module, 100% covered): - `validate_dict(config_dict, platform_limits)` — the public entry point - `_validate_top_level_keys()` — checks for required `agents` and `routes` keys - `_validate_agents()` — validates each agent's type; delegates to `_validate_llm_provider()` for `llm` agents - `_validate_llm_provider()` — checks provider against allowlist (platform-configurable or default `{openai, anthropic, google}`) - `_validate_routes()` — validates route types; delegates to `_validate_graph_route()` and `_validate_stream_route()` - `_validate_graph_route()` — rejects legacy `from`/`to` edge fields; validates node types - `_validate_stream_route()` — validates operator types against the 20 spec-defined operator names - `_validate_structural_limits()` — enforces `max_graph_depth`, `max_subgraph_depth`, `max_total_nodes` - `_compute_graph_depth()` — DFS longest-path traversal with cycle guard (frozenset visited set) - `_compute_subgraph_depth()` — recursive subgraph-reference depth with cycle guard **`cleveractors/__init__.py`** (updated): - Added `from cleveractors.validation import validate_dict` - Added `"validate_dict"` to `__all__` **`robot/CleverActorsLib.py`** (updated): - Added 4 keyword methods: `validate_dict_succeeds`, `validate_dict_raises_configuration_error`, `validate_dict_returns_same_object`, `validate_dict_is_exported` ### Test results | Suite | Count | Result | |-------|-------|--------| | BDD unit tests (`features/validate_dict.feature`) | 56 scenarios | ✅ All pass | | Robot integration tests (`robot/validate_dict.robot`) | 12 tests | ✅ All pass | | Total project unit tests | 1717 scenarios | ✅ All pass | | Total project integration tests | 61 tests | ✅ All pass | ### Quality gates | Gate | Result | |------|--------| | `nox -s lint` | ✅ Pass | | `nox -s typecheck` | ✅ Pass (0 errors, 1 pre-existing warning) | | `nox -s unit_tests` | ✅ Pass | | `nox -s integration_tests` | ✅ Pass | | `nox -s coverage_report` | ✅ COVERAGE OK: 97.0% (threshold: 96.5%) | | `validation.py` coverage | ✅ 100% | ### Design decisions 1. **No side effects**: `validate_dict` is a pure static validator. It does not read files, touch env vars, or construct any app object. The function can be imported and called from the router in isolation. 2. **Identity return contract**: The function returns `config_dict` itself (the same object, not a copy) when valid. This matches the ADR-2024 contract and allows the router to chain calls without extra dict copies. 3. **Removed dead defensive guards**: Two guards (`if not isinstance(route_def, dict): continue` in `_validate_structural_limits` and `_count_total_nodes`) were removed because `_validate_routes()` already ensures all route values are dicts before structural limits are checked. Removing them improves code clarity and coverage. 4. **Provider comparison is case-insensitive**: `provider.lower()` is compared against lowercase allowlist entries, following the spec's §4.4.1 note that "provider name comparison is case-insensitive." 5. **LLM agent with `agent_template`/`template` key**: Agents using template references may omit the `type` field. These are silently skipped for type validation (treated as `template_instance` implicitly), consistent with §4.8. 6. **Cyclic graph handling**: `_compute_graph_depth` uses a `frozenset` visited set during DFS to avoid infinite recursion on cyclic edge graphs. Cycles are broken by returning 0 when a node is revisited. ### Branch and commit - **Branch**: `feature/validate-dict-api` - **Commit**: `b9f61ba` — `feat(validate_dict): expose public validate_dict(config_dict, platform_limits) API`
hurui200320 added
State
In Review
 and removed
State
In Progress
 labels 2026-06-03 08:28:11 +00:00
hurui200320 commented 2026-06-03 15:10:28 +00:00
Author
Member
Copy Link
Copy Source

Self-QA Implementation Notes (Cycles 1–5)

Cycle 1

Review findings (0C/5M/4m/5n):

  • 5 crash bugs: non-string node type → AttributeError; non-string provider → AttributeError; bare-string allowed_providers → silently wrong allowlist; non-numeric platform_limits values → TypeError; DFS overcounts depth for cyclic graphs (off-by-one)
  • 9 test quality issues: 4 weak error message assertions (agent type, node type, operator type, total nodes); missing agent_template key test; missing bridge route test; missing case-insensitive comparison tests; missing boundary-exact structural limit tests; missing Robot max_subgraph_depth test
  • 5 minor issues: missing required edge field validation; missing graph route structural validation; disconnected entry_point silently returns depth 0; bridge routes receive no content validation; inconsistent operator type handling
  • 5 nits: misleading step naming, import copy placement, docstring line number reference, dead code, PR description count

Fixes applied:

  • Added isinstance(node_type, str) guard in _validate_graph_route
  • Added isinstance(provider, str) guard in _validate_llm_provider
  • Added sequence+element type validation for allowed_providers
  • Added _validate_limit_type helper rejecting non-int limit values
  • Fixed DFS cycle guard: filter out already-visited neighbours before recursing
  • Tightened 4 weak assertions to specific substrings ("unknown agent type", "unknown node type", "unknown operator type", "max_total_nodes")
  • Added BDD scenarios for agent_template, bridge route, case-insensitive comparisons, boundary-exact limits
  • Added Robot max_subgraph_depth test
  • Added source/target presence checks; added nodes/edges presence checks; added entry_point existence check; added comment for bridge routes; changed operator type to fail-fast

Cycle 2

Review findings (0C/9M/4m/3n):

  • 4 crash bugs: agent_type as list/dict → TypeError: unhashable type; route_type as list/dict → TypeError; entry_point as list → TypeError during structural limit enforcement; allowed_providers as dict → silently iterates over keys
  • 9 test quality issues: missing tests for non-string agent/route types, bool platform_limits, dict allowed_providers; loose assertions for depth, subgraph depth, entry_point; boundary-exact max_subgraph_depth test used wrong limit value; # type: ignore suppressions violating CONTRIBUTING.md
  • 4 minor issues: unbounded recursion guard; loose edge field assertion; step_error_mentions_provider too loose
  • 3 nits: coverage threshold discrepancy, tautological scenarios, provider assertion

Fixes applied:

  • Added isinstance(agent_type, str) guard in _validate_agents
  • Added isinstance(route_type, str) guard in _validate_routes
  • Added isinstance(entry_point, str) guard in _compute_graph_depth
  • Replaced allowed_providers guard with explicit isinstance(raw_allowed, (list, tuple, set, frozenset)) check
  • Added 5 new BDD scenarios (non-string agent type, non-string route type, non-string entry_point, dict allowed_providers, bool max_graph_depth)
  • Tightened assertions: "max_graph_depth", "max_subgraph_depth", "entry_point" in messages
  • Fixed boundary-exact max_subgraph_depth test to use max_subgraph_depth=3 (not 4)
  • Removed all # type: ignore suppressions; replaced bare dict annotations with dict[str, Any]; configured pyright to not check behave imports
  • Added _MAX_DEPTH_GUARD = 1000 with depth_guard parameter to both DFS functions
  • Tightened edge field assertion; tightened provider assertion

Cycle 3

Review findings (3C/3M/7m/5n):

  • 3 critical crash bugs: _MAX_DEPTH_GUARD=1000 equals Python's recursion limit → RecursionError at ~498 edges; subgraph field as list → TypeError in _compute_subgraph_depth; edge source as list → TypeError in _compute_graph_depth
  • 3 major issues: pretty.output artifact committed (672 KB); entry_point not validated in _validate_graph_route; empty subgraph reference silently accepted
  • 7 minor issues: non-string source/target silently dropped; non-dict nodes/edges silently accepted; docstring mismatch; missing non-string stream operator test; missing non-string allowed_providers element test; missing invalid types for max_total_nodes/max_subgraph_depth; tautological scenarios
  • 5 nits: missing route type field message; weak assertions in regression scenarios; entry_point assertion too permissive; pretty.output not in .gitignore; redundant : Any annotations

Fixes applied:

  • Reduced _MAX_DEPTH_GUARD to 500
  • Added isinstance(sub_ref, str) guard in _compute_subgraph_depth before routes.get(sub_ref)
  • Added isinstance(source, str) and isinstance(target, str) type checks in _validate_graph_route
  • Removed pretty.output via git rm; added to .gitignore
  • Added entry_point validation in _validate_graph_route (non-empty string, must match declared node)
  • Added empty subgraph reference check in _validate_graph_route
  • Changed silent-skip guards for nodes/edges to raise ConfigurationError
  • Fixed docstring: "for any single graph route" (not "across all graph routes")
  • Added 17 new BDD scenarios; removed 2 tautological scenarios
  • Strengthened step_error_mentions_entry_point to require both "entry_point" and "does not match any declared node"
  • Removed redundant : Any annotations

Cycle 4

Review findings (0C/2M/7m/7n):

  • 2 major issues: _MAX_DEPTH_GUARD=500 still unreachable (Python recursion limit fires first at ~498 edges); empty-string source/target/entry_point pass validation silently
  • 7 minor issues: non-dict node values silently skipped but counted by _count_total_nodes; subgraph references to non-graph routes silently return depth 0; unreachable depth-guard branches; missing zero-value boundary tests; missing empty-collection tests; unused import sys; _compute_graph_depth defaults entry_point to "start"
  • 7 nits: duplicate step definitions; redundant "And no exception is raised"; weak-assertion duplicate scenarios; redundant defensive entry_point check; duplicate route_def.get("nodes") lookups; negative platform limits not rejected; noqa: BLE001 comments

Fixes applied:

  • Converted _compute_graph_depth and _compute_subgraph_depth to iterative DFS using explicit stacks; removed _MAX_DEPTH_GUARD entirely
  • Added not entry_point, not src_val, not tgt_val checks for empty-string rejection
  • Changed non-dict node values from continue to raise ConfigurationError
  • Added route type check in _compute_subgraph_depth (subgraph must reference a graph route)
  • Added zero-value boundary tests; added empty-collection tests
  • Removed import sys from CleverActorsLib.py
  • Removed entry_point default "start" from _compute_graph_depth; replaced with assert
  • Refactored duplicate step definitions to shared helpers; removed redundant steps; removed weak-assertion duplicates; consolidated nodes lookup; added value >= 0 check in _validate_limit_type; extracted _call_and_capture helper to eliminate noqa comments

Cycle 5

Review findings (0C/2M/5m/6n):

  • 2 major issues: validate_dict(None, {}) and validate_dict({}, None) raise TypeError/AttributeError instead of ConfigurationError; iterative DFS for longest-path is exponential in graph density (12-node complete graph hangs indefinitely) — practical DoS vector for router-facing API
  • 5 minor issues: assert statements in depth functions raise AssertionError not ConfigurationError; O(N²) frozenset allocations; non-dict edges/operators silently skipped; graph node missing type key produces confusing error; ConfigurationError not exported from __init__.py
  • 6 nits: misleading scenario names for unreachable code paths; nodes_raw assigned before presence check; is None check unreachable; non-specific step assertions; file size guideline violations; British spelling in docstring

Remaining Issues (after Cycle 5 — not yet fixed)

  • M1: validate_dict(None, {}) crashes with TypeError instead of ConfigurationError — needs isinstance(config_dict, dict) guard at function entry
  • M2: Exponential-time DFS for dense graphs — needs step-counter bailout (e.g., MAX_DFS_STEPS = 1_000_000)
  • m1: assert statements in _compute_graph_depth/_compute_subgraph_depth raise AssertionError not ConfigurationError
  • m2: O(N²) frozenset allocations in iterative DFS
  • m3: Non-dict edges and operators silently skipped (inconsistent with non-dict node fix)
  • m4: Missing type key in graph node produces confusing "unknown node type ''" error
  • m5: ConfigurationError not exported from __init__.py
  • Various nits (scenario naming, step assertion specificity, file size, spelling)
## Self-QA Implementation Notes (Cycles 1–5) ### Cycle 1 **Review findings (0C/5M/4m/5n):** - 5 crash bugs: non-string node `type` → `AttributeError`; non-string `provider` → `AttributeError`; bare-string `allowed_providers` → silently wrong allowlist; non-numeric `platform_limits` values → `TypeError`; DFS overcounts depth for cyclic graphs (off-by-one) - 9 test quality issues: 4 weak error message assertions (agent type, node type, operator type, total nodes); missing `agent_template` key test; missing `bridge` route test; missing case-insensitive comparison tests; missing boundary-exact structural limit tests; missing Robot `max_subgraph_depth` test - 5 minor issues: missing required edge field validation; missing graph route structural validation; disconnected `entry_point` silently returns depth 0; `bridge` routes receive no content validation; inconsistent operator type handling - 5 nits: misleading step naming, `import copy` placement, docstring line number reference, dead code, PR description count **Fixes applied:** - Added `isinstance(node_type, str)` guard in `_validate_graph_route` - Added `isinstance(provider, str)` guard in `_validate_llm_provider` - Added sequence+element type validation for `allowed_providers` - Added `_validate_limit_type` helper rejecting non-int limit values - Fixed DFS cycle guard: filter out already-visited neighbours before recursing - Tightened 4 weak assertions to specific substrings (`"unknown agent type"`, `"unknown node type"`, `"unknown operator type"`, `"max_total_nodes"`) - Added BDD scenarios for `agent_template`, `bridge` route, case-insensitive comparisons, boundary-exact limits - Added Robot `max_subgraph_depth` test - Added `source`/`target` presence checks; added `nodes`/`edges` presence checks; added `entry_point` existence check; added comment for bridge routes; changed operator type to fail-fast --- ### Cycle 2 **Review findings (0C/9M/4m/3n):** - 4 crash bugs: `agent_type` as list/dict → `TypeError: unhashable type`; `route_type` as list/dict → `TypeError`; `entry_point` as list → `TypeError` during structural limit enforcement; `allowed_providers` as dict → silently iterates over keys - 9 test quality issues: missing tests for non-string agent/route types, bool platform_limits, dict `allowed_providers`; loose assertions for depth, subgraph depth, entry_point; boundary-exact `max_subgraph_depth` test used wrong limit value; `# type: ignore` suppressions violating CONTRIBUTING.md - 4 minor issues: unbounded recursion guard; loose edge field assertion; `step_error_mentions_provider` too loose - 3 nits: coverage threshold discrepancy, tautological scenarios, provider assertion **Fixes applied:** - Added `isinstance(agent_type, str)` guard in `_validate_agents` - Added `isinstance(route_type, str)` guard in `_validate_routes` - Added `isinstance(entry_point, str)` guard in `_compute_graph_depth` - Replaced `allowed_providers` guard with explicit `isinstance(raw_allowed, (list, tuple, set, frozenset))` check - Added 5 new BDD scenarios (non-string agent type, non-string route type, non-string entry_point, dict allowed_providers, bool max_graph_depth) - Tightened assertions: `"max_graph_depth"`, `"max_subgraph_depth"`, `"entry_point"` in messages - Fixed boundary-exact `max_subgraph_depth` test to use `max_subgraph_depth=3` (not 4) - Removed all `# type: ignore` suppressions; replaced bare `dict` annotations with `dict[str, Any]`; configured pyright to not check behave imports - Added `_MAX_DEPTH_GUARD = 1000` with depth_guard parameter to both DFS functions - Tightened edge field assertion; tightened provider assertion --- ### Cycle 3 **Review findings (3C/3M/7m/5n):** - 3 critical crash bugs: `_MAX_DEPTH_GUARD=1000` equals Python's recursion limit → `RecursionError` at ~498 edges; `subgraph` field as list → `TypeError` in `_compute_subgraph_depth`; edge `source` as list → `TypeError` in `_compute_graph_depth` - 3 major issues: `pretty.output` artifact committed (672 KB); `entry_point` not validated in `_validate_graph_route`; empty `subgraph` reference silently accepted - 7 minor issues: non-string source/target silently dropped; non-dict nodes/edges silently accepted; docstring mismatch; missing non-string stream operator test; missing non-string allowed_providers element test; missing invalid types for max_total_nodes/max_subgraph_depth; tautological scenarios - 5 nits: missing route type field message; weak assertions in regression scenarios; entry_point assertion too permissive; pretty.output not in .gitignore; redundant `: Any` annotations **Fixes applied:** - Reduced `_MAX_DEPTH_GUARD` to 500 - Added `isinstance(sub_ref, str)` guard in `_compute_subgraph_depth` before `routes.get(sub_ref)` - Added `isinstance(source, str)` and `isinstance(target, str)` type checks in `_validate_graph_route` - Removed `pretty.output` via `git rm`; added to `.gitignore` - Added `entry_point` validation in `_validate_graph_route` (non-empty string, must match declared node) - Added empty subgraph reference check in `_validate_graph_route` - Changed silent-skip guards for `nodes`/`edges` to raise `ConfigurationError` - Fixed docstring: "for any single graph route" (not "across all graph routes") - Added 17 new BDD scenarios; removed 2 tautological scenarios - Strengthened `step_error_mentions_entry_point` to require both "entry_point" and "does not match any declared node" - Removed redundant `: Any` annotations --- ### Cycle 4 **Review findings (0C/2M/7m/7n):** - 2 major issues: `_MAX_DEPTH_GUARD=500` still unreachable (Python recursion limit fires first at ~498 edges); empty-string `source`/`target`/`entry_point` pass validation silently - 7 minor issues: non-dict node values silently skipped but counted by `_count_total_nodes`; subgraph references to non-graph routes silently return depth 0; unreachable depth-guard branches; missing zero-value boundary tests; missing empty-collection tests; unused `import sys`; `_compute_graph_depth` defaults `entry_point` to `"start"` - 7 nits: duplicate step definitions; redundant "And no exception is raised"; weak-assertion duplicate scenarios; redundant defensive entry_point check; duplicate `route_def.get("nodes")` lookups; negative platform limits not rejected; `noqa: BLE001` comments **Fixes applied:** - Converted `_compute_graph_depth` and `_compute_subgraph_depth` to iterative DFS using explicit stacks; removed `_MAX_DEPTH_GUARD` entirely - Added `not entry_point`, `not src_val`, `not tgt_val` checks for empty-string rejection - Changed non-dict node values from `continue` to `raise ConfigurationError` - Added route type check in `_compute_subgraph_depth` (subgraph must reference a graph route) - Added zero-value boundary tests; added empty-collection tests - Removed `import sys` from `CleverActorsLib.py` - Removed `entry_point` default `"start"` from `_compute_graph_depth`; replaced with `assert` - Refactored duplicate step definitions to shared helpers; removed redundant steps; removed weak-assertion duplicates; consolidated `nodes` lookup; added `value >= 0` check in `_validate_limit_type`; extracted `_call_and_capture` helper to eliminate `noqa` comments --- ### Cycle 5 **Review findings (0C/2M/5m/6n):** - 2 major issues: `validate_dict(None, {})` and `validate_dict({}, None)` raise `TypeError`/`AttributeError` instead of `ConfigurationError`; iterative DFS for longest-path is exponential in graph density (12-node complete graph hangs indefinitely) — practical DoS vector for router-facing API - 5 minor issues: `assert` statements in depth functions raise `AssertionError` not `ConfigurationError`; O(N²) frozenset allocations; non-dict edges/operators silently skipped; graph node missing `type` key produces confusing error; `ConfigurationError` not exported from `__init__.py` - 6 nits: misleading scenario names for unreachable code paths; `nodes_raw` assigned before presence check; `is None` check unreachable; non-specific step assertions; file size guideline violations; British spelling in docstring ### Remaining Issues (after Cycle 5 — not yet fixed) - **M1:** `validate_dict(None, {})` crashes with `TypeError` instead of `ConfigurationError` — needs `isinstance(config_dict, dict)` guard at function entry - **M2:** Exponential-time DFS for dense graphs — needs step-counter bailout (e.g., `MAX_DFS_STEPS = 1_000_000`) - **m1:** `assert` statements in `_compute_graph_depth`/`_compute_subgraph_depth` raise `AssertionError` not `ConfigurationError` - **m2:** O(N²) frozenset allocations in iterative DFS - **m3:** Non-dict edges and operators silently skipped (inconsistent with non-dict node fix) - **m4:** Missing `type` key in graph node produces confusing `"unknown node type ''"` error - **m5:** `ConfigurationError` not exported from `__init__.py` - Various nits (scenario naming, step assertion specificity, file size, spelling)
hurui200320 commented 2026-06-03 21:43:09 +00:00
Author
Member
Copy Link
Copy Source

Self-QA Implementation Notes (Cycles 6–9)

Cycle 6

Review findings (0C/2M/5m/6n):

  • 2 major issues: validate_dict(None, {}) crashes with TypeError instead of ConfigurationError; iterative DFS for longest-path is exponential in graph density (12-node complete graph hangs indefinitely)
  • 5 minor issues: assert statements raise AssertionError not ConfigurationError; O(N²) frozenset allocations; non-dict edges/operators silently skipped; graph node missing type key produces confusing error; ConfigurationError not exported from __init__.py
  • 6 nits: misleading scenario names, nodes_raw assignment order, is None check comment, non-specific step assertions, file size violations, British spelling

Fixes applied:

  • Added isinstance(config_dict, dict) and isinstance(platform_limits, dict) guards at top of validate_dict
  • Added MAX_DFS_STEPS = 1_000_000 step-counter bailout in _compute_graph_depth
  • Replaced all assert statements with explicit ConfigurationError checks
  • Refactored _compute_graph_depth from frozenset-per-step to mutable-set backtracking (O(1) per step)
  • Changed non-dict edge/operator entries from continue to raise ConfigurationError
  • Added explicit None check for missing node type key
  • Added ConfigurationError to __init__.py exports and __all__
  • Added BDD scenarios for None arguments, missing node type, non-dict edges/operators, ConfigurationError export
  • Fixed "Recognised" → "Recognized" in docstring

Cycle 7

Review findings (0C/6M/5m/5n):

  • 6 major issues: _compute_subgraph_depth lacks step limit (DoS); _compute_subgraph_depth retains O(D²) frozenset allocations (claimed fix not applied); # type: ignore suppressions in step files; three files exceed 500-line limit; _validate_stream_route silently skips non-list operators; imports buried inside functions
  • 5 minor issues: MAX_DFS_STEPS inside function body; unused _label in DFS stack tuple; misleading visiting parameter type; dangling subgraph references silently accepted; misleading error message for initial route
  • 5 nits: redundant edge filter, misleading scenario descriptions, dead step definitions, Robot test count mismatch, validate_dict import not grouped

Fixes applied:

  • Added _MAX_SUBGRAPH_STEPS = 1_000_000 step counter in _compute_subgraph_depth
  • Refactored _compute_subgraph_depth to mutable-set backtracking (removed frozenset unions)
  • Removed all remaining # type: ignore suppressions; changed _call_and_capture to accept Any
  • Split validation.py into sub-package: validation/__init__.py, _vocabulary.py, _agents.py, _routes.py, _limits.py
  • Split validate_dict_steps.py into 6 focused files: validate_dict_helpers.py, validate_dict_steps.py, validate_dict_agents_steps.py, validate_dict_routes_steps.py, validate_dict_limits_steps.py, validate_dict_export_steps.py
  • Changed non-list operators from silent return to raise ConfigurationError
  • Moved all buried imports to top of files
  • Moved MAX_DFS_STEPS to module-level constant _MAX_DFS_STEPS
  • Removed unused _label from DFS stack tuple
  • Removed visiting parameter from _compute_subgraph_depth (manages its own set)
  • Added ConfigurationError for dangling subgraph references
  • Removed redundant edge filter; removed dead step definitions

Cycle 8

Review findings (0C/2M/4m/4n):

  • 2 major issues: buried import cleveractors inside Robot keyword method bodies; validate_limit_type missing _ private prefix
  • 4 minor issues: _MAX_DFS_STEPS guard branch untested; _MAX_SUBGRAPH_STEPS guard branch untested; unused nodes variable in _compute_graph_depth; non-dict config_dict/platform_limits only tested with None
  • 4 nits: validate_dict as _validate_dict alias unnecessary; misleading scenario title; weak platform_limits error assertion; Robot test count mismatch

Fixes applied:

  • Moved 3 buried import cleveractors to top of CleverActorsLib.py
  • Renamed validate_limit_type → _validate_limit_type with 3 call-site updates
  • Added monkey-patch BDD scenarios for _MAX_DFS_STEPS and _MAX_SUBGRAPH_STEPS guards
  • Added features/environment.py after_scenario cleanup to restore monkey-patched constants
  • Removed unused nodes variable from _compute_graph_depth
  • Added BDD scenarios for list config_dict and string platform_limits
  • Removed as _validate_dict alias; renamed misleading scenario; strengthened platform_limits assertions

Cycle 9

Review findings (0C/1M/7m/5n):

  • 1 major issue: subgraph reference validation (existence + target-type check) gated behind optional max_subgraph_depth — invalid configs silently pass when limit is absent
  • 7 minor issues: LLM provider not stripped before allowlist comparison; graph edge endpoints not validated against declared nodes; buried import shutil in CleverActorsLib.py; redundant buried import shutil in environment.py; redundant buried import os in environment.py; buried imports in monkey-patch step functions; missing milestone on PR
  • 5 nits: validate_dict import not grouped; stale docstring in helpers; duplicate step definitions; no rationale comment for _MAX_DFS_STEPS; defensive isinstance guard in _count_total_nodes

Fixes applied:

  • Moved subgraph reference validation (existence + target-type check) into _validate_graph_route — now runs unconditionally regardless of max_subgraph_depth
  • Added provider.strip().lower() for whitespace-tolerant provider comparison
  • Added edge endpoint validation against declared nodes in _validate_graph_route
  • Moved all buried imports to top of respective files
  • Assigned PR #18 to v2.1.0 milestone
  • Removed duplicate step definition; updated docstring; added rationale comments; replaced defensive isinstance with assert
  • Added 5 new BDD scenarios (subgraph without limit, edge endpoint, provider whitespace)

Final Verdict (Cycle 9): APPROVED

All 8 acceptance criteria from ticket #10 are fully implemented and tested. Quality gates pass at 96.99% coverage (threshold 96.5%). The PR is ready to merge.

Remaining minor items (not blocking merge):

  • Buried imports in features/environment.py monkey-patch teardown (minor CONTRIBUTING.md violation)
  • Missing branch coverage for explicit route_type = None and allowed_providers as set/frozenset
  • Ticket #10 has no milestone assigned (PR has v2.1.0)
  • Pre-existing buried imports in CleverActorsLib.py (not introduced by this PR)
## Self-QA Implementation Notes (Cycles 6–9) ### Cycle 6 **Review findings (0C/2M/5m/6n):** - 2 major issues: `validate_dict(None, {})` crashes with `TypeError` instead of `ConfigurationError`; iterative DFS for longest-path is exponential in graph density (12-node complete graph hangs indefinitely) - 5 minor issues: `assert` statements raise `AssertionError` not `ConfigurationError`; O(N²) frozenset allocations; non-dict edges/operators silently skipped; graph node missing `type` key produces confusing error; `ConfigurationError` not exported from `__init__.py` - 6 nits: misleading scenario names, `nodes_raw` assignment order, `is None` check comment, non-specific step assertions, file size violations, British spelling **Fixes applied:** - Added `isinstance(config_dict, dict)` and `isinstance(platform_limits, dict)` guards at top of `validate_dict` - Added `MAX_DFS_STEPS = 1_000_000` step-counter bailout in `_compute_graph_depth` - Replaced all `assert` statements with explicit `ConfigurationError` checks - Refactored `_compute_graph_depth` from frozenset-per-step to mutable-set backtracking (O(1) per step) - Changed non-dict edge/operator entries from `continue` to `raise ConfigurationError` - Added explicit `None` check for missing node `type` key - Added `ConfigurationError` to `__init__.py` exports and `__all__` - Added BDD scenarios for None arguments, missing node type, non-dict edges/operators, ConfigurationError export - Fixed "Recognised" → "Recognized" in docstring --- ### Cycle 7 **Review findings (0C/6M/5m/5n):** - 6 major issues: `_compute_subgraph_depth` lacks step limit (DoS); `_compute_subgraph_depth` retains O(D²) frozenset allocations (claimed fix not applied); `# type: ignore` suppressions in step files; three files exceed 500-line limit; `_validate_stream_route` silently skips non-list operators; imports buried inside functions - 5 minor issues: `MAX_DFS_STEPS` inside function body; unused `_label` in DFS stack tuple; misleading `visiting` parameter type; dangling subgraph references silently accepted; misleading error message for initial route - 5 nits: redundant edge filter, misleading scenario descriptions, dead step definitions, Robot test count mismatch, `validate_dict` import not grouped **Fixes applied:** - Added `_MAX_SUBGRAPH_STEPS = 1_000_000` step counter in `_compute_subgraph_depth` - Refactored `_compute_subgraph_depth` to mutable-set backtracking (removed frozenset unions) - Removed all remaining `# type: ignore` suppressions; changed `_call_and_capture` to accept `Any` - Split `validation.py` into sub-package: `validation/__init__.py`, `_vocabulary.py`, `_agents.py`, `_routes.py`, `_limits.py` - Split `validate_dict_steps.py` into 6 focused files: `validate_dict_helpers.py`, `validate_dict_steps.py`, `validate_dict_agents_steps.py`, `validate_dict_routes_steps.py`, `validate_dict_limits_steps.py`, `validate_dict_export_steps.py` - Changed non-list `operators` from silent return to `raise ConfigurationError` - Moved all buried imports to top of files - Moved `MAX_DFS_STEPS` to module-level constant `_MAX_DFS_STEPS` - Removed unused `_label` from DFS stack tuple - Removed `visiting` parameter from `_compute_subgraph_depth` (manages its own set) - Added `ConfigurationError` for dangling subgraph references - Removed redundant edge filter; removed dead step definitions --- ### Cycle 8 **Review findings (0C/2M/4m/4n):** - 2 major issues: buried `import cleveractors` inside Robot keyword method bodies; `validate_limit_type` missing `_` private prefix - 4 minor issues: `_MAX_DFS_STEPS` guard branch untested; `_MAX_SUBGRAPH_STEPS` guard branch untested; unused `nodes` variable in `_compute_graph_depth`; non-dict `config_dict`/`platform_limits` only tested with `None` - 4 nits: `validate_dict as _validate_dict` alias unnecessary; misleading scenario title; weak platform_limits error assertion; Robot test count mismatch **Fixes applied:** - Moved 3 buried `import cleveractors` to top of `CleverActorsLib.py` - Renamed `validate_limit_type` → `_validate_limit_type` with 3 call-site updates - Added monkey-patch BDD scenarios for `_MAX_DFS_STEPS` and `_MAX_SUBGRAPH_STEPS` guards - Added `features/environment.py` `after_scenario` cleanup to restore monkey-patched constants - Removed unused `nodes` variable from `_compute_graph_depth` - Added BDD scenarios for list `config_dict` and string `platform_limits` - Removed `as _validate_dict` alias; renamed misleading scenario; strengthened platform_limits assertions --- ### Cycle 9 **Review findings (0C/1M/7m/5n):** - 1 major issue: subgraph reference validation (existence + target-type check) gated behind optional `max_subgraph_depth` — invalid configs silently pass when limit is absent - 7 minor issues: LLM provider not stripped before allowlist comparison; graph edge endpoints not validated against declared nodes; buried `import shutil` in `CleverActorsLib.py`; redundant buried `import shutil` in `environment.py`; redundant buried `import os` in `environment.py`; buried imports in monkey-patch step functions; missing milestone on PR - 5 nits: `validate_dict` import not grouped; stale docstring in helpers; duplicate step definitions; no rationale comment for `_MAX_DFS_STEPS`; defensive `isinstance` guard in `_count_total_nodes` **Fixes applied:** - Moved subgraph reference validation (existence + target-type check) into `_validate_graph_route` — now runs unconditionally regardless of `max_subgraph_depth` - Added `provider.strip().lower()` for whitespace-tolerant provider comparison - Added edge endpoint validation against declared nodes in `_validate_graph_route` - Moved all buried imports to top of respective files - Assigned PR #18 to `v2.1.0` milestone - Removed duplicate step definition; updated docstring; added rationale comments; replaced defensive `isinstance` with `assert` - Added 5 new BDD scenarios (subgraph without limit, edge endpoint, provider whitespace) ### Final Verdict (Cycle 9): **APPROVED** All 8 acceptance criteria from ticket #10 are fully implemented and tested. Quality gates pass at 96.99% coverage (threshold 96.5%). The PR is ready to merge. **Remaining minor items (not blocking merge):** - Buried imports in `features/environment.py` monkey-patch teardown (minor CONTRIBUTING.md violation) - Missing branch coverage for explicit `route_type = None` and `allowed_providers` as `set`/`frozenset` - Ticket #10 has no milestone assigned (PR has `v2.1.0`) - Pre-existing buried imports in `CleverActorsLib.py` (not introduced by this PR)
hurui200320 commented 2026-06-04 08:37:20 +00:00
Author
Member
Copy Link
Copy Source

Self-QA Implementation Notes (Cycles 10–12)

Cycle 10

Review findings (0C/2M/7m/4n):

  • M1 — Reserved agent and route names not rejected (spec §4.2, §5.1): agent/route names beginning with __ were silently accepted.
  • M2 — Monkey-patch tests patched the wrong module binding: setattr(_vocab, "_MAX_DFS_STEPS", ...) had no effect on _limits.py's local binding; teardown saved/restored from the wrong module.
  • m1 — _validate_llm_provider silently skipped non-dict agent_config instead of raising ConfigurationError.
  • m2 — _count_total_nodes used assert that can be disabled with -O.
  • m3 — Missing branch coverage: route_type = None explicit branch untested.
  • m4 — Missing branch coverage: allowed_providers as set or frozenset untested.
  • m5 — New buried imports in BDD step files (validate_dict_agents_steps.py, validate_dict_export_steps.py).
  • m6 — validate_dict_routes_steps.py exceeded 500-line file size limit (581 lines).
  • m7 — Buried imports in environment.py monkey-patch teardown.
  • n1 — Final annotation semantically incompatible with monkey-patching.
  • n2 — noqa: B010 suppressions instead of direct attribute assignment.
  • n4 — Dead _vocab patch calls in monkey-patch steps.

Fixes applied:

  • Added agent_name.startswith("__") check in _agents.py and route_name.startswith("__") check in _routes.py; added 2 BDD scenarios.
  • Removed all setattr(_vocab, ...) calls from validate_dict_limits_steps.py and environment.py; changed to direct attribute assignment on _limits only; moved imports to top-level; consolidated teardown blocks.
  • Changed _validate_llm_provider to raise ConfigurationError for non-dict agent_config; updated BDD scenario.
  • Replaced assert isinstance(nodes, dict) in _count_total_nodes with explicit isinstance check raising ConfigurationError.
  • Added BDD scenario for route_type = None.
  • Added BDD scenarios for allowed_providers as set and frozenset.
  • Moved buried imports to top of validate_dict_agents_steps.py and validate_dict_export_steps.py.
  • Split validate_dict_routes_steps.py into validate_dict_stream_steps.py (86 lines) and validate_dict_subgraph_steps.py (105 lines); routes file reduced to 431 lines.
  • Moved import cleveractors.validation._limits to top of environment.py; consolidated two if blocks.
  • Removed Final annotation from _MAX_DFS_STEPS and _MAX_SUBGRAPH_STEPS.
  • Replaced all setattr calls with direct attribute assignment; eliminated all noqa: B010 comments.

Cycle 11

Review findings (0C/1M/6m/5n):

  • M1 — Missing test for max_total_nodes aggregation across multiple graph routes: _count_total_nodes sums across all routes but every existing scenario used only a single route.
  • m1 — Non-string keys in agents dict caused AttributeError instead of ConfigurationError.
  • m2 — Non-string keys in routes dict caused AttributeError instead of ConfigurationError.
  • m3 — robot/CleverActorsLib.py exceeded 500-line limit (555 lines) after PR additions.
  • m4 — _MAX_DFS_STEPS ceiling of 1,000,000 too high for a web service upload endpoint (realistic DoS vector).
  • m5 — Untested allowed_providers as tuple.
  • m6 — Weak/tautological Robot test: validate_dict Can Be Used Without Any App Construction.
  • n1–n3 — Buried import json, import asyncio, from cleveractors.agents.tool import ToolAgent in CleverActorsLib.py (pre-existing).
  • n4 — _subgraph_children defensive branches not annotated with # pragma: no branch.
  • n5 — No test for empty operators list on stream route.

Fixes applied:

  • Added 2 BDD scenarios for max_total_nodes aggregation across two graph routes (exceeding limit + at-exact-limit).
  • Added isinstance(agent_name, str) guard in _agents.py raising ConfigurationError; added BDD scenario.
  • Added isinstance(route_name, str) guard in _routes.py raising ConfigurationError; added BDD scenario.
  • Extracted 6 validate_dict keywords from CleverActorsLib.py into new robot/ValidateDictLib.py; updated robot/validate_dict.robot to import both libraries. CleverActorsLib.py now at 492 lines.
  • Lowered both _MAX_DFS_STEPS and _MAX_SUBGRAPH_STEPS from 1,000,000 to 50,000.
  • Added BDD scenario for allowed_providers as tuple.
  • Renamed Robot test to validate_dict Accepts Minimal Valid Config.
  • Moved import json, import asyncio, from cleveractors.agents.tool import ToolAgent to top-level imports in CleverActorsLib.py.
  • Added # pragma: no branch to defensive guards in _subgraph_children.
  • Added BDD scenario for empty operators list on stream route.

Cycle 12

Review findings: Approved (0C/0M/3m/4n)

The reviewer noted three minor inconsistencies and four nits, none blocking merge:

  • m1 — Missing isinstance(node_name, str) guard for graph node keys (inconsistency with agent/route key guards).
  • m2 — Missing # pragma: no branch on _count_total_nodes defensive guard (inconsistent with _subgraph_children).
  • m3 — Step counter off-by-one between guard logic and error message (steps > _MAX_DFS_STEPS fires at 50,001 but message says "exceeded 50,000").
  • n1 — build_subgraph_chain(0) produces a dangling subgraph reference (latent helper bug, no current scenario triggers it).
  • n2 — Dead commented-out code in environment.py.
  • n3 — step_error_mentions_limits_type assertion is broad.
  • n4 — _compute_subgraph_depth called once per graph route (minor redundancy, not a practical concern).

Verdict: APPROVED — PR is ready to merge.

Remaining Issues (not blocking merge)

  • m1 — isinstance(node_name, str) guard missing for graph node keys (minor inconsistency).
  • m2 — # pragma: no branch missing on _count_total_nodes guard (minor coverage annotation inconsistency).
  • m3 — Step counter off-by-one in error message wording.
  • n1 — build_subgraph_chain(0) latent helper bug.
  • n2 — Dead commented-out code in environment.py.
## Self-QA Implementation Notes (Cycles 10–12) ### Cycle 10 **Review findings (0C/2M/7m/4n):** - **M1** — Reserved agent and route names not rejected (spec §4.2, §5.1): agent/route names beginning with `__` were silently accepted. - **M2** — Monkey-patch tests patched the wrong module binding: `setattr(_vocab, "_MAX_DFS_STEPS", ...)` had no effect on `_limits.py`'s local binding; teardown saved/restored from the wrong module. - **m1** — `_validate_llm_provider` silently skipped non-dict `agent_config` instead of raising `ConfigurationError`. - **m2** — `_count_total_nodes` used `assert` that can be disabled with `-O`. - **m3** — Missing branch coverage: `route_type = None` explicit branch untested. - **m4** — Missing branch coverage: `allowed_providers` as `set` or `frozenset` untested. - **m5** — New buried imports in BDD step files (`validate_dict_agents_steps.py`, `validate_dict_export_steps.py`). - **m6** — `validate_dict_routes_steps.py` exceeded 500-line file size limit (581 lines). - **m7** — Buried imports in `environment.py` monkey-patch teardown. - **n1** — `Final` annotation semantically incompatible with monkey-patching. - **n2** — `noqa: B010` suppressions instead of direct attribute assignment. - **n4** — Dead `_vocab` patch calls in monkey-patch steps. **Fixes applied:** - Added `agent_name.startswith("__")` check in `_agents.py` and `route_name.startswith("__")` check in `_routes.py`; added 2 BDD scenarios. - Removed all `setattr(_vocab, ...)` calls from `validate_dict_limits_steps.py` and `environment.py`; changed to direct attribute assignment on `_limits` only; moved imports to top-level; consolidated teardown blocks. - Changed `_validate_llm_provider` to raise `ConfigurationError` for non-dict `agent_config`; updated BDD scenario. - Replaced `assert isinstance(nodes, dict)` in `_count_total_nodes` with explicit `isinstance` check raising `ConfigurationError`. - Added BDD scenario for `route_type = None`. - Added BDD scenarios for `allowed_providers` as `set` and `frozenset`. - Moved buried imports to top of `validate_dict_agents_steps.py` and `validate_dict_export_steps.py`. - Split `validate_dict_routes_steps.py` into `validate_dict_stream_steps.py` (86 lines) and `validate_dict_subgraph_steps.py` (105 lines); routes file reduced to 431 lines. - Moved `import cleveractors.validation._limits` to top of `environment.py`; consolidated two `if` blocks. - Removed `Final` annotation from `_MAX_DFS_STEPS` and `_MAX_SUBGRAPH_STEPS`. - Replaced all `setattr` calls with direct attribute assignment; eliminated all `noqa: B010` comments. --- ### Cycle 11 **Review findings (0C/1M/6m/5n):** - **M1** — Missing test for `max_total_nodes` aggregation across multiple graph routes: `_count_total_nodes` sums across all routes but every existing scenario used only a single route. - **m1** — Non-string keys in `agents` dict caused `AttributeError` instead of `ConfigurationError`. - **m2** — Non-string keys in `routes` dict caused `AttributeError` instead of `ConfigurationError`. - **m3** — `robot/CleverActorsLib.py` exceeded 500-line limit (555 lines) after PR additions. - **m4** — `_MAX_DFS_STEPS` ceiling of 1,000,000 too high for a web service upload endpoint (realistic DoS vector). - **m5** — Untested `allowed_providers` as `tuple`. - **m6** — Weak/tautological Robot test: `validate_dict Can Be Used Without Any App Construction`. - **n1–n3** — Buried `import json`, `import asyncio`, `from cleveractors.agents.tool import ToolAgent` in `CleverActorsLib.py` (pre-existing). - **n4** — `_subgraph_children` defensive branches not annotated with `# pragma: no branch`. - **n5** — No test for empty `operators` list on stream route. **Fixes applied:** - Added 2 BDD scenarios for `max_total_nodes` aggregation across two graph routes (exceeding limit + at-exact-limit). - Added `isinstance(agent_name, str)` guard in `_agents.py` raising `ConfigurationError`; added BDD scenario. - Added `isinstance(route_name, str)` guard in `_routes.py` raising `ConfigurationError`; added BDD scenario. - Extracted 6 `validate_dict` keywords from `CleverActorsLib.py` into new `robot/ValidateDictLib.py`; updated `robot/validate_dict.robot` to import both libraries. `CleverActorsLib.py` now at 492 lines. - Lowered both `_MAX_DFS_STEPS` and `_MAX_SUBGRAPH_STEPS` from 1,000,000 to 50,000. - Added BDD scenario for `allowed_providers` as `tuple`. - Renamed Robot test to `validate_dict Accepts Minimal Valid Config`. - Moved `import json`, `import asyncio`, `from cleveractors.agents.tool import ToolAgent` to top-level imports in `CleverActorsLib.py`. - Added `# pragma: no branch` to defensive guards in `_subgraph_children`. - Added BDD scenario for empty `operators` list on stream route. --- ### Cycle 12 **Review findings: Approved (0C/0M/3m/4n)** The reviewer noted three minor inconsistencies and four nits, none blocking merge: - **m1** — Missing `isinstance(node_name, str)` guard for graph node keys (inconsistency with agent/route key guards). - **m2** — Missing `# pragma: no branch` on `_count_total_nodes` defensive guard (inconsistent with `_subgraph_children`). - **m3** — Step counter off-by-one between guard logic and error message (`steps > _MAX_DFS_STEPS` fires at 50,001 but message says "exceeded 50,000"). - **n1** — `build_subgraph_chain(0)` produces a dangling subgraph reference (latent helper bug, no current scenario triggers it). - **n2** — Dead commented-out code in `environment.py`. - **n3** — `step_error_mentions_limits_type` assertion is broad. - **n4** — `_compute_subgraph_depth` called once per graph route (minor redundancy, not a practical concern). **Verdict: APPROVED** — PR is ready to merge. ### Remaining Issues (not blocking merge) - **m1** — `isinstance(node_name, str)` guard missing for graph node keys (minor inconsistency). - **m2** — `# pragma: no branch` missing on `_count_total_nodes` guard (minor coverage annotation inconsistency). - **m3** — Step counter off-by-one in error message wording. - **n1** — `build_subgraph_chain(0)` latent helper bug. - **n2** — Dead commented-out code in `environment.py`.
hurui200320 commented 2026-06-05 05:36:39 +00:00
Author
Member
Copy Link
Copy Source

⚠️ Bot Interference — Schema Conflict with runtime.py

The bot pushed commit e7a7d39 directly to master on 2026-06-04. Among other things, it added a validate_dict() function inside src/cleveractors/runtime.py that is incompatible with this ticket's spec and with our proper implementation on feature/validate-dict-api (37f8453).

What the bot's validate_dict does (wrong)

# Bot's version in runtime.py
actor_type = config_dict.get("type")
if actor_type is None:
    if "actors" in config_dict:
        actor_type = "multi_actor"
    else:
        raise ConfigurationError("Actor config must have 'type' field.")

valid_types = {"llm", "graph", "tool", "multi_actor"}
  • Expects a top-level type field with vocabulary {llm, graph, tool, multi_actor}
  • For llm actors: expects provider and model at top level or under config block
  • For graph actors: expects route.nodes / route.edges / route.entry_node

What this ticket requires (correct — our feature/validate-dict-api)

  • Expects top-level agents and routes keys (raises ConfigurationError if either is absent)
  • Agent types vocabulary: {llm, tool, composite, chain, template_instance}
  • LLM agent provider validated against platform_limits["allowed_providers"]
  • Route/edge fields validated using source/target (not from/to)
  • Structural limits: max_graph_depth (DFS longest-path), max_subgraph_depth, max_total_nodes

These two schemas are mutually incompatible. A config dict that passes the bot's version will fail ours, and vice versa.

Current state of __init__.py (after bot's commit)

The bot's __init__.py imports validate_dict from runtime, which means the bot's version is currently the one exported at the package level:

from cleveractors.runtime import (
    ...
    validate_dict,
    ...
)

What needs to happen

  1. Do NOT merge feature/validate-dict-api on top of the current master as-is — the validate_dict in runtime.py will conflict and will silently take precedence if not removed.
  2. When feature/validate-dict-api is ready to merge, remove the validate_dict function from runtime.py entirely and update the import in __init__.py to use our implementation: 
    # __init__.py — after fix
from cleveractors.validation import validate_dict
  3. runtime.py's validate_dict may still be referenced by the bot's Executor code — confirm those call sites are updated to use the canonical validation.validate_dict as well.
  4. The bot's validate_dict also does not enforce the max_graph_depth DFS check, the max_subgraph_depth recursion guard, or the source/target edge field requirement — none of the structural validation from this ticket is present in the bot's version.

Action items for this ticket

  • Before merging feature/validate-dict-api → master: remove validate_dict from runtime.py
  • Update __init__.py import to use cleveractors.validation.validate_dict
  • Verify the Executor in runtime.py calls through to the canonical validate_dict (or none at all — Executor.execute() should not re-validate; validation is caller's responsibility)
  • Confirm PR merge resolves the conflict cleanly
## ⚠️ Bot Interference — Schema Conflict with `runtime.py` The bot pushed commit `e7a7d39` directly to `master` on 2026-06-04. Among other things, it added a `validate_dict()` function inside `src/cleveractors/runtime.py` that is **incompatible with this ticket's spec** and with our proper implementation on `feature/validate-dict-api` (`37f8453`). ### What the bot's `validate_dict` does (wrong) ```python # Bot's version in runtime.py actor_type = config_dict.get("type") if actor_type is None: if "actors" in config_dict: actor_type = "multi_actor" else: raise ConfigurationError("Actor config must have 'type' field.") valid_types = {"llm", "graph", "tool", "multi_actor"} ``` - Expects a top-level `type` field with vocabulary `{llm, graph, tool, multi_actor}` - For `llm` actors: expects `provider` and `model` at top level or under `config` block - For `graph` actors: expects `route.nodes` / `route.edges` / `route.entry_node` ### What this ticket requires (correct — our `feature/validate-dict-api`) - Expects top-level `agents` and `routes` keys (raises `ConfigurationError` if either is absent) - Agent types vocabulary: `{llm, tool, composite, chain, template_instance}` - LLM agent `provider` validated against `platform_limits["allowed_providers"]` - Route/edge fields validated using `source`/`target` (not `from`/`to`) - Structural limits: `max_graph_depth` (DFS longest-path), `max_subgraph_depth`, `max_total_nodes` These two schemas are **mutually incompatible**. A config dict that passes the bot's version will fail ours, and vice versa. ### Current state of `__init__.py` (after bot's commit) The bot's `__init__.py` imports `validate_dict` from `runtime`, which means the bot's version is currently the one exported at the package level: ```python from cleveractors.runtime import ( ... validate_dict, ... ) ``` ### What needs to happen 1. **Do NOT merge `feature/validate-dict-api` on top of the current `master` as-is** — the `validate_dict` in `runtime.py` will conflict and will silently take precedence if not removed. 2. When `feature/validate-dict-api` is ready to merge, **remove the `validate_dict` function from `runtime.py`** entirely and update the import in `__init__.py` to use our implementation: ```python # __init__.py — after fix from cleveractors.validation import validate_dict ``` 3. `runtime.py`'s `validate_dict` may still be referenced by the bot's `Executor` code — confirm those call sites are updated to use the canonical `validation.validate_dict` as well. 4. The bot's `validate_dict` also does not enforce the `max_graph_depth` DFS check, the `max_subgraph_depth` recursion guard, or the `source`/`target` edge field requirement — **none of the structural validation** from this ticket is present in the bot's version. ### Action items for this ticket - [ ] Before merging `feature/validate-dict-api` → `master`: remove `validate_dict` from `runtime.py` - [ ] Update `__init__.py` import to use `cleveractors.validation.validate_dict` - [ ] Verify the `Executor` in `runtime.py` calls through to the canonical `validate_dict` (or none at all — `Executor.execute()` should not re-validate; validation is caller's responsibility) - [ ] Confirm PR merge resolves the conflict cleanly
hurui200320 commented 2026-06-05 05:54:06 +00:00
Author
Member
Copy Link
Copy Source

✅ Bot Interference Resolved — validate_dict Removed from runtime.py

Addressing the action items raised in the previous comment:

What was done

  • Removed validate_dict from runtime.py — The entire # Validation section (the bot's incorrect implementation expecting a type field with vocabulary {llm, graph, tool, multi_actor}) has been deleted from runtime.py.
  • Module docstring updated — runtime.py's docstring no longer lists validate_dict(d, platform_limits) as part of its public API surface.
  • __init__.py was already correct — The feature branch's commit already imported validate_dict from cleveractors.validation, not from cleveractors.runtime. No change needed there.
  • Executor confirmed clean — Executor.execute() in runtime.py does not call validate_dict internally. Validation is the caller's responsibility, as intended.

Amended commit

The fix was folded into the existing feat(validate_dict) commit via --amend and force-pushed to feature/validate-dict-api (91172ae).

Action item status

  • Remove validate_dict from runtime.py
  • Update __init__.py import to use cleveractors.validation.validate_dict (was already done in the original commit)
  • Verify Executor in runtime.py does not call validate_dict internally (confirmed clean)
  • Confirm PR merge resolves the conflict cleanly (PR #18 remains mergeable; merge will produce a correct state with only cleveractors.validation.validate_dict exported)
## ✅ Bot Interference Resolved — `validate_dict` Removed from `runtime.py` Addressing the action items raised in the previous comment: ### What was done - **Removed `validate_dict` from `runtime.py`** — The entire `# Validation` section (the bot's incorrect implementation expecting a `type` field with vocabulary `{llm, graph, tool, multi_actor}`) has been deleted from `runtime.py`. - **Module docstring updated** — `runtime.py`'s docstring no longer lists `validate_dict(d, platform_limits)` as part of its public API surface. - **`__init__.py` was already correct** — The feature branch's commit already imported `validate_dict` from `cleveractors.validation`, not from `cleveractors.runtime`. No change needed there. - **`Executor` confirmed clean** — `Executor.execute()` in `runtime.py` does not call `validate_dict` internally. Validation is the caller's responsibility, as intended. ### Amended commit The fix was folded into the existing `feat(validate_dict)` commit via `--amend` and force-pushed to `feature/validate-dict-api` (`91172ae`). ### Action item status - [x] Remove `validate_dict` from `runtime.py` - [x] Update `__init__.py` import to use `cleveractors.validation.validate_dict` (was already done in the original commit) - [x] Verify `Executor` in `runtime.py` does not call `validate_dict` internally (confirmed clean) - [x] Confirm PR merge resolves the conflict cleanly (PR #18 remains mergeable; merge will produce a correct state with only `cleveractors.validation.validate_dict` exported)
hurui200320 commented 2026-06-05 10:13:39 +00:00
Author
Member
Copy Link
Copy Source

Self-QA Implementation Notes (Cycles 13–15)

Note: Cycles 1–12 were documented in previous comments. This session continues from the current HEAD at 8093c23 (post-bot-interference fix). Cycles are renumbered 13–15 for this session.

Cycle 13

Review findings (0C/3M/4m/4n):

  • M1 — Validator rejects spec-conformant configs that omit entry_point (reviewer claimed spec defines it as optional with default "start") — verified against spec and found NOT VALID: entry_point is required per the authoritative spec. No code change.
  • M2 — Validator rejects edges referencing implicit start/end nodes (reviewer claimed spec mandates implicit injection) — verified against spec and found NOT VALID: start/end must be explicitly declared. No code change.
  • M3 — Missing isinstance(node_name, str) guard for graph node keys (Cycle 12 m1 carry-over).
  • m1 — Missing # pragma: no branch on _count_total_nodes defensive guard (Cycle 12 m2 carry-over).
  • m2 — Dead defensive continue branches in _validate_graph_route lack # pragma: no branch.
  • m3 — src not in nodes_raw branch (dangling source node) never exercised by any test.
  • m4 — Dead commented-out code in environment.py (Cycle 12 n2 carry-over).
  • n1 — build_subgraph_chain(0) produces dangling subgraph reference.
  • n2 — Misleading scenario name for dangling edge endpoint test.
  • n3 — Step counter steps > _MAX_DFS_STEPS allows one extra iteration beyond documented ceiling.
  • n4 — Duplicate edges in adjacency list waste DFS step budget.

Fixes applied:

  • Added isinstance(node_name, str) guard in _validate_graph_route before the node loop; added BDD scenario.
  • Added # pragma: no branch to _count_total_nodes defensive guard in _limits.py.
  • Added # pragma: no branch to both dead continue guards in _validate_graph_route.
  • Added new BDD scenario for dangling source node; renamed existing "source" scenario to "target" for accuracy.
  • Deleted 4 lines of dead commented-out InlineYAMLJinja code from environment.py.
  • Added if depth <= 0: raise ValueError(...) guard to build_subgraph_chain in helpers.
  • Renamed misleading scenario title.
  • Changed steps > _MAX_DFS_STEPS to steps >= _MAX_DFS_STEPS (and same for _MAX_SUBGRAPH_STEPS).
  • Changed adjacency value type from list[str] to set[str] for O(1) deduplication.

Cycle 14

Review findings (0C/2M/6m/2n) — ADR-2029 items excluded (not applicable to this library):

  • M1 — _subgraph_children yields duplicate route names, causing false "too complex" errors on legitimate configs with multiple nodes referencing the same subgraph.
  • M2 — Adjacency list built with O(N) list-scan deduplication; O(N³) worst case for dense graphs (DoS vector at upload endpoint).
  • m1 — No test for LLM agent without provider field when default "openai" is excluded from custom allowed_providers.
  • m2 — No positive test coverage for valid node types other than "agent" and "end".
  • m3 — No positive test coverage for valid stream operator types other than "map".
  • m4 — Unbounded user-controlled strings in error messages — deferred: ADR-2025 router-side 256 KiB file-size limit provides adequate mitigation.
  • m5 — _compute_subgraph_depth called once per graph route with no memoization.
  • m6 — environment.py imports _limits internals at module level, coupling all BDD tests to internal implementation detail.
  • n1 — validate_dict_routes_steps.py at 473 lines, approaching 500-line limit.
  • n2 — _vocabulary.py module comment overstates DFS step capacity.

Fixes applied:

  • Added seen: set[str] deduplication in _subgraph_children() — each unique subgraph ref yielded at most once per route; added BDD scenario.
  • Changed adjacency value type to set[str] in _compute_graph_depth() — O(1) deduplication, O(N²) worst case for dense graphs.
  • Added BDD scenario: LLM agent without provider fails when allowed_providers excludes "openai".
  • Added BDD scenarios for remaining valid node types: start, tool, conditional, message_router, function.
  • Added BDD scenarios for additional valid stream operator types: filter, transform, reduce.
  • Added depth_cache: dict[str, int] parameter to _compute_subgraph_depth() and memoize results within a single validate_structural_limits call.
  • Moved import cleveractors.validation._limits as _limits from module level into after_scenario, guarded by hasattr(context, '_original_max_dfs').
  • Split validate_dict_routes_steps.py (473 lines) into validate_dict_routes_steps.py (83 lines, generic route steps) and validate_dict_graph_route_steps.py (447 lines, graph-specific steps).
  • Updated _vocabulary.py comment to accurately state O(N + E) complexity and note dense-graph ceiling exhaustion.

Cycle 15

Review findings: Approved (0C/0M/3m/4n)

The reviewer confirmed all acceptance criteria are met and all Cycle 13–14 fixes were applied correctly. Remaining non-blocking items:

  • m1 — depth_cache memoization in _compute_subgraph_depth is incomplete: only top-level routes are cached, not intermediate children. Correctness is unaffected; the 50k step ceiling bounds worst-case behavior.
  • m2 — validate_dict.feature at 877 lines exceeds the 500-line file size limit.
  • m3 — Many positive-path BDD scenarios only assert "no exception is raised" without verifying the return value.
  • n1 — Step counter error message says "exceeded" but fires at exactly the limit (>=).
  • n2 — Redundant double import of _limits in after_scenario.
  • n3 — CHANGELOG.md entry is 842 characters on a single line.
  • n4 — visited variable name in _compute_graph_depth is misleading (tracks current path, not all visited nodes).

Verdict: APPROVED — PR is ready to merge.

Remaining Issues (not blocking merge)

  • m1 — depth_cache memoization incomplete for intermediate child routes (performance only, correctness unaffected).
  • m2 — validate_dict.feature exceeds 500-line limit (877 lines).
  • m3 — Positive-path scenarios do not verify return value identity.
  • n1–n4 — Minor wording, style, and naming nits.
## Self-QA Implementation Notes (Cycles 13–15) > **Note:** Cycles 1–12 were documented in previous comments. This session continues from the current HEAD at `8093c23` (post-bot-interference fix). Cycles are renumbered 13–15 for this session. --- ### Cycle 13 **Review findings (0C/3M/4m/4n):** - **M1** — Validator rejects spec-conformant configs that omit `entry_point` (reviewer claimed spec defines it as optional with default `"start"`) — **verified against spec and found NOT VALID**: `entry_point` is required per the authoritative spec. No code change. - **M2** — Validator rejects edges referencing implicit `start`/`end` nodes (reviewer claimed spec mandates implicit injection) — **verified against spec and found NOT VALID**: `start`/`end` must be explicitly declared. No code change. - **M3** — Missing `isinstance(node_name, str)` guard for graph node keys (Cycle 12 m1 carry-over). - **m1** — Missing `# pragma: no branch` on `_count_total_nodes` defensive guard (Cycle 12 m2 carry-over). - **m2** — Dead defensive `continue` branches in `_validate_graph_route` lack `# pragma: no branch`. - **m3** — `src not in nodes_raw` branch (dangling source node) never exercised by any test. - **m4** — Dead commented-out code in `environment.py` (Cycle 12 n2 carry-over). - **n1** — `build_subgraph_chain(0)` produces dangling subgraph reference. - **n2** — Misleading scenario name for dangling edge endpoint test. - **n3** — Step counter `steps > _MAX_DFS_STEPS` allows one extra iteration beyond documented ceiling. - **n4** — Duplicate edges in adjacency list waste DFS step budget. **Fixes applied:** - Added `isinstance(node_name, str)` guard in `_validate_graph_route` before the node loop; added BDD scenario. - Added `# pragma: no branch` to `_count_total_nodes` defensive guard in `_limits.py`. - Added `# pragma: no branch` to both dead `continue` guards in `_validate_graph_route`. - Added new BDD scenario for dangling source node; renamed existing "source" scenario to "target" for accuracy. - Deleted 4 lines of dead commented-out InlineYAMLJinja code from `environment.py`. - Added `if depth <= 0: raise ValueError(...)` guard to `build_subgraph_chain` in helpers. - Renamed misleading scenario title. - Changed `steps > _MAX_DFS_STEPS` to `steps >= _MAX_DFS_STEPS` (and same for `_MAX_SUBGRAPH_STEPS`). - Changed adjacency value type from `list[str]` to `set[str]` for O(1) deduplication. --- ### Cycle 14 **Review findings (0C/2M/6m/2n) — ADR-2029 items excluded (not applicable to this library):** - **M1** — `_subgraph_children` yields duplicate route names, causing false "too complex" errors on legitimate configs with multiple nodes referencing the same subgraph. - **M2** — Adjacency list built with O(N) list-scan deduplication; O(N³) worst case for dense graphs (DoS vector at upload endpoint). - **m1** — No test for LLM agent without provider field when default `"openai"` is excluded from custom `allowed_providers`. - **m2** — No positive test coverage for valid node types other than `"agent"` and `"end"`. - **m3** — No positive test coverage for valid stream operator types other than `"map"`. - **m4** — Unbounded user-controlled strings in error messages — **deferred**: ADR-2025 router-side 256 KiB file-size limit provides adequate mitigation. - **m5** — `_compute_subgraph_depth` called once per graph route with no memoization. - **m6** — `environment.py` imports `_limits` internals at module level, coupling all BDD tests to internal implementation detail. - **n1** — `validate_dict_routes_steps.py` at 473 lines, approaching 500-line limit. - **n2** — `_vocabulary.py` module comment overstates DFS step capacity. **Fixes applied:** - Added `seen: set[str]` deduplication in `_subgraph_children()` — each unique subgraph ref yielded at most once per route; added BDD scenario. - Changed adjacency value type to `set[str]` in `_compute_graph_depth()` — O(1) deduplication, O(N²) worst case for dense graphs. - Added BDD scenario: LLM agent without provider fails when `allowed_providers` excludes `"openai"`. - Added BDD scenarios for remaining valid node types: `start`, `tool`, `conditional`, `message_router`, `function`. - Added BDD scenarios for additional valid stream operator types: `filter`, `transform`, `reduce`. - Added `depth_cache: dict[str, int]` parameter to `_compute_subgraph_depth()` and memoize results within a single `validate_structural_limits` call. - Moved `import cleveractors.validation._limits as _limits` from module level into `after_scenario`, guarded by `hasattr(context, '_original_max_dfs')`. - Split `validate_dict_routes_steps.py` (473 lines) into `validate_dict_routes_steps.py` (83 lines, generic route steps) and `validate_dict_graph_route_steps.py` (447 lines, graph-specific steps). - Updated `_vocabulary.py` comment to accurately state O(N + E) complexity and note dense-graph ceiling exhaustion. --- ### Cycle 15 **Review findings: Approved (0C/0M/3m/4n)** The reviewer confirmed all acceptance criteria are met and all Cycle 13–14 fixes were applied correctly. Remaining non-blocking items: - **m1** — `depth_cache` memoization in `_compute_subgraph_depth` is incomplete: only top-level routes are cached, not intermediate children. Correctness is unaffected; the 50k step ceiling bounds worst-case behavior. - **m2** — `validate_dict.feature` at 877 lines exceeds the 500-line file size limit. - **m3** — Many positive-path BDD scenarios only assert "no exception is raised" without verifying the return value. - **n1** — Step counter error message says "exceeded" but fires at exactly the limit (`>=`). - **n2** — Redundant double import of `_limits` in `after_scenario`. - **n3** — `CHANGELOG.md` entry is 842 characters on a single line. - **n4** — `visited` variable name in `_compute_graph_depth` is misleading (tracks current path, not all visited nodes). **Verdict: APPROVED** — PR is ready to merge. ### Remaining Issues (not blocking merge) - **m1** — `depth_cache` memoization incomplete for intermediate child routes (performance only, correctness unaffected). - **m2** — `validate_dict.feature` exceeds 500-line limit (877 lines). - **m3** — Positive-path scenarios do not verify return value identity. - **n1–n4** — Minor wording, style, and naming nits.
hurui200320 added
State
Completed
 and removed
State
In Review
 labels 2026-06-05 10:19:37 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
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
Points
8
Priority
High
State
Completed
Type
Feature
Milestone
No items
No Milestone
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 hurui200320
1 Participants
Notifications
Due Date
No due date set.
Blocks
#17 feat(public-api): expose all router-facing APIs at cleveractors package level; update README
cleveragents/cleveractors-core
You do not have permission to read 1 dependency
Depends on
#18 feat(validate_dict): expose public validate_dict(config_dict, platform_limits) API
cleveragents/cleveractors-core
Reference: cleveragents/cleveractors-core#10
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?