9 Commits

Author	SHA1	Message	Date
HAL9000	09bc5222a5	test(e2e): fix M2 acceptance test LLM provider and actor validation ... - Replace hardcoded gpt-4/openai/gpt-4 references with Resolve LLM Actor keyword so the test falls back to Anthropic when OpenAI is unavailable - Add explicit return-code check (rc==0) for actor registration in Step 2 - Update CHANGELOG.md with entry for PR #11191	2026-06-04 00:33:43 -04:00
freemo	c3ed77a95c	test: restore complete M2 acceptance e2e test ... The M2 acceptance test was incomplete (truncated at line 48). Restore the full test case that exercises the complete M2 milestone: - Actor YAML compilation and registration - Resource registration and project creation - Action creation with custom actor configuration - Full plan lifecycle: use, strategize, execute, diff, apply - Verification of plan status and integrity Exercises real LLM API integration using OpenAI GPT-4 model. ISSUES CLOSED: #10812	2026-06-04 00:24:09 -04:00
freemo	8ea00f5185	fix: restore CI quality tests to passing state (#4175) ... Co-authored-by: Jeffrey Phillips Freeman <the@jeffreyfreeman.me> Co-committed-by: Jeffrey Phillips Freeman <the@jeffreyfreeman.me>	2026-04-08 11:02:14 +00:00
freemo	7966e97326	fix(e2e): add Skip If No LLM Keys to m1 and m2 acceptance tests ... m1_acceptance and m2_acceptance require real LLM API keys but did not call Skip If No LLM Keys at the start of their test cases. Without API keys configured in CI, these tests fail unconditionally rather than gracefully skipping. The Skip If No LLM Keys keyword is defined in common_e2e.resource and already used by other e2e suites (m6, wf04, wf05, wf07, wf12, wf16, wf17, wf18). This fix makes m1 and m2 consistent with that pattern. The e2e_tests CI job was failing before the 3 problematic direct-push commits (see commit 6dfd7e6b35 CI history) — this is a pre-existing issue. However, since #2597 requires all 11 CI gates to pass, we fix it here.	2026-04-04 20:38:16 +00:00
freemo	48cff5cfe0	refactor(cli): rename plan lifecycle-list and lifecycle-apply to match specification ... Renames `plan lifecycle-list` to `plan list` and `plan lifecycle-apply` to `plan apply` to align with the specification's canonical command names. Removes legacy V2 plan commands that occupied those names. - Renamed CLI command registrations from lifecycle-list/lifecycle-apply to list/apply - Removed legacy V2 apply and list commands (~200 lines) - Updated apply shortcut in main.py to delegate to v3 lifecycle - Added defensive null check for plan existence in apply command - Updated 63+ test, doc, and benchmark files for consistency Closes #881 Co-authored-by: Jeffrey Phillips Freeman <the@jeffreyfreeman.me> Co-committed-by: Jeffrey Phillips Freeman <the@jeffreyfreeman.me>	2026-04-02 19:09:04 +00:00
freemo	a9465c4865	fix(cli): derive actor name from config file instead of positional argument ... Remove the <NAME> positional argument from `actor add` command. The actor name is now derived from the `name` field inside the config YAML file, per the specification: `agents actor add (--config|-c) <FILE> [--update]`. Updated all BDD and integration tests to pass name via config file. ISSUES CLOSED: #914	2026-03-30 18:27:59 +00:00
hurui200320	414abb1396	fix(cli): add missing --yes flag to plan apply command (#1127) ... ## Summary Adds the `--yes`/`-y` flag to the `lifecycle-apply` CLI command as required by the specification (`agents plan apply [--yes|-y] <PLAN_ID>`). Without `--yes`, a confirmation prompt now displays before proceeding with the destructive Apply phase. With `--yes`, the apply proceeds immediately without prompting. Closes #932 ## Changes ### Source Code - **`src/cleveragents/cli/commands/plan.py`**: Added `yes: Annotated[bool, typer.Option("--yes", "-y", help="Skip confirmation prompt")] = False` parameter to `lifecycle_apply_plan`. Added `typer.confirm()` prompt before the apply operation, consistent with the pattern used by `rollback_plan`, `correct_plan`, and other destructive commands. - Confirmation prompt text matches spec exactly: `"Apply changes for plan {plan_id}?"` producing `Apply changes for plan <ID>? [y/N]:`. - Fixed redundant plan ID display when `pre_plan` is `None` — now shows `"Apply changes for plan X?"` instead of `"Apply plan X (X)?"`. - Added `except ValueError` handler consistent with sibling commands `lifecycle_execute_plan` and `_lifecycle_apply_with_id`. - Added `except Exception` catch-all handler with `isinstance(e, (typer.Abort, typer.Exit))` re-raise guard, consistent with `lifecycle_execute_plan`. - Moved `PlanPhase` and `ProcessingState` imports to module level per CONTRIBUTING.md §Import Guidelines. ### TDD Tag Removal (Bug Fix Workflow) - **`features/tdd_plan_apply_yes_flag.feature`**: Removed `@tdd_expected_fail` tag (leaving `@tdd_bug` and `@tdd_bug_932` as permanent regression guards). - **`robot/tdd_plan_apply_yes_flag.robot`**: Removed `tdd_expected_fail` tag (leaving `tdd_bug` and `tdd_bug_932`). ### Test Updates Updated all existing `lifecycle-apply` invocations across 17 test/benchmark files to pass `--yes`, since the new confirmation prompt would otherwise abort in non-interactive test environments: - 9 Behave step definition files - 3 Robot Framework helper scripts - 2 Robot Framework e2e acceptance tests - 3 ASV benchmark files (4 invocations: `cli_robot_flow_bench.py` ×2, `m1_sourcecode_smoke_bench.py` ×1, `plan_cli_smoke_bench.py` ×1) ### Confirmation Prompt Tests (New + Strengthened) - **`features/tdd_plan_apply_yes_flag.feature`**: 5 scenarios total: - `lifecycle-apply recognises the --yes long flag` — verifies flag acceptance, prompt suppression, exit code 0, and `apply_plan` was called - `lifecycle-apply recognises the -y short flag` — same as above for short flag - `lifecycle-apply without --yes prompts for confirmation and user declines` — verifies `"Apply cancelled."` message, `exit_code == 0`, and `apply_plan` was NOT called - `lifecycle-apply without --yes prompts for confirmation and user accepts` — verifies prompt appears, `exit_code == 0`, and `apply_plan` was called - `lifecycle-apply catches unexpected exceptions cleanly` — verifies `"Unexpected error"` output, no traceback leak, non-zero exit code (exercises the `except Exception` catch-all) - **`features/steps/tdd_plan_apply_yes_flag_steps.py`**: Refactored step definitions: - `_make_mock_plan` uses `PlanPhase` and `ProcessingState` enum types instead of raw strings - `_make_mock_plan` uses `datetime.now(tz=UTC)` instead of timezone-naive `datetime.now()` - Unified prompt suppression step handles both `--yes` and `-y` via parameterised step pattern - Added `When` step for unexpected error scenario with `RuntimeError` side_effect - Added `Then` step for non-zero exit code assertion - **Feature/Robot documentation**: Updated stale descriptions that said "implementation does not accept --yes" to reflect the flag is now implemented. ### Documentation - **`docs/reference/plan_cli.md`**: Updated `lifecycle-apply` section with: - `### Synopsis` heading with code block - `### Options` table listing `--yes/-y` and `--format/-f` flags - `### Arguments` table listing `PLAN_ID` - Matches the style used by other command sections in the same file ## Review Fixes (Cycle 3 — Luis's review) | ID | Severity | Issue | Resolution | |----|----------|-------|------------| | M1 | Medium | `typer.Abort()` on user decline produces exit code 1 and redundant "Aborted." | Changed to `raise typer.Exit(0)` — consistent with `correct_decision` and legacy `apply` | | M2 | Medium | Missing exit code assertion on decline scenario | Added `And the lifecycle-apply exit code should be 0` to the decline scenario | | M3 | Medium | Spec compliance: "summary of pending changes" not implemented | Deferred — spec example shows summary *after* confirmation, not before; implementation matches spec. Ticket-vs-spec ambiguity noted. | | L1 | Low | Missing `except Exception` catch-all handler | Added catch-all matching `lifecycle_execute_plan` pattern; re-raises `typer.Abort`/`typer.Exit` | | L2 | Low | Documentation description not updated | Expanded description in `plan_cli.md` to explain confirmation prompt and `--yes` | | L3 | Low | Dead code `is not None` guards | Removed both guards — `get_plan()` raises `NotFoundError`, never returns `None` | | I1 | Info | Duplicate `PlanPhase` import | Hoisted import to top of `try` block, eliminating duplicate at old line 2087 | | I2 | Info | `typer.confirm` without explicit `default=False` | Added `default=False` for consistency with sibling commands | | L4 | Low | No test for `--yes` after positional arg | Not addressed — Typer/Click handles both orderings; low risk | | L5 | Low | No test for auto-select + interactive prompt | Not addressed — separate concern outside ticket scope | | I3 | Info | Robot helper only tests flag recognition | By design — noted as informational | ## Review Fixes (Cycle 4 — Self-QA) | ID | Severity | Issue | Resolution | |----|----------|-------|------------| | Major-1 | Major | No test for `except Exception` catch-all handler | Added new scenario `"lifecycle-apply catches unexpected exceptions cleanly"` with `RuntimeError` side_effect; asserts `"Unexpected error"` output, no traceback, non-zero exit | | Minor-2 | Minor | Missing `ValueError` handler inconsistent with siblings | Added `except ValueError as e:` with `"[red]Execution Error:[/red]"` before catch-all, matching `lifecycle_execute_plan` and `_lifecycle_apply_with_id` | | Minor-3 | Minor | Flag scenarios don't verify `apply_plan` called | Added `And the lifecycle-apply should have called apply` to both `--yes` and `-y` scenarios | | Minor-4 | Minor | Stale docstring in Robot helper references `tdd_expected_fail` inversion | Updated to reflect bug is fixed and tests serve as regression guards | | Minor-5 | Minor | `plan_cli.md` lacks Options table for `lifecycle-apply` | Added Synopsis, Options, and Arguments sections matching sibling command style | | Nit-6 | Nit | Duplicated step defs for `--yes` vs `-y` prompt suppression | Unified into single parameterised step `"the lifecycle-apply {flag} output should not contain the confirmation prompt"` | | Nit-7 | Nit | `datetime.now()` timezone-naive | Changed to `datetime.now(tz=UTC)` | | Nit-8 | Nit | `_make_mock_plan` params use `str` instead of enum types | Changed to `PlanPhase` and `ProcessingState` enum types | ## Review Fixes (Cycle 5 — Jeff's approval note) | ID | Severity | Issue | Resolution | |----|----------|-------|------------| | Import-1 | Minor | `PlanPhase`/`ProcessingState` imports inside function body instead of module level | Moved to module-level import per CONTRIBUTING.md §Import Guidelines | ## Known Limitations / Deferred Items - **M3: Ticket AC mentions "summary of pending changes"** but the spec example only shows `"Apply changes for plan <ID>? [y/N]: y"` without a change summary. The implementation shows plan ID only (matching the spec), not a change summary. This is a ticket-vs-spec ambiguity; recommend discussing with ticket author. - **Legacy `apply` command** accepts `--yes` but does not pass it to `_lifecycle_apply_with_id()`. This is a pre-existing issue outside the scope of this ticket. - **`pre_plan is None` branch** has no explicit test. Pre-existing architectural issue; no action taken. ## Quality Gates | Gate | Result | |------|--------| | `nox -s lint` | ✅ passed | | `nox -s typecheck` | ✅ passed (0 errors) | | `nox -s unit_tests` | ✅ passed (471 features, 12,424 scenarios, 0 failures) | | `nox -s integration_tests` | ✅ passed (1,727 tests, 0 failures) | | `nox -s e2e_tests` | ✅ passed (41 tests, 0 failures) | | `nox -s coverage_report` | ✅ passed (≥97% coverage) | Reviewed-on: #1127 Reviewed-by: Jeffrey Phillips Freeman <jeffrey.freeman@cleverthis.com> Co-authored-by: Rui Hu <rui.hu@cleverthis.com> Co-committed-by: Rui Hu <rui.hu@cleverthis.com>	2026-03-26 07:50:09 +00:00
hurui200320	cbf8bcc993	test(e2e): E2E acceptance criteria for M5 (v3.4.0) — ACMS v1 and context scaling (#811) ... ## Summary Add `robot/e2e/m5_acceptance.robot` with **21 zero-mock E2E test cases** (in addition to existing M5 test suite) covering all M5 (v3.4.0) acceptance criteria: 1. **Context Assembly** — add/list/show/clear files in the context pipeline 2. **Context Scaling** — 10,000+ file project setup with simulate plumbing *(structural)* 3. **Context Policy Configuration** — per-view include/exclude paths, file-size limits 4. **Budget Enforcement** — max_file_size / max_total_size constraint storage *(structural)* 5. **Context Analysis** — ACMS pipeline inspect (tier schema) and simulate (JSON schema) *(structural)* 6. **Plan Execution** — real LLM calls via `openai/gpt-4o-mini` (`plan use` + `plan resume`) ### Structural vs. Behavioural Scope Tests in sections 1b–4 that use `project context simulate` or `inspect` are **structural / plumbing validations** — they verify CLI execution, JSON serialization, and stored configuration but do **not** exercise actual ACMS indexing or budget enforcement because the `ContextTierService` is an in-memory singleton that starts empty per CLI process. Each affected test has a `[Documentation]` note explaining this limitation. Behavioural ACMS validation is deferred until the full indexing pipeline is wired. ### Production Bug Fixes | Fix | File | Description | |-----|------|-------------| | `session.flush()` → `session.commit()` | `project_context.py` | Policy changes silently lost on `session.close()` | | `contextlib.suppress` rollback wrapper | `project_context.py` | Prevents rollback failure from masking original commit exception | | Add `session_factory` DI provider | `container.py` | `project context` commands hit `AttributeError` | | `providers.Factory` → `providers.Singleton` | `container.py` | Avoid creating duplicate engines per call | | Add Gemini API key pattern | `redaction.py` | `AIzaSy...` keys now redacted in logs | ### Review Feedback Addressed (Tenth Pass — @CoreRasurae Review #2410) | # | Severity | Finding | Fix | |---|----------|---------|-----| | P3-1 | Medium | "Clear Context" test tautological — never asserts files were present before clearing | Added `Should Contain ${list_before.stdout} config.py` precondition check after `context-load` and before `clear` | | P3-2 | Medium | Policy/budget verification uses substring matching (`Should Contain 262144`) | Replaced with `Extract JSON From Stdout` + `$rv.get('max_file_size') == 262144` parsed JSON assertions using `resolved_view` dict access | | P3-3 | Medium | Plan resume doesn't verify `phase` value, only existence | Added `Should Not Be Equal As Strings ${phase} queued` assertion to verify plan transitioned from queued | | P3-4 | Medium | Plan JSON extraction inconsistency (`rindex` vs `Extract JSON From Stdout`) | Replaced fragile `rindex`-based extraction with `Extract JSON From Stdout` keyword for consistency | | P3-6 | Medium | Context show summary weak content assertions | Added `Should Not Contain` guards against traceback/error output to reject false positives | | P3-8 | Medium | `_SafeSession` singleton may accumulate dirty state after rollback | Changed `_SafeSession.close()` from pure no-op to `real.rollback()` to reset session state between calls | | P3-14 | Medium | No test for `_save_policy_json` rollback path | Added BDD scenario "Save policy rollback re-raises after commit failure" with monkey-patched commit | | P3-15 | Medium | No test for `_save_policy_json` on nonexistent project | Added BDD scenario "Save policy on nonexistent project row updates zero rows" verifying silent 0-row behavior | | P4-1 | Low | Plan resume TRY/EXCEPT swallows assertion details | Moved field assertions outside TRY block; TRY only guards JSON extraction | | P4-2 | Low | `Safe Parse Json Field` logs stale error context | Fixed to track and report both Strategy 1 and Strategy 2 error contexts separately | | P4-4 | Low | SQLite WAL/SHM files not cleaned in regression test | Added cleanup loop for `-wal` and `-shm` suffixes alongside `.db` file | ### Deferred Items (Out of Scope) | ID | Severity | Reason | |----|----------|--------| | P2-1 | High | `execution_environment` silently dropped on subsequent `context set` — pre-existing production code bug in `_write_policy()`, not introduced by this PR | | P2-2 | High | Unhandled `ValidationError` on corrupt policy blob — pre-existing `_read_policy()` code, not changed by this PR | | P2-3 | High | Silent no-op UPDATE when `ns_projects` row missing — pre-existing `_save_policy_json` logic; this PR only changed error handling | | P3-5 | Medium | Structural tests cannot detect regressions — already honestly documented in every affected test's `[Documentation]` block | | P3-7 | Medium | View inheritance/override behavior not tested — nice-to-have, not in ticket acceptance criteria | | P3-9 | Medium | `context_set` double-writes when `execution_environment` set — pre-existing production logic | | P3-10 | Medium | `budget_tokens=0` silently replaced by default (falsy `or`) — pre-existing production code bug | | P3-11 | Medium | `context set` replaces entire view instead of merging — pre-existing design choice | | P3-12 | Medium | GEMINI_API_KEY propagated but potentially unused — security-first: propagating for redaction testing | | P3-13 | Medium | `reset_container()` doesn't dispose Singleton resources — pre-existing container lifecycle issue | | M5 | Medium | `_build_session_factory` engine never disposed — production code architecture, out of scope for testing ticket | | M6 | Medium | Missing `check_same_thread`/`isolation_level` — production code architecture, out of scope for testing ticket | | L1 | Low | `plan resume` not in spec CLI synopsis — informational | | L2 | Low | Context summary assertions depend on exact CLI wording — acceptable stability risk | | L3 | Low | Gemini regex minimum length slightly loose — acceptable security-first trade-off | | L4 | Low | Missing Google OAuth2 credential patterns — out of scope for this PR | | P4-3 | Low | `Run CLI` keyword duplicated — different purpose (uses `${WS}` as default cwd), not a true duplicate | | P4-5–P4-9 | Low | Various additional E2E coverage gaps — nice-to-have, not in ticket acceptance criteria | ### Quality Gates | Gate | Result | |------|--------| | lint | PASS | | typecheck | PASS (0 errors) | | unit_tests | **393/393** features, 11,210 scenarios | | integration_tests | **1,576/1,576** | | e2e_tests | **37/37** (21 M5 + 12 M6 + 2 smoke + 2 M1) | | coverage_report | **97%** (threshold: 97%) | ### Files Changed | File | Change | |------|--------| | `robot/e2e/m5_acceptance.robot` | **NEW** — 21 E2E test cases with honest structural documentation, parsed JSON assertions, prerequisite skip guards on all sections, safe assertion messages | | `robot/e2e/common_e2e.resource` | `on_timeout=kill` + return code checks + safe key evaluation via `os.environ.get` + fixed stale error logging in `Safe Parse Json Field` | | `robot/e2e/m1_acceptance.robot` | `on_timeout=kill` on git log | | `robot/e2e/m2_acceptance.robot` | `on_timeout=kill` + return code checks + safe assertion messages (no stderr embedding) | | `src/cleveragents/application/container.py` | Add `_build_session_factory` + `session_factory` Singleton | | `src/cleveragents/cli/commands/project_context.py` | `flush()` → `commit()` + `contextlib.suppress` rollback | | `src/cleveragents/shared/redaction.py` | Add Gemini API key pattern | | `noxfile.py` | Propagate `GEMINI_API_KEY` in e2e_tests | | `CHANGELOG.md` | 4 entries for #745 | | `features/application_container_coverage_boost.feature` | Updated title + 3 scenarios | | `features/steps/application_container_coverage_boost_steps.py` | Step defs for `_build_session_factory` | | `features/consolidated_security.feature` | 2 Gemini API key redaction scenarios | | `features/project_context_cli_coverage_boost.feature` | `flush()→commit()` regression test + rollback path + nonexistent project tests | | `features/steps/project_context_cli_coverage_boost_steps.py` | Separate engines for regression test + `try/finally` cleanup + `_SafeSession.close()` state reset + rollback/nonexistent test steps + WAL/SHM cleanup | Closes #745 ISSUES CLOSED: #745 Reviewed-on: #811 Reviewed-by: Jeffrey Phillips Freeman <jeffrey.freeman@cleverthis.com> Co-authored-by: Rui Hu <rui.hu@cleverthis.com> Co-committed-by: Rui Hu <rui.hu@cleverthis.com>	2026-03-19 06:53:06 +00:00
freemo	065171f21c	test(e2e): E2E acceptance criteria for M2 (v3.1.0) — actor compiler and LLM integration ... Add Robot Framework E2E test suite robot/e2e/m2_acceptance.robot exercising M2 acceptance criteria with zero mocking. Test creates a temp git repo with sample project files, registers a custom actor via CLI, sets up resource and project, creates an action referencing the actor, and runs the full plan lifecycle (use → execute strategize → execute → diff → apply). Validates actor YAML compilation, skill registry, tool lifecycle, and LLM integration through real CLI invocations with real provider API keys. Uses flexible structural assertions and expected_rc=None for LLM-dependent commands. ISSUES CLOSED: #742	2026-03-15 19:32:44 -04:00