hurui200320
48ecf4c00c
CI / lint (push) Successful in 19s
CI / build (push) Successful in 29s
CI / quality (push) Successful in 29s
CI / security (push) Successful in 43s
CI / typecheck (push) Successful in 54s
CI / benchmark-regression (push) Has been skipped
CI / unit_tests (push) Successful in 2m53s
CI / integration_tests (push) Successful in 3m39s
CI / docker (push) Successful in 56s
CI / e2e_tests (push) Successful in 3m53s
CI / coverage (push) Successful in 6m37s
CI / benchmark-publish (push) Successful in 22m2s
## Summary
Adds the missing `--execution-env-priority` flag to the `agents plan use` command, aligning the CLI with the specification (spec line 12501). The flag accepts `fallback` (default) or `override` and controls execution environment routing precedence per ADR-043:
- **`override`**: The specified execution environment always wins, bypassing devcontainer auto-detection.
- **`fallback`**: The specified environment defers to auto-detected devcontainers or project-level overrides.
### Changes
- **Domain model** (`cleveragents.domain.models.core.plan`):
- Added `ExecutionEnvPriority` StrEnum with `FALLBACK`/`OVERRIDE` values.
- Changed `execution_env_priority` field type to `ExecutionEnvPriority | None` (leverages Pydantic enum validation).
- Added `@model_validator` enforcing that `execution_env_priority` requires `execution_environment` (domain-level fail-fast invariant). Verified `validate_assignment=True` is set on `Plan.model_config`.
- Updated `Plan.as_cli_dict()` to include `execution_environment` and `execution_env_priority`, with fallback default of `"fallback"` for pre-migration data where `execution_env_priority` is `None`.
- **CLI** (`cleveragents.cli.commands.plan`):
- Added `--execution-env-priority` parameter to `use_action`.
- Validation: priority requires `--execution-environment`, enum value validation, case-insensitive input.
- Defaults to `"fallback"` when `--execution-environment` is set without explicit priority.
- Updated `_print_lifecycle_plan` and `_plan_spec_dict` to display the priority, defaulting to `"fallback"` for pre-migration data.
- Updated `_plan_spec_dict` docstring to mention new keys.
- Hoisted `ExecutionEnvPriority` import to function-entry deferred imports in both `_plan_spec_dict` and `_print_lifecycle_plan` (no longer conditional on execution environment being set).
- Guarded `service.save_plan(plan)` with `has_overrides` flag so it is only called when CLI overrides were actually applied.
- **Persistence** (`cleveragents.infrastructure.database`):
- Added `execution_environment` (`String(255)`, nullable) and `execution_env_priority` (`String(20)`, nullable) columns to `LifecyclePlanModel`.
- Updated `from_domain()`/`to_domain()` for round-trip serialization including `ExecutionEnvPriority` enum reconstruction. Uses direct attribute access (`plan.execution_environment`) instead of defensive `getattr` — Plan fields always exist on the Pydantic BaseModel.
- Updated `LifecyclePlanRepository.update()` to persist both fields using direct attribute access.
- Added Alembic migration `m4_003_plan_env_columns` adding both columns to the `v3_plans` table. Uses `String(255)` for `execution_environment` to accommodate namespaced resource names. Descends from `m6_005_profile_guards_json`.
- **Service** (`cleveragents.application.services.plan_lifecycle_service`):
- Added `save_plan()` public convenience method for callers that need to re-persist after post-creation mutations.
- **Tests**:
- 18 Behave scenarios covering:
- CLI acceptance criteria (valid values, defaults, validation errors, output display, case-insensitive input, service invocation with `call_args` verification).
- Domain model validator invariant: construction with priority but no environment raises `ValueError`; construction with both fields succeeds.
- `ExecutionEnvPriority` enum: values verification, `StrEnum` subclass assertion.
- `Plan.as_cli_dict()`: includes both fields when set, omits both when `None`, defaults priority to `"fallback"` for pre-migration data.
- DB round-trip serialization: `from_domain()` → `to_domain()` preserves both fields; preserves `None` values.
- 5 Robot Framework integration tests.
- Updated pre-existing `SimpleNamespace`-based plan test fixtures in `database_models_lifecycle_coverage_steps`, `database_models_new_coverage_steps`, `database_models_coverage_r2_steps`, and `repositories_error_handling_coverage_steps` to include `execution_environment` and `execution_env_priority` attributes.
- Simplified Robot helper `sys.path` pattern to standard approach.
- **Changelog**: Updated per CONTRIBUTING.md requirements.
### Review Fixes (Brent Edwards, Review #2384)
- **P2 #1 — Defensive `getattr`**: Replaced `getattr(plan, "execution_environment", None)` with direct `plan.execution_environment` access in both `LifecyclePlanModel.from_domain()` and `LifecyclePlanRepository.update()`. Plan is a Pydantic BaseModel with `default=None`, so the field always exists. Updated 4 pre-existing `SimpleNamespace`-based test fixtures to include the new attributes.
- **P2 #2 — Late conditional import**: Hoisted `ExecutionEnvPriority` import from inside conditional blocks to function-entry deferred imports in both `_plan_spec_dict` and `_print_lifecycle_plan`.
- **P3 — Migration naming**: Acknowledged as deferred (cosmetic only). Updated `m4_003` to descend from `m6_005_profile_guards_json` (post-rebase chain fix).
- **Rebase**: Branch rebased onto latest `master` with merge conflict in `CHANGELOG.md` resolved.
### Deferred Items
- **Partial failure atomicity** (#8 from review): If `save_plan()` fails after `use_action()` succeeds, the plan exists in the DB without CLI overrides. This requires service-layer restructuring beyond the scope of this ticket.
- **Alembic migration naming** (#11 from review): Migration `m4_003` depends on `m6_005`, creating non-sequential naming. Renaming an existing migration risks breaking the chain for anyone who has already applied it.
### Quality Gates
| Session | Result |
|---------|--------|
| lint | PASS |
| typecheck | PASS (0 errors) |
| unit_tests | PASS (11,125 scenarios, 0 failures) |
| integration_tests | PASS (1,562 tests, 0 failures) |
| coverage_report | 97% (threshold: 97%) |
Closes #886
Reviewed-on: #972
Reviewed-by: Brent Edwards <brent.edwards@cleverthis.com>
Co-authored-by: Rui Hu <rui.hu@cleverthis.com>
Co-committed-by: Rui Hu <rui.hu@cleverthis.com>
114 lines
6.3 KiB
Gherkin
114 lines
6.3 KiB
Gherkin
Feature: Plan use --execution-env-priority flag
|
|
As a developer
|
|
I want to set execution environment priority when using plan use
|
|
So that I can control whether the environment setting is a fallback or override
|
|
|
|
Background:
|
|
Given a plan env priority CLI runner
|
|
And a plan env priority mocked lifecycle service
|
|
And a plan env priority action exists
|
|
|
|
Scenario: Plan use with --execution-env-priority override and --execution-environment
|
|
When I run plan use with execution environment "container" and priority "override"
|
|
Then the plan env priority command should succeed
|
|
And the plan should have execution_env_priority set to "override"
|
|
And the plan should have execution_environment set to "container"
|
|
And the plan env priority service use_action should have been called
|
|
And the plan env priority service save_plan should have been called
|
|
|
|
Scenario: Plan use with --execution-env-priority fallback and --execution-environment
|
|
When I run plan use with execution environment "host" and priority "fallback"
|
|
Then the plan env priority command should succeed
|
|
And the plan should have execution_env_priority set to "fallback"
|
|
And the plan should have execution_environment set to "host"
|
|
|
|
Scenario: Plan use with --execution-env-priority without --execution-environment fails
|
|
When I run plan use with priority "override" but no execution environment
|
|
Then the plan env priority command should fail
|
|
And the plan env priority output should contain "--execution-env-priority requires --execution-environment"
|
|
|
|
Scenario: Plan use with --execution-environment but no --execution-env-priority defaults to fallback
|
|
When I run plan use with execution environment "container" but no priority
|
|
Then the plan env priority command should succeed
|
|
And the plan should have execution_env_priority set to "fallback"
|
|
And the plan env priority service save_plan should have been called
|
|
|
|
Scenario: Plan use with invalid --execution-env-priority value fails
|
|
When I run plan use with execution environment "container" and priority "invalid-value"
|
|
Then the plan env priority command should fail
|
|
And the plan env priority output should contain "Invalid execution env priority"
|
|
|
|
Scenario: Plan use output displays priority when execution environment is set
|
|
When I run plan use with execution environment "container" and priority "override"
|
|
Then the plan env priority command should succeed
|
|
And the plan env priority output should contain "Execution Env Priority"
|
|
And the plan env priority output should contain "override"
|
|
|
|
Scenario: Plan use JSON output includes execution_env_priority
|
|
When I run plan use with execution environment "container" and priority "override" in json format
|
|
Then the plan env priority command should succeed
|
|
And the plan env priority json output should include "execution_env_priority" as "override"
|
|
|
|
Scenario: Plan use accepts uppercase --execution-env-priority value
|
|
When I run plan use with execution environment "container" and priority "OVERRIDE"
|
|
Then the plan env priority command should succeed
|
|
And the plan should have execution_env_priority set to "override"
|
|
|
|
# --- Domain model validator tests ---
|
|
|
|
Scenario: Plan construction with execution_env_priority but no execution_environment raises ValueError
|
|
When I construct a Plan with execution_env_priority "override" and no execution_environment
|
|
Then the plan construction should raise ValueError
|
|
And the plan construction error should mention "execution_env_priority requires execution_environment"
|
|
|
|
Scenario: Plan construction with both execution_environment and execution_env_priority succeeds
|
|
When I construct a Plan with execution_environment "container" and execution_env_priority "override"
|
|
Then the plan construction should succeed
|
|
And the constructed plan should have execution_env_priority "override"
|
|
And the constructed plan should have execution_environment "container"
|
|
|
|
# --- ExecutionEnvPriority enum tests ---
|
|
|
|
Scenario: ExecutionEnvPriority enum has fallback value
|
|
Then the ExecutionEnvPriority enum should have value "fallback"
|
|
|
|
Scenario: ExecutionEnvPriority enum has override value
|
|
Then the ExecutionEnvPriority enum should have value "override"
|
|
|
|
Scenario: ExecutionEnvPriority is a StrEnum subclass
|
|
Then ExecutionEnvPriority should be a StrEnum subclass
|
|
|
|
# --- Plan.as_cli_dict execution environment tests ---
|
|
|
|
Scenario: as_cli_dict includes both execution fields when set
|
|
Given a plan with execution_environment "container" and execution_env_priority "override"
|
|
When I call as_cli_dict on the env priority plan
|
|
Then the env priority CLI dict should contain key "execution_environment" with value "container"
|
|
And the env priority CLI dict should contain key "execution_env_priority" with value "override"
|
|
|
|
Scenario: as_cli_dict omits execution fields when execution_environment is None
|
|
Given a plan with no execution_environment
|
|
When I call as_cli_dict on the env priority plan
|
|
Then the env priority CLI dict should not contain key "execution_environment"
|
|
And the env priority CLI dict should not contain key "execution_env_priority"
|
|
|
|
Scenario: as_cli_dict defaults execution_env_priority to fallback for pre-migration data
|
|
Given a plan with execution_environment "host" but no execution_env_priority
|
|
When I call as_cli_dict on the env priority plan
|
|
Then the env priority CLI dict should contain key "execution_environment" with value "host"
|
|
And the env priority CLI dict should contain key "execution_env_priority" with value "fallback"
|
|
|
|
# --- DB round-trip serialization tests ---
|
|
|
|
Scenario: LifecyclePlanModel round-trip preserves execution environment fields
|
|
Given a plan with execution_environment "container" and execution_env_priority "override"
|
|
When I serialize the plan through LifecyclePlanModel from_domain and to_domain
|
|
Then the round-trip plan should have execution_environment "container"
|
|
And the round-trip plan should have execution_env_priority "override"
|
|
|
|
Scenario: LifecyclePlanModel round-trip preserves None execution environment fields
|
|
Given a plan with no execution_environment
|
|
When I serialize the plan through LifecyclePlanModel from_domain and to_domain
|
|
Then the round-trip plan should have execution_environment None
|
|
And the round-trip plan should have execution_env_priority None
|