cleveragents/cleveragents-core

Fork 3

bug(cli): plan correct expects decision_id as first positional but M3 acceptance test passes plan_id #969

New issue

Closed

opened 2026-03-16 03:16:58 +00:00 by freemo · 2 comments

freemo commented

2026-03-16 03:16:58 +00:00

Owner

Metadata

Commit Message: fix(cli): make plan correct accept plan_id as primary identifier
Branch: bugfix/m3-plan-correct-plan-id

Background and Context

The M3 (v3.2.0) acceptance test (robot/e2e/m3_acceptance.robot, PR #799) calls plan correct with a plan ID as the first positional argument:

${r_correct}=    Run CleverAgents Command
...    plan    correct    ${plan_id}    --mode    revert
...    --guidance    Use SQLAlchemy ORM instead of raw SQL
...    --yes    --format    plain

The M3 milestone acceptance criteria state: "agents plan correct --mode=revert re-executes from the targeted decision point." The natural CLI pattern is plan correct <plan_id> --mode revert --guidance "..." to correct decisions in a plan.

Current Behavior

The correct_decision function in src/cleveragents/cli/commands/plan.py (line ~2310) declares its first positional argument as decision_id:

def correct_decision(
    decision_id: Annotated[str, typer.Argument(help="Decision ID to correct")],
    ...

When the M3 test passes a plan ID, the plan ID is used as target_decision_id in the call to svc.request_correction(plan_id=resolved_plan_id, target_decision_id=decision_id, ...). Since the plan ID is not a valid decision ID, the correction service raises an error (the targeted decision cannot be found in the plan's decision tree). The CLI catches the error and aborts with rc!=0.

The command does have a --plan option for specifying the plan ID separately, but the M3 test passes the plan ID as the first positional argument, which is the natural usage pattern.

Expected Behavior

plan correct <plan_id> --mode revert --guidance "..." accepts a plan ID as the first positional argument
When the identifier is recognized as a plan ID, the command automatically selects the root decision for that plan as the correction target
The correction is applied to the root decision of the plan
When the identifier is a decision ID (not a plan ID), backward compatibility is maintained
The command completes without error and outputs the correction result

Acceptance Criteria

plan correct <plan_id> succeeds when given a plan ID, auto-selecting the root decision
plan correct <decision_id> continues to work for backward compatibility
The command tries the identifier as a plan_id first (via service.get_plan()), then falls back to decision_id
When a plan_id is given, the root decision is found via decision_service.list_decisions(plan_id) and used as the correction target
The resolved plan_id is correctly set (avoiding double-resolution)
The M3 E2E acceptance test step 11 (plan correct) passes without Traceback and with rc=0
Tests (Behave): Existing plan correct scenarios continue to pass
Tests (Robot): M3 acceptance test plan correct step passes
Verify coverage >=97% via nox -s coverage_report
Run nox (all default sessions), fix any errors

Subtasks

Modify correct_decision to try the first positional as a plan_id first
If it's a plan_id, resolve the root decision via decision_service.list_decisions(plan_id)
Use the root decision as target_decision_id and the plan_id as resolved_plan_id
If not a plan_id, treat as decision_id with existing backward-compatible behavior
Tests (Behave): Verify existing plan correct scenarios pass
Tests (Robot): Verify M3 acceptance test step 11 passes
Verify coverage >=97% via nox -s coverage_report
Run nox (all default sessions), fix any errors

Definition of Done

This issue is complete when:

All subtasks above are completed and checked off.
A Git commit is created where the first line of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation.
The commit is pushed to the remote on the branch matching the Branch in Metadata exactly.
The commit is submitted as a pull request to master, reviewed, and merged before this issue is marked done.

## Metadata - **Commit Message**: `fix(cli): make plan correct accept plan_id as primary identifier` - **Branch**: `bugfix/m3-plan-correct-plan-id` ## Background and Context The M3 (v3.2.0) acceptance test (`robot/e2e/m3_acceptance.robot`, PR #799) calls `plan correct` with a plan ID as the first positional argument: ```robot ${r_correct}= Run CleverAgents Command ... plan correct ${plan_id} --mode revert ... --guidance Use SQLAlchemy ORM instead of raw SQL ... --yes --format plain ``` The M3 milestone acceptance criteria state: *"`agents plan correct --mode=revert` re-executes from the targeted decision point."* The natural CLI pattern is `plan correct <plan_id> --mode revert --guidance "..."` to correct decisions in a plan. ## Current Behavior The `correct_decision` function in `src/cleveragents/cli/commands/plan.py` (line ~2310) declares its first positional argument as `decision_id`: ```python def correct_decision( decision_id: Annotated[str, typer.Argument(help="Decision ID to correct")], ... ``` When the M3 test passes a plan ID, the plan ID is used as `target_decision_id` in the call to `svc.request_correction(plan_id=resolved_plan_id, target_decision_id=decision_id, ...)`. Since the plan ID is not a valid decision ID, the correction service raises an error (the targeted decision cannot be found in the plan's decision tree). The CLI catches the error and aborts with rc!=0. The command does have a `--plan` option for specifying the plan ID separately, but the M3 test passes the plan ID as the first positional argument, which is the natural usage pattern. ## Expected Behavior 1. `plan correct <plan_id> --mode revert --guidance "..."` accepts a plan ID as the first positional argument 2. When the identifier is recognized as a plan ID, the command automatically selects the root decision for that plan as the correction target 3. The correction is applied to the root decision of the plan 4. When the identifier is a decision ID (not a plan ID), backward compatibility is maintained 5. The command completes without error and outputs the correction result ## Acceptance Criteria - [ ] `plan correct <plan_id>` succeeds when given a plan ID, auto-selecting the root decision - [ ] `plan correct <decision_id>` continues to work for backward compatibility - [ ] The command tries the identifier as a plan_id first (via `service.get_plan()`), then falls back to decision_id - [ ] When a plan_id is given, the root decision is found via `decision_service.list_decisions(plan_id)` and used as the correction target - [ ] The resolved plan_id is correctly set (avoiding double-resolution) - [ ] The M3 E2E acceptance test step 11 (`plan correct`) passes without Traceback and with rc=0 - [ ] Tests (Behave): Existing plan correct scenarios continue to pass - [ ] Tests (Robot): M3 acceptance test `plan correct` step passes - [ ] Verify coverage >=97% via `nox -s coverage_report` - [ ] Run `nox` (all default sessions), fix any errors ## Subtasks - [ ] Modify `correct_decision` to try the first positional as a plan_id first - [ ] If it's a plan_id, resolve the root decision via `decision_service.list_decisions(plan_id)` - [ ] Use the root decision as `target_decision_id` and the plan_id as `resolved_plan_id` - [ ] If not a plan_id, treat as decision_id with existing backward-compatible behavior - [ ] Tests (Behave): Verify existing plan correct scenarios pass - [ ] Tests (Robot): Verify M3 acceptance test step 11 passes - [ ] Verify coverage >=97% via `nox -s coverage_report` - [ ] Run `nox` (all default sessions), fix any errors ## Definition of Done This issue is complete when: - All subtasks above are completed and checked off. - A Git commit is created where the **first line** of the commit message matches the Commit Message in Metadata exactly, followed by a blank line, then additional lines providing relevant details about the implementation. - The commit is pushed to the remote on the branch matching the **Branch** in Metadata exactly. - The commit is submitted as a **pull request** to `master`, reviewed, and **merged** before this issue is marked done.

freemo added the

labels

2026-03-16 03:17:17 +00:00

freemo added this to the v3.2.0 milestone

2026-03-16 03:17:19 +00:00

freemo referenced this issue

2026-03-16 16:00:05 +00:00

TDD: Write failing test for #969 — plan correct expects decision_id but test passes plan_id #979

freemo commented

2026-03-16 16:04:01 +00:00

Author

Owner

TDD workflow initiated for this bug:

Created TDD issue #979 to write a tagged test that captures this bug.
Dependency link needed: this issue is blocked by #979 (please add manually via the UI: #969 depends on #979).
Once #979 is merged to master, @freemo should create branch bugfix/m3-plan-correct-plan-id from master and implement the fix.
The fix must remove the @tdd_expected_fail tag from the test so it runs normally.

Assignees:

TDD issue #979: @brent.edwards
Bug fix (this issue): @freemo

TDD Workflow

This bug follows the project's mandatory TDD workflow for bug fixes (see CONTRIBUTING.md > Bug Fix Workflow).

**TDD workflow initiated for this bug:** - Created TDD issue #979 to write a tagged test that captures this bug. - Dependency link needed: this issue is blocked by #979 (please add manually via the UI: #969 depends on #979). - Once #979 is merged to `master`, @freemo should create branch `bugfix/m3-plan-correct-plan-id` from `master` and implement the fix. - The fix must remove the `@tdd_expected_fail` tag from the test so it runs normally. **Assignees:** - TDD issue #979: @brent.edwards - Bug fix (this issue): @freemo ### TDD Workflow This bug follows the project's mandatory TDD workflow for bug fixes (see `CONTRIBUTING.md` > Bug Fix Workflow).

freemo referenced this issue

2026-03-16 16:22:06 +00:00

fix(cli): Container.resolve() does not exist — plan tree/explain/correct crash with AttributeError #647

freemo referenced this issue

2026-03-17 18:16:43 +00:00

bug(plan): subplan spawn creates metadata but does not orchestrate real child plan execution #823

freemo added the

Points

label

2026-03-17 18:31:25 +00:00

brent.edwards was assigned by freemo

2026-03-17 19:20:08 +00:00

hurui200320 referenced this issue from a commit

2026-03-18 08:57:51 +00:00

test: add TDD bug-capture test for #969 — plan correct plan_id handling

hurui200320 referenced this issue

2026-03-18 08:58:12 +00:00

test: add TDD bug-capture test for #969 — plan correct plan_id handling #1051

hurui200320 referenced this issue from a commit

2026-03-18 11:56:02 +00:00

test: add TDD bug-capture test for #969 — plan correct plan_id handling

brent.edwards added

and removed

labels

2026-03-18 18:58:52 +00:00

brent.edwards referenced this issue from a commit

2026-03-18 19:29:24 +00:00

fix(cli): make plan correct accept plan_id as primary identifier

brent.edwards referenced this issue from a pull request that will close it,

2026-03-18 19:29:38 +00:00

fix(cli): make plan correct accept plan_id as primary identifier #1055

brent.edwards commented

2026-03-18 19:40:22 +00:00

Member

Implementation Notes — #969

Branch: bugfix/m3-plan-correct-plan-id | Commit: ed2f6ee6 | PR: #1055

Design Decisions

Modified correct_decision to accept both plan IDs and decision IDs:

Plan-ID-first resolution — calls container.plan_lifecycle_service().get_plan(identifier) first. If it returns a real Plan object, uses the plan's root decision (the one with parent_decision_id is None) as the correction target.
Decision-ID fallback — if not a plan, treats identifier as a decision_id with existing backward-compatible behavior.
Parameter renamed — decision_id → identifier with updated help text: "Plan ID (auto-selects root decision) or Decision ID to correct"
Docstring and examples updated to document dual-use identifier.

Quality Gates

Lint: PASS | Typecheck: PASS (0 errors) | Unit tests: 150 scenarios, 683 steps, 0 failures

## Implementation Notes — #969 **Branch:** `bugfix/m3-plan-correct-plan-id` | **Commit:** `ed2f6ee6` | **PR:** #1055 ### Design Decisions Modified `correct_decision` to accept both plan IDs and decision IDs: 1. **Plan-ID-first resolution** — calls `container.plan_lifecycle_service().get_plan(identifier)` first. If it returns a real `Plan` object, uses the plan's root decision (the one with `parent_decision_id is None`) as the correction target. 2. **Decision-ID fallback** — if not a plan, treats `identifier` as a decision_id with existing backward-compatible behavior. 3. **Parameter renamed** — `decision_id` → `identifier` with updated help text: "Plan ID (auto-selects root decision) or Decision ID to correct" 4. **Docstring and examples updated** to document dual-use identifier. ### Quality Gates - Lint: PASS | Typecheck: PASS (0 errors) | Unit tests: 150 scenarios, 683 steps, 0 failures

brent.edwards added a new dependency

2026-03-18 19:41:37 +00:00

#1055 fix(cli): make plan correct accept plan_id as primary identifier

brent.edwards added

and removed

labels

2026-03-18 19:41:42 +00:00

hurui200320 referenced this issue from a commit