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.

[dolho]

@obasilakis — blocking item fixed. Both alarms for finding 1 were inert against the fix they guard, exactly as you described, and there was a third instance of the same shape next to them.

1. The strict xfail now asserts the public seam. svc_b.hash_chain_enabled instead of svc_b._hash_chain_enabled. The private attribute is what #2026 deletes, and xfail treats the resulting AttributeError as an expected failure exactly like the assertion failure it replaces — so the marker went quiet permanently instead of loud, which is the opposite of what strict=True is for.

2. The backstop now checks the setter the route delegates to, resolved through the import rather than a fixed filename, so moving the service doesn't silence it. Asserted over the AST (Call nodes named set_setting/set_system_setting), not a substring scan — #2026's own docstring explains the persistence it adds, so a text scan matches the prose and reports "still in-memory" with the write sitting right there. Same trap as #2024 in this batch.

3. test_enabling_is_reflected_in_the_verdict — the third failure you listed under merge ordering. It forced only _hash_chain_enabled; it now forces both seams with raising=False on each, so it reads the same answer before and after the flag moves rather than failing on the merged tree.

Verified on both trees, by actually merging fix/2015-hash-chain-persistence into this branch:

this branch alone (bug present):   28 passed, 1 xfailed        <- finding still reported
merged with #2026 (bug fixed):     [XPASS(strict)] BUG: enabling the audit hash chain is in-memory only ...
                                   FAILED ...::test_the_enable_route_persists_nothing
                                   test_enabling_is_reflected_in_the_verdict PASSED

Both alarms are loud on the merged tree and say to retire the finding — which is the behaviour the strict marker was supposed to have.

Merge ordering is unchanged: this still wants to land alongside #2024/#2025/#2026, and the XPASS is the intended signal to drop the marker in that sequence rather than something to pre-empt here.

[obasilakis]

Re-reviewed. The blocking item is fixed, and fixed on both halves — which is what I was actually worried about.

test_enabling_the_hash_chain_survives_a_restart now asserts the public svc_b.hash_chain_enabled, so the marker flips to XPASS(strict) when #2026 lands instead of swallowing an AttributeError and reporting a dead bug as live forever. test_the_enable_route_persists_nothing now checks the setter the route delegates to, resolved through the import rather than a fixed filename, and does it over the AST — which it had to, for the same reason as #2024: #2026's docstring explains the persistence it adds, so a substring scan matches the prose. Forcing both seams in test_enabling_is_reflected_in_the_verdict with raising=False on each is a neat way to make one test read the same answer before and after the refactor.

One thing left, and it's merge ordering rather than anything you did wrong:

test_a_duplicated_pat_line_still_rotates is now XPASS(strict) — the suite is red

#2025 merged to dev on 2026-08-06, so _patch_env_github_pat no longer caps at count=1 and the #2016 marker passes. regression diff reports it as the one new failure introduced by HEAD.

I said in the first review that deleting each marker belongs to the PR that fixes the bug, not to this one. #2025 landed without deleting it, so it falls here by default — drop that xfail(strict=True) block (tests/unit/test_pat_propagation_properties.py:151-158) and keep the test as a plain assertion, since the behaviour it describes is now the correct one.

Leave the #2017 marker (test_a_backslash_in_the_token_does_not_raise) alone — #2024 is approved but not merged, so it's still a live bug on dev and the marker is doing its job. Whoever merges second between this and #2024 drops it.

Also worth correcting while you're in the file: the oracle's provenance note still cites docker/base-image/agent_server/services/execution_env.parse_env_file, which is introduced by #2010 and not on dev. The copied semantics are right either way; it's the citation that a reader can't follow from this branch.

Everything else from the first review stands as confirmed clean.

[vybe]

Re-reviewed after the 10:53 push, and resolved the merge-ordering item obasilakis flagged.

Finding-2 marker (#2016) — retired by dolho at 10:53, correct: #2025 is on dev, so the behaviour is now the expected one.

Finding-1 marker (#2015) — I merged #2026 first (b0126f3), which makes this the second merge, so per obasilakis's rule the marker fell to this PR. Retired in a5e7d29: the restart test drops its xfail and keeps its assertion verbatim (it is now the named regression test for #2015), and test_the_enable_route_persists_nothing is inverted to ..._persists_durably — still an AST assertion, so a docstring describing persistence can't satisfy it. pytest tests/unit/test_audit_chain_edges.py → 29 passed against the merged tree.

#2017 marker — deliberately left in place. #2024 is not merged, so that bug is still live on dev.

Also resolved the tests/registry.json conflict by rebuilding from git stages (160 dev entries + this PR's 2, no duplicates).

Blocking item fixed at 10:53 (#2016 marker retired). The remaining merge-ordering item is resolved in a5e7d29: #2026 merged first, so the #2015 marker was retired here. #2017 marker left live.