fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode #10932

hamza.khyari · 2026-04-29T14:04:03Z

hamza.khyari commented

2026-04-29 14:04:03 +00:00

Fixed three bugs in _run_execute_with_actor (formerly _run_execute_with_stub):

Merged error_details instead of replacing — preserves strategy_decisions_json stored by run_strategize() so retry plans retain the full decision hierarchy (Forgejo #10874).
Used type(self._execute_actor).__name__ instead of hardcoded "stub" at all three locations (success, on_error checkpoint, fail) so the mode field reflects the actual actor type (Forgejo #10874).
Renamed method from _run_execute_with_stub to _run_execute_with_actor since it accepts any configured actor (stub or LLM), not just ExecuteStubActor.

Added two Behave scenarios verifying:

strategy_decisions_json survives execute and is preserved
mode reflects actual actor type (LLMExecuteActor, ExecuteStubActor, etc.)

Updated test step comments and docs to match the rename.

ISSUES CLOSED: #10874

Fixed three bugs in `_run_execute_with_actor` (formerly `_run_execute_with_stub`): 1. **Merged `error_details` instead of replacing** — preserves `strategy_decisions_json` stored by `run_strategize()` so retry plans retain the full decision hierarchy (Forgejo #10874). 2. **Used `type(self._execute_actor).__name__` instead of hardcoded `"stub"`** at all three locations (success, on_error checkpoint, fail) so the mode field reflects the actual actor type (Forgejo #10874). 3. **Renamed method** from `_run_execute_with_stub` to `_run_execute_with_actor` since it accepts any configured actor (stub or LLM), not just ExecuteStubActor. Added two Behave scenarios verifying: - `strategy_decisions_json` survives execute and is preserved - `mode` reflects actual actor type (`LLMExecuteActor`, `ExecuteStubActor`, etc.) Updated test step comments and docs to match the rename. ISSUES CLOSED: #10874

hamza.khyari added 1 commit

2026-04-29 14:04:04 +00:00

fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode

CI / benchmark-publish (pull_request) Has been skipped

Details

CI / helm (pull_request) Successful in 37s

Details

CI / build (pull_request) Successful in 1m0s

Details

CI / lint (pull_request) Failing after 1m4s

Details

CI / quality (pull_request) Successful in 1m20s

Details

CI / security (pull_request) Successful in 1m23s

Details

CI / typecheck (pull_request) Successful in 1m38s

Details

CI / push-validation (pull_request) Successful in 31s

Details

CI / unit_tests (pull_request) Failing after 2m30s

Details

CI / coverage (pull_request) Has been skipped

Details

CI / docker (pull_request) Has been skipped

Details

CI / integration_tests (pull_request) Successful in 3m40s

Details

CI / e2e_tests (pull_request) Successful in 4m20s

Details

CI / status-check (pull_request) Failing after 3s

Details

cc3a5a6246

Fixed three bugs in _run_execute_with_actor (formerly _run_execute_with_stub):

1. Merged error_details instead of replacing - preserves
   strategy_decisions_json stored by run_strategize() so retry plans
   retain the full decision hierarchy (Forgejo #10874).

2. Used type(self._execute_actor).__name__ instead of hardcoded "stub"
   at all three locations (success, on_error checkpoint, fail) so the
   mode field reflects the actual actor type (Forgejo #10874).

3. Renamed method from _run_execute_with_stub to
   _run_execute_with_actor since it accepts any configured actor
   (stub or LLM), not just ExecuteStubActor.

Added two Behave scenarios verifying strategy_decisions_json survival
and actual actor mode reporting. Updated test step comments and docs.

ISSUES CLOSED: #10874

HAL9000 referenced this pull request

2026-04-29 21:17:00 +00:00

fix(plan): preserve strategy_decisions_json in error_details during execute and report actual actor mode #10936

HAL9001 requested changes

2026-04-30 04:31:38 +00:00

HAL9001 left a comment

Review Summary

What was reviewed

Bug fix that renames _run_execute_with_stub to _run_execute_with_actor, reports the actual actor type name instead of hardcoded "stub", and preserves existing error_details (including strategy_decisions_json) during execute instead of clobbering it.

Checklist Results

CORRECTNESS — The fix correctly preserves existing error_details by copying and merging instead of clobbering. The mode now reports type(self._execute_actor).__name__ for both success and error paths. The rename from stub to actor accurately reflects the configurable actor architecture. PASS
SPECIFICATION ALIGNMENT — The code aligns with the existing architecture where _execute_actor is configurable via constructor injection. The rename and mode reporting are consistent with the design. PASS
TEST QUALITY — Two new Gherkin scenarios added covering (a) strategy_decisions_json preservation through execute, and (b) actual actor type reporting in error_details.mode. However, see blocking issues with CI and orphaned test code.
TYPE SAFETY — All function signatures and variables are properly annotated. No # type: ignore comments introduced. PASS
READABILITY — Clear naming, well-structured diff. The existing.update() pattern is easy to follow. PASS
PERFORMANCE — No unnecessary overhead. Minimal dict copy operations only during plan persistence. PASS
SECURITY — No new attack surface. No injection risks or hardcoded credentials. PASS
CODE STYLE — Follows ruff conventions, SOLID principles (SRP maintained — executor only manages orchestration). PASS
DOCUMENTATION — Read-only actions doc updated to reflect renamed method. PASS
COMMIT AND PR QUALITY — See blocking issues below.

Blocking Issues

See inline comments.

## Review Summary ### What was reviewed Bug fix that renames `_run_execute_with_stub` to `_run_execute_with_actor`, reports the actual actor type name instead of hardcoded `"stub"`, and preserves existing `error_details` (including `strategy_decisions_json`) during execute instead of clobbering it. ### Checklist Results 1. **CORRECTNESS** — The fix correctly preserves existing error_details by copying and merging instead of clobbering. The mode now reports `type(self._execute_actor).__name__` for both success and error paths. The rename from `stub` to `actor` accurately reflects the configurable actor architecture. **PASS** 2. **SPECIFICATION ALIGNMENT** — The code aligns with the existing architecture where `_execute_actor` is configurable via constructor injection. The rename and mode reporting are consistent with the design. **PASS** 3. **TEST QUALITY** — Two new Gherkin scenarios added covering (a) strategy_decisions_json preservation through execute, and (b) actual actor type reporting in error_details.mode. However, see blocking issues with CI and orphaned test code. 4. **TYPE SAFETY** — All function signatures and variables are properly annotated. No `# type: ignore` comments introduced. **PASS** 5. **READABILITY** — Clear naming, well-structured diff. The `existing.update()` pattern is easy to follow. **PASS** 6. **PERFORMANCE** — No unnecessary overhead. Minimal dict copy operations only during plan persistence. **PASS** 7. **SECURITY** — No new attack surface. No injection risks or hardcoded credentials. **PASS** 8. **CODE STYLE** — Follows ruff conventions, SOLID principles (SRP maintained — executor only manages orchestration). **PASS** 9. **DOCUMENTATION** — Read-only actions doc updated to reflect renamed method. **PASS** 10. **COMMIT AND PR QUALITY** — See blocking issues below. ### Blocking Issues See inline comments.

Commit message follows Conventional Changelog format but is missing the required footer ISSUES CLOSED: #N. Since this is a fix(plan) commit, please add the linked issue number to the commit footer. Without it, the PR cannot be merged per project policy.

Commit message follows Conventional Changelog format but is missing the required footer `ISSUES CLOSED: #N`. Since this is a `fix(plan)` commit, please add the linked issue number to the commit footer. Without it, the PR cannot be merged per project policy.

The PR description is missing from the Forgejo API response (404), and no Closes #N or Fixes #N keyword was found in the title. Per project PR requirements (12-item checklist, item 1), every PR must include a closing keyword for its linked issue. This PR will not be reviewed until an issue is linked with a closing keyword. Additionally, the commit footer ISSUES CLOSED: #N is missing.

CRITICAL: CI is failing on lint and unit_tests checks, and coverage was skipped. Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved and merged. The author should run nox locally, fix the lint and unit test failures (likely related to the orphaned assert at end of plan_executor_coverage_steps.py line 1367), and retry.

The PR description is missing from the Forgejo API response (404), and no `Closes #N` or `Fixes #N` keyword was found in the title. Per project PR requirements (12-item checklist, item 1), every PR must include a closing keyword for its linked issue. This PR will not be reviewed until an issue is linked with a closing keyword. Additionally, the commit footer `ISSUES CLOSED: #N` is missing. **CRITICAL**: CI is failing on `lint` and `unit_tests` checks, and `coverage` was skipped. Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved and merged. The author should run `nox` locally, fix the lint and unit test failures (likely related to the orphaned assert at end of `plan_executor_coverage_steps.py` line 1367), and retry.

features/steps/plan_executor_coverage_steps.py

					
				@ -1270,0 +1364,4 @@

				    custom_actor.__class__.__name__ = "MockCustomExecuteActor"

				    context.cov2_executor._execute_actor = custom_actor

				    custom_actor.execute.return_value = ExecuteResult(

				        changeset_id="cs-001",

Orphaned assert statement referencing undefined result at the end of step_cov2_executor_with_custom_actor. This appears to be leftover from a copied @then scenario that was accidentally left behind. The line assert len(result.decisions) > 0, "Expected at least one decision" will never execute and references a variable that does not exist in this scope. It must be removed.

Orphaned assert statement referencing undefined `result` at the end of `step_cov2_executor_with_custom_actor`. This appears to be leftover from a copied `@then` scenario that was accidentally left behind. The line `assert len(result.decisions) > 0, "Expected at least one decision"` will never execute and references a variable that does not exist in this scope. It must be removed.

src/cleveragents/application/services/plan_executor.py

Suggestion: the mode field now stores type(self._execute_actor).__name__. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing error_details["mode"] may break unexpectedly. Consider using a stable identifier string like "stub" or "runtime" instead of class names.

Suggestion: the `mode` field now stores `type(self._execute_actor).__name__`. Consider whether class names are a stable contract — if classes are renamed or refactored in the future, downstream code parsing `error_details["mode"]` may break unexpectedly. Consider using a stable identifier string like `"stub"` or `"runtime"` instead of class names.