fix(async): four production blocking calls off the event loop, and a guard that could not see them (#17646) by mrveiss · Pull Request #17647 · mrveiss/AutoBot-AI

mrveiss · 2026-09-27T17:33:19Z

Thinking Path

tools/lint/check_no_blocking_io_in_async.py (#7444) knew exactly two vocabularies —
requests.* and Path.read/write_text/bytes — and passed clean over every file in this
diff. The codebase-analytics index independently reported 8 high-severity
performance_blocking_io_in_async findings in those same approved files. That gap is the
whole story of this PR, and it is two defects, not one.

Defect 1 — vocabulary. Four production sites were on the event loop:

site	call	loop blocked
VNC quality-metrics endpoint	`pgrep` reached through a sync helper	up to 5s
same endpoint	socket `connect_ex` after `settimeout(2)`	up to 2s
same endpoint	`pgrep -a websockify`, `timeout=5`	up to 5s
VNC clipboard endpoint	`Popen(...).communicate(timeout=5)`	up to 5s
SLM agent event sync	`sqlite3.connect` ×3, one held across a 30s POST	up to 30s + a DB handle

One request to the quality endpoint could therefore stall every other request for roughly
twelve seconds. Adding subprocess.*, sqlite3.connect, time.sleep, os.system to the
guard is a list update and closes this half.

Defect 2 — a design blind spot a list update does not reach. Two of the three
quality-endpoint calls were invisible to both the hook and to a hand-written AST walk
over the same files. is_vnc_running is a sync function called from async, and the
visitor resets its async depth inside def bodies — correctly, since a sync helper may
have sync callers. Blocking work one frame below an async def stays invisible however
wide the call list gets. Closing that needs reachability analysis, not matching. It is
recorded in the code at both sites so a reader of the widened guard does not assume the
class is now covered: it covers more of the class, not all of it.

Both review rounds supplied proof of that claim rather than an argument for it. Every site
found after the first push is this shape — a sync function reached only from async, one
frame below the visitor's depth reset — and none of them was found by the tool:

found in	site	reached from
round 1	`buffer_event` in the heartbeat failure handler	`async def _send_heartbeat_request`
round 2	`_process_code_change` → `buffer_event` → `sqlite3.connect`	`async def handle_code_change`, an HTTP endpoint
round 2	`is_vnc_running()` bare, `pgrep` at `timeout=5`	`async def get_vnc_status`, `async def ensure_vnc_running`

All are fixed here rather than deferred. The is_vnc_running pair was pre-existing and in
scope for one reason: it is the same defect in a file this PR already owns, and one defect
is fixed once. The alternative was a fourth and fifth rediscovery of a blocking pgrep in
vnc_manager.py, filed as new mysteries.

Because the guard structurally cannot make these assertions, the call sites are pinned by
AST tests instead — vnc_blocking_probes_call_sites_test.py and the agent's call-site
class. A bare is_vnc_running() returns the same boolean as an offloaded one; only the call
site records whether the loop was free while pgrep ran.

Defect 3, found by that same review — the matcher named a class wider than it saw. The
first cut compared the name subprocess to a literal, so import subprocess as sp then
sp.run(...) walked through the guard that exists to catch it, while an argument named
time with a sleep method was falsely flagged. That is this PR's own subject matter
landing inside the PR: a detector reporting clean over part of the class it names. The
receiver is now resolved through the file's own import bindings before it is classified,
which catches the alias and removes the false positive by construction — a receiver the
file never imported is not a module.

Scope decision, stated rather than buried. The stdlib group is not enforced in test
files — 18 existing hits live there, and a blocked loop in an async test is the test's own.
A full-repo run prints how many hits it skipped, so "no stdlib blocking calls" and
"stdlib blocking calls, not enforced here" cannot be confused. A silent exemption and a
clean tree read identically; that is the failure MEASUREMENT_DISCIPLINE.md calls did not
look wearing nothing found, and this PR exists because the guard's original two-item
list did exactly that. requests.* and Path.* stay enforced in test files, unchanged,
and a test pins that the widening did not quietly relax them.

Why an extraction rather than an in-place patch. Both host files sat at their frozen
size ceiling, so the fix was not landable without splitting — and the ratchet's own rule is
split, don't raise. The split is also what makes the property structural rather than a
comment somebody has to notice: api/vnc_blocking_probes.py functions block by contract
and import no asyncio, and in slm/agent/event_buffer.py a connection cannot outlive a
single call, so it cannot be held across an await again.

What Changed

autobot-backend/api/vnc_blocking_probes.py (new) — is_vnc_running,
run_clipboard_write, probe_connection_quality. All blocking, no asyncio import.
autobot-backend/api/vnc_manager.py — all four async callers of a blocking probe now
await asyncio.to_thread(...): the quality-metrics and clipboard endpoints, plus
get_vnc_status and ensure_vnc_running, which were calling is_vnc_running() bare.
The clipboard path replaces Popen(...).communicate(input=...) with
subprocess.run(input=...): same two syscalls in one call, and TimeoutExpired still
reaches the existing handler.
autobot-slm-backend/slm/agent/event_buffer.py (new, + ansible mirror) —
initialize / append / read_unsynced / mark_synced / prune.
autobot-slm-backend/slm/agent/agent.py (+ mirror) — sync_buffered_events schedules
each sqlite call through to_thread and no longer holds a connection across the POST.
autobot-slm-backend/ansible/roles/slm_agent/tasks/main.yml — a copy: task for the
new module; this role deploys one file at a time rather than syncing a tree.
tools/lint/check_no_blocking_io_in_async.py — _MODULE_BLOCKING_CALLS keyed on
(module, attr) so a bare engine.connect() or task.run() stays clean, plus
imported_modules() (resolves aliases, function-local imports, and import os.path
binding os), in_scope(), and the printed skip count.
Baselines — repo_tests/python_file_size_ratchet_baseline.py and
scripts/python_file_size_known_large.py, both ratcheted down:

file	before	after
`autobot-backend/api/vnc_manager.py`	1699	1651
`autobot-slm-backend/slm/agent/agent.py` (+ mirror)	675	630

Round 2 stayed at the 630 pin rather than moving it: the reasoning for the new hop went
into event_buffer, which is not size-capped. vnc_manager.py is unchanged at 1651.

Review round 1 — three Major findings, all confirmed against the code, none dismissed:

buffer_event was called inline from the aiohttp.ClientError handler in
async def _send_heartbeat_request — a sync sqlite insert on the event loop, on the
buffer's hot path, running on every failed heartbeat. Offloaded at the async entry
point; _process_code_change is a sync caller and keeps the sync path.
MAX_BUFFERED_EVENTS was enforced at the top of sync_buffered_events, which the run
loop calls only if success. During an admin outage every failed heartbeat appended a
row and nothing trimmed — the cap was dead in the one situation it exists for. Prune
moves to prune_event_buffer(), called unconditionally each cycle; only the upload stays
gated on connectivity. Not enforced inside append as suggested, because that puts a
COUNT(*) on every insert and gives the cap two enforcement points that can disagree;
this buffer has already been bitten once by a second enforcement path.
The receiver-binding defect above.

Review round 2 — two more, both confirmed by resolving the enclosing function rather than
reading line numbers:

buffer_event has two async callers and round 1 offloaded one. handle_code_change
called _process_code_change bare, reaching sqlite3.connect from an HTTP endpoint. One
hop now wraps the whole sync function — it writes a version file, clears a cache and
appends a row, so three wrappers buy nothing over one. buffer_event stays synchronous
because _process_code_change is a synchronous caller.
is_vnc_running() bare in get_vnc_status and ensure_vnc_running.

Review round 3 — the cap had a second escape, and two of this PR's own pins
certified fixes that do nothing:

Moving prune out of if success: stopped a rejected heartbeat from skipping it.
A heartbeat that times out raises asyncio.TimeoutError, which is not an
aiohttp.ClientError (verified: issubclass(...) is False on aiohttp 3.14.3),
so it escapes the heartbeat's handler into the run loop's catch-all and skips a
prune placed after it. Narrower than "an outage", and the precision matters: on
that path buffer_event is skipped too, since it lives in the same handler, so
the growth comes from handle_code_change appending on every git-hook post. The
failing case is a node whose admin times out while commits keep arriving. Prune
now runs first in the cycle. finally was suggested and declined: a prune
raising inside one escapes the catch-all that exists to keep the loop alive
(Fleet health degraded on fresh install: VM agent dies after early-404 heartbeats + optional assigned roles never activate (vnc detection gap) #9965), trading unbounded growth for a dead agent.
Both offload pins checked that a callable was passed to to_thread, not that
the call was awaited. asyncio.to_thread(f) without await returns a coroutine
that never runs, so f does not execute at all — the pin would have certified a
change that silently stopped processing code-change events entirely. Proved by
mutating the await away and watching both assertions still pass.
The prune pin asserted unconditional, which is not per-cycle: a call before
while self.running satisfied it while pruning once at startup.

Three of this PR's own pins claimed more than they enforced

The substring companion, the unawaited-to_thread pair, and the once-at-startup
prune — in the PR whose subject is detectors that report clean over part of the class
they name. Each was caught by a reviewer applying this PR's standard back to its own
tests, none by a tool. That is the finding worth more than any individual pin: the
class is easier to see in someone else's guard than in the one you just wrote.

Recorded here rather than quietly fixed, because a PR arguing that silent narrowing
and a clean tree read identically cannot then narrow silently.

Also corrected, because this PR's own argument applies to it: the sync_buffered_events
docstring opened "Every sqlite call goes through asyncio.to_thread", which was true of
the method and false of the module while the code-change path bypassed it. A
guarantee-shaped sentence over a partial fact is the prose form of a guard reporting clean
over part of its class. It now states what it covers, and after finding 4 the module-wide
form is true and a test keeps it that way.

After the extraction, agent.py contains zero sqlite3 references — all four sites the
review counted live in event_buffer.py, and both agent copies stay byte-identical.

Bug found while moving the agent code: the buffer prune reached a @staticmethod whose
db_path defaulted to DEFAULT_BUFFER_DB, invoked with no argument. An agent configured
with a non-default buffer_db pruned the default file, so its own buffer grew past the
500-row cap with nothing enforcing it. prune now takes the path with no default.

Verification

66 tests pass — 32 guard tests, 18 event-buffer/agent call-site tests, 4 VNC
call-site tests, 12 pre-existing.
8 binding-resolution tests: every kind under an alias, a local shadowing a module
name, a function-local import, a dotted import binding its root, and the map itself.
4 call-site tests over both agent copies, asserted against the AST on purpose: a
behavioural test passes either way, because prune trims correctly whether or not the run
loop reaches it and the insert succeeds whether or not the loop was free. Reachability and
scheduling live at the call site, so that is where they are pinned.
Guard tests cover: every stdlib kind flagged inside async def; the same calls ignored
in a sync def; ignored one frame down in a sync helper (the canonical fix shape, which
must pass or the fix cannot be written); the (module, attr) qualifier; the noqa; and
all three test-path spellings.
Mutation-proved — each of these fails a named test: dropping Popen from the table,
matching on attr alone, narrowing the test-path predicate to *_test.py, relaxing
requests.* into the exempt set, restoring the prune default, dropping the empty-ids
guard, reverting to literal-name matching (5 tests), re-gating prune under if success,
calling buffer_event inline again, un-offloading either is_vnc_running caller, and
un-offloading _process_code_change, moving prune back after the heartbeat, moving
prune to once-at-startup, and dropping the await from a to_thread call.
The empty-ids test watches sqlite3.connect rather than the row count, because SQLite
accepts IN () and the obvious row-count assertion passes with the guard removed.
Prune fix proved with two databases: pruning one leaves the other at 4 rows.
Production stdlib blocking count: 0 of 0. The 192 remaining violations are the
pre-existing Path.* backlog, unchanged by this PR; the 18 skipped test-file hits are
printed, not hidden. Re-measured after binding resolution: still 192 + 18, so no aliased
blocking call exists in the tree today — the hole the fix closes happened to be empty,
which is the right time to close one.
Mirror pairs byte-identical: 15/15 (pipeline-scripts/detect-agent-code-drift.sh).
File-size ratchet clean against the re-pinned ceilings (scripts/check_python_file_size.py).

Filed, not patched (different scope): #17648
— autobot-slm-backend/conftest.py installs a hollow api package into sys.modules at
conftest import time and never removes it, so any pytest selection spanning that tree and
another fails repo_tests/sys_modules_leak_guard.py. CI shards autobot-slm-backend alone,
so it never sees it; co-located-smoke.yml already carries the comment "Separate pytest
process: autobot-slm-backend/conftest.py stubs". No baseline line added — it only shrinks.

Model Used

Claude Opus 5 (1M context)

Closes #17646

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- SLM agents now retain events when a heartbeat fails, upload them after connectivity returns, and remove older events when the configured buffer limit is exceeded.
- VNC connection status, clipboard updates and connection-quality checks continue to run without holding up other app activity.
Bug Fixes
- Improved reliability of VNC checks and SLM event handling by moving time-consuming operations away from the main event loop.
Tests
- Added coverage for event buffering, pruning, and non-blocking operations.

…guard that could not see them (#17646) `tools/lint/check_no_blocking_io_in_async.py` knew two vocabularies, `requests.*` and `Path.read/write_text/bytes`, and passed clean over every file in this diff. The codebase-analytics index reported 8 high-severity `performance_blocking_io_in_async` findings in those same approved files. Four production sites, all on the event loop: - the VNC quality-metrics endpoint held three blocking probes -- a `pgrep` at `timeout=5`, a socket connect at `settimeout(2)`, another `pgrep` at `timeout=5` -- so one request could stall every other request for ~12s - the VNC clipboard endpoint ran `Popen(...).communicate(timeout=5)` - the SLM agent's event sync ran three `sqlite3.connect` calls and held one connection open across an `await` on a 30-second POST, so a slow admin held both the loop and a database handle for the whole timeout Both endpoints' blocking work moves into `api/vnc_blocking_probes.py`, whose functions block by contract and import no `asyncio`; the agent's moves into `slm/agent/event_buffer.py`, where a connection cannot outlive one call and so cannot be held across an await. Both host files sat at their frozen size ceiling, so this splits rather than raising it: vnc_manager 1699 -> 1653, agent.py 675 -> 631, and both baselines ratchet down. The guard now matches `subprocess.run/call/check_call/check_output/Popen`, `sqlite3.connect`, `time.sleep`, `os.system` on `(module, attr)` -- a bare `engine.connect()` or `task.run()` stays clean, which is what keeps this a gate rather than a nuisance. The stdlib group is not enforced in test files, where a blocked loop is the test's own; that narrowing is *counted and printed*, because a silent exemption and a clean tree read identically. Production stdlib count is now 0 of 0; the 192 remaining are the pre-existing `Path.*` backlog, unchanged. What the widening does NOT reach, recorded at both sites rather than left implicit: `is_vnc_running` is a *sync* function called from async, and the visitor resets its async depth inside `def` bodies -- correctly, since a sync helper may have sync callers. Blocking work one frame below an `async def` is invisible however wide the call list gets. That needs reachability, not matching. Found while moving the agent code: the buffer prune reached a `@staticmethod` whose `db_path` defaulted to `DEFAULT_BUFFER_DB` and was invoked with no argument, so an agent with a configured `buffer_db` pruned the default file and let its own buffer grow past the cap unbounded. Prune now takes the path with no default, proved by a two-database test. Tests: 24 new guard tests (each stdlib kind flagged in async, ignored in sync, ignored one frame down in a sync helper, the module qualifier, the noqa, and all three test-path spellings) and 9 event-buffer tests. Mutation-proved: dropping `Popen` from the table, matching on attr alone, narrowing the test-path predicate to `*_test.py`, relaxing `requests.*` into the exempt set, restoring the prune default, and dropping the empty-ids guard each fail a named test. The empty-ids test watches `sqlite3.connect` rather than the row count, because SQLite accepts `IN ()` and a row-count assertion passes with the guard removed. Closes #17646

coderabbitai · 2026-09-27T17:33:52Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 21 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 7155b444-c655-4655-8ee9-88b28238905e

📥 Commits

Reviewing files that changed from the base of the PR and between fdddfc1 and 582d10d.

📒 Files selected for processing (9)

autobot-backend/api/vnc_blocking_probes_call_sites_test.py
autobot-backend/utils/chromadb_client.py
autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/agent.py
autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/event_buffer.py
autobot-slm-backend/slm/agent/agent.py
autobot-slm-backend/slm/agent/event_buffer.py
autobot-slm-backend/tests/test_agent_event_buffer_7444.py
tools/lint/check_no_blocking_io_in_async.py
tools/lint/check_no_blocking_io_in_async_test.py

📝 Walkthrough

Walkthrough

The change moves VNC probes, clipboard writes, and SLM event-buffer database operations to worker threads. It adds event-buffer helpers used by both SLM agent copies and extends the async blocking-I/O checker.

Changes

VNC blocking operations

Layer / File(s)	Summary
VNC probes and endpoint offloading `autobot-backend/api/vnc_blocking_probes.py`, `autobot-backend/api/vnc_manager.py`, `autobot-backend/api/vnc_blocking_probes_call_sites_test.py`	Adds helpers for VNC status, connection-quality probes, and clipboard writes. Manager endpoints run these helpers in worker threads. AST tests check that the helpers are not called inline from async functions or passed to `to_thread` without awaiting it.

SLM event buffering

Layer / File(s)	Summary
Event-buffer persistence `autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/event_buffer.py`, `autobot-slm-backend/slm/agent/event_buffer.py`	Adds SQLite operations to initialise the event table, append and read events, mark events synced, and prune rows above the configured cap. Batch size and buffer cap have environment-variable defaults of 100 and 500.
Agent buffering and deployment `autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/agent.py`, `autobot-slm-backend/slm/agent/agent.py`, `autobot-slm-backend/ansible/roles/slm_agent/tasks/main.yml`	Both agents delegate buffer operations to the helper and offload blocking database calls. Each run-loop cycle prunes the configured database before the heartbeat attempt. The Ansible role deploys the helper.
Buffer and call-site tests `autobot-slm-backend/tests/test_agent_event_buffer_7444.py`	Tests buffer persistence, pruning against the supplied database path, and agent call-site use of worker threads.

Async blocking-I/O checks

Layer / File(s)	Summary
Import-aware blocking-call detection `tools/lint/check_no_blocking_io_in_async.py`	Adds detection for selected subprocess, SQLite, sleep, and system calls based on imported modules. Selected standard-library violations are excluded in test paths, and full-repository runs report the excluded-hit count.
Blocking-call checker tests `tools/lint/check_no_blocking_io_in_async_test.py`	Tests detection, import and alias resolution, synchronous-call exclusions, and test-path scope rules.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: 🟡 Moderate · up to fdddf

The blocking-work offloads are in place. However, a malformed or out-of-range buffer setting can prevent the SLM agent from starting or can discard buffered events, and a buffer fault can suppress heartbeats. Fix these before merging. The lint guard also still misses directly imported blocking functions.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to fdddf

Moving blocking work off the event loop improves responsiveness, but the new offline pruning behavior can discard unsent node events, including code-change events. The loss is bounded to an affected agent’s buffer; no new authentication bypass was established.

Retained concerns

Medium · reliability · inferred: Pruning during an admin outage now deletes the oldest buffered events irrespective of delivery state. Repeated failures or concurrent code-change notifications can therefore remove unsent events before recovery, leaving the server without that portion of the node’s history.

Security review details

Security Blast Radius

inferred — The established event-loss path affects the local buffer of an agent experiencing sustained failures or excess event volume; fleet-wide incidence depends on outage and configuration conditions not established here.

Security Findings and Attack Paths

observed — No newly introduced authentication bypass is established. The event-sync endpoint’s reliance on network access and a submitted node ID, rather than endpoint authentication, predates this PR; its actual network exposure was not established.

Trust Boundaries and Controls

observed — The changed VNC call sites retain admin-permission checks. Fixed process arguments and probe timeouts limit individual probe work, although concurrent authorized requests share executor capacity.

Resilience and Maintainability Implications

inferred — The retention cap contains offline disk growth, but deletion without delivery-state checks or a reported drop count can conceal gaps in node-change history after connectivity returns.

Hardening Proposals

proposed — Define the retention policy for unsent code-change events, report buffer depth and discarded-event counts, and validate positive buffer and batch limits before enabling the agent.
proposed — Separately address the pre-existing event-sync contract so acknowledgement reflects accepted events and retries have a stable server-side identity.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The implementation evidence supports [`#17646`]: the VNC probes and SLM SQLite work use `asyncio.to_thread`; the guard resolves `(module, attr)` for the required stdlib calls; test-file hits are counted…	Strengthen the VNC call-site test. Require an awaited `asyncio.to_thread(probe)` expression. Reject unawaited offloads and direct probe invocation before argument passing. Run the affected tests.
Out of Scope Changes check	⚠️ Warning	The new `SLM_MAX_BUFFERED_EVENTS` and `SLM_SYNC_BATCH_SIZE` environment configuration is not required by directly linked issue [`#17646`]. The PR summary identifies this work as addressing a separate cr…	Remove the environment-configuration changes from this pull request, or provide a directly linked active issue that requires them and assess that issue separately.
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 93 functions across 10 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarises the main changes: moving four production blocking calls off the event loop and extending the async blocking-call guard.

Full details: Linked Issues check

Explanation

The implementation evidence supports [#17646]: the VNC probes and SLM SQLite work use asyncio.to_thread; the guard resolves (module, attr) for the required stdlib calls; test-file hits are counted; pruning uses the configured path; and the sync-helper blind spot is documented. However, the VNC call-site test still accepts an unawaited to_thread call and a probe call evaluated before it is passed to to_thread. The test therefore does not reliably enforce the required offload form.

Full details: Out of Scope Changes check

Explanation

The new SLM_MAX_BUFFERED_EVENTS and SLM_SYNC_BATCH_SIZE environment configuration is not required by directly linked issue [#17646]. The PR summary identifies this work as addressing a separate criterion from [#16056], which is not supplied as a directly linked active issue. The buffer refactor and its tests are in scope, but the additional configurability is a separate objective.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-09-27T17:35:40Z

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

coderabbitai

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟠 Major · Offload the VNC status probe in both async handlers. · vnc_manager.py:180

autobot-backend/api/vnc_manager.py:180
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Offload the VNC status probe in both async handlers.

When either endpoint calls is_vnc_running() directly, its pgrep call can block the event loop for up to five seconds. Use await asyncio.to_thread(is_vnc_running) in both handlers. The new helper does not make its direct callers non-blocking.

Also applies to: 196-196

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 516d2517-a66c-48cf-94de-38971dbebfe5

📥 Commits

Reviewing files that changed from the base of the PR and between 969c195 and a9117f6.

⛔ Files ignored due to path filters (2)

repo_tests/python_file_size_ratchet_baseline.py is excluded by !repo_tests/python_file_size_ratchet_baseline.py
scripts/python_file_size_known_large.py is excluded by !scripts/python_file_size_known_large.py

📒 Files selected for processing (10)

autobot-backend/api/vnc_blocking_probes.py
autobot-backend/api/vnc_manager.py
autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/agent.py
autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/event_buffer.py
autobot-slm-backend/ansible/roles/slm_agent/tasks/main.yml
autobot-slm-backend/slm/agent/agent.py
autobot-slm-backend/slm/agent/event_buffer.py
autobot-slm-backend/tests/test_agent_event_buffer_7444.py
tools/lint/check_no_blocking_io_in_async.py
tools/lint/check_no_blocking_io_in_async_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

mrveiss · 2026-09-27T17:50:58Z

Approve. Verified against head `a9117f6613`, not the PR body.

The three things I was asked to check are all correct, and the one thing I want
added is the PR's own standard applied to itself.

What I verified independently

File-size baselines — correct, and in both files. The body's 1699→1651 and
675→631 match the tree exactly (I was briefed "1653"; the PR is right, the brief was
not). Three entries, not two, and both baseline files carry all three:

entry	`python_file_size_ratchet_baseline.py`	`python_file_size_known_large.py`	tree
`api/vnc_manager.py`	`:152` 1651	`:147` 1651	1651
`slm/agent/agent.py`	`:483` 631	`:478` 631	631
ansible mirror `agent.py`	`:443` 631	`:438` 631	631

Shrink-only in every case, and the mirror entry was not forgotten — which was the
thing most likely to be missed, since it lives 40 lines away from its twin.

The duplicate agent.py — handled properly. Both copies were byte-identical on
main (9811050d86) and both changed identically here, with event_buffer.py created
on both sides and a copy: task added at roles/slm_agent/tasks/main.yml so the
provisioned node actually receives the new module. The drift guard reporting 15/15
is the right evidence for this, and it is cited.

The prune bug is real and the description is accurate. self._prune_old_events()
called with no argument against a staticmethod whose db_path defaulted to
DEFAULT_BUFFER_DB: an agent configured with a non-default buffer_db pruned the
default file and let its own buffer grow past the cap unbounded. Worth noting
that this bug lived inside one of the functions the guard cannot see, so it was found
by reading rather than by tooling — which is the strongest available argument for the
blind-spot section being in the body at all.

The blind-spot section says what it needs to. "It covers more of the class, not
all of it", plus the reachability-analysis point, at the right level of specificity.
I would only add a number, because a count is unarguable where a qualitative caveat
is not: in slm/agent/agent.py before this PR there were four sqlite3.connect
sites and the widened guard could see one of them.

Not raising the stdlib test exemption. Stated, counted, and printed. That is the
opposite of the failure mode, and it is the correct call.

One thing to add before merge — not a scope change

event_buffer.append is the only one of the five extracted functions that is not
scheduled off the loop, and it is reachable from async on two paths:

path	frames
`agent.py:268` — `self.buffer_event("heartbeat", payload)` in `async def _send_heartbeat_request`	one
`agent.py:419` → sync `_process_code_change` → called at `:448` from `async def handle_code_change`	two

prune, read_unsynced and mark_synced all get asyncio.to_thread at :306,
:307, :330. append does not, and event_buffer.py:58-60 is a plain def
around sqlite3.connect.

To be fair about scope: this is pre-existing and outside the four sites this PR
names. buffer_event was already sync-sqlite-from-async on main, the guard could
never see it — it is a sync def, so the depth reset applies before and after this
change, and nothing here made that worse. I am not asking for it to be fixed in
this PR.

What I am asking is that it be stated, for one reason: this PR's own argument is
that a silent exemption and a clean tree read identically. Two things currently work
against a reader reaching that conclusion:

The docstring at agent.py:289 opens with "Every sqlite call goes through
asyncio.to_thread". That is true of sync_buffered_events, where it sits, but
it reads as a claim about the module — and append is the counterexample, one
screen above it.
agent.py no longer contains the token sqlite3 anywhere except that docstring.
That does not change what the guard sees, but it does mean a human grepping the
file for blocking storage calls now finds nothing.

A line in the extraction bullet — append stays on the caller's thread; its two
async callers are :268 and :419→:448, tracked in #NNNN — costs one sentence
and closes it.

Same shape on the VNC side, also pre-existing

is_vnc_running does subprocess.run at vnc_blocking_probes.py:33 and is called
bare from vnc_manager.py:180 (async def get_vnc_status) and :196
(async def ensure_vnc_running), while run_clipboard_write (:637) and
probe_connection_quality (:721) are both wrapped.

Inside the quality endpoint this is correct and deliberate — is_vnc_running now
runs within probe_connection_quality, which is itself one to_thread hop, and
the body's "three hops would buy nothing over one" is right. It is the other two
endpoints that still call it on the loop. Also pre-existing, also not for this PR,
and the body already names is_vnc_running as its blind-spot example — so it is
half-stated already. Finishing that sentence with the two call sites would make it
whole.

Verdict

Approving. The four named sites are genuinely off the loop, the guard widening is
sound on (module, attr), both baselines ratchet down rather than up, the mirror is
handled with a guard rather than a promise, and the prune fix is a real bug caught in
passing. The two remaining sites are adjacent work, correctly outside this scope —
they just should not be silent, by this PR's own standard.

…paths a call-site never reached (#17646) Three Major findings from the #17647 review. All three confirmed against the code, none dismissed. 1. The widened matcher compared the *name* `subprocess` to a literal, not the module. `import subprocess as sp` then `sp.run(...)` in an `async def` walked straight through the guard that exists to catch it, and an argument named `time` with a `sleep` method was falsely flagged. That is the same "detector sees less than it names" class this PR documents, introduced by the widening itself. `imported_modules()` now maps every name an `import` binds -- including aliases, and including function-local imports, where a blocking call tends to be written -- and Pattern 3 resolves through that map before it classifies. A receiver the file never imported is not a module and is skipped, which removes the false positive by construction rather than by exception. Re-measured after the change: the tree is unchanged at 192 + 18, so no aliased blocking call exists today -- a measurement, not an assumption. 2. `buffer_event` was called inline from the `aiohttp.ClientError` handler in `async def _send_heartbeat_request`: a synchronous sqlite insert on the event loop, on the buffer's hot path, running on *every* failed heartbeat -- that is, exactly when the admin is unreachable. Offloaded at the async entry point, not inside `buffer_event`, because `_process_code_change` is a sync caller and must stay one. This site is the blind spot this PR describes, found by review rather than by the guard, which is the evidence that widening a call list does not reach it. 3. `MAX_BUFFERED_EVENTS` was enforced at the top of `sync_buffered_events`, and the run loop calls that only `if success`. So during an admin outage every failed heartbeat appended a row and nothing ever trimmed: the cap was dead in the one situation it exists for. Prune moves to `prune_event_buffer()`, called unconditionally each cycle; only the upload stays gated on connectivity. Not enforced inside `append` as suggested, because that puts a `COUNT(*)` on every insert and gives the cap two enforcement points that can disagree -- the reasoning is recorded on `event_buffer.prune`. Tests: 8 binding-resolution tests (each kind under an alias, a local shadowing a module name, a function-local import, a dotted import binding its root, and the map itself) and 4 call-site tests over both agent copies. The call-site pair is asserted against the AST on purpose: a behavioural test passes either way, since prune trims correctly whether or not the loop reaches it and the insert succeeds whether or not the loop was free. Reachability and scheduling live at the call site, so that is where they are pinned. Mutation-proved: reverting to literal-name matching fails 5 tests; re-gating the prune under `if success` fails its test in the copy mutated; calling `buffer_event` inline again fails its test. agent.py 631 -> 630, re-pinned in all three baseline entries (canonical, ansible mirror, and vnc_manager unchanged at 1651). Mirror pairs 15/15 byte-identical. Refs #17646

mrveiss · 2026-09-27T18:05:24Z

Re-review at head `3634c656bc`. Still approve — one item is now inside scope rather than adjacent.

Every number re-measured against the pinned head SHA, not a branch ref.

claim	verified
`sqlite3` occurrences in `slm/agent/agent.py`	0
`api/vnc_manager.py`	1651
both `agent.py` (canonical + ansible mirror)	630, 630
baseline entries, both files	`:152`/`:443`/`:483` and `:147`/`:438`/`:478` — six, all consistent

Moving the whole sqlite concern out of the mirrored file is a better answer than wrapping
the three visible calls: it makes the byte-identical-copy divergence risk moot rather than
managed, because there is now no storage code in the file that gets mirrored.

The except aiohttp.ClientError detail is worse than what I reported, and the AST check
is why. I had :268 as "reached from an async path". It is reached from the failure
handler — so it blocked the loop on every failed heartbeat, which is precisely when the
admin is unreachable and heartbeats are failing in a row. A line-based caller scan cannot
distinguish those two, and this is the case where the difference decides severity.

:270 now offloads it, and the comment at :268-269 gives the right reason for offloading
at the caller rather than inside buffer_event — the other caller is sync, so wrapping the
method would break it.

On prune: the per-cycle placement is the right call. A COUNT(*) on every insert plus a
second enforcement point that can disagree with the first is a worse failure mode than a
bound that is per-cycle rather than per-insert, and this buffer has already been bitten once
by a second enforcement path. Stating the residual as capped per cycle, not per insert is
what makes it a decision rather than an oversight.

The one item: `buffer_event`'s other async caller is still on the loop

This was my "path 2" and it is unchanged:

:447 — self._process_code_change(commit, branch, message) called bare from
async def handle_code_change, an aiohttp request handler
:402 _process_code_change is sync and does three blocking things: save_version(...),
clear_cache(), then buffer_event(...) → event_buffer.append → sqlite3.connect

Why this is now in scope rather than adjacent. Before the fix push, buffer_event was
pre-existing work outside the four named sites and I explicitly did not ask for it. This push
brought buffer_event into the PR's scope and fixed one of its two async callers. Leaving the
other is now an internal inconsistency in the change rather than deferred work — the same
function, the same defect, one caller offloaded and one not.

Suggested shape, from this PR's own precedent: one to_thread hop around the whole
_process_code_change(...) call at :447, not a wrap inside buffer_event. All three of its
operations are blocking and sequential, so the reasoning already used for
probe_connection_quality applies unchanged — three hops buy nothing over one. Note also
that :448, the very next line, already uses async_compat.fire_and_forget for
_notify_code_change, so the offload is present one line below the call that needs it.

`is_vnc_running` — the finishing sentence, as asked

Unchanged on this head, and still bare from two async callers while its two siblings are
wrapped:

call site	enclosing	offloaded
`vnc_manager.py:180`	`async def get_vnc_status`	no
`vnc_manager.py:196`	`async def ensure_vnc_running`	no
`:637` `run_clipboard_write`	`async def vnc_clipboard_sync`	yes
`:721` `probe_connection_quality`	`async def get_connection_quality_metrics`	yes

Inside the quality endpoint this is correct and deliberate — is_vnc_running runs within
probe_connection_quality, which is itself one hop. It is get_vnc_status and
ensure_vnc_running that still call subprocess.run on the loop, via
vnc_blocking_probes.py:33.

Pre-existing, and genuinely outside the four named sites, so not a merge blocker. The body
already names is_vnc_running as its blind-spot example, which makes it half-stated; adding
"its two remaining bare callers are vnc_manager.py:180 and :196, tracked in #NNNN"
finishes the sentence and means the next reader does not have to re-derive it. That is the
same standard this PR applies to the stdlib exemption, applied to its own example.

…re VNC probes (#17646) Review found `buffer_event` has **two** async callers and the previous push offloaded one. `handle_code_change` -- an aiohttp request handler -- called `_process_code_change` bare, which writes a version file, clears a cache and appends a row through `sqlite3.connect`. A fix that covers one of two paths is not a fix, and this one was reachable from an HTTP endpoint. The hop wraps the whole of `_process_code_change` rather than the `buffer_event` call inside it: three wrappers buy nothing over one for three sequential blocking operations, the same reasoning the quality endpoint uses. `buffer_event` itself stays synchronous because `_process_code_change` is a synchronous caller, and the rule now lives on `event_buffer.append` where both paths can see it. Two more sites of the same shape in `vnc_manager.py`, in the file this PR already owns: `get_vnc_status` and `ensure_vnc_running` called `is_vnc_running()` bare -- a `pgrep` at `timeout=5` on the event loop -- while the quality endpoint and the clipboard endpoint wrapped theirs. Both now offload. That is three of one defect fixed once in one place rather than rediscovered per endpoint. Also corrected, because this PR's own argument applies to it: the `sync_buffered_events` docstring opened "Every sqlite call goes through `asyncio.to_thread`", which was true of the method and false of the module while the code-change path existed. A sentence that reads as a module-wide guarantee while one path bypasses it is the same failure as a guard reporting clean over part of its class. It now says what it covers, and after this commit the module-wide form is true. Tests: the agent call-site pin extends to both async callers, and a new `vnc_blocking_probes_call_sites_test.py` asserts that no `async def` in `vnc_manager.py` reaches any of the three sync probes without a `to_thread` hop -- with a companion test that fails if the probes stop being referenced at all, so an empty result cannot read as a clean one. Both pins are AST assertions on purpose: a bare `is_vnc_running()` returns the same boolean as an offloaded one, and the code-change insert succeeds either way. What differs is whether the loop was free while it happened, and only the call site records that. This is also the instrument the widened guard cannot be: these are sync functions reached from async, one frame below the depth reset. Mutation-proved: un-offloading either `is_vnc_running` caller fails the vnc pin; un-offloading `_process_code_change` fails the agent pin. agent.py stays at its 630 pin -- the reasoning moved into `event_buffer`, which is not size-capped, rather than the ceiling moving. vnc_manager unchanged at 1651. Mirror pairs 15/15 byte-identical. Refs #17646

mrveiss

Delta review only, 3634c656bc..b886dd928b — 7 files, +142/−16. Earlier passes not re-done.

Two findings. One is in the delta's own new test; the other is a ratchet the delta made stale.

1. Two ceilings are now stale, and this PR is what made them stale

Measured in the tree rather than from the claim:

file	`origin/main`	this head	declared ceiling
`autobot-slm-backend/slm/agent/agent.py`	675	630	675 (`scripts/python_file_size_known_large.py:478`)
`…/ansible/roles/slm_agent/files/slm/agent/agent.py`	675	630	675 (`:438`)
`autobot-backend/api/vnc_manager.py`	1699	1651	1699 (`:147`)

On main each file sits exactly at its ceiling. This PR shrinks them by 45 and 48 lines and leaves both ceilings where they were. scripts/check_python_file_size.py's own rule, line 24: "below its ceiling → fail; lower the ceiling to the count just achieved", and line 27: every lowering is mirrored in RATCHET_BASELINE in the ratchet test, "so the shrink is locked in rather than left as headroom to regrow into (#14498)".

So the PR has earned a 45-line and a 48-line reduction and not banked either. The next change to these files may regrow into that headroom with nothing objecting — which is the exact outcome #14498 exists to prevent.

"The reasoning moved into event_buffer, which is not size-capped, rather than the ceiling moving" explains why the code shrank. It does not explain why the recorded number should stay 45 above the file. The ratchet is about the count, not about where the logic went.

What I did not establish: whether the pre-commit path enforces the lower bound or only --audit-ceilings does — the module's docstring (:92) says the tree walk "runs independently of pre-commit". If only the audit enforces it, that is a second finding and a bigger one, because a shrink can then land unbanked on any PR. The discriminator is reading the checker's per-file branch, which I have not done.

2. The companion test is satisfied by the import line, so it cannot catch what it exists for

vnc_blocking_probes_call_sites_test.py:68 — test_the_probes_are_actually_reached_from_async:

reached = [p for p in _BLOCKING_PROBES if f"{p}" in source]
assert reached == list(_BLOCKING_PROBES), f"probe(s) no longer referenced: {reached}"

That is a plain substring test over the whole file, and the probes are imported by name at vnc_manager.py:55-57. So the import statement alone satisfies it. Delete every call site and the sweep at :59 passes vacuously and the companion still passes, because from … import is_vnc_running keeps the substring present.

The test's name claims reached from async. What it checks is the string appears somewhere in the file. To hold the property, count the probe's call sites — the machinery is already there: _async_functions() (:35) and the call-collection in _bare_calls (:40-52) both walk the AST, so the companion can assert len(calls) > 0 per probe from within an async def instead of doing a substring scan. Mutation form: deleting the four call sites must fail a named test.

That is the same shape as the class on #15826 and #17650 — an assertion satisfied by something weaker than the thing it names.

Confirmed, and the reasons hold

The pins read call sites, not behaviour, as intended: ast.parse at :36, ast.AsyncFunctionDef walk at :37, and offenders computed as all ast.Name calls minus those inside a to_thread call (:44-52). Correct instrument — a bare is_vnc_running() returns the same boolean as an offloaded one, so only the call site records whether the loop was free.
ast.Name is the right shape here, not an under-reach: the three probes are imported names (:55-57) and called bare, not as attributes. Worth noting the limit anyway — a future module.is_vnc_running() or self._probe() would not be seen by this detector.
All four call sites are offloaded at this head: :180 and :196 (is_vnc_running, the two 58 fixed rather than documented), :637 (run_clipboard_write), :721 (probe_connection_quality).
The mirror is byte-identical: agent.py and event_buffer.py under slm/agent/ and under ansible/roles/slm_agent/files/slm/agent/ have the same blob hashes at this head.
Fixing is_vnc_running rather than documenting it was the right call. A body sentence naming two unfixed callers, in a file the PR already owns, moves the defect into prose.

Verdict

The delta is sound on its subject. The two stale ceilings should be banked in this PR — it is the change that earned them, and the next one will not know the headroom was unearned. The companion test should count call sites rather than substrings; it is three lines from the machinery already in the file.

The companion test added alongside the VNC call-site sweep asked whether each probe's name appeared anywhere in `vnc_manager.py`. An `import` line satisfies that. So with every async call to `is_vnc_running` deleted and only the import left, the check passed while its name promised the probe was reached from async -- which is the exact name-claims-more-than-mechanism defect the sweep it guards exists to prevent, written into the guard for the sweep. Proved rather than argued. Mutating both call sites to constants leaves the name present once, in `from api.vnc_blocking_probes import is_vnc_running`: OLD substring predicate on the mutated file: PASSES name still present only via: ['is_vnc_running,'] NEW predicate: 1 failed, 5 passed It now walks the AST and requires each probe to be *called* from at least one `async def`, offloaded or not, so the sweep above it cannot go quietly vacuous: a probe that is renamed, moved, or no longer reached from async fails this test and the pin gets re-pointed instead of silently satisfied. Worth stating in the file rather than only in the log, because a non-vacuity check written vacuously buys nothing and reads like coverage -- the same reason this PR counts its skipped test-file hits instead of omitting them. Raised in review of #17647 by a peer applying this PR's own standard to its tests. That standard is the right one: a sweep asserting "no bad call sites" is satisfied by a file with no call sites. Refs #17646

mrveiss

Retracting Finding 1 — the ceilings are banked in this PR. My measurement was wrong.

I read the line counts at the PR head and the ceilings from the working tree, which is a checkout of main. One side current, one side stale. At b886dd928b both records carry the shrunk values:

scripts/python_file_size_known_large.py:147   api/vnc_manager.py            1651
                                     :438   …/files/slm/agent/agent.py     630
                                     :478   slm/agent/agent.py             630
repo_tests/python_file_size_ratchet_baseline.py:152 / :443 / :483   1651 / 630 / 630

Exact matches to the head's counts, in both the hook's list and the mirrored ratchet baseline, so the shrink is locked in as #14498 requires. The 1699 / 675 / 675 I quoted are main's values. Nothing was unbanked and there is no headroom to regrow into.

This is the error I flagged on #17089 four hours ago — a finding that is well-formed, specific and about a state that does not exist — and it is worse here, because that one I caught before publishing and this one I published on someone else's PR. A false "you left this undone" costs the author a trip to verify work they had already done, with a reviewer's verdict saying they had not.

The mechanism is worth naming because it is the third variant of it today: git show <head>:<file> for one side and a bare path for the other. A review that compares two revisions must name the revision on both sides of every comparison — the working tree is not a revision, it is whatever the last checkout left.

Apologies for the noise. Finding 2 stands and I see 58 had already proved it by mutation before my review landed: stubbing both call sites left the old predicate passing on 'is_vnc_running,' from the import line alone. Its rewrite — walk the AST and require each probe to be called from at least one async def — is the right instrument, and "a non-vacuity check written vacuously is worse than no check, because it occupies the slot where one would go" is the better statement of it.

My secondary question is unaffected and still open: whether the staged-file path enforces the below-ceiling rule, or only --audit-ceilings does. The audit side is confirmed in CI (code-quality.yml:701, ratchet-base-guard.yml:86); the pre-commit branch is unestablished and I have not read it.

Everything else in the delta review holds: the pins read call sites via AST rather than behaviour, ast.Name is the correct shape for imported-and-called-bare probes, all four probe calls are offloaded at this head, and the ansible mirror is byte-identical for both files.

coderabbitai

Actionable comments posted: 3

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 37b7ab4a-7c2a-4a48-81a3-6bce8a45c48b

📥 Commits

Reviewing files that changed from the base of the PR and between a9117f6 and afa66d1.

⛔ Files ignored due to path filters (2)

repo_tests/python_file_size_ratchet_baseline.py is excluded by !repo_tests/python_file_size_ratchet_baseline.py
scripts/python_file_size_known_large.py is excluded by !scripts/python_file_size_known_large.py

📒 Files selected for processing (9)

autobot-backend/api/vnc_blocking_probes_call_sites_test.py
autobot-backend/api/vnc_manager.py
autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/agent.py
autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/event_buffer.py
autobot-slm-backend/slm/agent/agent.py
autobot-slm-backend/slm/agent/event_buffer.py
autobot-slm-backend/tests/test_agent_event_buffer_7444.py
tools/lint/check_no_blocking_io_in_async.py
tools/lint/check_no_blocking_io_in_async_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

…ls (#17646, #16056) Found by asking which open issues this PR incidentally touches. #16056's AC5 asks for `max_events` to be env-var-backed "consistent with `DEFAULT_BUFFER_DB`", and the extraction had carried both numbers across as bare literals -- so a fleet-wide buffer cap could still only be changed by shipping code. That is the project's no-hardcoding rule, and the violation was introduced by this PR's own new module, not inherited. `SLM_MAX_BUFFERED_EVENTS` and `SLM_SYNC_BATCH_SIZE`, defaults unchanged at 500 and 100, following the `os.getenv` shape `DEFAULT_BUFFER_DB` already uses one file over. This settles the literal and nothing else in #16056: that issue's substance is that a node reports neither its buffer depth nor its cumulative drops, and the SLM persists and surfaces neither. None of that is in scope here and it stays open. Recorded on the constant so the next reader does not mistake one ticked criterion for the issue. Tests: each limit is read from its env var (via a module reload, which is how an import-time `getenv` is observable at all, with a teardown reload so no later test inherits a 7-row cap), and the defaults are pinned unchanged so env-backing cannot quietly alter behaviour for existing nodes. Mutation-proved: reverting either constant to a literal fails its test. Refs #17646 Refs #16056

… inert fixes (#17646) Round 3 of #17647's review. One Major and two labelled Trivial; the second Trivial is not trivial and the label is the only thing wrong with it. **The cap could still be skipped.** Moving prune out of `if success:` stopped a *rejected* heartbeat from skipping it. A heartbeat that times out raises `asyncio.TimeoutError`, which is not an `aiohttp.ClientError` -- verified against aiohttp 3.14.3, `issubclass(...) == False` -- so it escapes the heartbeat's own handler, lands in the run loop's catch-all, and skips a prune placed after it. Narrower than "an outage" and worth stating precisely: on a timeout the heartbeat path appends nothing either, because `buffer_event` sits in the ClientError handler that also does not run. The growth comes from the other path -- `handle_code_change`, the HTTP endpoint, appends on every git-hook post independently of heartbeats. So the failing case is a node whose admin times out while commits keep arriving, and it grows unbounded. Prune now runs first in the cycle, ahead of anything that can raise past it. `finally` was the suggestion and would also survive the timeout, but a prune raising inside a `finally` escapes the catch-all that exists to keep the loop alive (#9965); ordering costs nothing and cannot fail that way. **Two pins that passed on fixes doing nothing.** `asyncio.to_thread(f)` without `await` returns a coroutine that never runs, so `f` does not execute at all. Both offload assertions checked only that the callable was *passed* to `to_thread`. Proved rather than argued -- mutate the `await` away and both original assertions still pass: CURRENT pin on an inert (unawaited) fix: no-direct-call assertion: passes handed-to-to_thread assertion: passes => PIN PASSES ON A NO-OP FIX Worse than a missing pin: it would have certified a change that silently stopped processing code-change events altogether. Both now require the `to_thread` call to be the operand of an `await`. The prune pin asserted unconditional, which is not per-cycle: a call placed before `while self.running` satisfied it while pruning once at startup. It now requires containment in the loop *and* precedence over the heartbeat. That makes three pins in this PR that claimed more than they enforced -- the substring companion, and these two -- in the PR whose subject is detectors that do exactly that. Each was found by someone applying the standard back to my tests, which is the argument for the standard. Mutation-proved, each failing a named test: prune after the heartbeat, prune once at startup, and an unawaited `to_thread`. agent.py stays at its 630 pin; the timeout reasoning lives on `event_buffer.prune`, which is not size-capped. Mirror pairs 15/15 byte-identical. Refs #17646

mrveiss · 2026-09-27T19:02:53Z

Round four at `f11e0977f8` — delta only. The `await` requirement is not in this head.

I was told both offload assertions now require the to_thread call to be the operand of an
await. They do not. Await appears zero times in either test file, and all four offload
pins match to_thread by attribute name alone.

vnc_blocking_probes_call_sites_test.py     Await refs: 0
tests/test_agent_event_buffer_7444.py      Await refs: 0

Run against the pins' own helpers, not read off them:

form	pin verdict	should be
`await asyncio.to_thread(probe)`	passes	passes
`asyncio.to_thread(probe)` — no await, never runs	passes	fails
`await asyncio.to_thread(probe())` — called on the loop	passes	fails
`probe()` bare	fails	fails

Same result for the agent side: asyncio.to_thread(self._process_code_change, c, b, m) with no
await satisfies both test_the_offline_insert_is_offloaded_not_called_inline and
test_both_of_buffer_events_async_callers_are_offloaded. So the defect that made round three's
pin vacuous is still admitted by round four's.

A second gap, and it is the more interesting one

_bare_calls builds offloaded from id(arg) for every arg of any to_thread call, then
drops matching ast.Call ids. Trace the two shapes:

Correct — to_thread(probe): the arg is an ast.Name, so there is no ast.Call for the
probe anywhere. _bare_calls finds nothing with or without the exclusion. The exclusion is
dead code for the case it was written for.
Defective — to_thread(probe()): the arg is an ast.Call, so it lands in offloaded
and is excluded. But this is the form where the blocking call runs on the loop and only
its return value crosses into the thread.

So the exclusion's only actual effect is to hide a real defect. And that form evades the
non-vacuity test too — it is neither _offloaded_calls (which requires ast.Name) nor
_bare_calls, so reached does not count it. A file where every site was written that way
would fail the non-vacuity check, but a mixed file would not.

The code at this head is correct — this is a pin weakness, not a shipped defect

Every live call site uses the awaited form: vnc_manager.py:180, :196, :637, :721,
:1152; agent.py:270, :290, :302, :325, :447. :180 and :196 are the two bare
is_vnc_running callers I raised last round, and both are fixed. So nothing is blocking the
loop today; the exposure is that three broken forms would land green.

Suggested fix, verified against all four forms

def _bare_calls(node: ast.AST, name: str) -> list:
    """A reference to *name* that is not the callable of an AWAITED `to_thread`."""
    awaited = {
        id(n.value) for n in ast.walk(node)
        if isinstance(n, ast.Await) and isinstance(n.value, ast.Call)
    }
    offloaded = {
        id(arg)
        for sub in ast.walk(node)
        if isinstance(sub, ast.Call)
        and isinstance(sub.func, ast.Attribute)
        and sub.func.attr == "to_thread"
        and id(sub) in awaited
        for arg in sub.args
        if isinstance(arg, ast.Name)  # a Call here means it ran on the loop
    }
    return [
        sub for sub in ast.walk(node)
        if isinstance(sub, ast.Call) and isinstance(sub.func, ast.Name) and sub.func.id == name
    ] + [
        n for n in ast.walk(node)
        if isinstance(n, ast.Name) and n.id == name and id(n) not in offloaded
        and any(
            isinstance(p, ast.Call) and isinstance(p.func, ast.Attribute)
            and p.func.attr == "to_thread" and n in p.args
            for p in ast.walk(node)
        )
    ]

Two changes: to_thread counts only when awaited, and only an ast.Name argument counts as
offloaded so to_thread(probe()) is reported rather than excused. I ran this against all four
forms and it classifies each correctly. The agent-side pins need the same awaited gate on
their _calls_named(fn, "to_thread") lookups.

Two things in this round that are genuinely good

test_the_cap_is_not_reached_only_through_a_connectivity_check is the strongest assertion
in the PR. It pins that prune_event_buffer() is unconditional in run() rather than
pinning where it sits — "the assertion that survives a revert" is the right instinct, and the
docstring names the bug it prevents: heartbeats fail during an outage, every failure appends a
row, so a prune gated on if success was dead in the one situation the cap exists for.

And declining finally for the prune is right, for the reason given: a prune raising in
finally escapes the catch-all that keeps the loop alive (#9965). That is a case where the
tidier-looking construct is the wrong one.

The non-vacuity docstring deserves singling out — it records that the first version asked
whether the probe's name appeared in the file, which the import line alone satisfies, so it
would have passed with every call deleted. Writing that down rather than quietly fixing it is
what lets the next reader trust the rest of the file.

Verdict

Still approve on the code; the pins should gain the await gate before merge, because a
pin that passes on asyncio.to_thread(f) with no await certifies a fix that executes
nothing — and that is now the third pin in this PR to claim more than it enforces. The first
two read fine as well, which is the argument for testing a pin against its own negative case
rather than reviewing it.

mrveiss · 2026-09-27T19:06:42Z

Proof that the new offload pin is structural, not a source-text check

The unawaited-to_thread gap was the third pin in this PR to claim more than it enforced, and the first two also looked right on reading. So rather than assert the replacement is better, here are two adversarial mutations — the second built specifically to defeat a naive "does this function await a to_thread?" check.

A1 — drop the `await` from the target call

The function still contains another await, and the file contains 18, so any source-text or per-file check would pass:

awaits still in handle_code_change: 1
'await' still in file:              18
verdict: 1 failed, 17 passed

A2 — target unawaited, while a different `to_thread` is awaited in the same function

This is the case that matters. A check asking "is there an awaited to_thread here?" answers yes and passes. The mutation was verified by AST before the test ran, so the result is attributable rather than incidental:

to_thread calls in handle_code_change: 2  of which awaited: 1
_process_code_change reachable via an AWAITED to_thread: 0
verdict: 1 failed, 17 passed

The pin fails on both. It collects arguments only from to_thread calls that are the operand of an ast.Await, so an awaited call to a different callable does not launder an unawaited one.

A note on how the first run of this check was wrong

My first attempt at A2 reported awaited to_thread calls in that function: 0 while claiming to have added one — the sed range used for the diagnostic did not cover the inserted line. The test failed, but I could not attribute the failure to the mutation I intended, so it proved nothing. Redone with the mutation verified through the AST before asserting anything.

That is the same error class as the pins themselves: a check whose output looks like the answer to the question you asked. Worth stating because the alternative was a green line in a PR comment that no one could have re-derived.

mrveiss · 2026-09-27T19:08:26Z

Correction to my own comment above — head moved to `5eb7956fe` mid-review. Agent side: fixed. VNC side: still open.

My previous comment was measured at f11e0977f8 and was already stale when I posted it. Here
is the split at the current head, each half re-run rather than re-read.

Agent side — `_awaited_to_thread` is correct, verified against all three forms

form	pin verdict	correct
`await asyncio.to_thread(self._process_code_change, …)`	passes	yes
`asyncio.to_thread(self._process_code_change, …)` — no await	fails	yes
`await asyncio.to_thread(self._process_code_change(…))` — ran on the loop	fails	yes

The third one closes a gap I had not raised: the on-loop form fails because _calls_named
still sees the inner ast.Call, so inline_clean is false. That is the right structure —
the two assertions cover different shapes and neither excuses the other.

And the docstring is the part worth keeping: "passes on a fix that does nothing, which was
proved on this PR by mutating the await away and watching both original assertions still
pass." A pin whose negative case was actually executed is worth more than one that reads well.

VNC side — unchanged, and both findings stand

autobot-backend/api/vnc_blocking_probes_call_sites_test.py still contains zero ast.Await
references. It did not receive the _awaited_to_thread treatment, so at this head:

asyncio.to_thread(is_vnc_running) with no await still passes
test_no_async_function_calls_a_blocking_probe_inline.
await asyncio.to_thread(is_vnc_running()) — probe called on the loop, result handed to
the thread — still passes, and is excused by the inverted offloaded set described in my
previous comment: it collects every to_thread arg including ast.Call nodes, so the one
shape worth reporting is the one it drops. For the correct to_thread(probe) shape the arg
is an ast.Name and no ast.Call exists, so that exclusion never fires for the case it was
written for.

The fix is the same shape as the one already landed on the agent side: gate on ast.Await, and
restrict the offloaded set to ast.Name arguments. Code is in my previous comment and I ran it
against all four forms.

Unchanged from before

The code at both heads is correct — every live call site uses the awaited form
(vnc_manager.py:180, :196, :637, :721, :1152; agent.py:270, :290, :302, :325,
:447). So this remains a pin weakness rather than a shipped defect, on the one file that has
not yet had the treatment.

Verdict: approve once the VNC pin gets the same await gate the agent pin just got. One
file, one helper, already written and tested one directory over.

… exclusion hid the worst (#17646) Round four strengthened the agent offload pin and left its sibling untouched. `ast.Await` appeared zero times in `vnc_blocking_probes_call_sites_test.py`, so the fix covered one of the two files it already spanned. Checked against every form rather than against the code it was written for: 1 await to_thread(probe) [correct] passes correct 2 bare probe() [blocks] FLAGGED correct 3 to_thread(probe) unawaited [never runs] passes MISS 4 await to_thread(probe()) [blocks on loop] passes MISS Form 4 is the shape `check_no_blocking_io_in_async_test.py` has an explicit test for under `Path.read_text()`. So this PR contained a test for that shape in one file and a pin concealing it in another. The concealment was mechanical, not incidental. `_bare_calls` built an exclusion set from `id(arg)` over every `to_thread` argument, so a probe handed off would not also count as called: - correct form `to_thread(probe)` -- the argument is an `ast.Name`, no `Call` node exists, the exclusion never fires. Dead. - defective form `to_thread(probe())` -- the argument **is** a `Call`, lands in the exclusion, and is hidden. Its only live effect was to hide the one shape where the blocking work runs on the loop. Removed rather than corrected: the correct form passes a function object and produces no `Call` node for the probe, so any `Call` to a probe in async context is a violation and no exception is needed. Nested sync helpers are excluded by walking around nested `def` bodies, mirroring the depth reset in the guard this pin compensates for -- and that shape is now tested too, since it must keep passing or the canonical fix cannot be written. Three sweeps now, one per defect: executed on the loop (forms 2 and 4), offloaded but never awaited (form 3), and a non-vacuity check across all three so an empty file cannot read as a clean one. Verified against all five forms: 2, 3 and 4 flagged; 1 and 5 pass. This is the third pin in this PR to claim more than it enforced. All three were found by a reviewer applying the PR's own standard to its tests, none by a tool, which is the finding worth more than the individual pins. Refs #17646

…st three diagnoses (#17646) CI red at `d58af1f124f3`, both failures mine and one cause: the env-backing two commits ago used `int(os.getenv(...))`, which `repo_tests/env_var_bare_cast_test.py` forbids and its shrink-only ceiling counted (7 -> 11). The guard is right and the reason is worse for an agent than for a service: a bare cast raises `ValueError` at **import**, so a typo in one node's environment stops that agent from starting rather than degrading one setting. I introduced an import-time crash while satisfying the no-hardcoding rule. Both now go through `env_utils.env_int_clamped`, which also fixes something the literals never had: `min_v=1`. A cap of 0 deletes every row on each prune, a negative cap makes `count - max_events` exceed the table, and a batch size of 0 reads nothing forever while the buffer fills. The clamp is the substantive part; satisfying the guard is a side effect. I did not raise the ceiling or add an allowlist entry. A ratchet that only shrinks does not get widened to admit new work -- the same rule this PR applied twice to the file-size pins. Also in this commit, and the reason it is worth appending rather than filing: `autobot-backend/utils/chromadb_client.py:46` read "Host remains os.getenv-based: empty string = use local PersistentClient (dev mode)" directly above `_CHROMADB_HOST = _ssot_config.vm.chromadb`, whose default is `127.0.0.1`. The comment described an earlier revision and inverted the default. Three sessions diagnosing empty analytics panels today read it, observed that no host appears in any config file, and concluded the backend was opening an empty local store. The backend has been on the HTTP client throughout -- the deployed log says `ChromaDB HTTP client connected to 127.0.0.1:8100`. A stale comment about a mechanism outranks the mechanism for anyone who stops at the comment, which is the same failure as a guard whose name claims more than it enforces, in prose. Tests: a malformed value falls back instead of raising at import, and `0` clamps to 1, both per limit. Mutation-proved -- restoring the bare cast fails the local pins *and* the repo guard, which is the pairing worth having: the guard catches the class anywhere, the pin catches it here with the reason attached. Refs #17646

coderabbitai

Actionable comments posted: 6

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: ee35267d-3cef-48cf-8971-fff6855a556e

📥 Commits

Reviewing files that changed from the base of the PR and between afa66d1 and fdddfc1.

⛔ Files ignored due to path filters (2)

repo_tests/python_file_size_ratchet_baseline.py is excluded by !repo_tests/python_file_size_ratchet_baseline.py
scripts/python_file_size_known_large.py is excluded by !scripts/python_file_size_known_large.py

📒 Files selected for processing (9)

autobot-backend/api/vnc_blocking_probes_call_sites_test.py
autobot-backend/api/vnc_manager.py
autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/agent.py
autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/event_buffer.py
autobot-slm-backend/slm/agent/agent.py
autobot-slm-backend/slm/agent/event_buffer.py
autobot-slm-backend/tests/test_agent_event_buffer_7444.py
tools/lint/check_no_blocking_io_in_async.py
tools/lint/check_no_blocking_io_in_async_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

…he node, and two matchers were untested (#17646) Round five of #17647's review: six threads, four real, two already fixed by the previous push. **A prune fault now costs the heartbeat.** Round four moved the prune ahead of the heartbeat so a timeout could not skip it, and the run loop's catch-all wraps both -- so an unhandled buffer fault (full disk, corrupt sqlite) skipped health reporting and a node went dark while otherwise healthy. I traded one silent failure for another. `prune_event_buffer` now swallows and logs its own failure, which makes *both* orderings safe rather than picking the lesser one; `finally` was rejected for the mirror-image reason and this is why neither position was the answer. **`from time import sleep` walked through the widened guard.** Those calls are `ast.Name` nodes and `_maybe_record` returns early on anything that is not an `ast.Attribute`, so the module-qualified matcher could not see them at all. The alias fix two pushes ago covered `import x as y` and left `from x import y` -- one spelling of the class it named, which is this hook's own subject matter one level up. `imported_callables()` resolves those bindings to `(module, attr)` pairs, so `from subprocess import run as child_run` is caught and an unrelated local `run()` still is not. Re-measured: 192 + 18, unchanged -- this hole was empty too, so it is a proven-safe closure rather than a migration. **Two matchers accepted the target in any argument position.** `to_thread(func, *args)` calls only its first argument, so `await asyncio.to_thread(lambda unused: None, self.buffer_event)` satisfied both offload pins while never calling `buffer_event`. Fixed in the agent pin and the VNC pin. **And the matchers themselves had no tests**, which is why that mattered: `_offloaded` was widened twice on this PR -- awaited-only, then first-argument-only -- and each time I changed the matcher without pinning the property, so reverting either widening passed the whole suite. I found this by mutation, after fixing the finding: the fix was in place and the mutation still passed. Both matchers are now asserted directly against the shapes they must separate. **Test isolation.** The env teardown did `delenv` + reload, so on a machine that already sets a limit the module kept the default while pytest restored the variable, and every later test saw a module disagreeing with its environment. The helper now restores the original value; the default-value assertion skips where a limit is configured, because an environment that sets one cannot answer a question about the default. The first version of that helper returned the *module* rather than the value -- and `importlib.reload` returns the same object it mutates, so the teardown reload rewrote what the assertion then read. Mutation-proved, each failing a named test: prune propagating again, dropping from-import resolution, and either matcher accepting a later argument. agent.py stays at its 630 pin -- the round-five reasoning moved into `event_buffer`, which is not size-capped. Mirror 15/15. Refs #17646

mrveiss · 2026-09-27T20:38:10Z

Every finding, and the commit that fixes it, at head `582d10dea4`

Several review follow-ups report "the fix is not present at head <X>" — correctly, because I replied to each thread before pushing its fix. The reviewer inspected a9117f66, 5eb7956fe or 5c1160ef4 and the fixing commit landed afterwards. No follow-up raised a new finding; every one is a head-lag artifact of my own reply timing.

Rather than ask for a recheck per thread, here is the whole map, in commit order:

finding	fixed in
4 production blocking sites; guard widened to the stdlib	`a9117f66`
receiver binding resolved through imports (`imported_modules`)	`3634c656`
sync `buffer_event` from the heartbeat failure handler	`3634c656`
cap enforced only after a successful heartbeat	`3634c656`
`buffer_event`'s second async caller (`handle_code_change`)	`b886dd92`
two bare `is_vnc_running()` calls in async endpoints	`b886dd92`
the non-vacuity check that a bare `import` line satisfied	`afa66d18`
buffer limits as configuration, not literals	`f11e0977`
timeout escaping past the prune; two pins certifying inert fixes	`5eb7956fe`
VNC pin: `_bare_calls` exclusion, and 2 of 3 defective forms missed	`d58af1f1`
bare `int(os.getenv(...))` → `env_int_clamped(min_v=1)`	`5c1160ef`
stale `chromadb_client.py:46` comment	`5c1160ef`
`from time import sleep` bypassing the guard	`582d10dea4`
a prune fault skipping the heartbeat	`582d10dea4`
both offload matchers accepting any argument position	`582d10dea4`
env teardown leaving module and environment disagreeing	`582d10dea4`

Verified at the current head rather than asserted:

imported_callables                      -> check_no_blocking_io_in_async.py, and 3 test refs
"prune failed, continuing to heartbeat" -> both agent copies
call.args[0] first-argument matcher     -> test_agent_event_buffer_7444.py
_limit_with (env teardown helper)       -> 4 refs

A note on two follow-ups that were right about something real. At 5eb7956fe the reviewer observed that the VNC pin still accepted an unawaited to_thread and still carried the _bare_calls exclusion — true at that head, and the reason is worth keeping: round four fixed the agent pin and left its sibling, so the fix covered one of the two files it already spanned. That was corrected in d58af1f1, where the VNC pin was rewritten to classify all four call forms.

What this PR cost, stated plainly. Sixteen findings across five review rounds. Four were defects I introduced inside this PR, and of those, three were pins or matchers claiming more than they enforced — in the PR whose subject is detectors that do exactly that. Every one was found by a reviewer applying this PR's own standard back to its tests, none by a tool. The useful conclusion is not about any single pin: the class is markedly easier to see in someone else's guard than in the one you just wrote, which is the argument for cross-author review stated as an outcome rather than a policy.

mrveiss mentioned this pull request Sep 27, 2026

test(slm): autobot-slm-backend/conftest.py leaves its api stub in sys.modules for every later test file #17648

Open

4 tasks

coderabbitai Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/agent.py

Comment thread autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/event_buffer.py

Comment thread tools/lint/check_no_blocking_io_in_async.py

mrveiss commented Sep 27, 2026

View reviewed changes

coderabbitai Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread autobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/agent.py Outdated

Comment thread autobot-slm-backend/tests/test_agent_event_buffer_7444.py

Comment thread autobot-slm-backend/tests/test_agent_event_buffer_7444.py Outdated

This was referenced Sep 27, 2026

A node that falls behind drops buffered events silently — the SLM cannot see backlog or loss #16056

Open

Three lint hooks are red in full-repo mode: blocking-IO, LLMResponse dict access, local schemas #15505

Open

mrveiss and others added 3 commits September 27, 2026 22:14

Merge branch 'main' into issue-blocking-io-async-widen

fdddfc1

coderabbitai Bot reviewed Sep 27, 2026

View reviewed changes

mrveiss merged commit 897c3e0 into main Sep 27, 2026
81 checks passed

mrveiss deleted the issue-blocking-io-async-widen branch September 27, 2026 20:50

mrveiss mentioned this pull request Sep 27, 2026

fix(analytics): the call scanner read JSDoc examples as live calls — 3 of 4 'Potential Bugs' were documentation (#17668) #17670

Merged

Conversation

mrveiss commented Sep 27, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thinking Path

What Changed

Three of this PR's own pins claimed more than they enforced

Verification

Model Used

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (3 warnings)

Uh oh!

github-actions Bot commented Sep 27, 2026

✅ SSOT Configuration Compliance: Passing

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrveiss commented Sep 27, 2026

Approve. Verified against head a9117f6613, not the PR body.

What I verified independently

One thing to add before merge — not a scope change

Same shape on the VNC side, also pre-existing

Verdict

Uh oh!

mrveiss commented Sep 27, 2026

Re-review at head 3634c656bc. Still approve — one item is now inside scope rather than adjacent.

The one item: buffer_event's other async caller is still on the loop

is_vnc_running — the finishing sentence, as asked

Uh oh!

mrveiss left a comment

Choose a reason for hiding this comment

1. Two ceilings are now stale, and this PR is what made them stale

2. The companion test is satisfied by the import line, so it cannot catch what it exists for

Confirmed, and the reasons hold

Verdict

Uh oh!

mrveiss left a comment

Choose a reason for hiding this comment

Retracting Finding 1 — the ceilings are banked in this PR. My measurement was wrong.

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrveiss commented Sep 27, 2026

Round four at f11e0977f8 — delta only. The await requirement is not in this head.

A second gap, and it is the more interesting one

The code at this head is correct — this is a pin weakness, not a shipped defect

Suggested fix, verified against all four forms

Two things in this round that are genuinely good

Verdict

Uh oh!

mrveiss commented Sep 27, 2026

Proof that the new offload pin is structural, not a source-text check

A1 — drop the await from the target call

A2 — target unawaited, while a different to_thread is awaited in the same function

A note on how the first run of this check was wrong

Uh oh!

mrveiss commented Sep 27, 2026

Correction to my own comment above — head moved to 5eb7956fe mid-review. Agent side: fixed. VNC side: still open.

Agent side — _awaited_to_thread is correct, verified against all three forms

VNC side — unchanged, and both findings stand

Unchanged from before

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrveiss commented Sep 27, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 27, 2026 •

edited

Loading

Approve. Verified against head `a9117f6613`, not the PR body.

Re-review at head `3634c656bc`. Still approve — one item is now inside scope rather than adjacent.

The one item: `buffer_event`'s other async caller is still on the loop

`is_vnc_running` — the finishing sentence, as asked

Round four at `f11e0977f8` — delta only. The `await` requirement is not in this head.

A1 — drop the `await` from the target call

A2 — target unawaited, while a different `to_thread` is awaited in the same function

Correction to my own comment above — head moved to `5eb7956fe` mid-review. Agent side: fixed. VNC side: still open.

Agent side — `_awaited_to_thread` is correct, verified against all three forms

Every finding, and the commit that fixes it, at head `582d10dea4`