fix(plan-lifecycle): add rollback_plan method to PlanLifecycleService #3911
Open
freemo
wants to merge 2 commits from
fix/plan-lifecycle-service-rollback-method into master
pull from: fix/plan-lifecycle-service-rollback-method
merge into: cleveragents:master
cleveragents:master
cleveragents:fix/config-service-remove-undocumented-local-scope
cleveragents:bugfix/validation-attach-named-option-format
cleveragents:docs/add-example-tool-and-validation-management
cleveragents:bugfix/project-show-resource-name
cleveragents:bugfix/backlog-resource-schema-missing-overlay-strategy
cleveragents:fix/action-argument-schema/misleading-error-message
cleveragents:fix/remove-executable-resource-type
cleveragents:fix/config-get-output-missing-origin-panel-and-envelope
cleveragents:fix/tui-help-command-full-catalog-listing
cleveragents:fix/a2a-plan-execute-full-lifecycle
cleveragents:fix/invariant-service-action-scope-effective
cleveragents:fix/plan-explain-rich-output-panels
cleveragents:fix/a2a-dispatch-not-found-error-response
cleveragents:fix/project-service-namespaced-project
cleveragents:fix/automation-profile-remove-rich-output-panel
cleveragents:fix/container-handler-module-missing
cleveragents:fix/format-output-rich-color-renderers
cleveragents:fix/type-safety-legacy-migrator-type-ignore
cleveragents:spec/update-sse-streaming-event-example
cleveragents:fix/acms-skeleton-compressor-signature
cleveragents:controller-state-machine
cleveragents:fix/skill-add-yaml-wrapper-key
cleveragents:fix/1476-tool-list-cols
cleveragents:bugfix/permissions-diff-mode-cycle
cleveragents:fix/1444-access-type
cleveragents:fix/1429-node-ref
cleveragents:fix/1443-tier-defaults
cleveragents:bugfix/session-export-format-flag
cleveragents:feature/aws-cloud-handler-sdk
cleveragents:feat/output-renderer-registry
cleveragents:fix/1432-lsp
cleveragents:bugfix/1039-missing-validation-unit-tests-yaml
cleveragents:feature/audit-preserve-event-timestamp
cleveragents:feature/m8-tui-materializer
cleveragents:tdd/m4-automation-profile-di-bypass
cleveragents:bugfix/m7-audit-session-race
cleveragents:fix/1441-ctrl-tab
cleveragents:feature/m9-entity-sync
cleveragents:feature/extract-cleveractors-library
cleveragents:feature/m9-agent-card
cleveragents:feature/m9-team-collab
cleveragents:feature/m7-postgresql-backend
cleveragents:feature/m9-container-lifecycle
cleveragents:fix/issue-11189-config-actor-format
cleveragents:bugfix/m5-actor-options-ignored
cleveragents:fix-11004-tui-suggestions
cleveragents:feature/9827-wrap-plan-status-json-envelope
cleveragents:fix/arg-swap-validation-attachment-8177
cleveragents:pr-fix/9663-hot-warm-cold-tier-reliability
cleveragents:pr_fix-11000-conflict-report
cleveragents:bugfix/m3.6.0-lsp-7044-subprocess-cleanup
cleveragents:fix/7478-file-ops-security-fix
cleveragents:impl-tui-materializer
cleveragents:test/hierarchical-plan-4phase-lifecycle
cleveragents:feature/security-fix-relpath-pr-11217
cleveragents:feature/m2-implementation-pool-supervisor-checklist
cleveragents:fix-file-tools-path-validation
cleveragents:bugfix/m8-tui-input-live-refresh
cleveragents:feature/9126-fix-action-scope-invariant-merge
cleveragents:bugfix/m7-tool-calling-llm-options
cleveragents:fix-7478-startswith-bypass
cleveragents:bugfix/m3-cleanup-subprocess-on-failed-init
cleveragents:bugfix/m8-tui-anthropic-model-name
cleveragents:feat/integrate-cleveractors
cleveragents:feature/m8-tui-llm-dispatch
cleveragents:bugfix/m3.6.0-lsp-transport-header-injection-ascii
cleveragents:fix-11175
cleveragents:fix/auto_debug-partial-state
cleveragents:fix/issue-9124-add-bdd-tags
cleveragents:pr-9673-budget-enforcement
cleveragents:fix/actor-loader-list-actors-race-condition
cleveragents:pr-9675
cleveragents:feat/v3.3.0-three-way-merge-engine
cleveragents:fix/issue-7478-inline-executor-startswith-bypass
cleveragents:fix/plan-apply-json-envelope
cleveragents:feat/v3.4.0-acms-storage-tiers
cleveragents:feat/tui-tuimat-5326
cleveragents:fix-9675-context-show-clear
cleveragents:agents/final-working
cleveragents:feat/v3.4.0-context-show-clear-cli
cleveragents:fix/10356-eventbus-unsubscribe
cleveragents:11229-fix-acms-hot-max-tokens-regression-tests
cleveragents:pr-fix-7801
cleveragents:pr-8701-invariant-model
cleveragents:pr-fix/10597-lsp-transport-cleanup
cleveragents:bugfix/m3.6.0-lsp-transport-resource-leak
cleveragents:bugfix/9558-plan-conflict-detection
cleveragents:pr-fix-9608
cleveragents:feat/v3.3.0-plan-correct-revert-append
cleveragents:dmpipeline-v2
cleveragents:pr-fix-10608-header-injection
cleveragents:pr-9827-fix
cleveragents:bugfix/7492-validation-attachment-argument-swap
cleveragents:pr-fix-11002
cleveragents:feat/v3.4.0-context-list-add-cli
cleveragents:fix/plan-status-json-envelope
cleveragents:feat/v370/multi-session-tabs
cleveragents:fix-branch
cleveragents:fix/project-show-missing-panels
cleveragents:AUTO-IMP/PR-10069-checklist
cleveragents:feature/m2-pr-compliance-checklist
cleveragents:feature/pr-10592-cloud-resource-types
cleveragents:fix-lsp-transport-cleanup
cleveragents:feat/v360/cloud-resource-types
cleveragents:feature/context-strategy-protocol
cleveragents:refactor/v3.6.0-acp-to-a2a-rename
cleveragents:fix/context-cli-consolidation
cleveragents:fix/10608-lsp-header-injection
cleveragents:feat/acms-context-index
cleveragents:fix/plan-status-missing-output-panels
cleveragents:pr/fix-arg-swap-validation-attachment-8177
cleveragents:feature/issue-4748-actor-context-list-show-clear
cleveragents:fix-cli-plan-status-envelope
cleveragents:fix/plan-tree-color-format-ansi-output
cleveragents:pr/9981
cleveragents:pr/11153-auto-debug-fix
cleveragents:pr/10589-tui-materializer
cleveragents:fix/validate_path_security
cleveragents:pr-fix-11177-status-check-native-expressions
cleveragents:bugfix/m6-validate-path-startswith
cleveragents:security/relpath-containment-fallback
cleveragents:a2a-materializer-pr-fix
cleveragents:pr-fix-10608
cleveragents:bugfix/9250-a2a-session-id-validation-before-cleanup
cleveragents:pr-fix-11053
cleveragents:fix/10496-auto-debug-node-state-mutation
cleveragents:feat/tui-v370/tui-materializer
cleveragents:fix/a2a-handle-session-close-missing-session-id
cleveragents:fix/validation-attachment-arg-swap-8177
cleveragents:pr-fix-11196-invariant
cleveragents:feat/v3.4.0-acms-budget-enforcement
cleveragents:pr-fix-11196
cleveragents:bugfix/m5-fix-hot-max-tokens-tier
cleveragents:pr-fix-9675
cleveragents:perf/acms-large-project-indexing-optimization
cleveragents:perf-fix
cleveragents:pr-9608
cleveragents:feature/ten-way-merge-engine
cleveragents:pr-fix-branch
cleveragents:pr-11217
cleveragents:bugfix/9608-three-way-merge-engine
cleveragents:11101-three-way-merge-engine
cleveragents:feat/v3.4.0/acms-context-policy
cleveragents:fix/remove-silent-argument-swap
cleveragents:fix-pr-11000-structured-conflict-report
cleveragents:pr-fix-11053-session-id-validation
cleveragents:agents/fix-eventbus-unsubscribe
cleveragents:pr-10356
cleveragents:fix/invariant-action-scope
cleveragents:bugfix/issue-8395-sanitise-db-url
cleveragents:bugfix/m3-fix-action-scope-invariant-merge
cleveragents:pr-9671
cleveragents:feature/wire-missing-event-emitters
cleveragents:bugfix/m3.6.0-lsp-transport-post-spawn-cleanup
cleveragents:dmpipeline
cleveragents:bugfix/m5-acms-project-budget-override
cleveragents:fix/iterate-all-actors
cleveragents:pr/11217-fix-prefix-collision-bypass
cleveragents:fix/pr-11011-subprocess-cleanup
cleveragents:pr-11217-fix
cleveragents:pr-11217-relpath-fix
cleveragents:feat/v3.6.0-context-strategy-protocol
cleveragents:bugfix/tui-actor-overlay-render-shadow
cleveragents:bugfix/m5-revert-acms-budget-assembler
cleveragents:fix/eventbus-unsubscribe
cleveragents:feature/pr-9981
cleveragents:fix/v3.7.0/actor-add-update-flag
cleveragents:agents/fix-invariant-persistence-8573
cleveragents:fix/invariant-database-persistence
cleveragents:feat/tui-materializer-a2a
cleveragents:fix/tui-tui-materializer-a2a-event-queue
cleveragents:fix/unsubscribe-eventbus
cleveragents:pr-11153
cleveragents:feature/11201
cleveragents:pr-fix-11153-patched
cleveragents:pr-branch
cleveragents:fix/10813-strategy-decision-persistence
cleveragents:fix-pr-11145-status-check
cleveragents:pr-11053
cleveragents:pr-fix-10597-subprocess-cleanup
cleveragents:bugfix/mcp-infer-resource-slots-null-properties
cleveragents:pr-11166
cleveragents:pr-9675-fix
cleveragents:feat/structural-component-output-validation
cleveragents:fix/invariant-service-thread-safety
cleveragents:pr-fix-8179-implementation
cleveragents:pr-fix-9313
cleveragents:cleveragents-pr-fix-11038
cleveragents:fix/m2-acceptance-test
cleveragents:fix/pr-11042-rename-render
cleveragents:fix/action-scope-inmerge
cleveragents:fix/wf12-oom-sigkill
cleveragents:fix/wf18-container-clone-e2e
cleveragents:tdd/mcp-client-timer-cancel-race
cleveragents:feature/auto-debug-nodes
cleveragents:feat/v3.2.0-decision-recording-persistence
cleveragents:bugfix/m6-actor-overlay-render-shadow
cleveragents:bugfix/m7-plan-strategy-decisions-json
cleveragents:fix/10911-tui-suggestions-query-extraction
cleveragents:fix/lsp-transport-subprocess-cleanup
cleveragents:pr-fix-8177-validation
cleveragents:bugfix/m3-plan-status-json-envelope
cleveragents:fix/invariant-persistence-8573
cleveragents:pr-fix-11037
cleveragents:pr-11015-fix
cleveragents:pr_fix_11015
cleveragents:fix/m1-security-fix-startswith-bypass
cleveragents:fix/automation-profile-gates-lifecycle
cleveragents:fix-status-check-brittle-pipeline-11212
cleveragents:feat/pr-10590-dual-capability-strategies
cleveragents:feat/structural-output-validation
cleveragents:bugfix/m2-ci-status-check-resilience
cleveragents:fix-sandbox-cache-invalidation
cleveragents:feature/acp-a2a-rename-fix
cleveragents:feature/m3-plan-correction-data-model
cleveragents:pr-fix-10356-unsubscribe
cleveragents:pr-fix-11011
cleveragents:pr_fix/lsp-transport-header-injection-ascii
cleveragents:fix-pr-11002-startswith-bypass-7478
cleveragents:bugfix/acms-project-budget-override
cleveragents:fix/ci-status-check-resilience
cleveragents:bugfix/pr-fix-10597-cleanup-subprocess-on-init-failure
cleveragents:bugfix/sandbox-reexecute-cleanup
cleveragents:pr-fix-8701-invariant-model
cleveragents:fix/test-dotdot-traversal-assertion
cleveragents:fix/cleanup-stale-preserve-commits
cleveragents:fix/10592-pr-compliance
cleveragents:fix/security-file-tools-path-traversal-7478
cleveragents:pr-11180-fix
cleveragents:fix-combined-format
cleveragents:fix-9131-invariant-propagation
cleveragents:fix/tui-actor-selection-overlay
cleveragents:pr-11201
cleveragents:merge/pr-11196-invariant-fix
cleveragents:fix/issue-10813-strategize-decision-persistence
cleveragents:pr-fix-11170
cleveragents:pr/11165
cleveragents:temp-pr-11174
cleveragents:feat/invariant-enforcement-validation-pipeline
cleveragents:pr-fix-10356-unsubscribe-eventbus
cleveragents:pr-fix-11156-python313-deprecation
cleveragents:feature/pr-7801-fix-validate-path-security
cleveragents:fix/11039-render-refresh
cleveragents:fix/tui-actor-selection-render-rename
cleveragents:pr-fix-11089-session-close-validation
cleveragents:pr-fix/11089-session-close-validation
cleveragents:pr-fix-11182
cleveragents:feature/7926-persist-decision-dependencies
cleveragents:bugfix/m3-rxpy-subject-close
cleveragents:test/restore-e2e-tests
cleveragents:feature/m694-tui-materializer-a2a-integration-layer
cleveragents:feature/issue-pr-9271-hot-max-tokens
cleveragents:pr-fix-8177
cleveragents:test/v360/e2e-project-plan-correction
cleveragents:bugfix/issue-8426-stdio-cleanup
cleveragents:feature/eventbus-unsubscribe
cleveragents:bugfix/m3-integrate-mcp-transport
cleveragents:fix/concurrent-stdout-restoration
cleveragents:feat/a2a-stdio-transport-fix-264
cleveragents:PR-fix-wf18
cleveragents:feature/sandbox-cache-invalidation
cleveragents:fix/issue-10496-auto-debug-state-mutation
cleveragents:fix/python-313-asyncio-deprecations
cleveragents:pr-11128
cleveragents:pr-11180
cleveragents:pr-11165
cleveragents:pr-practice
cleveragents:structural-output-validation
cleveragents:fix/status-check-native-expressions
cleveragents:feat/merge-conflict-detection
cleveragents:11036-fix-acms-hot-max-tokens
cleveragents:pr/11166
cleveragents:fix/ci-status-check-native-expressions
cleveragents:fix/stdlib-transport-cleanup
cleveragents:fix/11176-actor-selection-render
cleveragents:pr-fix-10597
cleveragents:feature/pr-compliance-pool-supervisor
cleveragents:fix/actor-add-update-enforcement-fix
cleveragents:pr_fix/8209
cleveragents:pr-10590
cleveragents:fix/python313-asyncio-get-event-loop-deprecation
cleveragents:pr-fix-#11053-session-id-validation
cleveragents:pr-fix-11042-renamed-render
cleveragents:feat/v360/acp-to-a2a-rename
cleveragents:fix-arg-swap-validation-attachment-8177
cleveragents:fix/asyncio-get-event-loop-deprecation
cleveragents:fix_8395_pr
cleveragents:pr-fix-11153-auto-debug-mutation
cleveragents:pr/11051-thread-safety-invariant
cleveragents:fix-plan-status-json-envelope
cleveragents:bugfix/pr-11015-pool-supervisor-checklist
cleveragents:feature/fix-7478-validate-path
cleveragents:feature/plans-conflict-detection
cleveragents:pr-11141-cleanup-stale-commits-beyond-head
cleveragents:fix/pyyaml-vulnerability-upgrade
cleveragents:pr-fix-9244
cleveragents:bugfix/m3-invariant-propagation
cleveragents:feature/issue-10480-fix-validation-bypass
cleveragents:feature/m3-invariant-enforcement-validation-pipeline
cleveragents:feat/invariant-enforcement-strategize-phase
cleveragents:bugfix/mcp-race-condition-start
cleveragents:fix/action-schema-argument-default-type-validation
cleveragents:issue-10438-fix
cleveragents:fix/mcp-timer-race-10516
cleveragents:fix/10480-validation-bypass-fix
cleveragents:fix/cli-session-tell-format-flag
cleveragents:feat/agents-invariant-add-list-remove-commands
cleveragents:restore-e2e-cleanup
cleveragents:fix/events-eventbus-unsubscribe
cleveragents:fix/issue-11120-cleanup-stale-preserve-artifacts
cleveragents:feature/fix-issue-11121-cleanup-stale-reinvoke
cleveragents:fix/issue-10480-plan-validation
cleveragents:feature/m5-tdd-quality-gate
cleveragents:bugfix/11121-fix-cleanup_stale-preserve-meaningful-changes
cleveragents:bugfix/m8-set-active-persona-preset-reset
cleveragents:feat/context-priority-strategy
cleveragents:feature/issue-4381-docs-api-and-module-guides
cleveragents:m7-opencode-ruff
cleveragents:bugfix/m3-wf18-oom-sigkill
cleveragents:bugfix/acms-dual-strategy-capabilities-incompatible-fields
cleveragents:feature/benchmark-scheduled-workflow
cleveragents:feature/m8-tui-mainscreen
cleveragents:feat/v3.4.0/acms-project-indexer
cleveragents:fix/10932-preserve-strategy-decisions-json
cleveragents:fix/data-integrity-session-rollback-7489
cleveragents:fix/issue-6329-resource-remove-edge-table
cleveragents:fix/issue-7524-invariant-service-thread-safety
cleveragents:pr-10932-fix-plan-strategy-decisions
cleveragents:pr-fix-9244-pyyaml-upgrade
cleveragents:refactor/noxfile-parallel-test-architecture
cleveragents:task/ci-matrix-strategy-python-versions
cleveragents:bugfix/m3.6.0-ci-pipeline-flakiness-stabilization
cleveragents:feat/v3.3.0-plan-rollback
cleveragents:refactor/auto-guard-1-cli-a2a-boundary
cleveragents:feature/issue-10755-redirect-rich-panels-to-stderr
cleveragents:pr10871
cleveragents:fix/10881-propagate-invariants-to-child-plans
cleveragents:feat/resources-extension-interface
cleveragents:pr-fix-10901
cleveragents:ci/optimize-benchmarks-regression
cleveragents:fix/tui-extract-at-token-suggestions
cleveragents:feat/acms-index-data-model
cleveragents:feature-10887-eventbus-unsubscribe
cleveragents:feature/m5-add-repo-indexing-showcase
cleveragents:PR-10910-a2a-json-rpc-routing
cleveragents:feature/milestone-based-pr-prioritization
cleveragents:bugfix/m3-issue-9055
cleveragents:auto-time-3-day106-cycle2
cleveragents:feature/m39-timeline-day106-cycle2-2026-04-16
cleveragents:timeline/day-106-cycle2-2026-04-16-auto-time-3
cleveragents:feat/issue-10921-a2a-http-transport
cleveragents:pr/fix-10842
cleveragents:feature/issue-10746-fix-agents-graphs-plan-generation-validate-always-passes-for-code-longer-than-10-characters-making-llm-validation-ineffective
cleveragents:agents/fix-10866-permissions-screen-to-textual-screen
cleveragents:pr-10886
cleveragents:bugfix/m3-session-tell-format
cleveragents:fix/pr-10890-shell-safety-integration
cleveragents:fix/session-delete-json-envelope
cleveragents:pr-10851
cleveragents:test/v3.8.0-ci-quality-execution-time
cleveragents:feature/m7-timeline-day-106-update
cleveragents:bugfix/context-remove-path-traversal-10924
cleveragents:pr-10876
cleveragents:fix/gemini-fallback-order
cleveragents:fix/trailing-comma-opencode-json
cleveragents:pr/fix/mcp-client-start-race-condition
cleveragents:fix/project-switch-command
cleveragents:fix-pr-4211
cleveragents:feat/three-way-merge-engine-9608
cleveragents:pr/9673
cleveragents:fix/1469-plan-execute-structured-panels
cleveragents:fix/actor-provider-validation
cleveragents:implement-pr-9442
cleveragents:cleveragents-push-23420b48
cleveragents:fix/validation-repo-silent-swap
cleveragents:feat/context-strategy-plugin-system
cleveragents:fix/startswith-bypass-7478
cleveragents:fix-plan-status-envelope-11034
cleveragents:fix/invariant-thread-safety
cleveragents:fix-thread-safety-invariant-service
cleveragents:fix/8284-warned-sessions-reset
cleveragents:docs/milestone-plan-navigation
cleveragents:feat/v3.3.0-checkpoint-creation
cleveragents:feature/implementor-notification-11032
cleveragents:task/ci-optimize-e2e-tests-execution-time
cleveragents:feature/pr-9599-plan-correct-correction-engine
cleveragents:pr-fix-10593
cleveragents:pr9452
cleveragents:fix/isolate-checkpoint-prune-test
cleveragents:pr/fix-9601
cleveragents:pr/9234-hardening-bdd-tags
cleveragents:bugfix/9673-acms-budget-enforcement
cleveragents:pr-8667
cleveragents:auto-arch/spec-pr-10451-test-coverage
cleveragents:fix/10954-security-scan-dockerfile
cleveragents:bugfix/9183-bdd-tag-enforcement
cleveragents:fix/7566-engine_cache-toctou-race
cleveragents:fix/10934-preserve-strategy-decisions-json
cleveragents:bugfix/10608-lsp-header-injection
cleveragents:bugfix/9981-acms-indexing-optimize
cleveragents:bugfix/11077-security-escape-bypass
cleveragents:fix/auto-rev-sup-tracking-prefix
cleveragents:fix-lsp-subprocess-cleanup-10597
cleveragents:improvement/agent-evolution-pool-supervisor-pr-metadata
cleveragents:fix/plan-tree-json-output-envelope
cleveragents:pr-9313-fix
cleveragents:bugfix/9244-pyyaml-security-upgrade
cleveragents:feature/issue-1925-add-asv-tests-for-domain-module
cleveragents:test/domain-asv-benchmarks
cleveragents:feature/9250-fix-a2a-session-close
cleveragents:fix/pr-10027-acms-default-pipeline
cleveragents:bugfix/m2-plan-explain-alternatives-format
cleveragents:fix-invalidate-sandbox-dirs-cache-after-purge-7527
cleveragents:pr-fix-10958-async-cleanup-tests
cleveragents:feat/adr-049-layer-boundary-enforcement
cleveragents:fix/action-list-table-columns
cleveragents:fix/issue-7478-validate-path-startswith-bypass
cleveragents:pr-fix-ci-11000
cleveragents:fix/agent-skill-multi-scope-discovery
cleveragents:pr_fix_8675_switch_project_command
cleveragents:feat/m6/devcontainer-clone-into-sandbox
cleveragents:fix/tui-keybinding-preset-persona-cycling
cleveragents:pr-fix-10982
cleveragents:bugfix/m3-invariant-service-thread-safety
cleveragents:pr-fix-10937-close-reactive-eventbus
cleveragents:pr-fix-7478-path-traversal
cleveragents:feature/benchmark-scheduled-workflow-fix
cleveragents:pr-9183-add-bdd-tags
cleveragents:pr/11029-review-started-notification
cleveragents:fix/pyyaml-security-upgrade
cleveragents:fix-plan-status-panels
cleveragents:fix-pr-11037
cleveragents:feat/v3.6.0-database-resource-types
cleveragents:pr-10591-checkout
cleveragents:pr-10979
cleveragents:fix/invariant-thread-safety-8209
cleveragents:pr-fix-11002-validate-path-bypass
cleveragents:fix/10597-lsp-proc-cleanup
cleveragents:fix/plan/tree-envelope-9313
cleveragents:fix-6568-push
cleveragents:fix/issue-6425-tui-persona-cycling-keybinding
cleveragents:pr/11044
cleveragents:feature/m6-reduce-redundant-ci-status-reporting
cleveragents:fix/11041-plan-tree-envelope
cleveragents:fix/ca-test-infra-improver-health-spam
cleveragents:agents/pr-6628-fix
cleveragents:docs/add-showcase-cli-basics
cleveragents:auto-time-1-day107-cycle
cleveragents:improvement/agent-uat-tester-parallel-docs-pr-fix
cleveragents:fix/issue-11047-actor-add-rename-from-config
cleveragents:fix/pr-11050-subprocess-cleanup
cleveragents:pr-6741
cleveragents:ci/cache-helm-binary-auto-inf-1
cleveragents:fix/8675-project-switch
cleveragents:fix/7527-sandbox-cache-invalidation
cleveragents:fix/issue-6319-project-context-set-output
cleveragents:pr/fix-9183-bdd-tags
cleveragents:fix/issue-6325-plan-explain-decision-id
cleveragents:fix/1422-docs
cleveragents:pr-fix-1485-updates
cleveragents:spec/subplan-system-v3.3.0
cleveragents:pr/6723-fix-session-create-json
cleveragents:improvement/agent-bug-hunt-pool-supervisor-tracking-prefix-complete
cleveragents:fix/pr-6695-session-list-empty-json
cleveragents:fix/file-tools-startswith-bypass
cleveragents:pr_fix_8256
cleveragents:pr-9663-fix
cleveragents:docs/add-example-resource-and-skill-management
cleveragents:feature/m39-cli-basics-showcase
cleveragents:pr-fix-7478-startswith-bypass
cleveragents:fix/issue-11047-actor-add-remove-positional-name
cleveragents:fix/gemini-fallback-order-fix-3
cleveragents:pr_fix_8179
cleveragents:fix/gemini-fallback-order-fix-2
cleveragents:fix/validation-list-command
cleveragents:fix/validation-list-command-clean
cleveragents:fix-pr7957-complete-tracking-prefix
cleveragents:pr-7922-fix-lint
cleveragents:fix/validation-swap-8177
cleveragents:add-plan-start-alias
cleveragents:feature/pr-8304-container-clone-into
cleveragents:fix-pyyaml-11012
cleveragents:pr-fix-9461
cleveragents:fix/pr-11004-tui-token-extraction
cleveragents:fix/invariant-scope-handling
cleveragents:feat/plan-correction-8531
cleveragents:pr/8685-correction-data-model-persistence
cleveragents:bugfix/lsp-stdio-transport-cleanup-10597
cleveragents:pr-8660
cleveragents:feat-scope-chain-resolution
cleveragents:chore/pyyaml-upgrade
cleveragents:fix/9250-session-id-validation-handle-session-close
cleveragents:fix/issue-7478-file-tools-validate-path
cleveragents:pr-fix-9442-tui-ctrltab
cleveragents:spec/update-cycle8-validation-gate-empty-run-guard
cleveragents:fix/tui-sqlite-session-persistence-10648
cleveragents:fix/8661-plan-start-alias
cleveragents:fix-10649
cleveragents:refactor/add-return-type-get-services
cleveragents:pr-fix-cache-init
cleveragents:pr9407-timeline
cleveragents:feat/tui-prompt-symbol
cleveragents:pr_fix_9407-plan-alternatives-structured
cleveragents:feat/automation-profile-precedence-chain
cleveragents:bugfix/8179-remove-session-rollback-calls
cleveragents:feat/v360/pluggable-scope-chain-api
cleveragents:pr-9246
cleveragents:refactor/agent-configurable-limits-context-analysis-plan-generation
cleveragents:fix/issue-6452-session-tell-output
cleveragents:fix/v370/quality-gates-command-injection
cleveragents:pr-fix-10635-fixed
cleveragents:pr-10069
cleveragents:pr/fix-9313
cleveragents:pr-10643
cleveragents:invariant-pr-8684-fix
cleveragents:pr-fix-6676-resource-remove-edge-table
cleveragents:refactor/v360/audit-rename-acp-imports
cleveragents:fix/issue-7623-validation-pipeline-stdout
cleveragents:fix/acms-consolidate-strategycapabilities
cleveragents:fix/issue-7604-a2a-event-queue-concurrency
cleveragents:pr-fix-8661
cleveragents:auto-arch/spec-clarifications-cycle-1
cleveragents:feat/pure-graph-bdd-coverage
cleveragents:fix/9250-validate-session-id-before-cleanup
cleveragents:feature/issue-9442-fix-tui-correct-preset-cycling-keybinding-to-ctrl-tab-and-add-persona-tab-cycling
cleveragents:bugfix/m6-file-tools-validate-path-bypass
cleveragents:fix/invariant-add-scope
cleveragents:bugfix/m3-shell-safety-service-tui
cleveragents:pr-8684-persist-invariants
cleveragents:pr-8209-fix
cleveragents:docs/v360/repl-actor-run-showcase
cleveragents:feat/v360/cost-session-budget
cleveragents:bugfix/8177-remove-silent-argument-swap
cleveragents:fix/plan-apply-rich-output-panels
cleveragents:pr-fix-11012
cleveragents:pr-fix-11012-pyyaml-upgrade
cleveragents:pr-fix-8667
cleveragents:pr/fix/11012-pyinsec
cleveragents:pr-fix-9407
cleveragents:pr-8853
cleveragents:test/cli-lifecycle-e2e-full-plan-lifecycle
cleveragents:bugfix/m3-evlv-9824-implementation-pool-compliance-checklist
cleveragents:pr/10069
cleveragents:docs/pr-creator-state-priority-labels
cleveragents:fix/1514-structured-panels
cleveragents:test/core-asv-benchmarks
cleveragents:fix-8640-remove-positional-name
cleveragents:pr-fix-10995
cleveragents:refactor/v3.6.0-acp-to-a2a-rename-push
cleveragents:pr-9663
cleveragents:bugfix/m3.6.0-lsp-discovery-resource-exhaustion-dos
cleveragents:8660-move-namespace-filter-inside-lock
cleveragents:pr-fix-work
cleveragents:test/plan-correct-json-output-tdd
cleveragents:pr-8304
cleveragents:feat/v3.2.0-invariant-data-model-db-schema
cleveragents:pr_fix_1514_v2
cleveragents:timeline-update-2026-04-19
cleveragents:pr-fix-9313-plan-tree-envelope
cleveragents:test/v3.6.0/advanced-context-strategies-tests
cleveragents:pr/11004-fix-tui-suggestions-query-extraction
cleveragents:pr-fix-9817
cleveragents:feat/9558-plan-conflict-detection
cleveragents:docs/timeline-day-101
cleveragents:fix/v360/plugin-loader-security
cleveragents:feat/acms-context-policy-fix-9671
cleveragents:pr-9817-plan-apply-json
cleveragents:pr-fix-9460
cleveragents:pr-fix-6722-prompt-symbol
cleveragents:pr/9671
cleveragents:pr-fix-9671
cleveragents:pr-10592-fix
cleveragents:fix/issue-7478-file-path-validation
cleveragents:pr-fix-7478-validatepath
cleveragents:feat/pr-10590-context-strategy-fix
cleveragents:bugfix/m6-acms-path-matching-absolute
cleveragents:bugfix/pr-9183-bdd-tags
cleveragents:fix-pr-10975-path-matching-normalize
cleveragents:pr_fix/lsp-transport-subprocess-cleanup
cleveragents:pr-8177-validation-fix
cleveragents:feat/acms-context-show-clear-cli
cleveragents:feat/v360/plugin-architecture
cleveragents:fix/invariant-add-scope-required
cleveragents:pr-fix-10590-context-strategy
cleveragents:pr-fix-10590-local
cleveragents:pr-8662-fix
cleveragents:pr/1485
cleveragents:bugfix/8660-move-namespace-filter-inside-lock
cleveragents:pr/9460-project-show-invariants-validations
cleveragents:pr-11013
cleveragents:fix-1469-impl
cleveragents:fix/1469-impl
cleveragents:fix/cleanup-service-sandbox-cache-invalidation
cleveragents:pr-8257
cleveragents:pr-3329
cleveragents:feat/v3.2.0-decision-recording-strategize
cleveragents:fix/strategize-full-context-snapshots
cleveragents:clone-verify-test
cleveragents:fix/issue-6316-session-list-json-empty-case
cleveragents:AUTO-IMP/PR-9672-context-list-add
cleveragents:AUTO-IMP/PR-9663-storage-tiers
cleveragents:fix/issue-pr-11002
cleveragents:fix/plan-lifecycle-prompt-decision
cleveragents:fix/gemini-fallback-order-10906
cleveragents:AUTO-IMP/PR-10583-a2a-rename
cleveragents:fix-check-same-thread-migration-runner
cleveragents:d2188407
cleveragents:fix/a2a-handle-session-close-missing-session-id-pr-9250
cleveragents:fix/invariant-merge-action-scope
cleveragents:pr-fix-8179
cleveragents:bugfix/report-number-of-actors
cleveragents:bugfix/m6-devcontainer-autodiscovery-wiring
cleveragents:fix-gemini-fallback-order-10906
cleveragents:bugfix/m5-event-bus-exception-swallow
cleveragents:pr/3458
cleveragents:acms-parallel-indexing-fix
cleveragents:bugfix/m3-error-handling-fileconfig-unhandled-exception
cleveragents:acms-parallel-indexing
cleveragents:fix/resource-removal-children-check-6886
cleveragents:pr/9451-fix-tui-thinking-effort-presets
cleveragents:pr-fix-10958
cleveragents:fix/8179-remove-session-rollback-calls
cleveragents:pr/9817-plan-apply-json-envelope
cleveragents:fix/lsp-context-enrichment-acms-wiring
cleveragents:fix/cli-remove-positional-name-from-actor-add
cleveragents:fix/acms-context-cli
cleveragents:fix/tui-permissions-screen-wrong-base-class
cleveragents:bugfix/m6-session-create-suppress-exception-logging
cleveragents:fix/plan-tree-json-missing-decision-id
cleveragents:fix/plan-start-spec-alignment
cleveragents:fix-10957
cleveragents:fix/6726-tui-persona-cycling-keybinding
cleveragents:feat/plan-rollback-cli-checkpoint-restore
cleveragents:pr-8661-plan-start-alias
cleveragents:pr/1486/resource-handler-return-type
cleveragents:feature/8667-add-validation-list-command
cleveragents:auto-docs-1-mkdocs-setup
cleveragents:fix/actor-add-positional-name
cleveragents:feat/v3.3.0-merge-strategy-config
cleveragents:fix/invariant-precedence-chain-action-scope
cleveragents:improvement/agent-pr-review-pool-supervisor-tracking-prefix-complete
cleveragents:pr/fix/actor-loader-list-actors-race-condition
cleveragents:bugfix/m4-lsp-context-enrichment-acms-wiring
cleveragents:docs/auto-docs-2-v320-v330-features
cleveragents:bugfix/m-error-suppression-reactive-registry-adapter-v2
cleveragents:fix/7501-plan-repository-success-derivation
cleveragents:pr-10492
cleveragents:pr-8225
cleveragents:fix/plan-artifacts-missing-validation-apply-summary
cleveragents:feature/m9-v3.8.0-v3.9.0-documentation
cleveragents:docs/fix-automation-profile-default-supervised
cleveragents:fix/context-analysis-agent-path-traversal
cleveragents:pr-9229-path-traversal-fix
cleveragents:pr-10975
cleveragents:pr-fix-10986
cleveragents:pr/1486/fix-resource-handler-return-type
cleveragents:feat/m8/tui-main-screen
cleveragents:pr-9257-fix
cleveragents:fix/9222-guard-integration-e2e-jobs
cleveragents:refactor/clarify-behave-robot-framework-roles
cleveragents:docs/reference-glossary
cleveragents:feat/9088-a2a-message-send-stream
cleveragents:bugfix/m6-gemini-fallback-order
cleveragents:fix/validation-list-command-fixed
cleveragents:fix-executable-resource
cleveragents:test/plan-tree-correction-visual-tdd
cleveragents:auto-time/timeline-update-2026-04-18
cleveragents:pr-8179
cleveragents:spec/auto-arch-24-a2a-boundary-enforcement-adr
cleveragents:pr/10988/head
cleveragents:fix/7566-engine-cache-toctou-race
cleveragents:feat/v3.6.0-llm-provider-abstraction
cleveragents:fix/concurrency-catalog-cache-lock-7590-cleandiff
cleveragents:chore/test-infra-broad-exception-lint
cleveragents:issue-7502-fix-get-for-plan
cleveragents:fix/1500-impl
cleveragents:feat/context-show-cli-commands
cleveragents:pr-fix-7527-cache-invalidation
cleveragents:pr-fix-9407-plan-explain-structured-alternatives
cleveragents:fix/multi-scope-skill-discovery-9369
cleveragents:pr_9454
cleveragents:feat/agent-switch-cmd
cleveragents:pr-9329
cleveragents:8661-plan-start-alias
cleveragents:feat/acms-context-analysis-summaries
cleveragents:fix/invariant-add-repeatable-plan-action
cleveragents:tdd/m6-session-create-suppress-exception
cleveragents:test-push-check-only
cleveragents:pr-10889
cleveragents:pr-10889-fix
cleveragents:feature/issue-10952-provider-integration-tests
cleveragents:pr/10879-benchmark-caching-parallelism
cleveragents:bugfix/m3-eventbus-unsubscribe
cleveragents:spec/add-deleted-at-field-to-project-delete
cleveragents:fix/issue-6500-actor-context-list-regex
cleveragents:tdd/m8-tui-sqlite-session-persistence
cleveragents:fix/issue-6464-resource-add-auto-discovery
cleveragents:fix/bug-hunt-supervisor-tracking-prefix
cleveragents:feat/v3.2.0-plan-tree-cli
cleveragents:fix/issue-6491-actor-remove-format-option
cleveragents:fix/issue-6457-json-envelope-messages-text
cleveragents:improvement/agent-ca-test-infra-improver-duplicate-avoidance
cleveragents:fix/boundary-cost-budget-warning-re-trigger-7525
cleveragents:bugfix/6879-cli-format-option
cleveragents:feat/jwt-token-refresh
cleveragents:auto-discovered-stale-conflicts-review-task
cleveragents:docs/add-example-audit-log-and-security
cleveragents:docs/v3.8.0-api-and-module-guides
cleveragents:fix/issue-9169
cleveragents:improvement/reduce-redundant-ci-status-reporting
cleveragents:feat/v3.4.0-acms-index-data-model-traversal
cleveragents:bugfix/m3-sqlite-check-same-thread
cleveragents:issue-1-conversation-state
cleveragents:bugfix/m3-evlv-implementation-pool-compliance-checklist
cleveragents:feature/m9-a2a-jsonrpc
cleveragents:bugfix/m6-plan-execute-rich-output
cleveragents:fix/uat-checkpoint-prune-test-isolation
cleveragents:feature/issue-4749-split-monolithic-specification
cleveragents:bugfix/m8-suggestions-query-extraction
cleveragents:bugfix/m6-session-delete-format-json-envelope
cleveragents:bugfix/m3-langgraph-disposables
cleveragents:timeline/day-104-2026-04-14-auto-time-2
cleveragents:docs/quickstart-guide
cleveragents:fix/plan-prompt-json-timing-started
cleveragents:feat/v3.6.0-virtual-resource-types
cleveragents:feat/tui-v370/persona-registry
cleveragents:fix/1431-subgraph
cleveragents:bugfix/7529-a2a-terminal-phase-guard
cleveragents:bugfix/m3-bdd-feature-file-tags
cleveragents:ci/v360/isolate-slow-e2e-tests
cleveragents:feature/m3-consolidate-documentation
cleveragents:feature/m7-user-driven-review-agent
cleveragents:feature/m9-a2a-http
cleveragents:fix/1423-refactor
cleveragents:fix/tui-mainscreen-3state-sidebar-adr044
cleveragents:task/v3.8.0-ci-reusable-workflows
cleveragents:testbed/m9-hello
cleveragents:docs/add-label-verification-to-new-issue-creator
cleveragents:bugfix/m3-database-migration-runner-check-same-thread
cleveragents:feature/m4-plan-correction-revert
cleveragents:improvement/agent-architecture-pool-supervisor-milestone-assignment
cleveragents:docs/changelog-unreleased-cycle7
cleveragents:feature/m9-changelog-unreleased-cycle7
cleveragents:fix/issue-10512-mcptooladapter-rlock
cleveragents:fix/data-integrity-llm-trace-repository-7505
cleveragents:agents/auto-working-new
cleveragents:fix/resource-removal-guard-linked-children
cleveragents:fix/1468-impl
cleveragents:feature/1915-timezone-aware-datetime
cleveragents:feature/issue-4381-docs-add-invariantreconciliationactor-api-docs-devcontainer-discovery-module-guide-and-mkdocs-nav
cleveragents:task/ci-actor-context-mgmt-test-optimization
cleveragents:fix/7619-git-tools-base-env-toctou
cleveragents:pr-fix-8661-updates
cleveragents:feature/issue-2798-chore-agents-improve-ca-test-infra-improver-strengthen-duplicate-avoidance
cleveragents:bugfix/m3-migration-runner-check-same-thread
cleveragents:feature/issue-10952-fix-database-migration-runner-check-same-thread
cleveragents:fix/dependency-security-aiohttp-cves
cleveragents:test/uko-persistence-coverage
cleveragents:fix/security-b608-sql-fstring-migration-plan-phases
cleveragents:fix/cli-legacy-removal
cleveragents:feature/m39-auto-arch-23-minor-clarifications
cleveragents:bugfix/m3-langgraph-execute-state-bypass
cleveragents:feat/issue-6370-actor-context-clear
cleveragents:feat/acms-hot-storage-tier-lru-cache
cleveragents:feature/m3111-milestone-based-pr-prioritization
cleveragents:bugfix/m3-actor-run-response
cleveragents:fix/issue-7524-invariant-service-thread-safety-v2
cleveragents:pr-fix-10746
cleveragents:fix/tui-auto-generate-presets-actor-schema
cleveragents:feat/agent-card-discovery
cleveragents:feature/pr-10916-close-reactive-event-bus
cleveragents:feature/issue-1917-optimize-robot-actor-context-management-tests
cleveragents:feature/issue-10803-fix-nox-sessions-use-uv-sync-frozen
cleveragents:feature/issue-1923-missing-test-levels-core-module
cleveragents:feature/1928-add-test-coverage-for-tui-module
cleveragents:chore/ci-dockerfile-server-security-scan
cleveragents:task/ci-centralize-tool-versions
cleveragents:feature/m9-langgraph-platform
cleveragents:bugfix/m5-validation-attach-output-format
cleveragents:test/ci-execution-time-optimize-benchmark-regression
cleveragents:feature/issue-3105-add-mandatory-labels-to-supervisor-tracking-issue-creation
cleveragents:feat/acms-context-policy-configuration-schema
cleveragents:feat/context-sliding-window-strategy
cleveragents:feature/issue-5163-align-checkpoint-trigger-names
cleveragents:feature/issue-4221-docs-add-showcase-example-for-audit-log-and-security-commands
cleveragents:bugfix/m3-output-plan-results
cleveragents:fix/action-archive-output-panels
cleveragents:pr/9912-fix
cleveragents:fix/concurrency-catalog-cache-lock-7590
cleveragents:bugfix/executor-error-details-overwrite-mini-max
cleveragents:fix-10866-permissions-screen
cleveragents:feature/issue-7957-bug-hunt-pool-supervisor-tracking-prefix
cleveragents:fix-pr-10852
cleveragents:fix/10922-conversation-state-mgmt
cleveragents:pr-check
cleveragents:bugfix/10931-preserve-strategy-decisions-json
cleveragents:fix/10903-nox-showcase-docs
cleveragents:pr/10885-pyyaml-upgrade
cleveragents:pr-fix-10931
cleveragents:bugfix/executor-error-details-overwrite-qwen
cleveragents:fix-orchestrator-scaling-32-workers
cleveragents:fix-pr-1107-asgi-uvicorn
cleveragents:feature/m9-timeline-day-99
cleveragents:feat/issue-6369-actor-context-show
cleveragents:improvement/agent-label-compliance
cleveragents:fix-9912-branch
cleveragents:bugfix/10821-fix-tui-keybinding
cleveragents:feat/issue-6450-tui-escape-cascade
cleveragents:bugfix/m8-shell-safety-service-integration
cleveragents:fix/redaction-pattern-exception-handling
cleveragents:bugfix/m8-tui-on-input-changed
cleveragents:fix/action-schema-env-var-exfiltration
cleveragents:feature/spec-timeline-6003
cleveragents:feature/spec-timeline-6008
cleveragents:feature/issue-4746-update-spec-agents-diagnostics-all-9-providers
cleveragents:feat/v3.6.0/gemini-provider
cleveragents:pr/8194
cleveragents:tdd/prompt-input-textarea
cleveragents:feat/v3.6.0/cost-reporting-cli
cleveragents:fix/lsp-transport-security
cleveragents:feat/v3.6.0/semantic-context-strategy
cleveragents:feature/issue-10820-chore-agents-fix-bug-hunt-pool-supervisor-tracking-prefix-auto-bug-pool-to-auto-bug-sup-complete-fix
cleveragents:tdd/mN-registry-thread-safety
cleveragents:fix/v360/remove-acp-module
cleveragents:temp-squash
cleveragents:fix/v360/lsp-runtime-instantiation
cleveragents:feat/690-jsonrpc-routing
cleveragents:feat/v3.6.0-anthropic-gemini-backends
cleveragents:build/agents-system-rewrite
cleveragents:feat/v3.3.0-plan-rollback-cli
cleveragents:feat/v3.3.0-parallel-subplan-scheduler
cleveragents:feature/issue-10846-optimize-benchmark-regression-test-suite
cleveragents:feature/issue-10826-docs-spec-align-checkpoint-trigger-names-and-config-key-path-with-implementation
cleveragents:feature/issue-10744-fix-tui-convert-permissionsscreen-from-static-widget-to-proper-textual-screen-subclass
cleveragents:feature/issue-10794-feat-a2a-implement-a2a-http-transport-for-server-mode
cleveragents:fix/tui-preset-cycling
cleveragents:pr-10820
cleveragents:feature/696-implement-a2a-http-transport-for-server-mode
cleveragents:feature/issue-10792-feat-server-langgraph-platform-remotegraph-integration
cleveragents:feature/issue-1486-fix-v3-7-0-resourcehandler-return-type-1444
cleveragents:feature/issue-1488-fix-v3-7-0-resolve-issue-1432
cleveragents:bugfix/m1-plan-execute-sandbox-root
cleveragents:feature/issue-4663-day-97-schedule-adherence-update
cleveragents:feature/issue-10858-devops-run-linter
cleveragents:docs/milestone-v3.6.0-v3.7.0
cleveragents:feature/issue-10835-add-milestone-based-pr-prioritization
cleveragents:pr-8701-head
cleveragents:fix/7927-apply-phase-dod-gating
cleveragents:fix/sse-formatter-json-rpc-2.0
cleveragents:feat/v3.6.0/scope-chain-assembler-integration
cleveragents:fix/tui-bindings-block-cursor-navigation
cleveragents:fix/v360/compute-actor-impact-exceptions
cleveragents:feat/v360/openrouter-provider
cleveragents:docs/v360/cli-version-info-diagnostics
cleveragents:feat/context-semantic-chunking-strategy
cleveragents:feat/acms-cli-context-show-clear
cleveragents:feature/m7-actor-management-showcase-metadata
cleveragents:feature/m6-4213-resource-skill-showcase
cleveragents:feat/v360/anthropic-gemini-backends
cleveragents:feat/v3.6.0/safety-profile-enforcement
cleveragents:feat/context-dynamic-budget-allocation
cleveragents:refactor/v360/unify-error-handling-cli
cleveragents:fix/v370/tui-materializer-a2a
cleveragents:fix/auto-debug-agent-prompt-injection
cleveragents:refactor/v360/unify-api-naming
cleveragents:test/cli-docstring-example-validation
cleveragents:fix/v360/resource-kind-field
cleveragents:feat/v3.6.0/context-relevance-scoring
cleveragents:fix/v360/plugin-state-executing
cleveragents:fix/v360/lsp-path-traversal-file-reading
cleveragents:feat/acms-semantic-chunking-context-strategy
cleveragents:refactor/v360/unify-service-initialization
cleveragents:bugfix/m3.6.0-lsp-server-dos-message-read-timeout
cleveragents:feat/v360/pluggable-scope-chain-api-v2
cleveragents:docs/v360/actor-management-showcase
cleveragents:docs/v360/actor-removal-impact
cleveragents:docs/v360/align-depth-reduction-devcontainer
cleveragents:tdd/issue-10413-dollar-prefix-shell-mode
cleveragents:fix/issue-10503-session-export-json-stdout
cleveragents:fix/pr-10755
cleveragents:feat/v370/tui-web-mode
cleveragents:feat/v360/plugin-cli-discovery
cleveragents:fix/v360/llm-trace-latency-type
cleveragents:feat/v3.6.0/ollama-mistral-providers
cleveragents:feat/v3.6.0/adaptive-context-selector
cleveragents:feat/tui-v370/persona-registry-merge-v2
cleveragents:feat/v3.6.0/cost-tracker
cleveragents:fix/v360/resource-type-cycle-detection
cleveragents:refactor/auto-guard-1-address-todo-fixme-comments
cleveragents:feat/v3.6.0/pluggable-scope-chain
cleveragents:fix/v360/scope-chain-resolver-registration
cleveragents:test/v360/e2e-a2a-context-management
cleveragents:fix/v360/lsp-env-var-injection
cleveragents:feature/m6-sandbox-correction-invariant-docs
cleveragents:feature/m3-timeline-day97-update
cleveragents:fix/10480-validate-logic-error
cleveragents:feat/acms-cli-context-add
cleveragents:feat/acms-core-pipeline-components
cleveragents:feature/m4652-module-guides
cleveragents:feature/m5-extend-agents-diagnostics-example
cleveragents:feature/m5832-add-unreleased-changelog-entries
cleveragents:docs/add-repo-indexing-showcase
cleveragents:improvement/agent-pr-self-reviewer-blocking-vs-nonblocking
cleveragents:feature/issue-8225-validation-gate-empty-summary
cleveragents:spec/resource-type-yaml-format-canonical-5622
cleveragents:bugfix/m8179-fix-data-integrity-remove-session-rollback-calls-from-projectrepository
cleveragents:feat/v3.6.0/context-policy-strategy-config
cleveragents:test/v3.6.0/a2a-rename-regression-tests
cleveragents:fix/plan-lifecycle-root-decision-type
cleveragents:bugfix/cancel-worktree-cleanup
cleveragents:pr-10586
cleveragents:pr-9215
cleveragents:feat/issue-6357-tui-loading-states
cleveragents:temp-bug2-combined
cleveragents:timeline/day-105-2026-04-15-auto-time-1-v2
cleveragents:docs/consolidated-all-documentation
cleveragents:bugfix/m6-sandbox-reexecute-cleanup
cleveragents:fix/issue-9963-memory-service-timestamp-guards
cleveragents:docs/context-management-deep-dive-v2
cleveragents:docs/context-management-deep-dive
cleveragents:docs/agent-development-guide
cleveragents:feature/10008-file-level-correction-diff
cleveragents:feat/acms-scope-resolution-context-inheritance
cleveragents:docs/a2a-protocol-guide
cleveragents:fix/tui-bindings-reload-settings
cleveragents:docs/tui-user-guide-keybindings
cleveragents:fix/plan-generation-validate-logic
cleveragents:bugfix/issue-10408-dollar-prefix-shell-mode
cleveragents:test/issue-10500-persona-state-reset-tdd
cleveragents:docs/getting-started-tutorial
cleveragents:test/tdd-session-create-suppress-exception
cleveragents:fix/issue-10485-fallback-selector-budget-limits
cleveragents:docs/error-codes-guide
cleveragents:docs/common-tasks-recipes-guide
cleveragents:bugfix/mN-registry-thread-safety
cleveragents:test/migration-runner-sqlite-threading
cleveragents:docs/configuration-reference
cleveragents:pr-10678
cleveragents:pr-10681
cleveragents:test/issue-10510-mcptooladapter-rlock-tdd
cleveragents:feature/tui-screens-directory
cleveragents:fix/issue-10511-suppress-runtimeerror
cleveragents:pr-10676
cleveragents:fix/tui-block-cursor-bindings
cleveragents:pr-10680
cleveragents:test/issue-10502-session-export-json-tdd
cleveragents:fix/issue-10507-sqlite-check-same-thread
cleveragents:docs/installation-setup
cleveragents:test/v3.6.0/scope-chain-integration-tests
cleveragents:fix/v370/loading-throbber-restore
cleveragents:feat/v370/tui-settings-sessions-screens
cleveragents:fix/v370/tui-session-persistence
cleveragents:fix/v360/context-strategy-unification
cleveragents:fix/v370/shell-safety-regex
cleveragents:feat/v370/tui-rebase-merge
cleveragents:feat/v370/tui-complete-squashed
cleveragents:fix/v370/tui-shell-async
cleveragents:feat/v3.6.0/budget-enforcement
cleveragents:refactor/v360/decouple-cli-services
cleveragents:feat/v370/tui-session-persistence
cleveragents:auto-arch-1-spec-module-definitions
cleveragents:docs/v3.6.0-v3.7.0-updates
cleveragents:auto-time/timeline-update-2026-04-18-c3
cleveragents:auto-docs-2/add-changelog-contributing
cleveragents:auto-time/timeline-update-2026-04-18-c2
cleveragents:auto-docs-1/fix-mkdocs-nav-and-links
cleveragents:pr-5968
cleveragents:docs/timeline-day-107-2026-04-17
cleveragents:fix/issue-6323-project-context-show-output
cleveragents:improvement/agent-bug-hunt-pool-supervisor-tracking-prefix
cleveragents:auto-time/update-2026-04-17
cleveragents:docs/auto-docs-8-a2a-rename-documentation
cleveragents:auto-docs-3-v340-v350
cleveragents:docs/timeline-update-2026-04-15
cleveragents:auto-docs/initial-documentation-assessment
cleveragents:feature/m1-initial-documentation
cleveragents:fix/agent-task-list-memory-leak
cleveragents:bugfix/m4-plan-diff-correction-stub
cleveragents:pr-9247
cleveragents:docs/timeline-update-2026-04-17
cleveragents:timeline/day-106-2026-04-17-auto-time-1
cleveragents:fix/quality-gates-click82-compat
cleveragents:auto-arch-14/spec-anonymous-tool-enforcement
cleveragents:fix/issue-6441-session-create-json-output
cleveragents:fix/issue-6331-invariant-add-scope
cleveragents:timeline/day-106-2026-04-16-auto-time-1-v2
cleveragents:spec/auto-arch-23-minor-clarifications
cleveragents:timeline/day-106-2026-04-16-auto-time-2
cleveragents:docs/auto-docs-2-v380-v390
cleveragents:timeline/day-104-2026-04-14-auto-time-1
cleveragents:bugfix/m3-actor-add-v3-schema-validation
cleveragents:timeline/day-106-2026-04-16-auto-time-1
cleveragents:auto-docs/changelog-architecture-readme
cleveragents:spec/auto-arch-21-v350-autonomy-hardening
cleveragents:chore/timeline-day-105-2026-04-15
cleveragents:docs/timeline-update-2026-04-15-auto-time-1
cleveragents:timeline/day-105-2026-04-15-auto-time-1
cleveragents:benchmark-ci
cleveragents:fix/plan-phase-migration-raw-sql-root-plan-id
cleveragents:auto-arch-12/spec-acms-context-tier-hydrator
cleveragents:timeline/day-106-2026-04-15-auto-time-1
cleveragents:feat/invariant-enforcement-strategize
cleveragents:feat/plan-tree-decision-rendering
cleveragents:feat/plan-correct-revert-append-modes
cleveragents:docs/auto-docs-4-fix-conflicts
cleveragents:docs/auto-docs-1-milestone-docs-v3.0.0-v3.1.0
cleveragents:feat/v3.4.0-acms-lifecycle-policy
cleveragents:pr-9220
cleveragents:fix/a2a-facade-optional-param-validation
cleveragents:feat/ci-guard-llm-secrets
cleveragents:pr-9214
cleveragents:feat/v3.3.0-subplan-status-tracking
cleveragents:feat/v3.3.0-merge-conflict-detection
cleveragents:uat/checkpoint-rollback-merge-tests
cleveragents:fix/pr-review-pool-supervisor-prefix-mismatch
cleveragents:feat/v3.3.0-spawn-subplan-step
cleveragents:auto-time-1-day103-cycle1-session6
cleveragents:feat/v3.8.0-agent-card-endpoint
cleveragents:docs/auto-docs-cycle-24-showcase-nav
cleveragents:auto-inf-3-consolidate-behave-fixtures
cleveragents:fix/issue-7663-docs-writer-missing
cleveragents:auto-time-1-day103-cycle2
cleveragents:docs/timeline-day-104-auto-time-1
cleveragents:auto-arch-16/spec-xml-prompt-injection-mitigation
cleveragents:bugfix/m4-invariant-persistence
cleveragents:uat-a2a-facade-tests-v350
cleveragents:bugfix/m3-behave-parallel-failed-chunk-logs
cleveragents:bugfix/7664-automation-tracking-label-requirements
cleveragents:docs/auto-time-1-timeline-update-2026-04-14
cleveragents:docs/auto-docs-1-milestone-v3-updates
cleveragents:fix/issue-6344-plan-execute-rich-output
cleveragents:docs/action-config-schema-api
cleveragents:fix/bug-hunt-supervisor-nonexistent-file-preflight
cleveragents:fix/retry-policy-model-missing-fields
cleveragents:docs/validation-gate-empty-run-guard
cleveragents:auto-arch-15/spec-retry-policy-canonical-fields
cleveragents:docs/lockservice-advisory-locking
cleveragents:docs/changelog-plan-fix-4197
cleveragents:spec/milestone-plan-section
cleveragents:docs/update-changelog-recent-features
cleveragents:fix/test-infra-remove-redundant-python-variable-robot-files
cleveragents:timeline/day-104-2026-04-14-cycle2
cleveragents:fix/bdd-feature-file-tags
cleveragents:auto-arch-13/spec-default-automation-profile
cleveragents:docs/auto-docs-cycle-1-2026-04-12
cleveragents:docs/cycle-1-git-worktree-sandbox
cleveragents:spec/architecture-critical-gap-fixes
cleveragents:docs/timeline-day-104-auto-time-2
cleveragents:auto-arch-1/add-v380-v390-milestone-plan
cleveragents:docs/developer-setup-guide
cleveragents:fix/auto-profile-spec-prose-description
cleveragents:auto-arch-10/spec-tui-a2a-integration-layer
cleveragents:spec/resource-event-types-clarification
cleveragents:auto-docs-4/changelog-and-observability
cleveragents:auto-arch-4/adr-049-layered-boundary-enforcement
cleveragents:docs/a2a-protocol-autonomy-hardening
cleveragents:auto-arch-9/spec-v3.8.0-milestone-plan
cleveragents:docs/auto-docs-3-reference-index
cleveragents:auto-arch-7/spec-apply-git-worktree
cleveragents:docs/timeline-day104-cycle1-auto-time-4
cleveragents:docs/auto-docs-cycle-1-changelog-updates
cleveragents:auto-arch-6/adr-049-spec-restructuring
cleveragents:docs/auto-docs-1-v340-acms-context-management
cleveragents:docs/auto-docs-1-v320-v330-cli-reference
cleveragents:auto-arch-5/v3.9.0-milestone-plan
cleveragents:test/create-scripts
cleveragents:auto-time-1-day104
cleveragents:timeline/day-104-2026-04-14
cleveragents:docs/auto-time-4-day103-cycle5
cleveragents:auto-time-3-day103-cycle4
cleveragents:auto-docs-5-architecture-overview
cleveragents:spec/three-way-merge-strategy-v3.3.0
cleveragents:spec/checkpoint-system-v3.3.0
cleveragents:auto-docs-4-api-docs-update
cleveragents:auto-docs-1-changelog-expansion
cleveragents:spec/invariant-management-system-v3.2.0
cleveragents:pr-8289
cleveragents:spec/plan-correction-engine-v3.2.0
cleveragents:spec/layered-architecture-boundary-policy
cleveragents:spec/tui-materializer-a2a-integration-v3.7.0
cleveragents:spec/decision-recording-system-v3.2.0
cleveragents:docs/auto-docs-1-milestone-overview
cleveragents:pr-7484
cleveragents:pr-4212
cleveragents:auto-arch-3/v3.8.0-milestone-plan
cleveragents:auto-docs-6/troubleshooting-and-config
cleveragents:auto-time-1-day103-session5
cleveragents:auto-docs-5/contributor-guide-and-readme
cleveragents:docs/plan-tree-ulid-examples
cleveragents:docs/m3-spec-clarify-path-datetime-plugin-contracts
cleveragents:docs/auto-docs-cycle-10-diagnostics-ref
cleveragents:auto-docs-3/user-guide-and-architecture
cleveragents:docs/cycle-7-changelog-update
cleveragents:spec/reconciliation-failure-behavior
cleveragents:auto-docs-2/api-documentation
cleveragents:auto-arch-2/adr-053-repositories-decomposition
cleveragents:auto-docs-1/release-notes-v3.0-v3.1
cleveragents:spec/update-validation-attach-project-delete
cleveragents:spec/architecture-cycle2-impl-clarifications
cleveragents:auto-arch-1/adr-049-052-violations
cleveragents:auto-time-1-day103
cleveragents:docs/auto-docs-cycle-13-updates
cleveragents:docs/timeline-day-102-auto-time
cleveragents:timeline/day-103-2026-04-13
cleveragents:spec/arch-invariant-cli-completeness
cleveragents:spec/update-cycle1-validation-attach-project-delete
cleveragents:docs/add-session-management-showcase
cleveragents:spec/arch-sandbox-path-correction-cycle9
cleveragents:spec/architecture-v380-milestone-plan
cleveragents:docs/auto-docs-cycle-12-updates
cleveragents:docs/cycle-1-validation-gate-fix
cleveragents:docs/2026-04-08-unreleased-changelog
cleveragents:docs/auto-docs-cycle-2-2026-04-10
cleveragents:docs/session-4615-2026-04-08-cycle1
cleveragents:feat/issue-6361-shell-safety-service-tui
cleveragents:spec/architecture-cycle-25-new-features
cleveragents:fix/issue-6345-automation-profile-add-output
cleveragents:docs/timeline-day-102-2026-04-12
cleveragents:docs/cycle-2-git-worktree-acms-hydrator
cleveragents:spec/arch-sandbox-cleanup-discovery
cleveragents:docs/timeline-day96-2026-04-08
cleveragents:docs/auto-docs-cycle-11
cleveragents:spec/fix-sandbox-strategy-protocol-name
cleveragents:spec/arch-acms-tier-hydration
cleveragents:fix/v3.4.0/context-settings-defaults
cleveragents:docs/add-example-repl-and-actor-run
cleveragents:docs/auto-docs-cycle-10-updates
cleveragents:docs/session-4-2026-04-08-updates
cleveragents:docs/showcase-all-examples-consolidated
cleveragents:docs/timeline-day-97
cleveragents:docs/acms-context-hydrator-cycle2
cleveragents:docs/add-example-output-format-flags
cleveragents:spec/arch-failfast-cancel-semantics
cleveragents:timeline/day-101-2026-04-11
cleveragents:docs/timeline-day99-2026-04-09-v2
cleveragents:docs/auto-docs-cycle-2-worktree-acms
cleveragents:spec/architecture-v3.8.0-milestone-plan
cleveragents:docs/api-lsp-acms-reference
cleveragents:improvement/agent-bug-hunt-pool-supervisor-yaml-syntax-fix
cleveragents:spec/project-delete-deleted-at-field
cleveragents:spec/architecture-provider-registry-tui-materializer
cleveragents:spec/document-reconciliation-blocked-error-5942
cleveragents:fix/issue-7482-git-log-injection
cleveragents:spec/devcontainer-auto-discovery-schema
cleveragents:feat/issue-6350-conversation-content-pruning
cleveragents:docs/update-module-guides-2026-04-10
cleveragents:timeline/day-100-2026-04-10-auto-time-cycle1
cleveragents:timeline/day-99-2026-04-09-auto-time-v2
cleveragents:docs/cycle-3-module-guides
cleveragents:timeline/day-99-2026-04-09-auto-time
cleveragents:pr-4226
cleveragents:spec/additional-llm-providers-gemini-groq-cohere-together-ollama-mistral
cleveragents:spec/document-context-tier-hydrator-6175
cleveragents:docs/timeline-day99-2026-04-09
cleveragents:spec/invariant-cli-clarifications
cleveragents:docs/add-example-project-init-and-context-management
cleveragents:spec/reconciliation-blocked-error-documentation
cleveragents:spec/fix-invariant-precedence-reference-5861
cleveragents:spec/fix-plan-correct-accepts-plan-id-5558
cleveragents:spec/fix-validation-attach-synopsis-5328
cleveragents:docs/timeline-day-99-cycle-1
cleveragents:docs/timeline-day-99-cycle-2
cleveragents:fix/actor-context-list-regex-arg
cleveragents:docs/timeline-day-99-cycle-3
cleveragents:spec/arch-security-mode-init
cleveragents:docs/auto-docs-cycle-9-updates
cleveragents:fix-resource-fix-resource-remove-to-check-correct-edge-table
cleveragents:feat/issue-6434-tui-env-var-expansion
cleveragents:fix/issue-6321-plan-prompt-timing-field
cleveragents:fix/issue-6322-resource-add-url-flag
cleveragents:feat/issue-6348-sessions-screen
cleveragents:spec/plan-show-command
cleveragents:temp
cleveragents:feat/harden-label-restrictions-1775753628
cleveragents:spec/invariant-reconciliation-failure-behavior
cleveragents:spec/add-reconciliation-failure-behavior-5942
cleveragents:spec/architecture-corrections-cycle3
cleveragents:spec/checkpoint-trigger-names-and-config-key-fix
cleveragents:spec/fix-ai-provider-interface-5801
cleveragents:spec/azure-api-version-default-update
cleveragents:docs/auto-docs-writer-cycle1-labels
cleveragents:spec/fix-resource-type-yaml-format-5622
cleveragents:spec/add-plan-revert-resume-commands-5574
cleveragents:docs/auto-docs-cycle-1-2026-04-09
cleveragents:spec/plan-correct-plan-id-or-decision-id-5558
cleveragents:spec/fix-subgraph-node-actor-ref-field-5427
cleveragents:issue/5284-master-ci-fix
cleveragents:timeline/day-99-2026-04-09-v2
cleveragents:merge-me
cleveragents:docs/session-3377-initial-docs-update
cleveragents:fix/llm-provider-subpackage-exports
cleveragents:spec/arce-acronym-and-tui-keybinding-fixes
cleveragents:spec/architecture-corrections-cycle2
cleveragents:spec/architecture-corrections-cycle1
cleveragents:docs/cycle-1-updates
cleveragents:spec/tui-clarifications-session-export-persona
cleveragents:docs/session-4940-2026-04-08-cycle1
cleveragents:spec/architecture-milestone-plan-v3.2-v3.7
cleveragents:docs/session-4743-2026-04-08-cycle1
cleveragents:docs/timeline-day-98
cleveragents:docs/timeline-day98-2026-04-08-v2
cleveragents:docs/add-example-action-and-plan-management
cleveragents:docs/session-2026-04-06-updates
cleveragents:docs/ca-docs-writer-v3.8.1-2026-04-05
cleveragents:fix/session-tell-stub-missing-panels-and-actor-execution
cleveragents:improvement/agent-arch-guard-clone-failure-handling
cleveragents:improvement/agent-test-infra-health-spam-fix-v2
cleveragents:fix-tdd-invert-non-assertion-exceptions
cleveragents:improvement/agent-arch-guard-clone-failure
cleveragents:bugfix/3472-fix-tdd-inversion-logic
cleveragents:bugfix/989-fix-persistence-json-decode-error
cleveragents:improvement/agent-supervisor-tracking-labels-v2
cleveragents:docs/timeline-day95-v2
cleveragents:docs/timeline-day95-final
cleveragents:docs/update-lsp-api-and-changelog
cleveragents:fix/lsp-resource-handler-module-missing
cleveragents:docs/timeline-day95-final-2026-04-05
cleveragents:fix/a2a-plan-correct-rollback-wiring
cleveragents:docs/add-lsp-api-and-changelog-2026-04-05
cleveragents:fix/tool-registry-validation-type-discriminator
cleveragents:docs/v3.7.0-documentation-update
cleveragents:docs/ca-docs-writer-2026-04-05-cycle2
cleveragents:fix/invariant-set-merge-action-scope
cleveragents:docs/unreleased-feature-docs
cleveragents:fix/concurrency-cost-tracker-record-usage-race-condition
cleveragents:improvement/agent-ca-test-infra-improver-failure-handling
cleveragents:docs/update-changelog-mcp-plan-ci-2026-04-05
cleveragents:improvement/agent-pr-reviewer-milestone-prioritization
cleveragents:docs/timeline-day95-refresh-2026-04-05
cleveragents:improvement/agent-mandatory-labels-tracking-issues
cleveragents:docs/api-domain-providers-changelog-2026-04-05
cleveragents:docs/ca-docs-writer-2026-04-05
cleveragents:docs/timeline-day95-refresh
cleveragents:fix/skill-add-include-validation
cleveragents:docs/timeline-day-95-2026-04-05-update3
cleveragents:docs/timeline-day-95-2026-04-05-update2
cleveragents:docs/ci-incident-runbook-2597
cleveragents:improvement/agent-ca-test-infra-improver-worker-api-mode
cleveragents:docs/shell-safety-api-and-readme-highlights
cleveragents:docs/timeline-day-55-2026-04-04-v2
cleveragents:docs/timeline-day-55-2026-04-04
cleveragents:docs/timeline-day54-update3
cleveragents:improvement/agent-ca-test-infra-improver-fixes
cleveragents:spec/restructure-monolithic-to-split
cleveragents:docs/timeline-day54-update-v2
cleveragents:docs/timeline-day54-update
cleveragents:fix-agents
cleveragents:docs/shell-safety-and-domain-base-model
cleveragents:fix/1452-impl
cleveragents:fix/1473-plan-cancel
cleveragents:fix/1425-test
cleveragents:fix/1426-config
cleveragents:fix/1421-perf
cleveragents:fix/1424-impl
cleveragents:test/int-wf16-devcontainer
cleveragents:feature/m8-tui-persona-export
cleveragents:feature/m7-post-resource-equivalence
cleveragents:test/e2e-m4-acceptance
cleveragents:feature/m6-tantivy-backend
cleveragents:feature/m6-estimation
cleveragents:feature/m6-estimation-report-model
cleveragents:feature/observability-prometheus-audit
cleveragents:feat/server-auth-namespace
cleveragents:feature/m8-session-editing
cleveragents:feature/llm-actor-subplan-wiring
cleveragents:feature/m8-tui-first-run-actor-selection
cleveragents:feature/m8-tui-conversation-block-catalog
cleveragents:feature/m8-tui-settings-screen
cleveragents:feature/m7-e2e-porting
cleveragents:feature/m6-estimation-historical-stats
cleveragents:feature/m8-tui-persona-export-import
cleveragents:feature/m8-tui-sessions-screen
cleveragents:feature/m7-graph-backend
cleveragents:feature/m8-tui-block-context-menu
cleveragents:feature/m8-tui-tool-call-expand
cleveragents:feature/m4-missing-builtin-tools
cleveragents:docs/v3.7.0-release-docs
cleveragents:feature/m8-tui-session-export
cleveragents:test/e2e-wf15-disaster-recovery
cleveragents:test/e2e-wf03-refactoring
cleveragents:test/e2e-m3-acceptance
cleveragents:feature/m8-tui-prompt-history
cleveragents:feature/m8-tui-actor-thought-block-rendering
cleveragents:bugfix/m6-build-hierarchy-child-ids
cleveragents:feature/resource-inheritance-wiring
cleveragents:test/e2e-wf09-session
cleveragents:test/e2e-wf06-doc-generation
cleveragents:test/e2e-wf08-cloud-infra
cleveragents:test/e2e-wf02-test-generation
cleveragents:test/e2e-wf13-custom-profile
cleveragents:test/e2e-wf11-graph-actor
cleveragents:test/e2e-wf01-hello-world
cleveragents:test/int-wf17-explicit-container
cleveragents:test/int-wf12-hierarchical
cleveragents:test/int-wf15-disaster-recovery
cleveragents:test/int-wf13-custom-profile
cleveragents:test/int-wf03-refactoring
cleveragents:test/int-wf11-graph-actor
cleveragents:test/int-wf10-batch
cleveragents:test/int-wf09-session
cleveragents:feature/m3-tdd-issue-consistency-gate
cleveragents:feature/m3-invariant-enforcement-strategize
cleveragents:test/int-wf18-container-clone
cleveragents:test/int-wf01-hello-world
cleveragents:feature/m6-diagnostic-dashboard-health-categories
cleveragents:feature/m6-cli-polish
cleveragents:fix/e2e-db-isolation
cleveragents:feature/m7-post-tui
cleveragents:feature/m9-asgi-endpoint
cleveragents:feature/m7-post-server
cleveragents:tdd/m7-audit-session-race
cleveragents:tdd/m3-skill-add-regression
cleveragents:feature/m9-remote-repos
cleveragents:feature/fs-mount-file-types
cleveragents:tdd/container-resolve-crash
cleveragents:test/e2e-m1-acceptance
cleveragents:test/e2e-m2-acceptance
cleveragents:eugen.thaci-patch-3
cleveragents:eugen.thaci-patch-2
cleveragents:eugen.thaci-patch-1
cleveragents:aditya-fix-latest
cleveragents:feature/m4-secret-masking-llm-context
cleveragents:aditya-fix
cleveragents:refactor/m3-replace-mktemp
cleveragents:refactor/m3-remove-unittest-mock-integration
cleveragents:refactor/m3-remove-robot-mock-imports
cleveragents:refactor/m3-remove-mock-llm-integration
cleveragents:docs/improved-menu-adr
cleveragents:feature/m7-post-auth
cleveragents:feature/m3-fix-resource-bootstrap
cleveragents:feature/post-safety-profile-tests
cleveragents:integration/batch-2026-03-02
cleveragents:feat/slipcover
cleveragents:docs/safety-profile-spec-composition
cleveragents:integrate/freemo-batch-1
cleveragents:feature/m4-error-recovery
cleveragents:feature/m4-security-template
cleveragents:feature/m3-validation-pipeline
cleveragents:develop-aditya-2
cleveragents:feature/m3-diff-review
cleveragents:feature/m3-validation-apply
cleveragents:feature/m6-acp-stubs
cleveragents:feature/m4-correction-flows
cleveragents:feature/m1-plan-execute-runtime
cleveragents:feature/m4-security-exceptions
cleveragents:feature/m4-definition-of-done
cleveragents:feature/m4-correction-model
cleveragents:feature/m1-apply-pipeline
cleveragents:feature/m5-automation-profiles
cleveragents:feature/m2-lsp-stubs
cleveragents:feature/m3-invariants
cleveragents:feature/m1-actor-runtime
cleveragents:feature/docs-v2-restore
cleveragents:feature/m6-perf-scale
cleveragents:feature/m6-validation-edge
cleveragents:feature/m3-session-cli
cleveragents:feature/m1-persistence-tests-robot
cleveragents:feature/m3-config-cli
cleveragents:feature/m1-cli-tests-robot
cleveragents:feature/m5-subplan-tests
cleveragents:feature/m6-review-playbook
cleveragents:feature/aditya-m3-actor-loader
cleveragents:feature/m3-skill-protocol
cleveragents:feature/m4-automation-legacy-cleanup
cleveragents:feature/m3-change-model
cleveragents:feature/m3-skill-git
cleveragents:feature/m3-skill-registry
cleveragents:feature/m4-security-eval
cleveragents:fix/robot-tests
cleveragents:feature/m3-actor-registry
cleveragents:feature/m3-tool-cli
cleveragents:feature/m4-automation-profiles-cli
cleveragents:feature/m2-resource-cli-extensions
cleveragents:feature/m3-actor-loader
cleveragents:feature/m3-tool-domain-robot
cleveragents:feature/m3-skill-domain-robot
cleveragents:feature/m3-skill-cli
cleveragents:feature/m1-resource-db-robot-tests
cleveragents:feature/m3-session-domain-robot
cleveragents:feature/m1-persistence-tests
cleveragents:feature/m1-cli-tests
cleveragents:ten-branches-backup
cleveragents:feature/m3-skill-schema
cleveragents:feature/m3-session-persistence
cleveragents:feature/automation-profiles-and-resource-dag
cleveragents:feature/m1-plan-repo
cleveragents:feature/m1-db-plan-phase-rebaseline
cleveragents:feat/B4-sandbox
cleveragents:feat/B2-cli-wiring
cleveragents:feat/B5-project-persistence
cleveragents:feat/B1-project-data-models
cleveragents:feat/b1-data-models
cleveragents:feat-repo-manager-and-sourcegraph-support
cleveragents:feat/actor-schema
cleveragents:fix/component-isolation-security-fix
cleveragents:feat/ontology-agent
cleveragents:fix/error-handling-security-fix
cleveragents:fix/concurrency-security-fix
cleveragents:fix/serialization-security-fix
cleveragents:fix/server-side-request-forgery-security-fix
cleveragents:fix/file-system-security
cleveragents:fix/template-injection-fix
cleveragents:fix/data-injection-fix
cleveragents:tests/unit-tests
cleveragents:latest/poetry-generator
cleveragents:poetry-generator
cleveragents:config/contract-metadata-extractor
cleveragents:docs/readme-yaml-syntax
cleveragents:config/memory-yaml
cleveragents:fix/double-response
cleveragents:brent-additions
cleveragents:intel_2_demo
Labels
Clear labels
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.
controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.
auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.
auto/claimed-implementer
Currently being processed by an implementer worker.
auto/claimed-merge
Currently being processed by the merge driver.
auto/claimed-reviewer
Currently being processed by a reviewer worker.
auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.
auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.
auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.
auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.
auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.
auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.
Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.
auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.
auto/needs-implementer
Failing CI needs implementer attention.
auto/postmortem
Documenting a driver incident or rollback.
auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.
auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.
auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.
auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.
auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.
auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.
Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.
Bounty
$100
A bounty of $100 for any open-source contributor who provides a MR that solves this issue
Bounty
$1000
A bounty of $1000 for any open-source contributor who provides a MR that solves this issue
Bounty
$10000
A bounty of $10000 for any open-source contributor who provides a MR that solves this issue
Bounty
$20
A bounty of $20 for any open-source contributor who provides a MR that solves this issue
Bounty
$2000
A bounty of $2000 for any open-source contributor who provides a MR that solves this issue
Bounty
$250
A bounty of $250 for any open-source contributor who provides a MR that solves this issue
Bounty
$50
A bounty of $50 for any open-source contributor who provides a MR that solves this issue
Bounty
$500
A bounty of $500 for any open-source contributor who provides a MR that solves this issue
Bounty
$5000
A bounty of $5000 for any open-source contributor who provides a MR that solves this issue
Bounty
$750
A bounty of $750 for any open-source contributor who provides a MR that solves this issue
MoSCoW
Could have
Could have feature in order to satisfy the epic/legendary.
MoSCoW
Must have
Must have feature in order to satisfy the epic/legendary.
MoSCoW
Should have
Should have feature in order to satisfy the epic/legendary.
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.
Points
1
1 man-hours worth of work for an expert with no learning curve.
Points
13
13 man-hours worth of work for an expert with no learning curve.
Points
2
2 man-hours worth of work for an expert with no learning curve.
Points
21
21 man-hours worth of work for an expert with no learning curve.
Points
3
3 man-hours worth of work for an expert with no learning curve.
Points
34
34 man-hours worth of work for an expert with no learning curve.
Points
5
5 man-hours worth of work for an expert with no learning curve.
Points
55
55 man-hours worth of work for an expert with no learning curve.
Points
8
8 man-hours worth of work for an expert with no learning curve.
Points
88
88 man-hours worth of work for an expert with no learning curve.
Priority
Backlog
This ticket has backlogged priority and is not to be worked on yet
Priority
CI Blocker
Critical priority issue that blocks CI/CD pipeline and prevents PR merges
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.
Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.
State
Completed
The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.
State
Duplicate
A ticket that represents the same content as an existing ticket.
State
In Progress
A ticket that is actively being developed.
State
In Review
A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.
State
Paused
This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.
State
Unverified
All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.
State
Verified
The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.
State
Wont Do
This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.
Type
Automation
Any edits or discussion about the AI automated coding system.
Type
Bug
Something that doesnt work as intended.
Type
Discussion
Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.
Type
Documentation
An error or improvement needed in the documentation.
Type
Epic
Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.
Type
Feature
Some new functionality not present.
Type
Legendary
A type of Epic which will contain other Epics.
Type
Refactor
A code change that restructures existing code without changing its external behavior.
Type
Support
Someone needs help using the project.
Type
Task
A generic task that doesnt fit into the other type categories.
Type
Testing
Work exclusively focusing on fixing or expanding testing.
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".
No due date set.
Blocks
#3677 UAT: PlanLifecycleService missing rollback_plan service method - CLI bypasses service layer
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!3911
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/plan-lifecycle-service-rollback-method"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleService - What was implemented - Added PLAN_ROLLED_BACK event type to the EventType enum at src/cleveragents/infrastructure/events/types.py to properly represent successful rollbacks in the domain model. - Implemented rollback_plan(plan_id: str, checkpoint_id: str) -> RollbackResult in PlanLifecycleService (src/cleveragents/application/services/plan_lifecycle_service.py) with: - Plan state validation: rejects rollback when the plan is in terminal APPLIED or CANCELLED states. - Delegation to CheckpointService.selective_rollback() to perform the actual rollback logic and obtain a RollbackResult. - Emission of PLAN_ROLLED_BACK as a domain event to reflect the completed rollback. - checkpoint_service is accepted as an optional constructor parameter; if not provided, a PlanError is raised to preserve backward compatibility. - Updated CLI behavior in src/cleveragents/cli/commands/plan.py so agents plan rollback routes through PlanLifecycleService.rollback_plan() rather than calling CheckpointService.selective_rollback() directly. - Updated PlanLifecycleService module docstring to include rollback_plan in the documented API. - Added Behave feature file features/plan_lifecycle_rollback.feature with 11 scenarios covering state validation, domain events, and delegation. - Added step implementations in features/steps/plan_lifecycle_rollback_steps.py to support the new scenarios. - Key design decisions - rollback_plan returns RollbackResult (the same result type produced by CheckpointService.selective_rollback) so the CLI can display rollback details consistently. - Terminal states APPLIED and CANCELLED are disallowed for rollback to prevent inconsistent or invalid state transitions. - checkpoint_service is optional in the PlanLifecycleService constructor; when omitted (None), a PlanError is raised to retain backward compatibility while signaling explicit dependency requirements. - CLI UI remains powered by CheckpointService for metadata enrichment (e.g., confirmation prompts), but the actual rollback action is performed via PlanLifecycleService to ensure proper domain workflow and event emission. - Technical implications - All rollback logic now flows through the domain service layer (PlanLifecycleService) to preserve invariants and emit domain events, rather than allowing ad-hoc UI routes to bypass service validation. - The UI can still retrieve checkpoint metadata for user confirmation, but the operation that modifies state uses the new rollback_plan pathway. - Tests and behavior coverage were expanded via the new Behave feature and step implementations to validate state handling, events, and delegation. - Affected modules/components - src/cleveragents/infrastructure/events/types.py - src/cleveragents/application/services/plan_lifecycle_service.py - src/cleveragents/cli/commands/plan.py - PlanLifecycleService module docstring - features/plan_lifecycle_rollback.feature - features/steps/plan_lifecycle_rollback_steps.py ISSUES CLOSED: #3677 ```21cce4d9530f428c6b650f428c6b656f11ca5e0aPR #3911 Code Review —
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Reviewed all 6 changed files against the specification (
docs/specification.md§Plan Rollback at line 15948, §CheckpointService Operations at line 19332, §CleverAgents Extension Methods at line 23459), CONTRIBUTING.md rules, the linked issue #3677, and the existingPlanLifecycleServicepatterns (execute_plan,apply_plan,cancel_plan).✅ What Looks Good
Correct delegation target: The PR correctly delegates to
CheckpointService.selective_rollback()per the spec (line 19340), notrollback_to_checkpoint()as the issue body originally suggested. Good spec alignment.Terminal state rejection scope: Only APPLIED and CANCELLED are rejected. ERRORED and CONSTRAINED are correctly allowed, matching the spec's distinction between permanently terminal states and recoverable terminal states (see
Plan.is_terminaldocstring atplan.py:925-934).PLAN_ROLLED_BACKevent type: Follows the existingplan.<action>naming convention (plan.rolled_back), placed correctly in the Plan lifecycle section ofEventType, betweenPLAN_CANCELLEDandPLAN_ERRORED.Commit message: Follows Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓PR metadata: Has
Type/Buglabel,Closes #3677in body, branch name matches issue metadata. ✓Test structure: Feature file is well-organized with clear section headers (delegation, state validation, domain events, NotFoundError propagation). Step definitions use the
plr-prefix to avoid collisions. Scenarios are descriptive and test meaningful behavior.RollbackResultreturn type: The design decision to returnRollbackResultinstead ofNone(as the issue originally specified) is well-justified — it allows the CLI to display rollback details without reaching back intoCheckpointService.🔄 Required Changes
1. [SPEC] Docstring table inconsistency with actual behavior
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableIssue: The updated docstring table shows:
But Scenario 7 in the feature file tests rollback from Strategize/QUEUED state, and the PR description says "all other states are considered valid for rollback" (i.e., any non-permanently-terminal state). The docstring table restricts the "From" column to Execute only, which contradicts both the test coverage and the stated design intent.
Required: Update the docstring table to accurately reflect the actual behavior. If rollback is valid from any non-permanently-terminal phase/state, the table should say something like:
Or if the intent is truly Execute-only, remove the Strategize test scenario.
Reference: Spec line 15996 shows post-rollback state as
Phase: execute, State: queued, which implies rollback is an Execute-phase operation. Clarify whether Strategize-phase rollback is intentional or an over-extension.2. [ERROR-HANDLING] Missing test for CheckpointService exception propagation
Location:
features/plan_lifecycle_rollback.featureIssue: The spec at line 19340 explicitly defines two error cases for
selective_rollback:BusinessRuleViolationif recovery fails after a partial rollbackResourceNotFoundErrorif the checkpoint does not existThere is no test scenario verifying that exceptions raised by
CheckpointService.selective_rollback()propagate correctly throughrollback_plan. Per the project's fail-fast error handling principles (CONTRIBUTING.md), these exceptions should propagate without being caught or swallowed.Required: Add at least one scenario (ideally two) testing that:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure), it propagates throughrollback_planThis is critical because the service layer must not accidentally swallow checkpoint-level errors.
3. [DOC] Scenario count discrepancy
Location: PR description body
Issue: The PR description states "Added Behave feature file
features/plan_lifecycle_rollback.featurewith 11 scenarios" but the feature file contains 10 scenarios:Required: Correct the PR description to say 10 scenarios (or add the missing 11th scenario if one was intended but omitted).
💡 Non-Blocking Suggestions
4. [API-CONSISTENCY] Return type deviation from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified, but consider whether returning a richer type (e.g., a dataclass containing both the updatedPlanand theRollbackResult) would maintain API consistency while still providing rollback details to the CLI.5. [API-CONSISTENCY] Constructor error pattern divergence
Other optional services (
event_bus,decision_service,error_pattern_service,invariant_service) silently skip their functionality whenNone. The newcheckpoint_serviceraisesPlanErrorwhenNoneandrollback_planis called. While this is a reasonable backward-compatibility approach, it's a different error pattern. Consider:RuntimeErrororTypeErrorinstead ofPlanErrorfor a missing dependency (since this is a wiring/configuration error, not a domain error)6. [TEST] Consider testing ERRORED and CONSTRAINED state acceptance
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. Consider adding scenarios for ERRORED and CONSTRAINED states, since these are the recoverable-terminal states that the PR explicitly allows (and which distinguish
rollback_plan's validation fromcancel_plan'sis_terminalcheck).PR Metadata Check
Closes #3677)Type/labelType/BugDecision: REQUEST CHANGES 🔄
Issues #1 (spec/docstring inconsistency) and #2 (missing error propagation tests) must be addressed before approval. Issue #3 (scenario count) is a minor documentation fix.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
6f11ca5e0a49d34970bb🔍 Code Review — REQUEST CHANGES
Reviewer: ca-pr-self-reviewer | Focus areas: specification-compliance, error-handling-patterns, api-consistency
Overview
The overall design is sound: routing
agents plan rollbackthroughPlanLifecycleServiceinstead of callingCheckpointServicedirectly is the correct architectural fix. The state validation, event emission, and delegation pattern are well-implemented and consistent with existing service methods. However, there is one documentation issue that must be corrected before merge because it misrepresents the API contract.❌ Required Changes
1. [SPEC] Module docstring phase transition table is incorrect for
rollback_plansrc/cleveragents/application/services/plan_lifecycle_service.py, line 21ProcessingState.APPLIEDandProcessingState.CANCELLED— it does not restrict by phase.Execute/QUEUED. It validates, delegates toCheckpointService.selective_rollback(), emits an event, and returns theRollbackResult. The plan's state is unchanged by this method.✅ Good Aspects
✅ Specification Compliance (core design): The service-layer routing pattern is correct and consistent with
use_action,execute_plan,apply_plan, andcancel_plan. The spec definesagents plan rollbackas a CLI command, and having it go through the service layer for state validation and event emission is architecturally sound.✅ State Validation Logic: The choice to block only
APPLIEDandCANCELLED(permanently terminal) while allowingERROREDandCONSTRAINED(recoverable terminal) is correct per thePlan.is_terminaldocstring (lines 927-934 ofplan.py): "ERRORED and CONSTRAINED plans can still be reverted... APPLIED and CANCELLED are permanently terminal." Rollback is a recovery mechanism and should be available for recoverable states.✅ Error Handling Patterns: The fail-fast ordering is correct: checkpoint_service None check → plan existence (via
get_plan) → terminal state check → delegation. This matches the pattern used bycancel_planand other service methods. Exception propagation fromCheckpointService(BusinessRuleViolation,NotFoundError) is properly documented.✅ API Consistency: The method signature
rollback_plan(plan_id: str, checkpoint_id: str) -> RollbackResultis clean. The return type choice (returningRollbackResultrather thanNoneorbool) is well-reasoned — it allows the CLI to display rollback details without reaching back intoCheckpointService. The optionalcheckpoint_serviceconstructor parameter follows the same pattern as other optional dependencies (event_bus,invariant_service, etc.).✅ Event Type:
PLAN_ROLLED_BACK = "plan.rolled_back"follows the existing<domain>.<action>naming convention and is placed correctly in the Plan lifecycle section of theEventTypeenum.✅ Test Quality: 11 Behave scenarios with good coverage: delegation verification, missing checkpoint service, both terminal states rejected, multiple valid states accepted (Execute/PROCESSING, Execute/QUEUED, Strategize/QUEUED), event emission with content verification, no-event-bus graceful handling, and NotFoundError propagation. Step definitions use the
plr-prefix to avoid collisions.✅ Commit Format: Single atomic commit following Conventional Changelog format with
ISSUES CLOSED: #3677footer.✅ CLI Change: The CLI correctly separates read-only metadata enrichment (still via
CheckpointService.get_checkpoint()) from the state-mutating rollback operation (now viaPlanLifecycleService.rollback_plan()). The comment at line 3597-3599 explains this design decision clearly.💡 Minor Suggestions (Non-blocking)
State validation comment: Consider adding a brief comment at line 2133 explaining why only
APPLIEDandCANCELLEDare blocked (notERRORED/CONSTRAINED), referencing the distinction between "permanently terminal" and "recoverable terminal" states fromPlan.is_terminal. This would help future maintainers understand the intentional asymmetry withcancel_plan(which usesplan.is_terminalto block all four).Mock helper location: The helper functions
_make_mock_checkpoint_service()and_make_mock_event_bus()infeatures/steps/plan_lifecycle_rollback_steps.pycreate mock objects with configured return values. Per CONTRIBUTING.md, mock implementations should be infeatures/mocks/. Consider moving these helpers there for consistency, though this is a minor organizational concern.PR milestone: The PR has no milestone assigned. The linked issue #3677 is also backlog with no milestone, so this is acceptable, but worth noting for tracking purposes.
Decision: REQUEST CHANGES 🔄
The single required change (docstring table accuracy) is straightforward to fix. Once the phase transition table correctly reflects the method's actual behavior, this PR is ready for approval.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Code Review —
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Reviewer focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed:
plan_lifecycle_service.py,types.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.pySpec sections consulted: §
agents plan rollback(lines 15948–16119), §CheckpointService Operations (line 19332–19344), §CleverAgents Extension Methods (line 23459), §Plan-level rollback policy (lines 19324–19330)✅ What Looks Good
Core architectural fix is correct: Routing
agents plan rollbackthroughPlanLifecycleServiceinstead of callingCheckpointServicedirectly is the right fix. This aligns with the service layer pattern used by all other plan lifecycle operations and restores the architectural consistency identified as missing in issue #3677.Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec line 19340, notrollback_to_checkpoint()as the issue body originally suggested.Terminal state validation logic: Blocking only
APPLIEDandCANCELLED(permanently terminal) while allowingERROREDandCONSTRAINED(recoverable terminal) is correct per thePlan.is_terminaldocstring distinction between permanently terminal and recoverable states.PLAN_ROLLED_BACKevent type: Follows the<domain>.<action>naming convention (plan.rolled_back), placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED. Clean addition.Commit format: Single atomic commit, Conventional Changelog format (
fix(plan-lifecycle): ...),ISSUES CLOSED: #3677footer. ✓PR metadata:
Type/Buglabel ✓,Closes #3677✓, branch name matches issue metadata ✓.Test structure: Feature file is well-organized with clear section headers. Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior, not just coverage padding.❌ Required Changes
1. [SPEC] Three-way inconsistency: docstring table ≠ implementation ≠ specification
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableIssue: There is a three-way inconsistency between the docstring, the implementation, and the specification:
Specifically:
(a) "From" column is too narrow: The docstring says "Execute (any state)" but the state validation only checks
ProcessingState.APPLIEDandProcessingState.CANCELLED— it does not restrict by phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED and it passes.(b) "To" column claims a state transition that doesn't happen: The docstring says the method transitions the plan to "Execute/QUEUED", but the implementation does not mutate
plan.phaseorplan.processing_state. It validates, delegates toCheckpointService.selective_rollback(), emits an event, and returnsRollbackResult. The plan's state is unchanged.(c) Spec shows post-rollback state as execute/queued: The spec at lines 15996 and 16069–16072 shows
"phase": "execute"and"state": "queued (awaiting input)"in the CLI output. This could be interpreted as the expected post-rollback state, or simply the state of the example plan. However, given that checkpoints are created during the Execute phase (spec line 19130: "Execute must support checkpointing / rollback"), the spec's intent appears to be that rollback is an Execute-phase operation that resets the plan to execute/queued.Required: Resolve the three-way inconsistency. The recommended approach:
Any (non-terminal) | *(unchanged)*and add a code comment explaining why the spec example shows execute/queued (because that's the typical case, not a mandated transition).Why this matters: This table is the primary API contract documentation for
PlanLifecycleService. A mismatch between documented transitions and actual behavior will cause incorrect assumptions in downstream code, A2A protocol handlers, and future maintenance.2. [ERROR-HANDLING] Missing tests for CheckpointService exception propagation
Location:
features/plan_lifecycle_rollback.featureIssue: The spec at line 19340 explicitly defines two error cases for
selective_rollback:BusinessRuleViolationif recovery fails after a partial rollbackResourceNotFoundErrorif the checkpoint does not existThere are no test scenarios verifying that these exceptions propagate correctly through
rollback_plan. The existingNotFoundErrortest (scenario 10) tests the case where the plan doesn't exist (viaget_plan), not where the checkpoint doesn't exist (viaselective_rollback).Per the project's fail-fast error handling principles (CONTRIBUTING.md): exceptions must propagate without being caught or swallowed. The service layer must not accidentally absorb checkpoint-level errors.
Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planuncaughtCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure after partial rollback), it propagates throughrollback_planuncaughtThese are critical because the service layer wraps the checkpoint call — if a future maintainer adds a try/except around the delegation, these tests would catch the regression.
3. [DOC] Scenario count discrepancy in PR description and commit message
Location: PR description body and commit message
Issue: Both the PR description and the commit message state "11 scenarios" but the feature file contains 10 scenarios:
Required: Correct the PR description to say 10 scenarios (or 12+ after adding the exception propagation scenarios from issue #2 above). The commit message will also need amending during rebase.
💡 Non-Blocking Suggestions
4. [API-CONSISTENCY] Constructor error pattern divergence
All other optional services in the
PlanLifecycleServiceconstructor (event_bus,decision_service,error_pattern_service,invariant_service,config_service,job_store) silently skip their functionality whenNone. The newcheckpoint_serviceraisesPlanErrorwhenNoneandrollback_planis called. While this is a reasonable backward-compatibility approach, it's a different error pattern.Consider:
checkpoint_serviceraisesPlanErrorifrollback_planis called without it, because rollback cannot be silently skipped.")RuntimeErrororTypeErrorinstead ofPlanErrorfor a missing dependency — this is a wiring/configuration error, not a domain error.5. [TEST] Mock helpers should be in
features/mocks/The helper functions
_make_mock_checkpoint_service()and_make_mock_event_bus()infeatures/steps/plan_lifecycle_rollback_steps.pycreate mock objects with configured return values. Per CONTRIBUTING.md, mock implementations should reside infeatures/mocks/. Consider moving these helpers there for consistency with the existing pattern (seefeatures/mocks/mock_ai_provider.py,features/mocks/mock_devcontainer_cli.py, etc.).6. [TEST] Consider testing ERRORED and CONSTRAINED state acceptance
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. Consider adding scenarios for ERRORED and CONSTRAINED states, since these are the recoverable-terminal states that the PR explicitly allows and which distinguish
rollback_plan's validation fromcancel_plan'sis_terminalcheck. These states are the most interesting edge cases for rollback.7. [SPEC] Strategize-phase rollback validity question
The spec's rollback documentation (lines 15948–16119) only shows examples from Execute phase. Checkpoints are created during Execute phase (spec line 19130). It's worth considering whether a plan in Strategize phase would even have checkpoints to roll back to. If not, the Strategize acceptance test (scenario 7) may be testing a scenario that can't occur in practice. This doesn't need to block the PR, but the design decision should be documented with a comment explaining the rationale.
PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreDecision: REQUEST CHANGES 🔄
Issues #1 (three-way docstring/code/spec inconsistency), #2 (missing exception propagation tests), and #3 (scenario count) must be addressed before approval. Issue #1 is the most significant — the API contract documentation must accurately reflect the method's behavior.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
49d34970bbf6d48192b2f6d48192b2ff4e5ad021Code Review —
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed:
plan_lifecycle_service.py(module docstring + rollback_plan method),types.py(EventType enum),plan.py(CLI rollback command),plan_lifecycle_rollback.feature(10 scenarios),plan_lifecycle_rollback_steps.py(step definitions)Spec sections consulted: §
agents plan rollback(lines 15948–16119), §CheckpointService Operations (lines 19332–19344), §Plan.is_terminal docstring (lines 925–934)Issue reviewed: #3677
✅ What Looks Good
Core architectural fix is correct and well-motivated. Routing
agents plan rollbackthroughPlanLifecycleServiceinstead of callingCheckpointServicedirectly restores the service-layer pattern used byuse_action,execute_plan,apply_plan, andcancel_plan. This is the right fix for the gap identified in issue #3677.Correct delegation target. The PR delegates to
CheckpointService.selective_rollback()per spec line 19340, notrollback_to_checkpoint()as the issue body originally suggested. Good spec reading.Terminal state validation is well-reasoned. Blocking only
APPLIEDandCANCELLED(permanently terminal) while allowingERROREDandCONSTRAINED(recoverable terminal) is correct per thePlan.is_terminaldocstring distinction. Rollback is a recovery mechanism and should be available for recoverable states.PLAN_ROLLED_BACKevent type is clean. Follows the<domain>.<action>naming convention (plan.rolled_back), placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED.Test structure is solid. Feature file is well-organized with clear section headers (delegation, state validation, domain events, NotFoundError propagation). Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior.Commit format is correct. Single atomic commit, Conventional Changelog format (
fix(plan-lifecycle): ...),ISSUES CLOSED: #3677footer. ✓PR metadata is mostly complete.
Type/Buglabel ✓,Closes #3677✓, branch name matches issue metadata ✓.❌ Required Changes
1. [SPEC] Module docstring phase transition table is inaccurate in both columns
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableCurrent text:
Issue: This table entry is inaccurate in two independent ways:
(a) "From" column is too narrow. The table says "Execute (any state)" but the state validation only checks
ProcessingState.APPLIEDandProcessingState.CANCELLED— it does not restrict by phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED and it passes. The implementation accepts any non-permanently-terminal state regardless of phase.(b) "To" column claims a state transition that doesn't happen. The table says the method transitions the plan to "Execute/QUEUED", but the implementation does not mutate
plan.phaseorplan.processing_state. It validates, delegates toCheckpointService.selective_rollback(), emits an event, and returnsRollbackResult. The plan's state is unchanged by this method.Why this matters: This table is the primary API contract documentation for
PlanLifecycleService. Every other row in the table accurately describes the method's preconditions and postconditions. A mismatch here will cause incorrect assumptions in downstream code, A2A protocol handlers, and future maintenance.Required: Resolve the inconsistency. The recommended approach:
Any (non-terminal) | *(unchanged)*rollback_planmethod body explaining why the spec example shows execute/queued (because that's the typical case, not a mandated transition)2. [ERROR-HANDLING] Missing tests for CheckpointService exception propagation
Location:
features/plan_lifecycle_rollback.featureIssue: The spec at line 19340 explicitly defines two error cases for
selective_rollback:BusinessRuleViolationif recovery fails after a partial rollbackResourceNotFoundErrorif the checkpoint does not existThere are no test scenarios verifying that these exceptions propagate correctly through
rollback_plan. The existingNotFoundErrortest (scenario 10) tests the case where the plan doesn't exist (viaget_plan), not where the checkpoint doesn't exist (viaselective_rollback).Per the project's fail-fast error handling principles (CONTRIBUTING.md): exceptions must propagate without being caught or swallowed. The service layer wraps the checkpoint call — if a future maintainer adds a try/except around the delegation, these tests would catch the regression.
Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planuncaughtCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure after partial rollback), it propagates throughrollback_planuncaught3. [DOC] Scenario count discrepancy in PR description and commit message
Location: PR description body and commit message
Issue: Both the PR description and the commit message state "11 scenarios" but the feature file contains 10 scenarios:
Required: Correct the PR description to match the actual count (will be 12+ after adding the exception propagation scenarios from issue #2). The commit message will also need amending during rebase.
💡 Non-Blocking Suggestions
4. [ERROR-HANDLING] Verify fail-fast argument validation in
rollback_planPer CONTRIBUTING.md: "All public and protected methods must validate their arguments as the first step of execution (fail-fast). This includes checks for value ranges, nulls, expected types, and empty strings/collections."
Ensure that
rollback_plan(plan_id, checkpoint_id)validates both arguments are non-empty strings before proceeding to the checkpoint_service None check or plan lookup. The other lifecycle methods (execute_plan,apply_plan,cancel_plan) should set the pattern to follow.5. [API-CONSISTENCY] Constructor error pattern divergence
All other optional services in the constructor (
event_bus,decision_service,error_pattern_service,invariant_service,config_service,job_store) silently skip their functionality whenNone. The newcheckpoint_serviceraisesPlanErrorwhenNoneandrollback_planis called. While this is a reasonable backward-compatibility approach, it's a different error pattern.Consider using
RuntimeErrororTypeErrorinstead ofPlanErrorfor a missing dependency — this is a wiring/configuration error, not a domain error. At minimum, document this distinction in the constructor docstring.6. [TEST] Mock helpers should be in
features/mocks/The helper functions
_make_mock_checkpoint_service()and_make_mock_event_bus()infeatures/steps/plan_lifecycle_rollback_steps.pycreate mock objects. Per CONTRIBUTING.md, mock implementations should reside infeatures/mocks/. Consider moving these helpers there for consistency with existing patterns.7. [TEST] Consider testing ERRORED and CONSTRAINED state acceptance
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. Consider adding scenarios for ERRORED and CONSTRAINED states, since these are the recoverable-terminal states that the PR explicitly allows and which distinguish
rollback_plan's validation fromcancel_plan'sis_terminalcheck.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The core design is sound and the architectural fix is correct. Three issues must be addressed before approval:
Issues #1 and #2 are substantive; #3 is trivial. Once addressed, this PR should be ready for approval.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Code Review —
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed:
plan_lifecycle_service.py(module docstring, constructor,rollback_planmethod),types.py(EventTypeenum),plan.py(CLI rollback command),plan_lifecycle_rollback.feature(10 scenarios),plan_lifecycle_rollback_steps.py(step definitions)Spec sections consulted: §
agents plan rollback(lines 15948–16119), §CheckpointService Operations (lines 19332–19344), §Plan-level rollback policy (lines 19324–19330)CONTRIBUTING.md sections consulted: §Argument Validation (line 476), §Exception Propagation (line 496), §Fail-Fast Principles (line 506), mock location rule (line 1172)
Issue reviewed: #3677
✅ What Looks Good
Core architectural fix is correct. Routing
agents plan rollbackthroughPlanLifecycleServiceinstead of callingCheckpointServicedirectly restores the service-layer pattern used byuse_action,execute_plan,apply_plan, andcancel_plan. This closes the gap identified in issue #3677 where state validation and domain event emission were being bypassed.Correct delegation target. Delegates to
CheckpointService.selective_rollback()per spec line 19340, notrollback_to_checkpoint()as the issue body originally suggested.Terminal state validation logic is well-reasoned. Blocking only
APPLIEDandCANCELLED(permanently terminal) while allowingERROREDandCONSTRAINED(recoverable terminal) is correct per thePlan.is_terminaldocstring distinction. This is the right design — rollback is a recovery mechanism and should be available for recoverable states.PLAN_ROLLED_BACKevent type is clean. Follows the<domain>.<action>naming convention (plan.rolled_back), placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED. Verified against both master and branch versions oftypes.py.Test structure is solid. Feature file is well-organized with clear section headers (delegation, state validation, domain events, NotFoundError propagation). Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior, not just coverage padding.Commit format is correct. Single atomic commit, Conventional Changelog format (
fix(plan-lifecycle): ...),ISSUES CLOSED: #3677footer. ✓PR metadata is mostly complete.
Type/Buglabel ✓,Closes #3677✓, branch name matches issue metadata ✓.❌ Required Changes
1. [SPEC] Module docstring phase transition table is inaccurate in both columns
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions table (around line 21 on the branch)Current text:
Issue: This table entry is inaccurate in two independent ways:
(a) "From" column is too narrow. The table says "Execute (any state)" but the state validation in
rollback_planonly checksProcessingState.APPLIEDandProcessingState.CANCELLED— it does not restrict by phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED and it passes. The implementation accepts any non-permanently-terminal state regardless of phase.(b) "To" column claims a state transition that doesn't happen. The table says the method transitions the plan to "Execute/QUEUED", but the implementation does not mutate
plan.phaseorplan.processing_state. It validates, delegates toCheckpointService.selective_rollback(), emits an event, and returnsRollbackResult. The plan's state is unchanged by this method.I verified the spec at lines 15996 and 16069–16072 shows
"phase": "execute"and"state": "queued (awaiting input)"in the CLI output example. However, this appears to be the state of the example plan, not a mandated post-rollback state transition.Why this matters: This table is the primary API contract documentation for
PlanLifecycleService. Every other row in the table (lines 16–20 on master) accurately describes the method's preconditions and postconditions. A mismatch here will cause incorrect assumptions in downstream code, A2A protocol handlers, and future maintenance.Required: Resolve the inconsistency. Recommended approach:
Any (non-terminal) | *(unchanged)*rollback_planmethod body explaining why the spec example shows execute/queued (because that's the typical case, not a mandated transition)2. [ERROR-HANDLING] Missing tests for CheckpointService exception propagation
Location:
features/plan_lifecycle_rollback.featureIssue: The spec at line 19340 explicitly defines two error cases for
selective_rollback:BusinessRuleViolationif recovery fails after a partial rollbackResourceNotFoundErrorif the checkpoint does not existThere are no test scenarios verifying that these exceptions propagate correctly through
rollback_plan. The existingNotFoundErrortest (scenario 10) tests the case where the plan doesn't exist (viaget_plan), not where the checkpoint doesn't exist (viaselective_rollback).Per CONTRIBUTING.md §Exception Propagation (line 496): "CRITICAL: Do not suppress errors. Let exceptions propagate to top-level execution." The service layer wraps the checkpoint call — if a future maintainer adds a try/except around the delegation, these tests would catch the regression.
Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planuncaughtCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure after partial rollback), it propagates throughrollback_planuncaughtThese are critical for fail-fast compliance and regression prevention.
3. [DOC] Scenario count discrepancy in PR description and commit message
Location: PR description body and commit message
Issue: Both the PR description and the commit message state "11 scenarios" but the feature file contains 10 scenarios:
Required: Correct the PR description to match the actual count (will be 12+ after adding the exception propagation scenarios from issue #2). The commit message will also need amending during rebase.
💡 Non-Blocking Suggestions
4. [ERROR-HANDLING] Verify fail-fast argument validation in
rollback_planPer CONTRIBUTING.md §Argument Validation (line 476): "All public and protected class methods must validate arguments as the first guard." This includes checks for empty strings.
Ensure that
rollback_plan(plan_id, checkpoint_id)validates both arguments are non-empty strings before proceeding to the checkpoint_service None check or plan lookup. The other lifecycle methods (execute_plan,apply_plan,cancel_plan) should set the pattern to follow. If they don't validate either, this is a pre-existing gap — but new code should follow the documented standard.5. [API-CONSISTENCY] Constructor error pattern divergence
All other optional services in the
PlanLifecycleServiceconstructor (event_bus,decision_service,error_pattern_service,invariant_service,config_service,job_store) silently skip their functionality whenNone. The newcheckpoint_serviceraisesPlanErrorwhenNoneandrollback_planis called. While this is a reasonable backward-compatibility approach, it's a different error pattern.Consider:
checkpoint_serviceraisesPlanErrorifrollback_planis called without it, because rollback cannot be silently skipped.")RuntimeErrororTypeErrorinstead ofPlanErrorfor a missing dependency — this is a wiring/configuration error, not a domain error6. [TEST] Mock helpers should be in
features/mocks/The helper functions
_make_mock_checkpoint_service()and_make_mock_event_bus()infeatures/steps/plan_lifecycle_rollback_steps.pycreate mock objects with configured return values. Per CONTRIBUTING.md (line 1172): "Mocking code belongs underfeatures/mocks/." Consider moving these helpers there for consistency with existing patterns.7. [TEST] Consider testing ERRORED and CONSTRAINED state acceptance
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. Consider adding scenarios for ERRORED and CONSTRAINED states, since these are the recoverable-terminal states that the PR explicitly allows and which distinguish
rollback_plan's validation fromcancel_plan'sis_terminalcheck. These are the most interesting edge cases for rollback.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The core design is sound and the architectural fix is correct. Three issues must be addressed before approval:
Issues #1 and #2 are substantive; #3 is trivial. Once addressed, this PR should be ready for approval.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Code Review —
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed:
src/cleveragents/infrastructure/events/types.py(EventType enum),src/cleveragents/application/services/plan_lifecycle_service.py(module docstring + rollback_plan method + constructor),src/cleveragents/cli/commands/plan.py(CLI rollback command),features/plan_lifecycle_rollback.feature(10 scenarios),features/steps/plan_lifecycle_rollback_steps.py(step definitions)Spec sections consulted: §
agents plan rollback(lines 15948–16119), §CheckpointService Operations (lines 19332–19344), §Plan-level rollback policy (lines 19324–19330), §Plan.is_terminal docstring (lines 925–934)Issue reviewed: #3677
✅ What Looks Good
Core architectural fix is correct and well-motivated. Routing
agents plan rollbackthroughPlanLifecycleServiceinstead of callingCheckpointServicedirectly restores the service-layer pattern used byuse_action,execute_plan,apply_plan, andcancel_plan. This closes the architectural gap identified in issue #3677 where state validation and domain event emission were being bypassed.Correct delegation target. The PR delegates to
CheckpointService.selective_rollback()per spec line 19340, notrollback_to_checkpoint()as the issue body originally suggested. Good spec reading.Terminal state validation logic is well-reasoned. Blocking only
APPLIEDandCANCELLED(permanently terminal) while allowingERROREDandCONSTRAINED(recoverable terminal) is correct per thePlan.is_terminaldocstring distinction between permanently terminal and recoverable states. Rollback is a recovery mechanism and should be available for recoverable states.PLAN_ROLLED_BACKevent type is clean. Follows the<domain>.<action>naming convention (plan.rolled_back), placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED. Verified against master — this is the only change totypes.py(single line addition). ✅Test structure is solid. Feature file is well-organized with clear section headers (delegation, state validation, domain events, NotFoundError propagation). Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior, not just coverage padding. The event content verification (checkingcheckpoint_idin event details) is a good quality test.Commit format is correct. Single atomic commit, Conventional Changelog format (
fix(plan-lifecycle): ...), detailed body with "What was implemented", "Key design decisions", "Technical implications", and "Affected modules/components" sections.ISSUES CLOSED: #3677footer. ✓PR metadata is mostly complete.
Type/Buglabel ✓,Closes #3677✓, branch namefix/plan-lifecycle-service-rollback-methodmatches issue metadata ✓.CLI separation of concerns. The CLI correctly separates read-only metadata enrichment (still via
CheckpointService.get_checkpoint()) from the state-mutating rollback operation (now viaPlanLifecycleService.rollback_plan()). This is the right architectural boundary.❌ Required Changes
1. [SPEC / API-CONSISTENCY] Module docstring phase transition table contradicts implementation in both columns
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableCurrent text:
Issue: This table entry is inaccurate in two independent ways, creating a three-way inconsistency between the docstring, the implementation, and the specification:
(a) "From" column is too narrow. The table says "Execute (any state)" but the state validation in
rollback_planonly checksProcessingState.APPLIEDandProcessingState.CANCELLED— it does not restrict by phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED and it passes. The implementation accepts any non-permanently-terminal state regardless of phase.(b) "To" column claims a state transition that doesn't happen. The table says the method transitions the plan to "Execute/QUEUED", but the implementation does not mutate
plan.phaseorplan.processing_state. It validates, delegates toCheckpointService.selective_rollback(), emits an event, and returnsRollbackResult. The plan's state is unchanged by this method.The spec at lines 15996 and 16069–16072 shows
"phase": "execute"and"state": "queued (awaiting input)"in the CLI output example, but this appears to be the state of the example plan, not a mandated post-rollback state transition.Why this matters: This table is the primary API contract documentation for
PlanLifecycleService. Every other row in the table accurately describes the method's preconditions and postconditions. A mismatch here will cause incorrect assumptions in downstream code, A2A protocol handlers, and future maintenance. This is the most significant issue in the PR.Required: Resolve the inconsistency. Recommended approach:
Any (non-terminal) | *(unchanged)*rollback_planmethod body explaining why the spec example shows execute/queued (because that's the typical case, not a mandated transition)2. [ERROR-HANDLING] Missing tests for CheckpointService exception propagation through the service layer
Location:
features/plan_lifecycle_rollback.featureIssue: The spec at line 19340 explicitly defines two error cases for
selective_rollback:BusinessRuleViolationif recovery fails after a partial rollbackResourceNotFoundErrorif the checkpoint does not existThere are no test scenarios verifying that these exceptions propagate correctly through
rollback_plan. The existingNotFoundErrortest (scenario 10) tests the case where the plan doesn't exist (viaget_plan), not where the checkpoint doesn't exist (viaselective_rollback).Per CONTRIBUTING.md §Exception Propagation: "CRITICAL: Do not suppress errors. Let exceptions propagate to top-level execution." The service layer wraps the checkpoint call — if a future maintainer adds a try/except around the delegation, these tests would catch the regression. This is exactly the kind of error-handling gap that the fail-fast principle is designed to prevent.
Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planuncaughtCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure after partial rollback), it propagates throughrollback_planuncaughtThese can be implemented by configuring the mock
CheckpointServiceto raise the appropriate exception viaside_effect.3. [DOC] Scenario count discrepancy in PR description and commit message
Location: PR description body and commit message body
Issue: Both the PR description and the commit message state "11 scenarios" but the feature file contains 10 scenarios:
Required: Correct the PR description to match the actual count (will be 12+ after adding the exception propagation scenarios from issue #2). The commit message will also need amending during rebase — per CONTRIBUTING.md, no fix-up commits are allowed; the commit must be amended.
💡 Non-Blocking Suggestions
4. [ERROR-HANDLING] Verify fail-fast argument validation in
rollback_planPer CONTRIBUTING.md §Argument Validation: "All public and protected class methods must validate arguments as the first guard." This includes checks for empty strings and None values.
Ensure that
rollback_plan(plan_id, checkpoint_id)validates both arguments are non-empty strings before proceeding to the checkpoint_service None check or plan lookup. The other lifecycle methods (execute_plan,apply_plan,cancel_plan) should set the pattern to follow. If they don't validate either, this is a pre-existing gap — but new code should follow the documented standard.5. [API-CONSISTENCY] Constructor error pattern divergence — consider exception type
All other optional services in the
PlanLifecycleServiceconstructor (event_bus,decision_service,error_pattern_service,invariant_service,config_service,job_store) silently skip their functionality whenNone. The newcheckpoint_serviceraisesPlanErrorwhenNoneandrollback_planis called. While this is a reasonable backward-compatibility approach, it's a different error pattern.Consider using
RuntimeErrororTypeErrorinstead ofPlanErrorfor a missing dependency — this is a wiring/configuration error, not a domain error. At minimum, document this distinction in the constructor docstring (e.g., "Unlike other optional services,checkpoint_serviceraisesPlanErrorifrollback_planis called without it, because rollback cannot be silently skipped.").6. [CONTRIBUTING] Mock helpers should be in
features/mocks/The helper functions
_make_mock_checkpoint_service()and_make_mock_event_bus()infeatures/steps/plan_lifecycle_rollback_steps.pycreate mock objects with configured return values. Per CONTRIBUTING.md, mock implementations should reside infeatures/mocks/. Consider moving these helpers there for consistency with existing patterns (e.g.,features/mocks/mock_ai_provider.py,features/mocks/mock_devcontainer_cli.py).7. [TEST] Consider testing ERRORED and CONSTRAINED state acceptance
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. Consider adding scenarios for ERRORED and CONSTRAINED states, since these are the recoverable-terminal states that the PR explicitly allows and which distinguish
rollback_plan's validation fromcancel_plan'sis_terminalcheck. These are the most interesting edge cases for rollback and would strengthen the test suite.8. [CONTRIBUTING] Multi-level testing gap
Per CONTRIBUTING.md: "Every development task must include or update tests at the unit, integration, and performance benchmark levels." The PR only adds Behave unit tests. There are no Robot Framework integration tests or ASV benchmarks mentioned. For a backlog bug fix this may be acceptable, but it's worth noting for completeness. The existing CLI rollback integration tests (if any) may already cover the integration path.
PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The core design is sound and the architectural fix is correct — routing rollback through the service layer is the right approach. Three issues must be addressed before approval:
BusinessRuleViolationorResourceNotFoundErrorfromCheckpointService.Issues #1 and #2 are substantive; #3 is trivial. Once addressed, this PR should be ready for approval.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
PR #3911 Code Review —
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus the specification (docs/specification.md§Plan Rollback at line 15948, §CheckpointService Operations at line 19332, §A2A Extension Methods at line 23459) and the linked issue #3677.✅ What Looks Good
Correct delegation target: The PR correctly delegates to
CheckpointService.selective_rollback()per the spec (line 19340), notrollback_to_checkpoint()as the issue body originally suggested. Good spec alignment.Terminal state rejection scope: Only
APPLIEDandCANCELLEDare rejected.ERROREDandCONSTRAINEDare correctly allowed, matching the spec's distinction between permanently terminal states and recoverable states.Event emission pattern: The
PLAN_ROLLED_BACKevent emission follows the exact sametry/except+warning("event_bus_emit_failed")pattern used by all other lifecycle methods (execute_plan,apply_plan,cancel_plan). Consistent and correct.Constructor pattern: The
checkpoint_serviceoptional parameter with explicitPlanErroron use-when-None is well-documented and preserves backward compatibility. The docstring clearly explains the behavior.CLI metadata enrichment: The separation between read-only UI metadata (via
CheckpointService.get_checkpoint()) and state-mutating rollback (viaPlanLifecycleService.rollback_plan()) is a sound design decision. The enriched confirmation prompt with decision/child-plan counts is a nice UX improvement.Test coverage: 11 scenarios covering delegation, error cases (no checkpoint_service, terminal states, non-existent plan), multiple valid states, event emission, and event-bus-None safety. The
plr-prefix convention avoids step collisions. Well-structured BDD.Commit message: Follows Conventional Changelog format. Has
ISSUES CLOSED: #3677footer. Single atomic commit. ✓PR metadata: Has
Closes #3677,Type/Buglabel. ✓🔴 Required Changes
1. [SPEC] Docstring phase transition table is inaccurate
src/cleveragents/application/services/plan_lifecycle_service.py, line 21 (module docstring)ProcessingStatevalues (APPLIED,CANCELLED) — it does not restrict rollback to the Execute phase. The tests confirm this: scenario "rollback_plan accepts a plan in Strategize phase" passes fromStrategize/QUEUED. The "From" column is therefore wrong.Any non-terminal stateor!APPLIED, !CANCELLED.execute / queued, but this is one example, not a phase restriction. The implementation's permissive approach (any non-terminal state) is defensible, but the docstring must match reality.2. [SPEC] Missing plan state update after rollback — contradicts own docstring and spec
Location:
src/cleveragents/application/services/plan_lifecycle_service.py, lines 2097–2191Issue: The docstring table's "To" column says
Execute/QUEUED, and the spec example (line 15996–15998) shows:However, the
rollback_planmethod does not updateplan.phaseorplan.processing_stateat all. After rollback, a plan inExecute/PROCESSINGstays inExecute/PROCESSING; a plan inStrategize/QUEUEDstays inStrategize/QUEUED. The method only validates state, delegates toCheckpointService, emits an event, and returns the result.Every other lifecycle method in this service updates the plan's phase/state as part of the transition (e.g.,
execute_plansetsExecute/QUEUED,apply_plansetsApply/QUEUED,cancel_plansetsCANCELLED). The absence of a state update here is a significant deviation from both the documented API contract and the established pattern.Required: Either:
processing_state = ProcessingState.QUEUEDafter successful rollback, matching the docstring and spec), orOption (a) is recommended for spec compliance.
3. [TEST] No test verifying post-rollback plan state
features/plan_lifecycle_rollback.featurephaseorprocessing_stateafter a successful rollback. Given that the docstring claims the transition goes toExecute/QUEUED, there should be at least one scenario that asserts the plan's state was updated accordingly. This is a gap in behavioral coverage — the tests verify the rollback operation succeeded but not the plan lifecycle transition that the docstring promises.🟡 Non-Blocking Observations
4. [API] CLI wires checkpoint_service via runtime attribute assignment
src/cleveragents/cli/commands/plan.py, line 3606lifecycle_svc.checkpoint_service = svclifecycle_svcinstance elsewhere, the wiring is lost. Consider wiring this in the DI container's provider function in a future PR.5. [API] Return type breaks lifecycle method pattern
use_action,execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The PR description explains the rationale (CLI needs rollback details), which is reasonable. However, callers expecting the uniform-> Planpattern may be surprised. Consider documenting this deviation prominently, or returning a composite type that includes both the Plan and the RollbackResult in a future iteration.6. [PROCESS] No milestone assigned
Summary
The core implementation is sound — correct delegation, proper error handling patterns, good test coverage, and clean code. The three required changes are:
These are specification-compliance and correctness issues that should be addressed before merge.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
🔍 Code Review — REQUEST CHANGES
Reviewed PR #3911 with focus on specification-compliance, error-handling-patterns, and api-consistency.
The PR correctly identifies and addresses a real architectural gap: the CLI
agents plan rollbackcommand was bypassing the service layer. The overall approach — addingrollback_plantoPlanLifecycleService, routing the CLI through it, and emittingPLAN_ROLLED_BACKdomain events — is sound and well-motivated. However, I found two specification compliance issues and one error-handling concern that should be addressed before merge.Required Changes
1. [SPEC] Missing Plan State Transition After Rollback
src/cleveragents/application/services/plan_lifecycle_service.py—rollback_plan()method (around line 2097–2191)rollback_planmethod validates state and delegates toCheckpointService.selective_rollback(), but it never updates the plan'sphaseorprocessing_stateafter a successful rollback. The specification's example output foragents plan rollback(specification.md lines 15995–15998) explicitly shows: The plan should transition to aQUEUEDprocessing state after rollback to indicate it is awaiting further input. Without this, the plan remains in whatever state it was in before rollback (e.g.,PROCESSINGorCOMPLETE), which does not accurately reflect the post-rollback reality.selective_rollback()call and before event emission, update the plan's processing state (e.g.,plan.processing_state = ProcessingState.QUEUED) and persist the change if using the persisted mode. This aligns with how other lifecycle methods (execute_plan,apply_plan) update plan state as part of their transitions.2. [SPEC] Docstring Phase Transition Table Is Misleading
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring (line 21)rollback_planmethod docstring (line 2105) correctly says "any non-terminal plan state" but the table contradicts this.3. [ERROR-HANDLING] Missing Argument Validation
src/cleveragents/application/services/plan_lifecycle_service.py—rollback_plan()method (line 2097)rollback_planmethod does not validate thatplan_idandcheckpoint_idare non-empty strings before proceeding. Whileget_plan()will eventually raiseNotFoundErrorfor empty strings, explicit upfront validation provides clearer error messages and follows the established pattern.checkpoint_servicecheck:Suggestions (Non-blocking)
4. [API-CONSISTENCY] CLI Wiring Pattern
src/cleveragents/cli/commands/plan.py(line 3606)lifecycle_svc.checkpoint_service = svcsets a public attribute directly after construction, bypassing the constructor. While the comment explains this avoids a circular dependency, this is a fragile pattern that could lead to inconsistent state ifrollback_planis called before the attribute is set in other code paths.5. [TEST] Missing Error Propagation Test
features/plan_lifecycle_rollback.featureCheckpointService.selective_rollback()raises an exception (e.g.,BusinessRuleViolationfor a missing sandbox). The spec (line 19340) documents thatselective_rollbackcan raiseBusinessRuleViolationandResourceNotFoundError. A test verifying these propagate correctly throughrollback_planwould strengthen coverage.selective_rollbackraisesBusinessRuleViolationand verify it propagates to the caller.6. [TEST] Missing State Coverage
features/plan_lifecycle_rollback.featureGood Aspects
fix(plan-lifecycle): ...with properISSUES CLOSEDfooterplr-prefix to avoid step collisionsPlanErrorandNotFoundErrorconsistentlyPLAN_ROLLED_BACKevent with rich details (checkpoint_id, restored_files_count, changed_paths)checkpoint_serviceparameter preserves existing constructor signaturesDecision: REQUEST CHANGES 🔄
Issues #1 and #2 are specification compliance gaps that should be resolved before merge. Issue #3 is a project rule violation (fail-fast argument validation) that is straightforward to fix.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
PR #3911 Code Review —
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Reviewed against:
docs/specification.md(§Plan Rollback at line 15948, §CheckpointService Operations at line 19332, §CleverAgents Extension Methods at line 23459),CONTRIBUTING.md, linked issue #3677, and existingPlanLifecycleServicepatterns (execute_plan,apply_plan,cancel_plan).✅ What Looks Good
Correct delegation target: The PR correctly delegates to
CheckpointService.selective_rollback()per the spec (line 19340), notrollback_to_checkpoint()as the issue body originally suggested. Good spec alignment.Terminal state rejection scope: Only
APPLIEDandCANCELLEDare rejected.ERROREDandCONSTRAINEDare correctly allowed, matching the spec's distinction between permanently terminal states and recoverable states.PLAN_ROLLED_BACKevent type: Clean addition to theEventTypeenum following the existing<domain>.<action>naming convention (plan.rolled_back). Correctly placed in the Plan lifecycle section alongsidePLAN_APPLIED,PLAN_CANCELLED, etc.Optional
checkpoint_serviceconstructor parameter: Follows the established pattern of optional dependencies (event_bus,decision_service,invariant_service, etc.) that default toNone. Backward-compatible.Behave test structure: 11 well-organized scenarios with clear section headers (delegation, state validation, domain events, NotFoundError propagation). The
plr-step prefix avoids collisions with existing step definitions. Good use of descriptive scenario names.Commit message: Follows Conventional Changelog format. Single atomic commit.
ISSUES CLOSED: #3677footer present. Branch name matches issue metadata.PR metadata: Has
Closes #3677,Type/Buglabel,Priority/Backloglabel. Issue has no milestone (backlog), so no milestone on PR is consistent.🔴 Required Changes
1. [SPEC] Missing test for post-rollback plan state transition
features/plan_lifecycle_rollback.featureExecute (any state) → Execute/QUEUED. However, none of the 11 test scenarios verify that the plan'sprocessing_stateis updated toQUEUEDafter a successful rollback. The tests verify that rollback succeeds and returns aRollbackResult, but never assert the plan's post-rollback state.processing_state == ProcessingState.QUEUEDafter a successfulrollback_plancall. For example:2. [SPEC/API] Inconsistency between docstring table and implementation regarding allowed phases
Location:
src/cleveragents/application/services/plan_lifecycle_service.py(docstring table) andfeatures/plan_lifecycle_rollback.feature(scenario 7)Issue: The PR's docstring table documents the rollback transition as:
This states rollback is valid only from Execute phase. However, the implementation allows rollback from any non-terminal state, and scenario 7 explicitly tests rollback from
Strategize/QUEUEDstate. This is an internal contradiction within the PR.Furthermore, if rollback from Strategize is allowed, what should the post-rollback phase be? The spec (line 15996) only shows
Phase: executeas the post-rollback state. Rolling back from Strategize to Execute would be a phase regression, which may not be the intended behavior.Required: Resolve the inconsistency by either:
Reference: Spec line 15996 (Post-Rollback State), PR docstring table
🟡 Suggestions (Non-blocking)
3. [API] Return type deviates from lifecycle method pattern
src/cleveragents/application/services/plan_lifecycle_service.pyuse_action → Plan,execute_plan → Plan,apply_plan → Plan,cancel_plan → Plan) returnPlan. The newrollback_planreturnsRollbackResult, breaking the consistent API pattern. The spec's extension methods table (line 23459) groupsrollbackwith other plan lifecycle operations, suggesting it should follow the same conventions.Planfor API consistency. The CLI could obtain rollback details by havingrollback_planstore theRollbackResulton the plan object (e.g.,plan.last_rollback_result), or by returning a composite type. If the current return type is intentional, add a note in the docstring explaining whyrollback_plandeviates from the-> Planpattern used by all other lifecycle methods.4. [TEST] Missing test for CheckpointService exception propagation
features/plan_lifecycle_rollback.featureselective_rollbackraisesBusinessRuleViolationif recovery fails andResourceNotFoundErrorif the checkpoint does not exist. The tests verifyNotFoundErrorfor a non-existent plan, but do not verify exception propagation when the checkpoint does not exist or when the rollback operation itself fails.CheckpointService.selective_rollbackraisesResourceNotFoundError(bad checkpoint ID) andBusinessRuleViolation(recovery failure), and verify these propagate throughrollback_planper the fail-fast error handling principle.Summary
The PR correctly addresses the architectural gap identified in issue #3677 — routing rollback through the service layer with proper state validation and domain event emission. The implementation is well-structured and the test coverage is good. However, two specification-compliance issues must be resolved before merge:
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
🔍 Code Review — REQUEST CHANGES
Reviewed PR #3911 with focus on specification-compliance, error-handling-patterns, and api-consistency.
The PR addresses a legitimate architectural gap (issue #3677): the CLI
agents plan rollbackcommand was bypassingPlanLifecycleServiceand callingCheckpointService.selective_rollback()directly, skipping state validation and domain event emission. The overall approach — adding arollback_planservice method, wiring the CLI through it, and emittingPLAN_ROLLED_BACKevents — is sound and well-motivated.However, I found two issues that must be addressed before merge, plus several suggestions for improvement.
Required Changes
1. [SPEC/DOC] Phase transition table is inaccurate —
plan_lifecycle_service.pymodule docstringLocation:
src/cleveragents/application/services/plan_lifecycle_service.py, lines 18–22 (module docstring)Issue: The phase transition table claims:
This is inaccurate on both sides:
"From" column is wrong: The implementation allows rollback from any non-terminal state, not just Execute. The tests confirm this — scenario "rollback_plan accepts a plan in Strategize phase" passes with a
Strategize/QUEUEDplan. This is the correct behavior (rollback is a recovery mechanism), but the table says "Execute (any state)"."To" column is wrong: The implementation does not transition the plan to
Execute/QUEUED. Afterrollback_plancompletes, the plan'sphaseandprocessing_stateremain unchanged. The method validates state, delegates toCheckpointService, and emits an event — but performs no state mutation on the plan itself.Required: Update the table to accurately reflect the implementation. Either:
| ``rollback_plan(...)`` | Non-terminal (any) | *(unchanged)* |and add a footnote explaining rollback restores sandbox state without changing plan phase/state.The specification is the source of truth. If the spec intends rollback to trigger a state transition, the implementation must match. If not, the documentation must be accurate.
2. [API-CONSISTENCY] Should use explicit terminal-state guard with explanatory comment —
plan_lifecycle_service.py:~line 2138Location:
src/cleveragents/application/services/plan_lifecycle_service.py, around line 2138Issue: The method checks terminal states with:
Every other method in this service that guards against terminal states uses the
plan.is_terminalproperty (e.g.,cancel_planat line 2035, and at least 4 other locations). Theis_terminalproperty includesAPPLIED,CANCELLED,CONSTRAINED, andERRORED.I understand the design intent: ERRORED and CONSTRAINED plans should be rollback-able since rollback is a recovery mechanism. The
Plan.is_terminaldocstring itself says: "ERRORED and CONSTRAINED plans can still be reverted to an earlier phase via revert_plan or resumed through PlanResumeService." So the explicit check is correct behavior — but it looks like a bug to anyone reading the code because it deviates from the established pattern without explanation.Required: Add a code comment explaining why
is_terminalis deliberately not used here. For example:This makes the design decision self-documenting and prevents future maintainers from "fixing" it to use
is_terminal.Suggestions (Non-blocking)
3. [API-CONSISTENCY] CLI mutates service attribute post-construction —
plan.py:~line 3604The CLI does:
This bypasses constructor injection by mutating the service attribute directly after construction. The comment explains the circular-dependency rationale, which is fair. However, this creates temporal coupling — if
rollback_planis called before this wiring, it fails with a confusingPlanErrorinstead of a clear DI error. Consider whether the container could be updated to wire this dependency properly, or at minimum ensure this pattern is documented as a known limitation.4. [TEST] Missing scenario for
CheckpointServiceerror propagationThere is no test verifying that exceptions raised by
CheckpointService.selective_rollback()(e.g.,BusinessRuleViolationfor missing sandbox or wrong checkpoint) propagate correctly throughrollback_plan. The PR description's Raises section documents this propagation, but no test verifies it. Consider adding a scenario like:5. [TEST] Missing scenario for Action phase rollback
The tests cover Execute and Strategize phases but not the Action phase. Since Action-phase plans have no sandbox, a rollback attempt would likely fail at the
CheckpointServicelevel. A test confirming this behavior would strengthen coverage and document the expected interaction.Good Aspects
CheckpointServicePLAN_ROLLED_BACKevent type follows the established<domain>.<action>naming conventionRollbackResultreturn type is a good design decision — allows CLI to surface rollback details without reaching intoCheckpointServicecheckpoint_serviceconstructor parameter preserves backward compatibilityISSUES CLOSEDfooterplr-prefix to avoid step collisionsDecision: REQUEST CHANGES 🔄
The two required changes are straightforward: (1) fix the inaccurate phase transition table in the docstring, and (2) add an explanatory comment for the deliberate deviation from the
is_terminalpattern. Neither requires logic changes.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus cross-reference againstdocs/specification.md(§Plan Rollback, §CheckpointService Operations), CONTRIBUTING.md, and linked issue #3677.✅ What Looks Good
Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec (§CheckpointService Operations), notrollback_to_checkpoint()as the issue body originally suggested. Good spec alignment.Terminal state rejection: Only APPLIED and CANCELLED are rejected. ERRORED and CONSTRAINED are correctly permitted, matching the spec's distinction between permanently terminal and recoverable terminal states.
PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventType.Commit format: Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓PR metadata:
Type/Buglabel ✓,Closes #3677✓, branch name matches issue metadata ✓.Test structure: Feature file is well-organized with clear section headers. Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior, not just coverage padding.RollbackResultreturn type: Well-justified design decision — allows CLI to display rollback details without reaching back intoCheckpointService.🔄 Required Changes
1. [CONTRIBUTING] Mock helpers defined in step file instead of
features/mocks/Location:
features/steps/plan_lifecycle_rollback_steps.py—_make_mock_checkpoint_service()and_make_mock_event_bus()functionsIssue: Per CONTRIBUTING.md: "All mocking code, test doubles, and mock implementations belong exclusively in the
features/mocks/directory." The step file defines two mock factory functions (_make_mock_checkpoint_service()and_make_mock_event_bus()) inline. These are mock construction helpers that configureMagicMockinstances with specific return values and side effects — they are mocking code by definition.Required: Move
_make_mock_checkpoint_service()and_make_mock_event_bus()to a module infeatures/mocks/(e.g.,features/mocks/plan_lifecycle_rollback_mocks.py) and import them in the step file. The_create_service()helper can remain in the step file as it creates a realPlanLifecycleServiceinstance.Reference: CONTRIBUTING.md — Mocking and Test Doubles section.
2. [SPEC] Docstring phase transition table contradicts actual behavior
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableIssue: The updated docstring table shows:
But Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback." The docstring restricts the "From" column to Execute only, which contradicts both the test coverage and the stated design intent.
The spec at line 15996 shows post-rollback state as
Phase: execute, State: queued, which implies rollback is an Execute-phase operation. If Strategize-phase rollback is intentional (which the test suggests), the docstring must reflect this. If it's not intentional, the Strategize test scenario should be removed.Required: Either:
Any (non-terminal)in the "From" column, or3. [ERROR-HANDLING] Missing exception propagation tests for CheckpointService failures
Location:
features/plan_lifecycle_rollback.featureIssue: The spec defines two error cases for
selective_rollback:ResourceNotFoundErrorif the checkpoint does not existBusinessRuleViolationif recovery fails after a partial rollbackThere are no test scenarios verifying that exceptions raised by
CheckpointService.selective_rollback()propagate correctly throughrollback_plan. Per the project's fail-fast error handling principles (CONTRIBUTING.md), these exceptions must propagate without being caught or swallowed. Without tests, a future refactor could accidentally add atry/exceptthat swallows these critical errors.Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planunchangedCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure), it propagates throughrollback_planunchanged4. [DOC] Scenario count discrepancy in PR description
Location: PR description body
Issue: The PR description states "Added Behave feature file with 11 scenarios" but the feature file contains 10 scenarios. Counted:
Required: Correct the PR description to say 10 scenarios, or add the missing 11th scenario if one was intended but omitted. (Note: the two exception propagation scenarios from issue #3 above would bring the total to 12.)
💡 Non-Blocking Suggestions
5. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description, but consider whether returning a composite type (e.g., a dataclass containing both the updatedPlanand theRollbackResult) would maintain API consistency while still providing rollback details.6. [TEST] Missing ERRORED and CONSTRAINED state acceptance tests
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. Consider adding scenarios for ERRORED and CONSTRAINED states, since these are the key recoverable-terminal states that distinguish
rollback_plan's validation fromcancel_plan'sis_terminalcheck. These states are the most important to verify as "allowed" since they sit at the boundary of terminal/non-terminal.7. [ERROR-HANDLING]
PlanErrorfor missing dependency is semantically impreciseUsing
PlanErrorwhencheckpoint_service is Noneandrollback_planis called treats a configuration/wiring error as a domain error.RuntimeErroror a dedicatedConfigurationErrorwould be more semantically appropriate, since the issue is that the service was not properly wired, not that a plan operation failed. Other optional services (event_bus,decision_service) silently skip their functionality whenNone— this divergent pattern should at minimum be documented in the constructor docstring.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreDecision: REQUEST CHANGES 🔄
Issues #1 (mock location violation), #2 (docstring/spec inconsistency), #3 (missing error propagation tests), and #4 (scenario count) must be addressed before approval. Issues #1 and #3 are the most critical — #1 violates an explicit CONTRIBUTING.md rule, and #3 leaves a gap in error handling verification that could mask regressions.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, cross-referenced againstdocs/specification.md,CONTRIBUTING.md, and linked issue #3677.✅ What Looks Good
Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec, notrollback_to_checkpoint()as the issue body originally suggested. Good spec alignment.Terminal state rejection: Only
APPLIEDandCANCELLEDare rejected.ERROREDandCONSTRAINEDare correctly permitted, matching the spec's distinction between permanently terminal and recoverable terminal states.PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventType. Clean single-line addition.Commit format: Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓PR metadata:
Type/Buglabel ✓,Closes #3677✓, branch name matches issue metadata ✓.Test structure: Feature file is well-organized with clear section headers. Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior, not just coverage padding.RollbackResultreturn type: Well-justified design decision — allows CLI to display rollback details without reaching back intoCheckpointService.CLI refactoring: The CLI now correctly routes the state-mutating rollback through
PlanLifecycleService.rollback_plan()while keeping the read-only metadata enrichment (confirmation prompt) viaCheckpointService.get_checkpoint(). Good separation of concerns.Domain event payload: The
PLAN_ROLLED_BACKevent includescheckpoint_id,restored_files_count,changed_paths,plan_phase, andplan_state— comprehensive and useful for downstream consumers.🔄 Required Changes
1. [CONTRIBUTING] Mock helpers defined in step file instead of
features/mocks/Location:
features/steps/plan_lifecycle_rollback_steps.py—_make_mock_checkpoint_service()and_make_mock_event_bus()functionsIssue: Per CONTRIBUTING.md: "All mocking code, test doubles, and mock implementations belong exclusively in the
features/mocks/directory." The step file defines two mock factory functions that configureMagicMockinstances with specific return values and side effects — these are mocking code by definition.Required: Move
_make_mock_checkpoint_service()and_make_mock_event_bus()to a module infeatures/mocks/(e.g.,features/mocks/plan_lifecycle_rollback_mocks.py) and import them in the step file. The_create_service()and_create_plan_in_service()helpers can remain in the step file as they create real instances.Reference: CONTRIBUTING.md — Mocking and Test Doubles section.
2. [SPEC] Docstring phase transition table contradicts actual behavior
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring, Phase Transitions tableIssue: The updated docstring table shows:
But Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback." The implementation code only checks for
APPLIEDandCANCELLED— it does not restrict to Execute phase. The docstring's "From" column ofExecute (any state)is therefore incorrect.Required: Update the docstring table "From" column to accurately reflect the implementation, e.g.,
Any (non-terminal)orAny phase (non-APPLIED/CANCELLED).3. [ERROR-HANDLING] Missing exception propagation tests for CheckpointService failures
Location:
features/plan_lifecycle_rollback.featureIssue: The spec defines two error cases for
selective_rollback:ResourceNotFoundErrorif the checkpoint does not existBusinessRuleViolationif recovery fails after a partial rollbackThe
rollback_planmethod's own docstring explicitly documents thatBusinessRuleViolationpropagates fromCheckpointService.selective_rollback(). Yet there are no test scenarios verifying this propagation. Per the project's fail-fast error handling principles (CONTRIBUTING.md), these exceptions must propagate without being caught or swallowed. Without tests, a future refactor could accidentally add atry/exceptthat swallows these critical errors.Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError, it propagates throughrollback_planunchangedCheckpointService.selective_rollback()raisesBusinessRuleViolation, it propagates throughrollback_planunchanged4. [ERROR-HANDLING] Missing argument validation in
rollback_planLocation:
src/cleveragents/application/services/plan_lifecycle_service.py—rollback_plan()methodIssue: Per CONTRIBUTING.md: "All public and protected methods must validate their arguments as the first guard before any other logic. Checks should include value ranges, null checks, type verification, and checks for empty strings or collections."
The
rollback_plan(plan_id: str, checkpoint_id: str)method performs no argument validation before proceeding. Ifplan_idorcheckpoint_idare empty strings, the method will proceed toself.get_plan("")or pass an empty checkpoint ID toselective_rollback(), producing confusing downstream errors instead of failing fast with a clear message.Other lifecycle methods in this service (e.g.,
cancel_plan,execute_plan) follow the fail-fast pattern.rollback_planshould be consistent.Required: Add argument validation at the top of
rollback_plan:And add corresponding test scenarios for these validation cases.
💡 Non-Blocking Suggestions
5. [DOC] Scenario count discrepancy in PR description
The PR description states "Added Behave feature file with 11 scenarios" but the feature file contains 10 scenarios. The commit message also says 11. This should be corrected to 10 (or 12+ after adding the exception propagation scenarios from issue #3 above).
6. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description. Consider whether returning a composite type (e.g., a tuple or dataclass containing both the updatedPlanand theRollbackResult) would maintain API consistency while still providing rollback details. Non-blocking since the rationale is sound.7. [API-CONSISTENCY] Mutable service wiring in CLI
The CLI does
lifecycle_svc.checkpoint_service = svcto wire the checkpoint service at the call site. While the PR description explains this avoids circular dependencies at construction time, mutating a service object's dependencies after construction is a code smell that could lead to subtle bugs ifrollback_planis called before wiring. ThePlanErrorguard in the method mitigates this, but a factory method or explicit wiring step would be cleaner long-term.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreDecision: REQUEST CHANGES 🔄
Issues #1 (mock location violation), #2 (docstring/spec inconsistency), #3 (missing error propagation tests), and #4 (missing argument validation) must be addressed before approval. Issues #3 and #4 are the most critical — #3 leaves a gap in error handling verification, and #4 violates the project's explicit fail-fast argument validation rule from CONTRIBUTING.md.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus cross-reference againstdocs/specification.md(§Plan Rollback line 15948, §CheckpointService Operations line 19332), CONTRIBUTING.md, and linked issue #3677.✅ What Looks Good
PLAN_ROLLED_BACKevent type: Correctly follows the<domain>.<action>naming convention (plan.rolled_back), usesStrEnum, and is placed in the Plan lifecycle section ofEventType. Clean, minimal change. ✓Correct delegation to
selective_rollback(): The spec at line 19340 definesselective_rollback(plan_id, checkpoint_id)as the appropriate CheckpointService operation. The PR correctly delegates to this method rather thanrollback_to_checkpoint(). ✓Terminal state rejection logic: Only APPLIED and CANCELLED are rejected. ERRORED and CONSTRAINED are correctly permitted, matching the spec's distinction between permanently terminal states and recoverable states. This is a subtle but important design decision that aligns with the spec's intent that rollback is a recovery mechanism. ✓
Commit format: Single atomic commit following Conventional Changelog format (
fix(plan-lifecycle): ...), withISSUES CLOSED: #3677footer. ✓PR metadata:
Type/Buglabel ✓,Closes #3677✓, branch name matches issue metadata ✓.Test structure: Feature file uses clear section headers, step definitions use the
plr-prefix to avoid collisions, and scenarios test meaningful behavior (delegation, state validation, event emission, error propagation). ✓RollbackResultreturn type: Well-justified — allows CLI to display rollback details without reaching back into CheckpointService for post-rollback data. ✓🔄 Required Changes
1. [CONTRIBUTING] Mock factory functions defined in step file instead of
features/mocks/Location:
features/steps/plan_lifecycle_rollback_steps.py—_make_mock_checkpoint_service()and_make_mock_event_bus()Issue: CONTRIBUTING.md mandates: "All mocking code, test doubles, and mock implementations belong exclusively in the
features/mocks/directory." The step file defines two mock factory functions that construct and configureMagicMockinstances with specific return values and side effects. These are mock construction helpers — they configure test doubles — and therefore constitute mocking code.Required: Move
_make_mock_checkpoint_service()and_make_mock_event_bus()to a module infeatures/mocks/(e.g.,features/mocks/plan_lifecycle_rollback_mocks.py) and import them in the step file. The_create_service()and_create_plan_in_service()helpers can remain in the step file since they create realPlanLifecycleServiceinstances.Reference: CONTRIBUTING.md — Mocking and Test Doubles section.
2. [ERROR-HANDLING] Missing exception propagation tests for
CheckpointServicefailuresLocation:
features/plan_lifecycle_rollback.featureIssue: The spec at line 19340 explicitly defines two error cases for
selective_rollback:ResourceNotFoundErrorif the checkpoint does not existBusinessRuleViolationif recovery fails after a partial rollbackPer the project's fail-fast error handling principles (CONTRIBUTING.md), these exceptions must propagate through
rollback_planwithout being caught or swallowed. There are currently no test scenarios verifying this propagation behavior. Without tests, a future refactor could accidentally add atry/exceptthat swallows these critical errors — and the test suite would not catch the regression.Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planunchangedCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure), it propagates throughrollback_planunchangedThis is the most critical gap in the PR from an error-handling perspective.
3. [SPEC] Docstring phase transition table contradicts test coverage
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableIssue: The updated docstring table shows:
However, Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback." The docstring restricts the "From" column to Execute only, which contradicts both the test coverage and the stated design intent.
The spec's CLI example at line 15996 shows post-rollback state as
Phase: execute, State: queued, but this describes the output state (what the plan transitions TO after rollback), not a constraint on which phases can be rolled back FROM.Required: Update the docstring table's "From" column to
Any (non-terminal)to match the actual implementation and test coverage. If rollback is truly Execute-only per spec, remove the Strategize/QUEUED test scenario instead.4. [DOC] Scenario count discrepancy in PR description
Location: PR description body
Issue: The PR description states "Added Behave feature file with 11 scenarios" but the feature file contains 10 scenarios. I independently counted:
Required: Correct the PR description to say 10 scenarios, or add the missing 11th scenario if one was intended but omitted. (Note: the two exception propagation scenarios from issue #2 above would bring the total to 12.)
💡 Non-Blocking Suggestions
5. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description, but consider whether returning a composite type (e.g., a dataclass containing both the updatedPlanand theRollbackResult) would maintain API consistency while still providing rollback details. This is a design trade-off worth documenting in the method's docstring even if the current approach is kept.6. [TEST] Missing ERRORED and CONSTRAINED state acceptance tests
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. Consider adding scenarios for ERRORED and CONSTRAINED states, since these are the key recoverable-terminal states that distinguish
rollback_plan's validation fromcancel_plan'sis_terminalcheck. These states sit at the boundary of terminal/non-terminal and are the most important to verify as "allowed."7. [TEST] Broad exception catch in When steps
In
step_plr_call_rollback_plan()andstep_plr_call_rollback_nonexistent(), the except clause catches(PlanError, NotFoundError, Exception). The trailingExceptioncatch-all could mask unexpected errors during test execution, making failures harder to diagnose. Consider catching only the specific expected exception types and letting unexpected exceptions propagate as test failures.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The PR's core design is sound — routing rollback through the service layer with state validation and domain event emission is the correct architectural approach. The
PLAN_ROLLED_BACKevent type, terminal state rejection logic, and delegation toselective_rollback()all align with the specification.However, four issues must be addressed:
Issues #1 and #2 are the highest priority. #2 in particular addresses a real risk: without tests verifying that
ResourceNotFoundErrorandBusinessRuleViolationpropagate correctly, a future change could silently swallow these critical errors.Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus cross-reference againstdocs/specification.md(§CheckpointService Operations, §agents plan rollback), CONTRIBUTING.md, and linked issue #3677.✅ What Looks Good
Core service method is well-structured:
rollback_plan(lines 2097–2191) follows the established service-layer pattern — validates state, delegates to infrastructure, emits domain event, returns result. The method docstring is thorough with properArgs,Returns, andRaisessections.Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec (§CheckpointService Operations, line 19340), notrollback_to_checkpoint(). Good spec alignment.Terminal state rejection logic: Only
APPLIEDandCANCELLEDare rejected. The check usesProcessingStateenum values directly (lines 2134–2142), which is clean and explicit.PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventType. Event payload includescheckpoint_id,restored_files_count,changed_paths,plan_phase, andplan_state— comprehensive.CLI routing change: The CLI now correctly calls
lifecycle_svc.rollback_plan(plan_id, resolved_checkpoint_id)at line 3684 instead ofsvc.selective_rollback()directly. The comment at lines 3682–3683 explains the rationale and references issue #3677.Constructor backward compatibility:
checkpoint_serviceis optional (Nonedefault) with a clearPlanErrorwhenrollback_planis called without it. Constructor docstring (lines 232–236) documents this behavior.Event emission resilience: The
try/except Exceptionaroundevent_bus.emit(lines 2183–2189) correctly prevents event emission failures from blocking the rollback operation, with structured logging for diagnostics.Commit format: Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓Test structure: Feature file is well-organized with clear section headers and the
plr-prefix avoids step collisions. Scenarios test meaningful behavior — delegation verification, state rejection, event emission, and error propagation.🔄 Required Changes
1. [ERROR-HANDLING / API-CONSISTENCY] Missing
PlanErrorcatch in CLI rollback commandLocation:
src/cleveragents/cli/commands/plan.py— lines 3795–3806 (theexceptchain of the rollback command)Issue: Every other plan command in this file explicitly catches
PlanErrorbefore the genericCleverAgentsErrorcatch-all, with a context-specific error message:plan new"Plan Error:"plan use"Build Error:"plan cancel"Cannot cancel:"plan revert"Cannot revert:"plan diff"Diff Error:"plan artifacts"Artifacts Error:"plan resume"Cannot resume:"plan rollbackThe rollback command's
try/exceptblock (lines 3680–3806) catchesBusinessRuleViolation,ResourceNotFoundError,ValidationError, andCleverAgentsError— but NOTPlanError. SincePlanErrorinherits fromDomainError→CleverAgentsError, it falls through to the generic handler at line 3804, which displays"Error:"instead of a rollback-specific message.This matters because
rollback_planraisesPlanErrorin two cases:"Plan {plan_id} is in terminal state...""rollback_plan requires a CheckpointService..."Both should display a specific message like
"Cannot rollback:"to match the established CLI pattern.Required: Add
except PlanError as e:before theexcept CleverAgentsErrorline, with a message like:Reference: API consistency with all other plan commands in the same file.
2. [SPEC] Docstring phase transition table contradicts actual behavior
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring, line 21Issue: The phase transitions table shows:
But the implementation (lines 2134–2142) only rejects
APPLIEDandCANCELLEDstates — it does NOT restrict rollback to the Execute phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback."The spec's example output at line 15996 shows
Phase: execute, State: queuedas the post-rollback state, which suggests rollback is an Execute-phase operation. However, the implementation intentionally allows broader applicability.Required: Update the docstring table to accurately reflect the implementation:
Or, if the "To" column should reflect that rollback doesn't change the plan's phase/state (it only restores the sandbox), clarify that as well. The current table is misleading because it implies rollback is restricted to Execute phase.
💡 Non-Blocking Suggestions
3. [ERROR-HANDLING] Missing exception propagation tests for CheckpointService failures
Location:
features/plan_lifecycle_rollback.featureThe spec (line 19340) defines two error cases for
selective_rollback:ResourceNotFoundErrorif the checkpoint does not existBusinessRuleViolationif recovery fails after a partial rollbackSince
rollback_plandoes not catch these exceptions (correct — fail-fast), they should propagate unchanged. Adding test scenarios to verify this propagation would guard against future regressions where someone might accidentally add atry/exceptthat swallows them.4. [DOC] Scenario count discrepancy in PR description
The PR description states "11 scenarios" but the feature file contains 10 scenarios. Minor documentation error — consider correcting the PR body.
5. [TEST] Consider adding ERRORED and CONSTRAINED state acceptance tests
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. ERRORED and CONSTRAINED are the key recoverable-terminal states that sit at the boundary of the terminal/non-terminal distinction. Adding scenarios for these would strengthen confidence in the state validation logic.
6. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description, and I agree with the rationale. Just noting it for visibility — consider whether a composite return type (e.g., a tuple or dataclass containing bothPlanandRollbackResult) would serve both consistency and information needs.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The core implementation is solid — the service method, event type, constructor change, and test coverage are all well-done. The two required changes are:
Missing
PlanErrorcatch in CLI (issue #1) — a real error-handling gap that breaks the established pattern across all other plan commands. Users will see a generic "Error:" instead of "Cannot rollback:" when hitting terminal state rejection.Docstring table inaccuracy (issue #2) — the phase transition table claims Execute-only but the code allows any non-terminal state. This will mislead developers reading the docstring.
Both are straightforward fixes that shouldn't require significant rework.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus cross-reference againstdocs/specification.md(§CheckpointService Operations line 19340, §agents plan rollback line 15948, §CleverAgents Extension Methods line 23459),CONTRIBUTING.md(§File Organization line 1150, §BDD Framework line 1172).✅ What Looks Good
Correct architectural placement: The spec (line 23459) maps
rollbacktoPlanService, PlanLifecycle, CorrectionFlow. Addingrollback_plantoPlanLifecycleServiceis the correct service layer for this operation.Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec §CheckpointService Operations (line 19340), notrollback_to_checkpoint(). Good spec alignment.Terminal state rejection logic: Only
APPLIEDandCANCELLEDare rejected.ERROREDandCONSTRAINEDare correctly permitted as recoverable states. This matches the spec's distinction between permanently terminal and recoverable terminal states.PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED. Clean addition.Constructor backward compatibility:
checkpoint_serviceis optional (Nonedefault) with a clearPlanErrorwhenrollback_planis called without it. This preserves backward compatibility for all existing call sites.Commit format: Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓Test structure: Feature file is well-organized with clear section headers. Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior — delegation verification, state rejection, event emission, and error propagation.RollbackResultreturn type: Well-justified design decision — allows CLI to display rollback details without reaching back intoCheckpointService.🔄 Required Changes
1. [CONTRIBUTING] Mock helpers defined in step file instead of
features/mocks/Location:
features/steps/plan_lifecycle_rollback_steps.py—_make_mock_checkpoint_service()and_make_mock_event_bus()Issue: Per CONTRIBUTING.md line 1150: "Mocking code belongs under
/features/mocks/." Reiterated at line 1172: "Mocking code belongs underfeatures/mocks/." The step file defines two mock factory functions that configureMagicMockinstances with specific return values and side effects — these are mock construction helpers by definition.Required: Move
_make_mock_checkpoint_service()and_make_mock_event_bus()to a module infeatures/mocks/(e.g.,features/mocks/plan_lifecycle_rollback_mocks.py) and import them in the step file. The_create_service()and_create_plan_in_service()helpers can remain in the step file as they create realPlanLifecycleServiceandPlaninstances.Reference: CONTRIBUTING.md lines 1150, 1172.
2. [API-CONSISTENCY / ERROR-HANDLING] Missing
PlanErrorcatch in CLI rollback commandLocation:
src/cleveragents/cli/commands/plan.py— theexceptchain of the rollback commandIssue: Every other plan command in this file explicitly catches
PlanErrorbefore the genericCleverAgentsErrorcatch-all, with a context-specific error message:plan new"Plan Error:"plan use"Build Error:"plan cancel"Cannot cancel:"plan revert"Cannot revert:"plan diff"Diff Error:"plan artifacts"Artifacts Error:"plan resume"Cannot resume:"plan rollbackThe rollback command's
try/exceptblock catchesBusinessRuleViolation,ResourceNotFoundError,ValidationError, andCleverAgentsError— but NOTPlanError. SincePlanErrorinherits fromDomainError→CleverAgentsError, it falls through to the generic handler, which displays"Error:"instead of a rollback-specific message.This matters because
rollback_planraisesPlanErrorin two cases:"Plan {plan_id} is in terminal state...""rollback_plan requires a CheckpointService..."Both should display a specific message like
"Cannot rollback:"to match the established CLI pattern.Required: Add
except PlanError as e:before theexcept CleverAgentsErrorline:Reference: API consistency with all other plan commands in the same file.
3. [SPEC] Docstring phase transition table contradicts actual behavior
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableIssue: The updated docstring table shows:
But the implementation only rejects
APPLIEDandCANCELLEDstates — it does NOT restrict rollback to the Execute phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback."The spec's example output at line 15996 shows
Phase: execute, State: queuedas the post-rollback state, but this is an example, not a constraint. The implementation intentionally allows broader applicability.Required: Update the docstring table to accurately reflect the implementation:
The "To" column should also be corrected — rollback doesn't change the plan's phase/state (it only restores the sandbox), so
Execute/QUEUEDis misleading.4. [ERROR-HANDLING] Missing exception propagation tests for CheckpointService failures
Location:
features/plan_lifecycle_rollback.featureIssue: The spec (line 19340) defines two error cases for
selective_rollback:ResourceNotFoundErrorif the checkpoint does not existBusinessRuleViolationif recovery fails after a partial rollbackSince
rollback_planshould not catch these exceptions (per the project's fail-fast error handling principles in CONTRIBUTING.md), they must propagate unchanged. Without tests verifying this propagation, a future refactor could accidentally add atry/exceptthat swallows these critical errors.Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError, it propagates throughrollback_planunchangedCheckpointService.selective_rollback()raisesBusinessRuleViolation, it propagates throughrollback_planunchangedThese are important because the service method already has a
try/except Exceptionaround event emission. A developer reading that pattern might incorrectly extend it to wrap the delegation call too.💡 Non-Blocking Suggestions
5. [DOC] Scenario count discrepancy in PR description
The PR description states "11 scenarios" but the feature file contains 10 scenarios. Minor documentation error — correct the PR body. (Adding the two exception propagation scenarios from issue #4 would bring the total to 12.)
6. [TEST] Consider adding ERRORED and CONSTRAINED state acceptance tests
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. ERRORED and CONSTRAINED are the key recoverable-terminal states that sit at the boundary of the terminal/non-terminal distinction. Adding scenarios for these would strengthen confidence in the state validation logic, since they're the states most likely to be accidentally rejected in a future refactor.
7. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description. Consider whether a composite return type (e.g., a dataclass containing both the updatedPlanand theRollbackResult) would serve both consistency and information needs.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The core implementation is solid — the service method, event type, constructor change, and test coverage are all well-done. The four required changes are:
features/mocks/.PlanErrorcatch in CLI (issue #2) — breaks the established error-handling pattern across all other plan commands. Users will see generic "Error:" instead of "Cannot rollback:" when hitting terminal state rejection.try/exceptpattern around event emission in the same method.All four are straightforward fixes that shouldn't require significant rework.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus cross-reference againstdocs/specification.md(§CheckpointService Operations, §Plan Rollback, §CleverAgents Extension Methods),CONTRIBUTING.md(§Mocking and Test Doubles, §BDD Framework), and linked issue #3677.✅ What Looks Good
Correct architectural placement: The spec maps
rollbacktoPlanService, PlanLifecycle, CorrectionFlow. Addingrollback_plantoPlanLifecycleServiceis the correct service layer for this operation.Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec (§CheckpointService Operations), notrollback_to_checkpoint(). Good spec alignment.Terminal state rejection logic: Only
APPLIEDandCANCELLEDare rejected.ERROREDandCONSTRAINEDare correctly permitted as recoverable states, matching the spec's distinction between permanently terminal and recoverable terminal states.PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED. Event payload includescheckpoint_id,restored_files_count,changed_paths,plan_phase, andplan_state— comprehensive.Constructor backward compatibility:
checkpoint_serviceis optional (Nonedefault) with a clearPlanErrorwhenrollback_planis called without it. This preserves backward compatibility for all existing call sites.CLI routing change: The CLI now correctly calls
lifecycle_svc.rollback_plan(plan_id, resolved_checkpoint_id)instead ofsvc.selective_rollback()directly. The comment explains the rationale and references issue #3677.Commit format: Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓Test structure: Feature file is well-organized with clear section headers. Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior — delegation verification, state rejection, event emission, and error propagation.RollbackResultreturn type: Well-justified design decision — allows CLI to display rollback details without reaching back intoCheckpointService.🔄 Required Changes
1. [CONTRIBUTING] Mock helpers defined in step file instead of
features/mocks/Location:
features/steps/plan_lifecycle_rollback_steps.py—_make_mock_checkpoint_service()and_make_mock_event_bus()functionsIssue: Per CONTRIBUTING.md: "Mocking code belongs under
features/mocks/." The step file defines two mock factory functions that configureMagicMockinstances with specific return values and side effects — these are mock construction helpers by definition._make_mock_checkpoint_service()configures aMagicMockwith aselective_rollbackreturn value set to a specificRollbackResult._make_mock_event_bus()configures aMagicMockwith aside_effectonemitthat tracks emitted events. Both are test double construction code.Required: Move
_make_mock_checkpoint_service()and_make_mock_event_bus()to a module infeatures/mocks/(e.g.,features/mocks/plan_lifecycle_rollback_mocks.py) and import them in the step file. The_create_service()and_create_plan_in_service()helpers can remain in the step file as they create realPlanLifecycleServiceandPlaninstances.Reference: CONTRIBUTING.md — Mocking and Test Doubles section.
2. [API-CONSISTENCY / ERROR-HANDLING] Missing
PlanErrorcatch in CLI rollback commandLocation:
src/cleveragents/cli/commands/plan.py— theexceptchain of the rollback commandIssue: Every other plan command in this file explicitly catches
PlanErrorbefore the genericCleverAgentsErrorcatch-all, with a context-specific error message:plan new"Plan Error:"plan use"Build Error:"plan cancel"Cannot cancel:"plan revert"Cannot revert:"plan diff"Diff Error:"plan artifacts"Artifacts Error:"plan resume"Cannot resume:"plan rollbackThe rollback command's
try/exceptblock catchesBusinessRuleViolation,ResourceNotFoundError,ValidationError, andCleverAgentsError— but NOTPlanError. SincePlanErrorinherits fromDomainError→CleverAgentsError, it falls through to the generic handler, which displays"Error:"instead of a rollback-specific message.This matters because
rollback_planraisesPlanErrorin two cases:"Plan {plan_id} is in terminal state...""rollback_plan requires a CheckpointService..."Both should display a specific message like
"Cannot rollback:"to match the established CLI pattern.Required: Add
except PlanError as e:before theexcept CleverAgentsErrorline:Reference: API consistency with all other plan commands in the same file.
3. [SPEC] Docstring phase transition table contradicts actual behavior
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableIssue: The updated docstring table shows:
But the implementation only rejects
APPLIEDandCANCELLEDstates — it does NOT restrict rollback to the Execute phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback."The "To" column (
Execute/QUEUED) is also misleading — rollback doesn't change the plan's phase/state; it only restores the sandbox. The plan remains in whatever phase/state it was in before the rollback.Required: Update the docstring table to accurately reflect the implementation:
This aligns the documentation with both the implementation and the test coverage.
4. [ERROR-HANDLING] Missing exception propagation tests for CheckpointService failures
Location:
features/plan_lifecycle_rollback.featureIssue: The spec defines two error cases for
selective_rollback:ResourceNotFoundErrorif the checkpoint does not existBusinessRuleViolationif recovery fails after a partial rollbackSince
rollback_planshould not catch these exceptions (per the project's fail-fast error handling principles in CONTRIBUTING.md), they must propagate unchanged. Without tests verifying this propagation, a future refactor could accidentally add atry/exceptthat swallows these critical errors.This risk is elevated because the service method already contains a
try/except Exceptionaround event emission. A developer reading that pattern might incorrectly extend it to wrap the delegation call too.Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planunchangedCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure), it propagates throughrollback_planunchanged💡 Non-Blocking Suggestions
5. [DOC] Scenario count discrepancy in PR description
The PR description states "11 scenarios" but the feature file contains 10 scenarios. Minor documentation error — correct the PR body. (Adding the two exception propagation scenarios from issue #4 would bring the total to 12.)
6. [TEST] Consider adding ERRORED and CONSTRAINED state acceptance tests
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. ERRORED and CONSTRAINED are the key recoverable-terminal states that sit at the boundary of the terminal/non-terminal distinction. Adding scenarios for these would strengthen confidence in the state validation logic, since they're the states most likely to be accidentally rejected in a future refactor.
7. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description, and the rationale is sound. Just noting it for visibility — consider whether a composite return type (e.g., a dataclass containing both the updatedPlanand theRollbackResult) would serve both consistency and information needs.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The core implementation is solid — the service method, event type, constructor change, and test coverage are all well-done. The four required changes are:
features/mocks/.PlanErrorcatch in CLI (issue #2) — breaks the established error-handling pattern across all other plan commands. Users will see generic "Error:" instead of "Cannot rollback:" when hitting terminal state rejection.try/exceptpattern around event emission in the same method.All four are straightforward fixes that shouldn't require significant rework.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py. Cross-referenced againstdocs/specification.md(§CheckpointService Operations line 19340, §agents plan rollback line 15948, §Extension Methods line 43305),CONTRIBUTING.md(§File Organization line 1150, §Exception Propagation line 496, §Fail-Fast line 506), and linked issue #3677.✅ What Looks Good
Correct architectural placement: The spec (line 43305) explicitly maps
_cleveragents/plan/rollback→PlanLifecycle.rollback(). Addingrollback_plantoPlanLifecycleServiceis the correct service layer for this operation.Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec §CheckpointService Operations (line 19340), notrollback_to_checkpoint(). Good spec alignment.Terminal state rejection logic: Only
APPLIEDandCANCELLEDare rejected.ERROREDandCONSTRAINEDare correctly permitted as recoverable states, matching the spec's distinction between permanently terminal and recoverable terminal states.PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED. Clean addition.Constructor backward compatibility:
checkpoint_serviceis optional (Nonedefault) with a clearPlanErrorwhenrollback_planis called without it. This preserves backward compatibility for all existing call sites.Commit format: Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓Test structure: Feature file is well-organized with clear section headers. Step definitions use the
plr-prefix to avoid collisions. Scenarios test meaningful behavior — delegation verification, state rejection, event emission, and error propagation for non-existent plans.RollbackResultreturn type: Well-justified design decision — allows CLI to display rollback details without reaching back intoCheckpointService.PR metadata:
Type/Buglabel ✓,Closes #3677✓, branch name matches issue metadata ✓.🔄 Required Changes
1. [CONTRIBUTING] Mock helpers defined in step file instead of
features/mocks/Location:
features/steps/plan_lifecycle_rollback_steps.py—_make_mock_checkpoint_service()and_make_mock_event_bus()functionsIssue: CONTRIBUTING.md states this rule in three separate places:
/features/mocks/."features/mocks/."The step file defines two mock factory functions inline:
_make_mock_checkpoint_service()— configures aMagicMockwith aselective_rollbackreturn value set to a specificRollbackResult_make_mock_event_bus()— configures aMagicMockwith aside_effectonemitthat tracks emitted eventsBoth are mock construction helpers by definition — they configure
MagicMockinstances with specific return values and side effects.Required: Move
_make_mock_checkpoint_service()and_make_mock_event_bus()to a module infeatures/mocks/(e.g.,features/mocks/plan_lifecycle_rollback_mocks.py) and import them in the step file. The_create_service()and_create_plan_in_service()helpers can remain in the step file as they create realPlanLifecycleServiceandPlaninstances.Reference: CONTRIBUTING.md lines 555, 1150, 1172.
2. [API-CONSISTENCY / ERROR-HANDLING] Missing
PlanErrorcatch in CLI rollback commandLocation:
src/cleveragents/cli/commands/plan.py— theexceptchain of the rollback commandIssue: Every other plan command in this file explicitly catches
PlanErrorbefore the genericCleverAgentsErrorcatch-all, with a context-specific error message. I verified this pattern across the file:plan new"Plan Error:"plan use"Build Error:"plan cancel"Cannot cancel:"plan revert"Cannot revert:"plan diff"Diff Error:"plan artifacts"Artifacts Error:"plan resume"Cannot resume:"plan rollbackThe rollback command's
try/exceptblock catchesBusinessRuleViolation,ResourceNotFoundError,ValidationError, andCleverAgentsError— but NOTPlanError. SincePlanErrorinherits fromDomainError→CleverAgentsError, it falls through to the generic handler, which displays"Error:"instead of a rollback-specific message.This matters because
rollback_planraisesPlanErrorin two cases:"Plan {plan_id} is in terminal state...""rollback_plan requires a CheckpointService..."Both should display a specific message like
"Cannot rollback:"to match the established CLI pattern.Required: Add
except PlanError as e:before theexcept CleverAgentsErrorline:Reference: API consistency with all other plan commands in the same file.
3. [SPEC-COMPLIANCE] Docstring phase transition table contradicts actual behavior
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions table (around line 21)Issue: The updated docstring table shows:
But the implementation only rejects
APPLIEDandCANCELLEDstates — it does NOT restrict rollback to the Execute phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback."Additionally, the "To" column (
Execute/QUEUED) is misleading — rollback doesn't change the plan's phase/state; it only restores the sandbox. The plan remains in whatever phase/state it was in before the rollback.The spec's example output at line 15996 shows
Phase: execute, State: queuedas the post-rollback state, but this is an example of one specific scenario, not a constraint on which phases allow rollback.Required: Update the docstring table to accurately reflect the implementation:
This aligns the documentation with both the implementation and the test coverage.
4. [ERROR-HANDLING] Missing exception propagation tests for CheckpointService failures
Location:
features/plan_lifecycle_rollback.featureIssue: The spec (line 19340) explicitly defines two error cases for
selective_rollback:ResourceNotFoundErrorif the checkpoint does not existBusinessRuleViolationif recovery fails after a partial rollbackPer CONTRIBUTING.md (line 496): "CRITICAL: Do not suppress errors. Let exceptions propagate to top-level execution." And (line 530): "Let exceptions propagate; don't suppress them."
Since
rollback_planshould not catch these exceptions (correct — fail-fast), they must propagate unchanged through the service method. Without tests verifying this propagation, a future refactor could accidentally add atry/exceptthat swallows these critical errors.This risk is elevated because the service method already contains a
try/except Exceptionaround event emission. A developer reading that pattern might incorrectly extend it to wrap the delegation call too.Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planunchangedCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure), it propagates throughrollback_planunchanged💡 Non-Blocking Suggestions
5. [DOC] Scenario count discrepancy in PR description
The PR description states "11 scenarios" but the feature file contains 10 scenarios. Minor documentation error — correct the PR body. (Adding the two exception propagation scenarios from issue #4 would bring the total to 12.)
6. [TEST] Consider adding ERRORED and CONSTRAINED state acceptance tests
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. ERRORED and CONSTRAINED are the key recoverable-terminal states that sit at the boundary of the terminal/non-terminal distinction. Adding scenarios for these would strengthen confidence in the state validation logic, since they're the states most likely to be accidentally rejected in a future refactor.
7. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description, and the rationale is sound. Just noting it for visibility — consider whether a composite return type (e.g., a dataclass containing both the updatedPlanand theRollbackResult) would serve both consistency and information needs.PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The core implementation is solid — the service method, event type, constructor change, and test coverage are all well-done. The architectural placement is correct per spec, the delegation target is correct, and the terminal state logic is sound.
The four required changes are:
features/mocks/.PlanErrorcatch in CLI (issue #2) — breaks the established error-handling pattern across all other plan commands. Users will see generic "Error:" instead of "Cannot rollback:" when hitting terminal state rejection.try/exceptpattern around event emission in the same method.All four are straightforward fixes that shouldn't require significant rework.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — APPROVED ✅
Reviewed PR #3911 with focus on specification-compliance, error-handling-patterns, and api-consistency.
Examined all 6 changed files:
EventTypeenum addition,PlanLifecycleServiceconstructor + newrollback_planmethod, CLIplan rollbackcommand refactoring, Behave feature file (11 scenarios), and step definitions.Specification Compliance ✅
PlanLifecycleService.rollback_plan()instead of callingCheckpointService.selective_rollback()directly. This ensures plan state validation and domain event emission are not bypassed, which was the UAT gap identified in #3677.PLAN_ROLLED_BACK = "plan.rolled_back"follows the established<domain>.<action>naming convention and is placed correctly in the Plan lifecycle section of the enum.APPLIEDandCANCELLEDstates aligns with the spec's concept of terminal/immutable plan states.CheckpointService.selective_rollback(), maintaining the separation between plan lifecycle management and checkpoint/sandbox operations.Error Handling Patterns ✅
checkpoint_service is Nonecheck at method entry with clearPlanErrormessage is correct fail-fast behavior.self.get_plan(plan_id)naturally raisesNotFoundErrorfor missing plans — proper exception propagation without suppression.try/except Exceptionaround event emission (lines 2164-2189) correctly isolates non-critical side effects. Event emission failure is logged but does not abort the rollback operation. This follows the same pattern used by other methods in this service.BusinessRuleViolation→ResourceNotFoundError→ValidationError→CleverAgentsError) properly handles all exception types thatrollback_plancan raise.API Consistency ✅
rollback_plan(self, plan_id: str, checkpoint_id: str) -> RollbackResultis consistent with the service's existing API style (execute_plan,apply_plan,cancel_plan).RollbackResult(same type fromCheckpointService.selective_rollback()) is a good design choice — it avoids wrapping/unwrapping and lets the CLI display rollback details directly.checkpoint_serviceas optional withNonedefault preserves backward compatibility for all existing call sites.DomainEventemission includesplan_id,actor_name,project_name, and richdetailsdict — consistent with how other lifecycle events are emitted in this service.Test Quality ✅
plr-prefix avoids collisions with existing step definitions — good practice.Minor Suggestions (Non-blocking)
Docstring table inaccuracy — The module docstring (line 21) states
rollback_plantransitions fromExecute (any state)toExecute/QUEUED, but the implementation (a) accepts rollback from any non-terminal state including Strategize, and (b) does not actually change the plan's phase or processing state. Consider updating the table to accurately reflect the method's behavior, e.g.| rollback_plan(...) | Any non-terminal | (unchanged) |.CLI runtime wiring — Line 3606 in
plan.py(lifecycle_svc.checkpoint_service = svc) mutates the service instance after construction to avoid circular dependency. This is pragmatic but worth a brief comment explaining why it's done at the call site rather than at construction time. The existing comment is good but could note the shared-instance implications.PR metadata — The PR is missing a milestone assignment (
milestone: null). Per CONTRIBUTING.md, every PR must be assigned to the same milestone as its linked issue. This should be set before merge.Argument validation consistency — The
rollback_planmethod does not explicitly validateplan_id/checkpoint_idfor empty strings. This is consistent with other methods in this service (execute_plan,apply_plan, etc.) which also rely on downstream validation, but CONTRIBUTING.md calls for upfront argument validation in public methods. This is a pre-existing pattern gap, not introduced by this PR.Commit Quality ✅
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceISSUES CLOSED: #3677in footerDecision: APPROVED ✅
The implementation correctly addresses the UAT gap where the service layer was being bypassed during rollback operations. The code is well-structured, follows existing patterns, has comprehensive test coverage, and the design decisions (optional dependency, terminal state rejection, event emission) are sound.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py— cross-referenced againstdocs/specification.md(§Plan Rollback line 15948, §CheckpointService Operations line 19332, §A2A Extension Methods line 43305), CONTRIBUTING.md, and linked issue #3677.✅ What Looks Good
Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec §CheckpointService Operations (line 19340), notrollback_to_checkpoint()as the issue body originally suggested. This is the correct spec-aligned method.Terminal state rejection scope: Only APPLIED and CANCELLED are rejected. ERRORED and CONSTRAINED are correctly permitted as recoverable states. This matches the spec's distinction between permanently terminal and recoverable terminal states.
PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED. Consistent with existing event type patterns.RollbackResultreturn type: ReturningRollbackResultinstead ofNone(as the issue originally specified) is a well-justified improvement — it allows the CLI to surface rollback details without reaching back intoCheckpointService.Commit format: Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓PR metadata: Has
Type/Buglabel,Closes #3677in body, branch name matches issue metadata. Issue has no milestone (backlog), PR has no milestone — consistent. ✓Test structure: Feature file is well-organized with clear section headers (delegation, state validation, domain events, NotFoundError propagation). Step definitions use the
plr-prefix to avoid collisions with existing steps. Scenarios test meaningful behavior, not just coverage padding.Spec alignment for A2A mapping: The spec at line 43305 maps
_cleveragents/plan/rollbacktoPlanLifecycle.rollback(). This PR addsrollback_plan()toPlanLifecycleService, which correctly fills this gap.🔄 Required Changes
1. [SPEC] Docstring phase transition table contradicts actual behavior and tests
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableIssue: The updated docstring table shows:
But Scenario 7 (
rollback_plan accepts a plan in Strategize phase) explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback" (i.e., any non-permanently-terminal state). The docstring restricts the "From" column to Execute only, which contradicts both the test coverage and the stated design intent.Additionally, the spec at line 15996 shows post-rollback state as
Phase: execute, State: queued (awaiting input), which implies rollback is an Execute-phase operation. If Strategize-phase rollback is intentional (which seems reasonable as a recovery mechanism), the docstring should accurately reflect this. If it's unintentional, the Strategize test scenario should be removed.Required: Update the docstring table to accurately reflect the actual behavior:
Or if the intent is Execute-only, remove the Strategize test scenario and add state validation to enforce it.
2. [ERROR-HANDLING] Missing tests for CheckpointService exception propagation
Location:
features/plan_lifecycle_rollback.featureIssue: The spec at line 19340 explicitly defines two error cases for
selective_rollback:BusinessRuleViolationif recovery fails after a partial rollbackResourceNotFoundErrorif the checkpoint does not existThere are no test scenarios verifying that exceptions raised by
CheckpointService.selective_rollback()propagate correctly throughrollback_plan. Per the project's fail-fast error handling principles (CONTRIBUTING.md), these exceptions must propagate without being caught or swallowed. The service layer is a new intermediary — it's critical to verify it doesn't accidentally suppress checkpoint-level errors.Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planunchangedCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure after partial rollback), it propagates throughrollback_planunchangedThese can be implemented by configuring the mock
CheckpointServiceto raise the appropriate exceptions viaside_effect.3. [ERROR-HANDLING] Missing argument validation for plan_id and checkpoint_id
Location:
src/cleveragents/application/services/plan_lifecycle_service.py—rollback_planmethodIssue: Per CONTRIBUTING.md fail-fast principles: "All public and protected methods must validate their arguments as the first step of execution. This includes checks for nulls, incorrect types, empty strings/collections, and invalid value ranges."
The
rollback_plan(plan_id: str, checkpoint_id: str)method should validate that bothplan_idandcheckpoint_idare non-empty strings before proceeding. Other lifecycle methods in this service (e.g.,execute_plan,apply_plan) follow this pattern. There are also no test scenarios for empty/None argument handling.Required:
rollback_plan(e.g.,if not plan_id: raise ValidationError(...))4. [DOC] Scenario count discrepancy in PR description
Location: PR description body
Issue: The PR description states "Added Behave feature file
features/plan_lifecycle_rollback.featurewith 11 scenarios" but the feature file contains 10 scenarios:Required: Correct the PR description to say 10 scenarios, or add the missing 11th scenario if one was intended but omitted. (Note: the two exception propagation scenarios from issue #2 above would bring the total to 12.)
💡 Non-Blocking Suggestions
5. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description, but consider whether a richer return type (e.g., a dataclass containing both the updatedPlanand theRollbackResult) would maintain API consistency while still providing rollback details to the CLI. This would also allow callers to inspect the plan's post-rollback state without a separate lookup.6. [API-CONSISTENCY] Constructor error pattern diverges from other optional services
Other optional services (
event_bus,decision_service,error_pattern_service,invariant_service) silently skip their functionality whenNone. The newcheckpoint_serviceraisesPlanErrorwhenNoneandrollback_planis called. While this is a reasonable backward-compatibility approach, it's a different error pattern. Consider:checkpoint_serviceparam docs should explain the raise-on-use behavior)RuntimeErroror a more specific configuration error instead ofPlanError, since a missing dependency is a wiring/configuration error, not a domain error7. [TEST] Consider testing ERRORED and CONSTRAINED state acceptance
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. Consider adding scenarios for ERRORED and CONSTRAINED states, since these are the recoverable-terminal states that the PR explicitly allows and which distinguish
rollback_plan's validation from a simpleis_terminalcheck.Summary
Closes #3677)Type/labelType/BugDecision: REQUEST CHANGES 🔄
Issues #1 (docstring/behavior inconsistency), #2 (missing exception propagation tests), and #3 (missing argument validation) must be addressed before approval. Issue #4 (scenario count) is a minor documentation fix that should be corrected alongside the other changes.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911 — REQUEST_CHANGES
Reviewer: ca-pr-self-reviewer (independent review agent)
Review type: REQUEST_CHANGES (posted as COMMENT due to Forgejo self-review restriction)
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus cross-reference againstdocs/specification.md(§Plan Rollback at line 15948, §CheckpointService Operations at line 19340, §A2A Extension Methods at line 43305, §EventType enum at line 45845), CONTRIBUTING.md, and linked issue #3677.✅ What Looks Good
Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec §CheckpointService Operations (line 19340), which specifies atomic all-or-nothing semantics. The issue body originally suggestedrollback_to_checkpoint()but the implementation correctly chose the spec-aligned method.Terminal state rejection is well-reasoned: Only APPLIED and CANCELLED are rejected. The domain model's own
is_terminalproperty (plan.pyline 927-934) explicitly documents that "ERRORED and CONSTRAINED plans can still be reverted... APPLIED and CANCELLED are permanently terminal." Therollback_planmethod correctly uses this distinction rather than blindly usingplan.is_terminal.PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventType. While not yet in the spec's EventType enum, it follows the established pattern and is a reasonable extension.Commit format: Conventional Changelog format
fix(plan-lifecycle): ...withISSUES CLOSED: #3677footer. Single atomic commit. ✓CLI refactoring: The CLI correctly separates read-only UI concerns (checkpoint metadata for confirmation prompt via
CheckpointService.get_checkpoint()) from the state-mutating operation (routed throughPlanLifecycleService.rollback_plan()). Good architectural separation.Constructor pattern:
checkpoint_serviceas optional parameter withNonedefault is consistent with other optional services (decision_service,event_bus,job_store, etc.). Fail-fastPlanErrorwhen callingrollback_planwithout it configured.Test coverage: 11 BDD scenarios covering delegation, missing service, terminal state rejection (APPLIED + CANCELLED), valid state acceptance (Execute/PROCESSING, Execute/QUEUED, Strategize/QUEUED), event emission, no-event-bus path, and NotFoundError propagation. Well-structured with
plr-prefix to avoid step collisions.Error handling: Follows fail-fast pattern — validates
checkpoint_service is Nonefirst, thenget_plan()raisesNotFoundError, then terminal state check. Event emission failure is caught and logged (acceptable for non-critical side effects, consistent withcancel_planpattern at line 2084).🔄 Required Changes
1. [SPEC/DOC] Docstring phase transition table is inaccurate
Location:
plan_lifecycle_service.pymodule docstring, line ~21 of the branch fileIssue: The docstring table states:
This has two problems:
(a) The "From" column says "Execute (any state)" but the code actually allows rollback from any non-permanently-terminal state, including Strategize phase. The feature file explicitly tests
Strategize/QUEUEDacceptance. The docstring should reflect the actual behavior.(b) The "To" column says "Execute/QUEUED" but the
rollback_planmethod does not updateplan.phaseorplan.processing_state. Afterrollback_planreturns, the plan remains in whatever phase/state it was in before the call. The spec example (line 15995-15998) shows post-rollback state as "Phase: execute, State: queued (awaiting input)", suggesting the plan state should transition.Required: Either:
| rollback_plan(...) | Any non-terminal¹ state | (unchanged) |with a footnote that "non-terminal" means not APPLIED or CANCELLED.plan.processing_state = ProcessingState.QUEUEDafter successful rollback and commit the plan. This would require additional test scenarios.Why this matters: Future developers (and the A2A facade layer) will rely on this docstring to understand the method's contract. A documented state transition that doesn't happen is a correctness hazard.
2. [TEST] Missing error propagation test for
BusinessRuleViolationLocation:
features/plan_lifecycle_rollback.featureIssue: The
rollback_plandocstring (line 2121-2124) documents thatBusinessRuleViolationis propagated fromCheckpointService.selective_rollback()"when the sandbox is missing or the checkpoint does not belong to the plan." There is no test scenario verifying this propagation.Required: Add a scenario where the mock
CheckpointService.selective_rollback()raisesBusinessRuleViolationand verify it propagates throughrollback_planwithout being caught. This is important for the fail-fast error handling pattern — we need to verify the service layer doesn't accidentally swallow downstream exceptions.Example scenario:
💡 Non-Blocking Suggestions
API consistency — consider using
plan.is_terminalguard with refinement: Thecancel_planmethod (line 2035) usesplan.is_terminalfor its guard, whilerollback_planmanually checksProcessingState.APPLIEDandProcessingState.CANCELLED. The manual check is actually more correct for rollback (since ERRORED/CONSTRAINED should be rollbackable), but consider adding a brief inline comment explaining whyis_terminalis intentionally NOT used here, to prevent future "cleanup" PRs from "fixing" it.CLI wiring pattern: Line 3606 does
lifecycle_svc.checkpoint_service = svcto wire the dependency after construction. The comment explains this avoids circular dependency. Consider whether the DI container should handle this wiring instead, as a follow-up improvement.Spec update follow-up:
PLAN_ROLLED_BACKis not in the specification'sEventTypeenum (lines 45846-45853). Consider filing a follow-up issue to add it to the spec, maintaining spec-as-source-of-truth.PR metadata: No milestone assigned (issue is backlog). This is acceptable per the issue's backlog status, but note that CONTRIBUTING.md requires milestone assignment for PRs. Since the issue itself has no milestone, this is a grey area.
Summary
The core implementation is solid — correct delegation target, well-reasoned terminal state handling, proper error propagation, good test coverage, and clean architectural separation in the CLI. The two required changes are:
BusinessRuleViolationpropagationBoth are straightforward fixes that shouldn't require significant rework.
Decision: REQUEST_CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, error-handling-patterns, api-consistency
Files reviewed: All 6 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus cross-reference againstdocs/specification.md(§CheckpointService Operations line 19340, §agents plan rollback line 15948, §Plan lifecycle extensions line 23459),CONTRIBUTING.md.✅ What Looks Good
Correct architectural placement: The spec (line 23459) maps
rollbacktoPlanService, PlanLifecycle, CorrectionFlow. Addingrollback_plantoPlanLifecycleServiceis the correct service layer.Correct delegation target: Delegates to
CheckpointService.selective_rollback()per spec §CheckpointService Operations (line 19340). Good spec alignment.Terminal state rejection: Only
APPLIEDandCANCELLEDare rejected.ERROREDandCONSTRAINEDare correctly permitted as recoverable states, matching the spec's distinction between permanently terminal and recoverable terminal states.PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventTypebetweenPLAN_CANCELLEDandPLAN_ERRORED.Constructor backward compatibility:
checkpoint_serviceis optional (Nonedefault) with a clearPlanErrorwhenrollback_planis called without it. Consistent with the existing pattern where all dependencies are optional.Commit format: Conventional Changelog format (
fix(plan-lifecycle): ...), single atomic commit,ISSUES CLOSED: #3677in footer. ✓Test structure: Feature file is well-organized with clear section headers and the
plr-prefix avoids step collisions. Scenarios test meaningful behavior — delegation verification, state rejection, event emission, and error propagation.Event emission resilience: The
try/except Exceptionaroundevent_bus.emitcorrectly prevents event emission failures from blocking the rollback operation, with structured logging for diagnostics. Consistent with the pattern used bycancel_plan,apply_plan, etc.🔄 Required Changes
1. [CONTRIBUTING] Mock helpers defined in step file instead of
features/mocks/Location:
features/steps/plan_lifecycle_rollback_steps.py—_make_mock_checkpoint_service()(line ~38) and_make_mock_event_bus()(line ~52)Issue: Per CONTRIBUTING.md: "All mocking code, test doubles, and mock implementations belong exclusively in the
features/mocks/directory." The step file defines two mock factory functions that configureMagicMockinstances with specific return values and side effects — these are mock construction helpers by definition.Required: Move
_make_mock_checkpoint_service()and_make_mock_event_bus()to a module infeatures/mocks/(e.g.,features/mocks/plan_lifecycle_rollback_mocks.py) and import them in the step file. The_create_service()and_create_plan_in_service()helpers can remain in the step file as they create real instances.Reference: CONTRIBUTING.md — Mocking and Test Doubles section.
2. [API-CONSISTENCY] Missing
PlanErrorcatch in CLI rollback commandLocation:
src/cleveragents/cli/commands/plan.py— theexceptchain of the rollback commandIssue: Every other plan command in this file explicitly catches
PlanErrorbefore the genericCleverAgentsErrorcatch-all, with a context-specific error message (e.g.,cancel_plan→"Cannot cancel:",revert→"Cannot revert:"). The rollback command'stry/exceptblock catchesBusinessRuleViolation,ResourceNotFoundError,ValidationError, andCleverAgentsError— but NOTPlanError.Since
PlanErrorinherits fromDomainError→CleverAgentsError, it falls through to the generic handler, which displays"Error:"instead of a rollback-specific message. This matters becauserollback_planraisesPlanErrorin two cases: terminal state rejection and missing checkpoint_service.Required: Add
except PlanError as e:before theexcept CleverAgentsErrorline, with a message like"Cannot rollback:"to match the established CLI pattern.3. [SPEC] Docstring phase transition table contradicts actual behavior
Location:
src/cleveragents/application/services/plan_lifecycle_service.py— module docstring Phase Transitions tableIssue: The updated docstring table shows:
But the implementation only rejects
APPLIEDandCANCELLEDstates — it does NOT restrict rollback to the Execute phase. Scenario 7 in the feature file explicitly tests rollback from Strategize/QUEUED state, and the PR description states "all other states are considered valid for rollback."Additionally, the "To" column (
Execute/QUEUED) is misleading if the method does not actually transition the plan's phase/state after rollback. The spec's example output (line 15996) showsPhase: execute, State: queued (awaiting input)as the post-rollback state, which implies the plan state SHOULD be updated. If the implementation doesn't update the plan state, this is a spec deviation that needs to be either fixed in the code or documented as intentional.Required: Update the docstring table to accurately reflect the implementation. If rollback accepts any non-terminal state and does not change the plan's phase/state, the table should read:
4. [ERROR-HANDLING] Missing fail-fast argument validation
Location:
src/cleveragents/application/services/plan_lifecycle_service.py—rollback_planmethodIssue: Per CONTRIBUTING.md, the project requires fail-fast argument validation: "Code must be designed to fail immediately when something is wrong. This includes immediate validation of arguments in all public and protected methods (checking for nulls, invalid ranges, empty collections, etc.)."
The
rollback_plan(plan_id, checkpoint_id)method does not validate its string arguments before proceeding. Ifplan_idorcheckpoint_idis an empty string orNone, the method will either produce a confusingNotFoundErrordownstream (forplan_id) or pass an invalidcheckpoint_idtoCheckpointService.selective_rollback().Other service methods like
get_planrely on the repository to raiseNotFoundError, butcheckpoint_idhas no such guard — it's passed directly to the checkpoint service without validation.Required: Add argument validation at the top of
rollback_plan:This is consistent with the fail-fast principle and prevents confusing downstream errors.
5. [ERROR-HANDLING] Missing exception propagation tests for CheckpointService failures
Location:
features/plan_lifecycle_rollback.featureIssue: The spec (line 19340) defines two error cases for
selective_rollback:ResourceNotFoundErrorif the checkpoint does not existBusinessRuleViolationif recovery fails after a partial rollbackSince
rollback_planshould not catch these exceptions (per the project's fail-fast error handling principles), they must propagate unchanged. Without tests verifying this propagation, a future refactor could accidentally add atry/exceptthat swallows these critical errors — especially given the existingtry/except Exceptionpattern around event emission in the same method.Required: Add at least two scenarios:
CheckpointService.selective_rollback()raisesResourceNotFoundError(checkpoint not found), it propagates throughrollback_planunchangedCheckpointService.selective_rollback()raisesBusinessRuleViolation(recovery failure), it propagates throughrollback_planunchanged💡 Non-Blocking Suggestions
6. [TEST] Missing post-rollback plan state verification
The spec's example output (line 15996) shows
Phase: execute, State: queued (awaiting input)as the post-rollback state. No test scenario verifies that the plan's phase/state is correct after a successful rollback. Consider adding aThenstep that asserts the plan's phase and processing_state after rollback completes.7. [TEST] Consider adding ERRORED and CONSTRAINED state acceptance tests
The tests cover Execute/PROCESSING, Execute/QUEUED, and Strategize/QUEUED as valid rollback states. ERRORED and CONSTRAINED are the key recoverable-terminal states that sit at the boundary of the terminal/non-terminal distinction. Adding scenarios for these would strengthen confidence in the state validation logic.
8. [API-CONSISTENCY] Return type diverges from lifecycle method pattern
All other lifecycle methods (
execute_plan,apply_plan,cancel_plan) returnPlan.rollback_planreturnsRollbackResult. The design decision is documented and justified in the PR description. Consider whether a composite return type (e.g., a dataclass containing both the updatedPlanand theRollbackResult) would serve both consistency and information needs.9. [DOC] Scenario count discrepancy in PR description
The PR description states "11 scenarios" but the feature file contains 10 scenarios. Minor documentation error — correct the PR body.
PR Metadata Check
Closes #3677)Type/labelType/Bug# type: ignoreSummary
The core implementation is architecturally sound — the service method correctly wraps
CheckpointService.selective_rollback()with state validation and domain event emission, thePLAN_ROLLED_BACKevent type follows established conventions, and the constructor change preserves backward compatibility.Five required changes must be addressed:
features/mocks/.PlanErrorcatch in CLI (#2) — breaks the established error-handling pattern across all other plan commands.plan_idandcheckpoint_idare not validated, violating the project's fail-fast principle.ResourceNotFoundErrorandBusinessRuleViolationfromCheckpointService.All five are straightforward fixes that shouldn't require significant rework.
Decision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer
ff4e5ad021292938cb1b292938cb1ba321fb3b37Addressing review feedback
PlanLifecycleServicephase transition table to document rollback from any non-terminal state and clarify that rollback leaves the plan’s lifecycle metadata unchanged.CheckpointServiceerror propagation (bothResourceNotFoundErrorandBusinessRuleViolation), including the supporting step definitions.Unit test verification:
nox -e unit_tests -- features/plan_lifecycle_rollback.featureAutomated by CleverAgents Bot
Supervisor: Implementation | Agent: implementation-worker
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: APPROVED
Review focus areas: error-handling-patterns, edge-cases, boundary-conditions
Review reason: stale-review (prior review was COMMENT only — providing formal decision)
Files reviewed: All 5 changed files —
types.py,plan_lifecycle_service.py,plan.py(CLI),plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.py, plus cross-reference againstdocs/specification.md(§Plan Rollback line 16040, §CheckpointService Operations line 19433), CONTRIBUTING.md, and linked issue #3677.✅ CONTRIBUTING.md Compliance
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceCloses #3677Type/Bugfeatures/(Behave BDD)plan_lifecycle_rollback.feature# type: ignoreplan_lifecycle_rollback_steps.py(432 lines)✅ Specification Alignment
CheckpointService.selective_rollback()per spec §CheckpointService Operations (line 19441), notrollback_to_checkpoint()as the issue body originally suggested.APPLIEDandCANCELLEDare rejected —ERROREDandCONSTRAINEDare correctly permitted. This aligns with the spec'sis_terminaldocstring (plan.py line 926-934): "ERRORED and CONSTRAINED plans can still be reverted to an earlier phase." Rollback is a recovery mechanism, so blocking it for recoverable terminal states would be incorrect.PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventType.PlanLifecycleService.rollback_plan()while keeping the read-only metadata fetch (get_checkpoint()) direct — correct separation of concerns.✅ Error Handling Patterns (FOCUS AREA — Deep Dive)
This was a primary focus area. The error handling is well-implemented:
Fail-fast validation chain: The method validates in correct order:
checkpoint_service is None→PlanError(configuration error, checked first)get_plan(plan_id)→NotFoundError(plan existence, checked second)PlanError(business rule, checked third)selective_rollback()— no wasted work on invalid inputsException propagation:
ResourceNotFoundErrorandBusinessRuleViolationfromCheckpointService.selective_rollback()propagate naturally without being caught or wrapped. This is correct — the service layer should not mask domain-specific errors from the checkpoint layer.Event bus failure isolation: Event emission is wrapped in
try/except Exceptionwith a warning log, matching the exact pattern used bycancel_plan(line 2074). Event bus failures do not prevent the rollback result from being returned to the caller. This is the right trade-off — a successful rollback should not be reported as failed just because event emission had an issue.No event on failure: When
selective_rollback()raises, noPLAN_ROLLED_BACKevent is emitted. This is correct — events should only reflect successful state changes. Consistent withcancel_planwhich only emitsPLAN_CANCELLEDafter successful state mutation.Intentional divergence from
cancel_plan'sis_terminalcheck:cancel_planusesplan.is_terminal(which includes ERRORED/CONSTRAINED), whilerollback_planexplicitly checks onlyAPPLIED/CANCELLED. This is a deliberate and correct design decision — rollback is a recovery mechanism that should work on ERRORED/CONSTRAINED plans.✅ Edge Cases and Boundary Conditions (FOCUS AREA — Deep Dive)
plan.project_linksis None: Handled with(plan.project_links or [])— noAttributeErrorrisk. ✅project_names[0] if project_names else None— noIndexErrorrisk. ✅get_plan()raisingNotFoundError. Test scenario confirms. ✅ResourceNotFoundErrorpropagation tested. ✅BusinessRuleViolationpropagation tested. ✅if self.event_bus is not None. Test scenario confirms. ✅PlanErrorwith clear message. Test scenario confirms. ✅✅ Test Quality
_PLAN_ID,_CHECKPOINT_ID). No time dependencies, no randomness, no external calls, no file system contamination.plr-prefix avoids collisions with existing step definitions. Good practice.ℹ️ Informational Notes (Non-blocking)
TDD test gap: CONTRIBUTING.md (line 1223-1224) states that a bug fix PR closing issue
#Nwhere no@tdd_issue_Ntest exists is blocked by the CI quality gate. No@tdd_issue_3677test was found in the codebase. This is a CI enforcement concern — if the gate is active, it will catch this. The code itself is correct regardless.File size:
plan_lifecycle_service.py(2,649 lines) andplan.py(4,121 lines) exceed the 500-line limit, but these are pre-existing violations not introduced by this PR. The PR adds ~105 and ~9 lines respectively.CLI wiring pattern: The CLI wires
lifecycle_svc.checkpoint_service = svcvia direct attribute assignment at the call site to avoid circular dependency at construction time. This is documented in the code comments and is a pragmatic solution, though a factory or provider pattern might be cleaner long-term.Test accesses private attribute:
service._plans[plan_id] = planin the step helper_create_plan_in_service()accesses a private attribute to force plan state. This is a common test pattern to avoid running the full actor pipeline in unit tests, and is acceptable given the alternative complexity.Decision: APPROVED ✅
The implementation is solid, well-documented, and follows established patterns. Error handling is comprehensive with proper fail-fast validation, correct exception propagation, and graceful degradation for optional dependencies. Edge cases are thoroughly covered in both code and tests. The terminal state rejection scope is a thoughtful design decision that correctly distinguishes between permanently terminal (APPLIED/CANCELLED) and recoverable terminal (ERRORED/CONSTRAINED) states.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: specification-compliance, requirements-coverage, behavior-correctness
Files reviewed:
plan_lifecycle_service.py,plan.py(CLI),types.py,plan_lifecycle_rollback.feature,plan_lifecycle_rollback_steps.pyReference:
docs/specification.md§agents plan rollback (line 16040), §CheckpointService Operations (line 19433), CONTRIBUTING.md, linked issue #3677✅ What Looks Good
Correct delegation target:
CheckpointService.selective_rollback()is the right method per spec (§CheckpointService Operations, line 19441). ✅Terminal state rejection scope: Only
APPLIEDandCANCELLEDare rejected.ERROREDandCONSTRAINEDare correctly permitted as recoverable states. ✅PLAN_ROLLED_BACKevent type: Follows theplan.<action>naming convention (plan.rolled_back), usesStrEnum, placed correctly in the Plan lifecycle section ofEventType. ✅Commit message format: Conventional Changelog format is correct. ✅
Closing keyword:
Closes #3677is present in the PR description. ✅No
# type: ignore: None found in the new code. ✅Service method implementation: Clean, well-documented, with proper error propagation from
CheckpointService. ✅Behave test coverage: 11 scenarios covering state validation, event emission, delegation, and exception propagation. Good breadth. ✅
❌ Required Changes
1. [CRITICAL] Missing PR Metadata — No Milestone, No Type/ Label
CONTRIBUTING.md §11 (line 283–285):
CONTRIBUTING.md §12 (line 286–288):
This PR has no milestone and no labels at all. Issue #3677 is a bug report — this PR should carry
Type/Bugand be assigned to the same milestone as #3677.Required:
Type/Bug2. [CRITICAL] TDD Workflow Not Followed — Missing
@tdd_issue_3677TestsCONTRIBUTING.md §Bug Fix Workflow (lines 1092–1111):
Searching the entire
features/directory fortdd_issue_3677returns zero results. The TDD step was skipped entirely.The new scenarios in
plan_lifecycle_rollback.featureare tagged@phase2 @plan_lifecycle @rollbackbut have no@tdd_issue,@tdd_issue_3677, or@tdd_expected_failtags.Required:
tdd/branch must be created with a test tagged@tdd_issue @tdd_issue_3677 @tdd_expected_failthat proves the bug exists (i.e., the test assertsrollback_planexists and routes through the service layer, and it fails because the method is missing)masterfirst@tdd_expected_failfrom those tests (leaving@tdd_issueand@tdd_issue_3677permanently)3. [CRITICAL] Inline Imports Inside CLI Function Body
CONTRIBUTING.md §Import Guidelines (lines 1379–1382):
The
rollback_planCLI function (lines 3579–3584 and 3632 ofplan.py) contains inline imports:Note:
plan.pyhas a pre-existing pattern of inline imports throughout the file. However, this PR adds new inline imports that violate the rule. These must be moved to the top of the file.Required: Move all new imports introduced by this PR to the top-level import section of
plan.py.4. [IMPORTANT] JSON Output Format Does Not Match Spec
Spec §agents plan rollback (lines 16141–16165) defines the JSON output envelope as:
The PR produces (lines 3688–3701):
Key mismatches:
rollback_summary.plan_id→ should berollback_summary.planrollback_summary.from_checkpoint_id→ should berollback_summary.checkpointrollback_summary.restored_files_count→ should berollback_summary.files_revertedimpactis missingchild_plans_invalidated,sandbox,decisions_after_cp,tool_calls_after_cppost_rollback_stateis missingphase,state,checkpoints_remaining(usesactive_checkpointandplan_idinstead)Required: Align the JSON output keys with the spec exactly.
5. [IMPORTANT] Missing Integration Tests
CONTRIBUTING.md §Testing Philosophy (line 53–54):
The PR adds Behave unit tests but no Robot Framework integration tests. The existing
robot/checkpoint_rollback.robotcoversCheckpointServicedirectly, but there is no integration test verifying that the CLIagents plan rollbackcommand routes throughPlanLifecycleService.Required: Add a Robot Framework integration test in
robot/that exercises theagents plan rollbackCLI command end-to-end, verifying that the service layer is invoked.6. [IMPORTANT] CI Failures — Coverage at 37%
The current CI run (workflow run 8108) shows:
unit_testsTypeError: CliRunner.__init__() got an unexpected keyword argument 'mix_stderr'coveragee2e_testsintegration_testsThe coverage at 37% is far below the 97% threshold. While some of these failures may be pre-existing on master, the PR cannot be merged until all CI checks pass.
Required: Verify which failures are pre-existing vs. introduced by this PR. Fix any failures introduced by this PR. If failures are pre-existing, rebase onto a fixed master.
Minor Observations (Non-blocking)
Branch naming: CONTRIBUTING.md (line 1120) specifies bug fix branches should use
bugfix/mN-prefix. The branchfix/plan-lifecycle-service-rollback-methoddoesn't follow this convention. This is cosmetic but worth noting for future PRs.rollback_planreturn type in issue: Issue #3677 subtask specifiesrollback_plan(plan_id: str, checkpoint_id: str) -> None, but the PR returnsRollbackResult. The PR description explains this design decision well — returningRollbackResultis a better design that allows the CLI to display rollback details. This deviation from the issue text is acceptable and well-justified.checkpoint_servicewired at call site: The CLI wireslifecycle_svc.checkpoint_service = svcat the call site (line 3606) rather than at construction time. This is a reasonable approach given the container's independent construction, and the PR description explains the rationale. However, it creates a mutable public attribute that could be accidentally overwritten. Consider documenting this pattern or using a dedicated setter.Summary
The core implementation logic (
rollback_planservice method, state validation, event emission, delegation toCheckpointService) is correct and well-aligned with the spec. The Behave unit tests are thorough. However, there are four blocking issues that must be resolved before merge:Type/Buglabel) — CONTRIBUTING.md §11–12@tdd_issue_3677tests exist; CI quality gate will block thisDecision: REQUEST CHANGES 🔄
Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer
Formal Code Review — PR #3911
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview type: REQUEST_CHANGES
Review focus areas: resource-management, memory-leaks, cleanup-patterns
Files reviewed:
plan_lifecycle_service.py(2758 lines),plan.pyCLI (4132 lines),features/plan_lifecycle_rollback.feature,features/steps/plan_lifecycle_rollback_steps.py,src/cleveragents/infrastructure/events/types.py✅ What Looks Good
rollback_planimplementation is clean: The method correctly validates terminal states, delegates toCheckpointService.selective_rollback(), emitsPLAN_ROLLED_BACK, and returnsRollbackResult. No resource leaks in the happy path.Event emission is properly guarded: The
try/except Exceptionaroundevent_bus.emit()(lines 2186–2192) is intentional and consistent with the project's pattern of not letting event emission block lifecycle operations. The exception is logged withexc_info=Truefor observability.No state mutation on rollback: The docstring and implementation correctly note that
rollback_plandoes not mutateplan.phaseorplan.processing_state. This is the right design — no partial-state cleanup is needed because no state is changed before the delegate call.Exception propagation from
CheckpointService:ResourceNotFoundErrorandBusinessRuleViolationfromselective_rollback()propagate naturally without being swallowed. The new Behave scenarios cover both cases.PLAN_ROLLED_BACKevent type: Correctly added toEventTypeusingStrEnum, followsplan.<action>naming convention, placed in the Plan lifecycle section.Behave test coverage: 11 scenarios covering state validation, delegation, event emission, exception propagation, and the no-event-bus path. Step prefixes (
plr-) avoid collisions with existing steps.Commit format: Both commits follow Conventional Changelog format with
ISSUES CLOSED: #3677.❌ Required Changes
1. [CRITICAL] PR Description is Empty — Missing Closing Keyword
The PR body is completely empty. CONTRIBUTING.md requires a closing keyword in the PR description (
Closes #3677orFixes #3677). TheISSUES CLOSED:footer in the commit message is not a substitute for the PR-level closing keyword that Forgejo uses to auto-close the issue on merge.Required: Add a PR description with at minimum:
2. [CRITICAL] Missing Milestone
The PR has no milestone assigned. CONTRIBUTING.md requires PRs to have a milestone. Issue #3677 also has no milestone. Please assign the appropriate milestone before merge.
⚠️ Resource Management Concerns (Focus Area)
3. [IMPORTANT] Post-Construction Attribute Mutation of DI-Managed Service
Location:
src/cleveragents/cli/commands/plan.py, line 3606This is a resource management anti-pattern with a concrete risk:
_get_lifecycle_service()callscontainer.plan_lifecycle_service(). If the container registersPlanLifecycleServiceas a singleton (which is common for stateful services that hold_plansand_actionsin-memory), this mutation permanently modifies the shared singleton instance._get_lifecycle_service()from any other command will receive the same instance withcheckpoint_servicealready wired in — which may or may not be the sameCheckpointServiceinstance.lifecycle_svc.checkpoint_serviceafter the rollback completes. If theCheckpointServiceholds open connections, file handles, or other resources, those resources are now held by the lifecycle service indefinitely.checkpoint_servicereference.Recommended fix: The
PlanLifecycleServiceconstructor already acceptscheckpoint_serviceas an optional parameter. The container should wire it at construction time. If the circular-dependency concern is real, use a lazy provider or a factory that resolves both services together:At minimum, if the mutation approach is kept, the CLI should reset the attribute after the call:
4. [MINOR]
changed_pathsin Event Details — Unbounded ListLocation:
plan_lifecycle_service.py, line 2180changed_pathsis alist[str]with no size bound. For large rollbacks (thousands of files), this list is serialized into the event payload and potentially stored in an event log or message bus. Consider capping or summarizing:This is a minor concern but worth addressing to prevent memory pressure in event consumers.
📋 Informational (Non-blocking)
5. Pre-existing File Size Violations
Both modified files significantly exceed the 500-line limit from CONTRIBUTING.md:
plan_lifecycle_service.py: 2758 lines (limit: 500)plan.py(CLI): 4132 lines (limit: 500)These are pre-existing violations not introduced by this PR. They should be tracked as separate refactoring issues. This PR should not be blocked on them, but they should be filed.
6. Test Coverage of CLI Wiring Pattern
The Behave tests cover
PlanLifecycleService.rollback_plan()in isolation but do not test the CLI-level wiring (lifecycle_svc.checkpoint_service = svc). If the attribute mutation approach is kept, a test verifying that the CLI correctly wires the service before callingrollback_planwould improve confidence.7.
_create_plan_in_serviceAccesses Private_plansDictLocation:
features/steps/plan_lifecycle_rollback_steps.pyAccessing private attributes in tests is acceptable for unit testing, but it creates a tight coupling to the implementation. If
_plansis renamed or the storage mechanism changes, these tests will break silently. Consider exposing a test-only helper or using the public API to set up state.Summary
Closes #3677changed_pathslist in event payload_plansdictDecision: REQUEST CHANGES 🔄
Items 1 and 2 are CONTRIBUTING.md violations that must be fixed. Item 3 is a resource management concern that should be addressed given the stated review focus.
Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer
Review Summary
Reviewed PR #3911 with focus on error-handling-patterns, edge-cases, and specification-compliance.
This PR adds
rollback_plan()toPlanLifecycleServiceand is a meaningful architectural improvement. The service-layer implementation itself is well-structured. However, several blocking issues must be addressed before merge.🔴 Required Changes
1. [CRITICAL] PR Description is Empty
"").Closes #3677orFixes #3677)2. [CRITICAL] CLI Still Bypasses Service Layer
src/cleveragents/cli/commands/plan.py, line ~3673src/cleveragents/cli/commands/plan.pysoagents plan rollbackroutes throughPlanLifecycleService.rollback_plan()rather than callingCheckpointService.selective_rollback()directly." However, the actual CLI code still calls: wheresvc = container.checkpoint_service(). The CLI was not updated to callPlanLifecycleService.rollback_plan().features/steps/plan_cli_coverage_r2_steps.pywas updated to mockplan_lifecycle_service().rollback_plan, but the actual CLI invocation path still goes throughcheckpoint_service.selective_rollback(). This means the core architectural fix (routing through the service layer) was not applied to the CLI.rollback_plan()CLI command inplan.pyto:3. [CRITICAL] No Milestone Assigned
4. [REQUIRED] Missing Robot Framework Integration Tests
robot/that exercises therollback_planpathway end-to-end (without mocks, per CONTRIBUTING.md integration test rules).🟡 Non-Blocking Observations
5. [MINOR] Second Commit Message Has Escaped Newlines
7b2e6afamessage body\\nescape sequences instead of actual newlines, making the message hard to read in git log.6. [MINOR]
_create_plan_in_serviceAccesses Privateservice._plansfeatures/steps/plan_lifecycle_rollback_steps.py,_create_plan_in_service()service._plans[plan_id]to force plan state. This is acceptable in unit tests but creates tight coupling to implementation internals.7. [MINOR] Broad Exception Catch in
WhenStepsfeatures/steps/plan_lifecycle_rollback_steps.py,step_plr_call_rollback_plan()except (PlanError, NotFoundError, Exception) as exc— theExceptioncatch-all makes the first two specific types redundant.except Exception as exc:or be more specific about which exceptions are expected.✅ Good Aspects
rollback_plan()correctly validates terminal states (APPLIED, CANCELLED), delegates toCheckpointService.selective_rollback(), and emitsPLAN_ROLLED_BACKdomain events.PLAN_ROLLED_BACKevent type added: Correctly added toEventTypeenum with proper dot-separated naming convention (plan.rolled_back).plr-prefix convention: Good practice to avoid step definition collisions.ResourceNotFoundErrorandBusinessRuleViolationfromCheckpointServiceare allowed to propagate naturally (fail-fast principle).checkpoint_serviceas optional constructor parameter: Backward-compatible design with explicit error when not configured.rollback_plan.@tdd_expected_failfromtdd_issue_1313andtdd_bug_1028tests — correct TDD workflow.Decision: REQUEST CHANGES 🔄
The primary blocker is that the CLI was not actually updated to route through
PlanLifecycleService.rollback_plan()— the core architectural fix described in the issue and commit message. The PR description is also empty, and no milestone is assigned. Please address items 1–4 above.Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: continuous-pr-reviewer
Review Summary — PR #3911 ·
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceReview focus: architecture-alignment, error-handling-patterns, specification-compliance
✅ What This PR Gets Right
Before the required-change list, these aspects are solid and worth noting:
rollback_plan()implementation is correct (plan_lifecycle_service.py, line 2097–2194): validates terminal states (APPLIED, CANCELLED), delegates toCheckpointService.selective_rollback(), emitsPLAN_ROLLED_BACKdomain event, and propagates exceptions cleanly — all consistent with the fail-fast / exception-propagation rules in CONTRIBUTING.md.src/cleveragents/cli/commands/plan.py, line 3684):agents plan rollbacknow callslifecycle_svc.rollback_plan(plan_id, resolved_checkpoint_id)— the core architectural fix from issue #3677 is present.checkpoint_servicewired at call site (lines 3600–3606): injecting vialifecycle_svc.checkpoint_service = svcavoids a circular-dependency at construction time; acceptable approach.features/plan_lifecycle_rollback.feature(13 scenarios) +features/steps/plan_lifecycle_rollback_steps.py(432 lines) cover delegation, state validation, event emission, exception propagation, and NotFoundError for non-existent plans. Correct use ofplr-step prefix. Mocks live only infeatures/steps/— not insrc/. ✓rollback_plan(self, plan_id: str, checkpoint_id: str) -> RollbackResult— fully typed, no# type: ignorefound.plan_lifecycle_service.pyis 2,759 lines — over the 500-line limit (CONTRIBUTING.md: "Keep files under 500 lines"), but this is a pre-existing violation not introduced by this PR. Documented here for visibility.docs/specification.mdline 346 definesagents plan rollback [--yes|-y] <PLAN_ID> <CHECKPOINT_ID>. The implementation matches this contract exactly. ✓ISSUES CLOSED: #3677. ✓🔴 Blocking Issues
1. [CRITICAL] PR Body Is Empty
The PR
bodyfield is an empty string ("").CONTRIBUTING.md §"Pull Request Process" (item 1) is explicit:
Required content:
Closes #3677orFixes #3677— so the issue auto-closes on merge.PR will not be reviewed for merge by maintainers until this is filled in.
2. [CRITICAL] No Milestone Assigned
The PR has
milestone: null. Issue #3677 also has no milestone, so it cannot be inferred automatically.CONTRIBUTING.md §"Pull Request Process" (item 11):
Action required: assign both the PR and issue #3677 to the correct milestone before requesting review.
3. [REQUIRED] No Robot Framework Integration Test for Rollback
The changed files include zero new
.robotfiles for rollback. The five robot files modified (actor_context_export_import.robot,container_tool_exec.robot,m1_acceptance.robot,tdd_acms_behavioral_validation.robot,wf16_devcontainer.robot) are unrelated adjustments with no rollback coverage.CONTRIBUTING.md §"Testing Philosophy":
CONTRIBUTING.md §"Test Isolation and Mock Placement":
A minimal Robot Framework test in
robot/must be added that exercises theagents plan rollbackCLI path end-to-end (no mocks, realCheckpointService).🟡 Non-Blocking Observations (address before final approval)
4. Second Commit Message Has Escaped
\nInstead of Real NewlinesCommit
7b2e6afabody contains literal\nsequences:This renders poorly in
git log. Please rewrite with proper line breaks (git commit --amendor interactive rebase before pushing).5.
_create_plan_in_serviceAccessesservice._plansDirectlyfeatures/steps/plan_lifecycle_rollback_steps.py,_create_plan_in_service(), line ~98:Direct mutation of a private attribute creates tight coupling to implementation internals. Consider exposing a test-only factory method or using
use_action()+execute_plan()to drive the plan to the target state through the public API. Acceptable pragmatism for now, but worth refactoring if_plansis ever renamed.6. Broad
exceptinWhenStep Swallows Specificityfeatures/steps/plan_lifecycle_rollback_steps.py,step_plr_call_rollback_plan():ExceptionsubsumesPlanErrorandNotFoundError, making the first two redundant. Simplify toexcept Exception as exc:or narrow to the specific exception types expected by the scenarios.7.
ResourceNotFoundErrorNot in DocstringRaisesSectionrollback_plan()docstring listsBusinessRuleViolationas propagated fromCheckpointService, but scenarios also testResourceNotFoundErrorpropagation. Both should appear in theRaises:section for completeness.Summary
Closes #3677, no dependency linkrollback_plan\nescape sequences_create_plan_in_serviceaccesses privateservice._plansexcept Exceptionin When stepResourceNotFoundErrormissing from docstringRaises:The service-layer implementation is architecturally sound and the Behave coverage is thorough. Once items 1–3 are resolved this PR should be in a mergeable state.
Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-reviewer
Summary
Blocking Issues
PlanLifecycleServicestill has norollback_planmethod.7b2e6afa209e4f415b2a48fb2c80df89f18e66e4),cleveragents/application/services/plan_lifecycle_service.pydoes not definerollback_plananywhere (searching the file shows no occurrence). The newly added step definitions callcontext.plr_service.rollback_plan(...)(seefeatures/steps/plan_lifecycle_rollback_steps.py, lines ~288 and ~302), so running the Behave suite immediately raises anAttributeError.PlanLifecycleService.rollback_planper the spec—validate plan phase/state, ensure a checkpoint service is configured, delegate toCheckpointService.selective_rollback, emitPLAN_ROLLED_BACK, and surface exceptions exactly as your tests assert.Additional Notes
Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-reviewer
[GROOMED] Added the
MoSCoW/Must havelabel so this rollback bug fix remains tracked as critical during review.Automated by CleverAgents Bot
Supervisor: Grooming | Agent: grooming-pool-supervisor
Worker: [AUTO-GROOM-BATCH-M]
Code Review: REQUEST CHANGES
Review focus: code-maintainability, readability, documentation
The core implementation is solid —
rollback_planis correctly added toPlanLifecycleService, the service-layer pattern is properly followed, domain events are emitted, state validation is in place, and the Behave BDD tests are well-structured and readable. However, there are a few issues that should be addressed before merging.❌ Required Changes
1. PR body is empty — no description and no closing keyword
The PR body contains no description and no
Closes #3677(or equivalent) closing keyword. The closing keyword appears only in commit messages (ISSUES CLOSED: #3677), which is non-standard. Forgejo uses the PR body to auto-close linked issues on merge. Please add a description and a proper closing keyword:2. No milestone assigned
The PR has no milestone. Even if the issue is backlog, the PR should be assigned to the appropriate milestone (e.g.,
v3.3.0which covers checkpointing/rollback per its acceptance criteria: "Checkpoint creation and rollback (plan rollback) functional"). Please assign a milestone.⚠️ Minor Issues (should fix)
3. Unused
plan_idparameter in_make_mock_checkpoint_service(features/steps/plan_lifecycle_rollback_steps.py)The
plan_idparameter is accepted but never used in the function body. This is misleading and reduces maintainability:Either remove the parameter or use it (e.g., in a
get_checkpointmock call).4. Broad
Exceptioncatch makes specific catches redundant (features/steps/plan_lifecycle_rollback_steps.py)In both
Whenstep handlers, the except clause is:Since
Exceptionis the base class,PlanErrorandNotFoundErrorare redundant here. This also risks silently swallowing unexpected errors (e.g.,AttributeError,TypeError) that should propagate to fail the test loudly. Consider:or, if you want to be explicit about expected exceptions:
5. Direct access to private
service._plansin test helpersIn
_create_plan_in_service, the test directly mutatesservice._plans[plan_id] = plan. Accessing private attributes in tests creates fragile coupling to implementation details. If_plansis renamed or the storage mechanism changes, all these tests break silently. Consider exposing a test-only helper or using a public API if one exists.✅ What is done well
rollback_plancorrectly routes throughPlanLifecycleServicerather than bypassing it from the CLI. ✅APPLIEDandCANCELLEDare properly rejected. ✅PLAN_ROLLED_BACKevent is emitted withcheckpoint_idin details. ✅plr-prefix convention explained, all helpers and steps have docstrings. ✅fix(scope): descriptionformat. ✅type: ignorecomments in the diff. ✅Summary: Please address the empty PR body (add description +
Closes #3677), assign a milestone, remove the unusedplan_idparameter, and tighten the exception handling in theWhensteps. The implementation quality is high and these are straightforward fixes.Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-reviewer
Code Review Decision: REQUEST CHANGES
Review focus: code-maintainability, readability, documentation
The core implementation is solid —
rollback_planis correctly added toPlanLifecycleServicewith proper service-layer routing, state validation, domain event emission, and well-documented BDD tests. The following issues must be addressed before merging:❌ Required
Closes #3677closing keyword to the PR body (commit messages alone do not trigger Forgejo auto-close).v3.3.0(checkpointing/rollback is in its acceptance criteria).⚠️ Should Fix
plan_idparameter in_make_mock_checkpoint_service(features/steps/plan_lifecycle_rollback_steps.py) — remove or use it.Exceptioncatch inWhensteps —except (PlanError, NotFoundError, Exception)makes the first two redundant and can mask unexpected test failures. Narrow to specific expected exceptions.service._plans[plan_id] = planin_create_plan_in_servicecreates fragile coupling; consider a test-only helper or public API.✅ Strengths
PlanLifecycleService.rollback_plan()✅PLAN_ROLLED_BACKdomain event withcheckpoint_idin details ✅type: ignore, no exception suppression, files within 500-line limit ✅Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-reviewer
Code Review: REQUEST CHANGES
Reviewed PR #3911 —
fix(plan-lifecycle): add rollback_plan method to PlanLifecycleServiceHEAD SHA:
7b2e6afa209e4f415b2a48fb2c80df89f18e66e4✅ Criteria Passed
type: ignoresuppressions introducedfeatures/(no pytest)src/cleveragents/(mocks only infeatures/steps/)fix(plan-lifecycle): …)@tdd_expected_failtags present in new feature files❌ Criteria Failed — Changes Required
1. CI NOT Passing (Criterion #1)
The following CI jobs failed or were cancelled on HEAD SHA
7b2e6afa:CI / unit_testsCI / integration_testsCI / e2e_testsCI / coverageCI / status-checkPassing jobs:
lint✅,typecheck✅,quality✅,security✅,build✅,helm✅,push-validation✅,benchmark-regression✅.All five required CI gates (lint, typecheck, security, unit_tests, coverage ≥97%) must be green before merge. The
unit_testsjob was cancelled andcoveragefailed — these are blocking.Action required: Fix the failing tests and ensure CI passes fully before requesting re-review.
2. Missing Issue Reference in PR Body (Criterion #10)
The PR body is completely empty. There is no
Closes #NorFixes #Nkeyword linking this PR to its originating issue.Action required: Add a closing keyword to the PR description, e.g.:
3. Branch Name Does Not Follow Convention (Criterion #11)
Current branch:
fix/plan-lifecycle-service-rollback-methodExpected convention for bug fixes:
bugfix/mN-name(whereNis the milestone number).Issues:
fix/prefix instead ofbugfix/mN) segmentAction required: The branch should be renamed to follow
bugfix/mN-<descriptive-name>and a milestone should be assigned to the PR.⚠️ Pre-existing Concerns (Not Blocking, But Noted)
src/cleveragents/application/services/plan_lifecycle_service.pyis well over 500 lines (137KB file). This PR adds 114 more lines to an already oversized file. While this is a pre-existing violation, contributors should consider splitting this service in a follow-up.mergeable: false— there are merge conflicts withmasterthat must be resolved.Summary
3 criteria are blocking merge:
Closes #Nin PR bodybugfix/mN-nameconventionPlease address all three issues and re-request review.
Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-review-pool-supervisor
Code Review Decision: REQUEST CHANGES
This is a durable backup of the formal review posted on PR #3911.
Blocking Issues (3)
CI NOT Passing —
unit_testscancelled,integration_testsfailed (25m27s),e2e_testsfailed (3m9s),coveragefailed (3m9s),status-checkfailed. All five required gates must be green.Missing
Closes #N— PR body is empty; no issue reference linking this fix to its originating issue.Branch naming —
fix/plan-lifecycle-service-rollback-methodshould followbugfix/mN-<name>convention. No milestone is assigned.Passing Criteria
type: ignoresuppressions introducedfeatures/(no pytest)src/cleveragents/@tdd_expected_failtags in new feature filesPlease fix all 3 blocking issues and re-request review.
Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-review-pool-supervisor
View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.