DYN-10778: Detect a missing ADP Desktop SDK in the MCP token validation gate - #17310
DYN-10778: Detect a missing ADP Desktop SDK in the MCP token validation gate#17310RobertGlobant20 wants to merge 5 commits into
Conversation
…on gate The DYN-10775 gate asks one question: is AdskIdentitySDK.dll mapped, and does it export idsdk_mcp_validate_token? That catches the DYN-10773 cause, but it has a blind spot it cannot see past. DynamoMCP does not P/Invoke IDSDK directly. It P/Invokes AdpSDKIdentityWrapper.dll, which ships with the ADP Desktop SDK, and the wrapper reaches IDSDK from there. If the ADP Desktop SDK is not installed, that wrapper never resolves and Tier 3 validation is dead -- yet IDSDK itself is perfectly healthy and exports the entry point, so the probe reports Available and the gate opens a panel where every MCP tool call is rejected with HTTP 401. The probe now also checks that AdpSDKIdentityWrapper.dll could be resolved, mirroring the search DynamoMCP's own ResolveWrapperLibrary performs: the default OS search order, then the canonical ADP Desktop SDK install path under Common Files. It looks the file up rather than loading it -- loading would map a module into the process purely to run a diagnostic, and load order is exactly what went wrong in DYN-10773. A probe must not be the thing that changes the answer. Because the two causes need different guidance -- an out-of-date Identity Manager versus a missing ADP install -- the reason is reported alongside the availability and the gate logs a message per cause. Reporting "does not export" for a DLL that is not on disk at all would send the reader after the wrong thing. The AdpWrapperMissing verdict is deliberately not cached. A DLL that is absent can be installed without restarting Dynamo, so only "mapped but no export" and "available" are treated as settled for the session. This is the DynamoMCP-side-free half of DYN-10778 AC 6: it closes the failure mode the AC names without needing to read DynamoMCP's startup self-test, which cannot cross the isolated AssemblyLoadContext that MCPServer.dll is loaded into. See the PR description for the deferred half. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-10778
There was a problem hiding this comment.
Pull request overview
This PR extends Dynamo’s MCP token-validation capability probe (used to decide whether to offer/open Autodesk Assistant and MCP panels) to also detect when the ADP Desktop SDK wrapper (AdpSDKIdentityWrapper.dll) is not resolvable, addressing a case where IDSDK appears healthy but Tier 3 validation is still structurally impossible.
Changes:
- Added an
AdpSDKIdentityWrapper.dllresolvability check and a distinct “unavailable reason” signal (IdsdkExportMissingvsAdpWrapperMissing) inIdsdkMcpTokenValidation. - Updated
DynamoViewgating/logging to emit a dedicated localized message when the ADP wrapper is missing. - Expanded WPF UI tests to cover the new
AdpWrapperMissingverdict and reason-reporting behavior.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DynamoCoreWpfTests/DynamoViewMcpTokenValidationTests.cs | Adds tests for the new “ADP wrapper missing” unavailable-reason and its effects on extension gating. |
| src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs | Logs a different localized message based on the new unavailable-reason when withholding panels. |
| src/DynamoCoreWpf/Utilities/IdsdkMcpTokenValidation.cs | Introduces unavailable reasons and adds ADP wrapper resolvability probing plus caching semantics updates. |
| src/DynamoCoreWpf/PublicAPI.Unshipped.txt | Registers the new public resource getter for the added localized string. |
| src/DynamoCoreWpf/Properties/Resources.resx | Adds the ExtensionNotOfferedAdpWrapperMissing localized string. |
| src/DynamoCoreWpf/Properties/Resources.en-US.resx | Adds the en-US version of the new localized string. |
| src/DynamoCoreWpf/Properties/Resources.Designer.cs | Auto-generated accessor for the new localized string. |
Files not reviewed (1)
- src/DynamoCoreWpf/Properties/Resources.Designer.cs: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
SonarCloud flagged three empty catch(Exception) blocks in
WrapperSearchDirectories (S108 / S2486). The right fix is to delete them
rather than paper over them with a comment, because they were also wrong.
WrapperSearchDirectories is a lazy iterator whose body executes during the
foreach in IsAdpWrapperResolvable, which Probe calls from inside its own
try/catch. So the local catches were redundant -- and worse than redundant.
If Environment.GetEnvironmentVariable("PATH") throws under a restrictive
security policy, swallowing it meant the sweep carried on with a silently
truncated search path; if nothing else matched we then returned false,
reported AdpWrapperMissing, and withheld the Autodesk Assistant and MCP
panels on the strength of a search that never actually ran. Letting it
propagate reaches Probe's catch and reports Unknown, which fails open --
which is what the class documents and what a gate that withholds a feature
should do.
The remaining ArgumentException catch inside the foreach stays: one
malformed PATH entry genuinely should not invalidate the whole sweep, and
that one is scoped to a single entry rather than to the whole search.
No new test: this path only triggers when the OS refuses an environment
lookup, which the test seam cannot provoke. DynamoViewMcpTokenValidationTests
still passes 11/11, build clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the remaining github-code-quality findings on the probe. "Path.Combine may silently drop its earlier arguments" (x2): correct in general -- Path.Combine returns the later segment alone when that segment is rooted. Both call sites pass compile-time constant filenames that are never rooted, so it could not fire today, but Path.Join has no such behaviour and costs nothing. Switching also removes the footgun if anyone later passes a non-constant segment. That switch makes the surrounding try/catch provably dead, so it is gone: Path.Join does not throw where Path.Combine would reject a null segment, and File.Exists is documented to return false rather than throw for a malformed, too-long or unreadable path. Keeping a catch for an exception that cannot occur reads as protection that is not there. This also answers the review suggestion to catch PathTooLongException and NotSupportedException alongside ArgumentException -- on .NET neither is reachable from this code. Also corrected the doc comment on IsAdpWrapperResolvable. It claimed the default OS search order is visited "first, then" the canonical ADP path, which the implementation does not do -- the ADP directory is yielded before the PATH entries. Order carries no meaning here because the method only asks whether the file exists anywhere reachable, not which copy would win, so the comment now says that instead of implying a guarantee the code does not make. Left as-is: "Calls to unmanaged code" on GetModuleHandle. There is no managed API that answers "is this native module already mapped into my process" without taking a full module snapshot, the file already depends on P/Invoke for the sibling export check, and GetModuleHandle is deliberately the same question the ADP wrapper itself asks of IDSDK. Build clean, DynamoViewMcpTokenValidationTests 11/11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses "Missed opportunity to use Where" from the previous push: the foreach implicitly filtered its sequence via an if. Where/Any says the same thing more directly, and both stream, so it still stops at the first hit rather than materialising the whole search path. Build clean, DynamoViewMcpTokenValidationTests 11/11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jasonstratton
left a comment
jasonstratton
left a comment
There was a problem hiding this comment.
One potential issue identified. Please consider it before merging. Otherwise it looks great. Thank you.
|
|
||
| var availability = Probe(); | ||
| if (availability != McpTokenValidationAvailability.Unknown) | ||
| var result = Probe(); |
There was a problem hiding this comment.
Both GetAvailability() and GetUnavailableReason() can each independently call Evaluate() and re-run Probe(). SO potentially each could return mismatched results. ... Maybe this is not the case, but it looks possible and worth validating. Looks like it might be a rare race condition that would need to be protected against.
If so, GetAvailability() says Unavailable (so the extension gets disabled) but GetUnavailableReason()'s fresh probe now returns None or another reason and the wrong message gets logged, or the extension gets disabled on a state that a simultaneous re-check would call Unknown (which should fail open).
There was a problem hiding this comment.
Good catch — this is real, and it's sharper than "rare race". Fixed in 8fdccec.
It isn't only a timing window. AdpWrapperMissing and Unknown are deliberately not cached (a missing DLL can be installed, and an unmapped IDSDK can be mapped, without restarting Dynamo), so those two states re-probe on every Evaluate(). They are exactly the states the gate needs a reason for — so the split read was re-running the full PATH sweep on the second call, and the two probes had a genuine opportunity to disagree. Both failure modes you describe follow: the wrong cause logged, or an extension withheld on a verdict a re-check would have called Unknown and failed open on.
The second one is the one that bothers me, because it inverts the guarantee this gate is supposed to make — and "log confidently names the wrong cause" is the exact failure DYN-10773 cost us a day on.
Fix: GetUnavailableReason() is gone, replaced by GetStatus() returning the pair from a single evaluation. I removed the standalone accessor rather than just fixing the call site — a value that's only meaningful alongside another shouldn't be independently reachable, or the next caller reintroduces the pairing bug. GetAvailability() stays as-is; it's the pre-existing DYN-10775 entry point and is atomic on its own.
While in there, the extension-id check now returns early, so the status is read only for the two extensions the gate can apply to rather than on every call.
Added a test pinning the invariant the fix rests on: a reason implies Unavailable, and every other availability implies None. Fixture is 12/12, build clean.
Thanks for the review.
Review catch from @jasonstratton: the gate called GetAvailability() and GetUnavailableReason() separately, and each ran its own Evaluate(). The two are not interchangeable reads of one value. AdpWrapperMissing and Unknown are deliberately not cached, so a missing DLL that gets installed -- or an IDSDK that gets mapped -- is picked up without restarting Dynamo. That means precisely the states the gate needs a reason for are the ones that re-probe on every call. A split read could therefore pair an Unavailable verdict from the first probe with a reason from a second that no longer agrees: the wrong cause is logged, or worse, an extension is withheld on a verdict a concurrent re-check would have called Unknown and failed open on. The window is small and needs the filesystem or module state to change between two adjacent calls, but the fix is free and the failure mode is exactly the one this ticket exists to stop -- a log line confidently naming the wrong cause. GetUnavailableReason() is replaced by GetStatus(), which returns the pair from a single evaluation. Removing the standalone accessor rather than just avoiding it at the call site: a value that is only meaningful alongside another should not be independently reachable, or the pairing bug can be reintroduced by the next caller. GetAvailability() stays -- it is the pre-existing DYN-10775 entry point and is atomic on its own. The extension-id check in the gate now returns early, so the status is read only for the two extensions it can apply to instead of on every call. New test pins the invariant the fix depends on: a reason implies Unavailable, and every other availability implies None. DynamoViewMcpTokenValidationTests 12/12, build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Purpose
Part of DYN-10778, AC 6.
The DYN-10775 gate asks one question: is
AdskIdentitySDK.dllmapped, and does it exportidsdk_mcp_validate_token? That catches the DYN-10773 cause, but it has a blind spot.DynamoMCP does not P/Invoke IDSDK directly — it P/Invokes
AdpSDKIdentityWrapper.dll, which ships with the ADP Desktop SDK, and the wrapper reaches IDSDK from there. If the ADP Desktop SDK is not installed, that wrapper never resolves and Tier 3 validation is dead — yet IDSDK itself is perfectly healthy and exports the entry point, so the probe reportsAvailableand the gate opens a panel where every MCP tool call is rejected with HTTP 401.This PR adds a wrapper-resolvability check to the probe, mirroring the search DynamoMCP's own
ResolveWrapperLibraryperforms: the default OS search order, then the canonical ADP Desktop SDK install path under Common Files.Three details worth a reviewer's attention:
IdsdkExportMissingvsAdpWrapperMissing) so the log names the right one. "Does not export X" for a DLL that is not on disk at all would send the reader after the wrong thing. New resource stringExtensionNotOfferedAdpWrapperMissing.AdpWrapperMissingis deliberately not cached. A missing DLL can be installed without restarting Dynamo, so only "mapped but no export" and "available" are settled for the session.Behaviour change reviewers should weigh: the gate now blocks in a case where it previously failed open (
Unknown). On a machine with no ADP Desktop SDK, the Autodesk Assistant and MCP panels will stop opening. That is the intended outcome — they cannot work there — but it is a real expansion of the blocking surface, which is why the check only reports missing after every search location misses.Deferred: the other half of AC 6
AC 6 also asks that the gate consume DynamoMCP's startup self-test result. That is deliberately not in this PR.
MCPServer.dllis loaded into an isolated, collectibleAssemblyLoadContextand driven by reflection, so its statics are separate storage that DynamoCore cannot read at any type level — the same mechanism the team hit in DYN-10747. Consuming it would need a primitive-only handoff (a string in anAppDomaindata slot) plus a DynamoMCP version-pin bump.That deferred half would only add coverage for one further case: IDSDK exports the API but still fails to service a call. This PR covers the failure mode AC 6 explicitly names, in one repo, with no version coupling. The remaining case is worth paying for once it is actually observed — the DynamoMCP self-test is what would tell us.
Declarations
Check these if you believe they are true
Release Notes
The Autodesk Assistant and MCP view extension panels are now also withheld when the Autodesk ADP Desktop SDK is not installed. Previously only an out-of-date Autodesk Identity component was detected, so on a machine without the ADP SDK the panels opened into a state where every request failed with HTTP 401.
Reviewers
@jasonstratton (author of DYN-10778 and DYN-10775)
Testing: built with Visual Studio MSBuild (
dotnet buildfails on this repo with MSB4803 on theALtask, unrelated to these changes);DynamoViewMcpTokenValidationTestspasses 11/11 — 7 pre-existing DYN-10775 cases plus 4 new ones covering theAdpWrapperMissingverdict, the reason being reported separately, no reason leaking onAvailable/Unknown, and unrelated extensions staying unaffected.Localization: only
Resources.resxandResources.en-US.resxare hand-edited; the other locale files are populated by the master-localization process.Note on AC 7 of DYN-10778 — whether an unhealthy result should escalate from "panel won't open" to withholding the extensions outright — it has been split out to DYN-10816 and is not addressed here. It is a product decision with its own technical prerequisite: the signal does not exist until after
MCPViewExtension.Loaded()has run, and Autodesk Assistant has no guaranteed load order relative to it. DYN-10816 carries the argument against escalating.FYIs
N/A