Imsp integration#1
Open
Alexander-Sol wants to merge 5 commits into
Open
Conversation
- Fix hardcoded win-x64 RID in build.rs; now derived from CARGO_CFG_TARGET_OS/ARCH - Use platform-correct native library extension (dll/dylib/so) in build.rs and lib.rs - Fix NativeLoader to resolve full path via exe directory instead of bare filename lookup - Update .gitignore to exclude C# obj/bin artifacts and auto-generated src-tauri/natives/ - Untrack previously committed obj/ files - Add CLAUDE.md with architecture overview and development commands Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewer's GuideAdds cross-platform native library loading for IMSP conversion, including a new ImspConverter C# Native AOT library, platform-aware .NET build outputs, Rust/C#/JS tests around native invocation, and documentation for integrating MsBrowser with the TauriCS stack. Sequence diagram for IMSP conversion via call_backendsequenceDiagram
actor JsFrontend
participant TauriRust
participant NativeManager
participant ImspConverterLib
participant MzmlImspExportService
participant FileSystem
JsFrontend->>TauriRust: invoke call_backend(nativeName=imspconverter, jsonData)
TauriRust->>NativeManager: lookup imspconverter
NativeManager-->>TauriRust: LoadedNative (function pointers)
TauriRust->>ImspConverterLib: execute(jsonDataPtr)
ImspConverterLib->>ImspConverterLib: VerifyCurrentProcess(ALLOWED_PROCESSES)
alt security ok
ImspConverterLib->>ImspConverterLib: Deserialize ConvertRequest from JSON
ImspConverterLib->>MzmlImspExportService: ConvertToImspFile(MzmlPath, OutputPath)
MzmlImspExportService->>FileSystem: read mzML, write IMSP
FileSystem-->>MzmlImspExportService: ImspPath
MzmlImspExportService-->>ImspConverterLib: ImspPath
ImspConverterLib->>ImspConverterLib: Serialize ConvertResponse { ImspPath, Error = null }
ImspConverterLib-->>TauriRust: UTF8 JSON string pointer
TauriRust-->>JsFrontend: JSON response { ImspPath, Error }
else security failed or error
ImspConverterLib->>ImspConverterLib: Build ConvertResponse with Error
ImspConverterLib-->>TauriRust: UTF8 JSON string pointer
TauriRust-->>JsFrontend: JSON response { ImspPath = null, Error }
end
Class diagram for ImspConverter Native AOT library and JSON typesclassDiagram
class ConvertRequest {
string MzmlPath
string OutputPath
}
class ConvertResponse {
string ImspPath
string Error
}
class NativeJsonContext {
}
class NativeEntry {
+string[] ALLOWED_PROCESSES
+IntPtr GetNativeName()
+void FreeString(IntPtr ptr)
+IntPtr Execute(IntPtr jsonDataPtr)
+void ExecuteStreaming(IntPtr jsonDataPtr, IntPtr callbackPtr)
+IntPtr ExecuteExternal(IntPtr jsonDataPtr)
-IntPtr Respond(string imspPath, string error)
}
class Security {
+(bool,string) VerifyCurrentProcess(string[] allowedProcesses)
}
class MzmlImspExportService {
+string ConvertToImspFile(string mzmlPath, string outputPath)
}
ConvertRequest <.. NativeJsonContext : serializes
ConvertResponse <.. NativeJsonContext : serializes
NativeEntry ..> NativeJsonContext : uses
NativeEntry ..> ConvertRequest : deserializes
NativeEntry ..> ConvertResponse : serializes
NativeEntry ..> Security : verifies process
NativeEntry ..> MzmlImspExportService : converts mzML
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
ImspConverter.NativeEntry,ExecuteStreamingandExecuteExternalskip theSecurity.VerifyCurrentProcesscheck thatExecuteperforms; for consistent hardening across all entry points, consider applying the same verification before doing any work or returning a response. - The
ImspConverterTesttest class uses a hard-coded, user-specific absolute mzML path (/Users/alex/Downloads/...), which will break on other machines; it would be more robust to derive this path from a configurable test fixture directory or a relative test resource.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ImspConverter.NativeEntry`, `ExecuteStreaming` and `ExecuteExternal` skip the `Security.VerifyCurrentProcess` check that `Execute` performs; for consistent hardening across all entry points, consider applying the same verification before doing any work or returning a response.
- The `ImspConverterTest` test class uses a hard-coded, user-specific absolute mzML path (`/Users/alex/Downloads/...`), which will break on other machines; it would be more robust to derive this path from a configurable test fixture directory or a relative test resource.
## Individual Comments
### Comment 1
<location path="src-csharp/Globals/Globals.cs" line_range="73-82" />
<code_context>
+ string nativeExt = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "dll"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider using AppContext.BaseDirectory instead of Process.MainModule for resolving the executable directory.
`Process.GetCurrentProcess().MainModule` can be null or unsupported in some hosts (e.g., certain containers or single-file/AOT deployments), which makes this pattern brittle and forces the null-forgiving operator. `AppContext.BaseDirectory` is the recommended way to locate the app base path; you can replace this with something like `var exeDir = AppContext.BaseDirectory;` (or `Path.GetDirectoryName(AppContext.BaseDirectory)` if you specifically need a directory path) and keep your existing path logic unchanged.
Suggested implementation:
```csharp
var exeDir = AppContext.BaseDirectory;
```
If there are other occurrences of `Process.GetCurrentProcess().MainModule` in this file (or related files) used for path resolution, they should be migrated in the same way to `AppContext.BaseDirectory`. If any code later assumes `exeDir` has no trailing directory separator, you may want to normalize it with something like:
`var exeDir = AppContext.BaseDirectory.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);`
</issue_to_address>
### Comment 2
<location path="src-csharp/Native/ImspConverter/NativeEntry.cs" line_range="67-71" />
<code_context>
+ }
+ }
+
+ // Streaming: not applicable for this library; no-op to satisfy the interface.
+ [UnmanagedCallersOnly(EntryPoint = "execute_streaming")]
+ public static void ExecuteStreaming(IntPtr jsonDataPtr, IntPtr callbackPtr)
+ {
+ var callback = Marshal.GetDelegateForFunctionPointer<ProgressCallback>(callbackPtr);
+ callback(Marshal.StringToCoTaskMemUTF8("__STREAM_END__"));
+ }
</code_context>
<issue_to_address>
**🚨 issue (security):** Streaming entrypoint skips the security verification applied in `Execute`, which may be an unintended bypass.
`Execute` enforces `Security.VerifyCurrentProcess(ALLOWED_PROCESSES)` but `ExecuteStreaming` does not, while still exposing an unmanaged entrypoint that any caller can invoke to get a `ProgressCallback`. Even though it currently just emits `"__STREAM_END__"`, callers may reasonably expect consistent security across entrypoints. Please either apply the same `VerifyCurrentProcess` check here and return on failure, or clearly document and centralize the security policy so all entrypoints follow it consistently.
</issue_to_address>
### Comment 3
<location path="src-csharp/Tests/ImspConverterTests/ImspConverterTest.cs" line_range="1-9" />
<code_context>
+using System.IO;
+using Readers;
+using Xunit.Abstractions;
+
+namespace ImspConverterTests;
+
+public class ImspConverterTest(ITestOutputHelper output)
+{
+ private const string MzmlPath = "/Users/alex/Downloads/02-18-20_jurkat_td_rep2_fract7.mzML";
+
+ [Fact]
</code_context>
<issue_to_address>
**issue (testing):** Hard‑coded absolute mzML path will make this test non‑portable and likely to fail in CI or on other machines
This test relies on a user-specific absolute path (`/Users/alex/...`), which won’t exist on CI or most machines and will cause consistent failures. Instead, either:
- Add a small mzML test fixture to the repo (e.g., under `testdata/`) and reference it via a path relative to the test project, or
- If it’s only for local experimentation, mark it as skipped (e.g., `[Fact(Skip = "Requires local mzML test file")]`) and document how to enable it locally.
Right now it behaves like an environment-dependent integration test and will make automated runs brittle.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+73
to
+82
| string nativeExt = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "dll" | ||
| : RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? "dylib" | ||
| : "so"; | ||
| libraryName = $"{libraryName}.{nativeExt}"; | ||
|
|
||
| IntPtr libraryHandle; | ||
| // Check the cache first to see if the library is already loaded. | ||
| if (!_loadedLibraries.TryGetValue(libraryName, out libraryHandle)) | ||
| { | ||
| // If not in the cache, load the library using the modern NativeLibrary API. | ||
| // It will automatically search in the application's base directory (where all our DLLs are). | ||
| libraryHandle = NativeLibrary.Load(libraryName); | ||
| // Resolve the full path: all native libraries live in a "natives/" subdirectory |
There was a problem hiding this comment.
suggestion (bug_risk): Consider using AppContext.BaseDirectory instead of Process.MainModule for resolving the executable directory.
Process.GetCurrentProcess().MainModule can be null or unsupported in some hosts (e.g., certain containers or single-file/AOT deployments), which makes this pattern brittle and forces the null-forgiving operator. AppContext.BaseDirectory is the recommended way to locate the app base path; you can replace this with something like var exeDir = AppContext.BaseDirectory; (or Path.GetDirectoryName(AppContext.BaseDirectory) if you specifically need a directory path) and keep your existing path logic unchanged.
Suggested implementation:
var exeDir = AppContext.BaseDirectory;If there are other occurrences of Process.GetCurrentProcess().MainModule in this file (or related files) used for path resolution, they should be migrated in the same way to AppContext.BaseDirectory. If any code later assumes exeDir has no trailing directory separator, you may want to normalize it with something like:
var exeDir = AppContext.BaseDirectory.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
Comment on lines
+67
to
+71
| // Streaming: not applicable for this library; no-op to satisfy the interface. | ||
| [UnmanagedCallersOnly(EntryPoint = "execute_streaming")] | ||
| public static void ExecuteStreaming(IntPtr jsonDataPtr, IntPtr callbackPtr) | ||
| { | ||
| var callback = Marshal.GetDelegateForFunctionPointer<ProgressCallback>(callbackPtr); |
There was a problem hiding this comment.
🚨 issue (security): Streaming entrypoint skips the security verification applied in Execute, which may be an unintended bypass.
Execute enforces Security.VerifyCurrentProcess(ALLOWED_PROCESSES) but ExecuteStreaming does not, while still exposing an unmanaged entrypoint that any caller can invoke to get a ProgressCallback. Even though it currently just emits "__STREAM_END__", callers may reasonably expect consistent security across entrypoints. Please either apply the same VerifyCurrentProcess check here and return on failure, or clearly document and centralize the security policy so all entrypoints follow it consistently.
Comment on lines
+1
to
+9
| using System.IO; | ||
| using Readers; | ||
| using Xunit.Abstractions; | ||
|
|
||
| namespace ImspConverterTests; | ||
|
|
||
| public class ImspConverterTest(ITestOutputHelper output) | ||
| { | ||
| private const string MzmlPath = "/Users/alex/Downloads/02-18-20_jurkat_td_rep2_fract7.mzML"; |
There was a problem hiding this comment.
issue (testing): Hard‑coded absolute mzML path will make this test non‑portable and likely to fail in CI or on other machines
This test relies on a user-specific absolute path (/Users/alex/...), which won’t exist on CI or most machines and will cause consistent failures. Instead, either:
- Add a small mzML test fixture to the repo (e.g., under
testdata/) and reference it via a path relative to the test project, or - If it’s only for local experimentation, mark it as skipped (e.g.,
[Fact(Skip = "Requires local mzML test file")]) and document how to enable it locally.
Right now it behaves like an environment-dependent integration test and will make automated runs brittle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Integrate a new cross-platform IMSP conversion pipeline spanning Rust, C#, and JS, with supporting build, testing, and documentation updates.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: