6 Commits

Author	SHA1	Message	Date
Joseph Doherty	64e3fbe035	docs: backfill XML documentation across 756 files ... Adds <summary>, <param>, <typeparam>, and <inheritdoc/> tags to public members surfaced by commentchecker — resolves 5,847 of 5,869 issues (99.6%) across three /fixdocs passes.	2026-05-28 08:10:17 -04:00
Joseph Doherty	23d59d73f2	fix(scripting+alarms): close remaining re-review findings ... Single commit covering the four small/medium fixes from the updated code review. Core.Scripting-014 (Medium, Concurrency): CompiledScriptCache.Clear() used the key-only TryRemove(key, out var lazy) overload — same race shape Core.Scripting-006 closed in GetOrCompile's catch block. A concurrent re-add between snapshot and TryRemove was evicted + disposed while the new caller still held it. Replaced with the value-scoped TryRemove(KeyValuePair<,>) overload. Regression test Clear_uses_value_scoped_TryRemove_so_a_race_inserted_entry_survives added. Core.Scripting-013 (Medium, Security): Hand-rolled BuildWrapperSource pastes user source between literal braces; brace-balanced source could inject sibling methods/classes alongside CompiledScript.Run. Analyzer still walked the injected members so it wasn't a direct escape, but it relaxed the documented 'method body' authoring contract. Added EnforceSingleRunMember: after ParseText, the compilation unit must hold exactly one type (CompiledScript) and that type must hold exactly one member (the Run method). Any deviation throws CompilationErrorException with LMX001/ LMX002 diagnostic IDs and a Core.Scripting-013 reference in the message. Two regression tests added covering the sibling-method and sibling-class injection vectors. Core.Scripting-015 (Low, Correctness, latent): ToCSharpTypeName's generic branch truncated at the first backtick via IndexOf, silently dropping closed args of nested-generic shapes (Outer<T>.Inner<U>). No production caller exercises this shape today (all TContext/TResult are top-level non-nested), so the bug was latent. Rewrote the generic branch to walk the FullName segment-by- segment, consuming generic args per segment so nested shapes emit valid C# (global::Ns.Outer<T>.Inner<U> rather than the broken Outer<T,U>). Core.ScriptedAlarms-013 (Low, Documentation): The internal test accessors TryGetScratchReadCacheForTest / TryGetScratchContextForTest return live mutable scratch refilled in place under _evalGate. XML docs didn't warn future test authors about the synchronization contract. Added a <remarks> block to each documenting the only-safe-on-quiesced-engine + identity-or-single-key contract. Verification (suites green): Core.Scripting.Tests: 110/110 (was 107 — +3 new rejection/race tests) Core.ScriptedAlarms.Tests: 67/67 (unchanged — doc-only fix) Core.VirtualTags.Tests: 57/57 (unchanged) After this commit, all 12 findings from the updated re-review are closed (10 Resolved, 1 Won't Fix none, 1 Deferred — Driver.Galaxy-017). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 18:00:59 -04:00
Joseph Doherty	fb7c6c7046	fix(scripting): route engines through CompiledScriptCache (Core.Scripting-016) ... Both VirtualTagEngine.Load and ScriptedAlarmEngine.LoadAsync were calling ScriptEvaluator.Compile directly, bypassing CompiledScriptCache. The Core.Scripting-008 collectible-ALC fix wired Dispose only through the cache's Clear()/Dispose(), so the per-publish accretion the -008 fix was meant to eliminate was still in effect on the actual production path — the headline 'no more restarts needed' guarantee wasn't delivered. Resolution: - VirtualTagEngine + ScriptedAlarmEngine each gained a private CompiledScriptCache<TContext, TResult> instance. - Both Load methods now call _compileCache.GetOrCompile(source). - Publish-replace path: _compileCache.Clear() runs alongside the existing _tags / _alarms clears so the prior generation's ALCs are disposed before recompile. - Engine Dispose now calls _compileCache.Dispose() so shutdown actually releases the emitted assemblies. Side-fix in CompiledScriptCache: Dispose() set _disposed=true then called Clear(), but Clear() had a pre-existing 'if (_disposed) return' guard that aborted the drain unconditionally — making the Dispose-triggered cleanup a silent no-op. Removed the disposed-guard on Clear() (clearing an empty/ cleared cache is idempotent). Side-fix in ScriptedAlarmEngine.Dispose: cleared _alarms AFTER the Task.WhenAll drain. The drain guarantees no background callback is mid- flight, so clearing is safe. Previously _alarms was deliberately NOT cleared on Dispose (per Core.ScriptedAlarms-005), but that left the AlarmState records holding TimedScriptEvaluator → ScriptEvaluator → delegate references that rooted the emitted assemblies, defeating the cache's Dispose work on the engine side. Regression tests: - VirtualTagEngineTests.Dispose_unloads_compiled_script_assembly - ScriptedAlarmEngineTests.Dispose_unloads_compiled_predicate_assembly Both use WeakReference + bounded GC.Collect() to prove the emitted assembly is reclaimable after engine.Dispose(). The alarms test had to be synchronous (not 'async Task<WeakReference>') because async state machines capture locals as state-struct fields, keeping them alive past the method's apparent end and defeating GC. Verification: - Core.Scripting.Tests: 104/104 (unchanged). - VirtualTags.Tests: 57/57 (was 56 — +1 unload test). - ScriptedAlarms.Tests: 67/67 (was 66 — +1 unload test). - All other consumer suites still green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 17:33:34 -04:00
Joseph Doherty	7b6ab2ec6f	fix(scripting): unload compiled-script assemblies via collectible ALC ... Core.Scripting-008 resolution: replace the legacy CSharpScript.CreateDelegate path with hand-rolled CSharpCompilation + Emit + collectible AssemblyLoadContext, so per-publish compile accretion no longer requires a server restart to reclaim. Why this was needed: Roslyn's CSharpScript path emits dynamically-compiled script assemblies into the default AssemblyLoadContext, which is non-collectible. Across config- publish generations each Clear() drops dictionary entries but the emitted assemblies stay loaded for process lifetime, so memory grows steadily on long-running servers with frequent publishes. The accepted-limitation note in docs/VirtualTags.md recommended scheduled restarts as the workaround; operator feedback was that restarts are difficult, so the underlying limitation was the right thing to fix. Implementation: - New ScriptAssemblyLoadContext(name, isCollectible: true) hosts one emitted script assembly per evaluator. - ScriptEvaluator.Compile synthesises a wrapper class around the user source (CompiledScript.Run(globals) — explicit return required per ordinary C# semantics, which every existing script already uses), builds a CSharpCompilation against the sandbox references, runs the ForbiddenTypeAnalyzer over the semantic model unchanged, emits to an in-memory PE stream, loads via ScriptAssemblyLoadContext.LoadFromStream, and binds a strongly-typed Func<ScriptGlobals<TContext>, TResult> delegate via reflection. - ScriptEvaluator now implements IDisposable — Dispose calls AssemblyLoadContext.Unload(), which makes the emitted assembly eligible for GC at the next collection cycle. - CompiledScriptCache.Clear() disposes every materialised evaluator before dropping its dictionary entry; CompiledScriptCache itself is now IDisposable for graceful server shutdown. - ScriptSandbox.Build returns a new SandboxConfig (References + Imports) instead of a Roslyn ScriptOptions; references now span BCL via the TRUSTED_PLATFORM_ASSEMBLIES set filtered to System.* + netstandard + Microsoft.Win32.Registry, so forbidden BCL types resolve at compile and ForbiddenTypeAnalyzer is the sole security gate (consistent with the Core.Scripting-001 / -002 model — references-list-only restriction is porous against type forwarding, so the analyzer must be the real gate). Verification: - All 104 Core.Scripting tests pass (was 101 — three new regression tests locking the unload contract). - All 56 VirtualTags tests pass (unchanged). - All 63 ScriptedAlarms tests pass (unchanged). - New CompiledScriptCacheTests: - Dispose_unloads_compiled_script_assembly_load_context — proves single- evaluator ALC unload via WeakReference + bounded GC.Collect() loop. - Clear_disposes_every_materialised_evaluator — proves publish-replace releases every prior generation's ALC. - GetOrCompile_after_Dispose_throws_ObjectDisposedException — locks the post-dispose contract. Docs: - docs/VirtualTags.md "Compile cache" section rewritten: the accepted- limitation note replaced with the unload contract + the new authoring convention (explicit return). - docs/ScriptedAlarms.md cross-reference updated to drop the obsolete restart guidance. - code-reviews/Core.Scripting/findings.md Core.Scripting-008 flipped Won't Fix → Resolved with the implementation summary. - code-reviews/README.md regenerated. Pre-existing breakage note: Driver.Galaxy fails the solution-wide build on master because its ProjectReference to the sibling mxaccessgw repo's MxGateway.Client targets a path that the sibling repo no longer has after a recent restructuring. This is unrelated to Core.Scripting-008 and was verified to exist on master before this branch was cut. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 15:55:04 -04:00
Joseph Doherty	0a20de728d	fix(core-scripting): resolve Low code-review findings (Core.Scripting-005,006,008,009,011) ... - Core.Scripting-005: DependencyExtractor.HandleTagCall now recognises raw-string literal paths by checking the StringLiteralExpression node kind instead of the legacy StringLiteralToken kind. - Core.Scripting-006: scope CompiledScriptCache failed-compile eviction with TryRemove(KeyValuePair) so a racing retry entry is not evicted. - Core.Scripting-008: document the per-publish assembly accretion as an accepted limitation in docs/VirtualTags.md. - Core.Scripting-009: enumerate the authoritative deny-list (namespace prefixes + type-granular denies) in the Phase 7 decision-#6 entry to match ForbiddenTypeAnalyzer. - Core.Scripting-011: pin ScriptSandbox.Build, ScriptContext.Deadband boundary semantics, and end-to-end factory + companion-sink integration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-23 07:23:42 -04:00
Joseph Doherty	a25593a9c6	chore: organize solution into module folders (Core/Server/Drivers/Client/Tooling) ... Group all 69 projects into category subfolders under src/ and tests/ so the Rider Solution Explorer mirrors the module structure. Folders: Core, Server, Drivers (with a nested Driver CLIs subfolder), Client, Tooling. - Move every project folder on disk with git mv (history preserved as renames). - Recompute relative paths in 57 .csproj files: cross-category ProjectReferences, the lib/ HintPath+None refs in Driver.Historian.Wonderware, and the external mxaccessgw refs in Driver.Galaxy and its test project. - Rebuild ZB.MOM.WW.OtOpcUa.slnx with nested solution folders. - Re-prefix project paths in functional scripts (e2e, compliance, smoke SQL, integration, install). Build green (0 errors); unit tests pass. Docs left for a separate pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-17 01:55:28 -04:00