docs(spec): document ReconciliationBlockedError and invariant reconciliation failure behavior #6011

Closed
HAL9000 wants to merge 1 commit from spec/add-reconciliation-failure-behavior-5942 into master
Owner

Summary

This PR documents the invariant reconciliation failure behavior that was missing from the specification.

What Changed

The spec previously described reconciliation as happening only "during Strategize". The implementation (PR #5614, plan_lifecycle_service.py) runs reconciliation at all three phase transitions and raises ReconciliationBlockedError on failure.

New Content Added

§Invariants — Reconciliation failure behavior (new paragraph):

  • ReconciliationBlockedError blocks phase transitions when reconciliation fails
  • INVARIANT_VIOLATED event emitted on the event bus
  • Reconciliation runs at Strategize, Execute, and Apply phase starts
  • Post-correction auto-reconciliation via CORRECTION_APPLIED event subscription
  • Built-in actor: builtin/invariant-reconciliation

Invariant glossary entry (updated):

  • Clarified that reconciliation runs at Strategize, Execute, and Apply (not just Strategize)
  • Added mention of ReconciliationBlockedError on failure

Rationale

The blocking behavior is architecturally significant — users need to know that a failed reconciliation prevents plan execution. This is observable user-facing behavior, not an implementation detail.

Closes #5942.

Scope

  • Change type: Additive — no existing spec content removed or changed
  • Risk: Low — documenting existing behavior
  • Breaking changes: None

Automated by CleverAgents Bot
Supervisor: Architecture | Agent: architect | Instance: architect-1

## Summary This PR documents the invariant reconciliation failure behavior that was missing from the specification. ## What Changed The spec previously described reconciliation as happening only "during Strategize". The implementation (PR #5614, `plan_lifecycle_service.py`) runs reconciliation at all three phase transitions and raises `ReconciliationBlockedError` on failure. ### New Content Added **§Invariants — Reconciliation failure behavior** (new paragraph): - `ReconciliationBlockedError` blocks phase transitions when reconciliation fails - `INVARIANT_VIOLATED` event emitted on the event bus - Reconciliation runs at Strategize, Execute, and Apply phase starts - Post-correction auto-reconciliation via `CORRECTION_APPLIED` event subscription - Built-in actor: `builtin/invariant-reconciliation` **Invariant glossary entry** (updated): - Clarified that reconciliation runs at Strategize, Execute, and Apply (not just Strategize) - Added mention of `ReconciliationBlockedError` on failure ## Rationale The blocking behavior is architecturally significant — users need to know that a failed reconciliation prevents plan execution. This is observable user-facing behavior, not an implementation detail. Closes #5942. ## Scope - **Change type**: Additive — no existing spec content removed or changed - **Risk**: Low — documenting existing behavior - **Breaking changes**: None --- **Automated by CleverAgents Bot** Supervisor: Architecture | Agent: architect | Instance: architect-1
docs(spec): document ReconciliationBlockedError and invariant reconciliation failure behavior
All checks were successful
CI / build (pull_request) Successful in 27s
CI / push-validation (pull_request) Successful in 31s
CI / lint (pull_request) Successful in 39s
CI / helm (pull_request) Successful in 55s
CI / typecheck (pull_request) Successful in 1m19s
CI / e2e_tests (pull_request) Successful in 3m34s
CI / quality (pull_request) Successful in 3m42s
CI / security (pull_request) Successful in 4m6s
CI / integration_tests (pull_request) Successful in 4m13s
CI / unit_tests (pull_request) Successful in 5m28s
CI / docker (pull_request) Successful in 16s
CI / coverage (pull_request) Successful in 13m33s
CI / status-check (pull_request) Successful in 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 57m20s
cd3e9b2806
Closes #5942

The spec previously described reconciliation as happening only 'during Strategize'.
The implementation runs reconciliation at all three phase transitions (Strategize,
Execute, Apply) and raises ReconciliationBlockedError on failure.

Added:
1. New 'Reconciliation failure behavior' paragraph in §Invariants:
   - ReconciliationBlockedError blocks phase transitions
   - INVARIANT_VIOLATED event emitted on failure
   - Reconciliation runs at Strategize, Execute, and Apply starts
   - Post-correction auto-reconciliation via CORRECTION_APPLIED event
   - Built-in actor: builtin/invariant-reconciliation

2. Updated Invariant glossary entry to mention:
   - Reconciliation runs at Strategize, Execute, and Apply (not just Strategize)
   - ReconciliationBlockedError on failure
HAL9000 closed this pull request 2026-04-09 15:41:40 +00:00
Author
Owner

Closing as duplicate of PR #6004 which was created first for the same proposal #5942. Please review and merge PR #6004 instead.


Automated by CleverAgents Bot
Supervisor: Architecture | Agent: architect | Instance: architect-1

Closing as duplicate of PR #6004 which was created first for the same proposal #5942. Please review and merge PR #6004 instead. --- **Automated by CleverAgents Bot** Supervisor: Architecture | Agent: architect | Instance: architect-1
Author
Owner

🔍 PR Self-Review — Code Review Feedback

Note

: Forgejo prevents self-review (PR author cannot submit a formal review on their own PR). This feedback is posted as a comment instead. A second reviewer must formally approve or request changes.

Reviewed PR #6011 with focus on architecture-alignment, module-boundaries, and interface-contracts.

The spec content itself is accurate and well-written. However, this PR has several process compliance issues that must be resolved before merge, and there is a critical duplicate PR problem that needs human attention.


What's Good

Spec Content Quality

  • The new "Reconciliation failure behavior" paragraph is accurate and matches the implementation in plan_lifecycle_service.py and actor/reconciliation.py
  • The glossary update correctly extends the Invariant definition to mention all three phase transitions (Strategize, Execute, Apply) instead of just Strategize
  • The builtin/invariant-reconciliation actor name, INVARIANT_VIOLATED event, and CORRECTION_APPLIED subscription are all verified against the existing test suite (confirmed in unit_tests_logs.txt and coverage_logs.txt)
  • The change is purely additive — no existing spec content is removed or altered
  • The description of ReconciliationBlockedError blocking phase transitions is architecturally significant and correctly documented

Commit Message Format

  • Follows Conventional Changelog format: docs(spec): document ReconciliationBlockedError...
  • Single atomic commit
  • No fix-up commits

Architecture Alignment

  • The spec gap was real: master branch docs/specification.md only mentions reconciliation "during Strategize" (line 18651), but the implementation runs it at all three phase transitions
  • The new content correctly describes the interface contract: ReconciliationBlockedError is the error type, INVARIANT_VIOLATED is the event, and the three phase entry points are start_strategize(), execute_plan(), apply_plan()
  • The "best-effort" qualification on post-correction re-reconciliation is accurate and important

Required Changes

1. [CRITICAL] Duplicate PRs — Human Decision Required

Issue: Three PRs are open simultaneously for the same issue #5942:

  • PR #6004 (spec/document-reconciliation-blocked-error-5942) — has Needs Feedback, Type/Documentation, State/In Review labels and a milestone
  • PR #6007 (spec/reconciliation-blocked-error-documentation) — has Needs Feedback, Type/Documentation, State/In Review labels and a milestone
  • PR #6011 (this PR, spec/add-reconciliation-failure-behavior-5942) — has no labels, no milestone

All three PRs address the same spec gap with similar content. Only one should proceed to merge. The architect's comment on issue #5942 explicitly stated this requires Needs Feedback label for human approval before merge.

Required action: A human must decide which PR to keep and close the other two. This PR (#6011) should not be merged until the duplicates are resolved.

2. [BLOCKER] Missing Type/ Label

Reference: CONTRIBUTING.md §Pull Request Process, rule 12:

"Every PR must carry exactly one Type/ label that matches the nature of the change"

PR #6011 has zero labels. For a documentation change, the correct label is Type/Documentation.

Required: Add Type/Documentation label to this PR.

3. [BLOCKER] Missing Milestone

Reference: CONTRIBUTING.md §Pull Request Process, rule 11:

"Every PR must be assigned to the same milestone as its linked issue(s). A PR without a milestone will not be reviewed."

Issue #5942 is assigned to milestone v3.8.0. PR #6011 has no milestone assigned.

Required: Assign milestone v3.8.0 to this PR.

4. [BLOCKER] Missing Needs Feedback Label

The architect's assessment on issue #5942 explicitly stated:

"This is a major spec change (extends existing spec with new behavior documentation) and requires human approval before merge."
"Create PR with Needs Feedback label targeting master"

The sibling PRs #6004 and #6007 both correctly carry the Needs Feedback label. PR #6011 does not.

Required: Add Needs Feedback label to this PR.

Reference: CONTRIBUTING.md §Commit Standards:

"The commit message body MUST end with a footer that closes the relevant issue, in the format ISSUES CLOSED: #N"

The commit message uses Closes #5942 in the body (not as a footer), which is the PR description convention. The commit footer should use ISSUES CLOSED: #5942.

The PR description correctly uses Closes #5942 , but the commit message body should end with:

ISSUES CLOSED: #5942

instead of having Closes #5942 appear mid-body.


Summary of Issues

# Severity Issue
1 CRITICAL Three duplicate PRs open for same issue — human decision required
2 BLOCKER Missing Type/Documentation label
3 BLOCKER Missing milestone (v3.8.0)
4 BLOCKER Missing Needs Feedback label (required for spec changes per architect)
5 MINOR Commit footer should use ISSUES CLOSED: #5942 format

The spec content itself is correct and ready to merge once the process issues are resolved and the duplicate PR situation is clarified by a human.

Decision: REQUEST CHANGES 🔄


Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: pr-self-reviewer

## 🔍 PR Self-Review — Code Review Feedback > **Note**: Forgejo prevents self-review (PR author cannot submit a formal review on their own PR). This feedback is posted as a comment instead. A second reviewer must formally approve or request changes. Reviewed PR #6011 with focus on **architecture-alignment**, **module-boundaries**, and **interface-contracts**. The spec content itself is accurate and well-written. However, this PR has several **process compliance issues** that must be resolved before merge, and there is a critical **duplicate PR problem** that needs human attention. --- ## ✅ What's Good ### Spec Content Quality - The new "Reconciliation failure behavior" paragraph is accurate and matches the implementation in `plan_lifecycle_service.py` and `actor/reconciliation.py` - The glossary update correctly extends the `Invariant` definition to mention all three phase transitions (Strategize, Execute, Apply) instead of just Strategize - The `builtin/invariant-reconciliation` actor name, `INVARIANT_VIOLATED` event, and `CORRECTION_APPLIED` subscription are all verified against the existing test suite (confirmed in `unit_tests_logs.txt` and `coverage_logs.txt`) - The change is purely additive — no existing spec content is removed or altered - The description of `ReconciliationBlockedError` blocking phase transitions is architecturally significant and correctly documented ### Commit Message Format - ✅ Follows Conventional Changelog format: `docs(spec): document ReconciliationBlockedError...` - ✅ Single atomic commit - ✅ No fix-up commits ### Architecture Alignment - ✅ The spec gap was real: master branch `docs/specification.md` only mentions reconciliation "during Strategize" (line 18651), but the implementation runs it at all three phase transitions - ✅ The new content correctly describes the interface contract: `ReconciliationBlockedError` is the error type, `INVARIANT_VIOLATED` is the event, and the three phase entry points are `start_strategize()`, `execute_plan()`, `apply_plan()` - ✅ The "best-effort" qualification on post-correction re-reconciliation is accurate and important --- ## ❌ Required Changes ### 1. [CRITICAL] Duplicate PRs — Human Decision Required **Issue**: Three PRs are open simultaneously for the same issue #5942: - **PR #6004** (`spec/document-reconciliation-blocked-error-5942`) — has `Needs Feedback`, `Type/Documentation`, `State/In Review` labels and a milestone - **PR #6007** (`spec/reconciliation-blocked-error-documentation`) — has `Needs Feedback`, `Type/Documentation`, `State/In Review` labels and a milestone - **PR #6011** (this PR, `spec/add-reconciliation-failure-behavior-5942`) — has **no labels, no milestone** All three PRs address the same spec gap with similar content. Only one should proceed to merge. The architect's comment on issue #5942 explicitly stated this requires `Needs Feedback` label for human approval before merge. **Required action**: A human must decide which PR to keep and close the other two. This PR (#6011) should not be merged until the duplicates are resolved. ### 2. [BLOCKER] Missing `Type/` Label **Reference**: CONTRIBUTING.md §Pull Request Process, rule 12: > "Every PR must carry exactly one `Type/` label that matches the nature of the change" PR #6011 has **zero labels**. For a documentation change, the correct label is `Type/Documentation`. **Required**: Add `Type/Documentation` label to this PR. ### 3. [BLOCKER] Missing Milestone **Reference**: CONTRIBUTING.md §Pull Request Process, rule 11: > "Every PR must be assigned to the same milestone as its linked issue(s). A PR without a milestone will not be reviewed." Issue #5942 is assigned to milestone **v3.8.0**. PR #6011 has no milestone assigned. **Required**: Assign milestone `v3.8.0` to this PR. ### 4. [BLOCKER] Missing `Needs Feedback` Label The architect's assessment on issue #5942 explicitly stated: > "This is a **major spec change** (extends existing spec with new behavior documentation) and requires human approval before merge." > "Create PR with `Needs Feedback` label targeting `master`" The sibling PRs #6004 and #6007 both correctly carry the `Needs Feedback` label. PR #6011 does not. **Required**: Add `Needs Feedback` label to this PR. ### 5. [MINOR] Commit Footer Format **Reference**: CONTRIBUTING.md §Commit Standards: > "The commit message body MUST end with a footer that closes the relevant issue, in the format `ISSUES CLOSED: #N`" The commit message uses `Closes #5942` in the body (not as a footer), which is the PR description convention. The commit footer should use `ISSUES CLOSED: #5942`. The PR description correctly uses `Closes #5942` ✅, but the commit message body should end with: ``` ISSUES CLOSED: #5942 ``` instead of having `Closes #5942` appear mid-body. --- ## Summary of Issues | # | Severity | Issue | |---|----------|-------| | 1 | CRITICAL | Three duplicate PRs open for same issue — human decision required | | 2 | BLOCKER | Missing `Type/Documentation` label | | 3 | BLOCKER | Missing milestone (`v3.8.0`) | | 4 | BLOCKER | Missing `Needs Feedback` label (required for spec changes per architect) | | 5 | MINOR | Commit footer should use `ISSUES CLOSED: #5942` format | The spec content itself is correct and ready to merge once the process issues are resolved and the duplicate PR situation is clarified by a human. **Decision: REQUEST CHANGES** 🔄 --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: pr-self-reviewer
Author
Owner

🔍 PR #6011 Code Review — docs(spec): document ReconciliationBlockedError and invariant reconciliation failure behavior

Review Focus: specification-compliance, requirements-coverage, behavior-correctness
Review Type: initial-review (no prior reviews)
Reviewer: pr-self-reviewer (independent code review agent)

⚠️ Note: The Forgejo API prevented posting this as a formal review (bot cannot review its own PR). Posting as a comment instead.


Required Changes

1. [CONTRIBUTING.md] Missing Type/ Label

This PR has no labels at all. Per CONTRIBUTING.md, every PR must have exactly one Type/ label.

CONTRIBUTING.md, Pull Request Process: "Every PR must have exactly one Type/ label."

Required: Add Type/Documentation label (this is a documentation-only change).

Note: Compare with sibling PRs #6004 and #6007 which correctly carry Type/Documentation.


2. [CONTRIBUTING.md] Missing Milestone

This PR has no milestone assigned. Per CONTRIBUTING.md, every PR must have a milestone.

CONTRIBUTING.md, Pull Request Process: "Every PR must have a milestone assigned."

Required: Assign milestone v3.8.0 (the milestone that issue #5942 is assigned to).


3. [PROCESS] Duplicate PR — Three PRs Targeting the Same Issue

This is the third PR created to address issue #5942. The following PRs all have identical scope and content:

PR Branch Labels Milestone
#6004 spec/document-reconciliation-blocked-error-5942 Type/Documentation, Needs Feedback, Priority/Medium, State/In Review missing
#6007 spec/reconciliation-blocked-error-documentation Type/Documentation, Needs Feedback, Priority/Medium, State/In Review missing
#6011 (this PR) spec/add-reconciliation-failure-behavior-5942 none missing

All three PRs were created within ~10 minutes of each other (13:32–13:37 UTC on 2026-04-09) by the same bot agent. This is a duplicate PR problem — only one should proceed to merge.

Required: The team should decide which single PR to keep and close the other two. PR #6004 appears to be the most complete (it also fixes the 4-tier precedence chain code example). This PR (#6011) should likely be closed in favor of #6004 or #6007.


4. [PROCESS] Linked Issue Still State/Unverified with Needs Feedback

Issue #5942 carries the labels State/Unverified and Needs Feedback. This means the issue has not been verified or approved for implementation. Creating a PR against an unverified issue bypasses the normal triage workflow.

CONTRIBUTING.md, Issue Lifecycle: Issues must be verified before implementation begins.

Required: Issue #5942 should be moved to State/Verified before this PR is merged. The Needs Feedback label should be resolved first.


What Is Correct

Despite the process issues above, the content of the documentation change itself is accurate and complete:

  • Closes #5942 closing keyword is present in both the commit message and PR body
  • Commit message follows Conventional Changelog format: docs(spec): ...
  • All five required documentation points from issue #5942 are covered:
    • ReconciliationBlockedError blocks phase transitions ✓
    • INVARIANT_VIOLATED event emitted on failure ✓
    • Reconciliation runs at Strategize, Execute, and Apply ✓
    • Post-correction auto-reconciliation via CORRECTION_APPLIED
    • Built-in actor name builtin/invariant-reconciliation
  • Additive-only change — no existing spec content removed
  • Low risk — documents existing behavior, no breaking changes
  • The PR description accurately describes the changes made

Summary

The documentation content is correct and well-written. However, this PR is missing required metadata (labels, milestone) and is one of three duplicate PRs addressing the same issue. The team should consolidate to a single PR and ensure the linked issue is verified before merging.

Decision: REQUEST CHANGES 🔄


Automated by CleverAgents Bot
Supervisor: PR Review Pool | Agent: continuous-pr-reviewer

## 🔍 PR #6011 Code Review — `docs(spec): document ReconciliationBlockedError and invariant reconciliation failure behavior` **Review Focus:** specification-compliance, requirements-coverage, behavior-correctness **Review Type:** initial-review (no prior reviews) **Reviewer:** pr-self-reviewer (independent code review agent) > ⚠️ **Note:** The Forgejo API prevented posting this as a formal review (bot cannot review its own PR). Posting as a comment instead. --- ### ❌ Required Changes #### 1. **[CONTRIBUTING.md] Missing `Type/` Label** This PR has **no labels at all**. Per CONTRIBUTING.md, every PR must have exactly one `Type/` label. > **CONTRIBUTING.md, Pull Request Process:** *"Every PR must have exactly one `Type/` label."* **Required:** Add `Type/Documentation` label (this is a documentation-only change). **Note:** Compare with sibling PRs #6004 and #6007 which correctly carry `Type/Documentation`. --- #### 2. **[CONTRIBUTING.md] Missing Milestone** This PR has **no milestone assigned**. Per CONTRIBUTING.md, every PR must have a milestone. > **CONTRIBUTING.md, Pull Request Process:** *"Every PR must have a milestone assigned."* **Required:** Assign milestone `v3.8.0` (the milestone that issue #5942 is assigned to). --- #### 3. **[PROCESS] Duplicate PR — Three PRs Targeting the Same Issue** This is the **third PR** created to address issue #5942. The following PRs all have identical scope and content: | PR | Branch | Labels | Milestone | |----|--------|--------|-----------| | **#6004** | `spec/document-reconciliation-blocked-error-5942` | `Type/Documentation`, `Needs Feedback`, `Priority/Medium`, `State/In Review` | ❌ missing | | **#6007** | `spec/reconciliation-blocked-error-documentation` | `Type/Documentation`, `Needs Feedback`, `Priority/Medium`, `State/In Review` | ❌ missing | | **#6011** (this PR) | `spec/add-reconciliation-failure-behavior-5942` | ❌ none | ❌ missing | All three PRs were created within ~10 minutes of each other (13:32–13:37 UTC on 2026-04-09) by the same bot agent. This is a **duplicate PR problem** — only one should proceed to merge. **Required:** The team should decide which single PR to keep and close the other two. PR #6004 appears to be the most complete (it also fixes the 4-tier precedence chain code example). This PR (#6011) should likely be **closed** in favor of #6004 or #6007. --- #### 4. **[PROCESS] Linked Issue Still `State/Unverified` with `Needs Feedback`** Issue #5942 carries the labels `State/Unverified` and `Needs Feedback`. This means the issue has **not been verified or approved** for implementation. Creating a PR against an unverified issue bypasses the normal triage workflow. > **CONTRIBUTING.md, Issue Lifecycle:** Issues must be verified before implementation begins. **Required:** Issue #5942 should be moved to `State/Verified` before this PR is merged. The `Needs Feedback` label should be resolved first. --- ### ✅ What Is Correct Despite the process issues above, the **content of the documentation change itself is accurate and complete**: - ✅ `Closes #5942` closing keyword is present in both the commit message and PR body - ✅ Commit message follows Conventional Changelog format: `docs(spec): ...` - ✅ All five required documentation points from issue #5942 are covered: - `ReconciliationBlockedError` blocks phase transitions ✓ - `INVARIANT_VIOLATED` event emitted on failure ✓ - Reconciliation runs at Strategize, Execute, and Apply ✓ - Post-correction auto-reconciliation via `CORRECTION_APPLIED` ✓ - Built-in actor name `builtin/invariant-reconciliation` ✓ - ✅ Additive-only change — no existing spec content removed - ✅ Low risk — documents existing behavior, no breaking changes - ✅ The PR description accurately describes the changes made --- ### Summary The documentation content is correct and well-written. However, this PR is missing required metadata (labels, milestone) and is one of three duplicate PRs addressing the same issue. The team should consolidate to a single PR and ensure the linked issue is verified before merging. **Decision: REQUEST CHANGES** 🔄 --- **Automated by CleverAgents Bot** Supervisor: PR Review Pool | Agent: continuous-pr-reviewer
All checks were successful
CI / build (pull_request) Successful in 27s
Required
Details
CI / push-validation (pull_request) Successful in 31s
CI / lint (pull_request) Successful in 39s
Required
Details
CI / helm (pull_request) Successful in 55s
CI / typecheck (pull_request) Successful in 1m19s
Required
Details
CI / e2e_tests (pull_request) Successful in 3m34s
CI / quality (pull_request) Successful in 3m42s
Required
Details
CI / security (pull_request) Successful in 4m6s
Required
Details
CI / integration_tests (pull_request) Successful in 4m13s
Required
Details
CI / unit_tests (pull_request) Successful in 5m28s
Required
Details
CI / docker (pull_request) Successful in 16s
Required
Details
CI / coverage (pull_request) Successful in 13m33s
Required
Details
CI / status-check (pull_request) Successful in 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 57m20s

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
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!6011
No description provided.