diff --git a/CHANGELOG.md b/CHANGELOG.md index ad939a3f..132bef33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/). ### Fixed +- **`FORCE_GRAPH_REBUILD` now exists in code, not only in documentation.** The variable was described in five files -- `CLAUDE.md`, `ADR-002`, the deployment guide, `RUNBOOK_STALE_GRAPH.md` and the troubleshooting guide -- and read by nothing, since `a43595b` (2026-03-28). RUNBOOK Option B tells an operator to `export FORCE_GRAPH_REBUILD=1` while recovering from a stale-graph incident, and that command did nothing. It is now honoured by `refresh_call_graph_if_stale()`, which rebuilds unconditionally when it is `1`. This matters for the exact case ADR-002 names as its own risk and the stale flag cannot cover: when `call_graph_stale` is never set because the node that sets it crashed, the guard cannot fire and Step 5 reviews an outdated snapshot in silence — with no in-state signal for an operator to correct, so the override must come from outside the state. Implementing it made the ADR, the deployment guide and the troubleshooting guide true as written; none needed editing. +- **`RUNBOOK_STALE_GRAPH.md` Option A described behaviour the flag never had.** It implied that `FORCE_GRAPH_REBUILD=0` disabled stale-based rebuilding and that unsetting it re-enabled it. Stale rebuilding is driven by the `call_graph_stale` state flag and no environment variable; the override only ever *adds* a rebuild. Corrected, and pinned by a test so the wording cannot drift back. +- **`CLAUDE.md` documented the wrong default for `ENABLE_CI`.** The table said `false`; `ci.yml` reads `vars.ENABLE_CI` and falls back to `true`, so CI runs unless explicitly disabled. The row now also records that it is a repository variable rather than a process environment variable, which is why it behaves unlike every other flag in that table. + - **A dormant runtime-contract bug that predated this work** (fb7bbaa, 6982930). The `step0 -> step8` transition guard declared `orchestrator_result` as a `str` of at least 50 characters; the node has always written a `dict`, and `node_contracts.py` described it correctly as a dict -- so the two disagreed with each other and one disagreed with reality. It never fired only because `ENABLE_RUNTIME_VERIFICATION` defaults to `0`, which means the first operator to enable verification would have hit a failure that had nothing to do with their change. `verify_orchestrator_result()` carried the same `str` premise and is removed above. - **A guard test that was pinning the bug rather than the behaviour** (6982930). `test_guard_step1_to_step2_pass` fed the guard a 60-character string -- exactly what the wrong spec asked for -- so it would have stayed green no matter how wrong the spec became. A positive test alone cannot detect a wrong type, because it builds its input from the same mistaken premise. Paired with a negative case asserting that a `str` `orchestrator_result` is reported as a violation, verified capable of failing by reverting the spec and watching it go red. - **Step 1 Phase 2 no longer claims success it did not have** (e0d9372). `call_execution_script` reports failure as a status dict rather than raising, so the `try/except` meant to catch a failing decomposer could not fire and its warning never logged. Every script-level failure fell through to an empty TODO list with no diagnostic, after which the result was written with a hardcoded `"success": True` -- a decomposer that never ran was indistinguishable from a task with nothing to do. diff --git a/CLAUDE.md b/CLAUDE.md index 79a2f49f..b4d7e7b6 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/); 489 repo-wide | -| **Test Files** | 101 (92 unit, 5 integration, 3 e2e, 1 load) | +| **Total Python Files** | 248 (langgraph_engine/); 490 repo-wide | +| **Test Files** | 102 (93 unit, 5 integration, 3 e2e, 1 load) | | **Call Graph** | 578 classes, 3,985 methods, 4 languages (Python/Java/TS/Kotlin) | --- @@ -304,7 +304,7 @@ All integrations are configurable via environment variables (default: disabled): | `ENABLE_JENKINS` | `0` | Jenkins build validation (Step 5) | | `ENABLE_SONARQUBE` | `0` | SonarQube scan after implementation (Step 4) | | `ENABLE_FIGMA` | `0` | Figma design-to-code pipeline (Steps 4,5,6 -- extraction/injection now inside Step 1 template) | -| `ENABLE_CI` | `false` | GitHub Actions CI pipeline | +| `ENABLE_CI` | `true` | GitHub Actions CI pipeline. A **repo variable**, not a process env var: `ci.yml` reads `vars.ENABLE_CI` and falls back to `true`, so CI runs unless it is explicitly set to `false` | | `ENABLE_HEALTH_SERVER` | `0` | Start HTTP /health + /readiness on HEALTH_PORT (8080) | | `ENABLE_METRICS` | `0` | Start Prometheus /metrics server on METRICS_PORT (9090) | | `ENABLE_TRACING` | `0` | Enable OpenTelemetry tracing (OTLP to OTEL_EXPORTER_OTLP_ENDPOINT) | diff --git a/README.md b/README.md index 767e3d20..8cf1546e 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-101%20files%20·%205%20integration-brightgreen)](tests/) +[![Tests](https://img.shields.io/badge/Tests-102%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/ # 101 test_*.py files (122 total Python files) -│ ├── test_*.py # 92 unit tests +├── tests/ # 102 test_*.py files (123 total Python files) +│ ├── test_*.py # 93 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 -101 test_*.py files (122 total Python files in `tests/` including conftest and `__init__` files): +102 test_*.py files (123 total Python files in `tests/` including conftest and `__init__` files): | Category | Files | Notes | |---|:---:|---| -| Unit tests | 92 | In `tests/test_*.py` — no external dependencies required | +| Unit tests | 93 | 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/docs/guides/RUNBOOK_STALE_GRAPH.md b/docs/guides/RUNBOOK_STALE_GRAPH.md index 2baec6b1..d649b392 100644 --- a/docs/guides/RUNBOOK_STALE_GRAPH.md +++ b/docs/guides/RUNBOOK_STALE_GRAPH.md @@ -68,14 +68,15 @@ signals `refresh_call_graph_if_stale()` to rebuild before serving data. ### Option A — Automatic (standard path) -Ensure `FORCE_GRAPH_REBUILD` is not set to `0` (it defaults to rebuilding on -stale). If someone disabled it for debugging, re-enable: +Rebuilding on a stale graph is unconditional: it is driven by the +`call_graph_stale` state flag, not by any environment variable, so there is +nothing to re-enable. `FORCE_GRAPH_REBUILD` only ever *adds* a rebuild — setting +it to `0` or leaving it unset does not switch the stale path off. -```bash -unset FORCE_GRAPH_REBUILD -``` +Re-run the pipeline from Step 10 onward and let the flag do its work. -Re-run the pipeline from Step 10 onward. +If that does not help, the flag itself is probably not being set — which is the +case Option B exists for. ### Option B — Force full rebuild diff --git a/langgraph_engine/sdlc_pipeline/call_graph_analyzer.py b/langgraph_engine/sdlc_pipeline/call_graph_analyzer.py index 416d8440..0ac27a13 100644 --- a/langgraph_engine/sdlc_pipeline/call_graph_analyzer.py +++ b/langgraph_engine/sdlc_pipeline/call_graph_analyzer.py @@ -20,6 +20,7 @@ """ import logging +import os from pathlib import Path logger = logging.getLogger(__name__) @@ -1389,14 +1390,30 @@ def refresh_call_graph_if_stale(state, project_root): cached pre-implementation snapshot (e.g. step1_impact_analysis) should call this helper instead of reading state directly. + FORCE_GRAPH_REBUILD=1 bypasses the flag and always rebuilds. That is the + mitigation ADR-002 records for its own stated risk: if call_graph_stale is + never set -- because the node that sets it crashed -- the guard cannot fire + and Step 5 reviews an outdated snapshot in silence. An operator recovering + from that has no in-state signal to correct, so the override has to come + from outside the state. RUNBOOK_STALE_GRAPH.md Option B is this variable. + + It was documented in five files and implemented in none of them until + 2026-08-07, so the runbook's recovery step did nothing when an operator ran + it. Only "1" enables it; any other value, including "0" and unset, leaves + the stale-flag behaviour untouched. + Args: state: FlowState dict (or any dict with call_graph_stale key). project_root: Project root directory (str or Path). Returns: - dict - fresh snapshot from snapshot_call_graph() when stale, + dict - fresh snapshot from snapshot_call_graph() when forced or stale, best available cached snapshot otherwise (never raises). """ + if os.getenv("FORCE_GRAPH_REBUILD", "0") == "1": + logger.debug("refresh_call_graph_if_stale: FORCE_GRAPH_REBUILD=1, rebuilding graph") + return snapshot_call_graph(project_root) + if state.get("call_graph_stale"): logger.debug("refresh_call_graph_if_stale: flag set, rebuilding graph") return snapshot_call_graph(project_root) diff --git a/tests/test_force_graph_rebuild.py b/tests/test_force_graph_rebuild.py new file mode 100644 index 00000000..b9154074 --- /dev/null +++ b/tests/test_force_graph_rebuild.py @@ -0,0 +1,107 @@ +"""FORCE_GRAPH_REBUILD must actually bypass the stale flag. + +The variable was documented in five files -- CLAUDE.md, ADR-002, the deployment +guide, RUNBOOK_STALE_GRAPH.md and the troubleshooting guide -- and read by no +code at all. RUNBOOK Option B instructs an operator to `export +FORCE_GRAPH_REBUILD=1` while recovering from a stale-graph incident, and that +command did nothing. A wrong default is confusing; a recovery procedure that +silently fails is worse, because it is consulted precisely when something has +already gone wrong. + +The scenario ADR-002 names is the one that matters here, and it is the case the +stale flag cannot cover: if `call_graph_stale` is never set -- because the node +that sets it crashed -- the guard cannot fire, and Step 5 reviews an outdated +snapshot in silence. There is no in-state signal for the operator to correct, so +the override has to come from outside the state. Every test below therefore +leaves `call_graph_stale` unset and supplies a usable cached snapshot: without +the override, the cache is what the function returns. + +Each assertion is paired with its opposite. "Forced rebuild returns fresh data" +proves nothing on its own, because a function that always rebuilds satisfies it +too -- so the unset and "0" cases assert that the cache is still honoured. +""" + +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +_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.call_graph_analyzer import refresh_call_graph_if_stale # noqa: E402 + +CACHED = {"nodes": ["cached_node"], "call_graph_available": True} +FRESH = {"nodes": ["fresh_node"], "call_graph_available": True} + + +@pytest.fixture() +def not_stale_state(): + """State with a usable cache and no stale flag -- ADR-002's crash scenario.""" + return {"step4_pre_change_graph": dict(CACHED)} + + +def _run(state, tmp_path): + """Call the guard with snapshot_call_graph stubbed, returning (result, mock).""" + with patch("langgraph_engine.sdlc_pipeline.call_graph_analyzer.snapshot_call_graph") as mock_snap: + mock_snap.return_value = dict(FRESH) + result = refresh_call_graph_if_stale(state, str(tmp_path)) + return result, mock_snap + + +class TestTheOverrideBypassesTheStaleFlag: + """FORCE_GRAPH_REBUILD=1 rebuilds even when nothing marked the graph stale.""" + + def test_forced_rebuild_ignores_a_usable_cache(self, monkeypatch, not_stale_state, tmp_path): + monkeypatch.setenv("FORCE_GRAPH_REBUILD", "1") + result, mock_snap = _run(not_stale_state, tmp_path) + mock_snap.assert_called_once_with(str(tmp_path)) + assert result["nodes"] == ["fresh_node"], "the cache was returned despite the override" + + +class TestTheOverrideIsOffByDefault: + """The half that fails if the rebuild became unconditional.""" + + def test_unset_returns_the_cached_snapshot(self, monkeypatch, not_stale_state, tmp_path): + monkeypatch.delenv("FORCE_GRAPH_REBUILD", raising=False) + result, mock_snap = _run(not_stale_state, tmp_path) + mock_snap.assert_not_called() + assert result["nodes"] == ["cached_node"] + + def test_zero_returns_the_cached_snapshot(self, monkeypatch, not_stale_state, tmp_path): + """Documented default is 0, so 0 must behave exactly like unset.""" + monkeypatch.setenv("FORCE_GRAPH_REBUILD", "0") + result, mock_snap = _run(not_stale_state, tmp_path) + mock_snap.assert_not_called() + assert result["nodes"] == ["cached_node"] + + def test_only_the_literal_1_enables_it(self, monkeypatch, not_stale_state, tmp_path): + """Matches the ENABLE_* convention used across the engine: == "1", not truthiness. + + "true" is the value an operator is most likely to reach for by habit, so + it is worth pinning that it does NOT enable the override rather than + leaving the reader to infer it from the comparison. + """ + monkeypatch.setenv("FORCE_GRAPH_REBUILD", "true") + result, mock_snap = _run(not_stale_state, tmp_path) + mock_snap.assert_not_called() + assert result["nodes"] == ["cached_node"] + + +class TestTheOverrideDoesNotDisableTheStaleFlag: + """Setting it to 0 must not switch off ordinary stale-based rebuilding. + + RUNBOOK Option A used to read as though `FORCE_GRAPH_REBUILD=0` turned that + off and unsetting it turned it back on. It never did, and this pins that the + two paths are independent so the corrected wording cannot silently drift + back. + """ + + def test_stale_still_rebuilds_when_the_override_is_zero(self, monkeypatch, tmp_path): + monkeypatch.setenv("FORCE_GRAPH_REBUILD", "0") + state = {"call_graph_stale": True, "step4_pre_change_graph": dict(CACHED)} + result, mock_snap = _run(state, tmp_path) + mock_snap.assert_called_once_with(str(tmp_path)) + assert result["nodes"] == ["fresh_node"]