diff --git a/CLAUDE.md b/CLAUDE.md index 965fa7c1..6fa6ce26 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/); 492 repo-wide | -| **Test Files** | 104 (95 unit, 5 integration, 3 e2e, 1 load) | +| **Total Python Files** | 248 (langgraph_engine/); 493 repo-wide | +| **Test Files** | 105 (96 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 99bcdf92..9a0585b3 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-104%20files%20·%205%20integration-brightgreen)](tests/) +[![Tests](https://img.shields.io/badge/Tests-105%20files%20·%205%20integration-brightgreen)](tests/) [![Discussions](https://img.shields.io/badge/Discussions-GitHub-blueviolet)](../../discussions) --- @@ -469,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/ # 104 test_*.py files (125 total Python files) -│ ├── test_*.py # 95 unit tests +├── tests/ # 105 test_*.py files (126 total Python files) +│ ├── test_*.py # 96 unit tests │ ├── integration/ # 5 integration tests (GitHub, MCP, runtime verification) │ ├── e2e/ # 3 end-to-end scenario tests │ └── load/ # 1 concurrency / load test @@ -628,11 +628,11 @@ ENABLE_HEALTH_SERVER=1 ENABLE_METRICS=1 LOG_FORMAT=json \ ## Testing -104 test_*.py files (125 total Python files in `tests/` including conftest and `__init__` files): +105 test_*.py files (126 total Python files in `tests/` including conftest and `__init__` files): | Category | Files | Notes | |---|:---:|---| -| Unit tests | 95 | In `tests/test_*.py` — no external dependencies required | +| Unit tests | 96 | 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/core/config_loader.py b/langgraph_engine/core/config_loader.py index 7fd63c2d..7e60aa73 100644 --- a/langgraph_engine/core/config_loader.py +++ b/langgraph_engine/core/config_loader.py @@ -100,12 +100,54 @@ def _flatten(data: dict, prefix: str = "") -> dict[str, str]: return out +# Values that mean "you were supposed to fill this in". The shipped +# workflow-config.json carries them, so an untouched config injects them exactly +# as if they were real credentials. +# +# This is deliberately narrow. A false positive here rejects a working secret and +# breaks a setup that was fine, which is worse than the bug it guards against, so +# nothing is matched on length, entropy or character class -- only on shapes no +# real credential has. A real GitHub token starts ghp_/gho_/github_pat_ and a real +# API key is not spelled CHANGEME. +_PLACEHOLDER_PREFIXES = ("your_", "your-", "my_", "replace_", "replaceme", "changeme", "change_me", "todo", "xxx") + + +def _is_placeholder(value: str) -> bool: + """Return True when a config value is an unfilled template, not a secret. + + Args: + value: The raw value read from workflow-config.json. + + Returns: + bool: True if the value should be treated as absent. + """ + if not isinstance(value, str): + return False + stripped = value.strip() + if not stripped: + return True + if stripped.startswith("<") and stripped.endswith(">"): + return True + return stripped.lower().startswith(_PLACEHOLDER_PREFIXES) + + def load_workflow_config(path: Path | None = None) -> dict[str, str]: """Load workflow-config.json and inject into os.environ (no-override). Covers any remaining os.environ.get() call sites that have not yet been migrated to use get_section() directly. + Unfilled template values are skipped rather than injected. The reason is + specific and was expensive: the shipped config carries + ``"github": {"token": "YOUR_GITHUB_TOKEN"}``, this function injected it into + GITHUB_TOKEN, and that one string then defeated BOTH GitHub backends at once + -- the MCP client authenticated with it and got 401 Bad credentials, and the + gh CLI fallback also lost, because gh prefers an environment token over its + own keyring and reported itself unauthenticated. `gh auth login` therefore + appeared to do nothing, and Step 2 fell back to "no GitHub backend available" + on every run. Injecting nothing leaves the keyring reachable, which is the + working path. + Returns a dict of env-var-name -> value pairs that were injected. """ raw = _load_raw(path) @@ -114,10 +156,27 @@ def load_workflow_config(path: Path | None = None) -> dict[str, str]: flat = _flatten(raw) injected: dict[str, str] = {} + skipped: list[str] = [] for json_key, env_key in _MAPPING.items(): - if json_key in flat and env_key not in os.environ: - os.environ[env_key] = flat[json_key] - injected[env_key] = flat[json_key] + if json_key not in flat or env_key in os.environ: + continue + value = flat[json_key] + if _is_placeholder(value): + skipped.append(json_key) + continue + os.environ[env_key] = value + injected[env_key] = value + + # Said out loud, because a value silently ignored is indistinguishable from a + # value that was never read -- and the reader is someone wondering why their + # config has no effect. + if skipped: + import sys # noqa: PLC0415 + + print( + "[config_loader] ignored %d unfilled placeholder value(s): %s" % (len(skipped), ", ".join(sorted(skipped))), + file=sys.stderr, + ) return injected diff --git a/tests/test_config_loader_placeholders.py b/tests/test_config_loader_placeholders.py new file mode 100644 index 00000000..d7e5eba9 --- /dev/null +++ b/tests/test_config_loader_placeholders.py @@ -0,0 +1,156 @@ +"""An unfilled config template must not be injected as if it were a credential. + +The shipped workflow-config.json carries `"github": {"token": "YOUR_GITHUB_TOKEN"}`. +load_workflow_config injected that string into GITHUB_TOKEN, and one placeholder +then defeated BOTH GitHub backends at once: the MCP client authenticated with it +and got 401 Bad credentials, and the gh CLI fallback also lost, because gh prefers +an environment token over its own keyring and reported itself unauthenticated. + +`gh auth login` therefore appeared to do nothing. Step 2 fell back to "no GitHub +backend available" on every run, and the pipeline could not create an issue or a +branch on a machine that was, by every other measure, correctly authenticated. +Injecting nothing leaves the keyring reachable, which is the working path. + +The predicate is deliberately narrow, and half of these tests exist to hold it +that way. A false positive rejects a real secret and breaks a setup that was +working -- worse than the bug it guards against -- so nothing is matched on +length, entropy or character class. +""" + +import json +import sys +from pathlib import Path + +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.core import config_loader # noqa: E402 +from langgraph_engine.core.config_loader import _is_placeholder, load_workflow_config # noqa: E402 + + +@pytest.fixture(autouse=True) +def _clean_env(monkeypatch): + """Reset the env vars and the module's config cache before each test. + + _load_raw memoises into _CONFIG_CACHE on first read, after which the `path` + argument is silently ignored. Without this reset the first test to run would + pin the real ~/.claude/workflow-config.json for every test after it, and the + suite would be asserting against the developer's own machine -- which is how + a green suite ends up proving nothing. + """ + for var in ("GITHUB_TOKEN", "GITHUB_DEFAULT_LABEL", "CLAUDE_GITHUB_OWNER", "JIRA_API_TOKEN"): + monkeypatch.delenv(var, raising=False) + monkeypatch.setattr(config_loader, "_CONFIG_CACHE", None) + + +def _write_config(tmp_path, payload): + """Write a workflow-config.json and return its path.""" + path = tmp_path / "workflow-config.json" + path.write_text(json.dumps(payload), encoding="utf-8") + return path + + +class TestThePlaceholderIsNotInjected: + """The exact failure that cost a day.""" + + def test_the_shipped_github_placeholder_is_skipped(self, tmp_path): + path = _write_config(tmp_path, {"github": {"token": "YOUR_GITHUB_TOKEN"}}) + injected = load_workflow_config(path) + assert "GITHUB_TOKEN" not in injected + assert "GITHUB_TOKEN" not in __import__("os").environ + + def test_a_real_looking_token_is_still_injected(self, tmp_path): + """The half that fails if the predicate became greedy. + + Without this, a predicate that rejected everything would satisfy the test + above -- and would break every correctly configured machine instead. + """ + path = _write_config(tmp_path, {"github": {"token": "ghp_16CharsOrMoreOfRealLookingSecret"}}) + injected = load_workflow_config(path) + assert injected.get("GITHUB_TOKEN") == "ghp_16CharsOrMoreOfRealLookingSecret" + + def test_other_real_values_survive_alongside_a_skipped_one(self, tmp_path): + """One placeholder must not suppress the rest of the file.""" + path = _write_config( + tmp_path, + {"github": {"token": "YOUR_GITHUB_TOKEN", "owner": "techdeveloper-org", "default_label": "feature"}}, + ) + injected = load_workflow_config(path) + assert "GITHUB_TOKEN" not in injected + assert injected.get("CLAUDE_GITHUB_OWNER") == "techdeveloper-org" + assert injected.get("GITHUB_DEFAULT_LABEL") == "feature" + + +class TestTheSkipIsAnnounced: + """A value silently ignored is indistinguishable from one never read.""" + + def test_the_skip_is_reported(self, tmp_path, capsys): + path = _write_config(tmp_path, {"github": {"token": "YOUR_GITHUB_TOKEN"}}) + load_workflow_config(path) + assert "github.token" in capsys.readouterr().err + + def test_nothing_is_reported_when_nothing_was_skipped(self, tmp_path, capsys): + """The half that fails if the notice were unconditional.""" + path = _write_config(tmp_path, {"github": {"owner": "techdeveloper-org"}}) + load_workflow_config(path) + assert "config_loader" not in capsys.readouterr().err + + +class TestThePredicateIsNarrow: + """A false positive breaks a working setup, so the shapes are enumerated.""" + + @pytest.mark.parametrize( + "value", + [ + "YOUR_GITHUB_TOKEN", + "your-api-key", + "", + "CHANGEME", + "change_me", + "REPLACE_ME", + "TODO", + "xxxxxxxx", + "", + " ", + ], + ) + def test_template_shapes_are_placeholders(self, value): + assert _is_placeholder(value) is True + + @pytest.mark.parametrize( + "value", + [ + "ghp_realLookingToken1234567890", + "gho_anotherRealShape", + "github_pat_11ABCDEF", + "techdeveloper-org", + "feature", + "sk-ant-api03-abcdef", + "yourcompany-prod-key", # starts with "your" but not "your_" or "your-" + "0123456789abcdef", + ], + ) + def test_real_values_are_not_placeholders(self, value): + assert _is_placeholder(value) is False + + def test_a_non_string_is_not_a_placeholder(self): + """Config values may be bools or numbers; those are not templates.""" + assert _is_placeholder(True) is False + assert _is_placeholder(0) is False + + +class TestExistingBehaviourIsUnchanged: + """The no-override rule and missing-file handling must not have moved.""" + + def test_an_existing_env_var_still_wins(self, tmp_path, monkeypatch): + monkeypatch.setenv("CLAUDE_GITHUB_OWNER", "already-set") + path = _write_config(tmp_path, {"github": {"owner": "from-config"}}) + injected = load_workflow_config(path) + assert "CLAUDE_GITHUB_OWNER" not in injected + assert __import__("os").environ["CLAUDE_GITHUB_OWNER"] == "already-set" + + def test_a_missing_file_returns_empty(self, tmp_path): + assert load_workflow_config(tmp_path / "absent.json") == {}