Handle custom-title, agent-name, and agent-color transcript entry types#113
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds four Claude Code session-metadata snapshot types ( Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
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: 1
🧹 Nitpick comments (2)
claude_code_log/factories/transcript_factory.py (1)
218-226: Use.get("sessionId")for symmetry with the fallback guard.Line 221 uses
data["sessionId"](direct access), while both call sites already gate ondata.get("sessionId")truthiness (line 263 here, andconverter.py:329per the relevant snippet). That's fine today, but the dispatch call path at line 261 forcustom-title/agent-name/agent-colordoes not pre-checksessionId— it relies on Claude Code always emitting it for these types. If a malformed line ever omitssessionId, you'll get a rawKeyErrorinstead of theValidationErrorPydantic would otherwise raise (which the outerconverter.pyloop catches more gracefully).Switching to
data.get("sessionId")and letting Pydantic validate (sessionId: stris required on the model) yields a cleaner error path.♻️ Proposed change
- sessionId=data["sessionId"], + sessionId=data.get("sessionId"), # type: ignore[arg-type] # validated by Pydantic🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/factories/transcript_factory.py` around lines 218 - 226, The PassthroughTranscriptEntry construction uses direct indexing for sessionId (data["sessionId"]) which can raise KeyError; change it to use data.get("sessionId") so missing sessionId flows through Pydantic validation instead of raising KeyError—update the sessionId argument in the PassthroughTranscriptEntry call inside transcript_factory.py (the return constructing PassthroughTranscriptEntry) to use data.get("sessionId") for symmetry with other guards and to let the model (sessionId: str) produce a ValidationError.claude_code_log/models.py (1)
232-235: Nit: inline comments exceed typical line length.Minor — the new inline comments on lines 232 and 235 push the lines well past 100 chars. If the project runs
ruff/blackwith default line length, consider moving the note above the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/models.py` around lines 232 - 235, The inline comments on the dataclass fields uuid and timestamp are pushing those lines past the project line-length; move those notes above the affected fields as standalone comments (or a short docstring block) so the declarations remain under 100 chars. Specifically, for the fields uuid: Optional[str] = None and timestamp: Optional[str] = None in models.py, lift the explanatory notes ("Optional to support types like custom-title that lack uuid"/"timestamp") onto their own lines immediately above the corresponding field declarations to preserve semantics and keep uuid, parentUuid, sessionId, and timestamp lines short.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claude_code_log/models.py`:
- Around line 232-235: The build_message_index function currently dereferences
optional fields on PassthroughTranscriptEntry (accessing entry.timestamp and
entry.uuid) causing crashes and None keys; update build_message_index to skip
any PassthroughTranscriptEntry whose uuid or timestamp is None in both passes:
before using entry.timestamp or entry.uuid, guard with an if (e.g. if entry.uuid
is None or entry.timestamp is None: continue), avoid inserting None into the
nodes dict, and only pass validated timestamp/uuid into the DAGNode constructor
and subsequent logic so all assumptions about non-null uuid/timestamp hold.
---
Nitpick comments:
In `@claude_code_log/factories/transcript_factory.py`:
- Around line 218-226: The PassthroughTranscriptEntry construction uses direct
indexing for sessionId (data["sessionId"]) which can raise KeyError; change it
to use data.get("sessionId") so missing sessionId flows through Pydantic
validation instead of raising KeyError—update the sessionId argument in the
PassthroughTranscriptEntry call inside transcript_factory.py (the return
constructing PassthroughTranscriptEntry) to use data.get("sessionId") for
symmetry with other guards and to let the model (sessionId: str) produce a
ValidationError.
In `@claude_code_log/models.py`:
- Around line 232-235: The inline comments on the dataclass fields uuid and
timestamp are pushing those lines past the project line-length; move those notes
above the affected fields as standalone comments (or a short docstring block) so
the declarations remain under 100 chars. Specifically, for the fields uuid:
Optional[str] = None and timestamp: Optional[str] = None in models.py, lift the
explanatory notes ("Optional to support types like custom-title that lack
uuid"/"timestamp") onto their own lines immediately above the corresponding
field declarations to preserve semantics and keep uuid, parentUuid, sessionId,
and timestamp lines short.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 829d7808-f09c-47b6-8abc-fd7db878d2dc
📒 Files selected for processing (2)
claude_code_log/factories/transcript_factory.pyclaude_code_log/models.py
| uuid: Optional[str] = None # Optional to support types like custom-title that lack uuid | ||
| parentUuid: Optional[str] = None | ||
| sessionId: str | ||
| timestamp: str | ||
| timestamp: Optional[str] = None # Optional to support types like custom-title that lack timestamp |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm dag.py does not already guard against None uuid/timestamp on PassthroughTranscriptEntry,
# and surface any other call sites that dereference these fields unconditionally.
rg -nP -C3 '\b(entry|e)\.(uuid|timestamp)\b' --type=py
rg -nP -C2 'PassthroughTranscriptEntry' --type=pyRepository: daaain/claude-code-log
Length of output: 17628
🏁 Script executed:
cd /tmp && find . -name "*.py" -type f -exec grep -l "PassthroughTranscriptEntry" {} \; 2>/dev/null | head -20Repository: daaain/claude-code-log
Length of output: 48
🏁 Script executed:
# Check if PassthroughTranscriptEntry is used in any other context where timestamp/uuid are dereferenced
rg -A5 'isinstance.*PassthroughTranscriptEntry' --type=py | grep -E '\.uuid|\.timestamp'Repository: daaain/claude-code-log
Length of output: 396
🏁 Script executed:
# Verify the current PassthroughTranscriptEntry definition
sed -n '223,235p' claude_code_log/models.pyRepository: daaain/claude-code-log
Length of output: 682
Critical: dag.py::build_message_index unconditionally dereferences optional uuid/timestamp on PassthroughTranscriptEntry.
Making uuid and timestamp optional is correct for the schema, but build_message_index does not guard against None values. Both passes in that function directly access these fields:
- Line 94:
ts = entry.timestampfollowed by comparison will raiseTypeErrorwhen entry lacks timestamp - Line 103:
uuid = entry.uuidcreates aNonekey in nodes dict, colliding all uuid-less entries - Lines 115, 123: timestamp passed to DAGNode constructor without validation
Since this PR adds entries without these fields (custom-title, agent-name, agent-color), the DAG builder must skip PassthroughTranscriptEntry entries where uuid or timestamp is None.
Fix in claude_code_log/dag.py
for entry in entries:
if isinstance(entry, (SummaryTranscriptEntry, QueueOperationTranscriptEntry)):
continue
+ if isinstance(entry, PassthroughTranscriptEntry) and (
+ entry.uuid is None or entry.timestamp is None
+ ):
+ continue
sid = entry.sessionId
ts = entry.timestamp(Apply the same guard to the second pass.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude_code_log/models.py` around lines 232 - 235, The build_message_index
function currently dereferences optional fields on PassthroughTranscriptEntry
(accessing entry.timestamp and entry.uuid) causing crashes and None keys; update
build_message_index to skip any PassthroughTranscriptEntry whose uuid or
timestamp is None in both passes: before using entry.timestamp or entry.uuid,
guard with an if (e.g. if entry.uuid is None or entry.timestamp is None:
continue), avoid inserting None into the nodes dict, and only pass validated
timestamp/uuid into the DAGNode constructor and subsequent logic so all
assumptions about non-null uuid/timestamp hold.
|
Thanks! That's a start, I'll rebase and see if I can enhance that a bit. One idea from #94 was that, at least the session name if not the session color, could be reused in the rendering somewhere. |
These session metadata entry types are generated by Claude Code but were not recognized by the parser, causing 'Unknown transcript entry type' errors. Changes: - Make uuid and timestamp optional in PassthroughTranscriptEntry to support entry types that lack these fields - Add custom-title, agent-name, and agent-color to ENTRY_CREATORS registry - Update fallback in create_transcript_entry to handle any unknown type with a sessionId gracefully
Claude Code writes per-session state snapshots to the transcript with no uuid/parentUuid/timestamp — they are positional markers, not DAG nodes. They arrive frequently (e.g. a `permission-mode` after every mode toggle, an `agent-name`+`custom-title` pair after every /rename) and would otherwise drown the unrecognised-type warning introduced on top of daaain#112. Four types land in the silent-skip list: - permission-mode : {permissionMode} - custom-title : {customTitle} - agent-name : {agentName} - agent-color : {agentColor} All four will become load-bearing once we propagate their state onto conversational messages in a follow-up (see daaain#94). Dropping them silently for now is a strict improvement over the current state: either noisy warnings (reinstated warning branch on top of daaain#112) or silent loss (main). Extend test_silent_skip with a parameterised case covering all four and repoint the unrecognised-type parametrisation onto two hypothetical future types, since the original custom-title / agent-name samples now land on the silent path.
Spells out the deferred half of issue daaain#94 — turning the now-silent session-metadata types into a visible "Assistant · CCL (Monk)" decoration on conversational messages, with colour tinting from agent-color. Covers: data shape (no uuid/timestamp, pure positional markers), file-position propagation semantics (single self-contained session file), six concrete change sites (MessageMeta fields, load_transcript state tracker, private-attr channel onto pydantic entries, create_meta forwarding, title_AssistantTextMessage decoration, CSS), cache concerns, open questions (separator, colour palette, permission-mode surfacing, session nav integration), and risks (snapshot churn, private-attr subtlety, markdown heading compactor). This PR does NOT complete daaain#94. Wording here is deliberately free of GitHub close keywords: daaain#94 stays open as the tracker for the state-propagation implementation documented in work/session-state-propagation.md. The PR description should also be edited to use a plain `daaain#94` reference rather than a closing keyword so merging does not auto-close the issue.
cboos
force-pushed
the
fix/handle-custom-title-agent-name-agent-color
branch
from
164a728 to
4775ec9
Compare
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 (2)
test/test_silent_skip.py (2)
181-183:⚠️ Potential issue | 🟡 MinorAlign the warning expectation with the requested spelling.
The PR objective mentions changing “recognised” to “recognized,” but this assertion still locks in the old text.
📝 Proposed spelling update
assert messages == [] - assert "unrecognised message type" in captured.out + assert "unrecognized message type" in captured.out assert repr(entry["type"]) in captured.out🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_silent_skip.py` around lines 181 - 183, Update the test assertion that checks the warning text to use the new US spelling "recognized" instead of "unrecognised"; locate the assertions around variables messages, captured.out and entry["type"] (the lines asserting messages == [], "unrecognised message type" in captured.out, and repr(entry["type"]) in captured.out) and change the literal "unrecognised message type" to "unrecognized message type" so the test matches the updated wording.
8-9:⚠️ Potential issue | 🟡 MinorUpdate the stale unknown-type descriptions.
These docstrings still say
custom-title/agent-nameshould warn, but the new tests treat them as known silent-skip session metadata.📝 Proposed docstring cleanup
-- `custom-title`, `agent-name`: unknown types with sessionId but no uuid; - warn so new Claude Code metadata surfaces instead of being lost. +- `permission-mode`, `custom-title`, `agent-name`, `agent-color`: + known session metadata types with sessionId but no uuid; silently dropped. ... -class TestUnrecognisedTypesWarn: - """Unknown types with no DAG fields surface a warning so we notice - new Claude Code metadata worth supporting (custom-title, agent-name, - and anything that arrives later).""" +class TestUnrecognisedTypesWarn: + """Unknown types with no DAG fields surface a warning so we notice + future Claude Code metadata worth supporting."""Also applies to: 157-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_silent_skip.py` around lines 8 - 9, Update the stale docstrings in test_silent_skip.py that claim `custom-title` and `agent-name` are "unknown types" that should warn; change the wording to state they are known "silent-skip" session metadata (i.e., treated as silent-skip session metadata rather than producing warnings). Locate the docstring(s) that mention `custom-title` and `agent-name` (and the duplicate occurrence later in the same file) and revise the description to reflect the new test behavior where these keys are considered known silent-skip metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test_silent_skip.py`:
- Around line 65-69: The test currently only checks that "unrecognised" is not
present in captured.out; instead assert that there is no output at all for
silent-skip metadata by replacing that negative substring check with an
exact-empty check (e.g., assert captured.out == "" or assert
captured.out.strip() == ""), keeping the existing call to load_transcript and
use of capsys; update the assertion near the use of load_transcript and captured
to ensure the test fails if any text (including corrected spellings like
"unrecognized") is printed.
In `@work/session-state-propagation.md`:
- Around line 156-168: The HTML fragment currently interpolates
message.meta.agent_name and message.meta.agent_color directly; escape
message.meta.agent_name before rendering and do not interpolate
message.meta.agent_color raw into the class name—map message.meta.agent_color
through a fixed allowlist/enum (e.g., allowed colors -> "red","blue", etc.) and
only render a class like "agent-color-{safeColor}" when the color maps to a
known value; keep the existing fallback that returns plain base when markdown
renderer is used or name is empty.
- Around line 127-148: The code currently mixes per-entry private attrs
(_session_state vs _session_agent_name/_session_agent_color) so the session data
is lost; standardize on a single private-attribute shape (use
entry._session_state as a dict attached in the loader and keep
BaseTranscriptEntry tolerant of non-field attrs) and update meta_factory.py (the
MessageMeta construction) to read that dict via state = getattr(transcript,
"_session_state", {}) or {} and then use state.get("agent_name"),
state.get("agent_color"), state.get("custom_title"),
state.get("permission_mode") for the corresponding MessageMeta fields; also
replace any places that set _session_agent_name/_session_agent_color to instead
populate entry._session_state keys so the propagation is consistent.
---
Outside diff comments:
In `@test/test_silent_skip.py`:
- Around line 181-183: Update the test assertion that checks the warning text to
use the new US spelling "recognized" instead of "unrecognised"; locate the
assertions around variables messages, captured.out and entry["type"] (the lines
asserting messages == [], "unrecognised message type" in captured.out, and
repr(entry["type"]) in captured.out) and change the literal "unrecognised
message type" to "unrecognized message type" so the test matches the updated
wording.
- Around line 8-9: Update the stale docstrings in test_silent_skip.py that claim
`custom-title` and `agent-name` are "unknown types" that should warn; change the
wording to state they are known "silent-skip" session metadata (i.e., treated as
silent-skip session metadata rather than producing warnings). Locate the
docstring(s) that mention `custom-title` and `agent-name` (and the duplicate
occurrence later in the same file) and revise the description to reflect the new
test behavior where these keys are considered known silent-skip metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3a4e79e-459a-4c1b-af24-9cf9227d7bc9
📒 Files selected for processing (4)
claude_code_log/converter.pytest/test_dag_integration.pytest/test_silent_skip.pywork/session-state-propagation.md
✅ Files skipped from review due to trivial changes (2)
- claude_code_log/converter.py
- test/test_dag_integration.py
| Return `entry_state` as a second tuple element, or attach per-entry | ||
| via private attribute (`entry._session_state = dict(current_state)`). | ||
| The private-attr approach keeps the public signature stable at the | ||
| cost of an out-of-band channel; the tuple approach is more explicit | ||
| but ripples through every `load_transcript` call site. | ||
|
|
||
| Recommendation: **private attribute**, since the state is logically | ||
| part of the entry context (like `agentId`) and callers who don't care | ||
| stay unchanged. `BaseTranscriptEntry` already tolerates it (pydantic | ||
| v2 allows non-field attributes on instances). | ||
|
|
||
| ### 3. `claude_code_log/factories/meta_factory.py` | ||
|
|
||
| Forward the private attrs into `MessageMeta` via `getattr(..., None)`: | ||
|
|
||
| ```python | ||
| return MessageMeta( | ||
| ..., | ||
| agent_name=getattr(transcript, "_session_agent_name", None), | ||
| agent_color=getattr(transcript, "_session_agent_color", None), | ||
| ..., | ||
| ) |
There was a problem hiding this comment.
Use one private-attribute shape consistently.
The sketch stores entry._session_state, but meta_factory.py reads _session_agent_name and _session_agent_color. Pick one convention so the propagated state is not silently dropped.
📝 Proposed spec cleanup
-Return `entry_state` as a second tuple element, or attach per-entry
-via private attribute (`entry._session_state = dict(current_state)`).
+Return `entry_state` as a second tuple element, or attach per-entry
+via one private attribute (`entry._session_state = dict(current_state)`).
...
```python
+state = getattr(transcript, "_session_state", {}) or {}
return MessageMeta(
...,
- agent_name=getattr(transcript, "_session_agent_name", None),
- agent_color=getattr(transcript, "_session_agent_color", None),
+ agent_name=state.get("agent_name"),
+ agent_color=state.get("agent_color"),
+ custom_title=state.get("custom_title"),
+ permission_mode=state.get("permission_mode"),
...,
)
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @work/session-state-propagation.md around lines 127 - 148, The code currently
mixes per-entry private attrs (_session_state vs
_session_agent_name/_session_agent_color) so the session data is lost;
standardize on a single private-attribute shape (use entry._session_state as a
dict attached in the loader and keep BaseTranscriptEntry tolerant of non-field
attrs) and update meta_factory.py (the MessageMeta construction) to read that
dict via state = getattr(transcript, "_session_state", {}) or {} and then use
state.get("agent_name"), state.get("agent_color"), state.get("custom_title"),
state.get("permission_mode") for the corresponding MessageMeta fields; also
replace any places that set _session_agent_name/_session_agent_color to instead
populate entry._session_state keys so the propagation is consistent.
</details>
<!-- fingerprinting:phantom:poseidon:ibis -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ```python | ||
| base = "Sub-assistant" if message.meta.is_sidechain else "Assistant" | ||
| name = message.meta.agent_name | ||
| color = message.meta.agent_color | ||
| if name: | ||
| if color: | ||
| return f'{base} · <span class="agent-color-{color}">{name}</span>' | ||
| return f"{base} · {name}" | ||
| return base | ||
| ``` | ||
|
|
||
| HTML escape `name` (agent names can contain arbitrary characters). | ||
| Markdown renderer would emit plain text (no color span). |
There was a problem hiding this comment.
Allowlist agent_color before interpolating it into HTML.
Line 167 calls out escaping name, but the class suffix is transcript-controlled too. A malicious agentColor can break the class attribute unless it is mapped through a known palette.
🛡️ Proposed safer renderer sketch
base = "Sub-assistant" if message.meta.is_sidechain else "Assistant"
name = message.meta.agent_name
color = message.meta.agent_color
if name:
- if color:
- return f'{base} · <span class="agent-color-{color}">{name}</span>'
- return f"{base} · {name}"
+ safe_name = html.escape(name)
+ color_class = AGENT_COLOR_CLASSES.get(color)
+ if color_class:
+ return f'{base} · <span class="{color_class}">{safe_name}</span>'
+ return f"{base} · {safe_name}"
return base-HTML escape name (agent names can contain arbitrary characters).
+HTML escape name and map agent_color through an allowlist before
+using it as a CSS class.
Markdown renderer would emit plain text (no color span).
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @work/session-state-propagation.md around lines 156 - 168, The HTML fragment
currently interpolates message.meta.agent_name and message.meta.agent_color
directly; escape message.meta.agent_name before rendering and do not interpolate
message.meta.agent_color raw into the class name—map message.meta.agent_color
through a fixed allowlist/enum (e.g., allowed colors -> "red","blue", etc.) and
only render a class like "agent-color-{safeColor}" when the color maps to a
known value; keep the existing fallback that returns plain base when markdown
renderer is used or name is empty.
</details>
<!-- fingerprinting:phantom:poseidon:ibis -->
<!-- This is an auto-generated comment by CodeRabbit -->
Two small follow-ups on top of the SILENT_SKIP_TYPES work. The "unrecognised message type" warning landed in the prior PR using the British spelling, matching the pre-daaain#97 original phrasing. daaain#94 explicitly called out the preferred `s/recognised/recognized/`, so flip it here. Single user-facing string; the else-branch is only reached for unknown types, so unlikely to bite anyone scraping logs. CodeRabbit flagged the silent-skip test assertions — they used the weak form `"unrecognised" not in captured.out`, which only catches a specific substring and silently tolerates any other output the loader might accidentally start printing. Switch every silent-path test to call `load_transcript(..., silent=True)` and assert `captured.out == ""`; the warn-path test keeps `silent=False` and now matches the new American spelling.
2 participants
Addresses one half of #94: makes the transcript loader stop warning
on four session-metadata types that Claude Code writes as
positional markers (no
uuid/parentUuid/timestamp):permission-mode({permissionMode})custom-title({customTitle})agent-name({agentName})agent-color({agentColor})Changes
SILENT_SKIP_TYPES(introduced in Ignore 'last-prompt' message type #112) with the four typestest_silent_skip.pytest_multiple_passthrough_typeswhich had"permission-mode"hard-coded as a synthetic Passthrough label
work/session-state-propagation.md— design for the deferredhalf of Support messages for setting session title, name, and color #94 (propagate agent name/color onto conversational
message titles)
Not in this PR
Refs #94 but does not close it — the state-propagation
implementation lives in the linked plan doc and remains open work.
Note on the original approach
fuleinist's original commit registered these types as
PassthroughTranscriptEntryand relaxed the model to makeuuidand
timestampoptional. Maintainer feedback: Passthrough's role isDAG continuity through entries that have
uuid/parentUuid; thesefour types lack both and shouldn't be Passthrough. This PR preserves
fuleinist's commit as an empty attribution commit and reimplements
the intent via the silent-skip list.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation