From c4b1cb5c5084d9e45b9f2b8d68468e7885a99134 Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Fri, 7 Aug 2026 20:57:49 +0530 Subject: [PATCH] feat(orchestration): run verify_orchestration_prompt on what Step 1 emits The function was written, exported from runtime_verification and covered by four unit tests, and called by nothing. Its sibling verify_orchestrator_result was deleted in 94cbaf7 for that reason plus a false premise -- a str signature for a value that has always been a dict. This one encodes a true contract, so the choice was to wire it or delete it, and leaving it was the only option that got worse over time: a correct-but-uncalled validator looks like coverage that does not exist. Closes item 4 of #296. The gap it closes is narrower than "the prompt is unchecked", and checking first changed the justification. Both degraded paths in the node ALREADY log -- an ERROR from prompt_gen falls back to the raw task at logger.error, and an empty llm_response/prompt falls back at logger.warning. So this is not here to make those visible. It is here for the case neither covers: a response that is non-empty, and therefore silent, but not a usable orchestration prompt -- a truncated template, the wrong file, a stub. Until now that was indistinguishable from a good prompt. Warnings are recorded on orchestrator_result["prompt_warnings"] and logged, and are never fatal. STEP1_CONTRACT documents the short raw-task fallback as a legitimate degraded path, and that fallback trips both of the verifier's checks by design, so failing the step here would turn every recoverable run into a dead one. The same reasoning e0d9372 applied to Phase 2: loud and visible rather than fatal. Wiring was confirmed possible before it was done rather than after: the master template resolved from claude-global-library is 1,996 lines and 335 KB and contains "Phase" 386 times, so the happy path clears both the 200-character floor and the keyword check without a spurious warning on every run. The new test drives step1_task_analysis_node rather than calling the verifier directly, because tests/test_schema_verifier.py already covers the function in isolation and that coverage is exactly what failed to notice it had no callers. The two cases are a pair on purpose: "a good prompt produces no warnings" passes just as well when the verifier is never invoked, since an unwired check and a satisfied check both return an empty list. Proven by replacing the call with a literal [] and confirming the malformed-prompt case and the not-fatal case both go red while the well-formed case stayed green, then restoring it. Counts move to 489 / 101 / 92 / 122. Caught by tests/test_doc_counts_match_the_tree.py across all seven claim sites rather than by hand -- the second time that enforcement has paid for itself since it landed. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 + CLAUDE.md | 4 +- README.md | 10 +- .../sdlc_pipeline/nodes/task_orchestration.py | 21 ++++ .../test_orchestration_prompt_verification.py | 104 ++++++++++++++++++ 5 files changed, 136 insertions(+), 7 deletions(-) create mode 100644 tests/test_orchestration_prompt_verification.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d20112f..ad939a3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Added + +- **Step 1 now verifies the orchestration prompt it emits.** `verify_orchestration_prompt()` existed, was exported and had four unit tests, and was called by nothing; it is now wired in at the point the prompt is stored, and its findings are recorded on `orchestrator_result["prompt_warnings"]` and logged. The gap it closes is narrow and specific: both degraded paths in the node already log (an `ERROR` from prompt-gen, and an empty response), so this is not for those. It is for a response that is **non-empty and therefore silent** but not a usable prompt — a truncated template, the wrong file, a stub — which until now was indistinguishable from a good one. Warnings are never fatal: `STEP1_CONTRACT` documents the short raw-task fallback as a legitimate degraded path, and it trips both checks by design, so failing here would convert a recoverable run into a dead one. + ### Removed - **`langgraph_engine/helper_nodes/` is deleted** (5 modules, 538 lines). All nine of its public node functions were also defined in `orchestrator.py`, which is the module the graph actually wires, and nothing imported the package -- it was an extraction that was started and never completed. Two documentation corrections existed **only** in the orphaned copy and were ported to `orchestrator.py` before the deletion rather than discarded with it: `save_workflow_memory`'s comment named `~/.claude/memory/sessions/` while its code writes to `memory/logs/sessions/`, and `optimize_context_after_level1` still said "Level 3" in three places, a level that stopped existing in the `06b0463` rename. The live module had the stale text in both cases. diff --git a/CLAUDE.md b/CLAUDE.md index 37a1b00b..79a2f49f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,8 +20,8 @@ Claude Workflow Engine is a 3-level LangGraph-based orchestration pipeline for a | **Status** | Active Development | | **Primary Location** | langgraph_engine/ | | **MCP Servers** | 13 servers -- all in separate repos under [techdeveloper-org](https://github.com/orgs/techdeveloper-org/repositories); 1 also keeps an in-engine copy in `src/mcp/` | -| **Total Python Files** | 248 (langgraph_engine/); 488 repo-wide | -| **Test Files** | 100 (91 unit, 5 integration, 3 e2e, 1 load) | +| **Total Python Files** | 248 (langgraph_engine/); 489 repo-wide | +| **Test Files** | 101 (92 unit, 5 integration, 3 e2e, 1 load) | | **Call Graph** | 578 classes, 3,985 methods, 4 languages (Python/Java/TS/Kotlin) | --- diff --git a/README.md b/README.md index 51469b91..767e3d20 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ [![PyPI](https://img.shields.io/badge/PyPI-claude--workflow--engine-orange)](https://pypi.org/project/claude-workflow-engine/) [![CI](https://github.com/techdeveloper-org/claude-workflow-engine/actions/workflows/ci.yml/badge.svg)](https://github.com/techdeveloper-org/claude-workflow-engine/actions/workflows/ci.yml) [![License](https://img.shields.io/badge/License-MIT-yellow)](LICENSE) -[![Tests](https://img.shields.io/badge/Tests-100%20files%20·%205%20integration-brightgreen)](tests/) +[![Tests](https://img.shields.io/badge/Tests-101%20files%20·%205%20integration-brightgreen)](tests/) [![Discussions](https://img.shields.io/badge/Discussions-GitHub-blueviolet)](../../discussions) --- @@ -461,8 +461,8 @@ claude-workflow-engine/ # 369 Python files total ├── policies/ # non-.md policy state only (failure-kb.json) — see note below │ └── 03-execution-system/failure-prevention/failure-kb.json │ -├── tests/ # 100 test_*.py files (121 total Python files) -│ ├── test_*.py # 91 unit tests +├── tests/ # 101 test_*.py files (122 total Python files) +│ ├── test_*.py # 92 unit tests │ ├── integration/ # 5 integration tests (GitHub, MCP, runtime verification) │ ├── e2e/ # 3 end-to-end scenario tests │ └── load/ # 1 concurrency / load test @@ -620,11 +620,11 @@ ENABLE_HEALTH_SERVER=1 ENABLE_METRICS=1 LOG_FORMAT=json \ ## Testing -100 test_*.py files (121 total Python files in `tests/` including conftest and `__init__` files): +101 test_*.py files (122 total Python files in `tests/` including conftest and `__init__` files): | Category | Files | Notes | |---|:---:|---| -| Unit tests | 91 | In `tests/test_*.py` — no external dependencies required | +| Unit tests | 92 | In `tests/test_*.py` — no external dependencies required | | Integration tests | 5 | `tests/integration/` — require live GitHub token or MCP servers | | E2E tests | 3 | `tests/e2e/` — require full pipeline environment | | Load tests | 1 | `tests/load/` — enabled with `RUN_LOAD_TESTS=1` | diff --git a/langgraph_engine/sdlc_pipeline/nodes/task_orchestration.py b/langgraph_engine/sdlc_pipeline/nodes/task_orchestration.py index b0cea808..d1d45876 100644 --- a/langgraph_engine/sdlc_pipeline/nodes/task_orchestration.py +++ b/langgraph_engine/sdlc_pipeline/nodes/task_orchestration.py @@ -236,13 +236,34 @@ def step1_task_analysis_node(state: FlowState) -> Dict[str, Any]: "--- BEGIN ORCHESTRATION TEMPLATE ---\n\n" + orchestration_prompt + "\n\n--- END ORCHESTRATION TEMPLATE ---" ) + # Structural check on what is about to be emitted. Both degraded paths above + # already log -- an ERROR from prompt_gen, or an empty response -- so this is + # not here to catch those. It is here for the case neither covers: a response + # that is non-empty and therefore silent, but not a usable orchestration + # prompt (a truncated template, the wrong file, a stub). Until now that was + # indistinguishable from a good prompt. + # + # Warnings are recorded and logged, never fatal. The raw-task fallback is a + # legitimate degraded path -- STEP1_CONTRACT documents it as such -- and it + # trips both checks by design, so failing here would turn a recoverable run + # into a dead one. + from ...runtime_verification.schema_verifier import verify_orchestration_prompt # noqa: PLC0415 + + prompt_warnings = verify_orchestration_prompt(orchestration_prompt) + orch_result: Dict[str, Any] = { "mode": "emitted", "success": True, "prompt_chars": len(_full_prompt), "template_source": "claude-global-library/ORCHESTRATION_TEMPLATE.md", "library_version": _library_version(), + "prompt_warnings": prompt_warnings, } + if prompt_warnings: + logger.warning( + "[v2] Step 1 emitted a structurally questionable orchestration prompt: {}", + "; ".join(prompt_warnings), + ) logger.info( "[v2] Step 1 emitted orchestration prompt: {} chars, library v{}", orch_result["prompt_chars"], diff --git a/tests/test_orchestration_prompt_verification.py b/tests/test_orchestration_prompt_verification.py new file mode 100644 index 00000000..f4319329 --- /dev/null +++ b/tests/test_orchestration_prompt_verification.py @@ -0,0 +1,104 @@ +"""Step 1 must actually run verify_orchestration_prompt on what it emits. + +The function was written, exported and unit-tested, and then called by nothing. +Its sibling verify_orchestrator_result was deleted for that reason plus a false +premise; this one encodes a true contract, so it was wired in instead. + +A wire-in needs a test that would notice the wire coming loose, otherwise it +recreates the original problem in a new place -- a validator that exists, passes +its own unit tests, and never sees production data. So every assertion here goes +through step1_task_analysis_node rather than calling the verifier directly: +tests/test_schema_verifier.py already covers the function in isolation, and that +coverage is exactly what failed to notice it had no callers. + +The two cases are a pair on purpose. A test that only asserts "a good prompt +produces no warnings" passes just as well when the verifier is never invoked, +because an unwired check and a satisfied check both produce an empty list. +""" + +import sys +from pathlib import Path +from unittest.mock import patch + +_PROJECT_ROOT = Path(__file__).resolve().parent.parent +if str(_PROJECT_ROOT) not in sys.path: + sys.path.insert(0, str(_PROJECT_ROOT)) + +# Long enough to clear the 200-char floor and carrying the "Phase" keyword the +# verifier looks for, so it stands in for a real master template. +WELL_FORMED_PROMPT = "Phase A -- foundation. " + ("orchestration detail. " * 40) + +# Non-empty, so neither degraded path in the node fires and nothing else logs: +# this is the case the wire-in exists for. +MALFORMED_PROMPT = "do the thing" + + +def _script_mock(prompt_text): + """Return a call_execution_script stand-in yielding the given prompt.""" + + def _side_effect(script_name, args=None, model_tier=None, silence_interval=None): + if script_name == "prompt_gen_expert_caller": + return {"status": "SUCCESS", "llm_response": prompt_text, "prompt": "raw"} + return {"status": "SUCCESS"} + + return _side_effect + + +def _run_node(prompt_text): + """Drive step1_task_analysis_node with a mocked prompt-gen response.""" + from langgraph_engine.sdlc_pipeline.nodes.task_orchestration import step1_task_analysis_node + + state = {"user_message": "Add a new endpoint", "project_root": str(_PROJECT_ROOT)} + with patch( + "langgraph_engine.sdlc_pipeline.helpers.call_execution_script", + side_effect=_script_mock(prompt_text), + ): + return step1_task_analysis_node(state) + + +class TestTheVerifierRunsOnWhatIsEmitted: + """The check must reach the emitted prompt, not merely exist.""" + + def test_a_well_formed_prompt_records_no_warnings(self): + result = _run_node(WELL_FORMED_PROMPT) + orch = result["orchestrator_result"] + assert "prompt_warnings" in orch, "the verifier result must reach orchestrator_result" + assert orch["prompt_warnings"] == [] + + def test_a_malformed_prompt_records_warnings(self): + """The half that fails if the verifier is never called. + + A non-empty but unusable prompt is the case neither degraded path in the + node covers -- prompt_gen returned something, so nothing else logs. If + this list came back empty the check would be unwired, and the pass case + above could not tell the difference. + """ + result = _run_node(MALFORMED_PROMPT) + warnings = result["orchestrator_result"]["prompt_warnings"] + assert warnings, "a 12-character prompt with no 'Phase' keyword must be reported" + joined = " ".join(warnings).lower() + assert "short" in joined or "200" in joined + assert "phase" in joined + + +class TestWarningsAreNotFatal: + """A questionable prompt is reported, not turned into a failed run.""" + + def test_the_step_still_emits_and_reports_success(self): + """The raw-task fallback trips both checks by design. + + STEP1_CONTRACT documents a short raw task as a legitimate degraded path, + so a verifier that failed the step would convert every recoverable run + into a dead one. + """ + result = _run_node(MALFORMED_PROMPT) + orch = result["orchestrator_result"] + assert orch["prompt_warnings"] + assert orch["success"] is True + assert orch["mode"] == "emitted" + assert result["orchestration_prompt"] == MALFORMED_PROMPT + + def test_prompt_chars_still_counts_the_full_emitted_prompt(self): + """Guards against the wire-in being placed where it truncates the result.""" + result = _run_node(WELL_FORMED_PROMPT) + assert result["orchestrator_result"]["prompt_chars"] > len(WELL_FORMED_PROMPT)