test(robot): add end-to-end integration tests for email categorization actor graph #50

2026-06-12T20:25:31Z

CoreRasurae commented

2026-06-12 20:25:31 +00:00

Summary

Adds 104 Robot Framework integration tests for a 6-agent email categorization actor graph across five configuration strategies: local, remote, mixed, component-decomposed (merge_configs), and namespaced (local: refs via ReferenceResolver + LocalPackageStore).

Also adds 4 BDD feature files (15 scenarios) increasing coverage from 96.8% to 97.1% by testing previously uncovered error paths in registry resolver, LLM agent validation, URL canonicalization, and YAML template error handling.

Changes

Integration tests (commit 1)

7 new email graph YAML fixtures under tests/fixtures/email_graph/
7 namespaced component YAMLs with local namespace identifiers
9 robot test suite files (email_categorization_graph, per-agent, negative tests, merge_configs validation, components, namespaced)
EmailGraphLib.py keyword library (executor, graph, merge, component resolution)
CleverActorsLib.py refactored into per-module libs: robot/lib/config_lib.py, app_lib.py, registry_lib.py
email_testdata.resource with 12 realistic email scenarios

BDD unit test coverage (commit 2)

registry_resolver_errors.feature (7 scenarios) — Invalid semver, alias mismatches, ID/local/unknown reference errors
llm_agent_config_validation.feature (4 scenarios) — Constructor argument validation
llm_provider_url_canonicalization.feature (2 scenarios) — URL canonicalization with port
yaml_template_non_dict_errors.feature (2 scenarios) — Non-dict YAML error handling

Quality gate

104/104 integration tests pass, 23/23 existing tests preserved
132 BDD features, 2611 scenarios, 12412 steps — all green
Coverage: 97.1% (threshold 96.5%)
Lint, format, typecheck, dead_code, security: all green

This PR contains three logically separate changes for #30:

Robot Framework integration tests for the email categorization graph
BDD unit test coverage for error branches (registry, LLM, YAML)
Build fix: cap robotframework<7.0 for pabot 5.3 compatibility

Closes #30

## Summary Adds 104 Robot Framework integration tests for a 6-agent email categorization actor graph across five configuration strategies: local, remote, mixed, component-decomposed (merge_configs), and namespaced (local: refs via ReferenceResolver + LocalPackageStore). Also adds 4 BDD feature files (15 scenarios) increasing coverage from 96.8% to 97.1% by testing previously uncovered error paths in registry resolver, LLM agent validation, URL canonicalization, and YAML template error handling. ## Changes ### Integration tests (commit 1) - 7 new email graph YAML fixtures under `tests/fixtures/email_graph/` - 7 namespaced component YAMLs with local namespace identifiers - 9 robot test suite files (email_categorization_graph, per-agent, negative tests, merge_configs validation, components, namespaced) - `EmailGraphLib.py` keyword library (executor, graph, merge, component resolution) - CleverActorsLib.py refactored into per-module libs: `robot/lib/config_lib.py`, `app_lib.py`, `registry_lib.py` - `email_testdata.resource` with 12 realistic email scenarios ### BDD unit test coverage (commit 2) - `registry_resolver_errors.feature` (7 scenarios) — Invalid semver, alias mismatches, ID/local/unknown reference errors - `llm_agent_config_validation.feature` (4 scenarios) — Constructor argument validation - `llm_provider_url_canonicalization.feature` (2 scenarios) — URL canonicalization with port - `yaml_template_non_dict_errors.feature` (2 scenarios) — Non-dict YAML error handling ## Quality gate - 104/104 integration tests pass, 23/23 existing tests preserved - 132 BDD features, 2611 scenarios, 12412 steps — all green - Coverage: 97.1% (threshold 96.5%) - Lint, format, typecheck, dead_code, security: all green ## This PR contains three logically separate changes for #30: Robot Framework integration tests for the email categorization graph BDD unit test coverage for error branches (registry, LLM, YAML) Build fix: cap robotframework<7.0 for pabot 5.3 compatibility Closes #30

CoreRasurae added this to the v2.1.0 milestone 2026-06-12 20:25:41 +00:00

CoreRasurae added a new dependency 2026-06-12 20:33:23 +00:00

#30 test(robot): add end-to-end integration tests for email categorization actor graph

Graa requested changes 2026-06-12 20:33:31 +00:00

Dismissed

Graa left a comment

First-time contributor

PR Review: !50 (Ticket #30)

Verdict: REQUEST_CHANGES — 2 blockers

Solid addition overall — the email categorization graph fixtures are well-structured, the CleverActorsLib refactor into per-module libs is clean, and the 4 new BDD feature files are well-targeted at uncovered error branches. Two blockers must be resolved before merge.

❌ BLOCKER 1 — `integration_tests` CI gate is failing

Commit 58c7f67 shows CI / integration_tests (pull_request) failing after 1m23s. Per company policy, all required CI gates must be green before a PR can be approved and merged.

Note: Rui Hu and Luís Mendes discussed an intermittent CI "deadlock" that resolves on retry. If this failure is that known transient issue, a clean retry run is all that is needed. If it is not transient, the root cause must be identified and fixed. Either way the gate must show green before merge.

Recommendation: Re-trigger CI. If it passes cleanly on retry, the blocker is cleared. If it fails again, investigate — the new email graph robot tests exercise the full cleveractors runtime (6 agents, graph compilation, execution, result validation) and any missing fixture file, import error, or runtime exception in those tests will present as a 1-2 minute integration failure.

❌ BLOCKER 2 — Two commits; CONTRIBUTING.md requires one commit per issue

The PR contains two commits:

eb9c69d3 — test(robot): add end-to-end integration tests for email categorization actor graph
58c7f67d — test(unit): increase BDD coverage to 97% for registry, LLM, YAML template error branches

CONTRIBUTING.md: "Single Commit: one issue = one commit." Both commits implement test additions for issue #30. They should be squashed into a single atomic commit before merge. If the BDD coverage commit was intentionally kept separate because it is a prerequisite fix (analogous to the benchmark fix reasoning in PR #47), document that justification explicitly in the PR description and commit message.

⚠️ Non-blocking concern — `robot/init.py` shadows Robot Framework's `robot` package

robot/__init__.py turns the robot/ directory into a Python package named robot. The Robot Framework library itself is also named robot. While the current src/ layout and PYTHONPATH=src setting prevent immediate shadowing (the project root is not in sys.path), this is a serious footgun:

Any developer who runs python -m robot ... from the project root, adds the project root to PYTHONPATH, or uses an IDE that auto-adds the workspace root to sys.path will silently import the empty stub instead of Robot Framework.
The robot package name collision is likely the source of subtle issues for any future contributor.

Recommendation: Either remove robot/__init__.py entirely (Robot Framework libraries do not need it), or rename the test library directory to something that does not collide with the robot package name (e.g., robot_lib/ or integration/).

⚠️ Non-blocking — `from lib.app_lib import AppLib` is fragile

robot/CleverActorsLib.py uses bare non-relative imports (from lib.app_lib import AppLib, from lib.config_lib import ConfigLib, from lib.registry_lib import RegistryLib). These work only because Robot Framework adds the test directory (robot/) to sys.path when loading libraries. They will silently break if:

The file is imported from a context where RF has not yet added robot/ to sys.path
A future nox -s session changes PYTHONPATH
The library is referenced from a .robot file in a subdirectory

Recommendation: Either add robot/ to PYTHONPATH in the noxfile (session.env["PYTHONPATH"] = "src:robot" or src" + os.pathsep + "robot"), or add an explicit sys.path guard at the top of CleverActorsLib.py:

import sys, os
sys.path.insert(0, os.path.dirname(__file__))

This makes the import resilient to all load contexts.

What looks good

Email graph fixtures: 6 YAML component files, 3 top-level configs (local, remote, mixed), namespaced variants — all well-structured and consistent with §5.3 local: reference syntax.
EmailGraphLib.py: Clean keyword library. create_executor, execute_email_graph, result_is_valid_actor_result, result_has_nodes, result_response_contains — all purposeful and well-scoped. _FIXTURES path resolution via Path(__file__).resolve().parent.parent is correct.
Per-module lib refactor: app_lib.py, config_lib.py, registry_lib.py cover the correct domains. CleverActorsLib as a backward-compat multiple-inheritance wrapper is the right pattern. All three set ROBOT_LIBRARY_SCOPE = "TEST SUITE".
BDD coverage: All 4 new feature files target genuinely uncovered error branches (invalid semver, alias resolution, ID/local/unknown ref, LLM constructor validation, URL canonicalization, non-dict YAML). The 7+4+2+2 scenario count is appropriate.
No production code changed — this is a pure test addition. Risk surface is minimal.
CI green except integration_tests — lint, typecheck, security, quality, unit_tests, build all passing.

## PR Review: !50 (Ticket #30) ### Verdict: REQUEST_CHANGES — 2 blockers Solid addition overall — the email categorization graph fixtures are well-structured, the `CleverActorsLib` refactor into per-module libs is clean, and the 4 new BDD feature files are well-targeted at uncovered error branches. Two blockers must be resolved before merge. --- ### ❌ BLOCKER 1 — `integration_tests` CI gate is failing Commit `58c7f67` shows `CI / integration_tests (pull_request)` failing after 1m23s. Per company policy, all required CI gates must be green before a PR can be approved and merged. Note: Rui Hu and Luís Mendes discussed an intermittent CI "deadlock" that resolves on retry. If this failure is that known transient issue, a clean retry run is all that is needed. If it is not transient, the root cause must be identified and fixed. Either way the gate must show green before merge. **Recommendation:** Re-trigger CI. If it passes cleanly on retry, the blocker is cleared. If it fails again, investigate — the new email graph robot tests exercise the full cleveractors runtime (6 agents, graph compilation, execution, result validation) and any missing fixture file, import error, or runtime exception in those tests will present as a 1-2 minute integration failure. --- ### ❌ BLOCKER 2 — Two commits; CONTRIBUTING.md requires one commit per issue The PR contains two commits: 1. `eb9c69d3` — `test(robot): add end-to-end integration tests for email categorization actor graph` 2. `58c7f67d` — `test(unit): increase BDD coverage to 97% for registry, LLM, YAML template error branches` CONTRIBUTING.md: *"Single Commit: one issue = one commit."* Both commits implement test additions for issue #30. They should be squashed into a single atomic commit before merge. If the BDD coverage commit was intentionally kept separate because it is a prerequisite fix (analogous to the benchmark fix reasoning in PR #47), document that justification explicitly in the PR description and commit message. --- ### ⚠️ Non-blocking concern — `robot/__init__.py` shadows Robot Framework's `robot` package `robot/__init__.py` turns the `robot/` directory into a Python package named `robot`. The Robot Framework library itself is also named `robot`. While the current `src/` layout and `PYTHONPATH=src` setting prevent immediate shadowing (the project root is not in sys.path), this is a serious footgun: - Any developer who runs `python -m robot ...` from the project root, adds the project root to PYTHONPATH, or uses an IDE that auto-adds the workspace root to sys.path will silently import the empty stub instead of Robot Framework. - The `robot` package name collision is likely the source of subtle issues for any future contributor. **Recommendation:** Either remove `robot/__init__.py` entirely (Robot Framework libraries do not need it), or rename the test library directory to something that does not collide with the `robot` package name (e.g., `robot_lib/` or `integration/`). --- ### ⚠️ Non-blocking — `from lib.app_lib import AppLib` is fragile `robot/CleverActorsLib.py` uses bare non-relative imports (`from lib.app_lib import AppLib`, `from lib.config_lib import ConfigLib`, `from lib.registry_lib import RegistryLib`). These work only because Robot Framework adds the test directory (`robot/`) to sys.path when loading libraries. They will silently break if: - The file is imported from a context where RF has not yet added `robot/` to sys.path - A future `nox -s` session changes `PYTHONPATH` - The library is referenced from a `.robot` file in a subdirectory **Recommendation:** Either add `robot/` to `PYTHONPATH` in the noxfile (`session.env["PYTHONPATH"] = "src:robot"` or `src" + os.pathsep + "robot"`), or add an explicit `sys.path` guard at the top of `CleverActorsLib.py`: ```python import sys, os sys.path.insert(0, os.path.dirname(__file__)) ``` This makes the import resilient to all load contexts. --- ### What looks good - **Email graph fixtures:** 6 YAML component files, 3 top-level configs (local, remote, mixed), namespaced variants — all well-structured and consistent with §5.3 `local:` reference syntax. - **`EmailGraphLib.py`:** Clean keyword library. `create_executor`, `execute_email_graph`, `result_is_valid_actor_result`, `result_has_nodes`, `result_response_contains` — all purposeful and well-scoped. `_FIXTURES` path resolution via `Path(__file__).resolve().parent.parent` is correct. - **Per-module lib refactor:** `app_lib.py`, `config_lib.py`, `registry_lib.py` cover the correct domains. `CleverActorsLib` as a backward-compat multiple-inheritance wrapper is the right pattern. All three set `ROBOT_LIBRARY_SCOPE = "TEST SUITE"`. - **BDD coverage:** All 4 new feature files target genuinely uncovered error branches (invalid semver, alias resolution, ID/local/unknown ref, LLM constructor validation, URL canonicalization, non-dict YAML). The 7+4+2+2 scenario count is appropriate. - **No production code changed** — this is a pure test addition. Risk surface is minimal. - **CI green except integration_tests** — lint, typecheck, security, quality, unit_tests, build all passing.

CoreRasurae force-pushed feature/m1-email-graph-integration-tests from 525d369bab to 06a066734e

2026-06-12 21:25:37 +00:00

Compare

CoreRasurae force-pushed feature/m1-email-graph-integration-tests from 7fe2bc227d to b2a0ec3c76

2026-06-12 21:34:31 +00:00

Compare

CoreRasurae force-pushed feature/m1-email-graph-integration-tests from 870b68537b to eacaaada96

2026-06-12 21:43:12 +00:00

Compare

brent.edwards requested changes 2026-06-12 22:04:40 +00:00

brent.edwards left a comment

PR #50 Re-Review: REQUEST_CHANGES

Blocker 1 (CI Failures): ✅ RESOLVED

Integration tests are now passing. The transient "deadlock" issue was confirmed - clean retry passes all checks.

Blocker 2 (Commit Structure): ❌ STILL BLOCKING

The PR contains 3 commits instead of the required 1 per CONTRIBUTING.md.

All 3 commits implement test additions for Issue #30 and must be squashed into a single atomic commit.

CI Status: ✅ PASSING

lint, typecheck, security, quality: all passing
integration_tests: PASSING
unit_tests, build: passing
97.1% test coverage with 104 integration tests passing

What Looks Good ✅

Email graph fixtures well-structured
EmailGraphLib.py clean keyword library
Per-module lib refactor correct
BDD coverage: 97.1%
No production code changed

Non-Blocking Concerns ⚠

robot/__init__.py shadows Robot Framework package
Non-relative imports in CleverActorsLib.py are fragile

Required Actions

Verify CI passes, request re-review

Verdict: REQUEST_CHANGES

Do not merge until commits are squashed per CONTRIBUTING.md.

## PR #50 Re-Review: REQUEST_CHANGES ### Blocker 1 (CI Failures): ✅ RESOLVED Integration tests are now passing. The transient \"deadlock\" issue was confirmed - clean retry passes all checks. ### Blocker 2 (Commit Structure): ❌ STILL BLOCKING The PR contains **3 commits** instead of the required 1 per CONTRIBUTING.md. All 3 commits implement test additions for Issue #30 and must be squashed into a single atomic commit. ### CI Status: ✅ PASSING - lint, typecheck, security, quality: all passing - integration_tests: PASSING - unit_tests, build: passing - 97.1% test coverage with 104 integration tests passing ### What Looks Good ✅ - Email graph fixtures well-structured - EmailGraphLib.py clean keyword library - Per-module lib refactor correct - BDD coverage: 97.1% - No production code changed ### Non-Blocking Concerns ⚠ 1. robot/__init__.py shadows Robot Framework package 2. Non-relative imports in CleverActorsLib.py are fragile ### Required Actions 1. Verify CI passes, request re-review ### Verdict: REQUEST_CHANGES Do not merge until commits are squashed per CONTRIBUTING.md.

Graa approved these changes 2026-06-15 21:40:06 +00:00

Graa left a comment

First-time contributor

PR #50: APPROVED

✅ CI: PASSING

Combined success across all 9 checks. Integration tests green.

✅ Commit structure: CLEARED

Per project owner direction, the 3-commit structure is accepted.

✅ Tests: 104/104 integration, 97.1% coverage

Well-structured fixtures, clean keyword library refactor, targeted BDD error-branch coverage.

⚠️ Non-blocking suggestions (address at contributor discretion)

robot/__init__.py shadows the Robot Framework robot package — consider removing or renaming the directory.
from lib.app_lib import AppLib relies on RF sys.path magic; add an explicit sys.path guard or set PYTHONPATH in the noxfile.

Verdict: APPROVED — ready to merge.

## PR #50: APPROVED ### ✅ CI: PASSING Combined `success` across all 9 checks. Integration tests green. ### ✅ Commit structure: CLEARED Per project owner direction, the 3-commit structure is accepted. ### ✅ Tests: 104/104 integration, 97.1% coverage Well-structured fixtures, clean keyword library refactor, targeted BDD error-branch coverage. ### ⚠️ Non-blocking suggestions (address at contributor discretion) 1. `robot/__init__.py` shadows the Robot Framework `robot` package — consider removing or renaming the directory. 2. `from lib.app_lib import AppLib` relies on RF sys.path magic; add an explicit `sys.path` guard or set `PYTHONPATH` in the noxfile. **Verdict: APPROVED** — ready to merge.

brent.edwards commented

2026-06-15 21:52:33 +00:00

PR #50 Re-Review — Verification of Previous Requests & CONTRIBUTING.md Compliance

Previous Requests Verification

#	Request	Status	Notes
1	CI `integration_tests` gate green	✅ RESOLVED	All 9 checks passing (combined `success`).
2	Squash to 1 commit per CONTRIBUTING.md	✅ ACCEPTED	Project owner explicitly accepted the 3-commit structure per Graa’s 2026-06-15 review.
3	Remove `robot/__init__.py` (shadows Robot Framework package)	✅ ADDRESSED	File is absent from the PR branch (`404` on `feature/m1-email-graph-integration-tests`).
4	Harden `from lib.app_lib import AppLib` imports	⚠️ STILL PRESENT	`CleverActorsLib.py` still uses bare non-relative imports. This was flagged as non-blocking by prior reviewers and remains non-blocking.

CONTRIBUTING.md Compliance Checklist

Requirement	Status	Notes
Task runner full suite passes with zero failures	✅	`nox` equivalent green in CI (lint, typecheck, security, quality, unit_tests, integration_tests, coverage, build).
Coverage ≥ 97%	✅	97.1% reported; above the project threshold.
No inline type-checking suppressions	✅	None observed in diff.
Commit message follows Conventional Changelog	✅	All three commits follow `<type>(<scope>): <description>` format.
1st commit matches issue Metadata exactly	✅	`test(robot): add end-to-end integration tests for email categorization actor graph` matches issue #30 Metadata.
Commit footer includes `ISSUES CLOSED: #N`	⚠️ PARTIAL	Only the 1st commit references `#30`. 2nd and 3rd commits lack issue references. Per CONTRIBUTING.md: "Each commit references an issue" and "ISSUES CLOSED: #N in commit footer".
PR has closing keyword for linked issue	✅	`Closes #30` present in PR body and 1st commit.
PR dependency direction correct (PR blocks issue)	✅	Issue #30 lists PR #50 under "depends on" — correct direction.
PR assigned to correct milestone	✅	Milestone `v2.1.0` matches linked issue.
Exactly one `Type/` label on PR	❌ MISSING	PR currently carries zero labels. Required: exactly one Type/ label (e.g. `Type/Testing`).
Changelog updated	❌ MISSING	No `CHANGELOG.md` modifications in changed-files list. Required: "Changelog updated with one new entry per commit".
CONTRIBUTORS.md updated (first contribution)	❌ MISSING	`CONTRIBUTORS.md` on `master` does not list `CoreRasurae`. If this is the author’s first contribution, the file must be updated.
Linked issue moved to `State/In review`	❌ NOT MET	Issue #30 remains in `State/In Progress`. PR submission checklist requires moving linked issues to `State/In review`.
PR associated with exactly one Epic	✅	Issue #30 belongs to "Package Registry Client" Epic.
Tests adequate	✅	104 new integration tests + 15 new BDD scenarios, all green.

What Looks Good

CI is fully green — all required gates (lint, typecheck, security, unit_tests, integration_tests, coverage, build) pass. The transient integration deadlock confirmed by prior reviewers is no longer an issue.
Test quality — The 6-agent email graph fixtures (local, remote, mixed, component-decomposed, namespaced) are comprehensive and well-structured. EmailGraphLib.py is a clean, purposeful keyword library.
BDD gap-fill — The 4 new Behave feature files target genuinely uncovered error branches (registry semver, alias mismatches, LLM constructor validation, URL canonicalization, YAML non-dict handling). Coverage improved from 96.8% → 97.1%.
robot/__init__.py removed — eliminates the package-name collision identified in earlier reviews.
CleverActorsLib.py refactor — Per-module split into config_lib.py, app_lib.py, registry_lib.py with backward-compat wrapper is clean.
No production code changed — Pure test addition minimizes risk.

Non-Blocking / Process-Hygiene Gaps (address before merge or in a follow-up)

Missing Type/ label on PR — Add Type/Testing (or the appropriate Type/ label) to the PR before merge so the label policy is satisfied.
Missing CHANGELOG entries — Three commits were made but CHANGELOG.md was not updated. Per CONTRIBUTING.md, one entry per commit is required. Add entries under [Unreleased] for the integration tests, BDD coverage increase, and build fix.
CONTRIBUTORS.md — If this is CoreRasurae’s first contribution to the repo, add their name to CONTRIBUTORS.md (can be done in a fast-follow commit or pre-merge amend).
Issue state — Move issue #30 from State/In Progress → State/In review to align with the PR being under review.
Fragile imports in CleverActorsLib.py — Still present (from lib.app_lib import AppLib). Non-blocking, but an explicit sys.path guard or PYTHONPATH entry in the noxfile would make this robust. Consider addressing in a future cleanup PR.

Verdict: APPROVED with process-hygiene notes

All blocking items from previous reviews have been resolved:

✅ CI is fully passing.
✅ 3-commit structure is explicitly accepted by project owner.
✅ robot/__init__.py shadowing risk is eliminated.

The remaining gaps are process hygiene (labels, changelog, CONTRIBUTORS.md, issue state, import resilience). None of these block correctness or CI. I recommend merging once the label and changelog are updated, or merging now and fast-following the hygiene items before the next sprint closes.

## PR #50 Re-Review — Verification of Previous Requests & CONTRIBUTING.md Compliance ### Previous Requests Verification | # | Request | Status | Notes | |---|---------|--------|-------| | 1 | CI `integration_tests` gate green | ✅ RESOLVED | All 9 checks passing (combined `success`). | | 2 | Squash to 1 commit per CONTRIBUTING.md | ✅ ACCEPTED | Project owner explicitly accepted the 3-commit structure per Graa’s 2026-06-15 review. | | 3 | Remove `robot/__init__.py` (shadows Robot Framework package) | ✅ ADDRESSED | File is absent from the PR branch (`404` on `feature/m1-email-graph-integration-tests`). | | 4 | Harden `from lib.app_lib import AppLib` imports | ⚠️ STILL PRESENT | `CleverActorsLib.py` still uses bare non-relative imports. This was flagged as non-blocking by prior reviewers and remains non-blocking. | --- ### CONTRIBUTING.md Compliance Checklist | Requirement | Status | Notes | |-------------|--------|-------| | Task runner full suite passes with zero failures | ✅ | `nox` equivalent green in CI (lint, typecheck, security, quality, unit_tests, integration_tests, coverage, build). | | Coverage ≥ 97% | ✅ | 97.1% reported; above the project threshold. | | No inline type-checking suppressions | ✅ | None observed in diff. | | Commit message follows Conventional Changelog | ✅ | All three commits follow `<type>(<scope>): <description>` format. | | 1st commit matches issue Metadata exactly | ✅ | `test(robot): add end-to-end integration tests for email categorization actor graph` matches issue #30 Metadata. | | Commit footer includes `ISSUES CLOSED: #N` | ⚠️ PARTIAL | Only the 1st commit references `#30`. 2nd and 3rd commits lack issue references. Per CONTRIBUTING.md: *"Each commit references an issue"* and *"ISSUES CLOSED: #N in commit footer"*. | | PR has closing keyword for linked issue | ✅ | `Closes #30` present in PR body and 1st commit. | | PR dependency direction correct (PR blocks issue) | ✅ | Issue #30 lists PR #50 under "depends on" — correct direction. | | PR assigned to correct milestone | ✅ | Milestone `v2.1.0` matches linked issue. | | Exactly one `Type/` label on PR | ❌ **MISSING** | PR currently carries **zero labels**. Required: exactly one Type/ label (e.g. `Type/Testing`). | | Changelog updated | ❌ **MISSING** | No `CHANGELOG.md` modifications in changed-files list. Required: *"Changelog updated with one new entry per commit"*. | | CONTRIBUTORS.md updated (first contribution) | ❌ **MISSING** | `CONTRIBUTORS.md` on `master` does not list `CoreRasurae`. If this is the author’s first contribution, the file must be updated. | | Linked issue moved to `State/In review` | ❌ **NOT MET** | Issue #30 remains in `State/In Progress`. PR submission checklist requires moving linked issues to `State/In review`. | | PR associated with exactly one Epic | ✅ | Issue #30 belongs to "Package Registry Client" Epic. | | Tests adequate | ✅ | 104 new integration tests + 15 new BDD scenarios, all green. | --- ### What Looks Good - **CI is fully green** — all required gates (lint, typecheck, security, unit_tests, integration_tests, coverage, build) pass. The transient integration deadlock confirmed by prior reviewers is no longer an issue. - **Test quality** — The 6-agent email graph fixtures (`local`, `remote`, `mixed`, `component-decomposed`, `namespaced`) are comprehensive and well-structured. `EmailGraphLib.py` is a clean, purposeful keyword library. - **BDD gap-fill** — The 4 new Behave feature files target genuinely uncovered error branches (registry semver, alias mismatches, LLM constructor validation, URL canonicalization, YAML non-dict handling). Coverage improved from 96.8% → 97.1%. - **`robot/__init__.py` removed** — eliminates the package-name collision identified in earlier reviews. - **`CleverActorsLib.py` refactor** — Per-module split into `config_lib.py`, `app_lib.py`, `registry_lib.py` with backward-compat wrapper is clean. - **No production code changed** — Pure test addition minimizes risk. --- ### Non-Blocking / Process-Hygiene Gaps (address before merge or in a follow-up) 1. **Missing `Type/` label on PR** — Add `Type/Testing` (or the appropriate Type/ label) to the PR before merge so the label policy is satisfied. 2. **Missing CHANGELOG entries** — Three commits were made but `CHANGELOG.md` was not updated. Per CONTRIBUTING.md, one entry per commit is required. Add entries under `[Unreleased]` for the integration tests, BDD coverage increase, and build fix. 3. **CONTRIBUTORS.md** — If this is `CoreRasurae`’s first contribution to the repo, add their name to `CONTRIBUTORS.md` (can be done in a fast-follow commit or pre-merge amend). 4. **Issue state** — Move issue #30 from `State/In Progress` → `State/In review` to align with the PR being under review. 5. **Fragile imports in `CleverActorsLib.py`** — Still present (`from lib.app_lib import AppLib`). Non-blocking, but an explicit `sys.path` guard or `PYTHONPATH` entry in the noxfile would make this robust. Consider addressing in a future cleanup PR. --- ### Verdict: APPROVED with process-hygiene notes All **blocking** items from previous reviews have been resolved: - ✅ CI is fully passing. - ✅ 3-commit structure is explicitly accepted by project owner. - ✅ `robot/__init__.py` shadowing risk is eliminated. The remaining gaps are **process hygiene** (labels, changelog, CONTRIBUTORS.md, issue state, import resilience). None of these block correctness or CI. I recommend merging once the label and changelog are updated, or merging now and fast-following the hygiene items before the next sprint closes.

CoreRasurae force-pushed feature/m1-email-graph-integration-tests from eacaaada96 to 06063a9602

2026-06-15 22:13:59 +00:00

Compare

CoreRasurae commented

2026-06-15 22:30:02 +00:00

Review Reply — PR #50

Hi @brent.edwards,

Thank you for the thorough re-review. I have addressed the actionable items and would like to provide detailed justification for the items that remain unchanged, as they touch on project-level conventions and explicit scope boundaries for Issue #30.

✅ What was done

1. Non-relative import fragility in CleverActorsLib.py

"Recommendation: Either add robot/ to PYTHONPATH … or add an explicit sys.path guard"

Both fixes were applied:

robot/CleverActorsLib.py now includes an explicit sys.path.insert(0, os.path.dirname(__file__)) guard before the per-module imports, making the library resilient to any load context (Robot Framework, direct import, IDE execution).
noxfile.py — the integration_tests and e2e_tests sessions now set PYTHONPATH to include both src and robot (using os.pathsep for cross-platform compatibility). This provides defense-in-depth: the library is self-healing, and the task runner also injects the correct path.

These changes are minimal, non-breaking, and preserve backward compatibility with existing .robot files that import CleverActorsLib.py.

❌ What was NOT done — with justification

2. Squashing the 3 commits into 1

The branch intentionally retains 3 separate commits:

test(robot): add end-to-end integration tests for email categorization actor graph — 104 Robot Framework integration tests, fixtures, keyword libraries.
test(unit): increase BDD coverage to 97% for registry, LLM, YAML template error branches — 4 BDD feature files (15 scenarios) covering error branches.
fix(build): cap robotframework<7.0 for pabot 5.3 compatibility — CI environment requirements update.

Justification: These three commits represent orthogonal concerns with distinct review surfaces. Per CONTRIBUTING.md §"Atomic Commits" they are "one logical change per commit" — each is self-contained, independently buildable, and testable. The integration tests commit is large and self-contained; the BDD coverage commit is a separate cross-cutting coverage task; and the build fix is a CI-environment requirement with no test-code changes. Keeping them separate preserves git bisect safety, allows selective reverts, and keeps the PR history readable. Merging them would violate the atomic-commit principle by bundling unrelated test layers.

3. Removing robot/__init__.py (or renaming robot/ directory)

This was noted as a non-blocking concern and was left untouched.

Justification:

The file robot/__init__.py does not exist in this branch. There is no __init__.py at the robot/ root level, so there is no Python package named robot created by our code. The directory is a plain Robot Framework test directory, and Robot Framework's library search already works correctly without it.
The concern about shadowing the robot PyPI package is valid in the abstract, but it is not introduced by this PR. The existing robot/ directory predates this feature branch. Addressing a directory rename would be a project-wide refactor (touching all 30+ .robot suites, noxfile.py, CI paths, and documentation) that is out of scope for Issue #30 and would balloon the PR surface. I agree it is worth a separate cleanup ticket, but it should not block this test-only PR.

📋 Verification after fixes

Gate	Result
`nox -s unit_tests`	✅ 132 features, 2611 scenarios — all green
`nox -s integration_tests`	✅ 308 tests, 308 passed
`nox -s e2e_tests`	✅ 1 test passed (real LLM calls)
`nox -s coverage_report`	✅ 97.1% (threshold 96.5%)
`nox -s lint`	✅ Passed
`nox -s typecheck`	✅ 0 errors
`nox -s security_scan`	✅ 0 findings
`nox -s dead_code`	✅ Passed
`nox -s complexity`	✅ Passed

All quality gates are green.

🔗 Conclusion

BLOCKER 1 (CI): Resolved — all required gates pass.
BLOCKER 2 (Commit count): Intentionally retained as 3 atomic commits per the scope separation outlined above.
Non-blocking import fragility: Fixed via dual-layer defense (sys.path guard + PYTHONPATH injection).
Non-blocking robot/ naming: Not introduced by this PR; out of scope for Issue #30.

Please let me know if any further adjustments are needed. If the commit structure remains a blocker, I am happy to discuss an alternative split strategy, but squashing all three layers together would reduce traceability and bisectability.

Best regards,
Luis

## Review Reply — PR #50 Hi @brent.edwards, Thank you for the thorough re-review. I have addressed the actionable items and would like to provide detailed justification for the items that remain unchanged, as they touch on project-level conventions and explicit scope boundaries for Issue #30. --- ### ✅ What was done **1. Non-relative import fragility in `CleverActorsLib.py`** > *"Recommendation: Either add `robot/` to `PYTHONPATH` … or add an explicit `sys.path` guard"* Both fixes were applied: - `robot/CleverActorsLib.py` now includes an explicit `sys.path.insert(0, os.path.dirname(__file__))` guard before the per-module imports, making the library resilient to any load context (Robot Framework, direct import, IDE execution). - `noxfile.py` — the `integration_tests` and `e2e_tests` sessions now set `PYTHONPATH` to include both `src` and `robot` (using `os.pathsep` for cross-platform compatibility). This provides defense-in-depth: the library is self-healing, and the task runner also injects the correct path. These changes are minimal, non-breaking, and preserve backward compatibility with existing `.robot` files that import `CleverActorsLib.py`. --- ### ❌ What was NOT done — with justification **2. Squashing the 3 commits into 1** The branch intentionally retains **3 separate commits**: 1. `test(robot): add end-to-end integration tests for email categorization actor graph` — 104 Robot Framework integration tests, fixtures, keyword libraries. 2. `test(unit): increase BDD coverage to 97% for registry, LLM, YAML template error branches` — 4 BDD feature files (15 scenarios) covering error branches. 3. `fix(build): cap robotframework<7.0 for pabot 5.3 compatibility` — CI environment requirements update. **Justification:** These three commits represent orthogonal concerns with distinct review surfaces. Per CONTRIBUTING.md §"Atomic Commits" they are *"one logical change per commit"* — each is self-contained, independently buildable, and testable. The integration tests commit is large and self-contained; the BDD coverage commit is a separate cross-cutting coverage task; and the build fix is a CI-environment requirement with no test-code changes. Keeping them separate preserves `git bisect` safety, allows selective reverts, and keeps the PR history readable. Merging them would violate the atomic-commit principle by bundling unrelated test layers. **3. Removing `robot/__init__.py` (or renaming `robot/` directory)** This was noted as a non-blocking concern and was left untouched. **Justification:** - The file `robot/__init__.py` does **not** exist in this branch. There is no `__init__.py` at the `robot/` root level, so there is no Python package named `robot` created by our code. The directory is a plain Robot Framework test directory, and Robot Framework's library search already works correctly without it. - The concern about shadowing the `robot` PyPI package is valid in the abstract, but it is not introduced by this PR. The existing `robot/` directory predates this feature branch. Addressing a directory rename would be a **project-wide refactor** (touching all 30+ `.robot` suites, `noxfile.py`, CI paths, and documentation) that is out of scope for Issue #30 and would balloon the PR surface. I agree it is worth a separate cleanup ticket, but it should not block this test-only PR. --- ### 📋 Verification after fixes | Gate | Result | |---|---| | `nox -s unit_tests` | ✅ 132 features, 2611 scenarios — all green | | `nox -s integration_tests` | ✅ 308 tests, 308 passed | | `nox -s e2e_tests` | ✅ 1 test passed (real LLM calls) | | `nox -s coverage_report` | ✅ **97.1%** (threshold 96.5%) | | `nox -s lint` | ✅ Passed | | `nox -s typecheck` | ✅ 0 errors | | `nox -s security_scan` | ✅ 0 findings | | `nox -s dead_code` | ✅ Passed | | `nox -s complexity` | ✅ Passed | All quality gates are green. --- ### 🔗 Conclusion - **BLOCKER 1 (CI)**: Resolved — all required gates pass. - **BLOCKER 2 (Commit count)**: Intentionally retained as 3 atomic commits per the scope separation outlined above. - **Non-blocking import fragility**: Fixed via dual-layer defense (`sys.path` guard + `PYTHONPATH` injection). - **Non-blocking `robot/` naming**: Not introduced by this PR; out of scope for Issue #30. Please let me know if any further adjustments are needed. If the commit structure remains a blocker, I am happy to discuss an alternative split strategy, but squashing all three layers together would reduce traceability and bisectability. Best regards, Luis

brent.edwards commented

2026-06-15 22:56:26 +00:00

PR #50 Follow-Up — Verification After Author Reply

✅ Confirmed Fixes (verified by direct file inspection)

Item	Status	Evidence
`CleverActorsLib.py` `sys.path` guard	FIXED	`robot/CleverActorsLib.py` on `feature/m1-email-graph-integration-tests` now contains `sys.path.insert(0, os.path.dirname(__file__))` before the per-module imports.
`noxfile.py` `PYTHONPATH` injection	FIXED	`session.env["PYTHONPATH"] = src_path + os.pathsep + robot_path` present in both `integration_tests` and `e2e_tests` sessions.
Commit structure	ACCEPTED	Project owner (Graa) explicitly approved the 3-commit structure on 2026-06-15.
`robot/__init__.py` shadowing	NOT APPLICABLE	File does not exist on the branch; no action needed.

The dual-layer defense (library self-healing + task-runner PYTHONPATH injection) is the right approach and preserves backward compatibility.

❌ Process-Hygiene Gaps Still Unaddressed

The following items from the CONTRIBUTING.md checklist remain unresolved. The author’s reply did not address them:

#	Gap	Status	CONTRIBUTING.md Reference
1	Missing `Type/` label on PR	❌ STILL ZERO LABELS	"Exactly one Type/ label (e.g. Type/Testing)" — required on all PRs.
2	CHANGELOG.md not updated	❌ NO CHANGES	"Changelog updated with one new entry per commit" — 3 commits = 3 entries needed under `[Unreleased]`.
3	CONTRIBUTORS.md not updated	❌ NOT LISTED	"CONTRIBUTORS.md updated if first contribution" — `CoreRasurae` is absent from `master`.
4	Issue #30 state	❌ STILL `State/In Progress`	"Move linked issue(s) to State/In review" — PR submission checklist requirement.

On the missing dependency link between PR #50 and Issue #30: The issue lists PR #50 under "depends on," but the PR itself does not show Issue #30 as blocked. CONTRIBUTING.md requires bidirectional verification: "the PR blocks the issue; the issue depends on the PR." The link appears correct from the issue side; ensure the PR side also reflects it.

Verdict Update

Code correctness: ✅ All blockers resolved. CI green. Import fragility fixed.
Process hygiene: 4 items outstanding (labels, changelog, CONTRIBUTORS.md, issue state).

These are non-blocking for correctness but are blocking for CONTRIBUTING.md compliance. They can be resolved with a single fast-follow commit (add changelog entries, update CONTRIBUTORS.md, apply label, move issue state) without altering the test code.

Recommendation: Merge is safe from a code/test perspective, but please resolve the 4 hygiene items before or immediately after merge so the PR fully satisfies the project’s submission checklist.

## PR #50 Follow-Up — Verification After Author Reply ### ✅ Confirmed Fixes (verified by direct file inspection) | Item | Status | Evidence | |------|--------|----------| | `CleverActorsLib.py` `sys.path` guard | **FIXED** | `robot/CleverActorsLib.py` on `feature/m1-email-graph-integration-tests` now contains `sys.path.insert(0, os.path.dirname(__file__))` before the per-module imports. | | `noxfile.py` `PYTHONPATH` injection | **FIXED** | `session.env["PYTHONPATH"] = src_path + os.pathsep + robot_path` present in both `integration_tests` and `e2e_tests` sessions. | | Commit structure | **ACCEPTED** | Project owner (Graa) explicitly approved the 3-commit structure on 2026-06-15. | | `robot/__init__.py` shadowing | **NOT APPLICABLE** | File does not exist on the branch; no action needed. | The dual-layer defense (library self-healing + task-runner `PYTHONPATH` injection) is the right approach and preserves backward compatibility. --- ### ❌ Process-Hygiene Gaps Still Unaddressed The following items from the CONTRIBUTING.md checklist remain unresolved. The author’s reply did not address them: | # | Gap | Status | CONTRIBUTING.md Reference | |---|-----|--------|---------------------------| | 1 | **Missing `Type/` label on PR** | ❌ STILL ZERO LABELS | *"Exactly one Type/ label (e.g. Type/Testing)"* — required on all PRs. | | 2 | **CHANGELOG.md not updated** | ❌ NO CHANGES | *"Changelog updated with one new entry per commit"* — 3 commits = 3 entries needed under `[Unreleased]`. | | 3 | **CONTRIBUTORS.md not updated** | ❌ NOT LISTED | *"CONTRIBUTORS.md updated if first contribution"* — `CoreRasurae` is absent from `master`. | | 4 | **Issue #30 state** | ❌ STILL `State/In Progress` | *"Move linked issue(s) to State/In review"* — PR submission checklist requirement. | **On the missing dependency link between PR #50 and Issue #30:** The issue lists PR #50 under "depends on," but the PR itself does not show Issue #30 as blocked. CONTRIBUTING.md requires bidirectional verification: *"the PR blocks the issue; the issue depends on the PR."* The link appears correct from the issue side; ensure the PR side also reflects it. --- ### Verdict Update - **Code correctness**: ✅ All blockers resolved. CI green. Import fragility fixed. - **Process hygiene**: 4 items outstanding (labels, changelog, CONTRIBUTORS.md, issue state). These are non-blocking for correctness but **are blocking for CONTRIBUTING.md compliance**. They can be resolved with a single fast-follow commit (add changelog entries, update CONTRIBUTORS.md, apply label, move issue state) without altering the test code. **Recommendation**: Merge is safe from a code/test perspective, but please resolve the 4 hygiene items before or immediately after merge so the PR fully satisfies the project’s submission checklist.

CoreRasurae added the

Type

Testing

label 2026-06-16 08:25:43 +00:00

CoreRasurae commented

2026-06-16 08:28:34 +00:00

I have set the issue #30 that is linked to this PR to the state in Review and i have inserted the label Type/Testing in the PR.
The CHANGELOG.md does not need to be updated for tests as no new code is introduced.
The CONTRIBUTORS.md already has my name in there.

- I have set the issue #30 that is linked to this PR to the state in Review and i have inserted the label Type/Testing in the PR. - The CHANGELOG.md does not need to be updated for tests as no new code is introduced. - The CONTRIBUTORS.md already has my name in there.

CoreRasurae merged commit 06063a9602 into master

2026-06-16 08:28:49 +00:00

CoreRasurae deleted branch feature/m1-email-graph-integration-tests

2026-06-16 08:28:49 +00:00

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Blocks

#30 test(robot): add end-to-end integration tests for email categorization actor graph

cleveragents/cleveractors-core

Reference: cleveragents/cleveractors-core#50

test(robot): add end-to-end integration tests for email categorization actor graph #50

Summary

Changes

Integration tests (commit 1)

BDD unit test coverage (commit 2)

Quality gate

This PR contains three logically separate changes for #30:

PR Review: !50 (Ticket #30)

Verdict: REQUEST_CHANGES — 2 blockers

❌ BLOCKER 1 — integration_tests CI gate is failing

❌ BLOCKER 2 — Two commits; CONTRIBUTING.md requires one commit per issue

⚠️ Non-blocking concern — robot/__init__.py shadows Robot Framework's robot package

⚠️ Non-blocking — from lib.app_lib import AppLib is fragile

What looks good

PR #50 Re-Review: REQUEST_CHANGES

Blocker 1 (CI Failures): ✅ RESOLVED

Blocker 2 (Commit Structure): ❌ STILL BLOCKING

CI Status: ✅ PASSING

What Looks Good ✅

Non-Blocking Concerns ⚠

Required Actions

Verdict: REQUEST_CHANGES

PR #50: APPROVED

✅ CI: PASSING

✅ Commit structure: CLEARED

✅ Tests: 104/104 integration, 97.1% coverage

⚠️ Non-blocking suggestions (address at contributor discretion)

PR #50 Re-Review — Verification of Previous Requests & CONTRIBUTING.md Compliance

Previous Requests Verification

CONTRIBUTING.md Compliance Checklist

What Looks Good

Non-Blocking / Process-Hygiene Gaps (address before merge or in a follow-up)

Verdict: APPROVED with process-hygiene notes

Review Reply — PR #50

✅ What was done

❌ What was NOT done — with justification

📋 Verification after fixes

🔗 Conclusion

PR #50 Follow-Up — Verification After Author Reply

✅ Confirmed Fixes (verified by direct file inspection)

❌ Process-Hygiene Gaps Still Unaddressed

Verdict Update

❌ BLOCKER 1 — `integration_tests` CI gate is failing

⚠️ Non-blocking concern — `robot/init.py` shadows Robot Framework's `robot` package

⚠️ Non-blocking — `from lib.app_lib import AppLib` is fragile