Repository navigation
fix(osc): refuse non-finite skip/locate arguments at the mapping boundary - #848
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A
/onlycue/skipor/onlycue/locatecarrying a NaN or infinite float argument now yields no command, the same as a string argument already does. Before, the non-finite value flowed through to the dispatcher, where Swift'smaxand .NET'sMath.Maxresolve it differently.Closes #845
Root Cause
OSCParserpasses float arguments through unfiltered — correctly;golden/osc-v1.jsonpins the NaN bit patterns deliberately (#833).OSCCommand.from(_:)then accepted anynumericValue, sosecondscould be NaN or ±∞ by the time it reachedOSCServerHost.resolvedSeekTime:The two platforms'
maxdo not agree, and Swift's is not even symmetric:max(0.0, Double.nan)0.0max(Double.nan, 0.0)nanMath.Max(0.0, double.NaN)NaNMath.Max(double.NaN, 0.0)NaNSwift's
maxisy >= x ? y : x, andNaN >= 0is false — so it silently returns the other argument. The same datagram would rewind the playhead to 0 on macOS and produce a NaN seek destination on a naive C# port.Not a live parity break today:
resolvedSeekTimeis UI-layer and has not been ported (OnlyCue.App.ViewModelsarrives in M1e). This closes the hazard before the port can inherit it.Fix Description
Reject non-finite arguments at the mapping boundary (
OSCCommand/OscCommand), not at the dispatcher.Three reasons for that boundary rather than fixing
max:maxarguments would paper over it — it happens to give the .NET answer on that one line, and the nextmaxsomeone writes reintroduces the divergence.isFinite/double.IsFiniteare the same IEEE-754 predicate (not NaN, not ±∞). Unlikemax, they cannot drift apart between runtimes, so the rule is pinnable by a golden vector rather than being a per-call-site discipline.The check deliberately did not go inside
OSCMessage.numericValue/OscMessage.NumericValue. A float32 NaN argument does have a numeric value — it's NaN — and those are parser-layer accessors. Narrowing their meaning would be a trap for any future command that wants to see the raw value.Int32 arguments widen to a finite double always, so this only ever bites floats.
resolvedSeekTimeis untouched: it can no longer be reached with a non-finiteseconds.Changes
OnlyCue/OSC/OSCCommand.swift— newnumeric(_:_:)helper (mirroring the shape of C#'sNumeric) replaces the two inlinenumericValue.maparms and addsseconds.isFinite.windows/OnlyCue.Core/Osc/OscCommand.cs—Numericgains&& double.IsFinite(seconds).OnlyCueTests/OSCCommandTests.swift— five tests under a new#845section (red commit6c48b2a; four failed before the fix).OnlyCueTests/OSCGoldenFixtures.swift+golden/osc-v1.json— new-∞case; the three existing non-finite cases now recordcommand: nullwhile still pinning the argument bit-for-bit.docs/architecture.md— the OSC section states the rule and why it lives at the mapping boundary.How to Test
xcodebuild test -only-testing:OnlyCueTests/OSCCommandTests -only-testing:OnlyCueTests/OSCGoldenVectorTests -only-testing:OnlyCueTests/OSCParserTests— greencd windows && dotnet test— 554 passedswiftlint --strict— 0 violations, 716 filesgolden/osc-v1.jsonand re-run: it regenerates byte-identical to the committed file (bootstrap exits 65 by design)/onlycue/skip ,f <NaN>at a running app — the monitor still shows the message; the playhead does not moveMutation testing
isFinite!isNaN(lets ±∞ through)double.IsFinite!double.IsNaN5 mutants, 5 killed, 0 survivors. S2/C2 are the ones worth having: they prove the vectors distinguish "not NaN" from "finite", which is the mistake a porter is most likely to make. S3 is the over-rejection guard —
test_skip_withHugeButFiniteArgument_stillMapsexists so that "reject non-finite" cannot quietly become "reject implausible".Regression Risk
Low. A
skip/locatewhose first argument is NaN or ±∞ previously produced a seek (to 0 on macOS, by themaxaccident described above) and now produces nothing. No sender legitimately emits one. Finite arguments, int arguments, all other addresses, parsing, and the monitor tail are unchanged.UI tests do not run on PRs in this repo (gated on push-to-
dev), so this check does not cover XCUITests; OSC command mapping has no UI-test surface.OnlyCue verification (required)
Spec link:
docs/architecture.md#osc-remote-controlCloses: #845 (also updates parent Epic task list)
docs/