Skip to content

Make tailwind CLI subprocess handling robust - #28

Merged
vormwald merged 2 commits into
mainfrom
subprocess-robustness
Aug 22, 2026
Merged

Make tailwind CLI subprocess handling robust#28
vormwald merged 2 commits into
mainfrom
subprocess-robustness

Conversation

@vormwald

Copy link
Copy Markdown
Owner

Summary

  • Switch Open3.popen3 with a shell-joined command string to Open3.capture3 with an argv array — fixes paths containing spaces and removes the pipe-deadlock risk.
  • CLI exit status is now checked: on failure, stderr is logged loudly and the converter returns nil instead of silently writing an empty/placeholder stylesheet.
  • stderr on success is only warned when non-empty (previously an empty string was warned on every build).
  • Shared compile logic is extracted into one module so the two converters can't drift.
  • Namespace moved from Tailwindcss::Commands to Jekyll::Tailwindcss::Commands to avoid a constant clash with the tailwindcss-rails gem.

Test plan

  • 32 specs pass
  • standardrb clean

🤖 Generated with Claude Code

Switch Open3.popen3 with a shell-joined command string to
Open3.capture3 with an argv array, fixing paths containing spaces
and removing the pipe-deadlock risk. Check the CLI exit status:
on failure, stderr is logged loudly and the converter returns nil
instead of silently writing an empty/placeholder stylesheet.
stderr on success is only warned when non-empty (previously an
empty string was warned on every build). Shared compile logic is
extracted into one module so the two converters can't drift, and
the namespace moves from Tailwindcss::Commands to
Jekyll::Tailwindcss::Commands to avoid a constant clash with the
tailwindcss-rails gem.

Co-Authored-By: Claude <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves Tailwind CLI execution reliability and centralizes compilation behavior.

Changes:

  • Uses Open3.capture3 with argv-safe invocation and exit-status handling.
  • Moves commands under Jekyll::Tailwindcss to prevent namespace conflicts.
  • Updates converter integration and failure-path tests.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
spec/tailwindcss/commands_spec.rb Removes old namespace tests.
spec/jekyll/tailwindcss/commands_spec.rb Tests command execution and error handling.
spec/jekyll/converters/tailwindcss_spec.rb Tests shared compiler integration.
spec/jekyll/converters/css_spec.rb Updates converter behavior tests.
lib/tailwindcss/commands.rb Removes old command module.
lib/jekyll/tailwindcss/commands.rb Adds shared robust compiler implementation.
lib/jekyll/converters/tailwindcss.rb Delegates compilation to the shared module.
lib/jekyll/converters/css.rb Delegates CSS compilation to the shared module.
lib/jekyll-tailwindcss.rb Loads the relocated command module.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread lib/jekyll/converters/css.rb
Open3.capture3 raises when the tailwindcss binary is missing or not
executable. That exception was caught by the converter's rescue, which
returned the original source, so Jekyll would publish a .css file with
an unresolved `@import "tailwindcss"`. Return nil instead, matching the
.tailwindcss converter's failure path and log format.

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015kxK16Xgi7P4GeUfkmQNxr

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.


unless status.success?
Jekyll.logger.error "Jekyll Tailwind:", "tailwindcss CLI exited with status #{status.exitstatus}:\n#{stderr}"
return nil

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this against Jekyll 4.4.1's source and a real build, and the TypeError doesn't happen.

  • Jekyll::Renderer#run (renderer.rb:84) does convert(output.to_s) and returns the converter's value as-is — there's no type check on the way out.
  • Document#write (document.rb:281) does File.write(path, output, mode: "wb"), and IO.write calls to_s on a non-String argument, so nil becomes "".

Verified empirically with a scratch Jekyll site whose .css converter returns nil: the build completes and writes a 0-byte style.css, no exception. I also tested the nil-plus-layout path through place_in_layouts, since that's the one branch that could plausibly choke on nil — also fine.

So "Jekyll's converter contract expects a string" isn't enforced anywhere in this path, and the failure mode is an empty file rather than a crash.

The underlying concern about visibility is fair, but the failure isn't silent: Commands.compile logs Jekyll.logger.error with the CLI's exit status and full stderr before returning nil. An empty stylesheet plus a loud error is also a strict improvement over the previous behavior, which wrote the unresolved @import "tailwindcss" out as if it were real CSS.

Making a CLI failure fatal to the whole build is a reasonable alternative, but it's a behavior change beyond this PR's scope, and it would mean a transient Tailwind hiccup kills jekyll serve in development. Keeping it as-is.

@vormwald
vormwald merged commit 1df99b5 into main Aug 22, 2026
5 checks passed
@vormwald
vormwald deleted the subprocess-robustness branch August 22, 2026 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants