Actionable comments posted: 1

🧹 Nitpick comments (1)

src/VSCode.DebugAdapter/OscriptDebugSession.cs (1)

266-273: Remove unused method.

The NormalizeDriveLetter method is no longer called after commenting out line 228. Dead code increases maintenance burden and creates confusion.

Apply this diff to remove the unused method:

-    private string NormalizeDriveLetter(string path)
-    {
-        if (Path.IsPathRooted(path))
-            return path[0].ToString().ToUpperInvariant() + path.Substring(1);
-        else
-            return path;
-
-    }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50e1f14 and 5897c42.

📒 Files selected for processing (1)

• src/VSCode.DebugAdapter/OscriptDebugSession.cs (1 hunks)

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/VSCode.DebugAdapter/package.json (1)

89-92: Reconsider using empty strings as default values.

The default environment variables are set to empty strings "", which will pass the !string.IsNullOrEmpty() check in PathHandlingStrategy.cs but provide no meaningful remapping. This could cause unnecessary string operations and potential confusion for users.

Consider one of these approaches:

1. Remove the defaults entirely - Let users add these variables only when needed:

-                "default": {
-                  "OSCRIPT_DEBUGWORKSPACE_HOST": "",
-                  "OSCRIPT_DEBUGWORKSPACE_REMOTE": ""
-                }
+                "default": {}

2. Use null or undefined - But this might not be supported by the VSCode launch configuration schema.

3. Update the C# code to explicitly check for empty strings:

if (!string.IsNullOrWhiteSpace(hostWorkspace) && !string.IsNullOrWhiteSpace(remoteWorkspace))

Approach 1 is recommended as it keeps the configuration clean and only activates remapping when explicitly configured by the user.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5897c42 and ed6cdae.

📒 Files selected for processing (2)

• src/VSCode.DebugAdapter/PathHandlingStrategy.cs (1 hunks)
• src/VSCode.DebugAdapter/package.json (1 hunks)

🧰 Additional context used 🧬 Code graph analysis (1) src/VSCode.DebugAdapter/PathHandlingStrategy.cs (2)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

• ConvertClientPathToDebugger (114-117)

src/VSCode.DebugAdapter/DebugSession.cs (1)

• ConvertClientPathToDebugger (449-452)

🔇 Additional comments (3)

src/VSCode.DebugAdapter/PathHandlingStrategy.cs (2)

80-80: Consider cross-platform path comparison semantics.

The case-insensitive comparison using StringComparison.OrdinalIgnoreCase is appropriate for Windows but may cause issues in cross-platform scenarios where the host or remote workspace is on a case-sensitive file system (Linux, macOS).

Verify the expected deployment scenarios:

• Will this code run exclusively on Windows hosts?
• Could the remote workspace be on a case-sensitive file system?

If cross-platform support is needed, consider using Path.DirectorySeparatorChar and platform-appropriate comparison methods, or document that this feature is Windows-specific.

---

93-114: LGTM: Existing URI/path conversion logic preserved correctly.

The pre-existing URI/path conversion logic has been preserved and correctly integrated as a fallback after the new workspace remapping logic. The flow is appropriate: attempt remapping first, then fall back to the standard URI/path handling.

src/VSCode.DebugAdapter/package.json (1)

186-186: Verify consistency with AI summary.

The AI summary states that the oscript.web launcher's env.default was changed from null to include the workspace environment variables, but the annotated code shows line 186 as unchanged ("default": null).

Please verify:

• Should the oscript.web configuration (line 186) also have the same env defaults as the oscript configuration (lines 89-92)?
• Or is the AI summary incorrect and only the oscript launcher should have these defaults?

If both launchers should support workspace remapping, apply this diff:

                 "default": null
+                "default": {
+                  "OSCRIPT_DEBUGWORKSPACE_HOST": "",
+                  "OSCRIPT_DEBUGWORKSPACE_REMOTE": ""
+                }

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/VSCode.DebugAdapter/PathHandlingStrategy.cs (2)

72-75: Consider caching environment variables for performance.

Reading environment variables on every path conversion may be inefficient if this method is called frequently (e.g., during bulk breakpoint operations). Consider reading them once during initialization and storing as instance fields.

Example refactor:

+    private readonly string _hostWorkspace;
+    private readonly string _remoteWorkspace;
+
+    public PathHandlingStrategy()
+    {
+        _hostWorkspace = Environment.GetEnvironmentVariable("OSCRIPT_DEBUGWORKSPACE_HOST");
+        _remoteWorkspace = Environment.GetEnvironmentVariable("OSCRIPT_DEBUGWORKSPACE_REMOTE");
+    }
+
     public string ConvertClientPathToDebugger(string clientPath)
     {
         if (clientPath == null) {
             return null;
         }

-        string hostWorkspace = Environment.GetEnvironmentVariable("OSCRIPT_DEBUGWORKSPACE_HOST");
-        string remoteWorkspace = Environment.GetEnvironmentVariable("OSCRIPT_DEBUGWORKSPACE_REMOTE");
+        string hostWorkspace = _hostWorkspace;
+        string remoteWorkspace = _remoteWorkspace;

---

93-101: Consider using the Serilog logger instead of Console.Error.

While Console.Error works for debugging, the codebase already uses Serilog (mentioned in library context). Using a proper logger would provide better control over log levels, formatting, and destinations.

Example refactor:

// Add field at class level
private static readonly ILogger Logger = Log.ForContext<PathHandlingStrategy>();

// Replace Console.Error.WriteLine
Logger.Debug("Path mapped: {ClientPath} -> {Result}", clientPath, result);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed6cdae and b71749d.

📒 Files selected for processing (2)

• src/VSCode.DebugAdapter/PathHandlingStrategy.cs (1 hunks)
• src/VSCode.DebugAdapter/package.json (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/VSCode.DebugAdapter/package.json

🧰 Additional context used 🧬 Code graph analysis (1) src/VSCode.DebugAdapter/PathHandlingStrategy.cs (2)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

• ConvertClientPathToDebugger (114-117)

src/VSCode.DebugAdapter/DebugSession.cs (1)

• ConvertClientPathToDebugger (449-452)

🔇 Additional comments (4)

src/VSCode.DebugAdapter/PathHandlingStrategy.cs (4)

68-70: Good defensive check.

The null guard prevents downstream NullReferenceException and is a solid improvement.

---

103-116: Good fallback logic with helpful diagnostics.

The conditional logging clearly explains why path mapping is skipped and gracefully falls back to the existing conversion logic, preserving backward compatibility.

---

118-139: Existing URI conversion logic correctly preserved.

The fallback URI/path conversion maintains backward compatibility and includes appropriate error handling for malformed URIs.

---

75-91: No path validation changes needed—existing null/empty checks are sufficient.

The code already guards against invalid inputs via IsNullOrWhiteSpace() checks at line 75, preventing null, empty, or whitespace-only strings from entering the code block. All subsequent string operations (Replace, Trim, StartsWith, Substring) are safe on non-null strings and will not throw exceptions. The Substring call is guarded by a preceding StartsWith check. Existing error logging at lines 61-67 handles missing environment variables adequately. Adding Path.IsPathRooted() validation would be speculative and potentially break legitimate URI or UNC path formats used by these environment variables.

Likely an incorrect or invalid review comment.

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

94-102: Fallback to current process creates misleading debug session behavior.

When the target process isn't running, falling back to Process.GetCurrentProcess() ties the debug session to the adapter's lifetime rather than the actual debuggee. This causes several problems:

1. The Exited event subscription (line 106) will fire when the adapter exits, not when the remote process exits
2. Users won't know they're attached to the wrong process (no logging or error)
3. The exception message check is fragile and may fail with localized .NET or different versions

This approach was questioned in past review comments. Consider alternative solutions:

Option 1: If the process truly doesn't need to exist (remote attach scenario), set _process = null and guard all process access with null checks.

Option 2: Use a "no-process" state as suggested in past review comments, where the Exited event isn't monitored at all.

Apply this diff for Option 1:

 public void InitAttached()
 {
     var pid = _debugger.GetProcessId();
     
     try
     {
         _process = Process.GetProcessById(pid);
+        _attachMode = true;
+        _process.EnableRaisingEvents = true;
+        _process.Exited += Process_Exited;
+        Log.Information("Attached to process {Pid}", pid);
     }
-    catch (ArgumentException ex) when (ex.Message.Contains("is not running"))
+    catch (ArgumentException ex)
     {
-        _process = Process.GetCurrentProcess();
+        Log.Warning("Target process {Pid} is not running locally. Debug session will not monitor process lifetime.", pid);
+        _process = null;
+        _attachMode = true;
     }
-    
-    _attachMode = true;
-    _process.EnableRaisingEvents = true;
-    _process.Exited += Process_Exited;
-
 }

And guard process access:

-public bool HasExited => _process?.HasExited ?? true;
+public bool HasExited => _process?.HasExited ?? false;

🧹 Nitpick comments (1)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

62-64: Consider adding XML documentation for workspace properties.

These properties support remote debugging path mapping but lack documentation explaining their purpose and expected format.

Apply this diff to add documentation:

+/// <summary>
+/// Local workspace path on the host (client) machine for path mapping during remote debugging.
+/// </summary>
 public string HostWorkspace { get; set; }

+/// <summary>
+/// Remote workspace path on the debuggee machine for path mapping during remote debugging.
+/// </summary>
 public string RemoteWorkspace { get; set; }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b71749d and 716451b.

📒 Files selected for processing (4)

• src/VSCode.DebugAdapter/DebugeeProcess.cs (2 hunks)
• src/VSCode.DebugAdapter/OscriptDebugSession.cs (2 hunks)
• src/VSCode.DebugAdapter/PathHandlingStrategy.cs (1 hunks)
• src/VSCode.DebugAdapter/package.json (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/VSCode.DebugAdapter/PathHandlingStrategy.cs
• src/VSCode.DebugAdapter/package.json

🧰 Additional context used 🧬 Code graph analysis (2) src/VSCode.DebugAdapter/OscriptDebugSession.cs (2)

src/OneScriptDocumenter/ConsoleLogger.cs (1)

• Error (25-29)

src/oscript/Output.cs (2)

• WriteLine (48-52)
• WriteLine (54-57)

src/VSCode.DebugAdapter/DebugeeProcess.cs (3)

src/ScriptEngine.HostedScript/HostedScriptEngine.cs (2)

• Process (93-118)
• Process (143-149)

src/VSCode.DebugAdapter/ConsoleProcess.cs (1)

• Process (105-130)

src/VSCode.DebugAdapter/ServerProcess.cs (1)

• Process (30-53)

🔇 Additional comments (2)

src/VSCode.DebugAdapter/OscriptDebugSession.cs (2)

141-143: LGTM - Workspace properties populated correctly.

The workspace paths are extracted from attach arguments with sensible defaults, enabling optional path mapping for remote debugging scenarios.

---

276-313: Verify path mapping works correctly across platforms.

The workspace mapping logic assumes Windows paths for the host (using backslashes) and Unix paths for remote (using forward slashes). However, the host might be Linux or the remote might be Windows. The current implementation may not handle all cross-platform scenarios correctly.

Run the following script to search for platform detection patterns and verify how paths are handled elsewhere:

Actionable comments posted: 4

🧹 Nitpick comments (1)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

62-64: Consider adding validation for workspace paths.

These properties lack input validation and documentation. Consider adding XML documentation to explain their purpose and adding validation to ensure they represent valid paths.

Apply this diff to add validation:

+/// <summary>
+/// Gets or sets the workspace path on the host (client) machine.
+/// Used for path mapping when debugging remote processes.
+/// </summary>
 public string HostWorkspace { get; set; }

+/// <summary>
+/// Gets or sets the workspace path on the remote (debuggee) machine.
+/// Used for path mapping when debugging remote processes.
+/// </summary>
 public string RemoteWorkspace { get; set; }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 46d1f63 and c12dc8d.

📒 Files selected for processing (2)

• src/VSCode.DebugAdapter/DebugeeProcess.cs (5 hunks)
• src/VSCode.DebugAdapter/OscriptDebugSession.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/VSCode.DebugAdapter/OscriptDebugSession.cs

🧰 Additional context used 🧬 Code graph analysis (1) src/VSCode.DebugAdapter/DebugeeProcess.cs (3)

src/VSCode.DebugAdapter/ServerProcess.cs (1)

• Process (30-53)

src/VSCode.DebugAdapter/ConsoleProcess.cs (1)

• Process (105-130)

src/VSCode.DebugAdapter/DebugSession.cs (4)

• Source (93-114)
• Source (103-107)
• Source (109-113)
• Source (494-497)

Actionable comments posted: 1

♻️ Duplicate comments (4)

src/VSCode.DebugAdapter/DebugeeProcess.cs (4)

201-224: Path mapping helper works but remains stringy and platform-assuming

ConvertRemotePath covers the basic cases (null/empty guarding, avoiding double-conversion, simple prefix-based mapping), but it still has the same general weaknesses raised in the earlier review:

• Manual slash replacement and prefix handling instead of using System.IO.Path make subtle edge cases (relative segments, redundant separators, casing) easier to get wrong.
• Case-sensitivity is inferred solely from the host OS; if the remote workspace runs on a different platform, the StartsWith comparisons may not reflect its semantics.
• Building the result via toPrefix.TrimEnd('/', '\\') + "/" + relativePath can mix separator styles (e.g., Windows-style prefix plus /), which works in many cases but is brittle.

Given this method is now the central place for host⇄remote mapping, it would be more robust to:

• Normalize path and fromPrefix with Path.GetFullPath (where applicable) and Path.DirectorySeparatorChar / Path.AltDirectorySeparatorChar.
• Use a consistent comparison strategy (StringComparison.Ordinal vs OrdinalIgnoreCase) that either:
  • Detects case-sensitivity based on a configuration flag, or
  • At least isolates “host comparison” and “already in toPrefix” checks using separately normalized prefixes.
• Construct result with Path.Combine (or an equivalent that enforces the intended separator style for the remote side) instead of raw string concatenation.

This is not a blocker, but tightening it now will reduce future surprises when users run mixed Windows/Linux setups or configure slightly different workspace strings.

---

299-311: Avoid mutating incoming breakpoint objects in-place when remapping paths

The remapping logic itself (HostWorkspace → RemoteWorkspace before sending to the debugger) is reasonable, but SetBreakpoints currently mutates the Breakpoint instances supplied by the caller:

var breakpointsArray = breakpoints.ToArray();

for (int i = 0; i < breakpointsArray.Length; i++)
{
    breakpointsArray[i].Source = ConvertRemotePath(breakpointsArray[i].Source, HostWorkspace, RemoteWorkspace);
}

If the caller expects to retain the original Source values (e.g., for UI display, caching, or re-sending), this in-place rewrite can be surprising and hard to trace. The previous review already pointed this out; the concern is still valid.

If the Breakpoint type allows it, consider mapping to new instances instead of mutating the originals, for example:

-        var breakpointsArray = breakpoints.ToArray();
-
-        for (int i = 0; i < breakpointsArray.Length; i++)
-        {
-            breakpointsArray[i].Source = ConvertRemotePath(breakpointsArray[i].Source, HostWorkspace, RemoteWorkspace);
-        }
-
-        var confirmedBreaks = _debugger.SetMachineBreakpoints(breakpointsArray);
+        var breakpointsArray = breakpoints.ToArray();
+
+        var remapped = breakpointsArray
+            .Select(bp => new Breakpoint
+            {
+                // copy all relevant fields from bp
+                Line = bp.Line,
+                Source = ConvertRemotePath(bp.Source, HostWorkspace, RemoteWorkspace),
+                Condition = bp.Condition,
+                HitCondition = bp.HitCondition,
+                LogMessage = bp.LogMessage
+                // …any other properties that exist on Breakpoint
+            })
+            .ToArray();
+
+        var confirmedBreaks = _debugger.SetMachineBreakpoints(remapped);

This keeps the transformation local to the adapter while leaving the caller’s objects untouched.

---

318-337: Stack frame mutation for path remapping can also be made immutable

Similarly to breakpoints, GetStackTrace currently mutates the frames returned from _debugger:

for (int i = firstFrameIdx; i < limit && i < allFrames.Length; i++)
{
    allFrames[i].ThreadId = threadId;
    allFrames[i].Source = ConvertRemotePath(allFrames[i].Source, RemoteWorkspace, HostWorkspace);
    result.Add(allFrames[i]);
}

If the debugger service caches or reuses these StackFrame instances, changing ThreadId and Source in-place may have side effects elsewhere. The previous review suggested using an immutable transformation; that feedback still applies.

A safer pattern is to create new frames for the DAP side, copying fields from the originals and only replacing Source and ThreadId:

-        for (int i = firstFrameIdx; i < limit && i < allFrames.Length; i++)
-        {
-            allFrames[i].ThreadId = threadId;
-            allFrames[i].Source = ConvertRemotePath(allFrames[i].Source, RemoteWorkspace, HostWorkspace);
-            result.Add(allFrames[i]);
-        }
+        for (int i = firstFrameIdx; i < limit && i < allFrames.Length; i++)
+        {
+            var frame = allFrames[i];
+
+            var mappedSource = ConvertRemotePath(frame.Source, RemoteWorkspace, HostWorkspace);
+
+            result.Add(new StackFrame(
+                frame.Index,
+                frame.MethodName,
+                mappedSource,
+                frame.Line,
+                frame.Column)
+            {
+                ThreadId = threadId
+                // copy any additional mutable properties if the type exposes them
+            });
+        }

Adjust the constructor/fields to match the actual OneScript.DebugProtocol.StackFrame API, but the idea is to keep the adapter’s mapping logic from mutating shared state.

---

91-107: Attach fallback to Process.GetCurrentProcess() is still unsafe and catch-all is too broad

The concerns from the earlier review about InitAttached still apply:

• Falling back to Process.GetCurrentProcess() means _process refers to the adapter itself, so operations like Kill() could terminate the adapter instead of the debuggee, and lifetime tracking is misleading.
• A bare catch will hide unrelated failures (permission issues, API misuse, etc.) and make debugging harder; it also doesn’t log that attach degraded into a “no local process” mode.

A safer pattern is to treat “process not found locally” as “no process to track” rather than substituting the adapter process, and to only catch the expected exception type:

        public void InitAttached()
        {
            var pid = _debugger.GetProcessId();

-            try
-            {
-                _process = Process.GetProcessById(pid);
-            }
-            catch
-            {
-                _process = Process.GetCurrentProcess();
-            }
-
-            _attachMode = true;
-            _process.EnableRaisingEvents = true;
-            _process.Exited += Process_Exited;
+            try
+            {
+                _process = Process.GetProcessById(pid);
+                _process.EnableRaisingEvents = true;
+                _process.Exited += Process_Exited;
+            }
+            catch (ArgumentException ex)
+            {
+                // Process with this pid is not available on the local machine
+                Log.Information(ex, "Process with id {Pid} not found locally, attaching without local process tracking", pid);
+                _process = null;
+            }
+
+            _attachMode = true;

With this approach, _attachMode is still set, but there is no local process object, and process-exit events are only subscribed when a real process is present. If you adopt this, you’ll also want the Kill() method and any other _process usages to tolerate _process == null.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c12dc8d and 37c237d.

📒 Files selected for processing (1)

• src/VSCode.DebugAdapter/DebugeeProcess.cs (12 hunks)

🧰 Additional context used 🧬 Code graph analysis (1) src/VSCode.DebugAdapter/DebugeeProcess.cs (3)

src/OneScript.DebugProtocol/IDebuggerService.cs (3)

• GetProcessId (88-88)
• Execute (21-21)
• StackFrame (46-46)

src/VSCode.DebugAdapter/ConsoleProcess.cs (2)

• Process (105-130)
• InitInternal (35-103)

src/VSCode.DebugAdapter/DebugSession.cs (6)

• Source (93-114)
• Source (103-107)
• Source (109-113)
• Source (494-497)
• StackFrame (32-47)
• StackFrame (40-46)

🔇 Additional comments (2)

src/VSCode.DebugAdapter/DebugeeProcess.cs (2)

62-65: Host/remote workspace properties look reasonable

The explicit HostWorkspace / RemoteWorkspace properties make the mapping intent clear and keep the state on the debuggee side simple. No issues from my side here.

---

135-147: Encoding selection + logging is clear

The refactoring to resolve _dapEncoding from options or a sensible default and then log the chosen value is straightforward and helpful for diagnostics. No functional issues noticed.

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/VSCode.DebugAdapter/WorkspaceMapper.cs (2)

18-22: Fix inconsistent indentation (tabs vs spaces).

Lines 20-21 use tabs while the rest of the codebase uses spaces. Line 19 contains only whitespace and should be removed.

Apply this diff:

 public WorkspaceMapper(string localPath, string remotePath) {
-            
-		this._localWorkspace = new Workspace(localPath);
-		this._remoteWorkspace = new Workspace(remotePath);
-	}
+            this._localWorkspace = new Workspace(localPath);
+            this._remoteWorkspace = new Workspace(remotePath);
+        }

---

48-50: Consider adding a comment to clarify the early-return logic.

The condition on lines 48-49 implements two optimizations:

1. Skip conversion if path already starts with destination prefix (already converted)
2. Skip conversion if path doesn't start with source prefix (not in source workspace)

While functionally correct, this double-condition could benefit from a brief comment explaining the intent.

Apply this diff:

+    // Skip if already in target format or not in source workspace
     if (normalizedPath.StartsWith(toPrefix.Normalized, comparison) ||
         !normalizedPath.StartsWith(normalizedFrom, comparison))
         return path;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37c237d and 1bc5e29.

📒 Files selected for processing (5)

• src/VSCode.DebugAdapter/DebugeeProcess.cs (11 hunks)
• src/VSCode.DebugAdapter/OscriptDebugSession.cs (2 hunks)
• src/VSCode.DebugAdapter/PathHandlingStrategy.cs (1 hunks)
• src/VSCode.DebugAdapter/WorkspaceMapper.cs (1 hunks)
• src/VSCode.DebugAdapter/package.json (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• src/VSCode.DebugAdapter/PathHandlingStrategy.cs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/VSCode.DebugAdapter/package.json
• src/VSCode.DebugAdapter/DebugeeProcess.cs

🧰 Additional context used 🧬 Code graph analysis (1) src/VSCode.DebugAdapter/OscriptDebugSession.cs (1)

src/VSCode.DebugAdapter/WorkspaceMapper.cs (2)

• WorkspaceMapper (12-59)
• WorkspaceMapper (18-22)

🔇 Additional comments (3)

src/VSCode.DebugAdapter/OscriptDebugSession.cs (1)

40-52: LGTM! Path mapper initialization is safe and handles missing configuration.

The code correctly handles the optional pathsMapping configuration using GetFromContainer with safe defaults. Creating a WorkspaceMapper with empty strings when no mapping is provided is safe, as ConvertPath returns paths unchanged when workspace paths are empty.

src/VSCode.DebugAdapter/WorkspaceMapper.cs (2)

24-32: LGTM! Clean bidirectional mapping API.

The LocalToRemote and RemoteToLocal methods provide a clear, intuitive interface for path conversion by appropriately swapping workspace parameters.

---

61-72: LGTM! Clean helper class for path normalization.

The Workspace helper class appropriately stores both original and normalized path representations, making conversions more efficient.

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f659e9 and fc7e717.

📒 Files selected for processing (3)

• src/VSCode.DebugAdapter/DebugeeProcess.cs (11 hunks)
• src/VSCode.DebugAdapter/OscriptDebugSession.cs (2 hunks)
• src/VSCode.DebugAdapter/WorkspaceMapper.cs (1 hunks)

🧰 Additional context used 🧬 Code graph analysis (2) src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

src/VSCode.DebugAdapter/WorkspaceMapper.cs (5)

• WorkspaceMapper (13-74)
• WorkspaceMapper (31-31)
• WorkspaceMapper (33-37)
• LocalToRemote (39-42)
• RemoteToLocal (44-47)

src/VSCode.DebugAdapter/OscriptDebugSession.cs (1)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

• InitPathsMapper (113-125)

🔇 Additional comments (2)

src/VSCode.DebugAdapter/OscriptDebugSession.cs (1)

141-141: LGTM - Formatting improvement.

The added blank line improves code readability.

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

45-46: LGTM - Null safety improvements.

The addition of null-conditional operators and null checks for _process properly addresses the null reference concerns raised in previous reviews. The ExitCode property now safely returns 0 when the process is null, and the Kill method guards against null process references.

Also applies to: 260-261

Actionable comments posted: 3

♻️ Duplicate comments (1)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

289-301: In-place mutation of breakpoint and stack frame objects (optional refactor).

SetBreakpoints and GetStackTrace both mutate protocol DTOs in-place:

breakpointsArray[i].Source = PathsMapper.LocalToRemote(...);
...
allFrames[i].ThreadId = threadId;
allFrames[i].Source = PathsMapper.RemoteToLocal(...);
result.Add(allFrames[i]);

If the underlying debugger client caches or reuses these objects, in-place mutation can be surprising and may leak mapped paths into other contexts.

It’s not a correctness bug given the current usage, but you could make this safer by creating new instances (or shallow clones) with updated Source / ThreadId instead of mutating the originals before passing them on. A previous review already touched on this pattern for stack frames; the same consideration applies here.

Also applies to: 310-331

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc7e717 and 00a373d.

📒 Files selected for processing (2)

• src/VSCode.DebugAdapter/DebugeeProcess.cs (11 hunks)
• src/VSCode.DebugAdapter/OscriptDebugSession.cs (2 hunks)

🧰 Additional context used 🧬 Code graph analysis (2) src/VSCode.DebugAdapter/DebugeeProcess.cs (3)

src/VSCode.DebugAdapter/WorkspaceMapper.cs (5)

• WorkspaceMapper (13-74)
• WorkspaceMapper (31-31)
• WorkspaceMapper (33-37)
• LocalToRemote (39-42)
• RemoteToLocal (44-47)

src/VSCode.DebugAdapter/DebugeeOutputEventArgs.cs (2)

• DebugeeOutputEventArgs (12-22)
• DebugeeOutputEventArgs (14-18)

src/VSCode.DebugAdapter/DebugSession.cs (6)

• Source (93-114)
• Source (103-107)
• Source (109-113)
• Source (494-497)
• StackFrame (32-47)
• StackFrame (40-46)

src/VSCode.DebugAdapter/OscriptDebugSession.cs (2)

src/VSCode.DebugAdapter/DebugeeFactory.cs (1)

• DebugeeFactory (12-28)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

• InitPathsMapper (113-125)

🔇 Additional comments (2)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

223-225: Process termination and Kill() null-guards look good.

• HandleDisconnect now checks _process != null and _process.HasExited before waiting/killing, which avoids NullReferenceException when attach runs without a local process.
• Public Kill() now safely no-ops when _process is null and still waits for exit when it exists.

These changes align well with the new attach behaviour and make process lifetime management safer.

Also applies to: 241-253, 258-265

src/VSCode.DebugAdapter/OscriptDebugSession.cs (1)

33-40: Debuggee creation via DebugeeFactory with PathStrategy looks correct.

Initializing _debuggee with DebugeeFactory.CreateProcess(AdapterID, PathStrategy) in Initialize cleanly wires the path-handling strategy into the concrete debugee process and keeps construction centralized. No issues here.

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/VSCode.DebugAdapter/DebugeeProcess.cs (1)

106-115: Narrow the catch block and add logging.

The broad catch block can hide unexpected errors (e.g., security exceptions, invalid PID type issues). When Process.GetProcessById fails for reasons other than "process not found", those failures are silently swallowed.

Apply this diff to catch only expected exceptions and log the scenario:

-            try
-            {
-                _process = Process.GetProcessById(pid);
-                _process.EnableRaisingEvents = true;
-                _process.Exited += Process_Exited;
-            }
-            catch
-            {
-                _process = null;
-            }
+            try
+            {
+                _process = Process.GetProcessById(pid);
+                _process.EnableRaisingEvents = true;
+                _process.Exited += Process_Exited;
+            }
+            catch (ArgumentException ex)
+            {
+                Log.Information(ex, "Process {Pid} not found locally; attach will run without local process tracking", pid);
+                _process = null;
+            }

🧹 Nitpick comments (2)

src/VSCode.DebugAdapter/DebugeeProcess.cs (2)

314-329: Consider immutability: avoid mutating breakpoint objects.

While the null check for PathsMapper is good, the code still mutates the Source property of the original Breakpoint objects (line 322). Although ToArray() creates a new array, the breakpoint objects themselves are still shared references. If the caller or debugger retains references to these objects, the mutation could cause unexpected behavior.

Consider creating new breakpoint instances with the translated paths instead:

 public Breakpoint[] SetBreakpoints(IEnumerable<Breakpoint> breakpoints)
 {
     var breakpointsArray = breakpoints.ToArray();
 
     if (PathsMapper != null)
     {
         for (int i = 0; i < breakpointsArray.Length; i++)
         {
-            breakpointsArray[i].Source = PathsMapper.LocalToRemote(breakpointsArray[i].Source);
+            var original = breakpointsArray[i];
+            breakpointsArray[i] = new Breakpoint
+            {
+                Source = PathsMapper.LocalToRemote(original.Source),
+                Line = original.Line,
+                // Copy other properties as needed
+            };
         }
     }
 
     var confirmedBreaks = _debugger.SetMachineBreakpoints(breakpointsArray);
 
     return confirmedBreaks;
 }

Note: Adjust the property copying based on the actual Breakpoint class structure.

---

336-363: Consider immutability: avoid mutating stack frame objects.

The code directly mutates Source (line 355) and ThreadId (line 351) properties of stack frames returned from the debugger. If _debugger.GetStackFrames() returns cached frames or if these frames are referenced elsewhere, the mutation could cause unexpected behavior.

Consider creating new frame instances with the translated paths:

 var result = new List<StackFrame>();
 for (int i = firstFrameIdx; i < limit && i < allFrames.Length; i++)
 {
-
-    allFrames[i].ThreadId = threadId;
-
-    if (pathsMapperInit)
-    {
-        allFrames[i].Source = PathsMapper.RemoteToLocal(allFrames[i].Source);
-    }
-
-    result.Add(allFrames[i]);
+    var frame = allFrames[i];
+    var translatedSource = pathsMapperInit 
+        ? PathsMapper.RemoteToLocal(frame.Source) 
+        : frame.Source;
+    
+    result.Add(new StackFrame
+    {
+        Index = frame.Index,
+        ThreadId = threadId,
+        Source = translatedSource,
+        MethodName = frame.MethodName,
+        LineNumber = frame.LineNumber,
+        ColumnNumber = frame.ColumnNumber
+        // Copy other properties as needed
+    });
 }

Note: Adjust the property names based on the actual StackFrame class structure from OneScript.DebugProtocol.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 00a373d and 1f0fcc5.

📒 Files selected for processing (1)

• src/VSCode.DebugAdapter/DebugeeProcess.cs (11 hunks)

🧰 Additional context used 🧬 Code graph analysis (1) src/VSCode.DebugAdapter/DebugeeProcess.cs (3)

src/VSCode.DebugAdapter/WorkspaceMapper.cs (5)

• WorkspaceMapper (13-74)
• WorkspaceMapper (31-31)
• WorkspaceMapper (33-37)
• LocalToRemote (39-42)
• RemoteToLocal (44-47)

src/OneScript.DebugProtocol/IDebuggerService.cs (3)

• GetProcessId (88-88)
• Execute (21-21)
• StackFrame (46-46)

src/VSCode.DebugAdapter/DebugSession.cs (6)

• Source (93-114)
• Source (103-107)
• Source (109-113)
• Source (494-497)
• StackFrame (32-47)
• StackFrame (40-46)

🔇 Additional comments (4)

src/VSCode.DebugAdapter/DebugeeProcess.cs (4)

45-57: LGTM! HasExited now correctly handles remote attach scenarios.

The implementation properly distinguishes between "no local process" and "debuggee has exited". When in attach mode with an active debugger, it returns false to allow stepping operations to continue, which addresses the previous concern about breaking step operations in remote debugging scenarios.

---

59-59: LGTM! ExitCode now safely handles null process.

The null-coalescing operator prevents NullReferenceException when _process is null, returning a sensible default of 0 for remote attach scenarios.

---

126-150: LGTM! InitPathsMapper is now robust and handles edge cases correctly.

The method properly:

• Validates input arguments
• Extracts the specific mapping configuration from the correct location
• Gracefully handles missing or malformed configuration by setting PathsMapper to null
• Logs failures for debugging purposes

This addresses all previous concerns about NullReferenceException and improves observability.

---

283-290: LGTM! Kill method now safely handles null process.

The null check prevents NullReferenceException when Kill() is called after the process has been terminated or in remote attach scenarios where no local process exists.