RFC: plugin system (unified message-transformer mechanism)#166
Merged
Conversation
Design-only doc, no implementation. Proposes: - Entry-point discovery via importlib.metadata (stdlib, group "claude_code_log.tool_renderers"), recommended over pluggy. - ToolRenderer Protocol bundling InputModel / OutputModel / render_*_markdown (required) / render_*_html (optional, falls back to Markdown -> mistune). - Priority offset semantics: 0 = builtin, <0 supersedes, >0 fallback; tie-break stable alphabetical + warning. - min_detail only (no max_detail), with the rationale. - Worked example: clmail-communicate plugin replacing the synthetic "[clmail] new mail (#N)" line with the actual mail content at --detail low. Open for review; implementation gated on user approval. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughReplaces the prior two-mechanism proposal with a single MessageTransformer plugin model discovered via ChangesTool Renderer Plugin System Design
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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.
Actionable comments posted: 1
🧹 Nitpick comments (5)
work/tool-renderer-plugins.md (5)
381-382: Note: Protocol may need extension for format-agnostic rendering.The future vision describes tool renderers contributing
render_input_<format>methods for any format, but the current Protocol (lines 131-142) hardcodesrender_input_markdownandrender_input_html. To support arbitrary formats like RTF or JATS without Protocol changes, consider whether v1 should define a more generic signature (e.g.,render_input(format: str, ...)) or document that the Protocol will be extended in a backwards-compatible way when formatters are added.🤖 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 `@work/tool-renderer-plugins.md` around lines 381 - 382, The Protocol currently hardcodes render_input_markdown and render_input_html but the design text references render_input_<format> for arbitrary formats; update the Protocol to use a generic renderer signature (e.g., render_input(format: str, ...) and similarly render_output(format: str, ...)) or explicitly document that new format-specific methods will be added in a backwards-compatible manner; modify the Protocol definition (where render_input_markdown and render_input_html are declared) to replace/augment them with the generic render_input and render_output signatures and update any references in the docs to mention render_input_<format> as an implementation pattern or the compatibility commitment.
211-211: 💤 Low valueClarify the detail-level comparison direction.
The comment "'low' satisfies 'low' / 'medium' / 'high' / 'full' descending" is slightly ambiguous. Consider rephrasing to make the hierarchy explicit: e.g., "returns True when
detailis at or abovemin_detailin the hierarchy (low < medium < high < full)" to clarify that "low" is the minimum and "full" is the maximum detail level.🤖 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 `@work/tool-renderer-plugins.md` at line 211, The comment for the helper "_detail_ge" is ambiguous about ordering; update the text to explicitly state the hierarchy and comparison direction (low < medium < high < full) and that the function returns true when the current detail level is at or above the required minimum; e.g., replace the line with a clearer phrasing like: "Returns true when `detail` is at or above `min_detail` in the hierarchy (low < medium < high < full); e.g., 'low' satisfies 'low', 'medium', 'high', 'full' only when used as the minimum."
258-258: ⚡ Quick winClarify what priority
-5supersedes.The comment states "supersede the generic ToolUseContent fallback," but based on the earlier description (line 37-38), the
ToolUseContentfallback is what happens when no renderer is registered for a tool. Priority-5actually supersedes any builtin renderer that might be registered at priority0, not the "no-renderer-found" fallback. Consider rephrasing to "supersede any builtin renderer" for accuracy.🤖 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 `@work/tool-renderer-plugins.md` at line 258, Update the explanatory comment describing "priority = -5" so it accurately states what is being superseded: replace "supersede the generic ToolUseContent fallback" with wording like "supersede any builtin renderer (e.g., renderers registered at priority 0)" or "supersede builtin renderers" so it no longer implies it overrides the no-renderer-found ToolUseContent fallback; reference the priority assignment "priority = -5" and the earlier "ToolUseContent" fallback in the text to make the distinction clear.
176-179: ⚡ Quick winAvoid instantiating the renderer class just for validation.
Lines 176 and 179 instantiate the renderer class twice: once to check
isinstance(cls(), ToolRenderer)and again to append to candidates. Instantiating for validation can trigger constructor side effects and is inefficient. SinceToolRendereris marked@runtime_checkable, consider validating the class structure without instantiation (e.g., check for required class attributes), or instantiate once and validate the instance.♻️ Alternative validation approach
except Exception as e: warn(f"failed to load tool renderer plugin {ep.name!r}: {e}") continue - if not isinstance(cls, type) or not isinstance(cls(), ToolRenderer): - warn(f"plugin {ep.name!r} does not implement ToolRenderer") + if not isinstance(cls, type): + warn(f"plugin {ep.name!r} is not a class") continue - candidates[cls.tool_name].append(cls()) + instance = cls() + if not isinstance(instance, ToolRenderer): + warn(f"plugin {ep.name!r} does not implement ToolRenderer") + continue + candidates[instance.tool_name].append(instance)🤖 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 `@work/tool-renderer-plugins.md` around lines 176 - 179, The code instantiates cls twice when validating plugin entries (using isinstance(cls(), ToolRenderer)) which can cause side effects and inefficiency; change the check to avoid double construction by either (a) creating a single instance once (instance = cls()) then validating with isinstance(instance, ToolRenderer) and appending that instance to candidates[cls.tool_name], or (b) perform a class-level validation (e.g., check for required attributes/methods on cls or use typing.get_type_hints) and only instantiate when adding to candidates; update the block referencing cls, ToolRenderer, candidates, and ep.name to use one of these approaches so constructors are not called more than once during validation.
124-124: ⚡ Quick winClarify the
OutputModeltype annotation.The annotation
type[Any] | Noneis ambiguous. IfOutputModelis a ClassVar that holds a dataclass type, it should be annotated astype | None(since dataclasses aren't parameterized at the type level) or more precisely as the actual expected supertype. Thetype[Any]construction suggests a generic type, but dataclasses don't use that pattern.📝 Suggested annotation
- OutputModel: ClassVar[type[Any] | None] # dataclass; None for unstructured + OutputModel: ClassVar[type | None] # dataclass; None for unstructured🤖 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 `@work/tool-renderer-plugins.md` at line 124, OutputModel's annotation is ambiguous—replace the current ClassVar[type[Any] | None] with a non-generic class-type annotation: either ClassVar[type | None] (Python 3.10+) or ClassVar[Type[Any] | None] from typing to clearly indicate it holds a class object, or even better use ClassVar[Type[ExpectedBase] | None] (replace ExpectedBase with the actual dataclass/base class) if there is a known supertype to make the intent explicit; update the declaration of OutputModel accordingly.
🤖 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 `@work/tool-renderer-plugins.md`:
- Around line 198-201: The documentation for _dispatch_format is missing how to
choose which method on winners[tool_name] to call; update the text to explicitly
say that _dispatch_format should inspect the content object's type (e.g.,
isinstance(content, ToolUseContent) vs ToolResultContent) and the requested
output format, then call the corresponding renderer method on winners[tool_name]
(e.g., call render_input_markdown for ToolUseContent when format == "markdown",
or render_output_markdown for ToolResultContent when format == "markdown"); add
a short pseudocode example that shows selecting by type and format and invoking
winners[tool_name].render_input_markdown(...) or
winners[tool_name].render_output_markdown(...) so readers see the exact dispatch
pattern.
---
Nitpick comments:
In `@work/tool-renderer-plugins.md`:
- Around line 381-382: The Protocol currently hardcodes render_input_markdown
and render_input_html but the design text references render_input_<format> for
arbitrary formats; update the Protocol to use a generic renderer signature
(e.g., render_input(format: str, ...) and similarly render_output(format: str,
...)) or explicitly document that new format-specific methods will be added in a
backwards-compatible manner; modify the Protocol definition (where
render_input_markdown and render_input_html are declared) to replace/augment
them with the generic render_input and render_output signatures and update any
references in the docs to mention render_input_<format> as an implementation
pattern or the compatibility commitment.
- Line 211: The comment for the helper "_detail_ge" is ambiguous about ordering;
update the text to explicitly state the hierarchy and comparison direction (low
< medium < high < full) and that the function returns true when the current
detail level is at or above the required minimum; e.g., replace the line with a
clearer phrasing like: "Returns true when `detail` is at or above `min_detail`
in the hierarchy (low < medium < high < full); e.g., 'low' satisfies 'low',
'medium', 'high', 'full' only when used as the minimum."
- Line 258: Update the explanatory comment describing "priority = -5" so it
accurately states what is being superseded: replace "supersede the generic
ToolUseContent fallback" with wording like "supersede any builtin renderer
(e.g., renderers registered at priority 0)" or "supersede builtin renderers" so
it no longer implies it overrides the no-renderer-found ToolUseContent fallback;
reference the priority assignment "priority = -5" and the earlier
"ToolUseContent" fallback in the text to make the distinction clear.
- Around line 176-179: The code instantiates cls twice when validating plugin
entries (using isinstance(cls(), ToolRenderer)) which can cause side effects and
inefficiency; change the check to avoid double construction by either (a)
creating a single instance once (instance = cls()) then validating with
isinstance(instance, ToolRenderer) and appending that instance to
candidates[cls.tool_name], or (b) perform a class-level validation (e.g., check
for required attributes/methods on cls or use typing.get_type_hints) and only
instantiate when adding to candidates; update the block referencing cls,
ToolRenderer, candidates, and ep.name to use one of these approaches so
constructors are not called more than once during validation.
- Line 124: OutputModel's annotation is ambiguous—replace the current
ClassVar[type[Any] | None] with a non-generic class-type annotation: either
ClassVar[type | None] (Python 3.10+) or ClassVar[Type[Any] | None] from typing
to clearly indicate it holds a class object, or even better use
ClassVar[Type[ExpectedBase] | None] (replace ExpectedBase with the actual
dataclass/base class) if there is a known supertype to make the intent explicit;
update the declaration of OutputModel accordingly.
🪄 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: 162e44f7-f2d8-44d3-b952-be1187a2267a
📒 Files selected for processing (1)
work/tool-renderer-plugins.md
4 tasks
Cross-cuts with PR #167 (alice's dev/filter-hook-turns): the hard-coded [clmail]/[monitor] regex in detect_hook_notification() should live with the plugin that causes those notifications, not in core. Same discovery/priority machinery covers both renderers and transformers. Key design landings (after a back-and-forth with alice via clmail #3088-#3104): - Single entry-point group `claude_code_log.plugins`; loader type-dispatches each entry on Protocol conformance (ToolRenderer vs MessageTransformer). No aggregator class. - Plugins own their MessageContent subclasses; visibility lives on the class via `detail_visibility: ClassVar[DetailLevel]`. Built-in migration from `_HIGH_EXCLUDE_CLASSES` is a follow-up PR, doesn't block #166. - Transformers run inside the factory dispatch chain (not post-factory), preserving existing ordering invariants. - Built-in detector priorities exposed as module constants with gaps of 100 so plugins position relative without renumbering. - v1: first-non-None-wins, no transformer chaining; applies_to accepts any MessageContent subtype (no user-only restriction). Worked example now reduces alice's detect_hook_notification() to ~12 lines as a plugin transformer that owns UserHookNotificationMessage outright.
cboos
changed the title
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)
work/tool-renderer-plugins.md (1)
203-205:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify parse_output behavior—Protocols don't provide defaults.
The comment on line 204 says "default: identity (raw dict) or instantiate OutputModel" but
Protocolmethods don't have default implementations. This comment is misleading because it suggests the Protocol provides a default behavior, when actually it's describing what a conforming implementation should do.Consider rephrasing to: "Implementations should return the raw dict when OutputModel is None, otherwise instantiate OutputModel from the toolUseResult field" or moving this guidance to prose outside the Protocol definition.
🤖 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 `@work/tool-renderer-plugins.md` around lines 203 - 205, The comment on the Protocol method parse_output is misleading because Protocols don't provide defaults; update the docstring/comment for parse_output to state implementation guidance rather than implying a default implementation — e.g., explain that implementations of parse_output (signature: def parse_output(raw: dict, message: TemplateMessage) -> Any | None) should return the raw dict when there is no OutputModel, or instantiate and return the OutputModel using the toolUseResult field when OutputModel is present; alternatively move this guidance out of the Protocol and into adjacent prose/docs.
♻️ Duplicate comments (1)
work/tool-renderer-plugins.md (1)
406-409:⚠️ Potential issue | 🟠 MajorDispatch method selection still needs specification.
The integration point description (lines 406-408) states that
_dispatch_formatconsultswinners[tool_name]but still doesn't specify which method on the winner to invoke or how to distinguish betweenToolUseContent(→render_input_markdown) andToolResultContent(→render_output_markdown) based on content type and output format.This gap was flagged in a previous review and remains unaddressed.
🤖 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 `@work/tool-renderer-plugins.md` around lines 406 - 409, Clarify and implement the dispatch rule for _dispatch_format: when looking up winners[tool_name], call render_input_markdown on the winner if the content is an instance of ToolUseContent and the requested output format is "markdown", and call render_output_markdown on the winner if the content is an instance of ToolResultContent and the output format is "markdown"; otherwise fall back to existing format_<ClassName> MRO handlers or a default renderer. Update the _dispatch_format description to state these selection rules explicitly and reference the class checks (ToolUseContent, ToolResultContent) and the two methods (render_input_markdown, render_output_markdown) so implementers know which method to invoke for each content type and format.
🤖 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 `@work/tool-renderer-plugins.md`:
- Around line 286-291: The docs currently say the loader "binds"
format_<ClassName> and title_<ClassName> functions but never state how they are
discovered; update the text to explicitly describe the discovery mechanism used
by the loader: document that the loader introspects the plugin module for
callables named format_<OutputClassName> and title_<OutputClassName> and that
_register_format_methods(cls.OutputClass) will look up those names on the module
and bind them to the renderer class, and also state any fallback (e.g., if a
plugin can alternatively expose a format_methods mapping attribute, list the
precedence: explicit mapping > named functions > no-op). Mention the exact
naming pattern (format_ and title_ prefixes), the lookup behavior (callable
only), and that non-callables are ignored so plugin authors know how to
implement their functions.
- Around line 296-302: The example assigns DetailLevel.FULL (an Enum-like name)
but DetailLevel is a Literal type; update the class attribute detail_visibility
in UserHookNotificationMessage (and the other class at the later occurrence) to
use the string literal "full" instead of DetailLevel.FULL so the type matches
the Literal["full","high","medium","low"] definition.
---
Outside diff comments:
In `@work/tool-renderer-plugins.md`:
- Around line 203-205: The comment on the Protocol method parse_output is
misleading because Protocols don't provide defaults; update the
docstring/comment for parse_output to state implementation guidance rather than
implying a default implementation — e.g., explain that implementations of
parse_output (signature: def parse_output(raw: dict, message: TemplateMessage)
-> Any | None) should return the raw dict when there is no OutputModel, or
instantiate and return the OutputModel using the toolUseResult field when
OutputModel is present; alternatively move this guidance out of the Protocol and
into adjacent prose/docs.
---
Duplicate comments:
In `@work/tool-renderer-plugins.md`:
- Around line 406-409: Clarify and implement the dispatch rule for
_dispatch_format: when looking up winners[tool_name], call render_input_markdown
on the winner if the content is an instance of ToolUseContent and the requested
output format is "markdown", and call render_output_markdown on the winner if
the content is an instance of ToolResultContent and the output format is
"markdown"; otherwise fall back to existing format_<ClassName> MRO handlers or a
default renderer. Update the _dispatch_format description to state these
selection rules explicitly and reference the class checks (ToolUseContent,
ToolResultContent) and the two methods (render_input_markdown,
render_output_markdown) so implementers know which method to invoke for each
content type and format.
🪄 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: 73f094c5-7d61-4b0f-8589-0270d555a16a
📒 Files selected for processing (1)
work/tool-renderer-plugins.md
Reverses the plugin-owned-classes direction added in 356fc0c. Per mid-thread consensus (alice clmail #3106, main clmail #3108): existing-variants-only is the simpler v1 surface and matches PR #167 today. Plugin-owned MessageContent subclasses become a v2 extension that v1 does not foreclose (purely additive migration). Decisive argument: filter-bucket inheritance ("visibility is a property of the target class, owned by core, inherited by every transformer that targets it") removes the need for any detail_visibility class attribute, plugin-side renderer registration, or built-in migration. The contract reduces to a single method (transform) returning an instance of an existing core MessageContent variant. Practical consequence for #167's eventual plugin migration: deletion-only refactor in core (remove the source tuple, regex, and call site). Class, renderer, title, and _HIGH_EXCLUDE_CLASSES membership all remain in core unchanged. Also addresses CodeRabbit feedback on PR #166: adds a _dispatch_format invocation-pattern section spelling out how the dispatcher picks render_input_markdown vs render_output_markdown based on content type (ToolUseContent vs ToolResultContent) and output format (markdown vs html with mistune fallback).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
work/tool-renderer-plugins.md (2)
673-680:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove transformer format-binding from the v1 test plan; it contradicts the stated contract.
Line 679 still calls for testing “format-method binding for transformer-defined classes,” but this RFC explicitly scopes v1 transformers to rewriting into existing core variants only (no plugin-owned classes, no plugin-side format/title registration). Keeping this test target will misdirect implementation work.
🤖 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 `@work/tool-renderer-plugins.md` around lines 673 - 680, Remove the "format-method binding for transformer-defined classes" test target from the v1 test plan: locate the bullet that lists "format-method binding for transformer-defined classes" (paired with Plugin loader unit tests / Protocol dispatch items) and delete that clause so v1 tests only cover rewriting into core variants; ensure references to plugin-owned class format/title registration and transformer-side binding are not present and keep Protocol dispatch tests (ToolRenderer vs MessageTransformer) unchanged.
159-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit runtime validation for Protocol-discovered plugin metadata/methods
The discovery loop routes purely via
isinstance(instance, ToolRenderer)/isinstance(instance, MessageTransformer), but@runtime_checkableProtocols makeisinstance()check only that required members exist—not that their types match annotations or that callable signatures/return contracts are correct. Since the pseudocode immediately usescls.tool_name, sorts byp.priority, and later consultsplugin.min_detail(and calls methods likeparse_output/render_*/transform), malformed plugins can slip through and fail later.Add a registration-time validation step (before adding candidates) that checks each required class attribute’s runtime type/value (e.g.,
tool_name: str,priority: int,min_detailwithinDetailLevel,applies_toshape) and that required methods are callable with the expected call/return behavior; fail fast with a clear warning/error and skip the plugin.🤖 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 `@work/tool-renderer-plugins.md` around lines 159 - 163, Add a registration-time validation step in the discovery loop that, for each candidate class that passed isinstance(..., ToolRenderer) or MessageTransformer, validates required attributes and callables before adding to candidates: check cls.tool_name is a str, p.priority (or cls.priority) is an int, plugin.min_detail is a valid DetailLevel value, and that applies_to has the expected shape; also verify required methods (parse_output, render_*, transform) are present and callable and optionally smoke-test their call/return shape (eg. call with minimal valid args or inspect signatures) and on failure log a clear error/warning and skip that plugin. Ensure you place this validation immediately before using cls.tool_name / p.priority / plugin.min_detail in the dispatch-building code so malformed plugins are rejected early.
🤖 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.
Outside diff comments:
In `@work/tool-renderer-plugins.md`:
- Around line 673-680: Remove the "format-method binding for transformer-defined
classes" test target from the v1 test plan: locate the bullet that lists
"format-method binding for transformer-defined classes" (paired with Plugin
loader unit tests / Protocol dispatch items) and delete that clause so v1 tests
only cover rewriting into core variants; ensure references to plugin-owned class
format/title registration and transformer-side binding are not present and keep
Protocol dispatch tests (ToolRenderer vs MessageTransformer) unchanged.
- Around line 159-163: Add a registration-time validation step in the discovery
loop that, for each candidate class that passed isinstance(..., ToolRenderer) or
MessageTransformer, validates required attributes and callables before adding to
candidates: check cls.tool_name is a str, p.priority (or cls.priority) is an
int, plugin.min_detail is a valid DetailLevel value, and that applies_to has the
expected shape; also verify required methods (parse_output, render_*, transform)
are present and callable and optionally smoke-test their call/return shape (eg.
call with minimal valid args or inspect signatures) and on failure log a clear
error/warning and skip that plugin. Ensure you place this validation immediately
before using cls.tool_name / p.priority / plugin.min_detail in the
dispatch-building code so malformed plugins are rejected early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5bb92490-bb16-4326-86d8-c94837296039
📒 Files selected for processing (1)
work/tool-renderer-plugins.md
Per alice's read-pass flag #3 (clmail #3119): the RFC stated the principle ("UserTextMessage.text byte-equivalent to extract_text_content") but didn't pin what enforces it. A future factory PR sneaking in normalization between extraction and assignment would silently break plugin regex behaviour. Add a sentence naming the enforcement mechanism: a dedicated equivalence test in the plugin-system test suite that walks the existing JSONL test corpus and asserts byte-equality for every user entry. A normalization-introducing PR fails this test, surfacing the contract break.
This commit substantially reverses the existing-variants-only v1 scope that landed in 74b5ca6. The architectural shape changes from two parallel plugin Protocols (ToolRenderer + MessageTransformer) to one unified MessageTransformer that handles both tool-rendering and hook-demotion cases. Triggering question (from user via main, clmail #3132): "What if we'd solve the primary need (rendering of generic tools) via v2 as well? That is, intercept the generic tool use/result messages emitted for mcp__plugin_clmail_clmail__communicate (and al.) and convert those to specific messages (ClMailToolUse, ClMailToolResult), THEN register parsing methods for them." Main delegated the decision; I (carol) committed YES (unify) in clmail #3133 with concrete mechanics; alice gave cautious yes in clmail #3135 with two technical asks, both folded in here. Architectural justification: - _dispatch_format already does MRO walk + class-based dispatch. - Built-in tools already use specialized subclasses (BashInputContent and friends) flowing through that dispatch. - The winners[tool_name] table in earlier RFC drafts was a workaround for plugins not having method-binding; eliminate the workaround and one mechanism (transformers + plugin-defined subclasses with class-side format/title methods) suffices for both cases. What the RFC now specifies: - Single MessageTransformer Protocol: matches MessageContent (any subtype via applies_to MRO filter), returns a plugin-defined subclass. - Plugin-defined MessageContent subclasses carry format_markdown / format_html / title as methods on themselves (class-method pattern, not renderer monkey-patching, not global registry). - detail_visibility ClassVar[DetailLevel] on the class governs filter-bucket membership; monotone-down semantics (rendered iff current_detail >= cls.detail_visibility). - _HIGH_EXCLUDE_CLASSES bridge for built-ins not yet migrated; resolution order pinned: class attribute first, registry second. - _dispatch_format resolution: renderer-side format_<ClassName> first (preserves all built-in dispatch unchanged), class-side format_<output> second, MRO walk continues. - Test-embedded reference plugin in test/_plugins/clmail/ doubles as layer-4 fixture and canonical author example. What this costs: - v1 surface expands. Plugins define MessageContent subclasses, register format-method contributions, declare detail_visibility. These were "v2" in earlier RFC drafts; "v1" now. - Rendering philosophy shifts: content classes now carry format_* methods. Today's classes are pure data. Deliberate expansion. - PR #167's eventual plugin migration becomes a full relocation (class + format/title + CSS + tests all move) rather than a deletion-only refactor in core. Tractable; one-shot. A new "Reversal context and trade-offs" section in the RFC names this explicitly so future readers understand the trajectory, not just the destination (per alice's framing ask in clmail #3135). PR #167 stays parked per main's standing instruction; user decides whether to merge as-is and migrate later, or hold.
cboos
changed the title
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 `@work/tool-renderer-plugins.md`:
- Around line 423-428: The current tie-break uses type(t).__name__ in the sort
key (transformers.sort(key=lambda t: (t.priority, type(t).__name__))) which can
collide across plugins; change the tie-break to use a fully-qualified identifier
(e.g., module + qualname or the plugin/entry-point name) instead of
type(t).__name__, and ensure the warning loop that checks ties (for a, b in
zip(transformers, transformers[1:]): if a.priority == b.priority and
a.applies_to == b.applies_to: warn(...)) still reflects the new key so startup
ordering is stable across environments and reports the chosen fully-qualified
types in the warn message.
- Around line 317-333: The pseudocode in _dispatch_format incorrectly drops the
separate message argument and incorrectly reads content.message; update
_dispatch_format to accept the message parameter and pass that message to both
renderer-side handlers (call renderer_method(content, message)) and class-side
handlers without reading content.message (call class_method(self, content,
message) or class_method(self, content, message) consistent with how
format_<output> is defined), ensuring you do not access content.message
anywhere.
🪄 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: 96adb911-22e9-496e-b9b8-cec6c71014cc
📒 Files selected for processing (1)
work/tool-renderer-plugins.md
cboos
commented
May 23, 2026
Collaborator
Author
cboos
left a comment
cboos
left a comment
There was a problem hiding this comment.
(Claude) Read pass on 75153dc — acking the unified design.
Both technical asks from my clmail #3135 land cleanly:
_dispatch_format resolution order. The new section pins it precisely: MRO walk, renderer-side format_<ClassName> first (preserving all built-in dispatch unchanged), class-side format_<output> second, MRO walk continues. The four-way matrix table covers the edge cases; the worked example showing how ClMailCommunicateInput shadows format_ToolUseContent via MRO order makes the precedence rule concrete. No regression risk for built-ins.
detail_visibility semantics + built-in bridge. Monotone-down semantics (current_detail >= cls.detail_visibility) pinned with explicit enum ordering. Three-step resolution order (class attribute → _HIGH_EXCLUDE_CLASSES bridge → default visible) handles the migration period cleanly; built-ins stay unaltered in v1, can migrate one-at-a-time in a follow-up.
Reversal acknowledgement at the top of the doc + the dedicated "Reversal context and trade-offs" section + the verbatim user-question quote in the commit message all do the right thing for future readers — the trajectory is visible, not just the destination.
The central architectural insight (_dispatch_format already does MRO walk + class-based dispatch; the winners[tool_name] table was a workaround for plugins not having method-binding) is sound. The class-method-on-subclass pattern resolves my old #3119 binding-mechanism hand-wave without needing monkey-patching or a global registry — methods live where the MRO walk naturally finds them.
PR #167 (dev/filter-hook-turns) stays parked per the standing instruction. Under unified-v1 its eventual migration is a full relocation (class + format/title + CSS + tests all move to the clmail plugin) rather than the deletion-only refactor that existing-variants-only would have allowed — bigger one-shot but tractable, and the user can still merge it as-is in the interim.
No further flags from this pass. Ready for routing.
Collaborator
Author
|
Funny how this time the agents decided to "go public" and exchange on the PR. This didn't happen so far. I'll make them refrain from doing that in the future, but given this PR is about supporting mcp tools in general and that clmail will be used as an example to illustrate how to write such a plugin, the fact that conversations get visible here a bit is not completely inappropriate (to close the loop, I could even add those sessions in the test/data/real_sessions...). |
1. _dispatch_format pseudocode signature (CR #3293284687): pass message argument explicitly through both strategies rather than reading content.message (which is not part of the MessageContent contract). Plugin-defined class-side format methods already had the (self, renderer, message) signature throughout the worked example; only the dispatcher pseudocode was inconsistent. Added a paragraph below the pseudocode spelling out the call contract and the rationale for not storing a back-pointer on MessageContent. 2. Tie-break key stability (CR #3293284688): use (priority, __module__, __qualname__) instead of (priority, __name__) so two plugins shipping classes with identical short names don't get OS-dependent ordering. Warn message updated to print fully-qualified types.
4 tasks
cboos
added a commit
that referenced
this pull request
May 25, 2026
* Add plugin loader infrastructure (plugins.py, priorities.py)
Implements the unified plugin discovery + dispatch machinery from
the RFC at work/tool-renderer-plugins.md:
- claude_code_log.plugins module: MessageTransformer Protocol
(runtime_checkable with required ClassVar metadata), entry-point
loader at group "claude_code_log.plugins", priority sort using
(priority, __module__, __qualname__) for stable cross-environment
ordering, tie-break warning on duplicate (priority, applies_to),
module-level cache with reset hook for tests.
- claude_code_log.factories.priorities module: public constants for
built-in detector positions (COMMAND_MESSAGE=100, ...,
HOOK_NOTIFICATION=600, ..., TEXT_FALLBACK=1000, TOOL_INPUT_GENERIC=5000,
TOOL_OUTPUT_GENERIC=5100). Plugins import these to position
themselves relative to built-ins without renumbering on every core
change. Gaps of 100 leave room for plugin insertion.
apply_transformers() is the helper factories call: walks the
priority-ordered list, calls transform() on the first transformer
whose applies_to subclass-matches the candidate, returns first
non-None replacement. Transformer exceptions are caught and logged
so a buggy plugin can't crash conversion.
No call sites are wired yet; that's the next commit. Loader stands
alone and is fully covered by upcoming layer-1 tests.
* Wire apply_transformers into the user and tool factories
Three insertion points where the factory finishes building its
candidate MessageContent and the plugin pass should run:
- create_user_message: split into a thin wrapper + the existing body
renamed to _classify_user_message. The wrapper applies the plugin
transformer pass to the result, so every classification path
(slash-command, bash, teammate, task-notification, hook-detect,
generic text) becomes rewriteable by a plugin, not just the
fallback. Transformers whose applies_to doesn't subclass-match
pass through with no-op cost.
- create_tool_use_message: apply_transformers after ToolUseMessage
construction, before the ToolItemResult return.
- create_tool_result_message: apply_transformers after
ToolResultMessage construction, before the ToolItemResult return.
Without any plugin installed, all three sites are no-ops (the
loader cache is empty). With a plugin installed, transformers can
specialize the generic MessageContent into plugin-defined
subclasses (e.g. ClmailCommunicateInputMessage from generic
ToolUseMessage) and the renderer's _dispatch_format picks them up
via the existing MRO walk plus the upcoming class-side fallback.
The RFC's "in-factory-dispatch interleaved with built-in
detectors" framing is implemented here as a *post-classification*
pass: built-in detectors run first in their hardcoded order,
plugin transformers run after. The effect is the same for every
v1 use case (clmail hook-demotion, MCP tool rendering) because
plugins always operate on a candidate the built-in chain has
already classified (typically UserTextMessage or generic
ToolUseMessage). Documented as a deviation in the plugins.py
module docstring.
* Extend _dispatch_format with class-side fallback; add detail_visibility bridge
Two related changes that let plugin-defined MessageContent subclasses
participate in rendering and filtering without modifying core.
_dispatch_format / _dispatch_title resolution-order extension
==============================================================
Both dispatchers now use a two-strategy resolution per MRO node:
1. Renderer-side format_<ClassName>(self, obj, message) — preserves
all existing built-in dispatch unchanged. The renderer class
continues to carry hand-written format_BashInput,
format_ToolUseMessage, etc.
2. Class-side format_<output>(self, renderer, message) on the
content class itself (where <output> is "markdown" or "html"
per Renderer._class_dispatch_format). Used by plugin-defined
subclasses that carry their own render methods.
Renderer-side wins first per MRO node. A plugin subclass that wants
to shadow a built-in's renderer method defines the class-side method
on the plugin subclass — the MRO walk visits it first.
HtmlRenderer overrides _class_dispatch_format = "html" so plugin
classes' format_html is consulted (with format_markdown as the
mistune fallback at the call site, not the dispatcher).
detail_visibility class attribute + _HIGH_EXCLUDE_CLASSES bridge
================================================================
A MessageContent subclass may declare detail_visibility: ClassVar[DetailLevel]
to opt into class-based filter membership. Monotone-down semantics:
a message is visible iff current_detail is at least as verbose as
the declared minimum, using the canonical DetailLevel ordering
FULL > HIGH > LOW > MINIMAL > USER_ONLY pinned in a module-level
_DETAIL_ORDER map.
_content_visible_at() helper resolves visibility in three steps:
1. Class attribute (plugin classes + future migrated built-ins)
2. _HIGH_EXCLUDE_CLASSES / _LOW_EXCLUDE_CLASSES / etc. bridge for
built-ins not yet migrated (isinstance semantics, so plugin
subclasses of a built-in inherit its filter membership unless
they override).
3. Default visible.
_filter_template_by_detail rewritten to call this helper. The LOW
keep-list (_LOW_KEEP_TOOLS = {"WebSearch","WebFetch","Task","Agent"})
is a positive filter at LOW (ToolUseMessage isn't in
_LOW_EXCLUDE_CLASSES, only in _MINIMAL_EXCLUDE_CLASSES) and bypassed
for plugin classes that declare detail_visibility — so a clmail
communicate plugin with detail_visibility=LOW shows up at --detail
low without core needing to update _LOW_KEEP_TOOLS.
Built-in migration to detail_visibility is a follow-up; the bridge
keeps existing behaviour identical for the 1892 unit tests.
* Add test-embedded reference plugin (test/_plugins/clmail/)
A minimal claude-code-log-clmail-test package that exercises both
sides of the plugin contract and doubles as the canonical example
for third-party plugin authors.
Layout:
test/_plugins/clmail/
pyproject.toml # entry-point declarations
README.md # author-facing one-pager
src/claude_code_log_clmail_test/
__init__.py
transformers/
hook_demotion.py # UserTextMessage rewrite via "[testhook]" prefix
tool_communicate.py # ToolUseMessage rewrite for a specific MCP tool
Both transformers exercise the full v1 contract:
- Class-side format_markdown / format_html / title methods on
plugin-defined MessageContent subclasses (Strategy 2 of the new
_dispatch_format).
- detail_visibility ClassVar[DetailLevel] for filter-bucket
membership without touching core registries.
- applies_to MRO filter targeting an existing core class
(UserTextMessage, ToolUseMessage).
The tool_communicate transformer targets a stable test-fixture tool
name (mcp__test_plugin__clmail__communicate) so it doesn't collide
with any real tool that production fixtures might emit.
Wired as a dev-dependency via uv.sources with `path = ...,
editable = true`, so `uv sync` installs it automatically. Production
installs of claude-code-log don't see it; CI does. Forcing a
reinstall after pyproject changes: `uv sync --reinstall-package
claude-code-log-clmail-test`.
* Add plugin-system tests (loader / dispatch / transformers / equivalence)
Four layers covering the contract end-to-end, per the RFC's test
strategy section.
Layer 1 — loader unit tests (no fixtures, no entry points):
- _validate_transformer_class rejects missing name, non-int
priority, empty applies_to, non-MessageContent applies_to entry.
- _sort_and_warn orders by (priority, module, qualname) and emits
a "priority tie" warning on duplicate (priority, applies_to).
- load_transformers caches at module scope; force_reload=True
invalidates the cache.
Layer 2 — _dispatch_format four-way matrix:
- both renderer-method and class-method present → renderer wins
- renderer-method only → renderer wins
- class-method only → class wins
- neither → returns empty / None
- HtmlRenderer subclass dispatches format_html via Strategy 2
(verifies _class_dispatch_format override at the class level)
Layer 3 — transformer integration (uses the embedded reference plugin):
- Matching "[testhook]" text demotes to TestHookNotificationMessage.
- Non-matching text passes through as UserTextMessage.
- Multi-line guard: real prompts starting with "[testhook]" but
containing newlines pass through.
- Specialized tool subclass renders via class-side format_markdown
and title methods.
- apply_transformers catches plugin exceptions, logs, passes through.
Layer 4 — text-equivalence guarantee:
- Walks the existing JSONL test corpus (dag_simple/dag_fork/cron_tools).
- For each UserTextMessage candidate, asserts the joined text from
UserTextMessage.items matches what the factory's
extract_text_content would have produced. Catches future factory
PRs that sneak normalization between extraction and assignment.
Tests that depend on the embedded plugin use @reference_plugin_required
to skip cleanly when it's not installed (the package is a dev-dep so
`uv sync` brings it in; this skip is for stray environments).
All 21 plugin-system tests pass. Full unit-test suite: 1892 passed,
7 skipped. just ci clean.
* Address monk's 3 non-blocking notes from PR #169 review
1. MessageTransformer Protocol docstring: document the v1 trust
contract that transform()'s return SHOULD be an instance of
one of applies_to (or a subclass). v1 doesn't runtime-enforce
this; caller's typing narrowing (e.g. UserMessageContent in
create_user_message) assumes it. A v2 enhancement may add an
isinstance check.
2. renderer.py: assert _DETAIL_ORDER covers all DetailLevel values
at module load. Without this, adding a new DetailLevel value
would surface as a KeyError on first visibility check —
silent until exercised. Fail loudly at import instead.
3. tool_communicate.py: clarify the priority comment in the
embedded reference plugin. Under the v1 post-classification
implementation, priority orders transformers among themselves
(not against built-in classifiers, which have already run).
The earlier "Run before generic tool classification" phrasing
was mildly misleading for plugin-author readers consulting the
canonical example.
All three are doc / safety-rail changes; no behavioural impact.
just ci clean.
* Address 5 CR comments on PR #169
1. plugins.py:_sort_and_warn — group by (priority, applies_to) via
a dict instead of pairwise-adjacent zip. The sort key is
(priority, module, qualname), so two transformers with the same
priority but different applies_to can sit between two genuine
collision partners; the adjacent check would have missed them.
Group-by surfaces every collision regardless of sort position.
2. plugins.py:apply_transformers — runtime type-enforcement
upgrade. v1 now actively rejects transformer returns that
aren't MessageContent instances OR don't subclass-match
applies_to. Both monk (#3175) and CR independently flagged
this; promoting from documented-trust-contract to
actively-enforced makes plugin authoring errors loud and
keeps a buggy plugin from crashing downstream invisibly.
3. renderer.py:title_content — delegate to _dispatch_title
instead of re-implementing the renderer-only MRO walk inline.
Without this, plugin ToolUseMessage/ToolResultMessage
subclasses' class-side title() methods are silently shadowed
at the top level by title_ToolUseMessage / title_ToolResultMessage
on the base renderer. Snapshot regression caught a related
bug: title_ToolResultMessage returns "" (empty string,
meaning "no header") for non-error results; the walrus
truthy-check would incorrectly fall through to the
message_type default. Replaced with explicit
`if title is not None` to preserve the empty-string
sentinel semantics.
4. hook_demotion.py — move the multi-line guard to a pre-regex
whole-text check. The pattern's \s* after [testhook] would
otherwise consume a newline immediately after the marker,
slipping multi-line prompts past a body-only check. Made the
plugin's TestHookNotificationMessage a UserTextMessage subclass
(not a sibling) so it satisfies the new return-type enforcement.
5. test_plugin_system.py:test_sorts_by_priority — use local
subclasses with explicit priorities instead of mutating
_GoodTransformer's module-level ClassVar. Same concern monk
raised as a parting note.
Plus three new regression tests for the runtime enforcement
(TestApplyTransformersReturnTypeEnforcement), the non-adjacent
collision detection (TestSortAndWarnNonAdjacentCollision), and
the newline-immediately-after-prefix case
(test_newline_immediately_after_prefix_passes_through).
Test summary: 1896 passed, 7 skipped (+4 from previous CR-fixup
commit). just ci clean across all stages.
* dev-docs: add plugin system reference + author guide
Adds dev-docs/plugins.md as the as-built reference for plugin
authors writing third-party plugins via the entry-point group
``claude_code_log.plugins``. Designed so an external actor
(e.g. a clmail-plugin project) can read this single page and
produce a working plugin without further design context.
Covers:
- the MessageTransformer Protocol surface (name / priority /
applies_to / transform);
- the two-strategy _dispatch_format / _dispatch_title resolution
order (renderer-side first per MRO node, class-side second);
- detail_visibility semantics + the FULL > HIGH > LOW > MINIMAL
> USER_ONLY ordering pinned in _DETAIL_ORDER;
- runtime contract enforcement (exception safety + return-type
isinstance check against applies_to);
- entry-point discovery + priority ordering (with the v1
post-classification deviation from the RFC documented);
- first non-None wins semantics;
- a four-layer testing recipe mirroring test_plugin_system.py;
- common patterns and pitfalls (defensive narrowing, format_html
None-fallback to mistune, inline-code backtick widening,
inheritance requirement for return-type enforcement, priority
constants over literals, cache invalidation in tests);
- a v2 directions section so contributors don't keep
re-rediscovering them (interleaved dispatch, renderer-side
plugin extension, priority namespacing).
Heavily references the existing test-embedded reference plugin
at test/_plugins/clmail/ as the canonical example template, per
the RFC's intent that the test plugin double as the author
reference (and stay in sync with working code by construction).
Wires the new doc into both CLAUDE.md and
dev-docs/application_model.md per the dev-docs sync convention.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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
Design proposal for a tool-renderer plugin system. Doc only, no implementation — open for review and inline comment before any code lands.
The full proposal lives in
work/tool-renderer-plugins.md(393 lines, ~10–15 min read). Key shape:importlib.metadata.entry_points(stdlib, groupclaude_code_log.tool_renderers); pluggy considered and rejected as overkill for "one winner per tool name".ToolRendererProtocol bundlingInputModel/OutputModel/render_*_markdown(required) /render_*_html(optional — returnsNoneto fall back to Markdown →mistune).0reserved for builtins;<0supersedes;>0fallback; tie-break = stable alphabetical with a startup warning.min_detailonly (nomax_detail) — rationale in the doc.clmail-communicateplugin that replaces the synthetic[clmail] You've got a new mail (#3076)hook line with the actual mail content at--detail low. This is the driving use case.claude_code_log.formattersgroup without changing the v1 surface.The doc grounds itself in current architecture by file:line —
TOOL_INPUT_MODELS/TOOL_OUTPUT_PARSERS/_dispatch_format/_LOW_KEEP_TOOLS/ scattered icons. Those four surfaces become data-driven from the plugin loader's winners table.Test plan
min_detaildecision (vs addingmax_detail) matches the desired UX.actionfield driving per-action rendering) is the right shape.Once the design is acked, a follow-up PR will land the loader + Protocol + one in-tree builtin example refactored to the plugin shape (proof of concept). The clmail plugin ships as a separate package.
Summary by CodeRabbit