From d10474a3efeeae10ed9a7fc62b130e5d004a3afd Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 5 Aug 2026 12:21:37 +0300 Subject: [PATCH 1/4] test(edge-cases): audit hash chain + PAT propagation edge/property analysis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /edge-cases pass over the audit-chain (#1985) and PAT-rotation (#1979) features merged to dev. 84 passing cases plus 6 strict-xfails, each naming the issue it pins: - #2015 — enabling the audit hash chain is in-memory only, so a backend restart silently turns the integrity control off. #1985 made verify_chain honest about unhashed ranges; this is why ranges keep going unhashed. - #2016 — a duplicated GITHUB_PAT line survives `count=1` and wins under the agent's last-wins parser, so the agent keeps the revoked token while the rotation reports `updated`. - #2017 — a backslash in the token raises re.error (the line is an re.sub replacement), and the .env writer escapes a quote the reader never unescapes. The PAT contract is stated as a Hypothesis round-trip property against a copy of the agent's own .env parser, since what the agent reads back is the only definition of a successful rotation. It holds for every realistic single-line .env; the xfails are the inputs where it does not. Product code deliberately unchanged — findings are reported, fixing is a separate decision. Co-Authored-By: Claude Opus 5 (1M context) --- tests/registry.json | 27 ++ tests/unit/test_audit_chain_edges.py | 300 ++++++++++++++++++ tests/unit/test_pat_propagation_properties.py | 230 ++++++++++++++ 3 files changed, 557 insertions(+) create mode 100644 tests/unit/test_audit_chain_edges.py create mode 100644 tests/unit/test_pat_propagation_properties.py diff --git a/tests/registry.json b/tests/registry.json index eb177c29f..b622f804e 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -1553,6 +1553,33 @@ "parity" ], "description": "The agent server was outside ent#314's YAML sweep (#1965). utils/safe_yaml.py (PR #1961) put every author-controlled YAML reader in the backend behind one hardened loader, and its AST guard walks the whole backend with an EMPTY allowlist - but it walked _BACKEND.rglob only, so docker/base-image/agent_server/ kept six bare yaml.safe_load calls on documents the backend itself assigns REJECT: template.yaml (x2, credential_requirements_service), skill frontmatter (skill_packaging), dashboard.yaml (compatibility/static_checks) and .trinity/persistent-state.yaml. The vector is amplification at SERIALIZATION, not parse - a 416 B level-6 anchor bomb resolves in ~0.001 s and blows up to ~110 MB when something walks the graph - and the backend proxies /info and /dashboard, so the walk happens in-container then again across the wire. Covers: byte-parity of the vendored loader (the credential_paths.py shape, Invariant #5) plus proof the vendored COPY actually behaves - refuses a level-6 bomb under BUDGET, any alias under REJECT, duplicate keys, and still parses an honest document (byte parity is not behaviour parity if the file never imports); each of the four agent-authored sites on the shared loader with its backend counterpart's kind AND policy; /config/agent-config.yaml deliberately BUDGET not REJECT, stated as an exception because the platform writes it and bind-mounts it mode:'ro' so the agent cannot author it, and yaml.dump emits an anchor for any shared object reference - REJECT there would be a self-inflicted outage for no security; no bare safe_load left anywhere in the tree; and HardenedYamlError named in the except arms that previously caught only yaml.YAMLError (it is a ValueError, so without its own arm a refused bomb escapes to the generic handler and surfaces as the unnamed 500 the AC rules out - the trap static_checks._parse_yaml records backend-side). AC #4 end-to-end: a level-6 bomb in a container's template.yaml is refused by BOTH template.yaml readers with the expanded graph never reaching the response, an honest template still serves, and the metrics assertion checks the NAMED refusal rather than has_metrics:False - a bomb parses fine under bare safe_load and yields no metrics: key, so the flag alone passes against the very tree this issue reports. The AST-guard widening itself lives in test_ent314_hardened_yaml.py (both trees, still empty allowlist) rather than here, because splitting a guard across two files is how the second copy stops being run." + }, + { + "file": "unit/test_audit_chain_edges.py", + "feature": "#2015", + "added": "2026-08-05", + "categories": [ + "backend", + "unit", + "security", + "audit", + "edge-cases" + ], + "description": "Edge-case analysis of the audit hash chain as merged by #1985. Covers what the chain can and cannot DETECT (a mutated hashed field, a deleted middle row and a reordered pair are all caught; fields outside _compute_hash's subset - source/actor_email/actor_ip/endpoint/mcp_key_id - are silently mutable with the chain still reporting verified, and tail truncation is undetectable because verification is over a caller-supplied range), _compute_hash's real input domain (details as str vs dict must hash identically or every entry fails verification, key order in a details string, unparseable details hitting the except branch, non-ASCII, missing optional fields, and the four fields read with [] raising KeyError rather than hashing a hole), and range boundaries (inverted/absurd ranges degrade to empty_range, a single-row range never checks a link because of the i>0 guard, counts add up to total_in_range). Carries the xfail(strict) for #2015: enabling the hash chain is in-memory only, so a restart silently disables it, plus a static pin that the enable route persists nothing. Uses asyncio.run, not get_event_loop - the sibling #1984 suite closes the loop, so the latter passes standalone and fails on collection order." + }, + { + "file": "unit/test_pat_propagation_properties.py", + "feature": "#2016", + "added": "2026-08-05", + "categories": [ + "backend", + "unit", + "security", + "credentials", + "property-based", + "edge-cases" + ], + "description": "Round-trip property analysis of GITHUB_PAT propagation as merged by #1979. The contract is stated once as a Hypothesis property - after a rotation the agent reads back exactly the new token under all three key names (GITHUB_PAT/GH_TOKEN/GITHUB_TOKEN) - with the oracle being a byte-faithful copy of the agent-server's own last-wins .env parser, because what the agent sees is the only definition of success for a credential rotation. Plus idempotence, unrelated keys surviving, and the old token not surviving anywhere as text. Deterministic boundaries that hold: empty file, no trailing newline, leading whitespace/tab, commented-out line, present-but-empty, lookalike and prefixed keys, CRLF, append-not-glued. Three xfail(strict) findings: a duplicated GITHUB_PAT line survives count=1 and wins under last-wins parsing so the agent keeps the revoked token while the rotation reports updated (#2016); a backslash in the token raises re.error because the new line is used as an re.sub REPLACEMENT (#2017); and the writer escapes an embedded quote while the reader never unescapes it (#2017, a .env parse-contract issue deliberately left byte-faithful by #1999)." } ] } diff --git a/tests/unit/test_audit_chain_edges.py b/tests/unit/test_audit_chain_edges.py new file mode 100644 index 000000000..f9930b584 --- /dev/null +++ b/tests/unit/test_audit_chain_edges.py @@ -0,0 +1,300 @@ +"""Edge-case analysis of the audit hash chain (`/edge-cases`, 2026-08-05). + +Target: `services/platform_audit_service.PlatformAuditService.verify_chain`, +`_compute_hash` and `enable_hash_chain`, as merged on `dev` by #1985 (issue +#1984). + +#1984's own suite covers the tri-state verdict thoroughly. This file covers the +dimensions it does not: what the chain check can and cannot *detect* (deletion, +reordering, a tampered field the hasher doesn't read), what `_compute_hash` does +with hostile-but-reachable field values, and whether the control that produces +hashes in the first place survives the lifecycle it lives in. + +Cases marked `xfail(strict=True)` are REAL defects, not aspirational tests — +each one names the finding it pins. Per the skill's protocol, product code is +not changed here. +""" + +from __future__ import annotations + +import asyncio +import json +import sys +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parents[2] +_BACKEND_STR = str(_REPO / "src" / "backend") +while _BACKEND_STR in sys.path: + sys.path.remove(_BACKEND_STR) +sys.path.insert(0, _BACKEND_STR) + +pytestmark = pytest.mark.unit + + +@pytest.fixture +def svc(): + try: + from services.platform_audit_service import platform_audit_service + except ImportError: # pragma: no cover — backend venv required + pytest.skip("backend venv required") + return platform_audit_service + + +@pytest.fixture +def hasher(svc): + """`_compute_hash` is a staticmethod on the service, not a module global.""" + return svc._compute_hash + + +@pytest.fixture +def mod(): + try: + import services.platform_audit_service as m + except ImportError: # pragma: no cover + pytest.skip("backend venv required") + return m + + +def _rows(mod, monkeypatch, rows): + monkeypatch.setattr(mod.db, "get_audit_entries_range", + lambda s, e: rows, raising=False) + + +def _row(id_: int, **over) -> dict: + row = { + "id": id_, + "event_id": f"evt-{id_}", + "event_type": "configuration", + "event_action": "settings_change", + "actor_id": "1", + "target_id": None, + "timestamp": f"2026-08-04T00:00:{id_:02d}Z", + "details": None, + "entry_hash": None, + "previous_hash": None, + } + row.update(over) + return row + + +def _chain(hasher, n: int, **over) -> list: + """`n` rows correctly hashed and linked, as `log()` would have written them.""" + out, prev = [], None + for i in range(1, n + 1): + row = _row(i, previous_hash=prev, **over) + row["entry_hash"] = hasher(row) + prev = row["entry_hash"] + out.append(row) + return out + + +def _verify(svc, start=1, end=100): + """`asyncio.run`, not `get_event_loop().run_until_complete`. + + The sibling #1984 suite uses `asyncio.run`, which closes the loop and + clears the current-loop slot — so a `get_event_loop()` here passes when + this file runs alone and raises "no current event loop" the moment the two + are collected together. Collection-order-dependent, i.e. green locally and + red in CI. + """ + return asyncio.run(svc.verify_chain(start, end)) + + +# --------------------------------------------------------------------------- +# What the chain can detect — the reason it exists +# --------------------------------------------------------------------------- + +class TestTamperDetection: + + def test_an_intact_chain_verifies(self, svc, mod, hasher, monkeypatch): + _rows(mod, monkeypatch, _chain(hasher, 5)) + r = _verify(svc) + assert (r["valid"], r["status"], r["checked"]) == (True, "verified", 5) + + def test_a_mutated_field_is_detected(self, svc, mod, hasher, monkeypatch): + chain = _chain(hasher, 5) + chain[2]["event_action"] = "settings_change_TAMPERED" + _rows(mod, monkeypatch, chain) + r = _verify(svc) + assert r["valid"] is False and r["first_invalid_id"] == 3 + + def test_a_deleted_middle_row_is_detected(self, svc, mod, hasher, monkeypatch): + """Deletion is the attack the LINK check exists for: each surviving + hash is still self-consistent, so only `previous_hash` catches it.""" + chain = _chain(hasher, 5) + del chain[2] + _rows(mod, monkeypatch, chain) + r = _verify(svc) + assert r["valid"] is False, "a deleted row left the chain reporting intact" + + def test_reordered_rows_are_detected(self, svc, mod, hasher, monkeypatch): + chain = _chain(hasher, 5) + chain[1], chain[2] = chain[2], chain[1] + _rows(mod, monkeypatch, chain) + r = _verify(svc) + assert r["valid"] is False + + @pytest.mark.parametrize("field", ["source", "actor_email", "endpoint", + "actor_ip", "mcp_key_id"]) + def test_fields_outside_the_hashed_subset_are_silently_mutable( + self, svc, mod, hasher, monkeypatch, field + ): + """Documents the boundary rather than asserting a bug. + + `_compute_hash` covers event_id/type/action/actor_id/target_id/ + timestamp/details/previous_hash. Everything else on the row — including + `actor_ip` and `actor_email`, which an incident responder would read as + attributable evidence — can be edited with the chain still reporting + `verified`. Worth knowing before citing a green tick as proof of who + did something. + """ + chain = _chain(hasher, 3) + chain[1][field] = "rewritten-after-the-fact" + _rows(mod, monkeypatch, chain) + assert _verify(svc)["valid"] is True + + def test_truncation_at_the_tail_is_not_detectable(self, svc, mod, hasher, monkeypatch): + """Also a boundary, not a bug: verification is over a caller-supplied + range, so dropping the newest rows leaves a shorter intact chain. The + DB trigger (`audit_log_no_delete`) is what defends this, not the hash.""" + _rows(mod, monkeypatch, _chain(hasher, 5)[:3]) + assert _verify(svc)["valid"] is True + + +# --------------------------------------------------------------------------- +# _compute_hash — inputs it will actually meet +# --------------------------------------------------------------------------- + +class TestComputeHash: + + def test_details_str_and_dict_hash_identically(self, mod, hasher): + """The write path stores `details` as a JSON string and the read path + returns a dict; the hash has to be stable across that round-trip or + every entry fails verification.""" + d = {"b": 1, "a": [1, 2, {"z": None}]} + as_dict = _row(1, details=d) + as_str = _row(1, details=json.dumps(d)) + assert hasher(as_dict) == hasher(as_str) + + def test_key_order_in_a_details_string_does_not_change_the_hash(self, mod, hasher): + a = _row(1, details='{"x": 1, "y": 2}') + b = _row(1, details='{"y": 2, "x": 1}') + assert hasher(a) == hasher(b) + + def test_unparseable_details_still_hashes(self, mod, hasher): + """The `except (TypeError, ValueError): pass` branch — the value stays + a string and must not raise.""" + assert hasher(_row(1, details="{not json")) + + def test_non_ascii_details_hash_stably(self, mod, hasher): + row = _row(1, details={"note": "café ✓ 日本語"}) + assert hasher(row) == hasher(dict(row)) + + def test_a_missing_optional_field_is_not_a_crash(self, mod, hasher): + row = _row(1) + row.pop("actor_id") + row.pop("target_id") + assert hasher(row) + + @pytest.mark.parametrize("missing", ["event_id", "event_type", + "event_action", "timestamp"]) + def test_required_fields_raise_rather_than_hash_a_hole(self, mod, hasher, missing): + """These are read with `[]`, not `.get()`. A row missing one is a + programming error and should surface as one, not hash to a value that + silently differs from what was written.""" + row = _row(1) + row.pop(missing) + with pytest.raises(KeyError): + hasher(row) + + def test_details_none_and_details_null_string_collide(self, mod, hasher): + """`details=None` and `details="null"` both normalize to JSON `null`, + so they hash identically. Harmless (both mean 'no details') and + recorded so a future 'fix' doesn't treat it as a defect.""" + assert hasher(_row(1, details=None)) == \ + hasher(_row(1, details="null")) + + +# --------------------------------------------------------------------------- +# The control that produces the hashes +# --------------------------------------------------------------------------- + +class TestHashChainLifecycle: + + def test_enabling_is_reflected_in_the_verdict(self, svc, mod, hasher, monkeypatch): + monkeypatch.setattr(svc, "_hash_chain_enabled", True, raising=False) + _rows(mod, monkeypatch, _chain(hasher, 2)) + assert _verify(svc)["hash_chain_enabled"] is True + + @pytest.mark.xfail( + strict=True, + reason="BUG: enabling the audit hash chain is in-memory only — no " + "persistence, no boot restore, so a backend restart silently " + "turns the integrity control back off. See /edge-cases report " + "2026-08-05, finding 1.", + ) + def test_enabling_the_hash_chain_survives_a_restart(self, mod, hasher): + """`enable_hash_chain(True)` sets `self._hash_chain_enabled` and writes + nothing. A fresh process — every deploy, every config change, the + documented post-restart re-login — starts `False`, and nothing tells the + operator that hashing stopped. + + Modelled as "construct a second service instance", which is exactly what + the next process does. + """ + svc_a = mod.PlatformAuditService() + svc_a.enable_hash_chain(True) + + svc_b = mod.PlatformAuditService() # the process after a restart + + assert svc_b._hash_chain_enabled is True, ( + "hash chain silently reverted to disabled in a new process" + ) + + def test_the_enable_route_persists_nothing(self): + """Pins the mechanism behind the xfail above, so the finding survives a + refactor of the service: the router hands the flag straight to the + in-memory setter with no settings write.""" + src = (_REPO / "src" / "backend" / "routers" / "audit_log.py").read_text() + block = src[src.index("async def enable_hash_chain"):] + block = block[:block.index("\n@router") if "\n@router" in block else len(block)] + assert "platform_audit_service.enable_hash_chain" in block + assert "set_setting" not in block and "system_settings" not in block, ( + "the enable route now persists — update or remove the xfail above" + ) + + +# --------------------------------------------------------------------------- +# Range/argument boundaries +# --------------------------------------------------------------------------- + +class TestRangeBoundaries: + + @pytest.mark.parametrize("start,end", [(0, 0), (5, 1), (-1, -1), + (1, 10**12)]) + def test_odd_ranges_degrade_rather_than_raise(self, svc, mod, hasher, monkeypatch, + start, end): + """The accessor decides what an inverted or absurd range returns; the + verifier must not add a crash on top of an empty result.""" + _rows(mod, monkeypatch, []) + r = _verify(svc, start, end) + assert r["valid"] is None and r["status"] == "empty_range" + + def test_a_single_row_range_never_checks_a_link(self, svc, mod, hasher, monkeypatch): + """`i > 0` means the first row in ANY range has its `previous_hash` + unchecked. For a one-row range that is the whole verdict — self-hash + only. Documented so a caller doesn't read `verified` on a 1-row range + as 'linked to the row before it'.""" + chain = _chain(hasher, 3) + _rows(mod, monkeypatch, [chain[2]]) + r = _verify(svc) + assert (r["valid"], r["checked"]) == (True, 1) + + def test_counts_add_up(self, svc, mod, hasher, monkeypatch): + chain = _chain(hasher, 3) + [_row(4), _row(5)] + _rows(mod, monkeypatch, chain) + r = _verify(svc) + assert r["checked"] + r["skipped_unhashed"] == r["total_in_range"] == 5 + assert r["status"] == "verified_partial" diff --git a/tests/unit/test_pat_propagation_properties.py b/tests/unit/test_pat_propagation_properties.py new file mode 100644 index 000000000..270fb9c7b --- /dev/null +++ b/tests/unit/test_pat_propagation_properties.py @@ -0,0 +1,230 @@ +"""Edge-case + property analysis of PAT propagation (`/edge-cases`, 2026-08-05). + +Target: `services/github_pat_propagation_service._patch_env_github_pat` / +`_format_pat_line` / `_env_has_github_pat`, as merged on `dev` by #1979 +(issue #1967). + +#1967's suite covers eligibility, the remote rewrite, and per-agent failure +isolation. What it does not cover is the *text transform itself*: this is a +regex substitution that writes a credential into a file another process parses, +so the honest question is a round-trip one — **after patching, does the agent +read back exactly the token we rotated to?** + +That is stated once as a Hypothesis property and pinned as explicit cases for +the two inputs where it does not hold. The oracle is the agent's own `.env` +reader, copied here from +`docker/base-image/agent_server/services/execution_env.parse_env_file` — the +same last-wins, one-quote-pair semantics, because "what the agent sees" is the +only definition of success that matters for a credential rotation. + +Cases marked `xfail(strict=True)` are real defects; product code is unchanged +per the skill's protocol. +""" + +from __future__ import annotations + +import re +import sys +from pathlib import Path + +import pytest +from hypothesis import HealthCheck, given, settings +from hypothesis import strategies as st + +_REPO = Path(__file__).resolve().parents[2] +_BACKEND_STR = str(_REPO / "src" / "backend") +while _BACKEND_STR in sys.path: + sys.path.remove(_BACKEND_STR) +sys.path.insert(0, _BACKEND_STR) + +pytestmark = pytest.mark.unit + + +@pytest.fixture(scope="module") +def svc(): + try: + import services.github_pat_propagation_service as m + except ImportError: # pragma: no cover — backend venv required + pytest.skip("backend venv required") + return m + + +# --------------------------------------------------------------------------- +# The oracle: the agent's own .env reader +# --------------------------------------------------------------------------- + +_ENV_LINE_KEYS = ("GITHUB_PAT", "GH_TOKEN", "GITHUB_TOKEN") + + +def agent_reads(env_content: str) -> dict: + """Byte-faithful copy of the agent-server `.env` parser (last-wins). + + Deliberately a copy and not an import: the agent server ships in its own + image and the backend cannot import from it. Kept small enough to audit + against the original by eye. + """ + out = {} + for line in env_content.splitlines(): + line = line.strip() + if not line or line.startswith("#") or "=" not in line: + continue + key, _, value = line.partition("=") + key = key.strip() + if not key: + continue + out[key] = value.strip().strip('"').strip("'") + return out + + +# A GitHub PAT's real alphabet (`ghp_…`, `github_pat_…`) — the tokens this code +# will actually be handed. +REAL_PATS = st.from_regex(r"\A(ghp_[A-Za-z0-9]{36}|github_pat_[A-Za-z0-9_]{20,60})\Z") + +# Plausible surrounding .env content, excluding the keys under test so the +# fixture cannot pre-seed the very lines the property is about. +ENV_NOISE = st.lists( + st.tuples( + st.from_regex(r"\A[A-Z][A-Z0-9_]{0,12}\Z").filter( + lambda k: k not in _ENV_LINE_KEYS + ), + st.text(alphabet=st.characters(blacklist_categories=("Cs", "Cc")), max_size=20), + ), + max_size=6, +).map(lambda pairs: "".join(f'{k}="{v}"\n' for k, v in pairs)) + + +# --------------------------------------------------------------------------- +# The property +# --------------------------------------------------------------------------- + +class TestRoundTrip: + + @settings(max_examples=200, deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture]) + @given(pat=REAL_PATS, noise=ENV_NOISE, has_existing=st.booleans()) + def test_the_agent_reads_back_exactly_the_rotated_token( + self, svc, pat, noise, has_existing + ): + """The whole point of the feature, as one property: whatever the file + looked like, after a rotation the agent must read the NEW token under + all three key names.""" + env = noise + ('GITHUB_PAT="ghp_' + "o" * 36 + '"\n' if has_existing else "") + patched = svc._patch_env_github_pat(env, pat) + seen = agent_reads(patched) + for key in _ENV_LINE_KEYS: + assert seen.get(key) == pat, f"{key} reads back as {seen.get(key)!r}" + + @settings(max_examples=100, deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture]) + @given(pat=REAL_PATS, noise=ENV_NOISE) + def test_patching_is_idempotent(self, svc, pat, noise): + once = svc._patch_env_github_pat(noise, pat) + assert svc._patch_env_github_pat(once, pat) == once + + @settings(max_examples=100, deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture]) + @given(pat=REAL_PATS, noise=ENV_NOISE) + def test_unrelated_keys_survive(self, svc, pat, noise): + before = agent_reads(noise) + after = agent_reads(svc._patch_env_github_pat(noise, pat)) + for k, v in before.items(): + assert after.get(k) == v, f"rotation clobbered unrelated key {k}" + + @settings(max_examples=100, deadline=None, + suppress_health_check=[HealthCheck.function_scoped_fixture]) + @given(pat=REAL_PATS, noise=ENV_NOISE) + def test_no_token_is_left_behind_anywhere_in_the_file(self, svc, pat, noise): + """Stronger than the read-back: the OLD token must not survive as text + either, or a `.env` sourced by a shell (or grepped by a human) still + yields the revoked credential.""" + old = "ghp_" + "o" * 36 + patched = svc._patch_env_github_pat(f'GITHUB_PAT="{old}"\n' + noise, pat) + assert old not in patched + + +# --------------------------------------------------------------------------- +# Where the round-trip does not hold +# --------------------------------------------------------------------------- + +class TestKnownGaps: + + @pytest.mark.xfail( + strict=True, + reason="BUG: `count=1` replaces only the FIRST GITHUB_PAT line, and the " + "agent's .env parser is last-wins — so a duplicated line leaves " + "the agent authenticating with the REVOKED token while the " + "rotation reports `updated`. See /edge-cases report 2026-08-05, " + "finding 2.", + ) + def test_a_duplicated_pat_line_still_rotates(self, svc): + env = ('GITHUB_PAT="ghp_old"\n' + 'SOMETHING=1\n' + 'GITHUB_PAT="ghp_old"\n') + patched = svc._patch_env_github_pat(env, "ghp_new") + assert agent_reads(patched)["GITHUB_PAT"] == "ghp_new", ( + "the agent still reads the old token" + ) + + @pytest.mark.parametrize("pat", [r"tok\1", r"tok\g<1>", r"tok\slash"]) + @pytest.mark.xfail( + strict=True, + reason="BUG: the new line is used as an `re.sub` REPLACEMENT, so a " + "backslash in the token is parsed as a group reference and " + "raises re.error mid-rotation. See /edge-cases report " + "2026-08-05, finding 3.", + ) + def test_a_backslash_in_the_token_does_not_raise(self, svc, pat): + svc._patch_env_github_pat('GITHUB_PAT="old"\n', pat) + + @pytest.mark.xfail( + strict=True, + reason="Writer/reader asymmetry: `_format_pat_line` escapes `\"` but " + "the agent's .env parser never unescapes it. Belongs to the " + ".env parse contract, not to #1979. See /edge-cases report " + "2026-08-05, finding 4.", + ) + def test_a_quote_in_the_token_round_trips(self, svc): + pat = 'ghp_a"b' + patched = svc._patch_env_github_pat('GITHUB_PAT="old"\n', pat) + assert agent_reads(patched)["GITHUB_PAT"] == pat + + +# --------------------------------------------------------------------------- +# Deterministic boundary cases that DO hold (regression value) +# --------------------------------------------------------------------------- + +class TestBoundaries: + + @pytest.mark.parametrize("env,label", [ + ("", "empty file"), + ("\n", "just a newline"), + ("NO_TRAILING_NEWLINE=1", "no trailing newline"), + (" GITHUB_PAT=\"indented\"\n", "leading whitespace"), + ("\tGITHUB_PAT=\"tabbed\"\n", "leading tab"), + ("# GITHUB_PAT=\"commented\"\n", "commented-out line"), + ("GITHUB_PAT=\n", "present but empty"), + ("GITHUB_PATX=\"lookalike\"\n", "lookalike key"), + ("MY_GITHUB_PAT=\"suffix\"\n", "key with a prefix"), + ("A=1\r\nGITHUB_PAT=\"crlf\"\r\n", "CRLF line endings"), + ]) + def test_the_agent_reads_the_new_token(self, svc, env, label): + patched = svc._patch_env_github_pat(env, "ghp_new") + assert agent_reads(patched).get("GITHUB_PAT") == "ghp_new", label + + def test_a_lookalike_key_is_not_clobbered(self, svc): + patched = svc._patch_env_github_pat('GITHUB_PATX="keep"\n', "ghp_new") + assert agent_reads(patched)["GITHUB_PATX"] == "keep" + + def test_a_commented_line_is_not_treated_as_a_pat(self, svc): + assert svc._env_has_github_pat('# GITHUB_PAT="x"\n') is False + + def test_an_exported_line_is_not_recognised(self, svc): + """`export GITHUB_PAT=…` reads as "no PAT", so on the global path such + an agent is skipped as `skipped_no_pat`. Documented, not asserted as a + bug: the eligibility rule is deliberately `.env`-line-shaped.""" + assert svc._env_has_github_pat('export GITHUB_PAT="x"\n') is False + + def test_appending_does_not_glue_onto_a_previous_line(self, svc): + patched = svc._patch_env_github_pat("FOO=1", "ghp_new") + assert "FOO=1GITHUB_PAT" not in patched + assert agent_reads(patched)["FOO"] == "1" From 4ef06ea3705bb0d7a46964362796a4b0f79de7a5 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Mon, 10 Aug 2026 11:04:42 +0300 Subject: [PATCH 2/4] test(2018): make the #2015 alarms fire when the bug dies, not go quiet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both alarms for finding 1 were inert against the very fix they guard (#2026). 1. The strict xfail asserted `svc_b._hash_chain_enabled`. #2026 deletes that attribute when the flag moves to `system_settings`, and `xfail` treats the resulting AttributeError as an expected failure exactly like the assertion failure it replaces — so the marker would keep reporting 'BUG: enabling the audit hash chain is in-memory only' against a codebase where that is no longer true. `strict=True` exists to go loud when the bug dies; this went quiet, permanently. Now asserts the PUBLIC seam (`svc_b.hash_chain_enabled`), so the marker flips to XPASS(strict). 2. The backstop read only `routers/audit_log.py`. #2026 puts the write in the SERVICE (`db.set_setting`) and leaves the router a thin passthrough, so the router-only check passes with the fix in place. It now also inspects the setter the route delegates to, resolved through the import rather than a fixed filename — over the AST, because the fix's own docstring explains the persistence it adds and a substring scan matches that prose. 3. `test_enabling_is_reflected_in_the_verdict` forced only the private attribute, which is the third failure on the merged tree. It now forces both seams (`raising=False` on each), so it reads the same answer before and after the flag moves. Verified on both trees: this branch alone (bug present): 28 passed, 1 xfailed merged with #2026 (bug fixed): XPASS(strict) + backstop failure — both loud, telling you to retire the finding; the verdict test passes Related to #2015 Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/test_audit_chain_edges.py | 58 ++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_audit_chain_edges.py b/tests/unit/test_audit_chain_edges.py index f9930b584..72b5827d0 100644 --- a/tests/unit/test_audit_chain_edges.py +++ b/tests/unit/test_audit_chain_edges.py @@ -224,7 +224,17 @@ def test_details_none_and_details_null_string_collide(self, mod, hasher): class TestHashChainLifecycle: def test_enabling_is_reflected_in_the_verdict(self, svc, mod, hasher, monkeypatch): + # Forced through BOTH seams so this reads the same answer before and + # after #2026: today the verdict is computed from the private + # `_hash_chain_enabled`; once the flag moves to `system_settings` it is + # computed from the `hash_chain_enabled` property, and a test that + # forces only the private attribute fails on the merged tree while + # testing nothing about the flag. Both are `raising=False`, so whichever + # seam does not exist in the tree under test is simply unused. monkeypatch.setattr(svc, "_hash_chain_enabled", True, raising=False) + monkeypatch.setattr( + type(svc), "hash_chain_enabled", property(lambda self: True), raising=False + ) _rows(mod, monkeypatch, _chain(hasher, 2)) assert _verify(svc)["hash_chain_enabled"] is True @@ -249,14 +259,36 @@ def test_enabling_the_hash_chain_survives_a_restart(self, mod, hasher): svc_b = mod.PlatformAuditService() # the process after a restart - assert svc_b._hash_chain_enabled is True, ( + # The PUBLIC seam, not `_hash_chain_enabled`. The private attribute is + # what the fix (#2026) deletes when the flag moves to `system_settings`, + # and a `strict=True` xfail treats the resulting AttributeError as an + # expected failure exactly like the assertion failure it replaces — so + # the marker would go on reporting "BUG: ... in-memory only" against a + # codebase where the bug is dead, which is the opposite of what strict + # is for. Asserting the public seam makes it XPASS(strict) — loud — the + # moment the flag genuinely persists. + assert svc_b.hash_chain_enabled is True, ( "hash chain silently reverted to disabled in a new process" ) def test_the_enable_route_persists_nothing(self): """Pins the mechanism behind the xfail above, so the finding survives a - refactor of the service: the router hands the flag straight to the - in-memory setter with no settings write.""" + refactor: the flag is set in memory and nothing writes it durably. + + Checks BOTH ends of the delegation. The first draft read only + `routers/audit_log.py`, and #2026 puts the write in the *service* + (`db.set_setting(...)`) while leaving the router a thin passthrough — so + the router-only version passes with the fix in place, and this backstop + died silently alongside the xfail it exists to protect. + + The service half is asserted over the AST, not the text: the fix's own + docstring explains the persistence it adds, so a substring scan matches + the prose and reports "still in-memory" while the write sits next to it. + """ + import ast + import inspect + import textwrap + src = (_REPO / "src" / "backend" / "routers" / "audit_log.py").read_text() block = src[src.index("async def enable_hash_chain"):] block = block[:block.index("\n@router") if "\n@router" in block else len(block)] @@ -265,6 +297,26 @@ def test_the_enable_route_persists_nothing(self): "the enable route now persists — update or remove the xfail above" ) + # ...and the setter the route delegates to, resolved through the import + # rather than a fixed filename, so moving the service doesn't silence it. + if _BACKEND_STR not in sys.path: + sys.path.insert(0, _BACKEND_STR) + from services.platform_audit_service import PlatformAuditService + + setter = ast.parse( + textwrap.dedent(inspect.getsource(PlatformAuditService.enable_hash_chain)) + ) + persisted = [ + node for node in ast.walk(setter) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr in {"set_setting", "set_system_setting"} + ] + assert not persisted, ( + "`enable_hash_chain` now persists the flag — the #2015 finding is " + "fixed; update or remove the xfail above" + ) + # --------------------------------------------------------------------------- # Range/argument boundaries From 96bbc3764e9c63163d31fbc3bd10baabf3a65531 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Mon, 10 Aug 2026 13:53:31 +0300 Subject: [PATCH 3/4] =?UTF-8?q?test(2018):=20retire=20the=20finding-2=20xf?= =?UTF-8?q?ail=20=E2=80=94=20#2016=20is=20fixed=20on=20dev?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI's regression diff flagged this as a HEAD-only failure: [F] test_pat_propagation_properties.TestKnownGaps::test_a_duplicated_pat_line_still_rotates It is an XPASS(strict), not a broken test. The marker asserted finding 2 — that `count=1` replaced only the FIRST GITHUB_PAT line while the agent's .env parser is last-wins, so a duplicated line left the agent authenticating with the REVOKED token while the rotation reported `updated`. `31ba8d98` (#2016 via #2025) levels every occurrence on dev, so the strict marker went loud exactly as designed: the bug died and the alarm fired instead of going quiet. Retired to a plain regression test that now guards the fix instead of the defect, with the history in its docstring. The sibling markers stay: finding 3 (backslash parsed as an re.sub group reference) and finding 4 (writer escapes a quote the reader never unescapes) are still live on dev — their fixes are #2024 and #2030, both still open — and both correctly report XFAIL here. 47 passed, 5 xfailed (this file: 19 passed, 4 xfailed) Also merged latest dev. Related to #2015 Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/test_pat_propagation_properties.py | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/unit/test_pat_propagation_properties.py b/tests/unit/test_pat_propagation_properties.py index 270fb9c7b..fb1813f47 100644 --- a/tests/unit/test_pat_propagation_properties.py +++ b/tests/unit/test_pat_propagation_properties.py @@ -148,15 +148,19 @@ def test_no_token_is_left_behind_anywhere_in_the_file(self, svc, pat, noise): class TestKnownGaps: - @pytest.mark.xfail( - strict=True, - reason="BUG: `count=1` replaces only the FIRST GITHUB_PAT line, and the " - "agent's .env parser is last-wins — so a duplicated line leaves " - "the agent authenticating with the REVOKED token while the " - "rotation reports `updated`. See /edge-cases report 2026-08-05, " - "finding 2.", - ) def test_a_duplicated_pat_line_still_rotates(self, svc): + """Finding 2 — FIXED by #2016 (PR #2025), marker retired. + + This carried `xfail(strict=True)` while the bug was live: `count=1` + replaced only the FIRST `GITHUB_PAT` line, and the agent's `.env` parser + is last-wins, so a duplicated line left the agent authenticating with the + REVOKED token while the rotation reported `updated`. + + `31ba8d98` levels every occurrence, so the marker did its job and flipped + to XPASS(strict) — which is a FAILURE, and is why CI went red here rather + than quietly reporting a bug that no longer exists. Retired to a plain + regression test: it now guards the fix instead of the defect. + """ env = ('GITHUB_PAT="ghp_old"\n' 'SOMETHING=1\n' 'GITHUB_PAT="ghp_old"\n') From a5e7d292e1f6e655ae5d62b91bff2e912a81e245 Mon Sep 17 00:00:00 2001 From: trinity-ability <309458136+trinity-ability@users.noreply.github.com> Date: Mon, 10 Aug 2026 17:18:36 +0100 Subject: [PATCH 4/4] =?UTF-8?q?test(2018):=20retire=20the=20#2015=20xfail?= =?UTF-8?q?=20=E2=80=94=20#2026=20landed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2026 merged to `dev` (b0126f37), so `enable_hash_chain` now persists to `system_settings` and the finding-1 marker would flip to XPASS(strict), turning `dev` red the moment this branch lands. Per obasilakis's review the marker belongs to whoever merges second; #2026 went first, so it falls here. - `test_enabling_the_hash_chain_survives_a_restart` drops the xfail and keeps its assertion verbatim — the behaviour it describes is now the correct one, so it becomes the named regression test for #2015. - `test_the_enable_route_persists_nothing` → `..._persists_durably`, with both halves inverted: the router still delegates, and the AST check now requires a real `set_setting` call in the service rather than forbidding one. Kept as an AST assertion for the original reason — a docstring describing persistence must not be able to satisfy it. The #2017 marker is deliberately left alone: #2024 is not merged, so that bug is still live on `dev` and the marker is doing its job. Co-Authored-By: Claude Opus 5 (1M context) --- tests/unit/test_audit_chain_edges.py | 48 ++++++++++++---------------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/tests/unit/test_audit_chain_edges.py b/tests/unit/test_audit_chain_edges.py index 72b5827d0..0f06e6726 100644 --- a/tests/unit/test_audit_chain_edges.py +++ b/tests/unit/test_audit_chain_edges.py @@ -238,18 +238,14 @@ def test_enabling_is_reflected_in_the_verdict(self, svc, mod, hasher, monkeypatc _rows(mod, monkeypatch, _chain(hasher, 2)) assert _verify(svc)["hash_chain_enabled"] is True - @pytest.mark.xfail( - strict=True, - reason="BUG: enabling the audit hash chain is in-memory only — no " - "persistence, no boot restore, so a backend restart silently " - "turns the integrity control back off. See /edge-cases report " - "2026-08-05, finding 1.", - ) def test_enabling_the_hash_chain_survives_a_restart(self, mod, hasher): - """`enable_hash_chain(True)` sets `self._hash_chain_enabled` and writes - nothing. A fresh process — every deploy, every config change, the - documented post-restart re-login — starts `False`, and nothing tells the - operator that hashing stopped. + """Regression test for #2015 (fixed by #2026). + + Before the fix, `enable_hash_chain(True)` set `self._hash_chain_enabled` + and wrote nothing. A fresh process — every deploy, every config change, + the documented post-restart re-login — started `False`, and nothing told + the operator that hashing had stopped. #2026 moves the flag to + `system_settings`, so a second instance now reads it back. Modelled as "construct a second service instance", which is exactly what the next process does. @@ -271,19 +267,18 @@ def test_enabling_the_hash_chain_survives_a_restart(self, mod, hasher): "hash chain silently reverted to disabled in a new process" ) - def test_the_enable_route_persists_nothing(self): - """Pins the mechanism behind the xfail above, so the finding survives a - refactor: the flag is set in memory and nothing writes it durably. + def test_the_enable_route_persists_durably(self): + """Pins the mechanism behind the test above, so the #2015 fix survives a + refactor: the flag must be written durably, not just set in memory. - Checks BOTH ends of the delegation. The first draft read only - `routers/audit_log.py`, and #2026 puts the write in the *service* - (`db.set_setting(...)`) while leaving the router a thin passthrough — so - the router-only version passes with the fix in place, and this backstop - died silently alongside the xfail it exists to protect. + Checks BOTH ends of the delegation. #2026 puts the write in the + *service* (`db.set_setting(...)`) while leaving the router a thin + passthrough, so a router-only check would pass even if the service + write were later deleted. - The service half is asserted over the AST, not the text: the fix's own - docstring explains the persistence it adds, so a substring scan matches - the prose and reports "still in-memory" while the write sits next to it. + The service half is asserted over the AST, not the text, so a docstring + that merely *describes* persistence can't satisfy it — only a real call + can. """ import ast import inspect @@ -293,9 +288,6 @@ def test_the_enable_route_persists_nothing(self): block = src[src.index("async def enable_hash_chain"):] block = block[:block.index("\n@router") if "\n@router" in block else len(block)] assert "platform_audit_service.enable_hash_chain" in block - assert "set_setting" not in block and "system_settings" not in block, ( - "the enable route now persists — update or remove the xfail above" - ) # ...and the setter the route delegates to, resolved through the import # rather than a fixed filename, so moving the service doesn't silence it. @@ -312,9 +304,9 @@ def test_the_enable_route_persists_nothing(self): and isinstance(node.func, ast.Attribute) and node.func.attr in {"set_setting", "set_system_setting"} ] - assert not persisted, ( - "`enable_hash_chain` now persists the flag — the #2015 finding is " - "fixed; update or remove the xfail above" + assert persisted, ( + "`enable_hash_chain` no longer persists the flag — #2015 has " + "regressed: a restart will silently disable the hash chain" )