From 7cd4a37acb7437dd98b2bef179d8ce0e2389388e Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 03:32:09 -0400 Subject: [PATCH 1/9] docs(specs): state the rate-limit window and env key-identity requirements Two auth defects surfaced while classifying the 96 mutants that keep serving/api/auth/manager.py two points above its mutation threshold: - load() carries in-memory rate-limit windows over by plaintext key, but since audit S-6 every window is named kid:/kh:, so every reload -- SIGHUP or the one that ends each key create, rotate and revoke -- empties them. - AGENTFLOW_API_KEYS entries get a random key_id on every load, so their Redis bucket, usage rows and admin views change per reload, restart and replica. The requirements land first, on their own, so the fixes that follow are reviewed against a statement that predates them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- docs/specs/api-key-identity.md | 41 +++++++++++++++++++++++++++ docs/specs/api-key-rate-limiting.md | 43 +++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+) create mode 100644 docs/specs/api-key-identity.md create mode 100644 docs/specs/api-key-rate-limiting.md diff --git a/docs/specs/api-key-identity.md b/docs/specs/api-key-identity.md new file mode 100644 index 00000000..c2261a09 --- /dev/null +++ b/docs/specs/api-key-identity.md @@ -0,0 +1,41 @@ +# Capability: api-key-identity + +A key's `key_id` names its rate-limit bucket (`kid:`), its usage rows +(`api_usage.key_id`) and the admin views addressed by id. An id that changes +while the key stays the same splits all three. + +## Requirement: an environment-configured key has a stable id +A key configured through `AGENTFLOW_API_KEYS` SHALL get a `key_id` derived from +the key itself, so the same key has the same id on every load, every restart and +every replica that runs with the same key-lookup pepper. + +### Scenario: same key, two managers +- **GIVEN** `AGENTFLOW_API_KEYS="k1:Support Agent"` and a fixed key-lookup pepper +- **WHEN** two separate `AuthManager` instances load their keys +- **THEN** both give the key the same `key_id`, of the form `default-support-agent-<8 lowercase hex>` + +### Scenario: a reload keeps the id and the bucket +- **GIVEN** a loaded environment-configured key +- **WHEN** the manager reloads +- **THEN** the key's `key_id` is unchanged, and so is its rate-limit bucket + +### Scenario: different keys under one name get different ids +- **GIVEN** `AGENTFLOW_API_KEYS="k1:bot,k2:bot"` +- **WHEN** the manager loads its keys +- **THEN** the two keys have different `key_id` values + +## Requirement: the id never exposes the key +The derived id SHALL come from the peppered key-lookup digest +(`compute_key_lookup`), never from the plaintext key or an unpeppered hash of +it: the id is written to logs, Redis key names, usage rows and admin responses, +and must not let a guessed key be confirmed offline. + +### Scenario: the suffix is the lookup digest's prefix +- **GIVEN** an environment key `k1` and key-lookup pepper `P` +- **WHEN** the manager loads it +- **THEN** the `key_id` suffix equals the first 8 characters of `compute_key_lookup("k1", P)` + +### Scenario: a different pepper gives a different id +- **GIVEN** the same environment key and two different key-lookup peppers +- **WHEN** one manager loads it under each pepper +- **THEN** the two `key_id` values differ diff --git a/docs/specs/api-key-rate-limiting.md b/docs/specs/api-key-rate-limiting.md new file mode 100644 index 00000000..1c766da8 --- /dev/null +++ b/docs/specs/api-key-rate-limiting.md @@ -0,0 +1,43 @@ +# Capability: api-key-rate-limiting + +Per-key request budgets enforced by `AuthManager` +(`src/agentflow_runtime/serving/api/auth/manager.py`). The budget shared across +replicas lives in Redis behind `RateLimiter`. The manager also keeps an +in-memory window per bucket: it is the whole of `is_rate_limited()`, and it is +the secondary check in `check_rate_limit()` when the Redis limiter answers +"allowed, full quota" while a Redis handle is live. + +## Requirement: a bucket is named by non-secret key identity +The rate-limit bucket of a key SHALL be named `kid:` when the key has an +id, and no bucket name SHALL contain a plaintext API key. + +### Scenario: key with an id +- **GIVEN** a key whose `key_id` is `acme-support-1a2b3c4d` +- **WHEN** a request authenticated by that key is rate-limited +- **THEN** its bucket is `kid:acme-support-1a2b3c4d` + +## Requirement: reloading the key store keeps live windows +Reloading the key configuration — `load()`, a SIGHUP reload, or the reload that +ends every key create, rotate and revoke — SHALL keep the in-memory rate-limit +window of every key that is still configured, and SHALL drop the window of a +key the reload removed. + +### Scenario: a full window survives a reload +- **GIVEN** a key with `rate_limit_rpm: 1` that has already made its one request in the current window +- **WHEN** the key store is reloaded and the same key makes another request inside that window +- **THEN** that request is rate-limited + +### Scenario: the secondary window survives a reload +- **GIVEN** a Redis limiter that answers "allowed, full quota" with a live handle, and a key with `rate_limit_rpm: 1` that has made its one request +- **WHEN** the key store is reloaded and `check_rate_limit()` is called for the same key inside the window +- **THEN** the answer is not allowed, with zero remaining + +### Scenario: a removed key's window is dropped +- **GIVEN** two configured keys that both made a request in the current window +- **WHEN** one of them is removed from the key file and the store is reloaded +- **THEN** the in-memory windows hold the remaining key's bucket and not the removed key's + +### Scenario: no plaintext key becomes a window name +- **GIVEN** a plaintext key configured in the key file, which has made a request +- **WHEN** the store is reloaded +- **THEN** no in-memory window is named by that plaintext key, at any point of the reload From 418958930f8a8bb037b2c06244129d307ddb979d Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 04:09:27 -0400 Subject: [PATCH 2/9] docs: link the capability specs from the pages that own their subject 7cd4a37 added docs/specs/ with two capability files and no inbound link, and the docs-orphan gate (tests/unit/test_docs_orphans.py) failed the full unit run on them. The security section of architecture.md now points its API authentication and rate-limiting bullets at their requirements, and the docs index names specs/ as the home of required behaviour. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- docs/README.md | 5 ++++- docs/architecture.md | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/README.md b/docs/README.md index 0b2e05ad..ac3c966f 100644 --- a/docs/README.md +++ b/docs/README.md @@ -154,6 +154,7 @@ the former mutable pre-Q1.2 report is preserved as the | Current engineering gates | [Engineering status](STATUS.md) | Dated acceptance/evidence records | | Lifecycle and non-goals | [Project closure](PROJECT_CLOSURE.md) | Audit and planning records | | Runtime design | [Architecture reference](architecture.md) | [Walkthrough](architecture/index.md) and ADRs | +| Required behaviour | One file per capability in `specs/`: [API key rate limiting](specs/api-key-rate-limiting.md), [API key identity](specs/api-key-identity.md) | The tests that exercise each scenario | | API contract | [`openapi.json`](openapi.json) and running FastAPI schema | API guide/reference and SDKs | | Security policy | [`SECURITY.md`](../SECURITY.md) | Security audit and dated remediation evidence | | Release history | [Changelog](../CHANGELOG.md) | [Archived narrative](archive/release-history-v1-v2.md) | @@ -166,7 +167,9 @@ the former mutable pre-Q1.2 report is preserved as the `scripts/check_docs_root_placement.py`; update it only for an intentional stable entrypoint or current reference. - Put immutable measurements in `perf/` or `evidence/`, operational procedures - in `operations/` or `runbooks/`, and decisions in `decisions/`. + in `operations/` or `runbooks/`, decisions in `decisions/`, and the required + behaviour of a capability (requirements with scenarios) in `specs/`, one + file per capability. - Do not delete documentation. Move superseded or duplicate narrative to `archive/` with its original path, archive date, reason, and replacement. - Update every inbound link in the same commit as a move. Use `git mv` so file diff --git a/docs/architecture.md b/docs/architecture.md index cd666709..e08390a9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -134,9 +134,9 @@ See [Architecture Decision Records](decisions/) for detailed trade-off analysis. ## Security ### Implemented -- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var) +- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var). Requirements for a key's id: [API key identity](specs/api-key-identity.md) - **Transport gate (audit P2-3)**: `AGENTFLOW_PROFILE=production` refuses to boot over plaintext transport to an external ClickHouse/Redis/PostgreSQL (loopback exempt; deliberate exceptions named in `AGENTFLOW_INSECURE_TRANSPORT_OK`), and refuses a wildcard CORS origin outside demo mode. The ClickHouse client supports HTTPS with hostname verification and a private-CA bundle (`CLICKHOUSE_SECURE`, `CLICKHOUSE_CA_CERT`) -- **Rate limiting**: Per-key sliding window with Redis backing when available and in-memory fallback for local/test, configurable via `AGENTFLOW_RATE_LIMIT_RPM` (default: 120/min) +- **Rate limiting**: Per-key sliding window with Redis backing when available and in-memory fallback for local/test, configurable via `AGENTFLOW_RATE_LIMIT_RPM` (default: 120/min). Requirements: [API key rate limiting](specs/api-key-rate-limiting.md) - **Health/docs exempt**: `/v1/health`, `/docs`, `/metrics` don't require auth - **No secrets in code**: All credentials via environment variables - **Terraform state**: Encrypted S3 backend with DynamoDB locking From a681cb5dcd08683ef7a6c631d76f85716781b612 Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 04:09:27 -0400 Subject: [PATCH 3/9] docs(specs): a key-file entry without a persisted id needs a stable id too The key-identity requirement covered only AGENTFLOW_API_KEYS. A key-file entry without a key_id gets a random id from ensure_key_ids on every load, and load() writes it back only when the file is writable. The shipped production compose mounts ./config read-only and config/api_keys.yaml carries no key_id, so both of its keys draw a new id -- and with it a new rate-limit bucket -- on every load, restart and replica. The requirement now covers every key without a persisted id, derives the id from the stored key_lookup when there is one, and leaves a legacy hash-only entry random. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- docs/specs/api-key-identity.md | 41 +++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/docs/specs/api-key-identity.md b/docs/specs/api-key-identity.md index c2261a09..f8679f08 100644 --- a/docs/specs/api-key-identity.md +++ b/docs/specs/api-key-identity.md @@ -4,12 +4,15 @@ A key's `key_id` names its rate-limit bucket (`kid:`), its usage rows (`api_usage.key_id`) and the admin views addressed by id. An id that changes while the key stays the same splits all three. -## Requirement: an environment-configured key has a stable id -A key configured through `AGENTFLOW_API_KEYS` SHALL get a `key_id` derived from -the key itself, so the same key has the same id on every load, every restart and -every replica that runs with the same key-lookup pepper. - -### Scenario: same key, two managers +## Requirement: a key without a persisted id has a stable id +A key whose configuration carries no `key_id` SHALL get a `key_id` derived from +the key's identity, so the same key has the same id on every load, every +restart and every replica that runs with the same key-lookup pepper. This +covers every key from `AGENTFLOW_API_KEYS`, and every key-file entry without a +`key_id` — including one in a key file the process cannot write the id back +to, as when the file is mounted read-only. + +### Scenario: same environment key, two managers - **GIVEN** `AGENTFLOW_API_KEYS="k1:Support Agent"` and a fixed key-lookup pepper - **WHEN** two separate `AuthManager` instances load their keys - **THEN** both give the key the same `key_id`, of the form `default-support-agent-<8 lowercase hex>` @@ -24,17 +27,35 @@ every replica that runs with the same key-lookup pepper. - **WHEN** the manager loads its keys - **THEN** the two keys have different `key_id` values +### Scenario: an id-less entry in a read-only key file +- **GIVEN** a key file the process cannot write, holding an entry that has a `key_lookup` and no `key_id` +- **WHEN** two separate `AuthManager` instances load it, and one of them reloads +- **THEN** the entry has the same `key_id` in both managers and after the reload + +### Scenario: a writable key file persists the derived id +- **GIVEN** a writable key file holding an entry that has a plaintext `key` and no `key_id` +- **WHEN** the manager loads it +- **THEN** the file on disk now carries the `key_id` the manager uses, and it is the id derived from that key + ## Requirement: the id never exposes the key -The derived id SHALL come from the peppered key-lookup digest -(`compute_key_lookup`), never from the plaintext key or an unpeppered hash of -it: the id is written to logs, Redis key names, usage rows and admin responses, -and must not let a guessed key be confirmed offline. +The derived id SHALL come from the peppered key-lookup digest — the entry's +stored `key_lookup`, or `compute_key_lookup` over its plaintext key — never +from the plaintext key or an unpeppered hash of it: the id is written to logs, +Redis key names, usage rows and admin responses, and must not let a guessed +key be confirmed offline. An entry that has neither a plaintext key nor a +`key_lookup` (a legacy hash-only entry) has nothing to derive from and keeps a +random id. ### Scenario: the suffix is the lookup digest's prefix - **GIVEN** an environment key `k1` and key-lookup pepper `P` - **WHEN** the manager loads it - **THEN** the `key_id` suffix equals the first 8 characters of `compute_key_lookup("k1", P)` +### Scenario: a stored key_lookup is used as it is +- **GIVEN** a key-file entry with `key_lookup: L` and no `key_id` +- **WHEN** the manager loads it +- **THEN** the `key_id` suffix equals the first 8 characters of `L` + ### Scenario: a different pepper gives a different id - **GIVEN** the same environment key and two different key-lookup peppers - **WHEN** one manager loads it under each pepper From 63022aad1da36c3e558cc378e47c9e2c3c0a8e9d Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 04:17:41 -0400 Subject: [PATCH 4/9] fix(auth): stop every key-store reload from emptying the rate-limit windows AuthManager.load() carried the in-memory rate-limit windows over by the plaintext key index (keys_by_value). Since audit S-6 every window is named by its bucket (kid:), so no window ever matched: SIGHUP, and the reload that ends each key create, rotate and revoke, emptied them all. is_rate_limited() and the in-memory secondary check in check_rate_limit() then handed every tenant a fresh budget -- during a Redis outage, the whole limit. load() now keeps the windows whose bucket belongs to a key this reload still configures, and drops the rest. The new tests take each scenario of docs/specs/api-key-rate-limiting.md against both readers of the window; four of the five failed before the fix. Environment keys still draw a random key_id on every load, so their windows keep resetting; the next commit derives that id from the key. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- CHANGELOG.md | 14 ++ .../serving/api/auth/manager.py | 10 +- tests/unit/test_auth_rate_window_reload.py | 150 ++++++++++++++++++ 3 files changed, 173 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_auth_rate_window_reload.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 03010cec..07974bce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,20 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] +### Fixed — reloading the key store no longer resets rate-limit windows (T-46) + +`AuthManager.load()` carried the in-memory rate-limit windows over by the +plaintext key index (`keys_by_value`), but since audit S-6 every window is +named by its bucket (`kid:`), so no window ever matched: every reload +— SIGHUP, and the reload that ends every key create, rotate and revoke — +emptied them all. `is_rate_limited()` and the in-memory secondary check in +`check_rate_limit()` then handed every tenant a fresh budget, which during a +Redis outage is the whole limit. Windows are now carried over by bucket name: +a still-configured key keeps its window, a removed key loses it, and no +plaintext key names a window at any point of the reload. Keys from +`AGENTFLOW_API_KEYS` still get a new random `key_id` on every load, so their +windows keep resetting until T-47. + ### Docs — the pre-push hedges outlived the push * **Several notes described work the owner "still has to do" that has since diff --git a/src/agentflow_runtime/serving/api/auth/manager.py b/src/agentflow_runtime/serving/api/auth/manager.py index 49ed09fc..e14c4c7f 100644 --- a/src/agentflow_runtime/serving/api/auth/manager.py +++ b/src/agentflow_runtime/serving/api/auth/manager.py @@ -328,9 +328,17 @@ def load(self) -> None: if item.previous_key_lookup is not None: self._previous_keys_by_lookup[item.previous_key_lookup] = item self._key_rotator.schedule_rotation_cleanup(item) + # Carry windows over by bucket name (`_rate_limit_key`), never by the + # plaintext `keys_by_value` index: a still-configured key keeps its + # window across the reload, a removed key's window is dropped. (T-46) + live_buckets = {self._rate_limit_key(item) for item in config.keys} self._rate_windows = defaultdict( list, - {key: self._rate_windows.get(key, []) for key in self.keys_by_value}, + { + bucket: window + for bucket, window in self._rate_windows.items() + if bucket in live_buckets + }, ) # H-C4: drop cached plaintext entries for hashes that no longer # exist after this reload (revoked/rotated keys). Without this the diff --git a/tests/unit/test_auth_rate_window_reload.py b/tests/unit/test_auth_rate_window_reload.py new file mode 100644 index 00000000..5bfe294e --- /dev/null +++ b/tests/unit/test_auth_rate_window_reload.py @@ -0,0 +1,150 @@ +"""Reloading the key store keeps the in-memory rate-limit windows +(docs/specs/api-key-rate-limiting.md, T-46). + +``AuthManager.load()`` runs on SIGHUP and at the end of every key create, +rotate and revoke. It used to rebuild ``_rate_windows`` from ``keys_by_value`` +-- the PLAINTEXT key index -- while every window is named by +``_rate_limit_key()`` (``kid:``), so each reload emptied every window +and handed every tenant a fresh budget. One test per spec scenario, against +both readers of the window: ``is_rate_limited()`` and the secondary check in +``check_rate_limit()``. + +Keys live in a tmp key file with an explicit ``key_id``: environment keys get +a random key_id on every load (until T-47), so they cannot pin a bucket here. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from agentflow_runtime.serving.api.auth.manager import AuthManager + +ALPHA_PLAIN = "plain-secret-alpha" +BETA_PLAIN = "plain-secret-beta" +ALPHA_ID = "acme-support-1a2b3c4d" +BETA_ID = "acme-billing-5e6f7a8b" + + +class _FrozenClock: + def __init__(self, now: float = 1_000.0) -> None: + self.now = now + + def __call__(self) -> float: + return self.now + + +class _FullRemainingLimiter: + """Redis limiter that answers "allowed, full quota" while its `_redis` + handle is live -- the condition for `check_rate_limit`'s in-memory + secondary window.""" + + def __init__(self) -> None: + self._redis = object() + + async def check(self, key: str, rpm: int) -> tuple[bool, int, int]: + return True, rpm, 0 + + +def _entry(key_id: str, plain: str, name: str) -> str: + return ( + f" - key_id: {key_id}\n" + f" key: {plain}\n" + f" name: {name}\n" + " tenant: acme\n" + " rate_limit_rpm: 1\n" + " created_at: '2026-04-10'\n" + ) + + +def _write_keys(path: Path, *entries: str) -> None: + path.write_text("keys:\n" + "".join(entries), encoding="utf-8") + + +def _manager(tmp_path: Path, *entries: str, **overrides: object) -> AuthManager: + api_keys_path = tmp_path / "api_keys.yaml" + _write_keys(api_keys_path, *entries) + params: dict[str, object] = { + "api_keys_path": api_keys_path, + "db_path": tmp_path / "usage.duckdb", + "time_source": _FrozenClock(), + } + params.update(overrides) + manager = AuthManager(**params) # type: ignore[arg-type] + manager.load() + return manager + + +def test_a_key_with_an_id_is_rate_limited_in_its_kid_bucket(tmp_path: Path) -> None: + manager = _manager(tmp_path, _entry(ALPHA_ID, ALPHA_PLAIN, "Support")) + + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is False + + assert set(manager._rate_windows) == {f"kid:{ALPHA_ID}"} + + +def test_a_full_window_survives_a_reload(tmp_path: Path) -> None: + manager = _manager(tmp_path, _entry(ALPHA_ID, ALPHA_PLAIN, "Support")) + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is False + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is True + + manager.load() + + # Same frozen instant: still inside the window the first request opened. + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is True + + +@pytest.mark.asyncio +async def test_the_secondary_window_survives_a_reload(tmp_path: Path) -> None: + manager = _manager( + tmp_path, + _entry(ALPHA_ID, ALPHA_PLAIN, "Support"), + rate_limiter=_FullRemainingLimiter(), + ) + allowed, remaining, _ = await manager.check_rate_limit(manager.keys_by_value[ALPHA_PLAIN]) + assert (allowed, remaining) == (True, 0) + + manager.load() + + allowed, remaining, _ = await manager.check_rate_limit(manager.keys_by_value[ALPHA_PLAIN]) + assert (allowed, remaining) == (False, 0) + + +def test_a_removed_keys_window_is_dropped(tmp_path: Path) -> None: + alpha = _entry(ALPHA_ID, ALPHA_PLAIN, "Support") + beta = _entry(BETA_ID, BETA_PLAIN, "Billing") + manager = _manager(tmp_path, alpha, beta) + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is False + assert manager.is_rate_limited(manager.keys_by_value[BETA_PLAIN]) is False + + _write_keys(tmp_path / "api_keys.yaml", alpha) + manager.load() + + assert f"kid:{ALPHA_ID}" in manager._rate_windows + assert f"kid:{BETA_ID}" not in manager._rate_windows + + +def test_no_plaintext_key_becomes_a_window_name( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + manager = _manager(tmp_path, _entry(ALPHA_ID, ALPHA_PLAIN, "Support")) + assert manager.is_rate_limited(manager.keys_by_value[ALPHA_PLAIN]) is False + + # load() rebuilds the windows and then sweeps them; the sweep is the one + # point inside the reload where the rebuilt dict is observable before it + # is trimmed, so snapshot the names there as well as after the reload. + names_seen: list[set[str]] = [] + sweep = manager._sweep_expired_windows + + def recording_sweep() -> None: + names_seen.append(set(manager._rate_windows)) + sweep() + + monkeypatch.setattr(manager, "_sweep_expired_windows", recording_sweep) + manager.load() + names_seen.append(set(manager._rate_windows)) + + assert len(names_seen) == 2 + for names in names_seen: + assert not any(ALPHA_PLAIN in name for name in names), names From 1582ed1f60ba2bd7a504d41112c466ce5b6163fb Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 06:09:52 -0400 Subject: [PATCH 5/9] test(property): expect the reader's cents, not Python's rounding of them test_an_aggregate_sums_only_the_readers_rows compared the revenue metric with round(amount, 2). But orders_v2.total_amount is DECIMAL(10,2): DuckDB casts a double into it half away from zero, while Python's round() rounds the exact binary value half to even. Hypothesis drew amount=8.125. The store held 8.13, round() said 8.12, and the 0.01 tolerance missed by float noise. Once the example database saved that case, the failure replayed on every run and turned a full local gate red. The test now asserts that the metric is the reader's own amount to the cent (abs 0.006). That holds under either rounding mode and stays far below the 1.0 minimum a leaked row would add. amount=8.125 is pinned with @example. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- tests/property/test_tenant_isolation_properties.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/property/test_tenant_isolation_properties.py b/tests/property/test_tenant_isolation_properties.py index 41d9286d..251d214c 100644 --- a/tests/property/test_tenant_isolation_properties.py +++ b/tests/property/test_tenant_isolation_properties.py @@ -23,7 +23,7 @@ from collections.abc import Iterator import pytest -from hypothesis import assume, given, settings +from hypothesis import assume, example, given, settings from hypothesis import strategies as st from agentflow_runtime.serving.semantic_layer.catalog import DataCatalog @@ -170,6 +170,7 @@ def test_the_tenant_column_never_reaches_the_payload( @settings(max_examples=40) @given(tenant=_TENANT_IDS, order_id=_ENTITY_IDS, amount=st.floats(1.0, 10_000.0, width=32)) +@example(tenant="0", order_id="ORD-0", amount=8.125) def test_an_aggregate_sums_only_the_readers_rows( engine: QueryEngine, tenant: str, order_id: str, amount: float ) -> None: @@ -184,7 +185,9 @@ def test_an_aggregate_sums_only_the_readers_rows( metric = engine.get_metric("revenue", window="24h", tenant_id=scoped_tenant) - assert metric["value"] == pytest.approx(round(amount, 2), abs=0.01) + # total_amount is DECIMAL(10,2), which DuckDB fills half away from zero while Python's round() + # is half-even on the binary value: the reader's own amount to the cent, a leaked row adds >= 1.0. + assert metric["value"] == pytest.approx(amount, abs=0.006) @given(hostile=_HOSTILE_TENANT_IDS) From 6231b731f778e8296256e72b052e8a2512cbec89 Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 06:09:52 -0400 Subject: [PATCH 6/9] fix(auth): give a key configured without a key_id the same id on every load A key whose configuration carries no key_id -- every key from AGENTFLOW_API_KEYS, and a key-file entry without one -- drew a random id from generate_key_id on every load. A key-file entry kept its id only when load() could write it back, and docker-compose.prod.yml mounts the key file read-only, over a config/api_keys.yaml whose two entries carry no key_id. The id names the key's Redis bucket, its api_usage rows and the admin views keyed by id: replicas sharing Redis each kept their own bucket for the same key (N replicas, N times its rpm), and every restart and reload started a fresh bucket and split its usage history. ensure_key_ids now derives the id -- --<8 hex of the entry's peppered lookup digest>, from its stored key_lookup or compute_key_lookup over its plaintext -- and _legacy_env_keys runs environment keys through it. A clash lengthens the digest prefix, so ids stay unique and stable. A new pepper changes only an id derived from plaintext and never written back -- every environment key, and a plaintext entry of a key file the process cannot write; an entry with a stored key_lookup, and a derived id already written to a writable key file, keep theirs. A legacy hash-only entry has nothing to derive from and keeps a random id; generate_key_id and create_key are unchanged. tests/unit/test_auth_key_identity.py takes each scenario of docs/specs/api-key-identity.md (nine of its twelve tests fail against the code before this change), with false-reject controls that load the shipped key file and a persisted id beside a hash-only entry. The new key_rotation helpers are killed from test_key_rotation_mutation.py, the file the mutation gate runs; its one test that pinned a random id for entries with a plaintext key now builds hash-only entries, the case that keeps one. The CHANGELOG's ten LF-only lines are now CRLF like the rest of the file. docs/architecture.md and the reload requirement of docs/specs/api-key-rate-limiting.md now name the one key a reload still cannot keep: a legacy hash-only entry in a key file the process cannot write, whose random id -- and so its bucket and window -- changes on every load. docs/specs/api-key-identity.md already specified that id. Reviewed by opus-cli, the writer's own engine: glm-5.3 was rate-limited until 2026-09-14 and codex until 2026-10-05. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HHUBaNNPWihtsgeFWnRmaS --- CHANGELOG.md | 52 +++- docs/architecture.md | 2 +- docs/specs/api-key-rate-limiting.md | 5 +- .../serving/api/auth/key_rotation.py | 49 ++- .../serving/api/auth/manager.py | 10 +- tests/unit/test_auth_key_identity.py | 280 ++++++++++++++++++ tests/unit/test_key_rotation_mutation.py | 106 ++++++- 7 files changed, 480 insertions(+), 24 deletions(-) create mode 100644 tests/unit/test_auth_key_identity.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 07974bce..6ec2b553 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,28 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] -### Fixed — reloading the key store no longer resets rate-limit windows (T-46) +### Fixed — a key configured without a key_id keeps the same id on every load + +A key whose configuration carries no `key_id` — every key from +`AGENTFLOW_API_KEYS`, and a key-file entry without one — drew a random id on +every `AuthManager.load()`. A key-file entry kept its id only when the id could +be written back, and `docker-compose.prod.yml` mounts the key file read-only. +The id names the key's Redis bucket (`kid:`), its `api_usage` rows and +the admin views keyed by id, so replicas sharing Redis each kept their own +bucket for the same key — N replicas, N times its rpm — and every restart and +reload started a fresh bucket and split the key's usage history. Such a key now +gets `--`: the stored +`key_lookup`, else the peppered `compute_key_lookup` of its plaintext, never +the plaintext or an unpeppered hash of it. The same key gets the same id on +every load, restart and replica that shares the pepper. Changing the pepper +changes only an id derived from a plaintext key and never written back: every +environment key, and a plaintext key-file entry in a file the process cannot +write. A writable key file keeps the id written to it on the first load, and an +entry with a stored `key_lookup` keeps the id derived from that digest. An id +already taken lengthens the digest prefix. A legacy hash-only entry, with +neither a plaintext key nor a `key_lookup`, still gets a random id. + +### Fixed — reloading the key store no longer resets rate-limit windows `AuthManager.load()` carried the in-memory rate-limit windows over by the plaintext key index (`keys_by_value`), but since audit S-6 every window is @@ -14,9 +35,12 @@ emptied them all. `is_rate_limited()` and the in-memory secondary check in `check_rate_limit()` then handed every tenant a fresh budget, which during a Redis outage is the whole limit. Windows are now carried over by bucket name: a still-configured key keeps its window, a removed key loses it, and no -plaintext key names a window at any point of the reload. Keys from -`AGENTFLOW_API_KEYS` still get a new random `key_id` on every load, so their -windows keep resetting until T-47. +plaintext key names a window at any point of the reload. A window survives +only while its key keeps its `key_id`, which a key configured without one — +from `AGENTFLOW_API_KEYS`, or in a key file the process cannot write — now does +too (entry above), with one exception: a legacy hash-only entry, with neither a +plaintext key nor a `key_lookup`, in a key file the process cannot write still +gets a new id, and so an emptied window, on every reload. ### Docs — the pre-push hedges outlived the push @@ -605,16 +629,16 @@ applies `--ignore` only for `safety_id` values whose waiver scope matches that bucket. Expired waivers stop suppressing findings. A waiver whose scope is not scanned, or a duplicate `safety_id`, fails closed. -### Documentation — root records archived (2026-09-02–2026-09-04) - -Twenty-one immutable tracked Markdown records moved from the repository root -to `docs/evidence/records/` with unchanged filenames and SHA-256 digests. -Living citations now use those paths. After all eight documentation-cleanup -items closed, `plan_26_08_2026.md` moved to `docs/archive/plans/` with its -closure evidence intact. The retired local 2026-04-17 benchmark baseline moved -from the standalone `docs/benchmark-baseline-archive/` directory into the -performance archive. - +### Documentation — root records archived (2026-09-02–2026-09-04) + +Twenty-one immutable tracked Markdown records moved from the repository root +to `docs/evidence/records/` with unchanged filenames and SHA-256 digests. +Living citations now use those paths. After all eight documentation-cleanup +items closed, `plan_26_08_2026.md` moved to `docs/archive/plans/` with its +closure evidence intact. The retired local 2026-04-17 benchmark baseline moved +from the standalone `docs/benchmark-baseline-archive/` directory into the +performance archive. + ### Security — nltk 3.10.0 -> 3.10.3 in uv.lock (Dependabot GHSA-m4rf-3fr8-xwx3, GHSA-6hwm-xvph-95vm) - `uv lock --upgrade-package nltk` only; nltk is a transitive dependency of `llama-index-core` and is not part of the `cloud`/`postgres` export, so `requirements-docker.lock` is unchanged. Closes the critical (JVM argument injection in the Stanford wrappers) and high (uncontrolled `dot` search path) advisories GitHub reported on the default branch on 2026-09-01. diff --git a/docs/architecture.md b/docs/architecture.md index e08390a9..ec0ffe99 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -134,7 +134,7 @@ See [Architecture Decision Records](decisions/) for detailed trade-off analysis. ## Security ### Implemented -- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var). Requirements for a key's id: [API key identity](specs/api-key-identity.md) +- **API authentication**: API key via `X-API-Key` header (set `AGENTFLOW_API_KEYS` env var). A key configured without a `key_id` (every environment key, and a key-file entry without one) gets an id derived from its key-lookup digest, stable across reloads, restarts and replicas (except a legacy hash-only entry, with neither a plaintext key nor a `key_lookup`: it has nothing to derive from and keeps a random id, which in a key file the process cannot write changes on every load); changing the pepper changes only an id derived from a plaintext key and never written back (every environment key, and a plaintext key-file entry in a file the process cannot write), while a writable key file keeps the id written to it on the first load and an entry with a stored `key_lookup` keeps the id derived from that digest. Requirements for a key's id: [API key identity](specs/api-key-identity.md) - **Transport gate (audit P2-3)**: `AGENTFLOW_PROFILE=production` refuses to boot over plaintext transport to an external ClickHouse/Redis/PostgreSQL (loopback exempt; deliberate exceptions named in `AGENTFLOW_INSECURE_TRANSPORT_OK`), and refuses a wildcard CORS origin outside demo mode. The ClickHouse client supports HTTPS with hostname verification and a private-CA bundle (`CLICKHOUSE_SECURE`, `CLICKHOUSE_CA_CERT`) - **Rate limiting**: Per-key sliding window with Redis backing when available and in-memory fallback for local/test, configurable via `AGENTFLOW_RATE_LIMIT_RPM` (default: 120/min). Requirements: [API key rate limiting](specs/api-key-rate-limiting.md) - **Health/docs exempt**: `/v1/health`, `/docs`, `/metrics` don't require auth diff --git a/docs/specs/api-key-rate-limiting.md b/docs/specs/api-key-rate-limiting.md index 1c766da8..a540c8b0 100644 --- a/docs/specs/api-key-rate-limiting.md +++ b/docs/specs/api-key-rate-limiting.md @@ -20,7 +20,10 @@ id, and no bucket name SHALL contain a plaintext API key. Reloading the key configuration — `load()`, a SIGHUP reload, or the reload that ends every key create, rotate and revoke — SHALL keep the in-memory rate-limit window of every key that is still configured, and SHALL drop the window of a -key the reload removed. +key the reload removed. A key whose id changes on every load, a legacy +hash-only entry in a key file the process cannot write (see +[API key identity](api-key-identity.md)), gets a new bucket on every reload, so +its window starts empty. ### Scenario: a full window survives a reload - **GIVEN** a key with `rate_limit_rpm: 1` that has already made its one request in the current window diff --git a/src/agentflow_runtime/serving/api/auth/key_rotation.py b/src/agentflow_runtime/serving/api/auth/key_rotation.py index 19e855b8..3016dbd6 100644 --- a/src/agentflow_runtime/serving/api/auth/key_rotation.py +++ b/src/agentflow_runtime/serving/api/auth/key_rotation.py @@ -4,6 +4,7 @@ import re import secrets import threading +from collections.abc import Collection from datetime import UTC, datetime, timedelta try: @@ -22,6 +23,42 @@ is_permission_denied, ) +# A derived key_id ends in this many characters of the key's lookup digest: the +# same shape as the 8 hex characters `generate_key_id` draws at random. +KEY_ID_DIGEST_CHARS = 8 + + +def key_id_slug(value: str, fallback: str) -> str: + return "-".join(re.findall(r"[a-z0-9]+", value.lower())) or fallback + + +def derived_key_id(item: TenantKey, existing_ids: Collection[str]) -> str | None: + """A stable key_id for an entry configured without one, or None. + + The suffix is a prefix of the entry's peppered lookup digest -- its stored + `key_lookup`, else `compute_key_lookup` over its plaintext key -- so the + same key gets the same id on every load, restart and replica that shares + the pepper. Never the plaintext or an unpeppered hash of it: the id lands in + logs, Redis key names, usage rows and admin responses, and must not let a + guessed key be confirmed offline (audit FB-07). + + An id already taken lengthens the prefix, so the later entry of a clashing + pair still gets the same id on every load. None when there is nothing to + derive from (a legacy hash-only entry) or the whole digest is taken. + """ + if item.key_lookup is not None: + lookup = item.key_lookup + elif item.key is not None: + lookup = compute_key_lookup(item.key) + else: + return None + stem = f"{key_id_slug(item.tenant, 'tenant')}-{key_id_slug(item.name, 'agent')}-" + for size in range(KEY_ID_DIGEST_CHARS, len(lookup) + 1): + candidate = stem + lookup[:size] + if candidate not in existing_ids: + return candidate + return None + class KeyRotator: def __init__(self, manager: AuthManager) -> None: @@ -274,8 +311,8 @@ def generate_key_id( name: str, existing_ids: set[str] | None = None, ) -> str: - tenant_slug = re.sub(r"[^a-z0-9]+", "-", tenant.lower()).strip("-") or "tenant" - name_slug = re.sub(r"[^a-z0-9]+", "-", name.lower()).strip("-") or "agent" + tenant_slug = key_id_slug(tenant, "tenant") + name_slug = key_id_slug(name, "agent") seen_ids = set(existing_ids or ()) while True: candidate = f"{tenant_slug}-{name_slug}-{secrets.token_hex(4)}" @@ -288,7 +325,13 @@ def ensure_key_ids(self, config: ApiKeysConfig) -> bool: for index, item in enumerate(config.keys): if item.key_id is not None: continue - key_id = self.generate_key_id(item.tenant, item.name, existing_ids) + # Derived, not drawn: an id that never reaches the file (a read-only + # key store, or AGENTFLOW_API_KEYS) must come out the same on the next + # load and in every replica. Only a legacy hash-only entry, with + # nothing to derive from, still gets a random one. + key_id = derived_key_id(item, existing_ids) or self.generate_key_id( + item.tenant, item.name, existing_ids + ) existing_ids.add(key_id) config.keys[index] = item.model_copy(update={"key_id": key_id}) changed = True diff --git a/src/agentflow_runtime/serving/api/auth/manager.py b/src/agentflow_runtime/serving/api/auth/manager.py index e14c4c7f..6cc3660f 100644 --- a/src/agentflow_runtime/serving/api/auth/manager.py +++ b/src/agentflow_runtime/serving/api/auth/manager.py @@ -330,7 +330,7 @@ def load(self) -> None: self._key_rotator.schedule_rotation_cleanup(item) # Carry windows over by bucket name (`_rate_limit_key`), never by the # plaintext `keys_by_value` index: a still-configured key keeps its - # window across the reload, a removed key's window is dropped. (T-46) + # window across the reload, a removed key's window is dropped. live_buckets = {self._rate_limit_key(item) for item in config.keys} self._rate_windows = defaultdict( list, @@ -704,7 +704,6 @@ def _legacy_env_keys(self) -> list[TenantKey]: key, name = pair, "unnamed" items.append( TenantKey( - key_id=self._key_rotator.generate_key_id("default", name.strip(), set()), key=key.strip(), name=name.strip(), tenant="default", @@ -713,7 +712,12 @@ def _legacy_env_keys(self) -> list[TenantKey]: created_at=datetime.now(UTC).date(), ) ) - return items + # The same derivation as an id-less key-file entry: from the key's + # lookup digest, so the key keeps its id -- and its rate-limit bucket -- + # across reloads, restarts and replicas. + config = ApiKeysConfig(keys=items) + self._key_rotator.ensure_key_ids(config) + return config.keys def _rate_limit_key(self, tenant_key: TenantKey) -> str: # This string becomes a Redis sorted-set key NAME (``rate_limiter`` calls diff --git a/tests/unit/test_auth_key_identity.py b/tests/unit/test_auth_key_identity.py new file mode 100644 index 00000000..439ec451 --- /dev/null +++ b/tests/unit/test_auth_key_identity.py @@ -0,0 +1,280 @@ +"""Requirement tests for docs/specs/api-key-identity.md, one per scenario. + +A key configured without a `key_id` -- every key from `AGENTFLOW_API_KEYS`, and +a key-file entry without one -- gets an id derived from its peppered key-lookup +digest, so the same key keeps the same id (and so the same rate-limit bucket +and usage rows) on every load, restart and replica, even when the key file is +mounted read-only and the id can never be written back. +""" + +from __future__ import annotations + +import os +import re +import shutil +import stat +from pathlib import Path + +import pytest +import yaml + +from agentflow_runtime.serving.api.auth import manager as manager_module +from agentflow_runtime.serving.api.auth.manager import AuthManager +from agentflow_runtime.serving.api.security import compute_key_lookup + +PEPPER = "api-key-identity-test-pepper" +REPO_ROOT = Path(__file__).resolve().parents[2] +STORED_LOOKUP = "5e1f0c3a9b7d24866d3f2e1c0b9a8f7e6d5c4b3a29180716f5e4d3c2b1a09f8e" +HASHED_ENTRY_YAML = ( + "keys:\n" + ' - key_hash: "$2b$04$storedhashstoredhashstoredhashstoredhashstoredhashst"\n' + f' key_lookup: "{STORED_LOOKUP}"\n' + ' name: "Support Agent"\n' + ' tenant: "acme"\n' + " rate_limit_rpm: 60\n" + ' created_at: "2026-04-10"\n' +) + + +@pytest.fixture(autouse=True) +def _pinned_pepper(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("AGENTFLOW_KEY_LOOKUP_PEPPER", PEPPER) + monkeypatch.delenv("AGENTFLOW_PROFILE", raising=False) + monkeypatch.delenv("AGENTFLOW_API_KEYS", raising=False) + monkeypatch.delenv("REDIS_URL", raising=False) + + +def _env_manager(tmp_path: Path, db_name: str = "usage.duckdb") -> AuthManager: + manager = AuthManager(api_keys_path=None, db_path=tmp_path / db_name) + manager.load() + return manager + + +def _file_manager(path: Path, tmp_path: Path, db_name: str = "usage.duckdb") -> AuthManager: + manager = AuthManager(api_keys_path=path, db_path=tmp_path / db_name) + manager.load() + return manager + + +def _key_file(tmp_path: Path, content: str) -> Path: + path = tmp_path / "config" / "api_keys.yaml" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8", newline="\n") + return path + + +def test_same_environment_key_two_managers(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:Support Agent") + + first = _env_manager(tmp_path, "first.duckdb") + second = _env_manager(tmp_path, "second.duckdb") + + key_id = first.keys_by_value["k1"].key_id + assert key_id is not None + assert re.fullmatch(r"default-support-agent-[0-9a-f]{8}", key_id) + assert second.keys_by_value["k1"].key_id == key_id + + +def test_a_reload_keeps_the_id_and_the_bucket( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Environment keys carry the default rpm; one request fills a 1-rpm window. + monkeypatch.setattr(manager_module, "DEFAULT_RATE_LIMIT_RPM", 1) + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot") + manager = _env_manager(tmp_path) + key = manager.keys_by_value["k1"] + bucket = manager._rate_limit_key(key) + assert manager.is_rate_limited(key) is False + assert manager.is_rate_limited(key) is True + + manager.load() + + reloaded = manager.keys_by_value["k1"] + assert reloaded.key_id == key.key_id + assert manager._rate_limit_key(reloaded) == bucket + # The practical effect: the full window survived the reload. + assert manager.is_rate_limited(reloaded) is True + + +def test_different_keys_under_one_name_get_different_ids( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot,k2:bot") + + manager = _env_manager(tmp_path) + + first = manager.keys_by_value["k1"].key_id + second = manager.keys_by_value["k2"].key_id + assert first is not None + assert second is not None + assert first != second + + +def test_an_idless_entry_in_a_read_only_key_file(tmp_path: Path) -> None: + if hasattr(os, "geteuid") and os.geteuid() == 0: + pytest.skip("root writes a read-only file regardless of its mode") + path = _key_file(tmp_path, HASHED_ENTRY_YAML) + on_disk = path.read_bytes() + os.chmod(path, stat.S_IREAD) + try: + first = _file_manager(path, tmp_path, "first.duckdb") + second = _file_manager(path, tmp_path, "second.duckdb") + [first_key] = first._loaded_keys + [second_key] = second._loaded_keys + assert first_key.key_id is not None + assert second_key.key_id == first_key.key_id + + first.load() + + [reloaded] = first._loaded_keys + assert reloaded.key_id == first_key.key_id + # The id was never written back: it is the derivation, not the file, + # that keeps it stable. + assert path.read_bytes() == on_disk + finally: + os.chmod(path, stat.S_IREAD | stat.S_IWRITE) + + +def test_a_writable_key_file_persists_the_derived_id(tmp_path: Path) -> None: + path = _key_file( + tmp_path, + "keys:\n" + ' - key: "plain-file-key"\n' + ' name: "Report Bot"\n' + ' tenant: "Acme"\n' + ' created_at: "2026-04-10"\n', + ) + + manager = _file_manager(path, tmp_path) + + [key] = manager._loaded_keys + expected = f"acme-report-bot-{compute_key_lookup('plain-file-key', PEPPER)[:8]}" + assert key.key_id == expected + [stored] = yaml.safe_load(path.read_text(encoding="utf-8"))["keys"] + assert stored["key_id"] == expected + + +def test_the_suffix_is_the_lookup_digests_prefix( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot") + + manager = _env_manager(tmp_path) + + key_id = manager.keys_by_value["k1"].key_id + assert key_id == f"default-bot-{compute_key_lookup('k1', PEPPER)[:8]}" + + +def test_a_stored_key_lookup_is_used_as_it_is(tmp_path: Path) -> None: + path = _key_file(tmp_path, HASHED_ENTRY_YAML) + + manager = _file_manager(path, tmp_path) + + [key] = manager._loaded_keys + assert key.key_id == f"acme-support-agent-{STORED_LOOKUP[:8]}" + + +def test_a_different_pepper_gives_a_different_id( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot") + first = _env_manager(tmp_path, "first.duckdb") + monkeypatch.setenv("AGENTFLOW_KEY_LOOKUP_PEPPER", PEPPER + "-rotated") + second = _env_manager(tmp_path, "second.duckdb") + + assert first.keys_by_value["k1"].key_id != second.keys_by_value["k1"].key_id + + +def test_a_stored_key_lookup_keeps_its_id_under_another_pepper( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # Two copies of one file: a writable file gets the id written back, and the + # second load must derive it again rather than read it. + first_path = _key_file(tmp_path, HASHED_ENTRY_YAML) + second_path = tmp_path / "copy" / "api_keys.yaml" + second_path.parent.mkdir() + second_path.write_text(HASHED_ENTRY_YAML, encoding="utf-8", newline="\n") + + first = _file_manager(first_path, tmp_path, "first.duckdb") + monkeypatch.setenv("AGENTFLOW_KEY_LOOKUP_PEPPER", PEPPER + "-rotated") + second = _file_manager(second_path, tmp_path, "second.duckdb") + + [first_key] = first._loaded_keys + [second_key] = second._loaded_keys + assert first_key.key_id == f"acme-support-agent-{STORED_LOOKUP[:8]}" + assert second_key.key_id == first_key.key_id + + +def test_a_writable_key_file_keeps_its_written_id_under_another_pepper( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # The pepper changes only an id that was never written back: the first load + # persists the derived id, and a later load reads it from the file. + path = _key_file( + tmp_path, + "keys:\n" + ' - key: "plain-file-key"\n' + ' name: "Report Bot"\n' + ' tenant: "Acme"\n' + ' created_at: "2026-04-10"\n', + ) + first = _file_manager(path, tmp_path, "first.duckdb") + [written] = yaml.safe_load(path.read_text(encoding="utf-8"))["keys"] + monkeypatch.setenv("AGENTFLOW_KEY_LOOKUP_PEPPER", PEPPER + "-rotated") + second = _file_manager(path, tmp_path, "second.duckdb") + + [first_key] = first._loaded_keys + [second_key] = second._loaded_keys + expected = f"acme-report-bot-{compute_key_lookup('plain-file-key', PEPPER)[:8]}" + assert first_key.key_id == expected + assert written["key_id"] == expected + assert second_key.key_id == expected + + +# --------------------------------------------------------------------------- # +# False-reject control: the change derives ids, it rejects nothing that loaded. +# --------------------------------------------------------------------------- # + + +def test_the_shipped_key_file_still_loads_with_ids_from_its_stored_lookups( + tmp_path: Path, +) -> None: + # config/api_keys.yaml is the file docker-compose.prod.yml mounts + # read-only: two hashed entries with a key_lookup and no key_id. + path = tmp_path / "config" / "api_keys.yaml" + path.parent.mkdir(parents=True) + shutil.copyfile(REPO_ROOT / "config" / "api_keys.yaml", path) + + manager = _file_manager(path, tmp_path) + + by_name = {item.name: item for item in manager._loaded_keys} + assert set(by_name) == {"Support Agent", "Ops Agent"} + for name, slug in (("Support Agent", "support-agent"), ("Ops Agent", "ops-agent")): + item = by_name[name] + assert item.key_lookup is not None + assert item.key_id == f"default-{slug}-{item.key_lookup[:8]}" + assert manager._keys_by_lookup[item.key_lookup] is item + + +def test_a_persisted_id_and_a_hash_only_entry_still_load(tmp_path: Path) -> None: + path = _key_file( + tmp_path, + "keys:\n" + ' - key_id: "acme-kept-id"\n' + ' key: "kept-plain-key"\n' + ' name: "Kept"\n' + ' tenant: "acme"\n' + ' created_at: "2026-04-10"\n' + ' - key_hash: "$2b$04$legacyhashlegacyhashlegacyhashlegacyhashlegacyhash"\n' + ' name: "Legacy"\n' + ' tenant: "acme"\n' + ' created_at: "2026-04-10"\n', + ) + + manager = _file_manager(path, tmp_path) + + kept, legacy = manager._loaded_keys + assert kept.key_id == "acme-kept-id" + # Nothing to derive from: the legacy entry keeps generate_key_id's shape. + assert legacy.key_id is not None + assert re.fullmatch(r"acme-legacy-[0-9a-f]{8}", legacy.key_id) diff --git a/tests/unit/test_key_rotation_mutation.py b/tests/unit/test_key_rotation_mutation.py index 9497dd36..512a543f 100644 --- a/tests/unit/test_key_rotation_mutation.py +++ b/tests/unit/test_key_rotation_mutation.py @@ -787,8 +787,8 @@ def test_assigns_ids_to_idless_entries_accumulating_existing( manager._key_rotator, "generate_key_id", _rec_gen_id(id_calls, ["gen-1", "gen-2"]) ) keyed = _tk(key_id="keep-id", key_hash="h0") - first = _tk(key_id=None, key_hash="h1", tenant="t1", name="n1") - second = _tk(key_id=None, key_hash="h2", tenant="t2", name="n2") + first = _tk(key_id=None, key=None, key_hash="h1", tenant="t1", name="n1") + second = _tk(key_id=None, key=None, key_hash="h2", tenant="t2", name="n2") config = ApiKeysConfig(keys=[keyed, first, second]) changed = manager._key_rotator.ensure_key_ids(config) @@ -817,6 +817,108 @@ def _boom(*_a: object, **_k: object) -> str: assert manager._key_rotator.ensure_key_ids(config) is False assert called["n"] == 0 # no id generated when every entry has one + def test_derives_ids_for_entries_it_can_and_keeps_them_unique( + self, tmp_path: object, monkeypatch: pytest.MonkeyPatch + ) -> None: + manager = _build_manager(tmp_path, monkeypatch) + id_calls: list = [] + monkeypatch.setattr( + manager._key_rotator, "generate_key_id", _rec_gen_id(id_calls, ["gen-1"]) + ) + # The persisted id comes AFTER the id-less entries and still takes the + # 8-char id first; the same key configured twice lengthens again. + derivable = _tk(key_id=None, key=None, key_hash="h1", key_lookup=_DIGEST) + again = _tk(key_id=None, key=None, key_hash="h2", key_lookup=_DIGEST) + legacy = _tk(key_id=None, key=None, key_hash="h3", tenant="t3", name="n3") + persisted = _tk(key_id=f"acme-n-{_DIGEST[:8]}", key_hash="h0") + config = ApiKeysConfig(keys=[derivable, again, legacy, persisted]) + + assert manager._key_rotator.ensure_key_ids(config) is True + + assert [item.key_id for item in config.keys] == [ + f"acme-n-{_DIGEST[:9]}", + f"acme-n-{_DIGEST[:10]}", + "gen-1", + f"acme-n-{_DIGEST[:8]}", + ] + # Only the hash-only entry drew a random id, against every id taken so far. + assert id_calls == [ + ( + "t3", + "n3", + {f"acme-n-{_DIGEST[:8]}", f"acme-n-{_DIGEST[:9]}", f"acme-n-{_DIGEST[:10]}"}, + ) + ] + + +# --------------------------------------------------------------------------- # +# derived_key_id / key_id_slug (module-level; no manager needed) +# --------------------------------------------------------------------------- # + +_DIGEST = "0123456789abcdef" * 4 + + +class _ProbedIds(set): + # Records every membership test, so the prefix search order is observable. + def __init__(self, *args: object) -> None: + super().__init__(*args) + self.probes: list = [] + + def __contains__(self, value: object) -> bool: + self.probes.append(value) + return super().__contains__(value) + + +def _no_lookup(value: str) -> str: + raise AssertionError(f"compute_key_lookup must not be called (got {value!r})") + + +class TestDerivedKeyId: + def test_stored_lookup_is_used_as_it_is(self, monkeypatch: pytest.MonkeyPatch) -> None: + # A stored key_lookup wins even when a plaintext key is present too. + monkeypatch.setattr(kr, "compute_key_lookup", _no_lookup) + item = _tk(key="plain-key", key_lookup=_DIGEST) + assert kr.derived_key_id(item, set()) == f"acme-n-{_DIGEST[:8]}" + + def test_plaintext_key_goes_through_the_peppered_lookup( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + calls: list = [] + monkeypatch.setattr(kr, "compute_key_lookup", lambda value: calls.append(value) or _DIGEST) + item = _tk(key="plain-key", key_lookup=None) + assert kr.derived_key_id(item, set()) == f"acme-n-{_DIGEST[:8]}" + assert calls == ["plain-key"] + + def test_hash_only_entry_has_nothing_to_derive_from( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setattr(kr, "compute_key_lookup", _no_lookup) + item = _tk(key=None, key_hash="h", key_lookup=None) + assert kr.derived_key_id(item, set()) is None + + def test_slugs_follow_generate_key_id_rules(self) -> None: + item = _tk(key_lookup=_DIGEST, tenant=" Acme Corp!", name="Support Agent 2 ") + assert kr.derived_key_id(item, set()) == f"acme-corp-support-agent-2-{_DIGEST[:8]}" + empty = _tk(key_lookup=_DIGEST, tenant="!!!", name="***") + assert kr.derived_key_id(empty, set()) == f"tenant-agent-{_DIGEST[:8]}" + + def test_a_taken_id_lengthens_the_prefix(self) -> None: + item = _tk(key_lookup=_DIGEST) + taken = {f"acme-n-{_DIGEST[:8]}", f"acme-n-{_DIGEST[:9]}"} + assert kr.derived_key_id(item, taken) == f"acme-n-{_DIGEST[:10]}" + + def test_prefixes_are_tried_once_each_up_to_the_whole_digest(self) -> None: + item = _tk(key_lookup="0123456789") + candidates = ["acme-n-01234567", "acme-n-012345678", "acme-n-0123456789"] + # Only the whole digest is free: it is the last candidate. + free_at_end = _ProbedIds(candidates[:2]) + assert kr.derived_key_id(item, free_at_end) == candidates[2] + assert free_at_end.probes == candidates + # Every prefix taken: nothing left to derive, and no prefix probed twice. + exhausted = _ProbedIds(candidates) + assert kr.derived_key_id(item, exhausted) is None + assert exhausted.probes == candidates + class TestFindKeyIndex: def test_returns_matching_index( From 77825f630121725152aa645b767c89c2ce0cd9f2 Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 19:29:27 -0400 Subject: [PATCH 7/9] test(mutation): pin what manager.py's surviving mutants changed CI run 34464697021 scored serving/api/auth/manager.py 82.1% (430 killed of 524) on 6231b73, two points above its 0.80 threshold. Most of the 94 survivors changed something a behaviour test can see. That includes the log events and the Redis URL, which scripts/mutation_report.py still calls equivalents: a log event is the operator's interface, and the Redis URL decides which server holds the rate-limit budget. tests/unit/test_auth_manager_mutation.py is the only file the gate runs against the module. It now pins: - the key-store writability probe, read-only file included; - the construction wiring: the injected store and audit publisher, usage rows through the real UsageWriter, the default store following db_path, the security config path at construction and on load(), the grace-period fallback and its warning, and the Redis server the limiter targets; - the load() log events, what load() writes back, and the key file it leaves byte-for-byte alone; - that a match's slot comes from the material that matched, never from the stored entry; - that load() carries rate-limit windows over by bucket; - authenticate()'s scan order, and when check_rate_limit() trusts Redis; - AGENTFLOW_API_KEYS parsing. Two simplifications in manager.py remove mutants no test could kill. The writability probe opens the file in binary append mode: the text layer and its encoding did nothing for an open-and-close probe. _legacy_env_keys() stops passing allowed_entity_types=None, which is TenantKey's own default. rate_limit_rpm stays. TenantKey's default froze at import, while the call reads DEFAULT_RATE_LIMIT_RPM at load time. Dropping it too turned the full gate red on test_auth_key_identity, and a gate-file test now pins the load-time read. Nine equivalent mutants stay alive. Each sits on a line that also carries killable mutants, so the test file's docstring names them with their reasons instead of a line-level pragma. Two more lower-case an os.getenv name, so they survive only on Windows, whose environment names are case-insensitive. The local verdicts (77 of 94 killed, 6 removed by the simplifications) came from a scratch runner. scripts/mutation_local.py scores a mutant killed as soon as a tmp_path test runs, because it never creates the parent of its --basetemp; that gets its own fix. The threshold is unchanged here. It is set from the CI measurement of this commit. Two test comments still described the old window carry-over and the random ids of environment keys. Their words are corrected; the assertions are unchanged. Written by opus-high after grok's account ran out of its free usage. grok, on its second account, wrote the repair for the red gate. Reviewed by codex, which returned no findings. The controller skipped review of the repair delta, so the orchestrator reviewed that one-line restore and its test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UUTW71FTTNxbF8JAKwv3Am --- CHANGELOG.md | 35 + .../serving/api/auth/manager.py | 4 +- tests/unit/test_auth_manager_memory_bounds.py | 12 +- tests/unit/test_auth_manager_mutation.py | 690 +++++++++++++++++- tests/unit/test_auth_rate_window_reload.py | 4 +- 5 files changed, 734 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ec2b553..ab6e2060 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,41 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] +### Quality — the auth-manager mutation lane pins what its survivors changed + +* **`serving/api/auth/manager.py` sat two points above its 0.80 threshold.** + CI run 34464697021 on 6231b73 scored it 82.1% (430 killed of 524). Most of + the 94 survivors changed something a behaviour test can see, including the + logging and Redis-URL mutants that `scripts/mutation_report.py` calls + equivalents: a log event is the operator's interface, and the Redis URL + decides which server holds the rate-limit budget. + `tests/unit/test_auth_manager_mutation.py` now pins the key-store + writability probe, read-only file included; the construction wiring (the + injected store and audit publisher, usage rows flowing through the real + `UsageWriter`, the default store following `db_path`, the security config + path at construction and on `load()`, the grace-period fallback and its + warning, the Redis server the limiter targets); the `load()` log events + `api_keys_loaded`, `api_key_store_write_skipped_readonly` and + `hashed_key_count_exceeds_guidance` (strictly above the soft limit, + unindexed entries only); what `load()` writes back, and the key file it + leaves byte-for-byte alone; that a match's slot comes from the material that + matched, never from the stored entry; that `load()` carries rate-limit + windows over by bucket; scan order in `authenticate()`; when + `check_rate_limit()` trusts Redis; and `AGENTFLOW_API_KEYS` parsing. +* **Two simplifications remove mutants no test could kill.** The writability + probe opens the key file in binary append mode: the text layer and its + `encoding="utf-8"` did nothing for an open-and-close probe, and three + mutants changed only them. `_legacy_env_keys()` stops passing + `allowed_entity_types` at `TenantKey`'s own default (`None`); + `rate_limit_rpm=DEFAULT_RATE_LIMIT_RPM` stays because `TenantKey`'s Field + default is evaluated at import, while a monkeypatch of the module global + is a load-time read. +* **The residue is named, not suppressed.** Nine equivalent mutants stay + alive, each on a line that also carries killable mutants, so a line-level + `# pragma: no mutate` would silence those too; the test file's docstring + lists them with their reasons. The threshold is unchanged here: it is set + from the CI measurement of this tree. + ### Fixed — a key configured without a key_id keeps the same id on every load A key whose configuration carries no `key_id` — every key from diff --git a/src/agentflow_runtime/serving/api/auth/manager.py b/src/agentflow_runtime/serving/api/auth/manager.py index 6cc3660f..c27f9b8b 100644 --- a/src/agentflow_runtime/serving/api/auth/manager.py +++ b/src/agentflow_runtime/serving/api/auth/manager.py @@ -133,7 +133,8 @@ def probe_key_store_writable(path: Path | str | None) -> bool: def _file_is_writable(path: Path) -> bool: try: - with path.open("a", encoding="utf-8"): + # Binary: an open-and-close probe has no text to encode. + with path.open("ab"): return True except OSError as exc: if is_permission_denied(exc): @@ -708,7 +709,6 @@ def _legacy_env_keys(self) -> list[TenantKey]: name=name.strip(), tenant="default", rate_limit_rpm=DEFAULT_RATE_LIMIT_RPM, - allowed_entity_types=None, created_at=datetime.now(UTC).date(), ) ) diff --git a/tests/unit/test_auth_manager_memory_bounds.py b/tests/unit/test_auth_manager_memory_bounds.py index ca3543dc..f811c746 100644 --- a/tests/unit/test_auth_manager_memory_bounds.py +++ b/tests/unit/test_auth_manager_memory_bounds.py @@ -60,12 +60,12 @@ def test_load_sweeps_expired_rate_windows(self, manager: AuthManager) -> None: manager._rate_windows["fresh-key"] = [now - 5] manager.load() # triggers _sweep_expired_windows under config lock assert "stale-key" not in manager._rate_windows - # 'fresh-key' is also removed because load() rebuilds _rate_windows - # from keys_by_value (none configured in this fixture) — that - # rebuild is documented behaviour from session 17 and is independent - # of the H-C4 sweep, but the assertion below pins it so a future - # refactor that reuses the old `defaultdict` cannot reintroduce - # the unbounded-growth path silently. + # 'fresh-key' is also removed: load() keeps only the windows whose + # bucket belongs to a still-configured key, and no configured key + # names 'fresh-key' (the fixture configures none). That carry-over is + # independent of the H-C4 sweep, but the assertion below pins it so a + # future refactor that keeps every window cannot reintroduce the + # unbounded-growth path silently. assert "fresh-key" not in manager._rate_windows diff --git a/tests/unit/test_auth_manager_mutation.py b/tests/unit/test_auth_manager_mutation.py index c4e987ef..68549ab5 100644 --- a/tests/unit/test_auth_manager_mutation.py +++ b/tests/unit/test_auth_manager_mutation.py @@ -36,13 +36,39 @@ on ``find_spec("serving")`` -- NOT ``import src``, which stays importable via the editable install even inside the workspace (cont.21 duckdb-crash root cause). Under ordinary pytest no stub is installed and the real modules load. + +4. **Named residue.** The mutants below stay alive on purpose. Each is + equivalent, and each sits on a line that also carries killable mutants, so a + line-level ``# pragma: no mutate`` would silence those too. + + * ``authenticate`` plaintext path, ``update={"key": api_key, ...}`` -> + ``"XXkeyXX"`` / ``"KEY"``: ``compare_digest`` has just proved + ``item.key == api_key``, so the copy carries the same key either way. + * ``load`` ``skipped_readonly_write = False`` -> ``None``, and ``__init__`` + ``_key_store_readonly_skip_logged = False`` -> ``None``: both flags are + read only for truthiness. + * ``_sweep_expired_windows`` ``pop(key, None)`` -> ``pop(key)``, in both + loops: the key comes from a snapshot of the same dict, so the default + matters only if a concurrent sweep removed it in between -- a race guard + no deterministic test can reproduce. + * ``_load_config`` ``read_text(encoding="utf-8")`` -> ``"UTF-8"`` (codec + names are case-insensitive) and -> ``encoding=None`` (the locale encoding + is UTF-8 on the Linux CI host; the non-ASCII key-file test kills it on a + cp1252 Windows host). + * ``__init__`` default ``"redis://localhost:6379"`` -> upper case: + ``urlparse`` lower-cases the scheme and the host. """ from __future__ import annotations +import importlib +import os +import stat import sys import types -from datetime import date +from datetime import UTC, date, datetime, tzinfo +from pathlib import Path +from typing import Any, Self def _in_mutation_workspace() -> bool: @@ -101,6 +127,7 @@ def _connect(*_args: object, **_kwargs: object) -> object: from agentflow_runtime.serving.api.auth import manager as manager_module import pytest +import yaml AuthManager = manager_module.AuthManager TenantKey = manager_module.TenantKey @@ -1172,3 +1199,664 @@ def test_close_usage_writer_forwards_an_explicit_timeout(self) -> None: m._usage_writer = writer # type: ignore[assignment] m.close_usage_writer(0.5) assert writer.close_calls == [0.5] + + +# --------------------------------------------------------------------------- # +# Doubles and key-file helpers for the wiring, log-contract and load tests. +# --------------------------------------------------------------------------- # + +LogRecord = tuple[str, str, dict[str, object]] + + +class _RecordingLogger: + """Stands in for the auth package logger. manager.py imports that logger + inside its functions from ``agentflow_runtime.serving.api.auth`` -- the + installed package, even in the mutation workspace -- so it is swapped on + that module, which is the same object in both environments. A log event is + the operator's interface: its name and fields are the contract pinned here.""" + + def __init__(self) -> None: + self.records: list[LogRecord] = [] + + def info(self, event: str, **fields: object) -> None: + self.records.append(("info", event, fields)) + + def warning(self, event: str, **fields: object) -> None: + self.records.append(("warning", event, fields)) + + def named(self, event: str) -> list[LogRecord]: + return [record for record in self.records if record[1] == event] + + +def _record_auth_logs(monkeypatch: pytest.MonkeyPatch) -> _RecordingLogger: + recorder = _RecordingLogger() + auth_package = importlib.import_module("agentflow_runtime.serving.api.auth") + monkeypatch.setattr(auth_package, "logger", recorder) + return recorder + + +class _RecordingStore: + """The one store method the UsageWriter calls.""" + + def __init__(self) -> None: + self.batches: list[list[Any]] = [] + + def record_api_usage_batch(self, rows: list[Any]) -> None: + self.batches.append(list(rows)) + + +class _RecordingPublisher: + def __init__(self) -> None: + self.payloads: list[dict] = [] + + def publish(self, payload: dict) -> None: + self.payloads.append(payload) + + +class _ScriptedLimiter: + """A limiter that gives a fixed verdict and records the bucket it was asked + about. ``with_redis=False`` leaves out the ``_redis`` attribute entirely.""" + + def __init__(self, verdict: tuple[bool, int, int], *, with_redis: bool = True) -> None: + self.verdict = verdict + self.asked: list[tuple[str, int]] = [] + if with_redis: + self._redis = object() + + async def check(self, key: str, rpm: int) -> tuple[bool, int, int]: + self.asked.append((key, rpm)) + return self.verdict + + +def _entry(**overrides: object) -> dict[str, object]: + base: dict[str, object] = {"name": "support", "tenant": "acme", "created_at": date(2026, 1, 1)} + base.update(overrides) + return base + + +def _write_key_file(path: Path, entries: list[dict[str, object]]) -> Path: + path.write_text(yaml.safe_dump({"keys": entries}, sort_keys=False), encoding="utf-8") + return path + + +def _skip_if_root() -> None: + geteuid = getattr(os, "geteuid", None) + if geteuid is not None and geteuid() == 0: + pytest.skip("root writes a read-only file regardless of its mode") + + +def _make_read_only(path: Path) -> None: + os.chmod(path, stat.S_IREAD) + + +def _make_writable(path: Path) -> None: + # Restored before tmp_path cleanup: Windows refuses to delete a read-only file. + os.chmod(path, stat.S_IREAD | stat.S_IWRITE) + + +def _redis_target(m: AuthManager) -> tuple[object, object]: + # redis-py's from_url does not connect, so no server is needed to read this. + kwargs = m.rate_limiter._redis.connection_pool.connection_kwargs # type: ignore[union-attr] + return kwargs["host"], kwargs["port"] + + +# --------------------------------------------------------------------------- # +# Key-store writability probe. +# --------------------------------------------------------------------------- # + + +class TestKeyStoreWritableProbe: + def test_no_path_is_not_writable(self) -> None: + assert manager_module.probe_key_store_writable(None) is False + + def test_existing_writable_file_probes_writable_and_is_left_unchanged( + self, tmp_path: Path + ) -> None: + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes(b"keys: []\n") + assert manager_module.probe_key_store_writable(keys_file) is True + assert keys_file.read_bytes() == b"keys: []\n" + + def test_existing_read_only_file_probes_not_writable(self, tmp_path: Path) -> None: + # Opening for reading would succeed; only an attempt to open for writing + # tells a read-only mount from a writable one. + _skip_if_root() + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes(b"keys: []\n") + _make_read_only(keys_file) + try: + assert manager_module.probe_key_store_writable(keys_file) is False + # The file check answers on its own; it does not lean on the probe's + # outer handler to turn a permission error into a verdict. + assert manager_module._file_is_writable(keys_file) is False + finally: + _make_writable(keys_file) + + +# --------------------------------------------------------------------------- # +# __init__ wiring: store, usage writer, security policy, grace period, Redis. +# --------------------------------------------------------------------------- # + + +class TestConstructionWiring: + def test_injected_store_is_the_store_the_manager_uses(self) -> None: + store = _RecordingStore() + m = _build_manager(store=store) + assert m.store is store + + def test_default_store_resolves_usage_db_from_the_managers_db_path( + self, tmp_path: Path + ) -> None: + m = _build_manager(db_path=tmp_path / "usage.duckdb") + store = m.store + assert store._usage_db_path == tmp_path / "usage.duckdb" # type: ignore[attr-defined] + # The provider reads the manager's attribute at call time, so a manager + # whose db_path moves takes its usage database with it. + m.db_path = tmp_path / "moved.duckdb" + assert store._usage_db_path == tmp_path / "moved.duckdb" # type: ignore[attr-defined] + + def test_submitted_usage_reaches_the_injected_store_and_publisher(self) -> None: + from agentflow_runtime.serving.control_plane.store import UsageRow + + store = _RecordingStore() + publisher = _RecordingPublisher() + m = _build_manager(store=store, audit_publisher=publisher) + tenant_key = _key(key_id="kid-1", name="support", tenant="acme", matched_slot="previous") + try: + assert m.submit_usage(tenant_key, "/v1/entity/order/1") is True + assert m.flush_usage(timeout=5.0) is True + finally: + m.close_usage_writer() + expected = UsageRow( + tenant="acme", + key_name="support", + endpoint="/v1/entity/order/1", + key_id="kid-1", + key_slot="previous", + ) + assert store.batches == [[expected]] + assert publisher.payloads == [ + { + "event_type": "api_usage", + "tenant": "acme", + "key_name": "support", + "endpoint": "/v1/entity/order/1", + "key_id": "kid-1", + "key_slot": "previous", + } + ] + + def test_security_config_path_is_honoured_at_construction_and_by_load( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + monkeypatch.delenv("AGENTFLOW_API_KEYS", raising=False) + security_file = tmp_path / "security.yaml" + security_file.write_text( + "security:\n max_failed_auth_per_ip_per_hour: 3\n", encoding="utf-8" + ) + # The default path's policy must differ, or reading it instead would pass. + assert manager_module.load_security_policy(None).max_failed_auth_per_ip_per_hour != 3 + m = _build_manager(security_config_path=security_file) + assert [m.record_failed_auth("10.0.0.1") for _ in range(4)] == [False, False, False, True] + m.load() + assert [m.record_failed_auth("10.0.0.2") for _ in range(4)] == [False, False, False, True] + + def test_key_store_writable_probes_lazily_before_any_load(self, tmp_path: Path) -> None: + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes(b"keys: []\n") + m = _build_manager(api_keys_path=keys_file) + assert m.key_store_writable is True + + def test_unset_rotation_grace_period_uses_the_default_without_a_warning( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("AGENTFLOW_ROTATION_GRACE_PERIOD_SECONDS", raising=False) + logs = _record_auth_logs(monkeypatch) + m = _build_manager() + assert m.rotation_grace_period_seconds == DEFAULT_ROTATION_GRACE_PERIOD_SECONDS + assert logs.named("invalid_rotation_grace_period_seconds") == [] + + def test_invalid_rotation_grace_period_logs_one_warning_with_the_raw_value( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("AGENTFLOW_ROTATION_GRACE_PERIOD_SECONDS", "ten-minutes") + logs = _record_auth_logs(monkeypatch) + _build_manager() + assert logs.records == [ + ( + "warning", + "invalid_rotation_grace_period_seconds", + {"value": "ten-minutes", "fallback": DEFAULT_ROTATION_GRACE_PERIOD_SECONDS}, + ) + ] + + def test_redis_url_argument_targets_that_server(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("REDIS_URL", raising=False) + m = _build_manager(redis_url="redis://cache.internal:6380/0") + assert _redis_target(m) == ("cache.internal", 6380) + + def test_redis_url_env_is_used_without_an_argument( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("REDIS_URL", "redis://env-cache.internal:6381/0") + m = _build_manager() + assert _redis_target(m) == ("env-cache.internal", 6381) + + def test_redis_url_argument_wins_over_the_env(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("REDIS_URL", "redis://env-cache.internal:6381/0") + m = _build_manager(redis_url="redis://cache.internal:6380/0") + assert _redis_target(m) == ("cache.internal", 6380) + + +# --------------------------------------------------------------------------- # +# load(): the log contract. +# --------------------------------------------------------------------------- # + + +class TestLoadLogContract: + def test_env_only_load_logs_env_only_and_the_configured_count( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:alpha,k2:beta") + logs = _record_auth_logs(monkeypatch) + m = _build_manager() + m.load() + assert logs.named("api_keys_loaded") == [ + ("info", "api_keys_loaded", {"path": "env_only", "keys": 2}) + ] + # Environment keys have nothing to write back, so nothing was skipped. + assert logs.named("api_key_store_write_skipped_readonly") == [] + + def test_key_file_load_logs_the_file_path( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", [_entry(key_id="kid-a", key="plain-a")] + ) + logs = _record_auth_logs(monkeypatch) + m = _build_manager(api_keys_path=keys_file) + m.load() + assert logs.named("api_keys_loaded") == [ + ("info", "api_keys_loaded", {"path": str(keys_file), "keys": 1}) + ] + + def test_unwritable_changed_config_warns_once_across_loads( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + _skip_if_root() + # No key_id -> load() derives one and wants to write it back. + keys_file = _write_key_file(tmp_path / "api_keys.yaml", [_entry(key="plain-a")]) + before = keys_file.read_bytes() + logs = _record_auth_logs(monkeypatch) + _make_read_only(keys_file) + try: + m = _build_manager(api_keys_path=keys_file) + m.load() + m.load() + finally: + _make_writable(keys_file) + assert logs.named("api_key_store_write_skipped_readonly") == [ + ("warning", "api_key_store_write_skipped_readonly", {"path": str(keys_file)}) + ] + assert keys_file.read_bytes() == before + assert m.key_store_writable is False + + def test_write_skip_warning_names_env_only_without_a_key_file( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + logs = _record_auth_logs(monkeypatch) + m = _build_manager() + m._warn_key_store_write_skipped() + assert logs.records == [ + ("warning", "api_key_store_write_skipped_readonly", {"path": "env_only"}) + ] + + +def _hashed_entries(count: int, *, indexed: bool) -> list[dict[str, object]]: + entries = [] + for index in range(count): + entry = _entry(key_id=f"kid-{index}", name=f"agent-{index}", key_hash=f"hash-{index}") + if indexed: + entry["key_lookup"] = f"lookup-{index}" + entries.append(entry) + return entries + + +class TestHashedKeyGuidance: + """``hashed_key_count_exceeds_guidance`` is documented in + docs/runbooks/auth-401-spike.md with its ``hashed_keys`` and ``soft_limit`` + fields. Only entries without a ``key_lookup`` pay the O(n) verify scan, so + only they count, and the warning fires strictly above the soft limit.""" + + def _load_logs( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, entries: list[dict[str, object]] + ) -> list[LogRecord]: + keys_file = _write_key_file(tmp_path / "api_keys.yaml", entries) + logs = _record_auth_logs(monkeypatch) + _build_manager(api_keys_path=keys_file).load() + return logs.named("hashed_key_count_exceeds_guidance") + + def test_exactly_the_soft_limit_is_silent( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + limit = manager_module.HASHED_KEY_SOFT_LIMIT + assert self._load_logs(monkeypatch, tmp_path, _hashed_entries(limit, indexed=False)) == [] + + def test_one_unindexed_key_over_the_soft_limit_warns_with_the_count( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + limit = manager_module.HASHED_KEY_SOFT_LIMIT + entries = _hashed_entries(limit + 1, indexed=False) + assert self._load_logs(monkeypatch, tmp_path, entries) == [ + ( + "warning", + "hashed_key_count_exceeds_guidance", + { + "hashed_keys": limit + 1, + "soft_limit": limit, + "reason": "cold_cache_bcrypt_latency", + "guidance": "docs/runbooks/auth-401-spike.md", + }, + ) + ] + + def test_indexed_keys_do_not_count( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + limit = manager_module.HASHED_KEY_SOFT_LIMIT + entries = _hashed_entries(limit + 5, indexed=True) + assert self._load_logs(monkeypatch, tmp_path, entries) == [] + + +# --------------------------------------------------------------------------- # +# load(): what it writes back, and what it leaves alone. +# --------------------------------------------------------------------------- # + + +class TestLoadWriteBack: + def test_writable_key_file_gains_the_key_ids_it_lacked(self, tmp_path: Path) -> None: + keys_file = _write_key_file(tmp_path / "api_keys.yaml", [_entry(key="plain-alpha")]) + m = _build_manager(api_keys_path=keys_file) + m.load() + [loaded] = m._loaded_keys + assert loaded.key_id is not None + assert loaded.key_id.startswith("acme-support-") + [stored] = yaml.safe_load(keys_file.read_text(encoding="utf-8"))["keys"] + assert stored["key_id"] == loaded.key_id + + def test_key_file_that_needs_no_change_is_left_byte_for_byte(self, tmp_path: Path) -> None: + raw = ( + b"# hand-maintained -- keep the comments\n" + b"keys:\n" + b" - key_id: kid-alpha # pinned id\n" + b" key: plain-alpha\n" + b" name: support\n" + b" tenant: acme\n" + b" created_at: 2026-01-01\n" + ) + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes(raw) + m = _build_manager(api_keys_path=keys_file) + m.load() + assert m.configured_key_count == 1 + assert keys_file.read_bytes() == raw + + def test_key_file_is_read_as_utf8(self, tmp_path: Path) -> None: + keys_file = tmp_path / "api_keys.yaml" + keys_file.write_bytes( + "keys:\n" + " - key_id: kid-alpha\n" + " key: plain-alpha\n" + " name: Zoë Müller\n" + " tenant: acme\n" + " created_at: 2026-01-01\n".encode() + ) + m = _build_manager(api_keys_path=keys_file) + m.load() + assert [key.name for key in m._loaded_keys] == ["Zoë Müller"] + + +# --------------------------------------------------------------------------- # +# The slot a match reports is decided by the material that matched. +# --------------------------------------------------------------------------- # + + +class TestSlotComesFromTheMatchedMaterial: + """``matched_slot`` is excluded from serialisation but accepted on input, so + a key-file entry can say ``matched_slot: previous``. Usage rows carry the + slot and rotation decisions read it, so a match on current material must + say "current" whatever the stored entry says.""" + + def test_load_labels_the_plaintext_index_current(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", + [_entry(key_id="kid-a", key="plain-a", matched_slot="previous")], + ) + m = _build_manager(api_keys_path=keys_file) + m.load() + assert m._loaded_keys[0].matched_slot == "previous" # what the file says + assert m.keys_by_value["plain-a"].matched_slot == "current" + + def test_load_labels_a_runtime_cached_hashed_entry_current(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", + [_entry(key_id="kid-h", key_hash="hash-h", key_lookup="lk-h", matched_slot="previous")], + ) + m = _build_manager(api_keys_path=keys_file) + m._runtime_plaintext_by_hash = {"hash-h": "runtime-plain"} + m.load() + assert m.keys_by_value["runtime-plain"].matched_slot == "current" + + def test_plaintext_match_is_current(self) -> None: + m = _build_manager() + m.keys_by_value = {"plain-a": _key(key="plain-a", matched_slot="previous")} + out = m.authenticate("plain-a") + assert out is not None + assert out.matched_slot == "current" + + def test_indexed_match_is_current(self, monkeypatch: pytest.MonkeyPatch) -> None: + m = _build_manager() + m._keys_by_lookup = { + "lk": _key(key=None, key_hash="idx-hash", key_lookup="lk", matched_slot="previous") + } + monkeypatch.setattr(manager_module, "compute_key_lookup", lambda value: "lk") + monkeypatch.setattr( + manager_module, "verify_api_key", lambda value, h: (value, h) == ("k", "idx-hash") + ) + out = m.authenticate("k") + assert out is not None + assert out.matched_slot == "current" + + def test_legacy_hashed_match_is_current(self, monkeypatch: pytest.MonkeyPatch) -> None: + m = _build_manager() + m._hashed_keys = [_key(key=None, key_hash="legacy-hash", matched_slot="previous")] + monkeypatch.setattr(manager_module, "compute_key_lookup", lambda value: "no-hit") + monkeypatch.setattr( + manager_module, "verify_api_key", lambda value, h: (value, h) == ("k", "legacy-hash") + ) + out = m.authenticate("k") + assert out is not None + assert out.matched_slot == "current" + + +# --------------------------------------------------------------------------- # +# load(): rate-limit windows are carried over by bucket name. +# --------------------------------------------------------------------------- # + + +class TestLoadCarriesRateWindowsByBucket: + def test_still_configured_key_keeps_its_full_window_across_load(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", [_entry(key_id="kid-a", key="plain-a", rate_limit_rpm=2)] + ) + m = _build_manager(api_keys_path=keys_file) + m.load() + tenant_key = m.authenticate("plain-a") + assert tenant_key is not None + assert [m.is_rate_limited(tenant_key) for _ in range(2)] == [False, False] + m.load() + assert m.is_rate_limited(tenant_key) is True + + def test_removed_key_loses_its_window(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", + [_entry(key_id="kid-a", key="plain-a"), _entry(key_id="kid-b", key="plain-b")], + ) + m = _build_manager(api_keys_path=keys_file) + m.load() + m._rate_windows["kid:kid-a"] = [1_000.0] + m._rate_windows["kid:kid-b"] = [1_000.0] + _write_key_file(keys_file, [_entry(key_id="kid-a", key="plain-a")]) + m.load() + assert dict(m._rate_windows) == {"kid:kid-a": [1_000.0]} + + def test_no_window_is_named_by_a_plaintext_key(self, tmp_path: Path) -> None: + keys_file = _write_key_file( + tmp_path / "api_keys.yaml", [_entry(key_id="kid-a", key="plain-a")] + ) + m = _build_manager(api_keys_path=keys_file) + m._rate_windows["plain-a"] = [1_000.0] + m._rate_windows["kid:kid-a"] = [1_000.0] + m.load() + assert dict(m._rate_windows) == {"kid:kid-a": [1_000.0]} + + +# --------------------------------------------------------------------------- # +# authenticate(): a skipped entry never ends a scan. +# --------------------------------------------------------------------------- # + + +class TestAuthenticateScanOrder: + def test_plaintext_scan_steps_over_an_entry_without_a_runtime_key(self) -> None: + m = _build_manager() + m.keys_by_value = { + "hash-only": _key(key=None, key_hash="h"), + "plain-a": _key(key="plain-a", tenant="acme"), + } + out = m.authenticate("plain-a") + assert out is not None + assert out.tenant == "acme" + + def _rotating(self, index: int, **previous: object) -> TenantKey: + entry = _key(key=None, key_hash=f"cur-{index}", tenant=f"t{index}") + return entry.model_copy(update={"previous_key_hash": f"prev-{index}", **previous}) + + def test_legacy_previous_scan_steps_over_an_inactive_entry( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + m = _build_manager() + inactive, match = self._rotating(0), self._rotating(1) + m._loaded_keys = [inactive, match] + monkeypatch.setattr(manager_module, "compute_key_lookup", lambda value: "no-hit") + monkeypatch.setattr(m._key_rotator, "is_previous_key_active", lambda item: item is match) + monkeypatch.setattr( + manager_module, "verify_api_key", lambda value, h: (value, h) == ("old", "prev-1") + ) + out = m.authenticate("old") + assert out is not None + assert (out.tenant, out.matched_slot) == ("t1", "previous") + + def test_legacy_previous_scan_steps_over_an_indexed_entry( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + m = _build_manager() + m._loaded_keys = [self._rotating(0, previous_key_lookup="lk-0"), self._rotating(1)] + monkeypatch.setattr(manager_module, "compute_key_lookup", lambda value: "no-hit") + monkeypatch.setattr(m._key_rotator, "is_previous_key_active", lambda item: True) + monkeypatch.setattr( + manager_module, "verify_api_key", lambda value, h: (value, h) == ("old", "prev-1") + ) + out = m.authenticate("old") + assert out is not None + assert (out.tenant, out.matched_slot) == ("t1", "previous") + + +# --------------------------------------------------------------------------- # +# check_rate_limit(): what the Redis limiter is asked, and when it is trusted. +# --------------------------------------------------------------------------- # + + +class TestCheckRateLimitDecisions: + @pytest.mark.asyncio + async def test_limiter_is_asked_about_the_key_id_bucket(self) -> None: + limiter = _ScriptedLimiter((True, 4, 77)) + m = _build_manager(rate_limiter=limiter) + await m.check_rate_limit(_key(key_id="kid-7", rate_limit_rpm=5)) + assert limiter.asked == [("kid:kid-7", 5)] + + @pytest.mark.asyncio + async def test_partial_quota_answer_passes_through_verbatim(self) -> None: + m = _build_manager(rate_limiter=_ScriptedLimiter((True, 4, 77))) + assert await m.check_rate_limit(_key(rate_limit_rpm=5)) == (True, 4, 77) + + @pytest.mark.asyncio + async def test_refusal_passes_through_verbatim(self) -> None: + m = _build_manager(rate_limiter=_ScriptedLimiter((False, 0, 77))) + assert await m.check_rate_limit(_key(rate_limit_rpm=5)) == (False, 0, 77) + + @pytest.mark.asyncio + async def test_limiter_without_a_redis_handle_passes_through(self) -> None: + m = _build_manager(rate_limiter=_ScriptedLimiter((True, 5, 77), with_redis=False)) + assert await m.check_rate_limit(_key(rate_limit_rpm=5)) == (True, 5, 77) + + @pytest.mark.asyncio + async def test_secondary_window_is_per_bucket(self) -> None: + m = _build_manager(rate_limiter=_FullRemainingLimiter()) + first = await m.check_rate_limit(_key(key_id="kid-a", rate_limit_rpm=1)) + second = await m.check_rate_limit(_key(key_id="kid-b", rate_limit_rpm=1)) + assert (first, second) == ((True, 0, 1_060), (True, 0, 1_060)) + + @pytest.mark.asyncio + async def test_secondary_window_drops_a_stamp_exactly_at_the_cutoff(self) -> None: + clock = FrozenClock(1_000.0) + m = _build_manager(time_source=clock, rate_limiter=_FullRemainingLimiter()) + tenant_key = _key(key_id="kid-a", rate_limit_rpm=1) + assert await m.check_rate_limit(tenant_key) == (True, 0, 1_060) + clock.now = 1_000.0 + DEFAULT_RATE_LIMIT_WINDOW_SECONDS # cutoff == first stamp + assert await m.check_rate_limit(tenant_key) == (True, 0, 1_120) + + +# --------------------------------------------------------------------------- # +# _legacy_env_keys(): AGENTFLOW_API_KEYS parsing. +# --------------------------------------------------------------------------- # + + +class TestLegacyEnvKeyParsing: + def test_empty_middle_segment_is_skipped_and_later_keys_still_load( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:a, ,k2:b") + keys = _build_manager()._legacy_env_keys() + assert [(k.key, k.name) for k in keys] == [("k1", "a"), ("k2", "b")] + + def test_first_colon_separates_key_from_a_name_that_has_colons( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:team:alpha") + keys = _build_manager()._legacy_env_keys() + assert [(k.key, k.name) for k in keys] == [("k1", "team:alpha")] + + def test_created_at_is_the_utc_calendar_date(self, monkeypatch: pytest.MonkeyPatch) -> None: + class _JustAfterUtcMidnight(datetime): + # 00:00:05 UTC on 2 March is still 1 March on a wall clock west of UTC. + @classmethod + def now(cls, tz: tzinfo | None = None) -> Self: + if tz is None: + return cls(2026, 3, 1, 19, 0, 5) + return cls(2026, 3, 2, 0, 0, 5, tzinfo=UTC) + + monkeypatch.setattr(manager_module, "datetime", _JustAfterUtcMidnight) + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:a") + [k] = _build_manager()._legacy_env_keys() + assert k.created_at == date(2026, 3, 2) + + def test_env_key_reads_rate_limit_rpm_from_the_module_global( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + # TenantKey's Field default is the import-time value; the call in + # _legacy_env_keys reads the module global at load time. + monkeypatch.setattr(manager_module, "DEFAULT_RATE_LIMIT_RPM", 7) + monkeypatch.setenv("AGENTFLOW_API_KEYS", "k1:bot") + m = _build_manager() + m.load() + assert m.keys_by_value["k1"].rate_limit_rpm == 7 diff --git a/tests/unit/test_auth_rate_window_reload.py b/tests/unit/test_auth_rate_window_reload.py index 5bfe294e..72ec366a 100644 --- a/tests/unit/test_auth_rate_window_reload.py +++ b/tests/unit/test_auth_rate_window_reload.py @@ -9,8 +9,8 @@ both readers of the window: ``is_rate_limited()`` and the secondary check in ``check_rate_limit()``. -Keys live in a tmp key file with an explicit ``key_id``: environment keys get -a random key_id on every load (until T-47), so they cannot pin a bucket here. +Keys live in a tmp key file with an explicit ``key_id``, so each test names its +bucket directly. """ from __future__ import annotations From 54919f10fd50d1599231b142f2a114fbc467fb03 Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 19:56:54 -0400 Subject: [PATCH 8/9] fix(mutation): stop mutation_local counting a missing temp dir as a kill scripts/mutation_local.py scored every mutant that reached a tmp_path test as killed. measure_module() gives each mutant a --basetemp inside a per-run scratch directory that nothing created, and pytest creates --basetemp with a non-recursive mkdir. The first tmp_path fixture then errored at setup, pytest exited 1 under -x, and the runner counted a kill. A mutant that survived every test before that fixture came back killed, equivalents included, so a local "all killed" said nothing about a gate file that uses tmp_path. run_mutant() now creates that parent before it starts pytest. Two tests call run_mutant() on a real one-test file with the parent missing. A passing tmp_path test is scored survived. A failing one is still killed, and the test checks that its body ran, so the kill comes from the test and not from setup. Both tests fail on HEAD. grok's run of six manager.py mutants through the script now agrees with CI run 34542418689: the three named equivalents survive and the three killable mutants are killed. Written by grok on its second account. Reviewed by codex, which returned no findings and no comments, so the orchestrator also read the diff. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UUTW71FTTNxbF8JAKwv3Am --- CHANGELOG.md | 11 ++++++ scripts/mutation_local.py | 6 +++- tests/unit/test_mutation_local.py | 59 +++++++++++++++++++++++++++++-- 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab6e2060..341d01f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] +### Fixed — mutation_local no longer scores a missing pytest temp directory as a kill + +* **`scripts/mutation_local.py` recorded every mutant that reached a + `tmp_path` test as killed.** `measure_module()` handed pytest a `--basetemp` + under a scratch directory it never created, and pytest `mkdir`s that path + without parents, so the first `tmp_path` fixture failed at setup and pytest + exited 1. Under `-x` that false kill covered every mutant that survived the + tests before it. `run_mutant()` now creates the `--basetemp` parent before it + starts pytest, so a mutant that reaches a passing `tmp_path` test is scored + survived, and a real failure is still a kill. + ### Quality — the auth-manager mutation lane pins what its survivors changed * **`serving/api/auth/manager.py` sat two points above its 0.80 threshold.** diff --git a/scripts/mutation_local.py b/scripts/mutation_local.py index 42704f34..13fc361b 100644 --- a/scripts/mutation_local.py +++ b/scripts/mutation_local.py @@ -495,8 +495,12 @@ def run_mutant( Each mutant gets its own `--basetemp`: pytest wipes and recreates that directory at startup, so concurrent runs sharing one abort each other and - exit 2. + exit 2. pytest creates `--basetemp` itself with a non-recursive mkdir, so + the parent has to exist before pytest starts -- otherwise the first + `tmp_path` fixture errors at setup, pytest exits 1, and the mutant is + scored killed. """ + basetemp.parent.mkdir(parents=True, exist_ok=True) command = [ python, "-m", diff --git a/tests/unit/test_mutation_local.py b/tests/unit/test_mutation_local.py index bfdf3fe8..f7aeb74b 100644 --- a/tests/unit/test_mutation_local.py +++ b/tests/unit/test_mutation_local.py @@ -1,8 +1,10 @@ """Unit tests for the local mutation driver's own logic. -Everything that costs minutes -- the mutmut engine and the pytest subprocesses --- is stubbed here. A real mutation run belongs on the command line -(`python scripts/mutation_local.py --module ...`), not in the unit suite. +Everything that costs minutes -- the mutmut engine and a full mutation run -- +is stubbed here. `run_mutant` is exercised against a one-test file so the +`--basetemp` parent is a real pytest mkdir, not a stub. A real mutation run +belongs on the command line (`python scripts/mutation_local.py --module ...`), +not in the unit suite. """ from __future__ import annotations @@ -325,6 +327,57 @@ def test_two_invocations_sharing_a_workspace_get_separate_basetemps( assert not set(_basetemps(first)) & set(_basetemps(second)) +def _run_mutant_against_tmp_path_test(tmp_path: Path, body: str) -> tuple[str, int | None]: + """Call `run_mutant` the way `measure_module` does: the basetemp parent is missing.""" + test_file = tmp_path / "test_tmp_path_probe.py" + test_file.write_text( + f"from pathlib import Path\n\ndef test_uses_tmp_path(tmp_path):\n{body}", + encoding="utf-8", + ) + basetemp = tmp_path / "scratch" / "run-missing" / "mutant0" + assert not basetemp.parent.exists() + return mutation_local.run_mutant( + tmp_path, + (test_file.name,), + "pkg.thing.x__mutmut_1", + python=sys.executable, + shim_dir=tmp_path / "shim", + basetemp=basetemp, + timeout=20.0, + ) + + +def test_run_mutant_survives_a_passing_tmp_path_test_when_the_basetemp_parent_is_missing( + tmp_path: Path, +): + """pytest mkdir()s --basetemp without parents; a missing parent is not a kill.""" + name, exit_code = _run_mutant_against_tmp_path_test( + tmp_path, + " (tmp_path / 'marker').write_text('ok')\n", + ) + + assert name == "pkg.thing.x__mutmut_1" + assert exit_code == 0 + assert mutation_local.classify_exit_code(exit_code) == "survived" + + +def test_run_mutant_still_kills_a_failing_tmp_path_test_when_the_basetemp_parent_is_missing( + tmp_path: Path, +): + """Creating the parent must not turn a real failure into a survival.""" + name, exit_code = _run_mutant_against_tmp_path_test( + tmp_path, + " Path('ran').write_text('yes')\n" + " (tmp_path / 'marker').write_text('ok')\n" + " assert False\n", + ) + + assert name == "pkg.thing.x__mutmut_1" + assert (tmp_path / "ran").read_text(encoding="utf-8") == "yes" + assert exit_code == 1 + assert mutation_local.classify_exit_code(exit_code) == "killed" + + def test_measure_module_scores_verdicts_and_never_counts_an_error_as_a_kill( monkeypatch, tmp_path: Path, From dc911d3b354824635397f02a29bde23367f3eeb6 Mon Sep 17 00:00:00 2001 From: JuliaEdom Date: Thu, 10 Sep 2026 21:27:01 -0400 Subject: [PATCH 9/9] test(mutation): hold manager.py to 0.90 now that its survivors are named CI run 34542418689 scored serving/api/auth/manager.py 98.4% (553 killed of 562) on 77825f6, the commit that pinned its survivors. The nine left are the equivalents named in tests/unit/test_auth_manager_mutation.py. The threshold goes from 0.80 to 0.90, the bar the other serving modules hold. That is 8.4 points under the measured score, so the named equivalents cannot fail the gate, while a real loss of killed mutants still does. The comment above the module's ModuleTarget said that equivalents no behaviour test could kill dominated the survivors, and it named the structured-logging and Redis-URL mutants among them. T-48's tests killed those. The comment now gives the measured score, the run and the commit, and lists the nine equivalents by function with a short reason each; the test file's docstring keeps the full reasons. The [tool.mutmut] comment in pyproject.toml repeated the old claim ("an honest 0.80") and now points to scripts/mutation_report.py instead. Written by grok on its second account. codex, the first reviewer in the chain, hung without output for 38 minutes and was cancelled, so opus-cli reviewed the slice. Its first pass found four problems: - the CHANGELOG entry contradicted the comment it described; - pyproject.toml still said "an honest 0.80"; - the two __init__ survivors were listed out of source order; - one phrase was garbled. grok fixed all four in one repair pass. The second pass found them fixed and flagged one hard-to-read CHANGELOG sentence, which a second repair pass split. The third pass found the rewrite correct. It left one advisory note (sev 2): the entry names T-48, an internal task id that no other CHANGELOG entry carries. The id stays, because removing it would have taken a closing pass beyond the review limit. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UUTW71FTTNxbF8JAKwv3Am --- CHANGELOG.md | 15 +++++++++++++++ pyproject.toml | 10 +++------- scripts/mutation_report.py | 31 +++++++++++++++---------------- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 341d01f2..0480dd3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ All notable changes to AgentFlow are documented in this file. ## [Unreleased] +### Quality — the auth-manager mutation threshold ratchets to 0.90 + +* **`serving/api/auth/manager.py` now gates at 0.90.** CI run 34542418689 on + 77825f6 scored it 98.4% (553 killed of 562). The nine survivors are the + named residue in `tests/unit/test_auth_manager_mutation.py`. The old + comment above its `ModuleTarget` in `scripts/mutation_report.py` called + the structured-logging and Redis-URL survivors equivalents. T-48's + behaviour tests killed all of them except the upper-cased Redis URL + default, which is one of the nine. The comment now gives the measured + score and names the nine equivalent survivors, pointing to the test + file's docstring for their reasons. 0.90 is the bar the other serving + modules hold, and it sits 8.4 points under the measured 98.4%, so the + nine named equivalents cannot fail the gate while a real loss of killed + mutants still does. + ### Fixed — mutation_local no longer scores a missing pytest temp directory as a kill * **`scripts/mutation_local.py` recorded every mutant that reached a diff --git a/pyproject.toml b/pyproject.toml index 02e673ad..1939d271 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -338,13 +338,9 @@ asyncio_mode = "auto" # starting with `src.`, which (not duckdb) was the real blocker. (The gate runner # keeps mutmut workspaces free of relative pytest --basetemp overrides because # under py3.11 they break coverage->mutant mapping for file-I/O targets like -# key_rotation; see scripts/mutation_report.py.) manager.py runs at an honest 0.80 -# threshold (not 0.90): it is a -# large stateful auth class whose residual survivors are equivalent mutants -# (structured-logging args, model_copy updates equal to their defaults, redis-url -# strings masked by the `_redis = None` override, the env-only-dead write path) -- -# every behaviour-reachable mutant, including every auth-bypass and throttle -# off-by-one, is killed. See scripts/mutation_report.py. +# key_rotation; see scripts/mutation_report.py.) manager.py gates at 0.90 like +# the other serving modules; scripts/mutation_report.py gives its measured +# score and names its nine equivalent survivors. paths_to_mutate = [ "src/agentflow_runtime/serving/api/auth/manager.py", "src/agentflow_runtime/serving/api/auth/key_rotation.py", diff --git a/scripts/mutation_report.py b/scripts/mutation_report.py index 3d2ecd94..8c1e3990 100644 --- a/scripts/mutation_report.py +++ b/scripts/mutation_report.py @@ -76,23 +76,22 @@ class ModuleTarget: threshold=0.90, tests=("tests/unit/test_nl_queries_mutation.py",), ), - # manager.py runs at 0.80, not the 0.90 the pure-function guards (sql_guard, - # sql_builder, ...) hold. It is a ~400-line stateful auth class whose - # surviving mutants are dominated by EQUIVALENTS that no behaviour-level test - # can kill: structured-logging arguments (the auth logger event names / kwargs), - # `model_copy(update=...)` dicts whose mutated field equals its default - # ("matched_slot" already defaults to "current"; "key"==api_key on a plaintext - # match), the redis-url strings masked by the `_redis = None` override under the - # duckdb-free harness, and the config-file write path that is dead under the - # env-only test. Every BEHAVIOUR-reachable mutant is killed -- crucially every - # auth bypass (the verify_api_key argument-swap mutants on the indexed / legacy - # / previous-key paths and in _matches_key_material) and every rate-limit / - # failed-auth throttle off-by-one. Local mutmut (py3.10) scores 405/483 = 83.9%; - # 0.80 leaves headroom for equivalent-mutant noise while still enforcing a real - # floor (the do-nothing baseline was 76.5%). key_rotation is the next target and - # stays declared-only until it gets its own duckdb-free test. + # manager.py scored 553/562 killed (98.4%) on CI run 34542418689 against + # commit 77825f6. The nine survivors are the named residue in the module + # docstring of tests/unit/test_auth_manager_mutation.py, grouped: + # __init__ 55 and 93 -- _key_store_readonly_skip_logged flag and the + # upper-cased Redis URL default; + # _load_config 3 and 5 -- encoding="utf-8" case / None; + # _sweep_expired_windows 10 and 20 -- pop(key, None) race guard; + # authenticate 10 and 11 -- model_copy of a just-proven-equal key; + # load 1 -- skipped_readonly_write truthiness. The docstring has the + # one-line reason for each. Every behaviour-reachable mutant is killed. + # The threshold is 0.90 because that is the bar the other serving modules + # hold; it sits 8.4 points under the measured 98.4%, so the nine named + # equivalents cannot fail the gate while a real loss of killed mutants + # still does. Path("serving/api/auth/manager.py"): ModuleTarget( - threshold=0.80, + threshold=0.90, tests=("tests/unit/test_auth_manager_mutation.py",), ), # key_rotation runs at 0.90. Its residual survivors (local mutmut: 21 of 365)