Refactor: Rename 'kata' to 'entity' - #33
Conversation
📝 WalkthroughWalkthroughRenames public backend types from "Kata" → "Entity", updates logger names and default DB identifiers, adjusts client/MCP references, and hardens phoenix_sync message/token extraction with defensive parsing, payload summaries, and wider token-usage fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
actor Span
participant PhoenixSync
participant Parser as _parse_content
participant Normalizer
participant TokenUsage
Span->>PhoenixSync: deliver span (inputs, outputs, gen_ai/llm fields)
PhoenixSync->>Parser: parse string-encoded items
Parser-->>PhoenixSync: parsed dicts or failure
PhoenixSync->>Normalizer: normalize messages, skip non-dicts (log payload summary)
PhoenixSync->>TokenUsage: extract tokens (check gen_ai.usage.*, llm.usage.*, llm.token_count.*)
Normalizer-->>PhoenixSync: normalized messages
PhoenixSync-->>Span: return extracted messages + token usage (or logger.exception on error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@kaizen/schema/conflict_resolution.py`:
- Line 17: The module docstring in kaizen/schema/conflict_resolution.py contains
a grammatical error: change the phrase "Produced by the LLM, to be processed by
a entity backend." to use the correct article, e.g. "Produced by the LLM, to be
processed by an entity backend." Update the module-level docstring accordingly.
In `@kaizen/sync/phoenix_sync.py`:
- Around line 163-179: The fallback for plain string output.value was
accidentally removed so when self._parse_content returns a string output_msgs
stays None; restore the fallback by setting output_msgs to a single-message list
(e.g., [{"role": "assistant", "content": parsed_output or output_val}]) inside
the else branch of the output handling block in phoenix_sync.py (where
output_msgs, attrs.get("output.value") and self._parse_content are used), so
plain text outputs populate assistant context; keep it inside the try/except and
maintain existing logging/exception behavior.
🧹 Nitpick comments (2)
kaizen/frontend/mcp/mcp_server.py (1)
78-80: Consider usingjson.dumps()instead ofstr()for non-string content.When
message["content"]is a list or dict, usingstr()produces Python repr format (e.g.,{'key': 'value'}) rather than valid JSON. This may cause parsing issues if the stored content needs to be deserialized later.♻️ Suggested fix
Entity( type="trajectory", content=message["content"] if isinstance(message["content"], str) - else str(message["content"]), + else json.dumps(message["content"]),kaizen/backend/base.py (1)
42-49: Missing@abstractmethoddecorator onsearch_entities.Unlike all other methods in this ABC,
search_entitieslacks the@abstractmethoddecorator. This means subclasses can silently inherit the base implementation (which returnsNone) rather than being forced to provide their own implementation.♻️ Suggested fix
+ `@abstractmethod` def search_entities( self, namespace_id: str, query: str | None = None, filters: dict | None = None, limit: int = 10 ) -> list[RecordedEntity]: pass
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@kaizen/sync/phoenix_sync.py`:
- Around line 365-368: The total_tokens lookup uses the wrong attribute key;
update the fallback chain to use attrs.get("gen_ai.usage.total_tokens") instead
of attrs.get("llm.usage.total_tokens") so it matches the pattern used for
prompt_tokens and completion_tokens (keep the existing fallback to
attrs.get("llm.token_count.total")). Locate the dict building where attrs is
read (the block creating "prompt_tokens", "completion_tokens", "total_tokens")
and replace the incorrect key to ensure consistency.
🧹 Nitpick comments (3)
kaizen/sync/phoenix_sync.py (3)
135-136: Bareexceptclauses silently swallow all exceptions.These catch-all blocks hide parsing failures and make debugging difficult. Consider catching
Exceptionexplicitly and logging at debug level to aid troubleshooting without cluttering logs.Proposed fix
- except: - pass + except Exception: + logger.debug(f"Failed to parse input.value: {input_val[:100] if input_val else ''}")Apply similar pattern at lines 151-152:
- except: - pass + except Exception: + logger.debug(f"Failed to parse input message: {msg}")Also applies to: 151-152
186-187: Same bareexceptissue in output parsing.Apply the same pattern here for consistency and debuggability.
Proposed fix
except: - pass + except Exception: + logger.debug(f"Failed to parse output.value: {output_val[:100] if output_val else ''}")And at lines 198-199:
- except: - pass + except Exception: + logger.debug(f"Failed to parse output message: {msg}")Also applies to: 198-199
520-523: Uselogger.exceptionto capture traceback.When logging inside an exception handler,
logger.exception()automatically includes the stack trace, which aids debugging.Proposed 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)
8f5fccc to
5d77adf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@kaizen/sync/phoenix_sync.py`:
- Around line 148-152: Replace the bare except/pass around calls to
self._parse_content with an explicit Exception handler that logs the error at
debug level before continuing; specifically, in the blocks where msg is a str
and you call self._parse_content (the occurrences around the existing msg
parsing and the similar block later), change the except to "except Exception as
e" and call the module/class logger (or self.logger) to debug.log the exception
and the offending msg payload so malformed payloads are visible while preserving
behavior.
- Line 522: In the except Exception as e block where logger.error(error_msg) is
called (in phoenix_sync.py), replace the error-only call with
logger.exception(error_msg) so the exception stack trace is captured; locate the
except block around the logger.error call and change the logging invocation to
logger.exception(error_msg) (no need to pass e explicitly) to preserve traceback
for triage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@kaizen/sync/phoenix_sync.py`:
- Around line 135-137: The debug logs currently emit full raw payloads (e.g.,
logger.debug in the except block that logs input_val and similar calls at the
other locations), risking PII leakage; update these to avoid printing raw
content by logging only the payload type and length or a short redacted preview
(e.g., show first N chars + "...") and/or gate full payload logging behind a
secure flag; locate uses of logger.debug that reference input_val or input.value
(and the similar calls around lines 148-153, 186-187, 195-199) and replace them
with a call to a small helper (e.g., redact_preview(payload) or
format_payload_summary(payload)) that returns type/length or a redacted snippet
before logging.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@kaizen/sync/phoenix_sync.py`:
- Around line 381-385: The current usage dict uses the `or` operator with
`attrs.get(...)` which treats valid 0 values as falsy; update the three lookups
for prompt_tokens, completion_tokens, and total_tokens to prefer the first
non-None value instead of truthy-bytes (e.g., implement a small helper like
`_first_non_none` and call it with attrs.get("gen_ai.usage.prompt_tokens"),
attrs.get("llm.token_count.prompt"), attrs.get("llm.usage.prompt_tokens") for
prompt_tokens, similarly for completion and total) so that explicit 0 is
preserved; ensure you replace the `or` chains in the usage construction with
calls to the helper or explicit None checks using `is not None`.
🧹 Nitpick comments (1)
kaizen/sync/phoenix_sync.py (1)
200-204: Remove dead code comment.The commented
#passon line 202 is a leftover from when the fallback was disabled. It serves no purpose now and should be removed.Proposed fix
else: # Fallback for simple string output output_msgs = [{"role": "assistant", "content": output_val}] - `#pass` except Exception as e:
Description
This PR addresses issue #27 by renaming all references of "kata" to "entity" throughout the codebase to better align with the domain model.
Changes
BaseKataBackendto BaseEntityBackend.MilvusKataBackendto MilvusEntityBackend.FilesystemKataBackendto FilesystemEntityBackend.entities.milvus.dbentities.sqlite.dbBreaking Changes
entities.milvus.db/entities.sqlite.dbor migrate data if they wish to preserve existing content.Related Issue
Fixes #27
Please merge after PR#32
Summary by CodeRabbit
Bug Fixes
Refactoring
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.