fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode #10945
No reviewers
Labels
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
No project
4 participants
Notifications
Due date
No due date set.
Blocks
#10874 fix(plan): executor overwrites error_details destroying strategy_decisions_json and hardcodes mode as stub
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!10945
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bugfix/executor-error-details-overwrite"
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?
Summary
Fixes three bugs in
PlanExecutor._run_execute_with_stub(now renamed_run_execute_with_actor):Changes
type(self._execute_actor).__name__instead of hardcoded"stub"_run_execute_with_stub→_run_execute_with_actorTesting
Closes #10874
da216fc09d9b8d98848atest
Code Review Report - PR #10945 (bugfix/executor-error-details-overwrite)
Related Issue: #10874 (#10874)
Branch: bugfix/executor-error-details-overwrite onto master
Commit: 9b8d9884 fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode
Summary
This PR correctly addresses the three bugs described in #10874:
The merge pattern (dict(old).update(new)) is applied consistently in both the success and failure paths of _run_execute_with_actor. The mode field now correctly reports the actual actor class name.
Two global review cycles were performed covering: bugs, test quality, naming consistency, performance, and security. Below are all findings organized by category and severity.
FINDINGS
BUG: _build_decisions swallows unexpected errors in bare except clause
Severity: Medium | Category: Bug Detection (pre-existing, not introduced by this PR)
_build_decisions (lines 839-846) deserializes strategy_decisions_json and calls StrategyDecision.model_validate(d). ValidationError is caught by bare
except Exceptionat line 846, then silently falls back to parsing definition_of_done.While the fallback is intentional, bare
except Exceptionalso swallows unexpected errors (memory issues, recursion depth). Consider narrowing the exception scope to known error types (json.JSONDecodeError, pydantic.ValidationError).Location: src/cleveragents/application/services/plan_executor.py:846
Recommendation: Follow up in a related issue; out of scope for this PR but the retry path depends on _build_decisions.
TEST FLAW: Feature file section headers still reference obsolete _run_execute_with_stub naming
Severity: Low | Category: Naming Consistency
The plan_executor_coverage.feature file has 5 locations still referencing _run_execute_with_stub (the section comment at line 333, and scenario titles at lines 336, 344, 352, 361). The step-comment in plan_executor_coverage_steps.py:906 was correctly updated. Feature file should be updated for consistency.
Location: features/plan_executor_coverage.feature:333-368
TEST FLAW: Failing-execute mock uses bare MagicMock without spec
Severity: Low | Category: Test Quality / Fragility
In executor_error_details_steps.py line 138, the failing-execute scenario creates a bare MagicMock() without spec. The succeeding-execute scenario (line 119) correctly uses MagicMock(spec=ExecuteStubActor). Inconsistent approach means the failing scenario would silently pass even if ExecuteStubActor's execute() signature changes. Both should use spec.
Location: features/steps/executor_error_details_steps.py:138
TEST: New feature has no orchestration entry point
Severity: Low | Category: Test Quality
features/executor_error_details.feature uses the @executor-error-details tag with steps in features/steps/ (Behave auto-loads). No master/conductor feature or entry point references this new feature. It may be missed by orchestration/test-runner tools. Consider adding it to a master feature if applicable.
Location: features/executor_error_details.feature
NAMING: Docstring wording imprecise
Severity: Low | Category: Naming Consistency
Line 1012 docstring says "Execute using the configured execute actor with optional retry." The retry is specifically controlled by error_recovery.max_retries. No functional impact.
Location: src/cleveragents/application/services/plan_executor.py:1012
PRE-EXISTING: _run_execute_with_runtime still uses old dict-literal pattern
Severity: Low | Category: Code Consistency (pre-existing)
_run_execute_with_runtime (lines 924-1006) still uses
plan.error_details = {key: value}at lines 956 and 999, which does NOT preserve prior error_details. For full consistency, runtime mode should also use the merge pattern. Out of scope for this PR but worth tracking.Location: src/cleveragents/application/services/plan_executor.py:956, 999
NOT FOUND (verified across 2 global cycles)
All 6 review findings have been addressed across the 3 commits:
except Exceptionin_build_decisions(json.JSONDecodeError, ValidationError)_run_execute_with_stubspec=ExecuteStubActorErrorRecoveryService.max_retries_run_execute_with_runtimeuses dict-literal (inconsistent)features/*.featurefilesAll 71 related scenarios pass (55 executor coverage + 4 new error_details + 12 error recovery). Lint and typecheck clean.
— @hamza.khyari
3be2e3d933a18b4da775Review Summary -- PR #10945
Closes #10874: Fix three bugs in PlanExecutor error handling during execute phase.
Issue verification:
10-Category Checklist Results:
CORRECTNESS --
SPECIFICATION ALIGNMENT --
TEST QUALITY --
TYPE SAFETY --
READABILITY --
PERFORMANCE --
SECURITY --
CODE STYLE --
DOCUMENTATION --
COMMIT AND PR QUALITY --
Non-blocking observations:
Verdict: APPROVED -- All three bugs correctly fixed, tests adequately cover changes, CI green, no blocking issues.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Review Summary for PR #10945 — fix(plan): preserve strategy_decisions_json in error_details (#10874)
Issues:
All three bugs reported in #10874 are correctly fixed:
error_details merge — Previously plan.error_details = {...} replaced the entire dict, destroying strategy_decisions_json stored by run_strategize. Now the code does existing = dict(plan.error_details or {}); existing.update({...}); plan.error_details = existing, preserving stored strategy decisions across the execute phase.
Actual actor mode — Previously mode: "stub" was hardcoded in 3 places. Now mode reflects type(self._execute_actor).name, so LLMExecuteActor, ExecuteStubActor, etc. are accurately reported.
Method rename — _run_execute_with_stub to _run_execute_with_actor correctly reflects that the method runs whatever actor is configured, not just the stub.
Review checklist results:
Suggestion (non-blocking):
_run_execute_with_runtime still uses replacement (not merge) for error_details. This means plans executed in runtime mode with a prior strategize phase will also lose strategy_decisions_json. This is a pre-existing issue separate from this PR but worth following up on.
Verdict: APPROVED - All blockers fixed, all checklist categories pass, CI is fully green (14/14 passing).
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
a18b4da77586e1cdf6aeRe-Review of PR #10945
Issue #10874: executor overwrites error_details destroying strategy_decisions_json and hardcodes mode as stub.
Prior feedback status: No REQUEST_CHANGES reviews with comments found. Full review conducted.
All three bugs from issue #10874 fixed:
Review against checklist:
CI: All 14 checks passing.
Verdict: APPROVED
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Summary
Reviewed PR #10945 (
fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode). This is a clean, well-scoped bug fix addressing issue #10874.Issues addressed:
Overwrites error_details entirely — Fixed. Both the success path (line 1046 area) and the failure path (line 1115 area) now merge error_details via
dict(existing).update({...})instead of outright replacement.strategy_decisions_jsonis preserved.Hardcodes mode as "stub" — Fixed. All three occurrences (lines 1049, 1113, 1122) replaced with
type(self._execute_actor).__name__. The log message was also cleaned up (removed the parenthetical "(stub)").Method misnamed — Fixed.
_run_execute_with_stubrenamed to_run_execute_with_actoreverywhere. No residual references in any Python file.Test quality:
4 new Behave BDD scenarios across two scenarios per execute path (success/failure), verifying both preservation of
strategy_decisions_jsonand actual-mode reporting. Step definitions are well-structured with clear Gherkin naming using theeedprefix for readability. Good use of mocks for the lifecycle service and execute actor.CI:
All 14 CI checks pass (lint, typecheck, security, unit_tests, integration_tests, e2e_tests, coverage, build, docker, helm, status-check). All green.
Overall:
Acceptable with minor suggestions below. No blocking issues.
@ -331,3 +331,3 @@# ------------------------------------------------------------------# PlanExecutor._run_execute_with_stub - error recovery / retry# PlanExecutor._run_execute_with_actor - error recovery / retryNon-blocking: Comment still references
_run_execute_with_stub(line 333:# PlanExecutor._run_execute_with_stub - error recovery / retry). Consider updating to_run_execute_with_actorfor consistency, though this is a comment so not blocking. Suggestion: update for documentation clarity.This PR is a clean, well-scoped bug fix addressing issue #10874. All three reported issues are correctly resolved:
CI: All 14 checks passing. 4 new Behave scenarios cover both success and failure execution paths.
Non-blocking suggestion: update the leftover comment in plan_executor_coverage.feature (line 333) that still references the old method name.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Re-Review Summary -- PR #10945
Closes #10874: Fix three bugs in PlanExecutor error handling during execute phase.
Prior Feedback Resolution
All 6 findings from CoreRasurae REQUEST_CHANGES review have been addressed:
except Exception→Narrowed ✅ Changed to(json.JSONDecodeError, ValidationError)_run_execute_with_stubupdated toactorspec=on mock ✅ AddedMagicMock(spec=ExecuteStubActor)ErrorRecoveryService.max_retries_run_execute_with_runtimeFull Review Checklist
1. CORRECTNESS ✅ — All 3 bugs fixed across all 4 code paths (runtime success/failure, actor success/failure). Merge pattern handles None initial state.
2. SPECS ALIGNMENT ✅ — error_details is shared state across phases; preserving it during execute is correct per spec.
3. TEST QUALITY ✅ — 4 new BDD scenarios cover success/failure × preservation/mode. All 71 scenarios pass (55 + 4 + 12).
4. TYPE SAFETY ✅ — Fully annotated. No
# type: ignore.dict[str, Any] | Nonetypes correct.5. READABILITY ✅ — Merge pattern is clear and idiomatic. Method rename is accurate.
6. PERFORMANCE ✅ — Shallow dict copy of a few string fields. No new allocations or loops.
7. SECURITY ✅ — No new secrets, injection vectors, or unsafe patterns.
8. CODE STYLE ✅ — SOLID principles followed. Consistent with existing patterns. ruff compliant.
9. DOCUMENTATION ✅ — Method docstring expanded. Step file has module-level docs.
10. COMMIT AND PR QUALITY ✅ — 3 atomic commits. First line matches Metadata exactly. Labels, milestone, and dependency direction all correct.
CI Status
14/14 checks passing: lint, typecheck, security, unit_tests, integration_tests, e2e_tests, coverage, build, docker, helm, push-validation, quality, status-check, benchmark-publish.
Verdict: APPROVED — All three bugs correctly fixed, all prior feedback addressed, tests adequate, CI green, no blocking issues.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Implementation Attempt — Tier 0: qwen — No fixes neededAnalysis completed on the merged PR:- PR state: closed, successfully merged to master- All CI checks: 14/14 passing- All review findings from CoreRasurae REQUEST_CHANGES have been addressed- Multiple APPROVED reviews present, no open REQUEST_CHANGESThe PR is already in completion state. No changes needed.---Automated by CleverAgents BotSupervisor: Implementation | Agent: task-implementor