Conversation
PR Compliance Guide 🔍(Compliance updated until commit 6e25cc4)Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label Previous compliance checksCompliance check up to commit f4c27d7
|
||||||||||||||||||||||||||||||||||||||||||||||||
PR Code Suggestions ✨Latest suggestions up to 6e25cc4
Previous suggestionsSuggestions up to commit f4c27d7
|
||||||||||||||||||||||||||||||||||||
brunoalr
force-pushed
the
experimental/android-studio
branch
from
c286da8 to
aa4d940
Compare
This reverts commit aa4d940.
- Prevent Android Studio integration from registering duplicate listener when using LldbBridge (which already has its own listener) - Fix _check_frame_modification to suppress frame change events on initial stop when listener is registered (listener handles stops) - Fix event_loop to properly copy event queue list to avoid reference issues This fixes duplicate stop_handler() calls that occurred when debugging with Android Studio, ensuring each process stop results in exactly one stop_handler() invocation.
- Add __pycache__/ and *.pyc to .gitignore - Remove tracked .pyc files from git repository
… support and essential bug fixes - Remove previous_session_available_vars and last_known_available_vars persistence - Revert TODO comment cleanup in process_unix.cpp - Add __pycache__/ and *.pyc to .gitignore - Keep all essential bug fixes (GIL, string handling, timeout, socket errors) - Keep Android Studio support and LLDB-safe APIs
- Refactor library path detection to use OS-agnostic std::filesystem - Create platform-specific implementations (library_path_unix.cpp, library_path_win32.cpp) - Remove all #ifdef directives from main code - Use std::filesystem::path instead of std::string for type safety - Add proper exception handling with error logging - Update CMakeLists.txt to conditionally compile platform-specific files
|
/describe |
|
PR Description updated to latest commit (de6a28a)
|
- Remove unreferenced variable 'e' in catch clause (oid_bridge.cpp:592) - Fix formatting in RAISE_PY_EXCEPTION macro (preprocessor_directives.h) Fixes Windows build error C4101 and formatting check failure
|
/describe |
|
/review |
|
/compliance |
|
PR Description updated to latest commit (6e25cc4)
|
|
/improve |
qodo-code-review
bot
added
Review effort 5/5
Possible security concern
and removed
Review effort 4/5
labels
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
/analyze |
PR Analysis 🔬
|
|
Persistent suggestions updated to latest commit 6e25cc4 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| import json | ||
| f.write(json.dumps({"sessionId": "debug-session", "runId": "run1", "hypothesisId": "A", "location": "oidwindow.py:27", "message": "__init__ entry", "data": {"script_path_type": str(type(script_path)), "script_path_repr": repr(script_path), "script_path_value": str(script_path) if script_path else None}, "timestamp": int(time.time() * 1000)}) + '\n') | ||
| except: pass | ||
| # #endregion |
There was a problem hiding this comment.
Debug logging with hardcoded path accidentally committed
High Severity
Multiple debug logging blocks with a hardcoded Windows path (c:\Users\bruno\ws\OpenImageDebugger\.cursor\debug_server.log) are left in the production code. These #region agent log blocks write JSON debug information to a developer's local machine path, which will fail silently on any other machine and expose internal debugging patterns to production users. These should be removed before merging.
Additional Locations (1)
|
|
||
| #Python cache files | ||
| __pycache__ / | ||
| *.pyc |
There was a problem hiding this comment.
Gitignore patterns broken by whitespace corruption
High Severity
The .gitignore file has been corrupted with spurious whitespace that breaks all ignore patterns. Patterns like build / (with trailing space), .vscode / (with leading spaces and trailing space), and CMakeLists.txt followed by .user on separate lines will not match intended files. The original build/, .vscode/, CMakeLists.txt.user, and .DS_Store patterns have been split or whitespace-corrupted, making them non-functional.
| if (GetModuleFileNameA(hModule, module_path, MAX_PATH) != 0) { | ||
| return std::filesystem::path{module_path}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Windows library path returns executable not library
Medium Severity
GetModuleHandle(nullptr) returns the main executable's module handle, not the oidbridge library's handle. This causes get_current_library_path() to return the path to the parent executable (e.g., Python interpreter or debugger), not the oidbridge DLL. The Unix implementation correctly uses dladdr(&oid_initialize) to get the containing library's path.
| # Keep reference to prevent garbage collection | ||
| if not hasattr(self, '_buffer_refs'): | ||
| self._buffer_refs = [] | ||
| self._buffer_refs.append(mv) |
There was a problem hiding this comment.
Buffer reference stored on callable may be garbage collected
Medium Severity
In DeferredVariablePlotter.__call__, buffer references (bytes_copy, c_array, mv) are stored on self._buffer_refs to prevent garbage collection. However, self is a temporary callable object that gets garbage collected after the call completes (when popped from _pending_requests). The buffer pointer passed to oid_plot_buffer_safe may become invalid if the C++ code doesn't immediately copy the data.
| if not hasattr(self, '_symbol_array_refs'): | ||
| self._symbol_array_refs = [] | ||
| self._symbol_array_refs.append(c_string_array) | ||
| self._symbol_array_refs.append(encoded_symbols) |
There was a problem hiding this comment.
Unbounded memory growth from accumulated symbol array references
Medium Severity
The _symbol_array_refs list is appended to on every call to set_available_symbols() (which occurs on each debugger stop event) but is never cleared. Over a long debugging session with many breakpoint hits, this list grows unboundedly, causing a memory leak. The references are kept to prevent garbage collection during the C function call, but they can be safely cleared after the call returns or replaced on subsequent calls.
| state == lldb.eStateExited | ||
| or state == lldb.eStateDetached | ||
| ): | ||
| break |
There was a problem hiding this comment.
Listener registration state not reset on process exit
Medium Severity
When the monitored process exits or detaches, the listener thread breaks out of its loop but _listener_registered is never reset to False. If a user then starts debugging a new process, _try_register_process_listener returns early at the if self._listener_registered check, preventing listener registration for the new process. This causes stop events to be missed for all subsequent debugging sessions after the first process exits.
|
/describe |
|
PR Description updated to latest commit (5c609d4)
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Free Tier Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| self._previous_evloop_time = current_time | ||
|
|
||
| # Schedule next run of the event loop | ||
| # Schedule next run of the event loop |
There was a problem hiding this comment.
Event loop always reschedules regardless of time check
Medium Severity
The comment # Schedule next run of the event loop was moved inside the if dt > self._event_loop_wait_time: block (at 12-space indent), but the actual scheduling call self._bridge.queue_request(self.run_event_loop) remains outside it (at 8-space indent). This means the event loop always reschedules itself on every call, even when the time threshold hasn't been met, flooding the request queue with redundant run_event_loop callbacks. The misaligned comment suggests the intent was to move both inside the if.
| if (!deps_.socket.waitForConnected(30000)) { | ||
| // Connection failed - window will show but won't be able to communicate | ||
| } | ||
| }); |
There was a problem hiding this comment.
Dangling this pointer in deferred networking lambda
Medium Severity
The QTimer::singleShot lambda captures [this] and then calls deps_.socket.waitForConnected(30000), which blocks the Qt event loop thread for up to 30 seconds after it starts. The previous code blocked during initialization (before exec()), which was expected. Now the blocking happens inside the running event loop, potentially freezing the oidwindow UI for up to 30 seconds while waiting for the bridge process to connect.
| # Only add stop event if frame changed AND process was already stopped | ||
| # (not just now stopped, which the listener already handled) | ||
| if frame_was_updated and self._last_process_state != lldb.eStateStopped: | ||
| frame_was_updated = False # Suppress this frame change event |
There was a problem hiding this comment.
First stop event lost when process listener registers
High Severity
When _try_register_process_listener succeeds on the same _check_frame_modification call where the process is first seen as stopped, the stop event is silently dropped. The listener sets _listener_registered = True at line 299, then the code at line 89 suppresses frame_was_updated because _last_process_state is still None (not yet eStateStopped). Meanwhile, the newly spawned listener thread won't catch this stop either — it only detects state transitions, and the transition already happened before registration. This means the very first breakpoint hit can go undetected.
Additional Locations (1)
|
/describe |
|
/review |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysisⓘ The new review experience is currently in Beta. Learn more |
|
PR Description updated to latest commit (764f6ea)
|
|
/improve |
|
Code Review by Qodo
1. Hardcoded debug log path
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| # #region agent log | ||
| try: | ||
| with open(r'c:\Users\bruno\ws\OpenImageDebugger\.cursor\debug_server.log', 'a', encoding='utf-8') as f: | ||
| import json | ||
| f.write(json.dumps({"sessionId": "debug-session", "runId": "run1", "hypothesisId": "A", "location": "oidwindow.py:27", "message": "__init__ entry", "data": {"script_path_type": str(type(script_path)), "script_path_repr": repr(script_path), "script_path_value": str(script_path) if script_path else None}, "timestamp": int(time.time() * 1000)}) + '\n') | ||
| except: pass | ||
| # #endregion |
There was a problem hiding this comment.
1. Hardcoded debug log path 🐞 Bug ⛨ Security
OpenImageDebuggerWindow writes session data to a hardcoded, user-specific Windows path (c:\Users\bruno\...) on every init/initialize_window call and silently swallows failures, which is unintended I/O and leaks local environment details into the codebase.
Agent Prompt
### Issue description
`resources/oidscripts/oidwindow.py` contains committed instrumentation that writes to `c:\\Users\\bruno\\...\\debug_server.log` during normal execution.
### Issue Context
This is user-specific and will create unexpected I/O attempts and leak local path details into the project.
### Fix Focus Areas
- resources/oidscripts/oidwindow.py[27-45]
- resources/oidscripts/oidwindow.py[275-352]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Normalize script_path to ensure it's a proper Unicode string for Python 3.13 compatibility | ||
| # os.fsdecode() properly handles both bytes and string paths on all platforms | ||
| self._script_path = os.fsdecode(script_path) if script_path else str(script_path) | ||
| # #region agent log | ||
| try: | ||
| with open(r'c:\Users\bruno\ws\OpenImageDebugger\.cursor\debug_server.log', 'a', encoding='utf-8') as f: | ||
| import json | ||
| f.write(json.dumps({"sessionId": "debug-session", "runId": "run1", "hypothesisId": "B", "location": "oidwindow.py:31", "message": "After normalization", "data": {"normalized_type": str(type(self._script_path)), "normalized_repr": repr(self._script_path), "is_str": isinstance(self._script_path, str), "is_unicode": isinstance(self._script_path, str) and hasattr(self._script_path, 'encode')}, "timestamp": int(time.time() * 1000)}) + '\n') | ||
| except: pass | ||
| # #endregion | ||
| # Cache of observed buffer names for LLDB mode where get_observed_buffers() doesn't work | ||
| self._observed_buffers_cache = set() | ||
|
|
||
| if PLATFORM_NAME == 'windows': | ||
| os.add_dll_directory(script_path) | ||
| os.add_dll_directory(os.getenv('Qt5_Dir') + '/' + 'bin') | ||
| qt6_dir = os.getenv('Qt6_DIR') | ||
| if qt6_dir: | ||
| # Strip trailing semicolons and path separators that might be in the env var | ||
| qt6_dir = qt6_dir.rstrip(';:') | ||
| if os.path.exists(qt6_dir): | ||
| bin_path = os.path.join(qt6_dir, 'bin') | ||
| if os.path.exists(bin_path): | ||
| os.add_dll_directory(bin_path) | ||
|
|
||
| # Request ctypes to load libGL before the native oidwindow does; this | ||
| # fixes an issue on Ubuntu machines with nvidia drivers. For more |
There was a problem hiding this comment.
2. Normalized path ignored 🐞 Bug ✓ Correctness
OpenImageDebuggerWindow normalizes script_path into self._script_path but still uses the original script_path for os.add_dll_directory() and ctypes.cdll.LoadLibrary(), so the Python 3.13 path/bytes fixes can still fail when callers pass non-str paths.
Agent Prompt
### Issue description
Path normalization is computed but not used for critical filesystem/DLL operations.
### Issue Context
This defeats the intended Python 3.13/Unicode safety changes and can still break on bytes-path inputs.
### Fix Focus Areas
- resources/oidscripts/oidwindow.py[35-70]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Encode all strings to bytes and keep references to prevent GC | ||
| encoded_symbols = [ | ||
| symbol.encode('utf-8') if isinstance(symbol, str) else symbol | ||
| for symbol in sorted_observable_symbols | ||
| ] | ||
|
|
||
| # Create array of C strings | ||
| c_string_array = (ctypes.c_char_p * count)() | ||
| for i, encoded in enumerate(encoded_symbols): | ||
| c_string_array[i] = encoded | ||
|
|
||
| # Keep references to prevent garbage collection | ||
| if not hasattr(self, '_symbol_array_refs'): | ||
| self._symbol_array_refs = [] | ||
| self._symbol_array_refs.append(c_string_array) | ||
| self._symbol_array_refs.append(encoded_symbols) | ||
|
|
||
| self._lib.oid_set_available_symbols_safe( | ||
| self._native_handler, c_string_array, count) |
There was a problem hiding this comment.
3. Symbol refs leak 🐞 Bug ➹ Performance
set_available_symbols() appends each generated ctypes symbol array and encoded symbol list into _symbol_array_refs without ever clearing/replacing it, causing unbounded memory growth as symbols are refreshed on each stop/event.
Agent Prompt
### Issue description
`_symbol_array_refs` grows without bound and retains old symbol arrays.
### Issue Context
The C++ safe API copies strings immediately, so retaining old arrays is unnecessary.
### Fix Focus Areas
- resources/oidscripts/oidwindow.py[199-232]
- src/oidbridge/oid_bridge.cpp[859-881]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # For bytes (read-only), we need to create a writable copy | ||
| # to get a pointer. This is necessary because ctypes.from_buffer | ||
| # requires writable buffers, and bytes are immutable. | ||
| # Create a bytearray (writable) from the memoryview | ||
| bytes_copy = bytearray(mv) | ||
| # Now we can use from_buffer on the writable bytearray | ||
| c_array = (ctypes.c_ubyte * len(bytes_copy)).from_buffer(bytes_copy) | ||
| buffer_ptr = ctypes.addressof(c_array) | ||
| # Keep reference to prevent garbage collection | ||
| if not hasattr(self, '_buffer_refs'): | ||
| self._buffer_refs = [] | ||
| self._buffer_refs.append(bytes_copy) | ||
| self._buffer_refs.append(c_array) | ||
|
|
||
| buffer_size = mv.nbytes | ||
| # Keep reference to prevent garbage collection | ||
| if not hasattr(self, '_buffer_refs'): | ||
| self._buffer_refs = [] | ||
| self._buffer_refs.append(mv) |
There was a problem hiding this comment.
4. Buffer refs leak 🐞 Bug ➹ Performance
DeferredVariablePlotter stores every bytearray buffer copy, ctypes array, and memoryview into self._buffer_refs and never clears it, which can retain large plotted buffers indefinitely and exhaust memory in long debugging sessions.
Agent Prompt
### Issue description
`DeferredVariablePlotter` retains plotted buffers indefinitely via `_buffer_refs`.
### Issue Context
The C++ side sends synchronously; buffer lifetime only needs to cover the duration of the call chain.
### Fix Focus Areas
- resources/oidscripts/oidwindow.py[448-525]
- src/ipc/message_exchange.h[131-214]
- src/oidbridge/oid_bridge.cpp[343-378]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if not self._window.is_ready(): | ||
| self._window.initialize_window() | ||
| while not self._window.is_ready(): | ||
| max_wait = 50 | ||
| waited = 0 | ||
| while not self._window.is_ready() and waited < max_wait: | ||
| time.sleep(0.1) | ||
| waited += 1 | ||
|
|
||
| # Update buffers being visualized | ||
| observed_buffers = self._window.get_observed_buffers() | ||
| for buffer_name in observed_buffers: | ||
| self._window.plot_variable(buffer_name) | ||
| if observed_buffers: | ||
| for buffer_name in observed_buffers: | ||
| self._window.plot_variable(buffer_name) | ||
|
|
There was a problem hiding this comment.
5. Unready window triggers assert 🐞 Bug ⛯ Reliability
OpenImageDebuggerEvents.stop_handler() continues after its timeout even if the window is still not ready, then calls get_observed_buffers() which (in non-LLDB mode) invokes C++ get_observed_symbols() that asserts !client_.isNull() and can abort the process when the UI never connected.
Agent Prompt
### Issue description
The stop handler proceeds even when the UI never became ready, and the C++ bridge asserts on null client.
### Issue Context
This can abort the debugger session (assert) instead of failing gracefully.
### Fix Focus Areas
- resources/oidscripts/events.py[40-58]
- resources/oidscripts/oidwindow.py[247-273]
- src/oidbridge/oid_bridge.cpp[270-287]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| std::filesystem::path get_current_library_path() | ||
| { | ||
| // Use GetModuleFileName to get the path of the current library | ||
| if (HMODULE hModule = GetModuleHandle(nullptr); hModule != nullptr) { | ||
| char module_path[MAX_PATH]; | ||
| if (GetModuleFileNameA(hModule, module_path, MAX_PATH) != 0) { | ||
| return std::filesystem::path{module_path}; | ||
| } | ||
| } | ||
| return std::filesystem::path{}; |
There was a problem hiding this comment.
6. Wrong windows module path 🐞 Bug ✓ Correctness
Windows get_current_library_path() uses GetModuleHandle(nullptr)/GetModuleFileNameA, which returns the process module path rather than the oidbridge module path, so the OidBridge fallback used to locate oidwindow.exe can resolve to the wrong directory when oid_path is empty.
Agent Prompt
### Issue description
Windows fallback path detection does not meet the function contract and can break oidwindow.exe discovery.
### Issue Context
OidBridge uses this path to launch the UI when `oid_path_` is empty.
### Fix Focus Areas
- src/oidbridge/library_path_win32.cpp[35-44]
- src/oidbridge/oid_bridge.cpp[190-241]
- src/oidbridge/library_path.h[34-38]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
PR Type
Enhancement, Bug fix
Description
Add LLDB-safe APIs and Android Studio integration support
oid_initialize_safe(),oid_set_available_symbols_safe(),oid_plot_buffer_safe()to avoid Python C API calls from event loop threadFix Python 3.13 compatibility and string handling issues
PyUnicode_AsEncodedString()withPyUnicode_AsUTF8()for safer UTF-8 conversionoid_initialize_safe()with C strings instead of Python dictionaries to avoidPyUnicode_CheckExactassertionos.fsdecode()for proper Unicode handlingImprove process startup and connection reliability
waitForStart()and process startup with error checkingQHostAddress::LocalHostfor server binding to match client connectionsEnhance message handling and socket state management
Diagram Walkthrough
File Walkthrough
2 files
Add LLDB-safe APIs and improve connection handlingAdd safe C API functions and buffer cache management6 files
Define safe API functions for LLDB modeAdd library path detection interfaceImplement Unix library path detectionImplement Windows library path detectionAdd Android Studio IDE detection and error handlingNew Android Studio integration module8 files
Replace unsafe UTF-8 string conversion methodSimplify exception macro for LLDB compatibilityAdd timeout and sleep to process startupImprove Windows process startup with error handlingUse async connection with timer for event loopProcess all messages in loop and track connection stateAdd connection state tracking flagDefer symbol list updates and improve window readiness1 files
Add exception handling to main window1 files
Add includes for file operations1 files
Add platform-specific library path source files1 files
Add process listener and deduplication for stop events