Skip to content

Commit 32955f7

Browse files
JuliaEdomclaude
andcommitted
test(auth): make the auth-manager coverage gate measure what it claims
The gate runs the unit files it names, so a dedicated file that is not named buys the module nothing. tests/unit/test_key_store_readonly.py arrived with the read-only Secret mount work (F-02 B, 2026-08-23) and was never added to the list, so the ~50 statements of write-probe logic that landed with it counted as uncovered. serving/api/auth/manager.py measured 82% against its 90% gate -- not the 94% the step's own comment claims. Two weeks of that were invisible because an earlier step in the same job was failing: a job step is a sequence, and a failed step means every later step never runs. One red gate hides every gate behind it. Fixing the earlier failure is what surfaced this one. The branches nothing covered are the fail-closed ones. Whether an OSError means "read-only mount" decides between a 409 and a propagated error, and only a permission failure may become the 409 -- answering it for an EIO tells an operator whose disk is broken that their key store is merely read-only. Also untested: the parent-directory probe for a store file that does not exist yet (a first-boot Secret mount), the errno/winerror shapes one platform raises and the other cannot construct, and the load() path that downgrades the store in place when the write is denied between probe and write instead of crashing the pod at startup. test_auth_key_store_probe.py the probe and its classifier test_auth_manager_key_resolution.py legacy rotation-grace scan, the plaintext-cache guard, the sweep that trims rather than drops a live window, the batch debit, and the _rate_limit_key fallbacks that keep a plaintext key out of a Redis key name (S-6) Both files are pure calls -- no TestClient, no Redis, no event loop -- so they carry none of the teardown crashes that keep middleware cases out of the per-module gates. manager.py is at 97% with all eight files. What is left is the SIGHUP handler that does not exist on every platform, two load() branches that need the store to flip between probe and write, and thin delegations to KeyRotator and usage_table that the integration and e2e suites cover. Verified: the gate command as CI runs it reports 97% and exits 0 under --fail-under=90; ruff lint and format clean; the ci.yml gate contract in test_coverage_policy.py and the docs/release ratchets still pass (235 passed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018ybRVCaL1Lyd9d5kFTndh3
1 parent bc3d64b commit 32955f7

4 files changed

Lines changed: 533 additions & 4 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -300,18 +300,35 @@ jobs:
300300
run: |
301301
# Security-critical auth manager (key match/verify, tenant isolation,
302302
# rate-limit/failed-auth windows, rotation grace) and a mutmut target;
303-
# the gate runs its dedicated unit files. Module coverage is 94% so the
303+
# the gate runs its dedicated unit files. Module coverage is 97% so the
304304
# 90% gate keeps a cushion; the remaining gap is the platform-divergent
305-
# SIGHUP handler and bcrypt rotation paths the integration/e2e auth
306-
# suites cover.
305+
# SIGHUP handler, the two read-only-store branches load() reaches only
306+
# when the store flips between probe and write, and the thin
307+
# KeyRotator/usage-table delegations the integration/e2e auth suites
308+
# cover.
309+
#
310+
# This list IS the gate: a dedicated unit file that is not named here
311+
# buys the module nothing. test_key_store_readonly.py arrived with the
312+
# read-only Secret mount work (F-02 B, 2026-08-23) and was never added,
313+
# which by itself took the module from 94% to 82% -- unnoticed, because
314+
# an earlier step in this job was failing and this one never ran.
307315
#
308316
# NOTE: unlike the other per-module gates this uses `coverage run` +
309317
# `coverage report --include`, NOT `pytest --cov=<module>`. The auth
310318
# manager pulls in duckdb (usage table), and pytest-cov's source
311319
# instrumentation of a duckdb-importing module trips duckdb's lazy
312320
# `_duckdb._sqltypes` import at COLLECTION time, both locally and on CI
313321
# runners. `coverage run` imports duckdb normally and avoids the break.
314-
python -m coverage run -m pytest tests/unit/test_auth.py tests/unit/test_auth_manager_pure_logic.py tests/unit/test_auth_manager_memory_bounds.py tests/unit/test_auth_hashed_key_guidance.py tests/unit/test_auth_argon2_lookup.py -p no:schemathesis
322+
python -m coverage run -m pytest \
323+
tests/unit/test_auth.py \
324+
tests/unit/test_auth_manager_pure_logic.py \
325+
tests/unit/test_auth_manager_memory_bounds.py \
326+
tests/unit/test_auth_hashed_key_guidance.py \
327+
tests/unit/test_auth_argon2_lookup.py \
328+
tests/unit/test_auth_key_store_probe.py \
329+
tests/unit/test_auth_manager_key_resolution.py \
330+
tests/unit/test_key_store_readonly.py \
331+
-p no:schemathesis
315332
python -m coverage report --include="*/serving/api/auth/manager.py" --show-missing --fail-under=90
316333
- name: Run key rotation coverage gate
317334
run: |

‎CHANGELOG.md‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,34 @@ All notable changes to AgentFlow are documented in this file.
44

55
## [Unreleased]
66

7+
### Quality — the auth-manager coverage gate was measuring 82%, not 94%
8+
9+
* **The file list *is* the gate.** `Run auth manager coverage gate` runs the
10+
unit files it names, so a dedicated file that is not named buys the module
11+
nothing. `tests/unit/test_key_store_readonly.py` arrived with the read-only
12+
Secret mount work (F-02 B, 2026-08-23) and was never added to the list, so
13+
the ~50 statements of write-probe logic that landed with it counted as
14+
uncovered: the module measured 82% against its 90% gate, not the 94% the
15+
step's own comment claimed.
16+
* **It stayed invisible for two weeks** because an earlier step in the same
17+
job was failing, and a job step that fails means every later step never
18+
runs. One red gate hides every gate behind it — this one only surfaced once
19+
the earlier failure was fixed.
20+
* **The uncovered branches were the fail-closed ones.** Whether an `OSError`
21+
means "read-only mount" decides between a 409 and a propagated error, and a
22+
non-permission failure must propagate — answering 409 tells an operator
23+
whose disk is broken that their key store is merely read-only. Also
24+
untested: the parent-directory probe for a key store that does not exist
25+
yet (a first-boot Secret mount), and the `load()` path that downgrades the
26+
store in place when the write is denied between probe and write instead of
27+
crashing the pod at startup.
28+
* **Two new unit files cover them** — `test_auth_key_store_probe.py` and
29+
`test_auth_manager_key_resolution.py`, both free of TestClient and Redis so
30+
the gate can run them — together with the legacy rotation-grace scan, the
31+
batch rate-limit debit, and the `_rate_limit_key` fallbacks that keep a
32+
plaintext key out of a Redis key name. The module is at 97%; the remaining
33+
gap is the platform-divergent SIGHUP handler and thin delegations.
34+
735
### Security — the admin surface now leaves an audit trail (FB-10)
836

937
* **`require_admin_key` counted its refusals and wrote nothing else.** It
Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,250 @@
1+
"""Fail-closed branches of the key-store writability probe (auth.manager).
2+
3+
The probe decides whether key-lifecycle mutations can persist: a read-only
4+
Secret mount must answer 409 instead of raising a filesystem error (audit
5+
F-02 B). ``tests/unit/test_key_store_readonly.py`` drives the happy paths
6+
through a real ``chmod``-ed file; the branches here are the ones a filesystem
7+
will not produce on demand -- a non-permission ``OSError`` that has to
8+
propagate rather than be read as "read-only", the errno/winerror shapes only
9+
one platform raises, and a store path whose file does not exist yet.
10+
11+
Pure module-level calls: no TestClient, no event loop, so the CI auth-manager
12+
coverage gate can run this file without the teardown crashes that keep
13+
middleware cases out of the per-module gates.
14+
"""
15+
16+
from __future__ import annotations
17+
18+
import errno
19+
from pathlib import Path
20+
from typing import IO, Any
21+
22+
import pytest
23+
import structlog
24+
25+
from agentflow_runtime.serving.api.auth import AuthManager
26+
from agentflow_runtime.serving.api.auth.manager import (
27+
is_permission_denied,
28+
probe_key_store_writable,
29+
)
30+
31+
SEED_WITHOUT_KEY_IDS = (
32+
"keys:\n"
33+
' - key: "probe-acme-key"\n'
34+
' name: "Probe Agent"\n'
35+
' tenant: "acme"\n'
36+
" rate_limit_rpm: 100\n"
37+
" allowed_entity_types: null\n"
38+
' created_at: "2026-04-10"\n'
39+
)
40+
41+
42+
class _WinErrorOSError(OSError):
43+
"""An OSError carrying a Windows error code on any platform.
44+
45+
``winerror`` is a read-only member of ``OSError`` on Windows and absent on
46+
POSIX, so neither host can populate it on a plain instance. The subclass
47+
attribute shadows both, which is what lets one test assert the same branch
48+
on both platforms.
49+
"""
50+
51+
winerror = 5
52+
53+
54+
@pytest.fixture(autouse=True)
55+
def _uncached_auth_logger(monkeypatch: pytest.MonkeyPatch) -> None:
56+
from agentflow_runtime.serving.api import auth as auth_package
57+
58+
monkeypatch.setattr(auth_package, "logger", structlog.get_logger())
59+
60+
61+
def test_a_non_oserror_is_never_read_as_permission_denied() -> None:
62+
assert is_permission_denied(ValueError("not a filesystem failure")) is False
63+
64+
65+
def test_read_only_filesystem_errno_is_permission_denied() -> None:
66+
exc = OSError(errno.EROFS, "Read-only file system")
67+
68+
# EROFS is the one permission errno CPython does not map onto
69+
# PermissionError, so it reaches the errno check instead of the isinstance
70+
# short-circuit above it.
71+
assert not isinstance(exc, PermissionError)
72+
assert is_permission_denied(exc) is True
73+
74+
75+
def test_windows_access_denied_winerror_is_permission_denied() -> None:
76+
exc = _WinErrorOSError(errno.EIO, "Access is denied")
77+
78+
assert not isinstance(exc, PermissionError)
79+
assert exc.errno not in {errno.EACCES, errno.EPERM}
80+
assert is_permission_denied(exc) is True
81+
82+
83+
def test_an_unrelated_oserror_is_not_permission_denied() -> None:
84+
assert is_permission_denied(OSError(errno.EIO, "I/O error")) is False
85+
86+
87+
def test_a_missing_store_file_probes_its_parent_directory(tmp_path: Path) -> None:
88+
store = tmp_path / "api_keys.yaml"
89+
90+
assert probe_key_store_writable(store) is True
91+
# The probe file is temporary: a writable verdict must not leave a stray
92+
# dotfile next to the operator's key store.
93+
assert list(tmp_path.iterdir()) == []
94+
95+
96+
def test_a_missing_store_path_probes_the_nearest_existing_ancestor(tmp_path: Path) -> None:
97+
store = tmp_path / "not-created-yet" / "deeper" / "api_keys.yaml"
98+
99+
assert probe_key_store_writable(store) is True
100+
assert list(tmp_path.iterdir()) == []
101+
102+
103+
def test_a_store_path_with_no_existing_ancestor_is_not_writable(
104+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
105+
) -> None:
106+
# A mount that vanished under the process: nothing on the path exists, so
107+
# there is nowhere to write and no exception to classify.
108+
monkeypatch.setattr(Path, "exists", lambda self: False)
109+
110+
assert probe_key_store_writable(tmp_path / "api_keys.yaml") is False
111+
112+
113+
def test_a_parent_that_refuses_the_probe_file_is_not_writable(
114+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
115+
) -> None:
116+
real_open = Path.open
117+
118+
def _deny_create(self: Path, mode: str = "r", *args: Any, **kwargs: Any) -> IO[Any]:
119+
if "x" in mode:
120+
raise PermissionError(errno.EACCES, "Permission denied")
121+
return real_open(self, mode, *args, **kwargs)
122+
123+
monkeypatch.setattr(Path, "open", _deny_create)
124+
125+
assert probe_key_store_writable(tmp_path / "api_keys.yaml") is False
126+
127+
128+
def test_an_unwritable_existing_store_file_is_not_writable(
129+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
130+
) -> None:
131+
store = tmp_path / "api_keys.yaml"
132+
store.write_text("keys: []\n", encoding="utf-8")
133+
134+
def _deny_append(self: Path, mode: str = "r", *args: Any, **kwargs: Any) -> IO[Any]:
135+
raise PermissionError(errno.EACCES, "Permission denied")
136+
137+
monkeypatch.setattr(Path, "open", _deny_append)
138+
139+
assert probe_key_store_writable(store) is False
140+
141+
142+
def test_a_non_permission_failure_on_an_existing_store_file_propagates(
143+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
144+
) -> None:
145+
store = tmp_path / "api_keys.yaml"
146+
store.write_text("keys: []\n", encoding="utf-8")
147+
148+
def _io_error(self: Path, mode: str = "r", *args: Any, **kwargs: Any) -> IO[Any]:
149+
raise OSError(errno.EIO, "I/O error")
150+
151+
monkeypatch.setattr(Path, "open", _io_error)
152+
153+
# A failing disk is not a read-only mount. Reading it as one would answer
154+
# 409 "the key store is read-only" to an operator whose store is broken.
155+
with pytest.raises(OSError) as excinfo:
156+
probe_key_store_writable(store)
157+
assert excinfo.value.errno == errno.EIO
158+
159+
160+
def test_a_permission_denied_stat_reports_not_writable(
161+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
162+
) -> None:
163+
def _denied(self: Path) -> bool:
164+
raise PermissionError(errno.EACCES, "Permission denied")
165+
166+
monkeypatch.setattr(Path, "exists", _denied)
167+
168+
assert probe_key_store_writable(tmp_path / "api_keys.yaml") is False
169+
170+
171+
def test_a_non_permission_stat_failure_propagates(
172+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
173+
) -> None:
174+
def _io_error(self: Path) -> bool:
175+
raise OSError(errno.EIO, "I/O error")
176+
177+
monkeypatch.setattr(Path, "exists", _io_error)
178+
179+
with pytest.raises(OSError) as excinfo:
180+
probe_key_store_writable(tmp_path / "api_keys.yaml")
181+
assert excinfo.value.errno == errno.EIO
182+
183+
184+
def test_key_store_writable_probes_on_first_read(tmp_path: Path) -> None:
185+
store = tmp_path / "api_keys.yaml"
186+
store.write_text(SEED_WITHOUT_KEY_IDS, encoding="utf-8", newline="\n")
187+
manager = AuthManager(
188+
api_keys_path=store,
189+
db_path=tmp_path / "usage.duckdb",
190+
admin_key="admin-secret",
191+
)
192+
193+
# load() has not run, so nothing has probed yet: this property is what
194+
# every admin mutation asks before it decides 409 vs. write.
195+
assert manager._key_store_writable is None
196+
assert manager.key_store_writable is True
197+
assert manager._key_store_writable is True
198+
199+
200+
def test_a_write_denied_during_load_marks_the_store_read_only(tmp_path: Path) -> None:
201+
store = tmp_path / "api_keys.yaml"
202+
store.write_text(SEED_WITHOUT_KEY_IDS, encoding="utf-8", newline="\n")
203+
manager = AuthManager(
204+
api_keys_path=store,
205+
db_path=tmp_path / "usage.duckdb",
206+
admin_key="admin-secret",
207+
)
208+
manager.security_policy.bcrypt_rounds = 4
209+
210+
def _denied(_config: object) -> None:
211+
raise PermissionError(errno.EACCES, "Permission denied")
212+
213+
# The seed carries no key_id, so ensure_key_ids() mutates the config and
214+
# load() tries to persist it. A store that turns read-only between the
215+
# probe and the write must downgrade in place, not crash at startup.
216+
manager._key_rotator.write_config = _denied # type: ignore[method-assign]
217+
218+
manager.load()
219+
220+
assert manager.key_store_writable is False
221+
assert manager.configured_key_count == 1
222+
223+
224+
def test_a_non_permission_failure_creating_the_probe_file_propagates(
225+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
226+
) -> None:
227+
def _io_error(self: Path, mode: str = "r", *args: Any, **kwargs: Any) -> IO[Any]:
228+
raise OSError(errno.EIO, "I/O error")
229+
230+
monkeypatch.setattr(Path, "open", _io_error)
231+
232+
# Same rule as the existing-file case: only a permission failure means
233+
# "read-only mount". Anything else is a broken store and must surface.
234+
with pytest.raises(OSError) as excinfo:
235+
probe_key_store_writable(tmp_path / "api_keys.yaml")
236+
assert excinfo.value.errno == errno.EIO
237+
238+
239+
def test_a_probe_file_that_cannot_be_removed_still_reports_writable(
240+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
241+
) -> None:
242+
def _unlink_fails(self: Path, missing_ok: bool = False) -> None:
243+
raise OSError(errno.EBUSY, "Device or resource busy")
244+
245+
monkeypatch.setattr(Path, "unlink", _unlink_fails)
246+
247+
# The probe answered its question before the cleanup ran. A failed unlink
248+
# leaves a stray dotfile, which is not a reason to declare the operator's
249+
# writable key store read-only.
250+
assert probe_key_store_writable(tmp_path / "api_keys.yaml") is True

0 commit comments

Comments
 (0)