From 9c8deec29627cee965b4070ab7ddfd512e6cce97 Mon Sep 17 00:00:00 2001 From: bboonnee Date: Thu, 13 Aug 2026 19:20:10 +0800 Subject: [PATCH] fix: append-mode loop_back validation must allow targeting already-persisted nodes --- src/test_work_graph_plan.py | 42 ++++++++++++++++++++++++++++++++++++++++++ src/work_graph_plan.py | 29 ++++++++++++++++++++++++----- 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/src/test_work_graph_plan.py b/src/test_work_graph_plan.py index 9f15df7..0cb23bf 100644 --- a/src/test_work_graph_plan.py +++ b/src/test_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: diff --git a/src/work_graph_plan.py b/src/work_graph_plan.py index b8fa866..f72d27f 100644 --- a/src/work_graph_plan.py +++ b/src/work_graph_plan.py @@ -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()) -- libgit2 0.21.4