From 8ed8c25652ece264114756576ed2fe083ed66313 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Tue, 14 Apr 2026 12:58:41 +0000 Subject: [PATCH 1/2] fix(plan-lifecycle): record full context snapshots in Strategize phase The Strategize phase was recording decisions with minimal context snapshots (only a hash of question+chosen_option), violating the v3.2.0 acceptance criterion that decisions must include full context snapshots sufficient to replay the decision. Changes: - Add _build_strategize_context_snapshot() helper that builds a full ContextSnapshot from plan metadata (description, action_name, strategy_actor, project_links) - Update _try_record_decision() to accept an optional context_snapshot parameter and forward it to DecisionService - Update start_strategize() to build and pass a full context snapshot - Add 3 BDD scenarios in decision_recording.feature verifying that hot_context_hash, hot_context_ref, actor_state_ref, and relevant_resources are all populated for Strategize-phase decisions ISSUES CLOSED: #9056 --- CHANGELOG.md | 11 ++ CONTRIBUTORS.md | 4 +- features/decision_recording.feature | 29 ++++ features/steps/decision_recording_steps.py | 139 ++++++++++++++++++ .../services/plan_lifecycle_service.py | 77 +++++++++- 5 files changed, 257 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c9d662fd..fba107b5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,17 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). This wires the previously-isolated `discover_devcontainers()` function into the production code path, enabling the spec's zero-configuration devcontainer experience. +- **Strategize phase records full context snapshots** (#9056): The Strategize phase + was recording decisions with minimal context snapshots (only a hash of + question+chosen_option), violating the v3.2.0 acceptance criterion that decisions + must include full context snapshots sufficient to replay the decision. Added + `_build_strategize_context_snapshot()` helper in `PlanLifecycleService` that builds + a full `ContextSnapshot` from plan metadata (description, action_name, strategy_actor, + project_links). Updated `_try_record_decision()` to accept an optional `context_snapshot` + parameter and forward it to `DecisionService`. Added BDD scenarios verifying + `hot_context_hash`, `hot_context_ref`, `actor_state_ref`, and `relevant_resources` + are all populated for Strategize-phase decisions. + ### Changed - **`agents session list` now displays full 26-character session ULIDs** (#10970): The Rich table diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 51815111f..382d4fe35 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -32,5 +32,5 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed comprehensive milestone documentation for v3.6.0 (Advanced Concepts & Deferred Features) and v3.7.0 (TUI Implementation) (PR #9903): split into sub-documents covering context strategies, LLM backends, resource types, A2A rename, container tool execution, scope chain resolution, cost/safety budgets, E2E workflow tests, code review examples, plugin architecture, TUI layout, persona system, reference/command input, session management, configuration, and TuiMaterializer integration. * HAL 9000 has contributed the LLMTraceRepository data-integrity fix (PR #8185 / issue #7505): replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a dual-path implementation that respects the UnitOfWork pattern — flushing only when an external session is provided, and flushing + committing + closing when operating standalone. This eliminates premature transaction commits, loss of rollback capability, and a docstring/implementation mismatch. * HAL 9000 has contributed the ACMS Index Data Model and File Traversal Engine (PR #9664 / issue #9579): foundational data structures for indexed context entries with hot/warm/cold/archive storage tier classification, tag system, and a timeout-safe chunked file traversal engine for large projects with 10,000+ files. - -* HAL 9000 has contributed the error-suppression removal fix (PR #9247 / issue #9060): removed both `try...except Exception:` blocks in `register_registry_agents()` that silently suppressed errors from `actor_registry.list_actors()` and the route bridge refresh, enabling exceptions to propagate per CONTRIBUTING.md fail-fast policy. Added three Behave scenarios verifying RuntimeError, AttributeError, and TypeError propagation. \ No newline at end of file +* HAL 9000 has contributed the error-suppression removal fix (PR #9247 / issue #9060): removed both `try...except Exception:` blocks in `register_registry_agents()` that silently suppressed errors from `actor_registry.list_actors()` and the route bridge refresh, enabling exceptions to propagate per CONTRIBUTING.md fail-fast policy. Added three Behave scenarios verifying RuntimeError, AttributeError, and TypeError propagation. +* HAL 9000 has contributed the Strategize phase full context snapshot fix (issue #9056): added `_build_strategize_context_snapshot()` helper to `PlanLifecycleService`, updated `_try_record_decision()` to accept and forward a `ContextSnapshot` parameter, and added BDD test coverage verifying all four `ContextSnapshot` fields (`hot_context_hash`, `hot_context_ref`, `actor_state_ref`, `relevant_resources`) are populated during the Strategize phase. diff --git a/features/decision_recording.feature b/features/decision_recording.feature index b984862b8..32ba25492 100644 --- a/features/decision_recording.feature +++ b/features/decision_recording.feature @@ -401,3 +401,32 @@ Feature: Decision recording and snapshot store And I record a strategy_choice decision for plan "P1" with question "After restart" Then the dsvc decision sequence number should be 2 And the dsvc next sequence for plan "P1" should be 3 + + # --- Full context snapshot (issue #9056) --- + + Scenario: Strategize phase records decisions with full context snapshots + Given a plan lifecycle service with decision service wired + And an action "local/test-action" for strategize snapshot test + And a plan created from "local/test-action" with project "proj-snapshot" + When I start strategize for the snapshot test plan + Then the strategize decision should have a non-empty hot_context_hash + And the strategize decision should have a non-empty hot_context_ref + And the strategize decision hot_context_ref should start with "plan:" + And the strategize decision should have a non-empty actor_state_ref + And the strategize decision should have relevant_resources populated + + Scenario: Strategize context snapshot hash is content-addressable + Given a plan lifecycle service with decision service wired + And an action "local/test-action-hash" for strategize snapshot test + And a plan created from "local/test-action-hash" with project "proj-hash" + When I start strategize for the snapshot test plan + Then the strategize decision hot_context_hash should start with "sha256:" + And the strategize decision hot_context_hash should be 71 characters long + + Scenario: Strategize context snapshot without projects has empty relevant_resources + Given a plan lifecycle service with decision service wired + And an action "local/no-project-action" for strategize snapshot test + And a plan created from "local/no-project-action" without projects + When I start strategize for the snapshot test plan + Then the strategize decision should have a non-empty hot_context_hash + And the strategize decision should have empty relevant_resources diff --git a/features/steps/decision_recording_steps.py b/features/steps/decision_recording_steps.py index da55104c8..0819af57e 100644 --- a/features/steps/decision_recording_steps.py +++ b/features/steps/decision_recording_steps.py @@ -1105,3 +1105,142 @@ def step_try_store_duplicate(context: Context) -> None: def step_duplicate_error_raised(context: Context) -> None: assert context.decision_error is not None assert isinstance(context.decision_error, DuplicateDecisionError) + + +# --------------------------------------------------------------------------- +# Full context snapshot steps (issue #9056) +# --------------------------------------------------------------------------- + + +@given("a plan lifecycle service with decision service wired") +def step_lifecycle_with_decision_service(context): + """Create a PlanLifecycleService with a real DecisionService wired in.""" + from cleveragents.application.services.decision_service import DecisionService + from cleveragents.application.services.plan_lifecycle_service import ( + PlanLifecycleService, + ) + from cleveragents.config.settings import Settings + + Settings._instance = None + settings = Settings() + context.snapshot_decision_service = DecisionService() + context.snapshot_lifecycle_service = PlanLifecycleService( + settings=settings, + decision_service=context.snapshot_decision_service, + ) + context.snapshot_plan = None + context.snapshot_decision = None + context.snapshot_action_name = None + + +@given('an action "{action_name}" for strategize snapshot test') +def step_create_action_for_snapshot_test(context, action_name): + """Create an action for the strategize snapshot test.""" + context.snapshot_action_name = action_name + context.snapshot_lifecycle_service.create_action( + name=action_name, + description=f"Action {action_name} for snapshot test", + definition_of_done="Snapshot test done", + strategy_actor="openai/gpt-4", + execution_actor="openai/gpt-4", + ) + + +@given('a plan created from "{action_name}" with project "{project_name}"') +def step_create_plan_with_project(context, action_name, project_name): + """Create a plan from the given action with a project link.""" + from cleveragents.domain.models.core.plan import ProjectLink + + context.snapshot_plan = context.snapshot_lifecycle_service.use_action( + action_name=action_name, + project_links=[ProjectLink(project_name=project_name)], + ) + + +@given('a plan created from "{action_name}" without projects') +def step_create_plan_without_projects(context, action_name): + """Create a plan from the given action without any project links.""" + context.snapshot_plan = context.snapshot_lifecycle_service.use_action( + action_name=action_name, + project_links=[], + ) + + +@when("I start strategize for the snapshot test plan") +def step_start_strategize_snapshot_test(context): + """Start strategize and capture the recorded decision.""" + plan_id = context.snapshot_plan.identity.plan_id + context.snapshot_lifecycle_service.start_strategize(plan_id) + + decisions = context.snapshot_decision_service.list_decisions(plan_id) + assert len(decisions) >= 1, f"Expected at least 1 decision, got {len(decisions)}" + strategy_decisions = [ + d for d in decisions if d.decision_type.value == "strategy_choice" + ] + assert len(strategy_decisions) >= 1, "Expected at least 1 strategy_choice decision" + context.snapshot_decision = strategy_decisions[0] + + +@then("the strategize decision should have a non-empty hot_context_hash") +def step_check_snapshot_hash_not_empty(context): + """Verify the context snapshot hash is not empty.""" + snapshot = context.snapshot_decision.context_snapshot + assert snapshot.hot_context_hash, "hot_context_hash should not be empty" + + +@then("the strategize decision should have a non-empty hot_context_ref") +def step_check_snapshot_ref_not_empty(context): + """Verify the context snapshot ref is not empty.""" + snapshot = context.snapshot_decision.context_snapshot + assert snapshot.hot_context_ref, "hot_context_ref should not be empty" + + +@then('the strategize decision hot_context_ref should start with "{prefix}"') +def step_check_snapshot_ref_prefix(context, prefix): + """Verify the context snapshot ref starts with the expected prefix.""" + snapshot = context.snapshot_decision.context_snapshot + assert snapshot.hot_context_ref.startswith(prefix), ( + f"hot_context_ref should start with {prefix!r}" + ) + + +@then("the strategize decision should have a non-empty actor_state_ref") +def step_check_snapshot_actor_ref_not_empty(context): + """Verify the actor_state_ref is not empty.""" + snapshot = context.snapshot_decision.context_snapshot + assert snapshot.actor_state_ref, "actor_state_ref should not be empty" + + +@then("the strategize decision should have relevant_resources populated") +def step_check_snapshot_resources_populated(context): + """Verify relevant_resources is not empty.""" + snapshot = context.snapshot_decision.context_snapshot + assert len(snapshot.relevant_resources) > 0, "relevant_resources should not be empty" + + +@then('the strategize decision hot_context_hash should start with "{prefix}"') +def step_check_snapshot_hash_prefix(context, prefix): + """Verify the context snapshot hash starts with the expected prefix.""" + snapshot = context.snapshot_decision.context_snapshot + assert snapshot.hot_context_hash.startswith(prefix), ( + f"hot_context_hash should start with {prefix!r}" + ) + + +@then("the strategize decision hot_context_hash should be {length:d} characters long") +def step_check_snapshot_hash_length(context, length): + """Verify the context snapshot hash has the expected length.""" + snapshot = context.snapshot_decision.context_snapshot + actual_length = len(snapshot.hot_context_hash) + assert actual_length == length, ( + f"hot_context_hash length should be {length}, got {actual_length}" + ) + + +@then("the strategize decision should have empty relevant_resources") +def step_check_snapshot_resources_empty(context): + """Verify relevant_resources is empty.""" + snapshot = context.snapshot_decision.context_snapshot + assert len(snapshot.relevant_resources) == 0, ( + f"relevant_resources should be empty, got {snapshot.relevant_resources}" + ) diff --git a/src/cleveragents/application/services/plan_lifecycle_service.py b/src/cleveragents/application/services/plan_lifecycle_service.py index b3b28aa2d..26fdf939c 100644 --- a/src/cleveragents/application/services/plan_lifecycle_service.py +++ b/src/cleveragents/application/services/plan_lifecycle_service.py @@ -51,6 +51,8 @@ Based on ``docs/specification.md`` and implementation plan Stage A3. from __future__ import annotations +import hashlib +import json from contextlib import suppress from datetime import datetime from typing import TYPE_CHECKING, Any @@ -77,7 +79,7 @@ from cleveragents.domain.models.core.automation_profile import ( BUILTIN_PROFILES, AutomationProfile, ) -from cleveragents.domain.models.core.decision import DecisionType +from cleveragents.domain.models.core.decision import ContextSnapshot, DecisionType from cleveragents.domain.models.core.plan import ( AutomationProfileProvenance, AutomationProfileRef, @@ -265,11 +267,23 @@ class PlanLifecycleService: question: str, chosen_option: str, parent_decision_id: str | None = None, + context_snapshot: ContextSnapshot | None = None, ) -> None: """Record a decision if DecisionService is available. Failures are logged but never propagated — decision recording must not block lifecycle transitions. + + Args: + plan_id: ULID of the plan. + decision_type: Type of decision being recorded. + question: What question was being answered. + chosen_option: The option that was chosen. + parent_decision_id: Optional parent in the decision tree. + context_snapshot: Optional full context snapshot. When + provided, it is forwarded to + :meth:`DecisionService.record_decision` so that the + decision is stored with a complete context snapshot """ if self.decision_service is None: return @@ -281,6 +295,7 @@ class PlanLifecycleService: question=question, chosen_option=chosen_option, parent_decision_id=parent_decision_id, + context_snapshot=context_snapshot, ) except Exception: self._logger.warning( @@ -1398,15 +1413,75 @@ class PlanLifecycleService: self._commit_plan(plan) self._logger.info("Strategize started", plan_id=plan_id) + context_snapshot = self._build_strategize_context_snapshot(plan) self._try_record_decision( plan_id=plan_id, decision_type="strategy_choice", question="Which strategy should the plan follow?", chosen_option=f"Begin strategize phase for plan {plan_id}", + context_snapshot=context_snapshot, ) return plan + def _build_strategize_context_snapshot(self, plan: Plan) -> ContextSnapshot: + """Build a full context snapshot for a Strategize-phase decision. + + Captures the plan description, action name, strategy actor, and + project references as the hot context window. The hash is + computed over the serialised context so that identical context + windows produce the same hash (content-addressable). + + The ``hot_context_ref`` is set to a stable ``plan:`` + URI so that callers can locate the full context via the plan + record. ``relevant_resources`` is populated from the plan's + project links. ``actor_state_ref`` is set to the strategy + actor name when available. + + Per the v3.2.0 acceptance criteria, decisions recorded during + the Strategize phase must include full context snapshots with + all four :class:`ContextSnapshot` fields populated. + + Args: + plan: The plan entering the Strategize phase. + + Returns: + A :class:`ContextSnapshot` with all four fields populated. + """ + from cleveragents.domain.models.core.decision import ResourceRef + + plan_id = plan.identity.plan_id + + # Build the hot context window from plan metadata available at + # the start of the Strategize phase. + hot_context: dict[str, object] = { + "plan_id": plan_id, + "action_name": plan.action_name, + "description": plan.description or "", + "strategy_actor": plan.strategy_actor or "", + "projects": [ + pl.project_name + for pl in plan.project_links + ], + } + context_json = json.dumps(hot_context, sort_keys=True) + context_hash = hashlib.sha256(context_json.encode()).hexdigest() + + # Build resource refs from project links so the snapshot records + # which projects influenced the strategy decision. + relevant_resources = [ + ResourceRef(resource_id=pl.project_name) + for pl in plan.project_links + if pl.project_name + ] + + return ContextSnapshot( + hot_context_hash=f"sha256:{context_hash}", + hot_context_ref=f"plan:{plan_id}", + relevant_resources=relevant_resources, + actor_state_ref=plan.strategy_actor or "", + ) + def complete_strategize(self, plan_id: str) -> Plan: """Complete the Strategize phase. -- 2.52.0 From 241c2602b0361f1c15de87fc144c11dacd0686b1 Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Fri, 8 May 2026 02:40:02 +0000 Subject: [PATCH 2/2] fix(quality-gates): resolve ruff formatting and missing type annotations Fix two lint blockers identified in PR review: 1. Collapsible list comprehension format (ruff RUF015): Collapse the "projects" list comprehension in _build_strategize_context_snapshot() from 3 lines to a single line within the 88-character limit. 2. Missing type annotations on 13 new BDD step functions: All existing step functions use explicit `context: Context` and `-> None` return type annotations per project style. The 13 new step functions added for issue #9056 were missing these declarations, causing lint warning RUF012 (missing type annotations on function arguments). Verified: - nox -s lint passes (ruff check + ruff format --check) - nox -s typecheck passes (pyright 0 errors) - Pre-existing unit_tests/CI timeouts are infrastructure-related ISSUES CLOSED: #9056 --- features/steps/decision_recording_steps.py | 32 +++++++++++-------- .../services/plan_lifecycle_service.py | 5 +-- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/features/steps/decision_recording_steps.py b/features/steps/decision_recording_steps.py index 0819af57e..74f476145 100644 --- a/features/steps/decision_recording_steps.py +++ b/features/steps/decision_recording_steps.py @@ -1113,7 +1113,7 @@ def step_duplicate_error_raised(context: Context) -> None: @given("a plan lifecycle service with decision service wired") -def step_lifecycle_with_decision_service(context): +def step_lifecycle_with_decision_service(context: Context) -> None: """Create a PlanLifecycleService with a real DecisionService wired in.""" from cleveragents.application.services.decision_service import DecisionService from cleveragents.application.services.plan_lifecycle_service import ( @@ -1134,7 +1134,7 @@ def step_lifecycle_with_decision_service(context): @given('an action "{action_name}" for strategize snapshot test') -def step_create_action_for_snapshot_test(context, action_name): +def step_create_action_for_snapshot_test(context: Context, action_name: str) -> None: """Create an action for the strategize snapshot test.""" context.snapshot_action_name = action_name context.snapshot_lifecycle_service.create_action( @@ -1147,7 +1147,9 @@ def step_create_action_for_snapshot_test(context, action_name): @given('a plan created from "{action_name}" with project "{project_name}"') -def step_create_plan_with_project(context, action_name, project_name): +def step_create_plan_with_project( + context: Context, action_name: str, project_name: str +) -> None: """Create a plan from the given action with a project link.""" from cleveragents.domain.models.core.plan import ProjectLink @@ -1158,7 +1160,7 @@ def step_create_plan_with_project(context, action_name, project_name): @given('a plan created from "{action_name}" without projects') -def step_create_plan_without_projects(context, action_name): +def step_create_plan_without_projects(context: Context, action_name: str) -> None: """Create a plan from the given action without any project links.""" context.snapshot_plan = context.snapshot_lifecycle_service.use_action( action_name=action_name, @@ -1167,7 +1169,7 @@ def step_create_plan_without_projects(context, action_name): @when("I start strategize for the snapshot test plan") -def step_start_strategize_snapshot_test(context): +def step_start_strategize_snapshot_test(context: Context) -> None: """Start strategize and capture the recorded decision.""" plan_id = context.snapshot_plan.identity.plan_id context.snapshot_lifecycle_service.start_strategize(plan_id) @@ -1182,21 +1184,21 @@ def step_start_strategize_snapshot_test(context): @then("the strategize decision should have a non-empty hot_context_hash") -def step_check_snapshot_hash_not_empty(context): +def step_check_snapshot_hash_not_empty(context: Context) -> None: """Verify the context snapshot hash is not empty.""" snapshot = context.snapshot_decision.context_snapshot assert snapshot.hot_context_hash, "hot_context_hash should not be empty" @then("the strategize decision should have a non-empty hot_context_ref") -def step_check_snapshot_ref_not_empty(context): +def step_check_snapshot_ref_not_empty(context: Context) -> None: """Verify the context snapshot ref is not empty.""" snapshot = context.snapshot_decision.context_snapshot assert snapshot.hot_context_ref, "hot_context_ref should not be empty" @then('the strategize decision hot_context_ref should start with "{prefix}"') -def step_check_snapshot_ref_prefix(context, prefix): +def step_check_snapshot_ref_prefix(context: Context, prefix: str) -> None: """Verify the context snapshot ref starts with the expected prefix.""" snapshot = context.snapshot_decision.context_snapshot assert snapshot.hot_context_ref.startswith(prefix), ( @@ -1205,21 +1207,23 @@ def step_check_snapshot_ref_prefix(context, prefix): @then("the strategize decision should have a non-empty actor_state_ref") -def step_check_snapshot_actor_ref_not_empty(context): +def step_check_snapshot_actor_ref_not_empty(context: Context) -> None: """Verify the actor_state_ref is not empty.""" snapshot = context.snapshot_decision.context_snapshot assert snapshot.actor_state_ref, "actor_state_ref should not be empty" @then("the strategize decision should have relevant_resources populated") -def step_check_snapshot_resources_populated(context): +def step_check_snapshot_resources_populated(context: Context) -> None: """Verify relevant_resources is not empty.""" snapshot = context.snapshot_decision.context_snapshot - assert len(snapshot.relevant_resources) > 0, "relevant_resources should not be empty" + assert len(snapshot.relevant_resources) > 0, ( + "relevant_resources should not be empty" + ) @then('the strategize decision hot_context_hash should start with "{prefix}"') -def step_check_snapshot_hash_prefix(context, prefix): +def step_check_snapshot_hash_prefix(context: Context, prefix: str) -> None: """Verify the context snapshot hash starts with the expected prefix.""" snapshot = context.snapshot_decision.context_snapshot assert snapshot.hot_context_hash.startswith(prefix), ( @@ -1228,7 +1232,7 @@ def step_check_snapshot_hash_prefix(context, prefix): @then("the strategize decision hot_context_hash should be {length:d} characters long") -def step_check_snapshot_hash_length(context, length): +def step_check_snapshot_hash_length(context: Context, length: int) -> None: """Verify the context snapshot hash has the expected length.""" snapshot = context.snapshot_decision.context_snapshot actual_length = len(snapshot.hot_context_hash) @@ -1238,7 +1242,7 @@ def step_check_snapshot_hash_length(context, length): @then("the strategize decision should have empty relevant_resources") -def step_check_snapshot_resources_empty(context): +def step_check_snapshot_resources_empty(context: Context) -> None: """Verify relevant_resources is empty.""" snapshot = context.snapshot_decision.context_snapshot assert len(snapshot.relevant_resources) == 0, ( diff --git a/src/cleveragents/application/services/plan_lifecycle_service.py b/src/cleveragents/application/services/plan_lifecycle_service.py index 26fdf939c..6d3730625 100644 --- a/src/cleveragents/application/services/plan_lifecycle_service.py +++ b/src/cleveragents/application/services/plan_lifecycle_service.py @@ -1459,10 +1459,7 @@ class PlanLifecycleService: "action_name": plan.action_name, "description": plan.description or "", "strategy_actor": plan.strategy_actor or "", - "projects": [ - pl.project_name - for pl in plan.project_links - ], + "projects": [pl.project_name for pl in plan.project_links], } context_json = json.dumps(hot_context, sort_keys=True) context_hash = hashlib.sha256(context_json.encode()).hexdigest() -- 2.52.0