plugins: dev-docs gaps + public helper API + ToolResult example (Phase 2)#173
Merged
Conversation
Phase 2 of the plugin-system follow-up — addresses the seven
doc/code gaps clmail/main + clmail/carol surfaced after their
v1 plugin shipped (mails #3218 + #3241), plus the long-deferred
``work/`` shrink for the now-graduated RFC.
**Public plugin API (`claude_code_log/plugins.py`)** — re-export
``render_markdown`` and ``render_markdown_collapsible`` from
``html/utils.py`` under the stable ``claude_code_log.plugins``
namespace. Resolved lazily via PEP-562 ``__getattr__`` because the
``html`` subpackage closes a circular loop with ``factories`` /
``utils`` at module-init time; ``TYPE_CHECKING`` block makes the
names visible to static checkers (ruff F822, pyright dunder-all)
so the re-export reads as a real surface declaration.
The stable namespace lets ``html/utils.py`` churn without
breaking plugin authors. ``_PUBLIC_HELPERS`` keeps the surface
narrow on purpose — every entry is an API commitment.
**ToolResultMessage worked example** —
``test/_plugins/clmail/.../tool_communicate_result.py`` mirrors the
existing ``tool_communicate.py`` (same fixture tool name, companion
``applies_to=(ToolResultMessage,)`` transformer) and demonstrates
the long-Markdown-body pattern via the new public helper. Three
tests in ``test_plugin_system.py::TestToolResultTransformerWith
CollapsibleBody`` cover short-body inline / long-body collapsed /
Strategy-2 class-side dispatch.
This single addition closes both #3218 gap 4 ("no worked example
for ToolResultMessage") and the long-Markdown-body Request 2 from
#3241 — same example, both audiences.
**`dev-docs/plugins.md` expansion**:
- §2 Quick start gains a constructor-signature reference block
spelling out ``ToolUseMessage`` (with the kw-only ``skill_body``
call-out) and ``ToolResultMessage`` field-copy patterns. The
three-bullet reference-plugin list grows to include the new
``tool_communicate_result.py``.
- §4.1 "Plugin-facing helpers" is the new home for the
``render_markdown`` / ``render_markdown_collapsible`` signatures
and use-when guidance; the section explicitly disclaims a wider
surface ("add only on concrete plugin-author demand").
- §6 "Practical guide" picks up the ``HIGH`` vs ``FULL`` distinction
for hook-style content, with the right rule-of-thumb question
("would a reviewer skimming at HIGH want to know this happened?").
- §8.2 picks up the per-tool-offset convention for multi-transformer
plugins ("base ± single-digit offset to silence self-collisions
on shared ``applies_to``").
- §10 gains the CSS-wrap note (``<div class="markdown">…</div>``
for direct Markdown-shaped HTML returns) and the
``render_markdown_collapsible``-already-handles-it caveat.
- §12 picks up the dispatch-auto-wrap design question as an
informational v2 direction so the conversation has a home.
**`work/tool-renderer-plugins.md` shrink** — the RFC's main content
has graduated to ``dev-docs/plugins.md`` per the project's "work →
dev-docs on shipping" lifecycle convention. Retained ``Open
questions (deferred to v2)`` + ``Future extensions`` as the v2
backlog (drop -784 lines, keep the 11 deferred items + 4 future
extensions). Reframed to align with the v1-shipped reality.
Test summary: 1914 passed, 7 skipped — +3 from this PR
(``TestToolResultTransformerWithCollapsibleBody``). ``just ci``
clean across format / test-all / ruff / pyright / ty.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR implements the final plugin rendering dispatch system: ChangesPlugin Rendering Dispatch System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
✨ 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 |
Three findings from clmail/carol's read pass against e6218eb (mail #3249): **Wall A — `format_html(...) -> None` doesn't fall back to mistune.** The dispatcher returns `None` verbatim and the host template renders the literal string "None" in the card body. The reference test plugins' ``return None # fall back to mistune`` pattern was misleading — a copy-paste user inherits the bug. Fix in two places: - Honest doc note in `dev-docs/plugins.md` §4 explaining the v1 state and pointing at the v2 dispatch auto-wrap (§12) for the long-term fix. - Reference plugins (`hook_demotion.py`, `tool_communicate.py`) switch to the explicit ``render_markdown(self.format_markdown(...))`` pattern with a comment explaining why ``return None`` doesn't work today. The dispatcher fix itself (have the MRO walk continue on class-side None, with HTML re-attempting via the markdown path) is deliberately deferred to a separate PR — broader renderer behaviour change with snapshot implications. **Wall B — `is_error=True` on ToolResultMessage subclasses.** Setting it triggers the host's standard error chrome (🚨 emoji + red `.tool_result.error` CSS class) via `html/utils.py`'s `isinstance(content, ToolResultMessage) and content.is_error` gate. Bash errors use the same primitive. Documented in §4 right after the v1 caveat so plugin authors writing result-side transformers discover it without grepping the renderer. **Cleaner CSS-scope alternative — `has_markdown = True`.** The host template at `html/templates/transcript.html:146` reads `message.content.has_markdown` and flips the `markdown` class onto the wrapping `<div class='content'>` automatically. Built-ins (`AwaySummaryMessage`, `TeammateMessage`, `AssistantTextMessage`) use this exclusively — no inline `<div class="markdown">` wrapping in their `format_html`. §10 now presents `has_markdown = True` as the preferred path with the inline-wrap recipe as the fallback for fine-grained scope control. `tool_communicate_result.py` picks up the property as a worked example. One new test (`test_has_markdown_property_pins_markdown_css_scope`) pins the contract. Test summary: 1915 passed, 7 skipped (+1 from this fixup, +4 from PR #173 cumulatively). `just ci` clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Design pivot from user (relayed via main): with ``render_markdown`` and ``render_markdown_collapsible`` now public on ``claude_code_log.plugins``, the ``None``-as-fall-back-sentinel for ``format_X`` is dead complexity. New v1 contract: - **Plugin class defines ``format_html``** → dispatcher uses the return value as-is. Must be a real string; no None sentinel. - **Plugin class does NOT define ``format_html``** → dispatcher synthesizes HTML by rendering the class's ``format_markdown`` through mistune, wrapped in ``<div class="markdown">``. Supersedes the doc-only Wall A fix in b55e187 (which had plugin authors write a ``render_markdown(self.format_markdown(...))`` shim explicitly). The shim is now the dispatcher's job. **`HtmlRenderer._dispatch_format` override** (``html/renderer.py``): adds two actual-class precedence rules before deferring to the base MRO walk: 1. ``type(obj).__dict__["format_html"]`` exists → use it verbatim. 2. ``type(obj).__dict__["format_markdown"]`` exists (but not ``format_html``) → synthesize via mistune + wrap. 3. Otherwise → ``super()._dispatch_format(obj, message)``. Rule 2 deliberately wins over an ancestor's renderer-side ``format_<ClassName>``: a plugin author who defined ``format_markdown`` on a subclass meant for their Markdown to drive the rendering, not for the parent class's built-in renderer behaviour to take over. Without this, plugin subclasses of ``UserTextMessage`` etc. would silently fall through to the parent's HTML output. **Reference plugins**: - ``hook_demotion.py`` + ``tool_communicate.py`` drop the redundant ``format_html`` shim — they're now the canonical "Markdown only; HTML inherits via synthesis" demo. The comment block explains when to add ``format_html`` vs leave it absent. - ``tool_communicate_result.py`` keeps ``format_html`` because it legitimately does something different (collapsible ``<details>`` via ``render_markdown_collapsible`` for long bodies). Tightened the return annotation to ``str`` (was ``Optional[str]``) per the new contract. **`dev-docs/plugins.md`**: - §4 contract rewritten — table updated, the v1 caveat about None-rendering-as-literal-string replaced with the new clean rule. ``Error-shaped results`` paragraph kept (Wall B — ``is_error=True`` chrome). - §5.1 new subsection — HtmlRenderer extension explaining actual-class precedence + the Markdown synthesis path. - §10 picks up a note clarifying ``has_markdown`` semantics: synthesis path always wraps in ``<div class="markdown">`` (so ``has_markdown = True`` is not needed for that path); the property only matters for explicit ``format_html`` callers. - §12 ``Dispatch auto-wrap for Markdown-shaped returns`` removed — the pivot subsumes it. **`work/tool-renderer-plugins.md`**: companion v2-backlog entry dropped (same rationale). **Tests**: one new regression (``test_absent_format_html_synthesizes_from_format_markdown``) proves the synthesis path fires for a plugin subclass of ``UserTextMessage`` that defines only ``format_markdown`` — the actual-class precedence rule (synthesis wins over inherited Strategy 1) is the load-bearing behaviour. Asserts the rendered HTML wraps in ``<div class="markdown">``, contains the mistune-rendered ``<em>`` from the Markdown italics, and crucially that the literal string "None" does NOT appear (the old bug). Test summary: 1916 passed, 7 skipped (+1 from this commit). ``just ci`` clean. Worth re-pinging clmail/carol for a second-pass acceptance read since this changes the contract she validated against in #3249. Will do after pushing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
From clmail/carol's second-pass acceptance read on c65cfdd (mail #3285): the prior phrasing said `has_markdown = True` "doesn't matter" for the synthesis path — technically correct but under-prescriptive. The empirical pitfall: combining the two produces a nested-wrap DOM (`<div class="content markdown"> <div class="markdown">…</div></div>`) because the synthesizer wraps unconditionally AND the host template flips on the outer `markdown` class via `has_markdown`. Cache-masked-but-no-cache-visible — carol hit it during her shipped-render simplification audit. Benign for CSS (selectors don't care about depth), visible in the DOM, surprising on view-source. Sharpened from "doesn't matter" to "should NOT be set" for synthesis classes; explicit rule of thumb added. One paragraph under the existing §10 synthesis-path note, no contract change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Follow-ups needed... not the final word on the topic. |
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
Phase 2 follow-up on the plugin system landed in #169. Addresses the seven doc/code gaps clmail/main and clmail/carol surfaced after their v1 plugin shipped (federation mails #3218 + #3241), plus the long-deferred
work/shrink for the now-graduated RFC.Production code
claude_code_log/plugins.py— re-exportrender_markdownandrender_markdown_collapsiblefromhtml/utils.pyunder the stableclaude_code_log.pluginsnamespace. Resolved lazily via PEP-562__getattr__(thehtmlsubpackage closes a circular loop withfactories/utilsat module-init time);TYPE_CHECKINGblock makes the names visible to ruff F822 and pyrightreportUnsupportedDunderAll._PUBLIC_HELPERSkeeps the surface narrow on purpose.Test-embedded reference plugin
tool_communicate_result.py— new transformer mirroringtool_communicate.py(same fixture tool name, companionapplies_to=(ToolResultMessage,)). Demonstrates the long-Markdown-body pattern via the new public helper. Single addition closes both the original "no worked example forToolResultMessage" gap (#3218 gap 4) AND the long-body Request 2 from #3241.TestToolResultTransformerWithCollapsibleBody: short-body inline / long-body collapsed / Strategy-2 class-side dispatch.dev-docs/plugins.mdexpansionToolUseMessage(withskill_bodycall-out) andToolResultMessage; reference-plugin list grows to three entries.render_markdown/render_markdown_collapsiblesignatures + use-when guidance + the explicit "add only on concrete demand" disclaimer.HIGHvsFULLchoice for hook-style content.render_markdown_collapsiblealready handles it.work/tool-renderer-plugins.mdshrinkThe RFC's main content has graduated to
dev-docs/plugins.mdper the project's "work → dev-docs on shipping" lifecycle convention. RetainedOpen questions (deferred to v2)+Future extensionsas the v2 backlog (drop -784 lines, keep 11 deferred items + 4 future extensions).Test plan
just ciclean (format / test-all / ruff / pyright / ty)python -c "from claude_code_log.plugins import render_markdown, render_markdown_collapsible; ..."worksStanding instruction
Per user's standing instruction: do not auto-merge. Once approved, monk will request a read-pass from clmail/carol (the original plugin author) as the acceptance-test loop the user named in #3185, then await user ack.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation