Improve Artifact tool look&feel: label-linked result, lean tool use (#262)#263
Merged
Conversation
…262) Tool use now shows the description as the primary content — the favicon and version label belong to the published page, so the tool result line carries them instead. The redeploy addendum stays as a meta row: a 'redeploys existing artifact' link (URL in href/hover only) plus the force flag spelled out as 'force: overwrite without conflict check' (a normal redeploy sends the artifact's base version so a concurrent write from another session fails instead of being silently clobbered; force skips that check). Tool result becomes 'Published artifact (favicon) (label)': the label — falling back to page title, then source basename, then URL — is the clickable link, keeping the full URL out of the visible text (href + hover title carry it). The favicon renders as a real <img> only when the value is an is_safe_web_url http(s) URL (escaped src, referrerpolicy=no-referrer — the wire result is {url, path, title} and carries no favicon, so it is enriched from the paired tool_use input); the normal emoji value stays an inert text span, over-long non-URL values are dropped. A hostile-scheme result URL still degrades to the escaped URL itself rather than hiding behind a label. Hardening found while testing: the Markdown link-target encoder only percent-encoded parentheses, so an accepted https URL carrying quotes could smuggle attributes through a downstream Markdown→HTML conversion. The new safe_markdown_link_target() also encodes quotes, angle brackets and spaces, applies to every Markdown link/image destination, and is surfaced as a plugin helper (plugins._PUBLIC_HELPERS lazy re-export + dev-docs/plugins.md §4.1/§4.2) alongside is_safe_web_url. Snapshot churn is the appended .artifact-favicon CSS rule only. Closes #262 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up (#262): _artifact_favicon_md hardcoded 16 while the HTML path used a named module constant — same policy, two literals. Hoist ARTIFACT_FAVICON_TEXT_MAX to utils.py (alongside the shared is_safe_web_url policy) so the cap can't drift between renderers. 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR forwards Artifact tool ChangesArtifact rendering and link-safety updates
Estimated code review effort: 3 (Moderate) | ~35 minutes Sequence Diagram(s)sequenceDiagram
participant Artifact tool_use
participant tool_factory
participant HTML/Markdown renderers
Artifact tool_use->>tool_factory: paired input metadata
tool_factory->>tool_factory: create_tool_result_message(tool_result)
tool_factory->>tool_factory: create_tool_output() -> ArtifactOutput
tool_factory->>tool_factory: copy favicon, label from paired tool_use
tool_factory->>HTML/Markdown renderers: ArtifactOutput with favicon, label
HTML/Markdown renderers->>HTML/Markdown renderers: render labeled Published artifact line
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dev-docs/plugins.md (1)
286-300: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale helper count: "Five helpers" is now six.
Line 286 still says "Five helpers are re-exported..." but the import example just below (and the table further down) now lists six:
render_markdown,render_markdown_collapsible,escape_html,safe_markdown_inline,is_safe_web_url,safe_markdown_link_target.📝 Proposed fix
-Five helpers are re-exported from `claude_code_log.plugins` for use +Six helpers are re-exported from `claude_code_log.plugins` for use in `format_html` / `format_markdown` / `title` methods. The re-export is🤖 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 `@dev-docs/plugins.md` around lines 286 - 300, Update the plugins documentation to reflect the current stable API helper count: the opening sentence still says “Five helpers are re-exported” even though the `claude_code_log.plugins` import example and the helper list now include six symbols. Edit the descriptive text in the `plugins.md` section so it matches the actual exported helpers named in the example (`render_markdown`, `render_markdown_collapsible`, `escape_html`, `safe_markdown_inline`, `is_safe_web_url`, `safe_markdown_link_target`).
🤖 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 315-341: safe_markdown_link_target currently percent-encodes only
a few breakout characters, but it also needs to encode ASCII control characters
so transcript-derived URLs cannot terminate a Markdown link destination early.
Update the normalization in safe_markdown_link_target to cover the full control
range (U+0000-U+001F and U+007F) in addition to the existing characters, while
keeping the is_safe_web_url scheme check separate; use the existing
_MARKDOWN_TARGET_ENCODINGS and safe_markdown_link_target symbols to implement
the fix.
---
Outside diff comments:
In `@dev-docs/plugins.md`:
- Around line 286-300: Update the plugins documentation to reflect the current
stable API helper count: the opening sentence still says “Five helpers are
re-exported” even though the `claude_code_log.plugins` import example and the
helper list now include six symbols. Edit the descriptive text in the
`plugins.md` section so it matches the actual exported helpers named in the
example (`render_markdown`, `render_markdown_collapsible`, `escape_html`,
`safe_markdown_inline`, `is_safe_web_url`, `safe_markdown_link_target`).
🪄 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: 63e781e8-b074-437c-9cac-2aca24abc9b9
📒 Files selected for processing (12)
claude_code_log/factories/tool_factory.pyclaude_code_log/html/templates/components/message_styles.cssclaude_code_log/html/tool_formatters.pyclaude_code_log/markdown/renderer.pyclaude_code_log/models.pyclaude_code_log/plugins.pyclaude_code_log/utils.pydev-docs/plugins.mdtest/__snapshots__/test_snapshot_html.ambrtest/test_artifact_rendering.pytest/test_data/artifact_tool.jsonltest/test_plugin_xss_api.py
CodeRabbit review on #263: CommonMark terminates an un-bracketed link destination at ASCII space OR any ASCII control character, and is_safe_web_url only gates the scheme — so an accepted http(s) URL carrying \n/\r/\t (or any of U+0000-U+001F, U+007F) could still end the (target) slot early and let trailing text escape the link. Percent- encode the full control range after the named-character pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
(Claude) Fixes #262.
What changed
Tool use is now description-primary — the favicon and version label belong to the published page, so they move to the result line. The redeploy addendum stays as a meta row: a
redeploys existing artifactlink (URL inhref/hover only) plus the force flag spelled out asforce: overwrite without conflict check— a normal redeploy sends the artifact's base version so a concurrent write from another session fails instead of being silently clobbered;forceskips that check (hover tooltip explains this in the HTML).Tool result becomes
Published artifact (favicon) (label):hrefand the hover title instead of the visible text.{url, path, title}and carries no favicon/label, so both are enriched from the pairedtool_useinput.<img>only when the value is anis_safe_web_urlhttp(s) URL (escaped src,referrerpolicy="no-referrer"); the normal emoji value stays an inert text span; over-long non-URL values are dropped (ARTIFACT_FAVICON_TEXT_MAX, shared between renderers).Hardening found while testing
The Markdown link-target encoder only percent-encoded parentheses, so an accepted https URL carrying quotes could smuggle attributes through a downstream Markdown→HTML conversion. The new
safe_markdown_link_target()also encodes quotes, angle brackets and spaces, applies to every Markdown link/image destination, and is surfaced as a plugin helper (plugins.__all__lazy re-export + dev-docs/plugins.md §4.1/§4.2), following theis_safe_web_urlpattern.Verification
.artifact-faviconCSS rule only.🤖 Generated with Claude Code
Summary by CodeRabbit
safe_markdown_link_targetto the documented plugin helper surface and clarified safe Markdown link target guidance.