fix: address code review findings from PR #1175 (issue #10267) #10271

Merged
HAL9000 merged 1 commits from feature/m11-issue-10267-code-review-followup into master 2026-04-21 14:55:56 +00:00

1 Commits

Author SHA1 Message Date
CoreRasurae 7f29874156 fix: address code review findings from PR #1175 (issue #10267)
CI / push-validation (pull_request) Successful in 23s
CI / helm (pull_request) Successful in 37s
CI / build (pull_request) Successful in 3m48s
CI / lint (pull_request) Successful in 3m55s
CI / quality (pull_request) Successful in 4m18s
CI / typecheck (pull_request) Successful in 4m32s
CI / security (pull_request) Successful in 4m35s
CI / integration_tests (pull_request) Successful in 6m47s
CI / e2e_tests (pull_request) Successful in 6m56s
CI / unit_tests (pull_request) Successful in 7m23s
CI / docker (pull_request) Successful in 1m30s
CI / coverage (pull_request) Successful in 14m31s
CI / benchmark-regression (push) Waiting to run
CI / benchmark-publish (push) Waiting to run
CI / status-check (pull_request) Successful in 3s
CI / helm (push) Successful in 27s
CI / push-validation (push) Successful in 26s
CI / build (push) Successful in 3m43s
CI / lint (push) Successful in 3m54s
CI / quality (push) Successful in 4m16s
CI / typecheck (push) Successful in 4m28s
CI / security (push) Successful in 4m43s
CI / integration_tests (push) Successful in 7m3s
CI / e2e_tests (push) Successful in 7m13s
CI / unit_tests (push) Successful in 7m25s
CI / docker (push) Successful in 1m36s
CI / coverage (push) Successful in 13m40s
CI / status-check (push) Successful in 3s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Failing after 14m31s
Implement comprehensive fixes for all P2:should-fix and P3:nit findings
from the Strategy Actor code review:

Update 5 step functions that were calling the private _execute_with_llm() method
directly to instead use the new strategy_tree field from StrategizeResult:

 - step_execute_and_inspect_tree (line 619)
 - step_parse_self_dep (line 877)
 - step_parse_duplicate_step_numbers (line 968)
 - step_parse_non_sequential_steps (line 1075)
 - step_parse_non_sequential_steps_and_inspect (line 1755)
 - step_build_decisions_from_llm_tree (line 1299) - also added execute() call

This eliminates the fragile double-execution pattern that produced divergent ULIDs
and couples tests to private implementation details. Tests now use the public
StrategizeResult.strategy_tree field.

P2:should-fix Fixes:
 - Move json import to module level in plan_executor_coverage_steps.py
 - Remove redundant Exception clause in plan_executor.py exception handler
 - Add logging to silent config service fallback in plan.py
 - Improve structured content block handling in _extract_content() to properly
   extract text from LangChain MessageContent dicts
 - Remove duplicated _DEFAULT_ACTOR_NAME constant and import from canonical
   source (strategy_resolution.py)
 - Add strategy_tree field to StrategizeResult to expose tree for test inspection
   without coupling to private _execute_with_llm() method

P3:nit Fixes:
 - Improved docstrings and code clarity

Refs: #10267
2026-04-21 15:36:09 +01:00