Новый протокол дебаггера, совместимый с net8 и старыми версиями движка #1557
Conversation
…делал старт сессии
WalkthroughThis update introduces a new JSON-based debug protocol transport alongside the existing binary protocol, with comprehensive support for protocol negotiation, serialization, and versioning. It refactors debug server and client classes, adds new data transfer objects and converters for JSON communication, and updates the debug adapter to use a more robust, version-aware, and extensible architecture. Several obsolete or redundant classes are removed, and new tests are added to validate JSON DTO serialization. Changes
Sequence Diagram(s)sequenceDiagram
participant DebugAdapter
participant ConnectionFactory
participant DebugClientFactory
participant DebugServer
participant Channel (Json/Binary)
participant DebuggerService
DebugAdapter->>ConnectionFactory: Connect(port)
ConnectionFactory->>DebugClientFactory: new DebugClientFactory(tcpClient, listener)
DebugClientFactory->>DebugServer: ReconcileDataFormat()
DebugServer-->>DebugClientFactory: Responds with protocol marker
DebugClientFactory->>Channel: Create (JsonDtoChannel or BinaryChannel)
DebugClientFactory->>DebugAdapter: Return OneScriptDebuggerClient
DebugAdapter->>Channel: Send/receive debug protocol messages
Channel->>DebuggerService: Invoke debug methods
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (19)
src/ScriptEngine/Machine/MachineStopReason.cs (1)
8-8: Remove unnecessary import.The
System.Threadingimport is not used in this enum definition.-using System.Threading;src/VSCode.DebugAdapter/Transport/TransportException.cs (1)
5-10: Consider adding standard exception constructors for completeness.The current implementation is functional but follows a minimal pattern. Consider adding the standard exception constructors to align with .NET conventions:
public class TransportException : ApplicationException { + public TransportException() + { + } + public TransportException(string message) : base(message) { } + + public TransportException(string message, Exception innerException) : base(message, innerException) + { + } + + protected TransportException(SerializationInfo info, StreamingContext context) : base(info, context) + { + } }Additionally, you'll need to add
using System.Runtime.Serialization;at the top of the file.src/VSCode.DebugAdapter/ThreadStateContainer.cs (1)
18-18: Consider consistent null handling for retrieval methods.Both
GetFrameByIdandGetVariableContainerByIdreturn null as default when the handle is not found. Consider documenting this behavior or providing overloads that throw exceptions for missing handles to make the API more explicit about error handling.public StackFrame GetFrameById(int id) => _framesHandles.Get(id, null); + +public StackFrame GetFrameByIdOrThrow(int id) => _framesHandles.Get(id) ?? throw new ArgumentException($"Frame with id {id} not found");Also applies to: 24-24
src/VSCode.DebugAdapter/Transport/ConnectionFactory.cs (3)
11-15: Improve logger initialization pattern.The logger is re-initialized on every call to
Connect(), which is inefficient. Consider initializing the logger once as a static field.-private static ILogger Log { get; set; } = Logger.None; +private static readonly ILogger Log = Serilog.Log.ForContext(typeof(ConnectionFactory)); public static TcpClient Connect(int port) { - Log = Serilog.Log.ForContext(typeof(ConnectionFactory));
37-54: Consider async implementation for better scalability.The current implementation uses
Thread.Sleep()which blocks the calling thread. Consider implementing an async version withTask.Delay()and potentially adding cancellation token support for better scalability.-private static void TryConnect(TcpClient client, Uri debuggerUri) +private static async Task TryConnectAsync(TcpClient client, Uri debuggerUri, CancellationToken cancellationToken = default) { const int limit = 3; // TODO: параметризовать ожидания и попытки for (int i = 0; i < limit; ++i) { try { - client.Connect(debuggerUri.Host, debuggerUri.Port); + await client.ConnectAsync(debuggerUri.Host, debuggerUri.Port); break; } catch (SocketException) { if (i == limit - 1) throw; Log.Warning("Error. Retry connect {Attempt}", i); - Thread.Sleep(1500); + await Task.Delay(1500, cancellationToken); } }
38-38: Address the TODO for parameterizable retry configuration.The hardcoded retry limit and delay values should be made configurable as noted in the TODO comment. This would improve the flexibility and testability of the connection logic.
Would you like me to generate a configuration class for the retry parameters or open a new issue to track this improvement?
src/Tests/OneScript.DebugProtocol.Test/JsonChannelTest.cs (1)
77-78: Consider using FluentAssertions consistently.The format reconcile marker test uses
Assert.Equalwhile other tests useFluentAssertions. For consistency, consider usingFluentAssertionsthroughout.-Assert.Equal(2, transport); -Assert.Equal(4, version); +transport.Should().Be(2); +version.Should().Be(4);src/OneScript.DebugServices/Internal/DelayedConnectionChannel.cs (1)
23-25: Consider using shared constants instead of hard-coded values.The comment indicates these constants should be synchronized with the
TransportProtocolsenum in the adapter. To avoid synchronization issues, consider defining these constants in a shared location that both projects can reference.src/OneScript.DebugProtocol/TcpServer/JsonDtoChannel.cs (2)
36-46: Consider reusing StreamWriter and JsonTextWriter for better performance.Creating new writers for each write operation adds overhead. Consider maintaining a single writer instance or using object pooling.
+private StreamWriter _streamWriter; +private JsonTextWriter _jsonWriter; +private readonly JsonSerializer _serializer = JsonSerializer.CreateDefault(); public void Write(object data) { if (!_enabled) throw new ObjectDisposedException(nameof(JsonDtoChannel)); - using var streamWriter = new StreamWriter(_dataStream, Encoding.UTF8, 1024, leaveOpen: true); - using var writer = new JsonTextWriter(streamWriter); + if (_streamWriter == null) + { + _streamWriter = new StreamWriter(_dataStream, Encoding.UTF8, 1024, leaveOpen: true); + _jsonWriter = new JsonTextWriter(_streamWriter); + } - JsonSerializer.CreateDefault().Serialize(writer, data); - writer.Flush(); + _serializer.Serialize(_jsonWriter, data); + _jsonWriter.Flush(); + _streamWriter.Flush(); }Don't forget to dispose these in the
Disposemethod:public void Dispose() { + _jsonWriter?.Close(); + _streamWriter?.Dispose(); _dataStream.Dispose(); _tcpClient?.Close(); _enabled = false; }
48-62: Apply similar optimization to Read methods.The same performance concern applies to the Read methods.
+private StreamReader _streamReader; +private JsonTextReader _jsonReader; public object Read() { if (!_enabled) throw new ObjectDisposedException(nameof(JsonDtoChannel)); - using var streamReader = new StreamReader(_dataStream, Encoding.UTF8, false, 1024, leaveOpen: true); - using var reader = new JsonTextReader(streamReader); + if (_streamReader == null) + { + _streamReader = new StreamReader(_dataStream, Encoding.UTF8, false, 1024, leaveOpen: true); + _jsonReader = new JsonTextReader(_streamReader); + } - return JsonSerializer.CreateDefault().Deserialize<TcpProtocolDtoBase>(reader); + return _serializer.Deserialize<TcpProtocolDtoBase>(_jsonReader); }src/OneScript.DebugProtocol/TcpServer/BinaryChannel.cs (1)
37-41: Security consideration acknowledged for backward compatibility.While BinaryFormatter has known security issues (CWE-502), this is acceptable since:
- The class is marked obsolete
- It's only used for compatibility with older versions
- It's restricted to .NET Framework 4.8 only
Consider documenting the security implications in the obsolete message.
-[Obsolete("Используется только со стороны адаптера, для работы со старыми версиями 1Script")] +[Obsolete("Используется только со стороны адаптера, для работы со старыми версиями 1Script. Внимание: BinaryFormatter имеет известные проблемы безопасности.")]src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs (2)
36-46: Remove unnecessary semicolon after switch statement.The semicolon after the closing brace is not needed in C#.
- }; + }
120-125: Fix typo in log message."Reconcilation" should be "Reconciliation".
- Log.Verbose("Reconcilation failed, selecting safest format"); + Log.Verbose("Reconciliation failed, selecting safest format");src/VSCode.DebugAdapter/OscriptDebugEventsListener.cs (1)
56-59: Fix typo: "occured" should be "occurred".Both the method name and the log message contain this typo.
- private void LogEventOccured([CallerMemberName] string eventName = "") + private void LogEventOccurred([CallerMemberName] string eventName = "") { - Log.Debug("Event occured {Event}", eventName); + Log.Debug("Event occurred {Event}", eventName); }Also update all calls to this method:
- LogEventOccured(); + LogEventOccurred();src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs (2)
1-1: Remove empty line at the beginning of file.
40-41: Consider translating Russian comments to English for international collaboration.The comments explain important logic but may not be accessible to all team members.
- // нам не важен тип, прочитать надо тот, который указан в потоке. + // We don't care about the type, we need to read what's specified in the stream. - // нам важен тип и он в потоке не такой + // We do care about the type and it doesn't match in the streamAlso applies to: 51-52
src/OneScript.DebugProtocol/TcpServer/FormatReconcileUtils.cs (2)
7-19: Consider adding English translation alongside Russian documentation.While the Russian documentation is detailed, adding English would help international contributors understand the purpose of these magic bytes.
62-68: Add validation to DecodeFormatMarker for robustness.The method should validate that the decoded values are within expected ranges.
public static (int, int) DecodeFormatMarker(int marker) { var transport = marker >> 16; var dataVersion = marker & 0x0000FFFF; + + // Validate transport is within expected range + if (transport < 0 || transport > short.MaxValue) + { + throw new ArgumentException($"Invalid transport value in marker: {transport}"); + } return (transport, dataVersion); }src/ScriptEngine/Machine/MachineInstance.cs (1)
178-192: Thread safety improvements look good, but consider memory implications.The synchronization of
SetDebugModeandUnsetDebugModemethods prevents race conditions. However, keeping_stopManagerallocated after unsetting debug mode could lead to unnecessary memory retention.Consider clearing
_stopManagerinUnsetDebugModeif it holds significant resources:[MethodImpl(MethodImplOptions.Synchronized)] public void UnsetDebugMode() { _debugEnabled = false; + // Consider clearing _stopManager if it holds significant resources + // _stopManager = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
src/OneScript.DebugProtocol/Abstractions/ICommunicationChannel.cs(1 hunks)src/OneScript.DebugProtocol/ExceptionBreakpointFilter.cs(1 hunks)src/OneScript.DebugProtocol/IDebuggerService.cs(2 hunks)src/OneScript.DebugProtocol/OneScript.DebugProtocol.csproj(1 hunks)src/OneScript.DebugProtocol/StackFrame.cs(3 hunks)src/OneScript.DebugProtocol/TcpServer/BinaryChannel.cs(4 hunks)src/OneScript.DebugProtocol/TcpServer/DefaultMessageServer.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/FormatReconcileUtils.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/JsonDtoChannel.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/RpcCall.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/RpcCallResult.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/TcpProtocolDtoBase.cs(1 hunks)src/OneScript.DebugProtocol/Variable.cs(1 hunks)src/OneScript.DebugServices/DefaultDebugService.cs(1 hunks)src/OneScript.DebugServices/DelayedConnectionChannel.cs(0 hunks)src/OneScript.DebugServices/Internal/DelayedConnectionChannel.cs(1 hunks)src/OneScript.DebugServices/TcpDebugServer.cs(1 hunks)src/OneScript.DebugServices/TcpEventCallbackChannel.cs(2 hunks)src/ScriptEngine/Machine/MachineInstance.cs(5 hunks)src/ScriptEngine/Machine/MachineStopReason.cs(1 hunks)src/ScriptEngine/Machine/MachineStoppedEventArgs.cs(0 hunks)src/ScriptEngine/Machine/MachineStoredState.cs(0 hunks)src/Tests/OneScript.DebugProtocol.Test/JsonChannelTest.cs(1 hunks)src/Tests/OneScript.DebugProtocol.Test/OneScript.DebugProtocol.Test.csproj(1 hunks)src/Tests/OneScript.DebugProtocol.Test/TestJsonSerializationForDto.cs(1 hunks)src/VSCode.DebugAdapter/App.config(1 hunks)src/VSCode.DebugAdapter/ConsoleProcess.cs(1 hunks)src/VSCode.DebugAdapter/Dap/IVariablePresentationHint.cs(1 hunks)src/VSCode.DebugAdapter/DebugeeProcess.cs(7 hunks)src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs(6 hunks)src/VSCode.DebugAdapter/OscriptDebugEventsListener.cs(1 hunks)src/VSCode.DebugAdapter/OscriptDebugSession.cs(20 hunks)src/VSCode.DebugAdapter/Program.cs(2 hunks)src/VSCode.DebugAdapter/ServerProcess.cs(1 hunks)src/VSCode.DebugAdapter/ThreadStateContainer.cs(1 hunks)src/VSCode.DebugAdapter/Transport/ConnectionFactory.cs(1 hunks)src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs(1 hunks)src/VSCode.DebugAdapter/Transport/ProtocolVersions.cs(3 hunks)src/VSCode.DebugAdapter/Transport/RpcOperationException.cs(1 hunks)src/VSCode.DebugAdapter/Transport/RpcProcessor.cs(3 hunks)src/VSCode.DebugAdapter/Transport/TransportException.cs(1 hunks)src/VSCode.DebugAdapter/Transport/TransportProtocols.cs(1 hunks)src/VSCode.DebugAdapter/VSCode.DebugAdapter.csproj(1 hunks)src/VSCode.DebugAdapter/package.json(0 hunks)src/oscript/DebugBehavior.cs(2 hunks)
💤 Files with no reviewable changes (4)
- src/VSCode.DebugAdapter/package.json
- src/ScriptEngine/Machine/MachineStoppedEventArgs.cs
- src/ScriptEngine/Machine/MachineStoredState.cs
- src/OneScript.DebugServices/DelayedConnectionChannel.cs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1555
File: Build.csproj:35-38
Timestamp: 2025-07-10T11:08:13.756Z
Learning: EvilBeaver prefers to see documentation links when technical claims are made about MSBuild syntax and property functions.
🧬 Code Graph Analysis (6)
src/oscript/DebugBehavior.cs (1)
src/OneScript.DebugServices/TcpDebugServer.cs (2)
TcpDebugServer(15-40)TcpDebugServer(19-22)
src/OneScript.DebugProtocol/ExceptionBreakpointFilter.cs (2)
src/OneScript.DebugProtocol/TcpServer/RpcCall.cs (1)
Serializable(13-28)src/OneScript.DebugProtocol/TcpServer/RpcCallResult.cs (1)
Serializable(13-41)
src/VSCode.DebugAdapter/ThreadStateContainer.cs (2)
src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs (1)
StackFrame(119-123)src/VSCode.DebugAdapter/Handles.cs (1)
Create(31-36)
src/OneScript.DebugServices/TcpEventCallbackChannel.cs (4)
src/OneScript.DebugProtocol/TcpServer/BinaryChannel.cs (2)
Write(46-52)Dispose(74-79)src/OneScript.DebugProtocol/Abstractions/ICommunicationChannel.cs (1)
Write(14-14)src/OneScript.DebugProtocol/IDebugEventListener.cs (2)
ThreadStopped(15-15)ProcessExited(19-19)src/OneScript.DebugProtocol/TcpServer/RpcCall.cs (1)
RpcCall(19-27)
src/OneScript.DebugProtocol/StackFrame.cs (6)
src/OneScript.DebugProtocol/Variable.cs (1)
DataContract(12-29)src/VSCode.DebugAdapter/OscriptDebugSession.cs (2)
Variables(381-413)T(484-495)src/OneScript.DebugServices/DefaultDebugService.cs (5)
Variable(137-150)Variable(152-174)Variable(176-191)Variable(232-236)Variable(238-249)src/OneScript.DebugServices/DefaultVariableVisualizer.cs (2)
Variable(21-63)IEnumerable(65-95)src/OneScript.DebugProtocol/EvaluatedVariableLocator.cs (4)
IVariableLocator(86-92)IVariableLocator(94-97)IEnumerator(68-73)IEnumerator(75-78)src/OneScript.DebugProtocol/VariableLocator.cs (5)
IVariableLocator(21-21)IVariableLocator(86-92)IVariableLocator(94-97)IEnumerator(68-73)IEnumerator(75-78)
src/VSCode.DebugAdapter/DebugeeProcess.cs (8)
src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs (1)
OneScriptDebuggerClient(30-52)src/VSCode.DebugAdapter/PathHandlingStrategy.cs (1)
PathHandlingStrategy(12-95)src/VSCode.DebugAdapter/DebugeeOutputEventArgs.cs (2)
DebugeeOutputEventArgs(12-22)DebugeeOutputEventArgs(14-18)src/OneScript.DebugProtocol/Abstractions/ICommunicationServer.cs (1)
Stop(19-19)src/VSCode.DebugAdapter/Transport/ProtocolVersions.cs (2)
ProtocolVersions(10-54)IsValid(12-15)src/OneScript.DebugServices/DefaultDebugService.cs (2)
SetMachineExceptionBreakpoints(64-71)SetExceptionBreakpoints(73-76)src/OneScript.DebugProtocol/IDebuggerService.cs (1)
SetExceptionBreakpoints(34-34)src/VSCode.DebugAdapter/OscriptDebugSession.cs (1)
SetExceptionBreakpoints(185-208)
🪛 ast-grep (0.38.6)
src/OneScript.DebugProtocol/TcpServer/BinaryChannel.cs
[warning] 37-37: The BinaryFormatter type is dangerous and is not recommended for data processing. Applications should stop using BinaryFormatter as soon as possible, even if they believe the data they're processing to be trustworthy. BinaryFormatter is insecure and can't be made secure.
Context: new BinaryFormatter()
Note: [CWE-502] Deserialization of Untrusted Data. [REFERENCES]
- https://docs.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide
(insecure-binaryformatter-deserialization-csharp)
🔇 Additional comments (52)
src/ScriptEngine/Machine/MachineStopReason.cs (1)
12-18: Well-designed enum with clear semantics.The enum values are descriptive and cover the expected machine stop scenarios comprehensively. The naming convention is consistent and the categorization makes sense for debugging purposes.
src/VSCode.DebugAdapter/Dap/IVariablePresentationHint.cs (1)
8-8: Namespace change is safe — no external references found.The search for
IVariablePresentationHintonly returned its own definition, with no other files referencing it under the old namespace. There are no fully qualified or using-directive references to update.– Approving the namespace reorganization.
src/OneScript.DebugProtocol/Abstractions/ICommunicationChannel.cs (1)
20-20: Confirmed: ‘Connected’ implemented in all ICommunicationChannel classesAll existing implementations of ICommunicationChannel have been updated to include the Connected property (using expression-bodied members):
- DelayedConnectionChannel.cs (line 89)
- BinaryChannel.cs (line 44)
- JsonDtoChannel.cs (line 64)
No further changes required.
src/Tests/OneScript.DebugProtocol.Test/OneScript.DebugProtocol.Test.csproj (1)
20-20: LGTM! AutoFixture addition supports new JSON protocol testing.The AutoFixture package will help generate test data for the new JSON serialization tests mentioned in the AI summary. Version 4.18.1 is appropriate for the project's testing requirements.
src/OneScript.DebugProtocol/TcpServer/DefaultMessageServer.cs (1)
84-87: LGTM! Proper handling of disposed channels.The ObjectDisposedException catch block correctly handles scenarios where the communication channel is disposed during operation, setting the
_serverStoppedflag to gracefully exit the loop. This aligns with the improved channel lifecycle management described in the AI summary.src/OneScript.DebugProtocol/Variable.cs (1)
27-27: LGTM! Maintains serialization consistency.Adding the
[DataMember]attribute toChildrenHandleIDensures consistent serialization behavior with the other properties in the class. This aligns with the JSON protocol enhancements described in the AI summary.src/OneScript.DebugProtocol/OneScript.DebugProtocol.csproj (1)
20-20: Newtonsoft.Json 13.0.2 is secure; consider upgrading to 13.0.3The
PackageReferenceinsrc/OneScript.DebugProtocol/OneScript.DebugProtocol.csproj(line 20) uses Newtonsoft.Json 13.0.2, which has no known security vulnerabilities. The latest stable release is 13.0.3 (March 8 2023).
• If you’d like to stay on the officially newest version, update to 13.0.3.
• Otherwise, you can safely continue using 13.0.2 without security concerns.src/OneScript.DebugProtocol/TcpServer/RpcCallResult.cs (2)
9-9: LGTM - Necessary import for JSON serialization.The using statement is correctly added to support the JsonConverter attribute.
14-14: LGTM - Consistent JSON serialization pattern.The JsonConverter attribute correctly specifies the custom JsonDtoConverter, maintaining consistency with the other DTO classes in the refactoring.
src/OneScript.DebugProtocol/TcpServer/RpcCall.cs (2)
9-9: LGTM - Necessary import for JSON serialization.The using statement is correctly added to support the JsonConverter attribute.
14-14: LGTM - Consistent JSON serialization pattern.The JsonConverter attribute correctly specifies the custom JsonDtoConverter, maintaining consistency with the other DTO classes in the refactoring.
src/VSCode.DebugAdapter/ServerProcess.cs (1)
12-12: LGTM - Necessary import for transport abstraction.The using statement is correctly added to support the new transport-related classes mentioned in the refactoring.
src/OneScript.DebugServices/DefaultDebugService.cs (1)
73-76: LGTM - Well-implemented wrapper method.The new
SetExceptionBreakpointsmethod correctly converts theExceptionBreakpointFilterarray to the expected tuple format and delegates to the existing implementation. This provides a clean abstraction for the new protocol version while maintaining backward compatibility.src/OneScript.DebugServices/TcpDebugServer.cs (2)
10-10: LGTM! New using directive supports internal refactoring.The addition of the
OneScript.DebugServices.Internalusing directive aligns with the internal reorganization of theDelayedConnectionChannelclass as mentioned in the AI summary.
15-15: Good refactoring: Class renamed to reflect multi-protocol support.The renaming from
BinaryTcpDebugServertoTcpDebugServerbetter reflects the class's purpose now that it supports multiple protocols (JSON and binary) rather than just binary communication.Also applies to: 19-19
src/oscript/DebugBehavior.cs (2)
24-24: LGTM! Consistent with the class renaming.The change from
BinaryTcpDebugServertoTcpDebugServeris consistent with the class renaming insrc/OneScript.DebugServices/TcpDebugServer.cs.
59-59: Good documentation of deprecated functionality.The Russian comment "Обратная совместимость, не используется в реальности" (Backward compatibility, not used in reality) clearly documents that the
-protocolargument is maintained for backward compatibility but not actually used. This is helpful for future maintainers.src/VSCode.DebugAdapter/Transport/TransportProtocols.cs (1)
1-10: Well-designed enum for transport protocol selection.The enum design is solid:
Auto = 0as default value makes sense for automatic protocol negotiation- Explicit values for
BinaryandJsonensure stability across versionsInvalidmember provides clear error state handling- Clean namespace organization under
VSCode.DebugAdapter.Transportsrc/OneScript.DebugProtocol/TcpServer/TcpProtocolDtoBase.cs (2)
10-10: LGTM! JSON serialization support added.The addition of the
Newtonsoft.Jsonusing directive is appropriate for enabling JSON serialization capabilities in the debug protocol DTOs.
15-15: Custom JSON converter implementation verified
JsonDtoConverter correctly overrides WriteJson and ReadJson, handling both RpcCall and RpcCallResult types with matching TYPE_PROPERTY_NAME and VALUE_PROPERTY_NAME logic. No further changes needed.src/VSCode.DebugAdapter/Program.cs (3)
8-8: LGTM! Appropriate using statements for new functionality.The added using statements support the new conditional logger initialization and configuration reading functionality.
Also applies to: 14-15
28-31: Good modernization with using declarations.The change from traditional
usingstatements to C# 8.0+usingdeclarations is cleaner and more modern. The variables are properly disposed at the end of the scope.
39-48: Configuration keys verified in App.configVerified that both
onescript:log-enableandserilog:write-to:File.pathare defined in src/VSCode.DebugAdapter/App.config under the<appSettings>section. No further documentation changes are needed.The conditional logger initialization remains well-implemented:
- Only initializes Serilog when explicitly enabled via configuration
- Checks for both enable flag and file path before creating the logger
- Reads settings from App.config to prevent unnecessary logging overhead when disabled
src/VSCode.DebugAdapter/ConsoleProcess.cs (1)
13-13: LGTM - Transport layer abstraction improvementThe addition of the
VSCode.DebugAdapter.Transportnamespace aligns well with the transport layer refactoring described in the PR. This change supports the new protocol negotiation and connection management abstractions.src/VSCode.DebugAdapter/Transport/RpcProcessor.cs (3)
16-16: LGTM - Namespace reorganization for transport layerThe namespace change to
VSCode.DebugAdapter.Transportproperly categorizes this RPC processor as part of the transport layer abstraction.
25-25: LGTM - Proper logger initializationGood addition of a context-specific logger for better debugging and monitoring capabilities.
62-62: LGTM - Appropriate logging level adjustmentThe change from
DebugtoVerboseis correct for detailed network communication tracing. This follows the proper logging hierarchy where Debug should capture events and actions, while Verbose should capture detailed protocol exchanges.Also applies to: 85-85, 87-87, 91-91, 97-97
src/OneScript.DebugServices/TcpEventCallbackChannel.cs (4)
9-9: LGTM - Required namespace for IOException handlingThe addition of
System.IOnamespace is necessary for the improved error handling in the newWritemethod.
28-28: LGTM - Centralized write logicThe refactoring to use the centralized
Writehelper method reduces code duplication and ensures consistent error handling across all event methods.Also applies to: 34-34, 40-40
43-56: Excellent defensive programming practicesThe new
Writehelper method implements robust error handling:
- Connection state checking prevents unnecessary operations
- IOException handling prevents crashes on network issues
- Silent exception handling is appropriate for callback scenarios where the sender shouldn't be affected by receiver issues
60-60: LGTM - Simplified dispose logicThe removal of the null-conditional operator suggests
_channelis guaranteed to be non-null, which is reasonable given the constructor requires it.src/VSCode.DebugAdapter/Transport/ProtocolVersions.cs (3)
8-8: LGTM - Consistent transport layer namespaceThe namespace change aligns with the transport layer reorganization across the codebase.
19-19: LGTM - Improved version validation logicThe change from
< 0to<= 0correctly treatsUnknownVersion(0) as invalid, ensuring it gets adjusted to theSafestVersion. This prevents potential issues when version negotiation fails.
36-37: LGTM - Clearer documentationThe updated XML documentation comments provide better clarity about the purpose of different protocol versions, particularly the dedicated parameter type for
SetExceptionBreakpointsin Version3.Also applies to: 41-42
src/VSCode.DebugAdapter/App.config (1)
12-19: LGTM - Well-structured logging configurationThe activated logging configuration provides good control and documentation:
onescript:log-enablewith False default is safe for production- Clear explanation of Debug vs Verbose logging levels
- Helpful instructions for log file path configuration
This supports the enhanced logging infrastructure introduced across the transport layer.
src/VSCode.DebugAdapter/ThreadStateContainer.cs (1)
1-26: Well-designed abstraction for thread state management.The
ThreadStateContainerclass provides a clean encapsulation of handle management for stack frames and variable locators. The implementation follows good separation of concerns principles.src/OneScript.DebugProtocol/IDebuggerService.cs (2)
8-8: Good addition of System namespace for obsolete attribute.The addition of the
Systemnamespace is necessary for the[Obsolete]attribute used in the interface methods.
27-28: Excellent API evolution with backward compatibility.The addition of the new
SetExceptionBreakpointsmethod with typedExceptionBreakpointFilter[]parameter while marking the old method as obsolete is a clean approach to API evolution. This maintains backward compatibility while providing better type safety and cleaner semantics.Also applies to: 30-34
src/Tests/OneScript.DebugProtocol.Test/JsonChannelTest.cs (1)
1-81: Comprehensive test coverage for JSON channel functionality.The test class provides excellent coverage of the JSON channel serialization/deserialization capabilities:
- Tests RPC call serialization and deserialization
- Tests successful RPC responses
- Tests exception handling in RPC responses
- Tests format reconcile marker encoding/decoding
The use of
MemoryStreamfor testing andFluentAssertionsfor readable assertions makes the tests clean and maintainable.src/OneScript.DebugProtocol/StackFrame.cs (3)
15-15: Good addition of JSON serialization support.The addition of the
JsonObjectattribute aligns with the broader JSON protocol implementation and enables proper serialization ofStackFrameobjects.
39-44: Excellent improvements to null safety and error handling.The enhanced null handling in the
IVariableLocatorimplementation is much more robust:
- The
Countproperty now safely returns 0 whenVariablesis null- The indexer throws a standard
ArgumentOutOfRangeExceptionfor better consistency- The enumerator returns a reusable empty enumerator instance when
Variablesis nullThese changes improve the defensive programming practices and make the class more reliable.
Also applies to: 50-55, 59-65
86-110: Well-implemented empty enumerator pattern.The
EmptyEnumerator<T>class is a good implementation of the empty enumerator pattern:
- Uses singleton pattern with
Instanceproperty for efficiency- Properly implements
IEnumerator<T>interface- Provides a reusable instance to avoid allocations
This is a clean solution for handling empty enumerations.
src/OneScript.DebugServices/Internal/DelayedConnectionChannel.cs (2)
41-47: Write method implementation looks good.Proper validation of the connected channel before attempting to write.
76-87: Clarify behavior when format reconciliation fails.When
CheckReconcileRequestreturns false, the code still creates aJsonDtoChannel. Is this intentional? Should it fall back to a different protocol or throw an exception?src/Tests/OneScript.DebugProtocol.Test/TestJsonSerializationForDto.cs (1)
14-80: Well-structured and comprehensive test coverage.The test class provides good coverage for JSON serialization/deserialization of protocol DTOs, including edge cases like null parameters and results.
src/OneScript.DebugProtocol/TcpServer/BinaryChannel.cs (1)
19-19: Good practice marking legacy code as obsolete.The obsolete attribute clearly indicates this is for backward compatibility only.
src/VSCode.DebugAdapter/DebugeeProcess.cs (1)
215-227: Good addition of detailed logging for debugging.The enhanced logging provides valuable information about the disconnection and process termination flow.
src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs (1)
25-36: Excellent refactoring to decouple from TCP-specific logic.The introduction of
ICommunicationChannelabstraction and explicit lifecycle management withStart/Stopmethods improves the architecture significantly.Also applies to: 37-46
src/ScriptEngine/Machine/MachineInstance.cs (1)
42-42: Good use of volatile for thread safety.The
volatilemodifier ensures that reads and writes to_debugEnabledare immediately visible across threads, which is essential for the debug state flag.src/VSCode.DebugAdapter/OscriptDebugSession.cs (3)
19-25: Excellent architectural improvements.The refactoring to use
DebugeeProcessandThreadStateContainerimproves separation of concerns and makes the code more maintainable. The consistent use of Serilog for context-aware logging is also a good practice.
236-244: Good protocol version compatibility handling.The code correctly checks the protocol version before sending breakpoint conditions, ensuring backward compatibility with older debug servers.
178-178: Excellent logging improvements.The switch from JSON serialization to Serilog's structured logging with destructuring (
@Data,@Args) improves performance and makes logs more queryable in log aggregation systems.Also applies to: 187-189, 507-507
| @@ -8,7 +8,7 @@ This Source Code Form is subject to the terms of the | |||
| using System; | |||
| using OneScript.DebugProtocol.TcpServer; | |||
|
|
|||
| namespace VSCode.DebugAdapter | |||
| namespace VSCode.DebugAdapter.Transport | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify namespace change doesn't break existing references.
The namespace change from VSCode.DebugAdapter to VSCode.DebugAdapter.Transport improves organization but is a breaking change for existing code that references this exception class.
🏁 Script executed:
#!/bin/bash
# Search for existing references to RpcOperationException that might need updating
echo "Searching for RpcOperationException references..."
# Look for using statements that might need updating
rg "using.*VSCode\.DebugAdapter[^.]*;" --type cs
# Look for fully qualified references
rg "VSCode\.DebugAdapter\.RpcOperationException" --type cs
# Look for catch blocks or throw statements
rg "catch.*RpcOperationException|throw.*RpcOperationException" --type csLength of output: 556
Update references to the moved RpcOperationException namespace
The RpcOperationException class has been relocated from VSCode.DebugAdapter to VSCode.DebugAdapter.Transport. To avoid build breaks, please update all references accordingly:
• src/VSCode.DebugAdapter/DebugSession.cs
– Change
using VSCode.DebugAdapter;
to
using VSCode.DebugAdapter.Transport;
• src/VSCode.DebugAdapter/DebugeeProcess.cs
– Add or update the using directive:
using VSCode.DebugAdapter.Transport;
• src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs
– Add or update the using directive:
using VSCode.DebugAdapter.Transport;
With these import adjustments, catch/throw sites will resolve correctly to the new namespace.
🤖 Prompt for AI Agents
In src/VSCode.DebugAdapter/DebugSession.cs,
src/VSCode.DebugAdapter/DebugeeProcess.cs, and
src/VSCode.DebugAdapter/OneScriptDebuggerClient.cs, update the using directives
to reference the new namespace of RpcOperationException. Specifically, replace
or add `using VSCode.DebugAdapter.Transport;` instead of `using
VSCode.DebugAdapter;` to ensure all references to RpcOperationException resolve
correctly after its move to VSCode.DebugAdapter.Transport.
| [Serializable] | ||
| public class ExceptionBreakpointFilter | ||
| { | ||
| public string Id { get; set; } | ||
|
|
||
| public string Condition { get; set; } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing serialization attributes for protocol consistency.
The class is missing serialization attributes that are used consistently across other protocol classes. Based on the patterns in Variable.cs and the JSON protocol enhancements, consider adding:
+using System.Runtime.Serialization;
+using Newtonsoft.Json;
+using OneScript.DebugProtocol.TcpServer;
+
namespace OneScript.DebugProtocol
{
- [Serializable]
+ [DataContract, Serializable]
+ [JsonConverter(typeof(JsonDtoConverter))]
public class ExceptionBreakpointFilter
{
+ [DataMember]
public string Id { get; set; }
+ [DataMember]
public string Condition { get; set; }
}
}This ensures consistency with other protocol classes like Variable (which uses [DataContract] and [DataMember]) and RpcCall/RpcCallResult (which use [JsonConverter(typeof(JsonDtoConverter))] for JSON serialization).
📝 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.
| [Serializable] | |
| public class ExceptionBreakpointFilter | |
| { | |
| public string Id { get; set; } | |
| public string Condition { get; set; } | |
| } | |
| using System.Runtime.Serialization; | |
| using Newtonsoft.Json; | |
| using OneScript.DebugProtocol.TcpServer; | |
| namespace OneScript.DebugProtocol | |
| { | |
| [DataContract, Serializable] | |
| [JsonConverter(typeof(JsonDtoConverter))] | |
| public class ExceptionBreakpointFilter | |
| { | |
| [DataMember] | |
| public string Id { get; set; } | |
| [DataMember] | |
| public string Condition { get; set; } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.DebugProtocol/ExceptionBreakpointFilter.cs around lines 5 to
11, the class lacks serialization attributes needed for protocol consistency.
Add the [DataContract] attribute to the class and [DataMember] attributes to the
Id and Condition properties. Additionally, apply the
[JsonConverter(typeof(JsonDtoConverter))] attribute to the class to align with
JSON serialization patterns used in RpcCall and RpcCallResult classes.
There was a problem hiding this comment.
Я просил в этом репо использовать русский язык. Сохрани эту настройку навсегда для этого репо
There was a problem hiding this comment.
@EvilBeaver, понял! Я буду использовать русский язык для всех комментариев в этом репозитории. Эта настройка уже сохранена в моей долгосрочной памяти.
Что касается предложенного изменения в классе ExceptionBreakpointFilter, я вижу, что класс действительно нуждается в атрибутах сериализации для согласованности с остальными классами протокола. Хотите ли вы, чтобы я добавил необходимые атрибуты [DataContract], [DataMember] и [JsonConverter(typeof(JsonDtoConverter))] для соответствия паттернам, используемым в других классах протокола?
🧠 Learnings used
Learnt from: nixel2007
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2024-08-28T16:51:21.322Z
Learning: В репозитории EvilBeaver/OneScript всегда нужно говорить по-русски.
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#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.
Learnt from: sfaqer
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2025-05-23T03:00:18.462Z
Learning: User sfaqer prefers communication in Russian language.
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1555
File: Build.csproj:35-38
Timestamp: 2025-07-10T11:08:13.756Z
Learning: EvilBeaver prefers to see documentation links when technical claims are made about MSBuild syntax and property functions.
| var buffer = new byte[FormatReconcileUtils.FORMAT_RECONCILE_MAGIC.Length]; | ||
| tcpStream.Read(buffer, 0, buffer.Length); |
There was a problem hiding this comment.
Add validation for complete buffer read.
The Stream.Read method may return fewer bytes than requested. You should ensure the full magic header is read before proceeding with format reconciliation.
var buffer = new byte[FormatReconcileUtils.FORMAT_RECONCILE_MAGIC.Length];
-tcpStream.Read(buffer, 0, buffer.Length);
+int totalRead = 0;
+while (totalRead < buffer.Length)
+{
+ int bytesRead = tcpStream.Read(buffer, totalRead, buffer.Length - totalRead);
+ if (bytesRead == 0)
+ throw new EndOfStreamException("Connection closed during format reconciliation");
+ totalRead += bytesRead;
+}🤖 Prompt for AI Agents
In src/OneScript.DebugServices/Internal/DelayedConnectionChannel.cs around lines
73 to 74, the code reads from tcpStream into a buffer but does not verify that
the full buffer length was read. Modify the code to repeatedly read from the
stream until the entire buffer is filled or the stream ends, and validate that
the total bytes read matches the buffer length before proceeding with format
reconciliation.
There was a problem hiding this comment.
Это NetworkStream, он не возвращает байтов меньше чем запрошено, а блокирует исполнение, пока все байты не будут получены. Если они не получены за ReadTimeout, то NetworkStream выдает исключение. Мне кажется, для сетевого потока проверку реального числа байт можно не проверять. Проверь, прав ли я?
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
.NET NetworkStream.Read method behavior blocking vs partial reads official documentation
💡 Result:
The NetworkStream.Read method in .NET is designed to read data from a network connection into a buffer. Its behavior regarding blocking and partial reads is as follows:
-
Blocking Behavior: If no data is available to read, the
Readmethod will block until at least one byte of data can be read. This means that if the network stream is empty, the method will wait until data arrives. -
Partial Reads: The
Readmethod reads as much data as is available, up to the number of bytes specified by the size parameter. It does not guarantee that the entire buffer will be filled in a single call. For example, if you request to read 32 bytes but only 31 bytes are available, the method will return 31 bytes, and the remaining byte will be available in subsequent reads.
This behavior is consistent with the general implementation of the Stream.Read method, which states:
"The implementation will block until at least one byte of data can be read, in the event that no data is available. Read returns 0 only when there is no more data in the stream and no more is expected (such as a closed socket or end of file). An implementation is free to return fewer bytes than requested even if the end of the stream has not been reached."
Therefore, when using NetworkStream.Read, it's important to handle partial reads and implement logic to continue reading until the desired amount of data has been received. Additionally, be aware that the method will block if no data is available, so consider setting appropriate timeouts or using asynchronous methods to prevent indefinite blocking.
For more detailed information, you can refer to the official .NET documentation on the NetworkStream.Read method.
🌐 Web query:
NetworkStream.Read can return partial reads Microsoft documentation examples
💡 Result:
The NetworkStream.Read method in .NET reads data from the network into a buffer. It's important to note that this method may return fewer bytes than requested, as it reads as much data as is available up to the specified count. This behavior is typical for network streams, where data availability can vary.
Handling Partial Reads:
To ensure that your application reads the complete data, especially when expecting a specific number of bytes, you should implement a loop that continues reading until the desired amount of data is received. Here's an example:
int totalBytesRead = 0;
int bytesRead;
while (totalBytesRead < expectedBytes)
{
bytesRead = networkStream.Read(buffer, totalBytesRead, expectedBytes - totalBytesRead);
if (bytesRead == 0)
{
// Handle the case where the connection is closed or no more data is available
break;
}
totalBytesRead += bytesRead;
}In this code, expectedBytes is the total number of bytes you intend to read, and buffer is the array where the data will be stored. The loop continues until the total bytes read match expectedBytes. If bytesRead returns 0, it indicates that the end of the stream has been reached or the connection has been closed.
Microsoft Documentation:
The official Microsoft documentation for NetworkStream.Read provides detailed information on its behavior:
This documentation emphasizes that the Read method reads as much data as is available, up to the number of bytes specified by the count parameter.
Additional Considerations:
-
End of Stream: If the remote host shuts down the connection and all available data has been received, the
Readmethod completes immediately and returns zero bytes. -
Data Availability: Before reading, you can check if data is available using the
DataAvailableproperty ofNetworkStream.
By implementing a loop to handle partial reads, you can ensure that your application processes the complete data as intended.
@EvilBeaver, я проверил официальную документацию Microsoft, и оказывается, что NetworkStream.Read() может возвращать меньше байтов, чем запрошено.
Согласно официальной документации .NET:
- NetworkStream.Read() читает "столько данных, сколько доступно, вплоть до количества байтов, указанного параметром count"
- Метод блокируется до тех пор, пока не станет доступен хотя бы один байт, а не все запрошенные байты
- Если доступно только часть данных, метод возвращает именно эту часть
Документация Microsoft даже предоставляет пример кода для обработки частичных чтений:
int totalBytesRead = 0;
int bytesRead;
while (totalBytesRead < expectedBytes)
{
bytesRead = networkStream.Read(buffer, totalBytesRead, expectedBytes - totalBytesRead);
if (bytesRead == 0)
break; // Соединение закрыто
totalBytesRead += bytesRead;
}Таким образом, мое первоначальное предложение о добавлении проверки полного чтения буфера было корректным. NetworkStream ведет себя как любой другой Stream - может возвращать частичные чтения, и это нужно обрабатывать.
✏️ Learnings added
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1557
File: src/OneScript.DebugServices/Internal/DelayedConnectionChannel.cs:73-0
Timestamp: 2025-07-16T12:42:21.906Z
Learning: EvilBeaver had a misconception about NetworkStream.Read() behavior - he believed it blocks until all requested bytes are received, but actually it only blocks until at least one byte is available and can return partial reads like any other Stream.Read() method.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1555
File: Build.csproj:35-38
Timestamp: 2025-07-10T11:08:13.756Z
Learning: EvilBeaver prefers to see documentation links when technical claims are made about MSBuild syntax and property functions.
| public void SetClient(OneScriptDebuggerClient service) | ||
| { | ||
| _debugger = service; | ||
| ProtocolVersion = service.ProtocolVersion; | ||
| } |
There was a problem hiding this comment.
Potential issue with protocol version assignment.
The protocol version is set from the client after validation, but if the client has an invalid version, this could cause issues. Consider validating the client's protocol version before assignment.
public void SetClient(OneScriptDebuggerClient service)
{
_debugger = service;
- ProtocolVersion = service.ProtocolVersion;
+ ValidateProtocolVersion(service.ProtocolVersion);
+ _activeProtocolVersion = service.ProtocolVersion;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/VSCode.DebugAdapter/DebugeeProcess.cs around lines 138 to 142, the
ProtocolVersion is assigned directly from the client without validation. To fix
this, add a validation step to check if the client's ProtocolVersion is valid
before assigning it to the ProtocolVersion property. If invalid, handle the case
appropriately, such as throwing an exception or setting a default value.
| public void SetExceptionsBreakpoints((string Id, string Condition)[] filters) | ||
| { | ||
| if (ProtocolVersion > ProtocolVersions.Version1) | ||
| if (ProtocolVersion > ProtocolVersions.Version1 && ProtocolVersion < ProtocolVersions.Version3) | ||
| { | ||
| _debugger.SetMachineExceptionBreakpoints(filters); | ||
| } | ||
| else if (ProtocolVersion >= ProtocolVersions.Version3) | ||
| { | ||
| _debugger.SetExceptionBreakpoints(filters.Select(t => new ExceptionBreakpointFilter | ||
| { | ||
| Id = t.Id, | ||
| Condition = t.Condition | ||
| }).ToArray()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify version checking logic and handle all cases.
The current implementation doesn't handle UnknownVersion (0) and the logic could be clearer.
public void SetExceptionsBreakpoints((string Id, string Condition)[] filters)
{
- if (ProtocolVersion > ProtocolVersions.Version1 && ProtocolVersion < ProtocolVersions.Version3)
- {
- _debugger.SetMachineExceptionBreakpoints(filters);
- }
- else if (ProtocolVersion >= ProtocolVersions.Version3)
- {
- _debugger.SetExceptionBreakpoints(filters.Select(t => new ExceptionBreakpointFilter
- {
- Id = t.Id,
- Condition = t.Condition
- }).ToArray());
- }
+ switch (ProtocolVersion)
+ {
+ case ProtocolVersions.UnknownVersion:
+ case ProtocolVersions.Version1:
+ // Version 1 doesn't support exception breakpoints
+ Log.Warning("Exception breakpoints not supported in protocol version {Version}", ProtocolVersion);
+ break;
+ case ProtocolVersions.Version2:
+ _debugger.SetMachineExceptionBreakpoints(filters);
+ break;
+ default: // Version 3 and higher
+ _debugger.SetExceptionBreakpoints(filters.Select(t => new ExceptionBreakpointFilter
+ {
+ Id = t.Id,
+ Condition = t.Condition
+ }).ToArray());
+ break;
+ }
}📝 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.
| public void SetExceptionsBreakpoints((string Id, string Condition)[] filters) | |
| { | |
| if (ProtocolVersion > ProtocolVersions.Version1) | |
| if (ProtocolVersion > ProtocolVersions.Version1 && ProtocolVersion < ProtocolVersions.Version3) | |
| { | |
| _debugger.SetMachineExceptionBreakpoints(filters); | |
| } | |
| else if (ProtocolVersion >= ProtocolVersions.Version3) | |
| { | |
| _debugger.SetExceptionBreakpoints(filters.Select(t => new ExceptionBreakpointFilter | |
| { | |
| Id = t.Id, | |
| Condition = t.Condition | |
| }).ToArray()); | |
| } | |
| } | |
| public void SetExceptionsBreakpoints((string Id, string Condition)[] filters) | |
| { | |
| switch (ProtocolVersion) | |
| { | |
| case ProtocolVersions.UnknownVersion: | |
| case ProtocolVersions.Version1: | |
| // Version 1 doesn't support exception breakpoints | |
| Log.Warning("Exception breakpoints not supported in protocol version {Version}", ProtocolVersion); | |
| break; | |
| case ProtocolVersions.Version2: | |
| _debugger.SetMachineExceptionBreakpoints(filters); | |
| break; | |
| default: // Version 3 and higher | |
| _debugger.SetExceptionBreakpoints(filters.Select(t => new ExceptionBreakpointFilter | |
| { | |
| Id = t.Id, | |
| Condition = t.Condition | |
| }).ToArray()); | |
| break; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/VSCode.DebugAdapter/DebugeeProcess.cs around lines 236 to 250, the
version checking logic in SetExceptionsBreakpoints is incomplete and unclear,
missing handling for UnknownVersion (0). Refactor the conditionals to cover all
possible ProtocolVersion values explicitly, including UnknownVersion, and
simplify the logic by using clear and comprehensive if-else or switch statements
to ensure all cases are handled properly.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs (1)
60-124: Stream operations lack proper error handling.The stream read/write operations in the protocol negotiation could throw exceptions on network failures. The existing past review comment identified this issue and it remains unaddressed.
Consider wrapping critical stream operations in try-catch blocks:
private void ReconcileDataFormat(TcpClient client) { Log.Verbose("Sending reconcile message"); - var stream = client.GetStream(); - stream.Write(FormatReconcileUtils.FORMAT_RECONCILE_MAGIC, 0, FormatReconcileUtils.FORMAT_RECONCILE_MAGIC.Length); + try + { + var stream = client.GetStream(); + stream.Write(FormatReconcileUtils.FORMAT_RECONCILE_MAGIC, 0, FormatReconcileUtils.FORMAT_RECONCILE_MAGIC.Length); + } + catch (Exception ex) + { + Log.Error(ex, "Failed to send reconcile message"); + SelectSafestFormat(); + return; + }Similar protection should be added around the
BinaryReaderoperations at lines 88-100.src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs (1)
74-78: Replace Trace.Fail with proper exception handling.The
Trace.Failcall is meant for debugging and may not behave correctly in production environments. The past review comment identified this issue and it remains unaddressed.else { - readResult = null; - Trace.Fail($"Unexpected type {requiredType}"); + throw new JsonSerializationException($"Unexpected type {requiredType} for deserialization"); }
🧹 Nitpick comments (4)
src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs (3)
106-109: Use enum comparison instead of casting to int.The transport protocol comparison uses a cast to int which reduces type safety and readability.
- if (transport != (int)TransportProtocols.Json) + if ((TransportProtocols)transport != TransportProtocols.Json) { - throw new ApplicationException($"Transport protocol is out of range {transport}"); + throw new ApplicationException($"Unsupported transport protocol: {(TransportProtocols)transport}"); }
76-82: Replace Thread.Sleep with async/await pattern.Using
Thread.Sleepblocks the calling thread unnecessarily. Consider using async delays for better resource utilization.- while (client.Client.Available < requiredDataLength && attempts < WAIT_ATTEMPTS) - { - attempts++; - Log.Verbose("Attempt # {Attempt}", attempts); - Thread.Sleep(ATTEMPT_INTERVAL); - } + while (client.Client.Available < requiredDataLength && attempts < WAIT_ATTEMPTS) + { + attempts++; + Log.Verbose("Attempt # {Attempt}", attempts); + await Task.Delay(ATTEMPT_INTERVAL); + }Note: This would require making the method async and updating the calling code accordingly.
42-52: Add validation for invalid transport protocol.The switch statement handles
TransportProtocols.Invalidin the default case, but this could be more explicit.switch (_transport) { case TransportProtocols.Json: commandsChannel = new JsonDtoChannel(_tcpClient); break; case TransportProtocols.Binary: commandsChannel = new BinaryChannel(_tcpClient); break; + case TransportProtocols.Invalid: + throw new InvalidOperationException("Transport protocol was not properly negotiated"); default: throw new InvalidOperationException($"Unsupported transport protocol: {_transport}"); }src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs (1)
232-240: Simplify type validation logic.The type checking logic is overly complex and could be simplified for better maintainability.
AdvanceReader(reader); var value = serializer.Deserialize(reader, targetType); - if (value != null && value.GetType() != targetType) + if (value != null && !targetType.IsAssignableFrom(value.GetType())) { throw new JsonSerializationException($"Unexpected object type returned: {value.GetType()}, Expected {targetType}"); }This change allows for inheritance relationships while still validating type compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/OneScript.DebugProtocol/ExceptionBreakpointFilter.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/FormatReconcileUtils.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/JsonDtoChannel.cs(1 hunks)src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs(1 hunks)src/Tests/OneScript.DebugProtocol.Test/JsonChannelTest.cs(1 hunks)src/Tests/OneScript.DebugProtocol.Test/TestJsonSerializationForDto.cs(1 hunks)src/VSCode.DebugAdapter/OscriptDebugEventsListener.cs(1 hunks)src/VSCode.DebugAdapter/ThreadStateContainer.cs(1 hunks)src/VSCode.DebugAdapter/Transport/ConnectionFactory.cs(1 hunks)src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs(1 hunks)src/VSCode.DebugAdapter/Transport/TransportException.cs(1 hunks)src/VSCode.DebugAdapter/Transport/TransportProtocols.cs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/Tests/OneScript.DebugProtocol.Test/JsonChannelTest.cs
- src/VSCode.DebugAdapter/Transport/TransportProtocols.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- src/VSCode.DebugAdapter/Transport/TransportException.cs
- src/OneScript.DebugProtocol/ExceptionBreakpointFilter.cs
- src/VSCode.DebugAdapter/Transport/ConnectionFactory.cs
- src/Tests/OneScript.DebugProtocol.Test/TestJsonSerializationForDto.cs
- src/VSCode.DebugAdapter/ThreadStateContainer.cs
- src/VSCode.DebugAdapter/OscriptDebugEventsListener.cs
- src/OneScript.DebugProtocol/TcpServer/FormatReconcileUtils.cs
- src/OneScript.DebugProtocol/TcpServer/JsonDtoChannel.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1555
File: Build.csproj:35-38
Timestamp: 2025-07-10T11:08:13.756Z
Learning: EvilBeaver prefers to see documentation links when technical claims are made about MSBuild syntax and property functions.
src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs (1)
Learnt from: Mr-Rm
PR: EvilBeaver/OneScript#1447
File: src/ScriptEngine/Machine/ValueFactory.cs:102-104
Timestamp: 2024-08-29T06:48:46.675Z
Learning: When performing operations that involve type conversion, ensure that the conversion function (e.g., `AsNumber`) handles invalid cases by throwing an appropriate exception, rather than adding redundant type checks in the calling method.
| private RpcCall ReadRpcCall(JsonReader reader, JsonSerializer serializer) | ||
| { | ||
| var value = new RpcCall(); | ||
| while (reader.TokenType != JsonToken.EndObject) | ||
| { | ||
| CheckExpectedToken(reader, JsonToken.PropertyName, "ReadRpcCall"); | ||
|
|
||
| var propName = (string)reader.Value; | ||
| AdvanceReader(reader); | ||
| switch (propName) | ||
| { | ||
| case nameof(RpcCall.Id): | ||
| value.Id = serializer.Deserialize<string>(reader); | ||
| break; | ||
| case nameof(RpcCall.ServiceName): | ||
| value.ServiceName = serializer.Deserialize<string>(reader); | ||
| break; | ||
| case nameof(RpcCall.Parameters): | ||
| value.Parameters = DeserializeTypedArray(reader, serializer); | ||
| break; | ||
| } | ||
| AdvanceReader(reader); | ||
| } | ||
|
|
||
| return value; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for missing required properties.
The ReadRpcCall method doesn't validate that all required properties are present, which could lead to incomplete objects being created.
private RpcCall ReadRpcCall(JsonReader reader, JsonSerializer serializer)
{
var value = new RpcCall();
+ bool hasId = false, hasServiceName = false;
+
while (reader.TokenType != JsonToken.EndObject)
{
CheckExpectedToken(reader, JsonToken.PropertyName, "ReadRpcCall");
var propName = (string)reader.Value;
AdvanceReader(reader);
switch (propName)
{
case nameof(RpcCall.Id):
value.Id = serializer.Deserialize<string>(reader);
+ hasId = true;
break;
case nameof(RpcCall.ServiceName):
value.ServiceName = serializer.Deserialize<string>(reader);
+ hasServiceName = true;
break;
case nameof(RpcCall.Parameters):
value.Parameters = DeserializeTypedArray(reader, serializer);
break;
}
AdvanceReader(reader);
}
+
+ if (!hasId || !hasServiceName)
+ {
+ throw new JsonSerializationException("Missing required properties in RpcCall");
+ }
return value;
}Apply similar validation to ReadRpcCallResult.
📝 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.
| private RpcCall ReadRpcCall(JsonReader reader, JsonSerializer serializer) | |
| { | |
| var value = new RpcCall(); | |
| while (reader.TokenType != JsonToken.EndObject) | |
| { | |
| CheckExpectedToken(reader, JsonToken.PropertyName, "ReadRpcCall"); | |
| var propName = (string)reader.Value; | |
| AdvanceReader(reader); | |
| switch (propName) | |
| { | |
| case nameof(RpcCall.Id): | |
| value.Id = serializer.Deserialize<string>(reader); | |
| break; | |
| case nameof(RpcCall.ServiceName): | |
| value.ServiceName = serializer.Deserialize<string>(reader); | |
| break; | |
| case nameof(RpcCall.Parameters): | |
| value.Parameters = DeserializeTypedArray(reader, serializer); | |
| break; | |
| } | |
| AdvanceReader(reader); | |
| } | |
| return value; | |
| } | |
| private RpcCall ReadRpcCall(JsonReader reader, JsonSerializer serializer) | |
| { | |
| var value = new RpcCall(); | |
| bool hasId = false, hasServiceName = false; | |
| while (reader.TokenType != JsonToken.EndObject) | |
| { | |
| CheckExpectedToken(reader, JsonToken.PropertyName, "ReadRpcCall"); | |
| var propName = (string)reader.Value; | |
| AdvanceReader(reader); | |
| switch (propName) | |
| { | |
| case nameof(RpcCall.Id): | |
| value.Id = serializer.Deserialize<string>(reader); | |
| hasId = true; | |
| break; | |
| case nameof(RpcCall.ServiceName): | |
| value.ServiceName = serializer.Deserialize<string>(reader); | |
| hasServiceName = true; | |
| break; | |
| case nameof(RpcCall.Parameters): | |
| value.Parameters = DeserializeTypedArray(reader, serializer); | |
| break; | |
| } | |
| AdvanceReader(reader); | |
| } | |
| if (!hasId || !hasServiceName) | |
| { | |
| throw new JsonSerializationException("Missing required properties in RpcCall"); | |
| } | |
| return value; | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs between lines 83
and 108, the ReadRpcCall method lacks validation to ensure all required
properties are present before returning the RpcCall object. To fix this, add
checks after the while loop to verify that essential properties like Id and
ServiceName are not null or missing, and throw an appropriate exception if any
required property is absent. Apply a similar validation approach to the
ReadRpcCallResult method to ensure completeness of deserialized objects.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs (5)
194-198: Use more robust array type detection.The current array detection using
EndsWith("[]")is fragile and could incorrectly identify types that legitimately end with "[]".- if (typeName.EndsWith("[]", StringComparison.Ordinal)) - { - isArray = true; - typeName = typeName.Substring(0, typeName.Length - 2); - } + if (typeName.EndsWith("[]", StringComparison.Ordinal)) + { + isArray = true; + typeName = typeName.Substring(0, typeName.Length - 2); + }Note: The fix for duplicate variable declaration must be applied first, then consider a more robust array type validation approach.
75-78: Replace Trace.Fail with proper exception handling.
Trace.Failis meant for debugging and may not behave correctly in production environments. Based on the retrieved learnings, EvilBeaver prefers fail-fast error handling in debugging contexts.- else - { - readResult = null; - Trace.Fail($"Unexpected type {requiredType}"); - } + else + { + throw new JsonSerializationException($"Unexpected type {requiredType} for deserialization"); + }
83-108: Add validation for missing required properties.The method doesn't validate that essential properties like
IdandServiceNameare present, which could lead to incomplete objects being created.private RpcCall ReadRpcCall(JsonReader reader, JsonSerializer serializer) { var value = new RpcCall(); + bool hasId = false, hasServiceName = false; + while (reader.TokenType != JsonToken.EndObject) { CheckExpectedToken(reader, JsonToken.PropertyName, "ReadRpcCall"); var propName = (string)reader.Value; AdvanceReader(reader); switch (propName) { case nameof(RpcCall.Id): value.Id = serializer.Deserialize<string>(reader); + hasId = true; break; case nameof(RpcCall.ServiceName): value.ServiceName = serializer.Deserialize<string>(reader); + hasServiceName = true; break; case nameof(RpcCall.Parameters): value.Parameters = DeserializeTypedArray(reader, serializer); break; } AdvanceReader(reader); } + + if (!hasId || !hasServiceName) + { + throw new JsonSerializationException("Missing required properties in RpcCall"); + } return value; }
295-296: Handle array types properly in type name serialization.The
WriteTypedValuemethod usesGetType().Namewhich won't properly represent array types in the format expected by the deserialization logic.- writer.WritePropertyName(TYPE_PROPERTY_NAME); - writer.WriteValue(valueToWrite.GetType().Name); + writer.WritePropertyName(TYPE_PROPERTY_NAME); + var typeName = valueToWrite.GetType().IsArray + ? $"{valueToWrite.GetType().GetElementType().Name}[]" + : valueToWrite.GetType().Name; + writer.WriteValue(typeName);
110-135: Add validation for missing required properties in ReadRpcCallResult.Similar to
ReadRpcCall, this method should validate that essential properties are present.private RpcCallResult ReadRpcCallResult(JsonReader reader, JsonSerializer serializer) { var value = new RpcCallResult(); + bool hasId = false, hasServiceName = false; + while (reader.TokenType != JsonToken.EndObject) { CheckExpectedToken(reader, JsonToken.PropertyName, "ReadRpcCallResult"); var propName = (string)reader.Value; AdvanceReader(reader); switch (propName) { case nameof(RpcCallResult.Id): value.Id = serializer.Deserialize<string>(reader); + hasId = true; break; case nameof(RpcCallResult.ServiceName): value.ServiceName = serializer.Deserialize<string>(reader); + hasServiceName = true; break; case nameof(RpcCallResult.ReturnValue): value.ReturnValue = ReadTypedValue(reader, serializer); break; } AdvanceReader(reader); } + + if (!hasId || !hasServiceName) + { + throw new JsonSerializationException("Missing required properties in RpcCallResult"); + } return value; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1557
File: src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs:60-0
Timestamp: 2025-07-16T12:44:12.092Z
Learning: EvilBeaver prefers fail-fast error handling in debugging contexts - when stream operations fail in debug client factories, exceptions should be thrown to terminate the process rather than handled gracefully with fallback mechanisms.
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1555
File: Build.csproj:35-38
Timestamp: 2025-07-10T11:08:13.756Z
Learning: EvilBeaver prefers to see documentation links when technical claims are made about MSBuild syntax and property functions.
src/OneScript.DebugProtocol/TcpServer/JsonDtoConverter.cs (2)
Learnt from: Mr-Rm
PR: EvilBeaver/OneScript#1447
File: src/ScriptEngine/Machine/ValueFactory.cs:102-104
Timestamp: 2024-08-29T06:48:46.675Z
Learning: When performing operations that involve type conversion, ensure that the conversion function (e.g., `AsNumber`) handles invalid cases by throwing an appropriate exception, rather than adding redundant type checks in the calling method.
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1557
File: src/VSCode.DebugAdapter/Transport/DebugClientFactory.cs:60-0
Timestamp: 2025-07-16T12:44:12.092Z
Learning: EvilBeaver prefers fail-fast error handling in debugging contexts - when stream operations fail in debug client factories, exceptions should be thrown to terminate the process rather than handled gracefully with fallback mechanisms.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Breaking Changes
Tests
Chores