Skip to content

Conversation

@Bayselonarrend
Copy link
Contributor

@Bayselonarrend Bayselonarrend commented Nov 20, 2025

Хочу попробовать сделать свою версию #1554 с прекомпиляцией скриптов. Только не автоматическую, а ручную
WIP, пока в исследовательских целях)

Summary by CodeRabbit

  • New Features
    • Added script precompilation via a new command-line option to produce .osc precompiled scripts.
    • Added execution support for precompiled .osc scripts for faster load/startup.
    • Added ability to persist and restore compiled modules to disk, improving repeatable deployments and tooling.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Adds MessagePack dependency and module persistence for precompiled runtime modules, a ModuleSerializer for save/load, a CLI "-build" behavior to emit .osc files, a build behavior that compiles and saves modules, an engine overload to create processes from IExecutableModule, and ExecuteScriptBehavior branching to load .osc or source.

Changes

Cohort / File(s) Summary
Dependency Configuration
src/OneScript.Core/OneScript.Core.csproj
Added MessagePack v3.1.4 PackageReference for serialization support.
Module Serialization Infrastructure
src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs
Added PrecompiledRuntimeModule (MessagePack-annotated, implements IExecutableModule) and ModuleSerializer with SaveModule() and LoadModule() to persist modules as MessagePack bytes.
Script Engine Integration
src/ScriptEngine.HostedScript/HostedScriptEngine.cs
Added public Process CreateProcess(IHostApplication host, IExecutableModule module) overload to initialize engine and create a process directly from an executable module.
CLI Option Registration
src/oscript/BehaviorSelector.cs
Registered "-build" command-line option mapped to BuildPrecompiledBehavior.Create.
Build Behavior
src/oscript/BuildPrecompiledBehavior.cs
Added BuildPrecompiledBehavior class: compiles source to IExecutableModule, calls ModuleSerializer.SaveModule() to write .osc, returns exit codes and prints ScriptException messages.
Execution Logic
src/oscript/ExecuteScriptBehavior.cs
Modified to detect .osc files (case-insensitive): load via ModuleSerializer.LoadModule() and create process from module; otherwise load source and create process from source; each path has its own error handling.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as BehaviorSelector
    participant Build as BuildPrecompiledBehavior
    participant Hosted as HostedScriptEngine
    participant Serializer as ModuleSerializer
    participant Disk

    rect rgb(220, 240, 255)
    note over User,CLI: Build flow (-build)
    User->>CLI: invoke -build script.os
    CLI->>Build: Create & Execute
    Build->>Hosted: Load source from file
    Build->>Hosted: Compile -> IExecutableModule
    Build->>Serializer: SaveModule(module, script.osc)
    Serializer->>Disk: write MessagePack bytes
    Build-->>User: exit code (0/1)
    end

    rect rgb(255, 235, 220)
    note over User,CLI: Execute flow (script.osc)
    User->>CLI: run script.osc
    CLI->>Hosted: ExecuteScriptBehavior
    Hosted->>Serializer: LoadModule(script.osc)
    Serializer->>Disk: read MessagePack bytes
    Serializer-->>Hosted: return IExecutableModule
    Hosted->>Hosted: CreateProcess(host, module)
    Hosted-->>User: start process / run script
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Check MessagePack key ordering and compatibility in PrecompiledRuntimeModule.
  • Verify ModuleSerializer file I/O error handling and binary integrity.
  • Review BuildPrecompiledBehavior compilation path and exception reporting.
  • Ensure ExecuteScriptBehavior extension check is case-insensitive and both branches release resources consistently.
  • Confirm CreateProcess(IHostApplication, IExecutableModule) mirrors initialization without unintended side effects.

Poem

🐰
I nibbled bytes and packed them neat,
A .osc carrot—oh what a treat!
Save, then load, no parsing fuss,
Hop to run — swift code for us.
A tiny rabbit claps, "Build — let's trust!"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adding capability for pre-compiling scripts to accelerate startup, which aligns with the implementation of new build behavior, module serialization, and precompiled script execution features.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (2)
src/oscript/ExecuteScriptBehavior.cs (1)

56-84: Consider refactoring duplicated error handling.

Both the precompiled and interpreted branches have identical exception handling logic (lines 66-70 and 79-83). Additionally, catching generic Exception is quite broad and may mask specific error types that should be handled differently.

Consider extracting the common error handling or being more specific about caught exceptions:

-bool isPrecompiled = Path.GetExtension(_path).Equals(".osc", StringComparison.OrdinalIgnoreCase);
-
-Process process;   
-if (isPrecompiled)
-{
-    try
-    {
-        var module = ModuleSerializer.LoadModule(_path);
-        process = hostedScript.CreateProcess(this, module);
-    }
-    catch (Exception e)
-    {
-        ShowExceptionInfo(e);
-        return 1;
-    }
-}
-else
+Process process;
+try
 {
-    try
+    bool isPrecompiled = Path.GetExtension(_path).Equals(".osc", StringComparison.OrdinalIgnoreCase);
+    if (isPrecompiled)
     {
-        var source = hostedScript.Loader.FromFile(_path);
-        process = hostedScript.CreateProcess(this, source);
+        var module = ModuleSerializer.LoadModule(_path);
+        process = hostedScript.CreateProcess(this, module);
     }
-    catch (Exception e)
+    else
     {
-        ShowExceptionInfo(e);
-        return 1;
+        var source = hostedScript.Loader.FromFile(_path);
+        process = hostedScript.CreateProcess(this, source);
     }
 }
+catch (Exception e)
+{
+    ShowExceptionInfo(e);
+    return 1;
+}
src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (1)

43-58: Consider wrapping file I/O exceptions with more context.

The file I/O operations in both SaveModule and LoadModule can fail for various reasons (permissions, disk space, corruption), but the exceptions won't provide context about what was being saved or loaded.

Wrap I/O exceptions with contextual information:

 public static void SaveModule(IExecutableModule module, string filePath)
 {
     var serializableModule = new PrecompiledRuntimeModule 
     {
         // ... properties ...
     };
     
-    byte[] bytes = MessagePackSerializer.Serialize(serializableModule);
-    File.WriteAllBytes(filePath, bytes);
+    try
+    {
+        byte[] bytes = MessagePackSerializer.Serialize(serializableModule);
+        File.WriteAllBytes(filePath, bytes);
+    }
+    catch (Exception ex)
+    {
+        throw new InvalidOperationException($"Failed to save precompiled module to '{filePath}'", ex);
+    }
 }
 
 public static IExecutableModule LoadModule(string filePath)
 {
-    byte[] bytes = File.ReadAllBytes(filePath);
-    return MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes);
+    try
+    {
+        byte[] bytes = File.ReadAllBytes(filePath);
+        return MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes);
+    }
+    catch (Exception ex)
+    {
+        throw new InvalidOperationException($"Failed to load precompiled module from '{filePath}'", ex);
+    }
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5f1c1b5 and ce28b77.

📒 Files selected for processing (6)
  • src/OneScript.Core/OneScript.Core.csproj (1 hunks)
  • src/ScriptEngine.HostedScript/HostedScriptEngine.cs (1 hunks)
  • src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (1 hunks)
  • src/oscript/BehaviorSelector.cs (1 hunks)
  • src/oscript/BuildPrecompiledBehavior.cs (1 hunks)
  • src/oscript/ExecuteScriptBehavior.cs (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
🧬 Code graph analysis (4)
src/ScriptEngine.HostedScript/HostedScriptEngine.cs (3)
src/ScriptEngine.HostedScript/Process.cs (2)
  • Process (13-61)
  • Process (21-31)
src/StandaloneRunner/ProcessLoader.cs (1)
  • Process (25-57)
src/ScriptEngine/ScriptingEngine.cs (1)
  • Initialize (98-106)
src/oscript/BuildPrecompiledBehavior.cs (5)
src/oscript/BehaviorSelector.cs (4)
  • AppBehavior (15-35)
  • AppBehavior (37-59)
  • AppBehavior (61-74)
  • AppBehavior (76-100)
src/oscript/ConsoleHostBuilder.cs (1)
  • ConsoleHostBuilder (16-56)
src/ScriptEngine.HostedScript/HostedScriptEngine.cs (2)
  • Initialize (59-71)
  • SetGlobalEnvironment (143-148)
src/oscript/DoNothingHost.cs (1)
  • DoNothingHost (14-37)
src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (2)
  • ModuleSerializer (41-65)
  • SaveModule (43-58)
src/oscript/ExecuteScriptBehavior.cs (7)
src/ScriptEngine.HostedScript/HostedScriptEngine.cs (3)
  • Process (93-118)
  • Process (120-125)
  • Process (150-156)
src/ScriptEngine.HostedScript/Process.cs (2)
  • Process (13-61)
  • Process (21-31)
src/StandaloneRunner/ProcessLoader.cs (1)
  • Process (25-57)
src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (1)
  • ModuleSerializer (41-65)
src/ScriptEngine.HostedScript/IHostApplication.cs (1)
  • ShowExceptionInfo (15-15)
src/oscript/ConsoleHostImpl.cs (1)
  • ShowExceptionInfo (56-65)
src/StandaloneRunner/StandaloneApplicationHost.cs (1)
  • ShowExceptionInfo (24-27)
src/oscript/BehaviorSelector.cs (1)
src/oscript/BuildPrecompiledBehavior.cs (2)
  • BuildPrecompiledBehavior (15-53)
  • BuildPrecompiledBehavior (19-22)
🔇 Additional comments (2)
src/oscript/BehaviorSelector.cs (1)

50-50: LGTM!

The -build command registration follows the established pattern and correctly maps to BuildPrecompiledBehavior.Create.

src/OneScript.Core/OneScript.Core.csproj (1)

12-14: MessagePack 3.1.4 is current, secure, and compatible—no issues found.

Version 3.1.4 is the latest available. The resolved target framework is net8.0, which is supported by MessagePack 3.1.4. Regarding security, the package has no known vulnerabilities for this version; earlier vulnerability advisories are for older versions and have all been patched in prior releases.

Comment on lines 34 to 43
try
{
var module = compiler.Compile(source, hostedScript.Engine.NewProcess());
ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc"));
}
catch (ScriptException e)
{
Output.WriteLine(e.Message);
return 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Incomplete exception handling for compilation errors.

The code catches ScriptException but not CompilerException. The compiler.Compile call in HostedScriptEngine.CreateProcess can throw CompilerException for syntax/compilation errors (see lines 112-116 in HostedScriptEngine.cs). This means compilation errors won't be caught here and will propagate as unhandled exceptions.

Catch CompilerException as well:

 try
 {
     var module = compiler.Compile(source, hostedScript.Engine.NewProcess());
     ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc"));
 }
+catch (CompilerException e)
+{
+    Output.WriteLine(e.Message);
+    return 1;
+}
 catch (ScriptException e)
 {
     Output.WriteLine(e.Message);
     return 1;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try
{
var module = compiler.Compile(source, hostedScript.Engine.NewProcess());
ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc"));
}
catch (ScriptException e)
{
Output.WriteLine(e.Message);
return 1;
}
try
{
var module = compiler.Compile(source, hostedScript.Engine.NewProcess());
ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc"));
}
catch (CompilerException e)
{
Output.WriteLine(e.Message);
return 1;
}
catch (ScriptException e)
{
Output.WriteLine(e.Message);
return 1;
}
🤖 Prompt for AI Agents
In src/oscript/BuildPrecompiledBehavior.cs around lines 34 to 43, the try/catch
only handles ScriptException but not CompilerException thrown by
compiler.Compile, so add handling for CompilerException as well (either a
separate catch for CompilerException that writes e.Message to Output and returns
1, or a multi-exception catch that handles both ScriptException and
CompilerException the same way) to ensure compilation errors are reported and
the method returns 1 instead of letting the exception propagate.

Comment on lines 120 to 125
public Process CreateProcess(IHostApplication host, IExecutableModule module)
{
Initialize();
var bslProcess = _engine.NewProcess();
return InitProcess(bslProcess, host, module);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing SetGlobalEnvironment call may cause runtime issues.

The new overload skips the SetGlobalEnvironment(host, src) call present in the original CreateProcess method (line 96). This means _globalCtx.ApplicationHost and _globalCtx.CodeSource remain uninitialized, which could cause null reference exceptions if the global context accesses these properties during script execution.

Consider whether the global environment should be set. If a SourceCode instance is unavailable for precompiled modules, you might need to create a placeholder or refactor the global context initialization:

 public Process CreateProcess(IHostApplication host, IExecutableModule module)
 {
     Initialize();
+    SetGlobalEnvironment(host, module.Source);
     var bslProcess = _engine.NewProcess();
     return InitProcess(bslProcess, host, module);           
 }

Alternatively, if SetGlobalEnvironment requires a different approach for precompiled modules, document why it's omitted or add a placeholder implementation.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/ScriptEngine.HostedScript/HostedScriptEngine.cs around lines 120-125, the
new CreateProcess overload initializes a process but omits calling
SetGlobalEnvironment(host, src), leaving _globalCtx.ApplicationHost and
_globalCtx.CodeSource uninitialized; fix by ensuring the global environment is
set before InitProcess — either call SetGlobalEnvironment(host, src) with an
appropriate SourceCode instance (create a minimal/placeholder SourceCode when
module is precompiled), or explicitly set _globalCtx.ApplicationHost = host and
_globalCtx.CodeSource = placeholderSource before returning the process, and
document the chosen approach if omission is intentional.

Comment on lines 16 to 39
[MessagePackObject]
public class PrecompiledRuntimeModule : IExecutableModule
{
[Key(0)]
public IList<BslAnnotationAttribute> ModuleAttributes { get; set; }

[Key(1)]
public IList<BslScriptFieldInfo> Fields { get; set; }

[Key(2)]
public IList<BslScriptPropertyInfo> Properties { get; set; }

[Key(3)]
public IList<BslScriptMethodInfo> Methods { get; set; }

[Key(4)]
public BslScriptMethodInfo ModuleBody { get; set; }

[Key(5)]
public SourceCode Source { get; set; }

[Key(6)]
public IDictionary<Type, object> Interfaces { get; set; }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing namespace declaration pollutes global namespace.

Both PrecompiledRuntimeModule and ModuleSerializer classes lack a namespace declaration, placing them in the global namespace. This is poor practice and can lead to naming conflicts.

Add an appropriate namespace:

+namespace ScriptEngine.Machine
+{
+
 [MessagePackObject]
 public class PrecompiledRuntimeModule : IExecutableModule
 {
     // ... properties ...
 }
 
 public class ModuleSerializer
 {
     // ... methods ...
 }
+
+}

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs around lines 16 to 39,
the PrecompiledRuntimeModule (and the related ModuleSerializer class in the same
file) are declared in the global namespace; wrap these class declarations inside
an appropriate namespace (for example: namespace ScriptEngine.Machine) to avoid
polluting the global namespace and reduce naming conflicts; update the file by
adding the namespace declaration and adjusting indentation or usings as needed
so both classes are enclosed within that namespace.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify MessagePack serialization compatibility of complex types.

PrecompiledRuntimeModule serializes several complex types (e.g., SourceCode, BslScriptMethodInfo, BslScriptFieldInfo, BslScriptPropertyInfo, BslAnnotationAttribute) that may not have MessagePack serialization support. If these types lack the necessary attributes or custom serializers, serialization will fail at runtime.

Run the following script to check if these types have MessagePack attributes:

If these types don't have MessagePack support, you'll need to either:

  1. Add MessagePack attributes to them
  2. Create custom MessagePack formatters
  3. Use a different serialization approach

🏁 Script executed:

#!/bin/bash
# Search for MessagePack attributes on the types used in PrecompiledRuntimeModule

echo "Checking SourceCode:"
ast-grep --pattern $'[MessagePackObject]
class SourceCode {
  $$$
}'

echo -e "\nChecking BslScriptMethodInfo:"
ast-grep --pattern $'[MessagePackObject]
class BslScriptMethodInfo {
  $$$
}'

echo -e "\nChecking BslScriptFieldInfo:"
ast-grep --pattern $'[MessagePackObject]
class BslScriptFieldInfo {
  $$$
}'

echo -e "\nChecking BslScriptPropertyInfo:"
ast-grep --pattern $'[MessagePackObject]
class BslScriptPropertyInfo {
  $$$
}'

echo -e "\nChecking BslAnnotationAttribute:"
ast-grep --pattern $'[MessagePackObject]
class BslAnnotationAttribute {
  $$$
}'

Length of output: 148


I need to verify this more comprehensively. The ast-grep pattern didn't find results, which could mean these types either lack the attribute or use a different structure. Let me search more broadly for these type definitions and any MessagePack configuration.


🏁 Script executed:

#!/bin/bash
# Search for the type definitions and MessagePack support

echo "=== Searching for SourceCode type definition ==="
rg -n "class SourceCode" --type cs

echo -e "\n=== Searching for BslScriptMethodInfo definition ==="
rg -n "class BslScriptMethodInfo" --type cs

echo -e "\n=== Searching for BslScriptFieldInfo definition ==="
rg -n "class BslScriptFieldInfo" --type cs

echo -e "\n=== Searching for BslScriptPropertyInfo definition ==="
rg -n "class BslScriptPropertyInfo" --type cs

echo -e "\n=== Searching for BslAnnotationAttribute definition ==="
rg -n "class BslAnnotationAttribute" --type cs

echo -e "\n=== Searching for custom MessagePack formatters ==="
rg -n "IMessagePackFormatter" --type cs -A2

echo -e "\n=== Searching for MessagePackObject or MessagePack attributes in the codebase ==="
rg -n "MessagePackObject|MessagePack" --type cs -C1 | head -50

Length of output: 2589


I found the type definitions. Now I need to check each one directly to see if they have MessagePack attributes:


🏁 Script executed:

#!/bin/bash
# Check the actual type definitions for MessagePack attributes

echo "=== SourceCode type ==="
head -30 src/OneScript.Language/Sources/SourceCode.cs

echo -e "\n=== BslScriptMethodInfo type ==="
head -40 src/OneScript.Core/Contexts/BslScriptMethodInfo.cs

echo -e "\n=== BslScriptFieldInfo type ==="
head -40 src/OneScript.Core/Contexts/BslScriptFieldInfo.cs

echo -e "\n=== BslScriptPropertyInfo type ==="
head -40 src/OneScript.Core/Contexts/BslScriptPropertyInfo.cs

echo -e "\n=== BslAnnotationAttribute type ==="
head -30 src/OneScript.Core/Contexts/BslAnnotationAttribute.cs

Length of output: 5949


Now let me check if there's any MessagePack resolver configuration or dynamic registration that might support these types:


🏁 Script executed:

#!/bin/bash
# Check for MessagePack resolver setup or dynamic type registration

echo "=== Searching for MessagePack resolver configuration ==="
rg -n "CompositeResolver|StaticCompositeResolver|FormatterProvider|RegisterFormatter" --type cs

echo -e "\n=== Searching for MessagePack code generation or generated types ==="
rg -n "generated|Generated|Generate" --type cs -i src/ScriptEngine/Machine/ | head -20

echo -e "\n=== Checking if there's a MessagePack config or setup file ==="
find . -name "*.cs" -type f | xargs grep -l "MessagePack.*Setup\|SetupFormatter" 2>/dev/null

Length of output: 190


Add MessagePack support to complex types in PrecompiledRuntimeModule.

Verification confirms that SourceCode, BslScriptMethodInfo, BslScriptFieldInfo, BslScriptPropertyInfo, and BslAnnotationAttribute lack MessagePack serialization attributes. When MessagePackSerializer.Serialize() is called on PrecompiledRuntimeModule instances (line 56), serialization will fail at runtime for these unsupported types.

Add [MessagePackObject] attributes and [Key(n)] annotations to each of these five types, or implement custom IMessagePackFormatter implementations for them.

🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs around lines 16 to 39,
the complex member types (SourceCode, BslScriptMethodInfo, BslScriptFieldInfo,
BslScriptPropertyInfo, BslAnnotationAttribute) are not MessagePack-annotated
causing MessagePackSerializer.Serialize() to fail; add MessagePack support by
decorating each of these types with [MessagePackObject] and adding stable
[Key(n)] attributes for all serializable members (or alternatively implement and
register custom IMessagePackFormatter implementations for each type), ensuring
keys are deterministic and compatible with existing serialized layouts so
PrecompiledRuntimeModule serializes successfully.

Comment on lines 41 to 65
public class ModuleSerializer
{
public static void SaveModule(IExecutableModule module, string filePath)
{
var serializableModule = new PrecompiledRuntimeModule
{
ModuleAttributes = module.ModuleAttributes,
Fields = module.Fields,
Properties = module.Properties,
Methods = module.Methods,
ModuleBody = module.ModuleBody,
Source = module.Source,
Interfaces = module.Interfaces
};

byte[] bytes = MessagePackSerializer.Serialize(serializableModule);
File.WriteAllBytes(filePath, bytes);
}

public static IExecutableModule LoadModule(string filePath)
{
byte[] bytes = File.ReadAllBytes(filePath);
return MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes);
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add format versioning and validation to prevent compatibility issues.

The serialization format lacks versioning, meaning future changes to PrecompiledRuntimeModule structure will break compatibility with existing .osc files without any clear error message. Additionally, there's no validation of the deserialized data.

Consider adding format versioning and validation:

 [MessagePackObject]
 public class PrecompiledRuntimeModule : IExecutableModule
 {
+    [Key(7)]
+    public int FormatVersion { get; set; } = 1;
+
     [Key(0)]
     public IList<BslAnnotationAttribute> ModuleAttributes { get; set; }
     // ... other properties ...
 }
 
 public class ModuleSerializer
 {
+    private const int CURRENT_FORMAT_VERSION = 1;
+
     public static void SaveModule(IExecutableModule module, string filePath)
     {
+        if (module == null)
+            throw new ArgumentNullException(nameof(module));
+            
         var serializableModule = new PrecompiledRuntimeModule 
         {
+            FormatVersion = CURRENT_FORMAT_VERSION,
             ModuleAttributes = module.ModuleAttributes,
             // ... other properties ...
         };
         
         byte[] bytes = MessagePackSerializer.Serialize(serializableModule);
         File.WriteAllBytes(filePath, bytes);
     }
     
     public static IExecutableModule LoadModule(string filePath)
     {
+        if (!File.Exists(filePath))
+            throw new FileNotFoundException($"Precompiled module not found: {filePath}");
+            
         byte[] bytes = File.ReadAllBytes(filePath);
-        return MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes);
+        var module = MessagePackSerializer.Deserialize<PrecompiledRuntimeModule>(bytes);
+        
+        if (module.FormatVersion != CURRENT_FORMAT_VERSION)
+            throw new InvalidOperationException(
+                $"Incompatible precompiled module format. Expected version {CURRENT_FORMAT_VERSION}, got {module.FormatVersion}");
+                
+        return module;
     }
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs around lines 41 to 65,
the serializer currently writes raw MessagePack bytes with no format version or
validation; add a small framing header that prefixes serialized data with a
fixed magic string and a uint8/uint16 format version constant when saving, and
on load read and validate the magic and version before deserializing (reject
unknown versions with a clear exception or handle via a version switch for
backward compatibility). After deserialization perform basic validation of
required fields (non-null ModuleAttributes/Methods/ModuleBody etc.) and throw a
descriptive exception if validation fails; update SaveModule/LoadModule to use
the header, increment the version constant for future changes, and ensure error
messages provide the magic/version and a hint about incompatibility.

@EvilBeaver
Copy link
Owner

Сделай, пожалуйста, статус черновика в пулреквесте, чтобы было понятно, когда wip закончился

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
src/oscript/BuildPrecompiledBehavior.cs (1)

34-43: Compilation errors from compiler.Compile are not handled (CompilerException is missing).

compiler.Compile can throw CompilerException for syntax/compilation errors, but only ScriptException is caught here. In such cases -build will crash instead of printing a message and returning non‑zero.

Предлагаемый патч:

 			try
 			{
 				var module = compiler.Compile(source, hostedScript.Engine.NewProcess());
 				ModuleSerializer.SaveModule(module, Path.ChangeExtension(_path, ".osc"));
 			}
+			catch (CompilerException e)
+			{
+				Output.WriteLine(e.Message);
+				return 1;
+			}
 			catch (ScriptException e)
 			{
 				Output.WriteLine(e.Message);
 				return 1;
 			}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce28b77 and 9e63842.

📒 Files selected for processing (1)
  • src/oscript/BuildPrecompiledBehavior.cs (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
🧬 Code graph analysis (1)
src/oscript/BuildPrecompiledBehavior.cs (5)
src/oscript/BehaviorSelector.cs (4)
  • AppBehavior (15-35)
  • AppBehavior (37-59)
  • AppBehavior (61-74)
  • AppBehavior (76-100)
src/oscript/ConsoleHostBuilder.cs (1)
  • ConsoleHostBuilder (16-56)
src/ScriptEngine.HostedScript/HostedScriptEngine.cs (2)
  • Initialize (59-71)
  • SetGlobalEnvironment (143-148)
src/oscript/DoNothingHost.cs (1)
  • DoNothingHost (14-37)
src/ScriptEngine/Machine/PrecompiledRuntimeModule.cs (2)
  • ModuleSerializer (41-65)
  • SaveModule (43-58)
🔇 Additional comments (1)
src/oscript/BuildPrecompiledBehavior.cs (1)

48-52: Factory Create now returns the correct behavior for -build.

The factory correctly instantiates BuildPrecompiledBehavior when путь передан, and returns null otherwise, which aligns with how BehaviorSelector wires -build.

Comment on lines 8 to 12
using System;
using System.IO;
using OneScript.Language;
using ScriptEngine.Compiler;

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

ModuleSerializer may not resolve without the proper namespace import.

ModuleSerializer.SaveModule is used, but this file only imports ScriptEngine.Compiler. Given ModuleSerializer lives in ScriptEngine.Machine.PrecompiledRuntimeModule.cs, you likely need an explicit using ScriptEngine.Machine; (or a fully‑qualified name) to compile.

Рекомендуемый патч:

 using System;
 using System.IO;
 using OneScript.Language;
 using ScriptEngine.Compiler;
+using ScriptEngine.Machine;

Also applies to: 36-37

🤖 Prompt for AI Agents
In src/oscript/BuildPrecompiledBehavior.cs around lines 8-12 (and also where
ModuleSerializer.SaveModule is called at lines 36-37), the code references
ModuleSerializer but lacks the namespace import; add "using
ScriptEngine.Machine;" at the top of the file (or replace the ModuleSerializer
references with the fully-qualified ScriptEngine.Machine.ModuleSerializer) so
the compiler can resolve ModuleSerializer.SaveModule.

@Bayselonarrend
Copy link
Contributor Author

Bayselonarrend commented Nov 20, 2025

Сделай, пожалуйста, статус черновика в пулреквесте, чтобы было понятно, когда wip закончился

@EvilBeaver Пересоздать его как draft? Или в текущем что-то проставить можно?
А, все, нашел

@Bayselonarrend Bayselonarrend marked this pull request as draft November 20, 2025 08:11
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.

2 participants