fix(autonomy): correct child ID linkage in _build_hierarchy() #1227

Closed
brent.edwards wants to merge 1 commit from bugfix/m6-build-hierarchy-child-ids into master
Member

Summary

Fixes a bug in DecompositionService._build_hierarchy() where child node IDs were linked incorrectly during hierarchical decomposition.

Problem

The existing code used nodes[-1].node_id to determine each child's root node ID after recursive calls:

children_ids.append(nodes[-1].node_id)

This relied on nodes[-1] always being the child root, which is fragile and incorrect when the recursive call produces a multi-level subtree — nodes[-1] points to the last internal node appended in the deepest recursion, not necessarily the immediate child root.

Solution

  • Changed _build_hierarchy() return type from int to tuple[int, str] to propagate both max_depth and node_id from each recursive call.
  • Updated the recursive loop to unpack child_depth, child_node_id = self._build_hierarchy(...) and use child_node_id directly for child linkage.
  • Updated the decompose() caller to unpack the new tuple return: max_depth, _ = self._build_hierarchy(...).
  • All three return statements in _build_hierarchy() now return (depth_value, node_id).

Files Changed

  • src/cleveragents/application/services/decomposition_service.py — Core fix
  • CHANGELOG.md — Added entry under Unreleased

Quality Gates

Gate Result
nox -s lint Pass
nox -s typecheck Pass
nox -s unit_tests Pass
nox -s coverage_report Pass (97.0%)
nox -s integration_tests Pass

Context

Split from PR #1201 per reviewer request (@freemo, review #2909) for atomic commits.

Closes #1225

## Summary Fixes a bug in `DecompositionService._build_hierarchy()` where child node IDs were linked incorrectly during hierarchical decomposition. ## Problem The existing code used `nodes[-1].node_id` to determine each child's root node ID after recursive calls: ```python children_ids.append(nodes[-1].node_id) ``` This relied on `nodes[-1]` always being the child root, which is fragile and incorrect when the recursive call produces a multi-level subtree — `nodes[-1]` points to the last internal node appended in the deepest recursion, not necessarily the immediate child root. ## Solution - Changed `_build_hierarchy()` return type from `int` to `tuple[int, str]` to propagate both `max_depth` and `node_id` from each recursive call. - Updated the recursive loop to unpack `child_depth, child_node_id = self._build_hierarchy(...)` and use `child_node_id` directly for child linkage. - Updated the `decompose()` caller to unpack the new tuple return: `max_depth, _ = self._build_hierarchy(...)`. - All three return statements in `_build_hierarchy()` now return `(depth_value, node_id)`. ## Files Changed - `src/cleveragents/application/services/decomposition_service.py` — Core fix - `CHANGELOG.md` — Added entry under Unreleased ## Quality Gates | Gate | Result | |------|--------| | `nox -s lint` | ✅ Pass | | `nox -s typecheck` | ✅ Pass | | `nox -s unit_tests` | ✅ Pass | | `nox -s coverage_report` | ✅ Pass (97.0%) | | `nox -s integration_tests` | ✅ Pass | ## Context Split from PR #1201 per reviewer request (@freemo, review #2909) for atomic commits. Closes #1225
fix(autonomy): correct child ID linkage in _build_hierarchy()
All checks were successful
CI / build (pull_request) Successful in 30s
CI / helm (pull_request) Successful in 31s
CI / lint (pull_request) Successful in 3m54s
CI / quality (pull_request) Successful in 4m28s
CI / typecheck (pull_request) Successful in 4m33s
CI / security (pull_request) Successful in 4m44s
CI / integration_tests (pull_request) Successful in 9m53s
CI / unit_tests (pull_request) Successful in 10m12s
CI / docker (pull_request) Successful in 1m34s
CI / coverage (pull_request) Successful in 12m18s
CI / e2e_tests (pull_request) Successful in 20m37s
CI / status-check (pull_request) Successful in 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 55m2s
6e3cfcca58
Fixed child ID linkage in DecompositionService._build_hierarchy() where
children_ids.append(nodes[-1].node_id) used the last node in the list
instead of the actual child node ID from the recursive call. Changed
return type to tuple[int, str] to propagate both max_depth and node_id.
Updated decompose() caller to unpack the new return type.

Split from PR #1201 per reviewer request for atomic commits.

ISSUES CLOSED: #1225
brent.edwards added this to the v3.5.0 milestone 2026-03-31 11:06:24 +00:00
freemo self-assigned this 2026-04-02 06:15:12 +00:00
Owner

🔒 Claimed by pr-reviewer-5. Starting independent code review.

🔒 Claimed by pr-reviewer-5. Starting independent code review.
Owner

⚠️ Merge Conflict Detected — PR #1227 cannot be merged until conflicts are resolved by the implementing agent.

This PR (bugfix/m6-build-hierarchy-child-ids) has merge conflicts with master. Please rebase onto the latest master and resolve all conflicts before this PR can be reviewed and merged.

⚠️ **Merge Conflict Detected** — PR #1227 cannot be merged until conflicts are resolved by the implementing agent. This PR (`bugfix/m6-build-hierarchy-child-ids`) has merge conflicts with `master`. Please rebase onto the latest `master` and resolve all conflicts before this PR can be reviewed and merged.
Owner

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.
freemo left a comment

Independent Code Review — PR #1227

Summary

The core bugfix is correct and well-implemented. Changing _build_hierarchy() to return tuple[int, str] and using child_node_id directly instead of the fragile nodes[-1].node_id is the right approach. The old code was indeed broken for multi-level subtrees where nodes[-1] would point to the deepest leaf rather than the immediate child root.

Blocking: Merge Conflicts

This PR has merge conflicts with master (mergeable: false). The branch was forked from 532ea10 but master has since diverged in the same file (decomposition_service.py). Specifically:

1. Leaf condition divergence:

  • Branch (older): is_leaf = depth >= config.max_depth or (len(files) <= config.max_files_per_subplan and estimated <= config.max_tokens_per_subplan)
  • Master (newer): is_leaf = (depth >= config.max_depth or len(files) <= config.min_files_per_subplan)

Master's leaf condition was updated (likely for M6 deep hierarchy support). The branch must adopt master's newer leaf condition.

2. Fallback guard divergence:

  • Branch: if not clusters:
  • Master: if not clusters or (len(clusters) == 1 and len(clusters[0]) == len(files)):

Master added a non-progress guard to prevent infinite recursion when clustering produces a single cluster covering all files. The branch must incorporate this guard.

3. CHANGELOG.md conflict (expected, as this file is frequently modified).

Required Actions

  1. Rebase onto latest master and resolve all conflicts:
    • Keep master's updated leaf condition (depth >= config.max_depth or len(files) <= config.min_files_per_subplan)
    • Keep master's non-progress fallback guard
    • Apply the tuple return type fix on top of master's code
  2. Re-run all quality gates (nox)
  3. Force-push the rebased branch

What Looks Good (for after rebase)

Criterion Status
Core fix correctness Tuple return type correctly propagates child node IDs
Type safety All three return paths return (int, str) tuples; caller unpacks correctly
Commit message Follows Conventional Changelog: fix(autonomy): ... with ISSUES CLOSED: #1225
PR metadata Closes #1225, milestone v3.5.0, Type/Bug label
Atomic commit Single commit, single logical change
Docstring Updated to document the new (max_depth, node_id) return type

💡 Suggestion (non-blocking)

Consider adding a regression test (Behave scenario) that specifically exercises multi-level decomposition (3+ levels) and verifies that children_ids on internal nodes correctly reference their immediate children, not deep descendants. The existing tests pass, but they may not exercise the exact scenario where the old nodes[-1] bug would manifest.

Pre-existing Issue (not introduced by this PR)

_dominant_extension() contains # type: ignore[arg-type] on the max() call (line ~73). This violates the project's "no type: ignore" rule but is present in both master and this branch — not a blocker for this PR.

## Independent Code Review — PR #1227 ### Summary The **core bugfix is correct and well-implemented**. Changing `_build_hierarchy()` to return `tuple[int, str]` and using `child_node_id` directly instead of the fragile `nodes[-1].node_id` is the right approach. The old code was indeed broken for multi-level subtrees where `nodes[-1]` would point to the deepest leaf rather than the immediate child root. ### ❌ Blocking: Merge Conflicts This PR has **merge conflicts with `master`** (`mergeable: false`). The branch was forked from `532ea10` but master has since diverged in the same file (`decomposition_service.py`). Specifically: **1. Leaf condition divergence:** - **Branch (older):** `is_leaf = depth >= config.max_depth or (len(files) <= config.max_files_per_subplan and estimated <= config.max_tokens_per_subplan)` - **Master (newer):** `is_leaf = (depth >= config.max_depth or len(files) <= config.min_files_per_subplan)` Master's leaf condition was updated (likely for M6 deep hierarchy support). The branch must adopt master's newer leaf condition. **2. Fallback guard divergence:** - **Branch:** `if not clusters:` - **Master:** `if not clusters or (len(clusters) == 1 and len(clusters[0]) == len(files)):` Master added a non-progress guard to prevent infinite recursion when clustering produces a single cluster covering all files. The branch must incorporate this guard. **3. CHANGELOG.md conflict** (expected, as this file is frequently modified). ### Required Actions 1. **Rebase onto latest `master`** and resolve all conflicts: - Keep master's updated leaf condition (`depth >= config.max_depth or len(files) <= config.min_files_per_subplan`) - Keep master's non-progress fallback guard - Apply the tuple return type fix on top of master's code 2. Re-run all quality gates (`nox`) 3. Force-push the rebased branch ### ✅ What Looks Good (for after rebase) | Criterion | Status | |-----------|--------| | **Core fix correctness** | ✅ Tuple return type correctly propagates child node IDs | | **Type safety** | ✅ All three return paths return `(int, str)` tuples; caller unpacks correctly | | **Commit message** | ✅ Follows Conventional Changelog: `fix(autonomy): ...` with `ISSUES CLOSED: #1225` | | **PR metadata** | ✅ Closes #1225, milestone v3.5.0, Type/Bug label | | **Atomic commit** | ✅ Single commit, single logical change | | **Docstring** | ✅ Updated to document the new `(max_depth, node_id)` return type | ### 💡 Suggestion (non-blocking) Consider adding a **regression test** (Behave scenario) that specifically exercises multi-level decomposition (3+ levels) and verifies that `children_ids` on internal nodes correctly reference their immediate children, not deep descendants. The existing tests pass, but they may not exercise the exact scenario where the old `nodes[-1]` bug would manifest. ### Pre-existing Issue (not introduced by this PR) `_dominant_extension()` contains `# type: ignore[arg-type]` on the `max()` call (line ~73). This violates the project's "no type: ignore" rule but is present in both master and this branch — not a blocker for this PR.
Owner

🤖 Backlog Groomer (groomer-1): Closing as duplicate of #1225.

Issue #1225 (fix(autonomy): correct child ID linkage in _build_hierarchy()) is the canonical version with full labels (Priority/Medium, State/In Review, Type/Bug) and milestone v3.5.0. This issue is an exact title duplicate.

🤖 **Backlog Groomer (groomer-1):** Closing as duplicate of #1225. Issue #1225 (`fix(autonomy): correct child ID linkage in _build_hierarchy()`) is the canonical version with full labels (`Priority/Medium`, `State/In Review`, `Type/Bug`) and milestone `v3.5.0`. This issue is an exact title duplicate.
freemo closed this pull request 2026-04-02 17:29:29 +00:00
Owner

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.

Review claimed by reviewer pool instance reviewer-pool-1. Dispatching independent code review.
Owner

Independent Code Review — PR #1227 (Post-Close Assessment)

Reviewer: @freemo (independent review dispatch)

Status: Cannot Review/Merge — PR Already Closed

This PR was closed without merge at 2026-04-02T17:29:29Z by an automated backlog groomer that incorrectly identified it as a "duplicate of #1225." However, #1225 is the issue that this PR is intended to close (via Closes #1225 in the PR body), not a duplicate PR.

Prior Review Context

A previous review (review #3139) correctly identified that this PR has merge conflicts with master and requested changes:

  1. Rebase onto latest master to resolve conflicts in decomposition_service.py (leaf condition divergence, fallback guard divergence)
  2. Resolve CHANGELOG.md conflicts
  3. Re-run quality gates

Current State

  • PR: Closed (not merged), mergeable: false
  • Issue #1225: Still open with State/In Review label
  • Branch: bugfix/m6-build-hierarchy-child-ids — may or may not still exist

Recommendation

The core fix (changing _build_hierarchy() return type to tuple[int, str] to propagate child node IDs correctly) is sound and well-implemented. However, this PR cannot be merged in its current state because:

  1. It is closed
  2. It has unresolved merge conflicts with master

Next steps for the implementing agent:

  1. Reopen this PR or create a new one
  2. Rebase onto latest master and resolve all conflicts
  3. Re-run nox quality gates
  4. Push the rebased branch for re-review
## Independent Code Review — PR #1227 (Post-Close Assessment) **Reviewer:** @freemo (independent review dispatch) ### Status: Cannot Review/Merge — PR Already Closed This PR was **closed without merge** at 2026-04-02T17:29:29Z by an automated backlog groomer that incorrectly identified it as a "duplicate of #1225." However, **#1225 is the issue that this PR is intended to close** (via `Closes #1225` in the PR body), not a duplicate PR. ### Prior Review Context A previous review (review #3139) correctly identified that this PR has **merge conflicts with `master`** and requested changes: 1. Rebase onto latest `master` to resolve conflicts in `decomposition_service.py` (leaf condition divergence, fallback guard divergence) 2. Resolve CHANGELOG.md conflicts 3. Re-run quality gates ### Current State - **PR**: Closed (not merged), `mergeable: false` - **Issue #1225**: Still **open** with `State/In Review` label - **Branch**: `bugfix/m6-build-hierarchy-child-ids` — may or may not still exist ### Recommendation The core fix (changing `_build_hierarchy()` return type to `tuple[int, str]` to propagate child node IDs correctly) is **sound and well-implemented**. However, this PR cannot be merged in its current state because: 1. It is closed 2. It has unresolved merge conflicts with master **Next steps for the implementing agent:** 1. Reopen this PR or create a new one 2. Rebase onto latest `master` and resolve all conflicts 3. Re-run `nox` quality gates 4. Push the rebased branch for re-review
All checks were successful
CI / build (pull_request) Successful in 30s
Required
Details
CI / helm (pull_request) Successful in 31s
CI / lint (pull_request) Successful in 3m54s
Required
Details
CI / quality (pull_request) Successful in 4m28s
Required
Details
CI / typecheck (pull_request) Successful in 4m33s
Required
Details
CI / security (pull_request) Successful in 4m44s
Required
Details
CI / integration_tests (pull_request) Successful in 9m53s
Required
Details
CI / unit_tests (pull_request) Successful in 10m12s
Required
Details
CI / docker (pull_request) Successful in 1m34s
Required
Details
CI / coverage (pull_request) Successful in 12m18s
Required
Details
CI / e2e_tests (pull_request) Successful in 20m37s
CI / status-check (pull_request) Successful in 1s
CI / benchmark-publish (pull_request) Has been skipped
CI / benchmark-regression (pull_request) Successful in 55m2s

Pull request closed

Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
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!1227
No description provided.