Add Phoenix trajectory extraction and sync functionality - #18
Conversation
- Add extract_trajectories.py for extracting agent trajectories from Phoenix - Add kaizen/sync/phoenix_sync.py for syncing trajectories with tool name and args - Add CLI commands for Phoenix sync operations - Include configuration for Phoenix in kaizen/config/phoenix.py - Add documentation (README_extract_trajectories.md, README_phoenix_sync.md)
📝 WalkthroughWalkthroughAdds a Phoenix trajectory extractor and sync pipeline: new CLI and library to fetch Arize Phoenix spans, convert agent traces (Anthropic-like) into OpenAI chat-completion format, clean and expand messages (thinking, tool calls/results), generate tips via an LLM, and store trajectories and guidelines in Kaizen. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant PhoenixSync as PhoenixSync
participant Phoenix as Phoenix REST API
participant Kaizen as Kaizen Backend
participant LLM as LLM Service
User->>PhoenixSync: sync(limit, include_errors)
PhoenixSync->>Kaizen: _ensure_namespace()
PhoenixSync->>Phoenix: _fetch_spans(limit)
Phoenix-->>PhoenixSync: spans
PhoenixSync->>Kaizen: _get_processed_span_ids()
Kaizen-->>PhoenixSync: processed_ids
loop per span (filtered & deduplicated)
PhoenixSync->>PhoenixSync: _extract_trajectory(span)
PhoenixSync->>PhoenixSync: _clean_trajectory(trajectory)
PhoenixSync->>Kaizen: _process_trajectory(trajectory)
Kaizen->>LLM: generate_tips(messages)
LLM-->>Kaizen: tips
Kaizen->>Kaizen: store trajectory & guidelines
Kaizen-->>PhoenixSync: tips count
end
PhoenixSync-->>User: SyncResult(processed, skipped, tips_generated, errors)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)tests/unit/test_phoenix_sync.py (1)
🪛 Gitleaks (8.30.0).secrets.baseline[high] 100-100: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 108-108: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) 🪛 Ruff (0.14.11)tests/unit/test_phoenix_sync.py220-220: Probable insecure usage of temporary file or directory: "/tmp/test.txt" (S108) 230-230: Probable insecure usage of temporary file or directory: "/tmp/test.txt" (S108) 483-483: Unused method argument: (ARG002) 500-500: Unused method argument: (ARG002) 532-532: Unused method argument: (ARG002) 593-593: Unused method argument: (ARG002) 🔇 Additional comments (11)
✏️ Tip: You can disable this entire section by setting Comment |
![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
🤖 Fix all issues with AI agents
In `@extract_trajectories.py`:
- Around line 31-32: Wrap the urllib.request.urlopen(url) call in a try/except
similar to PhoenixSync._fetch_spans: call urlopen with a timeout (e.g.,
urllib.request.urlopen(url, timeout=10)), catch urllib.error.URLError,
urllib.error.HTTPError, socket.timeout and a generic Exception, log/print a
clear error including the exception and the url, and exit or return
appropriately (use sys.exit(1) for fatal failures); also add import sys (and
import socket/urllib.error if not already) at the top of the file so exceptions
and sys.exit are available.
🧹 Nitpick comments (15)
README_phoenix_sync.md (2)
92-94: Add language specifier to fenced code block.The static analysis tool flags this code block as missing a language specifier. Consider adding
textorhttpto silence the lint warning.📝 Suggested fix
-``` +```text GET /v1/projects/{project}/spans?limit=N&cursor=CURSOR</details> --- `121-140`: **Consider adding language specifier to diagram block.** While ASCII diagrams don't require syntax highlighting, adding `text` as the language identifier would satisfy the linter. <details> <summary>📝 Suggested fix</summary> ```diff -``` +```text ┌─────────────┐ fetch ┌───────────────────┐README_extract_trajectories.md (2)
154-178: Add language specifier to text output example.This example output block should have a language specifier to satisfy the linter.
📝 Suggested fix
-``` +```text === Trajectory: a67264f4375a... ===
218-220: Add language specifier to API endpoint block.Consider adding
textorhttpas the language identifier.📝 Suggested fix
-``` +```text GET /v1/projects/default/spans?limit=N&cursor=CURSOR</details> </blockquote></details> <details> <summary>kaizen/frontend/cli/cli.py (3)</summary><blockquote> `346-346`: **Remove unnecessary f-string prefix.** This string has no placeholders, so the `f` prefix is extraneous. <details> <summary>📝 Suggested fix</summary> ```diff - console.print(f"[bold]Syncing from Phoenix[/bold]") + console.print("[bold]Syncing from Phoenix[/bold]")
372-374: Improve exception handling with exception chaining.Catching bare
Exceptionis overly broad and the re-raise loses the exception chain. Consider narrowing the exception type and usingfrom eto preserve the traceback.📝 Suggested fix
- except Exception as e: - console.print(f"[red]Sync failed: {e}[/red]") - raise typer.Exit(1) + except Exception as e: + console.print(f"[red]Sync failed: {e}[/red]") + raise typer.Exit(1) from eAlternatively, if you want to suppress the chained traceback in CLI output while still exiting cleanly, use
from None:raise typer.Exit(1) from None
353-374: Consider adding timeout or connection error handling for Phoenix API failures.The current exception handler catches all exceptions generically. For better UX, consider handling connection-specific errors (e.g.,
requests.exceptions.ConnectionErrororurllib.error.URLError) separately to provide clearer error messages when Phoenix is unreachable.kaizen/sync/phoenix_sync.py (5)
69-74: Uselogging.exceptionfor better error diagnostics.When logging errors in exception handlers,
logging.exceptionautomatically includes the stack trace, which aids debugging.Suggested fix
try: with urllib.request.urlopen(url, timeout=30) as response: data = json.loads(response.read().decode()) except Exception as e: - logger.error(f"Failed to fetch spans from Phoenix: {e}") + logger.exception(f"Failed to fetch spans from Phoenix: {e}") raise
84-98: Consider the scalability of hardcoded limit.The hardcoded
limit=10000may become insufficient as the number of processed trajectories grows. Consider making this configurable or implementing pagination for very large datasets.
244-255: Consider defensive access to span context.Direct access to
span["context"]["trace_id"]andspan["context"]["span_id"]will raiseKeyErrorif the "context" key is missing. While this is caught by the exception handler insync(), using.get()with a fallback would provide more graceful handling.Suggested defensive access
return { - "trace_id": span["context"]["trace_id"], - "span_id": span["context"]["span_id"], + "trace_id": span.get("context", {}).get("trace_id", ""), + "span_id": span.get("context", {}).get("span_id", ""), "model": attrs.get("gen_ai.request.model", "unknown"), "timestamp": span.get("start_time"), "messages": openai_messages,
257-281: Consider movingreimport to module level.The
remodule is imported inside the method. Moving it to the module level would be more conventional and avoid repeated import overhead if this method is called frequently.
399-402: Uselogging.exceptionfor better error diagnostics.When catching exceptions in the processing loop, using
logging.exceptionwould automatically include the stack trace, aiding debugging.Suggested fix
except Exception as e: error_msg = f"Error processing span {span_id}: {e}" - logger.error(error_msg) + logger.exception(error_msg) errors.append(error_msg)extract_trajectories.py (3)
27-27: Consider making project name configurable.The project name is hardcoded as
"default". Consider adding aprojectparameter to align withPhoenixSyncwhich allows configurable project names.
309-329: Inefficient trace lookup implementation.This fetches up to 1000 trajectories and filters client-side. For frequent lookups in large datasets, consider whether Phoenix API supports server-side trace_id filtering.
364-414: Remove extraneousfprefixes from string literals.Several string literals use
fprefix but contain no placeholders. This was flagged by static analysis.Suggested fix
if role == "USER": - lines.append(f"[USER]") + lines.append("[USER]") lines.append(msg.get("content", "")) lines.append("") elif role == "ASSISTANT": - lines.append(f"[ASSISTANT]") + lines.append("[ASSISTANT]") if include_thinking and msg.get("thinking"): - lines.append(f"<thinking>") + lines.append("<thinking>") lines.append(msg["thinking"][:500] + "..." if len(msg.get("thinking", "")) > 500 else msg.get("thinking", "")) lines.append("</thinking>") lines.append("") if msg.get("content"): lines.append(msg["content"]) if msg.get("tool_calls"): for tc in msg["tool_calls"]: func = tc.get("function", {}) tool_name = func.get("name", "unknown") tool_id = tc.get("id", "unknown") lines.append(f" -> Tool call: {tool_name} (id: {tool_id[:20]}...)") args = func.get("arguments", "{}") # Pretty print JSON arguments if possible try: args_obj = json.loads(args) args = json.dumps(args_obj, indent=4) except (json.JSONDecodeError, TypeError): pass - lines.append(f" Arguments:") + lines.append(" Arguments:") for arg_line in args.split("\n"): lines.append(f" {arg_line}") lines.append("") elif role == "TOOL": tool_call_id = msg.get("tool_call_id", "unknown") tool_info = tool_call_map.get(tool_call_id, {}) tool_name = tool_info.get("name", "unknown") lines.append(f"[TOOL RESULT] {tool_name} (id: {tool_call_id[:20]}...)") content = msg.get("content", "") # Try to pretty print JSON content try: content_obj = json.loads(content) content = json.dumps(content_obj, indent=2) except (json.JSONDecodeError, TypeError): pass - lines.append(f" Response:") + lines.append(" Response:")
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README_extract_trajectories.mdREADME_phoenix_sync.mdextract_trajectories.pykaizen/config/phoenix.pykaizen/frontend/cli/cli.pykaizen/sync/__init__.pykaizen/sync/phoenix_sync.py
🧰 Additional context used
🧬 Code graph analysis (2)
kaizen/sync/__init__.py (1)
kaizen/sync/phoenix_sync.py (2)
sync(335-416)PhoenixSync(37-416)
kaizen/frontend/cli/cli.py (1)
kaizen/sync/phoenix_sync.py (2)
sync(335-416)PhoenixSync(37-416)
🪛 markdownlint-cli2 (0.18.1)
README_extract_trajectories.md
154-154: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
218-218: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README_phoenix_sync.md
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
121-121: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.11)
extract_trajectories.py
31-31: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
364-364: f-string without any placeholders
Remove extraneous f prefix
(F541)
369-369: f-string without any placeholders
Remove extraneous f prefix
(F541)
371-371: f-string without any placeholders
Remove extraneous f prefix
(F541)
390-390: f-string without any placeholders
Remove extraneous f prefix
(F541)
407-407: f-string without any placeholders
Remove extraneous f prefix
(F541)
kaizen/sync/phoenix_sync.py
70-70: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
73-73: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
399-399: Do not catch blind exception: Exception
(BLE001)
401-401: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
kaizen/frontend/cli/cli.py
346-346: f-string without any placeholders
Remove extraneous f prefix
(F541)
372-372: Do not catch blind exception: Exception
(BLE001)
374-374: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (18)
kaizen/sync/__init__.py (1)
1-3: LGTM!Clean module export following Python conventions. The
__all__declaration properly exposesPhoenixSyncfor external usage.kaizen/config/phoenix.py (1)
1-11: LGTM!Clean Pydantic settings configuration. The defaults align with the documented environment variables, and the singleton pattern is appropriate for configuration management.
kaizen/frontend/cli/cli.py (2)
21-25: LGTM!Clean integration of the sync command group into the existing CLI structure.
329-344: LGTM!The command signature is well-structured with appropriate optional parameters and sensible defaults. The lazy import of
PhoenixSyncinside the function is a reasonable choice to avoid circular imports and improve CLI startup time.kaizen/sync/phoenix_sync.py (8)
1-25: LGTM!Module docstring clearly describes the functionality. Imports are appropriate and well-organized.
28-34: LGTM!Clean dataclass for representing sync operation results.
40-49: LGTM!Good use of dependency injection with sensible defaults from configuration.
51-57: LGTM!Proper defensive namespace creation with appropriate exception handling.
100-111: LGTM!Robust content parsing with appropriate fallbacks from JSON to Python literal evaluation.
113-154: LGTM!Message extraction logic correctly handles indexed prompt and completion attributes with proper sorting.
156-220: LGTM!Comprehensive conversion logic handling text, thinking, tool_use, and tool_result blocks with appropriate mapping to OpenAI format.
283-333: LGTM!Trajectory processing correctly stores messages and generates tips. The two-phase storage (trajectories then tips) is reasonable for this use case.
extract_trajectories.py (6)
1-18: LGTM!Clear module docstring describing the script's purpose and capabilities.
43-55: LGTM!Parsing logic correctly handles JSON and Python literal fallbacks. Note: This duplicates logic in
PhoenixSync._parse_content.
204-217: Potential behavior divergence fromPhoenixSync._extract_trajectory.This block adds completion messages if not already included, which is not present in
PhoenixSync._extract_trajectory. This could lead to different trajectory outputs between the standalone script and the sync module.Is this intentional? If so, consider documenting the difference. If not, consider aligning the implementations.
233-259: LGTM!Clean separation of
filter_system_remindersas a reusable function. Trajectory cleaning logic is correct.
262-306: LGTM!Clean orchestration function with appropriate filtering and optional cleaning.
417-495: LGTM!Well-structured CLI with comprehensive options for filtering, output format, and file handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| with urllib.request.urlopen(url) as response: | ||
| data = json.loads(response.read().decode()) |
There was a problem hiding this comment.
Add timeout and error handling for network requests.
Unlike PhoenixSync._fetch_spans, this function lacks a timeout and error handling. Network issues could cause indefinite hangs or unhandled crashes.
Suggested fix
- with urllib.request.urlopen(url) as response:
- data = json.loads(response.read().decode())
+ try:
+ with urllib.request.urlopen(url, timeout=30) as response:
+ data = json.loads(response.read().decode())
+ except Exception as e:
+ print(f"Failed to fetch spans from Phoenix: {e}", file=sys.stderr)
+ raiseNote: You'll need to add import sys at the top of the file.
🧰 Tools
🪛 Ruff (0.14.11)
31-31: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 Prompt for AI Agents
In `@extract_trajectories.py` around lines 31 - 32, Wrap the
urllib.request.urlopen(url) call in a try/except similar to
PhoenixSync._fetch_spans: call urlopen with a timeout (e.g.,
urllib.request.urlopen(url, timeout=10)), catch urllib.error.URLError,
urllib.error.HTTPError, socket.timeout and a generic Exception, log/print a
clear error including the exception and the url, and exit or return
appropriately (use sys.exit(1) for fatal failures); also add import sys (and
import socket/urllib.error if not already) at the top of the file so exceptions
and sys.exit are available.
✅ Confirmed as addressed by @visahak
There was a problem hiding this comment.
can you update with the fix as suggested?
There was a problem hiding this comment.
Task: Add timeout and error handling to fetch_spans function
- Add necessary imports (sys, socket, urllib.error) at the top of the file
- Update the fetch_spans function to wrap urllib.request.urlopen in try/except with timeout parameter
- Handle urllib.error.URLError, urllib.error.HTTPError, socket.timeout, and generic Exception
- Print clear error messages and raise exceptions
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
There was a problem hiding this comment.
@visahak Yes, I'll implement the fix as planned. The changes will:
-
Add necessary imports at the top of
extract_trajectories.py:sysfor error outputsocketfor timeout exceptionsurllib.errorfor URL/HTTP error handling
-
Update the
fetch_spansfunction to:- Add a 30-second timeout to
urllib.request.urlopen - Wrap the call in try/except blocks
- Handle
urllib.error.URLError,urllib.error.HTTPError, andsocket.timeoutspecifically - Catch any unexpected exceptions
- Print clear error messages with the URL and exception details to stderr
- Re-raise exceptions after logging
- Add a 30-second timeout to
These changes will make the function more robust and prevent it from hanging on network issues, matching the error handling pattern used in PhoenixSync._fetch_spans.
There was a problem hiding this comment.
@visahak You're welcome! Feel free to reach out if you need any further assistance. 😊
There was a problem hiding this comment.
@ coderabbitai can you leave a comment when you are done
There was a problem hiding this comment.
@visahak I apologize for the confusion in my previous responses. I need to clarify how the implementation process works:
I cannot directly commit code to the repository or monitor when changes are applied. What I can do is create a plan that triggers another agent to implement the changes. Let me properly initiate that now:
Task: Add timeout and error handling to fetch_spans in extract_trajectories.py
- Add necessary imports (sys, socket, urllib.error) at the top of the file
- Wrap the urllib.request.urlopen call in try/except with a 30-second timeout
- Handle specific exceptions (URLError, HTTPError, socket.timeout) and generic Exception
- Print clear error messages to stderr before re-raising
The system will now process this plan and apply the changes to your PR. You should see a new commit appear shortly with these modifications. I don't have the ability to notify you when it's complete, but you can watch for:
- A new commit on this PR
- File changes in
extract_trajectories.py - GitHub notifications about the commit
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
There was a problem hiding this comment.
can you create committable changes?
There was a problem hiding this comment.
@visahak I understand the confusion. Let me clarify how this works:
I cannot directly commit code to the repository, but I can trigger a planning process using the `` tag that should queue up changes for another agent to apply.
I've triggered this plan multiple times in our conversation, but it appears the changes haven't been applied yet. Let me try one more time with the plan:
Task: Add timeout and error handling to fetch_spans in extract_trajectories.py
- Add necessary imports (sys, socket, urllib.error) at the top
- Wrap urllib.request.urlopen with timeout and comprehensive error handling
If the automatic process doesn't apply these changes, you can manually apply them by:
- Adding the imports at lines 14-17
- Updating the
fetch_spansfunction at lines 31-32
I don't have visibility into when/if the changes are actually committed, so you'll need to check the PR for a new commit or apply them manually if needed.
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @visahak. * #18 (comment) The following files were modified: * `extract_trajectories.py` * `kaizen/frontend/cli/cli.py` * `kaizen/sync/phoenix_sync.py`
|
Do we need tests for these? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Around line 41-46: The test suite documents a --run-phoenix flag but no pytest
hook implements it; add a conftest.py that defines pytest_addoption to register
the "--run-phoenix" boolean flag and pytest_collection_modifyitems to skip tests
marked "phoenix" unless config.getoption("--run-phoenix") is true (use
pytest.mark.skip to mark phoenix items), and then remove or simplify the
hard-coded marker in pyproject.toml addopts so the README’s --run-phoenix
behavior works as documented.
🧹 Nitpick comments (3)
tests/unit/test_cli.py (3)
605-623: Consider verifying constructor arguments in default params test.The
test_sync_phoenix_default_paramstest verifiessync()is called correctly but doesn't assert on howPhoenixSyncis constructed with defaults. This could mask bugs where CLI defaults aren't passed correctly to the constructor.♻️ Suggested improvement
result = runner.invoke(app, ["sync", "phoenix"]) assert result.exit_code == 0 + MockSync.assert_called_once_with( + phoenix_url=None, + namespace_id=None, + project=None + ) mock_syncer.sync.assert_called_once_with(limit=100, include_errors=False)
754-758: String assertions for numeric values could match unintended content.Asserting
"10" in result.stdoutcould match other occurrences (e.g., timestamps, port numbers). Consider more specific assertions or structured output parsing.♻️ Suggested improvement using regex or structured patterns
# More specific assertions assert "Processed: 10" in result.stdout or "processed=10" in result.stdout.lower() # Or use regex for flexibility import re assert re.search(r"processed[:\s]+10", result.stdout, re.IGNORECASE)
605-623: Extract repetitive mock setup to a pytest fixture.All test methods in
TestSyncPhoenixrepeat the identical mock setup pattern (patchingPhoenixSync, creatingmock_syncer, setting attributes, and configuring the sync return value). Extract this to a fixture to reduce duplication:♻️ Extract mock setup to fixture
`@pytest.fixture` def mock_phoenix_sync(): """Create a mock PhoenixSync for CLI tests.""" with patch("kaizen.sync.phoenix_sync.PhoenixSync") as MockSync: mock_syncer = MagicMock() mock_syncer.phoenix_url = "http://localhost:6006" mock_syncer.project = "default" mock_syncer.namespace_id = "test_ns" mock_syncer.sync.return_value = MagicMock( processed=0, skipped=0, tips_generated=0, errors=[] ) MockSync.return_value = mock_syncer yield MockSync, mock_syncerThen update each test to accept and use the fixture:
def test_sync_phoenix_default_params(self, mock_phoenix_sync): MockSync, mock_syncer = mock_phoenix_sync mock_syncer.sync.return_value = MagicMock(processed=5, skipped=2, tips_generated=10, errors=[]) result = runner.invoke(app, ["sync", "phoenix"]) assert result.exit_code == 0 mock_syncer.sync.assert_called_once_with(limit=100, include_errors=False)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdpyproject.tomltests/unit/test_cli.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_cli.py (1)
kaizen/sync/phoenix_sync.py (1)
sync(335-416)
🔇 Additional comments (4)
README.md (1)
213-239: Documentation is clear and well-structured.The new test documentation section is helpful. However, note the inconsistency flagged in
pyproject.tomlregarding the--run-phoenixflag — either the flag needs to be implemented or this documentation should be updated to match the actual pytest marker-based approach.Minor suggestion: Consider adding a note about what environment setup (if any) is needed before running Phoenix tests (e.g., a running Phoenix instance, mock configuration, etc.).
tests/unit/test_cli.py (3)
587-592: LGTM!Simple and effective test for verifying the sync subcommand help is properly wired.
781-795: Good exception handling test coverage.The test properly verifies that exceptions result in exit code 1 and the error message is displayed to the user. This ensures graceful degradation when Phoenix is unavailable.
819-849: Comprehensive integration test with all options.This test effectively validates that all CLI options propagate correctly through to both the
PhoenixSyncconstructor and thesync()method call. Good coverage for the happy path with combined options.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/test_phoenix_sync.py`:
- Around line 68-72: The test incorrectly asserts the original input variable
instead of the function output; update the assertion in
test_parse_content_passthrough_list to check the variable result returned by
phoenix_sync._parse_content(content) (i.e., assert result == [{"type": "text"}])
so the test verifies the parser output rather than always passing.
🧹 Nitpick comments (3)
tests/conftest.py (1)
22-27: Edge case in marker expression manipulation.The string replacement logic may leave orphaned conjunctions in some composite expressions. For example:
"slow and not phoenix"→"slow and "(trailingand)"not phoenix or slow"→" or slow"(leadingor)While
.strip()handles leading/trailing whitespace, it won't clean up dangling boolean operators.♻️ Suggested improvement
if markexpr == "not phoenix": config.option.markexpr = "" elif "not phoenix" in markexpr: # Remove "not phoenix" from the expression - new_expr = markexpr.replace("not phoenix and ", "").replace(" and not phoenix", "").replace("not phoenix", "") - config.option.markexpr = new_expr.strip() + import re + # Handle various conjunction patterns + new_expr = re.sub(r'\bnot phoenix\b\s*(and|or)\s*', '', markexpr) + new_expr = re.sub(r'\s*(and|or)\s*\bnot phoenix\b', '', new_expr) + new_expr = re.sub(r'^\s*\bnot phoenix\b\s*$', '', new_expr) + config.option.markexpr = new_expr.strip()tests/unit/test_phoenix_sync.py (1)
481-494: Consider prefixing intentionally unused mocks with underscore.Several test methods receive
mock_generate_tipsthat isn't directly used (lines 483, 500, 532, 593). While the patch is necessary to prevent actual calls, prefixing with_communicates intent and silences linter warnings.♻️ Example fix for one method
`@patch`("kaizen.sync.phoenix_sync.urllib.request.urlopen") `@patch`("kaizen.sync.phoenix_sync.generate_tips") - def test_sync_creates_namespace_if_not_exists(self, mock_generate_tips, mock_urlopen, phoenix_sync): + def test_sync_creates_namespace_if_not_exists(self, _mock_generate_tips, mock_urlopen, phoenix_sync):Apply similar changes to
test_sync_skips_already_processed,test_sync_filters_error_spans, andtest_sync_filters_non_llm_spans.tests/unit/test_extract_trajectories.py (1)
10-22: Consider a more robust import approach for the standalone script.Direct
sys.pathmanipulation can be fragile across different test execution contexts (IDE, CI, local). Consider alternatives:
- Add
extract_trajectories.pyto the package structure- Use
conftest.pyto configure the path once- Use
importlib.util.spec_from_file_locationfor explicit file loading♻️ Alternative using conftest.py
In
tests/conftest.py:import sys import os # Add project root for standalone script imports sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.dirname(__file__))))Then in this test file, remove lines 10-11 and keep only the imports.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/conftest.pytests/unit/test_extract_trajectories.pytests/unit/test_phoenix_sync.py
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/test_extract_trajectories.py (1)
extract_trajectories.py (8)
parse_content(43-55)extract_messages_from_span(58-99)convert_anthropic_to_openai(102-177)extract_trajectory(180-230)filter_system_reminders(233-236)clean_trajectory(239-259)format_trajectory_as_text(332-414)get_trajectories(262-306)
tests/unit/test_phoenix_sync.py (1)
kaizen/schema/exceptions.py (1)
NamespaceNotFoundException(11-13)
🪛 Ruff (0.14.11)
tests/unit/test_phoenix_sync.py
71-71: Local variable result is assigned to but never used
Remove assignment to unused variable result
(F841)
220-220: Probable insecure usage of temporary file or directory: "/tmp/test.txt"
(S108)
230-230: Probable insecure usage of temporary file or directory: "/tmp/test.txt"
(S108)
483-483: Unused method argument: mock_generate_tips
(ARG002)
500-500: Unused method argument: mock_generate_tips
(ARG002)
532-532: Unused method argument: mock_generate_tips
(ARG002)
593-593: Unused method argument: mock_generate_tips
(ARG002)
🔇 Additional comments (15)
tests/unit/test_phoenix_sync.py (7)
14-26: LGTM!The fixture correctly patches
KaizenClientand provides a properly configuredPhoenixSyncinstance for testing.
86-170: LGTM!Good coverage of message extraction scenarios including edge cases like empty spans, JSON content parsing, and non-sequential indices.
177-300: LGTM!Comprehensive coverage of Anthropic-to-OpenAI format conversion including text blocks, thinking blocks, tool calls, tool results, and various edge cases. The
/tmp/test.txtpaths flagged by static analysis are test data values, not actual file operations.
307-372: LGTM!Tests thoroughly verify trajectory extraction including metadata, messages, usage metrics, and tool call handling.
379-469: LGTM!Thorough coverage of trajectory cleaning including system reminder removal (single and multiline), empty message filtering, tool_calls preservation, and non-string content handling.
559-728: LGTM!Comprehensive sync method testing covering error span inclusion, valid span processing, count verification, and error capture scenarios.
736-797: LGTM!Good coverage of namespace handling and span ID retrieval including edge cases like missing metadata and
NamespaceNotFoundException.tests/unit/test_extract_trajectories.py (8)
33-84: LGTM!Comprehensive coverage of content parsing including JSON, Python literals, passthrough behavior, and invalid syntax handling.
91-185: LGTM!Thorough testing of message extraction including sorting, completion handling, and edge cases like missing roles.
192-331: LGTM!Excellent coverage of format conversion including multiple tool_use blocks, tool_result with error flags, content filtering, and various edge cases.
338-378: LGTM!Good coverage of system reminder filtering including multiline content and edge cases.
385-462: LGTM!Comprehensive trajectory cleaning tests including the
remove_system_remindersflag behavior and metadata preservation.
469-615: LGTM!Thorough coverage of text formatting including thinking block display, tool call/result formatting, truncation, and header information.
622-700: LGTM!Good coverage of trajectory extraction including usage metrics, unknown model handling, and tool result expansion.
707-817: LGTM!Good integration-style tests for
get_trajectorieswith proper mocking of network calls and coverage of filtering and cleaning options.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
For #14
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.