3837327564
CI / lint (push) Successful in 18s
CI / build (push) Successful in 27s
CI / quality (push) Successful in 29s
CI / typecheck (push) Successful in 44s
CI / benchmark-regression (push) Has been skipped
CI / security (push) Successful in 53s
CI / unit_tests (push) Successful in 3m14s
CI / integration_tests (push) Successful in 3m38s
CI / docker (push) Successful in 56s
CI / e2e_tests (push) Successful in 5m13s
CI / coverage (push) Successful in 7m0s
CI / benchmark-publish (push) Successful in 19m58s
## 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: #973 Co-authored-by: Rui Hu <rui.hu@cleverthis.com> Co-committed-by: Rui Hu <rui.hu@cleverthis.com>
157 lines
4.7 KiB
Python
157 lines
4.7 KiB
Python
"""Helper script for Robot Framework decision phase-gating smoke tests.
|
|
|
|
Usage:
|
|
python robot/helper_decision_phase_gating.py <subcommand>
|
|
|
|
Subcommands:
|
|
valid-strategize Record valid strategize-phase types
|
|
valid-execute Record valid execute-phase types
|
|
invalid-strategize Reject execute-only types in strategize
|
|
invalid-execute Reject strategize-only types in execute
|
|
phase-agnostic Phase-agnostic types work in both phases
|
|
error-attributes DecisionPhaseViolationError attributes
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import sys
|
|
from pathlib import Path
|
|
|
|
# Ensure src is importable when run from workspace root
|
|
sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src"))
|
|
|
|
from cleveragents.application.services.decision_service import DecisionService
|
|
from cleveragents.core.exceptions import DecisionPhaseViolationError
|
|
from cleveragents.domain.models.core.decision import (
|
|
EXECUTE_TYPES,
|
|
STRATEGIZE_TYPES,
|
|
DecisionType,
|
|
)
|
|
|
|
_PLAN_ID = "01HV000000000000000000PG01"
|
|
|
|
|
|
def _record(
|
|
svc: DecisionService,
|
|
dtype: DecisionType,
|
|
phase: str,
|
|
*,
|
|
parent: str | None = "01HV000000000000000000PP01",
|
|
) -> None:
|
|
"""Record a decision with phase-gating."""
|
|
if dtype == DecisionType.PROMPT_DEFINITION:
|
|
parent = None
|
|
svc.record_decision(
|
|
plan_id=_PLAN_ID,
|
|
decision_type=dtype,
|
|
question=f"Q ({dtype})",
|
|
chosen_option=f"A ({dtype})",
|
|
parent_decision_id=parent,
|
|
plan_phase=phase,
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Subcommands
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
def _valid_strategize() -> None:
|
|
svc = DecisionService()
|
|
for dt in STRATEGIZE_TYPES:
|
|
_record(svc, dt, "strategize")
|
|
print("valid-strategize-ok")
|
|
|
|
|
|
def _valid_execute() -> None:
|
|
svc = DecisionService()
|
|
for dt in EXECUTE_TYPES:
|
|
_record(svc, dt, "execute")
|
|
print("valid-execute-ok")
|
|
|
|
|
|
def _invalid_strategize() -> None:
|
|
svc = DecisionService()
|
|
execute_only = EXECUTE_TYPES - STRATEGIZE_TYPES
|
|
for dt in execute_only:
|
|
try:
|
|
_record(svc, dt, "strategize")
|
|
raise AssertionError(
|
|
f"Expected DecisionPhaseViolationError for {dt} in strategize"
|
|
)
|
|
except DecisionPhaseViolationError as exc:
|
|
assert exc.decision_type == str(dt)
|
|
assert exc.plan_phase == "strategize"
|
|
print("invalid-strategize-ok")
|
|
|
|
|
|
def _invalid_execute() -> None:
|
|
svc = DecisionService()
|
|
strategize_only = STRATEGIZE_TYPES - EXECUTE_TYPES
|
|
for dt in strategize_only:
|
|
try:
|
|
_record(svc, dt, "execute")
|
|
raise AssertionError(
|
|
f"Expected DecisionPhaseViolationError for {dt} in execute"
|
|
)
|
|
except DecisionPhaseViolationError as exc:
|
|
assert exc.decision_type == str(dt)
|
|
assert exc.plan_phase == "execute"
|
|
print("invalid-execute-ok")
|
|
|
|
|
|
def _phase_agnostic() -> None:
|
|
svc = DecisionService()
|
|
agnostic = STRATEGIZE_TYPES & EXECUTE_TYPES
|
|
assert len(agnostic) > 0, "No phase-agnostic types found"
|
|
for dt in agnostic:
|
|
_record(svc, dt, "strategize")
|
|
_record(svc, dt, "execute")
|
|
print("phase-agnostic-ok")
|
|
|
|
|
|
def _error_attributes() -> None:
|
|
svc = DecisionService()
|
|
try:
|
|
_record(svc, DecisionType.TOOL_INVOCATION, "strategize")
|
|
raise AssertionError("Expected DecisionPhaseViolationError")
|
|
except DecisionPhaseViolationError as exc:
|
|
assert exc.decision_type == "tool_invocation"
|
|
assert exc.plan_phase == "strategize"
|
|
assert isinstance(exc.allowed_types, frozenset)
|
|
assert len(exc.allowed_types) > 0
|
|
assert "tool_invocation" not in exc.allowed_types
|
|
msg = str(exc)
|
|
assert "tool_invocation" in msg
|
|
assert "strategize" in msg
|
|
assert "Allowed types" in msg
|
|
print("error-attributes-ok")
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Dispatcher
|
|
# ---------------------------------------------------------------------------
|
|
|
|
_COMMANDS = {
|
|
"valid-strategize": _valid_strategize,
|
|
"valid-execute": _valid_execute,
|
|
"invalid-strategize": _invalid_strategize,
|
|
"invalid-execute": _invalid_execute,
|
|
"phase-agnostic": _phase_agnostic,
|
|
"error-attributes": _error_attributes,
|
|
}
|
|
|
|
|
|
def main() -> None:
|
|
if len(sys.argv) < 2:
|
|
raise SystemExit(f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>")
|
|
command = sys.argv[1]
|
|
handler = _COMMANDS.get(command)
|
|
if handler is None:
|
|
raise SystemExit(f"Unknown command: {command}")
|
|
handler()
|
|
|
|
|
|
if __name__ == "__main__":
|
|
main()
|