Add typed TS SDK with C++ parity checks#2
Conversation
- Introduce `packages/ts-sdk` as the consumable JS/TS package - Move shared static scale data into `packages/shared-data` - Add C++ spec runner and parity tests to keep outputs aligned
📝 WalkthroughWalkthroughThis PR establishes a complete Turborepo monorepo architecture by introducing a comprehensive TypeScript SDK ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (22)
packages/ts-sdk/src/slonimsky/index.ts-17-28 (1)
17-28:⚠️ Potential issue | 🟠 Major
minIntervalreturnsundefinedfor sequences with fewer than 2 elements.When
sequence.length < 2,intervals()returns an empty array, makingsequenceIntervals[0]undefined. The function then returnsundefined, violating its declarednumberreturn type.🐛 Proposed fix: Add guard for empty intervals
export function minInterval(sequence: SlonimskySequence): number { var sequenceIntervals = intervals(sequence); + if (sequenceIntervals.length === 0) { + throw new Error("Cannot compute minInterval for sequence with fewer than 2 elements"); + } var current = sequenceIntervals[0]; for (var index = 1; index < sequenceIntervals.length; index += 1) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/slonimsky/index.ts` around lines 17 - 28, minInterval currently reads sequenceIntervals[0] which is undefined when intervals(sequence) returns an empty array for sequences with fewer than 2 elements; add a guard at the start of minInterval to handle an empty intervals array (e.g., const sequenceIntervals = intervals(sequence); if (sequenceIntervals.length === 0) return 0;) before accessing sequenceIntervals[0], keeping the rest of the loop and return behavior intact and referencing the minInterval function and intervals helper.packages/ts-sdk/test/support/spec.ts-6-16 (1)
6-16:⚠️ Potential issue | 🟠 MajorAdd timeout and make spec-runner path resolution robust.
The hardcoded executable path lacks platform awareness (no
.exefor Windows) and build-layout flexibility, andexecFileSyncwithout a timeout can hang test jobs indefinitely.Apply timeout bounds and add fallback path resolution with existence checks:
🔧 Proposed hardening patch
import { execFileSync } from "node:child_process"; +import { existsSync } from "node:fs"; import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; const currentDirectory = dirname(fileURLToPath(import.meta.url)); -const specRunner = resolve(currentDirectory, "../../../cpp-sdk/build/musicplusplus-spec"); +const specRunnerCandidates = [ + process.env.MUSICPP_SPEC_RUNNER, + resolve(currentDirectory, "../../../cpp-sdk/build/musicplusplus-spec"), + resolve(currentDirectory, "../../../cpp-sdk/build/musicplusplus-spec.exe"), + resolve(currentDirectory, "../../../cpp-sdk/build/Debug/musicplusplus-spec"), + resolve(currentDirectory, "../../../cpp-sdk/build/Debug/musicplusplus-spec.exe"), + resolve(currentDirectory, "../../../cpp-sdk/build/Release/musicplusplus-spec"), + resolve(currentDirectory, "../../../cpp-sdk/build/Release/musicplusplus-spec.exe"), +].filter(Boolean) as string[]; + +const specRunner = specRunnerCandidates.find((candidate) => existsSync(candidate)); +if (!specRunner) { + throw new Error("musicplusplus-spec executable not found. Set MUSICPP_SPEC_RUNNER or build cpp-sdk first."); +} export function spec(command: string, args: Record<string, string>): any { const cliArgs: string[] = [command]; const keys = Object.keys(args); @@ - const output = execFileSync(specRunner, cliArgs, { encoding: "utf8" }).trim(); + const output = execFileSync(specRunner, cliArgs, { + encoding: "utf8", + timeout: 15_000, + maxBuffer: 1024 * 1024, + }).trim();Note: Verify the exact output paths that CMake uses in your build layout and adjust the candidates list if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/test/support/spec.ts` around lines 6 - 16, The spec function currently uses a hardcoded specRunner path and calls execFileSync without a timeout; update spec to build a list of candidate paths (using currentDirectory and resolve) that include platform-specific names (add ".exe" on Windows) and alternate build-layout locations, check fs.existsSync for each candidate and pick the first existing path as specRunner, and invoke execFileSync with a reasonable timeout option (e.g., 30s) and proper encoding to avoid hanging tests; ensure errors throw if no candidate is found.packages/ts-sdk/package.json-27-27 (1)
27-27:⚠️ Potential issue | 🟠 Major
verifyshould build before smoke tests.Line 27 runs smoke checks that depend on
dist/index.jsanddist/index.mjs, butverifynever createsdist. This makes verification order-dependent.🐛 Proposed fix
- "verify": "npm run typecheck && npm run test && npm run test:smoke", + "verify": "npm run typecheck && npm run test && npm run build && npm run test:smoke",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/package.json` at line 27, The verify npm script currently runs typecheck, test, and test:smoke but never builds artifacts required by the smoke tests; update the "verify" script in package.json to invoke the build step before running "test:smoke" (e.g., ensure "npm run build" is executed prior to "npm run test:smoke") so that the smoke tests which depend on dist/index.js and dist/index.mjs have the produced artifacts.packages/cpp-sdk/package.json-8-8 (1)
8-8:⚠️ Potential issue | 🟠 Major
specscript does not execute the spec runner.Line 8 only prints the binary path, so no contract generation/verification actually happens.
🐛 Proposed fix
- "spec": "npm run build && node -e \"var p=process.platform==='win32'?'windows':'unix';var f=p==='windows'?'build/windows/musicplusplus-spec.exe':'build/unix/musicplusplus-spec';console.log(f);\"", + "spec": "npm run build && node -e \"var p=process.platform==='win32'?'windows':'unix';var f=p==='windows'?'build/windows/musicplusplus-spec.exe':'build/unix/musicplusplus-spec';require('child_process').execFileSync(f,[],{stdio:'inherit'});\"",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cpp-sdk/package.json` at line 8, The "spec" npm script currently only prints the path to the spec binary rather than running it; update the "spec" script in package.json so it actually invokes the platform-specific binary instead of console.log. Replace the node -e command so it resolves the correct binary (same p and f logic) and then runs it (e.g., via child_process.execFileSync or spawnSync with stdio:'inherit') so the spec runner executes and returns its exit code; keep the platform-detection logic but call the binary rather than printing its path.packages/cpp-sdk/package.json-9-9 (1)
9-9:⚠️ Potential issue | 🟠 Major
testcurrently masks regressions by always succeeding.Line 9 exits with code 0 unconditionally, so workspace test pipelines get a false green signal even when parity is broken.
🐛 Proposed fix
- "test": "node -e \"process.exit(0)\"", + "test": "npm run spec",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cpp-sdk/package.json` at line 9, The package.json "test" script currently always exits 0 ("test": "node -e \"process.exit(0)\"") which masks failures; update the "test" script to invoke the real test runner for this package (for example replace with the project's test command like "jest" or "vitest" or "npm run test:unit") so CI runs actual tests, and if there's no test suite yet change the script to a failing placeholder (e.g. a non-zero exit) to avoid false green signals until real tests are added.packages/ts-sdk/src/data/index.ts-11-13 (1)
11-13:⚠️ Potential issue | 🟠 MajorClone scale records at the API boundary.
readonlydoes not protect these objects at runtime. Storingscalesby reference and returningthis.scales.slice()means outside mutations can change laterfindScale()results and leak shared state betweenScaleDatabaseinstances.Suggested fix
public constructor(scales: readonly ScaleRecord[] = SCALE_RECORDS) { - this.scales = scales; + this.scales = scales.map(function (scale) { + return { + intervals: scale.intervals.slice(), + scaleName: scale.scaleName, + sheetName: scale.sheetName + }; + }); } @@ public getAllScales(): readonly ScaleMatch[] { - return this.scales.slice(); + return this.scales.map(function (scale) { + return { + intervals: scale.intervals.slice(), + scaleName: scale.scaleName, + sheetName: scale.sheetName + }; + }); }Also applies to: 62-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/data/index.ts` around lines 11 - 13, The constructor for ScaleDatabase currently stores the provided scales array by reference (public constructor(scales: readonly ScaleRecord[] = SCALE_RECORDS) { this.scales = scales; }) which allows external mutation to affect internal state and later findScale results; update the constructor to deep-clone or at least shallow-copy the incoming array and its objects (clone SCALE_RECORDS when used as default) before assigning to this.scales, and ensure any accessor that returns scales (the method that currently returns this.scales.slice() around lines 62-63) returns a cloned array of cloned ScaleRecord objects to prevent leaking mutable references; reference ScaleDatabase, constructor, this.scales, findScale, and SCALE_RECORDS when making these defensive copies.packages/ts-sdk/src/analysis/quantize.ts-37-39 (1)
37-39:⚠️ Potential issue | 🟠 MajorUse the destination modulus when synthesizing output notes.
outNoteand the returnednotesvector are keyed offinputScale.mod. If callers transpose between scales with different moduli, octave placement and the result metadata are wrong. If cross-mod input is unsupported, this should be validated up front instead.Suggested fix
- var mod = inputScale.mod; + var inputMod = inputScale.mod; + var outputMod = outputScale.mod; @@ - var inPitchClass = (note - inRoot) % mod; + var inPitchClass = (note - inRoot) % inputMod; if (inPitchClass < 0) { - inPitchClass += mod; + inPitchClass += inputMod; } - var octave = truncateTowardZero((note - inRoot) / mod); - if (((note - inRoot) < 0) && (((note - inRoot) % mod) !== 0)) { + var octave = truncateTowardZero((note - inRoot) / inputMod); + if (((note - inRoot) < 0) && (((note - inRoot) % inputMod) !== 0)) { octave -= 1; } @@ - var outNote = outPitchClass + outRoot + (octave * mod); + var outNote = outPitchClass + outRoot + (octave * outputMod); @@ inPitchClass = (notes[index] - inRoot) % mod; if (inPitchClass < 0) { - inPitchClass += mod; + inPitchClass += inputMod; } @@ - outNote = outPitchClass + outRoot + (octave * mod); + outNote = outPitchClass + outRoot + (octave * outputMod); } } } @@ notes: new PositionVector(notesData, { - mod: inputScale.mod, + mod: outputScale.mod, rangeUpdate: inputScale.rangeUpdate, user: inputScale.user, userRange: inputScale.userRangeAlso applies to: 65-66, 97-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/analysis/quantize.ts` around lines 37 - 39, The code currently keys synthesized outNote and the returned notes vector using inputScale.mod (var mod = inputScale.mod), causing wrong octave placement when transposing to a scale with a different modulus; update the logic in the quantize function to use the destination/outScale modulus (use outScale.mod or outRoot/outMod where outRoot/outMod are derived) when constructing outNote and populating the notes array (also adjust occurrences around the blocks at lines referenced: the initial mod assignment, the outNote creation, and the notes population at 65-66 and 97-102), or alternatively add an upfront validation that inputScale.mod === outScale.mod and throw/return an error if they differ; ensure all references to mod for output synthesis use the destination modulus variable.packages/ts-sdk/src/analysis/quantize.ts-5-26 (1)
5-26:⚠️ Potential issue | 🟠 MajorReplace the
-1sentinel inquantize.
-1is a valid scale value, so this misclassifies legitimate bounds and also makes an empty scale silently return-1.Suggested fix
export function quantize(note: number, scale: readonly number[], left: boolean = true): number { - var lower = -1; - var upper = -1; + var lower: number | undefined; + var upper: number | undefined; @@ - if (lower === -1) { - return upper; + if (typeof lower === "undefined") { + if (typeof upper === "undefined") { + throw new Error("Cannot quantize against an empty scale"); + } + return upper; } - if (upper === -1) { + if (typeof upper === "undefined") { return lower; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/analysis/quantize.ts` around lines 5 - 26, The function quantize misuses -1 as a sentinel (which can be a valid scale value) and silently returns -1 for an empty scale; change the sentinel variables lower and upper in quantize to use undefined (or null) instead of -1, update the comparisons/tests from === -1 to === undefined, and ensure the function handles an empty scale explicitly (e.g., return undefined or throw an Error) and the return type/signature is updated accordingly so callers know quantize may not always return a number.packages/ts-sdk/src/core/conversions.ts-73-77 (1)
73-77:⚠️ Potential issue | 🟠 MajorFix inconsistent normalization base in
positionsToBinary.At Line 75, normalization uses
positionData[0]while returned offset usesminimum. For non-sorted inputs, this produces incorrect bit placement.💡 Proposed fix
var minimum = minNumber(positionData); for (index = 0; index < positionData.length; index += 1) { - var normalizedPosition = positionData[index] - positionData[0]; + var normalizedPosition = positionData[index] - minimum; binaryData[euclideanDivision(normalizedPosition, range).remainder] = 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/conversions.ts` around lines 73 - 77, In positionsToBinary the normalization step uses positionData[0] but the offset variable is computed from minNumber(positionData) (minimum), causing incorrect bit indices for unsorted inputs; change the normalization to subtract minimum (use minimum instead of positionData[0]) when computing normalizedPosition so the remainder from euclideanDivision(normalizedPosition, range) aligns with the returned offset; update the line referencing normalizedPosition to use minimum and keep euclideanDivision(binaryIndex, range) and binaryData assignment unchanged.packages/ts-sdk/test/parity.test.ts-63-71 (1)
63-71:⚠️ Potential issue | 🟠 MajorHarden scalar parsing in
spec()to preventNaNmismatches from non-numeric strings.At lines 69-70, the function coerces non-JSON output via
Number(output)without validation. This yieldsNaNfor boolean strings ("true","false") and other non-numeric scalars, causing spurious parity test failures.Proposed fix
function spec(command: string, args: Record<string, string>): any { const cliArgs: string[] = [command]; const keys = Object.keys(args); for (let index = 0; index < keys.length; index += 1) { cliArgs.push(`--${keys[index]}`, args[keys[index]]); } const output = execFileSync(specRunner, cliArgs, { encoding: "utf8" }).trim(); - return output.startsWith("{") || output.startsWith("[") ? JSON.parse(output) : Number(output); + try { + return JSON.parse(output); + } catch { + const numeric = Number(output); + return Number.isNaN(numeric) ? output : numeric; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/test/parity.test.ts` around lines 63 - 71, The spec function currently parses non-JSON output by blindly calling Number(output), which turns non-numeric strings like "true"/"false" into NaN and breaks parity tests; update spec(command, args) to robustly coerce scalars: after trimming, if output starts with "{" or "[" keep JSON.parse(output); else if output matches boolean literals ("true" or "false") return the boolean; else if output matches a numeric pattern (integer or float, optionally with sign/decimal/exponent) or Number(output) is finite, return Number(output); otherwise return the original string. Ensure you only change parsing logic inside spec and preserve existing execFileSync usage and return conventions.packages/ts-sdk/src/core/distances.ts-48-59 (1)
48-59:⚠️ Potential issue | 🟠 MajorDon’t drop trailing elements from unequal-length distances.
Using
minLengthhere meanseuclideanDistance([1], [1, 100]),manhattanDistance([1], [1, 100]),hammingDistance([1], [1, 100]), anddifference([1], [1, 100])all ignore the extra dimension entirely. That can make unequal inputs compare as identical in downstream ranking. Either reject unequal lengths or define how the tail contributes to the metric.Also applies to: 97-123, 125-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/distances.ts` around lines 48 - 59, The distance functions (euclideanDistance, manhattanDistance, hammingDistance, difference, etc.) currently use the shorter length and ignore trailing dimensions, which can make unequal-length vectors appear identical; add an explicit length check at the start of each function (compare distanceSource(left).length and distanceSource(right).length) and throw a clear TypeError (or RangeError) when lengths differ, so callers must provide equal-length inputs (or handle padding/aggregation upstream); update euclideanDistance, manhattanDistance, hammingDistance and the difference helper(s) to perform this validation before computing the metric.packages/ts-sdk/src/selection/index.ts-125-134 (1)
125-134:⚠️ Potential issue | 🟠 MajorUse the rotated criterion when deriving the returned offset.
This branch builds
resultIntervalsfromrotatedPositionCriterion, butselectedOffsetstill comes from the unrotatedpositionCriterion. Any non-zerorotationtherefore returns correct interval spans with the wrong offset.Proposed fix
var selectedOffset = intervalSource.offset; - if (positionCriterion.at(0) >= 0) { - for (intervalIndex = 0; intervalIndex < positionCriterion.at(0); intervalIndex += 1) { + var firstPosition = rotatedPositionCriterion.at(0); + if (firstPosition >= 0) { + for (intervalIndex = 0; intervalIndex < firstPosition; intervalIndex += 1) { selectedOffset += intervalSource.at(intervalIndex); } } else { - for (intervalIndex = positionCriterion.at(0); intervalIndex < 0; intervalIndex += 1) { + for (intervalIndex = firstPosition; intervalIndex < 0; intervalIndex += 1) { selectedOffset -= intervalSource.at(intervalIndex); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/selection/index.ts` around lines 125 - 134, The computed selectedOffset is still derived from the unrotated positionCriterion while resultIntervals were built from rotatedPositionCriterion, so update the offset calculation to use rotatedPositionCriterion (and its at(0) value) instead of positionCriterion: replace references to positionCriterion.at(0) in the initial conditional and both for-loop bounds with rotatedPositionCriterion.at(0), and iterate against intervalSource the same way but using rotatedPositionCriterion to compute selectedOffset so offsets align with the rotated intervals (consider the rotation variable and resultIntervals creation in the same function to confirm consistency).packages/ts-sdk/src/core/rhythm.ts-160-161 (1)
160-161:⚠️ Potential issue | 🟠 MajorValidate
stepsbefore constructing a deep rhythm.
deepRhythmValues()usesstepsas the modulus, and this wrapper also storesstepsas the resulting vector modulus. Withsteps <= 0, the API currently builds an invalidPositionVectorinstead of rejecting bad input.Proposed fix
export function deepRhythm(steps: number, events: number, multiplicity: number, offset: number = 0): PositionVector { + if (steps <= 0) { + throw new Error("steps must be positive"); + } return new PositionVector(deepRhythmValues(steps, events, multiplicity), { mod: steps }).addScalar(offset); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/rhythm.ts` around lines 160 - 161, The wrapper deepRhythm must validate the steps parameter before constructing a PositionVector because deepRhythmValues and the PositionVector mod both rely on a positive modulus; add a guard at the start of deepRhythm that checks if steps is a positive integer (steps > 0) and throw a clear RangeError or TypeError if not, then call deepRhythmValues(steps, events, multiplicity) and construct the PositionVector as before; reference deepRhythm, deepRhythmValues, and PositionVector when applying the change.packages/ts-sdk/src/naming/noteNamingSystem.ts-277-278 (1)
277-278:⚠️ Potential issue | 🟠 MajorDefault
positionVectorToNoteNames()to the vector’s own modulus.Right now a
PositionVectorwithmod !== 12is still interpreted as 12-TET unless the caller repeatsmoduloValuemanually. That breaks non-12 systems and diverges from the C++ oracle path, which defaults from the vector itself.Proposed fix
public positionVectorToNoteNames(positionVector: PositionVector, options?: NoteMapperOptions): NoteResult { - return this.midiNumbersToNoteNames(positionVector.data, options); + return this.midiNumbersToNoteNames(positionVector.data, { + ...(options || {}), + moduloValue: options && typeof options.moduloValue === "number" ? options.moduloValue : positionVector.mod + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/naming/noteNamingSystem.ts` around lines 277 - 278, The method positionVectorToNoteNames currently always calls midiNumbersToNoteNames without defaulting the modulo to the PositionVector's own modulus; update positionVectorToNoteNames to detect when options is undefined or options.moduloValue is not set and set options.moduloValue = positionVector.mod (from the PositionVector object) before calling midiNumbersToNoteNames so non-12-TET vectors default to their own modulus; reference NoteMapperOptions and ensure you only mutate/clone options as appropriate before passing to midiNumbersToNoteNames.packages/ts-sdk/src/core/internal.ts-174-186 (1)
174-186:⚠️ Potential issue | 🟠 Major
lcm()still breaks on multiple zero inputs.If
valuescontains two zeroes,gcd(result, values[index])becomes0, so the recurrence computes an invalid result instead of the expected0. That can propagate a bad modulus into the LCM adapters.Proposed fix
export function lcm(values: readonly number[]): number { if (values.length === 0) { return 1; } if (values.length === 1) { return absoluteInteger(values[0]); } + if (values.some(function (value) { + return value === 0; + })) { + return 0; + } var result = absoluteInteger(values[0]); for (var index = 1; index < values.length; index += 1) { var divisor = gcd(result, values[index]); result = (result * absoluteInteger(values[index])) / divisor;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/internal.ts` around lines 174 - 186, The lcm(values) implementation doesn't handle multiple zero inputs: when gcd(result, values[index]) is 0 the recurrence produces an invalid value instead of returning 0. Fix lcm by normalizing each operand with absoluteInteger and if any normalized operand is 0 immediately return 0 (or if divisor === 0 return 0) instead of continuing the loop; update the loop in lcm (using variables result, divisor, values, absoluteInteger, gcd) to check for zero operands/divisor and return 0 early so the function yields 0 for any set containing more than one zero.packages/ts-sdk/src/core/positionVector.ts-408-444 (1)
408-444:⚠️ Potential issue | 🟠 Major
adaptToLCMmissing guard formod === 0vectors.Unlike
IntervalVector.adaptToLCMwhich checksif (vectors[index].mod === 0)and skips scaling, this implementation would cause division by zero at line 429 if any vector hasmod === 0.🐛 Proposed fix
for (index = 0; index < vectors.length; index += 1) { + if (vectors[index].mod === 0) { + adapted.push(vectors[index]); + continue; + } var scaleFactor = commonMod / vectors[index].mod;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/positionVector.ts` around lines 408 - 444, adaptToLCM currently divides by vectors[index].mod and will throw for vectors whose mod === 0; update PositionVector.adaptToLCM to guard against mod === 0 by treating those vectors as unscaled entries (skip scaling and push a copy with mod 0 into adapted) instead of computing scaleFactor or dividing by zero; mirror the behavior used in IntervalVector.adaptToLCM: when vectors[index].mod === 0, push vectors[index].slice/copy (preserving user and userRange) into adapted, and only compute commonMod and scaleFactor for non-zero mod vectors.packages/ts-sdk/src/core/binaryVector.ts-290-316 (1)
290-316:⚠️ Potential issue | 🟠 Major
adaptToLCMmay have undefined behavior if all input vectors havemod === 0.If all vectors have
mod === 0,uniquePositivereturns an empty array. Thenmodulos.length <= 1is true, and the function returns a shallow copy. This is handled correctly.However, if some vectors have
mod === 0and others have positive mods,uniquePositivefilters out zeros,lcmis computed, andscaleFactor = commonMod / vectors[index].modwould cause division by zero formod === 0vectors. But line 305 is inside the loop that doesn't check formod === 0.Wait - looking more carefully, there's no
if (vectors[index].mod === 0)guard in this implementation unlikeIntervalVector.adaptToLCM. This could cause division by zero.🐛 Proposed fix
for (var index = 0; index < vectors.length; index += 1) { + if (vectors[index].mod === 0) { + adapted.push(vectors[index]); + continue; + } var scaleFactor = commonMod / vectors[index].mod;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/binaryVector.ts` around lines 290 - 316, adaptToLCM in BinaryVector can divide by zero when some vectors have mod === 0 because uniquePositive drops zeros; update BinaryVector.adaptToLCM to handle zero-mod vectors like IntervalVector.adaptToLCM: compute modulos = uniquePositive(...), if modulos.length === 0 return vectors.slice(); compute commonMod = lcm(modulos) only when modulos non-empty, and inside the loop skip scaling/division for vectors[index].mod === 0 by either cloning them unchanged (preserve _data and offset with mod 0) or treating scaleFactor as 1 for mod 0 cases; ensure no division by zero when calculating scaleFactor and that new BinaryVector(...) uses the correct mod and offset.packages/ts-sdk/src/naming/chordAnalysis.ts-178-187 (1)
178-187:⚠️ Potential issue | 🟠 MajorSixth intervals should be mutually exclusive using
else if.Lines 178-187 incorrectly use two separate
ifstatements instead ofif-else, allowing bothhasFlatSixthandhasNaturalSixthto be set totruesimultaneously. This contradicts music theory—a chord cannot have both a flat sixth (♭6) and natural sixth (♮6) as they are distinct pitches. All other interval pairs in the function (thirds, fifths, sevenths, ninths, elevenths, thirteenths, seconds, and fourths) correctly useif-elseto enforce mutual exclusivity. Change the secondiftoelse ifon line 183.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/naming/chordAnalysis.ts` around lines 178 - 187, The sixth-interval checks currently allow both hasFlatSixth and hasNaturalSixth to be set because they use two separate ifs; update the second check so the natural-sixth branch is mutually exclusive with the flat-sixth branch by changing the second condition to an else if (i.e., make the intervals.indexOf(9) check an else if so only one of analysis.hasFlatSixth or analysis.hasNaturalSixth can be set and usedIntervals is updated accordingly); refer to the identifiers analysis.hasSixth, analysis.hasFlatSixth, analysis.hasNaturalSixth, intervals, and usedIntervals to locate the code to modify.packages/ts-sdk/src/core/positionVector.ts-270-284 (1)
270-284:⚠️ Potential issue | 🟠 Major
rotate()inPositionVectordiscards sign information usingabsoluteInteger(), breaking rotation directionality.Line 276 uses
absoluteInteger(rotationAmount), converting negative rotations to positive and makingrotate(-2)equivalent torotate(2). This inconsistently loses information compared toBinaryVector.rotate()which useseuclideanDivision(rotationAmount, length).remainderto preserve sign, andIntervalVector.rotate()which doesn't strip sign at all. The parity test atdifferential-parity.test.ts:85expectsrotate(-2)to preserve the-2in the spec, indicating directional rotation semantics are intended.Use
euclideanDivision(rotationAmount, size).remainderinstead ofabsoluteInteger()to preserve rotation direction consistently across all vector types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/positionVector.ts` around lines 270 - 284, The rotate method in PositionVector currently uses absoluteInteger(rotationAmount) which removes sign info; update PositionVector.rotate to compute the effective shift with euclideanDivision(rotationAmount, size).remainder (where size = this._data.length) instead of absoluteInteger so negative rotations are preserved, then use that remainder when indexing into rotatedData; keep the rest of the method (rotatedData population and return new PositionVector(..., asPositionVectorOptions(this))) unchanged.packages/ts-sdk/src/automation/index.ts-72-76 (1)
72-76:⚠️ Potential issue | 🟠 MajorDon't swallow every
align()failure here.This fallback converts any exception into center
0, so invalid inputs or regressions insidealign()get hidden behind a plausible-but-wrong result. Please only fall back for the expected “no aligned center” case and rethrow everything else.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/automation/index.ts` around lines 72 - 76, The calculateCenter function currently swallows all exceptions from requireAlignedCenter and returns fallbackCenter for any error; change it to only catch the specific "no aligned center" condition (e.g., the domain-specific error thrown by requireAlignedCenter or a sentinel like NoAlignedCenterError / a specific error.message) and return fallbackCenter in that case, but rethrow any other errors so real failures in requireAlignedCenter/align() surface; update the catch in calculateCenter to inspect the caught error and rethrow unless it matches the expected "no aligned center" indicator.packages/ts-sdk/src/analysis/measures.ts-423-430 (1)
423-430:⚠️ Potential issue | 🟠 MajorNormalize the input set before the generated-set comparison.
generate()is treated as order-insensitive here because each candidate is sorted before comparison, butvaluesis compared as-is. That makesisGenerated()return false negatives for the same set in a different order.Suggested fix
export function isGenerated(values: readonly number[], n: number): GeneratedResult { var size = values.length; + var normalized = values.slice().sort(function (left, right) { + return left - right; + }); for (var multiplier = 1; multiplier < n; multiplier += 1) { var generated = generate(multiplier, size, n).slice().sort(function (left, right) { return left - right; }); - if (arraysEqual(generated, values)) { + if (arraysEqual(generated, normalized)) { return { found: true, multiplier: multiplier };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/analysis/measures.ts` around lines 423 - 430, The comparison in isGenerated treats generated candidates as order-insensitive by sorting them but compares against values as-is, causing false negatives; before the loop (or at start of isGenerated) normalize values by creating a sorted copy (e.g., const normalized = values.slice().sort((a,b)=>a-b)) and then use normalized in the arraysEqual check instead of values so that arraysEqual(generated, normalized) compares two sorted arrays; ensure you don't mutate the original values array.packages/ts-sdk/src/analysis/measures.ts-455-476 (1)
455-476:⚠️ Potential issue | 🟠 MajorKeep
measureSummary()on the same modulus source as the rest of the module.Line 455 switches to
value.range, while the surrounding helpers compute againstvalue.mod. If those two fields diverge,aksakClassification,generated,regressionEvenness, and the returnedmodulono longer describe the same circle as the other summary fields.Suggested fix
- var modulo = value.range; + var modulo = value.mod;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/analysis/measures.ts` around lines 455 - 476, The measureSummary() block currently uses value.range for the local modulo, which diverges from other helpers that use value.mod; change the modulo source to value.mod so calls like isGenerated(positions, modulo), classifyAksakRhythm(modulo), isEuclidean(value, modulo) and calculateRegressionEvenness(positions, modulo) (and the returned modulo field) all reference the same circle; update the var modulo assignment to use value.mod so generated, regressionEvenness, aksakClassification and the returned modulo remain consistent with the rest of the module.
🟡 Minor comments (5)
README.md-220-228 (1)
220-228:⚠️ Potential issue | 🟡 MinorFix inconsistent numbering in the Contributing checklist.
Lines 226-228 restart numbering, which makes the instructions look broken in raw markdown and review UIs.
📝 Proposed fix
-3. Document with Doxygen-style comments -4. Rebuild and test: `npm run build && npm run docs` -5. Update this README if adding major features +5. Document with Doxygen-style comments +6. Rebuild and test: `npm run build && npm run docs` +7. Update this README if adding major features🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 220 - 228, The contributing checklist numbering is inconsistent (items restart at 3); update the final three entries so the sequence is continuous (e.g., after "Keep TS outputs aligned with C++ for mirrored features" change the subsequent "3. Document with Doxygen-style comments" to "5.", "4. Rebuild and test: `npm run build && npm run docs`" to "6.", and "5. Update this README if adding major features" to "7.") so the markdown list reads 1–7 in order and renders correctly; reference the checklist lines mentioning `packages/cpp-sdk/src/`, `packages/cpp-sdk/tools/spec_runner.cpp`, and `packages/ts-sdk/test` to locate the items to renumber.packages/cpp-sdk/src/scaleDictionary.h-135-136 (1)
135-136:⚠️ Potential issue | 🟡 MinorNormalize negative roots before indexing
noteNames.If the first interval is negative, C++
%returns a negative remainder, so this can indexnoteNamesout of bounds.Suggested fix
- int root = intervals[0] % 12; + int root = ((intervals[0] % 12) + 12) % 12; return noteNames[root];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cpp-sdk/src/scaleDictionary.h` around lines 135 - 136, The code computes int root = intervals[0] % 12 then returns noteNames[root], but C++'s % can yield a negative remainder and thus root may be negative and index noteNames out of bounds; change the normalization of root (used in the function in scaleDictionary.h where variables intervals, root and noteNames appear) to guarantee a 0..11 result (e.g., normalize by adding 12 then mod 12) before indexing noteNames so negative intervals are handled safely.packages/shared-data/scripts/generate-scale-data.mjs-60-64 (1)
60-64:⚠️ Potential issue | 🟡 MinorValidate interval values, not just the array shape.
Right now any non-integer entry inside
intervalsis emitted directly into generated C++/TS source. Failing fast here makes bad catalog edits much easier to diagnose.Suggested fix
if (!Array.isArray(scales) || scales.some((scale) => !scale || typeof scale.sheetName !== "string" || typeof scale.scaleName !== "string" || !Array.isArray(scale.intervals))) { throw new Error("Invalid scale catalog format"); }+if ( + !Array.isArray(scales) || + scales.some( + (scale) => + !scale || + typeof scale.sheetName !== "string" || + typeof scale.scaleName !== "string" || + !Array.isArray(scale.intervals) || + scale.intervals.some((interval) => !Number.isInteger(interval)) + ) +) { + throw new Error("Invalid scale catalog format"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared-data/scripts/generate-scale-data.mjs` around lines 60 - 64, The current validation only checks that scale.intervals is an array but not the values; update the validation around the scales variable (the block that throws "Invalid scale catalog format") to also confirm every entry in each scale.intervals is a finite integer (e.g., Number.isInteger(value) && Number.isFinite(value)), and throw a descriptive Error that includes the offending scale.sheetName/scale.scaleName and the invalid value(s); perform this check inline with the existing Array.isArray(scales) guard so malformed interval entries fail fast before generating C++/TS source.packages/ts-sdk/src/melody/index.ts-58-73 (1)
58-73:⚠️ Potential issue | 🟡 Minor
noteInfodoes not normalize negative MIDI values.Line 59:
var normalizedNote = inputNote % modproduces negative remainders for negative inputs in JavaScript. For example,-5 % 12 = -5, not7. This causes the pitch-class lookup at line 64 to fail for negative notes, returninginvalidNote().If negative MIDI notes are valid inputs, use proper modulo:
🐛 Proposed fix
function noteInfo(inputNote: number, value: readonly number[], mod: number): NoteInfo { - var normalizedNote = inputNote % mod; + var normalizedNote = ((inputNote % mod) + mod) % mod; var baseOctave = Math.floor(inputNote / mod);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/melody/index.ts` around lines 58 - 73, The function noteInfo uses inputNote % mod which yields negative remainders for negative inputNote; replace the normalization with a true positive modulo like normalizedNote = ((inputNote % mod) + mod) % mod and recompute baseOctave using baseOctave = Math.floor((inputNote - normalizedNote) / mod) so the octave and pitch-class lookup (value array scan and invalidNote() fallback) work correctly for negative MIDI notes; update references to normalizedNote and baseOctave in noteInfo accordingly.packages/ts-sdk/src/core/intervalVector.ts-161-177 (1)
161-177:⚠️ Potential issue | 🟡 MinorInconsistent error handling pattern:
divideVectorandmoduloVectorlack explicit zero-divisor checks.These vector methods rely on
euclideanDivisionto throw an error ifothercontains any zero values, unlikedivideScalarandmoduloScalar(lines 110-132), which explicitly validate the divisor before proceeding. While division by zero is ultimately caught (viaeuclideanDivision's check on line 142-143), the indirect error handling makes the contract less clear and inconsistent with the scalar variant. Consider adding explicit validation individeVectorandmoduloVectorfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/intervalVector.ts` around lines 161 - 177, The divideVector and moduloVector methods rely indirectly on euclideanDivision to throw on zeros, creating an inconsistent pattern versus divideScalar/moduloScalar; update divideVector and moduloVector to explicitly validate the divisor array returned by extractData(other) for any zero entries (or zero-equivalent values) before calling componentwiseOperation and throw the same error type/message used by divideScalar/moduloScalar when a zero divisor is found; locate checks in divideScalar/moduloScalar and mirror that logic here (use the same error text/throw behavior) so divideVector, moduloVector, euclideanDivision, and componentwiseOperation have consistent zero-divisor handling.
🧹 Nitpick comments (13)
packages/ts-sdk/fixtures/web-smoke/index.html (1)
10-12: Minor: Inconsistent indentation on closing</html>tag.The closing
</html>tag on line 12 has extra leading whitespace compared to the rest of the document structure.✏️ Suggested fix
<script type="module" src="./main.ts"></script> </body> - </html> +</html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/fixtures/web-smoke/index.html` around lines 10 - 12, The closing </html> tag has extra leading whitespace causing inconsistent indentation; adjust the indentation of the closing </html> tag so it aligns with the opening <html> and other top-level tags (remove the extra leading spaces before </html>), ensuring consistent formatting for the file's root tags.packages/ts-sdk/src/slonimsky/index.ts (1)
1-15: Consider usinglet/constinstead ofvar.The module consistently uses
varfor variable declarations. Modern TypeScript/JavaScript best practices favorletfor mutable variables andconstfor immutable ones, as they provide block scoping and help prevent accidental reassignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/slonimsky/index.ts` around lines 1 - 15, The file uses var for local variables; change them to let/const to follow modern TS best practices: in cloneNumberArray ensure any local variables (none currently) would use const where immutable; in intervals replace "var result: number[] = [];" with "const result: number[] = [];" and replace "for (var index = 0; index < sequence.length - 1; index += 1)" with "for (let index = 0; index < sequence.length - 1; index += 1)"; update any other var occurrences in this module (e.g., future locals in cloneNumberArray or other helpers) to let if they are reassigned or const if not.packages/cpp-sdk/src/distances.h (1)
19-31: Parameter should beconstsince it's not modified.The
inparameter is declared as a non-const reference, but the function never modifies it. This should beconst vector<int>& infor const-correctness and to allow passing temporaries.♻️ Suggested fix
-vector<double> normalize(vector<int>& in) { +vector<double> normalize(const vector<int>& in) { double sum = accumulate(in.begin(), in.end(), 0.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cpp-sdk/src/distances.h` around lines 19 - 31, The parameter of normalize should be a const reference because the function does not modify the input; update the signature of normalize from vector<int>& in to const vector<int>& in (preserving the function name normalize) so callers can pass temporaries and to enforce const-correctness, and ensure all uses inside (accumulate, transform) operate on the const input iterators (no other logic changes needed).packages/ts-sdk/fixtures/web-smoke/main.ts (1)
10-10: Handle missing#appcontainer with an explicit error.Line 10 uses a non-null assertion; if the fixture markup changes, this fails with a generic null dereference instead of a smoke-test-specific message.
♻️ Proposed fix
-document.getElementById("app")!.textContent = intervals.data.join(","); +const app = document.getElementById("app"); +if (!app) { + throw new Error('ESM smoke test failed: missing "#app" container'); +} +app.textContent = intervals.data.join(",");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/fixtures/web-smoke/main.ts` at line 10, The current use of document.getElementById("app")! in the fixture can produce an opaque null dereference if the markup changes; update the code around document.getElementById("app") (the expression that assigns textContent using intervals.data.join(",")) to explicitly handle a missing container: first call document.getElementById("app"), check if the result is null, and if so throw or log a clear, smoke-test-specific Error (e.g., "Missing `#app` container in web-smoke fixture") rather than using a non-null assertion, otherwise set textContent from intervals.data.join(",").packages/ts-sdk/src/analysis/shared.ts (1)
3-78: Consolidate duplicated numeric helpers to reduce drift.Several utilities here overlap with core helper primitives. Consider centralizing shared numeric/array helpers in one module and importing them here, to avoid behavior divergence between analysis and core paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/analysis/shared.ts` around lines 3 - 78, This module duplicates many basic numeric/array helpers (cloneNumberArray, sortAscending, arraysEqual, maxNumber, minNumber, truncateTowardZero, cppPrecision, occurrenceCountForDistance, containsNumber); extract these implementations into the central shared helper module used by core (or import the existing primitives if they already exist), replace the local implementations with imports, re-export any public APIs as needed, ensure function signatures and edge-case behavior (empty arrays, NaN/Infinity handling, truncate toward zero semantics) are preserved exactly, and run/update unit tests to confirm there is no behavioral drift.packages/ts-sdk/src/core/positionVector.ts (1)
126-130: Redundant cycle calculation.Line 128 recalculates cycles as
(index - division.remainder) / this._data.length, but this is equivalent todivision.quotientfrom the Euclidean division result. Consider using the quotient directly.♻️ Simplified element access
public element(index: number): number { if (this._data.length === 0) { return 0; } var division = euclideanDivision(index, this._data.length); - var cycles = (index - division.remainder) / this._data.length; - return this._data[division.remainder] + absoluteInteger(this._range) * cycles; + return this._data[division.remainder] + absoluteInteger(this._range) * division.quotient; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/positionVector.ts` around lines 126 - 130, The element access in PositionVector (in the method that computes an element by index) recomputes cycles using (index - division.remainder) / this._data.length even though euclideanDivision(index, this._data.length) already provides the quotient; replace the manual cycles calculation with division.quotient and use that in the returned expression (i.e., return this._data[division.remainder] + absoluteInteger(this._range) * division.quotient) to avoid redundant arithmetic and clarify intent.packages/ts-sdk/src/melody/index.ts (2)
33-40: Magic number-666used as invalid sentinel.The sentinel value
-666is used throughout to indicate invalid note info. Consider extracting this as a named constant for clarity and maintainability.const INVALID_VALUE = -666;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/melody/index.ts` around lines 33 - 40, Extract the magic sentinel -666 into a named constant (e.g., INVALID_VALUE) and replace its occurrences in the invalidNote function and any other usage sites so the function invalidNote(): NoteInfo returns fields set to INVALID_VALUE; update references to the NoteInfo invalid sentinel accordingly to improve clarity and maintainability.
251-268: Add a clarifying comment explaining the algorithm's intentional behavior.The algorithm loops repeatedly prepending
valuestoresult, then takes a suffix of the repeated pattern. ForornamentLoop([1, 2, 3], 5), this produces[2, 3, 1, 2, 3](elements 1–5 of the repeated pattern[1, 2, 3, 1, 2, 3, ...]), which matches the C++ reference implementation and is validated by tests.However, the algorithm is inefficient: repeated
concatoperations are O(n²). Consider optimizing this, such as by calculating indices into the pattern rather than building intermediate arrays. A comment explaining the intended behavior would also clarify why the suffix is taken rather than a prefix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/melody/index.ts` around lines 251 - 268, The ornamentLoop function prepends repeated copies of values then takes a trailing slice, which intentionally returns the first length elements of the infinitely repeated pattern (e.g., ornamentLoop([1,2,3],5) -> [2,3,1,2,3]); add a concise comment above ornamentLoop (and mention cloneNumberArray usage) explaining this intentional suffix-taking behavior and its correspondence to the C++ reference and tests, and then refactor the implementation to avoid O(n^2) concat by directly computing each output element with modular index arithmetic (compute result[i] = values[(i + offset) % values.length] or equivalent) rather than building intermediate arrays.packages/ts-sdk/src/matrix/index.ts (1)
306-341: Runtime type detection is fragile and relies on duck typing.The implementation uses property existence checks (
rows !== undefined,matrix !== undefined,transposition !== undefined,modeIndex !== undefined) to determine the matrix type at runtime. This is fragile because:
- If a new matrix type is added with overlapping properties, dispatch could be incorrect
- The checks assume first element represents all elements (line 327:
rows[0].matrix)Consider using a discriminant property (e.g.,
readonly kind: "modal" | "transposition" | ...) for safer type discrimination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/matrix/index.ts` around lines 306 - 341, The runtime dispatch is fragile because it duck-types on properties like rows, matrix, modeIndex and assumes rows[0] shape; update the matrix type hierarchy to include a readonly discriminant (e.g., kind: "rototranslation" | "modal" | "transposition"), then replace the property-existence checks in the function that inspects the local variable matrix with an explicit switch or if on matrix.kind (referencing RototranslationMatrix, ModalRototranslationMatrixRow, and the generic rows handling) and use the discriminant to safely access rotationMatrix.rows, modalRotations[*].matrix.rows, etc.; also remove the rows[0].matrix assumption by using the discriminant instead of peeking at the first element.packages/ts-sdk/src/naming/chordAnalysis.ts (2)
372-401: Inconsistent spacing in added note suffixes.The spacing logic introduces inconsistencies.
isAddedNoteis used to conditionally add a leading space, but each suffix also appends a trailing space (e.g.,"2 "). This results in double spacing when multiple notes are added and a trailing space before the(omit X)suffix.♻️ Suggested refactor for cleaner spacing
var isAddedNote = false; if (analysis.hasNaturalSecond && name.indexOf("sus2") === -1 && name.indexOf(" 9") === -1) { - name += isAddedNote ? "" : " "; - name += "2 "; + name += isAddedNote ? " 2" : " 2"; isAddedNote = true; } if (analysis.hasFlatSecond) { - name += isAddedNote ? "" : " "; - name += "b2 "; + name += isAddedNote ? " b2" : " b2"; isAddedNote = true; } // ... apply same pattern to other added notes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/naming/chordAnalysis.ts` around lines 372 - 401, The added-note concatenation logic in chordAnalysis.ts (using variables name and isAddedNote and checks like analysis.hasNaturalSecond / hasFlatSecond / hasNaturalFourth / hasSharpFourth / hasNaturalSixth / hasFlatSixth) appends both conditional leading spaces and trailing spaces (e.g., "2 "), causing double spaces and a trailing space before the "(omit X)" suffix; fix by making each note suffix have no trailing space (use "2", "b2", "4", "#4", "6", "b6") and only prepend a single leading space when isAddedNote is false (then set isAddedNote = true), and finally ensure you trim or conditionally add a single space before adding the "(omit ...)" suffix so no extra spaces remain.
69-86: Interval normalization may produce unexpected results for large intervals.The loop at lines 81-83 reduces intervals
>= 24by subtracting 12, but this only runs once per iteration. An interval of 36 would become 24 on first pass and exit the loop (since24 >= 24is false after subtraction). However, thewhilecondition ensures it loops until< 24. The logic is correct, but consider using modulo arithmetic for clarity and efficiency.♻️ Simplified interval normalization
var interval = midiNotes[index] - midiNotes[rootIndex]; - while (interval < 0) { - interval += 12; - } - while (interval >= 24) { - interval -= 12; - } + interval = ((interval % 24) + 24) % 24; + if (interval < 0) { + interval += 24; + }Or more simply, if intervals should be in range [0, 24):
+ interval = ((interval % 24) + 24) % 24;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/naming/chordAnalysis.ts` around lines 69 - 86, The interval normalization in analyzeChord currently adjusts negative intervals with a loop and reduces large intervals with a while-subtraction that is hard to read; replace both loops with a clear modulo operation to normalize interval into the desired range (e.g., interval = ((interval % 12) + 12) % 12 or into [0,24) if intended) when computed inside the analyzeChord function where interval is derived from midiNotes[rootIndex]; update the interval push site (intervals.push(interval)) to use this normalized value and remove the explicit while loops for clarity and efficiency.packages/ts-sdk/src/core/vectors.ts (1)
69-71: Method nameinversionis misleading.
inversiondelegates torototranslatePositions, which performs rotation and translation. In music theory and the context of this SDK (wherePositionVector.inversionandIntervalVector.inversionperform actual pitch-axis inversions), this naming is confusing.If this matches the C++ reference implementation for parity, consider adding a clarifying comment. Otherwise, rename to something like
rotoTranslateortranslateByMode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/core/vectors.ts` around lines 69 - 71, The method Vectors.inversion is misleading because it delegates to rototranslatePositions (rotation+translation) rather than performing a pitch-axis inversion; either rename Vectors.inversion to a clearer name like rotoTranslate or translateByMode (and update all callers, exports, tests and docs to match), or if it must match the C++ reference for parity, replace the method body with a concise clarifying comment referencing rototranslatePositions and the C++ implementation and update the public API docs to explain that Vectors.inversion performs rototranslation rather than musical inversion; ensure references to the symbol inversion and rototranslatePositions are updated consistently.packages/ts-sdk/src/harmony/index.ts (1)
33-56: Option resolvers are verbose but functional.The explicit null/undefined checks work correctly. Consider using nullish coalescing (
??) for cleaner code, though this may be intentional for older JS runtime compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-sdk/src/harmony/index.ts` around lines 33 - 56, The option resolvers resolveScaleOptions and resolveChordOptions use verbose typeof checks for each property; simplify them by replacing the repeated options && typeof ... ternaries with nullish coalescing (??) to provide defaults (e.g., degree ?? 0, invert ?? false, etc.) for each field (degree, inversionAxis, invert, mirror, mirrorAxis, mode, root in resolveScaleOptions and axis, invert, negativeOrMirror, negativeOrMirrorPos, position, preVoices, rotation, shift in resolveChordOptions) so the functions return the same Required<...> objects more concisely while preserving current defaults.
| for (var index = 0; index < targets.length; index += 1) { | ||
| result.push(voiceLeadingAutomation(targets[index], references[index], normalized[index]).vector); |
There was a problem hiding this comment.
Swap the reference and target arguments in this call.
voiceLeadingAutomation() expects (reference, target, complexity). Line 110 currently passes (targets[index], references[index], ...), which inverts the transformation for every pair.
Suggested fix
- result.push(voiceLeadingAutomation(targets[index], references[index], normalized[index]).vector);
+ result.push(voiceLeadingAutomation(references[index], targets[index], normalized[index]).vector);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (var index = 0; index < targets.length; index += 1) { | |
| result.push(voiceLeadingAutomation(targets[index], references[index], normalized[index]).vector); | |
| for (var index = 0; index < targets.length; index += 1) { | |
| result.push(voiceLeadingAutomation(references[index], targets[index], normalized[index]).vector); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ts-sdk/src/automation/index.ts` around lines 109 - 110, The call to
voiceLeadingAutomation in the loop is passing arguments in the wrong order:
voiceLeadingAutomation expects (reference, target, complexity) but the code
calls voiceLeadingAutomation(targets[index], references[index],
normalized[index]). Update that call to pass references[index] as the first
argument and targets[index] as the second (keeping normalized[index] as the
complexity) so the function receives (reference, target, complexity).
| var referenceDivision = euclideanDivision(reference.at(0), reference.range); | ||
| var targetDivision = euclideanDivision(target.at(0), target.range); | ||
| var differenceInOctaves = referenceDivision.quotient - targetDivision.remainder; | ||
| var index = differenceInOctaves * target.size; |
There was a problem hiding this comment.
align is subtracting the wrong division component.
differenceInOctaves should compare quotients. Using targetDivision.remainder mixes octave and pitch-class units, so the starting index is wrong for most non-zero targets.
Suggested fix
var referenceDivision = euclideanDivision(reference.at(0), reference.range);
var targetDivision = euclideanDivision(target.at(0), target.range);
- var differenceInOctaves = referenceDivision.quotient - targetDivision.remainder;
+ var differenceInOctaves = referenceDivision.quotient - targetDivision.quotient;
var index = differenceInOctaves * target.size;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var referenceDivision = euclideanDivision(reference.at(0), reference.range); | |
| var targetDivision = euclideanDivision(target.at(0), target.range); | |
| var differenceInOctaves = referenceDivision.quotient - targetDivision.remainder; | |
| var index = differenceInOctaves * target.size; | |
| var referenceDivision = euclideanDivision(reference.at(0), reference.range); | |
| var targetDivision = euclideanDivision(target.at(0), target.range); | |
| var differenceInOctaves = referenceDivision.quotient - targetDivision.quotient; | |
| var index = differenceInOctaves * target.size; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ts-sdk/src/core/helpers.ts` around lines 6 - 9, The align logic
subtracts the wrong division component: change the computation of
differenceInOctaves to compare quotients (use targetDivision.quotient) rather
than mixing octave and pitch-class units via targetDivision.remainder; update
the line that sets differenceInOctaves to referenceDivision.quotient -
targetDivision.quotient so the subsequent index = differenceInOctaves *
target.size uses a correct octave delta when aligning (keep uses of
euclideanDivision, reference.at(0)/target.at(0) and range/size as-is).
1 participant
Summary
Testing
Not runin this environment.Summary by CodeRabbit
Release Notes
New Features
Documentation