Files
cleveragents-core/features/consolidated_decision.feature
hurui200320 3837327564 feat(plan): enforce decision type phase-gating at recording time (#973)
## Summary

Adds phase-gating validation to `DecisionService.record_decision()` that enforces the specification's constraint: certain decision types are only valid during specific plan phases. This prevents invalid decisions (e.g., `tool_invocation` during Strategize, `strategy_choice` during Execute) from being persisted.

### Changes

- **Exception** (`cleveragents.core.exceptions`): Added `DecisionPhaseViolationError(BusinessRuleViolation)` with `decision_type`, `plan_phase`, and `allowed_types` attributes.
- **Phase constants** (`cleveragents.domain.models.core.decision`):
  - `resource_selection` added to `EXECUTE_TYPES` — now phase-agnostic (Strategize or Execute) per ADR-007 L72 and ADR-033 L74.
  - `subplan_spawn` / `subplan_parallel_spawn` in both sets; code comment documents divergence from ADRs per M4 subplan model (ticket #931).
  - `USER_INTERVENTION` remains phase-agnostic (both sets).
  - Module-level docstring table updated to match actual assignments.
  - `is_any_phase_type` property updated to check membership in both sets dynamically (was hardcoded to `USER_INTERVENTION` only).
- **Phase-gating module** (`cleveragents.application.services.phase_gating`):
  - Extracted from `DecisionService` to reduce `decision_service.py` line count (1010 → 913) and isolate the phase-gating concern.
  - `PHASE_ALLOWED_TYPES` typed as `Mapping[PlanPhase, frozenset[DecisionType]]`.
  - `resolve_plan_phase()` helper: supports explicit parameter, DB lookup, and graceful skip.
  - `validate_phase_gating()` enforcement raises `DecisionPhaseViolationError`.
  - Exception narrowing: DB lookup catches `(DatabaseError, OperationalError, OSError)` instead of bare `except Exception` — only absorbs infrastructure failures, not programming errors.
  - `# TODO(pg-migration):` marker on TOCTOU race documentation for future PostgreSQL migration.
- **Decision service** (`cleveragents.application.services.decision_service`):
  - Added `plan_phase` parameter to `record_decision()`.
  - Invalid `plan_phase` string now raises `ValidationError` (was uncaught `ValueError`).
  - Imports and delegates to `phase_gating` module for all phase-gating logic.
  - `PHASE_ALLOWED_TYPES` re-exported in `__all__` for backward compatibility.
- **CHANGELOG**: Added behavioral change entry for `resource_selection` reclassification.
- **Backward compatibility**: Phase-gating is opt-in — when neither `plan_phase` is provided nor a UnitOfWork is wired, validation is skipped, preserving all existing callers.
- **Unrelated drive-by reverted**: Removed `ULID_PATTERN` from `decision.py` `__all__` (was an unrelated export addition).
- **Tests**:
  - 36 Behave scenarios covering valid/invalid types per phase, phase-agnostic acceptance, DB-based resolution (Strategize and Execute plans), unknown plan in DB, PlanPhase enum pass-through, error attributes, and ungated phases.
  - 11 new Behave scenarios for `is_any_phase_type`: 4 dual-phase types (true) + 7 single-phase types (false), including `prompt_definition` root test.
  - 6 Robot Framework integration tests with stderr assertions.
  - Updated `consolidated_decision.feature` for new `EXECUTE_TYPES` member count (8 members).
  - Test cleanup now calls `uow.engine.dispose()` before file deletion.
  - `tempfile.mktemp()` replaced with `tempfile.mkstemp()`.
  - Inline imports moved to module top-level per CONTRIBUTING.md.
  - Flaky concurrency test timing increased in `subplan_execution_steps.py`.

### Review Round 1 + 2 Fixes

| # | Finding | Resolution |
|---|---------|------------|
| P1-1 | `except Exception` too broad in `_resolve_plan_phase` | Narrowed to `(DatabaseError, OperationalError, OSError)` — matches codebase pattern |
| P2-2 | `decision_service.py` at 1010 lines | Extracted to `phase_gating.py` module (1010 → 913 lines) |
| P2-3 | TOCTOU race — no programmatic guard | Added `# TODO(pg-migration):` marker with actionable guidance |
| P2-4 | `resource_selection` reclassification needs CHANGELOG | Added CHANGELOG entry documenting behavioral change |
| P2-7 | `is_any_phase_type` BDD gap for dual-phase types | Added 11 parametrized scenarios covering all 4 dual-phase + 7 single-phase types |
| P3-5 | `ULID_PATTERN` export is unrelated drive-by | Reverted — removed from `decision.py` `__all__` |
| P3-6 | `decision.py` at 514 lines (now 513) | No action — reviewer accepted as marginally over |

### Quality Gates

| Session | Result |
|---------|--------|
| lint | PASS |
| typecheck | PASS (0 errors) |
| unit_tests | PASS (11,153 scenarios, 0 failures) |
| integration_tests | PASS (1,563 tests, 0 failures) |
| e2e_tests | PASS (16 tests, 0 failures) |
| coverage_report | 97% (threshold: 97%) |

Closes #931

Reviewed-on: cleveragents/cleveragents-core#973
Co-authored-by: Rui Hu <rui.hu@cleverthis.com>
Co-committed-by: Rui Hu <rui.hu@cleverthis.com>
2026-03-19 07:53:43 +00:00

485 lines
19 KiB
Gherkin

Feature: Consolidated Decision
Combined scenarios from: decision_model, decision_persistence_serialization
# ============================================================
# Originally from: decision_model.feature
# Feature: Decision domain model
# ============================================================
Scenario: All 11 decision types are defined
Then the DecisionType enum should have exactly 11 members
Scenario: Strategize-phase types are correctly classified
Then STRATEGIZE_TYPES should contain "prompt_definition"
And STRATEGIZE_TYPES should contain "invariant_enforced"
And STRATEGIZE_TYPES should contain "strategy_choice"
And STRATEGIZE_TYPES should contain "resource_selection"
And STRATEGIZE_TYPES should contain "subplan_spawn"
And STRATEGIZE_TYPES should contain "subplan_parallel_spawn"
And STRATEGIZE_TYPES should contain "user_intervention"
And STRATEGIZE_TYPES should have exactly 7 members
Scenario: Execute-phase types are correctly classified
Then EXECUTE_TYPES should contain "implementation_choice"
And EXECUTE_TYPES should contain "resource_selection"
And EXECUTE_TYPES should contain "tool_invocation"
And EXECUTE_TYPES should contain "error_recovery"
And EXECUTE_TYPES should contain "validation_response"
And EXECUTE_TYPES should contain "subplan_spawn"
And EXECUTE_TYPES should contain "subplan_parallel_spawn"
And EXECUTE_TYPES should contain "user_intervention"
And EXECUTE_TYPES should have exactly 8 members
Scenario: user_intervention is a phase-agnostic type in both sets
Then STRATEGIZE_TYPES should contain "user_intervention"
And EXECUTE_TYPES should contain "user_intervention"
# ------------------------------------------------------------------
# Decision creation and ULID validation
# ------------------------------------------------------------------
Scenario: Create a minimal root decision
Given a valid plan ULID
When I create a prompt_definition decision with sequence 0
Then the decision should be created successfully
And the decision should be a root decision
And the decision should have a valid ULID as decision_id
And the decision type should be "prompt_definition"
And the decision is_strategize_type should be true
And the decision is_any_phase_type should be false
Scenario: Create a non-root decision with parent
Given a valid plan ULID
And a valid parent decision ULID
When I create a strategy_choice decision with sequence 1 and a parent
Then the decision should be created successfully
And the decision should not be a root decision
And the decision is_strategize_type should be true
Scenario: Create an execute-phase decision
Given a valid plan ULID
And a valid parent decision ULID
When I create an implementation_choice decision with sequence 2 and a parent
Then the decision should be created successfully
And the decision is_execute_type should be true
And the decision is_strategize_type should be false
Scenario: user_intervention is valid in any phase
Given a valid plan ULID
When I create a user_intervention decision with sequence 3
Then the decision should be created successfully
And the decision is_any_phase_type should be true
Scenario Outline: All dual-phase types report is_any_phase_type true
Given a valid plan ULID
And a valid parent decision ULID
When I create a "<decision_type>" decision with sequence 4 and a parent
Then the decision should be created successfully
And the decision is_any_phase_type should be true
Examples:
| decision_type |
| resource_selection |
| subplan_spawn |
| subplan_parallel_spawn |
| user_intervention |
Scenario Outline: Single-phase types report is_any_phase_type false
Given a valid plan ULID
And a valid parent decision ULID
When I create a "<decision_type>" decision with sequence 5 and a parent
Then the decision should be created successfully
And the decision is_any_phase_type should be false
Examples:
| decision_type |
| invariant_enforced |
| strategy_choice |
| implementation_choice |
| tool_invocation |
| error_recovery |
| validation_response |
Scenario: Invalid plan_id is rejected
When I try to create a decision with plan_id "not-a-ulid"
Then a decision validation error should be raised
And the decision error should mention "plan_id"
Scenario: Invalid parent_decision_id is rejected
Given a valid plan ULID
When I try to create a decision with parent_decision_id "bad-id"
Then a decision validation error should be raised
And the decision error should mention "parent_decision_id"
# ------------------------------------------------------------------
# prompt_definition root constraint
# ------------------------------------------------------------------
Scenario: prompt_definition with a parent is rejected
Given a valid plan ULID
And a valid parent decision ULID
When I try to create a prompt_definition with a parent
Then a decision validation error should be raised
And the decision error should mention "root"
# ------------------------------------------------------------------
# Confidence score validation
# ------------------------------------------------------------------
Scenario: Confidence score within valid range
Given a valid plan ULID
When I create a decision with confidence score 0.85
Then the decision should be created successfully
And the decision confidence score should be 0.85
Scenario: Confidence score of 0.0 is valid
Given a valid plan ULID
When I create a decision with confidence score 0.0
Then the decision should be created successfully
Scenario: Confidence score of 1.0 is valid
Given a valid plan ULID
When I create a decision with confidence score 1.0
Then the decision should be created successfully
Scenario: Confidence score above 1.0 is rejected
Given a valid plan ULID
When I try to create a decision with confidence score 1.5
Then a decision validation error should be raised
Scenario: Confidence score below 0.0 is rejected
Given a valid plan ULID
When I try to create a decision with confidence score -0.1
Then a decision validation error should be raised
Scenario: Confidence score of None is valid
Given a valid plan ULID
When I create a decision with confidence score None
Then the decision should be created successfully
And the decision confidence score should be None
# ------------------------------------------------------------------
# Correction metadata
# ------------------------------------------------------------------
Scenario: Create a correction decision
Given a valid plan ULID
And a valid corrected decision ULID
When I create a correction decision
Then the decision should be created successfully
And the decision is_correction should be true
Scenario: is_correction without corrects_decision_id is rejected
Given a valid plan ULID
When I try to create a decision with is_correction true but no corrects_decision_id
Then a decision validation error should be raised
And the decision error should mention "corrects_decision_id"
Scenario: corrects_decision_id without is_correction is rejected
Given a valid plan ULID
And a valid corrected decision ULID
When I try to create a decision with corrects_decision_id but is_correction false
Then a decision validation error should be raised
And the decision error should mention "is_correction"
Scenario: superseded_by marks decision as superseded
Given a valid plan ULID
When I create a decision that is superseded
Then the decision is_superseded should be true
Scenario: Invalid superseded_by ULID is rejected
Given a valid plan ULID
When I try to create a decision with superseded_by "invalid"
Then a decision validation error should be raised
And the decision error should mention "superseded_by"
Scenario: Invalid corrects_decision_id ULID is rejected
Given a valid plan ULID
When I try to create a decision with corrects_decision_id ULID "not-valid"
Then a decision validation error should be raised
And the decision error should mention "corrects_decision_id"
Scenario: with_superseded_by returns a new superseded copy
Given a valid plan ULID
When I create a prompt_definition decision with sequence 0
And I call with_superseded_by on the decision
Then the original decision should not be superseded
And the superseded copy should be superseded
# ------------------------------------------------------------------
# ContextSnapshot
# ------------------------------------------------------------------
Scenario: Create a decision with a populated context snapshot
Given a valid plan ULID
And a context snapshot with hash and resources
When I create a decision with the context snapshot
Then the decision should be created successfully
And the decision context snapshot hash should not be empty
And the decision context snapshot should have resources
Scenario: Default context snapshot is empty
Given a valid plan ULID
When I create a prompt_definition decision with sequence 0
Then the decision context snapshot hash should be empty
# ------------------------------------------------------------------
# ResourceRef and ArtifactRef
# ------------------------------------------------------------------
Scenario: ResourceRef requires non-empty resource_id
When I try to create a ResourceRef with empty resource_id
Then a decision validation error should be raised
Scenario: ArtifactRef requires non-empty artifact_path
When I try to create an ArtifactRef with empty artifact_path
Then a decision validation error should be raised
Scenario: Decision with artifacts_produced
Given a valid plan ULID
When I create a decision with artifacts
Then the decision should have 2 artifacts produced
# ------------------------------------------------------------------
# Serialization
# ------------------------------------------------------------------
Scenario: Decision round-trips through dict serialization
Given a valid plan ULID
When I create a fully populated decision
Then the decision should round-trip through model_dump and model_validate
Scenario: as_cli_dict returns expected keys
Given a valid plan ULID
When I create a prompt_definition decision with sequence 0
Then as_cli_dict should contain key "decision_id"
And as_cli_dict should contain key "type"
And as_cli_dict should contain key "confidence"
And as_cli_dict should contain key "parent"
# ------------------------------------------------------------------
# All 11 decision types can be instantiated
# ------------------------------------------------------------------
Scenario Outline: Each decision type can be instantiated
Given a valid plan ULID
When I create a decision of type "<dtype>"
Then the decision should be created successfully
Examples:
| dtype |
| prompt_definition |
| invariant_enforced |
| strategy_choice |
| implementation_choice |
| resource_selection |
| subplan_spawn |
| subplan_parallel_spawn |
| tool_invocation |
| error_recovery |
| validation_response |
| user_intervention |
# ============================================================
# Originally from: decision_persistence_serialization.feature
# Feature: Decision persistence via serialization round-trips
# ============================================================
Scenario: Persist a root decision through model_dump round-trip
Given a decision persistence plan ULID
When I decision persistence create a root prompt_definition with sequence 0
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored decision should match the original
Scenario: Persist a child decision with all fields populated
Given a decision persistence plan ULID
And a decision persistence parent ULID
When I decision persistence create a full strategy_choice child at sequence 1
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored decision should match the original
And the decision persistence restored alternatives should have 3 entries
Scenario: Persist an execute-phase decision with snapshot
Given a decision persistence plan ULID
And a decision persistence parent ULID
And a decision persistence context snapshot with 2 resources
When I decision persistence create an implementation_choice with the snapshot
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored snapshot hash should match
And the decision persistence restored snapshot should have 2 resources
# ------------------------------------------------------------------
# JSON serialization round-trips
# ------------------------------------------------------------------
Scenario: Decision survives JSON serialization round-trip
Given a decision persistence plan ULID
When I decision persistence create a root prompt_definition with sequence 0
And I decision persistence round-trip the decision through JSON
Then the decision persistence restored decision should match the original
Scenario: Full decision with artifacts survives JSON round-trip
Given a decision persistence plan ULID
And a decision persistence parent ULID
When I decision persistence create a decision with 3 artifacts
And I decision persistence round-trip the decision through JSON
Then the decision persistence restored decision should have 3 artifacts
# ------------------------------------------------------------------
# Snapshot persistence
# ------------------------------------------------------------------
Scenario: Empty context snapshot persists correctly
Given a decision persistence plan ULID
When I decision persistence create a root prompt_definition with sequence 0
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored snapshot hash should be empty
And the decision persistence restored snapshot should have 0 resources
Scenario: Context snapshot with actor state ref persists
Given a decision persistence plan ULID
And a decision persistence parent ULID
And a decision persistence snapshot with actor state ref "checkpoint://actor/42"
When I decision persistence create a strategy_choice with the snapshot
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored actor state ref should be "checkpoint://actor/42"
# ------------------------------------------------------------------
# Correction chain persistence
# ------------------------------------------------------------------
Scenario: Correction decision persists correction metadata
Given a decision persistence plan ULID
And a decision persistence corrected decision ULID
When I decision persistence create a correction decision
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored is_correction should be true
And the decision persistence restored corrects_decision_id should match
Scenario: Superseded decision persists superseded_by
Given a decision persistence plan ULID
When I decision persistence create a decision superseded by another
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored is_superseded should be true
Scenario: Correction chain of 3 decisions persists correctly
Given a decision persistence plan ULID
When I decision persistence build a correction chain of 3 decisions
And I decision persistence round-trip all decisions through model_dump
Then the decision persistence correction chain should be reconstructable
# ------------------------------------------------------------------
# Decision tree reconstruction
# ------------------------------------------------------------------
Scenario: Reconstruct a 3-level decision tree from serialized data
Given a decision persistence plan ULID
When I decision persistence build a 3-level decision tree
And I decision persistence serialize and deserialize all tree nodes
Then the decision persistence tree parent links should be intact
And the decision persistence tree should have 7 nodes
Scenario: Decision downstream_decision_ids survive round-trip
Given a decision persistence plan ULID
When I decision persistence create a decision with 4 downstream IDs
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored should have 4 downstream IDs
Scenario: Decision downstream_plan_ids survive round-trip
Given a decision persistence plan ULID
When I decision persistence create a decision with 2 downstream plan IDs
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored should have 2 downstream plan IDs
# ------------------------------------------------------------------
# Edge cases
# ------------------------------------------------------------------
Scenario: Decision with empty alternatives list persists
Given a decision persistence plan ULID
When I decision persistence create a decision with no alternatives
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored alternatives should have 0 entries
Scenario: Decision with max confidence 1.0 persists
Given a decision persistence plan ULID
When I decision persistence create a decision with confidence 1.0
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored confidence should be 1.0
Scenario: Decision with min confidence 0.0 persists
Given a decision persistence plan ULID
When I decision persistence create a decision with confidence 0.0
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored confidence should be 0.0
Scenario: Decision with None confidence persists
Given a decision persistence plan ULID
When I decision persistence create a decision with confidence None
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored confidence should be None
Scenario: All decision types round-trip through model_dump
Given a decision persistence plan ULID
When I decision persistence create and round-trip all decision types
Then all decision persistence round-trips should succeed
Scenario: Decision with long rationale text persists
Given a decision persistence plan ULID
When I decision persistence create a decision with a 2000 character rationale
And I decision persistence round-trip the decision through model_dump
Then the decision persistence restored rationale length should be 2000