Skip to content

(GH-1735) Fix directives version against running dsc version - #1744

Open
Gijs Reijn (Gijsreyn) wants to merge 4 commits into
PowerShell:mainfrom
Gijsreyn:gh-1735/main/fix-directives-version
Open

Gijs Reijn (Gijsreyn) wants to merge 4 commits into
PowerShell:mainfrom
Gijsreyn:gh-1735/main/fix-directives-version

Conversation

@Gijsreyn

Copy link
Copy Markdown
Collaborator

PR Summary

This change:

  • Validates the directives.version requirement of a configuration document against the version of DSC that is processing it, instead of the version of the dsc-lib crate.
  • Adds the Configurator::new_with_dsc_version() constructor, which sets the DSC version on the context before validating the configuration. Configurator::new() keeps its signature and still falls back to the dsc-lib crate version.
  • Updates the dsc config * subcommands and the MCP server invoke_dsc_config tool to use the new constructor.
  • Reports the running DSC version in the versionNotSatisfied error message.
  • Compares a prerelease build of DSC by its release version when the requirement doesn't define a prerelease segment. See the remark on preview builds below.
  • Adds Pester tests that pin the requirement to the running DSC version.
  • Fixes directives.version is checked against dsc-lib's crate version (3.2.0), not the running dsc version #1735

PR Context

Prior to this change, validate_config() parsed env!("CARGO_PKG_VERSION") to get the version to compare against the directives.version requirement. Inside dsc-lib, that macro resolves to the version of the dsc-lib crate, not the version of the dsc CLI. The two versions aren't kept in sync, so on DSC 3.3.0 the requirement =3.3.0 failed while =3.2.0 and <3.3.0 passed.

The CLI did set context.dsc_version to its own version, but only after Configurator::new() returned. Because new() calls validate_config(), the value wasn't available yet when the directive was checked. The result metadata already used context.dsc_version, which is why the output reported the correct version even though the directive was checked against the wrong one.

The existing tests used the requirements =999.0.0 and >=3.1, which give the same result for either version, so they didn't catch the problem.

Remark on preview builds

Using the real DSC version exposes a side effect of semantic version matching: a comparator only matches a prerelease version when the comparator itself defines a prerelease segment. With strict matching, 3.4.0-preview.2 doesn't satisfy >=3.1, so every preview build would reject configuration documents with an ordinary version requirement. This went unnoticed before because the dsc-lib crate version never had a prerelease segment.

To avoid that regression, this change compares a prerelease build of DSC by its release version (3.4.0 for 3.4.0-preview.2) unless a comparator in the requirement explicitly defines a prerelease segment. In that case the full version is compared with the normal semantic version rules.

The following table shows the results for DSC 3.4.0-preview.2:

directives.version Result
>=3.1 Satisfied
=3.4.0 Satisfied
<3.4.0 Not satisfied
=3.4.0-preview.2 Satisfied
=3.4.0-preview.1 Not satisfied
>=3.4.0-preview.1 Satisfied
>=3.4.0-preview.3 Not satisfied
=3.2.0 Not satisfied
<3.3.0 Not satisfied

The tradeoff is that =3.4.0 is satisfied by a 3.4.0 preview build, and <3.4.0 isn't, even though a preview sorts before its release. If strict semantic version matching is preferred, the prerelease handling in validate_config() can be removed, but then the existing >=3.1 test fails on preview builds and the new prerelease test cases need to change.

Copilot AI balanced review requested due to automatic review settings October 8, 2026 03:53

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.

🟢 Approval recommended

The implementation consistently fixes both CLI entry points and provides appropriate regression coverage.

0 open findings

What changed in this PR

Fixes DSC configuration version validation to use the running executable’s version rather than dsc-lib’s crate version.

Changes:

  • Adds a version-aware Configurator constructor.
  • Handles prerelease versions according to the documented matching rules.
  • Updates CLI/MCP callers and adds Pester coverage.
File Description
lib/​dsc-lib/​src/​configure/​mod.rs Validates directives using the supplied DSC version.
dsc/​src/​subcommand.rs Passes the CLI version into configuration processing.
dsc/​src/​server/​invoke_dsc_config.rs Passes the MCP server’s DSC version.
dsc/​tests/​dsc_version.tests.ps1 Tests running-version and prerelease matching.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I added a few notes for maintainability/validation.

I think we should also consider defining a helper function in dsc to retrieve and parse the current version from env!("CARGO_PKG_VERSION") that panics on invalid semver parsing. Something like

/// Returns the current version of DSC as a semantic version.
///
/// # Returns
///
/// - The `SemanticVersion` for DSC if correctly defined in the
///   cargo manifest.
///
/// # Panics
///
/// If the defined version in the cargo manifest isn't a valid
/// semantic version, this function panics and reports the parse
/// error.
pub(crate) fn current_dsc_version() -> SemanticVersion {
    let manifest_version = env!("CARGO_PKG_VERSION");
    match SemanticVersion::parse(manifest_version) {
        Ok(v) => v,
        Err(e) => panic!("unable to parse {manifest_version} as semver: {e:#?}"),
    }
}

If we want to be really clever, we could probably figure out how to generate the current version of DSC as a static or constant in dsc-lib with a build script that pulls the manifest version from dsc. Then we wouldn't need to pass anything from dsc to dsc-lib and we could keep the same public API.

Comment thread lib/dsc-lib/src/configure/mod.rs Outdated
Comment thread lib/dsc-lib/src/configure/mod.rs
Comment thread lib/dsc-lib/src/configure/mod.rs Outdated
@Gijsreyn
Gijs Reijn (Gijsreyn) force-pushed the gh-1735/main/fix-directives-version branch from 3b1e688 to 8982e57 Compare October 10, 2026 02:30
…dation

- Apply standard semantic version rules to prerelease builds
- Store the DSC version as a parsed SemanticVersion on the context
- Take SemanticVersion in Configurator::new_with_dsc_version()
- Add current_dsc_version() helper to dsc
- Document Configurator::new_with_context()
- Cover prerelease matching rules in the Pester tests
@Gijsreyn

Copy link
Copy Markdown
Collaborator Author

Thanks for the review, Mikey. All four points are addressed in the latest push. Per thread:

Prerelease special case in validate_config() — Removed. The check is now just version_req.matches(&self.context.dsc_version), so the standard rules apply: a prerelease build only satisfies a requirement that defines a prerelease segment for the same release, like ^3.4.0-preview. Two test consequences:

  • The existing >=3.1 Pester test would fail on every preview build, so it now derives its requirement from the running version: ^M.m.p-<label> on a prerelease build and >=3.1 on a stable build.
  • The old "compare by release version" test is replaced by five strict cases that are skipped on stable builds. For 3.4.0-preview.2: =3.4.0, >=3.4.0, and <3.4.0 are not satisfied; ^3.4.0-preview and >=3.4.0-preview.2 are satisfied.

SemanticVersion instead of &str — Configurator::new_with_dsc_version() now takes a SemanticVersion by value, and Context.dsc_version is a non-optional SemanticVersion that defaults to the dsc-lib crate version in Context::new(). That keeps the previous fallback behavior and removes the three string fallbacks in result metadata, execution information, and validation. The parse happens once when the context is created, so the ? on a parse failure in validate_config() is gone. One side effect worth a look: Context::new() panics if the dsc-lib manifest version isn't valid semver. Cargo rejects such a manifest at build time so the path is unreachable, but if you'd rather not have a panic path in a library constructor, switching the field to Option<SemanticVersion> is a small change. Execution information now also always reports a version instead of None when the host didn't set one, which matches what the result metadata already did.

Reference docs on new_with_context() — Added, with # Arguments and # Errors sections. I also corrected the argument name in the new_with_dsc_version() docs from config to json.

current_dsc_version() helper — Added to dsc/src/util.rs as pub(crate), following your snippet. The panic message goes through t! with a new util.invalidDscVersion string so the i18n test stays green. Both call sites, the config subcommand and the MCP invoke_dsc_config tool, use it instead of env!("CARGO_PKG_VERSION"). I didn't pursue the build-script idea for reading the dsc version into dsc-lib. Happy to look at that as a follow-up if you'd prefer to keep the public API unchanged.

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.

directives.version is checked against dsc-lib's crate version (3.2.0), not the running dsc version

3 participants