feat: add Hermes Agent rollout support#500
Conversation
- add the hermes_agent rollout format with ~/.hermes/sessions defaults - parse Hermes CLI session logs and gateway transcripts into normalized records - add config, reader, interface, and docs coverage for the new format Closes #496
Greptile SummaryThis PR adds
|
| Filename | Overview |
|---|---|
| packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/hermes_agent.py | New Hermes handler; is_handled_file correctly dispatches between CLI JSON and JSONL, but should_warn_unhandled_file has an unreachable True-returning branch |
| packages/data-designer-config/src/data_designer/config/seed_source.py | Adds HERMES_AGENT enum value, default path ~/.hermes/sessions, and .json glob pattern |
| packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/base.py | Adds should_warn_unhandled_file hook with default True to base handler |
| packages/data-designer-engine/src/data_designer/engine/resources/seed_reader.py | Uses should_warn_unhandled_file to selectively suppress unhandled-file warnings |
| packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/utils.py | Adds load_json_object() helper for parsing whole-document JSON files |
| packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/registry.py | Registers HermesAgentRolloutFormatHandler in built-in handler map |
| packages/data-designer-engine/tests/engine/resources/agent_rollout/test_hermes_agent.py | Unit tests for Hermes CLI session log and gateway transcript parsing |
| packages/data-designer-engine/tests/engine/resources/test_seed_reader.py | Integration tests for Hermes seed reader with mixed JSON and JSONL artifacts |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Matched file via *.json* glob] --> B{is_handled_file?}
B -->|sessions.json| C[Skip silently]
B -->|session_*.json| D[parse_hermes_cli_session_log]
B -->|*.jsonl| E[parse_hermes_gateway_transcript]
B -->|other .json files| F{should_warn_unhandled_file?}
F -->|suffix == .json| G[Skip silently]
F -->|other suffix| H[Log warning and skip]
D --> I[NormalizedAgentRolloutRecord]
E --> I
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/hermes_agent.py
Line: 71-84
Comment:
**Unreachable `True`-returning branch in `should_warn_unhandled_file`**
`is_handled_file` already returns `True` for every `.json` file whose name starts with `session_`, so `should_warn_unhandled_file` is only ever called for that path with non-`session_`-prefixed `.json` files (where `startswith("session_")` is `False`). The `True` branch is therefore unreachable, and the method is equivalent to `return path.suffix != ".json"`.
```suggestion
def should_warn_unhandled_file(self, relative_path: str) -> bool:
path = Path(relative_path)
return path.suffix != ".json"
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "polish Hermes warning hooks" | Re-trigger Greptile
|
Validated this branch against the local Hermes session corpus with the recipe itself, using the workspace code from commit uv run --group dev python docs/assets/recipes/trace_ingestion/agent_rollout_distillation.py \
--format hermes_agent \
--trace-dir ~/.hermes/sessions \
--num-records 3 \
--previewResult:
One note: running the script as plain |
nabinchha
left a comment
There was a problem hiding this comment.
Thanks for putting this together, @eric-tramel — clean addition that follows the established handler patterns well.
Summary
This PR adds built-in hermes_agent support to AgentRolloutSeedSource, enabling DataDesigner to ingest Hermes CLI session logs (session_*.json) and gateway transcripts (*.jsonl) without a custom reader. The implementation includes a new HermesAgentRolloutFormatHandler, a shared load_json_object utility, a should_warn_unhandled_file hook on the base handler to suppress noise from non-session Hermes JSON files, and comprehensive test and documentation coverage. The implementation matches the stated intent in the PR description.
Findings
Warnings — Worth addressing
packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/hermes_agent.py:82 — should_warn_unhandled_file returns True for session_*.json files
- What: The double-negation
not (path.suffix == ".json" and not path.name.startswith("session_"))evaluates toTrueforsession_*.jsonfiles. This means if asession_*.jsonfile somehow failsis_handled_file(which currently can't happen), it would emit a warning. More importantly, the logic is hard to reason about at a glance. - Why: Double negations are a readability hazard. A future maintainer adding a new
is_handled_filecondition could inadvertently create a case where a session JSON file is unhandled and triggers a noisy warning. Expressing the intent directly makes the contract clearer. - Suggestion: Consider rewriting for clarity:
def should_warn_unhandled_file(self, relative_path: str) -> bool:
path = Path(relative_path)
if path.suffix != ".json":
return True
return path.name.startswith("session_")This makes the intent explicit: suppress warnings for non-session .json files (request dumps, sessions.json, etc.), but warn for everything else including unexpected session files.
packages/data-designer/tests/interface/test_data_designer.py + packages/data-designer-engine/tests/engine/resources/test_seed_reader.py — Duplicated _write_hermes_trace_directory helper
- What: The
_write_hermes_trace_directoryhelper is copy-pasted across two test files (~95 lines each). The seed reader version takeswrite_jsonandwrite_jsonlas arguments while the interface version calls module-level_write_json/_write_jsonlhelpers directly, but the fixture data is identical. - Why: The existing ATIF and Claude Code helpers have the same duplication pattern, so this is a pre-existing convention — but this PR is a good opportunity to note it. If a Hermes schema change happens later, both copies need updating in lockstep.
- Suggestion: This is consistent with the existing pattern for other formats, so not blocking. If the team wants to consolidate in a follow-up, a shared
conftest.pyfixture or a test-utilities module undertests/engine/resources/could hold the canonical trace builders. Take it or leave it.
Suggestions — Take it or leave it
packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/base.py:29 — del relative_path could use a clarifying comment
- What: The base
should_warn_unhandled_fileimplementation usesdel relative_pathto discard the unused parameter, which is a valid Python idiom but not immediately obvious to all readers. - Why: This is a new method on the base handler class introduced by this PR. A brief comment explaining that the default always warns and subclasses can override to suppress specific files would make the intent clear without needing to read the Hermes override first.
- Suggestion:
def should_warn_unhandled_file(self, relative_path: str) -> bool:
"""Return whether unhandled files for this format should emit warnings."""
del relative_path # Default: warn for all unhandled files; subclasses may override.
return Truepackages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/hermes_agent.py:227 — Empty string system prompt is silently skipped
- What:
if system_prompt:is falsy for bothNoneand"". An empty-string system prompt from the Hermes payload would be silently dropped rather than materialized as an empty system message. - Why: This is likely the desired behavior (an empty system prompt is meaningless), but it's worth confirming since
coerce_optional_strcan return""for non-None non-string values coerced to empty strings. The existing Claude Code and Codex handlers don't have system prompt handling, so there's no precedent to follow. - Suggestion: If intentional, no change needed. If you want to be explicit about the contract,
if system_prompt is not None and system_prompt:or just a brief inline comment would clarify the intent.
packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/hermes_agent.py:176 — load_jsonl_rows materializes all rows into memory
- What:
rows = list(load_jsonl_rows(file_path))eagerly materializes the entire JSONL file. For very large gateway transcripts this could be memory-intensive. - Why: The existing Codex handler does the same thing (
rows = list(load_jsonl_rows(file_path))), so this is consistent. Just noting it as a potential scalability consideration for very large Hermes transcripts. - Suggestion: No change needed for this PR — just flagging for awareness. If Hermes transcripts grow large, a streaming approach could be considered in a future optimization pass.
packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/hermes_agent.py:310 — or fallback chain in require_string could mask one valid value with another
- What:
require_string(raw_tool_call.get("id") or raw_tool_call.get("call_id"), ...)uses Python'soroperator, which means if"id"is present but is an empty string"", it will fall through to"call_id". Similarly forfunction_payload.get("name") or raw_tool_call.get("name"). - Why: In practice, Hermes tool calls always have non-empty IDs, so this is unlikely to cause issues. The
orpattern is a pragmatic way to handle the two Hermes payload shapes (CLI vs gateway). Just noting the edge case. - Suggestion: No change needed unless you want to be defensive about it.
What Looks Good
- Follows established patterns faithfully. The handler structure, parse context dataclass, registry registration, and test organization all mirror the existing Claude Code and Codex handlers. This makes the codebase easy to navigate.
- Thoughtful
should_warn_unhandled_filehook. Adding this to the base handler class is a clean extension point that solves a real problem (Hermes directories contain non-session JSON files that would otherwise spam warnings). The*.json*glob pattern combined with handler-side filtering is a pragmatic approach. - Comprehensive test coverage. Both the unit tests (
test_hermes_agent.py) and integration tests (seed reader + interface) cover the happy path, error cases, file filtering, session index loading, and the warning suppression behavior. Thecaplogassertion for the no-warning case is a nice touch.
Verdict
Ship it (with nits) — The should_warn_unhandled_file readability issue is the only thing worth a second look. Everything else is solid and follows the project conventions well.
This review was generated by an AI assistant.
|
@nabinchha addressed the warning-hook readability nits from your review. Changes now on the branch:
Validated after the cleanup with:
Latest commit for that cleanup: |
📋 Summary
Add built-in
hermes_agentsupport toAgentRolloutSeedSourceso DataDesigner can ingest Hermes CLI session logs and gateway transcripts without a custom reader. The implementation was validated with synthetic fixtures and with live Hermes session files under~/.hermes/sessions.🔄 Changes
✨ Added
HermesAgentRolloutFormatHandlerthat normalizes Hermes CLIsession_*.jsonlogs and gateway*.jsonltranscripts into the shared rollout record schema🔧 Changed
AgentRolloutFormatwithHERMES_AGENTand defaulted it to~/.hermes/sessions🧪 Tests
uv run --group dev pytest packages/data-designer-config/tests/config/test_seed_source.py packages/data-designer-engine/tests/engine/resources/agent_rollout/test_hermes_agent.py packages/data-designer-engine/tests/engine/resources/test_seed_reader.py packages/data-designer/tests/interface/test_data_designer.pyAgentRolloutSeedReadersuccessfully parsed 12 real Hermes CLI session files from~/.hermes/sessions🔍 Attention Areas
packages/data-designer-config/src/data_designer/config/seed_source.py- Hermes uses a broader*.json*default so CLI JSON logs and gateway JSONL transcripts can coexist under one format; handler-side filtering is intentionally relied on.packages/data-designer-engine/src/data_designer/engine/resources/agent_rollout/hermes_agent.py- This is the new normalization path for the two Hermes file shapes, including compactsource_metamapping for model/session metadata.🤖 Generated with AI