Cross-link TaskOutput / TaskUpdate headers back to their spawn (#154)#158
Open
cboos wants to merge 2 commits into
Open
Cross-link TaskOutput / TaskUpdate headers back to their spawn (#154)#158cboos wants to merge 2 commits into
cboos wants to merge 2 commits into
Conversation
`TaskOutput` polls and `TaskUpdate` calls reference a `task_id` minted by an earlier tool_use. The polled card showed the id verbatim, but readers had no way to jump back to the originating spawn — they had to scroll-and-grep. Mirror the affordance already in place for async-agent notifications (#142), the `Monitor` tool (#142 / #147), and the Cron* family (#148 / #152): wire a renderer pass that walks the tool_result stream, indexes each id-minting tool_use, then stamps consumer cards with a `creating_call_message_index` so the title formatter can wrap `#<id>` in `<a class='task-id-backlink' href='#msg-d-N'>`. Three flows share the pass: 1. `Bash` with `run_in_background=true` → `TaskOutput` polls carrying `taskType: local_bash`. Background id sourced from `toolUseResult.backgroundTaskId` (structured field, not text parsing). 2. Async-agent `Task` launch → `TaskOutput` polls carrying `taskType: local_agent`. Agent id sourced via the existing `_async_agent_id_from_tool_result` helper (already handles both the structured `agentId` and the raw-text fallback). 3. `TaskCreate` → `TaskUpdate`. Todo-list `#N` ids form a parallel id space; the consumer's `TaskUpdateInput` carries it unambiguously so the same pass handles both shapes. Markdown stays plain-text — the same `#<id>` form surfaces in the title (the renderer omits the anchor wrapper there) so the reader can grep across the document; clickable backlinks remain HTML-only. Tests cover all three paths against a synthetic JSONL fixture and pin the anchor href against the originating tool_use's `msg-d-N` id by locating it via the title-tooltip's API id (stable across message-index renumbering). The async-agents snapshot now picks up the cccc333 backlink alongside the new CSS.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR wires task-id backlinking end-to-end: Bash parsers extract backgroundTaskId, models gain backlink fields, a renderer post-pass maps originating tool_use/TaskCreate messages to consumers, and HTML rendering wraps task ids with anchors using the stamped message indices. ChangesTask ID Backlinking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@claude_code_log/renderer.py`:
- Around line 2457-2458: The two global maps bg_task_id_to_call_index and
todo_task_id_to_call_index are keyed only by task ID which can collide across
sessions; change their keys to include session scope (for example use a
composite key like (session_id, task_id) or make the maps nested per session)
and update every access site that uses bg_task_id_to_call_index or
todo_task_id_to_call_index (including the lookup/manipulation sites referenced
near the current diff and the other occurrences) to construct/look up with the
session-aware key (or index into the per-session sub-dict) so task IDs from
different sessions cannot cross-link.
In `@test/test_task_id_linking.py`:
- Around line 208-215: The fixture _ensure_fixture_present currently calls
pytest.skip when FIXTURE is absent, which silently removes the backlink E2E
class from runs; change the behavior to fail fast by raising or calling
pytest.fail with a clear message (e.g., "Fixture missing: {FIXTURE}") instead of
pytest.skip so CI surfaces missing-fixture regressions; update the
_ensure_fixture_present fixture (and any uses via `@pytest.mark.usefixtures`) to
use pytest.fail when not FIXTURE.exists() and keep the same scope and docstring.
🪄 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: 0b21937b-ab4e-4b81-92a1-baee523cc6b8
📒 Files selected for processing (8)
claude_code_log/factories/tool_factory.pyclaude_code_log/html/renderer.pyclaude_code_log/html/templates/components/message_styles.cssclaude_code_log/models.pyclaude_code_log/renderer.pytest/__snapshots__/test_snapshot_html.ambrtest/test_data/task_id_linking.jsonltest/test_task_id_linking.py
CodeRabbit findings on #158: 1. Cross-session collision on todo `#N` ids. Two sessions in a combined-transcripts render each minting `TaskCreate #1` would collapse onto a single map entry under id-only keys — the second `TaskUpdate #1` would backlink to the FIRST session's `TaskCreate` card. Fix: key the maps on `(session_id, task_id)` tuples on both index and lookup. Background ids are random alphanumeric so collision is practically nil; they ride the same shape for symmetry. 2. Silent skip on missing fixture would mask a fixture-deletion regression in CI. Switch the `_ensure_fixture_present` helper to `pytest.fail` so the loud failure surfaces in the run. Adds a cross-session regression test (`task_id_linking_cross_session`) that builds two sessions both minting `#1` and asserts each `TaskUpdate` backlinks to its OWN session's `TaskCreate` — the test would fail loudly under the id-only-keyed regression.
4 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
TaskOutputpolls andTaskUpdatecalls reference atask_idminted by an earlier tool_use. The polled / updated card showed
the id verbatim but readers had no way to jump back to the
originating spawn — they had to scroll-and-grep.
Mirror the affordance already in place for async-agent notifications
(#142), the
Monitortool (#142 / #147), and the Cron* family(#148 / #152): wire a renderer pass (structural twin of
_link_cron_jobs_by_id, two-pass index → stamp) that indexes eachid-minting tool_use, then stamps consumer cards with a
creating_call_message_index. The title formatter wraps#<id>in<a class='task-id-backlink' href='#msg-d-N'>, same dotted-underlinevisual as
.cron-id-backlink.Three flows share the pass:
Bashwithrun_in_background=true→TaskOutputpollscarrying
taskType: local_bash. Background id sourced fromthe structured
toolUseResult.backgroundTaskIdfield (nottext parsing).
Tasklaunch →TaskOutputpolls carryingtaskType: local_agent. Agent id sourced via the existing_async_agent_id_from_tool_resulthelper.TaskCreate→TaskUpdate. Todo-list#Nids form a parallelid space; the consumer's
TaskUpdateInputcarries itunambiguously so the same pass handles both shapes.
Markdown stays plain-text — the
#<id>form surfaces in the titleso the reader can grep across the document; clickable backlinks
remain HTML-only.
Snapshot delta: the new
.task-id-backlinkCSS rule everywhere +two existing async-agents fixture cards picking up real backlinks
(
cccc333→ spawn,#1→ create card) — no new fixture cards.Closes #154.
Test plan
uv run pytest -m "not (tui or browser)"— 1697 passed,7 skipped, no regressions
ruff format/ruff checkcleanpyright— 0 errors, 0 warningstest/test_task_id_linking.pypin anchorhrefagainst the originating spawn for all three flows;_spawn_anchorlocates the spawn div by the title-tooltip'sAPI id (stable across message-index renumbering)
pytest test/test_snapshot_html.py -n0 --snapshot-update) verified against expected deltaFollow-up (optional, non-blocking)
The
else: agent_id = _async_agent_id_from_tool_result(...)branch in step 1 of
_link_task_id_consumersruns for everynon-Bash tool_result without a
background_task_id, includingTaskCreate / TaskList / generic tool_results whose raw text
won't match the agent-id regex. Functionally correct, just wasted
work on transcripts dominated by todo activity. A one-line gate
on
tool_name in {"Task"}would skip the regex when it can'tmatch. Not worth doing speculatively — leaving for a future
profile-driven cleanup.
Summary by CodeRabbit
New Features
Style
Tests