UAT: `SEQUENTIAL_APPLY` merge strategy is semantically equivalent to `LAST_WINS` — `_sequential_apply()` uses `SequentialMergeStrategy` (last-write-wins) for each iteration instead of applying diffs sequentially #6142

New issue

Open

opened 2026-04-09 15:28:49 +00:00 by HAL9000 · 0 comments

HAL9000 commented

2026-04-09 15:28:49 +00:00

Owner

Bug Report

Feature Area: Three-Way Merge — Merge Strategy Selection
Spec Reference: §Child Plan Result Merging; SubplanMergeStrategy enum
Severity: Medium — SEQUENTIAL_APPLY and LAST_WINS produce identical results, making one strategy redundant

What Was Tested

The SEQUENTIAL_APPLY merge strategy in SubplanMergeService._sequential_apply().

Expected Behavior

SEQUENTIAL_APPLY should apply each subplan's changes sequentially, treating each subplan's output as a patch on top of the previous result. The intent is to chain changes: subplan A's output becomes the base for subplan B's changes, which becomes the base for subplan C's changes, etc.

This is semantically different from LAST_WINS (which simply returns the last subplan's content unconditionally).

Actual Behavior

_sequential_apply() uses SequentialMergeStrategy for each iteration:

# subplan_merge_service.py:228-244
def _sequential_apply(self, path, base_content, contents, subplan_ids):
    current = base_content
    for content in contents:
        result: MergeResult = self._seq_merge.merge(base_content, current, content)
        current = result.content
    return FileMergeOutcome(path=path, content=current, ...)

SequentialMergeStrategy.merge() is defined as:

# infrastructure/sandbox/merge.py:189-204
def merge(self, base: str, ours: str, theirs: str) -> MergeResult:
    """Return *theirs* as the merged content."""
    return MergeResult(success=True, content=theirs, ...)

It ignores base and ours entirely and always returns theirs.

Therefore, _sequential_apply() iterates through all contents and always sets current = content (the latest theirs), resulting in current = contents[-1] — identical to LAST_WINS.

Verification

from cleveragents.application.services.subplan_merge_service import SubplanMergeService
from cleveragents.domain.models.core.plan import SubplanMergeStrategy

# SEQUENTIAL_APPLY
svc_seq = SubplanMergeService(SubplanMergeStrategy.SEQUENTIAL_APPLY)
result_seq = svc_seq.merge(
    base_files={"f.py": "a = 1\n"},
    subplan_outputs=[
        ("sp1", {"f.py": "a = 2\n"}),
        ("sp2", {"f.py": "a = 3\n"}),
    ]
)

# LAST_WINS
svc_lw = SubplanMergeService(SubplanMergeStrategy.LAST_WINS)
result_lw = svc_lw.merge(
    base_files={"f.py": "a = 1\n"},
    subplan_outputs=[
        ("sp1", {"f.py": "a = 2\n"}),
        ("sp2", {"f.py": "a = 3\n"}),
    ]
)

assert result_seq.merged_files[0].content == result_lw.merged_files[0].content  # Both "a = 3\n"

Code Location

src/cleveragents/application/services/subplan_merge_service.py:228–244 — _sequential_apply() — uses wrong merge strategy
src/cleveragents/infrastructure/sandbox/merge.py:182–204 — SequentialMergeStrategy — last-write-wins, not sequential patch application

Fix Required

_sequential_apply() should use GitMergeStrategy (or a proper diff-based approach) to apply each subplan's changes on top of the previous result:

def _sequential_apply(self, path, base_content, contents, subplan_ids):
    current = base_content
    for content in contents:
        # Apply content as a patch on top of current
        result = self._git_merge.merge(current, current, content)
        # Or: treat current as both base and ours, content as theirs
        current = result.content
    return FileMergeOutcome(path=path, content=current, ...)

Alternatively, the SequentialMergeStrategy should be renamed to LastWinsStrategy to accurately reflect its behavior, and a proper sequential patch application strategy should be implemented for SEQUENTIAL_APPLY.

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: uat-tester

## Bug Report **Feature Area**: Three-Way Merge — Merge Strategy Selection **Spec Reference**: §Child Plan Result Merging; `SubplanMergeStrategy` enum **Severity**: Medium — `SEQUENTIAL_APPLY` and `LAST_WINS` produce identical results, making one strategy redundant --- ## What Was Tested The `SEQUENTIAL_APPLY` merge strategy in `SubplanMergeService._sequential_apply()`. --- ## Expected Behavior `SEQUENTIAL_APPLY` should apply each subplan's changes sequentially, treating each subplan's output as a patch on top of the previous result. The intent is to chain changes: subplan A's output becomes the base for subplan B's changes, which becomes the base for subplan C's changes, etc. This is semantically different from `LAST_WINS` (which simply returns the last subplan's content unconditionally). --- ## Actual Behavior `_sequential_apply()` uses `SequentialMergeStrategy` for each iteration: ```python # subplan_merge_service.py:228-244 def _sequential_apply(self, path, base_content, contents, subplan_ids): current = base_content for content in contents: result: MergeResult = self._seq_merge.merge(base_content, current, content) current = result.content return FileMergeOutcome(path=path, content=current, ...) ``` `SequentialMergeStrategy.merge()` is defined as: ```python # infrastructure/sandbox/merge.py:189-204 def merge(self, base: str, ours: str, theirs: str) -> MergeResult: """Return *theirs* as the merged content.""" return MergeResult(success=True, content=theirs, ...) ``` It **ignores `base` and `ours` entirely** and always returns `theirs`. Therefore, `_sequential_apply()` iterates through all contents and always sets `current = content` (the latest `theirs`), resulting in `current = contents[-1]` — identical to `LAST_WINS`. ### Verification ```python from cleveragents.application.services.subplan_merge_service import SubplanMergeService from cleveragents.domain.models.core.plan import SubplanMergeStrategy # SEQUENTIAL_APPLY svc_seq = SubplanMergeService(SubplanMergeStrategy.SEQUENTIAL_APPLY) result_seq = svc_seq.merge( base_files={"f.py": "a = 1\n"}, subplan_outputs=[ ("sp1", {"f.py": "a = 2\n"}), ("sp2", {"f.py": "a = 3\n"}), ] ) # LAST_WINS svc_lw = SubplanMergeService(SubplanMergeStrategy.LAST_WINS) result_lw = svc_lw.merge( base_files={"f.py": "a = 1\n"}, subplan_outputs=[ ("sp1", {"f.py": "a = 2\n"}), ("sp2", {"f.py": "a = 3\n"}), ] ) assert result_seq.merged_files[0].content == result_lw.merged_files[0].content # Both "a = 3\n" ``` --- ## Code Location - `src/cleveragents/application/services/subplan_merge_service.py:228–244` — `_sequential_apply()` — uses wrong merge strategy - `src/cleveragents/infrastructure/sandbox/merge.py:182–204` — `SequentialMergeStrategy` — last-write-wins, not sequential patch application --- ## Fix Required `_sequential_apply()` should use `GitMergeStrategy` (or a proper diff-based approach) to apply each subplan's changes on top of the previous result: ```python def _sequential_apply(self, path, base_content, contents, subplan_ids): current = base_content for content in contents: # Apply content as a patch on top of current result = self._git_merge.merge(current, current, content) # Or: treat current as both base and ours, content as theirs current = result.content return FileMergeOutcome(path=path, content=current, ...) ``` Alternatively, the `SequentialMergeStrategy` should be renamed to `LastWinsStrategy` to accurately reflect its behavior, and a proper sequential patch application strategy should be implemented for `SEQUENTIAL_APPLY`. --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: uat-tester

HAL9000 added the

labels

2026-04-09 15:34:43 +00:00

HAL9000 referenced this issue

2026-04-09 17:46:31 +00:00

[AUTO-GROOMER] Backlog Grooming Report (Cycle 66) #6204