Skip to content

Add fail-closed base resolution and SemVer libraries for the versioning gates - #730

Merged
shschaefer merged 4 commits into
mainfrom
user/gudge/versioning_phase6a_gate_libraries
Aug 7, 2026
Merged

Add fail-closed base resolution and SemVer libraries for the versioning gates#730
shschaefer merged 4 commits into
mainfrom
user/gudge/versioning_phase6a_gate_libraries

Conversation

@MGudgin

@MGudgin Gudge (MGudgin) commented Aug 1, 2026

Copy link
Copy Markdown
Member

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.
Microsoft Reviewers: Open in CodeFlow

Copilot AI balanced review requested due to automatic review settings August 1, 2026 19:07
@MGudgin
Gudge (MGudgin) requested a review from a team as a code owner August 1, 2026 19:07
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 resolveBaseCommit expects one { argv, env } options object. This test currently exercises the no-local-fallback error, whose message also mentions MXC_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

Comment thread scripts/versioning/lib/version.js Outdated
Comment thread scripts/versioning/lib/version.js
Comment thread scripts/versioning/tests/git-base-integration.test.js Outdated
Copilot AI review requested due to automatic review settings August 1, 2026 19:15
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/versioning_phase6a_gate_libraries branch from beab2f5 to 1feaca3 Compare August 1, 2026 19:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

scripts/versioning/lib/version.js:74

  • Numeric prerelease identifiers are not bounded by parseNumericIdentifier, so converting them to Number recreates the fail-open case this library is meant to prevent: two distinct 400-digit identifiers both become Infinity, and Infinity - Infinity leaves compareVersions reporting equality. Compare the canonical digit strings by length and then lexically (or use BigInt) 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, but compareVersions requires patch and prerelease. Passing a major/minor result against a full version on the same version line returns NaN (undefined - patch) instead of throwing, so ordinary < 0/> 0 gates fail open. Add full-version field checks here; callers comparing line-only values already have compareMajorMinor.
function compareVersions(a, b) {
  assertParsed(a, "a");
  assertParsed(b, "b");
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings August 3, 2026 20:09
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/versioning_phase6a_gate_libraries branch from 1feaca3 to 18daed7 Compare August 3, 2026 20:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

scripts/versioning/lib/version.js:113

  • Number.isSafeInteger also 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 though parseVersion rejects 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

Comment thread scripts/versioning/check-tests-present.js Outdated
@MGudgin

Copy link
Copy Markdown
Member Author

Both version.js findings were real fail-opens and are fixed; the test-signature finding was raised against a commit that had already been force-pushed away. Details are in the per-comment replies.

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:

Branch Was Now
versioning_phase6a_gate_libraries (this PR) 1feaca3b 18daed71
versioning_phase7c_detector_lib (#731) d087d3c8 41ddb792
versioning_phase7d_dev_schema_gate (#732) 8977c188 04ca0fea
versioning_phase8a_version_windows 3ff44853 a9dd485e

7d and 8a were additionally stale against #731's own fix rounds from earlier today -- both still carried the original 14fced3e -- so this brings the whole stack back onto one line.

Verification at each rewritten tip:

  • npm test in scripts/versioning: 31 here, 82 on 7c, 90 on 7d, 112 on 8a, all passing.
  • check-schema-versions.js, validate-configs.js, check-rust-toolchain-sync.js and check-version-sync.js all pass on the amended base.
  • The 7d gate (check-dev-schema-compat.js) was run end to end against origin/main and reports no breaking change -- that is the real consumer of both libraries, so it exercises the tightened compareVersions in situ.
  • 8a's src/, sdk/, schemas/ and docs/ trees are byte-identical to their pre-rebase state, so its Rust and SDK code needed no re-verification; the only delta reaching it is this JS library, which its 112-test suite covers.

Worth flagging that compareVersions is now stricter and will throw rather than return NaN when handed a parseMajorMinor result. The current consumers in 8a (check-version-windows.js, lib/version-windows.js) only ever pass version lines to compareMajorMinor, which is unaffected, but any future caller comparing a line against a full version needs compareMajorMinor.

Comment thread scripts/versioning/lib/git-base.js
Comment thread scripts/versioning/lib/version.js
Copilot AI review requested due to automatic review settings August 3, 2026 21:05
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/versioning_phase6a_gate_libraries branch from 18daed7 to 3cb26ce Compare August 3, 2026 21:05
@MGudgin

Copy link
Copy Markdown
Member Author

Both suppressed findings from the latest Copilot review were real and are fixed. Recording them here since they have no reply thread.

version.js:113 -- Number.isSafeInteger admits negatives. Confirmed: compareVersions({major:-1,minor:0,patch:0,prerelease:""}, parseVersion("1.0.0")) returned -2, ordering a value no parser can produce instead of rejecting it.

version.js:128 -- the full-version guard was too weak. This one is my own code from earlier today, added to fix the previous round. Confirmed all four cases:

object before after
patch: -5 compared (-5) throws
prerelease: "01" compared (-1) throws
prerelease: "alpha..1" compared (-1) throws
prerelease: "has space" compared (-1) throws

parseVersion rejects every one of those inputs, so the guard had drifted from the parser it stands in for.

Both are fixed by checking the domain the parsers actually produce: a shared isVersionComponent requires a non-negative safe integer for major, minor and patch, and assertFullVersion validates prerelease against the same PRERELEASE_IDENTIFIER grammar via the existing validDotSeparated, rather than merely checking typeof. Reusing the parser's own predicate is deliberate -- it is what stops the two drifting again. The legitimate shapes still pass: an empty prerelease compares equal, and "alpha.1" still sorts below the release.


Stack. Rebased and force-pushed with lease:

Branch Was Now
versioning_phase6a_gate_libraries (this PR) 18daed71 3cb26ce8
versioning_phase7c_detector_lib (#731) 41ddb792 523b4dba
versioning_phase7d_dev_schema_gate (#732) 04ca0fea 2b42c509

The unopened versioning_phase8a_version_windows branch is deliberately not rebased this round -- it has uncommitted work in its worktree, so I left it alone. It is one commit behind and will need a rebase onto 2b42c509 before it opens.

Verification at each rewritten tip: npm test in scripts/versioning gives 39 here, 90 on 7c and 98 on 7d, all passing; check-schema-versions.js, validate-configs.js, check-rust-toolchain-sync.js and check-version-sync.js pass on this branch; and check-dev-schema-compat.js -- the real consumer of both libraries -- was run end to end on 7d against origin/main and reports no breaking change.

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, repoRoot as the path, prerelease grammar drift between guard and parser, and whether each new test actually fails against the pre-fix code.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-tree treats the brackets as a glob; readFileAtCommit then returns null for 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 push to main, 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's before SHA 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

@microsoft-github-policy-service microsoft-github-policy-service Bot added Needs-Author-Feedback Issue needs attention from issue or PR author Needs-Attention Issue needs attention from Microsoft and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Aug 3, 2026
@MGudgin
Gudge (MGudgin) force-pushed the user/gudge/versioning_phase6a_gate_libraries branch from 3cb26ce to 7852069 Compare August 3, 2026 21:46
@bbonaby
Branden Bonaby (bbonaby) force-pushed the user/gudge/versioning_phase6a_gate_libraries branch from 7852069 to 5262dab Compare August 7, 2026 16:44
@bbonaby
Branden Bonaby (bbonaby) force-pushed the user/gudge/versioning_phase6a_gate_libraries branch from 5262dab to 086b04b Compare August 7, 2026 17:30
Copilot AI review requested due to automatic review settings August 7, 2026 17:30
@bbonaby

Branden Bonaby (bbonaby) commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 and readFileAtCommit reports 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 as null, arrays, or arbitrary objects into plausible repository-relative names. If that coerced name is absent, readFileAtCommit returns null, 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
@bbonaby
Branden Bonaby (bbonaby) force-pushed the user/gudge/versioning_phase6a_gate_libraries branch from 086b04b to f2c34b8 Compare August 7, 2026 18:28
Copilot AI review requested due to automatic review settings August 7, 2026 18:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.json is looked up as schema/v1.json and can return null even 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

Comment on lines +135 to +140
const gitPath = repoRelativeGitPath(repoRoot, path);
const output = git(
repoRoot,
["ls-tree", "-r", "-z", "--name-only", commit, "--", gitPath],
{ trim: false }
);
Comment thread .github/workflows/Versioning.Checks.Job.yml Outdated
Comment thread scripts/versioning/lib/version.js Outdated
Comment thread scripts/versioning/lib/git-base.js Outdated
Comment thread scripts/versioning/lib/git-base.js Outdated
Comment thread scripts/versioning/lib/git-base.js
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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 whose raw value 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, so identifier.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 returns null. 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.json is therefore looked up as schema/v1.json and reported absent, violating the guarantee that null means 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
Copilot AI review requested due to automatic review settings August 7, 2026 21:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 expand tests/*.test.js, and Node 20 does not natively expand the positional glob. The pretest guard 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) so npm test runs 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.js to dotfiles, but this filter counts a hidden file such as tests/.placeholder.test.js. With that as the only test, pretest succeeds 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

@shschaefer
shschaefer merged commit 0be1ccc into main Aug 7, 2026
23 checks passed
@shschaefer
shschaefer deleted the user/gudge/versioning_phase6a_gate_libraries branch August 7, 2026 22:33
@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Needs-Attention Issue needs attention from Microsoft label Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants