From c256b9348e3fd21596656067efee06a24eb28cd5 Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Sat, 8 Aug 2026 08:19:30 +0530 Subject: [PATCH] fix(orchestration): Step 1 could not run on Windows at all Found by running the pipeline rather than by reading it. Every Step 1 run on Windows died, fell back to the raw task, and left the whole SDLC chain doing nothing while the run reported OK: Step 1 prompt_gen_expert_caller ERROR: [WinError 206] The filename or extension is too long -- falling back to raw task Step 1 emitted a structurally questionable orchestration prompt: orchestration_prompt is empty Step 2: Skipping issue creation -- prompt too short (0 chars) Step 3: Skipping branch creation -- no GitHub issue created The call graph is passed to prompt_gen_expert_caller as a command-line argument. On this repository it serialises to 1,031,711 characters -- 5,527 affected methods and 940 danger zones -- against a Windows CreateProcess limit of 32,767. Thirty-one times over. Introduced 2026-07-25 in 06b0463, silent for a month because the fallback path logged an ERROR nobody was reading and every downstream step then declined its work politely. A temp file was the obvious fix and is the wrong one. The receiver already rendered only affected_methods[:10], so nine hundred kilobytes were being serialised, marshalled across a process boundary, and discarded. danger_zones it rendered in FULL, so a faithful transport would have put 123 KB of names into every orchestration prompt -- 940 entries is not grounding, it is noise, and it alone still exceeds the limit. The defect is the payload, not the pipe. So the sender caps at ten and sends the true totals alongside. For affected_methods the rendered prompt is byte-identical to what the receiver produced before, because it was already truncating. For danger_zones it now reads "showing 10 of 942" -- this repository's own rule is that a cap must not be silent, and a prompt that presents ten of nine hundred as if they were all is asserting something false about the codebase it claims to describe. Fixing the size uncovered a second defect standing behind the first. The analyser yields dicts (fqn, callers_count, callers_count_high_confidence, and risk for affected methods), and the receiver did ", ".join(danger_zones) on them, which raises TypeError: sequence item 0: expected str instance, dict found. It had never surfaced because the argument never arrived. Fixing only the size would have swapped WinError 206 for a TypeError. _entry_label reads fqn and falls back to str() rather than requiring the key, so a future shape change degrades to an ugly line instead of killing prompt assembly. Measured before and after on this repository: command line 1,032,492 -> 3,720 chars (limit 32,767) orchestration_prompt 0 -> 334,745 chars prompt_warnings "empty" -> [] Step 2 skip -> ran, 14.6s, reached GitHub Step 2 now fails on credentials (401 Bad credentials, gh CLI not authenticated) rather than on an empty prompt. That is an environment problem and out of scope here; the point is that it gets far enough to have one. README: `--message "..."` was documented space-separated in six places. The parser matches on the `--message=` prefix, so the space form is silently ignored and the pipeline runs with an empty task -- verified: space form gives user_message length=0, equals form gives 14. Two invocations also lacked --invoked-by entirely and would have been refused. I introduced neither, but I carried both forward in yesterday's README pass while checking that --invoked-by passed the gate and never checking that --message landed. Also corrects the claim that --invoked-by selects behaviour. It only authorizes; mode is CLAUDE_HOOK_MODE and the per-command step ranges belong to the plugin dispatcher. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 4 +- README.md | 40 +++-- .../architecture/prompt_gen_expert_caller.py | 58 ++++++- .../sdlc_pipeline/nodes/task_orchestration.py | 21 ++- tests/test_step1_callgraph_payload_size.py | 146 ++++++++++++++++++ 5 files changed, 246 insertions(+), 23 deletions(-) create mode 100644 tests/test_step1_callgraph_payload_size.py diff --git a/CLAUDE.md b/CLAUDE.md index c52c7b78..28cbb3c2 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/); 490 repo-wide | -| **Test Files** | 102 (93 unit, 5 integration, 3 e2e, 1 load) | +| **Total Python Files** | 248 (langgraph_engine/); 491 repo-wide | +| **Test Files** | 103 (94 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 981c5514..7eec65df 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ [![Python](https://img.shields.io/badge/Python-3.10%2B-green)](https://python.org) [![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-102%20files%20·%205%20integration-brightgreen)](tests/) +[![Tests](https://img.shields.io/badge/Tests-103%20files%20·%205%20integration-brightgreen)](tests/) [![Discussions](https://img.shields.io/badge/Discussions-GitHub-blueviolet)](../../discussions) --- @@ -106,20 +106,28 @@ yourself with `--invoked-by=`: ```bash # Hook Mode (default) — analysis, GitHub issue, branch -python scripts/3-level-flow.py --invoked-by=implement --message "Fix the login timeout bug" +python scripts/3-level-flow.py --invoked-by=implement --message="Fix the login timeout bug" # Full Mode — all 9 active steps end-to-end (implements + PR + closes issue) CLAUDE_HOOK_MODE=0 python scripts/3-level-flow.py --invoked-by=run-pipeline \ - --message "Fix the login timeout bug" + --message="Fix the login timeout bug" # Template Fast-Path — Step 0 detects the template and skips Step 1 planning python scripts/3-level-flow.py --invoked-by=run-pipeline \ - --message "Add document Q&A feature" \ + --message="Add document Q&A feature" \ --orchestration-template=orchestration_template.example.json ``` -Omitting `--invoked-by=` prints `[REFUSED]` and exits 0 without running anything. A -misspelled command name exits 2, so a typo costs an error rather than a silent no-op. +Both flags need the **equals form**. The parser matches on the `--message=` prefix, so a +space-separated `--message "..."` is silently ignored: the run proceeds with an empty +task, the orchestration prompt comes out empty, and Steps 2 and 3 skip while the run +still reports OK. + +`--invoked-by=` selects nothing — it only authorizes. Mode comes from +`CLAUDE_HOOK_MODE`, and the per-command step ranges listed above are the plugin +dispatcher's doing, not this flag's. Omitting it prints `[REFUSED]` and exits 0 without +running anything; a misspelled command name exits 2, so a typo costs an error rather +than a silent no-op. ### What happens when you run it @@ -175,7 +183,7 @@ flowchart TD ```mermaid flowchart TD - START(["Task Input\npython scripts/3-level-flow.py --task ..."]) + START(["Task Input\npython scripts/3-level-flow.py --invoked-by=... --message=..."]) subgraph LM1["Level 0 : Pre-Flight Sanity Guard"] direction LR @@ -335,11 +343,11 @@ flowchart LR ### Template Fast-Path -Pre-fill a JSON orchestration template once, and every subsequent run skips Step 0 entirely: +Pre-fill a JSON orchestration template once, and Step 0 detects it and skips Step 1 planning: ```bash -python scripts/3-level-flow.py \ - --message "add document Q&A feature" \ +python scripts/3-level-flow.py --invoked-by=run-pipeline \ + --message="add document Q&A feature" \ --orchestration-template=orchestration_template.example.json # Planning time: ~0s (Step 0 detects the template and skips Step 1) ``` @@ -461,8 +469,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/ # 102 test_*.py files (123 total Python files) -│ ├── test_*.py # 93 unit tests +├── tests/ # 103 test_*.py files (124 total Python files) +│ ├── test_*.py # 94 unit tests │ ├── integration/ # 5 integration tests (GitHub, MCP, runtime verification) │ ├── e2e/ # 3 end-to-end scenario tests │ └── load/ # 1 concurrency / load test @@ -593,7 +601,7 @@ Step 6: COMMENT "Implementation complete" with PR link ```bash docker build -t claude-workflow-engine . docker run --env-file .env claude-workflow-engine \ - python scripts/3-level-flow.py --message "your task" + python scripts/3-level-flow.py --invoked-by=run-pipeline --message="your task" ``` ### Docker Compose @@ -613,18 +621,18 @@ kubectl apply -f k8s/secret.yaml -f k8s/configmap.yaml \ ```bash ENABLE_HEALTH_SERVER=1 ENABLE_METRICS=1 LOG_FORMAT=json \ - python scripts/3-level-flow.py --message "your task" + python scripts/3-level-flow.py --invoked-by=run-pipeline --message="your task" ``` --- ## Testing -102 test_*.py files (123 total Python files in `tests/` including conftest and `__init__` files): +103 test_*.py files (124 total Python files in `tests/` including conftest and `__init__` files): | Category | Files | Notes | |---|:---:|---| -| Unit tests | 93 | In `tests/test_*.py` — no external dependencies required | +| Unit tests | 94 | 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/architecture/prompt_gen_expert_caller.py b/langgraph_engine/sdlc_pipeline/architecture/prompt_gen_expert_caller.py index d9bebf48..b3a1ffa5 100644 --- a/langgraph_engine/sdlc_pipeline/architecture/prompt_gen_expert_caller.py +++ b/langgraph_engine/sdlc_pipeline/architecture/prompt_gen_expert_caller.py @@ -171,6 +171,58 @@ def _load_template(): ) +def _render_sample(items, total=None, limit=10): + """Render up to ``limit`` items, stating how many were left out. + + The caller sends a sample rather than the full list, because the full lists + reach a million characters on a large repository and a command line cannot + carry that on Windows. The count travels with the sample so the grounding + header can say "10 of 940" instead of presenting ten as if they were all -- + a truncation the reader cannot see is worse than no truncation, because the + prompt then asserts something false about the codebase. + + Args: + items: The sample to render. + total: How many existed before sampling. None means unknown, in which + case the sample is assumed complete -- which is what an older caller + that sends no total implies. + limit: Maximum entries to render. + + Returns: + str: Comma-joined entries, suffixed with the omitted count, or "none". + """ + if not items: + return "none" + shown = list(items)[:limit] + rendered = ", ".join(_entry_label(entry) for entry in shown) + if total is not None and total > len(shown): + return "%s (showing %d of %d)" % (rendered, len(shown), total) + return rendered + + +def _entry_label(entry): + """Return the human-meaningful name for one call-graph entry. + + The analyser yields dicts -- ``fqn``, ``callers_count``, + ``callers_count_high_confidence``, and ``risk`` for affected methods -- not + strings. The previous code joined them directly, which raises + ``TypeError: sequence item 0: expected str instance, dict found``. That never + surfaced because the argument carrying them exceeded the Windows command-line + limit and the caller died before rendering; one bug stood in front of the + other. Falling back to str() rather than requiring the key, so a shape change + degrades to an ugly line instead of an exception in prompt assembly. + + Args: + entry: A call-graph entry, normally a dict carrying ``fqn``. + + Returns: + str: The entry's fully-qualified name, or its string form. + """ + if isinstance(entry, dict): + return str(entry.get("fqn") or entry.get("name") or entry) + return str(entry) + + def _render_kg_routing_block(kg_routing): """Render the KG ROUTING grounding block for the ``{kg_routing_block}`` placeholder. @@ -241,9 +293,9 @@ def _build_filled_prompt(template, args): affected_methods = call_graph.get("affected_methods", []) hot_nodes = call_graph.get("hot_nodes", []) - danger_zones_str = ", ".join(danger_zones) if danger_zones else "none" - affected_str = ", ".join(affected_methods[:10]) if affected_methods else "none" - hot_nodes_str = ", ".join(hot_nodes[:10]) if hot_nodes else "none" + danger_zones_str = _render_sample(danger_zones, call_graph.get("danger_zones_total")) + affected_str = _render_sample(affected_methods, call_graph.get("affected_methods_total")) + hot_nodes_str = _render_sample(hot_nodes, call_graph.get("hot_nodes_total")) runtime_block = json.dumps(runtime_context, indent=2, ensure_ascii=True) diff --git a/langgraph_engine/sdlc_pipeline/nodes/task_orchestration.py b/langgraph_engine/sdlc_pipeline/nodes/task_orchestration.py index d1d45876..a52c487c 100644 --- a/langgraph_engine/sdlc_pipeline/nodes/task_orchestration.py +++ b/langgraph_engine/sdlc_pipeline/nodes/task_orchestration.py @@ -161,11 +161,28 @@ def step1_task_analysis_node(state: FlowState) -> Dict[str, Any]: # NFR-2 / ADR-016 replace it: unbounded unless an operator configures a # silence interval, which measures no-progress rather than duration. _pg_silence = _env_optional_seconds("STEP1_PROMPT_GEN_SILENCE") + # Send a sample, not the whole graph. These lists go across a process + # boundary as a command-line argument, and on this repository they serialise + # to 1,031,711 characters -- 5,527 affected methods and 940 danger zones. + # Windows caps a command line at 32,767, so every Step 1 run on Windows died + # with WinError 206, fell back to the raw task, and left orchestration_prompt + # empty; Steps 2 and 3 then skipped issue and branch creation and the whole + # SDLC chain quietly did nothing while the run reported OK. + # + # Capping here rather than transporting it all and discarding it there. + # prompt_gen_expert_caller already rendered only affected_methods[:10], so + # that half of this is byte-identical to what it produced before. danger_zones + # it rendered in full, so the totals travel alongside the sample and the + # grounding header states how many were omitted -- a cap that hides its own + # size is how a prompt starts lying about the codebase it describes. + _CG_SAMPLE = 10 _call_graph_json = _json.dumps( { "risk_level": call_graph_risk_level, - "danger_zones": call_graph_danger_zones, - "affected_methods": call_graph_affected_methods, + "danger_zones": call_graph_danger_zones[:_CG_SAMPLE], + "danger_zones_total": len(call_graph_danger_zones), + "affected_methods": call_graph_affected_methods[:_CG_SAMPLE], + "affected_methods_total": len(call_graph_affected_methods), } ) _kg_routing_json = _json.dumps(kg_routing_result) diff --git a/tests/test_step1_callgraph_payload_size.py b/tests/test_step1_callgraph_payload_size.py new file mode 100644 index 00000000..ea41101f --- /dev/null +++ b/tests/test_step1_callgraph_payload_size.py @@ -0,0 +1,146 @@ +"""Step 1's call-graph argument must fit in a Windows command line. + +The payload is passed to prompt_gen_expert_caller as a command-line argument. On +this repository the ungated lists serialise to 1,031,711 characters -- 5,527 +affected methods and 940 danger zones -- against a Windows CreateProcess limit of +32,767. Every Step 1 run on Windows therefore died with WinError 206, fell back +to the raw task, and left orchestration_prompt empty. Steps 2 and 3 then skipped +issue and branch creation, and the run reported OK having done nothing. + +That failure was silent for a month. It is the reason these tests assert on the +SIZE of what is sent rather than only on the content: a correctness test would +have passed throughout, because the values were right and merely undeliverable. + +The receiver already truncated affected_methods to ten, so capping the sender +changes nothing about the rendered prompt for that list. danger_zones it rendered +in full, so the totals are sent alongside the sample and _render_sample states +what was omitted -- a truncation the reader cannot see is worse than none, since +the prompt then asserts something false about the codebase. +""" + +import json +import sys +from pathlib import Path + +_PROJECT_ROOT = Path(__file__).resolve().parent.parent +if str(_PROJECT_ROOT) not in sys.path: + sys.path.insert(0, str(_PROJECT_ROOT)) + +from langgraph_engine.sdlc_pipeline.architecture.prompt_gen_expert_caller import _render_sample # noqa: E402 + +WINDOWS_CMDLINE_LIMIT = 32767 + +# Shaped like the real thing on this repository, big enough that an uncapped +# payload cannot fit. Built rather than measured so the test does not depend on +# how large the repository happens to be on the day it runs. +MANY_METHODS = ["langgraph_engine.module_%d.Class%d.method_%d" % (i, i, i) for i in range(5527)] +MANY_ZONES = ["langgraph_engine/danger/zone_%d.py" % i for i in range(940)] + + +def _build_payload(danger_zones, affected_methods, sample=10): + """Build the argument exactly as step1_task_analysis_node builds it.""" + return json.dumps( + { + "risk_level": "high", + "danger_zones": danger_zones[:sample], + "danger_zones_total": len(danger_zones), + "affected_methods": affected_methods[:sample], + "affected_methods_total": len(affected_methods), + } + ) + + +class TestThePayloadFitsInACommandLine: + """The size property the outage was caused by.""" + + def test_a_large_graph_still_fits(self): + payload = _build_payload(MANY_ZONES, MANY_METHODS) + assert len(payload) < WINDOWS_CMDLINE_LIMIT, "payload is %d chars, over the %d Windows limit" % ( + len(payload), + WINDOWS_CMDLINE_LIMIT, + ) + + def test_the_uncapped_payload_would_not_fit(self): + """The half that proves the cap is what makes the difference. + + Without this, the test above passes on any repository small enough to fit + anyway, and would have passed throughout the outage. + """ + uncapped = json.dumps({"risk_level": "high", "danger_zones": MANY_ZONES, "affected_methods": MANY_METHODS}) + assert len(uncapped) > WINDOWS_CMDLINE_LIMIT + + def test_the_totals_survive_the_cap(self): + """A sample without its total cannot be reported honestly downstream.""" + payload = json.loads(_build_payload(MANY_ZONES, MANY_METHODS)) + assert payload["danger_zones_total"] == 940 + assert payload["affected_methods_total"] == 5527 + assert len(payload["danger_zones"]) == 10 + assert len(payload["affected_methods"]) == 10 + + +class TestTheRenderedSampleDeclaresWhatItOmitted: + """A cap the reader cannot see makes the prompt assert something false.""" + + def test_a_truncated_list_states_the_true_total(self): + rendered = _render_sample(MANY_ZONES[:10], total=940) + assert "showing 10 of 940" in rendered + + def test_a_complete_list_carries_no_suffix(self): + """The half that fails if the suffix were unconditional.""" + rendered = _render_sample(["a", "b"], total=2) + assert rendered == "a, b" + assert "showing" not in rendered + + def test_an_absent_total_is_treated_as_complete(self): + """An older caller sends no total; it must not be reported as truncated.""" + rendered = _render_sample(["a", "b"], total=None) + assert rendered == "a, b" + assert "showing" not in rendered + + def test_an_empty_list_renders_as_none(self): + assert _render_sample([], total=0) == "none" + assert _render_sample(None, total=None) == "none" + + +class TestEntriesAreDictsNotStrings: + """The second bug, which the first one hid. + + The analyser yields dicts carrying fqn / callers_count / risk. The previous + code joined them directly, which raises TypeError on a dict. It never + surfaced because the argument carrying them blew the Windows command-line + limit and the caller died before reaching the join -- so fixing only the size + would have swapped a WinError for a TypeError. + """ + + def test_a_dict_entry_renders_its_fqn(self): + entries = [{"fqn": "pkg.mod.Class.method", "callers_count": 42, "risk": "high"}] + assert _render_sample(entries, total=1) == "pkg.mod.Class.method" + + def test_the_old_join_would_have_raised_on_these(self): + """Pins why this is a separate defect rather than a style preference.""" + entries = [{"fqn": "pkg.mod.Class.method", "callers_count": 42}] + try: + ", ".join(entries) + except TypeError: + return + raise AssertionError("joining dicts must raise TypeError, or this test proves nothing") + + def test_a_plain_string_entry_still_works(self): + """Older callers, and hot_nodes, may still send strings.""" + assert _render_sample(["a.b.c"], total=1) == "a.b.c" + + def test_an_unexpected_shape_degrades_instead_of_raising(self): + """Prompt assembly must not die because an entry grew a new shape.""" + assert _render_sample([{"unexpected": 1}], total=1) != "" + + +class TestTheNodeSendsTheCappedShape: + """The cap must live in the node, not only in this test's helper.""" + + def test_the_node_source_caps_both_lists_and_sends_totals(self): + source = (_PROJECT_ROOT / "langgraph_engine" / "sdlc_pipeline" / "nodes" / "task_orchestration.py").read_text( + encoding="utf-8" + ) + assert "danger_zones_total" in source, "the node must send the danger_zones total" + assert "affected_methods_total" in source, "the node must send the affected_methods total" + assert "_CG_SAMPLE" in source, "the node must cap the lists before serialising"