Skip to content

Rework workshop-banner skill to auto-read event frontmatter#17948

Open
dirien wants to merge 3 commits intomasterfrom
dirien/rework-workshop-banner
Open

Rework workshop-banner skill to auto-read event frontmatter#17948
dirien wants to merge 3 commits intomasterfrom
dirien/rework-workshop-banner

Conversation

@dirien
Copy link
Copy Markdown
Contributor

@dirien dirien commented Mar 12, 2026

Summary

  • Rewrites the workshop-banner skill to follow the blog-meta-image pattern: auto-detect events from arguments or git status, parse frontmatter for title/event_type/presenters, ask progressive questions only for ambiguous choices, save banners into the event directory, and update meta_image in frontmatter.
  • Hard-codes Pulumi logo (static/logos/brand/logo-on-black.png) instead of asking.
  • Removes unused Pillow fallback renderer (render_banner.py), template generator (create_clean_template.py), and template images (template.png, template_clean.png).
  • Formats render_banner_html.py with ruff.

Test plan

  • Tested with single-presenter event (designing-reusable-infrastructure-as-code) — auto-parsed frontmatter, rendered banner, updated meta_image
  • Tested with multi-presenter event (panel-ai-in-cloud-development) — correctly prompted for speaker selection
  • Verified Pulumi logo renders correctly from static/logos/brand/logo-on-black.png
  • Verified ruff lint and format pass on all Python files

Rewrites the workshop-banner skill to follow the blog-meta-image
pattern: auto-detect events from arguments or git status, parse
frontmatter for title/event_type/presenters, ask progressive
questions only for ambiguous choices, save banners into the event
directory, and update meta_image in frontmatter.

- Rewrite SKILL.md with 4-step frontmatter-driven workflow
- Hard-code Pulumi logo (static/logos/brand/logo-on-black.png)
- Remove unused Pillow fallback renderer and template files
- Format render_banner_html.py with ruff
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 12, 2026

Docs Review - PR #17948

Scope: 2 new files - .claude/commands/workshop-banner/SKILL.md and .claude/commands/workshop-banner/scripts/render_banner_html.py. These are internal skill/tooling files (not user-facing documentation), so most content-style criteria do not apply. Review focuses on correctness, code quality, and consistency.

Issue 1. PR description claims file deletions that are not in the diff. The PR body states it removes unused Pillow fallback renderer, template generator, and template images, and formats render_banner_html.py with ruff. However, the diff shows 689 additions and 0 deletions - only the two new files are present. If these deletions were intended, they appear to be missing from this PR. If they do not exist on the base branch, the PR description should be updated to avoid confusion for reviewers.
Issue 2. HTML injection risk in render_banner_html.py (line ~307). User-provided values (title, cta_text, speaker_name, speaker_title, speaker_company, badge_text) are injected directly into HTML via Python .format() without escaping. If a title contains characters like angle brackets, ampersands, or quotes, the HTML will break or render incorrectly. Since this is a local rendering tool (not a web server), the security risk is low, but it will cause rendering bugs for titles containing special characters. Suggestion: add html.escape() calls in build_html() for all user-provided string values before inserting them into the template.
Issue 3. Question numbering gap in SKILL.md. The skill document numbers interactive questions as 1, 2, then jumps to 4 (the Company logo section between them is labeled as hard-coded with no number). While intentional, this is slightly confusing - consider renumbering to keep continuity (1, 2, 3 for actual questions).

Minor observations (non-blocking): (a) Font loading timeout (render_banner_html.py:458) - page.wait_for_timeout(800) is a fixed delay for Google Fonts. Consider page.wait_for_load_state('networkidle') as a more robust alternative. (b) Default output path (render_banner_html.py:480) - When --output is omitted, the script defaults to ~/Desktop/ which may not exist on all systems. (c) to_kebab and auto_filename functions (render_banner_html.py:464-477) are effectively dead code in normal usage since SKILL.md always specifies --output.

Summary: The skill is well-structured and the Python renderer is clean. Main actionable items: (1) Fix or update the PR description regarding claimed file deletions, (2) Add HTML escaping for config values, (3) Optionally clarify the question numbering in SKILL.md. Mention me (@claude) if you would like additional reviews or help implementing fixes.

@dirien
Copy link
Copy Markdown
Contributor Author

dirien commented Mar 12, 2026

Could be related to the work @jeffmerrick did here:
#17937

@pulumi-bot
Copy link
Copy Markdown
Collaborator

@jeffmerrick
Copy link
Copy Markdown
Contributor

@dirien I've made a few changes!

  • I kept the same approach rather than the blog meta approach as you have more dynamic elements than those do.
  • Updated the design, including adding a square layout (mockup attached for you to check out).
  • To simplify the design and fit it more easily in a square I removed one of the arrays of logos, let me know if that's going to be an issue.
  • You can however, still leave the logo text out if you just want a few logos listed, or omit the logos completely.
  • I've added meta tags for the square image. Websites may or may not pick it up but it's there for you if you need it.

If you ever run into issues with the generator, I have this design in Figma and can always whip up custom images for special cases.

Screenshot 2026-03-17 at 3 09 50 PM

@claude re-review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Mar 17, 2026

Claude finished @jeffmerrick's task in 1m 56s —— View job


Re-review: PR #17948

  • Read changed files
  • Review SKILL.md
  • Review render_banner_html.py
  • Review head.html changes
  • Post findings

Good progress since the last review! Here's the updated assessment.

Issues from previous review ✅ All resolved

  1. HTML injectionhtml.escape() is now applied to all user-provided values (badge_text, title, cta_text, speaker_name, speaker_title, speaker_company, partner_text). Fixed.
  2. Font loadingpage.wait_for_load_state("networkidle") is now used instead of a fixed wait_for_timeout. Fixed.
  3. Question numbering — Questions are now numbered 1, 2, 3 with the hard-coded section clearly labeled between 2 and 3. Fixed.

New observations

1. Redundant chr(0) pattern in build_html_square (render_banner_html.py:603) — minor bug/smell

f'<div class="sq-partner-logos-row">{chr(0).join(logo_imgs).replace(chr(0), "")}</div>'

chr(0).join(x).replace(chr(0), "") is exactly "".join(x). This is likely a leftover from working around f-string curly-brace escaping. Contrast with the landscape version on line 365 which already uses "" .join(logo_imgs) (also works but has a stray space before the dot). Both should just be "".join(logo_imgs). Fix this →

2. SKILL.md notes mention wrong fonts — minor doc inconsistency

SKILL.md:187 says:

The HTML renderer uses Google Fonts (Outfit, DM Sans) and CSS gradients/shadows…

But both HTML_TEMPLATE and HTML_TEMPLATE_SQUARE now import Inter only. Should be updated to "Google Fonts (Inter)". Fix this →

3. title_len computed on HTML-escaped string — very minor

render_banner_html.py:293–300:

...
title_len = len(title)

If a title contains & (e.g., "IaC & DevOps"), it becomes "IaC &amp; DevOps" after escaping, making title_len longer than the visible character count. The font-size breakpoints would then undersize the text slightly. For typical event titles this is unlikely to matter, but computing title_len = len(config.get("title", "")) before escaping would be more accurate.

4. partner_text.capitalize() lowercases proper nouns — pre-existing, minor

Line 348 (and 584 in the square variant): .capitalize() lowercases all characters after the first. A partner text like "Powered by AWS" becomes "Powered by aws". Using .capitalize() here may not be intentional.


head.html — looks correct

The new meta_image_square block (lines 85–89) correctly places og:image:width/height immediately after their respective og:image tag, which matches the OGP spec. No issues here.

Square layout — looks good

The new HTML_TEMPLATE_SQUARE / build_html_square / single Playwright session producing both outputs is clean and well-structured. The sq-* CSS class namespace avoids conflicts with the landscape layout. Good design choice.


Summary: The main blocking issue (HTML injection) from the last review is fixed. Remaining items are all minor — the chr(0) pattern is the most worth cleaning up as it's confusing to read. The PR is otherwise in good shape.

@pulumi-bot
Copy link
Copy Markdown
Collaborator

@pulumi-bot
Copy link
Copy Markdown
Collaborator

@jeffmerrick
Copy link
Copy Markdown
Contributor

I'll need to revisit this before it's ready to go, there are some branding changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants