freemo/cleveragents-core
0
0
Fork 0
forked from cleveragents/cleveragents-core
Code Pull Requests Activity
1,442 Commits 352 Branches 3 Tags
task/ci-matrix-strategy-python-versions
Commit Graph

5 Commits

Author SHA1 Message Date
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: cleveragents/cleveragents-core#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: cleveragents/cleveragents-core#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