Commit a68f0adb081b41b0c1e8abcf9bf4d0339dd6e11e
1 parent
89482e7c
Org Graph unification Phase 2c: Validator self-plans its verification graph
Validator's terminal verdict is now a real self-planned node (node_type= 'final_verdict', judgment.kind='mechanical', on_fail.action= 'dynamic_loop_back') appended into the SAME shared work_graph_run Planner and Coder already contribute to (plan_work_graph(..., append_to_run_id= run_id)), instead of a flat post-graph org_node_trigger call against a separate hardcoded Validator manifest. This is what lets a failing verdict genuinely trigger Reflection-review + Fix-Skill + a precise loop back to whichever real ancestor node is at fault, reusing the existing dynamic_loop_back machinery instead of a parallel retry loop. The 2026-08-06 hard gate (a real non-zero exit from Validator's own test script force-overrides the verdict to FAIL, Python-enforced not just prompted) is preserved via a build_prompt wrapper + mechanical check closure sharing mutable state. INCONCLUSIVE is now treated the same as FAIL (triggers a bounded retry via dynamic_loop_back) rather than accepted as a terminal result. _VALIDATOR_MANIFEST/_VALIDATOR_DOC_PROMPTS/_VALIDATOR_VERDICT_PROMPT/ _validator_prompt_builder are kept unused (same precedent as _CODER_PROMPT). org_node column removal (§2.1), index narrowing (§2.2), and the new work-graph-node-review skill (§7.1) remain for a later sub-phase. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Showing
6 changed files
with
663 additions
and
359 deletions
docs/todo/org-graph-unification-phase2-universal-self-planning-design.md
| @@ -3,7 +3,7 @@ | @@ -3,7 +3,7 @@ | ||
| 3 | **日期**: 2026-08-08 | 3 | **日期**: 2026-08-08 |
| 4 | **性质**: 详细设计,对应 `docs/todo/org-graph-unification-and-universal-self-planning-requirements.md` 的 Phase 2(FR-2/FR-3/FR-4),按 §4/§4.1 已拍板的合图(方案 A)路径设计。 | 4 | **性质**: 详细设计,对应 `docs/todo/org-graph-unification-and-universal-self-planning-requirements.md` 的 Phase 2(FR-2/FR-3/FR-4),按 §4/§4.1 已拍板的合图(方案 A)路径设计。 |
| 5 | **依赖**: 硬依赖 Phase 1(`docs/done/org-graph-unification-phase1-review-gate-design.md`)——已实现(commit `ea19c7d`,2026-08-08)。§9 接口核对清单已于 2026-08-12 逐条核对完成:三项全部吻合(`plan_and_review_documents()` 经 `_review_planner_output_with_retries()` 透传 `plan_documents()` 的 `{"run_id", "artifact_ids"}` 返回形状,含 `run_id`;`task.status` 新值确认是 `reviewing_planner_output`;`roles/planner_reviewer/role.json` 目前 `skills: ["planner-output-review", "unattended-agent-operations"]`,未预留 §7.1 新 skill 位置,Phase 2 实施时新增一行即可,不是不兼容)。 | 5 | **依赖**: 硬依赖 Phase 1(`docs/done/org-graph-unification-phase1-review-gate-design.md`)——已实现(commit `ea19c7d`,2026-08-08)。§9 接口核对清单已于 2026-08-12 逐条核对完成:三项全部吻合(`plan_and_review_documents()` 经 `_review_planner_output_with_retries()` 透传 `plan_documents()` 的 `{"run_id", "artifact_ids"}` 返回形状,含 `run_id`;`task.status` 新值确认是 `reviewing_planner_output`;`roles/planner_reviewer/role.json` 目前 `skills: ["planner-output-review", "unattended-agent-operations"]`,未预留 §7.1 新 skill 位置,Phase 2 实施时新增一行即可,不是不兼容)。 |
| 6 | -**状态**: Phase 2a(commit `86fc0bb`)+ Phase 2b(commit `7c46a2f`)已实现,2026-08-12。Phase 2c(Validator 接入自规划 + `org_node` 列删除 + 精确 loop_back 收口)待实施,详见 §10。**2026-08-08 修订**:写作过程中,需求文档 §7 的四个开放问题已被用户直接拍板(详见该文档 §7 对应条目),其中两条推翻了本文档最初的设计判断——Coder/Validator 自规划改为"建议节点清单"而非完全自由(§3),`work_graph_run.org_node` 改为彻底移除而非保留(§2.1)——本版本已按拍板结果改写,不是原始草稿。**2026-08-08 追加**(同日,见需求文档新增 §3.1):FR-4 的目标被明确为"失败即进化"完整方法论(判断失败 → Reflection-review 动态诊断 → 系统性缺陷则先 Fix-Skill → 跳回执行),当时 §5 设计的 `dynamic_loop_back` 只覆盖第 1/2/4 步,第 3 步(Fix-Skill)是已知缺口。**2026-08-12 补齐**:§5 已扩写为完整四步设计(§5.4 Reflection-review 现在同时判断目标节点和缺陷性质,§5.5 新增 Fix-Skill,真实调用既有但从未真正投入使用的 `skill_update.py::update_skill()`),过程中发现两个既有事实(`skills:` frontmatter 只按名字动态加载内容、不需要重新部署;`org_level_loop.py` 的 `_ROUTE_FOR_CATEGORY` 从来没有 `skill_defect` 这个 key,`update_skill()` 在这个代码库里投产以来从未被真正调用过)——详见 §5 开头。由哪个身份执笔改 Skill 已通过 `AskUserQuestion` 确认(2026-08-12):统一用 Validator,见 §5.5。 | 6 | +**状态**: Phase 2a(commit `86fc0bb`)+ Phase 2b(commit `7c46a2f`)已实现,2026-08-12。Phase 2c 的 Validator 自规划改造(§6 核心部分)已实现,2026-08-12:`validate_implementation_with_own_graph()` 改为 `plan_work_graph(conn, "validator", ..., append_to_run_id=run_id)` 追加进 Planner/Coder 共享的同一个 `work_graph_run`,终审 verdict 变成图内一个真实节点(`node_type='final_verdict'`,`judgment.kind='mechanical'`、`check_ref='verdict_says_pass'`,`on_fail.action='dynamic_loop_back'`——Validator 图里唯一允许用这个 action 的节点),硬门槛(脚本真实非零退出码强制 FAIL,不听 LLM 辩解)通过 `_verdict_node_build_prompt`/`_verdict_says_pass_check` 的共享可变状态闭包保留,Python 强制而非只是 prompt 里说说。`_VALIDATOR_MANIFEST`/`_VALIDATOR_DOC_PROMPTS`/`_VALIDATOR_VERDICT_PROMPT`/`_validator_prompt_builder` 保留但不再使用(同 `_CODER_PROMPT` 先例)。`python3 -m unittest discover -s src`(756 tests)+ `discover -s supervisor`(126 tests)全绿。**尚未实施**:§2.1(`org_node` 列删除)、§2.2(`ux_work_graph_run_active` 收紧到 `UNIQUE(task_id)`)、§7.1(新 skill `work-graph-node-review` + 三个 role.json 追加)、§7.2 的收口验证(见 §10 Phase 2c 小节末尾新增的分析:一旦 verdict 节点自己的 `dynamic_loop_back` 内部消化重试,`validate_implementation_with_own_graph()` 正常返回时 verdict 必为 PASS,`org_level_loop.py` 的旧粗粒度路由对这条新路径自然不可达,不需要显式新旧流程判断——决定保留旧分支代码不删,加注释说明,不做额外改动)。**2026-08-08 修订**:写作过程中,需求文档 §7 的四个开放问题已被用户直接拍板(详见该文档 §7 对应条目),其中两条推翻了本文档最初的设计判断——Coder/Validator 自规划改为"建议节点清单"而非完全自由(§3),`work_graph_run.org_node` 改为彻底移除而非保留(§2.1)——本版本已按拍板结果改写,不是原始草稿。**2026-08-08 追加**(同日,见需求文档新增 §3.1):FR-4 的目标被明确为"失败即进化"完整方法论(判断失败 → Reflection-review 动态诊断 → 系统性缺陷则先 Fix-Skill → 跳回执行),当时 §5 设计的 `dynamic_loop_back` 只覆盖第 1/2/4 步,第 3 步(Fix-Skill)是已知缺口。**2026-08-12 补齐**:§5 已扩写为完整四步设计(§5.4 Reflection-review 现在同时判断目标节点和缺陷性质,§5.5 新增 Fix-Skill,真实调用既有但从未真正投入使用的 `skill_update.py::update_skill()`),过程中发现两个既有事实(`skills:` frontmatter 只按名字动态加载内容、不需要重新部署;`org_level_loop.py` 的 `_ROUTE_FOR_CATEGORY` 从来没有 `skill_defect` 这个 key,`update_skill()` 在这个代码库里投产以来从未被真正调用过)——详见 §5 开头。由哪个身份执笔改 Skill 已通过 `AskUserQuestion` 确认(2026-08-12):统一用 Validator,见 §5.5。 |
| 7 | 7 | ||
| 8 | --- | 8 | --- |
| 9 | 9 |
src/e2e_pipeline.py
| @@ -200,6 +200,15 @@ _VALIDATOR_PROMPT = ( | @@ -200,6 +200,15 @@ _VALIDATOR_PROMPT = ( | ||
| 200 | "Goal: {goal_text}\n\nBenchmark:\n\n{benchmark_content}" | 200 | "Goal: {goal_text}\n\nBenchmark:\n\n{benchmark_content}" |
| 201 | ) | 201 | ) |
| 202 | 202 | ||
| 203 | +# 2026-08-12 (Phase 2c): retired now that Validator self-plans its own | ||
| 204 | +# verification graph (validate_implementation_with_own_graph() below) via | ||
| 205 | +# plan_work_graph(conn, "validator", ..., append_to_run_id=run_id) instead | ||
| 206 | +# of running this hardcoded fixed chain -- the "held-out eval set, not the | ||
| 207 | +# Coder's own dev set" framing that motivated this dict lives on in | ||
| 208 | +# _VALIDATOR_PLANNING_CONTEXT's guidance text instead. Kept here, unused, | ||
| 209 | +# for the same "a future revert would want it back verbatim" reason | ||
| 210 | +# _CODER_PROMPT above is. | ||
| 211 | +# | ||
| 203 | # 2026-08-06: Validator gets its OWN benchmark/test_dataset/test_script, | 212 | # 2026-08-06: Validator gets its OWN benchmark/test_dataset/test_script, |
| 204 | # generated independently from SDD/PRD -- NOT reusing the Planner's/Coder's | 213 | # generated independently from SDD/PRD -- NOT reusing the Planner's/Coder's |
| 205 | # own copies. If the Coder could game the Planner's test_script (e.g. | 214 | # own copies. If the Coder could game the Planner's test_script (e.g. |
| @@ -240,10 +249,12 @@ _VALIDATOR_DOC_PROMPTS = { | @@ -240,10 +249,12 @@ _VALIDATOR_DOC_PROMPTS = { | ||
| 240 | ), | 249 | ), |
| 241 | } | 250 | } |
| 242 | 251 | ||
| 243 | -# validator_test_script node temporarily disabled (2026-08-07, same | ||
| 244 | -# stopgap as work_graph_plan._PROMPT_TEMPLATE -- see its comment for the | ||
| 245 | -# full rationale/upgrade path): 6/6 real repro rate for narrative-instead- | ||
| 246 | -# of-code output, docs/todo/validator-test-script-narrative-output-issue.md. | 252 | +# 2026-08-12 (Phase 2c): retired alongside _VALIDATOR_DOC_PROMPTS above -- |
| 253 | +# see that comment. validator_test_script node temporarily disabled | ||
| 254 | +# (2026-08-07, same stopgap as work_graph_plan._PROMPT_TEMPLATE -- see its | ||
| 255 | +# comment for the full rationale/upgrade path): 6/6 real repro rate for | ||
| 256 | +# narrative-instead-of-code output, docs/todo/validator-test-script- | ||
| 257 | +# narrative-output-issue.md. | ||
| 247 | _VALIDATOR_MANIFEST = { | 258 | _VALIDATOR_MANIFEST = { |
| 248 | "nodes": [ | 259 | "nodes": [ |
| 249 | {"node_id": "validator_benchmark", "artifact_type": "validator_benchmark", "depends_on": []}, | 260 | {"node_id": "validator_benchmark", "artifact_type": "validator_benchmark", "depends_on": []}, |
| @@ -255,6 +266,8 @@ _VALIDATOR_MANIFEST = { | @@ -255,6 +266,8 @@ _VALIDATOR_MANIFEST = { | ||
| 255 | "edges": [], | 266 | "edges": [], |
| 256 | } | 267 | } |
| 257 | 268 | ||
| 269 | +# 2026-08-12 (Phase 2c): retired alongside _VALIDATOR_MANIFEST above -- see | ||
| 270 | +# that comment. | ||
| 258 | _VALIDATOR_VERDICT_PROMPT = ( | 271 | _VALIDATOR_VERDICT_PROMPT = ( |
| 259 | "Give your final verdict for this goal, informed by your own independent Benchmark below " | 272 | "Give your final verdict for this goal, informed by your own independent Benchmark below " |
| 260 | "and the REAL execution result of your own test script (if you wrote one -- 'N/A' means you " | 273 | "and the REAL execution result of your own test script (if you wrote one -- 'N/A' means you " |
| @@ -303,7 +316,12 @@ def _planner_prompt_builder(goal_text, node_artifact_types): | @@ -303,7 +316,12 @@ def _planner_prompt_builder(goal_text, node_artifact_types): | ||
| 303 | 316 | ||
| 304 | 317 | ||
| 305 | def _validator_prompt_builder(goal_text, sdd_content, prd_content, node_artifact_types): | 318 | def _validator_prompt_builder(goal_text, sdd_content, prd_content, node_artifact_types): |
| 306 | - """Same shape as _planner_prompt_builder, but the FIRST node | 319 | + """2026-08-12 (Phase 2c): retired alongside _VALIDATOR_MANIFEST above -- |
| 320 | + see that comment; validate_implementation_with_own_graph() now uses | ||
| 321 | + dispatch_build_prompt(_planner_prompt_builder(...), generic_node_prompt_builder) | ||
| 322 | + like every other self-planning org_node. | ||
| 323 | + | ||
| 324 | + Same shape as _planner_prompt_builder, but the FIRST node | ||
| 307 | (validator_benchmark) needs sdd_content/prd_content from OUTSIDE | 325 | (validator_benchmark) needs sdd_content/prd_content from OUTSIDE |
| 308 | validator's own tiny graph -- those aren't a dependency within this | 326 | validator's own tiny graph -- those aren't a dependency within this |
| 309 | graph, they come from the Planner's already-completed one. Passed | 327 | graph, they come from the Planner's already-completed one. Passed |
| @@ -644,12 +662,16 @@ def plan_documents(conn, task_id, goal_text, target_folder_path, timeout=300): | @@ -644,12 +662,16 @@ def plan_documents(conn, task_id, goal_text, target_folder_path, timeout=300): | ||
| 644 | now carries its own `judgment` field (work_graph_plan.py's | 662 | now carries its own `judgment` field (work_graph_plan.py's |
| 645 | `_PLANNER_NODE_CATALOG` suggests one per node type, the Planner can | 663 | `_PLANNER_NODE_CATALOG` suggests one per node type, the Planner can |
| 646 | reuse or override it), which `run_graph()` resolves itself per node. | 664 | reuse or override it), which `run_graph()` resolves itself per node. |
| 647 | - `_combined_planner_verify_fn`/`_prd_consistency_verify_fn`/ | ||
| 648 | - `_test_script_verify_fn` are NOT deleted -- `validate_implementation_ | ||
| 649 | - with_own_graph()` (Validator's still-hardcoded manifest, unaffected by | ||
| 650 | - this round) continues to use them (FR-A8). `mechanical_check_registry` | ||
| 651 | - is still passed unconditionally: a self-planned node declaring | ||
| 652 | - `judgment.kind == "mechanical"` (e.g. reusing the catalog's suggested | 665 | + `_combined_planner_verify_fn`/`_prd_consistency_verify_fn` are NOT |
| 666 | + deleted -- this function still uses them below (FR-A8). | ||
| 667 | + `_test_script_verify_fn` is ALSO still used below for Planner's own | ||
| 668 | + `test_script` node (2026-08-12: Validator's own equivalent node no | ||
| 669 | + longer needs it -- validate_implementation_with_own_graph() now | ||
| 670 | + self-plans too, and its `final_verdict` node's hard gate is enforced | ||
| 671 | + via a mechanical_check_registry closure instead, see that function's | ||
| 672 | + own docstring). `mechanical_check_registry` is still passed | ||
| 673 | + unconditionally: a self-planned node declaring `judgment.kind == | ||
| 674 | + "mechanical"` (e.g. reusing the catalog's suggested | ||
| 653 | `check_script_compiles` for a test_script node) needs it registered, | 675 | `check_script_compiles` for a test_script node) needs it registered, |
| 654 | same as before. | 676 | same as before. |
| 655 | """ | 677 | """ |
| @@ -1089,105 +1111,214 @@ def _format_script_execution_result(result): | @@ -1089,105 +1111,214 @@ def _format_script_execution_result(result): | ||
| 1089 | ).format(result["exit_code"], result["stdout"][-4000:], result["stderr"][-4000:]) | 1111 | ).format(result["exit_code"], result["stdout"][-4000:], result["stderr"][-4000:]) |
| 1090 | 1112 | ||
| 1091 | 1113 | ||
| 1114 | +def _verdict_says_pass_check(shared_state): | ||
| 1115 | + """Phase 2c (2026-08-12): mechanical judgment for the final_verdict | ||
| 1116 | + node -- `shared_state` is a plain dict the matching build_prompt | ||
| 1117 | + wrapper (_verdict_node_build_prompt) populates with the REAL script | ||
| 1118 | + execution result as a side effect, read back here since a mechanical | ||
| 1119 | + check callable only ever receives `content`, no side channel of its | ||
| 1120 | + own. Two ways to fail: | ||
| 1121 | + 1. Hard gate (user-confirmed 2026-08-06, preserved verbatim): a real | ||
| 1122 | + non-zero exit from Validator's own test script is not up for LLM | ||
| 1123 | + debate -- force-reject regardless of what the verdict text says. | ||
| 1124 | + 2. The verdict itself isn't PASS -- INCONCLUSIVE is treated the SAME | ||
| 1125 | + as FAIL here (ok=False), not specially accepted: this node's job | ||
| 1126 | + is to keep trying (bounded by the loop_back attempt budget, same | ||
| 1127 | + as everything else on_fail.action='dynamic_loop_back' governs) to | ||
| 1128 | + actually resolve the goal, not to settle for genuine uncertainty | ||
| 1129 | + on the first attempt. If the budget is exhausted without reaching | ||
| 1130 | + PASS, run_graph() raises ConsistencyCheckFailedError same as any | ||
| 1131 | + other exhausted loop -- this function then never returns a non-PASS | ||
| 1132 | + verdict normally, it either returns PASS or raises (see this | ||
| 1133 | + function's own docstring for why that's the right contract).""" | ||
| 1134 | + def check(content): | ||
| 1135 | + script_execution = shared_state.get("script_execution") | ||
| 1136 | + if script_execution and script_execution["ran"] and script_execution["exit_code"] not in (0, None): | ||
| 1137 | + return False, "real test script exited non-zero (exit_code={}) -- hard gate, not up for debate".format( | ||
| 1138 | + script_execution["exit_code"] | ||
| 1139 | + ) | ||
| 1140 | + verdict = _parse_verdict_line(content) | ||
| 1141 | + if verdict == "PASS": | ||
| 1142 | + return True, None | ||
| 1143 | + return False, "verdict was {}, not PASS".format(verdict) | ||
| 1144 | + return check | ||
| 1145 | + | ||
| 1146 | + | ||
| 1147 | +def _verdict_node_build_prompt(inner_build_prompt, node_types, task_id, target_folder_path, shared_state, timeout): | ||
| 1148 | + """Wraps a build_prompt(node, outputs) callable: for the ONE node | ||
| 1149 | + declaring node_type='final_verdict', performs REAL test script | ||
| 1150 | + execution (if a node_type='test_script' dependency produced real | ||
| 1151 | + content) as a side effect before returning the prompt -- embedding | ||
| 1152 | + the real result into both the prompt text (so the model sees it) and | ||
| 1153 | + `shared_state` (so _verdict_says_pass_check's hard gate can enforce it | ||
| 1154 | + in Python, not just ask nicely). Every other node passes through to | ||
| 1155 | + `inner_build_prompt` completely unchanged.""" | ||
| 1156 | + def build_prompt(node, outputs): | ||
| 1157 | + if node.get("node_type") != "final_verdict": | ||
| 1158 | + return inner_build_prompt(node, outputs) | ||
| 1159 | + script_node_id = next( | ||
| 1160 | + (nid for nid, node_type in node_types.items() if node_type == "test_script" and nid in outputs), None | ||
| 1161 | + ) | ||
| 1162 | + if script_node_id: | ||
| 1163 | + script_execution = run_test_script_for_real( | ||
| 1164 | + outputs[script_node_id].get("content", ""), target_folder_path, task_id, | ||
| 1165 | + "validator_final_verdict_script", timeout=timeout, | ||
| 1166 | + ) | ||
| 1167 | + else: | ||
| 1168 | + script_execution = { | ||
| 1169 | + "ran": False, "exit_code": None, "stdout": "", "stderr": "", | ||
| 1170 | + "note": "N/A: this self-planned graph has no test_script-typed node", | ||
| 1171 | + } | ||
| 1172 | + shared_state["script_execution"] = script_execution | ||
| 1173 | + return inner_build_prompt(node, outputs) + ( | ||
| 1174 | + "\n\nThe REAL execution result of your own test script (if any):\n" | ||
| 1175 | + + _format_script_execution_result(script_execution) | ||
| 1176 | + ) | ||
| 1177 | + return build_prompt | ||
| 1178 | + | ||
| 1179 | + | ||
| 1180 | +_VALIDATOR_PLANNING_CONTEXT = ( | ||
| 1181 | + "{goal_text}\n\n" | ||
| 1182 | + "Approved SDD:\n\n{sdd_content}\n\n" | ||
| 1183 | + "PRD:\n\n{prd_content}\n\n" | ||
| 1184 | + "Guidance: you are independently verifying this goal's implementation. Form your OWN " | ||
| 1185 | + "acceptance criteria informed by (but not copied from) the SDD/PRD above -- don't assume " | ||
| 1186 | + "what the Coder actually did, you have no visibility into that beyond what you inspect " | ||
| 1187 | + "yourself. A held-out test script (if you plan one) will be executed for real against the " | ||
| 1188 | + "actual current repository state -- write real commands, not a description of what a script " | ||
| 1189 | + "would do. Your final verdict must be its own node (see the final_verdict entry in the " | ||
| 1190 | + "suggested node types), depending on whatever earlier nodes it genuinely needs." | ||
| 1191 | +) | ||
| 1192 | + | ||
| 1193 | + | ||
| 1092 | def validate_implementation_with_own_graph( | 1194 | def validate_implementation_with_own_graph( |
| 1093 | - conn, task_id, sdd_content, prd_content, goal_text, target_folder_path, timeout=1200, | 1195 | + conn, task_id, run_id, sdd_content, prd_content, goal_text, target_folder_path, timeout=1200, |
| 1094 | ): | 1196 | ): |
| 1095 | - """2026-08-06: Validator's own Work Graph (validator_benchmark -> | ||
| 1096 | - validator_test_dataset -> validator_test_script), independently | ||
| 1097 | - generated from SDD/PRD -- NOT reusing the Planner's/Coder's own | ||
| 1098 | - benchmark/dataset/script (see _VALIDATOR_DOC_PROMPTS's module comment | ||
| 1099 | - for why: a held-out eval set, not a re-run of the dev set the Coder | ||
| 1100 | - already developed against). The resulting test_script, if real (not | ||
| 1101 | - N/A), is then actually EXECUTED against the current repository state -- | ||
| 1102 | - a non-zero real exit code force-overrides the verdict to FAIL in code, | ||
| 1103 | - not just by asking the LLM nicely (user-confirmed 2026-08-06: script is | ||
| 1104 | - the hard gate, subjective judgment can only add INCONCLUSIVE/risk notes | ||
| 1105 | - on top of a passing script, never override a failing one to PASS). | ||
| 1106 | - | ||
| 1107 | - Returns {"verdict", "raw_response", "artifact_ids", "script_execution"}. | 1197 | + """Phase 2c (2026-08-12): Validator self-plans its own verification |
| 1198 | + graph via plan_work_graph(conn, "validator", ..., append_to_run_id= | ||
| 1199 | + run_id) -- appended onto the SAME shared run Planner/Coder already | ||
| 1200 | + contributed to, replacing the old hardcoded _VALIDATOR_MANIFEST/ | ||
| 1201 | + _VALIDATOR_DOC_PROMPTS/_validator_prompt_builder fixed-chain approach | ||
| 1202 | + (kept in the module, unused, for the same "a future revert would want | ||
| 1203 | + it back verbatim" reason implement_code()'s retired _CODER_PROMPT is). | ||
| 1204 | + | ||
| 1205 | + The final verdict is now a REAL node inside this graph (node_type= | ||
| 1206 | + 'final_verdict', judgment.kind='mechanical', check_ref= | ||
| 1207 | + 'verdict_says_pass', on_fail.action='dynamic_loop_back') instead of a | ||
| 1208 | + flat call after the graph finishes -- this is what lets a FAIL | ||
| 1209 | + genuinely trigger Reflection-review + [Fix-Skill] + a precise loop | ||
| 1210 | + back to whichever real ancestor node (possibly Planner's SDD, possibly | ||
| 1211 | + one of Coder's own implementation nodes, anywhere in the shared run) | ||
| 1212 | + is actually at fault, reusing work_graph_orchestrate.py's existing | ||
| 1213 | + dynamic_loop_back machinery (Phase 2a) rather than reimplementing a | ||
| 1214 | + parallel Python-level retry loop. The real test-script hard gate | ||
| 1215 | + (2026-08-06, user-confirmed: script is the hard gate, subjective | ||
| 1216 | + judgment can only add INCONCLUSIVE/risk notes on top of a passing | ||
| 1217 | + script, never override a failing one to PASS) is preserved via | ||
| 1218 | + _verdict_node_build_prompt/_verdict_says_pass_check's shared mutable | ||
| 1219 | + state -- still genuinely Python-enforced, not just prompted. | ||
| 1220 | + | ||
| 1221 | + **Contract change from the old flat-call version**: because the | ||
| 1222 | + final_verdict node's own mechanical judgment treats anything other | ||
| 1223 | + than PASS as ok=False (INCONCLUSIVE included, see _verdict_says_pass_ | ||
| 1224 | + check's own docstring for why) and retries via dynamic_loop_back up to | ||
| 1225 | + its attempt budget, this function can now only ever RETURN normally | ||
| 1226 | + with verdict='PASS' -- a goal that can't be resolved within the loop | ||
| 1227 | + budget raises ConsistencyCheckFailedError instead (same as any other | ||
| 1228 | + exhausted work_graph_orchestrate.py loop) rather than returning a | ||
| 1229 | + 'FAIL'/'INCONCLUSIVE' verdict. This is what naturally retires org_ | ||
| 1230 | + level_loop.py's involvement for tasks using this path: scheduler.py:: | ||
| 1231 | + finish_task_lifecycle's "verdict != PASS -> run_fix_loop()" branch | ||
| 1232 | + becomes unreachable here, not because of an explicit new-flow-vs-old- | ||
| 1233 | + flow check, but because the condition it's gated on can no longer | ||
| 1234 | + occur on this path (resolves design doc §8 point 4 without adding one). | ||
| 1235 | + A caller that genuinely needs the old three-way PASS/FAIL/INCONCLUSIVE | ||
| 1236 | + contract (e.g. this function failing for a reason unrelated to the | ||
| 1237 | + goal itself) still gets that distinction via scheduler.py's existing | ||
| 1238 | + exception-based blocked_reason routing, same mechanism prd_consistency_ | ||
| 1239 | + check_failed/test_script_verification_failed already use. | ||
| 1240 | + | ||
| 1241 | + Returns {"verdict": "PASS", "raw_response", "artifact_ids", "script_execution"}. | ||
| 1108 | """ | 1242 | """ |
| 1109 | - run_id = str(uuid.uuid4()) | ||
| 1110 | - conn.execute( | ||
| 1111 | - "INSERT INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " | ||
| 1112 | - "VALUES (?, ?, 'validator', ?, 'running')", | ||
| 1113 | - (run_id, task_id, json.dumps(_VALIDATOR_MANIFEST)), | 1243 | + planning_context = _VALIDATOR_PLANNING_CONTEXT.format( |
| 1244 | + goal_text=goal_text, sdd_content=sdd_content, prd_content=prd_content, | ||
| 1245 | + ) | ||
| 1246 | + plan_result = work_graph_plan.plan_work_graph( | ||
| 1247 | + conn, "validator", task_id, planning_context, target_folder_path, | ||
| 1248 | + timeout=300, append_to_run_id=run_id, | ||
| 1249 | + ) | ||
| 1250 | + graph_manifest = plan_result["graph_manifest"] | ||
| 1251 | + new_node_ids = set(plan_result["new_node_ids"]) | ||
| 1252 | + node_types = {n["node_id"]: n.get("node_type") for n in graph_manifest["nodes"]} | ||
| 1253 | + | ||
| 1254 | + verdict_node_ids = [nid for nid, nt in node_types.items() if nt == "final_verdict" and nid in new_node_ids] | ||
| 1255 | + if len(verdict_node_ids) != 1: | ||
| 1256 | + raise work_graph_plan.PlanningFailedError( | ||
| 1257 | + "validator's self-planned graph must have exactly one node_type='final_verdict' node, " | ||
| 1258 | + "found {}: {}".format(len(verdict_node_ids), verdict_node_ids) | ||
| 1259 | + ) | ||
| 1260 | + verdict_node_id = verdict_node_ids[0] | ||
| 1261 | + | ||
| 1262 | + node_artifact_types = {} | ||
| 1263 | + for n in graph_manifest["nodes"]: | ||
| 1264 | + if "artifact_type" in n: | ||
| 1265 | + node_artifact_types[n["node_id"]] = n["artifact_type"] | ||
| 1266 | + elif n.get("promotes_to_artifact"): | ||
| 1267 | + node_artifact_types[n["node_id"]] = n["promotes_to_artifact"]["artifact_type"] | ||
| 1268 | + | ||
| 1269 | + planner_builder = _planner_prompt_builder(goal_text, node_artifact_types) | ||
| 1270 | + base_build_prompt = work_graph_plan.dispatch_build_prompt(planner_builder, work_graph_plan.generic_node_prompt_builder) | ||
| 1271 | + shared_state = {} | ||
| 1272 | + build_prompt = _verdict_node_build_prompt( | ||
| 1273 | + base_build_prompt, node_types, task_id, target_folder_path, shared_state, timeout, | ||
| 1114 | ) | 1274 | ) |
| 1115 | - conn.commit() | ||
| 1116 | 1275 | ||
| 1117 | - node_artifact_types = {n["node_id"]: n["artifact_type"] for n in _VALIDATOR_MANIFEST["nodes"]} | ||
| 1118 | - # 2026-08-07: same run_graph()-partial-failure salvage as | ||
| 1119 | - # plan_documents() -- see _write_completed_node_artifacts's own | ||
| 1120 | - # docstring and docs/todo/work-graph-partial-artifact-persistence- | ||
| 1121 | - # requirements.md. `outputs` is only used below on the SUCCESS | ||
| 1122 | - # continuation (script execution + verdict prompt); the failure path | ||
| 1123 | - # re-raises before ever touching it. | ||
| 1124 | run_graph_error = None | 1276 | run_graph_error = None |
| 1125 | outputs = None | 1277 | outputs = None |
| 1126 | try: | 1278 | try: |
| 1127 | outputs = work_graph_orchestrate.run_graph( | 1279 | outputs = work_graph_orchestrate.run_graph( |
| 1128 | - conn, run_id, "validator", "validator-independent-verification", target_folder_path, _VALIDATOR_MANIFEST, | ||
| 1129 | - _validator_prompt_builder(goal_text, sdd_content, prd_content, node_artifact_types), | ||
| 1130 | - _DOC_INPUT_SCHEMA, _DOC_OUTPUT_SCHEMA, | ||
| 1131 | - verify_fn=_test_script_verify_fn(node_artifact_types, script_artifact_type="validator_test_script"), | ||
| 1132 | - timeout=timeout, | 1280 | + conn, run_id, "validator", "validator-independent-verification", target_folder_path, graph_manifest, |
| 1281 | + build_prompt, _DOC_INPUT_SCHEMA, _DOC_OUTPUT_SCHEMA, timeout=timeout, | ||
| 1282 | + mechanical_check_registry={ | ||
| 1283 | + "check_script_compiles": check_script_compiles, | ||
| 1284 | + "verdict_says_pass": _verdict_says_pass_check(shared_state), | ||
| 1285 | + }, | ||
| 1133 | ) | 1286 | ) |
| 1134 | except Exception as e: | 1287 | except Exception as e: |
| 1135 | run_graph_error = e | 1288 | run_graph_error = e |
| 1136 | 1289 | ||
| 1137 | artifact_ids = _write_completed_node_artifacts( | 1290 | artifact_ids = _write_completed_node_artifacts( |
| 1138 | - conn, target_folder_path, task_id, run_id, _VALIDATOR_MANIFEST, node_artifact_types, | ||
| 1139 | - lambda artifact_type: "txt" if artifact_type != "validator_benchmark" else "md", run_graph_error, | 1291 | + conn, target_folder_path, task_id, run_id, graph_manifest, node_artifact_types, |
| 1292 | + _planner_ext_for, run_graph_error, only_node_ids=new_node_ids, | ||
| 1140 | ) | 1293 | ) |
| 1141 | 1294 | ||
| 1142 | if run_graph_error is not None: | 1295 | if run_graph_error is not None: |
| 1143 | raise run_graph_error | 1296 | raise run_graph_error |
| 1144 | 1297 | ||
| 1145 | - if "validator_test_script" in outputs: | ||
| 1146 | - script_content = outputs["validator_test_script"]["content"] | ||
| 1147 | - script_execution = run_test_script_for_real( | ||
| 1148 | - script_content, target_folder_path, task_id, "validator_test_script", timeout=timeout, | ||
| 1149 | - ) | ||
| 1150 | - else: | ||
| 1151 | - script_execution = { | ||
| 1152 | - "ran": False, "exit_code": None, "stdout": "", "stderr": "", | ||
| 1153 | - "note": "N/A: validator_test_script node is currently disabled (see _VALIDATOR_MANIFEST)", | ||
| 1154 | - } | 1298 | + # Reaching here means the final_verdict node's own mechanical judgment |
| 1299 | + # passed -- see this function's own docstring for why that means | ||
| 1300 | + # verdict is unconditionally 'PASS' at this point, not something that | ||
| 1301 | + # needs re-parsing/re-checking. | ||
| 1302 | + response = outputs[verdict_node_id]["content"] | ||
| 1303 | + script_execution = shared_state.get("script_execution") or { | ||
| 1304 | + "ran": False, "exit_code": None, "stdout": "", "stderr": "", | ||
| 1305 | + "note": "N/A: no test_script-typed node in this self-planned graph", | ||
| 1306 | + } | ||
| 1155 | 1307 | ||
| 1156 | - response = org_node_trigger.trigger_org_node( | ||
| 1157 | - "validator", | ||
| 1158 | - _VALIDATOR_VERDICT_PROMPT.format( | ||
| 1159 | - goal_text=goal_text, | ||
| 1160 | - validator_benchmark_content=outputs["validator_benchmark"]["content"], | ||
| 1161 | - script_execution_result=_format_script_execution_result(script_execution), | ||
| 1162 | - ), | ||
| 1163 | - target_folder_path, timeout=timeout, | ||
| 1164 | - ) | ||
| 1165 | - verdict = _parse_verdict_line(response) | ||
| 1166 | - | ||
| 1167 | - # Hard gate (user-confirmed 2026-08-06): a real non-zero exit from | ||
| 1168 | - # validator's OWN test script is not up for LLM debate -- force FAIL | ||
| 1169 | - # regardless of what the verdict line said. A passing/not-run script | ||
| 1170 | - # leaves the LLM's own judgment (including INCONCLUSIVE) untouched. | ||
| 1171 | - if script_execution["ran"] and script_execution["exit_code"] not in (0, None): | ||
| 1172 | - verdict = "FAIL" | ||
| 1173 | - | ||
| 1174 | - # 2026-08-07 (real incident): this function INSERTs its work_graph_run | ||
| 1175 | - # row as status='running' at the top and, on the happy path (verify_fn | ||
| 1176 | - # returns ok=True, so _verify_and_maybe_regenerate never touches | ||
| 1177 | - # work_graph_run.status at all -- it only writes 'reflecting'/ | ||
| 1178 | - # 'verifying'/'blocked' on the RETRY path), never wrote anything past | ||
| 1179 | - # that -- regardless of PASS/FAIL/INCONCLUSIVE, a validation run that | ||
| 1180 | - # got this far genuinely finished executing. `ux_work_graph_run_active` | ||
| 1181 | - # (schema.py) is a unique index on (task_id, org_node) covering exactly | ||
| 1182 | - # this non-terminal status set, so a real re-verify later (org_level_ | ||
| 1183 | - # loop.run_fix_loop after a FAIL) collided with this row on INSERT -- | ||
| 1184 | - # "UNIQUE constraint failed: work_graph_run.task_id, work_graph_run. | ||
| 1185 | - # org_node" -- found for real via a task reconciliation resume. | 1308 | + # 2026-08-07 (real incident, preserved): this run_id is now the SHARED |
| 1309 | + # Planner+Coder+Validator run (Phase 2c), not a Validator-only one -- | ||
| 1310 | + # marking it 'done' here is still correct: reaching this line means the | ||
| 1311 | + # goal genuinely resolved to PASS, the task really is done. | ||
| 1312 | + # ux_work_graph_run_active's real re-verify collision this comment | ||
| 1313 | + # originally described no longer applies the same way once §2.2 lands | ||
| 1314 | + # (Phase 2c also narrows that index) -- kept here as the historical | ||
| 1315 | + # record of why this UPDATE exists at all, not because the exact old | ||
| 1316 | + # failure mode is still reachable. | ||
| 1186 | conn.execute("UPDATE work_graph_run SET status = 'done' WHERE run_id = ?", (run_id,)) | 1317 | conn.execute("UPDATE work_graph_run SET status = 'done' WHERE run_id = ?", (run_id,)) |
| 1187 | conn.commit() | 1318 | conn.commit() |
| 1188 | 1319 | ||
| 1189 | return { | 1320 | return { |
| 1190 | - "verdict": verdict, "raw_response": response, | 1321 | + "verdict": "PASS", "raw_response": response, |
| 1191 | "artifact_ids": artifact_ids, "script_execution": script_execution, | 1322 | "artifact_ids": artifact_ids, "script_execution": script_execution, |
| 1192 | } | 1323 | } |
| 1193 | 1324 | ||
| @@ -1246,7 +1377,7 @@ def run_task_lifecycle(conn, task_id, target_folder_path, goal_text): | @@ -1246,7 +1377,7 @@ def run_task_lifecycle(conn, task_id, target_folder_path, goal_text): | ||
| 1246 | 1377 | ||
| 1247 | org_node_lifecycle.trigger_org_node_full(conn, "validator", task_id, target_folder_path, _READY_PROMPT) | 1378 | org_node_lifecycle.trigger_org_node_full(conn, "validator", task_id, target_folder_path, _READY_PROMPT) |
| 1248 | verdict = validate_implementation_with_own_graph( | 1379 | verdict = validate_implementation_with_own_graph( |
| 1249 | - conn, task_id, sdd_content, prd_content, goal_text, target_folder_path, | 1380 | + conn, task_id, plan_result["run_id"], sdd_content, prd_content, goal_text, target_folder_path, |
| 1250 | ) | 1381 | ) |
| 1251 | 1382 | ||
| 1252 | return { | 1383 | return { |
src/task_execution.py
| @@ -64,6 +64,21 @@ def _latest_artifact_ids(conn, task_id, artifact_types): | @@ -64,6 +64,21 @@ def _latest_artifact_ids(conn, task_id, artifact_types): | ||
| 64 | return {r[0]: r[1] for r in rows} | 64 | return {r[0]: r[1] for r in rows} |
| 65 | 65 | ||
| 66 | 66 | ||
| 67 | +def _shared_run_id_for_task(conn, task_id): | ||
| 68 | + """Phase 2c: recovers the shared Planner+Coder+Validator work_graph_run | ||
| 69 | + id for a task resumed straight from 'coding'/'validating' (no plan_result | ||
| 70 | + in scope at that point, unlike the 'reviewing_planner_output' branch | ||
| 71 | + above) -- same "read it off a real artifact row's produced_by_run_id" | ||
| 72 | + trick e2e_pipeline._review_planner_output_with_retries() already uses, | ||
| 73 | + since every real artifact row has a non-null FK to the run that made it.""" | ||
| 74 | + row = conn.execute( | ||
| 75 | + "SELECT produced_by_run_id FROM artifact WHERE task_id = ? AND artifact_type = 'sdd' " | ||
| 76 | + "ORDER BY created_at DESC LIMIT 1", | ||
| 77 | + (task_id,), | ||
| 78 | + ).fetchone() | ||
| 79 | + return row[0] if row else None | ||
| 80 | + | ||
| 81 | + | ||
| 67 | def _artifact_content_or_default(conn, target_folder_path, task_id, artifact_type, default): | 82 | def _artifact_content_or_default(conn, target_folder_path, task_id, artifact_type, default): |
| 68 | """Soft-fail artifact content lookup: returns `default` instead of | 83 | """Soft-fail artifact content lookup: returns `default` instead of |
| 69 | raising when no such artifact exists for this task yet -- e.g. the | 84 | raising when no such artifact exists for this task yet -- e.g. the |
| @@ -164,8 +179,9 @@ def run_or_resume_task_lifecycle(conn, task_id, target_folder_path, goal_text): | @@ -164,8 +179,9 @@ def run_or_resume_task_lifecycle(conn, task_id, target_folder_path, goal_text): | ||
| 164 | 179 | ||
| 165 | _check_not_paused(conn, task_id) | 180 | _check_not_paused(conn, task_id) |
| 166 | org_node_lifecycle.trigger_org_node_full(conn, "coder", task_id, target_folder_path, e2e_pipeline._READY_PROMPT) | 181 | org_node_lifecycle.trigger_org_node_full(conn, "coder", task_id, target_folder_path, e2e_pipeline._READY_PROMPT) |
| 182 | + run_id = plan_result["run_id"] | ||
| 167 | code_artifact_ids = e2e_pipeline.implement_code( | 183 | code_artifact_ids = e2e_pipeline.implement_code( |
| 168 | - conn, task_id, plan_result["run_id"], sdd_content, goal_text, target_folder_path, | 184 | + conn, task_id, run_id, sdd_content, goal_text, target_folder_path, |
| 169 | benchmark_content=benchmark_content, test_dataset_content=test_dataset_content, | 185 | benchmark_content=benchmark_content, test_dataset_content=test_dataset_content, |
| 170 | test_script_content=test_script_content, | 186 | test_script_content=test_script_content, |
| 171 | ) | 187 | ) |
| @@ -180,6 +196,12 @@ def run_or_resume_task_lifecycle(conn, task_id, target_folder_path, goal_text): | @@ -180,6 +196,12 @@ def run_or_resume_task_lifecycle(conn, task_id, target_folder_path, goal_text): | ||
| 180 | sdd_content = _artifact_content_or_default(conn, target_folder_path, task_id, "sdd", "N/A: not provided") | 196 | sdd_content = _artifact_content_or_default(conn, target_folder_path, task_id, "sdd", "N/A: not provided") |
| 181 | prd_content = _artifact_content_or_default(conn, target_folder_path, task_id, "prd", "N/A: not provided") | 197 | prd_content = _artifact_content_or_default(conn, target_folder_path, task_id, "prd", "N/A: not provided") |
| 182 | code_artifact_ids = _latest_artifact_ids(conn, task_id, _CODE_ARTIFACT_TYPES) | 198 | code_artifact_ids = _latest_artifact_ids(conn, task_id, _CODE_ARTIFACT_TYPES) |
| 199 | + run_id = _shared_run_id_for_task(conn, task_id) | ||
| 200 | + if run_id is None: | ||
| 201 | + raise TaskExecutionError( | ||
| 202 | + "task {} is status={} but has no sdd artifact to recover a work_graph_run id from" | ||
| 203 | + .format(task_id, status) | ||
| 204 | + ) | ||
| 183 | 205 | ||
| 184 | _check_not_paused(conn, task_id) | 206 | _check_not_paused(conn, task_id) |
| 185 | if status != "validating": | 207 | if status != "validating": |
| @@ -188,7 +210,7 @@ def run_or_resume_task_lifecycle(conn, task_id, target_folder_path, goal_text): | @@ -188,7 +210,7 @@ def run_or_resume_task_lifecycle(conn, task_id, target_folder_path, goal_text): | ||
| 188 | # hit org_node_lifecycle's own PRE_STATE guard (needs "coding"). | 210 | # hit org_node_lifecycle's own PRE_STATE guard (needs "coding"). |
| 189 | org_node_lifecycle.trigger_org_node_full(conn, "validator", task_id, target_folder_path, e2e_pipeline._READY_PROMPT) | 211 | org_node_lifecycle.trigger_org_node_full(conn, "validator", task_id, target_folder_path, e2e_pipeline._READY_PROMPT) |
| 190 | verdict = e2e_pipeline.validate_implementation_with_own_graph( | 212 | verdict = e2e_pipeline.validate_implementation_with_own_graph( |
| 191 | - conn, task_id, sdd_content, prd_content, goal_text, target_folder_path, | 213 | + conn, task_id, run_id, sdd_content, prd_content, goal_text, target_folder_path, |
| 192 | ) | 214 | ) |
| 193 | 215 | ||
| 194 | return { | 216 | return { |
src/test_e2e_pipeline.py
| @@ -115,24 +115,45 @@ _FAKE_CODER_GRAPH_MANIFEST = { | @@ -115,24 +115,45 @@ _FAKE_CODER_GRAPH_MANIFEST = { | ||
| 115 | "edges": [], | 115 | "edges": [], |
| 116 | } | 116 | } |
| 117 | 117 | ||
| 118 | +# Phase 2c (2026-08-12): Validator now self-plans too, into the SAME shared | ||
| 119 | +# run as Planner/Coder -- depends_on "sdd" for the same "avoid landing in | ||
| 120 | +# Planner's own already-'done' in-degree-zero layer" reason | ||
| 121 | +# _FAKE_CODER_GRAPH_MANIFEST's own comment gives. A single node_type= | ||
| 122 | +# 'final_verdict' node is the minimal real shape validate_implementation_ | ||
| 123 | +# with_own_graph() accepts (see work_graph_plan._VALIDATOR_NODE_CATALOG's | ||
| 124 | +# own catalog entry) -- no test_script node here, this fixture is about | ||
| 125 | +# pipeline wiring, not the hard-gate mechanism (see VerdictHardGateTest/ | ||
| 126 | +# ValidateImplementationWithOwnGraphTest above for that). | ||
| 127 | +_FAKE_VALIDATOR_GRAPH_MANIFEST = { | ||
| 128 | + "nodes": [{ | ||
| 129 | + "node_id": "verdict", "depends_on": ["sdd"], "task_prompt": "give your final verdict", | ||
| 130 | + "node_type": "final_verdict", | ||
| 131 | + "judgment": {"kind": "mechanical", "check_ref": "verdict_says_pass"}, | ||
| 132 | + "on_fail": {"action": "dynamic_loop_back", "max_attempts": 3}, | ||
| 133 | + }], | ||
| 134 | + "edges": [], | ||
| 135 | +} | ||
| 136 | + | ||
| 118 | 137 | ||
| 119 | _real_subprocess_run = subprocess.run # captured before any test patches subprocess.run globally | 138 | _real_subprocess_run = subprocess.run # captured before any test patches subprocess.run globally |
| 120 | 139 | ||
| 121 | 140 | ||
| 122 | def _fake_plan_subprocess_run_dispatch(*args, **kwargs): | 141 | def _fake_plan_subprocess_run_dispatch(*args, **kwargs): |
| 123 | """Real plan_work_graph() dispatches by the --agent argv value -- this | 142 | """Real plan_work_graph() dispatches by the --agent argv value -- this |
| 124 | - mock does the same, so both Planner's and Coder's planning calls (which | ||
| 125 | - now share the same mocked subprocess.run patch target) each get their | ||
| 126 | - own realistic fake manifest instead of one hardcoded response serving | ||
| 127 | - both callers (which would hand Coder Planner's own brd/prd/... nodes, | ||
| 128 | - colliding with the ones already in the run it's appending onto). | 143 | + mock does the same, so Planner's/Coder's/Validator's planning calls |
| 144 | + (which now all share the same mocked subprocess.run patch target) each | ||
| 145 | + get their own realistic fake manifest instead of one hardcoded response | ||
| 146 | + serving every caller (which would hand Coder/Validator Planner's own | ||
| 147 | + brd/prd/... nodes, colliding with the ones already in the run they're | ||
| 148 | + appending onto). | ||
| 129 | 149 | ||
| 130 | patch("src.work_graph_plan.subprocess.run", ...) patches subprocess.run | 150 | patch("src.work_graph_plan.subprocess.run", ...) patches subprocess.run |
| 131 | GLOBALLY, not just within work_graph_plan.py -- `subprocess` is the same | 151 | GLOBALLY, not just within work_graph_plan.py -- `subprocess` is the same |
| 132 | singleton module object everywhere it's imported, so this side_effect | 152 | singleton module object everywhere it's imported, so this side_effect |
| 133 | also intercepts every OTHER real subprocess.run call this test's own | 153 | also intercepts every OTHER real subprocess.run call this test's own |
| 134 | call chain makes (artifact_write.write_artifact's `git add`/`git | 154 | call chain makes (artifact_write.write_artifact's `git add`/`git |
| 135 | - commit`, e2e_pipeline's `git diff`/`git rev-parse`, etc). Those aren't | 155 | + commit`, e2e_pipeline's `git diff`/`git rev-parse`, the final_verdict |
| 156 | + node's own REAL test-script execution, etc). Those aren't | ||
| 136 | `claude --agent ... -p ...` calls -- pass them through to the REAL | 157 | `claude --agent ... -p ...` calls -- pass them through to the REAL |
| 137 | subprocess.run instead of returning a canned fake "claude planning | 158 | subprocess.run instead of returning a canned fake "claude planning |
| 138 | response" CompletedProcess for them too (which happened to not visibly | 159 | response" CompletedProcess for them too (which happened to not visibly |
| @@ -144,7 +165,12 @@ def _fake_plan_subprocess_run_dispatch(*args, **kwargs): | @@ -144,7 +165,12 @@ def _fake_plan_subprocess_run_dispatch(*args, **kwargs): | ||
| 144 | if not argv or argv[0] != "claude": | 165 | if not argv or argv[0] != "claude": |
| 145 | return _real_subprocess_run(*args, **kwargs) | 166 | return _real_subprocess_run(*args, **kwargs) |
| 146 | org_node = argv[argv.index("--agent") + 1] | 167 | org_node = argv[argv.index("--agent") + 1] |
| 147 | - manifest = _FAKE_CODER_GRAPH_MANIFEST if org_node == "coder" else _FAKE_GRAPH_MANIFEST | 168 | + if org_node == "coder": |
| 169 | + manifest = _FAKE_CODER_GRAPH_MANIFEST | ||
| 170 | + elif org_node == "validator": | ||
| 171 | + manifest = _FAKE_VALIDATOR_GRAPH_MANIFEST | ||
| 172 | + else: | ||
| 173 | + manifest = _FAKE_GRAPH_MANIFEST | ||
| 148 | payload = {"is_error": False, "result": "```json\n" + json.dumps(manifest) + "\n```"} | 174 | payload = {"is_error": False, "result": "```json\n" + json.dumps(manifest) + "\n```"} |
| 149 | return subprocess.CompletedProcess(args=["claude"], returncode=0, stdout=json.dumps(payload), stderr="") | 175 | return subprocess.CompletedProcess(args=["claude"], returncode=0, stdout=json.dumps(payload), stderr="") |
| 150 | 176 | ||
| @@ -384,11 +410,10 @@ class RunEndToEndTest(PipelineTestBase): | @@ -384,11 +410,10 @@ class RunEndToEndTest(PipelineTestBase): | ||
| 384 | # content-keyed like every other self-planned node, not the | 410 | # content-keyed like every other self-planned node, not the |
| 385 | # old single synthetic node's response-keyed output. | 411 | # old single synthetic node's response-keyed output. |
| 386 | {"content": "Implemented. Ran `pytest`: 3 passed."}, | 412 | {"content": "Implemented. Ran `pytest`: 3 passed."}, |
| 387 | - # Validator's OWN independent benchmark/dataset (2026-08-06) -- | ||
| 388 | - # deliberately different content from the Planner's, to also prove | ||
| 389 | - # nothing here accidentally reuses the Planner's copies. | ||
| 390 | - {"content": "# Validator's own Benchmark\nfake"}, | ||
| 391 | - {"content": "N/A: validator judged no dataset needed either"}, | 413 | + # Phase 2c (2026-08-12): Validator's own verdict is now a |
| 414 | + # self-planned node too (_FAKE_VALIDATOR_GRAPH_MANIFEST), | ||
| 415 | + # not a flat org_node_trigger.trigger_org_node call. | ||
| 416 | + {"content": "VERDICT: PASS\nAll scenarios pass."}, | ||
| 392 | ] | 417 | ] |
| 393 | result = e2e_pipeline.run_end_to_end(self.conn, self.tmp.name, "实现贪吃蛇游戏") | 418 | result = e2e_pipeline.run_end_to_end(self.conn, self.tmp.name, "实现贪吃蛇游戏") |
| 394 | 419 | ||
| @@ -406,35 +431,38 @@ class RunEndToEndTest(PipelineTestBase): | @@ -406,35 +431,38 @@ class RunEndToEndTest(PipelineTestBase): | ||
| 406 | all_artifacts = self.conn.execute( | 431 | all_artifacts = self.conn.execute( |
| 407 | "SELECT COUNT(*) FROM artifact WHERE task_id = ?", (result["task_id"],) | 432 | "SELECT COUNT(*) FROM artifact WHERE task_id = ?", (result["task_id"],) |
| 408 | ).fetchone()[0] | 433 | ).fetchone()[0] |
| 409 | - # brd, prd, sdd, benchmark, test_dataset, code_diff, test_report, | ||
| 410 | - # validator_benchmark, validator_test_dataset | ||
| 411 | - # (test_script/validator_test_script disabled, see _VALIDATOR_MANIFEST) | ||
| 412 | - self.assertEqual(all_artifacts, 9) | ||
| 413 | - | ||
| 414 | - # Phase 2 §6 (2026-08-12): Coder no longer gets its own work_graph_run | ||
| 415 | - # -- implement_code() appends its self-planned nodes onto Planner's | ||
| 416 | - # SAME run (plan_work_graph(..., append_to_run_id=...)), so only TWO | ||
| 417 | - # rows exist now, not three: the shared Planner+Coder run (still | ||
| 418 | - # labeled org_node='planner' -- that column reflects who ORIGINATED | ||
| 419 | - # the run, not every org_node that has since contributed nodes to it; | ||
| 420 | - # Phase 2c is expected to fold Validator into the same run too, at | ||
| 421 | - # which point this column stops being a reliable "who's involved" | ||
| 422 | - # signal at all, see design doc §2.1) and Validator's own separate run. | 434 | + # brd, prd, sdd, benchmark, test_dataset, code_diff, test_report -- |
| 435 | + # Phase 2c: Validator's self-planned "verdict" node deliberately | ||
| 436 | + # does NOT declare promotes_to_artifact (a verdict is a judgment, | ||
| 437 | + # not a persisted deliverable -- see _generic_prompt_template's own | ||
| 438 | + # instruction), so no validator_* artifacts exist anymore. | ||
| 439 | + self.assertEqual(all_artifacts, 7) | ||
| 440 | + | ||
| 441 | + # Phase 2c (2026-08-12): Validator now ALSO appends its self-planned | ||
| 442 | + # nodes onto the SAME shared run (plan_work_graph(..., append_to_ | ||
| 443 | + # run_id=...)) instead of creating its own separate row -- only ONE | ||
| 444 | + # work_graph_run row exists for this task now, still labeled | ||
| 445 | + # org_node='planner' (that column reflects who ORIGINATED the run, | ||
| 446 | + # not every org_node that has since contributed nodes to it -- §2.1 | ||
| 447 | + # is expected to retire this column's meaning once it's no longer | ||
| 448 | + # a reliable "who's involved" signal at all). | ||
| 423 | run_rows = self.conn.execute( | 449 | run_rows = self.conn.execute( |
| 424 | - "SELECT run_id, org_node FROM work_graph_run WHERE task_id = ?", (result["task_id"],) | 450 | + "SELECT run_id, org_node, status FROM work_graph_run WHERE task_id = ?", (result["task_id"],) |
| 425 | ).fetchall() | 451 | ).fetchall() |
| 426 | - self.assertEqual(len(run_rows), 2) | ||
| 427 | - self.assertEqual({row[1] for row in run_rows}, {"planner", "validator"}) | 452 | + self.assertEqual(len(run_rows), 1) |
| 453 | + self.assertEqual(run_rows[0][1], "planner") | ||
| 454 | + self.assertEqual(run_rows[0][2], "done") | ||
| 428 | 455 | ||
| 429 | - # Coder's self-planned "impl" node genuinely landed in the SAME run | ||
| 430 | - # as Planner's nodes, not a separate one. | ||
| 431 | - planner_run_id = next(row[0] for row in run_rows if row[1] == "planner") | 456 | + # Coder's "impl" AND Validator's "verdict" nodes both genuinely |
| 457 | + # landed in the SAME run as Planner's own nodes. | ||
| 458 | + shared_run_id = run_rows[0][0] | ||
| 432 | node_ids = { | 459 | node_ids = { |
| 433 | row[0] for row in self.conn.execute( | 460 | row[0] for row in self.conn.execute( |
| 434 | - "SELECT node_id FROM work_node_execution WHERE run_id = ?", (planner_run_id,) | 461 | + "SELECT node_id FROM work_node_execution WHERE run_id = ?", (shared_run_id,) |
| 435 | ).fetchall() | 462 | ).fetchall() |
| 436 | } | 463 | } |
| 437 | self.assertIn("impl", node_ids) | 464 | self.assertIn("impl", node_ids) |
| 465 | + self.assertIn("verdict", node_ids) | ||
| 438 | self.assertIn("sdd", node_ids) | 466 | self.assertIn("sdd", node_ids) |
| 439 | 467 | ||
| 440 | 468 | ||
| @@ -603,7 +631,7 @@ class RunTaskLifecycleMissingArtifactTypesTest(PipelineTestBase): | @@ -603,7 +631,7 @@ class RunTaskLifecycleMissingArtifactTypesTest(PipelineTestBase): | ||
| 603 | captured["benchmark_content"] = kwargs.get("benchmark_content") | 631 | captured["benchmark_content"] = kwargs.get("benchmark_content") |
| 604 | return {"code_diff": "diff-1", "test_report": "report-1"} | 632 | return {"code_diff": "diff-1", "test_report": "report-1"} |
| 605 | 633 | ||
| 606 | - def fake_validate(conn, task_id, sdd_content, prd_content, goal_text, target_folder_path, **kwargs): | 634 | + def fake_validate(conn, task_id, run_id, sdd_content, prd_content, goal_text, target_folder_path, **kwargs): |
| 607 | captured["prd_content"] = prd_content | 635 | captured["prd_content"] = prd_content |
| 608 | return {"verdict": "PASS", "raw_response": "ok"} | 636 | return {"verdict": "PASS", "raw_response": "ok"} |
| 609 | 637 | ||
| @@ -919,205 +947,234 @@ class RunTestScriptForRealTest(unittest.TestCase): | @@ -919,205 +947,234 @@ class RunTestScriptForRealTest(unittest.TestCase): | ||
| 919 | self.assertTrue(os.path.exists(expected)) | 947 | self.assertTrue(os.path.exists(expected)) |
| 920 | 948 | ||
| 921 | 949 | ||
| 922 | -# validator_test_script disabled by default 2026-08-07 (see | ||
| 923 | -# e2e_pipeline._VALIDATOR_MANIFEST's own comment) -- tests below that need | ||
| 924 | -# to exercise the hard-gate/salvage mechanisms involving that node patch | ||
| 925 | -# _VALIDATOR_MANIFEST back to this 3-node shape for their own duration, so | ||
| 926 | -# that real code (still in the codebase, just not planned by default) stays | ||
| 927 | -# covered instead of silently rotting. | ||
| 928 | -_VALIDATOR_MANIFEST_WITH_TEST_SCRIPT = { | 950 | +def _fake_validator_plan_subprocess_run(manifest): |
| 951 | + # Same real-passthrough-for-non-'claude'-argv requirement as this | ||
| 952 | + # file's other subprocess.run fakes (see _fake_plan_subprocess_run_ | ||
| 953 | + # dispatch's own docstring) -- the final_verdict node's build_prompt | ||
| 954 | + # wrapper actually EXECUTES a real test script via a real subprocess.run | ||
| 955 | + # call of its own (run_test_script_for_real/_detect_script_language), | ||
| 956 | + # which would otherwise also get intercepted by this same globally- | ||
| 957 | + # patched target and handed the fake planning JSON instead of a real | ||
| 958 | + # exit code. | ||
| 959 | + def run(*args, **kwargs): | ||
| 960 | + argv = args[0] if args else kwargs.get("args") | ||
| 961 | + if not argv or argv[0] != "claude": | ||
| 962 | + return _real_subprocess_run(*args, **kwargs) | ||
| 963 | + payload = {"is_error": False, "result": "```json\n" + json.dumps(manifest) + "\n```"} | ||
| 964 | + return subprocess.CompletedProcess(args=["claude"], returncode=0, stdout=json.dumps(payload), stderr="") | ||
| 965 | + return run | ||
| 966 | + | ||
| 967 | + | ||
| 968 | +# on_fail.action='hard_fail' below (not the real catalog's suggested | ||
| 969 | +# 'dynamic_loop_back') is a deliberate test-fixture choice, not something a | ||
| 970 | +# real self-planned graph would use: it isolates the hard-gate mechanical | ||
| 971 | +# check itself (this file's job) from the separate dynamic_loop_back/ | ||
| 972 | +# Reflection-review retry machinery, which already has its own dedicated | ||
| 973 | +# coverage in test_work_graph_orchestrate.py (DynamicLoopBackContentErrorTest/ | ||
| 974 | +# DynamicLoopBackSkillDefectTest/TallyReflectionTest) -- mocking a 3-perspective | ||
| 975 | +# real reflection jury here would be expensive and wouldn't test anything new. | ||
| 976 | +_FAKE_VALIDATOR_MANIFEST_VERDICT_ONLY = { | ||
| 977 | + "nodes": [{ | ||
| 978 | + "node_id": "verdict", "depends_on": [], "task_prompt": "give your final verdict", | ||
| 979 | + "node_type": "final_verdict", | ||
| 980 | + "judgment": {"kind": "mechanical", "check_ref": "verdict_says_pass"}, | ||
| 981 | + "on_fail": {"action": "hard_fail"}, | ||
| 982 | + }], | ||
| 983 | + "edges": [], | ||
| 984 | +} | ||
| 985 | + | ||
| 986 | +_FAKE_VALIDATOR_MANIFEST_WITH_SCRIPT = { | ||
| 929 | "nodes": [ | 987 | "nodes": [ |
| 930 | - {"node_id": "validator_benchmark", "artifact_type": "validator_benchmark", "depends_on": []}, | ||
| 931 | { | 988 | { |
| 932 | - "node_id": "validator_test_dataset", "artifact_type": "validator_test_dataset", | ||
| 933 | - "depends_on": ["validator_benchmark"], | 989 | + "node_id": "test_script", "depends_on": [], "task_prompt": "write a real test script", |
| 990 | + "node_type": "test_script", | ||
| 991 | + "judgment": {"kind": "mechanical", "check_ref": "check_script_compiles"}, | ||
| 934 | }, | 992 | }, |
| 935 | { | 993 | { |
| 936 | - "node_id": "validator_test_script", "artifact_type": "validator_test_script", | ||
| 937 | - "depends_on": ["validator_test_dataset", "validator_benchmark"], | 994 | + "node_id": "verdict", "depends_on": ["test_script"], "task_prompt": "give your final verdict", |
| 995 | + "node_type": "final_verdict", | ||
| 996 | + "judgment": {"kind": "mechanical", "check_ref": "verdict_says_pass"}, | ||
| 997 | + "on_fail": {"action": "hard_fail"}, | ||
| 938 | }, | 998 | }, |
| 939 | ], | 999 | ], |
| 940 | "edges": [], | 1000 | "edges": [], |
| 941 | } | 1001 | } |
| 942 | 1002 | ||
| 943 | 1003 | ||
| 1004 | +class VerdictHardGateTest(unittest.TestCase): | ||
| 1005 | + """Phase 2c (2026-08-12): unit coverage of the two functions carrying | ||
| 1006 | + the safety-critical hard gate (2026-08-06, user-confirmed: a real | ||
| 1007 | + non-zero exit from Validator's own test script forces FAIL regardless | ||
| 1008 | + of what the LLM's verdict line says) forward into the self-planned | ||
| 1009 | + final_verdict node design -- direct/isolated rather than through the | ||
| 1010 | + full self-planning + run_graph() + work_node_run machinery, which the | ||
| 1011 | + integration tests below cover for the wiring, not this specific logic.""" | ||
| 1012 | + | ||
| 1013 | + def test_real_script_failure_overrides_pass_verdict_text(self): | ||
| 1014 | + shared_state = {"script_execution": {"ran": True, "exit_code": 1, "stdout": "", "stderr": "boom"}} | ||
| 1015 | + ok, detail = e2e_pipeline._verdict_says_pass_check(shared_state)("VERDICT: PASS\nlooks good") | ||
| 1016 | + self.assertFalse(ok) | ||
| 1017 | + self.assertIn("exit_code=1", detail) | ||
| 1018 | + | ||
| 1019 | + def test_passing_script_and_pass_verdict_is_ok(self): | ||
| 1020 | + shared_state = {"script_execution": {"ran": True, "exit_code": 0, "stdout": "", "stderr": ""}} | ||
| 1021 | + ok, detail = e2e_pipeline._verdict_says_pass_check(shared_state)("VERDICT: PASS\nlooks good") | ||
| 1022 | + self.assertTrue(ok) | ||
| 1023 | + self.assertIsNone(detail) | ||
| 1024 | + | ||
| 1025 | + def test_no_script_and_pass_verdict_is_ok(self): | ||
| 1026 | + ok, _ = e2e_pipeline._verdict_says_pass_check({})("VERDICT: PASS\nlooks good") | ||
| 1027 | + self.assertTrue(ok) | ||
| 1028 | + | ||
| 1029 | + def test_fail_verdict_is_not_ok_even_with_no_script(self): | ||
| 1030 | + ok, detail = e2e_pipeline._verdict_says_pass_check({})("VERDICT: FAIL\nreal bug found") | ||
| 1031 | + self.assertFalse(ok) | ||
| 1032 | + self.assertIn("FAIL", detail) | ||
| 1033 | + | ||
| 1034 | + def test_inconclusive_verdict_is_not_ok(self): | ||
| 1035 | + # 2026-08-12 (Phase 2c, design decision): INCONCLUSIVE is treated | ||
| 1036 | + # the same as FAIL here, not specially accepted -- see | ||
| 1037 | + # validate_implementation_with_own_graph's own docstring for why. | ||
| 1038 | + ok, _ = e2e_pipeline._verdict_says_pass_check({})("VERDICT: INCONCLUSIVE\nneeds human review") | ||
| 1039 | + self.assertFalse(ok) | ||
| 1040 | + | ||
| 1041 | + def test_build_prompt_passes_through_unchanged_for_non_verdict_nodes(self): | ||
| 1042 | + shared_state = {} | ||
| 1043 | + inner = lambda node, outputs: "inner prompt for " + node["node_id"] | ||
| 1044 | + wrapped = e2e_pipeline._verdict_node_build_prompt(inner, {}, "task-1", "/tmp", shared_state, 60) | ||
| 1045 | + result = wrapped({"node_id": "impl"}, {}) | ||
| 1046 | + self.assertEqual(result, "inner prompt for impl") | ||
| 1047 | + self.assertEqual(shared_state, {}) | ||
| 1048 | + | ||
| 1049 | + def test_build_prompt_runs_real_script_for_verdict_node_with_script_dependency(self): | ||
| 1050 | + shared_state = {} | ||
| 1051 | + inner = lambda node, outputs: "base prompt" | ||
| 1052 | + node_types = {"test_script": "test_script"} | ||
| 1053 | + outputs = {"test_script": {"content": "import sys\nsys.exit(0)\n"}} | ||
| 1054 | + with tempfile.TemporaryDirectory() as tmp: | ||
| 1055 | + subprocess.run(["git", "init"], cwd=tmp, check=True, capture_output=True) | ||
| 1056 | + wrapped = e2e_pipeline._verdict_node_build_prompt(inner, node_types, "task-1", tmp, shared_state, 60) | ||
| 1057 | + result = wrapped({"node_id": "verdict", "node_type": "final_verdict"}, outputs) | ||
| 1058 | + self.assertTrue(shared_state["script_execution"]["ran"]) | ||
| 1059 | + self.assertEqual(shared_state["script_execution"]["exit_code"], 0) | ||
| 1060 | + self.assertIn("base prompt", result) | ||
| 1061 | + self.assertIn("REAL execution result", result) | ||
| 1062 | + | ||
| 1063 | + def test_build_prompt_na_stub_for_verdict_node_with_no_script_dependency(self): | ||
| 1064 | + shared_state = {} | ||
| 1065 | + inner = lambda node, outputs: "base prompt" | ||
| 1066 | + wrapped = e2e_pipeline._verdict_node_build_prompt(inner, {}, "task-1", "/tmp", shared_state, 60) | ||
| 1067 | + wrapped({"node_id": "verdict", "node_type": "final_verdict"}, {}) | ||
| 1068 | + self.assertFalse(shared_state["script_execution"]["ran"]) | ||
| 1069 | + | ||
| 1070 | + | ||
| 944 | class ValidateImplementationWithOwnGraphTest(unittest.TestCase): | 1071 | class ValidateImplementationWithOwnGraphTest(unittest.TestCase): |
| 945 | - """2026-08-06: the safety-critical piece -- a real non-zero exit from | ||
| 946 | - Validator's own script must force FAIL no matter what the LLM's verdict | ||
| 947 | - line says (user-confirmed hard gate).""" | 1072 | + """Phase 2c (2026-08-12): Validator now self-plans its own verification |
| 1073 | + graph, appended onto the SAME shared run Planner/Coder already | ||
| 1074 | + contributed to (plan_work_graph(..., append_to_run_id=run_id)) -- see | ||
| 1075 | + e2e_pipeline.validate_implementation_with_own_graph's own docstring. | ||
| 1076 | + VerdictHardGateTest above covers the hard-gate logic itself in | ||
| 1077 | + isolation; these tests cover the wiring: planning dispatch, mechanical | ||
| 1078 | + check registration, run_id reuse, and result unwrapping.""" | ||
| 948 | 1079 | ||
| 949 | def setUp(self): | 1080 | def setUp(self): |
| 950 | self.tmp = tempfile.TemporaryDirectory() | 1081 | self.tmp = tempfile.TemporaryDirectory() |
| 951 | self.addCleanup(self.tmp.cleanup) | 1082 | self.addCleanup(self.tmp.cleanup) |
| 952 | subprocess.run(["git", "init"], cwd=self.tmp.name, check=True, capture_output=True) | 1083 | subprocess.run(["git", "init"], cwd=self.tmp.name, check=True, capture_output=True) |
| 953 | - self.conn = schema.connect(":memory:") | 1084 | + self.conn = schema.connect(":memory:", check_same_thread=False) |
| 954 | schema.create_schema(self.conn) | 1085 | schema.create_schema(self.conn) |
| 955 | project_scaffold.create_project(self.conn, self.tmp.name, init_git_if_missing=False) | 1086 | project_scaffold.create_project(self.conn, self.tmp.name, init_git_if_missing=False) |
| 956 | self.conn.execute( | 1087 | self.conn.execute( |
| 957 | "INSERT INTO task (task_id, project_id, goal_text, task_type, status, assigned_org_node) " | 1088 | "INSERT INTO task (task_id, project_id, goal_text, task_type, status, assigned_org_node) " |
| 958 | "VALUES ('task-1', (SELECT project_id FROM project), 'goal', 'goal', 'validating', 'validator')" | 1089 | "VALUES ('task-1', (SELECT project_id FROM project), 'goal', 'goal', 'validating', 'validator')" |
| 959 | ) | 1090 | ) |
| 1091 | + # The shared Planner+Coder run Validator appends onto -- empty on | ||
| 1092 | + # purpose (these tests aren't about cross-stage node dependencies, | ||
| 1093 | + # see test_task_execution.py for that), just needs to genuinely | ||
| 1094 | + # exist for append_to_run_id's own existence check. | ||
| 1095 | + self.conn.execute( | ||
| 1096 | + "INSERT INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " | ||
| 1097 | + "VALUES ('run-shared', 'task-1', 'planner', '{\"nodes\": [], \"edges\": []}', 'running')" | ||
| 1098 | + ) | ||
| 960 | self.conn.commit() | 1099 | self.conn.commit() |
| 961 | 1100 | ||
| 962 | def tearDown(self): | 1101 | def tearDown(self): |
| 963 | self.conn.close() | 1102 | self.conn.close() |
| 964 | 1103 | ||
| 965 | - def _run(self, script_content, verdict_line, with_test_script=False): | ||
| 966 | - mock_responses = [ | ||
| 967 | - {"content": "# Validator's own Benchmark"}, | ||
| 968 | - {"content": "N/A: no dataset needed"}, | ||
| 969 | - ] | ||
| 970 | - if with_test_script: | 1104 | + def _run(self, manifest, verdict_content, script_content=None): |
| 1105 | + mock_responses = [] | ||
| 1106 | + if script_content is not None: | ||
| 971 | mock_responses.append({"content": script_content}) | 1107 | mock_responses.append({"content": script_content}) |
| 972 | - with contextlib.ExitStack() as stack: | ||
| 973 | - stack.enter_context(patch.object(work_graph_execute, "write_workflow_script")) | ||
| 974 | - mock_execute = stack.enter_context(patch.object(work_graph_execute, "execute_work_graph")) | ||
| 975 | - stack.enter_context(patch.object(org_node_trigger, "trigger_org_node", return_value=verdict_line)) | ||
| 976 | - if with_test_script: | ||
| 977 | - stack.enter_context( | ||
| 978 | - patch.object(e2e_pipeline, "_VALIDATOR_MANIFEST", _VALIDATOR_MANIFEST_WITH_TEST_SCRIPT) | ||
| 979 | - ) | 1108 | + mock_responses.append({"content": verdict_content}) |
| 1109 | + with patch("src.work_graph_plan.subprocess.run", side_effect=_fake_validator_plan_subprocess_run(manifest)), \ | ||
| 1110 | + patch.object(work_graph_execute, "write_workflow_script"), \ | ||
| 1111 | + patch.object(work_graph_execute, "execute_work_graph") as mock_execute: | ||
| 980 | mock_execute.side_effect = mock_responses | 1112 | mock_execute.side_effect = mock_responses |
| 981 | return e2e_pipeline.validate_implementation_with_own_graph( | 1113 | return e2e_pipeline.validate_implementation_with_own_graph( |
| 982 | - self.conn, "task-1", "sdd content", "prd content", "goal text", self.tmp.name, | 1114 | + self.conn, "task-1", "run-shared", "sdd content", "prd content", "goal text", self.tmp.name, |
| 983 | ) | 1115 | ) |
| 984 | 1116 | ||
| 985 | def test_llm_says_pass_and_script_really_passes_stays_pass(self): | 1117 | def test_llm_says_pass_and_script_really_passes_stays_pass(self): |
| 986 | - result = self._run("import sys\nsys.exit(0)\n", "VERDICT: PASS\nlooks good", with_test_script=True) | 1118 | + result = self._run( |
| 1119 | + _FAKE_VALIDATOR_MANIFEST_WITH_SCRIPT, "VERDICT: PASS\nlooks good", | ||
| 1120 | + script_content="import sys\nsys.exit(0)\n", | ||
| 1121 | + ) | ||
| 987 | self.assertEqual(result["verdict"], "PASS") | 1122 | self.assertEqual(result["verdict"], "PASS") |
| 988 | self.assertTrue(result["script_execution"]["ran"]) | 1123 | self.assertTrue(result["script_execution"]["ran"]) |
| 989 | self.assertEqual(result["script_execution"]["exit_code"], 0) | 1124 | self.assertEqual(result["script_execution"]["exit_code"], 0) |
| 990 | 1125 | ||
| 991 | - def test_llm_says_pass_but_script_really_fails_is_force_overridden_to_fail(self): | 1126 | + def test_llm_says_pass_but_script_really_fails_raises(self): |
| 992 | # the exact case the user's hard-gate rule exists for: a script | 1127 | # the exact case the user's hard-gate rule exists for: a script |
| 993 | # that objectively fails must not be argued away by the LLM's own | 1128 | # that objectively fails must not be argued away by the LLM's own |
| 994 | - # verdict line. | ||
| 995 | - result = self._run( | ||
| 996 | - "import sys\nsys.exit(1)\n", "VERDICT: PASS\nlooks good to me", with_test_script=True | ||
| 997 | - ) | ||
| 998 | - self.assertEqual(result["verdict"], "FAIL") | ||
| 999 | - | ||
| 1000 | - def test_llm_says_fail_and_script_fails_stays_fail(self): | ||
| 1001 | - result = self._run("import sys\nsys.exit(1)\n", "VERDICT: FAIL\nreal bug found") | ||
| 1002 | - self.assertEqual(result["verdict"], "FAIL") | 1129 | + # verdict line -- hard_fail (see the fixture's own comment) makes |
| 1130 | + # this deterministic without invoking the loop-back/reflection jury. | ||
| 1131 | + with self.assertRaises(work_graph_orchestrate.ConsistencyCheckFailedError): | ||
| 1132 | + self._run( | ||
| 1133 | + _FAKE_VALIDATOR_MANIFEST_WITH_SCRIPT, "VERDICT: PASS\nlooks good to me", | ||
| 1134 | + script_content="import sys\nsys.exit(1)\n", | ||
| 1135 | + ) | ||
| 1003 | 1136 | ||
| 1004 | - def test_na_script_leaves_llm_judgment_untouched(self): | ||
| 1005 | - result = self._run("N/A: no mechanical check possible", "VERDICT: INCONCLUSIVE\nneeds human review") | ||
| 1006 | - self.assertEqual(result["verdict"], "INCONCLUSIVE") | 1137 | + def test_no_script_node_leaves_llm_judgment_as_the_only_signal(self): |
| 1138 | + result = self._run(_FAKE_VALIDATOR_MANIFEST_VERDICT_ONLY, "VERDICT: PASS\nlooks good") | ||
| 1139 | + self.assertEqual(result["verdict"], "PASS") | ||
| 1007 | self.assertFalse(result["script_execution"]["ran"]) | 1140 | self.assertFalse(result["script_execution"]["ran"]) |
| 1008 | 1141 | ||
| 1009 | - def test_writes_two_validator_artifacts_with_lineage(self): | ||
| 1010 | - # test_script disabled by default -- see _VALIDATOR_MANIFEST_WITH_ | ||
| 1011 | - # TEST_SCRIPT variant below for the 3-artifact/script-lineage case. | ||
| 1012 | - result = self._run("import sys\nsys.exit(0)\n", "VERDICT: PASS\nlooks good") | ||
| 1013 | - self.assertEqual(set(result["artifact_ids"]), {"validator_benchmark", "validator_test_dataset"}) | ||
| 1014 | - dataset_deps = json.loads( | ||
| 1015 | - self.conn.execute( | ||
| 1016 | - "SELECT depends_on_artifact_ids FROM artifact WHERE artifact_id = ?", | ||
| 1017 | - (result["artifact_ids"]["validator_test_dataset"],), | ||
| 1018 | - ).fetchone()[0] | ||
| 1019 | - ) | ||
| 1020 | - self.assertIn(result["artifact_ids"]["validator_benchmark"], dataset_deps) | ||
| 1021 | - | ||
| 1022 | - def test_writes_three_validator_artifacts_with_lineage_when_test_script_re_enabled(self): | ||
| 1023 | - result = self._run("import sys\nsys.exit(0)\n", "VERDICT: PASS\nlooks good", with_test_script=True) | ||
| 1024 | - self.assertEqual( | ||
| 1025 | - set(result["artifact_ids"]), | ||
| 1026 | - {"validator_benchmark", "validator_test_dataset", "validator_test_script"}, | ||
| 1027 | - ) | ||
| 1028 | - script_deps = json.loads( | ||
| 1029 | - self.conn.execute( | ||
| 1030 | - "SELECT depends_on_artifact_ids FROM artifact WHERE artifact_id = ?", | ||
| 1031 | - (result["artifact_ids"]["validator_test_script"],), | ||
| 1032 | - ).fetchone()[0] | ||
| 1033 | - ) | ||
| 1034 | - self.assertIn(result["artifact_ids"]["validator_benchmark"], script_deps) | ||
| 1035 | - self.assertIn(result["artifact_ids"]["validator_test_dataset"], script_deps) | ||
| 1036 | - | ||
| 1037 | - def test_creates_its_own_work_graph_run_row(self): | ||
| 1038 | - result = self._run("import sys\nsys.exit(0)\n", "VERDICT: PASS\nlooks good") | ||
| 1039 | - run_row = self.conn.execute( | ||
| 1040 | - "SELECT org_node, status FROM work_graph_run WHERE task_id = 'task-1'" | ||
| 1041 | - ).fetchone() | ||
| 1042 | - self.assertEqual(run_row[0], "validator") | ||
| 1043 | - # 2026-08-07 (real incident): this used to stay 'running' forever on | ||
| 1044 | - # the happy path -- verify_fn returning ok=True means | ||
| 1045 | - # _verify_and_maybe_regenerate never touches work_graph_run.status | ||
| 1046 | - # at all, so nothing else in this function's old code ever marked | ||
| 1047 | - # it terminal. schema.py's `ux_work_graph_run_active` unique index | ||
| 1048 | - # covers exactly this non-terminal status set, so a stuck 'running' | ||
| 1049 | - # row silently blocked any later re-verify of the SAME task from | ||
| 1050 | - # ever creating its own new validator run. | ||
| 1051 | - self.assertEqual(run_row[1], "done") | ||
| 1052 | - | ||
| 1053 | - def test_second_validation_run_does_not_collide_with_the_first(self): | ||
| 1054 | - # The actual regression: org_level_loop.run_fix_loop re-verifying | ||
| 1055 | - # after a FAIL (or a reconciliation-loop resume retrying) calls this | ||
| 1056 | - # function again for the SAME task_id -- must not raise | ||
| 1057 | - # "UNIQUE constraint failed: work_graph_run.task_id, | ||
| 1058 | - # work_graph_run.org_node" against a first run stuck non-terminal. | ||
| 1059 | - self._run("import sys\nsys.exit(1)\n", "VERDICT: FAIL\nreal bug found") | ||
| 1060 | - | ||
| 1061 | - # Benchmark/dataset content must differ from the first call's -- | ||
| 1062 | - # write_artifact does a real `git commit`, which fails with "nothing | ||
| 1063 | - # to commit" if a second write produces byte-identical file content | ||
| 1064 | - # (the file path isn't run_id-scoped). Real re-verify runs would | ||
| 1065 | - # naturally regenerate slightly different content each time; this | ||
| 1066 | - # just makes that explicit rather than accidentally relying on it. | ||
| 1067 | - with patch.object(work_graph_execute, "write_workflow_script"), \ | ||
| 1068 | - patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | ||
| 1069 | - patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS\nfixed now"): | ||
| 1070 | - mock_execute.side_effect = [ | ||
| 1071 | - {"content": "# Validator's own Benchmark (re-verify)"}, | ||
| 1072 | - {"content": "N/A: no dataset needed, still"}, | ||
| 1073 | - {"content": "import sys\nsys.exit(0)\n"}, | ||
| 1074 | - ] | ||
| 1075 | - second = e2e_pipeline.validate_implementation_with_own_graph( | ||
| 1076 | - self.conn, "task-1", "sdd content", "prd content", "goal text", self.tmp.name, | ||
| 1077 | - ) | 1142 | + def test_appends_into_the_shared_run_without_creating_a_new_row(self): |
| 1143 | + self._run(_FAKE_VALIDATOR_MANIFEST_VERDICT_ONLY, "VERDICT: PASS\nlooks good") | ||
| 1144 | + rows = self.conn.execute("SELECT run_id, status FROM work_graph_run WHERE task_id = 'task-1'").fetchall() | ||
| 1145 | + self.assertEqual(rows, [("run-shared", "done")]) | ||
| 1146 | + node_ids = { | ||
| 1147 | + r[0] for r in self.conn.execute( | ||
| 1148 | + "SELECT node_id FROM work_node_execution WHERE run_id = 'run-shared'" | ||
| 1149 | + ).fetchall() | ||
| 1150 | + } | ||
| 1151 | + self.assertIn("verdict", node_ids) | ||
| 1152 | + | ||
| 1153 | + def test_second_call_appends_fresh_nodes_without_colliding(self): | ||
| 1154 | + # A real re-verify plans its OWN new node_ids each time (a fresh | ||
| 1155 | + # LLM planning call, not a replay) -- node_id collision is what | ||
| 1156 | + # plan_work_graph's append mode actively guards against (raises | ||
| 1157 | + # PlanningFailedError), so this uses a different node_id for the | ||
| 1158 | + # second call, same as a real second planning call naturally would. | ||
| 1159 | + second_manifest = { | ||
| 1160 | + "nodes": [{ | ||
| 1161 | + "node_id": "verdict_v2", "depends_on": [], "task_prompt": "give your final verdict", | ||
| 1162 | + "node_type": "final_verdict", | ||
| 1163 | + "judgment": {"kind": "mechanical", "check_ref": "verdict_says_pass"}, | ||
| 1164 | + "on_fail": {"action": "hard_fail"}, | ||
| 1165 | + }], | ||
| 1166 | + "edges": [], | ||
| 1167 | + } | ||
| 1168 | + self._run(_FAKE_VALIDATOR_MANIFEST_VERDICT_ONLY, "VERDICT: PASS\nlooks good") | ||
| 1169 | + second = self._run(second_manifest, "VERDICT: PASS\nfixed now") | ||
| 1078 | 1170 | ||
| 1079 | self.assertEqual(second["verdict"], "PASS") | 1171 | self.assertEqual(second["verdict"], "PASS") |
| 1080 | - rows = self.conn.execute( | ||
| 1081 | - "SELECT status FROM work_graph_run WHERE task_id = 'task-1' ORDER BY rowid" | ||
| 1082 | - ).fetchall() | ||
| 1083 | - self.assertEqual([r[0] for r in rows], ["done", "done"]) | ||
| 1084 | - | ||
| 1085 | - def test_earlier_nodes_survive_a_later_node_failing_verification(self): | ||
| 1086 | - # 2026-08-07 (same real incident/fix as PlanDocumentsTest's own | ||
| 1087 | - # test of this name): validator_test_script fails verify_fn twice | ||
| 1088 | - # -- ConsistencyCheckFailedError must still propagate, but | ||
| 1089 | - # validator_benchmark/validator_test_dataset (genuinely produced | ||
| 1090 | - # and accepted) must be persisted, not discarded along with it. | ||
| 1091 | - # validator_test_script itself is disabled by default (see | ||
| 1092 | - # _VALIDATOR_MANIFEST_WITH_TEST_SCRIPT above) -- re-enabled here to | ||
| 1093 | - # keep the salvage mechanism (generic, not test_script-specific) covered. | ||
| 1094 | - with patch.object(work_graph_execute, "write_workflow_script"), \ | ||
| 1095 | - patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | ||
| 1096 | - patch.object(e2e_pipeline, "_VALIDATOR_MANIFEST", _VALIDATOR_MANIFEST_WITH_TEST_SCRIPT): | ||
| 1097 | - mock_execute.side_effect = [ | ||
| 1098 | - {"content": "# Validator's own Benchmark"}, | ||
| 1099 | - {"content": "N/A: no dataset needed"}, | ||
| 1100 | - {"content": "this is prose, not a real script"}, # fails compile check | ||
| 1101 | - {"content": "still not real code either"}, # the one regeneration attempt: also fails | ||
| 1102 | - ] | ||
| 1103 | - with self.assertRaises(work_graph_orchestrate.ConsistencyCheckFailedError): | ||
| 1104 | - e2e_pipeline.validate_implementation_with_own_graph( | ||
| 1105 | - self.conn, "task-1", "sdd content", "prd content", "goal text", self.tmp.name, | ||
| 1106 | - ) | ||
| 1107 | - | ||
| 1108 | - rows = self.conn.execute("SELECT artifact_type FROM artifact WHERE task_id = 'task-1'").fetchall() | ||
| 1109 | - persisted = {r[0] for r in rows} | ||
| 1110 | - self.assertEqual(persisted, {"validator_benchmark", "validator_test_dataset"}) | ||
| 1111 | - self.assertNotIn("validator_test_script", persisted) | ||
| 1112 | - # the work_graph_run itself must NOT be left stuck 'running' either | ||
| 1113 | - # (ux_work_graph_run_active would block any later re-verify) -- | ||
| 1114 | - # _verify_and_maybe_regenerate's own 'blocked' write on this path | ||
| 1115 | - # already covers it; confirming it here catches a regression if | ||
| 1116 | - # that ever stops being true. | ||
| 1117 | - run_status = self.conn.execute( | ||
| 1118 | - "SELECT status FROM work_graph_run WHERE task_id = 'task-1'" | ||
| 1119 | - ).fetchone()[0] | ||
| 1120 | - self.assertEqual(run_status, "blocked") | 1172 | + node_ids = { |
| 1173 | + r[0] for r in self.conn.execute( | ||
| 1174 | + "SELECT node_id FROM work_node_execution WHERE run_id = 'run-shared'" | ||
| 1175 | + ).fetchall() | ||
| 1176 | + } | ||
| 1177 | + self.assertEqual(node_ids, {"verdict", "verdict_v2"}) | ||
| 1121 | 1178 | ||
| 1122 | 1179 | ||
| 1123 | if __name__ == "__main__": | 1180 | if __name__ == "__main__": |
src/test_task_execution.py
| @@ -16,7 +16,7 @@ from unittest.mock import patch | @@ -16,7 +16,7 @@ from unittest.mock import patch | ||
| 16 | 16 | ||
| 17 | from src import ( | 17 | from src import ( |
| 18 | e2e_pipeline, org_node_lifecycle, org_node_trigger, project_scaffold, schema, scheduler, | 18 | e2e_pipeline, org_node_lifecycle, org_node_trigger, project_scaffold, schema, scheduler, |
| 19 | - task_execution, work_graph_execute, work_graph_plan, | 19 | + task_execution, work_graph_execute, work_graph_orchestrate, work_graph_plan, |
| 20 | ) | 20 | ) |
| 21 | 21 | ||
| 22 | 22 | ||
| @@ -41,9 +41,45 @@ _FAKE_CODER_MANIFEST = { | @@ -41,9 +41,45 @@ _FAKE_CODER_MANIFEST = { | ||
| 41 | "edges": [], | 41 | "edges": [], |
| 42 | } | 42 | } |
| 43 | 43 | ||
| 44 | +# Phase 2c (2026-08-12): Validator now self-plans too (validate_ | ||
| 45 | +# implementation_with_own_graph() calls plan_work_graph(conn, "validator", | ||
| 46 | +# ..., append_to_run_id=...), through the SAME mocked subprocess boundary | ||
| 47 | +# as Planner's/Coder's own planning calls) -- a single node_type= | ||
| 48 | +# 'final_verdict' node is the minimal real shape this mechanism accepts | ||
| 49 | +# (see work_graph_plan._VALIDATOR_NODE_CATALOG's own catalog entry). | ||
| 50 | +# depends_on ["sdd"] for the same "avoid landing in the same in-degree-zero | ||
| 51 | +# topological layer as an already-'done' node" reason _FAKE_CODER_MANIFEST | ||
| 52 | +# above documents. | ||
| 53 | +_FAKE_VALIDATOR_MANIFEST = { | ||
| 54 | + "nodes": [{ | ||
| 55 | + "node_id": "verdict", "depends_on": ["sdd"], "task_prompt": "give your final verdict", | ||
| 56 | + "node_type": "final_verdict", | ||
| 57 | + "judgment": {"kind": "mechanical", "check_ref": "verdict_says_pass"}, | ||
| 58 | + "on_fail": {"action": "dynamic_loop_back", "max_attempts": 3}, | ||
| 59 | + }], | ||
| 60 | + "edges": [], | ||
| 61 | +} | ||
| 44 | 62 | ||
| 45 | -def _fake_coder_plan_subprocess_run(*args, **kwargs): | ||
| 46 | - payload = {"is_error": False, "result": "```json\n" + json.dumps(_FAKE_CODER_MANIFEST) + "\n```"} | 63 | +_real_subprocess_run = subprocess.run # captured before any test patches subprocess.run globally |
| 64 | + | ||
| 65 | + | ||
| 66 | +def _fake_plan_subprocess_run_dispatch(*args, **kwargs): | ||
| 67 | + """Real plan_work_graph() dispatches by the --agent argv value -- this | ||
| 68 | + mock does the same, so Planner's/Coder's/Validator's planning calls | ||
| 69 | + (which all now share the same mocked subprocess.run patch target) each | ||
| 70 | + get their own realistic fake manifest instead of one hardcoded response | ||
| 71 | + serving all three callers. Same dispatch-by-argv + real-passthrough-for- | ||
| 72 | + non-'claude'-calls pattern as test_e2e_pipeline.py's helper of the same | ||
| 73 | + name -- see that module's docstring for the full rationale (this file's | ||
| 74 | + own git add/commit/diff calls inside the `with patch(...)` blocks below | ||
| 75 | + would otherwise also get intercepted, since `subprocess` is one shared | ||
| 76 | + module object).""" | ||
| 77 | + argv = args[0] | ||
| 78 | + if not argv or argv[0] != "claude": | ||
| 79 | + return _real_subprocess_run(*args, **kwargs) | ||
| 80 | + org_node = argv[argv.index("--agent") + 1] | ||
| 81 | + manifest = _FAKE_VALIDATOR_MANIFEST if org_node == "validator" else _FAKE_CODER_MANIFEST | ||
| 82 | + payload = {"is_error": False, "result": "```json\n" + json.dumps(manifest) + "\n```"} | ||
| 47 | return subprocess.CompletedProcess(args=["claude"], returncode=0, stdout=json.dumps(payload), stderr="") | 83 | return subprocess.CompletedProcess(args=["claude"], returncode=0, stdout=json.dumps(payload), stderr="") |
| 48 | 84 | ||
| 49 | 85 | ||
| @@ -95,6 +131,24 @@ class TaskExecutionTestBase(unittest.TestCase): | @@ -95,6 +131,24 @@ class TaskExecutionTestBase(unittest.TestCase): | ||
| 95 | "INSERT OR IGNORE INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " | 131 | "INSERT OR IGNORE INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " |
| 96 | "VALUES (?, ?, 'planner', ?, 'done')", (run_id, task_id, manifest), | 132 | "VALUES (?, ?, 'planner', ?, 'done')", (run_id, task_id, manifest), |
| 97 | ) | 133 | ) |
| 134 | + # Phase 2c (2026-08-12, found while adding Validator self-planning | ||
| 135 | + # tests): a real Planner run_graph() call ALSO leaves a real | ||
| 136 | + # work_node_execution row behind for this node -- without one here | ||
| 137 | + # too, a later self-planning append onto this SAME run_id (Coder's/ | ||
| 138 | + # Validator's own plan_work_graph(..., append_to_run_id=run_id)) | ||
| 139 | + # finds no existing (run_id, node_id) row for `artifact_type` and | ||
| 140 | + # spuriously RE-EXECUTES it for real via the mocked execute_work_ | ||
| 141 | + # graph, burning an extra mock_execute.side_effect slot the calling | ||
| 142 | + # test never accounted for (masked before this fix only because | ||
| 143 | + # those tests happened to provide enough spare list items and never | ||
| 144 | + # asserted on the exact content that ended up threaded through). | ||
| 145 | + work_graph_orchestrate.ensure_placeholder_skill(self.conn, "planner-doc-writing", "planner") | ||
| 146 | + self.conn.execute( | ||
| 147 | + "INSERT OR IGNORE INTO work_node_execution (node_exec_id, run_id, node_id, subagent_type, " | ||
| 148 | + "skill_ref, input_schema_ref, output_schema_ref, input_payload, output_payload, status) " | ||
| 149 | + "VALUES (?, ?, ?, 'planner', 'planner-doc-writing', 'in.json', 'out.json', '{}', ?, 'done')", | ||
| 150 | + ("node-exec-" + run_id, run_id, artifact_type, json.dumps({"content": content})), | ||
| 151 | + ) | ||
| 98 | rel_path = ".graphlemon/runs/{}/{}.md".format(task_id, artifact_type) | 152 | rel_path = ".graphlemon/runs/{}/{}.md".format(task_id, artifact_type) |
| 99 | abs_path = os.path.join(self.tmp.name, rel_path) | 153 | abs_path = os.path.join(self.tmp.name, rel_path) |
| 100 | os.makedirs(os.path.dirname(abs_path), exist_ok=True) | 154 | os.makedirs(os.path.dirname(abs_path), exist_ok=True) |
| @@ -130,22 +184,17 @@ class ResumeFromCodingTest(TaskExecutionTestBase): | @@ -130,22 +184,17 @@ class ResumeFromCodingTest(TaskExecutionTestBase): | ||
| 130 | self._write_artifact("task-1", "test_report", "report content") | 184 | self._write_artifact("task-1", "test_report", "report content") |
| 131 | 185 | ||
| 132 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ | 186 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ |
| 187 | + patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_plan_subprocess_run_dispatch), \ | ||
| 133 | patch.object(work_graph_execute, "write_workflow_script"), \ | 188 | patch.object(work_graph_execute, "write_workflow_script"), \ |
| 134 | - patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | ||
| 135 | - patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS") as mock_trigger: | ||
| 136 | - # validator's OWN benchmark/test_dataset/test_script graph (2026-08-06) | ||
| 137 | - mock_execute.side_effect = [ | ||
| 138 | - {"content": "# Validator's own Benchmark"}, | ||
| 139 | - {"content": "N/A: no dataset needed"}, | ||
| 140 | - {"content": "import sys\nsys.exit(0)\n"}, | ||
| 141 | - ] | 189 | + patch.object(work_graph_execute, "execute_work_graph") as mock_execute: |
| 190 | + # validator's self-planned final_verdict node (Phase 2c) | ||
| 191 | + mock_execute.side_effect = [{"content": "VERDICT: PASS\nlooks good"}] | ||
| 142 | result = task_execution.run_or_resume_task_lifecycle(self.conn, "task-1", self.tmp.name, "a goal") | 192 | result = task_execution.run_or_resume_task_lifecycle(self.conn, "task-1", self.tmp.name, "a goal") |
| 143 | 193 | ||
| 144 | self.assertEqual(result["verdict"]["verdict"], "PASS") | 194 | self.assertEqual(result["verdict"]["verdict"], "PASS") |
| 145 | self.assertEqual(result["code_artifact_ids"]["code_diff"], "art-code_diff") | 195 | self.assertEqual(result["code_artifact_ids"]["code_diff"], "art-code_diff") |
| 146 | - # only the validator's final-verdict real call happened -- no coder call | ||
| 147 | - mock_trigger.assert_called_once() | ||
| 148 | - self.assertEqual(mock_trigger.call_args[0][0], "validator") | 196 | + # only the validator's self-planned node ran -- no coder call |
| 197 | + mock_execute.assert_called_once() | ||
| 149 | row = self.conn.execute("SELECT status FROM task WHERE task_id = 'task-1'").fetchone() | 198 | row = self.conn.execute("SELECT status FROM task WHERE task_id = 'task-1'").fetchone() |
| 150 | self.assertEqual(row[0], "validating") | 199 | self.assertEqual(row[0], "validating") |
| 151 | 200 | ||
| @@ -168,17 +217,14 @@ class ResumeFromPlanningTest(TaskExecutionTestBase): | @@ -168,17 +217,14 @@ class ResumeFromPlanningTest(TaskExecutionTestBase): | ||
| 168 | self._write_artifact("task-1", "benchmark", "# Benchmark content") | 217 | self._write_artifact("task-1", "benchmark", "# Benchmark content") |
| 169 | 218 | ||
| 170 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ | 219 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ |
| 171 | - patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_coder_plan_subprocess_run), \ | 220 | + patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_plan_subprocess_run_dispatch), \ |
| 172 | patch.object(work_graph_execute, "write_workflow_script"), \ | 221 | patch.object(work_graph_execute, "write_workflow_script"), \ |
| 173 | patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | 222 | patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ |
| 174 | patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS") as mock_trigger, \ | 223 | patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS") as mock_trigger, \ |
| 175 | patch.object(e2e_pipeline, "plan_documents") as mock_plan: | 224 | patch.object(e2e_pipeline, "plan_documents") as mock_plan: |
| 176 | mock_execute.side_effect = [ | 225 | mock_execute.side_effect = [ |
| 177 | {"content": "done, tests pass"}, # coder's self-planned "impl" node | 226 | {"content": "done, tests pass"}, # coder's self-planned "impl" node |
| 178 | - # validator's OWN benchmark/test_dataset/test_script graph (2026-08-06) | ||
| 179 | - {"content": "# Validator's own Benchmark"}, | ||
| 180 | - {"content": "N/A: no dataset needed"}, | ||
| 181 | - {"content": "import sys\nsys.exit(0)\n"}, | 227 | + {"content": "VERDICT: PASS\nlooks good"}, # validator's self-planned final_verdict node (Phase 2c) |
| 182 | ] | 228 | ] |
| 183 | result = task_execution.run_or_resume_task_lifecycle(self.conn, "task-1", self.tmp.name, "a goal") | 229 | result = task_execution.run_or_resume_task_lifecycle(self.conn, "task-1", self.tmp.name, "a goal") |
| 184 | 230 | ||
| @@ -187,11 +233,11 @@ class ResumeFromPlanningTest(TaskExecutionTestBase): | @@ -187,11 +233,11 @@ class ResumeFromPlanningTest(TaskExecutionTestBase): | ||
| 187 | # existing artifacts were reused -- plan_documents() must NOT have been | 233 | # existing artifacts were reused -- plan_documents() must NOT have been |
| 188 | # called again (that's the whole point of the existence-probing retry loop). | 234 | # called again (that's the whole point of the existence-probing retry loop). |
| 189 | mock_plan.assert_not_called() | 235 | mock_plan.assert_not_called() |
| 190 | - # two real org_node_trigger.trigger_org_node calls: planner_reviewer's | ||
| 191 | - # judgment, then validator's final verdict -- not just validator anymore. | ||
| 192 | - self.assertEqual(mock_trigger.call_count, 2) | 236 | + # one real org_node_trigger.trigger_org_node call: planner_reviewer's |
| 237 | + # judgment -- Validator's own verdict is now a self-planned node (Phase | ||
| 238 | + # 2c), read via mock_execute above, not a flat org_node_trigger call. | ||
| 239 | + mock_trigger.assert_called_once() | ||
| 193 | self.assertEqual(mock_trigger.call_args_list[0][0][0], "planner_reviewer") | 240 | self.assertEqual(mock_trigger.call_args_list[0][0][0], "planner_reviewer") |
| 194 | - self.assertEqual(mock_trigger.call_args_list[1][0][0], "validator") | ||
| 195 | 241 | ||
| 196 | 242 | ||
| 197 | class ResumeFromReviewingTest(TaskExecutionTestBase): | 243 | class ResumeFromReviewingTest(TaskExecutionTestBase): |
| @@ -216,16 +262,14 @@ class ResumeFromReviewingTest(TaskExecutionTestBase): | @@ -216,16 +262,14 @@ class ResumeFromReviewingTest(TaskExecutionTestBase): | ||
| 216 | return _fake_ready_trigger(org_node, prompt, target_folder_path, timeout) | 262 | return _fake_ready_trigger(org_node, prompt, target_folder_path, timeout) |
| 217 | 263 | ||
| 218 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=tracking_ready_trigger), \ | 264 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=tracking_ready_trigger), \ |
| 219 | - patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_coder_plan_subprocess_run), \ | 265 | + patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_plan_subprocess_run_dispatch), \ |
| 220 | patch.object(work_graph_execute, "write_workflow_script"), \ | 266 | patch.object(work_graph_execute, "write_workflow_script"), \ |
| 221 | patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | 267 | patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ |
| 222 | patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS") as mock_trigger, \ | 268 | patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS") as mock_trigger, \ |
| 223 | patch.object(e2e_pipeline, "plan_documents") as mock_plan: | 269 | patch.object(e2e_pipeline, "plan_documents") as mock_plan: |
| 224 | mock_execute.side_effect = [ | 270 | mock_execute.side_effect = [ |
| 225 | {"content": "done, tests pass"}, # coder's self-planned "impl" node | 271 | {"content": "done, tests pass"}, # coder's self-planned "impl" node |
| 226 | - {"content": "# Validator's own Benchmark"}, | ||
| 227 | - {"content": "N/A: no dataset needed"}, | ||
| 228 | - {"content": "import sys\nsys.exit(0)\n"}, | 272 | + {"content": "VERDICT: PASS\nlooks good"}, # validator's self-planned final_verdict node (Phase 2c) |
| 229 | ] | 273 | ] |
| 230 | result = task_execution.run_or_resume_task_lifecycle(self.conn, "task-1", self.tmp.name, "a goal") | 274 | result = task_execution.run_or_resume_task_lifecycle(self.conn, "task-1", self.tmp.name, "a goal") |
| 231 | 275 | ||
| @@ -236,9 +280,11 @@ class ResumeFromReviewingTest(TaskExecutionTestBase): | @@ -236,9 +280,11 @@ class ResumeFromReviewingTest(TaskExecutionTestBase): | ||
| 236 | # this task into 'reviewing_planner_output' in the first place). | 280 | # this task into 'reviewing_planner_output' in the first place). |
| 237 | self.assertEqual(ready_calls, ["coder", "validator"]) | 281 | self.assertEqual(ready_calls, ["coder", "validator"]) |
| 238 | # but the real review judgment call DOES still run (that's the actual | 282 | # but the real review judgment call DOES still run (that's the actual |
| 239 | - # gap this status resumes into -- see the module docstring). | 283 | + # gap this status resumes into -- see the module docstring). Validator's |
| 284 | + # own verdict is now a self-planned node (Phase 2c), read via | ||
| 285 | + # mock_execute above, not a second org_node_trigger call. | ||
| 286 | + mock_trigger.assert_called_once() | ||
| 240 | self.assertEqual(mock_trigger.call_args_list[0][0][0], "planner_reviewer") | 287 | self.assertEqual(mock_trigger.call_args_list[0][0][0], "planner_reviewer") |
| 241 | - self.assertEqual(mock_trigger.call_args_list[1][0][0], "validator") | ||
| 242 | 288 | ||
| 243 | 289 | ||
| 244 | class ResumeFromPlanningWithZeroArtifactsTest(TaskExecutionTestBase): | 290 | class ResumeFromPlanningWithZeroArtifactsTest(TaskExecutionTestBase): |
| @@ -266,16 +312,14 @@ class ResumeFromPlanningWithZeroArtifactsTest(TaskExecutionTestBase): | @@ -266,16 +312,14 @@ class ResumeFromPlanningWithZeroArtifactsTest(TaskExecutionTestBase): | ||
| 266 | return {"run_id": "run-sdd", "artifact_ids": {"sdd": "art-sdd"}} | 312 | return {"run_id": "run-sdd", "artifact_ids": {"sdd": "art-sdd"}} |
| 267 | 313 | ||
| 268 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ | 314 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ |
| 269 | - patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_coder_plan_subprocess_run), \ | 315 | + patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_plan_subprocess_run_dispatch), \ |
| 270 | patch.object(work_graph_execute, "write_workflow_script"), \ | 316 | patch.object(work_graph_execute, "write_workflow_script"), \ |
| 271 | patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | 317 | patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ |
| 272 | patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS"), \ | 318 | patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS"), \ |
| 273 | patch.object(e2e_pipeline, "plan_documents", side_effect=fake_plan_documents) as mock_plan: | 319 | patch.object(e2e_pipeline, "plan_documents", side_effect=fake_plan_documents) as mock_plan: |
| 274 | mock_execute.side_effect = [ | 320 | mock_execute.side_effect = [ |
| 275 | {"content": "done, tests pass"}, # coder's self-planned "impl" node | 321 | {"content": "done, tests pass"}, # coder's self-planned "impl" node |
| 276 | - {"content": "# Validator's own Benchmark"}, | ||
| 277 | - {"content": "N/A: no dataset needed"}, | ||
| 278 | - {"content": "import sys\nsys.exit(0)\n"}, | 322 | + {"content": "VERDICT: PASS\nlooks good"}, # validator's self-planned final_verdict node (Phase 2c) |
| 279 | ] | 323 | ] |
| 280 | # must not raise (this is the actual regression: it used to KeyError/ | 324 | # must not raise (this is the actual regression: it used to KeyError/ |
| 281 | # TaskExecutionError before ever reaching here) | 325 | # TaskExecutionError before ever reaching here) |
| @@ -304,14 +348,10 @@ class ResumeFromValidatingTest(TaskExecutionTestBase): | @@ -304,14 +348,10 @@ class ResumeFromValidatingTest(TaskExecutionTestBase): | ||
| 304 | self._write_artifact("task-1", "test_report", "report content") | 348 | self._write_artifact("task-1", "test_report", "report content") |
| 305 | 349 | ||
| 306 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger) as mock_ready, \ | 350 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger) as mock_ready, \ |
| 351 | + patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_plan_subprocess_run_dispatch), \ | ||
| 307 | patch.object(work_graph_execute, "write_workflow_script"), \ | 352 | patch.object(work_graph_execute, "write_workflow_script"), \ |
| 308 | - patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | ||
| 309 | - patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS") as mock_trigger: | ||
| 310 | - mock_execute.side_effect = [ | ||
| 311 | - {"content": "# Validator's own Benchmark"}, | ||
| 312 | - {"content": "N/A: no dataset needed"}, | ||
| 313 | - {"content": "import sys\nsys.exit(0)\n"}, | ||
| 314 | - ] | 353 | + patch.object(work_graph_execute, "execute_work_graph") as mock_execute: |
| 354 | + mock_execute.side_effect = [{"content": "VERDICT: PASS\nlooks good"}] # validator's self-planned final_verdict node | ||
| 315 | result = task_execution.run_or_resume_task_lifecycle(self.conn, "task-1", self.tmp.name, "a goal") | 355 | result = task_execution.run_or_resume_task_lifecycle(self.conn, "task-1", self.tmp.name, "a goal") |
| 316 | 356 | ||
| 317 | self.assertEqual(result["verdict"]["verdict"], "PASS") | 357 | self.assertEqual(result["verdict"]["verdict"], "PASS") |
| @@ -321,14 +361,19 @@ class ResumeFromValidatingTest(TaskExecutionTestBase): | @@ -321,14 +361,19 @@ class ResumeFromValidatingTest(TaskExecutionTestBase): | ||
| 321 | # get the task into 'validating' in the first place, and calling it | 361 | # get the task into 'validating' in the first place, and calling it |
| 322 | # again would hit org_node_lifecycle's own "needs status=coding" guard. | 362 | # again would hit org_node_lifecycle's own "needs status=coding" guard. |
| 323 | mock_ready.assert_not_called() | 363 | mock_ready.assert_not_called() |
| 324 | - # only the real work_graph_run's own final call happened | ||
| 325 | - mock_trigger.assert_called_once() | ||
| 326 | - self.assertEqual(mock_trigger.call_args[0][0], "validator") | ||
| 327 | - # a brand-new validator work_graph_run was created (none existed before) | ||
| 328 | - rows = self.conn.execute( | ||
| 329 | - "SELECT run_id FROM work_graph_run WHERE task_id = 'task-1' AND org_node = 'validator'" | ||
| 330 | - ).fetchall() | ||
| 331 | - self.assertEqual(len(rows), 1) | 364 | + # only the validator's self-planned node ran |
| 365 | + mock_execute.assert_called_once() | ||
| 366 | + # Phase 2c: Validator appends its final_verdict node into the SAME | ||
| 367 | + # shared run the sdd artifact points at (via _shared_run_id_for_task), | ||
| 368 | + # not a brand-new work_graph_run row of its own -- confirm the merge | ||
| 369 | + # landed on "run-sdd" and no extra row was created for this task. | ||
| 370 | + row = self.conn.execute("SELECT graph_manifest FROM work_graph_run WHERE run_id = 'run-sdd'").fetchone() | ||
| 371 | + node_ids = {n["node_id"] for n in json.loads(row[0])["nodes"]} | ||
| 372 | + self.assertIn("verdict", node_ids) | ||
| 373 | + total_rows = self.conn.execute( | ||
| 374 | + "SELECT COUNT(*) FROM work_graph_run WHERE task_id = 'task-1'" | ||
| 375 | + ).fetchone()[0] | ||
| 376 | + self.assertEqual(total_rows, 5) | ||
| 332 | 377 | ||
| 333 | 378 | ||
| 334 | class ResumeUnresumableStatusTest(TaskExecutionTestBase): | 379 | class ResumeUnresumableStatusTest(TaskExecutionTestBase): |
| @@ -473,9 +518,24 @@ class StartTaskExecutionInBackgroundTest(unittest.TestCase): | @@ -473,9 +518,24 @@ class StartTaskExecutionInBackgroundTest(unittest.TestCase): | ||
| 473 | def _write_artifact(self, task_id, artifact_type, content): | 518 | def _write_artifact(self, task_id, artifact_type, content): |
| 474 | conn = schema.connect(self.db_path) | 519 | conn = schema.connect(self.db_path) |
| 475 | run_id = "run-" + artifact_type | 520 | run_id = "run-" + artifact_type |
| 521 | + # Real "nodes"/"edges" shape + a matching 'done' work_node_execution | ||
| 522 | + # row -- see TaskExecutionTestBase._write_artifact's own comment | ||
| 523 | + # (same file) for why both are needed once Validator self-plans and | ||
| 524 | + # appends onto this run via plan_work_graph(..., append_to_run_id=...). | ||
| 525 | + manifest = json.dumps({ | ||
| 526 | + "nodes": [{"node_id": artifact_type, "artifact_type": artifact_type, "depends_on": []}], | ||
| 527 | + "edges": [], | ||
| 528 | + }) | ||
| 476 | conn.execute( | 529 | conn.execute( |
| 477 | "INSERT OR IGNORE INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " | 530 | "INSERT OR IGNORE INTO work_graph_run (run_id, task_id, org_node, graph_manifest, status) " |
| 478 | - "VALUES (?, ?, 'planner', '{}', 'done')", (run_id, task_id), | 531 | + "VALUES (?, ?, 'planner', ?, 'done')", (run_id, task_id, manifest), |
| 532 | + ) | ||
| 533 | + work_graph_orchestrate.ensure_placeholder_skill(conn, "planner-doc-writing", "planner") | ||
| 534 | + conn.execute( | ||
| 535 | + "INSERT OR IGNORE INTO work_node_execution (node_exec_id, run_id, node_id, subagent_type, " | ||
| 536 | + "skill_ref, input_schema_ref, output_schema_ref, input_payload, output_payload, status) " | ||
| 537 | + "VALUES (?, ?, ?, 'planner', 'planner-doc-writing', 'in.json', 'out.json', '{}', ?, 'done')", | ||
| 538 | + ("node-exec-" + run_id, run_id, artifact_type, json.dumps({"content": content})), | ||
| 479 | ) | 539 | ) |
| 480 | rel_path = ".graphlemon/runs/{}/{}.md".format(task_id, artifact_type) | 540 | rel_path = ".graphlemon/runs/{}/{}.md".format(task_id, artifact_type) |
| 481 | abs_path = os.path.join(self.repo.name, rel_path) | 541 | abs_path = os.path.join(self.repo.name, rel_path) |
| @@ -506,14 +566,10 @@ class StartTaskExecutionInBackgroundTest(unittest.TestCase): | @@ -506,14 +566,10 @@ class StartTaskExecutionInBackgroundTest(unittest.TestCase): | ||
| 506 | self._write_artifact("task-1", "test_report", "report") | 566 | self._write_artifact("task-1", "test_report", "report") |
| 507 | 567 | ||
| 508 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ | 568 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ |
| 569 | + patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_plan_subprocess_run_dispatch), \ | ||
| 509 | patch.object(work_graph_execute, "write_workflow_script"), \ | 570 | patch.object(work_graph_execute, "write_workflow_script"), \ |
| 510 | - patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | ||
| 511 | - patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS"): | ||
| 512 | - mock_execute.side_effect = [ | ||
| 513 | - {"content": "# Validator's own Benchmark"}, | ||
| 514 | - {"content": "N/A: no dataset needed"}, | ||
| 515 | - {"content": "import sys\nsys.exit(0)\n"}, | ||
| 516 | - ] | 571 | + patch.object(work_graph_execute, "execute_work_graph") as mock_execute: |
| 572 | + mock_execute.side_effect = [{"content": "VERDICT: PASS\nlooks good"}] # validator's self-planned final_verdict node | ||
| 517 | thread = task_execution.start_task_execution_in_background( | 573 | thread = task_execution.start_task_execution_in_background( |
| 518 | self.db_path, "task-1", self.repo.name, "a goal", | 574 | self.db_path, "task-1", self.repo.name, "a goal", |
| 519 | ) | 575 | ) |
| @@ -561,14 +617,10 @@ class StartTaskExecutionInBackgroundTest(unittest.TestCase): | @@ -561,14 +617,10 @@ class StartTaskExecutionInBackgroundTest(unittest.TestCase): | ||
| 561 | return _fake_ready_trigger(org_node, prompt, target_folder_path, timeout) | 617 | return _fake_ready_trigger(org_node, prompt, target_folder_path, timeout) |
| 562 | 618 | ||
| 563 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=blocking_trigger), \ | 619 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=blocking_trigger), \ |
| 620 | + patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_plan_subprocess_run_dispatch), \ | ||
| 564 | patch.object(work_graph_execute, "write_workflow_script"), \ | 621 | patch.object(work_graph_execute, "write_workflow_script"), \ |
| 565 | - patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | ||
| 566 | - patch.object(org_node_trigger, "trigger_org_node", return_value="VERDICT: PASS"): | ||
| 567 | - mock_execute.side_effect = [ | ||
| 568 | - {"content": "# Validator's own Benchmark"}, | ||
| 569 | - {"content": "N/A: no dataset needed"}, | ||
| 570 | - {"content": "import sys\nsys.exit(0)\n"}, | ||
| 571 | - ] | 622 | + patch.object(work_graph_execute, "execute_work_graph") as mock_execute: |
| 623 | + mock_execute.side_effect = [{"content": "VERDICT: PASS\nlooks good"}] # validator's self-planned final_verdict node | ||
| 572 | thread = task_execution.start_task_execution_in_background( | 624 | thread = task_execution.start_task_execution_in_background( |
| 573 | self.db_path, "task-1", self.repo.name, "a goal", | 625 | self.db_path, "task-1", self.repo.name, "a goal", |
| 574 | ) | 626 | ) |
| @@ -591,17 +643,14 @@ class StartTaskExecutionInBackgroundTest(unittest.TestCase): | @@ -591,17 +643,14 @@ class StartTaskExecutionInBackgroundTest(unittest.TestCase): | ||
| 591 | self._write_artifact("task-1", "test_report", "report") | 643 | self._write_artifact("task-1", "test_report", "report") |
| 592 | 644 | ||
| 593 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ | 645 | with patch.object(org_node_lifecycle, "trigger_org_node", side_effect=_fake_ready_trigger), \ |
| 646 | + patch.object(work_graph_plan.subprocess, "run", side_effect=_fake_plan_subprocess_run_dispatch), \ | ||
| 594 | patch.object(work_graph_execute, "write_workflow_script"), \ | 647 | patch.object(work_graph_execute, "write_workflow_script"), \ |
| 595 | - patch.object(work_graph_execute, "execute_work_graph") as mock_execute, \ | ||
| 596 | - patch.object(org_node_trigger, "trigger_org_node", side_effect=RuntimeError("boom")): | ||
| 597 | - # validator's own benchmark/dataset/script graph completes fine -- | ||
| 598 | - # the failure this test cares about is the FINAL verdict call | ||
| 599 | - # (org_node_trigger.trigger_org_node), which happens after. | ||
| 600 | - mock_execute.side_effect = [ | ||
| 601 | - {"content": "# Validator's own Benchmark"}, | ||
| 602 | - {"content": "N/A: no dataset needed"}, | ||
| 603 | - {"content": "import sys\nsys.exit(0)\n"}, | ||
| 604 | - ] | 648 | + patch.object(work_graph_execute, "execute_work_graph", side_effect=RuntimeError("boom")): |
| 649 | + # Phase 2c: Validator's own verdict is now a self-planned node, | ||
| 650 | + # not a flat org_node_trigger.trigger_org_node call -- the | ||
| 651 | + # failure this test cares about (a real exception surfacing as | ||
| 652 | + # blocked_pending_human/execution_error) is injected at its own | ||
| 653 | + # node's real execution instead. | ||
| 605 | thread = task_execution.start_task_execution_in_background( | 654 | thread = task_execution.start_task_execution_in_background( |
| 606 | self.db_path, "task-1", self.repo.name, "a goal", | 655 | self.db_path, "task-1", self.repo.name, "a goal", |
| 607 | ) | 656 | ) |
src/work_graph_plan.py
| @@ -81,6 +81,12 @@ GRAPH_MANIFEST_SCHEMA = { | @@ -81,6 +81,12 @@ GRAPH_MANIFEST_SCHEMA = { | ||
| 81 | }, | 81 | }, |
| 82 | "subagent_type": {"type": "string"}, | 82 | "subagent_type": {"type": "string"}, |
| 83 | "skill_ref": {"type": "string"}, | 83 | "skill_ref": {"type": "string"}, |
| 84 | + # 2026-08-12 (Phase 2c): echoed-back catalog label, e.g. | ||
| 85 | + # "test_script"/"final_verdict" -- see _generic_prompt_ | ||
| 86 | + # template's own comment for why this exists (finding | ||
| 87 | + # role-specific nodes among arbitrarily-named | ||
| 88 | + # self-planned ones). | ||
| 89 | + "node_type": {"type": "string"}, | ||
| 84 | }, | 90 | }, |
| 85 | }, | 91 | }, |
| 86 | }, | 92 | }, |
| @@ -232,9 +238,22 @@ _VALIDATOR_NODE_CATALOG = [ | @@ -232,9 +238,22 @@ _VALIDATOR_NODE_CATALOG = [ | ||
| 232 | {"node_type": "test_dataset", "description": "Concrete test data, if this goal has a natural one."}, | 238 | {"node_type": "test_dataset", "description": "Concrete test data, if this goal has a natural one."}, |
| 233 | { | 239 | { |
| 234 | "node_type": "test_script", | 240 | "node_type": "test_script", |
| 235 | - "description": "A real, runnable script mechanically checking the independent benchmark's scenarios.", | 241 | + "description": "A real, runnable script mechanically checking the independent benchmark's scenarios. " |
| 242 | + "It will actually be EXECUTED against the real repository after you plan it -- write " | ||
| 243 | + "real commands, not a description of what a script would do.", | ||
| 236 | "suggested_judgment": {"kind": "mechanical", "check_ref": "check_script_compiles"}, | 244 | "suggested_judgment": {"kind": "mechanical", "check_ref": "check_script_compiles"}, |
| 237 | }, | 245 | }, |
| 246 | + { | ||
| 247 | + "node_type": "final_verdict", | ||
| 248 | + "description": "Exactly ONE node of this type, depending on every other node whose content your " | ||
| 249 | + "verdict needs. Its content must be your final judgment: a line 'VERDICT: PASS', " | ||
| 250 | + "'VERDICT: FAIL', or 'VERDICT: INCONCLUSIVE', followed by your reasoning (and, if not " | ||
| 251 | + "PASS, a root_cause_category of planning|coder|skill_defect with detail). This is the " | ||
| 252 | + "ONLY node in a Validator graph allowed to use on_fail.action='dynamic_loop_back' -- " | ||
| 253 | + "see the on_fail instructions below for why.", | ||
| 254 | + "suggested_judgment": {"kind": "mechanical", "check_ref": "verdict_says_pass"}, | ||
| 255 | + "suggested_on_fail": {"action": "dynamic_loop_back", "max_attempts": 3}, | ||
| 256 | + }, | ||
| 238 | ] | 257 | ] |
| 239 | 258 | ||
| 240 | _GENERIC_NODE_CATALOGS = {"coder": _CODER_NODE_CATALOG, "validator": _VALIDATOR_NODE_CATALOG} | 259 | _GENERIC_NODE_CATALOGS = {"coder": _CODER_NODE_CATALOG, "validator": _VALIDATOR_NODE_CATALOG} |
| @@ -245,15 +264,37 @@ def _generic_prompt_template(org_node): | @@ -245,15 +264,37 @@ def _generic_prompt_template(org_node): | ||
| 245 | org_node so each gets its own suggested catalog (or none, for any | 264 | org_node so each gets its own suggested catalog (or none, for any |
| 246 | future org_node this hasn't been extended for yet -- catalog_section | 265 | future org_node this hasn't been extended for yet -- catalog_section |
| 247 | is simply empty then, same as before this change existed). | 266 | is simply empty then, same as before this change existed). |
| 248 | - node_type is a label in the SUGGESTION data only, not a required | ||
| 249 | - graph_manifest node field -- node_id/task_prompt stay exactly as | ||
| 250 | - freeform as before.""" | 267 | + |
| 268 | + 2026-08-12 (Phase 2c): `node_type` is now asked to be ECHOED BACK onto | ||
| 269 | + each planned node (not just a label in the suggestion data) -- e2e_ | ||
| 270 | + pipeline.py's Validator rework needs to find "the test_script node"/ | ||
| 271 | + "the final_verdict node" among self-planned, arbitrarily-named nodes, | ||
| 272 | + and node_type is the one signal that survives self-planning for this | ||
| 273 | + (node_id is freeform, task_prompt is prose). `dynamic_loop_back` is | ||
| 274 | + now allowed, but ONLY for org_node='validator' AND only on a node | ||
| 275 | + that also declares node_type='final_verdict' -- this reverses the | ||
| 276 | + Phase 2a/2b instruction ("reserved for the terminal review stage | ||
| 277 | + outside this graph, not for nodes you plan yourself"), because | ||
| 278 | + Validator's terminal review IS now planned inside this graph, not a | ||
| 279 | + flat call after it (see _VALIDATOR_NODE_CATALOG's own 'final_verdict' | ||
| 280 | + entry for why: only a real graph node can use the loop_back mechanism | ||
| 281 | + _run_one_node already implements). Every OTHER org_node still can't | ||
| 282 | + use it -- Coder's catalog has no comparable terminal-judgment role.""" | ||
| 251 | catalog = _GENERIC_NODE_CATALOGS.get(org_node, []) | 283 | catalog = _GENERIC_NODE_CATALOGS.get(org_node, []) |
| 252 | catalog_section = ( | 284 | catalog_section = ( |
| 253 | "Suggested node types for this goal (use any subset, in any order, invent your own if " | 285 | "Suggested node types for this goal (use any subset, in any order, invent your own if " |
| 254 | "none fit -- these are starting points, not requirements):\n" + json.dumps(catalog) + "\n" | 286 | "none fit -- these are starting points, not requirements):\n" + json.dumps(catalog) + "\n" |
| 255 | if catalog else "" | 287 | if catalog else "" |
| 256 | ) | 288 | ) |
| 289 | + dynamic_loop_back_rule = ( | ||
| 290 | + "{\"action\": \"dynamic_loop_back\"} ONLY for the node also declaring " | ||
| 291 | + "node_type='final_verdict' (see the final_verdict catalog entry above) -- no on_failure " | ||
| 292 | + "edge needed for this one, the target is chosen dynamically at failure time, not declared " | ||
| 293 | + "now.\n" | ||
| 294 | + if org_node == "validator" else | ||
| 295 | + "do NOT use \"dynamic_loop_back\" -- that's reserved for Validator's terminal review node, " | ||
| 296 | + "not for nodes you plan yourself.\n" | ||
| 297 | + ) | ||
| 257 | return ( | 298 | return ( |
| 258 | "Plan (do not execute) a Work Graph for this goal: {goal_text}\n" | 299 | "Plan (do not execute) a Work Graph for this goal: {goal_text}\n" |
| 259 | + catalog_section + | 300 | + catalog_section + |
| @@ -267,6 +308,9 @@ def _generic_prompt_template(org_node): | @@ -267,6 +308,9 @@ def _generic_prompt_template(org_node): | ||
| 267 | "depends_on (list of other node_ids), and task_prompt (the FULL, self-contained prompt " | 308 | "depends_on (list of other node_ids), and task_prompt (the FULL, self-contained prompt " |
| 268 | "for that node's own execution -- assume the agent running it sees nothing except this " | 309 | "for that node's own execution -- assume the agent running it sees nothing except this " |
| 269 | "task_prompt and its dependencies' outputs, not this planning conversation), plus:\n" | 310 | "task_prompt and its dependencies' outputs, not this planning conversation), plus:\n" |
| 311 | + " - node_type (optional but STRONGLY suggested when a node matches one of the suggested " | ||
| 312 | + "types above): echo back the matching node_type string (e.g. \"test_script\") so it's " | ||
| 313 | + "identifiable later -- omit only for a node that doesn't match any suggested type.\n" | ||
| 270 | " - judgment (optional): how to tell if this node's output is acceptable -- " | 314 | " - judgment (optional): how to tell if this node's output is acceptable -- " |
| 271 | "{\"kind\": \"none\"} if nothing needs checking (the default if omitted), {\"kind\": " | 315 | "{\"kind\": \"none\"} if nothing needs checking (the default if omitted), {\"kind\": " |
| 272 | "\"llm\", \"judge_prompt\": \"...{content}...\"} for your own judgment prompt with a " | 316 | "\"llm\", \"judge_prompt\": \"...{content}...\"} for your own judgment prompt with a " |
| @@ -274,13 +318,14 @@ def _generic_prompt_template(org_node): | @@ -274,13 +318,14 @@ def _generic_prompt_template(org_node): | ||
| 274 | " - on_fail (only meaningful if judgment.kind != \"none\"): {\"action\": \"retry_self\"}, " | 318 | " - on_fail (only meaningful if judgment.kind != \"none\"): {\"action\": \"retry_self\"}, " |
| 275 | "{\"action\": \"hard_fail\"}, or {\"action\": \"loop_back\"} with a matching on_failure " | 319 | "{\"action\": \"hard_fail\"}, or {\"action\": \"loop_back\"} with a matching on_failure " |
| 276 | "edge to a genuine upstream dependency (same rule _PLANNER_NODE_CATALOG's own nodes " | 320 | "edge to a genuine upstream dependency (same rule _PLANNER_NODE_CATALOG's own nodes " |
| 277 | - "follow) -- do NOT use \"dynamic_loop_back\", that's reserved for the terminal review " | ||
| 278 | - "stage outside this graph, not for nodes you plan yourself.\n" | 321 | + "follow); " + dynamic_loop_back_rule + |
| 279 | " - promotes_to_artifact (optional): {\"artifact_type\": \"test_report\"} ONLY for a " | 322 | " - promotes_to_artifact (optional): {\"artifact_type\": \"test_report\"} ONLY for a " |
| 280 | "node producing this task's actual final deliverable text -- most nodes should omit " | 323 | "node producing this task's actual final deliverable text -- most nodes should omit " |
| 281 | "this; it marks a node's content for persistence beyond this graph's own execution. " | 324 | "this; it marks a node's content for persistence beyond this graph's own execution. " |
| 282 | "(code_diff, if relevant, is derived separately from real file changes, not from any " | 325 | "(code_diff, if relevant, is derived separately from real file changes, not from any " |
| 283 | - "node's own content -- never declare artifact_type: \"code_diff\" here.)\n" | 326 | + "node's own content -- never declare artifact_type: \"code_diff\" here. The final_verdict " |
| 327 | + "node, if any, should also NOT declare promotes_to_artifact -- a verdict is a judgment, " | ||
| 328 | + "not a persisted deliverable.)\n" | ||
| 284 | "Every edge needs edge_type: 'zero_token' for pure aggregation/reduce steps, " | 329 | "Every edge needs edge_type: 'zero_token' for pure aggregation/reduce steps, " |
| 285 | "'needs_judgment' for anything requiring the agent to actually reason about content.\n" | 330 | "'needs_judgment' for anything requiring the agent to actually reason about content.\n" |
| 286 | "End your reply with the graph_manifest as a single fenced ```json code block " | 331 | "End your reply with the graph_manifest as a single fenced ```json code block " |