Render the Artifact tool (deploy HTML/MD page → claude.ai URL) (#257)#260
Merged
Conversation
Typed input/output models + HTML and Markdown renderers for the
Artifact tool (Claude Code 2.1.172+): title '🖼️ Artifact <file_path>',
body with description/favicon/label/redeploy metadata, result as
'Published <title> → <url>'. Shapes match the wire format observed on
Claude Code 2.1.198 (toolUseResult {url, path, title}; title is the
page <title> for .html sources, the file basename for .md ones).
The page content never reaches the transcript (the tool takes a file
path), but every string field is transcript-derived: all fields render
escaped (#245 contract) and URLs only become links for http(s) schemes
— a hostile javascript: URL stays inert text in both renderers.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A successful Artifact publish also appends a uuid-less
{type: 'frame-link', path, frameUrl, timestamp} entry mapping the
source file to the deployed claude.ai page — fully redundant with the
Artifact tool_result, so skip it without the 'unrecognized message
type' warning. 'mode' ({mode: 'normal'|...}) joins the same
session-state-snapshot family as 'permission-mode'.
The silent-skip warn-once test now uses a made-up type as its unknown
example since 'mode' is recognized.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Artifact tool support across parsing, models, HTML and Markdown rendering, URL-safety helpers, silent-skip handling, sample transcripts, docs, and tests. ChangesArtifact Tool Support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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.
🧹 Nitpick comments (2)
claude_code_log/models.py (1)
1485-1502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring conflicts with the actual optionality of
favicon.The docstring states "Only
file_pathandfaviconare required in the current schema" (lines 1489-1490), butfavicon: str = ""has a default value, making it optional per Pydantic semantics — onlyfile_pathis actually required. This could mislead future maintainers into assumingfaviconmust always be supplied.📝 Proposed doc fix
- Deploys a self-contained HTML or Markdown file as a (default-private) - claude.ai web page. Only ``file_path`` and ``favicon`` are required in - the current schema; keep the rest optional and tolerate unknown fields - so schema tweaks across Claude Code versions don't drop the message to - the generic params table. + Deploys a self-contained HTML or Markdown file as a (default-private) + claude.ai web page. Only ``file_path`` is required in the current + schema; keep the rest optional and tolerate unknown fields so schema + tweaks across Claude Code versions don't drop the message to the + generic params table.🤖 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/models.py` around lines 1485 - 1502, The ArtifactInput docstring is inconsistent with the model definition because favicon is not required in the schema. Update the ArtifactInput class docstring to reflect the actual required fields, keeping file_path as required and describing favicon as optional/defaulted, so the documentation matches the Pydantic field defaults and optional parameters like description, label, url, and force remain clearly optional.claude_code_log/html/tool_formatters.py (1)
865-875: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate scheme-guard logic vs. Markdown renderer.
_artifact_linkandclaude_code_log/markdown/renderer.py's_markdown_safe_urlimplement the samehttp(s)scheme allow-list independently. Consider extracting a sharedis_safe_web_url(url: str) -> bool(or similar) helper into a common module so the scheme policy (and any future changes to it, e.g. adding case-insensitivity) is maintained in one place instead of two.The
ast-grep"cleartext HTTP" hint on this line is a false positive — this code isn't making an HTTP call, it's just permittinghttp://alongsidehttps://as a renderable link scheme.🤖 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 865 - 875, `_artifact_link` duplicates the same http(s) scheme allow-list already used by `_markdown_safe_url`, so factor that policy into a shared helper such as `is_safe_web_url(url: str) -> bool` in a common module and have both `_artifact_link` and `claude_code_log/markdown/renderer.py` call it. Keep the existing behavior of only emitting an `href` for safe web URLs, and ensure the shared helper is the single place to update scheme rules such as case-insensitive matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@claude_code_log/html/tool_formatters.py`:
- Around line 865-875: `_artifact_link` duplicates the same http(s) scheme
allow-list already used by `_markdown_safe_url`, so factor that policy into a
shared helper such as `is_safe_web_url(url: str) -> bool` in a common module and
have both `_artifact_link` and `claude_code_log/markdown/renderer.py` call it.
Keep the existing behavior of only emitting an `href` for safe web URLs, and
ensure the shared helper is the single place to update scheme rules such as
case-insensitive matching.
In `@claude_code_log/models.py`:
- Around line 1485-1502: The ArtifactInput docstring is inconsistent with the
model definition because favicon is not required in the schema. Update the
ArtifactInput class docstring to reflect the actual required fields, keeping
file_path as required and describing favicon as optional/defaulted, so the
documentation matches the Pydantic field defaults and optional parameters like
description, label, url, and force remain clearly optional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68926176-6a38-43ad-b6dc-f53d41dbdac5
📒 Files selected for processing (15)
claude_code_log/converter.pyclaude_code_log/factories/tool_factory.pyclaude_code_log/html/__init__.pyclaude_code_log/html/renderer.pyclaude_code_log/html/tool_formatters.pyclaude_code_log/markdown/renderer.pyclaude_code_log/models.pydev-docs/messages.mddev-docs/messages/tools/Artifact-tool_result.jsondev-docs/messages/tools/Artifact-tool_result.jsonldev-docs/messages/tools/Artifact-tool_use.jsondev-docs/messages/tools/Artifact-tool_use.jsonltest/test_artifact_rendering.pytest/test_data/artifact_tool.jsonltest/test_silent_skip.py
Address CodeRabbit review on #260: - The http(s) scheme whitelist was implemented twice (_artifact_link in html/tool_formatters.py, _markdown_safe_url in markdown/renderer.py). Extract is_safe_web_url() into claude_code_log/utils.py (format-neutral, import-cycle-free) and delegate both call sites; the policy — including its deliberate fail-closed case-sensitivity — now lives in one place. - Re-export it from claude_code_log.plugins (_PUBLIC_HELPERS + lazy PEP-562 resolution + TYPE_CHECKING import) and document it in dev-docs/plugins.md §4.1/§4.2: a plugin emitting a transcript-derived URL as an href / Markdown link target needs the same gate, since escaping can't neutralise a hostile javascript: scheme. - Fix the ArtifactInput docstring: only file_path is required by the model; favicon is required by the live tool schema but defaulted here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
(Claude) Both review comments addressed in c47d39c:
Full |
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.
Closes #257.
Adds rendering support for the Claude Code
Artifacttool (2.1.172+), which deploys a self-contained HTML or Markdown file as a (default-private) claude.ai web page.What it renders
🖼️ Artifact <file_path>— same full-path recipe as Read/Write; the icon is inside the wrench-suppression whitelist so no double 🛠️.favicon · label · redeploys <url> · forcemeta line (only fields that are present).Published <title> → <url>with a clickable link; error/denial results (e.g. a permission denial) fall back to their text.[url](url)link with a bold page title.Wire format
Field-for-field from the observed wire format on Claude Code 2.1.198 (real captured publishes, sanitized samples in
dev-docs/messages/tools/Artifact-*):tool_use.input:file_path+favicon(required),description/label/url(redeploy target) /forceoptional.ArtifactInputusesextra="allow"so schema drift across CC versions doesn't drop messages to the generic params table.toolUseResult:{url, path, title}—titleis the page<title>for.htmlsources but the file basename for.mdones.Published <path> at <url>on success; plain text +is_error(notoolUseResult) on denial. The parser preferstoolUseResult, falls back to the text line, then to raw text.A successful publish also appends a uuid-less
{type: "frame-link", path, frameUrl, timestamp}bookkeeping entry — fully redundant with the tool_result, so the second commit adds it toSILENT_SKIP_TYPES(together withmode, the same session-state family aspermission-mode).XSS
The page content itself never reaches the transcript (the tool takes a file path), but every string field — description, label, title, path, URL — is transcript-derived:
javascript:URL stays inert escaped text in HTML and inline code in Markdown (new_markdown_safe_urlhelper, parens percent-encoded in link targets).Tests
31 tests in
test/test_artifact_rendering.py(models, parser fallbacks, both renderers, scheme guards) including end-to-end HTML + Markdown runs overtest/test_data/artifact_tool.jsonl— 5 variants: HTML publish, Markdown publish, redeploy+force, classifier-denied error, and a hostile entry with<script>/onerror/javascript:payloads. Fulljust cigreen on this tip (rebased onto #258's main).🤖 Generated with Claude Code
Summary by CodeRabbit