Add validation framework with fast visual feedback

[erwindon]

• works on the CommandBox
• validations show indications for what they find
• separation between warnings and errors, warnings do not prevent execution of the command
• one such warning is "integer overflow", as suggested by @rawsun007

[rawsun007]

Thanks for the heads-up. BigInt then range check is the right shape - it avoids the thing that made #964 awkward, which is that the expected values in the existing assertions are themselves JS numbers and so round the same way the input does.

On the red quality gate here, since I spent two rounds on the same one: it is entirely the test preamble, not the framework code. SonarCloud's API gives the breakdown without opening the dashboard:

new_duplicated_lines = 332 of 1100 new lines  (30.2%)
  tests/unit/ParseCommandLine.test.js   185
  tests/unit/CommandBox.test.js         111
  saltgui/static/scripts/CommandBox.js   36

The largest cluster is one 14-line shape repeated thirteen times in ParseCommandLine.test.js - args = []; params = {}; tokens = [];, the parseCommandLine(...) call, then the same four assertions. Blocks at 39-52, 166-179, 176-191, 188-201, 208-221, 218-231, 338-350, 347-359, 600-614, 611-623, 629-644, 641-655, 679-691.

What cleared it on #964 was turning those into a table and looping: one for (const [input, expectedArg] of [...]) covering the same inputs took the six cells I had added from 39% to under the threshold, with no loss of coverage. The gate measures new code only, so inheriting the file's existing style is what trips it.

One other thing from that round, in case it bites here too: my second red gate was a reliability rating, not duplication, and it was assert.equal(String(args[0]), nr, nr) - the same variable passed as both the expected value and the failure message, which Sonar reads as a copy-paste slip. curl -s "https://sonarcloud.io/api/issues/search?componentKeys=erwindon_SaltGUI&pullRequest=974&resolved=false" lists those with file and line; it reports 0 for this PR right now, so only the duplication stands between it and a green gate.

Happy to send the table-driven rewrite of ParseCommandLine.test.js against your branch if that saves you a pass - it is mechanical and it is the file I already did this to.

Written with Claude Opus 5 in Claude Code, under my account; the numbers above come from the SonarCloud API for this PR.

[erwindon]

new_duplicated_lines = 332 of 1100 new lines (30.2%)

I noticed that too, but since the duplication is mostly in the unit tests (185+111=296 of 332), I did not care. I've created a new issue (#975) to remind myself to reduce this.

assert.equal(String(args[0]), nr, nr)

where did you find that?

Happy to send the table-driven rewrite of ParseCommandLine.test.js against your branch if that saves you a pass - it is mechanical and it is the file I already did this to.

yes, would be nice to see. can you add it to #975?

[erwindon]

@rawsun007
I have finally completed the code for this.
It took some time to use/test it (and a business trip and home-maintenance took time too).
Do you have time and energy to take a look at it too?

[rawsun007]

sure man, on it

[erwindon]

FYI:
The updated framework has:

• validation of fields
  • command is always required
  • nodegroup names are validated (but compound targeting has NO validations, even when using only N@)
  • strings "", lists [], dictionaries {} are validated for syntax including truncation
  • commands must have at least one item that is not name=value
  • etc
• validation of field combinations
  • target is required, except for runner commands
  • async is not supported for runner/wheel commands
  • etc

[erwindon]

sure man, on it

just curious... please let me know when you expect to be done with that.

[rawsun007]

Reviewed at 4326364. npm run test:unit (329 passing) and npm run eslint are clean. The validation rules (target required except runner, async refused for runner/wheel, wheel taking named parameters only, nodegroup lookup) behave as you described, and I found nothing wrong there. Everything below is in the number parsing in ParseCommandLine.js.

Two bugs:

1. A very large integer throws instead of parsing. 0x followed by 256 fs (or a decimal of 310+ digits) makes Number.parseInt return Infinity, and BigInt(Infinity) in _check64BitRange throws RangeError. Nothing catches it, neither the validation handlers nor the submit path in CommandBox.js. master just produced a number there.
2. 9223372036854775807 (exactly 2^63−1) gets the "exceeds integer range" warning, because the check runs on the value after JS has rounded it up to 2^63. Doing BigInt(pStr) on the original text would fix both.

Also, values between 2^53 and 2^63 still round with no warning (9007199254740993 → …992), which is the #963 case.

The new formats against salt's CLI. I compared against salt's own argument handling: yamlify_arg plus the leading-zero override in SaltYamlSafeLoader, run on PyYAML 6.0.3. It's a copy, not a salt install, so take it as strong evidence rather than proof.

input	master	this PR	salt
010	10	8	10
08:30	string	510	string
0:30	string	30	string
0X1F, 0B101	string	number	string
1:60	string	error	string

010 is a regression: salt strips the leading zeros, so master already agreed with salt. Base-60 only applies in salt when the first part has no leading zero, so times like 08:30 stay strings there. Hex and binary prefixes are lowercase-only in salt. And 1:60 blocks the command where salt just passes the string through.

Written with Claude Opus 5.5 in Claude Code, posted through @rawsun007's account after he read it.

[erwindon]

TODO list, extracted from @rawsun007's review:

• (FIX1) 9223372036854775807 (exactly 2^63−1) gets the "exceeds integer range" warning, because the check runs on the value after JS has rounded it up to 2^63. Doing BigInt(pStr) on the original text would fix both.
  FIXED: we properly raised the warning, but then failed to use the original string
• (FIX2) A very large integer throws instead of parsing. 0x followed by 256 fs (or a decimal of 310+ digits) makes Number.parseInt return Infinity, and BigInt(Infinity) in _check64BitRange throws RangeError. Nothing catches it, neither the validation handlers nor the submit path in CommandBox.js. master just produced a number there.
  FIXED: we now catch this error (and any other error as well)
• Also, values between 2^53 and 2^63 still round with no warning (9007199254740993 → …992), which is the Long integers other than job-ids are still silently rounded (residue of #40) #963 case.
  These numbers are beyond the precision for JS's number type. These number suffer from truncation in last few digits.
  BigInts look like a nice solution for that as these numbers have infinite precision (or at least far beyond 2^64).
  • requests
    ✅ Encoding requests is relatively easy. JSON.stringify has callbacks so that alternative data types can be used. The fact that this must result in strings can easily be corrected with a simple string manipulation trick. Encoding is used for commands and these are typically small, no performance problem is expected.
    decisions:
    • (FIX3) add logic to build a request with perfect representation of the large integers
    • (FIX4) add a validation warning for these numbers that explains that the value will be properly sent, but that similar values in responses are subject to rounding
  • responses
    ❌ Properly decoding responses is very hard. JSON.parse has callbacks to support in this, but up to ES2024 the callback-function receives only the already truncated value. Starting with ES2025, an additional parameter is present that holds the original string. Also, the responses may be big e.g. hundreds of minions each returning the grains.items data. The callback works for each numeric/string element in the json, so that adds up considerably.
    decisions:
    • (no-action) do not handle responses to counteract rounding in large numbers
    • (FIX5) add a single display warning for numbers in this range explaining that they may have been rounded (but we cannot tell).
• input | master | this PR | salt
  • 010 | 10 | 8 | 10 --> octal numbers have a non-overlapping notation and always a warning is shown, I leave this in SaltGUI
  • (FIX6) 08:30 | string | 510 | string --> adjusted logic to warn with leading zeros; also added unit tests
  • (FIX6) 0:30 | string | 30 | string --> adjusted logic to warn with leading zeros; also added unit tests
  • (FIX7) 0X1F, 0B101 | string | number | string --> adjusted logic to warn with X/B
  • (FIX8) 1:60 | string | error | string --> I made this a warning.
  • 010 is a regression: salt strips the leading zeros, so master already agreed with salt. Base-60 only applies in salt when the first part has no leading zero, so times like 08:30 stay strings there. --> see other items
  • (FIX7) Hex and binary prefixes are lowercase-only in salt.
  • (FIX8) And 1:60 blocks the command where salt just passes the string through. --> I made this a warning.

[erwindon]

FIX4:

FIX5:

[sonarqubecloud]

Quality Gate failed

Failed conditions
30.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

[erwindon]

@rawsun007
I've made 8 additional updates to handle the review comments.
2 important points:

• Integers that are part of the request are always exact. Integers in responses may still be precision-truncated, but a warning is added for that as needed.
• The alternative integer notations are improved. We still accept more variations than salt would allow on the command-line. But we now always warn for such cases.

can you see whether I still missed something?

[rawsun007]

Re-checked at 49a9235. npm run test:unit (329 passing) and npm run eslint are clean. FIX1 to FIX8 all behave as you describe. 9223372036854775807 and the 256-digit hex now parse without throwing, and the \x00BIGINT marker produces bare numbers in the request body.

Two things are still left in the PR's own changes:

• 1:059 now becomes 119 with no warning. master kept it as a string, and so does salt: YAML 1.1 only allows one or two digits after each :. The 0-59 check reads the value but not the digit count.
• The X/B warning is skipped when a range warning fires first. 0X20000000000000 only says "exceeds safe-integer range", because that return comes before the cleanStr[1] === "X" check. The same applies to the octal warning.

And one wording point: the octal warning says octal is "usually not supported in salt", but salt does accept 010. It reads it as 10, while SaltGUI sends 8. Saying the value differs would be more accurate than saying it isn't supported.

These differ from salt on master too, so they aren't caused by this PR. They're only worth a look if you want the warnings to cover them:

input	SaltGUI	salt
08, 089	number, no warning	string
1e5, 1.5e5, 1E5	number	string (PyYAML needs a dot and a signed exponent)
1:30.5	string	float 90.5
1_000, 0x_1F	string	int

Same method as last time: salt's yamlify_arg plus the SaltYamlSafeLoader leading-zero override, run on PyYAML 6.0.3. That's a copy of salt's logic, not a real salt install.

Written with Claude Opus 5.5 in Claude Code, posted through @rawsun007's account after he read it.