REG-D53..REG-D55 — review findings on PR #197, fixed after it merged
======================================================================

PR #197 ("fix(REG-D45..D52): close every post-merge finding on PR #196")
received a review round from CodeQL (7 alerts) and Sourcery (2 blocking
findings) after it was pushed, and merged (commit 9d67efa, main) before
they were addressed — the same "review lands after auto-merge" pattern
REG-D45..D52 already recorded for PR #196. Branch restarted from
origin/main per the merged-PR protocol (`git checkout -B
claude/aegis-v4-comprehensive-audit-m2qyka origin/main`), carrying the
three pending fixes forward as fresh, uncommitted work rather than
stacking them on the merged history. This file records what was done
about all three, on that branch, before a new PR.

=== REG-D53 — CodeQL "Statement has no effect": Protocol stub bodies ===

Finding (CodeQL, 7 alerts, PR #197 review comments):
  aegis/proxy/terminal_outbox.py lines 102, 104, 106, 108, 110, 112, 114 —
  each is the `...` (Ellipsis) body of one of the seven `@property` stub
  methods on the `TerminalSummary` Protocol that REG-D45 introduced:

      @property
      def response_hash(self) -> str: ...
      @property
      def response_size(self) -> int: ...
      ... (five more, same shape)

  `...` as a Protocol method body is the idiom PEP 544 itself documents,
  and it can never execute (a Protocol is never instantiated) — but this
  specific CodeQL query flags any bare Ellipsis expression statement as
  dead code regardless of the surrounding class kind.

Fix: each `def ...(self) -> T: ...` became a two-line `def ...(self) ->
T:\n    pass`. Semantically identical for a Protocol stub (neither body
ever runs); `pass` is a distinct no-op AST statement (ast.Pass, not
ast.Expr(ast.Constant(Ellipsis))) that this CodeQL rule does not match.

Verified:
  python3 -c "import ast; ast.parse(open('aegis/proxy/terminal_outbox.py').read())"
    -> no error
  mypy --strict aegis -> Success: no issues found in 208 source files
  ruff check aegis/proxy/terminal_outbox.py -> All checks passed
  ruff format --check aegis/proxy/terminal_outbox.py -> already formatted
  HERMES_SANDBOX=true pytest -q tests/test_terminal_outbox.py -> 51 passed

=== REG-D54 — commit_forensic_summary under-validates five fields ===

Finding (Sourcery, blocking, aegis/core/crypto_audit.py:1637):
  commit_forensic_summary's docstring states: "Every field is
  type-checked here, before the lock, so a malformed value is refused
  rather than failing inside signing and latching a fault." Four fields
  did not meet that:

    - `if response_size < 0 or token_count < 0:` compared directly with
      no isinstance check first. A non-comparable value (e.g. a str)
      raises the interpreter's own TypeError, not the ValueError every
      other malformed-input path in this function raises and the
      docstring promises — a caller catching ValueError (as documented)
      would not catch it.
    - `final_marker_included` had no check at all: any value flowed
      straight into build_stream_merkle_leaf's signed leaf and into the
      `params` dict that becomes part of the committed node.
    - `if not math.isfinite(elapsed_seconds) or elapsed_seconds < 0:`
      has the same incidental-TypeError shape as response_size/
      token_count for a non-numeric value.
    - `redaction_hits`'s check, `not key or not isinstance(value, int)
      or value < 0`, never confirmed `key` was a str at all, and
      `isinstance(value, int)` accepts `bool` (an int subclass) as a
      count -- the same gap this file's own request_digest size check
      five lines above already excludes with `isinstance(request_size,
      bool) or not isinstance(request_size, int)`.

Fix (aegis/core/crypto_audit.py, commit_forensic_summary): added
isinstance checks ahead of every comparison, following the file's own
existing request_size idiom:

    if (
        isinstance(response_size, bool)
        or not isinstance(response_size, int)
        or response_size < 0
    ):
        raise ValueError("response_size must be a non-negative integer")
    if isinstance(token_count, bool) or not isinstance(token_count, int) or token_count < 0:
        raise ValueError("token_count must be a non-negative integer")
    if not isinstance(final_marker_included, bool):
        raise ValueError("final_marker_included must be a bool")
    if (
        isinstance(elapsed_seconds, bool)
        or not isinstance(elapsed_seconds, (int, float))
        or not math.isfinite(elapsed_seconds)
        or elapsed_seconds < 0
    ):
        raise ValueError("elapsed_seconds must be finite and non-negative")
    ...
    if any(
        not isinstance(key, str)
        or not key
        or isinstance(value, bool)
        or not isinstance(value, int)
        or value < 0
        for key, value in hits.items()
    ):
        raise ValueError("redaction_hits must contain non-negative integer counts")

New test: tests/test_terminal_outbox.py::
  test_a_malformed_scalar_or_redaction_field_raises_value_error (14
  parametrized cases: non-int/negative/bool response_size and
  token_count; non-bool final_marker_included; non-numeric/bool/negative
  elapsed_seconds; non-str-key/bool-value/negative-value redaction_hits),
  mirroring the existing test_the_digest_form_is_validated_before_the_lock's
  shape (base kwargs + pytest.raises(ValueError, match=...), then asserts
  ledger._fault_state == "healthy").

Verified:
  mypy --strict aegis -> Success: no issues found in 208 source files
  ruff check aegis/core/crypto_audit.py -> All checks passed
  ruff format --check aegis/core/crypto_audit.py -> already formatted
  HERMES_SANDBOX=true pytest -q tests/test_terminal_outbox.py
    tests/test_crypto_audit_branch.py tests/test_crypto_audit_rollover.py
    tests/test_crypto_audit_scheme_binding.py tests/test_crypto_audit_wal_json.py
    tests/test_proxy_streaming.py tests/test_signature_scheme_binding.py
    -> 144 passed

--- Second Sourcery round on the same still-open PR (now #198) ---

Finding (Sourcery, blocking, aegis/core/crypto_audit.py:1726):
  `redaction_hits` itself was never checked to be a dict before
  `dict(redaction_hits or {})`. A falsy non-dict (`[]`, `0`, `False`)
  silently became `{}` and was accepted; an iterable of (key, value)
  pairs (`[("pii", 1)]`) was silently converted by `dict()` and accepted
  despite violating the annotated `dict[str, int]`; any other non-dict
  raised whatever TypeError/ValueError the built-in `dict()` call
  happened to raise, not the documented one.

Fix: one check ahead of the conversion --

    if redaction_hits is not None and not isinstance(redaction_hits, dict):
        raise ValueError("redaction_hits must be a dictionary")
    hits = dict(redaction_hits or {})

Four more parametrized cases added to
test_a_malformed_scalar_or_redaction_field_raises_value_error: `[]`,
`[("pii", 1)]`, `"not-a-dict"`, `5` (all matching "redaction_hits").

Verified:
  mypy --strict aegis -> Success: no issues found in 208 source files
  ruff check aegis/core/crypto_audit.py -> All checks passed
  ruff format --check aegis/core/crypto_audit.py -> already formatted
  HERMES_SANDBOX=true pytest -q tests/test_terminal_outbox.py
    tests/test_crypto_audit_branch.py tests/test_crypto_audit_rollover.py
    tests/test_crypto_audit_scheme_binding.py tests/test_crypto_audit_wal_json.py
    tests/test_proxy_streaming.py tests/test_signature_scheme_binding.py
    -> 149 passed

=== REG-D55 — terminal-outbox fd leak survives REG-D48's own fix ===

Finding (Sourcery, blocking, aegis/proxy/app.py:1129):
  REG-D48 reordered lifespan's REG-D32 outbox-open block ahead of SIEM/S3
  startup so a refusal from TerminalOutbox.open() itself leaks nothing
  above it. But once open() succeeds, state.terminal_outbox = outbox runs
  and several more startup steps follow before `yield`: replay_pending
  itself (confirmed by reading it in full -- its per-entry commit() call
  is caught inside a try/except, but the surrounding
  _committed_terminal_state_ids(ledger) and outbox.compact() calls that
  bracket the loop are not), attach_outbox, SIEM/S3 startup, the
  terminal-evidence handoff worker, the LSM guard, vault authentication,
  the forwarder, cross-replica gossip, and the seccomp filter. Any of
  those raising still means lifespan's @asynccontextmanager __aenter__
  propagates before reaching `yield`, so Starlette never runs this
  function's post-`yield` shutdown half (the exact mechanics REG-D48's
  own comment already documents) -- leaving the now-live outbox file
  descriptor with nothing to close it. REG-D48's fix closed the gap for
  open() raising; it did not close the gap for anything after open()
  succeeding.

Fix (aegis/proxy/app.py, lifespan): wrapped the whole startup span, from
the REG-D32 outbox block through `app.state.aegis = state` (immediately
before `yield`), in a `contextlib.ExitStack`:

    with ExitStack() as startup_cleanup:
        if cfg.terminal_outbox_enabled:
            ...
            outbox = TerminalOutbox.open(...)
            startup_cleanup.callback(outbox.close)
            report = await replay_pending(...)
            ...
        if state.siem_exporter is not None:
            ...
        ... (every other startup step, unindented body unchanged) ...
        app.state.aegis = state
        startup_cleanup.pop_all()
    yield

`startup_cleanup.callback(outbox.close)` registers only once open()
itself has already succeeded. If anything in the rest of the span then
raises, ExitStack.__exit__ runs the registered callback (outbox.close())
before the exception propagates out of lifespan -- covering every step
listed above, not just replay_pending. `startup_cleanup.pop_all()`
disarms the callback only once every step through the seccomp filter has
succeeded, transferring ownership back to the existing post-`yield`
shutdown code (`state.terminal_handoff.attach_outbox(None);
state.terminal_outbox.close()`), which is unchanged. No behavior changes
on any success path; import added: `from contextlib import ExitStack,
asynccontextmanager`.

New test: tests/test_terminal_outbox.py::
  test_a_later_startup_failure_still_closes_the_outbox -- monkeypatches
  TerminalCommitHandoff.start (the first unconditional step after the
  outbox is wired into state, with SIEM/S3 off by default) to raise, then
  asserts state.terminal_outbox is not None (wired in before the
  failure) and state.terminal_outbox._fd is None (ExitStack closed it).

Verified:
  python3 -c "import ast; ast.parse(open('aegis/proxy/app.py').read())"
    -> no error
  mypy --strict aegis -> Success: no issues found in 208 source files
  ruff check aegis/proxy/app.py -> All checks passed
  ruff format --check aegis/proxy/app.py -> already formatted (after one
    reformat: the +4-space reindent pushed three unrelated lines past the
    line-length limit; `ruff format` rewrapped them, no logic touched)
  HERMES_SANDBOX=true pytest -q tests/test_terminal_outbox.py -> 51 passed

--- Second Sourcery round on the same still-open PR (now #198) ---

Finding (Sourcery, blocking, aegis/proxy/app.py:1135-1285):
  The ExitStack fix above only registered a cleanup callback for the
  outbox. Once state.terminal_handoff.start() succeeded, several more
  startup steps still followed before `yield` (LSM, vault, the forwarder,
  gossip, seccomp); any of those raising left the already-running handoff
  worker -- and whatever else had already started (SIEM exporter thread,
  S3 archiver worker plus its archive task, forwarder, analysis worker
  tasks, gossip mesh) -- with nothing to stop them, for the same
  underlying reason one layer further out: Starlette still never runs
  lifespan's post-`yield` shutdown half.

Fix (aegis/proxy/app.py, lifespan): switched `ExitStack` to
`contextlib.AsyncExitStack` (import changed to `from contextlib import
AsyncExitStack, asynccontextmanager`; the `with` became `async with`) and
registered a stop callback for every resource right after it starts, not
just the outbox:

  - outbox: `startup_cleanup.callback(outbox.close)` (unchanged, right
    after open()); a second callback,
    `startup_cleanup.callback(state.terminal_handoff.attach_outbox,
    None)`, added right after attach_outbox() succeeds.
  - SIEM exporter: `startup_cleanup.push_async_callback(
    _stop_siem_exporter_on_startup_failure)` right after `.start()` -- a
    small async helper mirroring the post-`yield` shutdown's
    `asyncio.to_thread(state.siem_exporter.shutdown, 5.0, drain=False)`
    with the same TimeoutError handling.
  - S3 archiver + its archive task: one
    `push_async_callback(_stop_s3_archiver_on_startup_failure)` after
    both are created -- cancels+gathers the archive task, then closes
    the archiver with drain=True, mirroring the post-`yield` order.
  - terminal_handoff worker:
    `push_async_callback(state.terminal_handoff.stop,
    timeout=cfg.analysis_shutdown_timeout_seconds)` right after
    `.start()`.
  - forwarder: `push_async_callback(state.forwarder.stop)` right after
    `await state.forwarder.start()`.
  - analysis workers: `push_async_callback(
    _stop_analysis_workers_on_startup_failure)` right after they are
    created -- cancels each, then gathers with the same timeout-and-log
    behaviour as the post-`yield` shutdown.
  - gossip: `if state.gossip is not None:
    push_async_callback(state.gossip.aclose)`, guarded the same way the
    post-`yield` shutdown already guards it.

Each helper is a plain async closure defined at the top of `lifespan`
(so it can reach `state`/`cfg`/`logger` without arguments); the two that
read a plain-Optional attribute (`state.siem_exporter`,
`state.s3_archiver`) open with an `assert ... is not None` restating,
for the type checker, that they are only ever registered from the
`if ... is not None:` block right above them -- mypy cannot see that
invariant across the closure boundary on its own.

`AsyncExitStack` unwinds every registered callback -- sync via
`.callback()`, async via `.push_async_callback()` -- in one LIFO
sequence: since each resource registers its stop callback in the same
order it starts, the unwind order is the reverse of the start order
(gossip stops first, the outbox last), which also happens to keep
terminal_handoff.stop() running before outbox.close() (handoff was
registered after the outbox, so it unwinds first) -- the worker that
might still be flushing a queued commit through the outbox is stopped
before the outbox it writes to is closed. `startup_cleanup.pop_all()`
(unchanged call, now on an AsyncExitStack) still disarms every
registered callback once startup fully succeeds.

New test: tests/test_terminal_outbox.py::
  test_a_much_later_startup_failure_stops_the_handoff_worker_too --
  monkeypatches LLMForwarder.start to raise (well after the handoff
  worker is already running) and TerminalCommitHandoff.stop to a spy
  that still calls through to the real implementation; asserts the spy
  was called.

Verified:
  python3 -c "import ast; ast.parse(open('aegis/proxy/app.py').read())"
    -> no error
  mypy --strict aegis -> Success: no issues found in 208 source files
    (two union-attr errors surfaced first on the two closures reading
    Optional attributes; fixed by the asserts described above)
  ruff check aegis/proxy/app.py -> All checks passed
  ruff format --check aegis/proxy/app.py -> already formatted
  HERMES_SANDBOX=true pytest -q tests/test_terminal_outbox.py -> 56 passed

=== Full-suite confirmation ===

  HERMES_SANDBOX=true pytest -q -n auto (this branch, restarted from
    origin/main 9d67efa plus these three fixes, then the second Sourcery
    round on top)
    -> first run: 3 failed, 7419 passed, 39 skipped. Two of the three
       (test_compliance_wording_gate.py::test_the_documentation_surfaces_
       pass_the_gate_after_the_wording_fix,
       test_documentation_verifiers.py::
       test_repository_corpus_passes_every_structural_check) were local
       contamination, not a defect in this diff: both assertions named
       the exact path
       `.claude/worktrees/agent-ad7a51df34d88ccd1/docs/STYLE_GUIDE.md` —
       a full nested checkout the corpus walker picked up from an
       unrelated, still-running background audit agent's isolated
       worktree (launched from this same session, gitignored the same
       day this PR started, in a commit already on `main`). Not present
       in a clean CI checkout. The third, real:
       docs/MODULE_INVENTORY.md was stale (the new test's
       `from aegis.proxy.forwarder import LLMForwarder` import added a
       reachability edge); fixed by running
       `python scripts/generate_module_inventory.py` (one line changed:
       forwarder.py's test count, "+7 more" -> "+8 more").
    -> after the module-inventory fix: targeted re-run of all three
       -> test_module_inventory_current.py: 5 passed. The other two:
          left red locally (same contamination); CI's clean checkout is
          the authoritative confirmation for this pair.
  git diff --check -> clean
