CHANGELOG.md

@@ -6,6 +6,7 @@ Changed `wf10_batch.robot` to be less likely to create files, and
 `plan_generation_graph.robot` to give more test answers.
 
 ## [Unreleased]
+- **fix(cli/plan): plan correct JSON output envelope fix and BDD test coverage** (#8584 / PR #8662): Restructured `agents plan correct --format json` output to nest correction fields under `data.correction` (e.g., `data.correction.mode`) and populate the spec-required CLI envelope with `command="plan correct"`, `status`, `exit_code`, `timing`, and `messages` fields. Added three BDD scenarios in `features/tdd_plan_correct_json_output.feature` validating the envelope structure for both revert and append modes.
 - **fix(cli): add --url flag to resource add for git resource type** (#6322): Added support for the `--url` flag on `agents resource add git` command, allowing users to specify a remote URL for git resources. The flag is validated to only apply to git resource types. Includes Behave BDD tests in `features/resource_cli_git_url_flag.feature` and Robot Framework integration tests verifying correct URL validation and CLI behavior.
 - **Session create JSON envelope** (#6441): Fixed `agents session create --format json` returning a flat `data` dict instead of the spec-required nested structure with `data.session`, `data.settings`, and `data.actor_details` sub-objects. The `command` field is now populated correctly. Extended JSON envelope coverage to `agents session list`, `show`, `delete --format json`, `export --output-format json`, and `import --format json` so all session commands emit a structured `messages[].text` field (`"0 sessions listed"`, `"Session details loaded"`, `"Session deleted"`, `"Export completed"`, `"Import completed"`).
 - **fix(resources): remove unsupported executable resource type and fix resource list columns** (#3077 / PR #3248): Removed `executable` from `LSP_RESOURCE_TYPES` and `BUILTIN_TYPE_NAMES` (the specification defines no such built-in type). Updated `agents resource list` CLI table columns from `[ID, Name, Type, Status, Kind, Location, Description]` to the spec-required `[Name, ID, Type, Phys/Virt, Children, Projects]`. Deleted orphaned `examples/resource-types/executable.yaml`. Lifecycle state for container resources is now displayed as a note below the resource table.

CONTRIBUTORS.md

@@ -63,3 +63,4 @@ Below are some of the specific details of various contributions.
 * HAL 9000 has contributed the alembic fileConfig error handling fix (PR #8288 / issue #7874): wrapped the `fileConfig()` call in `alembic/env.py` with a `try/except` block to catch malformed INI logging configuration and emit clear, actionable error messages to stderr.
 * HAL 9000 has contributed the Definition-of-Done gating feature for the Apply phase (PR #8299 / issue #7927): `PlanLifecycleService.apply_plan` now evaluates DoD criteria before transitioning to Apply, raising `DoDGatingError` when required criteria fail and storing evaluation results in `plan.validation_summary`.
 * HAL 9000 has contributed the engine cache TOCTOU race condition fix (PR #8265 / issue #7566): added `MEMORY_ENGINES_LOCK` to `engine_cache.py` and wrapped the check-and-set operation in `UnitOfWork.engine` with `with MEMORY_ENGINES_LOCK:` to prevent concurrent threads from creating duplicate in-memory SQLite engine instances; also fixed a cache-hit bug where `self._engine` was never assigned on a cache hit.
+* HAL 9000 has contributed the plan correct JSON output envelope fix (PR #8662 / issue #8584): restructured `agents plan correct --format json` output to nest correction fields under `data.correction` and pass `command="plan correct"` to `format_output`, producing the spec-required CLI envelope. Added three BDD scenarios validating `data.correction.mode` (revert and append modes) and the `command` field.

features/mocks/tdd_plan_correct_json_output_fixtures.py

@@ -0,0 +1,159 @@
+"""Shared mock fixtures for TDD plan-correct JSON output envelope tests.
+
+Provides constants, mock builders, and CLI argument helpers used by the
+Behave step definitions
+(``features/steps/tdd_plan_correct_json_output_steps.py``).
+
+Centralising the mock builders ensures the test suite exercises the
+``plan correct`` CLI JSON output path with identically-shaped mock objects.
+
+Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/8584
+"""
+
+from __future__ import annotations
+
+from datetime import datetime
+from types import SimpleNamespace
+from unittest.mock import MagicMock
+
+from cleveragents.core.exceptions import ResourceNotFoundError
+from cleveragents.domain.models.core.plan import (
+    NamespacedName,
+    Plan,
+    PlanIdentity,
+    PlanPhase,
+    PlanTimestamps,
+    ProcessingState,
+    ProjectLink,
+)
+
+# ---------------------------------------------------------------------------
+# Patch targets
+# ---------------------------------------------------------------------------
+
+PATCH_CONTAINER: str = "cleveragents.application.container.get_container"
+
+# ---------------------------------------------------------------------------
+# Fixed identifiers for deterministic assertions
+# ---------------------------------------------------------------------------
+
+DECISION_ID: str = "DEC-8584-TARGET"
+ROOT_DECISION_ID: str = "DEC-8584-ROOT"
+CORRECTION_ID: str = "CORR-TDD-8584"
+
+
+# ---------------------------------------------------------------------------
+# Mock builders
+# ---------------------------------------------------------------------------
+
+
+def make_decision_ns(
+    decision_id: str,
+    parent_decision_id: str | None,
+) -> SimpleNamespace:
+    """Create a minimal decision-like namespace for list_decisions."""
+    return SimpleNamespace(
+        decision_id=decision_id,
+        parent_decision_id=parent_decision_id,
+    )
+
+
+def _make_plan() -> Plan:
+    """Build a real ``Plan`` in Execute/COMPLETE state."""
+    from ulid import ULID
+
+    return Plan(
+        identity=PlanIdentity(plan_id=str(ULID())),
+        namespaced_name=NamespacedName(
+            server=None, namespace="local", name="tdd-8584-plan"
+        ),
+        action_name="local/tdd-8584-action",
+        description="TDD plan for bug #8584 — JSON output envelope",
+        definition_of_done=None,
+        phase=PlanPhase.EXECUTE,
+        processing_state=ProcessingState.COMPLETE,
+        project_links=[ProjectLink(project_name="proj-1")],
+        strategy_actor="openai/gpt-4",
+        execution_actor="openai/gpt-4",
+        created_by=None,
+        reusable=False,
+        read_only=False,
+        timestamps=PlanTimestamps(created_at=datetime.now(), updated_at=datetime.now()),
+    )
+
+
+def make_container(mode: str = "revert") -> MagicMock:
+    """Build a mock DI container for the JSON output envelope test.
+
+    Args:
+        mode: The correction mode (``"revert"`` or ``"append"``).
+    """
+    plan = _make_plan()
+
+    mock_plan_svc = MagicMock()
+    # get_plan raises RNF for the decision_id (it is not a plan_id),
+    # ensuring the code falls through to the decision_id path.
+    mock_plan_svc.get_plan.side_effect = ResourceNotFoundError(
+        resource_type="Plan",
+        resource_id=DECISION_ID,
+    )
+    mock_plan_svc.list_plans.return_value = [plan]
+
+    decisions = [
+        make_decision_ns(ROOT_DECISION_ID, None),
+        make_decision_ns(DECISION_ID, ROOT_DECISION_ID),
+    ]
+
+    mock_decision_svc = MagicMock()
+    mock_decision_svc.list_decisions.return_value = decisions
+    mock_decision_svc.get_influence_edges.return_value = {}
+
+    mock_correction_svc = MagicMock()
+    mock_correction_svc.request_correction.return_value = SimpleNamespace(
+        correction_id=CORRECTION_ID,
+        mode=SimpleNamespace(value=mode),
+        target_decision_id=DECISION_ID,
+        guidance="Recompute decision subtree",
+    )
+    mock_correction_svc.execute_correction.return_value = SimpleNamespace(
+        correction_id=CORRECTION_ID,
+        status=SimpleNamespace(value="applied"),
+        reverted_decisions=[DECISION_ID],
+        new_decisions=[],
+    )
+
+    mock_container = MagicMock()
+    mock_container.plan_lifecycle_service.return_value = mock_plan_svc
+    mock_container.decision_service.return_value = mock_decision_svc
+    mock_container.correction_service.return_value = mock_correction_svc
+    return mock_container
+
+
+def build_cli_args(mode: str = "revert") -> list[str]:
+    """Build the CLI argument list for ``plan correct <decision_id> --format json``.
+
+    Args:
+        mode: The correction mode (``"revert"`` or ``"append"``).
+    """
+    return [
+        "correct",
+        DECISION_ID,
+        "--mode",
+        mode,
+        "--guidance",
+        "Recompute decision subtree",
+        "--yes",
+        "--format",
+        "json",
+    ]
+
+
+__all__: list[str] = [
+    "CORRECTION_ID",
+    "DECISION_ID",
+    "PATCH_CONTAINER",
+    "ROOT_DECISION_ID",
+    "build_cli_args",
+    "make_container",
+    "make_decision_ns",
+]

features/steps/tdd_plan_correct_json_output_steps.py

@@ -0,0 +1,192 @@
+"""Step definitions for tdd_plan_correct_json_output.feature.
+
+Captures bug #8584: the ``plan correct`` CLI command's JSON output
+(``--format json``) does not match the spec-required envelope structure.
+
+The current implementation places correction fields directly under ``data``
+(``data.mode``, ``data.correction_id``) instead of nesting them under
+``data.correction``.  This means ``data.correction.mode`` is absent from
+the output.  Additionally, the ``command`` field is empty instead of
+``"plan correct"``.
+
+The assertions verify the *expected* (correct) behaviour.  They will
+**fail** on the current codebase, proving the bug exists.  The
+``@tdd_expected_fail`` tag inverts the result so CI passes.
+
+All step text uses the ``tpcjo`` prefix to avoid collisions with other
+step files.
+
+Bug: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/8584
+"""
+
+from __future__ import annotations
+
+import json
+from unittest.mock import patch
+
+from behave import given, then, when
+from behave.runner import Context
+from typer.testing import CliRunner
+
+from features.mocks.tdd_plan_correct_json_output_fixtures import (
+    PATCH_CONTAINER,
+    build_cli_args,
+    make_container,
+)
+
+runner = CliRunner()
+
+
+def _get_plan_app():
+    """Lazily import the plan CLI app to avoid slow module-level import.
+
+    ``cleveragents.cli.commands.plan`` triggers a large import chain
+    (application container, all services, etc.) that takes ~100 s on a
+    cold interpreter.  Deferring the import to the first step execution
+    means the module is already cached by the time these scenarios run,
+    so the actual cost is near-zero.
+    """
+    from cleveragents.cli.commands.plan import app as plan_app
+
+    return plan_app
+
+
+# ---------------------------------------------------------------------------
+# GIVEN steps
+# ---------------------------------------------------------------------------
+
+
+@given(
+    "tpcjo a container with a plan and a correction service"
+    " that succeeds in revert mode"
+)
+def step_tpcjo_container_revert(context: Context) -> None:
+    """Set up a mock DI container for revert mode."""
+    context.tpcjo_container = make_container(mode="revert")
+    context.tpcjo_mode = "revert"
+
+
+@given(
+    "tpcjo a container with a plan and a correction service"
+    " that succeeds in append mode"
+)
+def step_tpcjo_container_append(context: Context) -> None:
+    """Set up a mock DI container for append mode."""
+    context.tpcjo_container = make_container(mode="append")
+    context.tpcjo_mode = "append"
+
+
+# ---------------------------------------------------------------------------
+# WHEN steps
+# ---------------------------------------------------------------------------
+
+
+@when("tpcjo I invoke plan correct with --format json in revert mode")
+def step_tpcjo_invoke_revert(context: Context) -> None:
+    """Invoke ``plan correct <decision_id> --format json --mode revert``.
+
+    Patches ``get_container`` so the command uses the mock DI container
+    instead of the real application container.
+    """
+    args = build_cli_args(mode="revert")
+    with patch(PATCH_CONTAINER, return_value=context.tpcjo_container):
+        context.tpcjo_result = runner.invoke(_get_plan_app(), args)
+    _parse_json_output(context)
+
+
+@when("tpcjo I invoke plan correct with --format json in append mode")
+def step_tpcjo_invoke_append(context: Context) -> None:
+    """Invoke ``plan correct <decision_id> --format json --mode append``.
+
+    Patches ``get_container`` so the command uses the mock DI container
+    instead of the real application container.
+    """
+    args = build_cli_args(mode="append")
+    with patch(PATCH_CONTAINER, return_value=context.tpcjo_container):
+        context.tpcjo_result = runner.invoke(_get_plan_app(), args)
+    _parse_json_output(context)
+
+
+# ---------------------------------------------------------------------------
+# THEN steps
+# ---------------------------------------------------------------------------
+
+
+@then('tpcjo the JSON output data.correction.mode should be "{expected_mode}"')
+def step_tpcjo_json_correction_mode(context: Context, expected_mode: str) -> None:
+    """Assert the parsed JSON output has data.correction.mode set correctly.
+
+    This assertion will FAIL on the current codebase because the
+    implementation places ``mode`` directly under ``data`` (as ``data.mode``)
+    instead of nesting it under ``data.correction`` (as ``data.correction.mode``).
+    """
+    parsed = context.tpcjo_parsed_json
+    assert "data" in parsed, (
+        f"Bug #8584: JSON output is missing top-level 'data' key.  "
+        f"Current output keys: {list(parsed.keys())}.  "
+        f"Full output: {context.tpcjo_result.output!r}"
+    )
+    data = parsed["data"]
+    assert isinstance(data, dict), (
+        f"Bug #8584: JSON output 'data' value is not a dict: {data!r}"
+    )
+    assert "correction" in data, (
+        f"Bug #8584: JSON output 'data' is missing 'correction' key.  "
+        f"Current data keys: {list(data.keys())}.  "
+        f"The spec requires correction fields to be nested under data.correction, "
+        f"but the current implementation places them directly under data "
+        f"(data.mode, data.correction_id, etc.)."
+    )
+    correction = data["correction"]
+    assert isinstance(correction, dict), (
+        f"Bug #8584: JSON output 'data.correction' is not a dict: {correction!r}"
+    )
+    assert "mode" in correction, (
+        f"Bug #8584: JSON output 'data.correction' is missing 'mode' key.  "
+        f"Current correction keys: {list(correction.keys())}"
+    )
+    actual_mode = correction["mode"]
+    assert actual_mode == expected_mode, (
+        f"Bug #8584: data.correction.mode is {actual_mode!r}, "
+        f"expected {expected_mode!r}"
+    )
+
+
+@then('tpcjo the JSON output command field should be "{expected_command}"')
+def step_tpcjo_json_command_field(context: Context, expected_command: str) -> None:
+    """Assert the parsed JSON output has the correct command field value.
+
+    This assertion will FAIL on the current codebase because the
+    ``command`` field is an empty string instead of ``"plan correct"``.
+    """
+    parsed = context.tpcjo_parsed_json
+    assert "command" in parsed, (
+        f"Bug #8584: JSON output is missing top-level 'command' key.  "
+        f"Current output keys: {list(parsed.keys())}.  "
+        f"Full output: {context.tpcjo_result.output!r}"
+    )
+    actual_command = parsed["command"]
+    assert actual_command == expected_command, (
+        f"Bug #8584: command field is {actual_command!r}, "
+        f"expected {expected_command!r}.  "
+        f"The spec requires command to be 'plan correct' but the current "
+        f"implementation sets it to an empty string."
+    )
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def _parse_json_output(context: Context) -> None:
+    """Parse the CLI output as JSON and store it on the context.
+
+    If the command failed or the output is not valid JSON, stores an
+    empty dict so that subsequent assertions produce clear failure messages.
+    """
+    result = context.tpcjo_result
+    try:
+        context.tpcjo_parsed_json = json.loads(result.output)
+    except (json.JSONDecodeError, ValueError):
+        context.tpcjo_parsed_json = {}

features/tdd_plan_correct_json_output.feature

@@ -0,0 +1,55 @@
+@tdd_issue @tdd_issue_8584
+Feature: TDD Bug #8584 - plan correct JSON output missing spec-required envelope format
+  As a developer
+  I want plan correct --format json to return the standard CLI envelope
+  So that JSON consumers receive the spec-required nested data structure
+
+  The v3.2.0 specification (section CLI Commands - agents plan correct, line 14912 in
+  docs/specification.md) defines the required JSON output envelope for
+  agents plan correct --format json.
+
+  The spec requires the correction data to be nested under data.correction:
+
+      {
+        "command": "plan correct",
+        "status": "ok",
+        "exit_code": 0,
+        "data": {
+          "correction": {
+            "mode": "revert",
+            "impact": "...",
+            ...
+          },
+          "affected_subtree": {...},
+          ...
+        },
+        "timing": {...},
+        "messages": ["Correction applied"]
+      }
+
+  The current implementation places correction fields directly under data
+  (data.mode, data.correction_id) instead of nesting them under data.correction.
+  This means data.correction.mode is absent from the output.
+
+  These scenarios assert the expected (correct) behaviour and validate
+  that the fix is in place.
+
+  All step text uses the tpcjo prefix to avoid collisions with other step files.
+
+  @tdd_issue @tdd_issue_8584
+  Scenario: plan correct --format json data.correction.mode is present in revert mode
+    Given tpcjo a container with a plan and a correction service that succeeds in revert mode
+    When tpcjo I invoke plan correct with --format json in revert mode
+    Then tpcjo the JSON output data.correction.mode should be "revert"
+
+  @tdd_issue @tdd_issue_8584
+  Scenario: plan correct --format json data.correction.mode is present in append mode
+    Given tpcjo a container with a plan and a correction service that succeeds in append mode
+    When tpcjo I invoke plan correct with --format json in append mode
+    Then tpcjo the JSON output data.correction.mode should be "append"
+
+  @tdd_issue @tdd_issue_8584
+  Scenario: plan correct --format json command field is set to plan correct
+    Given tpcjo a container with a plan and a correction service that succeeds in revert mode
+    When tpcjo I invoke plan correct with --format json in revert mode
+    Then tpcjo the JSON output command field should be "plan correct"

src/cleveragents/cli/commands/plan.py

@@ -3507,13 +3507,29 @@ def correct_decision(
 
         if fmt != OutputFormat.RICH.value:
             data = {
+                "correction": {
+                    "mode": correction_mode.value,
+                },
                 "correction_id": result.correction_id,
                 "status": result.status.value,
-                "mode": correction_mode.value,
                 "new_decisions": result.new_decisions,
                 "reverted_decisions": result.reverted_decisions,
             }
-            console.print(format_output(data, fmt))
+            console.print(
+                format_output(
+                    data,
+                    fmt,
+                    command="plan correct",
+                    status="ok",
+                    exit_code=0,
+                    messages=[
+                        {
+                            "level": "ok",
+                            "text": f"Correction applied ({result.correction_id})",
+                        }
+                    ],
+                )
+            )
         else:
             console.print(
                 f"[green]✓[/green] Correction applied: {result.correction_id}"