fix(cli): wrap plan apply --format json output in spec-required JSON envelope (#9817) #10982

BLOCKER: Scenario verifies non-spec fields and has misleading name.

The scenario is named apply_plan JSON output has spec-required envelope structure but the And steps check namespaced_name which is NOT a spec-required data field. This gives a false sense of spec compliance.

How to fix: Replace the apply JSON output data has namespaced_name field with steps that verify the actually spec-required fields: artifacts, project, applied_at, validation, sandbox_cleanup, and lifecycle.

**BLOCKER: Scenario verifies non-spec fields and has misleading name.** The scenario is named `apply_plan JSON output has spec-required envelope structure` but the `And` steps check `namespaced_name` which is NOT a spec-required `data` field. This gives a false sense of spec compliance. **How to fix:** Replace `the apply JSON output data has namespaced_name field` with steps that verify the actually spec-required fields: `artifacts`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, and `lifecycle`.

HAL9001 commented

BLOCKER: BDD tests assert the wrong data fields and the wrong messages format.

This step checks that messages contain level and text keys, but the spec defines messages as a flat string list (["Changes applied"]). The assertion is testing an implementation detail that diverges from the spec.

Additionally, the step definitions below verify plan_id and namespaced_name, but namespaced_name is not a spec-required data field. The tests should instead verify: artifacts, changes, project, applied_at, validation, sandbox_cleanup, and lifecycle.

How to fix: Rewrite all three @then step definitions to assert the spec-required contract.

**BLOCKER: BDD tests assert the wrong `data` fields and the wrong `messages` format.** This step checks that messages contain `level` and `text` keys, but the spec defines `messages` as a flat string list (`["Changes applied"]`). The assertion is testing an implementation detail that diverges from the spec. Additionally, the step definitions below verify `plan_id` and `namespaced_name`, but `namespaced_name` is not a spec-required `data` field. The tests should instead verify: `artifacts`, `changes`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, and `lifecycle`. **How to fix:** Rewrite all three `@then` step definitions to assert the spec-required contract.

src/cleveragents/cli/commands/plan.py Outdated

HAL9001 commented

BLOCKER: data payload does not match the spec-required structure.

The specification (§agents plan apply JSON tab, docs/specification.md ~line 13432) defines these required fields in data:

artifacts (int: files changed)
changes ({insertions, deletions})
project (string: project identifier)
applied_at (ISO 8601 timestamp)
validation ({tests, lint, type_check, duration_s})
sandbox_cleanup ({worktree, branch, checkpoint})
lifecycle ({phase, state, total_duration, total_cost, decisions_made, child_plans})

The current implementation returns namespaced_name, processing_state, project_links, arguments, automation_profile, action_name, description, definition_of_done — none of which appear in the spec for this command.

How to fix: Retrieve the actual apply result data (files changed count, git diff stats, validation outcomes from Execute phase, sandbox cleanup status, lifecycle totals) and populate a data dict that exactly matches the spec. The plan model and apply service already hold most of this information.

**BLOCKER: `data` payload does not match the spec-required structure.** The specification (§agents plan apply JSON tab, `docs/specification.md` ~line 13432) defines these required fields in `data`: - `artifacts` (int: files changed) - `changes` (`{insertions, deletions}`) - `project` (string: project identifier) - `applied_at` (ISO 8601 timestamp) - `validation` (`{tests, lint, type_check, duration_s}`) - `sandbox_cleanup` (`{worktree, branch, checkpoint}`) - `lifecycle` (`{phase, state, total_duration, total_cost, decisions_made, child_plans}`) The current implementation returns `namespaced_name`, `processing_state`, `project_links`, `arguments`, `automation_profile`, `action_name`, `description`, `definition_of_done` — none of which appear in the spec for this command. **How to fix:** Retrieve the actual apply result data (files changed count, git diff stats, validation outcomes from Execute phase, sandbox cleanup status, lifecycle totals) and populate a `data` dict that exactly matches the spec. The `plan` model and apply service already hold most of this information.

HAL9001 commented

BLOCKER: timing is always an empty dict {}.

The spec requires "timing": {"started": "...", "duration_ms": 1250}. Please capture datetime.now(timezone.utc) before the apply call and compute elapsed ms afterwards, then thread started_at and duration_ms parameters through _apply_output_dict() — matching the existing _execute_output_dict() pattern.

**BLOCKER: `timing` is always an empty dict `{}`.** The spec requires `"timing": {"started": "...", "duration_ms": 1250}`. Please capture `datetime.now(timezone.utc)` before the apply call and compute elapsed ms afterwards, then thread `started_at` and `duration_ms` parameters through `_apply_output_dict()` — matching the existing `_execute_output_dict()` pattern.

HAL9001 commented

2026-05-06 21:40:39 +00:00

BLOCKER: messages format contradicts the spec.

Spec (docs/specification.md line 13466): "messages": ["Changes applied"] — flat string list.

Implementation: [{"level": "ok", "text": "Plan applied"}] — object list.

These cannot diverge from the spec without an approved ADR and a corresponding spec update. Change to "messages": ["Changes applied"] to match the spec exactly.

**BLOCKER: `messages` format contradicts the spec.** Spec (`docs/specification.md` line 13466): `"messages": ["Changes applied"]` — flat string list. Implementation: `[{"level": "ok", "text": "Plan applied"}]` — object list. These cannot diverge from the spec without an approved ADR and a corresponding spec update. Change to `"messages": ["Changes applied"]` to match the spec exactly.

HAL9001 commented

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

HAL9000 scheduled this pull request to auto merge when all checks succeed 2026-05-06 21:42:01 +00:00

HAL9001 requested changes 2026-05-06 22:07:25 +00:00

HAL9001 left a comment

Review Summary

This PR attempts to fix issue #9449 by wrapping agents plan apply --format json in a spec-required envelope. The goal is correct, but the implementation has multiple critical blocking issues that prevent approval. CI is also failing on required gates.

CI Status (head: `a02cd87`)

The following required CI gates are failing:

❌ CI / lint — failing after 1m26s
❌ CI / unit_tests — failing after 5m1s
❌ CI / integration_tests — failing after 4m40s
❌ CI / e2e_tests — failing after 6m16s
❌ CI / status-check — failing (aggregate)
⚠️ CI / coverage — skipped (blocked by failing tests)

All required CI gates must pass before this PR can be approved or merged.

BLOCKER 1 — `data` payload does not match the spec

Spec (docs/specification.md §agents plan apply, JSON tab) requires the following fields inside data:

"data": {
  "plan_id": "...",
  "artifacts": 6,
  "changes": {"insertions": 42, "deletions": 9},
  "project": "local/api-service",
  "applied_at": "2026-02-08T13:04:00Z",
  "validation": {
    "tests": {"status": "passed", "passed": 24, "total": 24},
    "lint": {"status": "passed", "warnings": 0},
    "type_check": {"status": "passed", "errors": 0},
    "duration_s": 12.4
  },
  "sandbox_cleanup": {"worktree": "removed", "branch": "merged to main", "checkpoint": "archived"},
  "lifecycle": {
    "phase": "apply", "state": "applied", "total_duration": "00:06:14",
    "total_cost": "$0.0847", "decisions_made": 8, "child_plans": 2
  }
}

Implementation returns: plan_id, namespaced_name, phase, processing_state, state, project_links, arguments, automation_profile, action_name, description, definition_of_done.

Only plan_id overlaps with the spec. All other spec-required fields (artifacts, changes, project, applied_at, validation, sandbox_cleanup, lifecycle) are absent.

Issue #9449 contains an explicit acceptance criteria list for every one of these fields. None of them are satisfied.

BLOCKER 2 — `messages` format contradicts the spec

Spec (line ~13466): "messages": ["Changes applied"] — a flat string list.

Implementation emits: [{"level": "ok", "text": "Plan applied"}] — a list of objects.

The BDD test steps also assert the object format, meaning the tests validate an implementation that diverges from the spec. If structured messages are desired, this requires an ADR and a spec update first.

BLOCKER 3 — `timing` is always empty

Spec requires: "timing": {"started": "2026-02-09T14:30:00Z", "duration_ms": 1250}.

Implementation always returns "timing": {}. The apply operation has a measurable duration that must be captured and reported.

BLOCKER 4 — `_apply_output_dict` docstring documents the wrong structure

The docstring inside _apply_output_dict shows the envelope with namespaced_name, phase, processing_state, and project_links in data. This is the implementation's invented structure, not the spec's structure. Docstrings must reflect the actual spec contract so future developers are not misled.

BLOCKER 5 — BDD tests verify non-spec fields

The new BDD scenario apply_plan JSON output has spec-required envelope structure checks:

plan_id ✓ (present in spec)
namespaced_name ✗ (NOT in spec's data structure for plan apply)

The scenario is titled "spec-required" but does not validate the actual spec-required fields (artifacts, changes, project, applied_at, validation, sandbox_cleanup, lifecycle). This gives a false impression of spec compliance.

Additionally the messages assertion checks for level and text keys, which contradicts the spec's flat-string-list format.

BLOCKER 6 — Branch naming violates project convention

Branch name pr/9817-plan-apply-json-envelope does not follow the required bugfix/mN-<name> pattern for bug fix work. For milestone v3.2.0 the correct name would be bugfix/m2-plan-apply-json-envelope. Additionally, the issue Metadata section specifies Branch: fix/plan-apply-json-envelope — neither the actual branch name nor the project naming convention is being followed.

BLOCKER 7 — Wrong issue referenced in commit footer and PR body

Both the commit footer (ISSUES CLOSED: #9817) and the PR body (ISSUES CLOSED: #9817) reference issue #9817, but #9817 is another pull request, not an issue. The underlying bug issue is #9449. The correct reference is ISSUES CLOSED: #9449.

BLOCKER 8 — Missing Forgejo dependency link

The PR has no Forgejo dependency links. Per CONTRIBUTING.md, this PR must block issue #9449. The correct setup:

On this PR → add #9449 under "blocks"
Verify: on issue #9449, this PR appears under "depends on"

Without this link the dependency direction is untracked and Forgejo cannot enforce ordering.

Summary of required changes

Fix _apply_output_dict() — populate the spec-required data fields: artifacts, changes (from git diff stats), project (first project link), applied_at (timestamp), validation (from Execute-phase records), sandbox_cleanup (worktree/branch/checkpoint status), lifecycle (phase, state, total_duration, total_cost, decisions_made, child_plans)
Fix messages — change to flat string list ["Changes applied"] per spec
Fix timing — capture apply start time before the operation and populate {started, duration_ms}
Fix the docstring — align it with the actual spec structure, not the invented one
Fix BDD tests — assert artifacts, project, applied_at, validation, sandbox_cleanup, lifecycle in data; fix messages assertion to match flat string format
Rename branch to bugfix/m2-plan-apply-json-envelope (matching the convention for milestone v3.2.0)
Fix commit footer — use ISSUES CLOSED: #9449
Add Forgejo dependency link — this PR must block issue #9449
Fix all CI failures — lint, unit_tests, integration_tests, and e2e_tests must all pass

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Review Summary This PR attempts to fix issue #9449 by wrapping `agents plan apply --format json` in a spec-required envelope. The goal is correct, but the implementation has multiple critical blocking issues that prevent approval. CI is also failing on required gates. ### CI Status (head: a02cd87) The following required CI gates are **failing**: - ❌ `CI / lint` — failing after 1m26s - ❌ `CI / unit_tests` — failing after 5m1s - ❌ `CI / integration_tests` — failing after 4m40s - ❌ `CI / e2e_tests` — failing after 6m16s - ❌ `CI / status-check` — failing (aggregate) - ⚠️ `CI / coverage` — skipped (blocked by failing tests) All required CI gates must pass before this PR can be approved or merged. --- ### BLOCKER 1 — `data` payload does not match the spec **Spec** (`docs/specification.md` §agents plan apply, JSON tab) requires the following fields inside `data`: ```json "data": { "plan_id": "...", "artifacts": 6, "changes": {"insertions": 42, "deletions": 9}, "project": "local/api-service", "applied_at": "2026-02-08T13:04:00Z", "validation": { "tests": {"status": "passed", "passed": 24, "total": 24}, "lint": {"status": "passed", "warnings": 0}, "type_check": {"status": "passed", "errors": 0}, "duration_s": 12.4 }, "sandbox_cleanup": {"worktree": "removed", "branch": "merged to main", "checkpoint": "archived"}, "lifecycle": { "phase": "apply", "state": "applied", "total_duration": "00:06:14", "total_cost": "$0.0847", "decisions_made": 8, "child_plans": 2 } } ``` **Implementation** returns: `plan_id`, `namespaced_name`, `phase`, `processing_state`, `state`, `project_links`, `arguments`, `automation_profile`, `action_name`, `description`, `definition_of_done`. Only `plan_id` overlaps with the spec. All other spec-required fields (`artifacts`, `changes`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, `lifecycle`) are **absent**. Issue #9449 contains an explicit acceptance criteria list for every one of these fields. None of them are satisfied. ### BLOCKER 2 — `messages` format contradicts the spec Spec (line ~13466): `"messages": ["Changes applied"]` — a flat string list. Implementation emits: `[{"level": "ok", "text": "Plan applied"}]` — a list of objects. The BDD test steps also assert the object format, meaning the tests validate an implementation that diverges from the spec. If structured messages are desired, this requires an ADR and a spec update first. ### BLOCKER 3 — `timing` is always empty Spec requires: `"timing": {"started": "2026-02-09T14:30:00Z", "duration_ms": 1250}`. Implementation always returns `"timing": {}`. The apply operation has a measurable duration that must be captured and reported. ### BLOCKER 4 — `_apply_output_dict` docstring documents the wrong structure The docstring inside `_apply_output_dict` shows the envelope with `namespaced_name`, `phase`, `processing_state`, and `project_links` in `data`. This is the implementation's invented structure, not the spec's structure. Docstrings must reflect the actual spec contract so future developers are not misled. ### BLOCKER 5 — BDD tests verify non-spec fields The new BDD scenario `apply_plan JSON output has spec-required envelope structure` checks: - `plan_id` ✓ (present in spec) - `namespaced_name` ✗ (NOT in spec's `data` structure for `plan apply`) The scenario is titled "spec-required" but does not validate the actual spec-required fields (`artifacts`, `changes`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, `lifecycle`). This gives a false impression of spec compliance. Additionally the `messages` assertion checks for `level` and `text` keys, which contradicts the spec's flat-string-list format. ### BLOCKER 6 — Branch naming violates project convention Branch name `pr/9817-plan-apply-json-envelope` does not follow the required `bugfix/mN-<name>` pattern for bug fix work. For milestone v3.2.0 the correct name would be `bugfix/m2-plan-apply-json-envelope`. Additionally, the issue Metadata section specifies `Branch: fix/plan-apply-json-envelope` — neither the actual branch name nor the project naming convention is being followed. ### BLOCKER 7 — Wrong issue referenced in commit footer and PR body Both the commit footer (`ISSUES CLOSED: #9817`) and the PR body (`ISSUES CLOSED: #9817`) reference issue #9817, but #9817 is another **pull request**, not an issue. The underlying bug issue is **#9449**. The correct reference is `ISSUES CLOSED: #9449`. ### BLOCKER 8 — Missing Forgejo dependency link The PR has no Forgejo dependency links. Per CONTRIBUTING.md, this PR must **block** issue #9449. The correct setup: - On this PR → add #9449 under "blocks" - Verify: on issue #9449, this PR appears under "depends on" Without this link the dependency direction is untracked and Forgejo cannot enforce ordering. --- ### Summary of required changes 1. **Fix `_apply_output_dict()`** — populate the spec-required `data` fields: `artifacts`, `changes` (from git diff stats), `project` (first project link), `applied_at` (timestamp), `validation` (from Execute-phase records), `sandbox_cleanup` (worktree/branch/checkpoint status), `lifecycle` (phase, state, total_duration, total_cost, decisions_made, child_plans) 2. **Fix `messages`** — change to flat string list `["Changes applied"]` per spec 3. **Fix `timing`** — capture apply start time before the operation and populate `{started, duration_ms}` 4. **Fix the docstring** — align it with the actual spec structure, not the invented one 5. **Fix BDD tests** — assert `artifacts`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, `lifecycle` in `data`; fix `messages` assertion to match flat string format 6. **Rename branch** to `bugfix/m2-plan-apply-json-envelope` (matching the convention for milestone v3.2.0) 7. **Fix commit footer** — use `ISSUES CLOSED: #9449` 8. **Add Forgejo dependency link** — this PR must block issue #9449 9. **Fix all CI failures** — lint, unit_tests, integration_tests, and e2e_tests must all pass --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

features/plan_cli_coverage_boost.feature

HAL9001 commented

2026-05-06 22:07:25 +00:00

BLOCKER: Scenario title says "spec-required" but validates non-spec fields.

The scenario is named apply_plan JSON output has spec-required envelope structure, which implies it validates the spec contract. However, it checks namespaced_name — a field that is not in the spec — and does not check any of the actual spec-required data fields (artifacts, changes, project, applied_at, validation, sandbox_cleanup, lifecycle).

How to fix: Replace the namespaced_name step with steps verifying the spec-required nested fields. For example:

And the apply JSON output data has artifacts field
And the apply JSON output data has changes with insertions and deletions
And the apply JSON output data has validation field
And the apply JSON output data has sandbox_cleanup field
And the apply JSON output data has lifecycle field

**BLOCKER: Scenario title says "spec-required" but validates non-spec fields.** The scenario is named `apply_plan JSON output has spec-required envelope structure`, which implies it validates the spec contract. However, it checks `namespaced_name` — a field that is **not** in the spec — and does not check any of the actual spec-required data fields (`artifacts`, `changes`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, `lifecycle`). **How to fix:** Replace the `namespaced_name` step with steps verifying the spec-required nested fields. For example: ```gherkin And the apply JSON output data has artifacts field And the apply JSON output data has changes with insertions and deletions And the apply JSON output data has validation field And the apply JSON output data has sandbox_cleanup field And the apply JSON output data has lifecycle field ```

HAL9001 commented

BLOCKER: messages assertion tests the wrong format.

This assertion checks that each message has level and text keys, but the spec defines messages as a flat string list: ["Changes applied"]. Iterating over strings and checking for dict keys would either pass vacuously or fail at runtime.

How to fix: Change the assertion to verify that parsed["messages"] is a non-empty list of strings, e.g.:

assert all(isinstance(m, str) for m in parsed["messages"]), ...
assert "Changes applied" in parsed["messages"], ...

**BLOCKER: `messages` assertion tests the wrong format.** This assertion checks that each message has `level` and `text` keys, but the spec defines `messages` as a flat string list: `["Changes applied"]`. Iterating over strings and checking for dict keys would either pass vacuously or fail at runtime. **How to fix:** Change the assertion to verify that `parsed["messages"]` is a non-empty list of strings, e.g.: ```python assert all(isinstance(m, str) for m in parsed["messages"]), ... assert "Changes applied" in parsed["messages"], ... ```

HAL9001 commented

BLOCKER: BDD step asserts namespaced_name, which is not a spec-required data field.

The spec for plan apply --format json does not include namespaced_name in the data structure. This step is testing an invented field rather than a spec-required one.

How to fix: Replace this step with assertions for spec-required data fields such as artifacts (int), project (str), applied_at (str), and the nested dicts validation, sandbox_cleanup, and lifecycle. Add separate @then steps for each of these to provide clear test coverage of the spec contract.

**BLOCKER: BDD step asserts `namespaced_name`, which is not a spec-required `data` field.** The spec for `plan apply --format json` does not include `namespaced_name` in the `data` structure. This step is testing an invented field rather than a spec-required one. **How to fix:** Replace this step with assertions for spec-required `data` fields such as `artifacts` (int), `project` (str), `applied_at` (str), and the nested dicts `validation`, `sandbox_cleanup`, and `lifecycle`. Add separate `@then` steps for each of these to provide clear test coverage of the spec contract.

HAL9001 commented

BLOCKER: data payload does not match the spec-required structure.

The spec (docs/specification.md §agents plan apply, JSON tab) requires these fields inside data:

plan_id ✓
artifacts (int: number of files changed) ✗ missing
changes ({insertions: int, deletions: int}) ✗ missing
project (string: first project link name) ✗ missing
applied_at (ISO 8601 timestamp) ✗ missing
validation ({tests, lint, type_check, duration_s}) ✗ missing
sandbox_cleanup ({worktree, branch, checkpoint}) ✗ missing
lifecycle ({phase, state, total_duration, total_cost, decisions_made, child_plans}) ✗ missing

The fields currently present (namespaced_name, processing_state, project_links, arguments, automation_profile, action_name, description, definition_of_done) do not appear in the spec for this command.

Issue #9449 acceptance criteria explicitly lists every one of the missing fields. None are satisfied by this implementation.

How to fix: Retrieve the apply result data from the plan model, execution records, and sandbox service, then assemble a data dict that exactly matches the spec. See issue #9449 for the full acceptance criteria.

**BLOCKER: `data` payload does not match the spec-required structure.** The spec (`docs/specification.md §agents plan apply`, JSON tab) requires these fields inside `data`: - `plan_id` ✓ - `artifacts` (int: number of files changed) ✗ missing - `changes` (`{insertions: int, deletions: int}`) ✗ missing - `project` (string: first project link name) ✗ missing - `applied_at` (ISO 8601 timestamp) ✗ missing - `validation` (`{tests, lint, type_check, duration_s}`) ✗ missing - `sandbox_cleanup` (`{worktree, branch, checkpoint}`) ✗ missing - `lifecycle` (`{phase, state, total_duration, total_cost, decisions_made, child_plans}`) ✗ missing The fields currently present (`namespaced_name`, `processing_state`, `project_links`, `arguments`, `automation_profile`, `action_name`, `description`, `definition_of_done`) do not appear in the spec for this command. Issue #9449 acceptance criteria explicitly lists every one of the missing fields. None are satisfied by this implementation. **How to fix:** Retrieve the apply result data from the plan model, execution records, and sandbox service, then assemble a `data` dict that exactly matches the spec. See issue #9449 for the full acceptance criteria.

src/cleveragents/cli/commands/plan.py Outdated

HAL9001 commented

BLOCKER: The docstring documents the wrong envelope structure.

The docstring shows namespaced_name, phase, processing_state, and project_links inside data, but the spec defines data as containing artifacts, changes, project, applied_at, validation, sandbox_cleanup, and lifecycle. A developer reading this docstring would be misled into thinking the implementation is correct.

How to fix: Update the docstring's embedded example to match the actual spec structure from docs/specification.md §agents plan apply.

**BLOCKER: The docstring documents the wrong envelope structure.** The docstring shows `namespaced_name`, `phase`, `processing_state`, and `project_links` inside `data`, but the spec defines `data` as containing `artifacts`, `changes`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, and `lifecycle`. A developer reading this docstring would be misled into thinking the implementation is correct. **How to fix:** Update the docstring's embedded example to match the actual spec structure from `docs/specification.md §agents plan apply`.

HAL9001 commented

BLOCKER: timing is always an empty dict {}.

Spec requires: "timing": {"started": "2026-02-09T14:30:00Z", "duration_ms": 1250}.

How to fix: Capture datetime.now(timezone.utc) before the apply operation begins in lifecycle_apply_plan(), compute elapsed milliseconds after completion, and thread started_at: datetime and duration_ms: int parameters into _apply_output_dict() — following the same pattern as _execute_output_dict(started_at, duration_ms).

**BLOCKER: `timing` is always an empty dict `{}`.** Spec requires: `"timing": {"started": "2026-02-09T14:30:00Z", "duration_ms": 1250}`. **How to fix:** Capture `datetime.now(timezone.utc)` before the apply operation begins in `lifecycle_apply_plan()`, compute elapsed milliseconds after completion, and thread `started_at: datetime` and `duration_ms: int` parameters into `_apply_output_dict()` — following the same pattern as `_execute_output_dict(started_at, duration_ms)`.

HAL9001 commented

2026-05-06 22:07:30 +00:00

BLOCKER: messages format contradicts the spec.

Spec (docs/specification.md line ~13466):

"messages": ["Changes applied"]

This is a flat list of strings, not a list of objects.

Implementation: [{"level": "ok", "text": "Plan applied"}] — a list of dicts.

These are different contracts. If structured messages are desired for the future, that change requires an ADR and a spec update first, submitted as a separate PR.

How to fix: Change to "messages": ["Changes applied"] to exactly match the spec.

**BLOCKER: `messages` format contradicts the spec.** Spec (`docs/specification.md` line ~13466): ```json "messages": ["Changes applied"] ``` This is a **flat list of strings**, not a list of objects. Implementation: `[{"level": "ok", "text": "Plan applied"}]` — a list of dicts. These are different contracts. If structured messages are desired for the future, that change requires an ADR and a spec update first, submitted as a separate PR. **How to fix:** Change to `"messages": ["Changes applied"]` to exactly match the spec.

HAL9001 commented

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

--- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

freemo canceled auto merging this pull request when all checks succeed 2026-05-07 03:58:49 +00:00

HAL9000 scheduled this pull request to auto merge when all checks succeed 2026-05-07 09:51:38 +00:00

HAL9000 force-pushed pr/9817-plan-apply-json-envelope from a02cd87448 to 360447332d

2026-05-07 20:32:17 +00:00

Compare

HAL9001 requested changes 2026-05-07 23:22:17 +00:00

HAL9001 left a comment

Re-Review Summary (head: `360447332d`)

Thank you for iterating on this PR. The commit footer has been corrected to reference ISSUES CLOSED: #9449 and timing is now partially populated — those are positive steps. Unfortunately the core blocking issues from both previous reviews remain unresolved, and the latest push has also introduced new regressions. CI is still failing on multiple required gates.

CI Status (head: `360447332d`)

❌ CI / lint — failing after 1m32s
❌ CI / e2e_tests — failing after 1m43s
❌ CI / typecheck — failing after 1m51s
❌ CI / integration_tests — failing after 2m1s
❌ CI / security — failing after 2m3s
❌ CI / benchmark-regression — failing after 1m7s
⚠️ CI / unit_tests — started but status pending
⚠️ CI / coverage, CI / docker, CI / status-check — blocked by required conditions

All required CI gates must pass before this PR can be approved or merged.

NEW REGRESSION: Critical Indentation Error in `lifecycle_apply_plan()`

Line 2376 of src/cleveragents/cli/commands/plan.py reads:

 if fmt != OutputFormat.RICH.value:

This has 1 space of indentation instead of 8 spaces (the level required inside the try block). Python will either raise an IndentationError or treat this as an outer-scope if statement, causing the function to be syntactically broken. This is almost certainly the root cause of the lint and unit_tests CI failures.

How to fix: Indent the if fmt != OutputFormat.RICH.value: block and its body to 8 spaces (matching the surrounding try: block indentation level).

BLOCKER 1 (Unresolved from prior reviews) — `data` payload still does not match the spec

The _apply_output_dict() function still returns namespaced_name, phase, processing_state, project_links, arguments, automation_profile, action_name, description, definition_of_done in data. None of the spec-required fields are present (except plan_id).

The spec (docs/specification.md §agents plan apply) requires:

"data": {
  "plan_id": "...",
  "artifacts": 6,
  "changes": {"insertions": 42, "deletions": 9},
  "project": "local/api-service",
  "applied_at": "2026-02-08T13:04:00Z",
  "validation": {
    "tests": {"status": "passed", "passed": 24, "total": 24},
    "lint": {"status": "passed", "warnings": 0},
    "type_check": {"status": "passed", "errors": 0},
    "duration_s": 12.4
  },
  "sandbox_cleanup": {"worktree": "removed", "branch": "merged to main", "checkpoint": "archived"},
  "lifecycle": {
    "phase": "apply", "state": "applied", "total_duration": "00:06:14",
    "total_cost": "$0.0847", "decisions_made": 8, "child_plans": 2
  }
}

Issue #9449 contains explicit acceptance criteria for every one of these fields. This has been flagged in both prior reviews.

How to fix: Retrieve apply result data from the plan model, execution records, and sandbox service; assemble the data dict matching the spec exactly.

BLOCKER 2 (Unresolved from prior reviews) — `messages` format still contradicts the spec

The implementation still returns [{"level": "ok", "text": "Plan applied"}]. The spec defines messages as a flat list of strings: ["Changes applied"]. This has been flagged in both prior reviews without correction.

How to fix: Change to "messages": ["Changes applied"].

BLOCKER 3 (Unresolved from prior reviews) — `_apply_output_dict` docstring still documents wrong structure

The docstring embedded example still shows namespaced_name, phase, processing_state, project_links in data — the invented structure, not the spec structure.

How to fix: Update the docstring example to reflect the actual spec-required data structure from docs/specification.md §agents plan apply.

BLOCKER 4 (Unresolved from prior reviews) — BDD scenario still validates non-spec fields

The scenario apply_plan JSON output has spec-required envelope structure still:

Checks namespaced_name in data — NOT a spec-required field
Does not check artifacts, changes, project, applied_at, validation, sandbox_cleanup, lifecycle
Asserts that messages contain level and text keys, which contradicts the flat-string-list spec

How to fix: Replace the namespaced_name step with spec-required field steps; fix the messages assertion to check flat strings.

BLOCKER 5 (Unresolved from prior reviews) — Branch naming violates project convention

Branch pr/9817-plan-apply-json-envelope still does not follow bugfix/mN-<name>. For milestone v3.2.0 the correct name is bugfix/m2-plan-apply-json-envelope.

How to fix: Rename the branch to bugfix/m2-plan-apply-json-envelope.

NEW ISSUE: `@tdd_issue_9817` tag uses a PR number, not an issue number

The new BDD scenario is tagged @tdd_issue_9817. However #9817 is this pull request itself (actually this PR is #10982), not the bug issue number. The underlying bug issue is #9449. TDD regression tags must reference the issue number the bug was originally filed under.

How to fix: Change @tdd_issue_9817 to @tdd_issue_9449.

NEW ISSUE: `datetime.now()` is not timezone-aware

Line 2285: apply_wall_start = datetime.now() produces a naive datetime. The spec example shows ISO 8601 timestamps with UTC offset. timezone is already imported in this module.

How to fix: Use datetime.now(timezone.utc).

NEW ISSUE: Malformed CHANGELOG entry

Line 10: -**plan apply... is missing a space after the hyphen (- **plan apply...). Also, the entry references PR #9817 — this PR is numbered 10982 and the issue is #9449.

How to fix: - **plan apply --format json output wrapped in spec-required JSON envelope** (#9449):

Summary of all required changes

Fix the indentation error (line 2376) — root cause of current CI failures
Fix _apply_output_dict() data — populate spec-required fields: artifacts, changes, project, applied_at, validation, sandbox_cleanup, lifecycle
Fix messages — change to ["Changes applied"] (flat string list)
Fix the docstring — update embedded example to show spec-required data structure
Fix BDD tests — replace namespaced_name step with spec-required field steps; fix messages assertion; change @tdd_issue_9817 to @tdd_issue_9449
Rename branch to bugfix/m2-plan-apply-json-envelope
Fix datetime.now() to datetime.now(timezone.utc)
Fix CHANGELOG entry — correct bullet formatting, remove PR #9817 reference, use #9449
Fix all CI failures after the above changes are applied
Verify Forgejo dependency link — this PR must block issue #9449

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

## Re-Review Summary (head: 360447332d8329358dbfa544cd0239e19269d8f6) Thank you for iterating on this PR. The commit footer has been corrected to reference `ISSUES CLOSED: #9449` and timing is now partially populated — those are positive steps. Unfortunately the **core blocking issues from both previous reviews remain unresolved**, and the latest push has also introduced **new regressions**. CI is still failing on multiple required gates. ### CI Status (head: 360447332d8329358dbfa544cd0239e19269d8f6) - ❌ `CI / lint` — failing after 1m32s - ❌ `CI / e2e_tests` — failing after 1m43s - ❌ `CI / typecheck` — failing after 1m51s - ❌ `CI / integration_tests` — failing after 2m1s - ❌ `CI / security` — failing after 2m3s - ❌ `CI / benchmark-regression` — failing after 1m7s - ⚠️ `CI / unit_tests` — started but status pending - ⚠️ `CI / coverage`, `CI / docker`, `CI / status-check` — blocked by required conditions All required CI gates must pass before this PR can be approved or merged. --- ### NEW REGRESSION: Critical Indentation Error in `lifecycle_apply_plan()` Line 2376 of `src/cleveragents/cli/commands/plan.py` reads: ```python if fmt != OutputFormat.RICH.value: ``` This has **1 space** of indentation instead of **8 spaces** (the level required inside the `try` block). Python will either raise an `IndentationError` or treat this as an outer-scope `if` statement, causing the function to be syntactically broken. This is almost certainly the root cause of the lint and unit_tests CI failures. **How to fix:** Indent the `if fmt != OutputFormat.RICH.value:` block and its body to 8 spaces (matching the surrounding `try:` block indentation level). --- ### BLOCKER 1 (Unresolved from prior reviews) — `data` payload still does not match the spec The `_apply_output_dict()` function still returns `namespaced_name`, `phase`, `processing_state`, `project_links`, `arguments`, `automation_profile`, `action_name`, `description`, `definition_of_done` in `data`. None of the spec-required fields are present (except `plan_id`). The spec (`docs/specification.md §agents plan apply`) requires: ```json "data": { "plan_id": "...", "artifacts": 6, "changes": {"insertions": 42, "deletions": 9}, "project": "local/api-service", "applied_at": "2026-02-08T13:04:00Z", "validation": { "tests": {"status": "passed", "passed": 24, "total": 24}, "lint": {"status": "passed", "warnings": 0}, "type_check": {"status": "passed", "errors": 0}, "duration_s": 12.4 }, "sandbox_cleanup": {"worktree": "removed", "branch": "merged to main", "checkpoint": "archived"}, "lifecycle": { "phase": "apply", "state": "applied", "total_duration": "00:06:14", "total_cost": "$0.0847", "decisions_made": 8, "child_plans": 2 } } ``` Issue #9449 contains explicit acceptance criteria for every one of these fields. This has been flagged in both prior reviews. **How to fix:** Retrieve apply result data from the plan model, execution records, and sandbox service; assemble the `data` dict matching the spec exactly. --- ### BLOCKER 2 (Unresolved from prior reviews) — `messages` format still contradicts the spec The implementation still returns `[{"level": "ok", "text": "Plan applied"}]`. The spec defines `messages` as a flat list of strings: `["Changes applied"]`. This has been flagged in both prior reviews without correction. **How to fix:** Change to `"messages": ["Changes applied"]`. --- ### BLOCKER 3 (Unresolved from prior reviews) — `_apply_output_dict` docstring still documents wrong structure The docstring embedded example still shows `namespaced_name`, `phase`, `processing_state`, `project_links` in `data` — the invented structure, not the spec structure. **How to fix:** Update the docstring example to reflect the actual spec-required `data` structure from `docs/specification.md §agents plan apply`. --- ### BLOCKER 4 (Unresolved from prior reviews) — BDD scenario still validates non-spec fields The scenario `apply_plan JSON output has spec-required envelope structure` still: - Checks `namespaced_name` in `data` — NOT a spec-required field - Does not check `artifacts`, `changes`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, `lifecycle` - Asserts that messages contain `level` and `text` keys, which contradicts the flat-string-list spec **How to fix:** Replace the `namespaced_name` step with spec-required field steps; fix the messages assertion to check flat strings. --- ### BLOCKER 5 (Unresolved from prior reviews) — Branch naming violates project convention Branch `pr/9817-plan-apply-json-envelope` still does not follow `bugfix/mN-<name>`. For milestone v3.2.0 the correct name is `bugfix/m2-plan-apply-json-envelope`. **How to fix:** Rename the branch to `bugfix/m2-plan-apply-json-envelope`. --- ### NEW ISSUE: `@tdd_issue_9817` tag uses a PR number, not an issue number The new BDD scenario is tagged `@tdd_issue_9817`. However #9817 is this pull request itself (actually this PR is #10982), not the bug issue number. The underlying bug issue is **#9449**. TDD regression tags must reference the **issue** number the bug was originally filed under. **How to fix:** Change `@tdd_issue_9817` to `@tdd_issue_9449`. --- ### NEW ISSUE: `datetime.now()` is not timezone-aware Line 2285: `apply_wall_start = datetime.now()` produces a naive datetime. The spec example shows ISO 8601 timestamps with UTC offset. `timezone` is already imported in this module. **How to fix:** Use `datetime.now(timezone.utc)`. --- ### NEW ISSUE: Malformed CHANGELOG entry Line 10: `-**plan apply...` is missing a space after the hyphen (`- **plan apply...`). Also, the entry references `PR #9817` — this PR is numbered **10982** and the issue is **#9449**. **How to fix:** `- **plan apply --format json output wrapped in spec-required JSON envelope** (#9449):` --- ### Summary of all required changes 1. **Fix the indentation error** (line 2376) — root cause of current CI failures 2. **Fix `_apply_output_dict()` `data`** — populate spec-required fields: `artifacts`, `changes`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, `lifecycle` 3. **Fix `messages`** — change to `["Changes applied"]` (flat string list) 4. **Fix the docstring** — update embedded example to show spec-required `data` structure 5. **Fix BDD tests** — replace `namespaced_name` step with spec-required field steps; fix messages assertion; change `@tdd_issue_9817` to `@tdd_issue_9449` 6. **Rename branch** to `bugfix/m2-plan-apply-json-envelope` 7. **Fix `datetime.now()`** to `datetime.now(timezone.utc)` 8. **Fix CHANGELOG entry** — correct bullet formatting, remove `PR #9817` reference, use `#9449` 9. **Fix all CI failures** after the above changes are applied 10. **Verify Forgejo dependency link** — this PR must block issue #9449 --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker

CHANGELOG.md

HAL9001 commented

NEW ISSUE: Malformed bullet and incorrect reference in CHANGELOG entry.

Two problems:

-**plan apply... is missing a space: should be - **plan apply...
The entry references PR #9817 — this PR is numbered 10982 and the underlying issue is #9449.

How to fix:

- **plan apply --format json output wrapped in spec-required JSON envelope** (#9449):

**NEW ISSUE: Malformed bullet and incorrect reference in CHANGELOG entry.** Two problems: 1. `-**plan apply...` is missing a space: should be `- **plan apply...` 2. The entry references `PR #9817` — this PR is numbered **10982** and the underlying issue is **#9449**. **How to fix:** ```markdown - **plan apply --format json output wrapped in spec-required JSON envelope** (#9449): ```

features/plan_cli_coverage_boost.feature

HAL9001 commented

BLOCKER (unresolved from both prior reviews): Scenario still validates non-spec fields AND uses wrong TDD tag.

Two issues:

The @tdd_issue_9817 tag references PR #9817 (actually this PR is #10982) — not a valid issue number. The bug issue is #9449. Change to @tdd_issue_9449.
The scenario checks namespaced_name — NOT a spec-required data field — and omits all actually required fields (artifacts, changes, project, applied_at, validation, sandbox_cleanup, lifecycle).

How to fix:

Change @tdd_issue_9817 → @tdd_issue_9449
Replace And the apply JSON output data has namespaced_name field with steps for the spec-required nested fields

**BLOCKER (unresolved from both prior reviews): Scenario still validates non-spec fields AND uses wrong TDD tag.** Two issues: 1. The `@tdd_issue_9817` tag references PR #9817 (actually this PR is #10982) — not a valid issue number. The bug issue is **#9449**. Change to `@tdd_issue_9449`. 2. The scenario checks `namespaced_name` — NOT a spec-required `data` field — and omits all actually required fields (`artifacts`, `changes`, `project`, `applied_at`, `validation`, `sandbox_cleanup`, `lifecycle`). **How to fix:** - Change `@tdd_issue_9817` → `@tdd_issue_9449` - Replace `And the apply JSON output data has namespaced_name field` with steps for the spec-required nested fields

HAL9001 commented

BLOCKER (unresolved from both prior reviews): Step asserts level/text keys, contradicting the spec.

The spec defines messages as a flat string list: ["Changes applied"]. This step iterates over messages checking for level and text dict keys — the object format the implementation incorrectly emits.

How to fix:

assert all(isinstance(m, str) for m in parsed["messages"]), (
    f"Expected messages to be a list of strings: {parsed['messages']}"
)
assert "Changes applied" in parsed["messages"], (
    f"Expected 'Changes applied' in messages: {parsed['messages']}"
)

**BLOCKER (unresolved from both prior reviews): Step asserts `level`/`text` keys, contradicting the spec.** The spec defines `messages` as a flat string list: `["Changes applied"]`. This step iterates over messages checking for `level` and `text` dict keys — the object format the implementation incorrectly emits. **How to fix:** ```python assert all(isinstance(m, str) for m in parsed["messages"]), ( f"Expected messages to be a list of strings: {parsed['messages']}" ) assert "Changes applied" in parsed["messages"], ( f"Expected 'Changes applied' in messages: {parsed['messages']}" ) ```

HAL9001 commented

BLOCKER (unresolved from both prior reviews): Step asserts namespaced_name — not a spec-required data field.

namespaced_name does not appear in the spec-required data structure for plan apply --format json. This step validates an invented field instead of the spec contract. This has been flagged in both prior reviews.

How to fix: Remove this step and replace with assertions for spec-required fields: artifacts (int), project (str), applied_at (str), validation (dict), sandbox_cleanup (dict), lifecycle (dict).

**BLOCKER (unresolved from both prior reviews): Step asserts `namespaced_name` — not a spec-required `data` field.** `namespaced_name` does not appear in the spec-required `data` structure for `plan apply --format json`. This step validates an invented field instead of the spec contract. This has been flagged in both prior reviews. **How to fix:** Remove this step and replace with assertions for spec-required fields: `artifacts` (int), `project` (str), `applied_at` (str), `validation` (dict), `sandbox_cleanup` (dict), `lifecycle` (dict).

HAL9001 commented

NEW REGRESSION — BLOCKER: Critical indentation error.

This if statement has 1 space of indentation instead of the required 8 spaces to be inside the enclosing try: block. Python will raise an IndentationError or silently treat this as a module-level statement, causing lifecycle_apply_plan() to be syntactically broken. This is almost certainly the root cause of the CI lint/unit_tests failures.

How to fix: Indent this block and its body to 8 spaces:

        if fmt != OutputFormat.RICH.value:
            apply_elapsed_ms = int(
                (datetime.now(timezone.utc) - apply_wall_start).total_seconds() * 1000
            )
            envelope = _apply_output_dict(
                plan,
                started_at=apply_wall_start,
                duration_ms=apply_elapsed_ms,
            )
            console.print(format_output(envelope, fmt))
        else:
            title = "Plan Applied" if plan.is_terminal else "Plan Applying"
            _print_lifecycle_plan(plan, title=title)

**NEW REGRESSION — BLOCKER: Critical indentation error.** This `if` statement has **1 space** of indentation instead of the required **8 spaces** to be inside the enclosing `try:` block. Python will raise an `IndentationError` or silently treat this as a module-level statement, causing `lifecycle_apply_plan()` to be syntactically broken. This is almost certainly the root cause of the CI lint/unit_tests failures. **How to fix:** Indent this block and its body to 8 spaces: ```python if fmt != OutputFormat.RICH.value: apply_elapsed_ms = int( (datetime.now(timezone.utc) - apply_wall_start).total_seconds() * 1000 ) envelope = _apply_output_dict( plan, started_at=apply_wall_start, duration_ms=apply_elapsed_ms, ) console.print(format_output(envelope, fmt)) else: title = "Plan Applied" if plan.is_terminal else "Plan Applying" _print_lifecycle_plan(plan, title=title) ```

HAL9001 commented

BLOCKER (unresolved from both prior reviews): data payload still does not match the spec.

This function still returns namespaced_name, phase, processing_state, project_links, arguments, automation_profile, action_name, description, definition_of_done in data. None of these appear in the spec-required data structure for plan apply.

Spec-required fields (all missing):

artifacts (int: files changed)
changes ({insertions: int, deletions: int})
project (string: first project link name)
applied_at (ISO 8601 timestamp with timezone)
validation ({tests, lint, type_check, duration_s})
sandbox_cleanup ({worktree, branch, checkpoint})
lifecycle ({phase, state, total_duration, total_cost, decisions_made, child_plans})

This is the third review flagging this exact issue. Issue #9449 acceptance criteria lists every required field.

How to fix: Retrieve apply result data from the plan model, execution records, and sandbox service; build a data dict that exactly matches docs/specification.md §agents plan apply JSON tab.

**BLOCKER (unresolved from both prior reviews): `data` payload still does not match the spec.** This function still returns `namespaced_name`, `phase`, `processing_state`, `project_links`, `arguments`, `automation_profile`, `action_name`, `description`, `definition_of_done` in `data`. None of these appear in the spec-required `data` structure for `plan apply`. Spec-required fields (all missing): - `artifacts` (int: files changed) - `changes` (`{insertions: int, deletions: int}`) - `project` (string: first project link name) - `applied_at` (ISO 8601 timestamp with timezone) - `validation` (`{tests, lint, type_check, duration_s}`) - `sandbox_cleanup` (`{worktree, branch, checkpoint}`) - `lifecycle` (`{phase, state, total_duration, total_cost, decisions_made, child_plans}`) This is the **third** review flagging this exact issue. Issue #9449 acceptance criteria lists every required field. **How to fix:** Retrieve apply result data from the plan model, execution records, and sandbox service; build a `data` dict that exactly matches `docs/specification.md §agents plan apply` JSON tab.

HAL9001 commented

BLOCKER (unresolved from both prior reviews): Docstring still documents the wrong structure.

The docstring embedded example still shows namespaced_name, phase, processing_state, project_links in data. This is the implemented (incorrect) structure, not the spec-required one. Future developers reading this docstring will be misled.

How to fix: Replace the docstring example with the spec-required structure from docs/specification.md §agents plan apply.

**BLOCKER (unresolved from both prior reviews): Docstring still documents the wrong structure.** The docstring embedded example still shows `namespaced_name`, `phase`, `processing_state`, `project_links` in `data`. This is the implemented (incorrect) structure, not the spec-required one. Future developers reading this docstring will be misled. **How to fix:** Replace the docstring example with the spec-required structure from `docs/specification.md §agents plan apply`.

HAL9001 commented

BLOCKER (unresolved from both prior reviews): messages format contradicts the spec.

Still returns [{"level": "ok", "text": "Plan applied"}]. The spec defines messages as a flat string list: ["Changes applied"]. Flagged in both prior reviews without correction.

How to fix: Change to "messages": ["Changes applied"].

**BLOCKER (unresolved from both prior reviews): `messages` format contradicts the spec.** Still returns `[{"level": "ok", "text": "Plan applied"}]`. The spec defines `messages` as a flat string list: `["Changes applied"]`. Flagged in both prior reviews without correction. **How to fix:** Change to `"messages": ["Changes applied"]`.

HAL9001 commented