Skip to content

fix: type errors in tracing and a2a adapter, check extras in CI - #290

Merged
pontino merged 1 commit into
mainfrom
fix/mypy-extras
Sep 18, 2026
Merged

pontino merged 1 commit into
mainfrom
fix/mypy-extras

Conversation

@pontino

@pontino pontino commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Running mypy with all extras installed reports 18 errors in eggai/tracing.py, eggai/adapters/a2a/config.py and eggai/adapters/a2a/executor.py. CI never sees them because the lint and test jobs run poetry install without extras, so the a2a, mcp and otel modules are neither type checked nor tested. This is the same gap that let #289 pass CI green while breaking the a2a adapter at runtime.

Changes

tracing.py

  • exporter is typed as str | SpanExporter. A pre-built exporter is detected with isinstance instead of hasattr.
  • The two OTLPSpanExporter classes are imported under distinct names and receive endpoint directly instead of an unpacked dict.
  • The else branch that reassigned provider to the API type is removed. The variable was not used after it.
  • Service name is resolved with service_name or os.getenv("OTEL_SERVICE_NAME") or "eggai". The previous os.getenv(key, "eggai") form makes mypy infer str | None because the left operand of or is used as type context for the generic default.

a2a adapter

  • config.py imports SecurityScheme unconditionally. The executor already requires the a2a SDK, and the plugin is only imported when a2a kwargs are passed to the agent.
  • executor.py builds and enqueues the Message directly. The fallback that enqueued a raw dict was the type error and could not work at runtime.

CI

  • The lint and test jobs install with --all-extras.

tests/test_tracing.py

  • Skip on opentelemetry.sdk instead of opentelemetry. The bare name is a namespace package, so it imports from an empty directory left behind after the otel extras are uninstalled from a venv, and collection then fails on the next import.

Validation

Check Result
mypy, all extras no issues, 22 source files (was 18 errors)
mypy, no extras no issues
ruff check and format clean
pytest, all extras, live Redis and Redpanda 124 passed, 0 skipped
pytest, no extras, leftover otel directory present 117 passed, 1 skipped

Also checked at runtime that all four setup_tracing exporter paths attach the expected exporter, and that _send_agent_response enqueues a Message on an EventQueue.

mypy reported 18 errors in tracing.py and the a2a adapter when run with
all extras installed. CI never saw them because the lint and test jobs
install without extras. The same gap let #289 pass CI while breaking the
a2a adapter at runtime.

tracing.py: type the exporter argument, pass endpoint directly, import
the two OTLP exporters under distinct names, drop the dead provider
reassignment.

a2a: import SecurityScheme unconditionally, remove the executor fallback
that enqueued a raw dict.

CI: lint and test jobs install all extras.

test_tracing.py: skip on opentelemetry.sdk instead of the bare namespace
package, which resolves to an empty leftover directory after the extras
are uninstalled from a venv.
@github-actions

Copy link
Copy Markdown
Contributor

QualOps Code Quality Analysis

Status: ⚠️ FAILED - Critical or high severity issues found

Summary

  • Total Issues: 5
  • Critical: 0 🔴
  • High: 2 🟠
  • Medium: 1 🟡
  • Low: 2 🟢
  • Files Analyzed: 7

🟠 High Issues (2)

  • sdk/eggai/adapters/a2a/executor.py:201 - bug
    event_queue.enqueue_event is called with a raw Message object, but the A2A EventQueue API may expect an event wrapper (e.g. SendMessageResponse or a task-status event) rather than a bare Message.

  • sdk/eggai/tracing.py:148 - bug
    Removing the else branch causes setup_tracing to silently create and discard a TracerProvider+BatchSpanProcessor (and its background worker thread) when a non-ProxyTracerProvider is already registered, leaking an export thread that runs indefinitely.

🟡 Medium Issues (1)

  • sdk/eggai/tracing.py:138 - bug
    OTLP exporter now receives explicit endpoint=None instead of having the keyword argument omitted, changing behaviour when no endpoint is configured.

📊 Full Report

View detailed report


Powered by QualOps

@pontino

pontino commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator Author

Checked each QualOps finding against the installed libraries and against main. None of them holds. The two low findings were filtered out of the report by QualOps itself (the run log ends with "Low Issues: 0"), so there is nothing to check there.

1. enqueue_event with a bare Message (executor.py:201, high)

EventQueue.enqueue_event in the installed a2a-sdk is declared as:

(self, event: Message | Task | TaskStatusUpdateEvent | TaskArtifactUpdateEvent) -> None

Message is a member of the Event union. There is no wrapper type. Running a2a's own EventConsumer.consume_one() on the queue after _send_agent_response returns the Message with the payload intact:

EventConsumer.consume_one -> Message {'answer': 42}

The Message construction and the enqueue call are unchanged from main. The whitespace-insensitive diff of the method removes only the try/except wrapper and the fallback that enqueued a raw dict, which was the mypy error.

2. Removed else branch leaks a BatchSpanProcessor thread (tracing.py:148, high)

On main the provider and processor were also created before the ProxyTracerProvider check. The removed else only rebound a local variable that nothing read afterwards. Running main's tracing.py and this branch's in separate processes, with and without a real TracerProvider already registered, gives identical results:

variant provider pre-registered new threads after setup_tracing("console") processors on global provider
main yes OtelBatchSpanRecordProcessor 0
branch yes OtelBatchSpanRecordProcessor 0
main no OtelBatchSpanRecordProcessor 1
branch no OtelBatchSpanRecordProcessor 1

The orphaned processor when a provider already exists is pre-existing behaviour, not introduced here. Could be tightened in a separate change by building the provider only inside the proxy check.

3. endpoint=None vs omitting the argument (tracing.py:138, medium)

Both exporters declare endpoint: str | None = None, so omitting the argument binds None. Both resolve it as endpoint or environ.get(...). Constructing each exporter both ways and comparing the resolved _endpoint:

environment http omitted / endpoint=None grpc omitted / endpoint=None
none http://localhost:4318/v1/traces both localhost:4317 both
OTEL_EXPORTER_OTLP_ENDPOINT=http://collector:4318 http://collector:4318/v1/traces both collector:4318 both
OTEL_EXPORTER_OTLP_TRACES_ENDPOINT=http://traces:9999/v1/traces http://traces:9999/v1/traces both traces:9999 both

Every pair is equal.

@pontino
pontino merged commit 51f1af7 into main Sep 18, 2026
12 of 13 checks passed
@pontino
pontino deleted the fix/mypy-extras branch September 18, 2026 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant