fix(gp2rs): write arrangement XML as UTF-8 so GP import survives non-ASCII metadata#984
fix(gp2rs): write arrangement XML as UTF-8 so GP import survives non-ASCII metadata#984ChrisBeWithYou wants to merge 2 commits into
Conversation
…ASCII metadata
The GP→arrangement-XML writers persisted their output with
`Path.write_text(xml_str)` and no explicit encoding. On Windows that uses
the cp1252 default, so a non-ASCII metadata character — e.g. the © in an
album name like "Chrysalis©1982" — was written as the lone byte 0xA9.
The XML is read back as UTF-8 (expat's default), where 0xA9 is an invalid
start byte, so `parse_arrangement` died with:
xml.etree.ElementTree.ParseError: not well-formed (invalid token): line 10, column 22
and the whole Guitar Pro import failed (HTTP 500). All three arrangement
XML writes (gp2rs.py, gp2rs_gpx.py ×2) now pin encoding="utf-8".
CI runs on Linux (UTF-8 default) so the bug was invisible there; the new
test pins the locale-independent contract at the source level plus a
round-trip of a © album name.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Signed-off-by: ChrisBeWithYou <christian.a.cowan@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughGuitar Pro arrangement XML writers now explicitly emit UTF-8 in all output paths. Regression tests verify the encoding contract and preserve non-ASCII album metadata during XML parsing. The changelog documents the Windows import fix. ChangesUTF-8 arrangement XML output
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
tests/test_gp2rs_xml_encoding.py (1)
24-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert every XML write uses UTF-8.
This check only rejects exact bare
write_text(xml_str)calls and verifies that one UTF-8 call exists. If another writer changes to an explicit non-UTF-8 encoding, the test still passes. Enumerate every arrangement XML write and assert each call suppliesencoding="utf-8".🤖 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 `@tests/test_gp2rs_xml_encoding.py` around lines 24 - 37, Strengthen test_arrangement_xml_writes_specify_utf8 so it enumerates every arrangement XML write in gp2rs and gp2rs_gpx and validates each call explicitly supplies encoding="utf-8". Replace the current bare-call and single-occurrence checks with per-write assertions that also reject any explicit non-UTF-8 encoding.
🤖 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 `@tests/test_gp2rs_xml_encoding.py`:
- Around line 40-53: Update test_utf8_write_round_trips_non_ascii_album to use
minimal GP and GPX fixture files and generate arrangement XML through
gp2rs.convert_file and gp2rs_gpx.convert_file instead of writing XML directly.
Parse each converter’s output and assert the albumName remains “Chrysalis©1982”,
covering both production write paths and album propagation.
---
Nitpick comments:
In `@tests/test_gp2rs_xml_encoding.py`:
- Around line 24-37: Strengthen test_arrangement_xml_writes_specify_utf8 so it
enumerates every arrangement XML write in gp2rs and gp2rs_gpx and validates each
call explicitly supplies encoding="utf-8". Replace the current bare-call and
single-occurrence checks with per-write assertions that also reject any explicit
non-UTF-8 encoding.
🪄 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 Plus
Run ID: 7e88b1bd-e05c-4d3d-9ffe-572e5091a1f4
📒 Files selected for processing (4)
CHANGELOG.mdlib/gp2rs.pylib/gp2rs_gpx.pytests/test_gp2rs_xml_encoding.py
| def test_utf8_write_round_trips_non_ascii_album(): | ||
| # The behavioural end of the contract: a © album name written as UTF-8 | ||
| # parses cleanly and reads back intact (the cp1252 write does not). | ||
| from pathlib import Path | ||
| import tempfile | ||
|
|
||
| xml_str = ( | ||
| '<?xml version="1.0"?>\n<song>\n' | ||
| " <albumName>Chrysalis©1982</albumName>\n</song>\n" | ||
| ) | ||
| path = Path(tempfile.mkdtemp()) / "arr.xml" | ||
| path.write_text(xml_str, encoding="utf-8") | ||
| root = ET.parse(path).getroot() | ||
| assert root.findtext("albumName") == "Chrysalis©1982" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exercise the production converters in the round-trip test.
This test writes its own XML string and never calls gp2rs.convert_file or gp2rs_gpx.convert_file, so it cannot detect regressions in album propagation or the actual production write paths. Use a minimal GP/GPX fixture, generate the arrangement XML through the converter, then parse that output and assert the © album name survives.
🧰 Tools
🪛 Ruff (0.15.21)
[error] 52-52: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
🤖 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 `@tests/test_gp2rs_xml_encoding.py` around lines 40 - 53, Update
test_utf8_write_round_trips_non_ascii_album to use minimal GP and GPX fixture
files and generate arrangement XML through gp2rs.convert_file and
gp2rs_gpx.convert_file instead of writing XML directly. Parse each converter’s
output and assert the albumName remains “Chrysalis©1982”, covering both
production write paths and album propagation.
|
Review pass complete. Found and fixed one regression-test gap in 1e8302d: the original regex could miss incorrectly encoded or differently formatted write_text(xml_str, ...) calls, and the round-trip test leaked its temporary directory. The test now inspects every arrangement XML write via the Python AST, pins all three writer paths to UTF-8, and uses pytest's managed temp directory. Verification: 234 converter/notation tests pass on Windows, git diff --check is clean, and all PR checks (including CodeRabbit) are green. A second review pass found no further issues. |
1 participant
The bug
Importing a Guitar Pro file whose metadata contains a non-ASCII character fails on Windows with:
and the whole import returns HTTP 500. Repro'd with a real file whose album name is
Chrysalis©1982— the generated<albumName>line is exactly where it dies.Root cause
The GP→arrangement-XML writers persist their output with
Path.write_text(xml_str)and no explicit encoding.Path.write_textuses the platform default text encoding — cp1252 on Windows — so©(U+00A9) is written as the lone byte0xA9.parse_arrangementthen reads the file back as UTF-8 (expat's default), where0xA9is an invalid start byte → parse error.CI runs on Linux, where the default is UTF-8, so the bug is invisible there — which is why it shipped.
Fix
Pin
encoding="utf-8"on all three arrangement-XML writes:lib/gp2rs.py(1) andlib/gp2rs_gpx.py(2).Test
tests/test_gp2rs_xml_encoding.py— because a plain functional test passes on Linux CI even with the old code (UTF-8 default), the regression is pinned at the source-contract level (no barewrite_text(xml_str); the UTF-8 form is present), plus a round-trip that a©album name written as UTF-8 parses back intact. Converter suites: 215 passed.Context
Found while dogfooding the Arrangement Editor's GP import on Windows. Independent of the editor plugin — this is core
lib/.🤖 Generated with Claude Code
https://claude.ai/code/session_01EBQCHCNA81E9tHmSDHSe2Q
Summary by CodeRabbit