Skip to content

feat(driver): validate Loader compatibility with Driver across the boundary#27516

Open
agarwal-navin wants to merge 2 commits into
microsoft:mainfrom
agarwal-navin:driverCompatValidation
Open

feat(driver): validate Loader compatibility with Driver across the boundary#27516
agarwal-navin wants to merge 2 commits into
microsoft:mainfrom
agarwal-navin:driverCompatValidation

Conversation

@agarwal-navin

@agarwal-navin agarwal-navin commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Description

This adds bidirectional layer-compatibility validation across the Driver / Loader boundary.

Previously the Loader only validated that the Driver was compatible with it (validateDriverCompatibility). The reverse — that the Loader is compatible with the Driver — was never checked, because the Driver holds no reference to the Loader and so cannot validate it.

To close that gap without inverting the layer hierarchy, the Driver now publishes the requirements the Loader must meet via a new ILayerCompatSupportRequirements FluidObject exposed on each driver's IDocumentServiceFactory. The Loader reads those requirements and validates itself against them on the Driver's behalf, via the new validateLoaderCompatibilityWithDriver function. Both directions now run from the Container constructor.

Also included:

  • Bug fix: the Driver-boundary check was reusing loaderCompatDetailsForRuntime; it now uses a dedicated loaderCompatDetailsForDriver.
  • A separate IProvideLayerCompatSupportRequirements provider (rather than embedding requirements in ILayerCompatDetails), keeping "what I am" and "what I require of you" distinct.
  • Reverse-direction unit tests in container-loader, and the reverse (driver -> loader) direction added to the end-to-end layerCompat suite (runs per driver type across both create and load flows).

The new ILayerCompatSupportRequirements property on LocalDocumentServiceFactory and OdspDocumentServiceFactoryCore mirrors the existing ILayerCompatDetails property (added in #25120). A changeset is included.

Reviewer Guidance

The review process is outlined on this wiki page.

🤖 Generated with Claude Code

AB#58900

…undary

Add bidirectional layer-compatibility validation across the Driver / Loader
boundary. Previously the Loader only validated that the Driver was compatible
with it. The Driver has no reference to the Loader, so it now publishes the
requirements the Loader must meet via a new ILayerCompatSupportRequirements
FluidObject (exposed on each driver's document service factory), and the Loader
validates itself against them on the Driver's behalf via the new
validateLoaderCompatibilityWithDriver function.

Also fix the Driver-boundary check reusing loaderCompatDetailsForRuntime by
adding a dedicated loaderCompatDetailsForDriver, and add reverse-direction
coverage to the unit tests and the end-to-end layer-compat suite.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Copilot AI review requested due to automatic review settings June 8, 2026 21:46
@agarwal-navin agarwal-navin requested review from a team as code owners June 8, 2026 21:46
@agarwal-navin agarwal-navin requested a review from markfields June 8, 2026 21:46
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

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

Based on the diff (635 lines, 27 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

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.

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ⚠️ Approve with Suggestions

0 Disastrous, 0 Dangerous, 1 Disagreeable

Findings

Sev # Area File What Fix
🐍 Disagreeable M1 Testing packages/loader/container-loader/src/container.ts:752 The PR introduces driverCompatMonitoringContext (created from subLogger) and uses it for both validateDriverCompatibility and the new validateLoaderCompatibilityWithDriver. The allowIncompatibleLayersKey bypass inside validateLayerCompatibility reads mc.config.getBoolean(allowIncompatibleLayersKey). No test verifies that this bypass actually works through driverCompatMonitoringContext — if subLogger lacks config access, both the forward and reverse Driver/Loader compat checks would throw even when the caller sets allowIncompatibleLayersKey=true. The existing e2e bypass tests (allowIncompatibleLayersKey set to true disables validation) only exercise the runtime-dataStore path via this.mc, never the driverCompatMonitoringContext path. Add a test (in loaderLayerCompatValidation.spec.ts or the e2e layerCompat.spec.ts) where: (1) the Driver is configured with minSupportedGeneration = loaderCoreCompatDetails.generation + 1 (forcing reverse-direction incompatibility), (2) a configProvider with allowIncompatibleLayersKey=true is passed to the Loader, and (3) createAndAttachContainer does NOT reject — confirming the bypass is effective through driverCompatMonitoringContext. A parallel test for the forward direction (driver generation < loader's minSupportedGeneration) with the same bypass should also be added.

View workflow run

@markfields

Copy link
Copy Markdown
Member

The Loader reads those requirements and validates itself against them on the Driver's behalf

Can we write out the implications of the fact that for years, old loaders (before this change) will not be validating this compatibility? Related - would be helpful to explicitly state in the PR description and existing layer compat MD files why we want old loaders to give up if the driver is too much newer. You mention that drivers don't hold a reference to the Loader, so explaining that even so, this matters, would be good.

Update the Loader ↔ Driver boundary section to reflect that the boundary is
now validated in both directions: the Driver publishes the requirements the
Loader must meet, and the Loader validates itself against them on the Driver's
behalf. Adds the reverse arrow to the diagram, updates the per-layer
Validates/Validated By summaries, and adds a note about the edge case where a
pre-validation layer paired with a newer layer can still fail in the feature's
own failure mode rather than as a layerIncompatibilityError.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@github-actions

Copy link
Copy Markdown
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output

1: starting server using command "npm run serve -- --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"


> [email protected] serve
> docusaurus serve --no-open

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

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  290922 links
    1934 destination URLs
    2184 URLs ignored
       0 warnings
       0 errors


@@ -117,10 +130,48 @@ export function validateDriverCompatibility(
validateLayerCompatibility(
"loader",
"driver",
loaderCompatDetailsForRuntime,
loaderCompatDetailsForDriver,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any hard thinking we need to do on back compat here? Like, for loaders before this fix, the driver compatibility check is basically going to arbitrarily be right or wrong. We should probably enumerate (for our own sake) the set of loader-driver-runtime version combos that will be false positives or false negatives for this validation.

validateLayerCompatibility(
"driver",
"loader",
maybeDriverCompatDetails ?? defaultLayerCompatDetails,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Side note: This always surprises me seeing "layer 1"'s compat details passed in.

Maybe would be more self-documenting if its signature were something like this:

function validateLayerCompatibility(
	layer1: {
		layer: FluidLayer;
		versionInfo: Pick<ILayerCompatDetails, "pkgVersion" | "generation">;
		compatSupportRequirements: ILayerCompatSupportRequirements;
	},
	layer2: {
		layer: FluidLayer;
		compatDetails: ILayerCompatDetails | undefined;
		strictCompatibilityCheck?: boolean;
	},
	context: {
		disposeFn: (error?: IErrorBase) => void;
		mc: MonitoringContext;
	},
): void

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.

3 participants