feat(plan): enforce decision type phase-gating at recording time #973

Merged

hurui200320 merged 2 commits from feature/m4-decision-phase-gating into master 2026-03-19 07:53:44 +00:00

Conversation 7 Commits 2 Files changed 12 +1140 -14

hurui200320 commented 2026-03-16 08:12:06 +00:00

Member

Copy link

Summary

Adds phase-gating validation to DecisionService.record_decision() that enforces the specification's constraint: certain decision types are only valid during specific plan phases. This prevents invalid decisions (e.g., tool_invocation during Strategize, strategy_choice during Execute) from being persisted.

Changes

• Exception (cleveragents.core.exceptions): Added DecisionPhaseViolationError(BusinessRuleViolation) with decision_type, plan_phase, and allowed_types attributes.
• Phase constants (cleveragents.domain.models.core.decision):
  • resource_selection added to EXECUTE_TYPES — now phase-agnostic (Strategize or Execute) per ADR-007 L72 and ADR-033 L74.
  • subplan_spawn / subplan_parallel_spawn in both sets; code comment documents divergence from ADRs per M4 subplan model (ticket #931).
  • USER_INTERVENTION remains phase-agnostic (both sets).
  • Module-level docstring table updated to match actual assignments.
  • is_any_phase_type property updated to check membership in both sets dynamically (was hardcoded to USER_INTERVENTION only).
• Phase-gating module (cleveragents.application.services.phase_gating):
  • Extracted from DecisionService to reduce decision_service.py line count (1010 → 913) and isolate the phase-gating concern.
  • PHASE_ALLOWED_TYPES typed as Mapping[PlanPhase, frozenset[DecisionType]].
  • resolve_plan_phase() helper: supports explicit parameter, DB lookup, and graceful skip.
  • validate_phase_gating() enforcement raises DecisionPhaseViolationError.
  • Exception narrowing: DB lookup catches (DatabaseError, OperationalError, OSError) instead of bare except Exception — only absorbs infrastructure failures, not programming errors.
  • # TODO(pg-migration): marker on TOCTOU race documentation for future PostgreSQL migration.
• Decision service (cleveragents.application.services.decision_service):
  • Added plan_phase parameter to record_decision().
  • Invalid plan_phase string now raises ValidationError (was uncaught ValueError).
  • Imports and delegates to phase_gating module for all phase-gating logic.
  • PHASE_ALLOWED_TYPES re-exported in __all__ for backward compatibility.
• CHANGELOG: Added behavioral change entry for resource_selection reclassification.
• Backward compatibility: Phase-gating is opt-in — when neither plan_phase is provided nor a UnitOfWork is wired, validation is skipped, preserving all existing callers.
• Unrelated drive-by reverted: Removed ULID_PATTERN from decision.py __all__ (was an unrelated export addition).
• Tests:
  • 38 Behave scenarios covering valid/invalid types per phase, phase-agnostic acceptance, DB-based resolution (Strategize and Execute plans), unknown plan in DB, PlanPhase enum pass-through, error attributes, ungated phases, invalid plan_phase string validation, and DB error resilience in resolve_plan_phase.
  • 11 new Behave scenarios for is_any_phase_type: 4 dual-phase types (true) + 7 single-phase types (false), including prompt_definition root test.
  • 6 Robot Framework integration tests with stderr assertions.
  • Updated consolidated_decision.feature for new EXECUTE_TYPES member count (8 members).
  • Test cleanup now calls uow.engine.dispose() before file deletion.
  • tempfile.mktemp() replaced with tempfile.mkstemp().
  • Inline imports moved to module top-level per CONTRIBUTING.md.
  • Flaky concurrency test timing increased in subplan_execution_steps.py.

Review Round 1 + 2 Fixes

#	Finding	Resolution
P1-1	except Exception too broad in _resolve_plan_phase	Narrowed to (DatabaseError, OperationalError, OSError) — matches codebase pattern
P2-2	decision_service.py at 1010 lines	Extracted to phase_gating.py module (1010 → 913 lines)
P2-3	TOCTOU race — no programmatic guard	Added # TODO(pg-migration): marker with actionable guidance
P2-4	resource_selection reclassification needs CHANGELOG	Added CHANGELOG entry documenting behavioral change
P2-7	is_any_phase_type BDD gap for dual-phase types	Added 11 parametrized scenarios covering all 4 dual-phase + 7 single-phase types
P3-5	ULID_PATTERN export is unrelated drive-by	Reverted — removed from decision.py __all__
P3-6	decision.py at 514 lines (now 513)	No action — reviewer accepted as marginally over

Coverage Fix (Round 3)

Coverage was at 96.9446% (displayed as 97% but rounded to 96.9% at 1-decimal precision, failing the ≥97% threshold). Added 2 new Behave scenarios to cover previously untested paths in phase_gating.py:

Scenario	Lines Covered	Description
Invalid plan_phase string raises ValidationError	Lines 72-75	Tests that passing an invalid string like "not_a_real_phase" to plan_phase raises ValidationError with the invalid value in the message
resolve_plan_phase gracefully handles database errors	Lines 101, 106-109	Tests that resolve_plan_phase() catches DatabaseError from a corrupted SQLite DB and returns None (skip gating) instead of propagating

Coverage moved from 96.9446% → 96.9587%, which rounds to 97.0% and passes the threshold.

Quality Gates

Session	Result
lint	PASS
typecheck	PASS (0 errors)
unit_tests	PASS (11,250 scenarios, 0 failures)
integration_tests	PASS (1,563 tests, 0 failures)
e2e_tests	PASS (16 tests, 0 failures)
coverage_report	PASS (97.0%, threshold: 97%)

Closes #931

## Summary Adds phase-gating validation to `DecisionService.record_decision()` that enforces the specification's constraint: certain decision types are only valid during specific plan phases. This prevents invalid decisions (e.g., `tool_invocation` during Strategize, `strategy_choice` during Execute) from being persisted. ### Changes - **Exception** (`cleveragents.core.exceptions`): Added `DecisionPhaseViolationError(BusinessRuleViolation)` with `decision_type`, `plan_phase`, and `allowed_types` attributes. - **Phase constants** (`cleveragents.domain.models.core.decision`): - `resource_selection` added to `EXECUTE_TYPES` — now phase-agnostic (Strategize or Execute) per ADR-007 L72 and ADR-033 L74. - `subplan_spawn` / `subplan_parallel_spawn` in both sets; code comment documents divergence from ADRs per M4 subplan model (ticket #931). - `USER_INTERVENTION` remains phase-agnostic (both sets). - Module-level docstring table updated to match actual assignments. - `is_any_phase_type` property updated to check membership in both sets dynamically (was hardcoded to `USER_INTERVENTION` only). - **Phase-gating module** (`cleveragents.application.services.phase_gating`): - Extracted from `DecisionService` to reduce `decision_service.py` line count (1010 → 913) and isolate the phase-gating concern. - `PHASE_ALLOWED_TYPES` typed as `Mapping[PlanPhase, frozenset[DecisionType]]`. - `resolve_plan_phase()` helper: supports explicit parameter, DB lookup, and graceful skip. - `validate_phase_gating()` enforcement raises `DecisionPhaseViolationError`. - Exception narrowing: DB lookup catches `(DatabaseError, OperationalError, OSError)` instead of bare `except Exception` — only absorbs infrastructure failures, not programming errors. - `# TODO(pg-migration):` marker on TOCTOU race documentation for future PostgreSQL migration. - **Decision service** (`cleveragents.application.services.decision_service`): - Added `plan_phase` parameter to `record_decision()`. - Invalid `plan_phase` string now raises `ValidationError` (was uncaught `ValueError`). - Imports and delegates to `phase_gating` module for all phase-gating logic. - `PHASE_ALLOWED_TYPES` re-exported in `__all__` for backward compatibility. - **CHANGELOG**: Added behavioral change entry for `resource_selection` reclassification. - **Backward compatibility**: Phase-gating is opt-in — when neither `plan_phase` is provided nor a UnitOfWork is wired, validation is skipped, preserving all existing callers. - **Unrelated drive-by reverted**: Removed `ULID_PATTERN` from `decision.py` `__all__` (was an unrelated export addition). - **Tests**: - 38 Behave scenarios covering valid/invalid types per phase, phase-agnostic acceptance, DB-based resolution (Strategize and Execute plans), unknown plan in DB, PlanPhase enum pass-through, error attributes, ungated phases, invalid plan_phase string validation, and DB error resilience in `resolve_plan_phase`. - 11 new Behave scenarios for `is_any_phase_type`: 4 dual-phase types (true) + 7 single-phase types (false), including `prompt_definition` root test. - 6 Robot Framework integration tests with stderr assertions. - Updated `consolidated_decision.feature` for new `EXECUTE_TYPES` member count (8 members). - Test cleanup now calls `uow.engine.dispose()` before file deletion. - `tempfile.mktemp()` replaced with `tempfile.mkstemp()`. - Inline imports moved to module top-level per CONTRIBUTING.md. - Flaky concurrency test timing increased in `subplan_execution_steps.py`. ### Review Round 1 + 2 Fixes | # | Finding | Resolution | |---|---------|------------| | P1-1 | `except Exception` too broad in `_resolve_plan_phase` | Narrowed to `(DatabaseError, OperationalError, OSError)` — matches codebase pattern | | P2-2 | `decision_service.py` at 1010 lines | Extracted to `phase_gating.py` module (1010 → 913 lines) | | P2-3 | TOCTOU race — no programmatic guard | Added `# TODO(pg-migration):` marker with actionable guidance | | P2-4 | `resource_selection` reclassification needs CHANGELOG | Added CHANGELOG entry documenting behavioral change | | P2-7 | `is_any_phase_type` BDD gap for dual-phase types | Added 11 parametrized scenarios covering all 4 dual-phase + 7 single-phase types | | P3-5 | `ULID_PATTERN` export is unrelated drive-by | Reverted — removed from `decision.py` `__all__` | | P3-6 | `decision.py` at 514 lines (now 513) | No action — reviewer accepted as marginally over | ### Coverage Fix (Round 3) Coverage was at 96.9446% (displayed as 97% but rounded to 96.9% at 1-decimal precision, failing the ≥97% threshold). Added 2 new Behave scenarios to cover previously untested paths in `phase_gating.py`: | Scenario | Lines Covered | Description | |----------|---------------|-------------| | Invalid plan_phase string raises ValidationError | Lines 72-75 | Tests that passing an invalid string like `"not_a_real_phase"` to `plan_phase` raises `ValidationError` with the invalid value in the message | | resolve_plan_phase gracefully handles database errors | Lines 101, 106-109 | Tests that `resolve_plan_phase()` catches `DatabaseError` from a corrupted SQLite DB and returns `None` (skip gating) instead of propagating | Coverage moved from 96.9446% → 96.9587%, which rounds to 97.0% and passes the threshold. ### Quality Gates | Session | Result | |---------|--------| | lint | PASS | | typecheck | PASS (0 errors) | | unit_tests | PASS (11,250 scenarios, 0 failures) | | integration_tests | PASS (1,563 tests, 0 failures) | | e2e_tests | PASS (16 tests, 0 failures) | | coverage_report | PASS (97.0%, threshold: 97%) | Closes #931

hurui200320 added this to the v3.3.0 milestone 2026-03-16 08:12:14 +00:00

hurui200320 added the

Type

Feature

label 2026-03-16 08:12:15 +00:00

freemo reviewed 2026-03-16 16:14:08 +00:00

freemo left a comment

Copy link

PM Day 36 Triage: Decision phase-gating implementation. Closes #931. M4 scope. Reviewer needed: @freemo (decision framework expert). Verify alignment with ADR-007 decision tree and ADR-033 decision recording protocol.

PM Day 36 Triage: Decision phase-gating implementation. Closes #931. M4 scope. Reviewer needed: @freemo (decision framework expert). Verify alignment with ADR-007 decision tree and ADR-033 decision recording protocol.

hurui200320 force-pushed feature/m4-decision-phase-gating from 031339fe06 to 20b6f0d5b9 2026-03-17 07:27:04 +00:00 Compare

hurui200320 commented 2026-03-17 08:04:00 +00:00

Author

Member

Copy link

Self-QA Review — ✅ Approved

Iterations: 2 review/fix cycles
Final verdict: Approve

---

Cycle 1: 16 issues found → 16 fixed

The initial review identified 1 critical, 4 major, 6 minor, and 4 nit issues. The critical finding was that resource_selection was incorrectly excluded from EXECUTE_TYPES, violating ADR-007, ADR-033, and the specification. All 16 issues were fixed in a single amended commit.

Key fixes applied:

• Critical: Added DecisionType.RESOURCE_SELECTION to EXECUTE_TYPES; updated BDD scenarios and consolidated feature member counts
• Major: Documented subplan_spawn/subplan_parallel_spawn ADR divergence with ticket reference; added TOCTOU race condition comment; added 2 missing DB-resolution test scenarios (empty DB + Execute phase)
• Minor: Fixed is_any_phase_type to use dynamic set membership; wrapped PlanPhase() in try/except for ValidationError; added DB error resilience; updated stale docstring table; added engine disposal in test cleanup; added PlanPhase enum direct-pass test
• Nits: Replaced deprecated tempfile.mktemp(); changed to Mapping type hint; moved imports to top-level; added stderr checks in Robot tests

Cycle 2: 0 critical/major issues — Approved

All 16 previous fixes verified correct. 8 minor style/coverage gaps and 5 nits remain — all non-blocking (defensive code path coverage, Robot Framework conventions, cosmetic code patterns).

Quality Gates

Session	Result
nox -e lint	✅ PASS
nox -e typecheck	✅ PASS (0 errors)
nox -e unit_tests	✅ PASS (10,851 scenarios)
nox -e integration_tests	✅ PASS (1,517 tests)
nox -e e2e_tests	✅ PASS (4 tests)
nox -e coverage_report	✅ PASS (97%)

Full implementation notes posted on ticket #931.

## Self-QA Review — ✅ Approved **Iterations:** 2 review/fix cycles **Final verdict:** Approve --- ### Cycle 1: 16 issues found → 16 fixed The initial review identified **1 critical, 4 major, 6 minor, and 4 nit** issues. The critical finding was that `resource_selection` was incorrectly excluded from `EXECUTE_TYPES`, violating ADR-007, ADR-033, and the specification. All 16 issues were fixed in a single amended commit. Key fixes applied: - **Critical:** Added `DecisionType.RESOURCE_SELECTION` to `EXECUTE_TYPES`; updated BDD scenarios and consolidated feature member counts - **Major:** Documented `subplan_spawn`/`subplan_parallel_spawn` ADR divergence with ticket reference; added TOCTOU race condition comment; added 2 missing DB-resolution test scenarios (empty DB + Execute phase) - **Minor:** Fixed `is_any_phase_type` to use dynamic set membership; wrapped `PlanPhase()` in try/except for `ValidationError`; added DB error resilience; updated stale docstring table; added engine disposal in test cleanup; added PlanPhase enum direct-pass test - **Nits:** Replaced deprecated `tempfile.mktemp()`; changed to `Mapping` type hint; moved imports to top-level; added stderr checks in Robot tests ### Cycle 2: 0 critical/major issues — Approved All 16 previous fixes verified correct. 8 minor style/coverage gaps and 5 nits remain — all non-blocking (defensive code path coverage, Robot Framework conventions, cosmetic code patterns). ### Quality Gates | Session | Result | |---------|--------| | `nox -e lint` | ✅ PASS | | `nox -e typecheck` | ✅ PASS (0 errors) | | `nox -e unit_tests` | ✅ PASS (10,851 scenarios) | | `nox -e integration_tests` | ✅ PASS (1,517 tests) | | `nox -e e2e_tests` | ✅ PASS (4 tests) | | `nox -e coverage_report` | ✅ PASS (97%) | Full implementation notes posted on [ticket #931](https://git.cleverthis.com/cleveragents/cleveragents-core/issues/931#issuecomment-66057).

hurui200320 force-pushed feature/m4-decision-phase-gating from 20b6f0d5b9 to cb7edf2227 2026-03-17 08:24:47 +00:00 Compare

brent.edwards requested changes 2026-03-17 19:52:59 +00:00

Dismissed

brent.edwards left a comment

Copy link

Code Review — PR #973 feat(plan): enforce decision type phase-gating at recording time

Reviewer: @brent.edwards | Size: L (+802/−9, 8 files) | Focus: Domain invariants, service design, backward compat

---

P1:must-fix (1)

1. except Exception too broad in _resolve_plan_phase
decision_service.py:~807 — The comment says "database errors" but catches everything including TypeError, AttributeError, and other programming errors. The opt-in contract ("don't break if DB is unavailable") should only absorb infrastructure failures. Narrow to (OperationalError, DatabaseError, OSError) — the same pattern PR #971 correctly uses in _build_skill_service.

---

P2:should-fix (3)

2. decision_service.py at 1010 lines — well over the 500-line guideline. _resolve_plan_phase + _validate_phase_gating (~50 lines) could be extracted to a PhaseGatingPolicy class or phase_gating.py module.

3. TOCTOU race is documented but not programmatically guarded — the code assumes SQLite single-writer serialization. If someone switches to PostgreSQL, this assumption silently breaks. Add a # TODO(pg-migration): marker.

4. resource_selection reclassification to both-phases is a semantic breaking change — is_strategize_type and is_execute_type now return True for more types. The ticket #931 rationale is documented in code, but this deserves a CHANGELOG entry as a behavioral change.

---

P3:nit (2)

5. ULID_PATTERN export added to __all__ is an unrelated drive-by fix.
6. decision.py at 514 lines — marginally over guideline, acceptable.

---

Positive Observations

• DecisionPhaseViolationError with structured attributes (decision_type, plan_phase, allowed_types: frozenset) — excellent for programmatic error handling
• Phase-gating is fully opt-in: no plan_phase + no UoW = skip — backward compatible
• PHASE_ALLOWED_TYPES as Mapping[PlanPhase, frozenset[DecisionType]] — immutable, O(1) lookup
• Behave scenario outlines cover all type×phase combinations — thorough
• tempfile.mktemp() → tempfile.mkstemp() and uow.engine.dispose() before file deletion — good cleanup fixes

Verdict: REQUEST_CHANGES — P1-1 is a targeted fix.

## Code Review — PR #973 `feat(plan): enforce decision type phase-gating at recording time` **Reviewer:** @brent.edwards | **Size:** L (+802/−9, 8 files) | **Focus:** Domain invariants, service design, backward compat --- ### P1:must-fix (1) **1. `except Exception` too broad in `_resolve_plan_phase`** `decision_service.py:~807` — The comment says "database errors" but catches everything including `TypeError`, `AttributeError`, and other programming errors. The opt-in contract ("don't break if DB is unavailable") should only absorb infrastructure failures. Narrow to `(OperationalError, DatabaseError, OSError)` — the same pattern PR #971 correctly uses in `_build_skill_service`. --- ### P2:should-fix (3) **2.** `decision_service.py` at 1010 lines — well over the 500-line guideline. `_resolve_plan_phase` + `_validate_phase_gating` (~50 lines) could be extracted to a `PhaseGatingPolicy` class or `phase_gating.py` module. **3.** TOCTOU race is documented but not programmatically guarded — the code assumes SQLite single-writer serialization. If someone switches to PostgreSQL, this assumption silently breaks. Add a `# TODO(pg-migration):` marker. **4.** `resource_selection` reclassification to both-phases is a semantic breaking change — `is_strategize_type` and `is_execute_type` now return `True` for more types. The ticket #931 rationale is documented in code, but this deserves a CHANGELOG entry as a behavioral change. --- ### P3:nit (2) **5.** `ULID_PATTERN` export added to `__all__` is an unrelated drive-by fix. **6.** `decision.py` at 514 lines — marginally over guideline, acceptable. --- ### Positive Observations - `DecisionPhaseViolationError` with structured attributes (`decision_type`, `plan_phase`, `allowed_types: frozenset`) — excellent for programmatic error handling - Phase-gating is fully opt-in: no `plan_phase` + no UoW = skip — backward compatible - `PHASE_ALLOWED_TYPES` as `Mapping[PlanPhase, frozenset[DecisionType]]` — immutable, O(1) lookup - Behave scenario outlines cover all type×phase combinations — thorough - `tempfile.mktemp()` → `tempfile.mkstemp()` and `uow.engine.dispose()` before file deletion — good cleanup fixes **Verdict:** REQUEST_CHANGES — P1-1 is a targeted fix.

brent.edwards requested changes 2026-03-17 20:36:41 +00:00

brent.edwards left a comment

Copy link

Code Review Round 2 — PR #973 feat(plan): enforce decision type phase-gating

Reviewer: @brent.edwards | Focus: Verification of Round 1 fixes + deep second pass

---

Prior Findings: 0 of 6 resolved

All Round 1 findings remain open:

#	Sev	Finding	Status
P1-1	except Exception too broad in _resolve_plan_phase	OPEN — still bare except Exception at line ~807. Comment added explaining intent but catch scope unchanged.
P2-2	decision_service.py at 1010 lines	OPEN
P2-3	TOCTOU race — no programmatic SQLite assertion	OPEN (excellent inline comment added, but still aspirational not enforceable)
P2-4	resource_selection reclassification needs CHANGELOG	OPEN
P3-5	ULID_PATTERN export is unrelated	OPEN
P3-6	decision.py at 514 lines	OPEN

---

New Finding (1)

P2: is_any_phase_type BDD gap for 3 newly dual-phase types
The PR promotes resource_selection, subplan_spawn, and subplan_parallel_spawn from single-phase to both-phase, changing is_any_phase_type from False → True for all three. But the only BDD scenario testing is_any_phase_type uses user_intervention. If someone later removes one of these from a phase set, the regression goes undetected.
Fix: Add a parametrized scenario covering all 4 dual-phase types.

---

Confirmed Clean from Second Pass

Area	Verdict
Phase constants (STRATEGIZE_TYPES ∪ EXECUTE_TYPES)	Correct — all 11 DecisionType members covered, no orphans
record_decision backward compat	Safe — plan_phase is keyword-only, defaults to None, gating skipped when None
DecisionPhaseViolationError upstream handling	Correct — caught by all upstream BusinessRuleViolation handlers (a2a, CLI, error_handling)
is_any_phase_type implementation	Dynamically correct — checks both-set membership for 4 types
Test quality	Good — real DecisionService with real UnitOfWork + SQLite, no mocking of service layer

---

Verdict: REQUEST_CHANGES — P1-1 (except Exception at line ~807) remains the sole merge-blocker. Narrowing to except (OperationalError, DatabaseError, OSError) resolves it. All other findings are P2/P3 and can be tracked as follow-up.

## Code Review Round 2 — PR #973 `feat(plan): enforce decision type phase-gating` **Reviewer:** @brent.edwards | **Focus:** Verification of Round 1 fixes + deep second pass --- ### Prior Findings: 0 of 6 resolved All Round 1 findings remain open: | # | Sev | Finding | Status | |---|-----|---------|--------| | P1-1 | `except Exception` too broad in `_resolve_plan_phase` | **OPEN** — still bare `except Exception` at line ~807. Comment added explaining intent but catch scope unchanged. | | P2-2 | `decision_service.py` at 1010 lines | OPEN | | P2-3 | TOCTOU race — no programmatic SQLite assertion | OPEN (excellent inline comment added, but still aspirational not enforceable) | | P2-4 | `resource_selection` reclassification needs CHANGELOG | OPEN | | P3-5 | `ULID_PATTERN` export is unrelated | OPEN | | P3-6 | `decision.py` at 514 lines | OPEN | --- ### New Finding (1) **P2: `is_any_phase_type` BDD gap for 3 newly dual-phase types** The PR promotes `resource_selection`, `subplan_spawn`, and `subplan_parallel_spawn` from single-phase to both-phase, changing `is_any_phase_type` from `False` → `True` for all three. But the only BDD scenario testing `is_any_phase_type` uses `user_intervention`. If someone later removes one of these from a phase set, the regression goes undetected. **Fix:** Add a parametrized scenario covering all 4 dual-phase types. --- ### Confirmed Clean from Second Pass | Area | Verdict | |------|---------| | Phase constants (STRATEGIZE_TYPES ∪ EXECUTE_TYPES) | Correct — all 11 DecisionType members covered, no orphans | | `record_decision` backward compat | Safe — `plan_phase` is keyword-only, defaults to `None`, gating skipped when `None` | | `DecisionPhaseViolationError` upstream handling | Correct — caught by all upstream `BusinessRuleViolation` handlers (a2a, CLI, error_handling) | | `is_any_phase_type` implementation | Dynamically correct — checks both-set membership for 4 types | | Test quality | Good — real `DecisionService` with real `UnitOfWork` + SQLite, no mocking of service layer | --- **Verdict:** REQUEST_CHANGES — P1-1 (`except Exception` at line ~807) remains the sole merge-blocker. Narrowing to `except (OperationalError, DatabaseError, OSError)` resolves it. All other findings are P2/P3 and can be tracked as follow-up.

hurui200320 force-pushed feature/m4-decision-phase-gating from cb7edf2227 to ef37aa3dfc 2026-03-18 07:22:28 +00:00 Compare

hurui200320 commented 2026-03-18 07:23:26 +00:00

Author

Member

Copy link

Review Fix Summary — Addressing Rounds 1 & 2 (@brent.edwards)

All 7 findings from both review rounds have been addressed. Branch rebased onto latest master and force-pushed.

P1-1 (must-fix): except Exception too broad — ✅ RESOLVED

Narrowed the catch in resolve_plan_phase() from bare except Exception to except (DatabaseError, OperationalError, OSError). This follows the established codebase pattern from repositories.py:

• DatabaseError (from cleveragents.core.exceptions) — catches repository-wrapped DB errors
• OperationalError (from sqlalchemy.exc) — catches raw SQLAlchemy connection/operation errors from the UoW layer
• OSError — catches filesystem-level SQLite access failures

Programming errors (TypeError, AttributeError, etc.) now correctly propagate instead of being silently swallowed.

P2-2 (should-fix): decision_service.py at 1010 lines — ✅ RESOLVED

Extracted phase-gating concern to new module cleveragents.application.services.phase_gating:

• PHASE_ALLOWED_TYPES constant
• resolve_plan_phase() (was _resolve_plan_phase instance method)
• validate_phase_gating() (was _validate_phase_gating static method)

decision_service.py reduced from 1010 → 913 lines. The service imports and delegates to the new module. PHASE_ALLOWED_TYPES is re-exported in decision_service.__all__ for backward compatibility.

P2-3 (should-fix): TOCTOU race — no programmatic guard — ✅ RESOLVED

Added # TODO(pg-migration): marker to the TOCTOU comment with actionable guidance: "At minimum, a single-writer assertion or advisory lock should guard this section under multi-writer engines."

P2-4 (should-fix): resource_selection reclassification needs CHANGELOG — ✅ RESOLVED

Added CHANGELOG entry under ## Unreleased documenting the behavioral change: resource_selection reclassified from Execute-only to phase-agnostic, with impact note for code relying on is_strategize_type/is_execute_type.

P2-7 (new in Round 2): is_any_phase_type BDD gap — ✅ RESOLVED

Added 11 new Behave scenarios to consolidated_decision.feature:

• Scenario Outline: All dual-phase types report is_any_phase_type true — parametrized over resource_selection, subplan_spawn, subplan_parallel_spawn, user_intervention (4 scenarios)
• Scenario Outline: Single-phase types report is_any_phase_type false — parametrized over invariant_enforced, strategy_choice, implementation_choice, tool_invocation, error_recovery, validation_response (6 scenarios)
• prompt_definition tested separately (existing root scenario) with added is_any_phase_type should be false assertion

P3-5 (nit): ULID_PATTERN export is unrelated drive-by — ✅ RESOLVED

Removed ULID_PATTERN from decision.py __all__.

P3-6 (nit): decision.py at 514 lines — No action

Reviewer accepted as marginally over guideline.

Quality Gates

Session	Result
lint	✅ PASS
typecheck	✅ PASS (0 errors)
unit_tests	✅ PASS (11,153 scenarios, 0 failures, 0 errors)
integration_tests	✅ PASS (1,563 tests, 0 failures)
e2e_tests	✅ PASS (16 tests, 0 failures)
coverage_report	✅ PASS (97%)

## Review Fix Summary — Addressing Rounds 1 & 2 (@brent.edwards) All 7 findings from both review rounds have been addressed. Branch rebased onto latest `master` and force-pushed. ### P1-1 (must-fix): `except Exception` too broad — ✅ RESOLVED Narrowed the catch in `resolve_plan_phase()` from bare `except Exception` to `except (DatabaseError, OperationalError, OSError)`. This follows the established codebase pattern from `repositories.py`: - `DatabaseError` (from `cleveragents.core.exceptions`) — catches repository-wrapped DB errors - `OperationalError` (from `sqlalchemy.exc`) — catches raw SQLAlchemy connection/operation errors from the UoW layer - `OSError` — catches filesystem-level SQLite access failures Programming errors (`TypeError`, `AttributeError`, etc.) now correctly propagate instead of being silently swallowed. ### P2-2 (should-fix): `decision_service.py` at 1010 lines — ✅ RESOLVED Extracted phase-gating concern to new module `cleveragents.application.services.phase_gating`: - `PHASE_ALLOWED_TYPES` constant - `resolve_plan_phase()` (was `_resolve_plan_phase` instance method) - `validate_phase_gating()` (was `_validate_phase_gating` static method) `decision_service.py` reduced from 1010 → 913 lines. The service imports and delegates to the new module. `PHASE_ALLOWED_TYPES` is re-exported in `decision_service.__all__` for backward compatibility. ### P2-3 (should-fix): TOCTOU race — no programmatic guard — ✅ RESOLVED Added `# TODO(pg-migration):` marker to the TOCTOU comment with actionable guidance: *"At minimum, a single-writer assertion or advisory lock should guard this section under multi-writer engines."* ### P2-4 (should-fix): `resource_selection` reclassification needs CHANGELOG — ✅ RESOLVED Added CHANGELOG entry under `## Unreleased` documenting the behavioral change: `resource_selection` reclassified from Execute-only to phase-agnostic, with impact note for code relying on `is_strategize_type`/`is_execute_type`. ### P2-7 (new in Round 2): `is_any_phase_type` BDD gap — ✅ RESOLVED Added 11 new Behave scenarios to `consolidated_decision.feature`: - **Scenario Outline: All dual-phase types report is_any_phase_type true** — parametrized over `resource_selection`, `subplan_spawn`, `subplan_parallel_spawn`, `user_intervention` (4 scenarios) - **Scenario Outline: Single-phase types report is_any_phase_type false** — parametrized over `invariant_enforced`, `strategy_choice`, `implementation_choice`, `tool_invocation`, `error_recovery`, `validation_response` (6 scenarios) - **prompt_definition** tested separately (existing root scenario) with added `is_any_phase_type should be false` assertion ### P3-5 (nit): `ULID_PATTERN` export is unrelated drive-by — ✅ RESOLVED Removed `ULID_PATTERN` from `decision.py` `__all__`. ### P3-6 (nit): `decision.py` at 514 lines — No action Reviewer accepted as marginally over guideline. ### Quality Gates | Session | Result | |---------|--------| | lint | ✅ PASS | | typecheck | ✅ PASS (0 errors) | | unit_tests | ✅ PASS (11,153 scenarios, 0 failures, 0 errors) | | integration_tests | ✅ PASS (1,563 tests, 0 failures) | | e2e_tests | ✅ PASS (16 tests, 0 failures) | | coverage_report | ✅ PASS (97%) |

hurui200320 referenced this pull request 2026-03-18 07:23:50 +00:00

feat(plan): enforce decision type phase-gating at recording time #931

hurui200320 force-pushed feature/m4-decision-phase-gating from ef37aa3dfc to 1f016bea33 2026-03-18 07:29:13 +00:00 Compare

hurui200320 force-pushed feature/m4-decision-phase-gating from 1f016bea33 to 35eb7b762a 2026-03-18 08:26:06 +00:00 Compare

freemo approved these changes 2026-03-19 04:56:07 +00:00

Dismissed

freemo left a comment

Copy link

Code Review — PR #973 feat(plan): enforce decision type phase-gating at recording time

Cleanly scoped feature with good architectural separation. The extraction of phase-gating logic into phase_gating.py (148 lines) with clear API boundary (resolve_plan_phase(), validate_phase_gating(), PHASE_ALLOWED_TYPES) is well-done. The opt-in design (gating skipped when neither plan_phase nor UoW is provided) ensures backward compatibility.

The resource_selection reclassification to phase-agnostic is a breaking behavioral change, but it's properly documented in the CHANGELOG with ADR references (ADR-007 L72, ADR-033 L74). The TOCTOU race condition is documented with a clear TODO(pg-migration) marker.

36 Behave scenarios + 6 Robot tests + proper exception hierarchy (DecisionPhaseViolationError(BusinessRuleViolation)) demonstrate thorough implementation.

Approved. No issues found.

## Code Review — PR #973 `feat(plan): enforce decision type phase-gating at recording time` Cleanly scoped feature with good architectural separation. The extraction of phase-gating logic into `phase_gating.py` (148 lines) with clear API boundary (`resolve_plan_phase()`, `validate_phase_gating()`, `PHASE_ALLOWED_TYPES`) is well-done. The opt-in design (gating skipped when neither `plan_phase` nor UoW is provided) ensures backward compatibility. The `resource_selection` reclassification to phase-agnostic is a breaking behavioral change, but it's properly documented in the CHANGELOG with ADR references (ADR-007 L72, ADR-033 L74). The TOCTOU race condition is documented with a clear `TODO(pg-migration)` marker. 36 Behave scenarios + 6 Robot tests + proper exception hierarchy (`DecisionPhaseViolationError(BusinessRuleViolation)`) demonstrate thorough implementation. **Approved.** No issues found.

freemo requested reviews from brent.edwards, freemo 2026-03-19 05:19:32 +00:00

hurui200320 force-pushed feature/m4-decision-phase-gating from 35eb7b762a to 52f1bb2abb 2026-03-19 05:46:57 +00:00 Compare

hurui200320 dismissed freemo's review 2026-03-19 05:46:57 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings

hurui200320 scheduled this pull request to auto merge when all checks succeed 2026-03-19 05:50:58 +00:00

hurui200320 force-pushed feature/m4-decision-phase-gating from 52f1bb2abb to 1ec6b2ac27 2026-03-19 06:56:28 +00:00 Compare

hurui200320 force-pushed feature/m4-decision-phase-gating from 1ec6b2ac27 to 231c3656e0 2026-03-19 07:10:54 +00:00 Compare

hurui200320 force-pushed feature/m4-decision-phase-gating from 231c3656e0 to 296daebe59 2026-03-19 07:14:58 +00:00 Compare

hurui200320 commented 2026-03-19 07:15:47 +00:00

Author

Member

Copy link

Coverage Fix — Branch rebased and pushed

What changed

The coverage gate was failing at 96.9% (displayed as 97% but the precise value 96.9446% rounds to 96.9% at 1 decimal). Two untested paths in phase_gating.py were the gap:

1. Invalid plan_phase string → ValidationError — Added scenario testing that plan_phase="not_a_real_phase" raises ValidationError with the invalid value in the message.
2. DB error resilience in resolve_plan_phase — Added scenario that corrupts a real SQLite DB and verifies resolve_plan_phase() catches the DatabaseError and returns None (graceful skip).

Rebase

Branch rebased onto latest origin/master (cbf8bcc9). Resolved CHANGELOG.md conflict (kept both entries).

All quality gates pass (post-rebase)

• ✅ lint
• ✅ typecheck (0 errors)
• ✅ unit_tests (11,264 scenarios)
• ✅ integration_tests
• ✅ e2e_tests (37 tests)
• ✅ coverage_report: 97.0% (was 96.9%)

CI running: https://git.cleverthis.com/cleveragents/cleveragents-core/actions/runs/2381

## Coverage Fix — Branch rebased and pushed ### What changed The coverage gate was failing at **96.9%** (displayed as 97% but the precise value 96.9446% rounds to 96.9% at 1 decimal). Two untested paths in `phase_gating.py` were the gap: 1. **Invalid plan_phase string → ValidationError** — Added scenario testing that `plan_phase="not_a_real_phase"` raises `ValidationError` with the invalid value in the message. 2. **DB error resilience in resolve_plan_phase** — Added scenario that corrupts a real SQLite DB and verifies `resolve_plan_phase()` catches the `DatabaseError` and returns `None` (graceful skip). ### Rebase Branch rebased onto latest `origin/master` (`cbf8bcc9`). Resolved CHANGELOG.md conflict (kept both entries). ### All quality gates pass (post-rebase) - ✅ lint - ✅ typecheck (0 errors) - ✅ unit_tests (11,264 scenarios) - ✅ integration_tests - ✅ e2e_tests (37 tests) - ✅ **coverage_report: 97.0%** (was 96.9%) CI running: https://git.cleverthis.com/cleveragents/cleveragents-core/actions/runs/2381

hurui200320 added 1 commit 2026-03-19 07:45:36 +00:00

Merge branch 'master' into feature/m4-decision-phase-gating a3706a393d

hurui200320 merged commit 3837327564 into master 2026-03-19 07:53:44 +00:00

hurui200320 deleted branch feature/m4-decision-phase-gating 2026-03-19 07:53:44 +00:00

hurui200320 referenced this pull request from a commit 2026-03-19 07:53:45 +00:00

feat(plan): enforce decision type phase-gating at recording time (#973)

CoreRasurae referenced this pull request 2026-03-19 21:55:35 +00:00

refactor(cli): align actor run signature with spec positional args #1072

Sign in to join this conversation.

Reviewers

No reviewers

freemo

brent.edwards

Labels

Clear labels   

auto/needs-reevaluation

Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  

controller-managed

Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  

auto/blocked-by-deps

PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  

auto/ci-timeout

Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  

auto/claimed-implementer

Currently being processed by an implementer worker.

  

auto/claimed-merge

Currently being processed by the merge driver.

  

auto/claimed-reviewer

Currently being processed by a reviewer worker.

  

auto/driver-down

Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  

auto/invariant-violation

Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  

auto/last-attempt-tier-0

In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  

auto/last-attempt-tier-1

In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  

auto/last-attempt-tier-2

In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  

auto/last-attempt-tier-min

In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  

Automation Tracking

Tracking issues used by the AI Automation system for agents to communicate and report.

  

auto/needs-conflict-resolution

Rebase conflict needs LLM conflict-resolver.

  

auto/needs-implementer

Failing CI needs implementer attention.

  

auto/postmortem

Documenting a driver incident or rollback.

  

auto/ready-to-merge

Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  

auto/restart-throttled

Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  

auto/revert

Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  

auto/sentinel

Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  

auto/stale-inactivity

No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  

auto/unstable

Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  

Blocked

A ticket in a blocked state and unable to complete until some other task is completed first.

  

Bounty

$100

A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$1000

A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$10000

A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$20

A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$2000

A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$250

A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$50

A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$500

A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$5000

A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  

Bounty

$750

A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  

MoSCoW

Could have

Could have feature in order to satisfy the epic/legendary.

  

MoSCoW

Must have

Must have feature in order to satisfy the epic/legendary.

  

MoSCoW

Should have

Should have feature in order to satisfy the epic/legendary.

  

Needs Feedback

There are questions in the ticket that can not be completed until the project owner provides clarity.

  

Points

1

1 man-hours worth of work for an expert with no learning curve.

  

Points

13

13 man-hours worth of work for an expert with no learning curve.

  

Points

2

2 man-hours worth of work for an expert with no learning curve.

  

Points

21

21 man-hours worth of work for an expert with no learning curve.

  

Points

3

3 man-hours worth of work for an expert with no learning curve.

  

Points

34

34 man-hours worth of work for an expert with no learning curve.

  

Points

5

5 man-hours worth of work for an expert with no learning curve.

  

Points

55

55 man-hours worth of work for an expert with no learning curve.

  

Points

8

8 man-hours worth of work for an expert with no learning curve.

  

Points

88

88 man-hours worth of work for an expert with no learning curve.

  

Priority

Backlog

This ticket has backlogged priority and is not to be worked on yet

  

Priority

CI Blocker

Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Signed-off: Owner

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Signed-off: Scrum Master

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Signed-off: Tech Lead

When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  

Spike

A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  

State

Completed

The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  

State

Duplicate

A ticket that represents the same content as an existing ticket.

  

State

In Progress

A ticket that is actively being developed.

  

State

In Review

A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  

State

Paused

This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  

State

Unverified

All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  

State

Verified

The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  

State

Wont Do

This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  

Type

Automation

Any edits or discussion about the AI automated coding system.

  

Type

Bug

Something that doesnt work as intended.

  

Type

Discussion

Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  

Type

Documentation

An error or improvement needed in the documentation.

  

Type

Epic

Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  

Type

Feature

Some new functionality not present.

  

Type

Legendary

A type of Epic which will contain other Epics.

  

Type

Refactor

A code change that restructures existing code without changing its external behavior.

  

Type

Support

Someone needs help using the project.

  

Type

Task

A generic task that doesnt fit into the other type categories.

  

Type

Testing

Work exclusively focusing on fixing or expanding testing.

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

Milestone

Clear milestone

No items

No milestone

v3.3.0

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

cleveragents/cleveragents-core!973

No description provided.