Repository navigation
feat(sync): per-section sync apply command and piecewise map builder (#25) - #70
Conversation
…der (#25) Implements baking of per-section warp proposals into chart timing with undo/redo, including piecewise map construction, skip semantics (objects starting in skipped sections keep exact timing), span/sustain handling, beat grid remapping, and history integration.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Two blocking defects: src/main.js imports a binding tempo.js never exports, so the editor fails to load; and the piecewise map is not continuous (nor strictly increasing) across skipped sections, which reorders baked events. Details inline.
Reviewed changes — Initial review of PR #70 at 48949e9.
- Piecewise map builder (
src/section-sync.js):_buildPiecewiseMapPureturns accepted proposals into per-interval affine maps, with identity for low-confidence/failed sections; adds a_applyTimeWithMaphelper. - Apply command (
src/tempo.js):TempoPerSectionCmdremaps the beat grid and every_eachTimedobject, preserving skipped objects' exact seconds and mapping sustain/span endpoints;applyPerSectionSync/_applyPerSectionSyncPureconstruct and execute it. - Surface (
src/main.js,src/history.js): window exports for the new entry points and aTempoPerSectionCmdhistory label.
big-pickle (free) ｜ 𝕏
There was a problem hiding this comment.
Important
The new commit is cleanup-only: it removes the two unused section-sync imports flagged previously and simplifies the skip scan without changing behavior. It does not touch the outstanding blocking defects from the prior review — src/main.js:89 still imports _buildPiecewiseMapPure from ./tempo.js, which never exports it, so the editor still fails to load (SyntaxError: The requested module './tempo.js' does not provide an export named '_buildPiecewiseMapPure'), and the map-continuity and span-clamp issues are unchanged. Merge stays blocked until those land.
Reviewed changes — Incremental review of PR #70 at 4a05394, delta since the prior review at 48949e9.
- Removed unused imports in
src/tempo.js: dropped_alignSectionsPureand_sectionRegionsPurefrom the./section-sync.jsimport; both remaining bindings are used. This addresses the priortempo.js:34note. - Simplified
TempoPerSectionCmd.exec's skip scan: reordered the end-of-domain guard and collapsed the nestedifinto one assignment — logically identical (&&commutes; thebreakwas already unconditional).
big-pickle (free) ｜ 𝕏
|
Pullfrog stalled The agent stopped emitting events for 120s and was killed by the activity-timeout watchdog. The model produced no output at all before the stall — the request was sent but nothing came back. This is usually transient; re-running often succeeds.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| // Enforce continuity: if a section is skipped, then W = identity there. | ||
| // An accepted section adjacent to skipped must be pinned to identity at the shared boundary. | ||
| for (let i = 0; i < temp.length; i++) { |
There was a problem hiding this comment.
CRITICAL: Continuity enforcement overwrites left boundary when both neighbors are skipped
When an accepted interval is sandwiched between two skipped intervals (lines 645-652), the right boundary constraint at line 650-652 overwrites the left boundary constraint at line 645-647 because both assign to iv.intercept. The left boundary pin to identity is lost.
Per the design (docs/PER-SECTION-SYNC-DESIGN.md L90-98), an accepted run adjacent to skipped sections must have pinned identity endpoints at BOTH boundaries. If it cannot satisfy both (i.e., would require slope=1 but proposal has different slope), the apply should be rejected/flagged rather than silently concatenated.
| for (let i = 0; i < temp.length; i++) { | |
| // Check boundaries - if neighbor is skipped, constrain to pass through (boundary, boundary) | |
| // Left boundary | |
| const leftSkipped = i > 0 && !temp[i - 1].accepted; | |
| // Right boundary | |
| const rightSkipped = i < temp.length - 1 && !temp[i + 1].accepted; | |
| if (leftSkipped && rightSkipped) { | |
| // Cannot satisfy both pinned boundaries unless slope == 1; reject this run | |
| iv.accepted = false; | |
| iv.slope = 1; | |
| iv.intercept = 0; | |
| } else if (leftSkipped) { | |
| // Previous is skipped; constrain iv so that W(iv.start) = iv.start (identity) | |
| iv.intercept = iv.start - iv.slope * iv.start; | |
| } else if (rightSkipped) { | |
| // Next is skipped; constrain iv so that W(iv.end) = iv.end | |
| iv.intercept = iv.end - iv.slope * iv.end; | |
| } |
There was a problem hiding this comment.
Fixed in 6529806. Runs with both neighbors skipped are now rejected instead of overwriting the left boundary pin.
| if (endKind === 'sustain') { | ||
| if (typeof o.sustain === 'number' && o.sustain > 0) { | ||
| const ntEnd = this.mapResult.map(tOrig + o.sustain); | ||
| o.sustain = Math.max(0, _r3((typeof ntEnd === 'number' && isFinite(ntEnd)) ? (ntEnd - nt) : o.sustain)); |
There was a problem hiding this comment.
CRITICAL: Silent clamping of negative sustain violates design rejection rule
Math.max(0, ...) silently truncates a sustain whose mapped endpoint lands at or before its mapped start (e.g., a note starting in an accepted section that sustains into a skipped/identity section). The design (docs/PER-SECTION-SYNC-DESIGN.md L130-136) requires rejecting the apply for zero/negative results, not clamping.
| o.sustain = Math.max(0, _r3((typeof ntEnd === 'number' && isFinite(ntEnd)) ? (ntEnd - nt) : o.sustain)); | |
| const ntEnd = this.mapResult.map(tOrig + o.sustain); | |
| const mappedSustain = (typeof ntEnd === 'number' && isFinite(ntEnd)) ? (ntEnd - nt) : o.sustain; | |
| if (mappedSustain <= 0) return false; // reject apply | |
| o.sustain = _r3(mappedSustain); |
There was a problem hiding this comment.
Fixed in 4cd190e. The sustain is now rejected (kept original) instead of clamped to 0 when the mapped duration is zero/negative.
| } | ||
| } else if (endKind === 'span') { | ||
| if (typeof o.end_time === 'number') { | ||
| const ntEnd = this.mapResult.map(o.end_time); |
There was a problem hiding this comment.
WARNING: No validation for span endpoint ordering
When mapping end_time for span notes (lines 4022-4027), there's no check that the mapped end time is >= mapped start time. If the map is non-monotone or the span crosses a skipped/accepted boundary, end_time could end up before the mapped start, creating an invalid span.
| const ntEnd = this.mapResult.map(o.end_time); | |
| if (typeof o.end_time === 'number') { | |
| const ntEnd = this.mapResult.map(o.end_time); | |
| if (typeof ntEnd === 'number' && isFinite(ntEnd)) { | |
| if (ntEnd < nt) return false; // reject apply | |
| o.end_time = _r3(ntEnd); | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in 79c79f5. Span end_time that maps before the mapped start is now rejected.
| * Objects starting in a skipped section retain their timing fields exactly. | ||
| * For accepted sections, map the timestamp. | ||
| */ | ||
| export function _applyTimeWithMap(t, mapResult, proposals, confidenceFloor = 0.7) { |
There was a problem hiding this comment.
SUGGESTION: Unused export _applyTimeWithMap
This function is exported but never called — TempoPerSectionCmd.exec (src/tempo.js:4000-4015) re-implements the identical skip scan inline. Keeping two copies invites divergence; either route the command through this helper or drop it.
Consider removing this export and using the helper in TempoPerSectionCmd.exec for DRY, or remove it if not needed.
There was a problem hiding this comment.
Fixed in 23e5dec. Removed the unused _applyTimeWithMap export.
| if (iv.accepted) { | ||
| // Check boundaries - if neighbor is skipped, constrain to pass through (boundary, boundary) | ||
| // Left boundary | ||
| if (i === 0) { |
There was a problem hiding this comment.
WARNING: First interval continuity at domain start not enforced
The comment at line 644 says "domain start is identity by default" but no constraint is applied. If the first interval is accepted and the domain start != 0, or if the first interval's map doesn't naturally pass through (domain.start, domain.start), there will be a discontinuity at the domain boundary. The map should be constrained so W(domain.start) = domain.start.
| if (i === 0) { | |
| // Left boundary | |
| if (i === 0) { | |
| // Leftmost - constrain to identity at domain start | |
| iv.intercept = domain.start - iv.slope * domain.start; | |
| } else if (!temp[i - 1].accepted) { |
There was a problem hiding this comment.
Fixed in 6529806. First interval is now pinned to identity at domain.start.
| if (i < temp.length - 1 && !temp[i + 1].accepted) { | ||
| // Next is skipped; constrain iv so that W(iv.end) = iv.end | ||
| iv.intercept = iv.end - iv.slope * iv.end; | ||
| } else if (i === temp.length - 1) { |
There was a problem hiding this comment.
WARNING: Last interval continuity at domain end not enforced
The comment at line 654 acknowledges this but does nothing. If the last interval is accepted, the map should be constrained so W(domain.end) = domain.end for continuity at the domain boundary.
| } else if (i === temp.length - 1) { | |
| } else if (i === temp.length - 1) { | |
| // Last interval - constrain to identity at domain end | |
| iv.intercept = domain.end - iv.slope * domain.end; | |
| } |
There was a problem hiding this comment.
Fixed in 6529806. Last interval is now pinned to identity at domain.end.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous Review Summaries (6 snapshots, latest commit 942d22d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 942d22d)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit 3bfd9bc)This review did not finish. The model reached its output limit before it Previous review (commit 3bfd9bc)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit f33669e)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 6529806)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 435eb7f)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by efficient · Input: 52.9K · Output: 18.3K · Cached: 495.8K |
There was a problem hiding this comment.
Important
The re-export commits fix the module link error — the editor links again and window._buildPiecewiseMapPure now receives a value. But the continuity pass added in 435eb7f only pins an accepted interval when a neighbour is skipped, and its two seam assignments overwrite each other. An accepted section flanked by skipped sections still yields a discontinuous, non-monotone map (event order reversal on apply); details inline. Prior non-blocking threads on _applyTimeWithMap and the sustain clamp remain unaddressed.
Reviewed changes — Incremental review of PR #70 at 435eb7f, delta since the prior review at 4a05394.
- Re-exported
_buildPiecewiseMapPure(src/tempo.js): addedexport { _buildPiecewiseMapPure } from './section-sync.js', resolving the missing named export thatsrc/main.js:89imports; verified the binding is present on the module. - Continuity pass in
_buildPiecewiseMapPure(src/section-sync.js): acceptance is now recorded in a firsttemppass, then a second pass re-assigns an accepted interval'sinterceptwhen its left or right neighbour is skipped.
big-pickle (free) ｜ 𝕏
| } else if (!temp[i - 1].accepted) { | ||
| // Previous is skipped; constrain iv so that W(iv.start) = iv.start (identity) | ||
| iv.intercept = iv.start - iv.slope * iv.start; | ||
| } | ||
| // Right boundary | ||
| if (i < temp.length - 1 && !temp[i + 1].accepted) { | ||
| // Next is skipped; constrain iv so that W(iv.end) = iv.end | ||
| iv.intercept = iv.end - iv.slope * iv.end; |
There was a problem hiding this comment.
The continuity pass only pins an accepted interval when a neighbour is skipped, and both seam assignments write intercept — so the right-edge pin erases the left-edge pin instead of reconciling the two. An accepted section flanked by skipped sections therefore ends discontinuous and non-monotone at its left seam, and adjacent accepted sections are never made continuous at all (neither neighbour is skipped).
Technical details
# Piecewise map continuity enforced on one side only, with the two pins overwriting
## Affected sites
- `src/section-sync.js:647` — left-edge pin sets `intercept = iv.start - iv.slope * iv.start`.
- `src/section-sync.js:652` — right-edge pin overwrites with `intercept = iv.end - iv.slope * iv.end`; when both neighbours are skipped the left pin is lost (a single affine line cannot pass through identity at both edges unless `slope === 1`).
- `src/section-sync.js:650` — the `!temp[i + 1].accepted` guard means an accepted↔accepted seam is never pinned, so `W` can still jump or reverse between two adjacent accepted sections.
## Evidence
`_buildPiecewiseMapPure([{ok:false,start:0,end:10,confidence:0},{ok:true,start:10,end:20,slope:2,intercept:0,confidence:0.9},{ok:false,start:20,end:30,confidence:0}], {start:0,end:30})` yields the accepted interval with `intercept: -20`, and `map(9.9) = 9.9`, `map(10) = 0`, `map(10.1) = 0.2` — a backwards jump at the left seam. `docs/PER-SECTION-SYNC-DESIGN.md` L12 and L90-98 require one continuous, strictly increasing map with identity-pinned edges around skipped intervals (and rejection of a run that cannot be fit).
## Required outcome
- Resolve each accepted interval's `intercept` once from the running map value at its start (identity when the previous interval is skipped), so both seams are consistent and the map stays strictly increasing.
- A run that cannot be fit with a pinned identity edge must be flagged/rejected rather than silently emitting a non-monotone map.
- A unit test over the accepted-between-two-skipped case would have caught this; none exists for `_buildPiecewiseMapPure`.
There was a problem hiding this comment.
Important
The rewritten continuity pass in 6529806 fixes the seam overwrite and the sandwich case, but it now treats both domain edges as identity seams. That silently reduces a single accepted section covering the whole chart to a no-op — the design explicitly permits the uniform affine map there — and rewrites an adjacent accepted section's intercept, reintroducing a non-monotone map at an accepted↔accepted seam. Details inline.
Reviewed changes — Incremental review of PR #70 at 6529806, delta since the prior pullfrog review at 435eb7f.
- Fixed the module link error at its source (
src/main.js,src/tempo.js):src/main.jsnow imports_buildPiecewiseMapPuredirectly from./section-sync.js, and thetempo.jsre-export was removed. - Rewrote the continuity pass in
_buildPiecewiseMapPure(src/section-sync.js): seams are computed asleftPinned/rightPinned; a run pinned on both sides is demoted to identity instead of the twointerceptassignments overwriting each other. - Removed the dead
_applyTimeWithMapexport (src/section-sync.js) and its now-orphaned callers note. - Sustain/span rejection (
src/tempo.js): a mapped sustain<= 0and a span end mapping before its start nowreturninstead of clamping.
big-pickle (free) ｜ 𝕏
| if (skip) return; | ||
| const nt = this.mapResult.map(tOrig); | ||
| if (typeof nt === 'number' && isFinite(nt)) { | ||
| o[tf] = _r3(nt); |
There was a problem hiding this comment.
CRITICAL: Partial mapping creates inconsistent note state
The start time o[tf] is updated at line 4016 BEFORE the sustain/span validation at lines 4022 and 4030. If validation fails (early return), the note has a mapped start time but original sustain/end_time, creating an inconsistent state during command execution. The fix should validate the full mapping (including sustain/span) before mutating any fields, or revert the start time on validation failure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (endKind === 'sustain') { | ||
| if (typeof o.sustain === 'number' && o.sustain > 0) { | ||
| const ntEnd = this.mapResult.map(tOrig + o.sustain); | ||
| if (typeof ntEnd === 'number' && isFinite(ntEnd)) { |
There was a problem hiding this comment.
WARNING: ntEnd not finite leaves start time mapped but sustain unchanged
If this.mapResult.map(tOrig + o.sustain) returns non-finite, the sustain is not updated (line 4020-4024 skipped) but the start time at line 4016 was already mutated. Same partial-mapping issue as line 4016.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } else if (endKind === 'span') { | ||
| if (typeof o.end_time === 'number') { | ||
| const ntEnd = this.mapResult.map(o.end_time); | ||
| if (typeof ntEnd === 'number' && isFinite(ntEnd)) { |
There was a problem hiding this comment.
WARNING: ntEnd not finite leaves start time mapped but end_time unchanged
If this.mapResult.map(o.end_time) returns non-finite, the end_time is not updated (line 4029-4031 skipped) but the start time at line 4016 was already mutated. Same partial-mapping issue.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| iv.slope = 1; | ||
| iv.intercept = 0; | ||
| } else if (leftPinned) { | ||
| const boundary = i === 0 ? domain.start : iv.start; |
There was a problem hiding this comment.
SUGGESTION: Use temp[i-1].end for boundary instead of iv.start
The boundary for a left-pinned accepted interval (whose previous neighbor is skipped) should be the shared seam temp[i-1].end, which equals iv.start only if regions are perfectly contiguous. Using temp[i-1].end is more explicit and robust against any future region-splitting changes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
The run-based approach now uses / for anchors, which is equivalent and more robust for multi-section runs. No change needed.
| const boundary = i === 0 ? domain.start : iv.start; | ||
| iv.intercept = boundary - iv.slope * boundary; | ||
| } else if (rightPinned) { | ||
| const boundary = i === temp.length - 1 ? domain.end : iv.end; |
There was a problem hiding this comment.
SUGGESTION: Use temp[i+1].start for boundary instead of iv.end
The boundary for a right-pinned accepted interval (whose next neighbor is skipped) should be the shared seam temp[i+1].start, which equals iv.end only if regions are perfectly contiguous. Using temp[i+1].start is more explicit and robust.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
The run-based approach now uses first.start/last.end for anchors, which is equivalent and more robust for multi-section runs. No change needed.
|
Addressed the review on Pushed as
|
Prevents inconsistent note state where start time was mapped but sustain/end_time validation failed, leaving partial mutations.
Independent per-section fits can propose different target times at shared boundaries. Now detects and exposes seam conflicts.
There was a problem hiding this comment.
Important
The new interior seam check in _buildPiecewiseMapPure refuses Apply on the engine's own two-section fixture — independently fitted sections differ by ~25 ms at their shared boundary, which the 1e-6 threshold flags as a conflict and _applyPerSectionSyncPure then returns { applied: false }. Details inline.
Reviewed changes — Incremental review of PR #70 at 3bfd9bc, delta since the prior pullfrog review at f33669e.
- Detected adjacent accepted-section seams (
src/section-sync.js): added an interior loop that compares the left and right intervals' mapped value at each shared boundary and pushes aseam-conflictwhen they differ by more than1e-6. - Validated the full note mapping before mutating (
src/tempo.js):TempoPerSectionCmd.execnow computes the mapped sustain/end_time first and writes the note's fields only once every check passes, so a rejected sustain or span no longer leaves a partially mutated note. - Returned a structured apply result (
src/tempo.js):_applyPerSectionSyncPurenow returns{ applied, reason, conflicts }instead of a bare boolean.
big-pickle (free) ｜ 𝕏
|
Addressed the one review thread on PR #70 ( Fix: All 401 node tests pass and Task list (6/6 completed)
|
#25) Independently fitted accepted sections disagree by ordinary fit noise at their shared boundary (~25 ms on the engine's two-section fixture). The interior 1e-6 seam check flagged that as a conflict, so _applyPerSectionSyncPure refused the canonical apply. Reconcile each interior seam by shifting the later line's phase to the earlier line's mapped edge, preserving one continuous piecewise map; keep the conflicts path for the both-ends-identity case. Add a regression test running _alignSectionsPure output through _buildPiecewiseMapPure, plus the #25 CHANGELOG entry.
…itive slopes (#25) CodeFactor flagged two Complex Methods newly added by this PR — _buildPiecewiseMapPure and TempoPerSectionCmd.exec. Extract each into single-purpose helpers with no behavior change: - _buildPiecewiseMapPure -> _proposalAcceptedPure + _enforceRunContinuityPure (_reconcileRunSeamsPure / _pinRunToIdentityPure) + _piecewiseAtPure. - TempoPerSectionCmd.exec -> _mapTimed (_isSkipped / _mappedSustain / _mappedSpanEnd), which validates the whole mapping before any write. While extracting helpers, a proposal with a nonpositive slope is now treated as inapplicable (design L78-80): accepting one could bake a flat or backwards section and reverse event order on apply. Add apply-path tests (exact skip preservation, span mapping, undo/redo, a surfaced both-sided conflict) and map tests for the slope guard and seam monotonicity.
There was a problem hiding this comment.
Important
The new nonpositive-slope guard is applied on the builder side only. _isSkipped still classifies a section as skipped on ok / confidence alone, so an object that starts in an inapplicable (nonpositive-slope) section can have its sustain or span endpoint recomputed from an accepted neighbour — violating the design's skip guarantee. Details inline.
Reviewed changes — Incremental review of PR #70 at 416a483, delta since the prior pullfrog review at 942d22d.
- Split
_buildPiecewiseMapPureinto named helpers (src/section-sync.js):_proposalAcceptedPure,_reconcileRunSeamsPure,_pinRunToIdentityPure,_enforceRunContinuityPureand_piecewiseAtPure; the map closure and continuity pass are otherwise behavior-preserving. - Added a strictly-positive-slope acceptance rule: a nonpositive (or non-finite)
slopenow makes a proposal inapplicable, so its section degrades to identity instead of baking a non-monotone line. - Extracted
TempoPerSectionCmd's inline skip scan and mapping (src/tempo.js): new_isSkipped,_mappedSustain,_mappedSpanEndand_mapTimedmethods; the exec path is unchanged. - Added apply-path and builder tests: new
tests/per_section_apply.test.mjs(skip preservation, span mapping, undo/redo symmetry, identity-conflict refusal) and nonpositive-slope / differing-slope cases intests/section_sync.test.mjs.
big-pickle (free) ｜ 𝕏
|
Addressed the review finding on PR #70 (nonpositive-slope skip divergence):
|
Implements per-section sync application/baking infrastructure per the design in #23 and engine in #24.
Changes
src/section-sync.js):_buildPiecewiseMapPureconstructs a continuous piecewise map from accepted proposals (respecting confidence floor), with identity mapping for skipped/low-confidence sections. Added_applyTimeWithMaphelper for time classification by original section.src/tempo.js): AddedTempoPerSectionCmd(undoable) that applies the piecewise map to beats and all timed objects via_eachTimedwalk. Skip semantics: objects starting in skipped sections retain their exact original timing. For accepted sections, mapped start times and spans/sustains are recomputed (span endpoints mapped separately, sustain length derived from mapped difference). Beat grid is remapped and loops reprojected.applyPerSectionSync(params)and_applyPerSectionSyncPure(mapResult, proposals, confidenceFloor)exported fromtempo.js, also exposed onwindowinmain.js. History label added forTempoPerSectionCmd.Testing
docs/PER-SECTION-SYNC-DESIGN.md(skip preservation, boundary handling, offline baking, undoable application).tests/section_sync.test.mjs) and all other suites unaffected.Relates to #8, #25. Closes #25 when merged (depending on review scope).