300a5d6ddc
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 18s
CI / helm (pull_request) Successful in 23s
CI / lint (pull_request) Failing after 26s
CI / quality (pull_request) Successful in 34s
CI / security (pull_request) Failing after 46s
CI / typecheck (pull_request) Failing after 50s
CI / coverage (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Has been skipped
CI / unit_tests (pull_request) Failing after 1m46s
CI / docker (pull_request) Has been skipped
CI / e2e_tests (pull_request) Failing after 14m9s
CI / integration_tests (pull_request) Failing after 20m57s
CI / status-check (pull_request) Failing after 1s
Add ULID format validation to all v3 plan commands to prevent users from
accidentally mixing legacy ('agents tell') and v3 ('agents plan use')
workflows. The two systems use separate storage backends and cannot be
mixed; this change detects the mismatch early and provides actionable
error messages.
Changes:
- Add _validate_plan_ulid() with proper Crockford Base32 regex
(^[0-9A-HJKMNP-TV-Z]{26}$, re.IGNORECASE) that correctly rejects
invalid characters (I, L, O, U), hyphens, and wrong-length strings
- Add _PLAN_ULID_RE compiled regex and _ULID_VALIDATION_ERROR_MSG constant
with actionable guidance explaining the legacy/v3 incompatibility
- Apply ULID validation to all v3 commands: execute_plan,
_lifecycle_apply_with_id, lifecycle_apply_plan, plan_status,
plan_errors, cancel_plan (only on user-provided IDs, not auto-discovered)
- Update _LEGACY_DEPRECATION_MSG and tell/build command warnings to
explicitly state that the two workflows are INCOMPATIBLE and cannot be mixed
- Add comprehensive BDD tests in features/plan_ulid_validation.feature
with step definitions using Typer CLI runner (not subprocess)
- Update CONTRIBUTING.md with 'Workflow Choice: Legacy vs. v3 Plan
Lifecycle' section documenting the incompatibility and migration path
ISSUES CLOSED: #1560
360 lines
13 KiB
Python
360 lines
13 KiB
Python
"""Step definitions for plan_ulid_validation.feature.
|
|
|
|
Tests ULID format validation for v3 plan commands and the updated
|
|
legacy deprecation messages that explain workflow incompatibility.
|
|
|
|
All step text uses the ``ulid-validation`` prefix to avoid collisions
|
|
with other step definition files.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
from datetime import datetime
|
|
from typing import Any
|
|
from unittest.mock import MagicMock, patch
|
|
|
|
from behave import given, then, when # type: ignore[import-untyped]
|
|
from typer.testing import CliRunner
|
|
|
|
from cleveragents.cli.commands.plan import app as plan_app
|
|
from cleveragents.core.exceptions import ValidationError
|
|
from cleveragents.domain.models.core.plan import (
|
|
NamespacedName,
|
|
Plan,
|
|
PlanIdentity,
|
|
PlanPhase,
|
|
PlanTimestamps,
|
|
ProcessingState,
|
|
)
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Helpers
|
|
# ---------------------------------------------------------------------------
|
|
|
|
_VALID_ULID = "01HXM8C2ZK4Q7C2B3F2R4VYV6J"
|
|
_runner = CliRunner()
|
|
|
|
|
|
def _make_plan(
|
|
*,
|
|
plan_id: str = _VALID_ULID,
|
|
name: str = "local/test-plan",
|
|
phase: PlanPhase = PlanPhase.EXECUTE,
|
|
processing_state: ProcessingState = ProcessingState.COMPLETE,
|
|
) -> Plan:
|
|
"""Create a minimal Plan instance for testing."""
|
|
timestamps = PlanTimestamps(
|
|
created_at=datetime.now(),
|
|
updated_at=datetime.now(),
|
|
)
|
|
return Plan(
|
|
identity=PlanIdentity(plan_id=plan_id),
|
|
namespaced_name=NamespacedName.parse(name),
|
|
action_name="local/test-action",
|
|
description="Test plan",
|
|
definition_of_done=None,
|
|
phase=phase,
|
|
processing_state=processing_state,
|
|
strategy_actor=None,
|
|
execution_actor=None,
|
|
project_links=[],
|
|
automation_profile=None,
|
|
invariants=[],
|
|
validation_summary=None,
|
|
error_message=None,
|
|
last_completed_step=-1,
|
|
last_checkpoint_id=None,
|
|
arguments={},
|
|
arguments_order=[],
|
|
estimation_actor=None,
|
|
invariant_actor=None,
|
|
timestamps=timestamps,
|
|
created_by=None,
|
|
reusable=True,
|
|
read_only=False,
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# _validate_plan_ulid unit test steps
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when('I call _validate_plan_ulid with "{plan_id}"')
|
|
def step_call_validate_plan_ulid(context: Any, plan_id: str) -> None:
|
|
"""Call _validate_plan_ulid and capture result or exception."""
|
|
from cleveragents.cli.commands.plan import _validate_plan_ulid
|
|
|
|
context.ulid_validation_result = None
|
|
context.ulid_validation_error = None
|
|
try:
|
|
context.ulid_validation_result = _validate_plan_ulid(plan_id)
|
|
except ValidationError as exc:
|
|
context.ulid_validation_error = exc
|
|
|
|
|
|
@then('ulid-val the validation should succeed and return "{expected}"')
|
|
def step_validation_succeeds(context: Any, expected: str) -> None:
|
|
"""Assert that validation succeeded and returned the expected value."""
|
|
assert context.ulid_validation_error is None, (
|
|
f"Expected success but got ValidationError: {context.ulid_validation_error}"
|
|
)
|
|
assert context.ulid_validation_result == expected, (
|
|
f"Expected '{expected}' but got '{context.ulid_validation_result}'"
|
|
)
|
|
|
|
|
|
@then("ulid-val the validation should raise ValidationError")
|
|
def step_validation_raises(context: Any) -> None:
|
|
"""Assert that validation raised a ValidationError."""
|
|
assert context.ulid_validation_error is not None, (
|
|
"Expected ValidationError but validation succeeded with result: "
|
|
f"'{context.ulid_validation_result}'"
|
|
)
|
|
|
|
|
|
@then('ulid-val the error message should contain "{text}"')
|
|
def step_error_message_contains(context: Any, text: str) -> None:
|
|
"""Assert that the ValidationError message contains the given text."""
|
|
assert context.ulid_validation_error is not None, (
|
|
"No ValidationError was raised — cannot check error message."
|
|
)
|
|
error_msg = context.ulid_validation_error.message
|
|
assert text in error_msg, (
|
|
f"Expected error message to contain '{text}' but got:\n{error_msg}"
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# CLI runner setup
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@given("a ulid-validation CLI runner")
|
|
def step_ulid_validation_runner(context: Any) -> None:
|
|
"""Set up a CLI runner and clean up patches after the scenario."""
|
|
context.ulid_runner = _runner
|
|
context.ulid_result = None
|
|
context.ulid_cleanups = [] # list[Any]
|
|
|
|
def _cleanup(ctx: Any) -> None:
|
|
for stop in getattr(ctx, "ulid_cleanups", []):
|
|
stop()
|
|
|
|
context.add_cleanup(_cleanup, context)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Mocked service setup steps
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@given("a mocked lifecycle service for ulid-validation execute")
|
|
def step_mock_service_execute(context: Any) -> None:
|
|
"""Patch the lifecycle service and executor for execute_plan tests."""
|
|
mock_svc = MagicMock()
|
|
mock_executor = MagicMock()
|
|
|
|
plan = _make_plan(
|
|
plan_id=_VALID_ULID,
|
|
phase=PlanPhase.STRATEGIZE,
|
|
processing_state=ProcessingState.QUEUED,
|
|
)
|
|
mock_svc.get_plan.return_value = plan
|
|
mock_svc.list_plans.return_value = [plan]
|
|
mock_executor.run_strategize.return_value = None
|
|
|
|
p1 = patch(
|
|
"cleveragents.cli.commands.plan._get_lifecycle_service",
|
|
return_value=mock_svc,
|
|
)
|
|
p2 = patch(
|
|
"cleveragents.cli.commands.plan._get_plan_executor",
|
|
return_value=mock_executor,
|
|
)
|
|
p1.start()
|
|
p2.start()
|
|
context.ulid_cleanups.extend([p1.stop, p2.stop])
|
|
context.ulid_mock_svc = mock_svc
|
|
|
|
|
|
@given("a mocked lifecycle service for ulid-validation status")
|
|
def step_mock_service_status(context: Any) -> None:
|
|
"""Patch the lifecycle service for plan_status tests."""
|
|
mock_svc = MagicMock()
|
|
mock_svc.list_plans.return_value = []
|
|
|
|
p = patch(
|
|
"cleveragents.cli.commands.plan._get_lifecycle_service",
|
|
return_value=mock_svc,
|
|
)
|
|
p.start()
|
|
context.ulid_cleanups.append(p.stop)
|
|
context.ulid_mock_svc = mock_svc
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# CLI invocation steps — execute
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when('I invoke ulid-validation plan execute with "{plan_id}"')
|
|
def step_invoke_execute(context: Any, plan_id: str) -> None:
|
|
"""Invoke 'agents plan execute <plan_id>' via the CLI runner."""
|
|
context.ulid_result = context.ulid_runner.invoke(
|
|
plan_app, ["execute", plan_id], catch_exceptions=False
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# CLI invocation steps — apply
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when('I invoke ulid-validation plan apply with "{plan_id}"')
|
|
def step_invoke_apply(context: Any, plan_id: str) -> None:
|
|
"""Invoke 'agents plan apply <plan_id>' via the CLI runner."""
|
|
context.ulid_result = context.ulid_runner.invoke(
|
|
plan_app, ["apply", plan_id], catch_exceptions=False
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# CLI invocation steps — status
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when('I invoke ulid-validation plan status with "{plan_id}"')
|
|
def step_invoke_status(context: Any, plan_id: str) -> None:
|
|
"""Invoke 'agents plan status <plan_id>' via the CLI runner."""
|
|
context.ulid_result = context.ulid_runner.invoke(
|
|
plan_app, ["status", plan_id], catch_exceptions=False
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# CLI invocation steps — errors
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when('I invoke ulid-validation plan errors with "{plan_id}"')
|
|
def step_invoke_errors(context: Any, plan_id: str) -> None:
|
|
"""Invoke 'agents plan errors <plan_id>' via the CLI runner."""
|
|
context.ulid_result = context.ulid_runner.invoke(
|
|
plan_app, ["errors", plan_id], catch_exceptions=False
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# CLI invocation steps — cancel
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when('I invoke ulid-validation plan cancel with "{plan_id}"')
|
|
def step_invoke_cancel(context: Any, plan_id: str) -> None:
|
|
"""Invoke 'agents plan cancel <plan_id>' via the CLI runner."""
|
|
context.ulid_result = context.ulid_runner.invoke(
|
|
plan_app, ["cancel", plan_id], catch_exceptions=False
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# CLI assertion steps
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@then("the ulid-validation command should abort")
|
|
def step_command_aborts(context: Any) -> None:
|
|
"""Assert that the CLI command exited with a non-zero exit code."""
|
|
assert context.ulid_result is not None, "No CLI result captured."
|
|
assert context.ulid_result.exit_code != 0, (
|
|
f"Expected non-zero exit code but got {context.ulid_result.exit_code}.\n"
|
|
f"Output: {context.ulid_result.output}"
|
|
)
|
|
|
|
|
|
@then("the ulid-validation command should not abort due to ULID validation")
|
|
def step_command_does_not_abort_on_ulid(context: Any) -> None:
|
|
"""Assert that the CLI command did not abort due to ULID validation.
|
|
|
|
The command may still abort for other reasons (e.g., plan not found
|
|
in the mocked service), but it should not abort with a ULID error.
|
|
"""
|
|
assert context.ulid_result is not None, "No CLI result captured."
|
|
output = context.ulid_result.output
|
|
# If it aborted, it should NOT be because of ULID validation
|
|
if context.ulid_result.exit_code != 0:
|
|
assert "ULID" not in output or "not found" not in output, (
|
|
f"Command aborted due to ULID validation when it should have passed.\n"
|
|
f"Output: {output}"
|
|
)
|
|
|
|
|
|
@then('the ulid-validation output should contain "{text}"')
|
|
def step_output_contains(context: Any, text: str) -> None:
|
|
"""Assert that the CLI output contains the given text."""
|
|
assert context.ulid_result is not None, "No CLI result captured."
|
|
output = context.ulid_result.output
|
|
assert text in output, f"Expected output to contain '{text}' but got:\n{output}"
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Legacy deprecation warning steps
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
@when("I call tell_command programmatically for ulid-validation")
|
|
def step_call_tell_command(context: Any) -> None:
|
|
"""Invoke the legacy tell command via CLI runner and capture its output.
|
|
|
|
The legacy commands emit deprecation warnings via ``console.print``, not
|
|
via Python's ``warnings.warn``. We therefore use the Typer CLI runner to
|
|
capture the printed output and store it for assertion steps.
|
|
"""
|
|
runner = CliRunner()
|
|
result = runner.invoke(plan_app, ["tell", "test prompt"], catch_exceptions=False)
|
|
context.ulid_deprecation_warnings = [result.output]
|
|
|
|
|
|
@when("I call build_command programmatically for ulid-validation")
|
|
def step_call_build_command(context: Any) -> None:
|
|
"""Invoke the legacy build command via CLI runner and capture its output.
|
|
|
|
The legacy commands emit deprecation warnings via ``console.print``, not
|
|
via Python's ``warnings.warn``. We therefore use the Typer CLI runner to
|
|
capture the printed output and store it for assertion steps.
|
|
"""
|
|
runner = CliRunner()
|
|
result = runner.invoke(plan_app, ["build"], catch_exceptions=False)
|
|
context.ulid_deprecation_warnings = [result.output]
|
|
|
|
|
|
@then('the ulid-validation deprecation warning should mention "{text}"')
|
|
def step_deprecation_mentions(context: Any, text: str) -> None:
|
|
"""Assert that the CLI output mentions the given text (case-insensitive)."""
|
|
assert context.ulid_deprecation_warnings, "No CLI output was captured."
|
|
combined = " ".join(context.ulid_deprecation_warnings).lower()
|
|
assert text.lower() in combined, (
|
|
f"Expected CLI output to mention '{text}' (case-insensitive) but got:\n{combined}"
|
|
)
|
|
|
|
|
|
@then("the ulid-validation deprecation warning should not suggest simple command swap")
|
|
def step_deprecation_no_simple_swap(context: Any) -> None:
|
|
"""Assert that the deprecation output does not suggest a 1:1 command swap.
|
|
|
|
The old message said "Use 'agents plan use <action> [project]' for the v3
|
|
lifecycle" which implied a simple substitution. The new message must
|
|
explain the workflow incompatibility instead.
|
|
"""
|
|
assert context.ulid_deprecation_warnings, "No CLI output was captured."
|
|
combined = " ".join(context.ulid_deprecation_warnings)
|
|
# The new message must NOT contain the old misleading phrasing
|
|
old_misleading_phrase = (
|
|
"Use 'agents plan use <action> [project]' for the v3 lifecycle."
|
|
)
|
|
assert old_misleading_phrase not in combined, (
|
|
f"Deprecation output still contains the misleading 1:1 swap suggestion:\n"
|
|
f"{combined}"
|
|
)
|