feat: Add tip provenance tracking and metadata - #73
Merged
Conversation
- Injects source_task_id and creation_mode to tip entities via save_trajectory and MCP create_entity. - Updates phoenix_sync to standardize source_task_id schema. - Adds comprehensive unit and E2E tests for metadata verification. - Updates main README and LOW_CODE_TRACING docs for tip provenance schema.
📝 WalkthroughWalkthroughAdds tip provenance metadata across code and docs: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server as MCP Server
participant Phoenix_Sync as Phoenix Sync
participant Backend
Client->>MCP_Server: save_trajectory(task_id)
MCP_Server->>MCP_Server: generate_tips()
MCP_Server->>MCP_Server: inject metadata\nsource_task_id=task_id\ncreation_mode="auto-mcp"
MCP_Server->>Backend: update_entities(guidelines)
Phoenix_Sync->>Phoenix_Sync: _process_trajectory(trace_id)
Phoenix_Sync->>Phoenix_Sync: inject metadata\nsource_task_id=trace_id\nsource_span_id=span_id\ncreation_mode="auto-phoenix"
Phoenix_Sync->>Backend: update_entities(tips)
Client->>MCP_Server: create_entity(type="guideline", metadata?)
MCP_Server->>MCP_Server: parse metadata\ninject creation_mode="manual" if missing
MCP_Server->>Backend: update_entities(entity)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/test_mcp.py (1)
1-5:⚠️ Potential issue | 🟠 MajorRuff formatting check is failing CI — run
ruff formatbefore merging.The GitHub Actions pipeline reports this file (among three) would be reformatted. As per coding guidelines, Ruff must be used for formatting (
**/*.py).uv run ruff format tests/e2e/test_mcp.py kaizen/frontend/mcp/mcp_server.py tests/unit/test_mcp_server.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/test_mcp.py` around lines 1 - 5, The file tests/e2e/test_mcp.py fails the Ruff formatting check; run the formatter and commit the changes so CI passes: run `ruff format` (or your editor integration) against tests/e2e/test_mcp.py (and the other reported files) to normalize imports/spacing and reformat per project rules, then stage the updated file(s) and push; ensure the import block (e.g., the top-level imports like os, pytest, json, Client, Path) is ordered and spaced according to Ruff so the GitHub Actions pipeline no longer requests reformatting.
🧹 Nitpick comments (2)
tests/unit/test_mcp_server.py (1)
37-37: Prefer.kwargsover index[1]forcall_argsaccess.
call_args_list[1][1]andcall_args[1]use the deprecated tuple-style access. The.kwargsattribute is more readable and won't silently break if mock internals change.📝 Proposed fix (apply similarly at lines 59, 78, 97)
- call_args = mock_get_client.update_entities.call_args_list[1][1] + call_args = mock_get_client.update_entities.call_args_list[1].kwargs- call_args = mock_get_client.update_entities.call_args[1] + call_args = mock_get_client.update_entities.call_args.kwargs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_mcp_server.py` at line 37, Replace tuple-style access into mock call args with the .kwargs attribute: locate usages of mock_get_client.update_entities.call_args_list[1][1] (and the similar occurrences at the other noted spots) in tests/unit/test_mcp_server.py and change them to use mock_get_client.update_entities.call_args_list[1].kwargs so the test reads the keyword arguments via .kwargs instead of indexing the call_args tuple; update each occurrence referenced (including the ones at the other commented lines) accordingly.docs/LOW_CODE_TRACING.md (1)
219-237:source_span_idandcategoryare undocumented in the bullet list.The sample JSON at lines 219–230 exposes four fields, but only
creation_modeandsource_task_idhave bullet-point explanations.source_span_idandcategoryare left without descriptions, which will confuse readers.📝 Suggested additions
* **`source_task_id`**: The originating trace ID (for Phoenix) or task ID (for MCP), linking the tip back to the specific execution that inspired it. +* **`source_span_id`**: The specific span ID within the trace (Phoenix only) that the tip was derived from. +* **`category`**: A descriptive label for the tip (e.g., `"optimization"`, `"strategy"`) assigned by the tip-generation model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/LOW_CODE_TRACING.md` around lines 219 - 237, Add missing explanations for the JSON fields "source_span_id" and "category" shown in the sample block: describe that source_span_id links to the specific trace/span within Phoenix (a finer-grained identifier than source_task_id) and that category classifies the tip (e.g., "optimization", "bug", "style") to aid filtering, and append these two bullet points to the existing list after the creation_mode and source_task_id bullets so the four fields in the sample JSON are all documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 62: Update the README line describing `source_task_id` by replacing the
incorrect word "audibility" with "auditability" so the sentence reads that it
provides full auditability; locate the text containing the `source_task_id`
description and make that single-word correction.
In `@tests/e2e/test_mcp.py`:
- Around line 123-130: The test creates a new KaizenClient instance
(KaizenClient()) and never closes it, which leaks a Milvus connection; update
the mcp fixture to yield the existing kaizen_client (or otherwise expose it) so
tests reuse the fixture's kaizen_client instead of instantiating a new
KaizenClient, and modify the test to call the fixture-provided kaizen_client for
search_entities (or if you must create a new KaizenClient, ensure you call its
close/disconnect method in a finally block or use a context manager) so no extra
Milvus connection remains open.
In `@tests/unit/test_mcp_server.py`:
- Around line 15-102: Add the pytest unit marker to these tests by either
decorating each test function (test_save_trajectory_metadata_injection,
test_create_entity_metadata_injection_manual_guideline,
test_create_entity_metadata_injection_manual_policy,
test_create_entity_no_metadata_injection_for_other_types) with `@pytest.mark.unit`
(and ensure pytest is imported) or set a module-level marker via pytestmark =
pytest.mark.unit at top of the file; choose one approach and apply consistently
so pytest -m unit will pick up all four tests.
---
Outside diff comments:
In `@tests/e2e/test_mcp.py`:
- Around line 1-5: The file tests/e2e/test_mcp.py fails the Ruff formatting
check; run the formatter and commit the changes so CI passes: run `ruff format`
(or your editor integration) against tests/e2e/test_mcp.py (and the other
reported files) to normalize imports/spacing and reformat per project rules,
then stage the updated file(s) and push; ensure the import block (e.g., the
top-level imports like os, pytest, json, Client, Path) is ordered and spaced
according to Ruff so the GitHub Actions pipeline no longer requests
reformatting.
---
Nitpick comments:
In `@docs/LOW_CODE_TRACING.md`:
- Around line 219-237: Add missing explanations for the JSON fields
"source_span_id" and "category" shown in the sample block: describe that
source_span_id links to the specific trace/span within Phoenix (a finer-grained
identifier than source_task_id) and that category classifies the tip (e.g.,
"optimization", "bug", "style") to aid filtering, and append these two bullet
points to the existing list after the creation_mode and source_task_id bullets
so the four fields in the sample JSON are all documented.
In `@tests/unit/test_mcp_server.py`:
- Line 37: Replace tuple-style access into mock call args with the .kwargs
attribute: locate usages of mock_get_client.update_entities.call_args_list[1][1]
(and the similar occurrences at the other noted spots) in
tests/unit/test_mcp_server.py and change them to use
mock_get_client.update_entities.call_args_list[1].kwargs so the test reads the
keyword arguments via .kwargs instead of indexing the call_args tuple; update
each occurrence referenced (including the ones at the other commented lines)
accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.mddocs/LOW_CODE_TRACING.mdkaizen/frontend/mcp/mcp_server.pykaizen/sync/phoenix_sync.pytests/e2e/test_mcp.pytests/unit/test_mcp_server.pytests/unit/test_phoenix_sync.py
|
|
||
| Kaizen automatically tracks the origin of every guideline it generates or stores. Every tip entity contains `metadata` identifying its source: | ||
| - `creation_mode`: Identifies how the tip was created (`auto-phoenix` via trace observability, `auto-mcp` via trajectory saving tools, or `manual`). | ||
| - `source_task_id`: The ID of the original trace or task that inspired the tip, providing full audibility. |
There was a problem hiding this comment.
Typo: "audibility" → "auditability"
audibility is not the intended word here.
📝 Proposed fix
- `source_task_id`: The ID of the original trace or task that inspired the tip, providing full audibility.
+ `source_task_id`: The ID of the original trace or task that inspired the tip, providing full auditability.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - `source_task_id`: The ID of the original trace or task that inspired the tip, providing full audibility. | |
| - `source_task_id`: The ID of the original trace or task that inspired the tip, providing full auditability. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 62, Update the README line describing `source_task_id` by
replacing the incorrect word "audibility" with "auditability" so the sentence
reads that it provides full auditability; locate the text containing the
`source_task_id` description and make that single-word correction.
Comment on lines
+123
to
+130
| from kaizen.frontend.client.kaizen_client import KaizenClient | ||
| from kaizen.config.kaizen import kaizen_config | ||
| client = KaizenClient() | ||
| entities = client.search_entities( | ||
| namespace_id=kaizen_config.namespace_id, | ||
| filters={"type": "guideline"}, | ||
| limit=10, | ||
| ) |
There was a problem hiding this comment.
New KaizenClient instance is not explicitly closed.
The client created at line 125 is never closed within the test body. For the Milvus backend this adds a third connection to the same Milvus Lite instance (alongside the fixture's kaizen_client and the MCP server's _client), and relies entirely on the fixture teardown's blanket connections.disconnect / server_manager_instance.release_all() calls.
Consider yielding kaizen_client from the mcp fixture so the test can reuse the already-open connection instead of spawning a new one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/test_mcp.py` around lines 123 - 130, The test creates a new
KaizenClient instance (KaizenClient()) and never closes it, which leaks a Milvus
connection; update the mcp fixture to yield the existing kaizen_client (or
otherwise expose it) so tests reuse the fixture's kaizen_client instead of
instantiating a new KaizenClient, and modify the test to call the
fixture-provided kaizen_client for search_entities (or if you must create a new
KaizenClient, ensure you call its close/disconnect method in a finally block or
use a context manager) so no extra Milvus connection remains open.
Comment on lines
+15
to
+102
| def test_save_trajectory_metadata_injection(mock_get_client): | ||
| # Mock tip generation to prevent actual LLM calls | ||
| with patch("kaizen.frontend.mcp.mcp_server.generate_tips") as mock_generate_tips: | ||
| mock_result = MagicMock() | ||
| mock_tip = MagicMock() | ||
| mock_tip.content = "Always write unit tests" | ||
| mock_tip.category = "testing" | ||
| mock_tip.rationale = "Helps catch bugs early" | ||
| mock_tip.trigger = "writing code" | ||
| mock_result.tips = [mock_tip] | ||
| mock_result.task_description = "Add feature" | ||
| mock_generate_tips.return_value = mock_result | ||
|
|
||
| trajectory_data = json.dumps([{"role": "user", "content": "hi"}]) | ||
| task_id = str(uuid.uuid4()) | ||
|
|
||
| save_trajectory.fn(trajectory_data=trajectory_data, task_id=task_id) | ||
|
|
||
| # Ensure update_entities was called twice (once for trajectory, once for tips) | ||
| assert mock_get_client.update_entities.call_count == 2 | ||
|
|
||
| # Second call is for tips | ||
| call_args = mock_get_client.update_entities.call_args_list[1][1] | ||
| entities = call_args["entities"] | ||
|
|
||
| assert len(entities) == 1 | ||
| tip_entity = entities[0] | ||
| assert tip_entity.type == "guideline" | ||
| assert tip_entity.metadata["source_task_id"] == task_id | ||
| assert tip_entity.metadata["creation_mode"] == "auto-mcp" | ||
|
|
||
| def test_create_entity_metadata_injection_manual_guideline(mock_get_client): | ||
| mock_update = EntityUpdate(id="123", type="guideline", content="docstrings", event="ADD", metadata={"creation_mode": "manual"}) | ||
| mock_get_client.update_entities.return_value = [mock_update] | ||
|
|
||
| # Missing explicit metadata, should auto-inject "manual" | ||
| result_str = create_entity.fn( | ||
| content="Write clear docstrings", | ||
| entity_type="guideline" | ||
| ) | ||
| result = json.loads(result_str) | ||
| assert result["event"] == "ADD" | ||
| assert "id" in result | ||
|
|
||
| call_args = mock_get_client.update_entities.call_args[1] | ||
| entities = call_args["entities"] | ||
| assert len(entities) == 1 | ||
| entity = entities[0] | ||
|
|
||
| assert entity.type == "guideline" | ||
| assert entity.metadata["creation_mode"] == "manual" | ||
|
|
||
| def test_create_entity_metadata_injection_manual_policy(mock_get_client): | ||
| mock_update = EntityUpdate(id="123", type="policy", content="PR reviews", event="ADD", metadata={"creation_mode": "manual"}) | ||
| mock_get_client.update_entities.return_value = [mock_update] | ||
|
|
||
| result_str = create_entity.fn( | ||
| content="Require PR reviews", | ||
| entity_type="policy" | ||
| ) | ||
| result = json.loads(result_str) | ||
| assert result["event"] == "ADD" | ||
|
|
||
| call_args = mock_get_client.update_entities.call_args[1] | ||
| entities = call_args["entities"] | ||
| entity = entities[0] | ||
|
|
||
| assert entity.type == "policy" | ||
| assert entity.metadata["creation_mode"] == "manual" | ||
|
|
||
| def test_create_entity_no_metadata_injection_for_other_types(mock_get_client): | ||
| mock_update = EntityUpdate(id="123", type="log", content="App started", event="ADD", metadata={}) | ||
| mock_get_client.update_entities.return_value = [mock_update] | ||
|
|
||
| # A generic log entity shouldn't get creation_mode injected | ||
| result_str = create_entity.fn( | ||
| content="App started", | ||
| entity_type="log" | ||
| ) | ||
| result = json.loads(result_str) | ||
| assert result["event"] == "ADD" | ||
|
|
||
| call_args = mock_get_client.update_entities.call_args[1] | ||
| entities = call_args["entities"] | ||
| entity = entities[0] | ||
|
|
||
| assert entity.type == "log" | ||
| assert "creation_mode" not in (entity.metadata or {}) |
There was a problem hiding this comment.
Missing @pytest.mark.unit on all test functions.
All four tests are pure unit tests (fully mocked, no I/O) but none carry the @pytest.mark.unit marker. This prevents targeted test runs (e.g., pytest -m unit) from including them. As per coding guidelines, unit tests in tests/**/*.py must use the unit pytest marker.
📝 Proposed fix
+@pytest.mark.unit
def test_save_trajectory_metadata_injection(mock_get_client):
+@pytest.mark.unit
def test_create_entity_metadata_injection_manual_guideline(mock_get_client):
+@pytest.mark.unit
def test_create_entity_metadata_injection_manual_policy(mock_get_client):
+@pytest.mark.unit
def test_create_entity_no_metadata_injection_for_other_types(mock_get_client):Alternatively, add a module-level pytestmark:
+pytestmark = pytest.mark.unit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/test_mcp_server.py` around lines 15 - 102, Add the pytest unit
marker to these tests by either decorating each test function
(test_save_trajectory_metadata_injection,
test_create_entity_metadata_injection_manual_guideline,
test_create_entity_metadata_injection_manual_policy,
test_create_entity_no_metadata_injection_for_other_types) with `@pytest.mark.unit`
(and ensure pytest is imported) or set a module-level marker via pytestmark =
pytest.mark.unit at top of the file; choose one approach and apply consistently
so pytest -m unit will pick up all four tests.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/test_mcp_server.py (1)
17-98:⚠️ Potential issue | 🟡 MinorAdd the
unitpytest marker for these tests.These are unit tests but lack the required marker, which breaks
pytest -m unitfiltering. Consider a module-level marker for consistency.✅ Proposed fix (module-level marker)
import json import uuid import pytest from unittest.mock import patch, MagicMock from kaizen.frontend.mcp.mcp_server import save_trajectory, create_entity from kaizen.schema.conflict_resolution import EntityUpdate + +pytestmark = pytest.mark.unitAs per coding guidelines "tests/**/*.py: Use pytest markers
e2efor end-to-end tests andunitfor unit tests in test files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_mcp_server.py` around lines 17 - 98, These tests are missing the pytest "unit" marker; add a module-level marker by importing pytest at the top and setting pytestmark = pytest.mark.unit so all functions (test_save_trajectory_metadata_injection, test_create_entity_metadata_injection_manual_guideline, test_create_entity_metadata_injection_manual_policy, test_create_entity_no_metadata_injection_for_other_types) are marked as unit tests for pytest -m unit filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/test_mcp_server.py`:
- Around line 17-98: These tests are missing the pytest "unit" marker; add a
module-level marker by importing pytest at the top and setting pytestmark =
pytest.mark.unit so all functions (test_save_trajectory_metadata_injection,
test_create_entity_metadata_injection_manual_guideline,
test_create_entity_metadata_injection_manual_policy,
test_create_entity_no_metadata_injection_for_other_types) are marked as unit
tests for pytest -m unit filtering.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
kaizen/frontend/mcp/mcp_server.pytests/e2e/test_mcp.pytests/unit/test_mcp_server.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/test_mcp.py
- kaizen/frontend/mcp/mcp_server.py
vinodmut
approved these changes
Feb 24, 2026
jayaramkr
approved these changes
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Tests