test(edge-cases): audit hash chain + PAT propagation — edge/property analysis of #1985 and #1979

[dolho]

/edge-cases pass over two features recently merged to dev. Tests only — no product code changed, per the skill's protocol: bugs are reported, fixing is a separate decision.

Findings (filed)

	Issue	Severity
Enabling the audit hash chain is in-memory only — a backend restart silently turns the integrity control off	#2015	P2
A duplicated GITHUB_PAT line survives count=1 and wins under last-wins parsing — the agent keeps the revoked token while the rotation reports updated	#2016	P3
A backslash in the token raises re.error mid-rotation; .env quote-escaping is write-only	#2017	P3

Each is pinned by an xfail(strict=True) naming its issue, so the day one is fixed the corresponding test flips to passing and tells you.

What the analysis actually established

The chain works. A mutated hashed field, a deleted middle row and a reordered pair are all detected. What it does not cover is worth knowing before citing a green tick as evidence: _compute_hash hashes event_id/type/action/actor_id/target_id/timestamp/details/previous_hash, so actor_ip, actor_email, endpoint, source and mcp_key_id can be rewritten after the fact with the range still reporting verified — precisely the attribution fields an incident responder would lean on. Tail truncation is likewise undetectable (verification is over a caller-supplied range; the audit_log_no_delete trigger is what defends that, not the hash). Both are documented as boundaries, not filed as bugs.

The PAT round-trip holds for real input. 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 — with the oracle being a byte-faithful copy of the agent-server's own last-wins .env parser, because what the agent reads is the only definition of a successful rotation. It passes across 200 examples for every realistic single-line .env. The xfails are the inputs where it doesn't: a pre-existing duplicate line, and a token outside the PAT alphabet.

Two harness traps worth noting

Both were mine, caught before they could mislead:

1. My first draft called _compute_hash off the module; it is a @staticmethod on the service. 23 red tests that said nothing about the product.
2. The audit file used asyncio.get_event_loop().run_until_complete, which passes standalone and raises "no current event loop" the moment it is collected alongside the bug(security): audit-log verify returns valid:true with checked:0 — an unhashed chain reports as intact #1984 suite — that file uses asyncio.run, which closes the loop. Green locally, red in CI, purely on collection order. Now asyncio.run with the reason in the docstring.

Verification

84 passed, 6 xfailed
branch coverage: platform_audit_service 69%, github_pat_propagation_service 74%
  (misses are log() / accessors / the fleet loop — outside the target functions;
   verify_chain, _compute_hash and _patch_env_github_pat are fully covered)

Run together with the sibling suites (test_1984_*, test_1967_*, test_1574_*) rather than alone, for the reason above.

Not covered

Scoped to the two riskiest surfaces — hashing/verification and credential text-patching. #1981 (ask_trinity), #1980 (Codex auth), #1975/#1974 (scheduler audit + initiator) were not analyzed; they are mostly wiring, and the boundary bugs live where parsing and crypto do. Happy to do a second pass on those if useful.

[obasilakis]

The analysis itself is good and the protocol was followed exactly — three real bugs found, product code untouched, each one marked xfail(strict=True) with a findings reference per edge-cases/SKILL.md:96, issues filed the same minute. The round-trip property (patch the .env, then read it back through the agent's own parser) is the right question to ask of a credential rewrite, and it is what surfaced #2016 and #2017.

One blocking item, and it is narrow: the strict-xfail for #2015 can never fire. Everything else below is a merge-ordering note for the three fix PRs, not something I want changed here.

Blocking — the #2015 marker is inert, and so is its backstop

test_enabling_the_hash_chain_survives_a_restart asserts on a private attribute:

assert svc_b._hash_chain_enabled is True, (
    "hash chain silently reverted to disabled in a new process"
)

#2026 removes _hash_chain_enabled (the flag moves to system_settings, read through the new hash_chain_enabled property). So after that fix the assertion does not pass — it raises:

$ pytest ...::test_enabling_the_hash_chain_survives_a_restart --runxfail
E   AttributeError: 'PlatformAuditService' object has no attribute '_hash_chain_enabled'

xfail treats any failure as expected, so the marker stays XFAIL and keeps reporting "BUG: enabling the audit hash chain is in-memory only" against a codebase where that is no longer true. The whole point of strict=True is that it turns loud when the bug dies; here it goes quiet instead, permanently.

The backstop you wrote for exactly this case does not catch it either:

def test_the_enable_route_persists_nothing(self):
    """Pins the mechanism behind the xfail above, so the finding survives a
    refactor of the service..."""
    src = (_REPO / "src" / "backend" / "routers" / "audit_log.py").read_text()
    ...
    assert "set_setting" not in block and "system_settings" not in block, (
        "the enable route now persists — update or remove the xfail above"
    )

It reads routers/audit_log.py. #2026 put the write in the service — services/platform_audit_service.py:232, db.set_setting(self.HASH_CHAIN_SETTING, ...) — and left the router a thin passthrough. So the router still contains no set_setting, and this test passes with the fix in place. Verified on the merged tree: 1 passed.

Both alarms for #2015 are therefore dead after its own fix lands.

Suggested fix, small:

• assert on the public seam (svc_b.hash_chain_enabled) rather than _hash_chain_enabled, so the marker flips to XPASS when the flag genuinely persists;
• have the backstop check the service as well as the router, or assert against whatever function the route delegates to rather than a fixed filename.

This is the same shape as the guard misses already in docs/memory/learnings.md — a check that reads narrower than the thing it protects, and so reports safe. Worth one more entry given it is now the fourth instance.

Not blocking — merge ordering for #2024 / #2025 / #2026

Merging all four onto dev (composed resolution of the _patch_env_github_pat conflict) gives:

5 failed, 45 passed, 2 xfailed

[XPASS(strict)] BUG: `count=1` replaces only the FIRST GITHUB_PAT line...     (#2016)
[XPASS(strict)] BUG: the new line is used as an `re.sub` REPLACEMENT...  x3   (#2017)
FAILED  TestHashChainLifecycle::test_enabling_is_reflected_in_the_verdict

The four XPASS are the markers working as designed — that is the signal to delete them, and it belongs in the PR that fixes each bug, not here. Concretely: #2025 should drop the #2016 marker, #2024 the #2017 one.

The plain failure is a separate coupling to the same private attribute:

monkeypatch.setattr(svc, "_hash_chain_enabled", True, raising=False)

With raising=False this quietly binds a new attribute that nothing reads once #2026 lands, and verify_chain then reports hash_chain_enabled: False. Cleanest in #2026 alongside the property change, but flagging it here since it is this file.

Minor

The oracle's provenance note in test_pat_propagation_properties.py cites:

copied here from docker/base-image/agent_server/services/execution_env.parse_env_file

That module is not on dev — it is introduced by #2010 (fix/1999-env-ghost), still open. On dev the reader is docker/base-image/agent_server/routers/credentials.py:379-386, which is what #2024 and #2025 cite. The copied semantics are correct either way (I checked it line by line against credentials.py — strip, skip blank/#/no-=, partition("="), strip quotes, last write wins), so this is only the citation. Worth correcting because the docstring invites the reader to audit the copy against the original, and right now they cannot from this branch.

Confirmed clean

• Scope: 3 files, tests plus a registry entry, no product code. Matches the protocol.
• Green on its own branch: 46 passed, 6 xfailed.
• tests/registry.json conflicts against dev but resolves cleanly by re-serializing the parsed JSON; noise, not a finding.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.