feat: persist task description in tip entity metadata#58
feat: persist task description in tip entity metadata#58jayaramkr merged 5 commits intoAgentToolkit:mainfrom
Conversation
generate_tips() now returns a TipGenerationResult containing both the tips and the source task_description. Both callers (PhoenixSync and MCP save_trajectory) store task_description in tip entity metadata, enabling future clustering of tips by task similarity. Trajectories without a task description default to "Task description unknown".
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughgenerate_tips now returns TipGenerationResult (tips + task_description); consumers (mcp_server, phoenix_sync) and tests updated to use result.tips and persist result.task_description in guideline/tip metadata. Only update guidelines when result.tips is non-empty. Changes
Sequence Diagram(s)(Skipped — changes are plumbing/metadata threading between generator and consumers; no new complex multi-component sequential flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
kaizen/frontend/mcp/mcp_server.py (1)
101-119: Missing guard for empty tips list.Unlike
phoenix_sync.py(line 474), which checksif result.tips:before callingupdate_entities, this code unconditionally callsupdate_entitieseven whenresult.tipsis empty, passing an empty entity list. This is a pre-existing inconsistency, but worth aligning now to avoid a needless round-trip (or potential error if the backend doesn't expect an empty list).Proposed fix
result = generate_tips(messages) - get_client().update_entities( - namespace_id=kaizen_config.namespace_id, - entities=[ - Entity( - type="guideline", - content=tip.content, - metadata={ - "category": tip.category, - "rationale": tip.rationale, - "trigger": tip.trigger, - "task_description": result.task_description, - }, - ) - for tip in result.tips - ], - enable_conflict_resolution=True, - ) + if result.tips: + get_client().update_entities( + namespace_id=kaizen_config.namespace_id, + entities=[ + Entity( + type="guideline", + content=tip.content, + metadata={ + "category": tip.category, + "rationale": tip.rationale, + "trigger": tip.trigger, + "task_description": result.task_description, + }, + ) + for tip in result.tips + ], + enable_conflict_resolution=True, + )tests/unit/test_phoenix_sync.py (1)
537-579: Consider assertingtask_descriptionin entity metadata.The mock correctly uses
TipGenerationResult, but no test verifies thattask_descriptionactually appears in the guideline entity metadata passed toupdate_entities. Since persistingtask_descriptionis the core goal of this PR, a targeted assertion would prevent regressions.For example, in
test_sync_processes_valid_spans:# After result assertions, verify metadata includes task_description tip_update_call = phoenix_sync.client.update_entities.call_args_list[-1] tip_entities = tip_update_call[1]["entities"] # or tip_update_call.kwargs["entities"] assert all("task_description" in e.metadata for e in tip_entities)
Skip update_entities call when no tips are generated, aligning with the existing guard in phoenix_sync.py.
|
Addresses #61 |
|
We're using tips and guidelines somewhat interchangeably. Let's pick one. See #62 No need to block this PR on this, but let's decide soon. |
Merge upstream's error handling (empty response, JSON parse, validation errors with logging) while preserving this branch's TipGenerationResult return type with task_description tracking.
generate_tips() now returns a TipGenerationResult containing both the tips and the source task_description. Both callers (PhoenixSync and MCP save_trajectory) store task_description in tip entity metadata, enabling future clustering of tips by task similarity.
Trajectories without a task description default to "Task description unknown".
Summary by CodeRabbit