feat(editor): import GoPlayAlong sync sidecars (parser + endpoint) - #76
Conversation
A GoPlayAlong (goplayalong.com) export is a <track> XML that points at a Guitar Pro score + an audio file and stores bar->audio sync points -- it carries no chart, so feeding it to the arrangement importer failed with "not a recognised EOF arrangement XML". It's a different format, not a formatting bug. New goplayalong.py parser (pure stdlib, defusedxml-hardened) + a /parse-goplayalong-sync endpoint turn that XML into the same sync_points/audio_offset shape autosync-gp/extract-gp-sync already return, so the existing convert-gp path applies GoPlayAlong's authored sync to the referenced .gp instead of re-deriving it via onset detection. score_url/audio_url tell the caller which files the project references. This slice is the parser + endpoint. Tests: tests/test_goplayalong.py (10 cases, verified against a real GoPlayAlong export -- count header, per-bar audio-time + tempo mapping, bar-1 offset extrapolation, tolerant parsing, malformed guards). Full editor suite: 190 passed. The New-dialog UI hook that routes a dropped GoPlayAlong .xml through this path is the remaining slice, best finished + e2e-verified with a real .gp + .mp3 in hand. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UR2Cr7GEu3yMY7SrfxH6c1
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds GoPlayAlong XML sync-sidecar parsing, exposes it through a new editor API, wires create-flow staging and autosync around it, and adds parser tests plus changelog documentation. ChangesGoPlayAlong sync import
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ScreenUI as screen.js
participant EditorAPI
participant GoplayalongModule
User->>ScreenUI: drop GoPlayAlong XML and GP file
ScreenUI->>ScreenUI: stage sync sidecar and metadata
ScreenUI->>EditorAPI: POST /api/plugins/editor/parse-goplayalong-sync
EditorAPI->>GoplayalongModule: parse_goplayalong(text)
GoplayalongModule-->>EditorAPI: sync_points + audio_offset
EditorAPI-->>ScreenUI: parsed sync data
ScreenUI->>ScreenUI: store lastSync and continue create flow
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🧹 Nitpick comments (4)
goplayalong.py (3)
64-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
declared_countis stored but never validated.The field comment says "for validation," but nothing here (or in the routes.py snippet, which computes
sync_point_countfromlen(proj.sync_points)) ever comparesdeclared_countagainst the actual parsed point count. As-is it's dead data. Consider surfacing a mismatch (e.g., as a warning field in the returned project) or dropping the field/comment if validation isn't planned.Also applies to: 166-178
🤖 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 `@goplayalong.py` at line 64, The declared_count field in the project model is currently never checked against the actual number of parsed sync points, so it is effectively dead data. Update the parsing/return flow around goplayalong.py and the sync_point_count logic in routes.py to compare declared_count with len(proj.sync_points), and either surface a mismatch in the returned project payload (for example as a warning flag/message) or remove the field/comment if validation is not intended. Use the declared_count, sync_point_count, and proj.sync_points symbols to locate the affected code.
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlind
except Exceptionflagged by static analysis.Both are intentional per the docstrings ("Returns False (never raises)"), but static analysis (Ruff BLE001) flags them. Given the design intent is explicit, this is acceptable as-is; narrowing to
(ImportError, Exception)/XML parse errors would only marginally help debuggability.Also applies to: 82-83
🤖 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 `@goplayalong.py` at line 42, The broad exception handlers in goplayalong.py are intentional but are still tripping Ruff BLE001, so make the intent explicit at the two `except Exception` sites (including the one around the XML parsing path) by adding the appropriate lint suppression and keeping the existing “never raises” behavior. Use the surrounding functions that return `False` on failure to locate both handlers and ensure no control flow changes are introduced.Source: Linters/SAST tools
132-147: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffBar-1 offset extrapolation only considers the first two points.
If the first two points are noisy/out-of-order outliers, the extrapolated
audio_offsetcould be off, whereas a least-squares fit across all points would be more robust. This is a reasonable MVP simplification per the docstring, so treating this as optional rather than blocking.🤖 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 `@goplayalong.py` around lines 132 - 147, The bar-1 offset extrapolation in _extrapolate_bar1_offset currently uses only the first two GpaSyncPoint entries, which can skew audio_offset when those points are noisy or out of order. Update the calculation to derive the per-bar slope from all available points in a more robust way, such as a least-squares fit, while preserving the existing fallback behavior for zero or one point. Keep the function’s contract and return type unchanged.tests/test_goplayalong.py (1)
1-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the new endpoint itself.
This suite thoroughly covers
goplayalong.py's parsing/detection logic, but there's no test exercisingPOST /api/plugins/editor/parse-goplayalong-sync(400 on non-GoPlayAlong XML, 400 onValueErrorfrom the parser, 200 response shape). Worth a smallTestClient-based test alongside the parser tests.🤖 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_goplayalong.py` around lines 1 - 126, Add a small TestClient-based test suite for the POST /api/plugins/editor/parse-goplayalong-sync endpoint, since the current tests only cover goplayalong.py internals. Exercise the endpoint with non-GoPlayAlong XML to assert a 400, with input that makes parse_goplayalong raise ValueError to assert a 400, and with valid SAMPLE XML to assert a 200 and the expected response shape. Place the new tests near the existing parse/detection tests and reference parse_goplayalong and is_goplayalong_xml to keep the coverage aligned.
🤖 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 `@goplayalong.py`:
- Around line 38-43: Make defusedxml mandatory in goplayalong.py by removing the
fallback to xml.etree.ElementTree in the import block. Update the top-level
import logic around the _ET alias so it fails fast if defusedxml is unavailable,
instead of silently downgrading to the stdlib parser and reintroducing unsafe
XML parsing for untrusted uploads.
In `@routes.py`:
- Around line 4994-5002: Update _load_goplayalong() so the optional parser
import cannot bubble up an ImportError into parse_goplayalong_sync. Mirror the
autosync-gp/refine-sync pattern by wrapping the bare import goplayalong fallback
in try/except ImportError and returning a clear failure path (or sentinel) that
the caller can turn into a 503, while keeping the load_sibling("goplayalong")
path intact.
- Around line 5017-5024: Make hardened XML parsing mandatory in the GoPlayAlong
upload flow by updating _load_goplayalong and the goplayalong.py parser path so
it no longer falls back to xml.etree.ElementTree for parse_goplayalong or
is_goplayalong_xml. Require defusedxml as a hard dependency (or remove the
fallback entirely) and ensure the upload handler that calls
gpa.parse_goplayalong only ever uses the hardened parser implementation.
---
Nitpick comments:
In `@goplayalong.py`:
- Line 64: The declared_count field in the project model is currently never
checked against the actual number of parsed sync points, so it is effectively
dead data. Update the parsing/return flow around goplayalong.py and the
sync_point_count logic in routes.py to compare declared_count with
len(proj.sync_points), and either surface a mismatch in the returned project
payload (for example as a warning flag/message) or remove the field/comment if
validation is not intended. Use the declared_count, sync_point_count, and
proj.sync_points symbols to locate the affected code.
- Line 42: The broad exception handlers in goplayalong.py are intentional but
are still tripping Ruff BLE001, so make the intent explicit at the two `except
Exception` sites (including the one around the XML parsing path) by adding the
appropriate lint suppression and keeping the existing “never raises” behavior.
Use the surrounding functions that return `False` on failure to locate both
handlers and ensure no control flow changes are introduced.
- Around line 132-147: The bar-1 offset extrapolation in
_extrapolate_bar1_offset currently uses only the first two GpaSyncPoint entries,
which can skew audio_offset when those points are noisy or out of order. Update
the calculation to derive the per-bar slope from all available points in a more
robust way, such as a least-squares fit, while preserving the existing fallback
behavior for zero or one point. Keep the function’s contract and return type
unchanged.
In `@tests/test_goplayalong.py`:
- Around line 1-126: Add a small TestClient-based test suite for the POST
/api/plugins/editor/parse-goplayalong-sync endpoint, since the current tests
only cover goplayalong.py internals. Exercise the endpoint with non-GoPlayAlong
XML to assert a 400, with input that makes parse_goplayalong raise ValueError to
assert a 400, and with valid SAMPLE XML to assert a 200 and the expected
response shape. Place the new tests near the existing parse/detection tests and
reference parse_goplayalong and is_goplayalong_xml to keep the coverage aligned.
🪄 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: 6acb3d9b-5b76-46f5-a18a-9243bfa50bbb
📒 Files selected for processing (4)
CHANGELOG.mdgoplayalong.pyroutes.pytests/test_goplayalong.py
| # Prefer defusedxml (hardened against entity-expansion / external-entity attacks | ||
| # on untrusted uploads); fall back to the stdlib parser when it isn't installed. | ||
| try: # pragma: no cover - import wiring | ||
| from defusedxml import ElementTree as _ET | ||
| except Exception: # pragma: no cover - import wiring | ||
| import xml.etree.ElementTree as _ET |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files of interest ==\n'
git ls-files | rg '(^|/)(goplayalong\.py|routes\.py|requirements(\.txt)?|pyproject\.toml|Pipfile|poetry\.lock|setup\.py|setup\.cfg)$'
printf '\n== goplayalong.py import + XML parsing locations ==\n'
ast-grep outline goplayalong.py --view expanded || true
printf '\n== routes.py upload flow ==\n'
ast-grep outline routes.py --view expanded || true
printf '\n== defusedxml dependency declaration ==\n'
rg -n --hidden --glob '!**/.git/**' 'defusedxml' . || true
printf '\n== Relevant snippets ==\n'
sed -n '1,120p' goplayalong.py
printf '\n--- routes.py ---\n'
sed -n '1,220p' routes.pyRepository: got-feedBack/feedBack-plugin-editor
Length of output: 19001
Make defusedxml mandatory here. goplayalong.py:38-43 still falls back to xml.etree.ElementTree, which reopens XXE/entity-expansion risk when parsing untrusted uploads. Fail fast instead of silently downgrading to the stdlib parser.
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 42-42: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@goplayalong.py` around lines 38 - 43, Make defusedxml mandatory in
goplayalong.py by removing the fallback to xml.etree.ElementTree in the import
block. Update the top-level import logic around the _ET alias so it fails fast
if defusedxml is unavailable, instead of silently downgrading to the stdlib
parser and reintroducing unsafe XML parsing for untrusted uploads.
| def _load_goplayalong(): | ||
| """Load the sibling goplayalong parser (namespaced via load_sibling when | ||
| the host provides it; bare import otherwise — the plugin dir is on | ||
| sys.path and the module name is unique).""" | ||
| _ls = context.get("load_sibling") | ||
| if _ls: | ||
| return _ls("goplayalong") | ||
| import goplayalong # noqa: PLC0415 - lazy, optional fallback | ||
| return goplayalong |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
No fallback for a failed module load.
_load_goplayalong() can raise ImportError (bare import goplayalong failing) with nothing catching it in parse_goplayalong_sync, producing an unhandled 500 instead of a clear error. Compare with autosync-gp/refine-sync above, which wrap their optional imports in try/except ImportError and return a clean 503.
Suggested fix
`@app.post`("/api/plugins/editor/parse-goplayalong-sync")
async def parse_goplayalong_sync(file: UploadFile = File(...)):
...
raw = await file.read()
- gpa = _load_goplayalong()
+ try:
+ gpa = _load_goplayalong()
+ except ImportError:
+ return JSONResponse(
+ {"error": "GoPlayAlong parser module is unavailable on this server."}, 503)🤖 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 `@routes.py` around lines 4994 - 5002, Update _load_goplayalong() so the
optional parser import cannot bubble up an ImportError into
parse_goplayalong_sync. Mirror the autosync-gp/refine-sync pattern by wrapping
the bare import goplayalong fallback in try/except ImportError and returning a
clear failure path (or sentinel) that the caller can turn into a 503, while
keeping the load_sibling("goplayalong") path intact.
| raw = await file.read() | ||
| gpa = _load_goplayalong() | ||
| if not gpa.is_goplayalong_xml(raw): | ||
| return JSONResponse( | ||
| {"error": "Not a GoPlayAlong file — a GoPlayAlong export is a " | ||
| "<track> XML with <sync> data alongside a .gp score."}, 400) | ||
| try: | ||
| proj = gpa.parse_goplayalong(raw) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is Python's xml.etree.ElementTree vulnerable to billion laughs / entity expansion attacks by default?
💡 Result:
Yes, Python's xml.etree.ElementTree module can be vulnerable to the "billion laughs" (exponential entity expansion) and other entity-based attacks depending on the underlying Expat library version used by your Python installation [1][2][3]. The module relies on the Expat XML parser. Whether it is vulnerable depends on the version of Expat installed on your system [4][3]. Recent versions of Expat (specifically 2.4.1 and newer) contain built-in protections against the "billion laughs" and "quadratic blowup" vulnerabilities [4][3]. However, if your environment is linked against an older version of Expat, it may remain vulnerable [4][1][3]. You can check your current version using pyexpat.EXPAT_VERSION [4][3]. Because Python's standard library XML modules, including ElementTree, are generally not considered secure against maliciously constructed data, official documentation explicitly recommends using the defusedxml package when parsing untrusted or unauthenticated XML data [3]. Key takeaways: - Vulnerability status is dependent on the underlying system Expat library, not just the Python version [4][3]. - Expat versions below 2.4.1 are known to be vulnerable to billion laughs and quadratic blowup attacks [4][3]. - Even with updated Expat versions, the standard library XML modules are advised against for untrusted data because they are not hardened against other XML-based attacks [3]. - Using defusedxml is the standard recommended practice for securing XML parsing in Python [3].
Citations:
- 1: https://github.com/python/cpython/blob/main/Doc/library/xml.rst
- 2: https://docs.python.org/3/library/xml.html
- 3: https://docs.python.org/3.11/library/xml.html
- 4: https://docs.python.org/release/3.7.14/library/xml.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the GoPlayAlong helpers and the route in question.
git ls-files | rg '(^|/)(routes\.py|.*goplayalong.*|.*xml.*)$'
echo '--- routes.py around the cited lines ---'
sed -n '4995,5045p' routes.py
echo '--- search for GoPlayAlong parser definitions/usages ---'
rg -n "is_goplayalong_xml|parse_goplayalong|_load_goplayalong|xml\.etree|ElementTree|fromstring|defusedxml" .Repository: got-feedBack/feedBack-plugin-editor
Length of output: 5557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- goplayalong.py outline ---'
ast-grep outline goplayalong.py --view expanded
echo '--- goplayalong.py relevant sections ---'
sed -n '1,220p' goplayalong.py
echo '--- dependency mentions for defusedxml ---'
rg -n "defusedxml" pyproject.toml requirements*.txt setup*.py README* . .Repository: got-feedBack/feedBack-plugin-editor
Length of output: 8905
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the project pins or documents defusedxml, and whether tests cover malformed XML / entity handling.
rg -n "defusedxml|billion laughs|entity|DOCTYPE|external entity|XXE" goplayalong.py tests pyproject.toml requirements*.txt README* .Repository: got-feedBack/feedBack-plugin-editor
Length of output: 2010
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- goplayalong.py lines 1-210 ---'
cat -n goplayalong.py | sed -n '1,210p'
echo '--- dependency/config files mentioning defusedxml ---'
git ls-files | rg '^(pyproject\.toml|requirements.*\.txt|setup\.py|setup\.cfg|Pipfile|poetry\.lock|uv\.lock|README.*)$' | xargs -r -I{} sh -c 'echo "--- {} ---"; rg -n "defusedxml" "{}" || true'Repository: got-feedBack/feedBack-plugin-editor
Length of output: 8911
Make hardened XML parsing mandatory for GoPlayAlong uploads. goplayalong.py still falls back to xml.etree.ElementTree when defusedxml isn't installed, so this unauthenticated upload path can still hit entity-expansion DoS unless the dependency is required or the fallback is removed.
🤖 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 `@routes.py` around lines 5017 - 5024, Make hardened XML parsing mandatory in
the GoPlayAlong upload flow by updating _load_goplayalong and the goplayalong.py
parser path so it no longer falls back to xml.etree.ElementTree for
parse_goplayalong or is_goplayalong_xml. Require defusedxml as a hard dependency
(or remove the fallback entirely) and ensure the upload handler that calls
gpa.parse_goplayalong only ever uses the hardened parser implementation.
|
Verified the parser + GP import end-to-end against the reporter's real files (
So the full path (parse GoPlayAlong sync → import the GP → |
Wire the New-dialog Content Import to accept a GoPlayAlong <track> .xml as an
authored sync source alongside a Guitar Pro tab + audio. The staged .xml is
content-sniffed (<track> + <sync>) so it stages as a "Sync . GoPlayAlong" source
instead of being mistaken for an EOF arrangement (or silently dropped when a GP
is also added), and it prefills title/artist from the file.
On Import, editorDoCreate() sources the per-bar sync from parse-goplayalong-sync
instead of onset autosync-gp, populating createState.lastSync identically so the
existing convert-gp warp path applies GoPlayAlong's authored sync
(sync_applied: "warp"). No onset refine -- the authored points are already
per-bar accurate.
All GoPlayAlong UI logic is gated on createState.goplayalongFile, so normal GP /
EOF / audio imports are byte-for-byte unchanged. screen.js syntax clean; all 24
editor JS tests pass.
Verified end-to-end against a real GoPlayAlong project ("Would?" -- Alice in
Chains): 73 sync points -> the referenced .gp (87 bars, 7 tracks) -> 73 warp
anchors -> a monotonic per-bar warp.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UR2Cr7GEu3yMY7SrfxH6c1
|
UI landed + verified end-to-end with the reporter's real files. The New-dialog UI hook is in (2nd commit): drop the Guitar Pro tab + audio + the GoPlayAlong End-to-end verification (
Note: full in-browser click-through wasn't run this session — it needs the up-to-date core + this editor branch served together (the local testbed's core mirror is behind |
The sync-point loop caught (ValueError, IndexError) but int(float('inf'))
raises OverflowError, and float('inf') in the audio-ms field produced a
non-JSON-serialisable inf time_secs — either escaped the endpoint's 400
handler as an opaque 500. Skip non-finite points (math.isfinite), guard int()
by parsing bar as a float first, and broaden both excepts to include
OverflowError. Regression test added.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Problem
A user exports from GoPlayAlong (a
.gp+.mp3+ a.xml) and adding the.xmlfails with "not a valid EOF xml." It's not a formatting bug — it's a different format. A GoPlayAlong export is a<track>sync sidecar: it points at a Guitar Pro score + audio and stores bar→audio sync points, but carries no chart. The arrangement importer runs it through the<song>-root loader, finds no arrangements, and rejects it.What this adds
The editor already has the whole GP + audio + sync pipeline (
extract-gp-sync→refine-sync→convert-gp, usingGpSyncData/SyncPoint). GoPlayAlong is the same "GP + audio + sync" shape in a different container, so this teaches the editor to produce those sync points from the GoPlayAlong XML:goplayalong.py(pure stdlib, defusedxml-hardened) —is_goplayalong_xml()+parse_goplayalong(). Parses the<sync>payload (count#audioMs;bar;beat;msPerBeat#…) into bar→audio-time points, convertsmsPerBeat→bpm, and extrapolates the bar-1audio_offset.POST /api/plugins/editor/parse-goplayalong-sync— returns the exactsync_points/audio_offsetshapeautosync-gp/extract-gp-syncreturn (plusscore_url/audio_url), so the existingconvert-gppath applies the authored sync to the referenced.gp.Scope / status
<track>.xmland route it (with the user's.gp+ audio) throughparse-goplayalong-sync→convert-gpinstead of the EOF importer. The hook point is the.xmlfilter inscreen.js(currently →import-xml-project). Scoped separately because it's best finished and end-to-end-verified with a real.gp+.mp3(the reporter's files), which weren't available here.Tests
tests/test_goplayalong.py— 10 cases, verified against the real reported export ("Would?" — Alice in Chains): detection vs<song>/ MusicXML, metadata + referenced files, the 73-point count header, first/last bar→audio-time + tempo mapping, time ordering, bar-1 offset extrapolation, tolerant parsing (stray#/ blank fields / missing count header), and malformed-input guards. Full editor suite: 190 passed.🤖 Generated with Claude Code
https://claude.ai/code/session_01UR2Cr7GEu3yMY7SrfxH6c1
Summary by CodeRabbit
sync_points/audio_offset.