Conversation
Claude Code's Skill invocation lands on disk as three disjoint
entries:
1. assistant `Skill` tool_use
2. user tool_result with the literal string "Launching skill: <name>"
3. user `isMeta=True` entry whose `sourceToolUseID` matches (1) and
whose text is the expanded skill body (markdown, often 100+
lines)
Rendered as-is, (3) shows up as a disjoint "🧑 User (slash command)"
block visually unrelated to (1) — which is exactly what #93 flags:
"seeing the skill content as a user message (right aligned) breaks
the 'flow'."
The linkage we need is already in the data. (3) carries a top-level
`sourceToolUseID` equal to (1)'s tool_use id. No heuristic, no
parent-chain walk, no proximity guess — a field lookup.
## Changes
- `UserTranscriptEntry` gains `sourceToolUseID: Optional[str]` so
the field survives Pydantic parsing.
- `MessageMeta` gains `source_tool_use_id` and `meta_factory` forwards
the value through.
- `ToolUseMessage` gains `skill_body: Optional[str]` — set only for
Skill invocations that found a matching (3).
- `_pair_skill_tool_uses(ctx)` in renderer.py walks ctx.messages
once: it indexes slash-command TemplateMessages by
`meta.source_tool_use_id`, then for each `ToolUseMessage` with
`tool_name == "Skill"` attaches the matching slash-command's
text as `skill_body` and marks both the slash-command and the
matching "Launching skill" tool_result as consumed. Consumed
indices feed through the existing `_reindex_filtered_context` so
the rest of the pipeline sees a clean, gap-free list.
- Called before the detail-level post-render filter so the body
survives alongside the tool_use at HIGH — arguably correct per
main's "body is content of the Skill tool_use, not an independent
message" framing. MINIMAL/LOW still drop the whole skill
invocation since tool_use itself is filtered there.
- HTML renderer overrides `format_ToolUseMessage`: after the
standard params-table render, appends the body via
`render_markdown_collapsible(..., "skill-body", …)` so long
bodies collapse like existing slash-command / compacted-summary
rendering.
- Markdown renderer does the same with raw markdown passthrough.
- New `.skill-body` CSS rule gives the embedded body a left border
and inset padding so it reads as nested content under the
tool invocation, not a sibling message.
## Tests (8 new in test_skill_pairing.py)
Template-level:
- Body folds into the ToolUseMessage as skill_body
- Slash-command TemplateMessage is consumed (removed from ctx.messages)
- "Launching skill: X" tool_result is dropped
- Non-Skill tool_uses (e.g. Bash) are untouched — their tool_result stays
- isMeta entries without sourceToolUseID still render as slash-commands
- Orphan skill body (pointing at a missing tool_use) stays as a standalone
slash-command
Renderer output:
- HTML: `.skill-body` class and rendered markdown (<strong> etc.) land
in the output; "Launching skill:" string is absent
- Markdown: raw markdown passes through; "Launching skill:" absent
## Scope checks
- No new pair primitive — uses `_reindex_filtered_context` to drop
consumed messages.
- Narrow dispatch: only `tool_name == "Skill"` is paired.
- Fall-back-clean: entries without `sourceToolUseID` (older Claude
Code versions) behave identically to current main.
- Edge-case snapshot regenerated mechanically (CSS addition only).
All 922 unit tests pass. pyright / ty / ruff clean on modified files.
Refs #93.
|
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 (1)
📝 WalkthroughWalkthroughFold Skill slash-command user entries into their corresponding Skill ToolUseMessage by pairing on (session_id, tool_use_id), attach the skill markdown as Changes
Sequence DiagramsequenceDiagram
participant Transcript as Transcript
participant Pairer as SkillPairingLogic
participant Messages as MessageStream
participant Renderer as Renderer
participant Output as RenderedOutput
Transcript->>Pairer: Emit entries (tool_use "Skill", tool_result "Launching skill:", user meta with sourceToolUseID)
Pairer->>Pairer: Detect Skill tool_use and find matching meta by (session_id, sourceToolUseID)
Pairer->>Pairer: Extract slash-command body -> attach to ToolUseMessage.skill_body
Pairer->>Pairer: Mark matched user/meta and canonical tool_result as consumed
Pairer->>Messages: Remove consumed entries and reindex stream
Messages->>Renderer: Provide ToolUseMessage (with skill_body)
Renderer->>Output: Render tool_use + collapsible / appended skill body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🤖 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/renderer.py`:
- Around line 2032-2091: The pairing currently matches only on tool_use_id and
can collide across sessions and can drop real results; modify
_pair_skill_tool_uses to key slash_by_source by (source_tool_use_id,
session_key) where session_key is taken from a session/render identifier on
messages (e.g. msg.meta.render_session_id or msg.meta.session_id) so lookups use
the same session, update the lookup and the later slash =
slash_by_source.get(...) usage to use that composite key, and when marking
matching tool_result entries for removal only include tool_result messages in
the same session whose visible text/content begins with "Launching skill:" (and
skip any tool_result that indicates an error/status != success if such meta
exists) instead of removing all tool_results with the same tool_use_id. This
ensures you only fold the intended slash body and drop the redundant "Launching
skill:" result within the same session.
🪄 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: 93790577-4afc-4767-906c-c6381acefe04
📒 Files selected for processing (8)
claude_code_log/factories/meta_factory.pyclaude_code_log/html/renderer.pyclaude_code_log/html/templates/components/message_styles.cssclaude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/renderer.pytest/__snapshots__/test_snapshot_html.ambrtest/test_skill_pairing.py
CodeRabbit on PR #121 caught two failure modes in `_pair_skill_tool_uses` that the original commit (dc1770a) didn't defend against. Both are session-/payload-precision concerns that surface in combined transcripts or with malformed/error tool_results. ## #1 — Cross-session pairing collision The lookup was keyed on `tool_use_id` alone. `RenderingContext.messages` spans combined transcripts (multiple sessions), but Anthropic tool_use ids are only session-unique. A stray collision would let session B's slash body fold into session A's Skill tool_use (or vice versa) silently. Fix: key the lookup by `(render_session_id, source_tool_use_id)` and match the same compound on the consuming side. `render_session_id` is fork-aware so within-session branches stay distinct too. ## #2 — Real error tool_results silently dropped The original code dropped EVERY tool_result with the matching `tool_use_id`, on the assumption that only the canonical `"Launching skill: <name>"` result carries that id. A real error result ("Skill 'X' not found", `is_error=True`) would have the same id and get swallowed too — hiding the failure from the user entirely. Fix: only drop tool_results that are (a) in the same session, (b) `is_error=False`, AND (c) whose payload starts with `"Launching skill:"`. New `_is_launching_skill_payload` helper handles both string- and list-shaped `ToolResultContent.content`. ## Tests (3 new in TestSkillPairing) - `test_same_tool_use_id_across_sessions_does_not_cross_pair` — combined transcript with two sessions sharing `tool_use_id="toolu_DUP"` but different bodies; asserts each Skill keeps its own session's body. - `test_error_tool_result_with_same_id_is_preserved` — a Skill failure flow where the error result shares the tool_use_id; asserts the error survives while the canonical "Launching skill:" result is still dropped. - `test_non_launching_skill_result_with_same_id_is_preserved` — divergent payload sharing the id (e.g. malformed transcript) survives. All three fail on the pre-fix code (cross-pair, swallow error, swallow divergent payload) and pass after. Refs #93 / #121. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The merge with main brings in the teammates fixture and snapshot tests added by the recent rendering work. The teammates snapshot embeds the full message_styles.css bundle, which now includes a `.skill-body` rule from this branch's Skill pairing changes. CI on the merged head fails because the snapshot was captured pre-`.skill-body`. Sequential `--snapshot-update` (per the project's snapshot guidance) adds the missing 11 lines to the teammates fixture snapshot. Both parallel and sequential test runs now show 12/12 passing.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
claude_code_log/renderer.py (1)
2148-2175: Optional: pre-index tool_results for the inner scan.PR description already calls out the O(K·N) inner search as a later perf pass. If you revisit it, a
(render_session_id, tool_use_id) -> list[TemplateMessage]map built in the same sweep asslash_by_sourceturns the nested loop into a direct lookup, with the prefix /is_errorguards unchanged. Skipping for now given the PR is already large and the Skill tool fan-out is small in practice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/renderer.py` around lines 2148 - 2175, The inner O(K·N) scan over ctx.messages to find matching ToolResultMessage should be replaced by a pre-indexed map so we can do a direct lookup: during the same sweep that builds slash_by_source, also build a dict mapping (render_session_id, tool_use_id) -> list[ToolResultMessage] (only include messages with message_index not None and not is_error), then in the loop for each ToolUseMessage (the block that references slash_by_source, consumed_indices, UserSlashCommandMessage) replace the nested for other in ctx.messages scan with a lookup into that dict and iterate that small list, applying the existing _is_launching_skill_payload(other.content.output) check and adding other.message_index to consumed_indices when matched.claude_code_log/html/renderer.py (1)
427-436: Optional: mirror the Markdown renderer's empty-renderedguard.
MarkdownRenderer.format_ToolUseMessagereturnscontent.skill_bodyverbatim whensuper()yields""(see snippet fromclaude_code_log/markdown/renderer.py:676-690). Here the concatenation is unconditional, so a SkillToolUseMessagewhose params table renders to""would emit a leading newline beforebody_html. Harmless in HTML, but the two renderers drift in behavior for the same content.♻️ Optional parity tweak
rendered = super().format_ToolUseMessage(content, message) if content.skill_body: body_html = render_markdown_collapsible( content.skill_body, "skill-body", line_threshold=30, preview_line_count=10, ) - rendered = f"{rendered}\n{body_html}" + rendered = f"{rendered}\n{body_html}" if rendered else body_html return rendered🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/html/renderer.py` around lines 427 - 436, The HTML renderer unconditionally appends the collapsible skill body which causes a leading newline when super().format_ToolUseMessage returns an empty string; update claude_code_log/html/renderer.py in format_ToolUseMessage to mirror the Markdown renderer's empty-rendered guard (check the local rendered variable returned by super()) and only prepend a newline and append body_html when rendered is non-empty, otherwise set rendered to body_html directly; reference rendered, content.skill_body, format_ToolUseMessage, and render_markdown_collapsible to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@claude_code_log/html/renderer.py`:
- Around line 427-436: The HTML renderer unconditionally appends the collapsible
skill body which causes a leading newline when super().format_ToolUseMessage
returns an empty string; update claude_code_log/html/renderer.py in
format_ToolUseMessage to mirror the Markdown renderer's empty-rendered guard
(check the local rendered variable returned by super()) and only prepend a
newline and append body_html when rendered is non-empty, otherwise set rendered
to body_html directly; reference rendered, content.skill_body,
format_ToolUseMessage, and render_markdown_collapsible to locate the change.
In `@claude_code_log/renderer.py`:
- Around line 2148-2175: The inner O(K·N) scan over ctx.messages to find
matching ToolResultMessage should be replaced by a pre-indexed map so we can do
a direct lookup: during the same sweep that builds slash_by_source, also build a
dict mapping (render_session_id, tool_use_id) -> list[ToolResultMessage] (only
include messages with message_index not None and not is_error), then in the loop
for each ToolUseMessage (the block that references slash_by_source,
consumed_indices, UserSlashCommandMessage) replace the nested for other in
ctx.messages scan with a lookup into that dict and iterate that small list,
applying the existing _is_launching_skill_payload(other.content.output) check
and adding other.message_index to consumed_indices when matched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4405d23b-7975-46b3-b003-3f5922d44be8
📒 Files selected for processing (6)
claude_code_log/factories/meta_factory.pyclaude_code_log/html/renderer.pyclaude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/renderer.pytest/__snapshots__/test_snapshot_html.ambr
✅ Files skipped from review due to trivial changes (2)
- claude_code_log/factories/meta_factory.py
- test/snapshots/test_snapshot_html.ambr
🚧 Files skipped from review as they are similar to previous changes (1)
- claude_code_log/markdown/renderer.py
pyright (strict) flags two checks as `reportUnnecessaryIsInstance`:
- `isinstance(content, list)` after `isinstance(content, str)` returned
False — content is `Union[str, list[dict[str, Any]]]` (Pydantic-typed),
so after the str-narrow the type IS list.
- `isinstance(item, dict)` inside the iteration — items in
`list[dict[str, Any]]` are already typed `dict`.
Both checks were defensive runtime guards, but the Pydantic schema makes
them unreachable. Drop the type-redundant isinstance, keep the
`isinstance(text, str)` for the `Any`-typed `.get("text")` value (that
one IS necessary — the dict value type is `Any`).
Inline comment explains the surviving narrowing path.
ty (which doesn't enforce reportUnnecessaryIsInstance) was already
clean. Local + CI pyright now both green.
Fixup for #121: a Skill invocation rendered as the generic tool header ("🛠️ Skill") plus a "skill / <name>" params row, leaving the folded body visually disjoint from the title that names it. Now the title carries everything — "💡 Skill <name>" — and the params row is suppressed since the only field (skill) is in the title and the body folds in below via skill_body. - New SkillInput typed model (skill: str, extra="allow" so future Skill input shapes don't fall back to the generic table). - title_SkillInput / format_SkillInput in HTML and Markdown renderers produce the new title and an empty body slot. - The .skill-body inner left accent is gone — the outer .tool_use green border already frames the block, the inner accent only made it look doubly-framed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #93.
Claude Code's Skill invocation lands on disk as three disjoint entries:
Skilltool_use"Launching skill: <name>"isMeta=Trueentry whosesourceToolUseIDmatches (1) and whose text is the expanded skill body (markdown, often 100+ lines)Rendered as-is, (3) appears as a "🧑 User (slash command)" block visually disjoint from (1) — which is exactly what #93 flags: "seeing the skill content as a user message (right aligned) breaks the 'flow'."
The linkage is already in the data. (3) carries a top-level
sourceToolUseIDequal to (1)'s tool_use id. No heuristic, no parent-chain walk, no proximity guess — a field lookup.Changes
UserTranscriptEntry.sourceToolUseID: Optional[str]— Pydantic field.MessageMeta.source_tool_use_id— propagated viameta_factory(getattr-with-default handles older transcripts).ToolUseMessage.skill_body: Optional[str]— set only when paired._pair_skill_tool_uses(ctx)inrenderer.py: single-pass indexing of slash-command TemplateMessages bysource_tool_use_id, then folds the body into each SkillToolUseMessageand drops the slash-command + the redundant"Launching skill"tool_result. Runs right after_render_messages(before detail filtering) and reuses_reindex_filtered_contextfor the drops.format_ToolUseMessage: appends the body viarender_markdown_collapsible(same collapsible primitive as slash-command / compacted-summary)..skill-bodyCSS — left border + inset padding — makes the folded body read as nested content under the params table.Detail-level interaction
Pairing runs before
_filter_template_by_detail. At HIGH the body survives alongside the tool_use — intended, since once paired the body is content of the Skill tool_use, not an independent message. MINIMAL/LOW continue to drop the entire Skill invocation becauseToolUseMessageitself is in those exclude chains.Tests (8 new in
test/test_skill_pairing.py)Template-level:
ToolUseMessage.skill_body."Launching skill: X"tool_result is dropped.isMetaentries withoutsourceToolUseIDstill render as standalone slash-commands.Renderer output:
.skill-bodyclass + rendered markdown present,"Launching skill"absent."Launching skill"absent.Validation
just test(excluding tui/browser/pagination): 922 passed, 7 skipped.uv run pyrighton modified files: 0 errors, 0 warnings.uv run ty check: All checks passed (0 diagnostics).ruff format+ruff check: clean.Review
Monk approved at
dc1770a. Optional non-blocker flagged: the inner tool_result search in_pair_skill_tool_usesis O(N·M) for typical transcripts with a handful of Skills; could be tightened to O(N+M) by indexing tool_results once. Left for a later perf pass since the actual cost is negligible.Summary by CodeRabbit
New Features
Bug Fixes
Style
Tests