[cchacs]

What

Fixes #94. A mastery change in keys_highway_3d rebuilt the playable notes and called _resetScoring(), wiping hits, misses, streak and best streak. With Difficulty Ladder adjusting mastery during play, the score was wiped at every adjustment and the stats posted at song end only covered the time since the last change.

The mastery path now keeps the run totals. The judged-note sets are keyed by noteKey(t, midi), not list index, so they stay valid across the rebuild (and stop an already-judged note being counted twice if it comes back into view). Only the sweep cursor indexes the old list, so _anchorMissSweep(now) still re-anchors it and already-passed notes aren't retroactively counted as misses.

Also adds a regression test for #103 (silent first voice in a GP vocal bar). The break described there isn't present on main and the repro already returns the lyric, so no code change was needed; the test pins that behavior. Does not close #103.

• Version bump: keys_highway_3d 0.2.5 → 0.2.6
• Not changed: the hand-filter change path still resets scoring (outside keys_highway_3d: every mastery change resets the run's score #94).

feedpak surface

• This PR does not change how the app reads/writes feedpaks (manifest keys, pack files, folder layout)
• …or it does, and the spec change landed first via the FEP process

Checklist

• CHANGELOG.md [Unreleased] updated (user-visible changes)
• Tests added/updated for new behaviour
• Commits are DCO signed off (git commit -s)

Tests: node --test in plugins/keys_highway_3d (93 pass); pytest tests/test_gp2rs_gpx.py (105 pass). The #94 test is a source-level invariant, not a live-renderer test (needs WebGL).

🤖 Generated with Claude Code

https://claude.ai/code/session_01UarYwGSPw2H3i54yUixA5G

---

Generated by Claude Code

[pullfrog]

Fixed CodeFactor B314 warnings in the new test: replaced xml.etree.ElementTree.fromstring calls with safe_xml.safe_fromstring in tests/test_gp2rs_gpx.py (test for vocal lyrics when first voice is silent). Resolved merge conflict with the PR branch's version of the test. Tests pass locally: keys_highway_3d JS tests (6/6), pytest tests/test_gp2rs_gpx.py (105 passed), and full node --test suite (1403 passed). Changes pushed to existing PR #129.

Task list (2/8 completed)

• Create task list and start Fix mode execution
• Checkout PR fix(keys_highway_3d): keep run score across mastery changes #129 branch
• Get check suite logs to understand failure
• Investigate failure - determine if introduced by PR
• Diagnose root cause and fix
• Verify fix by reproducing CI command locally
• Commit and push fix to PR
• Report final 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 0 complexity · 0 duplication

Metric	Results
Complexity	0
Duplication	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[pullfrog]

ℹ️ The fix is sound — I traced _resetScoring() statement by statement and every side effect it had is either deliberately kept (run totals, judged sets) or re-established by _anchorMissSweep(now), including the _cancelAllKeyFlashes() that the function no longer reaches (that one self-heals inside _animateFeedback within WRONG_FLASH_MS, so nothing sticks red). Two follow-ups and three comment nits below; none block the merge.

Reviewed changes — the #94 keys_highway_3d mastery scoring fix (0.2.5 → 0.2.6), plus a regression pin for the unrelated #103 that rides along in the same branch.

• Mastery change no longer resets the run's score — plugins/keys_highway_3d/screen.js:3977 removes the _resetScoring() call from _maybeApplyMasteryFilter() and keeps only _anchorMissSweep(now), so hits/misses/streak/best streak and the noteKey(t, midi)-keyed judged sets survive the rebuild of _notation.playable.
• Rationale comment rewritten — records that a mastery change is the same run and that only the list-index-based sweep cursor needs re-anchoring.
• Source-level regression guard — plugins/keys_highway_3d/tests/instance_invariants.test.js:67 scans the bounded function body for _resetScoring()'s absence and _anchorMissSweep(now)'s presence. I confirmed it genuinely fails against origin/main's screen.js, so it is coverage rather than theatre.
• GP vocal-import pin — tests/test_gp2rs_gpx.py:1355 builds the #103 multi-voice repro (silent first voice, lyric-bearing second voice) and asserts count="1" / lyric="hello". It passes on current main unchanged, so it pins existing behaviour rather than fixing anything.
• Version + changelog — plugin.json 0.2.5 → 0.2.6 and an [Unreleased] entry under Fixed; the second commit restores CRLF (verified 848/848 lines end in CR, net one-line diff).

What I ran locally: node --test tests/js/*.test.js 'tests/plugins/*/js/*.test.js' 'plugins/*/tests/*.test.js' → 1403 pass, pytest tests/test_gp2rs_gpx.py → 105 pass, and the new invariant test against the base screen.js → fails as it should. Every ci/* check is green on 68972bc; the lone CodeFactor failure predates this branch.

ℹ️ The hand-filter path still wipes the run's score

_setHandFilter() still calls _resetScoring() followed by _anchorMissSweep(curTime), so a mid-song hand change zeroes the run exactly the way a mastery change used to — and the reasoning that justified the reset there is now known to be unnecessary, since the judged sets are noteKey-keyed and buildNoteMeshes()'s _clearNoteCaches() only disposes geometry/material caches. It is not a theoretical entry point: handFilter is this plugin's declared per-instance visualization capability setting, which splitscreen renders in its per-panel control popover while a song is playing. You scoped it out of #94 and said so in the description, so this is not a merge blocker — but it is now an internal inconsistency rather than a deliberate boundary, since the two sibling rebuild paths answer the same question differently.

Technical details

# Mid-song hand-filter changes still call `_resetScoring()`

## Affected sites
- `plugins/keys_highway_3d/screen.js:3306` — `_setHandFilter()` calls `_resetScoring()`, then `_anchorMissSweep(curTime)` at `:3309`
- `plugins/keys_highway_3d/screen.js:3307` — comment reads "same as the mastery path: resume the sweep where playback is now", which stopped being true with this PR
- `plugins/keys_highway_3d/screen.js:3293-3297` — the function's own doc comment still justifies its early-return in terms of "can't reset scoring mid-song"

## Required outcome
Either drop the `_resetScoring()` here too, or open a follow-up issue so the gap is tracked. Deleting the line leaves the invariant this PR's new comment block establishes — a mid-run rebuild is the same run, only the list-index-based sweep cursor needs re-anchoring — holding across both paths.

## Suggested approach
Drop `_resetScoring()` at `:3306` and keep `_anchorMissSweep(curTime)`. The `const curTime = _latestTime` read at `:3304` is already positioned correctly: it exists precisely to survive the zeroing, so with the zeroing gone it can just be inlined into the `_anchorMissSweep` call. The existing behavioural coverage (`scoring.test.js`, "mid-run filter change: anchored sweep skips the elapsed tail, still sweeps after") already pins the anchoring half.

## Open questions for the human
Was scoping this out deliberate (it needs its own issue because it is reachable through the splitscreen panel popover), or incidental?

ℹ️ #103 is closed with two of its three requested regressions outstanding

The PR body says it does not close #103, but #103 was referenced by 075c688 and closed two minutes later. Its remediation asked for three regressions plus a voice/tie policy fix on the pitch sidecar; the branch delivers one regression. The gap it names still exists in code: a lyric-bearing beat with no pitched note produces a note=0 <vocal> in the arrangement XML but no entry at all in the pitch sidecar, so a caller following the module's documented "call both" workflow gets two timelines with different onsets. Closing the issue implies that paragraph is resolved.

Technical details

# #103 closure is broader than the work delivered

## Affected sites
- `lib/gp2rs_gpx.py:2351` — `convert_vocal_track()` leaves `midi_note = 0` for a lyric-bearing beat with no pitched note and still emits a `<vocal>` (`:2385-2394`)
- `lib/gp2rs_gpx.py:1321-1347` — `convert_vocal_track_to_pitch_sidecar()` iterates `notes_text` and appends nothing when it is empty, so that same onset is absent from the sidecar entirely
- `tests/test_gp2rs_gpx.py:1355` — covers only regression 1 (silent first voice); #103 also asked for two simultaneous lyrical voices, and for a pin on matching onsets

## Required outcome
Reopen #103 or file a follow-up covering the sidecar/XML onset mismatch. The reported `count="0"` symptom genuinely does not reproduce — the new test establishes that — so the "not reproducible" closure is defensible for the headline bug, but the consistency gap documented in the same issue was never addressed.

## Open questions for the human
Is the sidecar/XML onset divergence intentional (the sidecar is a pitch ribbon and legitimately omits unpitched syllables), or an oversight worth a separate issue?

ℹ️ Nitpicks

• plugins/keys_highway_3d/screen.js:3535-3537 — _anchorMissSweep()'s doc comment still describes the miss floor as "a 'run restart', matching what the reset already does to hits/streak". The mastery path no longer resets, so that parenthetical now describes only the hand-filter path; naming the path explicitly would keep it true.
• plugins/keys_highway_3d/screen.js:3983-3984 — "keeping them stops a note already judged from being counted again if a later mastery change brings it back into view" describes a double-count that playback monotonicity already prevents. The reason retaining the sets actually matters is the opposite direction: an already-hit note sitting just ahead of the re-anchored cursor must not be swept as a miss. That is the invariant a future refactor has to preserve, so it is the one worth writing down.
• plugins/keys_highway_3d/screen.js:3307 — "same as the mastery path" is now false; see the section above.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}

[pullfrog]

✅ No new issues found. cc6ab0f swaps the new vocal fixture's ET.fromstring for safe_fromstring to clear CodeFactor B314, and it is a faithful, minimal change — hoisting convert_vocal_track to module scope also retires the function-local import without duplicating the existing convert_file binding, ET is still used 70 times elsewhere in the file so its top-level import stays live, and defusedxml is already a requirements.txt runtime dependency so this adds no test-environment requirement. Worth noting for the earlier review: CodeFactor was in fact failing because of this PR's own new test rather than being pre-existing, and it is green on this head.

Reviewed changes since the last pullfrog review — one commit, cc6ab0f, swapping the #103 fixture parser only.

• Fixture parsing moved to the hardened helper — tests/test_gp2rs_gpx.py:16 widens the module-scope import to convert_file, convert_vocal_track and :17 adds from safe_xml import safe_fromstring.
• New vocal test uses safe_fromstring throughout — tests/test_gp2rs_gpx.py:1359-1376 replaces all eight ET.fromstring calls in test_convert_vocal_track_keeps_lyrics_when_first_voice_is_silent and drops the in-function import.

Verified on this head: pytest tests/test_gp2rs_gpx.py → 105 pass, and every check is now green (ci / test, ci / lint, ci / manifest-validation, ci / tailwind-fresh, ci / feedpak-spec, core-commit-labeling, Codacy, CodeQL, CodeFactor). The commit touches only tests/, so the fork's core-commit-labeling exemption list covers the bare test: subject.

  ^{｜ View workflow run ｜ Using big-pickle (free) ｜ 𝕏}

[kilo-code-bot]

WARNING: Retaining _hitNoteKeys across a mastery change turns a replayed already-hit note into a wrong-note miss

judgeHit (screen.js:669) skips any note whose noteKey is already in hitKeys and returns null. With the judged sets no longer cleared by _resetScoring() here, a note that was hit, then dropped out of the playable set by a mastery change, then brought back into view and played again falls through to the else branch in _checkHit (screen.js:3483-3496): matchesHiddenHandNote returns false (hand filter is not excluding it), so it books a _misses++ and a _streak = 0. A correctly replayed note is penalised and breaks the run's streak — the exact class of bug #94 is about.

_onSeek('back') → forgetJudgmentsFrom does clear the stale keys, so loop-wrap and Section Practice rewinds are covered, and a note already in the past cannot be replayed without a seek — so this needs a hit within HIT_TOLERANCE_S of the mastery change. That is narrow, which is why this is a WARNING rather than CRITICAL, but it is a real hole in the retention the new comment relies on.

Cheap hardening: when a mastery change rebuilds the playable set, drop keys for notes that are no longer present (for (const k of _hitNoteKeys) if (!newKeys.has(k)) _hitNoteKeys.delete(k)), or have judgeHit distinguish "already hit" from "no match" so a replay is idempotent rather than a miss.

---

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

[cchacs]

Checked this against judgeHit and _checkHit; I don't think it's a regression, so I'm leaving the code as is.

• judgeHit only iterates _notation.playable, so a retained key for a note that is no longer in the playable set is never consulted. A stale key can only matter for a note that is present, within HIT_TOLERANCE_S of the press, and already hit.
• That case is a second press of an already-hit note inside the tolerance window. main already books it as a miss, with no mastery change involved, because judgeHit skips keys in hitKeys. The doc comment on judgeHit lists "a duplicate" among the null returns.
• Before this PR, a mastery change in that window cleared hitKeys, so the duplicate press would score a second hit on one chart note. Retaining the keys makes the behavior consistent with the no-mastery-change case, and the totals stay correct.
• The "dropped then brought back" detail doesn't change this: the note was still hit once, and a second press within the window is still a duplicate.

Pruning keys for absent notes would be harmless but wouldn't change any reachable outcome, so I'm not adding it. If you'd rather make duplicate presses neutral instead of misses, that would be a separate change to _checkHit, since it affects non-mastery play too.

---

Generated by Claude Code

[kilo-code-bot]

Code Review Summary

Status: 1 Issues Found | Recommendation: Address before merge

Overview

Severity	Count
CRITICAL	0
WARNING	1
SUGGESTION	0

Issue Details (click to expand)

WARNING

File	Line	Issue
plugins/keys_highway_3d/screen.js	3983	Retaining _hitNoteKeys across a mastery change makes a replayed already-hit note fall through to the wrong-note branch in _checkHit, booking a miss and resetting the streak.

Files Reviewed (5 files)

• CHANGELOG.md - 0 issues (CRLF restored, single [Unreleased] → ### Fixed entry)
• plugins/keys_highway_3d/plugin.json - 0 issues (version bump 0.2.5 → 0.2.6)
• plugins/keys_highway_3d/screen.js - 1 issue
• plugins/keys_highway_3d/tests/instance_invariants.test.js - 0 issues (source-level invariant asserts no _resetScoring() in _maybeApplyMasteryFilter and that _anchorMissSweep(now) remains)
• tests/test_gp2rs_gpx.py - 0 issues (fixture moved to safe_fromstring; safe_xml is already a requirements.txt dependency so CI is unaffected)

Notes & assumptions

• Reviewed the net diff db83297..cc6ab0f (3 commits). Commits 1 and 2 rewrote CHANGELOG.md wholesale (LF conversion, then CRLF restore); the net effect is the single intended line, so the intermediate churn is not a finding.
• The keys_highway_3d fix itself is sound in the common case: _anchorMissSweep(now) correctly re-anchors _sweepCursor.idx against the new playable list via sweepStartIndex, so already-passed notes are not retroactively swept. _hitNoteKeys/_missedNoteKeys are genuinely keyed by noteKey(t, midi) rather than list index, so they do survive the rebuild as the comment claims.
• The residual hole is narrow: _onSeek('back') already calls forgetJudgmentsFrom, so loop-wrap and Section Practice rewinds are covered, and a past note cannot be replayed without a seek. Exploiting it needs a note hit within HIT_TOLERANCE_S of the mastery change that is subsequently filtered out and brought back.
• The _maybeApplyMasteryFilter test is a source-regex invariant, not a behavioural test. It cannot catch the finding above; a headless test over judgeHit + a simulated rebuild would.
• The GP vocal import drops lyrics when the first bar voice is silent #103 regression test pins existing correct behaviour (the described break is not present on main), consistent with the PR description stating it does not close GP vocal import drops lyrics when the first bar voice is silent #103.

Fix these issues in Kilo Cloud

---

_{Reviewed by free · Input: 0 · Output: 0 · Cached: 0}