Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDowngrades orphan warnings for expected Changes
Sequence DiagramsequenceDiagram
autonumber
participant Walk as _walk_session_with_forks()
participant Children as Same-Session Children
participant Classify as _is_structural_subtree()
participant Stitch as Variant‑3 Stitcher
participant Chain as DAG Chain
Walk->>Children: enumerate same-session children (timestamps, types)
Children-->>Walk: list of children
Walk->>Classify: classify each child (structural vs non-structural)
Classify-->>Walk: classifications
alt exactly one non-structural passthrough found
Walk->>Stitch: select single live passthrough sibling
Stitch->>Stitch: collect other siblings' structural descendants (dead-end), order by timestamp
Stitch->>Chain: append dead-end descendants inline
Stitch->>Chain: continue traversal from live passthrough node
Chain-->>Walk: resume traversal from passthrough continuation
else multiple/none passthrough special-case
Walk->>Chain: apply existing compaction-replay or rewind-fork rules
Chain-->>Walk: continue per timestamp-based rules
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (8)
claude_code_log/markdown/renderer.py (1)
443-468: Documented Markdown branch-header parity gap.The TODO clearly captures the divergence: HTML uses
content.title(composed by_branch_label) for branch headers, while this Markdown path re-derives a Session-style title and so renders branch headers indistinguishably from trunk session headers.A minimal, low-risk parity fix could honor
content.titlewhenis_branch:♻️ Sketch
def title_SessionHeaderMessage( self, content: SessionHeaderMessage, _: TemplateMessage ) -> str: - """Title → '📋 Session `abc12345`: summary — Team: `t`'.""" - # TODO: branch session headers (``content.is_branch``) lose the - # ``Branch • <uuid8> • <preview>`` shape that the renderer's - # ``_branch_label`` helper composes for HTML output. The Markdown - # path here ignores ``content.title`` and re-derives a Session - # title from the synthetic branch session_id (which starts with - # the trunk uuid), so a branch shows up as ``📋 Session - # `d602eb5f`: <summary>`` indistinguishable from the trunk. Mirror - # the HTML behavior by honoring ``is_branch`` and surfacing - # ``content.title`` (already laid out by ``_branch_label``) when - # rendering a branch header. See Variant 3 follow-up note in - # ``renderer.py::_branch_label``. + """Title → '📋 Session `abc12345`: summary — Team: `t`' or branch label.""" + if content.is_branch and content.title: + # HTML's ``_branch_label`` already laid out + # ``Branch • <uuid8> • <preview>``; surface that verbatim + # so Markdown branch headers don't look like trunk headers. + return f"🌿 {content.title}" session_short = content.session_id[:8]Want me to open a follow-up issue to track the parity work, or push a small commit that mirrors the HTML branch-label format here (and updates Markdown tests for the branch case)?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/markdown/renderer.py` around lines 443 - 468, The Markdown branch header path currently ignores content.is_branch and content.title and instead builds a generic "📋 Session `<short>`" string; update the logic in the renderer function that computes title (the block using session_short and content.summary) to first check if content.is_branch and, if so, use the precomposed content.title (or call the same helper used by HTML, _branch_label, to produce the branch label) instead of deriving a session-style title; preserve the existing team_name handling (apply _inline_code to content.team_name) when appending the "— Team:" suffix.test/test_dag.py (1)
1649-1682: Test placement vs. behavior under test.
test_deep_passthrough_chain_classified_structuralis grouped underTestParallelToolUseViaPassthrough, but the live continuation here goes through the user childu2— that's the structural-side-branch collapse path (Shape B in dev-docs/dag.md), not Variant 3 (where the live continuation is the passthrough). It still validates the depth-cap removal that Variant 3 depends on, so the placement is defensible — just consider tightening the docstring to call out that this exercises the depth-unboundedness invariant relied on by Variant 3, not Variant 3 itself, to make the test's intent unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_dag.py` around lines 1649 - 1682, The test test_deep_passthrough_chain_classified_structural in TestParallelToolUseViaPassthrough currently implies it exercises Variant 3 (live continuation through the passthrough) but actually uses a live continuation via the user child u2 (the structural-side-branch collapse / Shape B); update the test docstring to explicitly state that it exercises the depth-unboundedness invariant relied on by Variant 3 (i.e., removal of the 20-depth clamp) rather than Variant 3 itself, mentioning test_deep_passthrough_chain_classified_structural, TestParallelToolUseViaPassthrough and the u2 live continuation to make intent unambiguous.test/test_skill_pairing.py (1)
580-649: First test exercises only the no-shift case.The setup drops indices 3 and 4, but the fork point sits at index 2 — so
parent_message_index=2passes through_reindex_filtered_contextunchanged. The assertion still verifies link integrity, but the remap logic itself isn't exercised here; that's covered bytest_parent_message_index_remapped_when_fork_shifts.Consider tightening the comment on Line 638-642 (currently says "the fork point's NEW index (2 — unchanged...)") so the regression intent is unambiguous, e.g. "no shift is needed in this scenario; see
test_parent_message_index_remapped_when_fork_shiftsfor the shifting case." Optional — coverage is fine across the two tests together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_skill_pairing.py` around lines 580 - 649, The comment in test_parent_message_index_remapped is misleading because this test exercises the no-shift case (fork at index 2 remains unchanged) so update the comment around the branch assertion (referencing branch, branch.content.parent_message_index and _reindex_filtered_context) to explicitly say "no shift is needed in this scenario; shifting behavior is covered by test_parent_message_index_remapped_when_fork_shifts" so the regression intent is unambiguous and readers know where the shifting case is tested.dev-docs/dag.md (1)
503-510: Taxonomy table has overlapping rows.Row 1 ("The session's actual first entry... or a
progresshook onSessionStart") and Row 4 ("PassthroughTranscriptEntrytype="progress"from a session-start hook") both cover the SessionStart-hook case. Worth disambiguating so readers don't double-count: e.g., narrow row 1 to "auserprompt" only, and let row 4 own the SessionStart-hook shape.📝 Suggested wording tweak
-| **The session's actual first entry** (a `user` prompt, or a `progress` hook on `SessionStart`) | `null` | It's the earliest message — no preceding turn exists. | +| **The session's actual first entry** (a `user` prompt) | `null` | It's the earliest message — no preceding turn exists. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-docs/dag.md` around lines 503 - 510, The taxonomy table duplicates the SessionStart-hook case across two rows; edit the table so row 1 (currently "The session's actual first entry") is explicitly limited to "a `user` prompt" only, and remove any mention of hooks from that row, then make row 4 the sole owner of the SessionStart-hook shape by keeping its description as "`PassthroughTranscriptEntry` `type=\"progress\"` from a session-start hook (e.g. `SessionStart:clear`)`" and clarifying it fires before the first user turn; update the brief rationale cells accordingly so SessionStart-hook behavior is documented only under the `PassthroughTranscriptEntry` row and not duplicated in the first row.claude_code_log/renderer.py (3)
740-765: Branch label format coupling: parsingtitle.split(" • ", 2)here and re-composing via_branch_labelis fragile.The fork-point box parses the body header's title to recover
preview_text, then calls_branch_label(branch_sid, preview_text)and slices off the literal"Branch • "prefix. This works only because both ends agree on the exact"Branch • {uuid8} • {preview}"shape — any future change to_branch_label(e.g., a different separator, an extra segment, an i18n'd word) breaks bothlink_suffixandprepare_session_navigation'sbranch_previewsparser at lines 1186-1202.Consider storing the raw
previewonSessionHeaderMessage(alongsidetitle) and recomputing the label from(branch_sid, preview)everywhere, instead of round-tripping through the composed string. That removes the parser-and-recompose dance.🤖 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 740 - 765, The code is fragile because it parses SessionHeaderMessage.content.title with title.split(" • ", 2) to extract preview_text and then re-composes via _branch_label and slices off "Branch • " to build link_suffix; instead, add a dedicated preview attribute on SessionHeaderMessage (e.g., SessionHeaderMessage.preview) and populate it where headers are created, then change this block to read branch_header.content.preview and call _branch_label(branch_sid, preview) and trim nothing; update all uses (including prepare_session_navigation and any branch_previews parsing) to reconstruct labels from (branch_sid, preview) rather than parsing titles so changes to _branch_label or separators won’t break the logic.
988-1015:_enrich_branch_titles"richer preview = longer string" heuristic can lose semantics.Lines 1014-1015 only widen the title when
len(preview) > len(existing_preview). That's a reasonable proxy for "more informative" but conflates length with meaning: a short slash-command name (e.g.,"/exit") captured at render time will be replaced by any longer first-UserTextMessagetext. The docstring at lines 1006-1009 frames this as protecting slash-command captures, but a 5-char slash command is replaced by a 6-char throwaway user reply.Two cleaner options:
- Skip enrichment whenever
existing_previewis non-empty (the original_render_messagespass already had a chance and produced something).- Or, gate enrichment on the original preview being a UUID-only fallback (i.e., the title equals
f"Branch • {short_uuid}"exactly).Both eliminate the length comparison's ambiguity.
🤖 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 988 - 1015, In _enrich_branch_titles, stop using the length comparison to decide enrichment; instead only replace the SessionHeaderMessage title when the existing_preview is empty or is the UUID-only fallback produced by the earlier _render_messages pass (e.g., title pattern "Branch • {short_uuid}"). Locate the block that reads existing_parts/existing_preview and replace the len(preview) > len(existing_preview) check with a guard that returns unless existing_preview == "" or matches the short-UUID fallback pattern, then set content.title = _branch_label(sid, preview) as before; keep all other logic (branch_headers, first_user_text, SessionHeaderMessage assertion, and use of _branch_label) unchanged.
1018-1051: Truncation produces 80 chars + "..." (83 visible chars), not the documented "80 chars + ellipsis cap".
_branch_label's docstring says "Truncatespreviewto 80 chars + ellipsis". The implementation slicespreview[:80]then appends"..."when the original is longer than 80, giving a final string up to 83 characters. Whether that's what "on one line" needs is up to the layout, but it's worth aligning the docstring with the behavior — or switching to…(single character) and a tighter cap if true single-line layout is the goal.Minor, non-blocking.
🤖 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 1018 - 1051, The docstring and implementation of _branch_label disagree about the truncation: change the implementation to use a single-character ellipsis and ensure the preview portion is exactly 80 chars plus that ellipsis by slicing preview[:80] and appending "…" when len(preview) > 80; update the _branch_label docstring to mention the single-character ellipsis and the 80-char preview cap so both behavior and docs match.claude_code_log/dag.py (1)
567-607: Variant 3 logic is sound; consider an ordering note for future maintainers.The parallel-tool_use-via-passthrough collapse is correctly placed after
_stitch_tool_resultsand before the same-timestamp compaction / different-timestamp rewind branches. A few observations:
- The "any passthrough type with a live subtree" predicate (lines 589-594) is intentionally broader than today's only known case (
progress). The inline comment at lines 581-588 owns this trade-off; if Claude Code ever introduces a passthrough type that should surface as a real fork sibling rather than being followed silently, this branch will swallow it. A targeted assertion orlogger.debugwhen a non-progresspassthrough is followed here would make that scenario observable without changing behavior.if others and all(...)at line 598: inside this branchlen(same_session_children) >= 2and exactly one ispassthrough_lives, soothersis necessarily non-empty. Theif othersguard is harmless but redundant — fine to leave for readability._is_structural_subtreeis invoked twice per sibling here (lines 593 and 599) for any sibling that happens to be a non-live passthrough. Not a hot-path concern given session-bounded sizes, but a smalldict[str, bool]cache keyed byuuidwould tidy it up if this ever shows in a profile.No correctness issues — Variant 3 looks good.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/dag.py` around lines 567 - 607, The Variant 3 passthrough handling is correct but please (1) add a logger.debug (or assertion behind debug) in the passthrough detection block in dag.py (the passthrough_lives computation and following branch around variables passthrough_lives, live, others, and current) to record when a followed passthrough is not of type "progress" so future non-progress passthroughs are observable without changing behavior, and (2) reduce duplicated work by caching _is_structural_subtree results (use a small dict keyed by child uuid used in the list comprehension and the later all(...) check) to avoid recomputing the predicate for the same node; the `if others` guard may be left as-is.
🤖 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/dag.py`:
- Around line 567-607: The Variant 3 passthrough handling is correct but please
(1) add a logger.debug (or assertion behind debug) in the passthrough detection
block in dag.py (the passthrough_lives computation and following branch around
variables passthrough_lives, live, others, and current) to record when a
followed passthrough is not of type "progress" so future non-progress
passthroughs are observable without changing behavior, and (2) reduce duplicated
work by caching _is_structural_subtree results (use a small dict keyed by child
uuid used in the list comprehension and the later all(...) check) to avoid
recomputing the predicate for the same node; the `if others` guard may be left
as-is.
In `@claude_code_log/markdown/renderer.py`:
- Around line 443-468: The Markdown branch header path currently ignores
content.is_branch and content.title and instead builds a generic "📋 Session
`<short>`" string; update the logic in the renderer function that computes title
(the block using session_short and content.summary) to first check if
content.is_branch and, if so, use the precomposed content.title (or call the
same helper used by HTML, _branch_label, to produce the branch label) instead of
deriving a session-style title; preserve the existing team_name handling (apply
_inline_code to content.team_name) when appending the "— Team:" suffix.
In `@claude_code_log/renderer.py`:
- Around line 740-765: The code is fragile because it parses
SessionHeaderMessage.content.title with title.split(" • ", 2) to extract
preview_text and then re-composes via _branch_label and slices off "Branch • "
to build link_suffix; instead, add a dedicated preview attribute on
SessionHeaderMessage (e.g., SessionHeaderMessage.preview) and populate it where
headers are created, then change this block to read
branch_header.content.preview and call _branch_label(branch_sid, preview) and
trim nothing; update all uses (including prepare_session_navigation and any
branch_previews parsing) to reconstruct labels from (branch_sid, preview) rather
than parsing titles so changes to _branch_label or separators won’t break the
logic.
- Around line 988-1015: In _enrich_branch_titles, stop using the length
comparison to decide enrichment; instead only replace the SessionHeaderMessage
title when the existing_preview is empty or is the UUID-only fallback produced
by the earlier _render_messages pass (e.g., title pattern "Branch •
{short_uuid}"). Locate the block that reads existing_parts/existing_preview and
replace the len(preview) > len(existing_preview) check with a guard that returns
unless existing_preview == "" or matches the short-UUID fallback pattern, then
set content.title = _branch_label(sid, preview) as before; keep all other logic
(branch_headers, first_user_text, SessionHeaderMessage assertion, and use of
_branch_label) unchanged.
- Around line 1018-1051: The docstring and implementation of _branch_label
disagree about the truncation: change the implementation to use a
single-character ellipsis and ensure the preview portion is exactly 80 chars
plus that ellipsis by slicing preview[:80] and appending "…" when len(preview) >
80; update the _branch_label docstring to mention the single-character ellipsis
and the 80-char preview cap so both behavior and docs match.
In `@dev-docs/dag.md`:
- Around line 503-510: The taxonomy table duplicates the SessionStart-hook case
across two rows; edit the table so row 1 (currently "The session's actual first
entry") is explicitly limited to "a `user` prompt" only, and remove any mention
of hooks from that row, then make row 4 the sole owner of the SessionStart-hook
shape by keeping its description as "`PassthroughTranscriptEntry`
`type=\"progress\"` from a session-start hook (e.g. `SessionStart:clear`)`" and
clarifying it fires before the first user turn; update the brief rationale cells
accordingly so SessionStart-hook behavior is documented only under the
`PassthroughTranscriptEntry` row and not duplicated in the first row.
In `@test/test_dag.py`:
- Around line 1649-1682: The test
test_deep_passthrough_chain_classified_structural in
TestParallelToolUseViaPassthrough currently implies it exercises Variant 3 (live
continuation through the passthrough) but actually uses a live continuation via
the user child u2 (the structural-side-branch collapse / Shape B); update the
test docstring to explicitly state that it exercises the depth-unboundedness
invariant relied on by Variant 3 (i.e., removal of the 20-depth clamp) rather
than Variant 3 itself, mentioning
test_deep_passthrough_chain_classified_structural,
TestParallelToolUseViaPassthrough and the u2 live continuation to make intent
unambiguous.
In `@test/test_skill_pairing.py`:
- Around line 580-649: The comment in test_parent_message_index_remapped is
misleading because this test exercises the no-shift case (fork at index 2
remains unchanged) so update the comment around the branch assertion
(referencing branch, branch.content.parent_message_index and
_reindex_filtered_context) to explicitly say "no shift is needed in this
scenario; shifting behavior is covered by
test_parent_message_index_remapped_when_fork_shifts" so the regression intent is
unambiguous and readers know where the shifting case is tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b957c7e-b981-4184-a7cc-7a1e6302956f
📒 Files selected for processing (6)
claude_code_log/dag.pyclaude_code_log/markdown/renderer.pyclaude_code_log/renderer.pydev-docs/dag.mdtest/test_dag.pytest/test_skill_pairing.py
Real Claude Code teammate transcripts (CC 2.1.32+) thread parallel
tool_uses through `progress` passthrough callbacks rather than as
direct sibling tool_use blocks. Each parallel turn produces a 2-child
fork at the spawning assistant: the user(tool_result) for the first
parallel call sits beside a passthrough that chains into the next
assistant tool_use. The passthrough subtree carries the live
continuation; the user(tool_result) carries only structural callbacks
(a hook acknowledgement leaf), so it dead-ends. Without intervention,
each parallel turn became a spurious 2-branch fork with the
"trunk continuation" branch's first message filtered out by the nav,
surfacing as ghost "(1 branches)" entries that clutter the index and
nest deeply when several parallel turns fire in sequence.
`_walk_session_with_forks` gains Variant 3, gated on the specific
shape — exactly one passthrough child has a non-structural subtree,
every other sibling has a structural subtree (no user/assistant
descendants). The passthrough sibling becomes the chain continuation;
the dead-end user/assistant siblings are appended inline and their
descendants are collected into `skipped`. Distinct from real user
rewinds, which never include passthrough children, so existing
within-session fork tests continue to pass.
`_is_structural_subtree` had a `max_depth=20` clamp that returned
False ("could be live") for chains exceeding the limit. Real
`progress` chains under a parallel-tool_use anchor regularly run >20
deep, which suppressed the structural-collapse paths. The clamp is
removed; traversal is bounded by the `seen` set and `session_uuids`,
so termination is still guaranteed without the depth cap.
Also classify `progress` passthroughs as expected roots to silence
the noisy "X roots found (Y unexpected)" warning on long-running
compacted sessions. Six known shapes legitimately appear as
parentless roots, including:
- SessionStart hooks firing before the first user turn
(parentUuid:null naturally).
- In-flight tool hooks orphan-promoted by `/compact` (the spawning
tool_use is in the discarded pre-compaction context).
Both shapes are routine async-hook artifacts, not data corruption.
`_EXPECTED_ROOT_PASSTHROUGH_TYPES` mirrors the existing
`_EXPECTED_ROOT_SYSTEM_SUBTYPES` set; the per-orphan log in
`build_dag` is also demoted from WARNING to DEBUG when the orphan is
a progress passthrough — the multi-root classification already
covers them and per-node noise multiplies poorly on long sessions.
dev-docs/dag.md gains:
- A Variant 3 section under Tool-Result Side-Branches (mermaid +
detection criteria + summary-table row).
- An "Expected Root Types" subsection that tabulates all six
legitimate root shapes (the six above plus subagent roots,
which are resolved one layer earlier by
`_integrate_agent_entries`).
test/test_dag.py:
- `TestParallelToolUseViaPassthrough` with two cases (basic shape
+ the >20-deep passthrough chain that exercises the
`_is_structural_subtree` depth-cap removal).
- `test_progress_passthrough_roots_are_expected` covering both
parentUuid:null and orphan-promoted shapes.
On the BCT/clmail/monk fixture (d602eb5f): 5 fork points / 10
branches → 2 fork points / 4 branches (the remaining are real
slash-command rewinds). On the larger d4bea21c fixture: zero DAG
warnings emitted.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cboos
force-pushed
the
dev/fix-single-branch-forks
branch
from
04d6edc to
9ff7eba
Compare
Collaborator
Author
|
Addressed all 8 nitpicks in fixups (autosquashed onto the existing two commits, no new commits): Substantive
Doc / test polish
Skipped
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
claude_code_log/renderer.py (1)
720-775:⚠️ Potential issue | 🟡 MinorStale
branch_idxinjunction_forward_linksafter post-render reindex.
fork_msg.junction_forward_linksis populated at lines 720–775 withbranch_idxvalues pulled fromctx.session_first_message. When detail ≠ FULL, the post-render filter at line 781 calls_reindex_filtered_context, which remapsctx.session_first_messageand (newly)SessionHeaderMessage.parent_message_index, but does not updateTemplateMessage.junction_forward_links.Since
SessionHeaderMessageis never filtered (not in any exclude list), the target header survives—but itsmessage_indexchanges. The storedbranch_idxints become stale, and the template anchor#msg-d-{{ branch_idx }}resolves to the wrong message element, identical to the mismatch this PR fixes forparent_message_index.Two options:
- Move junction-link population to after the post-render filter (requires only final indices and previews).
- Extend
_reindex_filtered_contextto also remap eachTemplateMessage.junction_forward_linkstuple'sbranch_idxthroughindex_remap.🤖 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 720 - 775, The junction_forward_links store stale branch_idx ints because they’re populated before post-render reindexing; fix by updating _reindex_filtered_context to remap TemplateMessage.junction_forward_links using the same index_remap logic applied to SessionHeaderMessage.parent_message_index: iterate ctx.messages (TemplateMessage), for each junction_forward_links entry map branch_idx = index_remap.get(branch_idx) (drop the tuple if missing), replace the tuple with the remapped branch_idx, and after remapping ensure the same “<2 links → clear preview/links” logic is applied so fork_msg.fork_point_preview and junction_forward_links remain consistent with ctx.session_first_message and SessionHeaderMessage.message_index.
🧹 Nitpick comments (1)
claude_code_log/renderer.py (1)
752-762: Avoid hand-stripping the"Branch • "head; have_branch_labelreturn the parts.
link_suffix = link_text[len("Branch • ") :]works today only because every_branch_labelreturn path begins with that exact literal. It's a fragile coupling between the composer and this consumer — a future tweak to the prefix (i18n, an icon, an extra separator) silently breaks the fork-point box.Two cheaper alternatives keep the single-source-of-truth without re-parsing:
- Have
_branch_labelaccept aninclude_prefix: bool = Trueflag and just call it withFalsehere.- Or expose a small
_branch_label_suffix(branch_sid, preview)helper that both_branch_labeland this site call.🤖 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 752 - 762, The code fragilely hand-strips the "Branch • " prefix from link_text (link_suffix = link_text[len("Branch • "):]) which couples this consumer to _branch_label's exact output; modify _branch_label to either accept an include_prefix: bool = True parameter (call it with include_prefix=False here) or add a small helper _branch_label_suffix(branch_sid, preview_text) that returns only the suffix, then replace the slicing logic and pass the suffix into fork_msg.junction_forward_links (reference symbols: _branch_label, link_text, link_suffix, fork_msg.junction_forward_links) so the prefix is single-source-of-truth and not re-parsed.
🤖 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/markdown/renderer.py`:
- Around line 442-456: The branch headers currently render distinct titles
(content.is_branch / content.title) but their Markdown anchor still uses the
trunk-derived session_id[:8] in format_SessionHeaderMessage(), causing duplicate
anchors; change the anchor generation in format_SessionHeaderMessage() to use a
branch-unique key (for example the full content.session_id or content.session_id
plus a branch suffix) instead of session_id[:8], and update _generate_toc() to
consume and link to that same branch-unique anchor key so TOC/backlinks target
the correct branch heading; ensure any helper that composes the visible label
(e.g. _branch_label or _inline_code usage) remains unchanged while the anchor
slug uses the new unique identifier.
In `@claude_code_log/renderer.py`:
- Around line 962-1022: _enrich_branch_titles currently lifts the first
UserTextMessage by matching msg.render_session_id, but this can pick up
sidechain/sub-agent user prompts that inherit the parent session id; inside the
inner loop that builds first_user_text (the for msg in ctx.messages block that
checks isinstance(msg.content, UserTextMessage) and uses msg.render_session_id),
add a guard to skip messages that are sidechain/sub-agent entries (e.g. detect
and skip when the message/content has a sidechain/agent-parent marker such as
content.agent_parent_session, content.sidechain_parent, msg.is_sidechain or the
actual property your model uses for sub-agent wrapping) so only true
branch-local human user texts are considered; keep the rest of the logic
unchanged (use the same symbols: _enrich_branch_titles, branch_headers,
first_user_text, render_session_id, UserTextMessage).
---
Outside diff comments:
In `@claude_code_log/renderer.py`:
- Around line 720-775: The junction_forward_links store stale branch_idx ints
because they’re populated before post-render reindexing; fix by updating
_reindex_filtered_context to remap TemplateMessage.junction_forward_links using
the same index_remap logic applied to SessionHeaderMessage.parent_message_index:
iterate ctx.messages (TemplateMessage), for each junction_forward_links entry
map branch_idx = index_remap.get(branch_idx) (drop the tuple if missing),
replace the tuple with the remapped branch_idx, and after remapping ensure the
same “<2 links → clear preview/links” logic is applied so
fork_msg.fork_point_preview and junction_forward_links remain consistent with
ctx.session_first_message and SessionHeaderMessage.message_index.
---
Nitpick comments:
In `@claude_code_log/renderer.py`:
- Around line 752-762: The code fragilely hand-strips the "Branch • " prefix
from link_text (link_suffix = link_text[len("Branch • "):]) which couples this
consumer to _branch_label's exact output; modify _branch_label to either accept
an include_prefix: bool = True parameter (call it with include_prefix=False
here) or add a small helper _branch_label_suffix(branch_sid, preview_text) that
returns only the suffix, then replace the slicing logic and pass the suffix into
fork_msg.junction_forward_links (reference symbols: _branch_label, link_text,
link_suffix, fork_msg.junction_forward_links) so the prefix is
single-source-of-truth and not re-parsed.
🪄 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: f9580c72-81f3-4c4b-81c7-9e8bbac9f097
📒 Files selected for processing (7)
claude_code_log/dag.pyclaude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/renderer.pydev-docs/dag.mdtest/test_dag.pytest/test_skill_pairing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test_skill_pairing.py
cboos
force-pushed
the
dev/fix-single-branch-forks
branch
from
9ff7eba to
8fb608f
Compare
Collaborator
Author
|
Second pass — addressed all 4 follow-up concerns. Squashed into the existing renderer commit. Major (markdown duplicate anchors) Minor (sidechain pickup in Outside-diff Potential issue (junction_forward_links stale after re-reindex) Nitpick (hand-strip prefix is fragile)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
claude_code_log/markdown/renderer.py (2)
1289-1298: TOC branch-fallback label format diverges from the heading format.When
first_user_messageis missing, the TOC fallback emitsBranch `<uuid8>`while the corresponding header (viatitle_SessionHeaderMessage+_branch_label) usesBranch • <uuid8> • <preview>with bullet separators rather than backticks. In normal operationfirst_user_messageshould always be populated byprepare_session_navigation, so this fallback is unlikely to trigger — but if it does, the TOC entry will look stylistically distinct from the body heading.Minor — consider aligning the fallback string to
f"Branch • {session_id.split('@')[-1][:8]}"to match the_branch_labelshape, or simply assertingfirst_user_messageis always set for branches and dropping the fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/markdown/renderer.py` around lines 1289 - 1298, The TOC fallback for branch sessions uses backticks and a different separator, causing mismatch with the header format; update the fallback in renderer.py where label is computed (using session.get("first_user_message", ...)) to produce the same shape as _branch_label/title_SessionHeaderMessage by replacing the fallback string with something like f"Branch • {session_id.split('@')[-1][:8]}" so the TOC entry matches the body header, or alternatively remove the fallback and assert prepare_session_navigation always sets first_user_message for branches.
155-178: LGTM — branch-aware anchor scheme resolves the duplicate-anchor issue.
_session_anchorcorrectly differentiates trunk (session-<uuid8>) from branch (branch-<uuid8>derived from the deepest@segment) anchors, which directly addresses the prior duplicate-anchor TOC collision. Mirroring the visibleBranch • <uuid8>label in the anchor key is a nice touch for reader correlation.One minor note: the parameter is typed
Anyto accept either a string or aSessionHeaderMessage-like object. Consider tightening this toSessionHeaderMessage | strfor clearer typing, though the duck-typed contract is well documented in the docstring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/markdown/renderer.py` around lines 155 - 178, The parameter type for _session_anchor is too broad (Any); change it to a union of the explicit expected types (SessionHeaderMessage | str) so callers and type-checkers know it accepts either a raw session id or an object exposing session_id; update the function signature to use that union type and import or reference the SessionHeaderMessage type where needed so the duck-typed contract in the docstring is enforced by static typing.
🤖 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/markdown/renderer.py`:
- Around line 481-485: When content.is_branch is True but content.title is
empty, avoid falling through to the trunk header; instead generate a
branch-aware fallback title so the visible prefix matches the anchor. Modify the
branch block (where content.is_branch is checked) to detect empty content.title
and build a fallback like "🌿 Branch <id-or-deepest>" by deriving the same
identifier used by the anchor (use the existing _session_anchor(content) or the
same extraction logic used there) and format team name with
_inline_code(content.team_name) as currently done; ensure this branch-only path
returns the constructed title so heading and anchor remain consistent.
---
Nitpick comments:
In `@claude_code_log/markdown/renderer.py`:
- Around line 1289-1298: The TOC fallback for branch sessions uses backticks and
a different separator, causing mismatch with the header format; update the
fallback in renderer.py where label is computed (using
session.get("first_user_message", ...)) to produce the same shape as
_branch_label/title_SessionHeaderMessage by replacing the fallback string with
something like f"Branch • {session_id.split('@')[-1][:8]}" so the TOC entry
matches the body header, or alternatively remove the fallback and assert
prepare_session_navigation always sets first_user_message for branches.
- Around line 155-178: The parameter type for _session_anchor is too broad
(Any); change it to a union of the explicit expected types (SessionHeaderMessage
| str) so callers and type-checkers know it accepts either a raw session id or
an object exposing session_id; update the function signature to use that union
type and import or reference the SessionHeaderMessage type where needed so the
duck-typed contract in the docstring is enforced by static typing.
🪄 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: c04b1ab7-1387-4d6f-8867-f48a83fd3efc
📒 Files selected for processing (4)
claude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/renderer.pytest/test_skill_pairing.py
🚧 Files skipped from review as they are similar to previous changes (3)
- claude_code_log/models.py
- test/test_skill_pairing.py
- claude_code_log/renderer.py
cboos
force-pushed
the
dev/fix-single-branch-forks
branch
from
8fb608f to
b775dd6
Compare
Collaborator
Author
|
Third pass — addressed all three concerns from the latest review (1 actionable + 2 nitpicks). Squashed into the existing renderer commit; force-pushed. Actionable: defensive branch heading when Nit 1: TOC fallback format mismatch Nit 2: tighten
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
claude_code_log/markdown/renderer.py (1)
155-178: Consider importing_branch_short_uuidfrom the parent renderer module to maintain a single source of truth.The PR introduces
_branch_short_uuidinclaude_code_log/renderer.pyas the authoritative implementation, butclaude_code_log/markdown/renderer.pystill duplicatessplit("@")[-1][:8]at lines 176, 493, and 1313. Importing and using_branch_short_uuidhere would prevent drift if the suffix length or splitting rule ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/markdown/renderer.py` around lines 155 - 178, The _session_anchor function duplicates branch-id extraction logic; import the canonical helper _branch_short_uuid from the parent renderer module and use it instead of inline split("@")[-1][:8] so all branch-short logic is centralized; update _session_anchor to call _branch_short_uuid(sid) when "@" in sid and replace the other duplicated occurrences that compute branch_short (the same split/slice pattern) to call _branch_short_uuid as well, preserving the current behavior of accepting either a raw session id string or a SessionHeaderMessage object.
🤖 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/markdown/renderer.py`:
- Around line 155-178: The _session_anchor function duplicates branch-id
extraction logic; import the canonical helper _branch_short_uuid from the parent
renderer module and use it instead of inline split("@")[-1][:8] so all
branch-short logic is centralized; update _session_anchor to call
_branch_short_uuid(sid) when "@" in sid and replace the other duplicated
occurrences that compute branch_short (the same split/slice pattern) to call
_branch_short_uuid as well, preserving the current behavior of accepting either
a raw session id string or a SessionHeaderMessage object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93e4a476-918d-4991-b85a-d3113a1e57e1
📒 Files selected for processing (4)
claude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/renderer.pytest/test_skill_pairing.py
🚧 Files skipped from review as they are similar to previous changes (3)
- claude_code_log/models.py
- test/test_skill_pairing.py
- claude_code_log/renderer.py
Two related issues with within-session fork rendering surfaced after
the DAG-level fork-elision pass.
1. Stale fork-point backlinks after `_pair_skill_tool_uses`
`_pair_skill_tool_uses` folds `isMeta=True` slash-command bodies
into their spawning Skill `tool_use` and drops the now-redundant
slash-command + "Launching skill" tool_result via
`_reindex_filtered_context`. The reindex rewrote `ctx.messages`,
remapped each TemplateMessage's `message_index`, and fixed up
`ctx.session_first_message` — but the cached `parent_message_index`
field on `SessionHeaderMessage` instances created earlier in
`_render_messages` was never remapped. The "from ⑂ Fork point"
backlink in the body branch header therefore jumped to whatever
message ended up at the *old* index after the reindex shift,
manifesting as e.g. `Branch • c36e76a6 from #msg-d-510` when the
fork point was actually d-496. `_reindex_filtered_context` now
walks the filtered list and remaps every branch / child
SessionHeaderMessage's `parent_message_index` through `index_remap`.
Regression tests in `TestReindexBranchBackrefs`
(`test/test_skill_pairing.py`):
- The unshifted case: dropping later messages must not move a
branch backlink off its fork-point uuid.
- The shifted case: dropping an *earlier* message moves the
fork-point's index, and the branch backlink must follow the
shift.
2. Inconsistent `Branch • <uuid8> • <preview>` labels
The same logical branch was labelled differently in three places:
the body branch-header carried `Branch • <preview>` (no uuid) when
the first user message was extractable, else `Branch • <uuid8>`;
the session/graph index carried `<preview>` (no uuid, no "Branch")
when a UserTextMessage existed in the branch, else `Branch <uuid8>`;
the fork-point box stripped `Branch • <uuid8>` if the title fell
back to UUID-only on either side. Real branches frequently mixed
"rich preview" and "UUID-only" siblings, so the same fork looked
different at the top vs. inline.
Unify on `Branch • <uuid8> • <preview>` (or `Branch • <uuid8>`
when no preview is available). New helpers `_branch_short_uuid`
and `_branch_label` are the single source of truth; called from
the body title, the index nav `first_user_message`, and the
fork-point box link composition. The fork-point indicator's
"≥ 2 non-empty previews" elision is relaxed to "≥ 2 navigable
branches" — the DAG-level Variant 3 fix already collapses
single-branch shells, so this layer can surface the indicator
unconditionally when a real fork remains.
`_enrich_branch_titles(ctx)` is a new post-pass running after
`_pair_skill_tool_uses`. `_render_messages` composes the branch
title from the branch's literal first transcript entry, which
fails when a branch starts with an assistant turn (a "No response
requested." after `/exit`) or with a tool_result — the user
content arrives later. The post-pass scans the final ctx for the
first UserTextMessage in each branch and re-labels the header
when a richer preview exists. Idempotent: never overwrites a
preview already richer than what UserTextMessage scanning yields.
Result: the same `Branch • <uuid8> • <preview>` string appears in
the index, the body header, and the fork-point box. Every backlink
points at the actual fork-point message rather than a stale index
shadow.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cboos
force-pushed
the
dev/fix-single-branch-forks
branch
from
b775dd6 to
bcf658e
Compare
Collaborator
Author
|
Promoted Updated docstring on
|
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/markdown/renderer.py`:
- Around line 1292-1321: The TOC generation incorrectly uses
_session_anchor(session_id) for compaction landmarks, producing anchors like
"session-compact-…" that don't match the actual landmark ids; change the logic
in the loop that computes anchor (where _session_anchor(session_id) is called)
to detect compaction landmarks (e.g., session_id.startswith("compact-") or a
session flag if available) and for those set anchor to the actual compact id
(the raw session_id like "compact-<n>") instead of calling _session_anchor; keep
using _session_anchor(session_id) for normal sessions so other links remain
unchanged.
🪄 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: 8e0db1c3-545a-4ac1-a05f-651c7faec2ca
📒 Files selected for processing (4)
claude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/renderer.pytest/test_skill_pairing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test_skill_pairing.py
Compact-point nav items (``is_compaction_point=True``, populated by
``prepare_session_navigation``) carry an ``id`` shaped like
``"compact-{message_index}"`` — e.g. ``compact-42``, ``compact-77``,
``compact-115`` for a session with three ``/compact`` runs. The HTML
nav links each one to its ``msg-d-{N}`` anchor and lands at the
specific compact summary; Markdown only has session-level
``<a id="…">`` anchors and compact summary messages don't emit one of
their own.
Worse, when ``_generate_toc`` routed a compact item through
``_session_anchor``, the ``[:8]`` slice of every ``compact-{N}``
collapsed to the literal ``"compact-"`` — so a long compacted session
emitted multiple TOC links all pointing at the same malformed
``#session-compact-`` anchor target (one per compact, all duplicated,
none resolving anywhere).
Skip them in the Markdown TOC alongside fork-point items, which were
already filtered for the same reason. Same shape, same fix; updated
the comment to call out both.
The d4bea21c-... fixture (a long-running session with nine ``/compact``
runs) was the trigger — its Markdown TOC was emitting nine duplicate
broken links. After this change the Markdown TOC only surfaces
session-level entries (trunk + branch headers), which all have real
anchor targets emitted by ``format_SessionHeaderMessage``.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Added a new commit on top (953fc55) for the Markdown compact-point duplication you flagged: The compact-point nav items in Skipped them in Branch is now 3 commits:
|
cboos
added a commit
that referenced
this pull request
Apr 27, 2026
…h ghosting Surfaced during the PR #132 review pass: every "drop messages and reindex" pass that adds new index-bearing fields creates a fresh "remember to remap X" trap. The async-agents PR hit three in succession (pair refs, session_nav, ancestry/backlinks) before switching to ghosting. PR #131 added a fourth remap target (`SessionHeaderMessage.parent_message_index`). The note records the architectural assessment of generalizing ghosting to the two existing `_reindex_filtered_context` callers (`_pair_skill_tool_uses` — easy, same shape as the async-agents fix; `_filter_template_by_detail` — medium refactor, needs tree-build child grafting + pair-id skip + render-loop elision flag), and proposes a migration path that ends with deleting `_reindex_filtered_context` entirely. Not work for this PR — captured here so the design rationale is preserved when someone (likely Claude later) picks it up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cboos
added a commit
that referenced
this pull request
Apr 27, 2026
* async-agents: typed models + parsers for TaskOutput / TaskNotification Phase 1 of dev/async-agents (issue #90). Adds the data layer underneath the upcoming rendering work; nothing visible changes yet. ## What's new - ``TaskOutputInput`` (Pydantic): typed input for the ``TaskOutput`` polling tool — ``task_id`` / ``block`` / ``timeout``. - ``TaskOutputResult`` (dataclass): the parsed XML-tagged tool result — ``retrieval_status``, ``task_id``, ``task_type``, ``status``, plus a flag + path captured from the ``[Truncated. Full output: …]`` marker. The bulky ``<output>`` snapshot itself is **not** kept; the agent's full transcript already lands inline as a sidechain in our rendering, and the completion result reaches the trunk via the ``<task-notification>`` user entry. - ``TaskNotificationUsage`` (dataclass): ``total_tokens`` / ``tool_uses`` / ``duration_ms``. - ``TaskNotificationMessage`` (MessageContent): typed shape for the User entry Claude Code injects on async-agent completion. Mirrors ``TeammateMessage``'s data-layer shape: fields for ``task_id`` / ``status`` / ``summary``, the ``<result>`` body (markdown), the parsed ``<usage>``, and the trailing ``Full transcript available at:`` path. ## Parsers - ``factories/tool_factory.py``: register ``TOOL_INPUT_MODELS["TaskOutput"] = TaskOutputInput`` and ``TOOL_OUTPUT_PARSERS["TaskOutput"] = parse_taskoutput_output``. The parser captures the four metadata tags + truncation marker; malformed payloads return ``None`` so the generic raw fallback keeps the visible content. - ``factories/task_notification_factory.py`` (new): mirror of ``teammate_factory`` — ``has_task_notification`` for the cheap detector, ``create_task_notification_message`` for the typed payload. Single-tag fields, ``<result>`` block, ``<usage>`` key:value lines, trailing transcript path. Returns ``None`` for empty / malformed payloads so the User card falls back to its default text rendering. - ``factories/user_factory.create_user_message``: hook ``create_task_notification_message`` ahead of the default text path, right after the teammate detection. ``UserMessageContent`` union extended. ## Plan / tracker - ``work/async-agents.md`` carries the full 4-phase plan (this is Phase 1) plus data-shape notes verified against the test fixture ``d602eb5f-…/.jsonl`` from the clmail-monk session in #90. ## Verified - All 1040 unit tests still pass; pyright + ruff clean. - End-to-end on the real fixture: 4 ``TaskNotificationMessage`` and 3 ``TaskOutput`` tool_use/tool_result pairs are parsed into the typed shapes (rendering still falls back to the generic formatters until Phase 2 lands). Refs: #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: HTML + Markdown rendering for TaskOutput / TaskNotification Phase 2 of dev/async-agents (issue #90). Wires Phase 1's typed models into the renderers; the user-visible cards now look right (without the Phase 3 dedup / fold-into-anchor work yet). ## HTML New ``html/async_formatter.py`` module (mirrors ``teammate_formatter`` style): - ``format_taskoutput_input`` / ``format_taskoutput_output`` — minimal cards with ``<dl class="teammate-tool-card task-output-card">``. The result card shows ``retrieval_status`` / ``task_type`` / ``status`` + a transcript-path hint when truncation was reported, and deliberately drops the bulky ``<output>`` snapshot (the agent's full transcript already lands inline as a sidechain). - ``format_task_notification_content`` — metadata ``<dl class="task-notification-card">`` (task_id, status pill, usage fields, transcript path) + ``render_markdown_collapsible`` for the ``<result>`` body. ``HtmlRenderer`` adds the dispatch methods + the title formatters: - ``title_TaskOutputInput`` → ``🔍 TaskOutput #<task_id>`` (the ``🔍`` short-circuits the template's default ``🛠️`` and reads as "look up / inspect" — distinct from the spawning ``🔧 Task``). - ``title_TaskNotificationMessage`` → ``🔄 Async result • <summary>``. - ``title_TaskInput`` extended: when ``run_in_background=True``, appends a muted ``[async]`` hint so the reader can tell async spawns from sync ones at a glance. CSS additions in ``teammate_styles.css``: - ``.task-async-hint`` (blue, muted) for the ``[async]`` title tag. - ``.task-output-card`` (cyan border). - ``.task-notification-card`` (blue border). ## Markdown Mirrors the HTML in ``markdown/renderer.py``: - ``format_TaskOutputInput`` / ``format_TaskOutputResult`` — terse ``key:value`` lines, transcript path appended. - ``format_TaskNotificationMessage`` — bulleted metadata + a ``<details><summary>Result</summary>`` block carrying the result Markdown. - ``title_TaskInput`` extended with ``*[async]*`` italic hint when ``run_in_background=True``. - ``title_TaskOutputInput`` and ``title_TaskNotificationMessage`` parallel the HTML titles. ## Verified on the clmail-monk fixture Rendering the test JSONL produces: - 8 ``[async]`` hints on Task tool_use titles (every async spawn). - 4 ``🔄 Async result`` notifications. - 3 ``🔍 TaskOutput`` polling cards. All 1040 unit tests + 5 snapshot fixtures green; pyright + ruff clean. Refs: #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: dedup notification bodies + spawn-backlink anchor Phase 3 of dev/async-agents (issue #90). The async-agent flow ends with a User entry whose ``<task- notification>`` ``<result>`` body duplicates the spawning Task's last sidechain sub-assistant — the agent's actual answer is rendered twice (once inline as the spawn's sidechain content, once below in the notification card). This commit collapses the second copy to a backlink-only stub. ## What's new - ``TaskNotificationMessage`` gains two fields: - ``result_is_duplicate: bool`` — set when the renderer pass confirms the body matches the spawning sidechain content. - ``spawning_task_message_index: Optional[int]`` — the message index of the spawning Task's tool_use, used as a backlink anchor for the reader to navigate to the actual spawn. - New ``_link_async_notifications`` pass in ``renderer.py``, scheduled after ``_populate_task_metadata`` (post tree-build): - Indexes notifications by ``task_id``. - Walks Task/Agent tool_results, extracting the async-agent's ``agent_id`` via ``_async_agent_id_from_tool_result`` (preferring ``TaskOutput.metadata.agent_id`` set by ``parse_agent_result_metadata``, then ``TaskOutput.agent_id``, then a regex fallback on the raw text). - For each match: wires the spawning Task's ``pair_first`` (tool_use index) as the backlink anchor, then walks the tool_result's descendants in document order via ``_last_sidechain_assistant_text`` and compares with ``_normalize_for_dedup`` against the notification's ``result_text``. On match, flags ``result_is_duplicate``. - ``format_task_notification_content`` (HTML) renders a ``Spawn: ↱ Task`` row with a ``<a class="task-notification-backlink" href="#msg-d-N">`` anchor, and elides the markdown body when the flag is set. - ``format_TaskNotificationMessage`` (Markdown) mirrors: ``- **Spawn:** ↱ Task `#d-N``` line + body suppression on dup. - New ``.task-notification-backlink`` CSS rule (blue, no underline, underline on hover). ## Verified on the clmail-monk fixture All 4 notifications (a8b740b / a5de609 / a9d6832 / a70b9c2) match their spawning Task's last sidechain sub-assistant (after fixing a walk-order bug in the descendant traversal — the naive ``stack.pop()`` after ``stack.extend(children)`` reverses document order; corrected to push reversed). Each notification card now ends with ``Spawn: ↱ Task`` linked to the spawn's ``msg-d-N``, and the duplicated markdown body is gone. Test count holds at 1065; pyright + ruff clean. Refs: #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: place task_notification at level 3 in the hierarchy Main flagged that the rendered async-agents output didn't look right on their side. Rebasing onto current main (PR #119, user-content markdown rendering) didn't change the rendering, but spotting the issue did: ``TaskNotificationMessage`` is a User entry, but ``_get_message_hierarchy_level`` had no explicit branch for the ``"task_notification"`` type. It fell through to the default level 1, and since the next conversation turn is an assistant (level 2), the assistant ended up nested as a *child* of the notification — the notification claimed every subsequent turn as its descendant ("1 assistant + 3 tools" hanging off d-118 in the test fixture). Conceptually the notification is more like a tool_result: it's a delayed status update for work the previous assistant initiated, not a new user turn that the next assistant is responding to. Place it at level 3 explicitly: - Pops anything ≥ 3 from the stack but keeps the level-2 spawning assistant on top. - Sits as a sibling of the spawning assistant's tool_use/tool_result entries. - Subsequent level-2 assistants pop the notification (≥ 2) and start a fresh turn — siblings of the spawning assistant, not descendants of the notification. Verified on the clmail-monk fixture: d-118 (a8b740b notification) now renders with zero descendants, and the following assistant d-119 becomes its sibling under their shared parent d-11. Doc: ``dev-docs/FOLD_STATE_DIAGRAM.md`` level table updated to call out ``task_notification`` alongside the other Level-3 types. Refs: #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: fold last sub-assistant into spawning Task tool_result Main flagged that Phase 3 only got the *notification* dedup right — the spawning Task tool_result still showed only "Async agent launched successfully", and the agent's actual answer remained buried at the tail of the relocated sidechain. The reader had to scroll past the agent's working tools (Reads, Bash) to find the final summary. This commit closes the gap: the agent's last sub-assistant content folds *into* the spawning Task's tool_result rendering, with the matching sidechain message removed so the answer appears exactly once at the natural reading position. ## Mechanics - ``TaskOutput`` gains ``async_final_answer: Optional[str]``. Populated by ``_link_async_notifications`` on every async-agent match; ``None`` for sync Tasks. - ``_last_sidechain_assistant`` (replaces ``…_text``) returns ``(msg, parent, index)`` so the caller can both inspect the text and ``del parent.children[index]`` — same pattern ``_cleanup_sidechain_duplicates`` uses for sync Tasks. - ``_link_async_notifications`` now does three things on each match: copies the answer onto ``TaskOutput.async_final_answer``, drops the duplicate sub-assistant from the sidechain tree, and flags the notification body as duplicate (with the spawn backlink). Three views — spawn / sidechain / notification — converge on a single visible copy at the spawn. - ``format_task_output`` (HTML) renders the folded answer as a ``<div class="task-async-answer-label">Result <small>(from async notification)</small></div>`` followed by the answer in a ``render_markdown_collapsible`` block. - ``format_TaskOutput`` (Markdown) appends a second ``<details><summary>Result (from async notification)</summary>`` block. - New ``.task-async-answer-label`` CSS rule. ## Verified on the clmail-monk fixture All 4 async Task tool_results (a8b740b / a5de609 / a9d6832 / a70b9c2) now render: - their original "Async agent launched successfully" stub, - followed by the "Result (from async notification)" fold, - followed by the collapsible agent answer. Sub-assistant count drops by 4 (the duplicates removed from the sidechain trees). Notification cards stay backlink-only. Refs: #90. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: stop tool_result negative-margin from yanking the async-answer fold over its label The .tool_result .collapsible-code rule applies margin-top:-2.5em to tuck the *first* collapsible under the tool title. Inside an async-task fold, the second collapsible (.task-async-answer) was also caught by it and overlapped the "Result (from async notification)" label. Reset the margin only on .task-async-answer .collapsible-code; the first .task- result collapsible keeps its tucked-up alignment. Snapshots refreshed for the CSS bump. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: Phase 4 — tests + minimal sliced fixture Adds a synthetic fixture sliced down from the canonical clmail-monk session (`d602eb5f-…`) and a focused test module exercising the new async-agents pipeline end-to-end: - `test/test_data/async_agents/` — 7-entry main session + 3-entry sidechain. Covers the four pieces the renderer has to handle: `Task` with `run_in_background=true`, the canonical `Async agent launched successfully\nagentId: …` tool_result, a `TaskOutput` poll with `<retrieval_status>/<task_id>/<output> [Truncated…]` shape, and a `<task-notification>` whose `<result>` matches the last sub-assistant verbatim. - `test/test_async_agents.py` — 25 tests: * `has_task_notification` and `create_task_notification_message` parser coverage (positive + edge cases: empty, missing usage, partial usage). * `parse_taskoutput_output` coverage (full payload, in-progress status without `<output>`, non-TaskOutput rejection). * Dispatch-table assertions (defensive against accidental churn). * Fixture loading and factory dispatch. * Phase 3 rendering pipeline assertions: - notification flagged `result_is_duplicate=True` - `TaskOutput.async_final_answer` populated on the spawning Task - duplicate sub-assistant dropped from the rendered tree - the agent's final answer text appears exactly once across the entire tree (folded under the spawn). - Snapshot coverage in both `test_snapshot_html.py` and `test_snapshot_markdown.py` for the new fixture, locking in: the `*[async]*` hint badge on the Task title, the ``Async agent launched successfully`` stub, the ``Result (from async notification)`` fold, the TaskOutput poll card, and the notification-collapsed-to-backlink stub. Verified: 1115 unit tests pass, pyright + ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: source the spawn-fold from the notification (Plan A) The Phase 3 fold disappeared at `--detail low`: the linker walked the sidechain looking for the last sub-assistant to fold, but `_filter_by_detail` had already dropped sidechain entries pre-render. With nothing to match against, the fold was skipped and the agent's final answer was buried in the (now-collapsed) `<task-notification>` card. Plan A splits the pass: - **Spawn-fold (FULL/HIGH/LOW):** when a notification's `task_id` matches the spawning Task tool_result's `agent_id`, fold the notification's `result_text` directly onto `TaskOutput.async_final_answer` and flag the notification `result_is_duplicate`. Sidechain text is no longer required for the fold itself — the notification body is the canonical source. - **Sidechain dedup (FULL/HIGH only):** when the last sub-assistant text matches the notification's `result_text`, drop it from the tree. This is the only piece that needs the sidechain — at LOW the sidechain is gone and there's nothing to remove anyway. - **MINIMAL/USER_ONLY:** the spawn fold is skipped (the spawning Task tool_result is dropped post-render — there's nothing to fold onto). The notification card retains its body so the agent's answer stays visible. `_link_async_notifications` now takes the active `DetailLevel` so it can decide whether the spawn target survives. Tests: - 5 new parametrized cases in `TestAsyncAgentsDetailLevels` cover FULL/HIGH/LOW (fold present + notification flagged duplicate) and MINIMAL/USER_ONLY (no fold + notification body kept visible). - New `test_async_agents_fixture_html_low` snapshot locks in the rendered LOW shape — guard against silent regressions to the fold pipeline at that detail level. Verified: 1121 unit tests pass, pyright + ruff clean. Fold count on the canonical clmail-monk fixture: 4 at FULL/HIGH/LOW (was 0 at LOW before the fix), 0 at MINIMAL/USER_ONLY (notification body is the surviving copy at those levels). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: drop duplicate-flagged notifications at --detail low At LOW the spawn-fold survives (Plan A), so the standalone `<task-notification>` card duplicates the answer the reader is already seeing folded under the spawning Task tool_result. FULL/HIGH keep the card for transcript fidelity; MINIMAL/USER_ONLY don't flag duplicates (the Task tool_result is filtered there, so the notification body is the surviving copy). Implementation note: main's mail #2631 suggested adding the rule inside `_filter_template_by_detail`. That filter runs before `_link_async_notifications` in the rendering pipeline (line 754 vs 826 in renderer.py), so `result_is_duplicate` is still False when the filter visits each message. Instead, a small post-link pass `_drop_duplicate_notifications_at_low` runs right after the linker when `detail == LOW`. Survivors are remapped via the existing `_reindex_filtered_context`; tree children are pruned so the notification doesn't linger as a sub-message of its parent. Tests: - New `test_duplicate_notification_dropped_at_low` confirms the notification is gone from `ctx.messages` at LOW. - New `test_notification_flagged_duplicate_at_full_and_high` keeps the assertions previously bundled into the LOW test (notification remains in ctx, flagged duplicate, with backlink wired). - Existing detail-level parametrized cases retained. Snapshot: `test_async_agents_fixture_html_low` regenerated to lock in the new LOW shape — fold visible at the spawn, no notification card. Verified: 1123 unit tests pass, pyright + ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: register TaskNotificationMessage in CSS_CLASS_REGISTRY The runtime message-type filter in transcript.html (line 484) hides any message whose CSS classes don't include one of the toolbar's known types: `user, system, assistant, thinking, tool_use, tool_result, sidechain, image`. `TaskNotificationMessage` was missing from `CSS_CLASS_REGISTRY`, so `css_class_from_message` fell back to bare `msg.type` = `task_notification` — matching no toolbar type, hence permanently flagged `filtered-hidden` even when "All filters" was active. Register the content type with the same shape as the other user- variant entries (`UserSteeringMessage: ["user", "steering"]`, `TeammateMessage: ["user", "teammate"]`, etc.). The notification's underlying JSONL entry is a plain User message — Claude Code injects it as `type: "user"` with the `<task-notification>` block in `message.content` — so the User toggle controlling its visibility is the natural mapping. Snapshot impact: the FULL async-agents fixture's notification div class string changes from `task_notification` to `user task_notification` (single-line diff). The LOW snapshot is unchanged because the duplicate notification is dropped pre-template by `_drop_duplicate_notifications_at_low`. Verified: 1123 unit tests pass, pyright + ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: re-pair messages after dropping duplicate notifications at LOW `_drop_duplicate_notifications_at_low` calls `_reindex_filtered_context` to remap message_index after removing the dropped notifications, but that helper *clears* every message's pair_first/pair_middle/pair_last on the assumption the caller will re-run pair identification. `_identify_message_pairs` runs at renderer.py:775 — before `_link_async_notifications` (826) and the drop pass (829) — so nothing re-establishes the pairs the helper just cleared. This broke two LOW-only behaviors: - **Markdown LOW**: tool_use renders Instructions but not Report or the async-fold "Result (from async notification)" body. The Markdown renderer's `_render_message` only emits a tool_result body when its tool_use is `is_first_in_pair` (renderer.py:1417); without `pair_last` set, the body is dropped, and the tool_result has no `title_ToolResultMessage` to render itself. - **HTML LOW**: Task tool_use and tool_result render with a visual gap because the `pair_first`/`pair_last` CSS classes that flush adjacent cards together are absent. Fix: re-run `_identify_message_pairs(ctx.messages)` immediately after `_reindex_filtered_context` in the drop pass. Pairs are reconstructed from scratch via the standard two-pass algorithm. Verified on the canonical clmail-monk fixture at LOW: 7 `pair_first`/`pair_last` HTML divs (Task tool_use ↔ tool_result), Markdown shows Instructions + Report + "Result (from async notification)" for every async Task. The LOW snapshot diff is limited to two added pair classes on the synthetic fixture's Task tool_use and tool_result. Verified: 1123 unit tests pass, pyright + ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: drop unused type-ignore comments in test helpers `ty check` flagged four `# type: ignore[no-untyped-def]` comments in test/test_async_agents.py as unused — the inferred parameter types already satisfy ty's checks. Carried over from when I sketched the helpers without type annotations; no longer needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: document the async-task-agent flow + clarify TaskOutput naming - New `dev-docs/agents.md` covers all three flavors of Task-spawned agents (sync sub-agents #79, async task agents #90, teammates #91). The async-agents § is the new detail: pipeline shape diagram, the two `TaskOutput`-named dataclasses (`TaskOutput` on the Task tool_result vs `TaskOutputResult` on the polling tool), the Phase 3 fold mechanics, the per-detail-level visibility matrix, key files, and the test fixture pointer. - `dev-docs/messages.md` gains: - A `TaskOutput` polling-tool row in the Tool Results table and the Available Tools matrix (was previously absent). - A note on the `TaskOutput` vs `TaskOutputResult` name collision, forwarding to agents.md § 2.2. - A new "Async Task Notification" subsection under user content documenting `TaskNotificationMessage` (Phase 3 dedup markers included), forwarding to agents.md § 2 for the end-to-end flow. - `dev-docs/teammates.md` § 10.1 ("Standard sub-agents and async task agents") now reflects that #90 is shipped — points readers at agents.md § 2 for the as-built reference. The doc's top-of- file companion-doc list and References block both gain agents.md. Surfaced by user feedback on PR #132: the `TaskOutput` ↔ `TaskOutputResult` name collision is genuinely confusing; documenting the distinction in the canonical message-type reference + a focused agents.md should keep future readers (and Claude) on track. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: only fold + flag duplicate when fold target is a TaskOutput CodeRabbit review on PR #132: `_link_async_notifications` set `notification.result_is_duplicate = True` whenever a Task tool_result matched a notification by `agent_id`, even when `content.output` was not a parsed `TaskOutput`. `_async_agent_id_from_tool_result` has three pathways — the third (regex fallback on raw text) supports shapes the parser couldn't structure into a `TaskOutput`. On those shapes we'd skip the actual fold (no `async_final_answer` field to write into) but still suppress the notification body, silently losing the agent's only visible answer. Move the duplicate flag inside the `isinstance(content.output, TaskOutput)` guard. The notification body now stays visible whenever the fold can't land — preserving "answer visible at least once" at every detail level. Also rebuild `session_nav` after `_drop_duplicate_notifications_at_low` runs at LOW. The drop pass remaps `ctx.session_first_message` indices, but `session_nav` was built earlier with the pre-drop indices baked into its `message_index` and `parent_message_index` fields. The single-session canonical fixture didn't surface this (notifications sit after the only session header, so dropping them doesn't shift the header), but a multi-session transcript with notifications between session headers would land nav anchors one (or more) message slot off after the LOW reindex. Reuses the same `prepare_session_navigation` call with the up-to-date ctx. Verified: 1123 unit tests pass, pyright + ruff + ty clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: rename CSS modifier to task-notification, surface in timeline CodeRabbit review on PR #132 flagged two paired issues: - Naming convention: every other modifier in `CSS_CLASS_REGISTRY` uses hyphens (`system-hook`, `slash-command`, `command-output`, `bash-input`, `bash-output`); the actual CSS rules in `teammate_styles.css` also use hyphens (`task-notification-card`, `task-notification-backlink`). The registry entry I added for `TaskNotificationMessage` was the underscored `"task_notification"` — no `.task_notification` CSS rule exists, making the underscore version dead. - Timeline misclassification: `timeline.html` only knew the seven toolbar types plus `teammate`/`sidechain`/etc. With both `user` and `task-notification` classes on the rendered div, the generic `.find()` returned `user` first — async notifications landed in the User row of the timeline rather than getting their own group. Rename the modifier to `task-notification` (Python message-type identifier `"task_notification"` in `models.py` / `renderer.py` unchanged — that's a separate code identifier), add a `task-notification` entry to `messageTypeGroups` in `timeline.html`, and add a `classList.includes('task-notification')` branch ahead of the generic `.find()` (mirroring the existing `teammate` branch, for the same reason: both carry the `user` class). The new group is inserted in the timeline's `groupOrder` between `teammate` and `system`. Snapshot: HTML async-agents fixture now renders the div as `message user task-notification …`; LOW snapshot still drops the duplicate-flagged notification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: ghost duplicate-flagged notifications at LOW (no reindex) CodeRabbit review on PR #132 found a third remap cascade — after session_nav and pair refs, `TemplateMessage.ancestry` and the `spawning_task_message_index` / `parent_message_index` backlink fields are also frozen at hierarchy-build time, so any reindex after the tree is built leaves them pointing at stale slots. The "each fix unblocks the next bug" pattern was a sign the approach was wrong. Switch to "ghosting": the duplicate notification stays in `ctx.messages` with its original `message_index`. Only its *rendered output* disappears at LOW. Implementation: gate `format_TaskNotificationMessage` and `title_TaskNotificationMessage` in both `HtmlRenderer` and `MarkdownRenderer` on `self.detail == LOW and content.result_is_duplicate` → return `""`. The rendering loop's existing "skip empty messages" elision (HTML: `if title or html or msg.children:`; Markdown: `_render_message` returning `""` when there's no title and no content) drops the entry from the visible output without touching ancestry classes, backlinks, session nav, or pair refs. This deletes: - `_drop_duplicate_notifications_at_low` (the survivor list + `_reindex_filtered_context` + `_identify_message_pairs` re-run + tree-children prune). - The post-link call site at the end of `generate_template_messages`. - The `session_nav` rebuild that the reindex required. - The `_identify_message_pairs` re-run that the pair-clear required. Net: -88 lines in `renderer.py`, +42 across the two formatter gates, no behavior change in the rendered HTML/Markdown at any detail level. Test refresh: `test_duplicate_notification_dropped_at_low` → `test_duplicate_notification_ghosted_at_low`. Asserts the notification is *still* in `ctx.messages` with `result_is_duplicate=True`, and that both `HtmlRenderer` and `MarkdownRenderer` return `""` for its title and body when configured at LOW. LOW snapshot regenerated: `message_id` indices for messages after the (now ghosted) notification stay at their original values instead of shifting down by one. Visible rendered output is otherwise byte-equal. Verified: 1123 unit tests pass, pyright + ruff + ty clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * work: capture follow-up plan to replace _reindex_filtered_context with ghosting Surfaced during the PR #132 review pass: every "drop messages and reindex" pass that adds new index-bearing fields creates a fresh "remember to remap X" trap. The async-agents PR hit three in succession (pair refs, session_nav, ancestry/backlinks) before switching to ghosting. PR #131 added a fourth remap target (`SessionHeaderMessage.parent_message_index`). The note records the architectural assessment of generalizing ghosting to the two existing `_reindex_filtered_context` callers (`_pair_skill_tool_uses` — easy, same shape as the async-agents fix; `_filter_template_by_detail` — medium refactor, needs tree-build child grafting + pair-id skip + render-loop elision flag), and proposes a migration path that ends with deleting `_reindex_filtered_context` entirely. Not work for this PR — captured here so the design rationale is preserved when someone (likely Claude later) picks it up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: don't let <result> body bleed into header-field parsing CodeRabbit review on PR #132: ``_FIELD_RE`` (the regex matching ``<task-id>``, ``<status>``, ``<summary>``) was running over the full notification body, including the ``<result>`` payload. The ``<result>`` body is agent-authored markdown and frequently contains literal HTML/XML — an agent quoting a ``<summary>`` tag verbatim, for instance, would clobber the real notification ``<summary>`` field. Same risk for ``<status>`` and ``<task-id>``. Downstream this poisons the fold/dedup path: the spawning Task tool_result wouldn't match the right notification, and the wrong status badge would render on the card. Extract ``<result>`` and ``<usage>`` first, strip their full match strings from the search surface, then run ``_FIELD_RE`` over the residual header. The result body is unchanged — it still ships the agent's verbatim XML-ish content. New regression test: a notification whose ``<result>`` includes ``<task-id>fake999</task-id> <status>failed</status> <summary>Bogus summary</summary>`` must not overwrite the real ``real123`` / ``completed`` / ``Real summary`` header metadata. Inline tags are preserved verbatim in ``result_text``. Verified: 1129 unit tests pass (33 in test_async_agents), pyright + ruff + ty clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * async-agents: drop the unconditional Task/Agent tool_name early-skip CodeRabbit review on PR #132: ``_link_async_notifications`` skipped every tool_result whose ``tool_name`` was not exactly ``"Task"`` or ``"Agent"`` before even calling ``_async_agent_id_from_tool_result``. ``tool_name`` is populated by pair-id, which can leave a tool_result orphaned in fork/branch shapes where the spawning tool_use sits in a different branch — yet that orphan still carries the canonical ``agentId:`` line, so the notification ought to fold onto it. Drop the unconditional pre-filter. After the agent-id detector returns a hit, gate the non-Task/Agent path on a stronger signal — a parsed ``TaskOutput`` output OR an ``agentId`` already tagged on the entry's meta — so an unrelated tool_result that happens to mention "agentId:" in its raw text doesn't hijack a notification meant for a real spawn. The canonical path (paired Task/Agent tool_result with parsed ``TaskOutput``) is unaffected; only the fallback fork/branch case gains coverage. No behavior change on any existing fixture. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Long-running team-spawn / parallel-tool_use sessions surfaced a cascade
of within-session-fork rendering artifacts:
Fork point (1 branches)ghost entries in the index, deeplynested in the parallel-tool_use case where every parallel turn became
a 2-child DAG fork (one continuation through a
progresspassthrough,one dead-end
tool_result).branch header, and the in-flow fork-point box — same fork shown three
different ways (rich preview here, UUID-only there, "Branch <8 hex>"
in the third spot).
from ⑂ Fork pointbacklinks pointing at the wrong message after theSkill-pair reindex shifted indices: the cached
parent_message_indexon
SessionHeaderMessageinstances created earlier in_render_messageswas never remapped, so the link jumped to whatevermessage ended up at the old index after the shift.
Session …: 15 roots found (7 unexpected)warnings onevery long compacted session, with 6 individual
Orphan node …log lines per
/compactboundary.This PR addresses all four under one consistent framing.
Two commits
DAG: collapse parallel-tool_use forks via passthrough chain
_walk_session_with_forks: at a fork where exactlyone passthrough child has a live (non-structural) subtree and every
other child is structural, follow the passthrough as the chain
continuation and stitch the user/assistant siblings inline as
dead-end side entries. Distinct from real user rewinds, which never
include passthrough children.
max_depth=20clamp on_is_structural_subtree—progresspassthrough chains under a parallel-tool_use anchorregularly exceed the limit, which was misclassifying them as live.
The
seenset +session_uuidsfilter still bound termination.progresspassthroughs as expected roots(
_EXPECTED_ROOT_PASSTHROUGH_TYPES = {"progress"}). Async hooks(
SessionStart:*,PostToolUse:*) routinely fire as parentlessroots — either as a session's first event before any user turn, or
orphan-promoted when their spawning
tool_useis in the discardedpre-compaction context. Per-orphan log demoted to DEBUG when the
orphan is a progress passthrough; multi-root warning stays quiet.
dev-docs/dag.mdgets a Variant 3 section (mermaid + detectioncriteria + summary-table row) and a new "Expected Root Types"
subsection tabulating all six legitimate root shapes (compact
boundary, local_command, two flavors of progress passthrough,
session's actual first entry, subagent root).
Renderer: consistent branch labels + fix stale fork-point backlinks
_reindex_filtered_contextnow also remapsparent_message_indexon every branch / child
SessionHeaderMessage, not justctx.session_first_message. Otherwise the_pair_skill_tool_usesreindex left every branch's "from ⑂ Fork point" link pointing at a
stale index.
_branch_short_uuidand_branch_labelare the singlesource of truth for the
Branch • <uuid8> • <preview>(orBranch • <uuid8>when no preview) shape. Used by:_render_messages,first_user_messageinprepare_session_navigation,_enrich_branch_titlespost-pass scans the finalctxforeach branch's first
UserTextMessagewhen the branch's literalfirst transcript entry is an assistant (e.g. "No response
requested." after
/exit) or a tool_result — so the body headersurfaces the same rich preview the index already had.
"≥ 2 navigable branches": the DAG-level Variant 3 already collapses
spurious single-branch shells, so this layer surfaces the indicator
reliably for every real fork.
Test plan
TestParallelToolUseViaPassthrough(basic shape + the >20-deeppassthrough chain that exercises the depth-cap removal)
TestRootClassification::test_progress_passthrough_roots_are_expectedcovering both
parentUuid:nulland orphan-promoted shapesTestReindexBranchBackrefs(test_skill_pairing.py) — two cases:unshifted (drop later messages) and shifted (drop an earlier
message moves the fork-point's index, the backlink must follow)
just ciclean: 1206 tests, ruff, pyright, ty all green/compact14k-node session withmultiple parallel-tool_use turns: 5 fork points / 10 branches →
2 fork points / 4 branches (the two remaining are real
slash-command rewinds), and zero DAG warnings emitted.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests