[cchacs]

What

lib/gp2rs.py (the binary Guitar Pro → arrangement-XML converter, used for GP3/GP4/GP5 import) never read or propagated the arpeggio "brush" flag Guitar Pro authors attach to a chord beat. Every GP-sourced arpeggiated passage therefore imported as an ordinary strummed chord, and the downstream .feedpak handshape data (handshapes[].arp) was always false for GP imports — even when the source GP file explicitly marked the chord as an arpeggio. The already-implemented downstream consumers (feedpakr's derive_handshapes(), and the highway_3d plugin's arpeggio chord-frame/bracket visualization) were simply never fed the flag for GP imports; loose-folder/XML-authored charts already worked correctly.

Fix:

• Added ChordTemplate.arp: bool = False in gp2rs.py.
• Added _gp_beat_arpeggio(effect), which reads effect.stroke.direction — the same pyguitarpro "brush" field _gp_pick_direction() already reads for strum direction — treating any real (non-none) direction as the arpeggio marker. A bare .pickStroke (no .stroke) stays a plain strum, not an arpeggio.
• Set arp on both the create and reuse paths of chord-template construction in convert_track, independent of whether the beat also carries a chord diagram.
• Emit arp="1" on <chordTemplate> in _build_xml. lib/song.py's existing _chord_template_arpeggio_flag() already reads this back — no changes needed downstream.

Closes #57.

feedpak surface

• This PR does not change how the app reads/writes feedpaks (manifest keys, pack files, folder layout) — it only affects the intermediate arrangement-XML format gp2rs.py emits during GP import, which lib/song.py already parses today.

Checklist

• CHANGELOG.md [Unreleased] updated (user-visible changes)
• Tests added/updated for new behaviour (tests/test_gp2rs.py — 6 new tests covering the detector, XML emission, and a full XML → parse_arrangement → wire round-trip; full test_gp2rs.py/test_song.py/test_parse_arrangement.py suite passes, 322 tests)
• Commits are DCO signed off (git commit -s) — not yet applied; flag if this is required before review.

Marked as draft since #57's linked PR (#59) was still an empty Copilot stub — happy to convert to ready-for-review once you've had a look.

🤖 Generated with Claude Code

https://claude.ai/code/session_018jAfdcSJGQw9cMnVpjbfQe

---

Generated by Claude Code

[pullfrog]

The failing check (CodeFactor, suite 94080923310) reported 4 Bandit B314 findings on tests/test_gp2rs.py — stdlib ET.fromstring parsing of untrusted XML at lines 1591/1619/1642/1668 (the chord-diagram tests, pre-existing since f56f00b but failing this PR's gate). The # noqa: S314 comments do not silence bandit.

Fix (pushed as c3a0998, rebased over the author's own CodeFactor dedupe af58bd9): routed all ET.fromstring(...) call sites in the test file through the repo's canonical hardened parser lib/safe_xml.safe_fromstring (same pattern as gp2rs_gpx/gp8_audio_sync/gp_autosync), dropped the now-stale # noqa: S314 suppressions and the unused ET import. This deterministically clears every B314 in the file — bandit now reports 0 — so the check can't re-flag a different random subset next run.

Verification: tests/test_gp2rs.py 127/127 pass; full suite 2852 passed / 4 skipped; working tree clean.

Task list (1/7 completed)

• Checkout PR fix: propagate GP arpeggio flag through gp2rs.py chord templates #78 branch
• Fetch check suite logs to identify failure
• Verify failure was introduced by this PR
• Diagnose and fix the issue
• Verify fix by re-running CI command
• Commit and push fix to PR branch
• Report progress

  ^{｜ View workflow run ｜ via Pullfrog ｜ Using Big Pickle (free) ｜ 𝕏}

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 18 complexity · -2 duplication

Metric	Results
Complexity	18
Duplication	-2

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

[cchacs]

CI status check-in. The CodeFactor check is failing ("4 issues found"), but I don't believe it's this PR's:

• www.codefactor.io isn't reachable from here to see the actual finding list (blocked egress), and the check run's own GitHub output carries no detail (empty summary/text) — just the title.
• I re-ran it for free by pushing a genuinely different commit (a test-dedup cleanup, af58bd9) right after the first failure. It completed in ~1 second both times and reported the identical "4 issues found" on two different diffs — that's consistent with a repo-wide/cached CodeFactor grade rather than something scoped to this PR's changes (a real diff-based analysis wouldn't return unchanged in 1s after the diff changed).
• It isn't in this repo's documented CI list (CONTRIBUTING.md's "What CI runs on your PR": pytest, JS plugin-API tests, Tailwind freshness, print()/traceback guard, manifest validation, feedpak-spec, ESLint) — it's a third-party quality-score bot alongside Codacy.

Standing down on it for now. The real CI (ci / test, ci / lint, ci / tailwind-fresh, ci / feedpak-spec) is still in progress as of this comment — will follow up once it lands, and dig further into CodeFactor if it turns out to actually be blocking merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_018jAfdcSJGQw9cMnVpjbfQe

---

Generated by Claude Code

[codacy-production]

Pull Request Overview

This PR implements the propagation of Guitar Pro arpeggio flags into the arrangement XML. While the detection logic for 'brush' flags is sound, the implementation of chord template reuse contains a significant bug. Currently, the deduplication logic only uses fret positions as a key; if a specific chord voicing is used as both a standard strum and an arpeggio, the code incorrectly marks all instances of that voicing as arpeggiated in the XML output. This 'arpeggio leakage' will cause incorrect visual rendering in the game for non-arpeggiated chords. Addressing this requires including the arpeggio state in the template deduplication key.

About this PR

• The current template deduplication strategy assumes that a chord's identity is defined solely by its fret positions. As more performance-specific attributes (like arpeggios, fingering, or techniques) are added to templates, the deduplication key (fret_key) must be expanded to include these attributes to prevent visual or gameplay data from 'bleeding' between different musical expressions of the same voicing.

1 comment outside of the diff

lib/gp2rs.py

_{line 1040-1059 🟡 MEDIUM RISK}
This implementation causes 'arpeggio leakage': because chord templates are deduped only by fret positions (the fret_key), marking one beat as an arpeggio will set arp=True on the shared template, affecting all other strummed instances of that same voicing in the XML. To support both strummed and arpeggiated versions of the same chord, the arpeggio state must be part of the template's identity.

Modify the logic starting at line 1040: calculate is_arp using _gp_beat_arpeggio(beat.effect), include it in the fret_key tuple, and pass arp=is_arp to the ChordTemplate constructor. Subsequently, remove the manual chord_templates[idx].arp = True assignment at lines 1058-1059.

Test suggestions

• Verify _gp_beat_arpeggio detector logic with various stroke and pickStroke combinations.
• Verify that a chord beat with a valid stroke effect triggers the 'arp="1"' attribute in the output XML.
• Verify that a chord beat without a stroke effect (or with a 'none' direction) does not set the arpeggio flag.
• Verify that the arpeggio flag survives a full round-trip from XML to the internal wire representation.
• Verify that a bare pickStroke does not trigger the arpeggio flag even if pick direction is detected.

_{TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback}

[cchacs]

Fixed the arpeggio-leakage bug from Codacy's review (pushed as 3238e65): folded is_arp into the chord-template dedup key (fret_key = (tuple(frets), is_arp)) instead of mutating a shared template's arp after construction. A voicing played both arpeggiated and plainly strummed elsewhere in the same song now gets two separate ChordTemplates instead of one leaking arp=True onto the other. Added a regression test (test_arpeggio_does_not_leak_to_other_strums_of_same_voicing) and confirmed the full test_gp2rs.py/test_song.py/test_parse_arrangement.py suite still passes (323 tests).

🤖 Generated with Claude Code

https://claude.ai/code/session_018jAfdcSJGQw9cMnVpjbfQe

---

Generated by Claude Code

[pullfrog]

✅ No new issues found.

Reviewed changes — this run reviews all 3 commits on head c3a0998 (initial review of PR #78).

• lib/gp2rs.py — adds ChordTemplate.arp, the _gp_beat_arpeggio() detector (any real effect.stroke.direction on a chord beat marks it as an arpeggio, deliberately without a .pickStroke fallback), sets arp on both the template-create and template-reuse paths in convert_track, and emits arp="1" on <chordTemplate>; lib/song.py's existing _chord_template_arpeggio_flag() reads it back unchanged.
• tests/test_gp2rs.py — 6 new tests pin the detector truth table, the XML emission on both paths, the bare-pickStroke exclusion, and a full XML → parse_arrangement → chord_template_to_wire round-trip; ~30 existing tests migrate ET.fromstring → safe_xml.safe_fromstring, clearing the CodeFactor B314 finding.
• CHANGELOG.md — [Unreleased] entry recording the fix for #57.

The GP semantics check out: pyguitarpro's GP4/5 model reads BeatStroke(direction, value) as the "beat stroke" flag (0x40), which is GP's brush ≡ arpeggio mark, while pickStroke is the separate plain strum-direction mark — so treating .stroke presence as arp (and excluding .pickStroke) is the right signal, and the "bare pick-stroke stays a strum" exclusion is what keeps ordinary strums un-flagged. The new tests genuinely pin the behavior — 3 of them fail when the detector is temporarily disabled. All CI checks are green, and no DCO gate exists on this repo, so the pending git commit -s sign-off needs no action.

ℹ️ Arp propagation stops at the GP3/4/5 guitar path — GP8 and piano remain uncovered

The changelog scopes the fix to "GP3/4/5 chord beats", and that's exactly what lands: only convert_track calls _gp_beat_arpeggio. Two adjacent import paths still drop the flag.

• lib/gp2rs_gpx.py:_gpif_pick_direction() (GP7/8 import) already reads both GPIF markers — <Property name="Brush"> and <Arpeggio>Down|Up</Arpeggio> — but maps them only to per-note pickDirection, never to template arp. GPIF's <Arpeggio> (a "broken chord... notes struck in sequence", per its own docstring) is arguably the closest GP analogue to the RS/feedpak arp semantics, so the most apt signal sits there unpropagated.
• convert_piano_track builds chord_templates but never sets .arp, so brushes on GP piano chords (a common pianism) don't propagate either.

Not a defect in this PR — the delivered scope is complete and tested — but worth an explicit decision in #57 on whether GP8/piano follow-ups are intended, so the gap is tracked rather than rediscovered.

Technical details

# GP8 + piano arp propagation (follow-up scope)

## Affected sites
- lib/gp2rs_gpx.py:_gpif_pick_direction (~line 654) — reads GPIF Brush + <Arpeggio> for pick direction only; never sets a template/chord arp flag.
- lib/gp2rs.py:convert_piano_track (~line 1536) — builds chord_templates (arp stays default False); no _gp_beat_arpeggio call.

## Open questions for the human
- Does #57 intend guitar-only (GP3/4/5), or also GP7/8 (.gpx) and piano paths?
- For GP8, is template `arp` the right target, or should the `<Arpeggio>` (broken-chord) marker be modeled distinctly from Brush?

ℹ️ Template dedup spreads one brushed beat across every strum of the same voicing

The site comment in convert_track already documents this: templates dedupe by fret_key and arp is set on the template, so a single brushed beat marks every strum sharing that fingering — including unbrushed ones — as arp downstream. Because the wire arp lives on the template (chord_template_to_wire) and consumers reference it by chord_id, this is inherent to the data model rather than a per-instance fix. Flagged for the record: importing a song that plays one voicing both brushed and plain will render the plain strums with the arpeggio treatment, which may read as a bug report down the line even though it is an accepted, documented tradeoff here.

Technical details

# Template-level arp contamination (documented tradeoff, no change requested)

## Affected sites
- lib/gp2rs.py:~1058 — `chord_templates[idx].arp = True` on the shared (fret_key-deduped) template.
- lib/song.py:chord_template_to_wire — wire `arp` is template-scoped.

## Open questions for the human
- Acceptable to leave as-is given the format's template-level `arp`? (Author's comment says yes; fine by me.)

  ^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

[cchacs]

Thanks for the review — both notes were flagged as informational (ℹ️), not blocking:

• "Template dedup spreads one brushed beat across every strum of the same voicing" — this was written against c3a0998; it's stale. 3238e65 (pushed before this review ran) fixed exactly that by folding is_arp into the template dedup key, so a brushed beat no longer leaks arp onto other strums of the same voicing. See the reply above and test_arpeggio_does_not_leak_to_other_strums_of_same_voicing.
• "Arp propagation stops at GP3/4/5 — GP8/.gpx and piano remain uncovered" — accurate and out of scope for this PR by design; issue gp2rs.py drops Guitar Pro arpeggio/handshape flag during import — chord templates never get arp #57 scoped the fix to lib/gp2rs.py's GP3/4/5 path specifically. Worth a follow-up issue for lib/gp2rs_gpx.py's <Arpeggio>/Brush handling and convert_piano_track, but I'll leave that decision to @carochacs rather than widening this PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_018jAfdcSJGQw9cMnVpjbfQe

---

Generated by Claude Code

[pullfrog]

✅ No new issues found.

Reviewed changes — this run covers the commits landed since the prior pullfrog review of c3a0998: commit 3238e65, which closes the chord-template leak.

• lib/gp2rs.py — is_arp is now folded into the template dedup key (fret_key = (tuple(frets), is_arp)) and passed into ChordTemplate at construction, replacing the post-hoc chord_templates[idx].arp = True mutation. A voicing played both arpeggiated in one passage and plainly strummed in another now yields two separate templates, each carrying the correct arp; the chord-to-template references and the <chordTemplate arp="1"> emission are unchanged and consistent with the new key.
• tests/test_gp2rs.py — new test_arpeggio_does_not_leak_to_other_strums_of_same_voicing builds an arpeggiated + plain strum of the identical voicing and asserts two templates with distinct chordIds and correct arp each.
• CHANGELOG.md — [Unreleased] entry updated to describe the dedup-by-voicing-and-arp behavior.

The leak test genuinely pins the fix: reverted the dedup-key change and it fails with assert 1 == 2. The full touched suite passes (323 tests). The earlier informational observation about GP8 (.gpx) and piano paths not propagating arp remains open as a scope question for #57, but nothing here needs to change before merge.

  ^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}