Fix 17 findings from deep code review (10 high + 7 medium)

CHANGELOG.md

@@ -7,6 +7,80 @@ public API may still change between minor versions.
 
 ## [Unreleased]
 
+### Security
+
+- **The standalone HTML visualizer now escapes trace data.** `render_trace_html`
+  (`visualizer.py`) interpolated payloads, engine names, type ids, and the title into HTML and
+  into an inlined `<script>` JSON block with no escaping — the same stored-XSS class 0.6.1 fixed
+  in the served viewer and PR/MR renderers, but this module was missed. Server-rendered values are
+  now HTML-escaped, the embedded JSON is escaped so a `</script>` in trace data cannot break out,
+  and the client-side inspector escapes values before assigning `innerHTML`.
+- **The GitHub Action regression gate can no longer be bypassed by a crafted trace.**
+  `action/run_gate.py` wrote the multi-line `summary` (which embeds attacker-influenceable step
+  `type_identifier`s) to `$GITHUB_OUTPUT` with a fixed heredoc delimiter, so a candidate step name
+  containing that delimiter line plus a forged `passed=true` could close the heredoc early and
+  override the real verdict — silently passing a real regression. Each value now uses a random
+  per-write delimiter verified absent from the content. The `$GITHUB_STEP_SUMMARY` code fence is
+  likewise sized to exceed any backtick run it encloses, so a step name cannot inject markdown.
+- **The LlamaIndex adapter redacts secrets from captured payloads.** With payload capture on
+  (the default) it stringified every non-structural value, including `EventPayload.SERIALIZED` —
+  the serialized LLM config, which has shipped an `api_key` in some llama-index versions — into a
+  trace store this toolkit encourages committing as a golden baseline. Secret-keyed values, nested
+  ones included, are now replaced with a redaction placeholder while non-secret structure (model
+  name, token counts) is preserved.
+- **The local trace viewer validates the `Host` header on loopback binds.** The API returned
+  trace prompts/outputs with no `Host`/`Origin` check, so a malicious page could reach it via DNS
+  rebinding (rebinding its hostname to `127.0.0.1`). A loopback-bound viewer now rejects requests
+  whose `Host` isn't loopback; an explicit non-loopback `--host` bind leaves filtering off.
+
+### Fixed
+
+- **Nested boolean queries return correct results on the SQLite backend.** The query compiler
+  joined compound `AND`/`OR`/`missing_step` members with bare `INTERSECT`/`UNION`/`EXCEPT`; SQLite
+  gives those operators equal, left-to-right precedence, so a nested member was silently re-grouped
+  and diverged from the in-memory evaluator (e.g. `has(a) OR missing(b)` returned the wrong runs).
+  Each compiled member is now isolated in a sub-select.
+- **The write buffer no longer over-sheds a run's events after a capacity burst.** The per-run depth
+  counter was written back from a value captured before global-capacity eviction, so evicting a row
+  from the enqueuing run left the counter permanently inflated, spuriously tripping the soft per-run
+  cap. The counter now tracks actual occupancy.
+- **Early-terminating a `@traced` generator records a normal end, not a CRITICAL error.** Breaking
+  out of a traced generator (or async generator) raised `GeneratorExit`, which was recorded as a
+  spurious CRITICAL `.error`, diverging partially-consumed streams from fully-consumed ones and
+  tripping error-keyed anomaly rules. It now records `.end`.
+- **The google-genai wrapper nests calls under the enclosing span.** It set only the current span,
+  leaving the parent pointing at the enclosing span's parent (the grandparent), so nested
+  `generate_content` calls attached to the wrong node. It now sets the parent span too.
+- **The alignment engine detects reordering under every profile, keyed on sequence.** Reorder
+  detection was suppressed in `LINEAR` mode, so the strictest audit profile (`strict_audit_v1`)
+  never flagged critical-step reordering that the debug profile caught; and it compared list
+  position rather than the authoritative `sequence`, flagging logically identical but unsorted
+  runs as a spurious HIGH regression. `align()` now sorts by sequence and reorder detection is
+  mode-independent.
+- **Trace replay surfaces span-cycle events as orphans instead of dropping them.** A parent cycle
+  (`A↔B`) or self-parent left its spans neither rooted nor orphaned, so their events vanished from
+  the reconstructed tree while the manifest still counted them. Unreachable spans are now reported
+  as orphaned events.
+- **The trace-graph cycle validator catches cycles on partial graphs and survives deep chains.** It
+  seeded the search only from `graph.nodes`, missing cycles among nodes that appear solely in edges
+  (as `lineage()`/`impact()` can produce), and recursed per hop so a long valid causal chain raised
+  `RecursionError`. The search now seeds from all edge endpoints and is iterative.
+- **`UnregisteredToolRule` no longer fails open on a string registry.** When the registry field was
+  a bare string rather than a list, `tool_name not in registry` degraded to substring matching, so
+  an unregistered tool whose name was a substring of the registry string was treated as allowed. A
+  string registry is now compared as a single exact entry (fail closed).
+- **The in-memory live-subscription consumer survives a raising subscriber.** A subscriber callback
+  raising once killed the shared daemon consumer thread, silently stopping *all* live delivery while
+  `record` kept enqueuing into an unbounded queue. The consumer now logs and continues.
+- **Framework adapters and the SQLite writer log previously-silent failures.** The CrewAI listener
+  swallowed every translation error with no trace (a version whose events lack the assumed
+  correlation fields produced empty traces with no clue why); `SQLiteTraceStore.flush` swallowed a
+  failed runs-table write that leaves events durable but unreadable. Both now log while preserving
+  the non-fatal behavior.
+- **The trace viewer serializes payloads via `to_dict()`.** `_json_serializable` checked `__dict__`
+  first, which dataclasses always have, so the `to_dict()` branch was dead and the viewer showed
+  internal field names instead of each payload's canonical, export-consistent shape.
+
 ## [0.6.1] - 2026-07-15
 
 ### Security

action/run_gate.py

@@ -18,11 +18,26 @@
 
 import json
 import os
+import secrets
 import subprocess
 import sys
 
-# An unlikely delimiter for the $GITHUB_OUTPUT multiline (heredoc) format.
-_DELIM = "__DPROV_OUTPUT_EOF__"
+
+def _fresh_delimiter(value: str) -> str:
+    """A random heredoc delimiter guaranteed absent from ``value``.
+
+    The ``$GITHUB_OUTPUT`` multiline format is ``key<<DELIM\\n<value>\\nDELIM``. A *fixed*
+    delimiter lets attacker-influenced content (a candidate step's ``type_identifier``,
+    which flows verbatim into the multi-line ``summary``) embed a line equal to the
+    delimiter, closing the heredoc early and injecting forged output commands such as
+    ``passed=true`` — silently bypassing the gate. A fresh random delimiter per write,
+    verified absent from the value, is GitHub's recommended defense.
+    """
+    lines = value.splitlines()
+    while True:
+        delim = f"ghadelimiter_{secrets.token_hex(16)}"
+        if delim not in lines:
+            return delim
 
 
 def resolve_run(env, run_key, context_key, db, run=subprocess.run):
@@ -87,12 +102,18 @@ def render_outputs(report):
 
 
 def write_outputs(pairs, path):
-    """Append ``pairs`` to ``$GITHUB_OUTPUT`` using the multiline heredoc format."""
+    """Append ``pairs`` to ``$GITHUB_OUTPUT`` using the multiline heredoc format.
+
+    Each value gets its own random delimiter that is verified absent from that value, so
+    no attacker-influenced content can close the heredoc early and forge later outputs.
+    """
     if not path:
         return
     with open(path, "a", encoding="utf-8") as fh:
         for key, value in pairs:
-            fh.write(f"{key}<<{_DELIM}\n{value}\n{_DELIM}\n")
+            value = str(value)
+            delim = _fresh_delimiter(value)
+            fh.write(f"{key}<<{delim}\n{value}\n{delim}\n")
 
 
 def main(env=None):
@@ -135,10 +156,27 @@ def main(env=None):
     step_summary = env.get("GITHUB_STEP_SUMMARY")
     if step_summary:
         with open(step_summary, "a", encoding="utf-8") as fh:
-            fh.write("### DProvenanceKit regression gate\n\n```\n" + summary + "\n```\n")
+            fh.write("### DProvenanceKit regression gate\n\n" + _fenced(summary) + "\n")
     return 0
 
 
+def _fenced(text: str) -> str:
+    """Wrap ``text`` in a Markdown code fence that it cannot break out of.
+
+    ``summary`` embeds attacker-influenced step ``type_identifier``s; a fixed 3-backtick
+    fence lets a step name containing ```` ``` ```` close the block and inject arbitrary
+    Markdown into the job summary. Markdown allows fences longer than any backtick run
+    they enclose, so size the fence to one more than the longest run in ``text``.
+    """
+    longest = 0
+    run = 0
+    for ch in text:
+        run = run + 1 if ch == "`" else 0
+        longest = max(longest, run)
+    fence = "`" * max(3, longest + 1)
+    return f"{fence}\n{text}\n{fence}"
+
+
 if __name__ == "__main__":
     raise SystemExit(main())
 

dprovenancekit/alignment_engine.py

@@ -55,6 +55,14 @@ def align(
         comp_events = [
             e for e in comparison.events if e.payload.priority >= minimum_priority
         ]
+        # Reorder detection compares list position, so order by the authoritative
+        # ``sequence`` first. ``align()`` is public and accepts arbitrary runs (merged
+        # shards, OTel-ingested, hand-built) that need not arrive sequence-sorted; without
+        # this, two logically identical traces whose events merely arrive in a different
+        # list order were flagged as a spurious HIGH reordering regression. A stable sort
+        # is a no-op for store-backed runs, which already emerge ``ORDER BY sequence``.
+        base_events.sort(key=lambda e: e.sequence)
+        comp_events.sort(key=lambda e: e.sequence)
 
         collector = (
             AlignmentEvidenceCollector()

dprovenancekit/alignment_interpreter.py

@@ -6,7 +6,6 @@
 from typing import Callable, List, Optional
 
 from .alignment_contract import AlignmentExecutionContract
-from .alignment_config import AlignmentMode
 from .alignment_evidence import EvidenceCollector, InterpretationStep
 from .alignment_meta import AlignmentMetaEvent
 from .alignment_models import (
@@ -162,10 +161,14 @@ def emit_meta(payload: AlignmentMetaEvent) -> None:
                     used_comparison_indices.add(match_idx)
                 else:
                     used_comparison_indices.add(match_idx)
-                    is_reordered = (
-                        config.profile.alignment_mode != AlignmentMode.LINEAR
-                        and b_event.id in reordered_base_ids
-                    )
+                    # Reorder detection is a pure matched-pair inversion check; it does not
+                    # depend on span-aware *scoring*, so it must not be suppressed in LINEAR
+                    # mode. Gating it on ``!= LINEAR`` (as before) made the strictest audit
+                    # profile — strict_audit_v1, which is LINEAR — blind to critical-step
+                    # dependency inversion (e.g. GenerateInvoice before CreateCustomer),
+                    # the exact HIGH-risk failure the engine documents it exists to catch,
+                    # so it detected strictly *less* than the developer_debug profile.
+                    is_reordered = b_event.id in reordered_base_ids
                     if is_reordered:
                         state = AlignmentState.reordered(
                             b_event.sequence, c_event.sequence

dprovenancekit/instrument.py

@@ -298,6 +298,22 @@ async def agwrapper(*args, **kwargs):
                 try:
                     async for item in func(*args, **kwargs):
                         yield item
+                except GeneratorExit:
+                    # The consumer stopped early (``aclose()``/``break``/cancellation of
+                    # the iterating task). That is ordinary control flow, not a failure —
+                    # record a normal ``.end`` so a partially-consumed stream matches a
+                    # fully-consumed one instead of emitting a spurious CRITICAL error.
+                    end_id = _record_in_span(
+                        f"{step_name}.end",
+                        priority,
+                        {"name": step_name},
+                        engine=step_name,
+                        span_id=span_id,
+                        parent_span_id=parent,
+                    )
+                    if link_lifecycle:
+                        _link(start_id, end_id, TraceEdgeType.DERIVED_FROM)
+                    raise
                 except BaseException as error:  # noqa: BLE001 - record then re-raise
                     err_id = _record_in_span(
                         f"{step_name}.error",
@@ -345,6 +361,22 @@ def gwrapper(*args, **kwargs):
                     _link(_enclosing_step.get(), start_id, TraceEdgeType.INFORMED)
                 try:
                     result = yield from func(*args, **kwargs)
+                except GeneratorExit:
+                    # The consumer stopped early (``close()``/``break``/``islice``). That
+                    # is ordinary control flow, not a failure — record a normal ``.end``
+                    # so a partially-consumed stream matches a fully-consumed one instead
+                    # of emitting a spurious CRITICAL error that trips anomaly rules.
+                    end_id = _record_in_span(
+                        f"{step_name}.end",
+                        priority,
+                        {"name": step_name},
+                        engine=step_name,
+                        span_id=span_id,
+                        parent_span_id=parent,
+                    )
+                    if link_lifecycle:
+                        _link(start_id, end_id, TraceEdgeType.DERIVED_FROM)
+                    raise
                 except BaseException as error:  # noqa: BLE001 - record then re-raise
                     err_id = _record_in_span(
                         f"{step_name}.error",

dprovenancekit/integrations/crewai.py

@@ -66,6 +66,7 @@
 from __future__ import annotations
 
 import json
+import logging
 import threading
 import uuid
 from dataclasses import dataclass, field
@@ -75,6 +76,8 @@
 from ..event import TraceableEvent, TraceEvent
 from ..priority import TracePriority
 
+logger = logging.getLogger(__name__)
+
 # Subclass CrewAI's ``BaseEventListener`` when installed so we are a first-class listener
 # whose construction registers handlers on the global bus; fall back to ``object``
 # otherwise so the translation logic stays importable and unit-testable without the
@@ -527,7 +530,15 @@ def handle(self, kind: str, phase: str, source: Any, event: Any) -> None:
             with self._lock:
                 self._handle_locked(kind, phase, source, event)
         except Exception:  # noqa: BLE001 - instrumentation must never break the crew
-            pass
+            # Still swallowed so a translation bug can't crash the crew, but logged at
+            # debug: a silently-broken adapter (e.g. a CrewAI version whose events lack
+            # the assumed correlation fields) otherwise produces empty traces with no clue.
+            logger.debug(
+                "[DProvenanceKit] failed to record CrewAI %s.%s event",
+                kind,
+                phase,
+                exc_info=True,
+            )
 
     def _handle_locked(self, kind: str, phase: str, source: Any, event: Any) -> None:
         if kind == "crew":

dprovenancekit/integrations/google_genai.py

@@ -237,10 +237,15 @@ def __getattr__(self, name: str) -> Any:
     def generate_content(self, *args, **kwargs):
         model_name = _model_name(args, kwargs)
         # Start and end of one generate_content call share a single span so the
-        # pair reads as one node in the span tree. `record()` reads the span from
-        # the contextvar (there is no span_id kwarg), so set it for the call.
+        # pair reads as one node in the span tree. `record()` reads both the span and
+        # its parent from contextvars (there is no span_id kwarg), so set both for the
+        # call: the previously-current span becomes this call's parent. Setting only
+        # current_span_id (as before) left parent_span_id pointing at the *enclosing*
+        # span's parent — the grandparent — so a nested call attached to the wrong node.
         call_span = str(uuid.uuid4())
+        parent_span = TraceContext.current_span_id.get()
         span_token = TraceContext.current_span_id.set(call_span)
+        parent_token = TraceContext.parent_span_id.set(parent_span)
         try:
             start_event = GoogleGenAITraceEvent.make(
                 type_name="generateContentStarted",
@@ -273,6 +278,7 @@ def generate_content(self, *args, **kwargs):
 
             return response
         finally:
+            TraceContext.parent_span_id.reset(parent_token)
             TraceContext.current_span_id.reset(span_token)
 
 

dprovenancekit/integrations/llama_index.py

@@ -139,6 +139,49 @@ def _key(key: Any) -> str:
 # Bulky payload lists reduced to counts instead of being captured or dropped.
 _COUNTED_KEYS = {"nodes": "node_count", "chunks": "chunk_count"}
 
+_REDACTED = "***redacted***"
+
+# Key-name fragments that mark a value as a secret. Deliberately precise — bare "token"
+# is excluded so token-count telemetry (prompt_tokens/total_tokens) is still captured.
+_SENSITIVE_KEY_FRAGMENTS = (
+    "api_key",
+    "apikey",
+    "secret",
+    "password",
+    "passwd",
+    "authorization",
+    "credential",
+    "bearer",
+    "private_key",
+    "access_token",
+    "refresh_token",
+    "auth_token",
+    "session_token",
+)
+
+
+def _is_sensitive_key(key: Any) -> bool:
+    normalized = str(key).lower().replace("-", "_")
+    return any(fragment in normalized for fragment in _SENSITIVE_KEY_FRAGMENTS)
+
+
+def _redact_secrets(value: Any) -> Any:
+    """Recursively replace secret-keyed values with a placeholder.
+
+    LlamaIndex's ``EventPayload.SERIALIZED`` is a nested LLM/embedding config dict that has
+    shipped an ``api_key`` in some versions; stringifying it verbatim leaked the key into
+    the trace store (which this toolkit encourages committing as a golden baseline). This
+    scrubs sensitive sub-keys while preserving useful structure like the model name.
+    """
+    if isinstance(value, Mapping):
+        return {
+            _key(k): (_REDACTED if _is_sensitive_key(k) else _redact_secrets(v))
+            for k, v in value.items()
+        }
+    if isinstance(value, (list, tuple)):
+        return [_redact_secrets(v) for v in value]
+    return value
+
 
 def _payload_attributes(
     payload: Optional[Mapping[Any, Any]], capture: bool
@@ -147,7 +190,8 @@ def _payload_attributes(
 
     Node/chunk lists become counts (always — they are structural metadata); every other
     value is captured only when ``capture`` is on, stringified and truncated so no
-    single attribute can exceed the truncation limit. The exception payload is handled
+    single attribute can exceed the truncation limit, with secret-keyed values (including
+    those nested inside the serialized config) redacted. The exception payload is handled
     by the error path, never here.
     """
     attrs: Dict[str, Any] = {}
@@ -161,7 +205,10 @@ def _payload_attributes(
         if counted is not None and isinstance(v, (list, tuple)):
             attrs[counted] = len(v)
         elif capture:
-            attrs[key] = _truncate(str(v))
+            if _is_sensitive_key(key):
+                attrs[key] = _REDACTED
+            else:
+                attrs[key] = _truncate(str(_redact_secrets(v)))
     return attrs