Refuse integers that cannot be sent exactly (#963) - #964
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.
9fb0425 to
b3dcf92
Compare
|
@rawsun007 |
|
nothing reached me, and i think i know why: there are no review comments visible on this pr at all. the reviews endpoint returns an empty list, so does the inline comments one, and the only comments on the thread are sonarcloud's from the 9th and yours from today. if you wrote them in a review you never pressed "submit review" on, they stay private to you until you do. your pushes did not break anything. the branch is still my three commits (681fdb7, 98f9a81, b3dcf92) and the diff is the safe-integer guard plus the table-driven tests, so whatever went in on the 9th you did undo cleanly. i also did not mean to fight the draft state. i marked it ready yesterday because the wip check was blocking on "draft mode override" and i read the silence as it being finished; i had not realised you set it to draft deliberately on the 7th. it is back in draft now and i will leave it there. so: please resend or submit the review and i will get on with it. quality gate is green and the duplication issue from the 7th is fixed. Assisted-by: Claude Opus 5 (Claude Code). |
| // The salt command-line keeps the exact value (as an int, or as a string | ||
| // once it is long enough), so refusing is the only honest answer here. | ||
| // Job-ids are matched as strings before this point, see getPatJid(). | ||
| return { error: "Integer argument is too large to be sent exactly" }; |
There was a problem hiding this comment.
Can we suggest a way forward also?
e.g.
Integer argument is too large to be sent exactly. Make it a string by surrounding it with quotes or make it a float by appending .0
|
my bad... I did not click on "submit review" after making notes. |
isSafeInteger refuses everything above 2**53-1, but the value is sent as its own decimal rendering, so what matters is whether that rendering is the text that was typed. 2**53, 2**54 and 10**16 all survive; they were being refused. Comparing the rendering against the input accepts those and still refuses 12345678901234567, which comes back as ...68. The error now names the two ways out, as suggested in review.
|
you were right about isSafeInteger, thanks. pushed ecbc965. what actually matters here is not whether the double is exact but whether the number is sent as the same text that was typed, because it goes out as its own decimal rendering. that is a wider range than isSafeInteger allows: so the check is now on your third note, testing whether the value is still an integer does not separate these: one case worth knowing, since it is not intuitive: 2**60 is exactly representable as a double, but javascript renders it as 1152921504606847000, so it is still refused. that is correct, salt would receive the rendered number rather than the one typed. error message now uses your wording: "Integer argument is too large to be sent exactly. Make it a string by surrounding it with quotes or make it a float by appending .0" your hex example needs #966 first, since patInteger does not match 0x… at all yet; it falls through and is sent as a string today. 300 unit tests pass; reverting just the source leaves 1 failing. eslint clean. Assisted-by: Claude Opus 5 (Claude Code). |
Assisted-by: Claude Opus 5 (Claude Code)
|
|
observation: I thought that e.g. "011" was treated as a octal-integer in SaltGUI. in reality, it is not recognized as an integer (it does not match our pre-check regexp). it is then parsed as a float (Number.parseFloat) which actually returns an integer! The solution in this PR conflicts with the goals that I have since registered in #966. For that issue I now have a vision which handles all the formats described there and warns for it while you type. This means that I will not merge this PR. But your issue was certainly the starting point of all that and will be credited! |
2 participants
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.