Skip to content

Tweak support for 3.x in OldestSupportedClientVersion - #28022

Open
Craig Macomber (Microsoft) (CraigMacomber) wants to merge 9 commits into
microsoft:mainfrom
CraigMacomber:OldestSupportedClientVersion3
Open

Tweak support for 3.x in OldestSupportedClientVersion#28022
Craig Macomber (Microsoft) (CraigMacomber) wants to merge 9 commits into
microsoft:mainfrom
CraigMacomber:OldestSupportedClientVersion3

Conversation

@CraigMacomber

Copy link
Copy Markdown
Contributor

Description

Tweak how we support version 3, removing the complications that we likely shouldn't have stabilized for 1 and 2 from the new 3 support before we stabilize.

Reviewer Guidance

The review process is outlined in the pull request guidelines.

@github-actions github-actions Bot added area: tools area: runtime Runtime related issues area: repo Repo related work area: website public api change Changes to a public API base: main PRs targeted against main branch labels Aug 20, 2026
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (102 lines, 25 files), I've queued these reviewers:

  • Correctness — logic errors, race conditions, lifecycle issues
  • Security — vulnerabilities, secret exposure, injection
  • API Compatibility — breaking changes, release tags, type design
  • Performance — algorithmic regressions, memory leaks
  • Testing — coverage gaps, hollow tests

How this works

  • Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

  • Tick Start review below to dispatch the review fleet.

  • After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

  • This comment updates as new commits land; your reviewer selections are preserved.

  • Start review

@github-actions github-actions Bot added area: dds Issues related to distributed data structures dependencies Pull requests that update a dependency file area: tests Tests to add, test infrastructure improvements, etc area: dds: tree labels Aug 20, 2026

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

This PR refines how Fluid Framework v3 is represented in OldestSupportedClientVersion, simplifying v3 support by restricting it to major+minor (with patch 0) while keeping legacy flexibility for v1/v2, and updates tests and end-to-end test packages accordingly.

Changes:

  • Tightens OldestSupportedClientVersion so v3 is expressed as 3.<minor>.0 (minor-only), while retaining v1/v2 patch + prerelease forms.
  • Updates multiple tests to use cleanedPackageVersion / featureVersion(pkgVersion) when supplying “current” version values.
  • Adds @fluidframework/driver-definitions dependency to service-client end-to-end test packages (and updates lockfile) to use featureVersion.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pnpm-lock.yaml Updates workspace links to reflect added dependency usage.
packages/test/test-end-to-end-tests/src/test/containerRuntime.spec.ts Uses cleanedPackageVersion for min-version telemetry assertions.
packages/service-clients/end-to-end-tests/odsp-client/src/test/containerCreate.spec.ts Uses featureVersion(pkgVersion) for service-client version formatting.
packages/service-clients/end-to-end-tests/odsp-client/src/test/audience.spec.ts Uses featureVersion(pkgVersion) for service-client version formatting.
packages/service-clients/end-to-end-tests/odsp-client/package.json Adds @fluidframework/driver-definitions dependency.
packages/service-clients/end-to-end-tests/azure-client/src/test/utils.ts Uses featureVersion(pkgVersion) and documents internal import.
packages/service-clients/end-to-end-tests/azure-client/package.json Adds @fluidframework/driver-definitions dependency.
packages/runtime/runtime-utils/src/test/compatibilityBase.spec.ts Aligns tests to validate against cleanedPackageVersion.
packages/runtime/runtime-definitions/src/compatibilityDefinitions.ts Adjusts v3 version shape in OldestSupportedClientVersion and updates docs.
packages/runtime/runtime-definitions/api-report/runtime-definitions.public.api.md Regenerated API report reflecting updated type.
packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.public.api.md Regenerated API report reflecting updated type.
packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.beta.api.md Regenerated API report reflecting updated type.
packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.alpha.api.md Regenerated API report reflecting updated type.
packages/runtime/runtime-definitions/api-report/runtime-definitions.beta.api.md Regenerated API report reflecting updated type.
packages/runtime/container-runtime/src/test/containerRuntime.spec.ts Updates test naming/behavior around minVersionForCollab.
packages/dds/tree/src/test/shared-tree/fuzz/baseModel.ts Uses cleanedPackageVersion for fuzz factory min-version configuration.
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/runtime/runtime-definitions/src/compatibilityDefinitions.ts Outdated
export type OldestSupportedClientVersion =
| `${1 | 2 | 3}.${bigint}.${bigint}`
| `${1 | 2 | 3}.${bigint}.${bigint}-${string}`;
| `3.${bigint}.0`

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.

  1. Let's not force .0. If we want to ignore patch and use 3.${bigint}, then I am okay with that.
  2. I would like to be convinced there is not case where it would be useful to limit the oldest to some patch. Perhaps it isn't meaningful to FF SDK directly but say there is some issue fixed in a patch and it could be useful on customer side to say they don't want to let anyone open document if they don't have the patch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a package version, so it needs to have all three digits: any change to that would be a major change, and I think makes it out of scope as something we can do now.

Specifying patches doesn't work as one would expect, since if you say 3.1.8, you are saying there might be clients whos version is as old as 3.1.8. Our versioning age logic however considers 3.2.0 newer than that, despite it possibility being older and not having the patch.

Remember that this controls opting into FF features. If you specify 3.1.8 you are saying it's ok to opt into features which are only supported by that patch release but not the prior patch. But you are also implicitly supporting 3.2.0 (because of how we sort versions) which might not have the patch.

We don't add features in patch releases, and if a given feature is bugged in some minor, not having OldestSupportedClientVersion opt into it until the next minor seems safe generally and reducing confusion and risk of hitting bugs.

Also note that letting people specify patch version in the future if we have some case where for some reason its desired would be a non-breaking change. At that point we would want to clarify the semantics around it.

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.

I am not convinced that it is a package version. In semver syntax I think it is the expression that completes ">=ver" spec.
I would not be surprised if using 3.{bigint} just worked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think our docs phrase it as a package version at least as low as any version of any (non-loader) fluid framework client package that may be used on this document, though you are right we could rephrase that into something that compares less than or equal to any such package versions.

I suspect with some changes to the implementation, validation and normalization we could let 3.{bigint} work, but currently I don't think it does. Allowing that in the future would be a non breaking change though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have updated the privateRemarks calling out the option of, as a non breaking change, allowing dropping the .0, but also calling out a potential risk of that.

@WillieHabi WillieHabi Aug 26, 2026

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.

Assuming we keep the 3.minor.0 contract, I think the runtime validator needs to enforce the same rule. validateMinimumVersionForCollab currently checks only valid semver, the lower floor, and <= cleanedPackageVersion .

For example, once 3.2.0 ships, validateMinimumVersionForCollab("3.1.8") would still succeed even though this type rejects it.

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.

I played around last night trying out dropping patch. It looked good for a while but there is too much code that relies on comparing exact versions and major.minor is not a valid semantic version (is a range, but does not imply .0).

This is a big ugly, reveals complications trying to do it, and points out some changes we should have in this PR. The diff is here: https://github.com/CraigMacomber/FluidFramework/compare/OldestSupportedClientVersion3...microsoft:FluidFramework:OldestSupportedClientVersion3_without_patch?expand=1

I will try to comment on individual places to make feedback more digestible.

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.

I had trouble commenting elsewhere since related files aren't touched. One of the comments is what I see Willie noting. I have a version of validation at

const parsedOldestVersion = parse(minVersionForCollab);
const isValidSemver = parsedOldestVersion !== null && parsedOldestVersion.build.length === 0;
const isGteLowestMinVersion =
isValidSemver && gte(minVersionForCollab, lowestMinVersionForCollab);
const isLtePkgVersion = isValidSemver && lte(minVersionForCollab, highestBaseVersion);
// Starting with 3.x versions, the patch may only be 0 and no prerelease is allowed.
const isValidOldestSupportedClientVersion =
isGteLowestMinVersion &&
isLtePkgVersion &&
(parsedOldestVersion.major < 3 ||
(parsedOldestVersion.patch === 0 && parsedOldestVersion.prerelease.length === 0));
return {
isValidSemver,
isGteLowestMinVersion,
isLtePkgVersion,
isValidOldestSupportedClientVersion,
};
and includes a pre-existing hole for use of build in version (runtime defense in depth). There are some related changes to see as well.

Comment thread packages/service-clients/end-to-end-tests/azure-client/src/test/utils.ts Outdated
workloadName: "SharedTree (Reference Forest)",
factory: new SharedTreeFuzzTestFactory(createOnCreate(undefined), undefined, {
minVersionForCollab: pkgVersion,
minVersionForCollab: cleanedPackageVersion,

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.

It doesn't seem great to get version from another package since it could be higher than this package's version. But since this is a test that probably works out.
Since cleanedPackageVersion is a bit dangerous, it would best to have a /test export/import path.
I see there is one production use in tree/codec with a good-sized disclaimer. Seems like that case is a candidate to use base version (major.minor.0 or major.minor).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I feel like pkgVersion is the dangerous thing.

Not using the cleaned version can cause a feature that you think you are testing (enabled in the current version) to not be tested on CI, since CI changes the pkgVersion to be lower semver wise (by adding a prerelase tag) before running the test, and thus skips testing the new feature which this code claimed to be testing and will be enabled in the next releases.

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.

I'm not saying pkgVersion isn't dangerous. Just that ~ dep on runtime-utils technically means it could be more advanced that current package's version.
Building cleaned version from pkgVersion seems like the way to go. We could even generate the clean version into packageVersion.ts.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have made this use the existing package local currentVersion (which already documents this issue) so this PR doesn't regress this.

I like the idea of including the cleaned/feature version in generated packageVersion.ts as a longer term solution though.

@WillieHabi WillieHabi Aug 26, 2026

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.

Might be misunderstanding, but with this PR’s new type, a 3.1.2 build would produce currentVersion === "3.1.2", even though OldestSupportedClientVersion now accepts only 3.minor.0 ?

@github-actions github-actions Bot added the area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct label Aug 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔗 Found some broken links! 💔

Run a link check locally to find them. See Checking for Broken Links for more information.

linkcheck output

$ start-server-and-test "npm run serve -- --host 127.0.0.1 --no-open" http://127.0.0.1:3000 check-links
1: starting server using command "npm run serve -- --host 127.0.0.1 --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-website@0.0.0 serve
> docusaurus serve --host 127.0.0.1 --no-open

[SUCCESS] Serving "build" directory at: http://127.0.0.1:3000/

> fluid-framework-website@0.0.0 check-links
> linkcheck http://127.0.0.1:3000 --skip-file skipped-urls.txt

Crawling...

http://127.0.0.1:3000/docs/data-structures/tree/schema-evolution/feature-flag-schema-upgrades
- (72:12) 'isStaged..' => http://127.0.0.1:3000/docs/api/fluid-framework/treeviewalpha-interface#isstagedupgradeenabled-methodsignature (HTTP 200 but missing anchor)


Stats:
  338443 links
    2041 destination URLs
    2297 URLs ignored
       1 warnings
       0 errors

Error: Command failed with exit code 1: npm run check-links
    at makeError (/home/runner/work/FluidFramework/FluidFramework/website/node_modules/.pnpm/execa@5.1.1/node_modules/execa/lib/error.js:60:11)
    at handlePromise (/home/runner/work/FluidFramework/FluidFramework/website/node_modules/.pnpm/execa@5.1.1/node_modules/execa/index.js:118:26)
    at process.processTicksAndRejections (node:internal/process/task_queues:103:5) {
  shortMessage: 'Command failed with exit code 1: npm run check-links',
  command: 'npm run check-links',
  escapedCommand: '"npm run check-links"',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,
  stdout: undefined,
  stderr: undefined,
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}
[ELIFECYCLE] Command failed with exit code 1.

@github-actions

Copy link
Copy Markdown
Contributor

Bundle size comparison

Base commit: e4c4feace03bbb55fba6cb125a8b9ce5b9eca400
Head commit: 70cf3b9fd6af83e7e278cc916dde86ff4bab7a5f

Notable changes

No bundles changed by ≥ 500 bytes parsed.

Per-bundle deltas

@fluid-example/bundle-size-tests

  • fluidFrameworkAllAlpha.js: parsed 790593 → 790735 (+142), gzip 217166 → 217269 (+103)
  • azureClient.js: parsed 633339 → 633334 (-5), gzip 169746 → 169819 (+73)
  • odspClient.js: parsed 604608 → 604717 (+109), gzip 162547 → 162684 (+137)
  • aqueduct.js: parsed 537215 → 537228 (+13), gzip 144316 → 144361 (+45)
  • fluidFramework.js: parsed 409151 → 409184 (+33), gzip 115982 → 116007 (+25)
  • sharedTree.js: parsed 398530 → 398556 (+26), gzip 113400 → 113420 (+20)
  • containerRuntime.js: parsed 314014 → 313992 (-22), gzip 86160 → 86158 (-2)
  • sharedString.js: parsed 175205 → 175212 (+7), gzip 49660 → 49666 (+6)
  • experimentalSharedTree.js: parsed 161812 → 161812 (0), gzip 46711 → 46711 (0)
  • matrix.js: parsed 159584 → 159591 (+7), gzip 45903 → 45910 (+7)
  • loader.js: parsed 147289 → 147305 (+16), gzip 40028 → 40037 (+9)
  • odspDriver.js: parsed 105655 → 105713 (+58), gzip 32926 → 32991 (+65)
  • directory.js: parsed 65635 → 65642 (+7), gzip 18481 → 18489 (+8)
  • 578.js: parsed 58686 → 58686 (0), gzip 17657 → 17657 (0)
  • odspPrefetchSnapshot.js: parsed 45884 → 45865 (-19), gzip 15335 → 15351 (+16)
  • map.js: parsed 45786 → 45793 (+7), gzip 14109 → 14116 (+7)
  • 252.js: parsed 44362 → 44362 (0), gzip 13735 → 13735 (0)
  • summarizerDelayLoadedModule.js: parsed 31287 → 31287 (0), gzip 7929 → 7929 (0)
  • socketModule.js: parsed 26992 → 26962 (-30), gzip 8019 → 8052 (+33)
  • createNewModule.js: parsed 12464 → 12464 (0), gzip 4792 → 4805 (+13)
  • summaryModule.js: parsed 3888 → 3888 (0), gzip 1874 → 1874 (0)
  • connectionState.js: parsed 909 → 909 (0), gzip 500 → 500 (0)
  • sharedTreeAttributes.js: parsed 845 → 852 (+7), gzip 493 → 503 (+10)
  • debugAssert.js: parsed 429 → 429 (0), gzip 299 → 299 (0)
  • FluidFramework-HashFallback.js: parsed 419 → 419 (0), gzip 313 → 313 (0)

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.

API changes look good, and seem aligned with the outcomes of the PR discussions.

But I did not review the code changes in detail. It would be good to get Jason and/or Willie to sign off as well, since they've been doing work in this space.

Docs question: are there any existing changesets around oldestSupportedClient that should be updated to reflect the new requirements?

@CraigMacomber

Copy link
Copy Markdown
Contributor Author

API changes look good, and seem aligned with the outcomes of the PR discussions.

But I did not review the code changes in detail. It would be good to get Jason and/or Willie to sign off as well, since they've been doing work in this space.

Docs question: are there any existing changesets around oldestSupportedClient that should be updated to reflect the new requirements?

This is adjusting the work from #28003 which had not changeset, so I don't think there is one yet.

I think it might make sense to leave that for #27972 which also adjusts this, and is the actual breaking change for 3.0 here

@jason-ha Jason Hartman (jason-ha) 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.

I'm now good with .0 but we need at least some validation changes.

"@types/sinon": "^17.0.3",
"c8": "^10.1.3",
"eslint": "catalog:eslint",
"fluid-framework": "workspace:~",

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.

How confident are you that a dev dep is okay for this test package?
I have a niggling suspicion that this needs to be a prod dep.

export type OldestSupportedClientVersion =
| `${1 | 2 | 3}.${bigint}.${bigint}`
| `${1 | 2 | 3}.${bigint}.${bigint}-${string}`;
| `3.${bigint}.0`

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.

I played around last night trying out dropping patch. It looked good for a while but there is too much code that relies on comparing exact versions and major.minor is not a valid semantic version (is a range, but does not imply .0).

This is a big ugly, reveals complications trying to do it, and points out some changes we should have in this PR. The diff is here: https://github.com/CraigMacomber/FluidFramework/compare/OldestSupportedClientVersion3...microsoft:FluidFramework:OldestSupportedClientVersion3_without_patch?expand=1

I will try to comment on individual places to make feedback more digestible.

} from "@fluidframework/driver-definitions/internal";
export {
createBasicRegistryKey,
featureVersion,

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.

Yes, it is alpha, but exposing this seems like it will make it just a bit harder to lop off the .0 in the future if we want to.
Also am not a fan of the name FWIW. Is there no technical term for this? I've used "base" version for this in some cases. Just to spit ball two names "patchZeroVersion" or "baseMinorVersion" (tried out in my diff)? Since dropping patch didn't work I am less opposed to featureVersion as that becomes intended use - just the usually naming difficulties.

Its comments need updating. There should be clarity about patch being .0. You can reference comments I was making for "baseMinorVersion" at

* Strips patch and prerelease from a SemVer string, returning only the major and minor version with a .0 patch.

I think runtime-utils cleanedPackageVersion should use this instead of parse and then comments would need updated see

* Use of this function with `pkgVersion` is same as `cleanedPackageVersion`

Comment on lines +42 to +45
* "a value that compares less than or equal to the oldest Fluid Framework client version that must be able to open and process the container".
* As a non-breaking change, we could then allow values such as "3.1" without requiring the trailing ".0".
* However, omitting ".0" might make the value look less like a version and obscure semver ordering; for example, how "3.21" is greater than "3.3".
* We may therefore want to retain the ".0" for clarity.

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.

I think we can drop this or update to note that it is complicated and probably not worth it.

export type OldestSupportedClientVersion =
| `${1 | 2 | 3}.${bigint}.${bigint}`
| `${1 | 2 | 3}.${bigint}.${bigint}-${string}`;
| `3.${bigint}.0`

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.

I had trouble commenting elsewhere since related files aren't touched. One of the comments is what I see Willie noting. I have a version of validation at

const parsedOldestVersion = parse(minVersionForCollab);
const isValidSemver = parsedOldestVersion !== null && parsedOldestVersion.build.length === 0;
const isGteLowestMinVersion =
isValidSemver && gte(minVersionForCollab, lowestMinVersionForCollab);
const isLtePkgVersion = isValidSemver && lte(minVersionForCollab, highestBaseVersion);
// Starting with 3.x versions, the patch may only be 0 and no prerelease is allowed.
const isValidOldestSupportedClientVersion =
isGteLowestMinVersion &&
isLtePkgVersion &&
(parsedOldestVersion.major < 3 ||
(parsedOldestVersion.patch === 0 && parsedOldestVersion.prerelease.length === 0));
return {
isValidSemver,
isGteLowestMinVersion,
isLtePkgVersion,
isValidOldestSupportedClientVersion,
};
and includes a pre-existing hole for use of build in version (runtime defense in depth). There are some related changes to see as well.

@jason-ha
Jason Hartman (jason-ha) dismissed their stale review August 26, 2026 21:45

comments to address have been acknowledged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: dds: tree area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: repo Repo related work area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc area: tools area: website base: main PRs targeted against main branch dependencies Pull requests that update a dependency file public api change Changes to a public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants