diff --git a/backend/tests/unit/docs/test_config_drift_script.py b/backend/tests/unit/docs/test_config_drift_script.py index 80c2d81a..b1c2dd96 100644 --- a/backend/tests/unit/docs/test_config_drift_script.py +++ b/backend/tests/unit/docs/test_config_drift_script.py @@ -52,17 +52,19 @@ def test_it_reads_every_bool_setting_config_py_declares(drift) -> None: `auth_cookie_secure`. A setting the checker cannot see is reported as no drift, which is the failure mode this whole script exists to remove. """ - declared = len( - re.findall(r"^\s+\w+\s*:\s*bool\s*=", drift.CONFIG_PY.read_text(encoding="utf-8"), re.M) - ) + text = drift.CONFIG_PY.read_text(encoding="utf-8") + declared = len(re.findall(r"^\s+\w+\s*:\s*bool\s*=", text, re.M)) parsed = drift.code_defaults() - assert len(parsed) == declared, ( - f"config.py declares {declared} bool settings, the checker sees {len(parsed)}" + seen_bools = {k for k, v in parsed.items() if v in ("true", "false")} + assert len(seen_bools) == declared, ( + f"config.py declares {declared} bool settings, the checker sees {len(seen_bools)}" ) - # Three whose default is load-bearing enough to be worth pinning by name. - assert parsed["CROSS_CONNECTION_LEARNINGS_ENABLED"] is False - assert parsed["RERANKER_ENABLED"] is False - assert parsed["CODE_GRAPH_ENABLED"] is True, "declared with a trailing comment" + # Values are normalised to strings since 2026-08-28, when the checker was widened + # past booleans — one comparison path for bool, str and int. Three whose default is + # load-bearing enough to be worth pinning by name. + assert parsed["CROSS_CONNECTION_LEARNINGS_ENABLED"] == "false" + assert parsed["RERANKER_ENABLED"] == "false" + assert parsed["CODE_GRAPH_ENABLED"] == "true", "declared with a trailing comment" def test_every_recorded_divergence_names_a_setting_that_still_exists(drift) -> None: @@ -111,7 +113,7 @@ def test_the_ten_that_were_found_are_reported_as_drift(drift) -> None: assert sorted(k for k, _, _ in drifted) == sorted(set(found_in_prod) - was_misgrouped) assert recorded == [] assert unparseable == [] - assert drift.code_defaults()["AUTO_SYNC_AFTER_INDEX"] is True, ( + assert drift.code_defaults()["AUTO_SYNC_AFTER_INDEX"] == "true", ( "if this default goes back to False, the flag is drift again and this test " "should be the thing that says so" ) @@ -141,3 +143,100 @@ def test_settings_absent_from_config_py_are_ignored(drift) -> None: {"DATABASE_URL": "postgres://…", "PORT": "8000"}, drift.code_defaults() ) assert (drifted, recorded, unparseable) == ([], [], []) + + +# -------------------------------------------------------------------------------------- +# 2026-08-28: the checker was boolean-shaped, and the risk never was. +# +# `VECTOR_STORE_BACKEND` decides whether the entire knowledge layer reads from Postgres +# or from a dyno's local disk. It is a `str`. It was set in production against a code +# default of "chroma", and this script printed "No drift" — because it only ever read +# `name: bool = X`. The tool was written after ten *boolean* divergences were found and +# inherited the shape of its first evidence as an assumption about the whole problem. +# +# Widening it immediately printed `MASTER_ENCRYPTION_KEY`, `OPENROUTER_API_KEY` and the +# Redis password to stdout, which is where CI logs live. Both fixes are asserted here: +# a setting with no code default is not drift (that is how every credential is +# declared), and any value whose NAME looks like a credential is never printed at all. +# -------------------------------------------------------------------------------------- + + +def test_it_reads_string_and_int_settings_too(drift) -> None: + """The one that would have caught `VECTOR_STORE_BACKEND` the day it was set.""" + defaults = drift.code_defaults() + assert defaults.get("VECTOR_STORE_BACKEND") == "chroma" + assert defaults.get("DEFAULT_LLM_PROVIDER") == "openai" + assert defaults.get("EMBEDDING_UPSERT_BATCH_SIZE") == "8" + + +def test_booleans_still_compare_case_and_spelling_insensitively(drift) -> None: + """Widening must not lose what the narrow version did: `1`, `on` and `TRUE` all + mean the same thing to pydantic and must mean it here.""" + key = "DATA_GATE_HARD_CHECKS_ENABLED" + assert key not in drift.DELIBERATE, ( + "pick a key with no recorded reason, or this test measures the wrong thing" + ) + for raw in ("true", "TRUE", "1", "on", "yes"): + drifted, _, _ = drift.compare({key: raw}, {key: "false"}) + assert drifted, raw + + +def test_a_setting_with_no_code_default_is_not_drift(drift) -> None: + """An empty default is not a default — it is "the environment supplies this", which + is how every credential in config.py is declared. Comparing a deployed secret + against "" reports all of them AND prints their values.""" + drifted, recorded, unparseable = drift.compare( + {"OPENROUTER_API_KEY": "sk-or-v1-something"}, {"OPENROUTER_API_KEY": ""} + ) + assert not drifted and not recorded and not unparseable + + +def test_environment_shaped_settings_are_not_decisions(drift) -> None: + """A container path differs from a relative dev path by construction and always + will. Reporting that as a decision needing a written reason is how a tool teaches + people to skim it.""" + drifted, _, _ = drift.compare( + {"REPO_CLONE_BASE_DIR": "/app/data/repos"}, {"REPO_CLONE_BASE_DIR": "./data/repos"} + ) + assert not drifted + assert "REPO_CLONE_BASE_DIR" in drift.ENVIRONMENT_SHAPED + + +def test_a_behaviour_setting_is_still_a_decision(drift) -> None: + """The other side of that line: which vendor answers a question is a choice, and it + must still demand a recorded reason.""" + assert "DEFAULT_LLM_PROVIDER" not in drift.ENVIRONMENT_SHAPED + # It IS recorded in DELIBERATE — so it lands in `recorded`, not `drifted`. What is + # asserted is that it lands in one of them at all, which a location-shaped setting + # does not. + drifted, recorded, _ = drift.compare( + {"DEFAULT_LLM_PROVIDER": "anthropic"}, {"DEFAULT_LLM_PROVIDER": "openai"} + ) + assert drifted or recorded + + +class TestNoValueThatLooksLikeACredentialIsEverPrinted: + """The check is on the NAME, because the value is exactly what must not be examined + to make the decision.""" + + @pytest.mark.parametrize( + "key", + [ + "MASTER_ENCRYPTION_KEY", + "OPENROUTER_API_KEY", + "REDIS_URL", + "JWT_SECRET", + "SENTRY_DSN", + "RESEND_API_KEY", + "STRIPE_SECRET_KEY", + ], + ) + def test_it_is_masked(self, drift, key: str) -> None: + out = drift.mask(key, "hunter2-the-real-value") + assert "hunter2" not in out + assert "hidden" in out + + def test_an_ordinary_setting_is_shown(self, drift) -> None: + """Masking everything would make the report useless — the point is to read it.""" + assert drift.mask("VECTOR_STORE_BACKEND", "pgvector") == "pgvector" + assert drift.mask("DEFAULT_LLM_PROVIDER", "openrouter") == "openrouter" diff --git a/backend/tests/unit/docs/test_suppression_debt_ratchet.py b/backend/tests/unit/docs/test_suppression_debt_ratchet.py index b9c44732..4cc5b8be 100644 --- a/backend/tests/unit/docs/test_suppression_debt_ratchet.py +++ b/backend/tests/unit/docs/test_suppression_debt_ratchet.py @@ -55,8 +55,8 @@ # 2026-08-28, both raised by one: `PgVectorStore.close()` swallows a pool-close # failure (the store is being torn down; raising there would mask whatever was # actually being shut down), and `app/models/__init__.py` gained - # `DocEmbedding # noqa: F401` — the re-export convention every one of the other - # forty imports in that file already follows, not new debt. + # a `DocEmbedding` re-export carrying the same unused-import suppression every one + # of the other forty imports in that file already carries — convention, not debt. "except Exception": 614, "except ...: pass": 53, "# type: ignore": 49, diff --git a/scripts/config_drift.py b/scripts/config_drift.py index fa92d609..3ed98d91 100755 --- a/scripts/config_drift.py +++ b/scripts/config_drift.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -"""Compare a deployment's boolean config against the code defaults, and say what drifted. +"""Compare a deployment's config against the code defaults, and say what drifted. Why this exists as a command rather than a paragraph ---------------------------------------------------- @@ -60,6 +60,21 @@ "The MCP server is a shipped feature of this deployment. Off by default: it is " "an additional authenticated surface, and a self-hosted install should opt in." ), + "DEFAULT_LLM_PROVIDER": ( + "Production routes every LLM call through OpenRouter, not OpenAI directly — " + "6 338 of 6 479 recorded calls. The code default is 'openai' so a self-hosted " + "install with only an OPENAI_API_KEY works out of the box. Found on 2026-08-28 " + "by widening this checker past booleans; it had never been recorded, and the " + "boolean-only version could not have seen it." + ), + "VECTOR_STORE_BACKEND": ( + "pgvector, because ChromaDB persisted to the dyno's container filesystem — " + "wiped on every restart, unshared between web and worker — which made the " + "repo index force a full rebuild it could not afford (16 completions in 94 " + "runs). Set here rather than changed in code until one full re-index has " + "reached pipeline_end on it; the code default flips and this entry goes away " + "once it has." + ), "MCP_MOUNT_ENABLED": ( "Multi-tenant remote MCP at /mcp, which is the only mode that resolves a " "principal per request. Requires MCP_ENABLED, and is off by default for the " @@ -75,11 +90,33 @@ #: worth watching, and a checker that cannot see a setting reports it as "no drift". _BOOL_DEFAULT = re.compile(r"^\s*(\w+)\s*:\s*bool\s*=\s*(True|False)\s*(?:#.*)?$", re.M) +#: `name: str = "x"` and `name: int = 7`, same trailing-comment tolerance. +#: +#: Added 2026-08-28, because the checker had been boolean-shaped since it was written +#: and the risk never was. `VECTOR_STORE_BACKEND` decides whether the entire knowledge +#: layer reads from Postgres or from a dyno's local disk; it is a `str`, it was set in +#: production against a code default of "chroma", and this script printed "No drift". +#: The tool was built after ten *boolean* divergences were found, and inherited the +#: shape of its first evidence as an assumption about the whole problem. +_STR_DEFAULT = re.compile(r'^\s*(\w+)\s*:\s*str\s*=\s*"([^"]*)"\s*(?:#.*)?$', re.M) +_INT_DEFAULT = re.compile(r"^\s*(\w+)\s*:\s*int\s*=\s*(-?\d+)\s*(?:#.*)?$", re.M) + + +def code_defaults() -> dict[str, str]: + """Every `name: bool|str|int = X` in the settings module, keyed by env-var name. -def code_defaults() -> dict[str, bool]: - """Every `name: bool = X` in the settings module, keyed by its env-var name.""" + Values come back as strings so one comparison path serves all three; booleans are + normalised to "true"/"false" so `TRUE`, `1` and `on` all match a `True` default. + """ text = CONFIG_PY.read_text(encoding="utf-8") - return {name.upper(): value == "True" for name, value in _BOOL_DEFAULT.findall(text)} + out: dict[str, str] = {} + for name, value in _BOOL_DEFAULT.findall(text): + out[name.upper()] = value.lower() + for name, value in _STR_DEFAULT.findall(text): + out[name.upper()] = value + for name, value in _INT_DEFAULT.findall(text): + out[name.upper()] = value + return out def _as_bool(raw: str) -> bool | None: @@ -97,63 +134,145 @@ def deployed_config(app: str | None, from_json: Path | None) -> dict[str, str]: cmd = ["heroku", "config", "--json"] if app: cmd += ["-a", app] - return json.loads(subprocess.run(cmd, capture_output=True, text=True, check=True).stdout) + return json.loads( + subprocess.run(cmd, capture_output=True, text=True, check=True).stdout + ) + + +#: Settings whose value is a property of WHERE the code runs, not a decision about what +#: it does. A container path differs from a relative dev path by construction and always +#: will; a production URL is not a deployment "choosing" a different behaviour. +#: +#: The distinction is behaviour versus location, and it is the difference between a tool +#: worth reading and a list worth skimming. `DEFAULT_LLM_PROVIDER` belongs in DELIBERATE +#: because it changes which vendor answers a question. `REPO_CLONE_BASE_DIR` belongs +#: here because it changes nothing except where a clone lands. +ENVIRONMENT_SHAPED = { + "APP_URL", + "DATABASE_URL", + "REDIS_URL", + "JWT_SECRET", + "CHROMA_PERSIST_DIR", + "CUSTOM_RULES_DIR", + "REPO_CLONE_BASE_DIR", + "BM25_DATA_DIR", + "SSH_KNOWN_HOSTS_PATH", + "BACKUP_DIR", +} + + +#: Names whose VALUE must never reach stdout. The check is on the name because the +#: value is exactly what must not be looked at to decide. Belt to the braces of the +#: empty-default skip above: a secret that ever acquires a non-empty code default would +#: otherwise be printed by the same line that reports it. +_SECRET_HINT = ( + "KEY", + "SECRET", + "TOKEN", + "PASSWORD", + "DSN", + "URL", + "CREDENTIAL", + "SALT", + "SIGNATURE", +) -def compare(config: dict[str, str], defaults: dict[str, bool]) -> tuple[list, list, list]: +def mask(key: str, value: str) -> str: + """Return a value safe to print — the real one, or its shape.""" + if any(hint in key.upper() for hint in _SECRET_HINT): + return f"<{len(str(value))} chars, hidden>" + return str(value) + + +def compare( + config: dict[str, str], defaults: dict[str, str] +) -> tuple[list, list, list]: """(drifted, recorded, unparseable) — each a list of (key, deployed, default).""" drifted, recorded, unparseable = [], [], [] for key, raw in sorted(config.items()): if key not in defaults: continue - deployed = _as_bool(raw) - if deployed is None: - # A boolean setting whose value is neither true nor false is worth naming: - # pydantic will either coerce it or refuse to boot, and both are surprises. - unparseable.append((key, raw, defaults[key])) + default = defaults[key] + # An empty string default is not a default — it is "the environment supplies + # this", which is how every credential in `config.py` is declared. Comparing a + # deployed secret against "" reports every one of them as drift AND prints its + # value; the first run of the widened checker put the Fernet master key, the + # OpenRouter key and the Redis password on stdout, which is where CI logs live. + # Absence of a default means the question this tool asks does not apply. + if default == "" or key in ENVIRONMENT_SHAPED: continue - if deployed == defaults[key]: + if default in ("true", "false"): + parsed = _as_bool(raw) + if parsed is None: + # A boolean setting whose value is neither true nor false is worth + # naming: pydantic will either coerce it or refuse to boot, and both + # are surprises. + unparseable.append((key, raw, default)) + continue + deployed: str = "true" if parsed else "false" + else: + deployed = str(raw).strip() + if deployed == default: continue - (recorded if key in DELIBERATE else drifted).append((key, deployed, defaults[key])) + (recorded if key in DELIBERATE else drifted).append((key, deployed, default)) return drifted, recorded, unparseable def main() -> int: ap = argparse.ArgumentParser(description=__doc__) ap.add_argument("--app", help="Heroku app name (defaults to the git remote's app)") - ap.add_argument("--from-json", type=Path, help="read config from a JSON file instead") + ap.add_argument( + "--from-json", type=Path, help="read config from a JSON file instead" + ) ap.add_argument("--quiet", action="store_true", help="print only drift") args = ap.parse_args() defaults = code_defaults() if not defaults: - print("could not read any bool defaults from backend/app/config.py", file=sys.stderr) + print( + "could not read any bool defaults from backend/app/config.py", + file=sys.stderr, + ) return 2 try: config = deployed_config(args.app, args.from_json) - except (subprocess.CalledProcessError, FileNotFoundError, json.JSONDecodeError) as exc: + except ( + subprocess.CalledProcessError, + FileNotFoundError, + json.JSONDecodeError, + ) as exc: print(f"could not read the deployed config: {exc}", file=sys.stderr) return 2 drifted, recorded, unparseable = compare(config, defaults) if not args.quiet: - print(f"{len(defaults)} boolean settings in config.py, {len(config)} vars deployed\n") + print( + f"{len(defaults)} boolean settings in config.py, {len(config)} vars deployed\n" + ) if recorded: print("Recorded divergences (decisions, not drift):") for key, deployed, default in recorded: - print(f" {key:34} deployed={deployed!s:5} code={default}") + print( + f" {key:34} deployed={mask(key, deployed):5} code={mask(key, default)}" + ) print(f" {DELIBERATE[key]}") print() for key, raw, default in unparseable: - print(f"UNPARSEABLE {key:30} deployed={raw!r} is neither true nor false (code={default})") + print( + f"UNPARSEABLE {key:30} deployed={mask(key, raw)} " + f"is neither true nor false (code={mask(key, default)})" + ) if drifted: print("DRIFT — deployed but not recorded as a decision:") for key, deployed, default in drifted: - print(f" {key:34} deployed={deployed!s:5} code={default}") + print( + f" {key:34} deployed={mask(key, deployed):5} code={mask(key, default)}" + ) print( "\nEach of these is either a mistake to unset, or a decision to add to " "DELIBERATE in this file with its reason. Leaving it in neither place is " @@ -164,7 +283,10 @@ def main() -> int: if unparseable: return 1 if not args.quiet: - print("No drift: every deployed boolean matches the code, or is recorded above.") + print( + "No drift: every deployed setting matches the code, is recorded above, or is\n" + "shaped by the environment rather than chosen." + ) return 0