Render the built-in Monitor tool with Task-end backlink (#142)#147
Open
Render the built-in Monitor tool with Task-end backlink (#142)#147
Conversation
Two changes addressing #142: ## 1. Monitor tool — typed input/output models, formatters, title The built-in ``Monitor`` tool (shell-stream watcher; ``description`` / ``command`` / ``timeout_ms`` / ``persistent``) used to fall through to the generic params-table render. Add full specialised rendering: - ``MonitorInput`` (Pydantic) and ``MonitorOutput`` (dataclass) on ``models.py``, registered in the ``ToolInput`` and ``ToolOutput`` unions. - Factory: ``"Monitor"`` registered in ``TOOL_INPUT_MODELS`` so ``ToolUseMessage.input`` deserialises into ``MonitorInput``; ``parse_monitor_output`` registered in ``TOOL_OUTPUT_PARSERS`` and pulls the task id out of the start-confirmation paragraph for optional cross-reference (the body text is what actually renders). - HTML formatter (``html/tool_formatters.py``): - Title: ``🔭 Monitor <description>``. - Input body: 4-row ``tool-params-table`` with ``description``, ``command``, ``timeout_ms``, ``persistent``. The ``command`` cell uses the existing ``render_collapsible_code`` helper when the body is multi-line / long, matching the line-count-badge affordance already used for other tool bodies; short single-line commands render in a plain ``<pre class='monitor-command'>``. - Output body: start-confirmation paragraph verbatim inside a ``<div class='monitor-output'>``. - Markdown formatter mirrors the shape: bullet list of the three scalar fields, command in a fenced ``bash`` block, output text passes through. ## 2. Task-end → originating tool_use backlink When a ``<task-notification>`` carries ``<tool-use-id>`` pointing back at the originating tool_use (built-in Monitor's start-confirmation returns the task id; later, when the stream ends, Claude Code injects a notification carrying both the task id and the tool_use id), the notification's Task ID value should link back to the originating tool_use card. - ``task_notification_factory`` now extracts ``<tool-use-id>`` from the notification block alongside ``<task-id>`` / ``<status>`` / ``<summary>``. Older notifications without the field still parse — the new ``tool_use_id`` is just ``None``. - New renderer pass ``_link_tool_use_notifications`` (in ``renderer.py``, runs after ``_link_async_notifications``) builds a ``tool_use_id → message_index`` map and sets ``spawning_task_message_index`` on each notification carrying a matching id. Reuses the existing field name to keep the formatter single-shape — both flows now wire the same backlink slot. - HTML formatter: when ``spawning_task_message_index`` is set, the Task ID's ``<code>`` is wrapped in ``<a class='task-notification-backlink' href='#msg-d-{N}'>``. The separate "Spawn" row that used to carry the backlink is removed — the Task ID value itself is the more discoverable affordance and matches the spec in #142. This unifies the async-agent and Monitor flows behind one rendering path. Async-agent notifications gain the same Task-ID-as-link shape, which is a small intentional improvement to that flow as well. ## Tests New ``test/test_monitor_rendering.py`` (20 cases): - ``TestMonitorInputModel`` / ``TestMonitorOutputParser`` — direct factory tests including the missing-task-id fallback. - ``TestMonitorHtmlFormatter`` — input grid, short-command-inline, long-command-collapsible, optional-fields-omitted, output paragraph. - ``TestMonitorFixtureRendering`` — drives the real renderers against a small JSONL fixture (Monitor call + result + matching task-notification). Asserts: telescope icon and description in the title, four-row grid, collapsible command with line-count badge, result paragraph, and (most importantly) the backlink — the Task ID ``<a>`` href matches the Monitor tool_use's ``msg-d-N`` anchor. - ``TestTaskNotificationToolUseIdBacklink`` — orthogonal to the Monitor flow: factory extracts ``tool-use-id``, legacy notifications without the field still parse, an orphan ``tool-use-id`` (no matching tool_use in the transcript) renders as plain ``<code>`` rather than an anchor pointing at nothing. Snapshot drift is one line in ``test_snapshot_html.ambr`` — exactly the async-agent fixture's notification card shifting from the Spawn row + plain-Task-ID shape to the Task-ID-as-link shape. ## CI - ``just test``: 1610 pass, 7 skipped (was 1591 before; +19 new) - ``just test-tui``: 66 pass - ``just test-browser``: 39 pass, 1 skipped - ruff + pyright clean on production files.
|
ℹ️ 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 (3)
📝 WalkthroughWalkthroughAdds Monitor tool support and task-notification backlinking: new MonitorInput/MonitorOutput models, factory parsing (including ), HTML/Markdown formatters, renderer backlink pass linking notifications to tool_use messages, and tests/fixture updates. ChangesMonitor Tool Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
![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
🧹 Nitpick comments (1)
claude_code_log/html/tool_formatters.py (1)
700-724: ⚡ Quick winRemove the unused
paramsdict — it's dead code that contradicts the comment.The
paramsdict is populated (lines 702, 722–724) but is never referenced in the final output. The table rows are composed manually (lines 728–746), soparamsis unreachable. The comment at line 703 ("pass a placeholder through the table builder, then replace") describes an approach that was replaced by the manual row-building strategy — the dict is a leftover.🧹 Proposed cleanup
def format_monitor_input(monitor_input: MonitorInput) -> str: - params: dict[str, Any] = {"description": monitor_input.description} - # Command rendered separately for adaptive collapsibility — pass - # a placeholder through the table builder, then replace with the - # specialised cell. This keeps the row layout consistent with - # ``render_params_table`` (same CSS hooks downstream) while letting - # us keep the line-count badge for the multi-line case. command = monitor_input.command line_count = command.count("\n") + 1 escaped_command = escape_html(command) if line_count > 5 or len(command) > 300: preview_lines = "\n".join(command.splitlines()[:3]) preview_html = f"<pre>{escape_html(preview_lines)}</pre>" full_html = f"<pre>{escaped_command}</pre>" command_cell = render_collapsible_code(preview_html, full_html, line_count) else: command_cell = f"<pre class='monitor-command'>{escaped_command}</pre>" - if monitor_input.timeout_ms is not None: - params["timeout_ms"] = monitor_input.timeout_ms - if monitor_input.persistent is not None: - params["persistent"] = monitor_input.persistent - # Compose the table by hand so the ``command`` row carries the # specialised cell instead of a generic stringification.🤖 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 `@claude_code_log/html/tool_formatters.py` around lines 700 - 724, The params dict created in tool_formatters.py (params) is dead code — remove the params = {"description": monitor_input.description} declaration and the later conditional assignments to params["timeout_ms"] and params["persistent"], and also delete or update the outdated comment about "pass a placeholder through the table builder"; verify that nothing else references params and that the code continues to use the manually built command_cell (render_collapsible_code / escape_html usage remains unchanged).
🤖 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/markdown/renderer.py`:
- Around line 1421-1423: The title_MonitorInput method currently injects
input.description raw into the markdown heading; sanitize it by escaping
markdown control characters before interpolation. Update title_MonitorInput to
pass input.description through an escaping helper (e.g., escape_markdown or
similar utility) or implement one that escapes characters like *, _, `, [, ], (,
), #, +, -, !, >, | so the returned string returns f"🔭 Monitor
{escaped_description}" instead of the raw input.description.
In `@test/test_monitor_rendering.py`:
- Around line 362-366: The current module-level autouse fixture
_ensure_fixture_present skips the entire module if FIXTURE is missing; change it
to not be autouse and limit its scope to the test class that requires the
fixture. Modify the fixture signature to remove autouse and set scope="class"
(pytest.fixture(scope="class")), keep the same body referencing FIXTURE, and
then add `@pytest.mark.usefixtures`("_ensure_fixture_present") to the
TestMonitorFixtureRendering test class so only that class is skipped when the
JSONL fixture is absent.
---
Nitpick comments:
In `@claude_code_log/html/tool_formatters.py`:
- Around line 700-724: The params dict created in tool_formatters.py (params) is
dead code — remove the params = {"description": monitor_input.description}
declaration and the later conditional assignments to params["timeout_ms"] and
params["persistent"], and also delete or update the outdated comment about "pass
a placeholder through the table builder"; verify that nothing else references
params and that the code continues to use the manually built command_cell
(render_collapsible_code / escape_html usage remains 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: 911df1eb-0932-44d0-ad3d-94f914d9e94e
📒 Files selected for processing (11)
claude_code_log/factories/task_notification_factory.pyclaude_code_log/factories/tool_factory.pyclaude_code_log/html/async_formatter.pyclaude_code_log/html/renderer.pyclaude_code_log/html/tool_formatters.pyclaude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/renderer.pytest/__snapshots__/test_snapshot_html.ambrtest/test_data/monitor_tool.jsonltest/test_monitor_rendering.py
Three findings, all applied. None blocking; all small.
## 1. Markdown title — escape via inline-code wrap
`title_MonitorInput` in `markdown/renderer.py` interpolated
`input.description` raw into the title heading. A description
containing markdown emphasis / heading metacharacters (``*``,
``_``, ``` ` ```, ``[``, ...) would leak into the rendered title and
either italicise unwanted runs or open code spans that swallow
following text.
Fix: wrap the description in inline code via the existing
`_inline_code` helper, matching the recipe already used by
`title_WebSearchInput` / `title_WebFetchInput` / `title_SkillInput`.
The helper widens the fence past any backtick run in the value and
pads when the value starts/ends with a tick — same protection,
established convention. Title becomes ``🔭 Monitor `<description>` ``.
The HTML side was already safe (`_tool_title` routes through
`escape_html`); the asymmetry is because HTML and Markdown have
different metacharacter sets.
## 2. Test fixture scope — class-only via `@pytest.mark.usefixtures`
`_ensure_fixture_present` was `scope="module", autouse=True`, which
means the *entire* test module skips when `monitor_tool.jsonl` is
missing — including the model-parsing / formatter-unit /
backlink-contract tests that don't need the fixture at all.
Fix: drop `autouse`, set `scope="class"`, and apply via
`@pytest.mark.usefixtures("_ensure_fixture_present")` on
`TestMonitorFixtureRendering` only. Now only the fixture-driven
end-to-end tests skip when the JSONL is absent; the rest of the
suite still runs.
(Pyright is added to the ignore list for `reportUnusedFunction` on
the fixture itself — pytest accesses it by name string via the mark,
which the static checker can't see. Standard pytest fixture pattern.)
## 3. Dead `params` dict removed (nitpick)
`format_monitor_input` populated a `params` dict but never used it —
leftover from an earlier draft that routed through
`render_params_table`. The final shape composes rows by hand to
keep the specialised collapsible-command cell. Dropped the dict, the
two later assignments, and the stale comment that mentioned the
discarded approach.
## Tests
- `just test`: 1626 pass, 7 skipped (unchanged count — no test added,
just adjusted assertions and fixture wiring).
- `test_markdown_title_and_command_fence` updated to expect the new
inline-code-wrapped title.
- ruff + pyright clean on the touched files.
Collaborator
Author
|
(Claude) Status update on the latest CodeRabbit review (round 1 on All three findings applied —
CI on alice (pre-push): Thanks — the markdown-escape catch in particular would have been a real bug the first time someone passed a description containing a backtick. |
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
Closes #142. Two related pieces in one commit:
Monitortool (the shell-stream watcher withdescription/command/timeout_ms/persistent). NewMonitorInput(Pydantic) +MonitorOutput(dataclass), factory parser, HTML title🔭 Monitor <description>+ 4-row params grid + collapsible command via the existingrender_collapsible_codehelper, mirrored Markdown.<task-notification>carries<tool-use-id>(e.g. the harness emits this when a Monitor stream ends), the notification's Task ID value is wrapped in an anchor pointing at the originating tool_use card. Extendedtask_notification_factoryto extract<tool-use-id>(older notifications without the field still parse —tool_use_iddefaults toNone). New renderer pass_link_tool_use_notificationsruns after_link_async_notificationsand setsspawning_task_message_indexonly on virgin notifications carrying a matching id. The HTML formatter now wraps the Task ID's<code>in<a class='task-notification-backlink'>whenever the index is set; the previous separate "Spawn" row is replaced by the Task-ID-as-link affordance. This unifies the async-agent and Monitor flows behind one rendering shape on the HTML side.The Markdown side keeps the existing two-row shape — the markdown renderer's own comment notes that "Markdown only has session-level
<a id=…>anchors", so message-level backlinks aren't structurally available in MD output.Out of scope:
mcp__plugin_clmail_clmail__controlwithaction="monitor"(the actor-monitoring MCP tool) — distinct shape, can be a follow-up issue.Test plan
just test: 1626 pass, 7 skipped (was 1607 onmain; +19 new tests)just test-tui: 66 passjust test-browser: 39 pass, 1 skippedjust ciend-to-end clean (ruff + pyright + ty all green)test/test_monitor_rendering.py(20 cases) covers:TestMonitorInputModel/TestMonitorOutputParser— direct factory tests including the missing-task-id fallbackTestMonitorHtmlFormatter— short-command-inline, long-command-collapsible, optional-fields-omitted, output paragraphTestMonitorFixtureRendering— drives the real renderers against a small JSONL fixture (Monitor call + result + matching task-notification linked bytoolu_test_monitor_42); asserts the backlink anchor matches the Monitor tool_use's div idTestTaskNotificationToolUseIdBacklink— orthogonal contract: factory extractstool-use-id, legacy notifications without it still parse, an orphantool-use-id(no matching tool_use) renders as plain<code>rather than a dangling anchortest_snapshot_html.ambr— the async-agent fixture's notification card shifting fromTask ID:<code> + Spawn:<a>toTask ID:<a><code>. Content-equivalent.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests