fix(langextract): bridge ContextTraceEmitter so real agent events produce a non-empty trace#1420
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a context→action bridge for Langextract observability, exposes Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/App / Command
participant CtxEmitter as ContextTraceEmitter
participant Bridge as _ContextToActionBridge
participant ActionEmitter as TraceEmitter (Action)
participant Sink as LangextractSink
CLI->>CtxEmitter: set_context_emitter(ContextTraceEmitter(session_id))
Note over CLI,CtxEmitter: context emitter installed (bridge uses session_id)
CtxEmitter->>Bridge: emit(context_event)
activate Bridge
Bridge->>Bridge: map context_event -> ActionEvent(s)
Bridge->>ActionEmitter: emit(action_event)
deactivate Bridge
ActionEmitter->>Sink: emit(action_event)
Sink->>Sink: record event, update source_text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a bridge between the core agent runtime's context events and the Langextract observability sink, ensuring that single-agent flows, tool calls, and LLM responses are correctly captured in traces. The implementation includes a new _ContextToActionBridge adapter and updates to the CLI and render commands to initialize this bridge. Review feedback suggests refactoring duplicated setup logic into a centralized helper method to improve maintainability and enhancing error handling consistency in the render command to ensure observability failures are reported to the user.
| try: | ||
| from praisonaiagents.trace.context_events import ( | ||
| ContextTraceEmitter, | ||
| set_context_emitter, | ||
| ) | ||
| context_emitter = ContextTraceEmitter( | ||
| sink=sink.context_sink(), | ||
| session_id="praisonai-cli", | ||
| enabled=True, | ||
| ) | ||
| set_context_emitter(context_emitter) | ||
| except Exception as e: # pragma: no cover - defensive | ||
| if verbose: | ||
| typer.echo(f"Warning: could not bridge context emitter: {e}", err=True) |
There was a problem hiding this comment.
The logic for setting up the ContextTraceEmitter and bridging it to the LangextractSink is duplicated here and in src/praisonai/praisonai/cli/commands/langextract.py. Consider refactoring this into a helper method (e.g., LangextractSink.bridge_context_events(session_id)) to centralize the lazy imports and configuration. This would improve maintainability and ensure consistent behavior across different CLI entry points.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
This exception catch is silent, unlike the similar block in src/praisonai/praisonai/cli/app.py which reports a warning when verbose. Since the render command is specifically intended for generating traces, a failure to bridge the context emitter will result in an empty or incomplete trace. It would be better to echo a warning to the user so they understand why the output might be missing data.
References
- Observability failures should be reported to the user when they are the primary focus of the command, even if they are non-fatal to the core execution.
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Pull request overview
Fixes the langextract observability integration so “real” agent runtime lifecycle events (emitted via ContextTraceEmitter) are bridged into the existing ActionEvent-based sink pipeline, preventing empty langextract renders for typical runs.
Changes:
- Added a
_ContextToActionBridgeadapter and exposed it viaLangextractSink.context_sink()to mapContextEvent→ActionEvent. - Updated CLI wiring (
--observe langextractandpraisonai langextract render) to also install aContextTraceEmitterusing the bridge. - Removed a redundant inner
import osthat could shadow the module import and causeUnboundLocalErrorwhen ACP/LSP are disabled.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/praisonai/praisonai/observability/langextract.py | Adds the context→action bridge and exposes context_sink() for wiring ContextTraceEmitter into LangextractSink. |
| src/praisonai/praisonai/cli/app.py | Extends --observe langextract setup to also install a ContextTraceEmitter bridging into LangextractSink. |
| src/praisonai/praisonai/cli/commands/langextract.py | Updates render to install a ContextTraceEmitter bridge during the run. |
| src/praisonai/tests/unit/test_langextract_sink.py | Adds regression tests for the context bridge and CLI setup behavior. |
| src/praisonai/praisonai/agents_generator.py | Removes redundant inner import os to avoid UnboundLocalError when ACP/LSP are off. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_setup_observability_registers_context_emitter(self): | ||
| """`--observe langextract` must install the bridge on the context emitter.""" | ||
| import praisonai.cli.app as cli_app | ||
| from praisonaiagents.trace.context_events import get_context_emitter, set_context_emitter | ||
|
|
||
| previous_emitter = get_context_emitter() | ||
| try: | ||
| # Make test deterministic even when optional dependency is not installed. |
| except Exception: | ||
| pass |
| if et_value == self._CTX_AGENT_START: | ||
| self._sink.emit(ActionEvent( | ||
| event_type=ActionEventType.AGENT_START.value, | ||
| timestamp=ts, | ||
| agent_name=agent, | ||
| metadata={"input": data.get("input") or data.get("goal") or ""}, | ||
| )) |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Phase 1: Architecture Review per AGENTS.md
Phase 2: Address Reviewer Issues
Phase 3: Final Verdict
Architecture Analysis✅ Protocol-Driven Design: The implementation correctly follows AGENTS.md §4.1:
✅ Backward Compatibility:
✅ Performance:
Issues Fixed✅ Error Handling: Fixed silent exception swallowing in ✅ Test Reliability: Maintained proper test determinism with mocking while adding ✅ Code Duplication: Refactored duplicated context emitter setup into Final Assessment✅ APPROVED: This PR implements a clean, protocol-driven solution to bridge ContextTraceEmitter events into the langextract observability sink. The implementation:
The PR is ready to merge and will resolve the langextract integration gap while maintaining the framework's architectural principles. Files Modified:
Commit: 79100c99 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/praisonai/praisonai/cli/commands/langextract.py (1)
85-97: Swallowed exception hides bridge setup failures.
except Exception: passsilently discards the very error this PR is trying to fix (empty-trace regressions). This is also inconsistent with_setup_langextract_observabilityincli/app.pywhich surfaces a warning. Since this is an interactive CLI command (not a hot path), always warning on stderr is appropriate.Proposed diff
- try: - from praisonaiagents.trace.context_events import ( - ContextTraceEmitter, - set_context_emitter, - ) - context_emitter = ContextTraceEmitter( - sink=sink.context_sink(), - session_id="praisonai-langextract-render", - enabled=True, - ) - set_context_emitter(context_emitter) - except Exception: - pass + try: + from praisonaiagents.trace.context_events import ( + ContextTraceEmitter, + set_context_emitter, + ) + context_emitter = ContextTraceEmitter( + sink=sink.context_sink(), + session_id="praisonai-langextract-render", + enabled=True, + ) + set_context_emitter(context_emitter) + except Exception as e: # pragma: no cover - defensive + typer.echo(f"Warning: could not bridge context emitter: {e}", err=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/cli/commands/langextract.py` around lines 85 - 97, The bare except in the langextract CLI bridge setup swallows failures when importing/initializing ContextTraceEmitter and set_context_emitter; replace the silent except with code that captures the exception (e) and emits a warning to stderr (matching the behavior in _setup_langextract_observability in cli/app.py) so failures are visible during interactive use — locate the try block that references ContextTraceEmitter and set_context_emitter in langextract.py and change the except Exception: pass to an except Exception as e: that logs or prints a clear warning including the exception details (e).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/tests/unit/test_langextract_sink.py`:
- Around line 410-417: The test
test_setup_observability_registers_context_emitter mutates global emitter state
via praisonai.cli.app._setup_langextract_observability and then dereferences
emitter.enabled which can raise AttributeError if langextract isn't installed;
modify the test to capture the pre-existing emitter from
praisonaiagents.trace.context_events.get_context_emitter (and/or
set_context_emitter if available), call _setup_langextract_observability, assert
safely by first checking that get_context_emitter() is not None (skip or xfail
if langextract absent), and finally restore the original emitter (via
set_context_emitter or equivalent) in a teardown/finally block so global state
is returned to its prior value to avoid test order dependence.
---
Nitpick comments:
In `@src/praisonai/praisonai/cli/commands/langextract.py`:
- Around line 85-97: The bare except in the langextract CLI bridge setup
swallows failures when importing/initializing ContextTraceEmitter and
set_context_emitter; replace the silent except with code that captures the
exception (e) and emits a warning to stderr (matching the behavior in
_setup_langextract_observability in cli/app.py) so failures are visible during
interactive use — locate the try block that references ContextTraceEmitter and
set_context_emitter in langextract.py and change the except Exception: pass to
an except Exception as e: that logs or prints a clear warning including the
exception details (e).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0acfcca-e600-4f2e-9bd4-bd99edde97cd
📒 Files selected for processing (5)
src/praisonai/praisonai/agents_generator.pysrc/praisonai/praisonai/cli/app.pysrc/praisonai/praisonai/cli/commands/langextract.pysrc/praisonai/praisonai/observability/langextract.pysrc/praisonai/tests/unit/test_langextract_sink.py
💤 Files with no reviewable changes (1)
- src/praisonai/praisonai/agents_generator.py
Completed a full PR audit and incorporated available reviewer feedback (only CodeRabbit had actionable follow-ups; no substantive Qodo/Gemini findings were present on this PR). Findings:
Addressed in commit Also re-ran targeted unit tests: |
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Implements 4 follow-ups after PR #1420 to enhance langextract observability: **Follow-up 1: Richer llm_response content wiring** - Add _extract_llm_response_content() helper in chat_mixin.py - Extract actual message content instead of str(response) for better observability - Improves HTML trace quality by showing actual agent responses **Follow-up 2: LangfuseSink context-emitter bridge** - Add _ContextToActionBridge class for forwarding ContextEvent → ActionEvent - Add context_sink() method to LangfuseSink for ContextTraceSinkProtocol - Update _setup_langfuse_observability to wire both action + context emitters - Enables LangfuseSink to capture rich agent lifecycle events **Follow-up 3: langextract_tools.py tool registration** - Create first-class langextract_extract and langextract_render_file tools - Add to tools/__init__.py TOOL_MAPPINGS for lazy loading - Follows AGENTS.md patterns (decorator-based, lazy imports, optional deps) - Agents can now call langextract functionality directly as tools **Follow-up 4: Documentation updates** - Add comprehensive langextract.mdx in external PraisonAIDocs repo - Cover CLI usage (--observe langextract, render, view), Python API, tools - Created PR: MervinPraison/PraisonAIDocs#162 Architecture: Protocol-driven design per AGENTS.md - core protocols in praisonaiagents, heavy implementations in praisonai wrapper, zero regressions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/praisonai/praisonai/observability/langextract.py (2)
82-91: Minor: calldata.get("result")once.The ternary invokes
data.get("result")three times. Cache it into a local.♻️ Proposed refactor
elif et_value == self._CTX_TOOL_END: + result = data.get("result") self._sink.emit(ActionEvent( event_type=ActionEventType.TOOL_END.value, timestamp=ts, agent_name=agent, tool_name=data.get("tool_name"), duration_ms=(data.get("duration_ms") or 0.0), status=data.get("status") or "ok", - tool_result_summary=str(data.get("result"))[:500] if data.get("result") is not None else None, + tool_result_summary=str(result)[:500] if result is not None else None, ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/observability/langextract.py` around lines 82 - 91, The ternary for tool_result_summary repeatedly calls data.get("result"); cache this once in a local variable (e.g., result = data.get("result")) before the ActionEvent construction in the _CTX_TOOL_END branch, then use that local for duration_ms, status fallback if needed and for computing tool_result_summary (e.g., str(result)[:500] if result is not None else None) to avoid multiple dictionary lookups; update the code around the _CTX_TOOL_END branch where _sink.emit(ActionEvent(...)) is created.
92-101: Consider bridgingLLM_REQUEST(prompt) alongsideLLM_RESPONSE.Per
context_events.pythe emitter also publishesLLM_REQUEST,SESSION_START/END,AGENT_HANDOFF, andMESSAGE_ADDED. Your current bridge silently drops them. For langextract rendering, the LLM prompt is often more useful than the raw response, and handoffs help distinguish multi-agent flows. At minimum, consider mappingLLM_REQUESTto anINPUT/LLM_REQUESTActionEvent so the rendered HTML shows what was asked — otherwise single-agent runs without tool calls can still look sparse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/observability/langextract.py` around lines 92 - 101, The handler currently only maps self._CTX_LLM_RESPONSE to an OUTPUT ActionEvent and drops other context events; add a branch for self._CTX_LLM_REQUEST (and optionally for SESSION_START/END, AGENT_HANDOFF, MESSAGE_ADDED) that extracts the prompt/request (e.g., from data.get("request_content") or data.get("prompt") or data.get("content")) and calls self._sink.emit(ActionEvent(...)) with event_type=ActionEventType.INPUT.value (and include a distinguishing sub-type or tool_result_summary like "LLM_REQUEST" if you want to preserve the original kind), reusing the same pattern as the existing et_value == self._CTX_LLM_RESPONSE block and ensuring you reference et_value, self._CTX_LLM_REQUEST, self._sink.emit, ActionEvent, and ActionEventType to locate and implement the fix.src/praisonai/tests/unit/test_langextract_sink.py (1)
418-421: Redundant patch when paired with theskipif.If the
skipifabove stays,langextractis already importable when this test runs, so patchingimportlib.util.find_specto returnobject()is a no-op. If you take the "drop the skip" fix from the previous comment, this patch becomes necessary and correct — just make sure the two choices stay consistent. No change needed on its own.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/unit/test_langextract_sink.py` around lines 418 - 421, The patch of importlib.util.find_spec in the test for _setup_langextract_observability is redundant if the surrounding skipif guarantees langextract is importable; decide and make consistent: either keep the skipif and remove the patch("importlib.util.find_spec", return_value=object()) call from the test, or remove the skipif and retain the patch so the test simulates langextract being present; update the test named around _setup_langextract_observability accordingly so the skip/patch choice is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/observability/langextract.py`:
- Around line 157-184: The bridge_context_events helper currently calls
set_context_emitter(context_emitter) and drops the returned token, which
silently overwrites any existing emitter and prevents restoring prior state;
update bridge_context_events to first call get_context_emitter() and if an
emitter already exists call warn_callback (or compose emitters if composition is
supported) instead of blindly clobbering, then call set_context_emitter(...) and
capture the returned token and return that token (or return an uninstall
callable that calls reset_context_emitter(token)) so callers can restore the
previous emitter; reference ContextTraceEmitter, set_context_emitter,
get_context_emitter, and reset_context_emitter in your changes and ensure
ImportError handling/warn_callback behavior remains intact.
In `@src/praisonai/tests/unit/test_langextract_sink.py`:
- Line 410: The skip decorator references an undefined helper
_langextract_available causing import-time NameError and breaking test
collection; fix by either removing the line "@pytest.mark.skipif(not
_langextract_available(), reason=...)" entirely (preferred) or by adding a
simple helper function named _langextract_available that returns a boolean
(e.g., try: import langextract; return True except ImportError: return False) so
the decorator can call it without error; update references to
_langextract_available in the test module accordingly.
---
Nitpick comments:
In `@src/praisonai/praisonai/observability/langextract.py`:
- Around line 82-91: The ternary for tool_result_summary repeatedly calls
data.get("result"); cache this once in a local variable (e.g., result =
data.get("result")) before the ActionEvent construction in the _CTX_TOOL_END
branch, then use that local for duration_ms, status fallback if needed and for
computing tool_result_summary (e.g., str(result)[:500] if result is not None
else None) to avoid multiple dictionary lookups; update the code around the
_CTX_TOOL_END branch where _sink.emit(ActionEvent(...)) is created.
- Around line 92-101: The handler currently only maps self._CTX_LLM_RESPONSE to
an OUTPUT ActionEvent and drops other context events; add a branch for
self._CTX_LLM_REQUEST (and optionally for SESSION_START/END, AGENT_HANDOFF,
MESSAGE_ADDED) that extracts the prompt/request (e.g., from
data.get("request_content") or data.get("prompt") or data.get("content")) and
calls self._sink.emit(ActionEvent(...)) with
event_type=ActionEventType.INPUT.value (and include a distinguishing sub-type or
tool_result_summary like "LLM_REQUEST" if you want to preserve the original
kind), reusing the same pattern as the existing et_value ==
self._CTX_LLM_RESPONSE block and ensuring you reference et_value,
self._CTX_LLM_REQUEST, self._sink.emit, ActionEvent, and ActionEventType to
locate and implement the fix.
In `@src/praisonai/tests/unit/test_langextract_sink.py`:
- Around line 418-421: The patch of importlib.util.find_spec in the test for
_setup_langextract_observability is redundant if the surrounding skipif
guarantees langextract is importable; decide and make consistent: either keep
the skipif and remove the patch("importlib.util.find_spec",
return_value=object()) call from the test, or remove the skipif and retain the
patch so the test simulates langextract being present; update the test named
around _setup_langextract_observability accordingly so the skip/patch choice is
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b217da5-2bbc-4ef2-b357-f359022ef211
📒 Files selected for processing (4)
src/praisonai/praisonai/cli/app.pysrc/praisonai/praisonai/cli/commands/langextract.pysrc/praisonai/praisonai/observability/langextract.pysrc/praisonai/tests/unit/test_langextract_sink.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/praisonai/praisonai/cli/commands/langextract.py
| @staticmethod | ||
| def bridge_context_events(sink: "LangextractSink", session_id: str, warn_callback=None) -> None: | ||
| """ | ||
| Helper method to set up context event bridging for the given sink. | ||
|
|
||
| Args: | ||
| sink: LangextractSink instance to bridge | ||
| session_id: Session ID for the context emitter | ||
| warn_callback: Optional callback function for warnings, called with message string | ||
| """ | ||
| try: | ||
| from praisonaiagents.trace.context_events import ( | ||
| ContextTraceEmitter, | ||
| set_context_emitter, | ||
| ) | ||
| context_emitter = ContextTraceEmitter( | ||
| sink=sink.context_sink(), | ||
| session_id=session_id, | ||
| enabled=True, | ||
| ) | ||
| set_context_emitter(context_emitter) | ||
| except ImportError: | ||
| # Context emitter bridging is optional if not available | ||
| if warn_callback: | ||
| warn_callback("ContextTraceEmitter not available") | ||
| except Exception as e: | ||
| if warn_callback: | ||
| warn_callback(f"could not bridge context emitter: {e}") |
There was a problem hiding this comment.
Unconditionally clobbers any pre-existing context emitter, and discards the reset token.
set_context_emitter(context_emitter) returns a token that callers can use with reset_context_emitter(token) to restore prior state (see src/praisonai/praisonai/recipe/core.py lines 197-206). Here the token is thrown away, which has two consequences:
- Silent clobbering: If another subsystem (e.g. the replay ContextTraceWriter in
recipe/core.py) has already installed a context emitter, this call replaces it with no warning — events meant for replay stop being captured, or the order of setup determines who "wins". At minimum, detect an existing emitter (get_context_emitter()) and either compose or log a warning viawarn_callback. - No way to unwind: For tests and for any caller that wants to scope the bridge to a single run, the installed emitter is permanent for the process. Returning the token (or an "uninstall" callable) from this helper would also simplify the follow-up test that currently has to save/restore state manually.
🛡️ Suggested shape
`@staticmethod`
- def bridge_context_events(sink: "LangextractSink", session_id: str, warn_callback=None) -> None:
+ def bridge_context_events(sink: "LangextractSink", session_id: str, warn_callback=None):
"""
Helper method to set up context event bridging for the given sink.
- ...
+
+ Returns:
+ The token from ``set_context_emitter`` (or ``None`` on failure) so
+ callers can later call ``reset_context_emitter(token)``.
"""
try:
from praisonaiagents.trace.context_events import (
ContextTraceEmitter,
+ get_context_emitter,
set_context_emitter,
)
+ existing = get_context_emitter()
+ if existing is not None and getattr(existing, "enabled", False) and warn_callback:
+ warn_callback(
+ "overriding pre-existing context emitter; prior observers will stop receiving events"
+ )
context_emitter = ContextTraceEmitter(
sink=sink.context_sink(),
session_id=session_id,
enabled=True,
)
- set_context_emitter(context_emitter)
+ return set_context_emitter(context_emitter)
except ImportError:
# Context emitter bridging is optional if not available
if warn_callback:
warn_callback("ContextTraceEmitter not available")
+ return None
except Exception as e:
if warn_callback:
warn_callback(f"could not bridge context emitter: {e}")
+ return None🧰 Tools
🪛 Ruff (0.15.10)
[warning] 182-182: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/observability/langextract.py` around lines 157 - 184,
The bridge_context_events helper currently calls
set_context_emitter(context_emitter) and drops the returned token, which
silently overwrites any existing emitter and prevents restoring prior state;
update bridge_context_events to first call get_context_emitter() and if an
emitter already exists call warn_callback (or compose emitters if composition is
supported) instead of blindly clobbering, then call set_context_emitter(...) and
capture the returned token and return that token (or return an uninstall
callable that calls reset_context_emitter(token)) so callers can restore the
previous emitter; reference ContextTraceEmitter, set_context_emitter,
get_context_emitter, and reset_context_emitter in your changes and ensure
ImportError handling/warn_callback behavior remains intact.
| assert "agent_end" in types | ||
| assert sink._source_text == "Write a haiku" | ||
|
|
||
| @pytest.mark.skipif(not _langextract_available(), reason="langextract not installed") |
There was a problem hiding this comment.
Blocker: _langextract_available is undefined — breaks test module collection.
@pytest.mark.skipif(not _langextract_available(), ...) is evaluated at import time, so pytest will raise NameError: name '_langextract_available' is not defined while collecting this file and all tests in the module (not just this one) will fail to run. Ruff flags this as F821.
Since the code path under test does not actually require langextract to be importable (setup only imports praisonaiagents.trace.context_events, and import langextract is lazy inside _render), the simplest fix is to drop the skip entirely. If you do want to keep it, define the helper.
🐛 Proposed fix — drop the skip (preferred)
- `@pytest.mark.skipif`(not _langextract_available(), reason="langextract not installed")
def test_setup_observability_registers_context_emitter(self):Alternative — define the helper if the skip is intentional
_REAL_IMPORT = builtins.__import__
+
+
+def _langextract_available() -> bool:
+ import importlib.util
+ return importlib.util.find_spec("langextract") is not None🧰 Tools
🪛 Ruff (0.15.10)
[error] 410-410: Undefined name _langextract_available
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/tests/unit/test_langextract_sink.py` at line 410, The skip
decorator references an undefined helper _langextract_available causing
import-time NameError and breaking test collection; fix by either removing the
line "@pytest.mark.skipif(not _langextract_available(), reason=...)" entirely
(preferred) or by adding a simple helper function named _langextract_available
that returns a boolean (e.g., try: import langextract; return True except
ImportError: return False) so the decorator can call it without error; update
references to _langextract_available in the test module accordingly.
…w into sink Problem ------- The merged langextract integration (#1412, PR #1413) wired LangextractSink to get_default_emitter()/ActionEvent only. That pipeline has just two producers in the entire core SDK (RouterAgent token usage, PlanningAgent plan_created), so a typical single-agent run with '--observe langextract' or 'praisonai langextract render <yaml>' produced an empty HTML file. The base agent runtime (chat_mixin, tool_execution, unified_execution_mixin) actually emits rich lifecycle events via ContextTraceEmitter / ContextTraceSinkProtocol in praisonaiagents.trace.context_events. Fix (wrapper-only, zero core changes) ------------------------------------- - observability/langextract.py: add _ContextToActionBridge adapter that forwards ContextEvent -> ActionEvent (agent_start, tool_call_start/end, llm_response->OUTPUT, agent_end). Expose LangextractSink.context_sink(). - cli/app.py: _setup_langextract_observability now also installs a ContextTraceEmitter wired to the bridge. - cli/commands/langextract.py: 'render' command does the same wiring for the duration of the workflow run. - tests: add 3 regression tests covering the bridge and setup path. Incidental fix -------------- agents_generator.py:1112 had a local 'import os' inside an 'if acp/lsp enabled' block that shadowed the module-level import and caused UnboundLocalError at line 1179 for any YAML run with acp/lsp disabled. Removed the redundant local import. Verification ------------ - 19/19 langextract unit tests pass (16 existing + 3 new). - Real agentic e2e: Agent.start() with --observe langextract now produces non-empty trace.html (3036 B) + trace.jsonl (1356 B) with agent_run and final_output extractions. - 'praisonai langextract render simple.yaml' end-to-end now produces render.html and render.jsonl; previously printed 'Trace was not rendered'.
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/cec32ee8-0573-405f-86d1-6a28fb51b609 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
…endency Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/cec32ee8-0573-405f-86d1-6a28fb51b609 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
…e code duplication - Add LangextractSink.bridge_context_events() helper method to centralize context emitter setup - Fix silent exception swallowing in langextract render command - now properly reports bridge failures - Add proper skipif guard for optional dependency tests - Use callback pattern for clean separation of CLI concerns Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
force-pushed
the
fix/langextract-context-bridge
branch
from
79100c9 to
2bd0287
Compare
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Summary
Follow-up to #1412 / PR #1413. The merged langextract integration passed unit tests but captured zero events from real agent runs, so
--observe langextractandpraisonai langextract render <yaml>produced an empty HTML file on any typical flow.Root cause
LangextractSinkwas wired throughget_default_emitter()/ActionEventonly. A grep of the entire core shows that pipeline has just two producers:praisonaiagents/agent/router_agent.py:253— RouterAgent token usagepraisonaiagents/agents/agents.py:2344— PlanningAgentplan_createdAll the rich lifecycle events (
agent_start,agent_end,tool_call_start/end,llm_request,llm_response) are emitted exclusively viaContextTraceEmitter/ContextTraceSinkProtocolinpraisonaiagents/trace/context_events.py(seechat_mixin.py,tool_execution.py,unified_execution_mixin.py).Fix (wrapper-only, zero core changes)
observability/langextract.py: add_ContextToActionBridgeadapter implementingContextTraceSinkProtocolthat forwardsContextEvent→ActionEvent(agent_start,tool_call_start/end,llm_response→OUTPUT,agent_end). ExposeLangextractSink.context_sink().cli/app.py—_setup_langextract_observabilitynow also installs aContextTraceEmitterwired to the bridge.cli/commands/langextract.py—rendercommand performs the same wiring for the duration of the run.tests/unit/test_langextract_sink.py— 3 new regression tests:test_context_sink_returns_bridgetest_bridge_maps_context_events_to_action_eventstest_setup_observability_registers_context_emitterIncidental fix
agents_generator.py:1112hadimport osinside anif acp_enabled or lsp_enabled:block, which shadowed the module-level import and triggeredUnboundLocalErrorat line 1179 for any YAML run with both flags disabled. Removed the redundant local import. This was blockingpraisonai <yaml>and thereforepraisonai langextract render <yaml>.Invariants preserved
ContextTraceSinkProtocoladapter, no core SDK changes).langextractstill imported only inside_render();ContextTraceEmitterimported lazily inside setup).LangextractSink.context_sink()is additive; existingTraceSinkProtocolpath still active).Verification
Unit tests
Real agentic e2e (before → after)
Before this PR:
After this PR:
Python-level real agent test also green —
Agent.start("Write a haiku about code.")now yieldstrace.html/trace.jsonlcontainingagent_run+final_outputextractions.Scope
This PR addresses the wiring gap only. Nice-to-haves deferred:
ChatCompletionstring content to the actual text body inllm_responseemissions (requires touchingchat_mixin.py, out of scope here).LangfuseSink(same wiring pattern has the same issue; tracked separately).praisonaiagents/tools/langextract_tools.pyas a first-class agent tool.Checklist
langextractnot installedSummary by CodeRabbit
New Features
Chores
Tests