BUG-HUNT: [spec-alignment] Default value of `effective_profile_snapshot` violates spec intent #1275

New issue

Open

opened 2026-04-02 08:23:10 +00:00 by freemo · 0 comments

freemo commented

2026-04-02 08:23:10 +00:00

Owner

Bug Report: [spec-alignment] — Default value of `effective_profile_snapshot` violates spec intent

Severity Assessment

Impact: If developers forget to explicitly set effective_profile_snapshot when creating a Plan, the plan will be persisted with an empty profile snapshot ({}), violating the auditability and reproducibility requirements of the specification.
Likelihood: Medium. The code comment indicates this is a known issue, but it's still a pitfall for developers.
Priority: Low

Location

File: src/cleveragents/domain/models/core/plan.py
Function/Class: Plan
Lines: 739-744

Description

The effective_profile_snapshot field on the Plan model has a default value of "{}". The comment on this field explicitly states that this default is for backward compatibility and that new plans should have this field explicitly set to a JSON snapshot of the automation profile. This creates a situation where the domain model allows a state that is considered invalid by the specification's intent.

Evidence

# Relevant code snippet showing the issue
class Plan(BaseModel):
    # ...
    # NOTE: The default '{}' exists for backward compatibility with code
    # paths that create Plan objects before the snapshot is populated.
    # New plans SHOULD explicitly set this field to the resolved profile
    # JSON at creation time; the empty default does not satisfy the spec
    # intent of capturing a frozen profile for audit purposes.
    effective_profile_snapshot: str = Field(
        default="{}",
        description=(
            "Frozen JSON snapshot of the automation profile at plan creation time"
        ),
    )

Expected Behavior

The domain model should enforce the specification's intent as much as possible. The effective_profile_snapshot field should not have a default value that represents an invalid state for new plans. Ideally, it should be a required field, or the default should be handled in a way that doesn't allow for invalid data to be persisted.

Actual Behavior

The Plan model can be instantiated without providing a value for effective_profile_snapshot, and it will default to "{}". This allows for the creation of Plan objects that do not have the required audit information.

Suggested Fix

There are a few options to fix this:

Make the field required: Remove the default value. This would force all code paths that create Plan objects to provide a value for this field. This is the most robust solution but may require more refactoring.
Use a sentinel value: Change the default to None and add a validator that ensures that the field is not None when the plan is in a state where the profile should have been resolved.
Improve documentation: At a minimum, the warning in the code comment should be elevated to a more prominent place in the developer documentation.

Metadata

Branch: fix/spec-alignment-plan-effective-profile-snapshot
Commit Message: fix(spec-alignment): enforce non-empty effective_profile_snapshot on Plan creation
Milestone: v3.3.0
Parent Epic: #362

Subtasks

Write a TDD issue-capture Behave scenario tagged @tdd_issue, @tdd_expected_fail that asserts instantiating a Plan without an explicit effective_profile_snapshot raises a validation error (or equivalent enforcement)
Evaluate the three fix options (required field, sentinel None + validator, or documentation-only) and select the most appropriate approach in consultation with the team
Implement the chosen fix in src/cleveragents/domain/models/core/plan.py (lines 739–744)
Update all call sites that construct Plan objects to explicitly provide a valid effective_profile_snapshot value
Remove the @tdd_expected_fail tag from the TDD scenario once the fix is in place and verify it passes
Run nox -e typecheck to confirm no type regressions
Run nox -e unit_tests to confirm all Behave tests pass
Run nox -e coverage_report to confirm coverage remains ≥ 97%

Definition of Done

The @tdd_expected_fail TDD capture scenario is committed first and demonstrates the invalid-default pitfall
The chosen enforcement mechanism is implemented in Plan.effective_profile_snapshot (lines 739–744 of plan.py)
The TDD scenario passes without @tdd_expected_fail after the fix
All call sites that construct Plan objects explicitly provide a valid effective_profile_snapshot
No regressions in existing plan-related tests
All nox stages pass (nox -e lint, nox -e typecheck, nox -e unit_tests, nox -e integration_tests, nox -e coverage_report)
Coverage >= 97%
Commit is on branch fix/spec-alignment-plan-effective-profile-snapshot with message fix(spec-alignment): enforce non-empty effective_profile_snapshot on Plan creation
PR is merged and this issue is closed

## Bug Report: [spec-alignment] — Default value of `effective_profile_snapshot` violates spec intent ### Severity Assessment - **Impact**: If developers forget to explicitly set `effective_profile_snapshot` when creating a `Plan`, the plan will be persisted with an empty profile snapshot (`{}`), violating the auditability and reproducibility requirements of the specification. - **Likelihood**: Medium. The code comment indicates this is a known issue, but it's still a pitfall for developers. - **Priority**: Low ### Location - **File**: `src/cleveragents/domain/models/core/plan.py` - **Function/Class**: `Plan` - **Lines**: 739-744 ### Description The `effective_profile_snapshot` field on the `Plan` model has a default value of `"{}"`. The comment on this field explicitly states that this default is for backward compatibility and that new plans *should* have this field explicitly set to a JSON snapshot of the automation profile. This creates a situation where the domain model allows a state that is considered invalid by the specification's intent. ### Evidence ```python # Relevant code snippet showing the issue class Plan(BaseModel): # ... # NOTE: The default '{}' exists for backward compatibility with code # paths that create Plan objects before the snapshot is populated. # New plans SHOULD explicitly set this field to the resolved profile # JSON at creation time; the empty default does not satisfy the spec # intent of capturing a frozen profile for audit purposes. effective_profile_snapshot: str = Field( default="{}", description=( "Frozen JSON snapshot of the automation profile at plan creation time" ), ) ``` ### Expected Behavior The domain model should enforce the specification's intent as much as possible. The `effective_profile_snapshot` field should not have a default value that represents an invalid state for new plans. Ideally, it should be a required field, or the default should be handled in a way that doesn't allow for invalid data to be persisted. ### Actual Behavior The `Plan` model can be instantiated without providing a value for `effective_profile_snapshot`, and it will default to `"{}"`. This allows for the creation of `Plan` objects that do not have the required audit information. ### Suggested Fix There are a few options to fix this: 1. **Make the field required**: Remove the `default` value. This would force all code paths that create `Plan` objects to provide a value for this field. This is the most robust solution but may require more refactoring. 2. **Use a sentinel value**: Change the default to `None` and add a validator that ensures that the field is not `None` when the plan is in a state where the profile should have been resolved. 3. **Improve documentation**: At a minimum, the warning in the code comment should be elevated to a more prominent place in the developer documentation. ### Category spec-alignment --- ## Metadata - **Branch**: `fix/spec-alignment-plan-effective-profile-snapshot` - **Commit Message**: `fix(spec-alignment): enforce non-empty effective_profile_snapshot on Plan creation` - **Milestone**: v3.3.0 - **Parent Epic**: #362 ## Subtasks - [ ] Write a TDD issue-capture Behave scenario tagged `@tdd_issue`, `@tdd_expected_fail` that asserts instantiating a `Plan` without an explicit `effective_profile_snapshot` raises a validation error (or equivalent enforcement) - [ ] Evaluate the three fix options (required field, sentinel `None` + validator, or documentation-only) and select the most appropriate approach in consultation with the team - [ ] Implement the chosen fix in `src/cleveragents/domain/models/core/plan.py` (lines 739–744) - [ ] Update all call sites that construct `Plan` objects to explicitly provide a valid `effective_profile_snapshot` value - [ ] Remove the `@tdd_expected_fail` tag from the TDD scenario once the fix is in place and verify it passes - [ ] Run `nox -e typecheck` to confirm no type regressions - [ ] Run `nox -e unit_tests` to confirm all Behave tests pass - [ ] Run `nox -e coverage_report` to confirm coverage remains ≥ 97% ## Definition of Done - [ ] The `@tdd_expected_fail` TDD capture scenario is committed first and demonstrates the invalid-default pitfall - [ ] The chosen enforcement mechanism is implemented in `Plan.effective_profile_snapshot` (lines 739–744 of `plan.py`) - [ ] The TDD scenario passes without `@tdd_expected_fail` after the fix - [ ] All call sites that construct `Plan` objects explicitly provide a valid `effective_profile_snapshot` - [ ] No regressions in existing plan-related tests - [ ] All nox stages pass (`nox -e lint`, `nox -e typecheck`, `nox -e unit_tests`, `nox -e integration_tests`, `nox -e coverage_report`) - [ ] Coverage >= 97% - [ ] Commit is on branch `fix/spec-alignment-plan-effective-profile-snapshot` with message `fix(spec-alignment): enforce non-empty effective_profile_snapshot on Plan creation` - [ ] PR is merged and this issue is closed

freemo added the

labels

2026-04-02 08:23:48 +00:00

freemo added this to the v3.3.0 milestone