fix: use UTF-8 encodings for module file read/write on Windows - #227
fix: use UTF-8 encodings for module file read/write on Windows#227AshSgDe29071999 wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe change standardizes module file encoding. Reads use BOM-aware UTF-8 decoding, and writes use explicit UTF-8 encoding across parsers, CLI commands, shared targets, and target integrations. Tests cover emoji content and BOM-prefixed frontmatter. ChangesUTF-8 encoding support
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71380d5711
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/lola/frontmatter.py`:
- Line 50: Update the normal frontmatter file-loading path in frontmatter.load
to read markdown with UTF-8-SIG instead of the default UTF-8, ensuring a leading
BOM is stripped before frontmatter detection and parsing. Preserve the existing
return behavior and decoding for other paths.
In `@src/lola/models.py`:
- Line 234: Update the sibling lola.yaml read in Module.from_path to open
lola_yaml with encoding="utf-8-sig", matching the existing MCP configuration
read and supporting BOM-prefixed or non-ASCII values.
In `@src/lola/targets/claude_code.py`:
- Line 63: Update the SKILL.md destination writes to use explicit UTF-8 encoding
while preserving utf-8-sig reads: modify the write in
src/lola/targets/claude_code.py at lines 63-63, src/lola/targets/cursor.py at
lines 72-72, and src/lola/targets/openclaw.py at lines 73-73.
In `@tests/test_utf8_encoding.py`:
- Around line 8-15: Update test_parse_file_with_emoji to assert that body
contains the exact emoji content written by the fixture, including ⚠️, rather
than checking only the ASCII word “warning”.
- Around line 18-26: Extend test_parse_file_with_utf8_bom to call parse_file on
the BOM-prefixed skill and assert the parsed metadata contains the expected name
and description, along with the expected body content. Keep the existing
validate_skill assertion intact.
🪄 Autofix
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 Plus
Run ID: 228ab844-76ca-4dd2-99a4-c405240eb22c
📒 Files selected for processing (13)
src/lola/cli/install.pysrc/lola/cli/mod.pysrc/lola/frontmatter.pysrc/lola/models.pysrc/lola/targets/base.pysrc/lola/targets/claude_code.pysrc/lola/targets/copilot.pysrc/lola/targets/cursor.pysrc/lola/targets/gemini.pysrc/lola/targets/install.pysrc/lola/targets/openclaw.pysrc/lola/targets/opencode.pytests/test_utf8_encoding.py
|
Addressed bot/review feedback:
|
SecKatie
left a comment
SecKatie
left a comment
There was a problem hiding this comment.
LGTM — verified all review findings are fixed, the full test suite (1074 tests) passes, and ruff is clean.
|
@AshSgDe29071999 Thanks for the fix — review feedback is now all addressed and tests pass. Could you rebase onto the latest |
Closes #224 Path.read_text()/write_text() without encoding use the locale codec on Windows (often cp1252), which raises UnicodeDecodeError on emoji and other non-ASCII module content. Read with utf-8-sig (strips BOM) and write with utf-8 across the CLI and target install paths.
- Write skill files with encoding=utf-8 in claude_code/cursor/openclaw - parse_file reads utf-8-sig then frontmatter.loads so BOM works - Open lola.yaml as UTF-8 - Tests assert emoji content and exercise parse_file under BOM
|
Rebased onto latest |
3693ff1 to
a04c791
Compare
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 (1)
src/lola/targets/opencode.py (1)
34-60: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse OpenCode’s native tool permission keys and deny unlisted tools.
_normalize_tool_name()convertswebfetchtofetch, but OpenCode configuration useswebfetch/websearch, while todo tools usetodowrite/todoreadkeys.KNOWN_OPENCODE_TOOLSalso includestodoreplace, which is not a native OpenCode permission key. Keep exact supported keys, add missing native keys to the deny list, and cover web/todo/custom tools in OpenCode target tests so source allowlists do not get converted into broken access controls.🤖 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 `@src/lola/targets/opencode.py` around lines 34 - 60, The OpenCode tool normalization and allowlist deny set use incorrect native permission keys. Update _normalize_tool_name and KNOWN_OPENCODE_TOOLS to preserve webfetch/websearch and use todowrite/todoread, remove todoreplace, and ensure unlisted tools remain denied; extend the OpenCode target tests to cover web, todo, and custom tool mappings.Source: MCP tools
🤖 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 `@src/lola/targets/opencode.py`:
- Around line 34-60: The OpenCode tool normalization and allowlist deny set use
incorrect native permission keys. Update _normalize_tool_name and
KNOWN_OPENCODE_TOOLS to preserve webfetch/websearch and use todowrite/todoread,
remove todoreplace, and ensure unlisted tools remain denied; extend the OpenCode
target tests to cover web, todo, and custom tool mappings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 12d0792d-0cda-4d4d-8d59-55ef511be6fe
📒 Files selected for processing (6)
src/lola/cli/mod.pysrc/lola/targets/base.pysrc/lola/targets/claude_code.pysrc/lola/targets/copilot.pysrc/lola/targets/cursor.pysrc/lola/targets/opencode.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lola/targets/cursor.py
- src/lola/targets/claude_code.py
- src/lola/cli/mod.py
2 participants
Summary
On Windows,
Path.read_text()/write_text()without an explicitencodinguse the locale codec (typicallycp1252). Module files with emoji or other non-ASCII UTF-8 content then raiseUnicodeDecodeErrorduringlola mod info/lola install. A UTF-8 BOM also breaks thestartswith("---")frontmatter check.Changes
encoding="utf-8-sig"(strips BOM when present).encoding="utf-8".Closes #224
Summary by CodeRabbit
Bug Fixes
Tests