Keep fork points visible when their host message is detail-filtered#240
Conversation
…233 follow-up) A fork point is navigational structure, not message content: the branches it connects (session headers) always survive --detail filtering, so the fork point must too. Previously the fork-point box was a rider on its host message, and when _ghost_template_by_detail ghosted that host to None at reduced detail, the box vanished — leaving branches with no visible fork point above them and (per #233's belt-and-suspenders) plain-text back-links. Now, instead of ghosting a slot that carries junction_forward_links, keep it as a "fork_only" landmark: non-None slot, message body suppressed, the template renders only the ⟂ fork-point box at the slot's original position. Because the slot stays non-None, the branch back-links and nav anchor reactivate for free (_repair_stale_anchor_refs only nulls refs to None slots). Net: fork points are now visible at every detail level, full parity with the branches they connect. Verified at full/high/low/minimal/user-only. - TemplateMessage.fork_only flag; set in _ghost_template_by_detail. - transcript.html: a fork_only render branch emitting just the box, with the anchor id on the box so back-links / nav resolve to it. Test updates (the behavior this supersedes was pinned by three tests): - test_ghost_repair: the fork anchor is now KEPT as a fork_only landmark at USER_ONLY (not ghosted); back-link stays live. Deeper "no dead anchor" invariant unchanged (also pinned by TestHtmlAnchorIntegrity). - test_detail_levels: a fork_only landmark is exempt from the "only recaps survive among system messages" check — it renders only the box, no body. - test_dag_integration: a filtered fork anchor's branch back-link is now active (resolves to the kept landmark), not omitted. - New TestForkSurvivesDetailFiltering: box survives / body suppressed / back-links reactivate / no dead anchors, across all reduced levels. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
📝 WalkthroughWalkthroughThe renderer now keeps fork-point messages as ChangesFork-only landmarks at reduced detail
Sequence Diagram(s)sequenceDiagram
participant HtmlRenderer
participant _ghost_template_by_detail
participant TemplateMessage
participant render_message
HtmlRenderer->>_ghost_template_by_detail: filter hidden slots by detail
_ghost_template_by_detail->>TemplateMessage: set fork_only=True on fork-point slots
render_message->>TemplateMessage: read fork_only during rendering
render_message->>render_message: render message.children
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
🧹 Nitpick comments (1)
test/test_detail_levels.py (1)
1528-1537: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
continuealso skips children recursion forfork_onlylandmarks.The
fork_onlylandmark's own body is suppressed (so skipping its system check is correct), but its children are rendered by the template ({% for child in message.children %}intranscript.html). Early-continuehere also bypasses the recursive descent into those children, so a non-recap system message nested under a fork landmark would render in the HTML yet escape this assertion — a false negative in the invariant guard.Skip only the system-type check, not the subtree:
♻️ Keep recursing into fork-landmark children
for msg in messages: - if getattr(msg, "fork_only", False): - continue - if msg.type == "system": + if not getattr(msg, "fork_only", False) and msg.type == "system": cls = type(msg.content).__name__ assert cls == "AwaySummaryMessage", ( f"{label}: non-recap system message survived: {cls}" ) if hasattr(msg, "children"): _assert_system_messages_are_recaps(msg.children, label)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_detail_levels.py` around lines 1528 - 1537, The recursive invariant check in _assert_system_messages_are_recaps is skipping entire fork_only messages, which also prevents descending into their children and can miss nested non-recap system messages. Adjust the loop so fork_only only bypasses the msg.type == "system" assertion, but still recurses into msg.children when present. Keep the existing recursion and uses of getattr(msg, "fork_only", False), msg.type, and hasattr(msg, "children") intact while changing the control flow to avoid early continue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/test_detail_levels.py`:
- Around line 1528-1537: The recursive invariant check in
_assert_system_messages_are_recaps is skipping entire fork_only messages, which
also prevents descending into their children and can miss nested non-recap
system messages. Adjust the loop so fork_only only bypasses the msg.type ==
"system" assertion, but still recurses into msg.children when present. Keep the
existing recursion and uses of getattr(msg, "fork_only", False), msg.type, and
hasattr(msg, "children") intact while changing the control flow to avoid early
continue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e857875-6f74-4dc2-94a9-ef327e19b304
📒 Files selected for processing (6)
claude_code_log/html/templates/transcript.htmlclaude_code_log/renderer.pytest/test_dag_integration.pytest/test_detail_levels.pytest/test_fork_invisible_node.pytest/test_ghost_repair.py
A follow-up to #233. A fork point is navigational structure, not message content — the branches it connects (session headers) always survive
--detailfiltering, so the fork point should too. Previously the fork-point box was a rider on its host message, and when_ghost_template_by_detailghosted that host toNoneat reduced detail, the box vanished — leaving branches with no visible fork point above them and (per #233's belt-and-suspenders) plain-text back-links.Change
Instead of ghosting a slot that carries
junction_forward_links, keep it as afork_onlylandmark: a non-Noneslot with its.contentintact, body suppressed at render time only. A newTemplateMessage.fork_onlyflag is set in the ghost pass; a template branch inrender_message(checked first, beforeis_session_header/should_render) emits just the ⟂ fork-point box —message-node > children > fork-point, with the anchoridon the box — no message card.Because the slot stays non-
None, the branch back-links and nav anchor reactivate for free:_repair_stale_anchor_refsonly nulls refs toNoneslots. Linking runs before ghosting, so the box data already exists at ghost time. Non-fork slots still ghost toNone, so #233's genuine-ghost sanitize path is preserved for non-fork anchors and degenerate (<2-branch) forks.Net: fork points are now visible at every detail level, full parity with the branches they connect. Verified at full/high/low/minimal/user-only — FULL shows card+box (unchanged #233); reduced suppresses the body, keeps the box + active back-links, zero dead anchors.
Latent-coupling guard
The no-dead-anchor guarantee rests on an unstated cross-class invariant: branch session headers are always visible (
SessionHeaderMessagedeclares nodetail_visibility). If a branch could be ghosted, a 2-branch fork could drop below 2 survivors → box suppressed → the kept slot renders with noid→ a sibling back-link dangles. A comment at thefork_onlyassignment documents this, andtest_branch_headers_always_visible_keeps_fork_anchoredpins it directly — so a futureSessionHeaderMessage.detail_visibilitytrips a test instead of silently activating a dead anchor.Testing
Three existing tests pinned the superseded #233 behavior ("fork ghosted → omit/sanitize backlink"); each is updated to the new "fork survives" behavior while preserving the deeper invariant (no dead
#msg-d-N, D11 trunk-before-branch):test_ghost_repair: fork anchor kept as afork_onlylandmark, back-link live (the "no dead anchor" invariant stays independently pinned byTestHtmlAnchorIntegrity).test_detail_levels:fork_onlylandmarks are exempt from "only recaps survive among system messages" (they render only the box, no body leaks).test_dag_integration(D11): a filtered fork anchor's back-link is now active (resolves to the kept landmark); the trunk-before-branch invariant is unaffected.New
TestForkSurvivesDetailFiltering: box survives / body suppressed / back-links reactivate / no dead anchors, across all reduced levels; plus the always-visible-branch invariant pin.just cigreen (unit + TUI + browser + snapshots, ruff, pyright, ty); no snapshot churn.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests