fix: append-mode loop_back validation must allow targeting already-persisted nodes

Real incident (2026-08-13, task b243dfe8): _normalize_graph_manifest() calls _validate_loop_back_targets() on the newly-planned manifest BEFORE it's merged with the existing persisted one (append mode). A new node's on_failure edge legitimately targeting an already-persisted node from an earlier append (e.g. a new verification node looping back to an earlier SDD node) was wrongly rejected as "does not exist in this graph_manifest" -- it genuinely doesn't exist in the brand-new manifest by itself, only once merged. _validate_loop_back_targets() gains additional_valid_target_ids, threaded from plan_work_graph()'s existing_manifest through _normalize_graph_manifest(). Ancestor-chain validation is skipped for a target in this set -- an already-persisted node's ancestry was validated when it was originally planned. New unit test for the validation function directly, plus a real end-to-end test through plan_work_graph() in append mode. 817/817 tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

fix: append-mode loop_back validation must allow targeting already-persisted nodes
Real incident (2026-08-13, task b243dfe8): _normalize_graph_manifest() calls _validate_loop_back_targets() on the newly-planned manifest BEFORE it's merged with the existing persisted one (append mode). A new node's on_failure edge legitimately targeting an already-persisted node from an earlier append (e.g. a new verification node looping back to an earlier SDD node) was wrongly rejected as "does not exist in this graph_manifest" -- it genuinely doesn't exist in the brand-new manifest by itself, only once merged. _validate_loop_back_targets() gains additional_valid_target_ids, threaded from plan_work_graph()'s existing_manifest through _normalize_graph_manifest(). Ancestor-chain validation is skipped for a target in this set -- an already-persisted node's ancestry was validated when it was originally planned. New unit test for the validation function directly, plus a real end-to-end test through plan_work_graph() in append mode. 817/817 tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bboonnee
1 parent 1a2baa02
Showing 2 changed files with 66 additions and 5 deletions
src/test_work_graph_plan.py
src/work_graph_plan.py
@@ -286,6 +286,24 @@ class ValidateLoopBackTargetsTest(unittest.TestCase):
         with self.assertRaises(work_graph_plan.PlanningFailedError):
             work_graph_plan._normalize_graph_manifest(manifest)
  
+    def test_target_in_additional_valid_target_ids_passes(self):
+        # Real incident (2026-08-13, task b243dfe8): append mode validates a
+        # newly-planned manifest BEFORE merging it with the already-persisted
+        # one -- a new node legitimately looping back to an earlier,
+        # already-persisted node (from a prior append) must not be rejected
+        # just because that node isn't part of THIS manifest yet.
+        manifest = {
+            "nodes": [
+                {"node_id": "test_script_verify", "depends_on": [], "on_fail": {"action": "loop_back"}},
+            ],
+            "edges": [{"from": "test_script_verify", "to": "sdd", "edge_type": "on_failure"}],
+        }
+        work_graph_plan._validate_loop_back_targets(
+            manifest, additional_valid_target_ids={"sdd"}
+        )  # must not raise
+        with self.assertRaises(work_graph_plan.PlanningFailedError):
+            work_graph_plan._validate_loop_back_targets(manifest)  # without it, still rejected
+
  
 class HappyPathTest(PlanWorkGraphTestBase):
     def test_returns_run_id_and_graph_manifest(self):
@@ -509,6 +527,30 @@ class AppendToRunIdTest(PlanWorkGraphTestBase):
         )
         self.assertEqual([n["node_id"] for n in persisted["nodes"]], ["sdd"])
  
+    def test_new_node_loop_back_targeting_an_existing_node_is_allowed(self):
+        # Real incident (2026-08-13, task b243dfe8): a genuinely valid
+        # append -- a newly-planned verification node's on_failure edge
+        # targets an already-persisted node from an earlier append. This
+        # must NOT be rejected just because the earlier node isn't part of
+        # the brand-new manifest by itself (it only exists once merged).
+        self._seed_run("run-1", {"nodes": [{"node_id": "sdd", "artifact_type": "sdd", "depends_on": []}], "edges": []})
+        new_manifest = {
+            "nodes": [{
+                "node_id": "verify", "depends_on": ["sdd"], "task_prompt": "verify it",
+                "on_fail": {"action": "loop_back"},
+            }],
+            "edges": [{"from": "verify", "to": "sdd", "edge_type": "on_failure"}],
+        }
+        with patch("src.work_graph_plan.subprocess.run") as run:
+            run.return_value = _completed_process(
+                stdout_obj={"is_error": False, "result": _fenced_result(new_manifest)}
+            )
+            result = work_graph_plan.plan_work_graph(  # must not raise
+                self.conn, "coder", "task-1", "goal", "/tmp/proj-1", append_to_run_id="run-1",
+            )
+        merged_node_ids = {n["node_id"] for n in result["graph_manifest"]["nodes"]}
+        self.assertEqual(merged_node_ids, {"sdd", "verify"})
+
     def test_append_to_nonexistent_run_id_raises(self):
         new_manifest = {"nodes": [{"node_id": "impl", "depends_on": [], "task_prompt": "do it"}], "edges": []}
         with patch("src.work_graph_plan.subprocess.run") as run:
@@ -420,7 +420,7 @@ def _transitive_ancestors(graph_manifest, node_id):
     return ancestors
  
  
-def _validate_loop_back_targets(manifest):
+def _validate_loop_back_targets(manifest, additional_valid_target_ids=()):
     """2026-08-08 (Work Graph self-planning + loop-back): every node
     declaring `on_fail.action == 'loop_back'` must have exactly one
     `edge_type: 'on_failure'` edge originating from it (see
@@ -430,8 +430,22 @@ def _validate_loop_back_targets(manifest):
     PlanningFailedError (not a generic ValueError) so this surfaces the
     same way every other planning failure does, before any node ever runs
     -- catching a malformed loop_back here is far cheaper than discovering
-    it mid-execution."""
+    it mid-execution.
+
+    `additional_valid_target_ids` (2026-08-13, real incident task b243dfe8):
+    append mode (see plan_work_graph's own docstring) calls this on the
+    NEWLY-planned manifest alone, before it's merged with the existing
+    persisted one -- a new node legitimately looping back to an
+    already-persisted node from an earlier append is completely valid
+    (e.g. a new verification node targeting an earlier SDD node), but
+    without this, that target genuinely isn't in THIS manifest's own
+    node_ids yet, and got wrongly rejected as nonexistent. Ancestor-chain
+    validation below is skipped for a target in this set -- an
+    already-persisted node's ancestry was already validated when IT was
+    planned, and re-deriving `_transitive_ancestors` across a manifest that
+    doesn't yet contain those nodes isn't meaningful anyway."""
     node_ids = {n.get("node_id") for n in manifest.get("nodes", [])}
+    additional_valid_target_ids = set(additional_valid_target_ids)
     for node in manifest.get("nodes", []):
         if (node.get("on_fail") or {}).get("action") != "loop_back":
             continue
@@ -446,6 +460,8 @@ def _validate_loop_back_targets(manifest):
                 "expected exactly 1".format(node_id, len(on_failure_edges))
             )
         target = on_failure_edges[0].get("to")
+        if target in additional_valid_target_ids:
+            continue
         if target not in node_ids:
             raise PlanningFailedError(
                 "node {}'s on_failure edge targets {!r}, which does not exist in this graph_manifest".format(
@@ -459,7 +475,7 @@ def _validate_loop_back_targets(manifest):
             )
  
  
-def _normalize_graph_manifest(manifest):
+def _normalize_graph_manifest(manifest, additional_valid_loop_back_target_ids=()):
     """Real LLM output has been observed (2026-08-01, U-031 integration
     validation) to use `id` instead of the documented `node_id` key, and to
     express dependencies purely via `edges` (from/to) without ever
@@ -505,7 +521,7 @@ def _normalize_graph_manifest(manifest):
                 if node.get("node_id") == to_id and from_id not in node["depends_on"]:
                     node["depends_on"].append(from_id)
  
-    _validate_loop_back_targets(manifest)
+    _validate_loop_back_targets(manifest, additional_valid_loop_back_target_ids)
     return manifest
  
  
@@ -592,7 +608,10 @@ def plan_work_graph(conn, org_node, task_id, goal_text, target_folder_path, time
     new_manifest = _extract_graph_manifest(payload.get("result", ""))
     if not new_manifest:
         raise PlanningFailedError("could not extract a graph_manifest for task {}: {}".format(task_id, payload))
-    new_manifest = _normalize_graph_manifest(new_manifest)
+    existing_node_ids = (
+        {n["node_id"] for n in existing_manifest["nodes"]} if existing_manifest is not None else ()
+    )
+    new_manifest = _normalize_graph_manifest(new_manifest, existing_node_ids)
  
     if existing_manifest is None:
         run_id = str(uuid.uuid4())