test(runtime): add failing regression test for executor skipping agent-configuration validation (#122) #127
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#124 TDD: Executor never calls AgentFactory.validate_configuration(), so malformed agents.<name> entries fail silently
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!127
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "tdd/m1-executor-validate-configuration"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Companion TDD issue-capture PR for #122 ("Executor never calls
AgentFactory.validate_configuration(), so malformedagents.<name>entries fail silently"). Adds a failing-first Behave regression scenario infeatures/credential_executor_validation.feature, proving thatExecutor.__init__accepts a malformedagents.<name>entry (missing the required"type"key) without raising aConfigurationError, in violation ofdocs/index.md§11 (agent validation MUST be enforced before any agent is instantiated).The scenario constructs an
Executorwith a single-node graph route whose only agent entry has no"type"key, executes it, and asserts aConfigurationErrornaming the offending agent is raised. Verified locally that removing@tdd_expected_failmakes the assertion fail against currentmasterwithAssertionError: Expected error message to contain "Agent 'worker' must specify a type", got 'missing credentials for provider: openai'— confirming the bug is genuinely reproduced (the malformed entry silently defaults to an emptytype: llmagent and fails several layers downstream with an unrelated credentials error).Tagged
@tdd_issue @tdd_issue_122 @tdd_expected_failper this project's TDD bug-fix workflow, so CI's tag-inversion policy treats the expected failure as a pass until the fix lands on abugfix/branch for #122.Verification
nox -s unit_tests: green (3001 scenarios passed, including this one via inversion)nox -s integration_tests(Robot): greennox -s e2e_tests: greennox -s coverage_report: 96.7% (>= 96.5% threshold)nox -s lint/nox -s format -- --check: greennox -s typecheck(Pyright strict): greennox -s security_scan(bandit + semgrep) /nox -s dead_code(vulture): greennox -s complexity: greennox -s benchmark_regression(ASV vs origin/master): no regressionnox -s docs/nox -s build: greenCloses #124
Refs #122
PR Review: !127 (Ticket #124)
Verdict: Request Changes
The PR adds the required failing Behave regression scenario for #122 and is correctly tagged for the TDD workflow. However, the step that builds the malformed
Executordoes not capture construction-time exceptions. Because #122's acceptance criteria explicitly state thatExecutor.__init__should invoke the validation, a fix that validates in__init__will make theGivenstep raiseConfigurationErrordirectly. The@tdd_expected_failhook only invertsAssertionErrorfailures, so the scenario would fail on the bugfix branch even after the tag is removed. Restructuring the step to capture construction exceptions (or deferring executor creation to theWhenstep) is needed before this TDD capture can safely land.Critical Issues
None
Major Issues
features/steps/credential_executor_validation_steps.py, lines 97-131step_executor_malformed_agent_missing_typestep createsExecutor(...)directly and stores it incontext.executor. If the eventual fix for #122 validates agent configuration insideExecutor.__init__(as the issue's acceptance criteria state), construction itself raisesConfigurationError. This exception is not anAssertionError, sofeatures/tdd_expected_fail.pywill not invert it, and the scenario will fail both with and without the@tdd_expected_failtag. The established pattern infeatures/steps/pure_graph_edge_target_validation_steps.pybuilds the config inGiven, then creates the executor and executes inWheninside a try/except that records the exception. Adopting that pattern here would make the test robust regardless of whether validation is placed in__init__,create_executor, or the dispatch path.Minor Issues
features/credential_executor_validation.feature, lines 32-36agents.<name>entry that is "not a mapping, or is missing the required 'type' key". The added scenario only exercises the missing-type case. Consider adding a companion scenario for a non-mapping value (e.g.,worker: "not a dict") to ensure the fix covers both structural violations required bydocs/index.md§11.1.3.Nits
None
Summary
This PR is narrowly focused and follows the TDD workflow correctly: the scenario is tagged
@tdd_issue @tdd_issue_122 @tdd_expected_fail, the assertion fails on current master with anAssertionError(so inversion works), and the steps extend an existing related step file. The major concern is that the test assumes validation will happen during execution rather than construction, which conflicts with #122's stated acceptance criteria. Fixing the step to capture construction exceptions will make the TDD capture usable on any bugfix branch regardless of where the validation is inserted.cce1575f972679df70edThanks for the review — both findings check out against docs/index.md §11.1 and #122/#124, and both are fixed in the amended commit (force-pushed, same TDD-capture commit, not squashed onto anything else):
Major (construction-time exception handling): Confirmed this was a real risk —
Executor.__init__currently never validates, but #122's acceptance criteria require the fix to validate there, so aConfigurationErrorwould previously be raised straight out of the unguardedGivenstep and never reach theThenassertion (which is the only place@tdd_expected_failcan invert anAssertionError). Restructured to build the config inGivenand construct+execute inside a single try/except inWhen, mirroringpure_graph_edge_target_validation_steps.py. Verified locally by temporarily adding avalidate_configuration()call toExecutor.__init__(simulating the eventual #122 fix, then reverted): the scenario now correctly runs through to theThenassertion and passes, with the TDD tag-policy hook correctly flagging that@tdd_expected_failshould be removed — exactly the intended signal for the future bugfix PR.Minor (non-mapping agent entry): Added a second scenario for the non-mapping case (docs/index.md §11.1 item 3 requires both violations to be rejected), asserting the
AgentFactory.validate_configuration()message"Configuration for agent 'worker' must be a dictionary".Both new scenarios were confirmed to fail with a genuine
AssertionErroragainst current master when run without@tdd_expected_fail, andnox -s unit_testsis green (150 features / 3002 scenarios / 13946 steps).2679df70edb6eac8edebPR Review: !127 (Ticket #124)
Verdict: Request Changes
The previous round of feedback has been addressed: construction and execution are now wrapped in the
Whenstep, and a non-mapping scenario was added. However, the non-mapping scenario uses a string value, which the codebase now treats as a valid bare package reference. This means it does not actually exercise the structural violation fromdocs/index.md§11.1.3 and will not produce the asserted error message once Executor is wired to callAgentFactory.validate_configuration(). Fixing the value to a non-dict, non-string type is needed before this TDD capture can land.Critical Issues
None
Major Issues
features/steps/credential_executor_validation_steps.py, line 137"not-a-mapping"is a non-empty string. Per ADR-2037 and existing tests (features/agent_package_references.feature:111andfeatures/steps/agent_modules_steps.py:820-831),AgentFactory.validate_configuration()accepts non-empty strings as bare package references. Consequently, this scenario does not exercise theagents.<name>value that is "not a mapping" structural violation fromdocs/index.md§11.1.3. On current unfixed code it fails with a package-resolution error wrapped asConfigurationError; after the #122 fix simply callsvalidate_configuration(), the string would still pass validation and the expected"Configuration for agent 'worker' must be a dictionary"message would not be produced.["not-a-mapping"]) or integer (e.g.,123), matching the existing pattern infeatures/steps/agent_modules_steps.py:824.Minor Issues
None
Nits
None
Summary
This is a narrowly focused TDD capture PR that follows the project's workflow correctly. The missing-type scenario is solid and the construction-time exception handling is now robust. Once the non-mapping scenario's value is changed to a type that is unambiguously rejected by
validate_configuration(), both regression captures will accurately document the bug for #122.@ -97,0 +134,4 @@}},"agents": {agent_name: "not-a-mapping",This string is treated as a valid bare package reference by
AgentFactory.validate_configuration()(ADR-2037). To exercise theagents.<name>"not a mapping" structural violation, use a non-dict, non-string value such as["not-a-mapping"]or123, matching the pattern infeatures/steps/agent_modules_steps.py:824.b6eac8edebdcca897e73Thanks for catching this — confirmed against ADR-2037 (D-1) and
AgentFactory.validate_configuration()'s current implementation: a non-empty string is accepted as a valid bare package reference, so"not-a-mapping"never exercised theagents.<name>"not a mapping" structural violation fromdocs/index.md§11.1.3.Fixed in the amended commit (force-pushed, same TDD-capture commit, not squashed onto anything else): changed the non-mapping scenario's value from the string
"not-a-mapping"to123, matching the existing precedent infeatures/steps/agent_modules_steps.py:820-824for exercising this exact "neither dict nor package reference" branch.Confirmed locally: both scenarios still fail via a genuine
AssertionErroragainst current master when run without@tdd_expected_fail(non-mapping case now fails with"Failed to create agent 'worker': 'int' object has no attribute 'get'"instead of the previous unrelated string-resolution error), andnox -s unit_testsscoped tofeatures/credential_executor_validation.featureis green (6 scenarios / 14 steps passed).nox -s lintandnox -s typecheckare also green.PR Review: !127 (Ticket #124)
Verdict: Approve
The previous review feedback has been fully addressed. Construction and execution are now wrapped together in the
Whenstep, so the regression scenarios capture aConfigurationErrorregardless of whether the eventual #122 fix validates agent configuration insideExecutor.__init__or later in the dispatch path. The non-mapping scenario now uses an integer (123) instead of a string, correctly avoiding the bare package-reference form accepted byAgentFactory.validate_configuration(). Both scenarios are appropriately tagged@tdd_issue @tdd_issue_122 @tdd_expected_fail, and theirThensteps assert viaassert(raisingAssertionErroronly), so the TDD pass/fail inversion policy will work correctly.Critical Issues
None
Major Issues
None
Minor Issues
None
Nits
None
Summary
This is a focused, well-structured TDD capture PR for #122. It adds two Behave regression scenarios covering both structural violations required by
docs/index.md§11.1: anagents.<name>entry missing the requiredtypekey, and an entry that is not a mapping at all. The step definitions extend the existing executor-validation step file rather than creating a new one, matching the project’s conventions. Once the fix for #122 lands on itsbugfix/branch, these scenarios should be removed from the@tdd_expected_failtag set and will then serve as passing regression guards. Approved.dcca897e73052add3dbc