Add fail-closed base resolution and SemVer libraries for the versioning gates - #730
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds shared fail-closed Git base resolution and SemVer utilities for versioning gates.
Changes:
- Adds Git history and strict SemVer helpers.
- Adds unit and integration tests.
- Ensures CI runs tests against full Git history.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/Versioning.Checks.Job.yml |
Configures base refs, full history, and tests. |
scripts/versioning/check-tests-present.js |
Fails when no test files exist. |
scripts/versioning/lib/git-base.js |
Adds Git base and historical file helpers. |
scripts/versioning/lib/version.js |
Adds SemVer parsing and comparison. |
scripts/versioning/package.json |
Defines versioning test commands. |
scripts/versioning/tests/git-base-integration.test.js |
Tests Git helpers with scratch repositories. |
scripts/versioning/tests/git-base.test.js |
Tests base-ref selection. |
scripts/versioning/tests/version.test.js |
Tests SemVer behavior. |
Review details
Suppressed comments (2)
scripts/versioning/tests/git-base-integration.test.js:149
- The third argument is ignored because
resolveBaseCommitexpects one{ argv, env }options object. This test currently exercises the no-local-fallback error, whose message also mentionsMXC_VERSIONING_BASE_REF, rather than the GitHub Actions requirement branch.
() => resolveBaseCommit(dir, [], { GITHUB_ACTIONS: "true" }),
scripts/versioning/tests/git-base-integration.test.js:134
- The options are passed with the wrong signature, so the CI environment and unavailable ref are ignored. This still passes because the local fallback fails and the broad
/resolve/alternative matches that unrelated error, leaving the intended fail-closed CI path untested.
resolveBaseCommit(dir, [], {
GITHUB_ACTIONS: "true",
MXC_VERSIONING_BASE_REF: "origin/does-not-exist",
}),
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
beab2f5 to
1feaca3
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/lib/version.js:74
- Numeric prerelease identifiers are not bounded by
parseNumericIdentifier, so converting them toNumberrecreates the fail-open case this library is meant to prevent: two distinct 400-digit identifiers both becomeInfinity, andInfinity - InfinityleavescompareVersionsreporting equality. Compare the canonical digit strings by length and then lexically (or useBigInt) so every accepted prerelease remains orderable.
if (leftNumeric && rightNumeric) {
const difference = Number(left[i]) - Number(right[i]);
if (difference) return difference < 0 ? -1 : 1;
scripts/versioning/lib/version.js:103
- The shared guard accepts
parseMajorMinor()results, butcompareVersionsrequirespatchandprerelease. Passing a major/minor result against a full version on the same version line returnsNaN(undefined - patch) instead of throwing, so ordinary< 0/> 0gates fail open. Add full-version field checks here; callers comparing line-only values already havecompareMajorMinor.
function compareVersions(a, b) {
assertParsed(a, "a");
assertParsed(b, "b");
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
1feaca3 to
18daed7
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/lib/version.js:113
Number.isSafeIntegeralso accepts negative values, although neither parser can produce a negative major or minor. A hand-built or mutated object such as{ major: -1, minor: 0 }is therefore ordered as a valid version instead of failing closed. Require both fields to be nonnegative.
!Number.isSafeInteger(value.major) ||
!Number.isSafeInteger(value.minor)
scripts/versioning/lib/version.js:128
- This full-version guard accepts a negative patch and any prerelease string (for example
"01"or"alpha..1"), even thoughparseVersionrejects those values. Such malformed or mutated objects are then compared rather than rejected, weakening the fail-closed contract. Validate the patch domain and prerelease grammar here as well.
if (!Number.isSafeInteger(value.patch) || typeof value.prerelease !== "string") {
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
|
Both Stack rebase. This PR is the base of #731 (7c) and #732 (7d), plus the unopened 8a branch, so all three were rebased onto the amended base and force-pushed with lease:
7d and 8a were additionally stale against #731's own fix rounds from earlier today -- both still carried the original Verification at each rewritten tip:
Worth flagging that |
18daed7 to
3cb26ce
Compare
|
Both suppressed findings from the latest Copilot review were real and are fixed. Recording them here since they have no reply thread.
Both are fixed by checking the domain the parsers actually produce: a shared Stack. Rebased and force-pushed with lease:
The unopened Verification at each rewritten tip: Given that this is the third consecutive round where findings landed in freshly written fix code, I also ran a dedicated adversarial pass over this diff before pushing, targeting new fail-opens in the fixes themselves. It came back clean, having probed drive-relative and UNC paths, mismatched drive letters, |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/lib/git-base.js:138
- The final argument is interpreted as a Git pathspec, not a literal path. For example, a tracked filename containing bracket syntax can fail to appear because
ls-treetreats the brackets as a glob;readFileAtCommitthen returnsnullfor a file that exists, violating the fail-closed contract. Disable pathspec interpretation for this lookup.
["ls-tree", "-r", "-z", "--name-only", commit, "--", gitPath],
.github/workflows/Versioning.Checks.Job.yml:11
- On a
pushtomain,HEAD^is only the parent of the final commit, not the repository state before the push. A multi-commit push can therefore put a forbidden version change in an earlier commit and this job will compare only the last commit against that already-changed state. Use the push event'sbeforeSHA so the gate covers the complete pushed range.
MXC_VERSIONING_BASE_REF: ${{ github.event_name == 'pull_request' && format('origin/{0}', github.base_ref) || 'HEAD^' }}
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
3cb26ce to
7852069
Compare
7852069 to
5262dab
Compare
5262dab to
086b04b
Compare
|
did another rebase of the stack like above. No direct code changes from me. Some changes on main fixed a pipeline issue, so rebase of the stack on the latest main was necessary for the CI to be green again. |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/lib/git-base.js:138
- The final argument is still interpreted as a Git pathspec. A valid tracked filename beginning with pathspec magic (for example
:(literal)name) is interpreted rather than matched literally, so this function can return an empty list andreadFileAtCommitreports an existing file as absent. Disable pathspec interpretation for this lookup.
["ls-tree", "-r", "-z", "--name-only", commit, "--", gitPath],
scripts/versioning/lib/git-base.js:84
String(path)coerces malformed inputs such asnull, arrays, or arbitrary objects into plausible repository-relative names. If that coerced name is absent,readFileAtCommitreturnsnull, so an invalid lookup is indistinguishable from a genuinely absent file and violates the helper's fail-closed contract. Require a string before normalizing it.
const text = String(path).split("\\").join("/");
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ng gates
This PR adds the two shared libraries every history-aware versioning gate needs
-- resolving what a pull request is being compared against, and ordering schema
versions -- together with a guard that the versioning job cannot report success
without running its tests.
Both libraries fail closed. A gate that cannot determine its base, or cannot
order two versions, reports failure rather than skipping, because a check that
goes quiet when confused is a check that passes on exactly the inputs it exists
to catch.
Details
* `lib/git-base.js` resolves the pull-request base commit and reads files as
they were at that commit. It returns `null` only when a file genuinely does
not exist there, and throws when git itself fails, so "I could not look" is
never mistaken for "it was deleted".
* Paths are normalised to git's forward-slash syntax, and `ls-tree` is read
NUL-delimited. Git C-quotes any path containing non-ASCII bytes, quotes,
backslashes or control characters, and a Windows caller naturally produces
backslashes; either would miss the literal comparison and make an existing
file read as absent.
* The same normalisation collapses `.` and `..` segments and doubled slashes,
and an absolute path is rebased onto the repository root -- `path.join(repoRoot,
...)` is what a caller naturally writes, and it matches no `ls-tree` entry. A
path that cannot name a file in the repository at all is refused outright
rather than reported as absent, so `null` keeps meaning exactly "not present
at that commit" and a gate cannot read a malformed path as "newly added,
nothing to compare".
* `lib/version.js` parses strict SemVer. Numeric identifiers reject leading
zeros, and the core components reject anything not exactly representable,
since an unbounded digit run converts to `Infinity` and `Infinity - Infinity`
is `NaN`, for which every ordinary ordering check is false. Build metadata is
accepted, kept for round-tripping, and excluded from precedence as the
specification requires.
* Numeric *prerelease* identifiers are instead ordered exactly, without ever
converting them to `Number`: SemVer leaves them unbounded, and the grammar has
already rejected leading zeros, so the longer digit string is the larger value
and equal lengths compare lexically. Converting would collapse distinct values
above 2^53 onto one float and long ones onto `Infinity`, whose difference is
`NaN` -- falsy, so the comparison would report equality.
* The parsers take strings only. `RegExp.exec` coerces its argument, so a
single-element array, a boxed `String` or any object with a `toString` would
otherwise parse, and `raw` would carry the non-string.
* The comparison helpers reject anything that is not a parsed version. Reading
`.major` off a raw string or a failed parse yields `undefined`, falls through
every branch, and reports the two versions as equal. `compareVersions` applies
the stricter check, since a `parseMajorMinor` result carries no `patch` or
`prerelease` and would otherwise subtract `undefined` and yield `NaN`;
comparing version lines is what `compareMajorMinor` is for. Both guards check
the domain the parsers actually produce -- non-negative components, and a
prerelease matching the grammar `parseVersion` applies -- since
`Number.isSafeInteger` alone admits negatives and a bare `typeof` check admits
a prerelease the parser would have refused, either of which would be ordered
rather than rejected.
* `check-tests-present.js` runs as `pretest` and fails when the tests directory
is missing or holds no test files. It reads entry types rather than names, so a
*directory* named `foo.test.js` does not satisfy it. `node --test tests/*.test.js` exits 0 when
the pattern matches nothing, so moving or renaming the directory would leave
the job green while executing nothing.
* The versioning workflow checks out full history so a base ref can be
resolved.
Tests
* 39 unit and integration tests pass. The git helpers are exercised against
real scratch repositories, since fail-closed base resolution and path
handling cannot be tested any other way.
* Path coverage includes a backslash-separated lookup and a non-ASCII filename,
each of which read as absent before normalisation and NUL-delimited output.
* A shallow clone with no reachable base is asserted to raise, not to skip.
* SemVer coverage includes leading zeros, unrepresentable components, build
metadata parsing and its exclusion from ordering, malformed prerelease and
build identifiers, prerelease precedence, and the comparison type guards.
* Numeric prerelease ordering is pinned across the 2^53 boundary and at 400
digits, in both directions and for equal values, together with the full
precedence chain from the specification's own example, which fixes
numeric-before-alphanumeric and numeric-not-lexical ordering at once.
* `compareVersions` is asserted to throw for a version line, in either argument
position, and for hand-rolled objects missing `patch` or `prerelease`, while
`compareMajorMinor` still accepts both shapes.
* The presence check now has its own tests, run as real processes against
scratch layouts so the exit code -- the only thing CI reads -- is what is
asserted: a missing directory, an empty one, a directory named `foo.test.js`
with nothing else, and that same directory alongside a real test file.
* Path coverage adds the absolute `path.join(repoRoot, ...)` form, doubled
slashes and a `..` round trip, each of which read as absent before; a file
that is genuinely missing still reads as absent, and a path outside the
repository raises instead.
* The parsers are asserted to reject arrays, boxed strings, `toString` objects
and numbers, and the comparison guards to reject negative components and every
prerelease spelling `parseVersion` refuses, while still accepting the shapes
it produces.
* `check-schema-versions.js` and `validate-configs.js` both still pass, so the
parser change does not disturb existing consumers.
* The resolver takes its options as a single `{ argv, env }` object, and a test
pins that: passing them positionally silently falls back to `process.argv`
and `process.env`, which would make these tests read whatever the ambient job
happens to set. The suite is run both with and without the workflow's own
`MXC_VERSIONING_BASE_REF` and `GITHUB_ACTIONS` values.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cd48fff2-bde9-487a-ab67-012e9bbc0796
Generated-with: claude-opus-5
086b04b to
f2c34b8
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
scripts/versioning/lib/git-base.js:84
- On POSIX, backslash is a legal filename character, not a separator. Unconditionally replacing it means a tracked file such as
schema\\v1.jsonis looked up asschema/v1.jsonand can returnnulleven though the requested file exists, violating this helper's fail-closed contract. Convert backslashes only for Windows-native paths, or make the input path flavor explicit so literal POSIX names remain representable.
function toGitPath(path) {
const text = String(path).split("\\").join("/");
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
| const gitPath = repoRelativeGitPath(repoRoot, path); | ||
| const output = git( | ||
| repoRoot, | ||
| ["ls-tree", "-r", "-z", "--name-only", commit, "--", gitPath], | ||
| { trim: false } | ||
| ); |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 077de171-bf09-4f76-921d-f7a856d1d7c8
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 077de171-bf09-4f76-921d-f7a856d1d7c8
There was a problem hiding this comment.
Review details
Suppressed comments (5)
scripts/versioning/lib/version.js:51
- The parser accepts a trailing line terminator because JavaScript's
$may match before it. For example,parseVersion("1.2.3\n")returns a parsed object whoserawvalue is not valid SemVer, allowing malformed gate input through. Verify that the regex consumed the complete input.
if (!match) return null;
scripts/versioning/lib/version.js:183
- This has the same end-anchor gap as the full-version parser:
parseMajorMinor("1.2\n")is accepted because$can match before the final newline. Require the match to consume the complete string so malformed version lines fail closed.
if (!match) return null;
scripts/versioning/lib/version.js:40
- JavaScript's
$anchor also matches immediately before a final line terminator, soidentifier.test("alpha\n")succeeds. Because the outer prerelease capture can include that newline,parseVersion("1.2.3-alpha\n")is accepted despite the strict SemVer contract. Require each regex match to consume the entire identifier.
This issue also appears in the following locations of the same file:
- line 51
- line 183
return parts.length > 0 && parts.every((part) => identifier.test(part));
scripts/versioning/lib/git-base.js:165
- An empty/root path normalizes to
""; this then lists the whole tree, misses the empty string, and returnsnull. That makes malformed inputs such as"",".","a/..", or the repository root indistinguishable from a genuinely absent file, contrary to this helper's fail-closed contract. Reject an empty repository-relative file path before listing.
const gitPath = repoRelativeGitPath(repoRoot, path);
scripts/versioning/lib/git-base.js:88
- Backslash is a valid literal filename character on POSIX, but this unconditional replacement treats it as a separator on every platform. A tracked file named
schema\\v1.jsonis therefore looked up asschema/v1.jsonand reported absent, violating the guarantee thatnullmeans genuine absence. Preserve backslashes on POSIX (while normalizing them on Windows) and adjust the cross-platform separator tests accordingly.
const text = path.split("\\").join("/");
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 077de171-bf09-4f76-921d-f7a856d1d7c8
There was a problem hiding this comment.
Review details
Suppressed comments (2)
scripts/versioning/package.json:8
- This test command is not cross-platform with the configured Node 20 runtime. On Windows, npm uses
cmd.exe, which does not expandtests/*.test.js, and Node 20 does not natively expand the positional glob. Thepretestguard therefore finds real files and passes, while the runner receives the literal unmatched path and may execute nothing. Use Node's default test discovery (or enumerate paths in JavaScript) sonpm testruns the same suite on Windows as it does in this Ubuntu job.
"test": "node --test tests/*.test.js",
scripts/versioning/check-tests-present.js:32
- This count can still pass when no test is runnable: Bash does not expand
tests/*.test.jsto dotfiles, but this filter counts a hidden file such astests/.placeholder.test.js. With that as the only test,pretestsucceeds and the following test command receives an unmatched pattern, recreating the silent green run this guard is meant to prevent. Exclude dotfiles (and add the corresponding process-level regression case) so this check mirrors the command's match set.
.filter((entry) => entry.isFile() && entry.name.endsWith(".test.js"))
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This PR adds the two shared libraries every history-aware versioning gate needs
-- resolving what a pull request is being compared against, and ordering schema
versions -- together with a guard that the versioning job cannot report success
without running its tests.
Both libraries fail closed. A gate that cannot determine its base, or cannot
order two versions, reports failure rather than skipping, because a check that
goes quiet when confused is a check that passes on exactly the inputs it exists
to catch.
Details
lib/git-base.jsresolves the pull-request base commit and reads files asthey were at that commit. It returns
nullonly when a file genuinely doesnot exist there, and throws when git itself fails, so "I could not look" is
never mistaken for "it was deleted".
ls-treeis readNUL-delimited. Git C-quotes any path containing non-ASCII bytes, quotes,
backslashes or control characters, and a Windows caller naturally produces
backslashes; either would miss the literal comparison and make an existing
file read as absent.
.and..segments and doubled slashes,and an absolute path is rebased onto the repository root --
path.join(repoRoot, ...)is what a caller naturally writes, and it matches nols-treeentry. Apath that cannot name a file in the repository at all is refused outright
rather than reported as absent, so
nullkeeps meaning exactly "not presentat that commit" and a gate cannot read a malformed path as "newly added,
nothing to compare".
lib/version.jsparses strict SemVer. Numeric identifiers reject leadingzeros, and the core components reject anything not exactly representable,
since an unbounded digit run converts to
InfinityandInfinity - Infinityis
NaN, for which every ordinary ordering check is false. Build metadata isaccepted, kept for round-tripping, and excluded from precedence as the
specification requires.
converting them to
Number: SemVer leaves them unbounded, and the grammar hasalready rejected leading zeros, so the longer digit string is the larger value
and equal lengths compare lexically. Converting would collapse distinct values
above 2^53 onto one float and long ones onto
Infinity, whose difference isNaN-- falsy, so the comparison would report equality.RegExp.execcoerces its argument, so asingle-element array, a boxed
Stringor any object with atoStringwouldotherwise parse, and
rawwould carry the non-string..majoroff a raw string or a failed parse yieldsundefined, falls throughevery branch, and reports the two versions as equal.
compareVersionsappliesthe stricter check, since a
parseMajorMinorresult carries nopatchorprereleaseand would otherwise subtractundefinedand yieldNaN;comparing version lines is what
compareMajorMinoris for. Both guards checkthe domain the parsers actually produce -- non-negative components, and a
prerelease matching the grammar
parseVersionapplies -- sinceNumber.isSafeIntegeralone admits negatives and a baretypeofcheck admitsa prerelease the parser would have refused, either of which would be ordered
rather than rejected.
check-tests-present.jsruns aspretestand fails when the tests directoryis missing or holds no test files. It reads entry types rather than names, so a
directory named
foo.test.jsdoes not satisfy it.node --test tests/*.test.jsexits 0 whenthe pattern matches nothing, so moving or renaming the directory would leave
the job green while executing nothing.
resolved.
Tests
real scratch repositories, since fail-closed base resolution and path
handling cannot be tested any other way.
each of which read as absent before normalisation and NUL-delimited output.
metadata parsing and its exclusion from ordering, malformed prerelease and
build identifiers, prerelease precedence, and the comparison type guards.
digits, in both directions and for equal values, together with the full
precedence chain from the specification's own example, which fixes
numeric-before-alphanumeric and numeric-not-lexical ordering at once.
compareVersionsis asserted to throw for a version line, in either argumentposition, and for hand-rolled objects missing
patchorprerelease, whilecompareMajorMinorstill accepts both shapes.scratch layouts so the exit code -- the only thing CI reads -- is what is
asserted: a missing directory, an empty one, a directory named
foo.test.jswith nothing else, and that same directory alongside a real test file.
path.join(repoRoot, ...)form, doubledslashes and a
..round trip, each of which read as absent before; a filethat is genuinely missing still reads as absent, and a path outside the
repository raises instead.
toStringobjectsand numbers, and the comparison guards to reject negative components and every
prerelease spelling
parseVersionrefuses, while still accepting the shapesit produces.
check-schema-versions.jsandvalidate-configs.jsboth still pass, so theparser change does not disturb existing consumers.
{ argv, env }object, and a testpins that: passing them positionally silently falls back to
process.argvand
process.env, which would make these tests read whatever the ambient jobhappens to set. The suite is run both with and without the workflow's own
MXC_VERSIONING_BASE_REFandGITHUB_ACTIONSvalues.Microsoft Reviewers: Open in CodeFlow