cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.0k Pull Requests 115 Actions Packages Projects Releases Wiki Activity

test: add TDD bug-capture test for #1038 — validation add --required flag #1133

Merged
brent.edwards merged 2 commits from tdd/m5-validation-required-flag into master 2026-03-28 18:09:28 +00:00
Conversation 4 Commits 2 Files Changed 5
+707
brent.edwards commented 2026-03-23 12:36:22 +00:00
Member
Copy Link
Copy Source

Summary

Adds TDD bug-capture tests for bug #1038 — the agents validation add command is missing --required/--informational flags that the specification describes at line 22334 and in numerous walkthrough examples.

Behave BDD Tests

Four Behave BDD scenarios verify the correct expected behavior:

  1. --required flag is accepted — invokes validation add --config <file> --required and asserts exit code 0 and mode "required" in output.
  2. --informational flag is accepted — same pattern with --informational.
  3. --required overrides YAML config mode — creates a YAML config with mode: informational, invokes with --required, and asserts the CLI output and service layer both reflect mode: "required".
  4. --informational overrides YAML config mode — creates a YAML config with mode: required, invokes with --informational, and asserts the CLI output and service layer both reflect mode: "informational".

Robot Framework Integration Tests

Four Robot Framework integration tests exercise the same CLI paths as the Behave scenarios via a subprocess helper script:

  1. TDD Validation Add Required Flag Accepted — verifies --required is recognised by the CLI.
  2. TDD Validation Add Informational Flag Accepted — verifies --informational is recognised.
  3. TDD Validation Add Required Flag Overrides YAML Config — verifies --required overrides a YAML config with mode: informational.
  4. TDD Validation Add Informational Flag Overrides YAML Config — verifies --informational overrides a YAML config with mode: required.

The helper script (robot/helper_tdd_validation_required_flag.py) uses typer.testing.CliRunner with mocked services, following the same pattern as other TDD Robot helpers (e.g., helper_tdd_plan_apply_yes_flag.py).

Tags

All scenarios (Behave and Robot) are tagged @tdd_bug @tdd_bug_1038 @tdd_expected_fail. Because the bug is still present, the underlying assertions fail (proving the bug exists) and the @tdd_expected_fail tag inverts the result so CI passes. Once bug #1038 is fixed, the tag must be removed.

Spec Contradiction Note

Rui Hu's investigation (issue #1038 comment #70755) found that the formal CLI reference (specification.md lines 9279–9290) does NOT include --required/--informational flags — they appear only in walkthrough examples and specification.md line 22334. Additionally, specification.md line 30761 states: "For entity registration commands (actor add, skill add, tool add, validation add, …), the YAML configuration file is the sole source of truth — the --config file fully defines the entity and no CLI override flags are accepted." The resolution may be to add the flags to the CLI OR to clean up the spec. This is documented in both the feature file header and step definition module docstring.

Key Design Decisions

  • YAML configs use flat mode key — matches what Validation.from_config() actually reads (config.get("mode", "required")). The specification's Validation Configuration schema (specification.md line 34109) nests mode under a validation key, but the current from_config() reads a flat top-level mode key. A comment in the step definitions documents this discrepancy.
  • Mock uses side_effect = lambda v: v — the register_tool mock returns the actual Validation object passed by the CLI, so the CLI output reflects real CLI processing rather than hard-coded mock configuration. This prevents false positives where the mock masks an incomplete fix.
  • False-positive guard — the Then step verifying mode checks both CLI output (via f"mode: {expected_mode}" prefix match) AND register_tool call args to prevent the mock from masking an incomplete fix.
  • Robot helper follows established pattern — uses typer.testing.CliRunner with mocked _get_tool_registry_service, matching the pattern from helper_tdd_plan_apply_yes_flag.py. Reports real outcome (exit 0 + sentinel on bug fixed, exit 1 on bug present) and relies on tdd_expected_fail_listener for result inversion.
  • Symmetric override scenarios — both directions are tested: --required overriding informational YAML config, and --informational overriding required YAML config.
  • Positional NAME omitted — the bug report includes a positional NAME argument, but that is a separate spec inconsistency not under test here.
  • Mutual exclusivity deferred — testing what happens when both --required and --informational are passed simultaneously is deferred to the bug-fix PR for #1038.

Closes #1102

## Summary Adds TDD bug-capture tests for bug #1038 — the `agents validation add` command is missing `--required`/`--informational` flags that the specification describes at line 22334 and in numerous walkthrough examples. ### Behave BDD Tests Four Behave BDD scenarios verify the correct expected behavior: 1. **`--required` flag is accepted** — invokes `validation add --config <file> --required` and asserts exit code 0 and mode `"required"` in output. 2. **`--informational` flag is accepted** — same pattern with `--informational`. 3. **`--required` overrides YAML config mode** — creates a YAML config with `mode: informational`, invokes with `--required`, and asserts the CLI output and service layer both reflect `mode: "required"`. 4. **`--informational` overrides YAML config mode** — creates a YAML config with `mode: required`, invokes with `--informational`, and asserts the CLI output and service layer both reflect `mode: "informational"`. ### Robot Framework Integration Tests Four Robot Framework integration tests exercise the same CLI paths as the Behave scenarios via a subprocess helper script: 1. **TDD Validation Add Required Flag Accepted** — verifies `--required` is recognised by the CLI. 2. **TDD Validation Add Informational Flag Accepted** — verifies `--informational` is recognised. 3. **TDD Validation Add Required Flag Overrides YAML Config** — verifies `--required` overrides a YAML config with `mode: informational`. 4. **TDD Validation Add Informational Flag Overrides YAML Config** — verifies `--informational` overrides a YAML config with `mode: required`. The helper script (`robot/helper_tdd_validation_required_flag.py`) uses `typer.testing.CliRunner` with mocked services, following the same pattern as other TDD Robot helpers (e.g., `helper_tdd_plan_apply_yes_flag.py`). ### Tags All scenarios (Behave and Robot) are tagged `@tdd_bug @tdd_bug_1038 @tdd_expected_fail`. Because the bug is still present, the underlying assertions fail (proving the bug exists) and the `@tdd_expected_fail` tag inverts the result so CI passes. Once bug #1038 is fixed, the tag must be removed. ### Spec Contradiction Note Rui Hu's investigation (issue #1038 comment #70755) found that the formal CLI reference (specification.md lines 9279–9290) does NOT include `--required`/`--informational` flags — they appear only in walkthrough examples and specification.md line 22334. Additionally, specification.md line 30761 states: "For entity registration commands (`actor add`, `skill add`, `tool add`, `validation add`, …), the YAML configuration file is the sole source of truth — the `--config` file fully defines the entity and no CLI override flags are accepted." The resolution may be to add the flags to the CLI OR to clean up the spec. This is documented in both the feature file header and step definition module docstring. ### Key Design Decisions - **YAML configs use flat `mode` key** — matches what `Validation.from_config()` actually reads (`config.get("mode", "required")`). The specification's Validation Configuration schema (specification.md line 34109) nests mode under a `validation` key, but the current `from_config()` reads a flat top-level `mode` key. A comment in the step definitions documents this discrepancy. - **Mock uses `side_effect = lambda v: v`** — the `register_tool` mock returns the actual Validation object passed by the CLI, so the CLI output reflects real CLI processing rather than hard-coded mock configuration. This prevents false positives where the mock masks an incomplete fix. - **False-positive guard** — the Then step verifying mode checks both CLI output (via `f"mode: {expected_mode}"` prefix match) AND `register_tool` call args to prevent the mock from masking an incomplete fix. - **Robot helper follows established pattern** — uses `typer.testing.CliRunner` with mocked `_get_tool_registry_service`, matching the pattern from `helper_tdd_plan_apply_yes_flag.py`. Reports real outcome (exit 0 + sentinel on bug fixed, exit 1 on bug present) and relies on `tdd_expected_fail_listener` for result inversion. - **Symmetric override scenarios** — both directions are tested: `--required` overriding informational YAML config, and `--informational` overriding required YAML config. - **Positional NAME omitted** — the bug report includes a positional NAME argument, but that is a separate spec inconsistency not under test here. - **Mutual exclusivity deferred** — testing what happens when both `--required` and `--informational` are passed simultaneously is deferred to the bug-fix PR for #1038. Closes #1102
brent.edwards added this to the v3.5.0 milestone 2026-03-23 12:36:28 +00:00
brent.edwards added the
Type
Testing
 label 2026-03-23 12:36:29 +00:00
brent.edwards force-pushed tdd/m5-validation-required-flag from 2b7f239bea to 6fff40a2e7 2026-03-23 17:14:04 +00:00 Compare
brent.edwards force-pushed tdd/m5-validation-required-flag from 6fff40a2e7 to 982f46c35c 2026-03-23 18:05:53 +00:00 Compare
freemo requested changes 2026-03-24 15:24:46 +00:00
Dismissed
freemo left a comment
Owner
Copy Link
Copy Source

Review: REQUEST CHANGES

The Behave tests are excellent — 4 well-designed scenarios covering --required, --informational, and override-mode edge cases. The TDD tags are all correct (@tdd_bug @tdd_bug_1038 @tdd_expected_fail), step definitions are fully implemented with real assertions (not placeholders), and the feature file has a thorough 30-line header comment documenting the spec contradiction. This is high-quality test work.

Blocking Issue

Missing Robot Framework integration tests. This is the only TDD bug-capture PR in the current batch that does not include a .robot file + helper script. Per CONTRIBUTING.md §BDD Framework: integration tests use Robot Framework under robot/. Per §Testing Philosophy: "Every coding task must include or update tests at multiple levels: unit tests, integration tests, and performance benchmarks."

All other recent TDD PRs (e.g., #1148, #1144, #1110, #1113, #1109, #1130) include both Behave and Robot tests. This PR should follow the same pattern for consistency and to satisfy the multi-level testing mandate.

Action Item

  • Add a Robot Framework test file (robot/tdd_validation_required_flag.robot) and corresponding helper script that exercises the validation add --required CLI path via subprocess, with matching TDD tags.

Everything else about this PR is solid — once the Robot tests are added, this is ready to approve.

## Review: REQUEST CHANGES The Behave tests are excellent — 4 well-designed scenarios covering `--required`, `--informational`, and override-mode edge cases. The TDD tags are all correct (`@tdd_bug @tdd_bug_1038 @tdd_expected_fail`), step definitions are fully implemented with real assertions (not placeholders), and the feature file has a thorough 30-line header comment documenting the spec contradiction. This is high-quality test work. ### Blocking Issue **Missing Robot Framework integration tests.** This is the only TDD bug-capture PR in the current batch that does not include a `.robot` file + helper script. Per CONTRIBUTING.md §BDD Framework: integration tests use Robot Framework under `robot/`. Per §Testing Philosophy: *"Every coding task must include or update tests at multiple levels: unit tests, integration tests, and performance benchmarks."* All other recent TDD PRs (e.g., #1148, #1144, #1110, #1113, #1109, #1130) include both Behave and Robot tests. This PR should follow the same pattern for consistency and to satisfy the multi-level testing mandate. ### Action Item - [ ] Add a Robot Framework test file (`robot/tdd_validation_required_flag.robot`) and corresponding helper script that exercises the `validation add --required` CLI path via subprocess, with matching TDD tags. Everything else about this PR is solid — once the Robot tests are added, this is ready to approve.
brent.edwards commented 2026-03-24 23:34:47 +00:00
Author
Member
Copy Link
Copy Source

Self-QA status: QA process was started but interrupted before completing. The PR has not yet been reviewed by the self-QA loop. Manual review recommended.

**Self-QA status:** QA process was started but interrupted before completing. The PR has not yet been reviewed by the self-QA loop. Manual review recommended.
brent.edwards force-pushed tdd/m5-validation-required-flag from 982f46c35c to d81f783917 2026-03-25 01:46:59 +00:00 Compare
brent.edwards force-pushed tdd/m5-validation-required-flag from b11d41386e to 54c748f5c4 2026-03-25 21:58:23 +00:00 Compare
brent.edwards commented 2026-03-25 22:00:01 +00:00
Author
Member
Copy Link
Copy Source

Review Response

@freemo — thanks for the thorough review and the good catch on the missing Robot tests. You're right that this was the only TDD bug-capture PR without .robot coverage.

Addressed: Missing Robot Framework integration tests ✅

Added robot/tdd_validation_required_flag.robot and robot/helper_tdd_validation_required_flag.py with four test cases exercising the same CLI paths as the Behave scenarios:

Robot Test Case Behave Scenario Helper Subcommand
TDD Validation Add Required Flag Accepted --required flag is accepted check-required
TDD Validation Add Informational Flag Accepted --informational flag is accepted check-informational
TDD Validation Add Required Flag Overrides YAML Config --required overrides YAML config mode check-required-overrides-config
TDD Validation Add Informational Flag Overrides YAML Config --informational overrides YAML config mode check-informational-overrides-config

All test cases are tagged tdd_bug, tdd_bug_1038, tdd_expected_fail. The helper follows the established pattern from helper_tdd_plan_apply_yes_flag.py — reports the real outcome and relies on tdd_expected_fail_listener for result inversion.

All 4 new Robot tests pass via the expected-fail mechanism (underlying assertions fail correctly, confirming the bug exists).

Quality Gates

  • lint: ✅ | typecheck: ✅ | unit_tests: ✅ | integration_tests: ✅ (new tests pass; pre-existing failures only) | coverage: 98%

Branch rebased onto latest origin/master and force-pushed.

## Review Response @freemo — thanks for the thorough review and the good catch on the missing Robot tests. You're right that this was the only TDD bug-capture PR without `.robot` coverage. ### Addressed: Missing Robot Framework integration tests ✅ Added `robot/tdd_validation_required_flag.robot` and `robot/helper_tdd_validation_required_flag.py` with four test cases exercising the same CLI paths as the Behave scenarios: | Robot Test Case | Behave Scenario | Helper Subcommand | |---|---|---| | TDD Validation Add Required Flag Accepted | `--required` flag is accepted | `check-required` | | TDD Validation Add Informational Flag Accepted | `--informational` flag is accepted | `check-informational` | | TDD Validation Add Required Flag Overrides YAML Config | `--required` overrides YAML config mode | `check-required-overrides-config` | | TDD Validation Add Informational Flag Overrides YAML Config | `--informational` overrides YAML config mode | `check-informational-overrides-config` | All test cases are tagged `tdd_bug`, `tdd_bug_1038`, `tdd_expected_fail`. The helper follows the established pattern from `helper_tdd_plan_apply_yes_flag.py` — reports the real outcome and relies on `tdd_expected_fail_listener` for result inversion. All 4 new Robot tests pass via the expected-fail mechanism (underlying assertions fail correctly, confirming the bug exists). ### Quality Gates - `lint`: ✅ | `typecheck`: ✅ | `unit_tests`: ✅ | `integration_tests`: ✅ (new tests pass; pre-existing failures only) | `coverage`: 98% Branch rebased onto latest `origin/master` and force-pushed.
freemo approved these changes 2026-03-27 17:12:36 +00:00
Dismissed
freemo left a comment
Owner
Copy Link
Copy Source

Review: test: add TDD bug-capture test for #1038 — validation add --required flag

Approved. Clean TDD bug-capture test with proper conventional commit format, @tdd_expected_fail tags, and issue reference.

## Review: test: add TDD bug-capture test for #1038 — validation add --required flag **Approved.** Clean TDD bug-capture test with proper conventional commit format, `@tdd_expected_fail` tags, and issue reference.
brent.edwards force-pushed tdd/m5-validation-required-flag from 54c748f5c4 to 55fd37c54a 2026-03-28 02:41:01 +00:00 Compare
brent.edwards dismissed freemo's review 2026-03-28 02:41:01 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

brent.edwards force-pushed tdd/m5-validation-required-flag from 45bcbc457e to 1507425240 2026-03-28 05:55:08 +00:00 Compare
brent.edwards scheduled this pull request to auto merge when all checks succeed 2026-03-28 05:55:59 +00:00
brent.edwards merged commit 3abf25f17f into master 2026-03-28 18:09:28 +00:00
brent.edwards deleted branch tdd/m5-validation-required-flag 2026-03-28 18:09:28 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.
 controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.
 overdue
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.
 auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.
 auto/claimed-implementer
Currently being processed by an implementer worker.
 auto/claimed-merge
Currently being processed by the merge driver.
 auto/claimed-reviewer
Currently being processed by a reviewer worker.
 auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.
 auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.
 auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.
 auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.
 auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.
 Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.
 auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.
 auto/needs-implementer
Failing CI needs implementer attention.
 auto/postmortem
Documenting a driver incident or rollback.
 auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.
 auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.
 auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.
 auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.
 auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.
 auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.
 Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.
Bounty
$100
A bounty of $100 for any open-source contributor who provides a MR that solves this issue
Bounty
$1000
A bounty of $1000 for any open-source contributor who provides a MR that solves this issue
Bounty
$10000
A bounty of $10000 for any open-source contributor who provides a MR that solves this issue
Bounty
$20
A bounty of $20 for any open-source contributor who provides a MR that solves this issue
Bounty
$2000
A bounty of $2000 for any open-source contributor who provides a MR that solves this issue
Bounty
$250
A bounty of $250 for any open-source contributor who provides a MR that solves this issue
Bounty
$50
A bounty of $50 for any open-source contributor who provides a MR that solves this issue
Bounty
$500
A bounty of $500 for any open-source contributor who provides a MR that solves this issue
Bounty
$5000
A bounty of $5000 for any open-source contributor who provides a MR that solves this issue
Bounty
$750
A bounty of $750 for any open-source contributor who provides a MR that solves this issue
MoSCoW
Could have
Could have feature in order to satisfy the epic/legendary.
MoSCoW
Must have
Must have feature in order to satisfy the epic/legendary.
MoSCoW
Should have
Should have feature in order to satisfy the epic/legendary.
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.
Points
1
1 man-hours worth of work for an expert with no learning curve.
Points
13
13 man-hours worth of work for an expert with no learning curve.
Points
2
2 man-hours worth of work for an expert with no learning curve.
Points
21
21 man-hours worth of work for an expert with no learning curve.
Points
3
3 man-hours worth of work for an expert with no learning curve.
Points
34
34 man-hours worth of work for an expert with no learning curve.
Points
5
5 man-hours worth of work for an expert with no learning curve.
Points
55
55 man-hours worth of work for an expert with no learning curve.
Points
8
8 man-hours worth of work for an expert with no learning curve.
Points
88
88 man-hours worth of work for an expert with no learning curve.
Priority
Backlog
This ticket has backlogged priority and is not to be worked on yet
Priority
CI Blocker
Critical priority issue that blocks CI/CD pipeline and prevents PR merges
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
 Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.
State
Completed
The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.
State
Duplicate
A ticket that represents the same content as an existing ticket.
State
In Progress
A ticket that is actively being developed.
State
In Review
A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.
State
Paused
This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.
State
Unverified
All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.
State
Verified
The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.
State
Wont Do
This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.
Type
Automation
Any edits or discussion about the AI automated coding system.
Type
Bug
Something that doesnt work as intended.
Type
Discussion
Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.
Type
Documentation
An error or improvement needed in the documentation.
Type
Epic
Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.
Type
Feature
Some new functionality not present.
Type
Legendary
A type of Epic which will contain other Epics.
Type
Refactor
A code change that restructures existing code without changing its external behavior.
Type
Support
Someone needs help using the project.
Type
Task
A generic task that doesnt fit into the other type categories.
Type
Testing
Work exclusively focusing on fixing or expanding testing.
No Label
Type
Testing
Milestone
No items
No Milestone v3.5.0
Projects
Clear projects
No project
Assignees
Clear assignees
aditya (Aditya Chhabra) aleenaumair (Aleena Umair) brent.edwards (Brent Edwards) CoreRasurae (Luis Mendes) drew (Drew Morris) eugen.thaci (Eugen Thaci) freemo (Jeffrey Phillips Freeman) HAL9000 (HAL 9000) HAL9001 (HAL9001) hamza.khyari (Hamza Khyari) hurui200320 (Rui Hu) justin.morris khird (Kyle Hird) org.cleveragents
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: cleveragents/cleveragents-core#1133
Delete Branch "tdd/m5-validation-required-flag"

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?