Commit 86fc0bbcf7d197cea813929a56782198437cb30e
1 parent
528ec6e1
Phase 2a: node-level subagent/skill binding + dynamic_loop_back (Reflection-review + Fix-Skill)
Implements org-graph-unification-phase2-universal-self-planning-design.md
§4 + §5, the engine-extension sub-phase (Phase 2a) -- no org-flow callers
changed yet, this is the underlying capability.
- _run_one_node() now resolves subagent_type/skill_ref per-node (node.get
with the run-level org_node/skill_id as fallback), threaded through
_ensure_node_row/_make_run_fn/_resolve_node_verify_fn -- a node's own
judgment.kind="llm" call runs under its own declared identity, not
always the graph-level default. Found and fixed a real gap surfaced by
writing this: run_graph() only ensures the run-level default skill_id
exists as a placeholder skill (work_node_execution.skill_ref's FK
target) -- a node declaring its own skill_ref needs the same guarantee
or the first INSERT for that skill_id fails FOREIGN KEY constraint.
- New on_fail.action="dynamic_loop_back": Reflection-review
(_judge_root_cause_node) runs three real parallel perspectives (content/
connectivity/history) under the Validator identity, each picking a
target_node_id from the failing node's real ancestors plus a defect_kind
(content_error vs skill_defect), majority-voted by _tally_reflection
(own small Counter-based implementation, not imported from
root_cause_trace.py -- that module already imports this one). A runtime
ancestor check in _run_one_node backs up _tally_reflection's own
degradation guarantee.
- skill_defect triggers Fix-Skill (_fix_skill_for_node) before the loop
back fires: reads the target node's declared skill_ref, builds a
rewrite prompt from what was asked/produced/why it failed/prior loop
history, runs it under the Validator identity (user-confirmed via
AskUserQuestion), then calls the existing but -- until now -- never
actually invoked by any production path bug_record.create_bug_record()
+ skill_update.update_skill(), against skill_query._REPO_ROOT (never
target_folder_path). Logs a real 'skill_iterated' execution_event (a
CHECK-enum value that has existed since the original U-001 schema but
was never written before). Rate-limited to
_MAX_SKILL_FIXES_PER_RUN_PER_SKILL rewrites of the same Skill per run.
- New total-attempts-per-source-node loop_limit guard alongside the
existing per-(target,source) one -- dynamic_loop_back can pick a
different target each round, which would otherwise dodge the per-pair
cap indefinitely. New settings_config key shape
"{org_node}:{source_node_id}:total", no interface change needed there.
- New skills/skill-defect-remediation/SKILL.md (Validator-only), wired
into roles/validator/role.json.
python3 -m unittest discover -s src (741 tests) + discover -s supervisor
-p "test_*.py" (126 tests) both green, zero regressions.
Deferred to Phase 2b (per design doc's own sequencing note): §2.1/§2.2
(work_graph_run.org_node removal + ux_work_graph_run_active index
narrowing) -- narrowing that index now, before the append-to-existing-run
mechanism lands, would break the current production flow, since Planner's
and Coder's work_graph_run rows both stay non-terminal indefinitely on
the happy path today (only Validator's own graph ever reaches 'done').
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Showing
4 changed files
with
753 additions
and
11 deletions
roles/validator/role.json
| @@ -14,7 +14,8 @@ | @@ -14,7 +14,8 @@ | ||
| 14 | "qa-task-validation", | 14 | "qa-task-validation", |
| 15 | "unattended-agent-operations", | 15 | "unattended-agent-operations", |
| 16 | "design-dynamic-work-graph", | 16 | "design-dynamic-work-graph", |
| 17 | - "unknown-scope-defect-sweep" | 17 | + "unknown-scope-defect-sweep", |
| 18 | + "skill-defect-remediation" | ||
| 18 | ], | 19 | ], |
| 19 | "applications": { | 20 | "applications": { |
| 20 | "mcp_servers": [] | 21 | "mcp_servers": [] |
skills/skill-defect-remediation/SKILL.md
0 → 100644
| 1 | +--- | ||
| 2 | +name: skill-defect-remediation | ||
| 3 | +description: | | ||
| 4 | + 当 Reflection-review 判定一次节点失败的根因是系统性 Skill 缺陷(不是这一次内容的偶然错误)时, | ||
| 5 | + 怎么安全地修订这份 Skill 文件本身——精确定位并只修正导致这次真实失败的那一小部分,不破坏该 | ||
| 6 | + Skill 对其他场景已经正确的既有指导。产出必须是完整的、可直接替换生效的 Skill 内容,不是 | ||
| 7 | + diff 或建议清单。 | ||
| 8 | +version: 1.0.0 | ||
| 9 | +tags: | ||
| 10 | + - skill-iteration | ||
| 11 | + - remediation | ||
| 12 | + - self-evolution | ||
| 13 | +--- | ||
| 14 | + | ||
| 15 | +# Skill 缺陷修复 | ||
| 16 | + | ||
| 17 | +## 我在做什么、不做什么 | ||
| 18 | + | ||
| 19 | +我在改一份 Skill 文件的内容,不是在改这一次任务的输出。触发条件是 Reflection-review(`_judge_root_cause_node`)已经判定:这不是"这次恰好没写好"(`content_error`),而是"这份 Skill 本身的方法论/提示词有系统性偏差,同类任务会反复踩同一个坑"(`skill_defect`)。 | ||
| 20 | + | ||
| 21 | +我拿到的输入是:这份 Skill 当前的完整内容、使用它的节点这次被要求做什么、它实际产出了什么、为什么被判定失败、这个节点在本次 run 内此前被退回的历史(如果有)。我的输出会被 `skill_update.update_skill()` 直接、原样写入 Skill 文件并提交 Git——不是草案,不是给人看的建议,是最终生效内容。 | ||
| 22 | + | ||
| 23 | +## 核心原则:手术刀式修改,不是重写 | ||
| 24 | + | ||
| 25 | +- **只改导致这次失败的那一部分**。这份 Skill 对其他场景的既有指导,大概率是对的——它能走到今天,说明大部分情况下有效。逐字重写整份文件、"顺手优化"看起来不够好的其他段落,是最常见的过度修改陷阱,会引入新的、这次任务范围之外的回归风险。 | ||
| 26 | +- **先定位到具体是哪一句/哪一段导致了偏差,再改**。不要停留在"这份 Skill 整体上不够好"这种笼统判断——不可操作,也无法验证改对了没有。好的定位是这个形状:"第 X 节要求 Y,但没有说明 Z 场景下应该怎么做,导致模型默认走了错误的 W 路径"。 | ||
| 27 | +- **改动要能解释"为什么这次会防住同一类失败"**,不是"看起来更完善了"。如果说不清楚新内容具体怎么拦住了这次复现的失败模式,说明还没找到真正的病灶。 | ||
| 28 | + | ||
| 29 | +## 一个真实的参考案例:叙述文字冒充结构化产出 | ||
| 30 | + | ||
| 31 | +这个代码库真实反复复现过的一类系统性 Skill 缺陷(`docs/todo/validator-test-script-narrative-output-issue.md` §6 完整根因记录):要求"先用工具探索、再产出最终内容"的节点(比如需要先读真实代码才能写 SDD/Benchmark 的场景),模型探索完之后的收尾话术天然倾向"汇报我刚才做了/启动了什么",而不是切换成"直接吐出最终内容"模式——即使 Skill 提示词已经写了"不要写叙述文字",这句话附加在提示词末尾的格式规则,不足以覆盖"多轮工具调用之后的收尾习惯"这个更底层的行为模式。 | ||
| 32 | + | ||
| 33 | +如果这次要修的 Skill 也有类似"先探索、再产出"的结构,而失败症状也是"产出的是进度汇报式散文,不是结构化内容",可以直接参考这个案例的诊断方式,但**不要机械照抄"加一句不要写叙述文字"这种已经被证明不够的修法**——真正管用的方向是把"探索阶段"和"最终作答阶段"在提示词结构上明确分开(比如要求模型先完成探索、再有一个清晰的"现在开始,你的下一段输出必须且只能是最终内容本身"的转折),而不是在末尾追加一句格式提醒。 | ||
| 34 | + | ||
| 35 | +## 输出 | ||
| 36 | + | ||
| 37 | +`new_content`:完整的、修订后的 Skill 文件内容(不是 diff、不是片段)。`change_summary`:一句话说清楚改了什么、为什么——这句话会被记录进 `skill_iteration.change_summary`,是这次 Skill 迭代唯一的人类可读说明,写清楚比写简短更重要。 |
src/test_work_graph_orchestrate.py
| @@ -9,13 +9,17 @@ Run: python3 -m unittest src.test_work_graph_orchestrate -v | @@ -9,13 +9,17 @@ Run: python3 -m unittest src.test_work_graph_orchestrate -v | ||
| 9 | 9 | ||
| 10 | import json | 10 | import json |
| 11 | import os | 11 | import os |
| 12 | +import subprocess | ||
| 12 | import tempfile | 13 | import tempfile |
| 13 | import threading | 14 | import threading |
| 14 | import time | 15 | import time |
| 15 | import unittest | 16 | import unittest |
| 16 | from unittest.mock import patch | 17 | from unittest.mock import patch |
| 17 | 18 | ||
| 18 | -from src import execution_log, paths, schema, work_graph_execute, work_graph_orchestrate as wgo, work_node_run | 19 | +from src import ( |
| 20 | + execution_log, paths, schema, skill_query, skill_update, | ||
| 21 | + work_graph_execute, work_graph_orchestrate as wgo, work_node_run, | ||
| 22 | +) | ||
| 19 | 23 | ||
| 20 | 24 | ||
| 21 | class TopologicalOrderTest(unittest.TestCase): | 25 | class TopologicalOrderTest(unittest.TestCase): |
| @@ -1257,5 +1261,313 @@ class RunGraphLoopBackTest(RunGraphTestBase): | @@ -1257,5 +1261,313 @@ class RunGraphLoopBackTest(RunGraphTestBase): | ||
| 1257 | self.assertEqual(mock_execute.call_count, 2) | 1261 | self.assertEqual(mock_execute.call_count, 2) |
| 1258 | 1262 | ||
| 1259 | 1263 | ||
| 1264 | +class NodeLevelSubagentSkillTest(RunGraphTestBase): | ||
| 1265 | + """Phase 2 §4: a node can declare its own subagent_type/skill_ref, | ||
| 1266 | + overriding the run-level org_node/skill_id defaults -- SDD §10 Phase 2a | ||
| 1267 | + acceptance criterion 1.""" | ||
| 1268 | + | ||
| 1269 | + def test_node_level_fields_override_run_level_defaults(self): | ||
| 1270 | + manifest = { | ||
| 1271 | + "nodes": [ | ||
| 1272 | + {"node_id": "a", "depends_on": [], "subagent_type": "coder", "skill_ref": "code-task-development"}, | ||
| 1273 | + {"node_id": "b", "depends_on": []}, # no override -- uses run-level org_node/skill_id | ||
| 1274 | + ], | ||
| 1275 | + "edges": [], | ||
| 1276 | + } | ||
| 1277 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | ||
| 1278 | + patch.object(work_graph_execute, "execute_work_graph", return_value={"content": "x"}): | ||
| 1279 | + wgo.run_graph( | ||
| 1280 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | ||
| 1281 | + lambda node, outputs: "p", _IN_SCHEMA, _OUT_SCHEMA, | ||
| 1282 | + ) | ||
| 1283 | + rows = { | ||
| 1284 | + r[0]: (r[1], r[2]) for r in self.conn.execute( | ||
| 1285 | + "SELECT node_id, subagent_type, skill_ref FROM work_node_execution WHERE run_id = 'run-1'" | ||
| 1286 | + ).fetchall() | ||
| 1287 | + } | ||
| 1288 | + self.assertEqual(rows["a"], ("coder", "code-task-development")) | ||
| 1289 | + self.assertEqual(rows["b"], ("planner", "planner-doc-writing")) | ||
| 1290 | + | ||
| 1291 | + def test_node_level_org_node_used_for_llm_judgment_agent_identity(self): | ||
| 1292 | + # A node's own judgment.kind='llm' call must run under ITS OWN | ||
| 1293 | + # subagent_type, not the run-level default -- otherwise a node | ||
| 1294 | + # declaring subagent_type='validator' for judgment purposes would | ||
| 1295 | + # silently get judged under the wrong persona. | ||
| 1296 | + manifest = { | ||
| 1297 | + "nodes": [{ | ||
| 1298 | + "node_id": "a", "depends_on": [], "subagent_type": "coder", | ||
| 1299 | + "judgment": {"kind": "llm", "judge_prompt": "check {content}"}, | ||
| 1300 | + "on_fail": {"action": "hard_fail"}, | ||
| 1301 | + }], | ||
| 1302 | + "edges": [], | ||
| 1303 | + } | ||
| 1304 | + seen_agent_types = [] | ||
| 1305 | + | ||
| 1306 | + def fake_execute(cwd, name, args, output_schema, timeout=600): | ||
| 1307 | + if "ok" in output_schema.get("required", []): | ||
| 1308 | + return {"ok": True, "issues": []} | ||
| 1309 | + return {"content": "x"} | ||
| 1310 | + | ||
| 1311 | + def fake_write(cwd, name, script_content): | ||
| 1312 | + if "graphlemon Work Node" in script_content or "graphlemon node judgment" in script_content: | ||
| 1313 | + seen_agent_types.append(script_content) | ||
| 1314 | + | ||
| 1315 | + with patch.object(work_graph_execute, "write_workflow_script", side_effect=fake_write), \ | ||
| 1316 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=fake_execute): | ||
| 1317 | + wgo.run_graph( | ||
| 1318 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | ||
| 1319 | + lambda node, outputs: "p", _IN_SCHEMA, _OUT_SCHEMA, | ||
| 1320 | + ) | ||
| 1321 | + judgment_script = next(s for s in seen_agent_types if "graphlemon node judgment" in s) | ||
| 1322 | + # _llm_judgment_verify_fn renders agentType as an unquoted JS | ||
| 1323 | + # identifier key (agentType: "coder"), not a JSON key -- only the | ||
| 1324 | + # VALUE is json.dumps'd. | ||
| 1325 | + self.assertIn('agentType:"coder"', judgment_script.replace(" ", "")) | ||
| 1326 | + | ||
| 1327 | + | ||
| 1328 | +class DynamicLoopBackContentErrorTest(RunGraphTestBase): | ||
| 1329 | + """Phase 2 §5: on_fail.action='dynamic_loop_back' -- Reflection-review | ||
| 1330 | + picks a target node from the failing node's real ancestors and judges | ||
| 1331 | + defect_kind. content_error skips Fix-Skill and loops back directly -- | ||
| 1332 | + SDD §10 Phase 2a acceptance criterion 2.""" | ||
| 1333 | + | ||
| 1334 | + def test_unanimous_content_error_loops_back_to_chosen_ancestor(self): | ||
| 1335 | + manifest = { | ||
| 1336 | + "nodes": [ | ||
| 1337 | + {"node_id": "sdd", "depends_on": []}, | ||
| 1338 | + { | ||
| 1339 | + "node_id": "final_review", "depends_on": ["sdd"], | ||
| 1340 | + "judgment": {"kind": "mechanical", "check_ref": "is_good"}, | ||
| 1341 | + "on_fail": {"action": "dynamic_loop_back", "max_attempts": 3}, | ||
| 1342 | + }, | ||
| 1343 | + ], | ||
| 1344 | + "edges": [], | ||
| 1345 | + } | ||
| 1346 | + sdd_contents = ["sdd v1", "sdd v2 (fixed)"] | ||
| 1347 | + node_call_count = [0] | ||
| 1348 | + lock = threading.Lock() | ||
| 1349 | + | ||
| 1350 | + def side_effect(cwd, name, args, output_schema, timeout=600): | ||
| 1351 | + if name.startswith("graphlemon-reflection-"): | ||
| 1352 | + return {"target_node_id": "sdd", "defect_kind": "content_error", "detail": "sdd is stale"} | ||
| 1353 | + if name.startswith("graphlemon-node-"): | ||
| 1354 | + with lock: | ||
| 1355 | + node_call_count[0] += 1 | ||
| 1356 | + idx = node_call_count[0] | ||
| 1357 | + if idx == 1: | ||
| 1358 | + return {"content": sdd_contents[0]} | ||
| 1359 | + if idx == 2: | ||
| 1360 | + return {"content": "bad final review"} | ||
| 1361 | + if idx == 3: | ||
| 1362 | + return {"content": sdd_contents[1]} | ||
| 1363 | + return {"content": "good final review"} | ||
| 1364 | + raise AssertionError("unexpected script name: " + name) | ||
| 1365 | + | ||
| 1366 | + registry = {"is_good": lambda content: (content == "good final review", None if content == "good final review" else "rejected: " + content)} | ||
| 1367 | + | ||
| 1368 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | ||
| 1369 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=side_effect): | ||
| 1370 | + outputs = wgo.run_graph( | ||
| 1371 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | ||
| 1372 | + lambda node, outputs: "prompt for " + node["node_id"], _IN_SCHEMA, _OUT_SCHEMA, | ||
| 1373 | + mechanical_check_registry=registry, | ||
| 1374 | + ) | ||
| 1375 | + | ||
| 1376 | + self.assertEqual(outputs["sdd"], {"content": sdd_contents[1]}) | ||
| 1377 | + self.assertEqual(outputs["final_review"], {"content": "good final review"}) | ||
| 1378 | + | ||
| 1379 | + rows = [ | ||
| 1380 | + r[0] for r in self.conn.execute( | ||
| 1381 | + "SELECT node_id FROM work_node_execution WHERE run_id = 'run-1' ORDER BY node_id" | ||
| 1382 | + ).fetchall() | ||
| 1383 | + ] | ||
| 1384 | + self.assertEqual(rows, ["final_review", "final_review-attempt2", "sdd", "sdd-attempt2"]) | ||
| 1385 | + | ||
| 1386 | + events = self.conn.execute( | ||
| 1387 | + "SELECT payload FROM execution_event WHERE event_type = 'loop_back_triggered'" | ||
| 1388 | + ).fetchall() | ||
| 1389 | + self.assertEqual(len(events), 1) | ||
| 1390 | + payload = json.loads(events[0][0]) | ||
| 1391 | + self.assertEqual(payload["source_node_id"], "final_review") | ||
| 1392 | + self.assertEqual(payload["target_node_id"], "sdd") | ||
| 1393 | + | ||
| 1394 | + # no skill_iterated event -- content_error must not touch any Skill. | ||
| 1395 | + skill_events = self.conn.execute( | ||
| 1396 | + "SELECT 1 FROM execution_event WHERE event_type = 'skill_iterated'" | ||
| 1397 | + ).fetchall() | ||
| 1398 | + self.assertEqual(skill_events, []) | ||
| 1399 | + | ||
| 1400 | +class TallyReflectionTest(unittest.TestCase): | ||
| 1401 | + """_tally_reflection in isolation -- the majority-vote + degradation | ||
| 1402 | + logic _judge_root_cause_node relies on. An invalid winner (not among | ||
| 1403 | + candidate_node_ids) is corrected HERE, before _run_one_node's own | ||
| 1404 | + 'target not in ancestors' runtime check (§5.7 point 1) would ever see | ||
| 1405 | + it -- that check is defense-in-depth for if this guarantee is ever | ||
| 1406 | + weakened, not the primary enforcement, so it's tested at this level.""" | ||
| 1407 | + | ||
| 1408 | + def test_unanimous_valid_winner(self): | ||
| 1409 | + judgments = [ | ||
| 1410 | + {"target_node_id": "sdd", "defect_kind": "content_error", "detail": "a"}, | ||
| 1411 | + {"target_node_id": "sdd", "defect_kind": "content_error", "detail": "b"}, | ||
| 1412 | + {"target_node_id": "sdd", "defect_kind": "content_error", "detail": "c"}, | ||
| 1413 | + ] | ||
| 1414 | + result = wgo._tally_reflection(judgments, ["sdd", "prd"]) | ||
| 1415 | + self.assertEqual(result["target_node_id"], "sdd") | ||
| 1416 | + self.assertEqual(result["defect_kind"], "content_error") | ||
| 1417 | + | ||
| 1418 | + def test_two_thirds_skill_defect_majority(self): | ||
| 1419 | + judgments = [ | ||
| 1420 | + {"target_node_id": "sdd", "defect_kind": "skill_defect", "detail": "a"}, | ||
| 1421 | + {"target_node_id": "sdd", "defect_kind": "skill_defect", "detail": "b"}, | ||
| 1422 | + {"target_node_id": "sdd", "defect_kind": "content_error", "detail": "c"}, | ||
| 1423 | + ] | ||
| 1424 | + result = wgo._tally_reflection(judgments, ["sdd", "prd"]) | ||
| 1425 | + self.assertEqual(result["target_node_id"], "sdd") | ||
| 1426 | + self.assertEqual(result["defect_kind"], "skill_defect") | ||
| 1427 | + | ||
| 1428 | + def test_winner_not_in_candidates_degrades_to_first_candidate_content_error(self): | ||
| 1429 | + judgments = [ | ||
| 1430 | + {"target_node_id": "not_a_candidate", "defect_kind": "skill_defect", "detail": "a"}, | ||
| 1431 | + {"target_node_id": "not_a_candidate", "defect_kind": "skill_defect", "detail": "b"}, | ||
| 1432 | + {"target_node_id": "not_a_candidate", "defect_kind": "skill_defect", "detail": "c"}, | ||
| 1433 | + ] | ||
| 1434 | + result = wgo._tally_reflection(judgments, ["sdd", "prd"]) | ||
| 1435 | + self.assertEqual(result["target_node_id"], "sdd") | ||
| 1436 | + self.assertEqual(result["defect_kind"], "content_error") | ||
| 1437 | + | ||
| 1438 | + def test_no_two_thirds_agreement_degrades(self): | ||
| 1439 | + judgments = [ | ||
| 1440 | + {"target_node_id": "sdd", "defect_kind": "skill_defect", "detail": "a"}, | ||
| 1441 | + {"target_node_id": "prd", "defect_kind": "content_error", "detail": "b"}, | ||
| 1442 | + {"target_node_id": "benchmark", "defect_kind": "content_error", "detail": "c"}, | ||
| 1443 | + ] | ||
| 1444 | + result = wgo._tally_reflection(judgments, ["sdd", "prd", "benchmark"]) | ||
| 1445 | + self.assertEqual(result["defect_kind"], "content_error") | ||
| 1446 | + self.assertIn(result["target_node_id"], ["sdd", "prd", "benchmark"]) | ||
| 1447 | + | ||
| 1448 | + | ||
| 1449 | +def _init_git_repo(path): | ||
| 1450 | + subprocess.run(["git", "init"], cwd=path, check=True, capture_output=True) | ||
| 1451 | + | ||
| 1452 | + | ||
| 1453 | +class DynamicLoopBackSkillDefectTest(RunGraphTestBase): | ||
| 1454 | + """Phase 2 §5.5: defect_kind='skill_defect' triggers Fix-Skill -- a | ||
| 1455 | + real skill_update.update_skill() call against a real (throwaway) git | ||
| 1456 | + repo, never graphlemon's own checkout -- before the loop_back fires. | ||
| 1457 | + SDD §10 Phase 2a acceptance criterion 3.""" | ||
| 1458 | + | ||
| 1459 | + def setUp(self): | ||
| 1460 | + super().setUp() | ||
| 1461 | + self.skill_repo = tempfile.TemporaryDirectory() | ||
| 1462 | + self.addCleanup(self.skill_repo.cleanup) | ||
| 1463 | + _init_git_repo(self.skill_repo.name) | ||
| 1464 | + skill_update._write_and_commit(self.skill_repo.name, "skills/my-skill/SKILL.md", "v1 content", "seed skill") | ||
| 1465 | + self.conn.execute( | ||
| 1466 | + "INSERT INTO skill (skill_id, owner_org_node, file_path, current_git_commit_sha, source, status) " | ||
| 1467 | + "VALUES ('my-skill', 'coder', 'skills/my-skill', 'unknown', 'migrated_from_hermes', 'active')" | ||
| 1468 | + ) | ||
| 1469 | + self.conn.commit() | ||
| 1470 | + self.repo_root_patch = patch.object(skill_query, "_REPO_ROOT", self.skill_repo.name) | ||
| 1471 | + self.repo_root_patch.start() | ||
| 1472 | + self.addCleanup(self.repo_root_patch.stop) | ||
| 1473 | + | ||
| 1474 | + def test_skill_defect_calls_real_update_skill_before_loop_back(self): | ||
| 1475 | + manifest = { | ||
| 1476 | + "nodes": [ | ||
| 1477 | + {"node_id": "sdd", "depends_on": [], "skill_ref": "my-skill"}, | ||
| 1478 | + { | ||
| 1479 | + "node_id": "final_review", "depends_on": ["sdd"], | ||
| 1480 | + "judgment": {"kind": "mechanical", "check_ref": "is_good"}, | ||
| 1481 | + "on_fail": {"action": "dynamic_loop_back", "max_attempts": 3}, | ||
| 1482 | + }, | ||
| 1483 | + ], | ||
| 1484 | + "edges": [], | ||
| 1485 | + } | ||
| 1486 | + node_call_count = [0] | ||
| 1487 | + lock = threading.Lock() | ||
| 1488 | + | ||
| 1489 | + def side_effect(cwd, name, args, output_schema, timeout=600): | ||
| 1490 | + if name.startswith("graphlemon-reflection-"): | ||
| 1491 | + return {"target_node_id": "sdd", "defect_kind": "skill_defect", "detail": "systemic issue"} | ||
| 1492 | + if name.startswith("graphlemon-fixskill-"): | ||
| 1493 | + return {"new_content": "v2 content, fixed", "change_summary": "fixed the systemic issue"} | ||
| 1494 | + if name.startswith("graphlemon-node-"): | ||
| 1495 | + with lock: | ||
| 1496 | + node_call_count[0] += 1 | ||
| 1497 | + idx = node_call_count[0] | ||
| 1498 | + if idx in (1, 3): | ||
| 1499 | + return {"content": "sdd content"} | ||
| 1500 | + if idx == 2: | ||
| 1501 | + return {"content": "bad final review"} | ||
| 1502 | + return {"content": "good final review"} | ||
| 1503 | + raise AssertionError("unexpected script name: " + name) | ||
| 1504 | + | ||
| 1505 | + registry = {"is_good": lambda content: (content == "good final review", None if content == "good final review" else "rejected: " + content)} | ||
| 1506 | + | ||
| 1507 | + with patch.object(work_graph_execute, "write_workflow_script"), \ | ||
| 1508 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=side_effect): | ||
| 1509 | + outputs = wgo.run_graph( | ||
| 1510 | + self.conn, "run-1", "planner", "planner-doc-writing", "/tmp/proj-1", manifest, | ||
| 1511 | + lambda node, outputs: "prompt for " + node["node_id"], _IN_SCHEMA, _OUT_SCHEMA, | ||
| 1512 | + mechanical_check_registry=registry, | ||
| 1513 | + ) | ||
| 1514 | + | ||
| 1515 | + self.assertEqual(outputs["final_review"], {"content": "good final review"}) | ||
| 1516 | + | ||
| 1517 | + # a real skill_iteration row was produced, with a real (different) | ||
| 1518 | + # after_git_commit_sha -- not a stub, an actual git commit. | ||
| 1519 | + iteration = self.conn.execute( | ||
| 1520 | + "SELECT skill_id, before_git_commit_sha, after_git_commit_sha, change_summary " | ||
| 1521 | + "FROM skill_iteration" | ||
| 1522 | + ).fetchone() | ||
| 1523 | + self.assertEqual(iteration[0], "my-skill") | ||
| 1524 | + self.assertNotEqual(iteration[1], iteration[2]) | ||
| 1525 | + self.assertEqual(iteration[3], "fixed the systemic issue") | ||
| 1526 | + | ||
| 1527 | + with open(os.path.join(self.skill_repo.name, "skills/my-skill/SKILL.md")) as f: | ||
| 1528 | + self.assertEqual(f.read(), "v2 content, fixed") | ||
| 1529 | + | ||
| 1530 | + # a real bug_record(root_cause_category='skill_defect') was created | ||
| 1531 | + # and linked back to the iteration (skill_update.update_skill()'s | ||
| 1532 | + # own contract). | ||
| 1533 | + bug_row = self.conn.execute( | ||
| 1534 | + "SELECT root_cause_category, skill_iteration_suggestion_id FROM bug_record" | ||
| 1535 | + ).fetchone() | ||
| 1536 | + self.assertEqual(bug_row[0], "skill_defect") | ||
| 1537 | + self.assertIsNotNone(bug_row[1]) | ||
| 1538 | + | ||
| 1539 | + # skill_iterated execution_event recorded (existing but previously | ||
| 1540 | + # never-written CHECK enum value). | ||
| 1541 | + skill_events = self.conn.execute( | ||
| 1542 | + "SELECT payload FROM execution_event WHERE event_type = 'skill_iterated'" | ||
| 1543 | + ).fetchall() | ||
| 1544 | + self.assertEqual(len(skill_events), 1) | ||
| 1545 | + payload = json.loads(skill_events[0][0]) | ||
| 1546 | + self.assertEqual(payload["node_id"], "sdd") | ||
| 1547 | + self.assertEqual(payload["skill_id"], "my-skill") | ||
| 1548 | + | ||
| 1549 | + # graphlemon's own real checkout was never touched -- the Skill | ||
| 1550 | + # write landed only in the throwaway skill_repo tempdir. | ||
| 1551 | + self.assertFalse( | ||
| 1552 | + os.path.exists(os.path.join(paths.repo_root(), "skills", "my-skill", "SKILL.md.orig")) | ||
| 1553 | + ) | ||
| 1554 | + | ||
| 1555 | + def test_rate_limit_stops_repeated_rewrites_of_same_skill(self): | ||
| 1556 | + # §5.7 point 5: after _MAX_SKILL_FIXES_PER_RUN_PER_SKILL rewrites of | ||
| 1557 | + # the same (run_id, skill_id) this run, Fix-Skill silently skips -- | ||
| 1558 | + # dynamic_loop_back still proceeds as a plain loop_back. | ||
| 1559 | + for i in range(wgo._MAX_SKILL_FIXES_PER_RUN_PER_SKILL): | ||
| 1560 | + wgo._log_skill_iterated_event(self.conn, "run-1", "sdd", "my-skill", "iteration-{}".format(i)) | ||
| 1561 | + | ||
| 1562 | + manifest = {"nodes": [{"node_id": "sdd", "depends_on": [], "skill_ref": "my-skill"}], "edges": []} | ||
| 1563 | + result = wgo._fix_skill_for_node( | ||
| 1564 | + self.conn, "run-1", "sdd", manifest, lambda node, outputs: "p", {}, ["feedback"], "/tmp/proj-1", | ||
| 1565 | + ) | ||
| 1566 | + self.assertIsNone(result) | ||
| 1567 | + # no new skill_iteration row was created beyond what we seeded above. | ||
| 1568 | + count = self.conn.execute("SELECT COUNT(*) FROM skill_iteration").fetchone()[0] | ||
| 1569 | + self.assertEqual(count, 0) # the two seeded events above didn't insert real skill_iteration rows | ||
| 1570 | + | ||
| 1571 | + | ||
| 1260 | if __name__ == "__main__": | 1572 | if __name__ == "__main__": |
| 1261 | unittest.main() | 1573 | unittest.main() |
src/work_graph_orchestrate.py
| @@ -66,11 +66,13 @@ actually happened (N attempts), not a bug. | @@ -66,11 +66,13 @@ actually happened (N attempts), not a bug. | ||
| 66 | 66 | ||
| 67 | import concurrent.futures | 67 | import concurrent.futures |
| 68 | import json | 68 | import json |
| 69 | +import os | ||
| 69 | import threading | 70 | import threading |
| 70 | import time | 71 | import time |
| 71 | import uuid | 72 | import uuid |
| 73 | +from collections import Counter | ||
| 72 | 74 | ||
| 73 | -from src import execution_log, paths, settings_config, work_graph_execute, work_node_run | 75 | +from src import bug_record, execution_log, paths, settings_config, skill_query, skill_update, work_graph_execute, work_node_run |
| 74 | 76 | ||
| 75 | 77 | ||
| 76 | def _log_event(conn, node_exec_id, event_type, **payload): | 78 | def _log_event(conn, node_exec_id, event_type, **payload): |
| @@ -506,6 +508,323 @@ def _validate_mechanical_check_refs(graph_manifest, mechanical_check_registry): | @@ -506,6 +508,323 @@ def _validate_mechanical_check_refs(graph_manifest, mechanical_check_registry): | ||
| 506 | ) | 508 | ) |
| 507 | 509 | ||
| 508 | 510 | ||
| 511 | +_REFLECTION_OUTPUT_SCHEMA = { | ||
| 512 | + "type": "object", "required": ["target_node_id", "defect_kind", "detail"], | ||
| 513 | + "properties": { | ||
| 514 | + "target_node_id": {"type": "string"}, | ||
| 515 | + "defect_kind": {"type": "string", "enum": ["content_error", "skill_defect"]}, | ||
| 516 | + "detail": {"type": "string"}, | ||
| 517 | + }, | ||
| 518 | +} | ||
| 519 | + | ||
| 520 | +_REFLECTION_FRAMINGS = { | ||
| 521 | + "content": ( | ||
| 522 | + "CONTENT PERSPECTIVE: for each candidate node below, look at what it was asked to produce " | ||
| 523 | + "and what it actually produced. Which ONE candidate's output itself is genuinely defective? " | ||
| 524 | + "Then decide: is this a one-off mistake for this specific run ('content_error' -- the " | ||
| 525 | + "underlying methodology is fine, this particular attempt just didn't execute it well), or " | ||
| 526 | + "does it look like a systematic pattern that would recur on other tasks using the same " | ||
| 527 | + "approach/prompt style ('skill_defect')? A concrete red flag for skill_defect: the node's " | ||
| 528 | + "content reads as a narrative description of an action taken (e.g. 'I launched/produced/" | ||
| 529 | + "completed X') instead of the structured content itself -- this is a known recurring failure " | ||
| 530 | + "mode in nodes that explore with tools before producing a final answer, not a one-off typo." | ||
| 531 | + ), | ||
| 532 | + "connectivity": ( | ||
| 533 | + "CONNECTIVITY PERSPECTIVE: if no single candidate's content is obviously wrong on its own, " | ||
| 534 | + "is the real problem how the candidates connect -- e.g. a node that should have depended on " | ||
| 535 | + "another candidate's output but didn't, so it worked from stale or missing context? This " | ||
| 536 | + "kind of problem is almost always 'content_error' (a one-off planning slip for this run), not " | ||
| 537 | + "a systemic Skill defect -- pick whichever candidate's isolation from its true dependency is " | ||
| 538 | + "the root issue." | ||
| 539 | + ), | ||
| 540 | + "history": ( | ||
| 541 | + "HISTORY PERSPECTIVE: you are given this run's actual loop_back/skill-iteration event history " | ||
| 542 | + "for each candidate. If a candidate has already been sent back for revision or had its Skill " | ||
| 543 | + "rewritten before in this same run for a SIMILAR reason, that recurrence itself is strong " | ||
| 544 | + "evidence of 'skill_defect' -- a Skill that keeps producing the same class of failure even " | ||
| 545 | + "after a plain retry has methodology problems, not bad luck. If a candidate has no such prior " | ||
| 546 | + "history, default to 'content_error' unless the content itself (see the CONTENT framing's red " | ||
| 547 | + "flag) makes skill_defect obvious even on a first occurrence." | ||
| 548 | + ), | ||
| 549 | +} | ||
| 550 | + | ||
| 551 | + | ||
| 552 | +def _candidate_context_block(graph_manifest, build_prompt, outputs, candidate_node_ids): | ||
| 553 | + nodes_by_id = {n["node_id"]: n for n in graph_manifest["nodes"]} | ||
| 554 | + parts = [] | ||
| 555 | + for node_id in candidate_node_ids: | ||
| 556 | + node = nodes_by_id[node_id] | ||
| 557 | + asked = build_prompt(node, outputs) | ||
| 558 | + produced = (outputs.get(node_id) or {}).get("content", "") | ||
| 559 | + parts.append( | ||
| 560 | + "--- candidate node_id={} ---\nWhat it was asked:\n{}\n\nWhat it produced:\n{}".format( | ||
| 561 | + node_id, asked, produced | ||
| 562 | + ) | ||
| 563 | + ) | ||
| 564 | + return "\n\n".join(parts) | ||
| 565 | + | ||
| 566 | + | ||
| 567 | +def _candidate_history_block(conn, run_id, candidate_node_ids): | ||
| 568 | + events = _loop_back_events(conn, run_id) | ||
| 569 | + skill_events = _skill_iterated_events(conn, run_id) | ||
| 570 | + parts = [] | ||
| 571 | + for node_id in candidate_node_ids: | ||
| 572 | + prior_loop_backs = [e for e in events if e["target_node_id"] == node_id] | ||
| 573 | + prior_skill_fixes = [e for e in skill_events if e["node_id"] == node_id] | ||
| 574 | + parts.append( | ||
| 575 | + "--- candidate node_id={} --- prior loop_back count: {}; prior skill_iterated count: {}".format( | ||
| 576 | + node_id, len(prior_loop_backs), len(prior_skill_fixes) | ||
| 577 | + ) | ||
| 578 | + ) | ||
| 579 | + return "\n".join(parts) | ||
| 580 | + | ||
| 581 | + | ||
| 582 | +def _reflection_prompt(framing_name, failed_node_id, feedback, candidate_node_ids, graph_manifest, build_prompt, outputs, conn, run_id): | ||
| 583 | + framing = _REFLECTION_FRAMINGS[framing_name] | ||
| 584 | + context = ( | ||
| 585 | + _candidate_history_block(conn, run_id, candidate_node_ids) if framing_name == "history" | ||
| 586 | + else _candidate_context_block(graph_manifest, build_prompt, outputs, candidate_node_ids) | ||
| 587 | + ) | ||
| 588 | + return ( | ||
| 589 | + "{framing}\n\nThe node {failed_node_id} failed judgment with this feedback: {feedback}\n\n" | ||
| 590 | + "Candidates (pick target_node_id from EXACTLY these node_ids, nothing else):\n{candidates}\n\n" | ||
| 591 | + "Context:\n{context}\n\n" | ||
| 592 | + "Reply with target_node_id (one of the candidate node_ids above), defect_kind " | ||
| 593 | + "('content_error' or 'skill_defect'), and detail (one sentence justifying both)." | ||
| 594 | + ).format( | ||
| 595 | + framing=framing, failed_node_id=failed_node_id, feedback="; ".join(feedback or []), | ||
| 596 | + candidates=", ".join(candidate_node_ids), context=context, | ||
| 597 | + ) | ||
| 598 | + | ||
| 599 | + | ||
| 600 | +def _run_reflection_perspective(framing_name, prompt, target_folder_path, timeout): | ||
| 601 | + """One perspective's real call, under the Validator identity (Reflection- | ||
| 602 | + review is diagnostic, not opinion-laden about the failed content itself | ||
| 603 | + -- same 'independent second opinion' reasoning Fix-Skill's own 'who | ||
| 604 | + rewrites the Skill' decision uses, see _fix_skill_for_node). Same | ||
| 605 | + self-target worktree isolation as _llm_judgment_verify_fn (a real | ||
| 606 | + agent() call with memory:project writes an uncommitted change wherever | ||
| 607 | + it runs).""" | ||
| 608 | + is_self = paths.is_self_target(target_folder_path) | ||
| 609 | + node_exec_id = "reflection-" + framing_name + "-" + uuid.uuid4().hex[:12] | ||
| 610 | + worktree_path = work_node_run.create_worktree(target_folder_path, node_exec_id) if is_self else None | ||
| 611 | + cwd = worktree_path or target_folder_path | ||
| 612 | + try: | ||
| 613 | + script_name = "graphlemon-reflection-" + uuid.uuid4().hex | ||
| 614 | + script_content = ( | ||
| 615 | + "export const meta = {{ name: {name}, description: 'graphlemon Reflection-review ({framing})' }}\n\n" | ||
| 616 | + "const result = await agent({prompt}, {{ agentType: 'validator', schema: {schema} }})\n\n" | ||
| 617 | + "return result\n" | ||
| 618 | + ).format( | ||
| 619 | + name=json.dumps(script_name), framing=framing_name, prompt=json.dumps(prompt), | ||
| 620 | + schema=json.dumps(_REFLECTION_OUTPUT_SCHEMA), | ||
| 621 | + ) | ||
| 622 | + work_graph_execute.write_workflow_script(cwd, script_name, script_content) | ||
| 623 | + result = work_graph_execute.execute_work_graph(cwd, script_name, {}, _REFLECTION_OUTPUT_SCHEMA, timeout=timeout) | ||
| 624 | + if worktree_path: | ||
| 625 | + work_node_run.ensure_worktree_committed(worktree_path, node_exec_id) | ||
| 626 | + work_node_run.ensure_worktree_committed(target_folder_path, node_exec_id + "-main-checkout-drift") | ||
| 627 | + work_node_run.merge_worktree(target_folder_path, node_exec_id) | ||
| 628 | + return { | ||
| 629 | + "target_node_id": result["target_node_id"], | ||
| 630 | + "defect_kind": result["defect_kind"], | ||
| 631 | + "detail": result.get("detail", ""), | ||
| 632 | + } | ||
| 633 | + finally: | ||
| 634 | + if worktree_path: | ||
| 635 | + work_node_run.remove_worktree(target_folder_path, worktree_path) | ||
| 636 | + | ||
| 637 | + | ||
| 638 | +def _tally_reflection(judgments, candidate_node_ids): | ||
| 639 | + """>=2/3 agreement on target_node_id, same threshold root_cause_trace.py's | ||
| 640 | + _tally_perspectives uses -- not imported from there (would create a | ||
| 641 | + circular import, root_cause_trace.py already imports this module) and | ||
| 642 | + small enough to duplicate, same reasoning work_graph_plan.py's own | ||
| 643 | + _transitive_ancestors duplication already documents. defect_kind is | ||
| 644 | + taken from whichever judgment(s) picked the winning target_node_id | ||
| 645 | + (majority among just those, defaulting to 'content_error' on a tie -- | ||
| 646 | + the conservative choice per §5.7 point 2). <2/3 agreement on | ||
| 647 | + target_node_id degrades to the nearest direct ancestor + 'content_error' | ||
| 648 | + rather than escalating to a human -- this is one retry inside a single | ||
| 649 | + Work Graph run, not the terminal cross-Org-Agent judgment | ||
| 650 | + root_cause_trace.py's InconclusiveEscalateError guards.""" | ||
| 651 | + target_counts = Counter(j["target_node_id"] for j in judgments) | ||
| 652 | + winner, count = target_counts.most_common(1)[0] | ||
| 653 | + if count * 2 < len(judgments) or winner not in candidate_node_ids: | ||
| 654 | + return {"target_node_id": candidate_node_ids[0], "defect_kind": "content_error", "detail": "no >=2/3 agreement"} | ||
| 655 | + kind_counts = Counter(j["defect_kind"] for j in judgments if j["target_node_id"] == winner) | ||
| 656 | + defect_kind = "skill_defect" if kind_counts.get("skill_defect", 0) > kind_counts.get("content_error", 0) else "content_error" | ||
| 657 | + detail = " | ".join(j["detail"] for j in judgments if j["target_node_id"] == winner) | ||
| 658 | + return {"target_node_id": winner, "defect_kind": defect_kind, "detail": detail} | ||
| 659 | + | ||
| 660 | + | ||
| 661 | +def _judge_root_cause_node( | ||
| 662 | + conn, run_id, failed_node_id, feedback, candidate_node_ids, graph_manifest, build_prompt, outputs, | ||
| 663 | + target_folder_path, timeout=180, | ||
| 664 | +): | ||
| 665 | + """Reflection-review (org-graph-unification-and-universal-self-planning- | ||
| 666 | + requirements.md §3.1 step 2): three independent real perspectives -- | ||
| 667 | + content, connectivity, history -- each picks a target_node_id from | ||
| 668 | + candidate_node_ids and a defect_kind, run concurrently (own real claude | ||
| 669 | + call, own worktree if self-target). Majority vote via _tally_reflection. | ||
| 670 | + candidate_node_ids must be non-empty (the caller, _run_one_node's | ||
| 671 | + dynamic_loop_back branch, only calls this when the failing node has at | ||
| 672 | + least one real ancestor -- a terminal judgment node with zero ancestors | ||
| 673 | + has nothing to loop back to and shouldn't declare dynamic_loop_back).""" | ||
| 674 | + with concurrent.futures.ThreadPoolExecutor(max_workers=3) as pool: | ||
| 675 | + futures = [ | ||
| 676 | + pool.submit( | ||
| 677 | + _run_reflection_perspective, name, | ||
| 678 | + _reflection_prompt(name, failed_node_id, feedback, candidate_node_ids, graph_manifest, build_prompt, outputs, conn, run_id), | ||
| 679 | + target_folder_path, timeout, | ||
| 680 | + ) | ||
| 681 | + for name in _REFLECTION_FRAMINGS | ||
| 682 | + ] | ||
| 683 | + judgments = [f.result() for f in futures] | ||
| 684 | + return _tally_reflection(judgments, candidate_node_ids) | ||
| 685 | + | ||
| 686 | + | ||
| 687 | +_SKILL_REWRITE_SCHEMA = { | ||
| 688 | + "type": "object", "required": ["new_content", "change_summary"], | ||
| 689 | + "properties": {"new_content": {"type": "string"}, "change_summary": {"type": "string"}}, | ||
| 690 | +} | ||
| 691 | + | ||
| 692 | +_SKILL_REWRITE_PROMPT = ( | ||
| 693 | + "You are revising a Skill file (the reusable methodology/prompt guidance a graphlemon Org Agent " | ||
| 694 | + "follows) because it produced a real, repeated failure. Your job is surgical: fix the SPECIFIC " | ||
| 695 | + "gap that caused this failure, without breaking the Skill's existing correct guidance for other " | ||
| 696 | + "cases it already handles well.\n\n" | ||
| 697 | + "Goal this task is working on: {goal_text}\n\n" | ||
| 698 | + "Skill being revised (skill_id={skill_id}):\n{current_content}\n\n" | ||
| 699 | + "What the node using this Skill was asked to do:\n{what_was_asked}\n\n" | ||
| 700 | + "What it actually produced (the defective output):\n{what_it_produced}\n\n" | ||
| 701 | + "Why this was judged a failure: {failure_feedback}\n\n" | ||
| 702 | + "This node's prior rejection history in this same run (if any): {loop_history}\n\n" | ||
| 703 | + "Reply with new_content (the COMPLETE revised Skill file content -- not a diff, not a partial " | ||
| 704 | + "excerpt) and change_summary (one sentence describing what changed and why)." | ||
| 705 | +) | ||
| 706 | + | ||
| 707 | + | ||
| 708 | +def _read_skill_content(skill_id): | ||
| 709 | + """Skill library content lives in graphlemon's own repo checkout | ||
| 710 | + (skill_query._REPO_ROOT), never in target_folder_path -- same | ||
| 711 | + established boundary c_skill_diff/c_skill_rollback's HTTP routes | ||
| 712 | + already rely on (05 §5.4.1.4), reused here rather than re-derived.""" | ||
| 713 | + path = os.path.join(skill_query._REPO_ROOT, "skills", skill_id, "SKILL.md") | ||
| 714 | + with open(path, encoding="utf-8") as f: | ||
| 715 | + return f.read() | ||
| 716 | + | ||
| 717 | + | ||
| 718 | +def _log_skill_iterated_event(conn, run_id, node_id, skill_id, iteration_id): | ||
| 719 | + """Persists a 'skill_iterated' execution_event -- this CHECK-enum value | ||
| 720 | + has existed since the original U-001 schema but was never actually | ||
| 721 | + written by any code path before Fix-Skill (skill_update.update_skill() | ||
| 722 | + itself doesn't log one; it's this caller's job, same division of | ||
| 723 | + responsibility loop_back_triggered already has between run_graph() and | ||
| 724 | + _record_loop_back_event). No schema migration needed.""" | ||
| 725 | + row = conn.execute( | ||
| 726 | + "SELECT wgr.task_id, t.project_id FROM work_graph_run wgr " | ||
| 727 | + "JOIN task t ON wgr.task_id = t.task_id WHERE wgr.run_id = ?", (run_id,) | ||
| 728 | + ).fetchone() | ||
| 729 | + task_id, project_id = row | ||
| 730 | + conn.execute( | ||
| 731 | + "INSERT INTO execution_event (event_id, project_id, task_id, run_id, event_type, payload) " | ||
| 732 | + "VALUES (?, ?, ?, ?, 'skill_iterated', ?)", | ||
| 733 | + (str(uuid.uuid4()), project_id, task_id, run_id, json.dumps({ | ||
| 734 | + "node_id": node_id, "skill_id": skill_id, "iteration_id": iteration_id, | ||
| 735 | + })), | ||
| 736 | + ) | ||
| 737 | + conn.commit() | ||
| 738 | + | ||
| 739 | + | ||
| 740 | +def _skill_iterated_events(conn, run_id): | ||
| 741 | + rows = conn.execute( | ||
| 742 | + "SELECT payload FROM execution_event WHERE run_id = ? AND event_type = 'skill_iterated' " | ||
| 743 | + "ORDER BY created_at", (run_id,) | ||
| 744 | + ).fetchall() | ||
| 745 | + return [json.loads(p) for (p,) in rows] | ||
| 746 | + | ||
| 747 | + | ||
| 748 | +_MAX_SKILL_FIXES_PER_RUN_PER_SKILL = 2 # §5.7 point 5: cap real Git rewrites of the same Skill within one run | ||
| 749 | + | ||
| 750 | + | ||
| 751 | +def _fix_skill_for_node(conn, run_id, target_node_id, graph_manifest, build_prompt, outputs, feedback, target_folder_path, timeout=300): | ||
| 752 | + """Fix-Skill (org-graph-unification-and-universal-self-planning- | ||
| 753 | + requirements.md §3.1 step 3): a real, Git-versioned Skill rewrite, not a | ||
| 754 | + per-run prompt patch -- calls the existing but (until now) never | ||
| 755 | + actually invoked by production code skill_update.py::update_skill() | ||
| 756 | + (see design doc §5's 'existing fact B': org_level_loop.py's | ||
| 757 | + _ROUTE_FOR_CATEGORY has no 'skill_defect' key, so this capability has | ||
| 758 | + sat unused since it was built). Always runs under the Validator | ||
| 759 | + identity (user-confirmed 2026-08-12 via AskUserQuestion): letting a | ||
| 760 | + node's own org_node rewrite its own Skill risks reproducing the exact | ||
| 761 | + blind spot that caused the failure. | ||
| 762 | + | ||
| 763 | + §5.7 point 5 rate limit: if this (run_id, skill_id) pair has already | ||
| 764 | + been rewritten _MAX_SKILL_FIXES_PER_RUN_PER_SKILL times this run, skip | ||
| 765 | + the rewrite silently (the caller's dynamic_loop_back still proceeds as | ||
| 766 | + a plain re-run of target_node_id -- not touching the Skill again is a | ||
| 767 | + safe degradation, not a failure).""" | ||
| 768 | + target_node = next(n for n in graph_manifest["nodes"] if n["node_id"] == target_node_id) | ||
| 769 | + skill_id = target_node.get("skill_ref") | ||
| 770 | + if not skill_id: | ||
| 771 | + return None # nothing declared for this node to attribute the fix to -- see caller's docstring | ||
| 772 | + | ||
| 773 | + prior_fixes = sum(1 for e in _skill_iterated_events(conn, run_id) if e["skill_id"] == skill_id) | ||
| 774 | + if prior_fixes >= _MAX_SKILL_FIXES_PER_RUN_PER_SKILL: | ||
| 775 | + return None | ||
| 776 | + | ||
| 777 | + row = conn.execute("SELECT task_id FROM work_graph_run WHERE run_id = ?", (run_id,)).fetchone() | ||
| 778 | + task_id = row[0] | ||
| 779 | + goal_text = conn.execute("SELECT goal_text FROM task WHERE task_id = ?", (task_id,)).fetchone()[0] | ||
| 780 | + current_content = _read_skill_content(skill_id) | ||
| 781 | + | ||
| 782 | + prompt = _SKILL_REWRITE_PROMPT.format( | ||
| 783 | + goal_text=goal_text, skill_id=skill_id, current_content=current_content, | ||
| 784 | + what_was_asked=build_prompt(target_node, outputs), | ||
| 785 | + what_it_produced=(outputs.get(target_node_id) or {}).get("content", ""), | ||
| 786 | + failure_feedback="; ".join(feedback or []), | ||
| 787 | + loop_history=_loop_history_prompt_suffix(conn, run_id, target_node_id) or "(none)", | ||
| 788 | + ) | ||
| 789 | + | ||
| 790 | + is_self = paths.is_self_target(target_folder_path) | ||
| 791 | + node_exec_id = "fixskill-" + uuid.uuid4().hex[:12] | ||
| 792 | + worktree_path = work_node_run.create_worktree(target_folder_path, node_exec_id) if is_self else None | ||
| 793 | + cwd = worktree_path or target_folder_path | ||
| 794 | + try: | ||
| 795 | + script_name = "graphlemon-fixskill-" + uuid.uuid4().hex | ||
| 796 | + script_content = ( | ||
| 797 | + "export const meta = {{ name: {name}, description: 'graphlemon Fix-Skill' }}\n\n" | ||
| 798 | + "const result = await agent({prompt}, {{ agentType: 'validator', schema: {schema} }})\n\n" | ||
| 799 | + "return result\n" | ||
| 800 | + ).format(name=json.dumps(script_name), prompt=json.dumps(prompt), schema=json.dumps(_SKILL_REWRITE_SCHEMA)) | ||
| 801 | + work_graph_execute.write_workflow_script(cwd, script_name, script_content) | ||
| 802 | + rewrite_result = work_graph_execute.execute_work_graph(cwd, script_name, {}, _SKILL_REWRITE_SCHEMA, timeout=timeout) | ||
| 803 | + if worktree_path: | ||
| 804 | + work_node_run.ensure_worktree_committed(worktree_path, node_exec_id) | ||
| 805 | + work_node_run.ensure_worktree_committed(target_folder_path, node_exec_id + "-main-checkout-drift") | ||
| 806 | + work_node_run.merge_worktree(target_folder_path, node_exec_id) | ||
| 807 | + finally: | ||
| 808 | + if worktree_path: | ||
| 809 | + work_node_run.remove_worktree(target_folder_path, worktree_path) | ||
| 810 | + | ||
| 811 | + # skill_query._REPO_ROOT (graphlemon's own checkout), NOT target_folder_path | ||
| 812 | + # -- the Skill library is graphlemon's own platform asset (05 §5.4.1.4 | ||
| 813 | + # precedent), independent of which target project this task is working on. | ||
| 814 | + bug_result = bug_record.create_bug_record( | ||
| 815 | + conn, task_id=task_id, | ||
| 816 | + description="Skill {} produced a repeated defective output at node {}".format(skill_id, target_node_id), | ||
| 817 | + root_cause_category="skill_defect", root_cause_detail=rewrite_result["change_summary"], | ||
| 818 | + suggested_route="full_replan", | ||
| 819 | + ) | ||
| 820 | + iteration = skill_update.update_skill( | ||
| 821 | + conn, skill_id, bug_result["bug_id"], rewrite_result["new_content"], | ||
| 822 | + rewrite_result["change_summary"], repo_path=skill_query._REPO_ROOT, | ||
| 823 | + ) | ||
| 824 | + _log_skill_iterated_event(conn, run_id, target_node_id, skill_id, iteration["iteration_id"]) | ||
| 825 | + return iteration | ||
| 826 | + | ||
| 827 | + | ||
| 509 | _DEFAULT_LOOP_LIMIT = 3 # only reached if even the "default" config_id row is unseeded | 828 | _DEFAULT_LOOP_LIMIT = 3 # only reached if even the "default" config_id row is unseeded |
| 510 | 829 | ||
| 511 | 830 | ||
| @@ -528,6 +847,23 @@ def _resolve_loop_limit(conn, org_node, graph_manifest, source_node_id): | @@ -528,6 +847,23 @@ def _resolve_loop_limit(conn, org_node, graph_manifest, source_node_id): | ||
| 528 | return _DEFAULT_LOOP_LIMIT | 847 | return _DEFAULT_LOOP_LIMIT |
| 529 | 848 | ||
| 530 | 849 | ||
| 850 | +def _resolve_total_loop_limit(conn, org_node, source_node_id): | ||
| 851 | + """Phase 2 §5.7 point 3: total dynamic_loop_back attempts for | ||
| 852 | + source_node_id, regardless of which target each attempt picked -- | ||
| 853 | + guards against dynamic_loop_back dodging _resolve_loop_limit's | ||
| 854 | + per-(target,source) cap by picking a different target each round. | ||
| 855 | + Lookup: a settings_config.py row keyed "{org_node}:{source_node_id}:total" | ||
| 856 | + (config_id has no format constraint, this key shape needs no schema | ||
| 857 | + change), then "default" (shared with _resolve_loop_limit's own default | ||
| 858 | + row -- deliberately the SAME fallback chain/value, so a source_node_id | ||
| 859 | + that never uses dynamic_loop_back sees an unrestrictive default here).""" | ||
| 860 | + for config_id in ("{}:{}:total".format(org_node, source_node_id), "default"): | ||
| 861 | + config = settings_config.get_loop_limit_config(conn, config_id) | ||
| 862 | + if config: | ||
| 863 | + return config["max_loop_attempts"] | ||
| 864 | + return _DEFAULT_LOOP_LIMIT | ||
| 865 | + | ||
| 866 | + | ||
| 531 | def _record_loop_back_event(conn, run_id, sig, attempt_number): | 867 | def _record_loop_back_event(conn, run_id, sig, attempt_number): |
| 532 | """Persists one 'loop_back_triggered' execution_event row -- this IS | 868 | """Persists one 'loop_back_triggered' execution_event row -- this IS |
| 533 | the loop history (2026-08-08 design §7): no separate table, so a | 869 | the loop history (2026-08-08 design §7): no separate table, so a |
| @@ -567,6 +903,13 @@ def _loop_attempt_count_from_events(conn, run_id, target_node_id, source_node_id | @@ -567,6 +903,13 @@ def _loop_attempt_count_from_events(conn, run_id, target_node_id, source_node_id | ||
| 567 | ) | 903 | ) |
| 568 | 904 | ||
| 569 | 905 | ||
| 906 | +def _loop_attempt_count_from_events_total(conn, run_id, source_node_id): | ||
| 907 | + """Real count of every prior loop_back triggered by source_node_id, | ||
| 908 | + regardless of target -- see _resolve_total_loop_limit's own docstring | ||
| 909 | + for why this differs from _loop_attempt_count_from_events.""" | ||
| 910 | + return sum(1 for e in _loop_back_events(conn, run_id) if e["source_node_id"] == source_node_id) | ||
| 911 | + | ||
| 912 | + | ||
| 570 | def _loop_history_prompt_suffix(conn, run_id, node_id): | 913 | def _loop_history_prompt_suffix(conn, run_id, node_id): |
| 571 | """When node_id is about to be (re-)executed, append the full history | 914 | """When node_id is about to be (re-)executed, append the full history |
| 572 | of every prior time it was sent back for revision (not just the most | 915 | of every prior time it was sent back for revision (not just the most |
| @@ -814,12 +1157,24 @@ def _run_layers( | @@ -814,12 +1157,24 @@ def _run_layers( | ||
| 814 | conn, run_id, sig.target_node_id, sig.source_node_id | 1157 | conn, run_id, sig.target_node_id, sig.source_node_id |
| 815 | ) + 1 | 1158 | ) + 1 |
| 816 | limit = _resolve_loop_limit(conn, org_node, graph_manifest, sig.source_node_id) | 1159 | limit = _resolve_loop_limit(conn, org_node, graph_manifest, sig.source_node_id) |
| 817 | - if attempt_number > limit: | 1160 | + # Phase 2 §5.7 point 3: attempt_number/limit above count only |
| 1161 | + # THIS (target, source) pair -- dynamic_loop_back can pick a | ||
| 1162 | + # DIFFERENT target each round, which would dodge that per-pair | ||
| 1163 | + # limit indefinitely. total_attempts/total_limit count every | ||
| 1164 | + # loop_back triggered by this source_node_id regardless of | ||
| 1165 | + # target. For retry_self/static loop_back (always the same | ||
| 1166 | + # target every time), total_attempts == attempt_number by | ||
| 1167 | + # construction, so this is a no-op there -- only dynamic_loop_back | ||
| 1168 | + # can actually make them diverge. | ||
| 1169 | + total_attempts = _loop_attempt_count_from_events_total(conn, run_id, sig.source_node_id) + 1 | ||
| 1170 | + total_limit = _resolve_total_loop_limit(conn, org_node, sig.source_node_id) | ||
| 1171 | + if attempt_number > limit or total_attempts > total_limit: | ||
| 818 | conn.execute("UPDATE work_graph_run SET status = 'blocked' WHERE run_id = ?", (run_id,)) | 1172 | conn.execute("UPDATE work_graph_run SET status = 'blocked' WHERE run_id = ?", (run_id,)) |
| 819 | conn.commit() | 1173 | conn.commit() |
| 820 | raise ConsistencyCheckFailedError( | 1174 | raise ConsistencyCheckFailedError( |
| 821 | - "loop_back from {} to {} exceeded max_attempts={}".format( | ||
| 822 | - sig.source_node_id, sig.target_node_id, limit | 1175 | + "loop_back from {} to {} exceeded max_attempts={} (per-target={}/{}, total={}/{})".format( |
| 1176 | + sig.source_node_id, sig.target_node_id, max(limit, total_limit), | ||
| 1177 | + attempt_number, limit, total_attempts, total_limit, | ||
| 823 | ), | 1178 | ), |
| 824 | node_id=sig.source_node_id, | 1179 | node_id=sig.source_node_id, |
| 825 | ) | 1180 | ) |
| @@ -849,17 +1204,34 @@ def _run_one_node( | @@ -849,17 +1204,34 @@ def _run_one_node( | ||
| 849 | node_id -- see that function's own docstring for why (a loop_back | 1204 | node_id -- see that function's own docstring for why (a loop_back |
| 850 | target must never collide with, and short-circuit against, its own | 1205 | target must never collide with, and short-circuit against, its own |
| 851 | stale pre-loop_back 'done' row; every other node keeps resuming | 1206 | stale pre-loop_back 'done' row; every other node keeps resuming |
| 852 | - exactly like before).""" | 1207 | + exactly like before). |
| 1208 | + | ||
| 1209 | + 2026-08-12 (Phase 2 §4, node-level subagent/skill binding): a node can | ||
| 1210 | + declare its own `subagent_type`/`skill_ref`, overriding the run-level | ||
| 1211 | + `org_node`/`skill_id` defaults this whole function used to use | ||
| 1212 | + unconditionally for every node. A node with neither field (every | ||
| 1213 | + existing graph_manifest -- Planner's fixed chain, Coder's synthetic | ||
| 1214 | + single-node manifest) behaves identically to before: `node_org_node`/ | ||
| 1215 | + `node_skill_id` just equal the run-level defaults.""" | ||
| 853 | node_id = node["node_id"] | 1216 | node_id = node["node_id"] |
| 1217 | + node_org_node = node.get("subagent_type", org_node) | ||
| 1218 | + node_skill_id = node.get("skill_ref", skill_id) | ||
| 1219 | + # run_graph() only ensures the RUN-LEVEL default skill_id exists as a | ||
| 1220 | + # placeholder row (work_node_execution.skill_ref's FK target) -- a | ||
| 1221 | + # node declaring its own skill_ref needs the same guarantee, or the | ||
| 1222 | + # INSERT below fails FOREIGN KEY constraint the first time that | ||
| 1223 | + # specific skill_id is ever seen. Idempotent, cheap to call per node. | ||
| 1224 | + if node_skill_id != skill_id: | ||
| 1225 | + ensure_placeholder_skill(conn, node_skill_id, node_org_node) | ||
| 854 | exec_node_id_str = _next_exec_node_id(conn, run_id, node_id, graph_manifest) | 1226 | exec_node_id_str = _next_exec_node_id(conn, run_id, node_id, graph_manifest) |
| 855 | 1227 | ||
| 856 | node_exec_id = _ensure_node_row( | 1228 | node_exec_id = _ensure_node_row( |
| 857 | - conn, run_id, exec_node_id_str, org_node, skill_id, | 1229 | + conn, run_id, exec_node_id_str, node_org_node, node_skill_id, |
| 858 | "work_graph_node_input.schema.json", "work_graph_node_output.schema.json", | 1230 | "work_graph_node_input.schema.json", "work_graph_node_output.schema.json", |
| 859 | {"node_id": node_id}, | 1231 | {"node_id": node_id}, |
| 860 | ) | 1232 | ) |
| 861 | prompt = build_prompt(node, outputs) + _loop_history_prompt_suffix(conn, run_id, node_id) | 1233 | prompt = build_prompt(node, outputs) + _loop_history_prompt_suffix(conn, run_id, node_id) |
| 862 | - run_fn = _make_run_fn(conn, target_folder_path, node_exec_id, org_node, prompt, output_schema, timeout) | 1234 | + run_fn = _make_run_fn(conn, target_folder_path, node_exec_id, node_org_node, prompt, output_schema, timeout) |
| 863 | output = work_node_run.run_work_node( | 1235 | output = work_node_run.run_work_node( |
| 864 | conn, node_exec_id, {"node_id": node_id}, input_schema, output_schema, run_fn, | 1236 | conn, node_exec_id, {"node_id": node_id}, input_schema, output_schema, run_fn, |
| 865 | needs_worktree=needs_worktree, repo_path=target_folder_path, sleep=sleep, now=now, | 1237 | needs_worktree=needs_worktree, repo_path=target_folder_path, sleep=sleep, now=now, |
| @@ -868,7 +1240,7 @@ def _run_one_node( | @@ -868,7 +1240,7 @@ def _run_one_node( | ||
| 868 | judgment = node.get("judgment") | 1240 | judgment = node.get("judgment") |
| 869 | has_own_judgment = bool(judgment) and judgment.get("kind", "none") != "none" | 1241 | has_own_judgment = bool(judgment) and judgment.get("kind", "none") != "none" |
| 870 | resolved_verify_fn = _resolve_node_verify_fn( | 1242 | resolved_verify_fn = _resolve_node_verify_fn( |
| 871 | - node, verify_fn, org_node, target_folder_path, mechanical_check_registry | 1243 | + node, verify_fn, node_org_node, target_folder_path, mechanical_check_registry |
| 872 | ) | 1244 | ) |
| 873 | if resolved_verify_fn is None: | 1245 | if resolved_verify_fn is None: |
| 874 | return output | 1246 | return output |
| @@ -881,7 +1253,7 @@ def _run_one_node( | @@ -881,7 +1253,7 @@ def _run_one_node( | ||
| 881 | # every node, once redundantly), regardless of any on_fail field | 1253 | # every node, once redundantly), regardless of any on_fail field |
| 882 | # (legacy nodes don't have one). | 1254 | # (legacy nodes don't have one). |
| 883 | return _verify_and_maybe_regenerate( | 1255 | return _verify_and_maybe_regenerate( |
| 884 | - conn, run_id, org_node, skill_id, target_folder_path, node, node_id, prompt, | 1256 | + conn, run_id, node_org_node, node_skill_id, target_folder_path, node, node_id, prompt, |
| 885 | output, input_schema, output_schema, timeout, needs_worktree, resolved_verify_fn, sleep, now, | 1257 | output, input_schema, output_schema, timeout, needs_worktree, resolved_verify_fn, sleep, now, |
| 886 | ) | 1258 | ) |
| 887 | 1259 | ||
| @@ -899,6 +1271,26 @@ def _run_one_node( | @@ -899,6 +1271,26 @@ def _run_one_node( | ||
| 899 | ), | 1271 | ), |
| 900 | node_id=node_id, | 1272 | node_id=node_id, |
| 901 | ) | 1273 | ) |
| 1274 | + if action == "dynamic_loop_back": | ||
| 1275 | + # Phase 2 §5: Reflection-review -> [Fix-Skill] -> Re-loop. See | ||
| 1276 | + # _judge_root_cause_node/_fix_skill_for_node docstrings for the | ||
| 1277 | + # full four-step methodology (org-graph-unification-and-universal- | ||
| 1278 | + # self-planning-requirements.md §3.1). | ||
| 1279 | + ancestors = sorted(_transitive_ancestors(graph_manifest, node_id) - {node_id}) | ||
| 1280 | + diagnosis = _judge_root_cause_node( | ||
| 1281 | + conn, run_id, node_id, feedback, ancestors, graph_manifest, build_prompt, outputs, | ||
| 1282 | + target_folder_path, | ||
| 1283 | + ) | ||
| 1284 | + target = diagnosis["target_node_id"] | ||
| 1285 | + if target not in ancestors: | ||
| 1286 | + raise ValueError( | ||
| 1287 | + "dynamic_loop_back judge picked {!r}, not a real ancestor of {}".format(target, node_id) | ||
| 1288 | + ) | ||
| 1289 | + if diagnosis["defect_kind"] == "skill_defect": | ||
| 1290 | + _fix_skill_for_node( | ||
| 1291 | + conn, run_id, target, graph_manifest, build_prompt, outputs, feedback, target_folder_path, | ||
| 1292 | + ) | ||
| 1293 | + raise _LoopBackRequested(source_node_id=node_id, target_node_id=target, feedback=feedback) | ||
| 902 | target = node_id if action == "retry_self" else _on_failure_target(graph_manifest, node_id) | 1294 | target = node_id if action == "retry_self" else _on_failure_target(graph_manifest, node_id) |
| 903 | raise _LoopBackRequested(source_node_id=node_id, target_node_id=target, feedback=feedback) | 1295 | raise _LoopBackRequested(source_node_id=node_id, target_node_id=target, feedback=feedback) |
| 904 | 1296 |
-
mentioned in commit 82368fc6d3a38e988a10509bc8526465276529a8