⚡ Match Highlights: realsenseai/librealsense/pull/

AviaAv · 2026-06-09T08:05:18Z

Tracked on: [RSDEV-11500]

Nir-Az

[rs-agentic bot — automated review]

Reviewed commit 9fa49f7435b (metadata-registry-enable → development). 13 files changed, +175 / -12.

Inline comments below cover: 🐛 potential bugs, 💡 enhancement suggestions, ⚠️ guideline/best-practice notes, and 🧪 missing test coverage. No approval or code push has been made.

Nir-Az · 2026-06-09T08:09:00Z

+    Uses .NET ProcessStartInfo directly (via PowerShell) so the `-c "expr"` arg
+    survives the elevation hop intact. ``expr`` must contain no double quotes.
+    """
+    ps = (f"$psi = [Diagnostics.ProcessStartInfo]@{{FileName='{sys.executable}'; "


🐛 BUG – shell injection via sys.executable path (auto-generated by rs-agentic bot)

sys.executable is interpolated directly into a PowerShell single-quoted string without escaping single quotes. On many Windows installs the Python path contains no single quotes, but the restriction is undocumented and fragile. If the path ever does contain a ' (or a dollar sign, backtick, or other PowerShell metacharacter), the generated script will be syntactically broken or execute unexpected code.

Recommended fix – escape single quotes at minimum:

exe = sys.executable.replace("'", "''") # PS single-quote escape ps = (f"$psi = [Diagnostics.ProcessStartInfo]@{{FileName='{exe}'; " ...)

A more robust alternative is to write a temporary .ps1 file and invoke powershell.exe -File <path>, which avoids all shell-quoting issues entirely.

Fixed — escaped ' in sys.executable before interpolation.

Nir-Az · 2026-06-09T08:09:00Z

+
+
+@router.post("/enable-metadata")
+async def enable_metadata():


🐛 BUG – blocking subprocess.run inside an async endpoint (auto-generated by rs-agentic bot)

subprocess.run(...) is synchronous and blocks the asyncio event loop for up to 120 seconds (the full timeout value). During that window no other request can be served by this FastAPI process.

Fix – run the blocking call in a thread pool:

import asyncio @router.post("/enable-metadata") async def enable_metadata(): if platform.system() != "Windows": return {"status": "noop", "note": "Metadata management is Windows-only."} try: rc = await asyncio.to_thread( _run_elevated_python, "import pyrealsense2 as rs; rs.enable_metadata()" ) except subprocess.TimeoutExpired: raise HTTPException(504, "Metadata enable timed out (user did not approve UAC?).") ...

asyncio.to_thread (Python 3.9+) or loop.run_in_executor(None, ...) both work.

Deferred — this BE is a single-user local UI; the UAC dialog dominates the blocking window anyway. Negligible practical impact for concurrent requests.

Nir-Az

[rs-agentic bot — automated review, continued] React / hooks findings.

Nir-Az · 2026-06-09T08:09:23Z

+        },
+      }
+    )
+  }, [devices])


⚠️ React exhaustive-deps violation – stale closures (auto-generated by rs-agentic bot)

The dependency array is [devices], but the effect body calls addToast, removeToast, and enableMetadata — all of which are either re-created each render (functions defined inline in the component) or come from the store selector. This will trigger the react-hooks/exhaustive-deps ESLint rule and, more importantly, can cause stale-closure bugs if those functions ever change identity meaningfully.

Options:

Wrap addToast / removeToast in useCallback (with stable deps) and add them to the dep array.

Use stable refs (e.g., addToastRef.current = addToast) and call through the ref inside the effect.

Move the toast call outside the effect by using a derived state / reducer pattern.

The promptedSetRef / enableInFlightRef guard is a good idea to prevent duplicate toasts, but without stable function references the effect's correctness relies on an implicit assumption that those helpers never change.

Deferred — addToast/removeToast/enableMetadata are stable refs (useState setters / zustand selectors); no real stale-closure risk here.

Nir-Az · 2026-06-09T08:09:23Z

+        'Enable it once (admin required) to surface per-frame metadata.',
+      {
+        label: 'Enable',
+        onClick: async (toastId) => {


🐛 Missing unmount cleanup – state update on unmounted component (auto-generated by rs-agentic bot)

The onClick handler is an async function that calls removeToast / addToast after await enableMetadata(). If the DevicePanel unmounts while the request is in-flight (e.g., the user navigates away), the set* calls in the try / catch / finally blocks will attempt to update state on an unmounted component, causing a React warning and possible memory-leak-style bugs in strict mode.

Recommended mitigation – track mount status via a ref:

const mountedRef = useRef(true) useEffect(() => () => { mountedRef.current = false }, []) // inside onClick: if (mountedRef.current) removeToast(toastId) if (mountedRef.current) addToast(variant, result.note ?? 'Done.')

Or use an AbortController to cancel the API call on unmount.

Deferred — DevicePanel is top-level and doesn't unmount in this app. React strict-mode dev warning, not a real bug.

Nir-Az

[rs-agentic bot — automated review, continued] Enhancement suggestions and missing test coverage.

Nir-Az · 2026-06-09T08:10:22Z

+          f"Arguments='-c \"{expr}\"'; Verb='runas'; UseShellExecute=$true; WindowStyle='Hidden'}}; "
+          f"try {{ $p = [Diagnostics.Process]::Start($psi); $p.WaitForExit(); exit $p.ExitCode }} "
+          f"catch {{ exit $_.Exception.NativeErrorCode }}")
+    return subprocess.run(["powershell.exe", "-NoProfile", "-Command", ps], timeout=timeout).returncode


💡 Enhancement – unhandled OSError / FileNotFoundError if PowerShell is absent (auto-generated by rs-agentic bot)

If powershell.exe is not in PATH (e.g., non-standard Windows Server installs or future environments), subprocess.run will raise FileNotFoundError / OSError. This propagates as an unhandled 500 with a raw Python traceback in the response body.

Suggested fix:

try: rc = await asyncio.to_thread( _run_elevated_python, "import pyrealsense2 as rs; rs.enable_metadata()" ) except subprocess.TimeoutExpired: raise HTTPException(504, "Metadata enable timed out (user did not approve UAC?).") except OSError as exc: raise HTTPException(500, f"Could not launch PowerShell: {exc}")

Fixed — added except OSError → 500.

Nir-Az · 2026-06-09T08:10:22Z

  const isMotion = ['gyro', 'accel', 'motion'].includes(streamType.toLowerCase())
+  // Mirrors C++ viewer (common/stream-model.cpp): when SDK falls back to system_time,
+  // per-frame metadata is unavailable from the kernel UVC driver.
+  const metadataUnavailable = metadata.clock_domain === 'system_time'


💡 Enhancement – system_time string is magic / fragile (auto-generated by rs-agentic bot)

The clock_domain check is a raw string comparison against 'system_time'. If the backend ever changes that string (casing, suffix, etc.), the UI warning silently stops appearing. Consider defining a TypeScript literal type for valid clock-domain values in types.ts:

// types.ts export type ClockDomain = 'global_time' | 'system_time' | 'hardware_clock'

and typing metadata.clock_domain as ClockDomain | undefined so any drift becomes a compile-time error.

Deferred — a literal type doesn't catch backend drift (BE could send any string), only FE consumer typos. Low value for the surface area.

Nir-Az · 2026-06-09T08:10:22Z

@@ -155,13 +155,13 @@ describe('ToastContainer', () => {
    expect(onClose).toHaveBeenCalledWith('toast-1')


🧪 Missing tests for new action prop behavior (auto-generated by rs-agentic bot)

The PR adds a significant new behavior to Toast: the action prop makes toasts sticky (no auto-dismiss) and renders a call-to-action button. The following scenarios are not yet covered by tests:

Toast with action does not auto-dismiss (timer should never fire).

Clicking the action button calls action.onClick with the correct toast id.

Toast without action still auto-dismisses (regression guard).

ToastContainer correctly passes action through to Toast.

These are behaviorally critical: if the auto-dismiss timer were accidentally left enabled for action-bearing toasts, the UAC-prompt toast would disappear before the user can click 'Enable'.

Fixed — added 3 tests: sticky-no-auto-dismiss, onClick receives toast id, ToastContainer forwards action.

Nir-Az · 2026-06-09T08:10:34Z

[rs-agentic bot — automated review] 🧪 Missing test coverage for DevicePanel metadata popup flow

DevicePanel.test.tsx was not modified in this PR, but the component gained significant new logic tied to metadata_enabled. The following test cases should be added to tests/unit/components/DevicePanel.test.tsx:

Shows info toast when a device has metadata_enabled === false — verifies the useEffect fires and addToast is called.
No duplicate toast when the same set of disabled devices re-renders — exercises the promptedSetRef guard.
Re-shows toast when a new device with metadata_enabled === false is added — verifies the ref is invalidated on set change.
Clicking 'Enable' calls enableMetadata from the store.
Success path (status === 'ok') — original toast is removed, success toast appears, fetchDevices is called.
Declined path (status === 'declined') — original toast removed, info toast appears.
Error path — original toast removed, error toast appears.

Without these, a regression in the metadata_enabled === false vs null vs undefined guard — or in the de-duplication logic — would go undetected.

aikido-pr-checks · 2026-06-09T08:41:05Z

+    Uses .NET ProcessStartInfo directly (via PowerShell) so the `-c "expr"` arg
+    survives the elevation hop intact. ``expr`` must contain no double quotes.
+    """
+    exe = sys.executable.replace("'", "''")  # PS single-quote escape


Constructing and directly executing a PowerShell command from a runtime 'expr' (ps variable + subprocess.run) enables dynamic execution of arbitrary code; consider restricting or validating 'expr' input.

Details

✨ AI Reasoning
The change builds a PowerShell command string by interpolating a caller-provided Python expression (expr) into a PowerShell -Command string, then calls subprocess.run to execute it. Dynamic construction and execution of code strings makes the effective behavior depend on runtime input and can hide executed actions from quick review. This pattern can be used legitimately (elevation), but it also matches common obfuscation/execution techniques used in malware (dynamic command generation and invocation). The concern arises specifically from the new use of f-string concatenation to craft a PS command and the subsequent subprocess invocation that runs it.

🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

@AikidoSec ignore: Same as above — the PowerShell script is a hard-coded local literal; no runtime/user-controlled input. Pattern matches malware heuristics but there is no dynamic expression.

✅ Based on your feedback, we ignored this issue because of the following reason:

Same as above — the PowerShell script is a hard-coded local literal; no runtime/user-controlled input. Pattern matches malware heuristics but there is no dynamic expression.

aikido-pr-checks · 2026-06-09T08:51:05Z

    updateFirmwareFromFile(deviceId, file)
  }

+  // Prompt once per metadata-disabled device-set per session. Set changes


DevicePanel grew additional async UI logic (promptedSetRef, enableInFlightRef, and an enable-metadata useEffect). Extract this new metadata-enable flow into a smaller helper/component to reduce DevicePanel's complexity.

Details

✨ AI Reasoning
1. I looked for functions that were changed in this diff. 2. DevicePanel (the React component function) was modified: fetchDevices call updated, addToast signature changed, and a new useEffect plus refs (promptedSetRef, enableInFlightRef) and async enableMetadata flow were added. 3. The change introduces new responsibilities into DevicePanel: device enumeration semantics, toast control, and an admin metadata-enable flow with async side-effects and state guarding. 4. This increases cognitive load: consumers of DevicePanel must now reason about session-prompting state, in-flight enable operations, toast lifecycle, and error handling in addition to existing rendering and device-control logic. 5. The added code lives inside the already-large DevicePanel function rather than extracted helpers, worsening maintainability and testing surface area. 6. Therefore this change worsened the long-function/code-complexity issue for DevicePanel and is worth addressing.

🔧 How do I fix it?
Break down long functions into smaller helper functions. Aim for functions under 60 lines with fewer than 10 local variables.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

Resolved — extracted the metadata-enable flow into the showMetadataEnablePromptIfNeeded helper. DevicePanel function shrunk by ~30 LOC.

…t + popup warning)

…rning

aikido-pr-checks · 2026-06-09T09:06:24Z

+          f"Verb='runas'; UseShellExecute=$true; WindowStyle='Hidden'}}; "
+          f"try {{ $p = [Diagnostics.Process]::Start($psi); $p.WaitForExit(); exit $p.ExitCode }} "
+          f"catch {{ exit $_.Exception.NativeErrorCode }}")
+    return subprocess.run(["powershell.exe", "-NoProfile", "-Command", ps], timeout=timeout).returncode


Constructing and executing a dynamically built PowerShell command via subprocess.run can hide behavior. Prefer explicit, well-documented invocation or minimize dynamic construction to improve transparency.

Details

✨ AI Reasoning
The change builds a PowerShell command string at runtime and executes it. This creates a dynamic execution path where arbitrary or unexpected commands could be composed and run, which is a common technique used to hide or obfuscate behavior. Even if intended for a legitimate elevation flow, the pattern of programmatically constructing and shelling out a command increases risk and reduces transparency. Reviewers should ensure the constructed command is minimal, well-documented, and cannot be influenced by untrusted input. The rest of the file is straightforward and does not exhibit additional obfuscation techniques.

🔧 How do I fix it?
Ensure code is transparent and not intentionally obfuscated. Avoid hiding functionality from code review. Focus on intent and deception, not specific patterns.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

@AikidoSec ignore: The PowerShell script is a hard-coded local literal (script = "import pyrealsense2 as rs; rs.enable_metadata()") — no runtime/user-controlled input. The f-string only interpolates sys.executable (escaped) and that literal. Pattern matches malware heuristics but there is no dynamic expression. UAC self-elevation requires shelling out to PowerShell.

✅ Based on your feedback, we ignored this issue because of the following reason:

The PowerShell script is a hard-coded local literal (script = "import pyrealsense2 as rs; rs.enable_metadata()") — no runtime/user-controlled input. The f-string only interpolates sys.executable (escaped) and that literal. Pattern matches malware heuristics but there is no dynamic expression. UAC self-elevation requires shelling out to PowerShell.

remibettan · 2026-06-09T09:36:20Z

+    """
+    script = "import pyrealsense2 as rs; rs.enable_metadata()"
+    exe = sys.executable.replace("'", "''")  # PS single-quote escape
+    ps = (f"$psi = [Diagnostics.ProcessStartInfo]@{{FileName='{exe}'; "


catch block can silently report success on non-Win32 failures

exit $_.Exception.NativeErrorCode evaluates to exit $null → exit code 0 whenever the thrown exception isn't a Win32Exception (e.g. MethodInvocationException from a malformed ProcessStartInfo, or any other
.NET exception where NativeErrorCode is absent). The caller then maps rc == 0 to {"status": "ok", "note": "Metadata enabled."} and the UI shows a success toast even though nothing ran.

Suggested fix — default to a non-zero code when NativeErrorCode is null/missing:

f"try {{ $p = [Diagnostics.Process]::Start($psi); $p.WaitForExit(); exit $p.ExitCode }} " f"catch {{ $c = $_.Exception.NativeErrorCode; if (-not $c) {{ $c = 1 }}; exit $c }}")

While you're here, it's also worth capturing the elevated child's stderr (RedirectStandardError + UseShellExecute=$false won't work with runas, so log to a temp file via the inner -c script instead) —
right now WindowStyle='Hidden' means a non-zero exit reaches the REST caller with no diagnostic. Optional, but the next person debugging a registry-write failure will thank you.

remibettan · 2026-06-09T09:36:56Z

@@ -0,0 +1,44 @@
+# License: Apache 2.0. See LICENSE file in root directory.
+# Copyright(c) 2026 RealSense, Inc. All Rights Reserved.


Missing tests for the new endpoint

remibettan

few comments - great job!

…tests

AviaAv requested a review from Nir-Az June 9, 2026 08:05

Nir-Az reviewed Jun 9, 2026

View reviewed changes

AviaAv force-pushed the metadata-registry-enable branch from 9fa49f7 to 069352a Compare June 9, 2026 08:40

aikido-pr-checks Bot reviewed Jun 9, 2026

View reviewed changes

AviaAv force-pushed the metadata-registry-enable branch from 069352a to b52b128 Compare June 9, 2026 08:50

aikido-pr-checks Bot reviewed Jun 9, 2026

View reviewed changes

AviaAv added 3 commits June 9, 2026 12:04

rest-api: add Windows UVC metadata-registry enable endpoint

6413058

react-viewer: integrate Windows UVC metadata enable flow (toast promp…

0294a2f

…t + popup warning)

react-viewer: add tests for Toast action prop and metadata-overlay wa…

87ee564

…rning

AviaAv force-pushed the metadata-registry-enable branch from b52b128 to 87ee564 Compare June 9, 2026 09:05

aikido-pr-checks Bot reviewed Jun 9, 2026

View reviewed changes

AviaAv requested a review from remibettan June 9, 2026 09:14

remibettan reviewed Jun 9, 2026

View reviewed changes

rest-api: capture exit code of non-Win32 PS exceptions, add endpoint …

54bbc28

…tests

remibettan approved these changes Jun 9, 2026

View reviewed changes

AviaAv merged commit 6cbf829 into realsenseai:development Jun 9, 2026
29 of 30 checks passed

AviaAv deleted the metadata-registry-enable branch June 9, 2026 12:17



		@router.post("/enable-metadata")
		async def enable_metadata():

		@@ -155,13 +155,13 @@ describe('ToastContainer', () => {
		expect(onClose).toHaveBeenCalledWith('toast-1')

		@@ -0,0 +1,44 @@
		# License: Apache 2.0. See LICENSE file in root directory.
		# Copyright(c) 2026 RealSense, Inc. All Rights Reserved.

Conversation

AviaAv commented Jun 9, 2026

Uh oh!

Nir-Az left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nir-Az left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nir-Az left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nir-Az commented Jun 9, 2026

Uh oh!

aikido-pr-checks Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Jun 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remibettan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects