cleveragents/cleveractors-core

Fork 0

fix(runtime): validate agent configuration in Executor before agent creation #139

Open

CoreRasurae wants to merge 1 commit from bugfix/m1-executor-validate-configuration into master

CoreRasurae commented

2026-08-20 22:19:17 +00:00

Member

Summary

Executor.init / create_executor never called AgentFactory.validate_configuration(), unlike ReactiveCleverAgentsApp.load_configuration(), which already does. A malformed agents. entry (missing "type", or a non-mapping value) silently defaulted to an empty type: llm agent instead of being rejected up front (docs/index.md §11), surfacing only as an unrelated "missing credentials for provider: openai" error several layers into agent creation.

Fixed by validating the agents. section against a throwaway AgentFactory instance at the end of Executor.init, before any agent is created or any credential/provider lookup is attempted. Reuses AgentFactorys existing validate_configuration() rather than duplicating its logic; ReactiveCleverAgentsApps behavior is unchanged.

The issue #122 regression scenario (added in #124/#127, tagged @tdd_issue @tdd_issue_122 @tdd_expected_fail) now passes normally; the @tdd_expected_fail tag has been removed. Its step definitions were updated to capture the ConfigurationError raised during Executor construction (the new, correct failure point) instead of during execute().

Closes #122

Test plan

nox -s lint (ruff check) — green
nox -s format -- --check — green
nox -s typecheck (Pyright strict) — 0 errors
nox -s security_scan (bandit + semgrep) — 0 findings
nox -s dead_code (vulture) — green
nox -s unit_tests (Behave) — 154 features, 3083 scenarios, 14274 steps, 0 failed; issue #122 scenario passes without @tdd_expected_fail
nox -s coverage_report (Slipcover) — 96.9% (threshold 96.5%)
nox -s integration_tests (Robot Framework via pabot) — 362 tests, 0 failed
nox -s benchmark_regression — intentionally not run for this PR

## Summary Executor.__init__ / create_executor never called AgentFactory.validate_configuration(), unlike ReactiveCleverAgentsApp.load_configuration(), which already does. A malformed agents.<name> entry (missing "type", or a non-mapping value) silently defaulted to an empty type: llm agent instead of being rejected up front (docs/index.md §11), surfacing only as an unrelated "missing credentials for provider: openai" error several layers into agent creation. Fixed by validating the agents.<name> section against a throwaway AgentFactory instance at the end of Executor.__init__, before any agent is created or any credential/provider lookup is attempted. Reuses AgentFactorys existing validate_configuration() rather than duplicating its logic; ReactiveCleverAgentsApps behavior is unchanged. The issue #122 regression scenario (added in #124/#127, tagged @tdd_issue @tdd_issue_122 @tdd_expected_fail) now passes normally; the @tdd_expected_fail tag has been removed. Its step definitions were updated to capture the ConfigurationError raised during Executor construction (the new, correct failure point) instead of during execute(). Closes #122 ## Test plan - [x] nox -s lint (ruff check) — green - [x] nox -s format -- --check — green - [x] nox -s typecheck (Pyright strict) — 0 errors - [x] nox -s security_scan (bandit + semgrep) — 0 findings - [x] nox -s dead_code (vulture) — green - [x] nox -s unit_tests (Behave) — 154 features, 3083 scenarios, 14274 steps, 0 failed; issue #122 scenario passes without @tdd_expected_fail - [x] nox -s coverage_report (Slipcover) — 96.9% (threshold 96.5%) - [x] nox -s integration_tests (Robot Framework via pabot) — 362 tests, 0 failed - [ ] nox -s benchmark_regression — intentionally not run for this PR

CoreRasurae added this to the v2.1.0 milestone

2026-08-20 22:19:18 +00:00

CoreRasurae added 1 commit

2026-08-20 22:19:21 +00:00

fix(runtime): validate agent configuration in Executor before agent creation

CI / lint (pull_request) Successful in 48s

Details

CI / security (pull_request) Successful in 1m34s

Details

CI / typecheck (pull_request) Successful in 2m44s

Details

CI / quality (pull_request) Successful in 1m1s

Details

CI / build (pull_request) Successful in 50s

Details

CI / integration_tests (pull_request) Successful in 2m35s

Details

CI / unit_tests (pull_request) Successful in 4m40s

Details

CI / coverage (pull_request) Failing after 14m53s

Details

CI / benchmark (pull_request) Failing after 24m44s

Details

CI / status-check (pull_request) Failing after 6s

Details

bb99f4d05a

Executor.__init__ never called AgentFactory.validate_configuration(),
unlike ReactiveCleverAgentsApp.load_configuration(), which already does.
A malformed agents.<name> entry (missing "type", or a non-mapping value)
silently defaulted to an empty type: llm agent instead of being rejected
up front (docs/index.md §11), surfacing only as an unrelated
"missing credentials for provider: openai" error several layers into
agent creation.

Fixed by validating the agents.<name> section against a throwaway
AgentFactory instance at the end of Executor.__init__, before any agent
is created or any credential/provider lookup is attempted. This reuses
AgentFactory's existing validate_configuration() rather than duplicating
its logic; ReactiveCleverAgentsApp's behavior is unchanged.

Removed the now-obsolete @tdd_expected_fail tag from the issue #122
regression scenario (added in #124) and updated its step definitions to
capture the ConfigurationError raised during Executor construction
instead of during execute().

ISSUES CLOSED: #122

CoreRasurae added the

Type

Bug

label

2026-08-20 22:19:52 +00:00

CoreRasurae added a new dependency

2026-08-20 22:20:30 +00:00

#122 Executor never calls AgentFactory.validate_configuration(), so malformed agents.<name> entries fail silently

hurui200320 approved these changes

2026-08-22 06:21:06 +00:00

hurui200320 left a comment

PR Review: !139 (Ticket #122)

Verdict: Approve

The fix correctly closes the validation gap in Executor.__init__ by calling AgentFactory.validate_configuration() before any agent is instantiated, matching the requirement in issue #122. The implementation reuses existing validation logic, does not perform network I/O or credential lookups during validation, and leaves ReactiveCleverAgentsApp unchanged. The regression scenario now passes without @tdd_expected_fail.

No critical or major issues were found. The remaining items are minor test-coverage completeness and a small comment precision nit.

Critical Issues

None

Major Issues

None

Minor Issues

Executor-level Behave coverage is incomplete for the acceptance criteria
- File: features/credential_executor_validation.feature (lines 30–36)
- Problem: Issue #122’s acceptance criteria ask for Behave scenarios covering (a) missing type, (b) non-mapping agents.<name> value, and (c) typo’d-key case. The PR only updates the existing typo’d-key scenario. While AgentFactory.validate_configuration() itself is tested elsewhere (features/agent_package_references.feature, features/steps/agent_modules_steps.py), the Executor path is not explicitly covered for the missing-type and non-mapping cases.
- Recommendation: Add two small scenarios (or a scenario outline) to features/credential_executor_validation.feature that construct an Executor with agents: {worker: {config: {}}} and agents: {worker: 123}, asserting the same ConfigurationError behavior. This directly satisfies the ticket’s acceptance criteria without relying on indirect coverage.

Nits

Comment overstates the spec section being enforced
- File: src/cleveractors/runtime.py (lines 137–144)
- Problem: The comment references "docs/index.md §11" generally, but the code only enforces the agent-entry structural rules from §11.1/§11.2 (each agents.<name> is a mapping with a known type). It does not enforce route validation, merge/split validation, environment-variable validation, or the requirement that agents itself be present.
- Recommendation: Change the comment to something like "Validate the agents.<name> section eagerly (docs/index.md §11.1–11.2 agent structural validation)" to avoid implying full §11 compliance.

Summary

This is a focused, low-risk bugfix that addresses the root cause described in #122. The implementation is minimal and reuses the existing validator rather than duplicating logic. The test update correctly captures the new failure point during construction. I recommend approving after considering the minor test-coverage expansion and comment tweak, neither of which blocks functionality.

## PR Review: !139 (Ticket #122) ### Verdict: Approve The fix correctly closes the validation gap in `Executor.__init__` by calling `AgentFactory.validate_configuration()` before any agent is instantiated, matching the requirement in issue #122. The implementation reuses existing validation logic, does not perform network I/O or credential lookups during validation, and leaves `ReactiveCleverAgentsApp` unchanged. The regression scenario now passes without `@tdd_expected_fail`. No critical or major issues were found. The remaining items are minor test-coverage completeness and a small comment precision nit. ### Critical Issues None ### Major Issues None ### Minor Issues 1. **Executor-level Behave coverage is incomplete for the acceptance criteria** - **File:** `features/credential_executor_validation.feature` (lines 30–36) - **Problem:** Issue #122’s acceptance criteria ask for Behave scenarios covering (a) missing `type`, (b) non-mapping `agents.<name>` value, and (c) typo’d-key case. The PR only updates the existing typo’d-key scenario. While `AgentFactory.validate_configuration()` itself is tested elsewhere (`features/agent_package_references.feature`, `features/steps/agent_modules_steps.py`), the *Executor* path is not explicitly covered for the missing-type and non-mapping cases. - **Recommendation:** Add two small scenarios (or a scenario outline) to `features/credential_executor_validation.feature` that construct an `Executor` with `agents: {worker: {config: {}}}` and `agents: {worker: 123}`, asserting the same `ConfigurationError` behavior. This directly satisfies the ticket’s acceptance criteria without relying on indirect coverage. ### Nits 1. **Comment overstates the spec section being enforced** - **File:** `src/cleveractors/runtime.py` (lines 137–144) - **Problem:** The comment references "docs/index.md §11" generally, but the code only enforces the agent-entry structural rules from §11.1/§11.2 (each `agents.<name>` is a mapping with a known `type`). It does not enforce route validation, merge/split validation, environment-variable validation, or the requirement that `agents` itself be present. - **Recommendation:** Change the comment to something like "Validate the `agents.<name>` section eagerly (docs/index.md §11.1–11.2 agent structural validation)" to avoid implying full §11 compliance. ### Summary This is a focused, low-risk bugfix that addresses the root cause described in #122. The implementation is minimal and reuses the existing validator rather than duplicating logic. The test update correctly captures the new failure point during construction. I recommend approving after considering the minor test-coverage expansion and comment tweak, neither of which blocks functionality.

CI / lint (pull_request) Successful in 48s

Details

CI / security (pull_request) Successful in 1m34s

Details

CI / typecheck (pull_request) Successful in 2m44s

Details

CI / quality (pull_request) Successful in 1m1s

Details

CI / build (pull_request) Successful in 50s

Details

CI / integration_tests (pull_request) Successful in 2m35s

Details

CI / unit_tests (pull_request) Successful in 4m40s

Details

CI / coverage (pull_request) Failing after 14m53s

Details

CI / benchmark (pull_request) Failing after 24m44s

Details

CI / status-check (pull_request) Failing after 6s

Details

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin bugfix/m1-executor-validate-configuration:bugfix/m1-executor-validate-configuration

git switch bugfix/m1-executor-validate-configuration

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master

git merge --no-ff bugfix/m1-executor-validate-configuration

git switch bugfix/m1-executor-validate-configuration

git rebase master

git switch master

git merge --ff-only bugfix/m1-executor-validate-configuration

git switch bugfix/m1-executor-validate-configuration

git rebase master

git switch master

git merge --no-ff bugfix/m1-executor-validate-configuration

git switch master

git merge --squash bugfix/m1-executor-validate-configuration