Refuse integers that cannot be sent exactly (#963) - #964
Draft
rawsun007 wants to merge 3 commits into
Draft
Conversation
An integer above 2**53-1 was accepted and rounded on the way to salt, so `12345678901234567` arrived as `...68`. The command-line keeps the value the user typed, as you showed on the issue, so the number SaltGUI sends was simply a different one. The integer branch now returns an error, matching the float branch beside it that already refuses a value it cannot represent. Job-ids are unaffected: getPatJid() matches them as strings before this point. The three existing expectations for long integers compared against numeric literals, which the test file rounded identically, so they could not tell a correct value from a wrong one. They now compare against strings and assert the refusal, with the 2**53-1 boundary and a real job-id added either side. ai-assisted-by: Claude Code (Claude Opus 5) Signed-off-by: Roshan Ramani <roshanramani.dev@gmail.com>
SonarQube flagged 39% duplication on new code: the added cases repeated the same four-line setup-and-assert block six times. Two loops over a list of values cover the same inputs in half the lines, and name the value in each assertion message so a failure still says which one broke. No change to what is asserted. ai-assisted-by: Claude Code (Claude Opus 5) Signed-off-by: Roshan Ramani <roshanramani.dev@gmail.com>
SonarQube read `assert.equal(String(args[0]), nr, nr)` as a likely copy-paste slip, which is fair: the same variable served as both the expected value and the failure message. Each loop now builds a label once and uses it for the messages, leaving the value argument on its own.
|
erwindon
marked this pull request as draft
September 7, 2026 10:37
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
2 participants
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.
Fixes #963, taking the "raise an error" direction you asked for rather than the string coercion I had suggested — your command-line output settles it: salt keeps the value the user typed, so anything SaltGUI rounds is simply a different number, and there is nothing sensible to send.
The integer branch now refuses when the value is not exactly representable, matching the float branch immediately below it that already refuses a value it cannot represent:
The boundary is 2**53-1, so everything through
9007199254740991is unaffected. Job-ids are matched as strings before the integer branch, so #41 keeps working.One thing worth flagging about the tests. The three existing expectations for long integers were written as numeric literals:
The test file rounds that literal exactly as the parser did, so the assertion compared one rounded value against an identically rounded one and could never fail. They now compare against strings and assert the refusal, with the 2**53-1 boundary and a real job-id added on either side. That is why the diff touches assertions that look unrelated to the change.
npm run test:unit— 300 passing. Reverting onlyParseCommandLine.jsfails one of the new assertions.npm run eslintandnpm run stylelintare clean.AI disclosure per your policy: the code and this description were written by Claude Code (Claude Opus 5) working through my account, recorded in the commit's
ai-assisted-by:trailer. I have not reviewed the diff line by line myself yet — say the word if you would rather I did that before you spend time on it.