feat(export): include accumulated warnings in output json (#113)

[aryanputta]

Closes #113.

What

Accumulated TraceWarnings that fire during processing are now exported into the output json under otherData.issues, grouped by severity:

{
  "otherData": {
    "issues": {
      "warning": {
        "zero_gap_time": "Detected 3 identical timestamps between consecutive events. ...",
        "flex_mismatch": "CAT: Found 21 categorization mismatches compared to FLEX src classifier."
      },
      "error": {
        "similarity_div0": "UTL: Found 1 errors while computing similarity (divide by zero). ..."
      }
    }
  }
}

This reuses the existing TraceWarning class and lets the exporter emit the active warnings, so problems are not lost when the console output is ignored or drowned out.

How

• AbstractContext.drain() returns emit_issue_events(), so warnings leave the context through the existing drain contract. No processor-side accumulator.
• Warning meta-events are DiagnosticEvents, a TraceEvent subtype. EventProcessor.process() short-circuits that type through convert_events(), so diagnostic data is not filtered or mutated by domain stages.
• AbstractVerificationContext.drain() emits verification data directly, so verification events and issue events are not double-emitted.
• The args key is the severity (warning or error, from TraceWarning.severity()), and JsonFileTraceExporter folds those events into otherData.issues under that key. Empty issue sets add no section.

Tests

• test_context.py: issue events for both a warning and an is_error warning.
• test_exporter.py: json exporter writes otherData.issues, separates errors from warnings, keeps the meta-events out of traceEvents, and skips the section when there are no warnings.
• test_processing_issues.py: end-to-end drain to exporter, plus the bypass invariant.
• Full suite: 206 passed, 5 xfailed. flake8 clean.

Scope note

This covers the pipeline stage warnings. Ingestion-side warnings live on the importer and could be folded in as a follow-up if you want them in the same section.

[lasch]

Need to hold this for a bit, sorry. There's a design change coming with respect to the verification pipeline that would simplify the approach.

Comment on current approach:

• not yet convinced that additional tracking of contexts in the processing is needed. Kind of stores the contexts twice.

[aryanputta]

Understood, happy to hold for the verification-pipeline change.

On the double-storage point: the second store exists only because warnings raised during processing have no path back to the context at export time today. If the redesign gives the exporter access to the context's warnings directly, I would drop the processor-side tracking entirely and just read from the context. Does the new pipeline expose that, or should I rebase onto it once it lands and resimplify then?

[lasch]

I just created #117 introducing a path from context warnings to exporter using metadata events. The basics should probably work for the regular processing pipeline too.

[lasch]

I suggest to follow the concepts that were introduced in #117 to couple the warnings into the exported files. Something along the lines of:

• special meta-data events passed through the pipeline
• extended exporter captures the events and turns them into fields within the output

[lasch]

My impression is that it's introducing too many redundant paths.
I was hoping the existing verificiation-specific approach could be generalized a bit to avoid this duplication (see details below).

I strongly encourage to address this in incremental steps to allow review and discussion of small contained steps and avoid convoluted threads of interaction.

Analysis of the top-level concern:

F1 — Architecture: emit_issue_events() belongs in drain(), not in EventProcessor

processing.py:143-155, context.py:55-66

The codebase already has the right pattern: AbstractVerificationContext.drain() emits M-events by extending drain(). The PR's emit_issue_events() call in EventProcessor.drain() bypasses that contract and couples the processor to the internal warning mechanism of contexts.

The proper fix is to have AbstractContext.drain() return emit_issue_events(), following the same override chain as AbstractVerificationContext. Remove the issue_events accumulator and emit_issue_events() call from EventProcessor.drain() entirely.

One critical nuance to preserve: the current code deliberately passes drain-returned events through self.process() (all remaining pipeline stages) but passes emit_issue_events() results only through self.convert_events(), bypassing pipeline stages. When moving emit_issue_events() into drain(), the issue events would then also flow through self.process(). That may be fine since diagnostic meta-events shouldn't be modified by domain stages, but the decision needs to be explicit — either with a comment or by having the remaining stages pass M-events with TRACE_ISSUE_EVENT_NAME through unchanged.

Recommended approach: encode the skip in the event type rather than in caller logic or stage contracts. Introduce a DiagnosticEvent(TraceEvent) subclass in types.py and add a single guard at the top of EventProcessor.process():

def process(self, event: TraceEvent) -> list[aiuev.AbstractEventType]:
    if isinstance(event, DiagnosticEvent):
        return self.convert_events([event])   # bypass pre_process; diagnostic events are internally constructed
    event_list = self.pre_process(event)
    ...

emit_issue_events() and _emit_verification_events() both return DiagnosticEvent objects. AbstractContext.drain() can then return emit_issue_events() naturally — those events short-circuit through self.process() even when they flow through the drain loop. This resolves multiple issues together: the external emit_issue_events() call disappears from EventProcessor.drain(), AbstractVerificationContext.drain() stops calling super().drain() (eliminating double-emission), and the bypass is declared in the type rather than as undocumented processor magic. A subclass is preferable to a reserved dict key ("_diagnostic": True) because it carries no data payload and requires no stripping before export — convert_events() only reads the dict content, not the Python type.

[lasch]

unnecessary comment

[lasch]

What's the reason for adding this extra list of events?
If the drain() function emits an event, it will be passed through the pipeline. I don't think there's a need for an additional mechanism.

[lasch]

same as above, I think, this is unnecessary when using the drain mechanism to emit issue events.

[lasch]

Sorry for the delay in the review. Got pulled into too many other things.
One minor comment below.

[lasch]

instead of finding can this use either warning or error depending on whether the warning had is_error flag set or not?

[aryanputta]

Done in bf1d7e5. TraceWarning now exposes severity() ("error" if is_error was set, otherwise "warning") and the event args use that as the key:

"args": {w.severity(): name, "text": str(w)}

Since the key now carries the severity, the json exporter groups otherData.issues by it rather than keeping one flat map:

"issues": {
  "warning": {"zero_gap_time": "Detected 3 identical timestamps ..."},
  "error":   {"similarity_div0": "UTL: Found 1 errors while computing similarity ..."}
}

Happy to keep it flat and only carry the severity in the event if you prefer the simpler output shape.

[lasch]

The verification report uses warnings and errors as higher level keys. I suggest we do the same here and skip the issues level. Going straight to:

    "warnings": [{}, {}, ...],
    "errors": [{}, {}, ...],

We can't really match the verification report, but if we match closely enough, it will simplify any automation that consumes the json files.

[lasch]

Just noticed this (and it existed this way before). I think, this becomes more complex than it should be. We're setting the loglevel based in __init__ based on arg is_error and then later retrieving the info by checking the loglevel.

If you don't mind, something like is_error should become a first-class member of the class replacing warn_level. If warn_level is needed for logging (one place in the code, if I see this correctly), use the code that's currently in __init__ to determine the warn_level from the is_error flag.
It won't save much but it's a less convoluted detour to handle warning vs. error.

[lasch]

@aryanputta thanks for the updates. You may want to check the failed tests. Looks like the linter has complaints.
I'll review asap.