fix(api): bound run-ui-command waits and always disconnect the listener

[herikwebb]

Problem

wait_for_run_ui_command_response polled forever with no timeout and, unlike its sibling wait_for_chat_user_input, had no try/finally around the signal disconnect. If the browser tab that must answer a UI command disappears (page reload, closed websocket), the request thread kept spinning at 10 Hz indefinitely with the tool loop wedged, and any exception or task cancellation leaked the callback onto the signal.

Change

• Timeout (default 1800s via NBI_RUN_UI_COMMAND_RESPONSE_TIMEOUT; <= 0 disables) raising TimeoutError. Generous default because run-cell legitimately blocks for the duration of a long-running cell.
• Listener disconnected in a finally on every exit path — return, timeout, and task cancellation.
• 6 tests covering the return, timeout, cancellation, env-default, and disabled-bound paths.

Testing

Full Python suite passes (1298 tests).

[github-actions]

Automated PR Review

Model: gpt-5.5
Verdict: CHANGES_REQUESTED

Changed files:

notebook_intelligence/api.py
tests/test_run_ui_command_wait.py

Review result:

Automated PR Review Findings

Medium — notebook_intelligence/api.py

Import-time crash on malformed timeout environment variable

RUN_UI_COMMAND_RESPONSE_TIMEOUT is initialized with:

float(os.getenv("NBI_RUN_UI_COMMAND_RESPONSE_TIMEOUT", "1800"))

If the environment variable is set to an invalid value such as "", "30s", or another non-float string, importing notebook_intelligence.api will raise ValueError and can prevent the backend from starting at all. Since this is runtime configuration, it should be parsed defensively with a fallback/default or a clear configuration error that does not make unrelated imports fail unexpectedly.

Suggested fix: move parsing into a helper that catches ValueError, logs the invalid value, and falls back to 1800, or raise a controlled application configuration error during startup.

---

Low — notebook_intelligence/api.py

Timeout calculation uses wall-clock time instead of monotonic time

The new timeout logic uses time.time():

started = time.time()
...
if timeout > 0 and time.time() - started > timeout:

Wall-clock time can jump forward or backward due to NTP adjustments, VM suspend/resume behavior, or manual clock changes. That can cause premature timeout or significantly delayed timeout. Timeout/deadline logic should use time.monotonic().

Suggested fix: use time.monotonic() for started and elapsed timeout calculations.

[github-actions]

Automated PR Review

Model: gpt-5.5
Verdict: CHANGES_REQUESTED

Changed files:

notebook_intelligence/api.py
tests/test_run_ui_command_wait.py

Review result:

Findings

Medium — notebook_intelligence/api.py

wait_for_run_ui_command_response uses None as both the “no response received yet” sentinel and the possible response payload:

resp = {"result": None}
...
resp["result"] = data["result"]
...
if resp["result"] is not None:
    return resp["result"]

If the frontend returns a valid JSON null / Python None result for a successful void UI command, the wait loop will ignore that response and eventually raise TimeoutError, even though the callback arrived. This can incorrectly fail commands that intentionally have no return value.

Use a separate received flag or a unique sentinel object, e.g. received = False, or initialize with sentinel = object() and compare by identity. Please also add a test covering {"callback_id": ..., "result": None}.

[github-actions]

Automated PR Review

Model: gpt-5.5
Verdict: CHANGES_REQUESTED

Changed files:

notebook_intelligence/api.py
tests/test_run_ui_command_wait.py

Review result:

Findings

• Severity: Medium — notebook_intelligence/api.py
  • _float_env() accepts non-finite float values such as NaN, inf, and -inf. This can silently defeat the new timeout behavior:
    • NBI_RUN_UI_COMMAND_RESPONSE_TIMEOUT=nan makes timeout > 0 evaluate false, effectively disabling the bound.
    • NBI_RUN_UI_COMMAND_RESPONSE_TIMEOUT=inf makes the timeout practically unreachable.
  • Since this PR’s goal is to prevent unbounded waits when the frontend disappears, the parser should reject non-finite values with math.isfinite() and fall back to the default, while still allowing documented <= 0 finite values to disable the timeout intentionally.

[github-actions]

Automated PR Review

Model: gpt-5.5
Verdict: APPROVE

Changed files:

notebook_intelligence/api.py
tests/test_run_ui_command_wait.py

Review result:

No automated findings were found.

Residual risks: this change introduces a new timeout behavior for UI command round trips, so very long-running frontend commands may now fail unless NBI_RUN_UI_COMMAND_RESPONSE_TIMEOUT is tuned or disabled. Call-site behavior on TimeoutError should still be validated in integration/manual testing.