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

Merged
CoreRasurae merged 3 commits from feature/m1-email-graph-integration-tests into master 2026-06-16 08:28:49 +00:00
Member

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
Graa requested changes 2026-06-12 20:33:31 +00:00
Dismissed
Graa left a comment

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. eb9c69d3test(robot): add end-to-end integration tests for email categorization actor graph
  2. 58c7f67dtest(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
Some checks failed
CI / unit_tests (pull_request) Has started running
CI / lint (pull_request) Successful in 35s
CI / quality (pull_request) Successful in 35s
CI / build (pull_request) Successful in 57s
CI / typecheck (pull_request) Successful in 1m8s
CI / security (pull_request) Successful in 1m9s
CI / integration_tests (pull_request) Failing after 1m17s
CI / coverage (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
to 06a066734e
Some checks failed
CI / lint (pull_request) Successful in 50s
CI / typecheck (pull_request) Successful in 48s
CI / security (pull_request) Successful in 49s
CI / quality (pull_request) Successful in 37s
CI / unit_tests (pull_request) Has started running
CI / build (pull_request) Successful in 41s
CI / integration_tests (pull_request) Failing after 1m8s
CI / coverage (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
2026-06-12 21:25:37 +00:00
Compare
CoreRasurae force-pushed feature/m1-email-graph-integration-tests from 7fe2bc227d
Some checks failed
CI / unit_tests (pull_request) Has started running
CI / quality (pull_request) Successful in 36s
CI / lint (pull_request) Successful in 38s
CI / typecheck (pull_request) Successful in 1m12s
CI / security (pull_request) Successful in 1m12s
CI / integration_tests (pull_request) Has been cancelled
CI / coverage (pull_request) Has been cancelled
CI / build (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
to b2a0ec3c76
Some checks failed
CI / quality (pull_request) Successful in 48s
CI / security (pull_request) Successful in 49s
CI / lint (pull_request) Successful in 50s
CI / typecheck (pull_request) Successful in 50s
CI / integration_tests (pull_request) Failing after 1m7s
CI / build (pull_request) Successful in 33s
CI / unit_tests (pull_request) Successful in 3m5s
CI / coverage (pull_request) Has started running
CI / status-check (pull_request) Has been cancelled
2026-06-12 21:34:31 +00:00
Compare
CoreRasurae force-pushed feature/m1-email-graph-integration-tests from 870b68537b
Some checks failed
CI / lint (pull_request) Successful in 49s
CI / typecheck (pull_request) Successful in 49s
CI / security (pull_request) Has been cancelled
CI / quality (pull_request) Has been cancelled
CI / unit_tests (pull_request) Has been cancelled
CI / integration_tests (pull_request) Has been cancelled
CI / coverage (pull_request) Has been cancelled
CI / build (pull_request) Has been cancelled
CI / status-check (pull_request) Has been cancelled
to eacaaada96
All checks were successful
CI / lint (pull_request) Successful in 53s
CI / typecheck (pull_request) Successful in 1m4s
CI / security (pull_request) Successful in 1m4s
CI / quality (pull_request) Successful in 54s
CI / integration_tests (pull_request) Successful in 1m28s
CI / build (pull_request) Successful in 34s
CI / unit_tests (pull_request) Successful in 3m28s
CI / coverage (pull_request) Successful in 3m10s
CI / status-check (pull_request) Successful in 3s
2026-06-12 21:43:12 +00:00
Compare
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 ⚠

  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.

## 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

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.

## 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.
Member

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 ProgressState/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.

## 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
All checks were successful
CI / lint (pull_request) Successful in 53s
CI / typecheck (pull_request) Successful in 1m4s
CI / security (pull_request) Successful in 1m4s
CI / quality (pull_request) Successful in 54s
CI / integration_tests (pull_request) Successful in 1m28s
CI / build (pull_request) Successful in 34s
CI / unit_tests (pull_request) Successful in 3m28s
CI / coverage (pull_request) Successful in 3m10s
CI / status-check (pull_request) Successful in 3s
to 06063a9602
All checks were successful
CI / lint (pull_request) Successful in 42s
CI / typecheck (pull_request) Successful in 1m11s
CI / security (pull_request) Successful in 1m9s
CI / quality (pull_request) Successful in 36s
CI / build (pull_request) Successful in 1m1s
CI / integration_tests (pull_request) Successful in 1m33s
CI / unit_tests (pull_request) Successful in 3m37s
CI / coverage (pull_request) Successful in 3m16s
CI / status-check (pull_request) Successful in 4s
CI / lint (push) Successful in 55s
CI / typecheck (push) Successful in 1m6s
CI / security (push) Successful in 1m9s
CI / build (push) Successful in 46s
CI / quality (push) Successful in 57s
CI / integration_tests (push) Successful in 1m24s
CI / unit_tests (push) Successful in 3m28s
CI / coverage (push) Successful in 3m25s
CI / status-check (push) Successful in 3s
2026-06-15 22:13:59 +00:00
Compare
Author
Member

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

## 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
Member

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.
Author
Member
  • 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 deleted branch feature/m1-email-graph-integration-tests 2026-06-16 08:28:49 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveractors-core!50
No description provided.