You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds a generic read-only query entry point to CaptureServer and uses it to list the MCAL files a capture host offers, so a client can discover them and select them by name. It is independent of calibration: it needs no worker, store or glove, and it stands on main.
/query route. The existing listener serves /query before the control connection's owner admission, so a query works before capture and while capturing, without taking ownership or touching a session. It answers only QueryDevices (MessageType 11) with Devices (12), one request and one reply per connection with bounded waits, and closes with 1002 on anything else, including every capture command. Values are numeric and additive; existing values and the protocol version are unchanged.
Devices reply. One entry per device kind. The Manus entry carries mcal_status (READY, DISABLED when the server has no folder, UNAVAILABLE with a reason when the folder cannot be read) and mcal_files. A listing over the file limit is an error, never a truncated list.
MCAL folder.--manus-calibration-dir DIR names a plain folder. Any nonempty, readable .mcal regular file directly in it is offered, whether placed by hand or written by a calibration tool; symbolic links are never offered. Scans run off the event loop on a small bounded pool.
Selection at Begin.PluginRequest gains optional left_calibration / right_calibration basenames (omitted keeps the earlier wire form). The server checks them against the same scan the listing uses, after duplicate-Begin replay and before any hardware starts, then passes the resolved paths to the Manus plugin's own --left-calibration / --right-calibration arguments. Whether a glove accepts a file is still the plugin's per-hand status.
Client.CaptureClient.query_devices() uses its own short-lived connection and raises DeviceQueryError on any failure (never an empty list); ManusPluginConfig(left_calibration, right_calibration) selects files in start(plugins=...).
Behavior to know about
A server that predates /query treats the path as an ordinary control connection, so a query to it can briefly claim and release the owner slot; the client reports DeviceQueryError with the close code.
Selection is by name against the folder at Begin time; a file that vanished or became empty since it was listed is refused with no substitute.
Validation
tests/capture_server: 464 passed; mcal_catalog.py and protocol.py fully covered.
Auto incremental reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
The change adds a /query connection for retrieving device listings and introduces protocol support for Manus calibration catalogue status and file selections. The server scans the configured directory, reports catalogue status, and checks selected files before starting a new session. The client adds query_devices() and ManusPluginConfig; launch code passes resolved calibration paths to tracking configuration.
sequenceDiagram
participant CaptureClient
participant CaptureServer
participant mcal_catalog
CaptureClient->>CaptureServer: Send QueryDevices on /query
CaptureServer->>mcal_catalog: Scan configured calibration directory
mcal_catalog-->>CaptureServer: Return catalogue status and files
CaptureServer-->>CaptureClient: Send Devices reply
Loading
Merge Risk: Minimal · previous review 1ee36
This adds a read-only device query and per-glove calibration selection, with the protocol, server, client and launch paths consistent in the supplied summaries. No concrete merge-blocking risk was identified. Real-glove calibration loading and end-to-end testing on a capture host have not been done, so they are worth confirming after merge.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 50.98% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "Add device query entry point" matches the main change: a new read-only /query route and QueryDevices/DevicesReply protocol messages, with CaptureClient.query_devices() as the client entry p…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title "Add device query entry point" matches the main change: a new read-only /query route and QueryDevices/DevicesReply protocol messages, with CaptureClient.query_devices() as the client entry point. The objectives also name this PR (#1409, "Add device query entry point"). The title does not mention the Manus MCAL calibration selection, which is a substantial secondary part of the changeset, but it does not need to cover every detail.
Full details: Docstring Coverage
Explanation
Docstring coverage is 50.98% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 51 functions across 8 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Tip
🎯 Customize your risk assessments
Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.
✅ Automatic approvals, with your rules
Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Add a read-only /query route on the existing listener. One QueryDevices
request gets one Devices reply on its own connection, without taking the
control connection's ownership or touching a session, so it works before
and during capture. The Manus entry lists the .mcal files in the folder
set with --manus-calibration-dir.
Begin can select those files by basename per glove (left_calibration /
right_calibration). The server checks them against the same folder policy
the listing uses, before any hardware starts, and passes the resolved
paths to the Manus plugin's own calibration arguments. The folder is a
plain folder of .mcal files, independent of how they were produced.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
Narrow the permit-release handler to Exception and document the intentional
pass in the query test helper.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
The reason will be displayed to describe this comment to others. Learn more.
🤖 Agent-generated (Codex · GPT-6)
Summary
Reviewed 0f0a6c4 after reading repository contribution guidance, with separate readability, conciseness, and correctness passes. The change largely meets its objective: queries bypass owner admission, listing and selection share a policy, duplicate Begin replay happens before rescanning, and selected files reach the Manus plugin's existing arguments. The structure is understandable, but a slow query scan breaks orderly shutdown. The findings below include explicit repository-standard violations as blockers.
Legend: 🚫 Blocker · 💡 Suggestion · 🔍 Nit
Finding
🚫
examples/egocap/capture_server/capture_server/server.py:1254 — Shutdown raises during an in-flight query scan. Shutting down the executor with wait=False leaves the query handler awaiting its shielded scan for up to 10 seconds. Closing the socket does not interrupt that await, and wait_closed() waits for connection handlers, so its two-second deadline expires. Independently reproduced with a scan blocked on a threading event: close() raised TimeoutError after 2.00 seconds. Stop/cancel outstanding handler waits during shutdown while retaining the worker's permit until it actually finishes.
💡
examples/egocap/capture_server/capture_server/mcal_catalog.py:102 — Keep path resolution off the event loop too. The scan resolves the directory in its worker, but launch._calibration_path() resolves it again during synchronous source construction in _handle_begin(). A slow filesystem/automount can therefore freeze the listener outside the scan deadline. A trace confirms the second Path.resolve() runs on MainThread. Carry the resolved root/paths from validation into source construction instead.
🚫
examples/egocap/capture_server/capture_server/server.py:1603 — New diagnostics bypass the required logger tree.print(..., file=sys.stderr) and traceback.print_exc() are diagnostics, whereas root AGENTS.md reserves raw output for terminal UX and requires an isaaccapture.* logger. Use a named logger's exception() without introducing an Isaac Capture runtime dependency.
🚫
examples/egocap/capture_server/capture_server/protocol.py:1976 — New production symbols miss required docstrings.DeviceEntry.__post_init__, DevicesReply.__post_init__ (2031), and ManusCalibrationCatalogStatus (98) lack docstrings required by examples/egocap/AGENTS.md; its validation-block comment requirement also applies here. Add brief purpose/invariant documentation. Overlaps CodeRabbit's existing docstring-coverage warning; this identifies concrete new symbols.
💡
examples/egocap/capture_server/README.md:701 — Reconcile the canonical API/envelope reference. The table includes query messages while the preceding text still says every JSON message carries session_id; query parsing actually rejects that extra key and requires request_id. The public signature/table at 124/146 also omit ManusPluginConfig, and 137 still lists only start/stop. Update those existing sections and show where the new config/error types are imported.
🔍
examples/egocap/capture_server/capture_server/protocol.py:93 — Remove the unused HandSide enum. No request, reply, or consumer uses it; the named calibration fields already encode sides. Keeping the protocol surface limited to actual wire concepts makes it easier to understand.
Validation
Python 3.12, declared dev dependencies: the capture-server suite produced 449 passes and 15 setup errors from macOS UNIX-socket path length. Rerunning the affected GMSL module with --basetemp=/tmp/ic1409 passed all 21 tests, resolving those errors. mcal_catalog.py and protocol.py reached 100% coverage in the initial run. Ruff lint and format checks passed for the capture-server package and new query tests. The shutdown probe above was separate from the existing suite. No hardware calibration loading was exercised.
Actionables (for bots — copy-paste-ready for AI)
Agent-generated suggestions, not human-vetted obligations. Skip anything wrong, already addressed, or not worth the churn.
examples/egocap/capture_server/capture_server/server.py:1254 — Make shutdown settle query/selection handler waits within its existing bound; add a regression that synchronizes inside a blocked scan and calls close() before releasing it.
examples/egocap/capture_server/capture_server/mcal_catalog.py:102 — Pass the already resolved catalog root/selected paths through Begin validation to the launch factory; avoid repeating filesystem resolution on the event loop.
examples/egocap/capture_server/capture_server/server.py:1603 — Replace the raw diagnostic/traceback pair with logging.getLogger("isaaccapture.capture_server.server").exception(...).
examples/egocap/capture_server/capture_server/protocol.py:1976 — Add the missing production docstrings and short validation-invariant comments, including the status enum and reply validator; keep tests exempt.
examples/egocap/capture_server/README.md:701 — Distinguish session and query envelopes; update the public plugin types, operations, and imports.
Log the scan failure through the isaaccapture logger tree, document the
new protocol symbols and their validation, drop the unused HandSide enum,
and reconcile the README envelope and public API sections.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
…AL folder once
close() cancels in-flight /query handlers so a slow scan cannot hold
wait_closed() past its deadline. ServerConfig resolves the MCAL folder at
construction, so Begin no longer touches the filesystem to build a path.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
Shutdown during a query scan — close() now cancels in-flight /query handlers before wait_closed(); the scan thread finishes on its own and its slot is freed by the existing done-callback. Regression test blocks inside a scan and calls close().
Path resolution on the event loop — ServerConfig resolves the MCAL folder once at construction, and mcal_catalog.resolve() is now a pure join, so Begin does no filesystem access to build a path.
Logger tree — the scan failure goes through isaaccapture.capture_server.server with exception() instead of print/traceback.
Docstrings — added for DeviceEntry.__post_init__, DevicesReply.__post_init__ and ManusCalibrationCatalogStatus, with short validation-block comments.
README — separated the session and /query envelopes and updated the public plugin types, operations and imports.
The reason will be displayed to describe this comment to others. Learn more.
🤖 Automated first-pass review (Claude, for Dejun) — I'll follow up personally where needed.
Summary — This adds a read-only /query route that lists the MCAL files in a new --manus-calibration-dir. It also lets start(plugins=...) select left/right files by name; they are checked at Begin and passed to the Manus plugin's existing calibration arguments. The protocol additions are additive and strictly validated, and the tests are thorough. There is one blocker: the earlier shutdown fix covers /query handlers but not the Begin path, which awaits the same scan.
Must fix
server.py:1545 — close() can still time out during a Begin's MCAL scan. _handle_begin now awaits _scan_mcal() (up to _QUERY_DEADLINE_S = 10 s) inside the owner connection's message loop, but close() cancels only /query handlers (server.py:1263). Suppose shutdown starts while a Begin that names a file is waiting on a slow scan. The owner handler keeps running, and wait_for(wait_closed(), 2 s) (server.py:1267) raises TimeoutError. This is the earlier shutdown finding again, reached through the scan's other caller; before this PR, _handle_begin had no long await. _run_cli calls close() in its finally (server.py:1994), so a SIGTERM at that moment would likely end in a traceback.
Fix: make _scan_mcal stop waiting once _stop_event is set. Race the shielded future against self._stop_event.wait() and return UNAVAILABLE "server is shutting down"; _mcal_scan_finished still frees the slot. close() sets the event first, so both callers settle and _query_tasks can go. Run the post-scan _stop_event re-check before acting on the refusal, so the client gets BUSY rather than INVALID_CONFIG. Add a Begin variant of test_close_returns_within_its_deadline_while_a_query_waits_on_a_scan.
Suggestions
client.py:745 — DevicesReply is now in the shared _JSON_PARSERS_BY_TYPE (protocol.py:2076), so this comment ("only ever returns one of the types already handled above") no longer holds. A type-12 message on the control connection reaches event.session_id (:547, :702) and raises AttributeError, not ProtocolError. Have query_devices() call p.DevicesReply.from_json() directly, and keep the table for session messages only.
client.py:793 — {exc or type(exc).__name__} never falls back, because exception objects are always truthy. A timeout therefore reads "the device query failed: " with nothing after it. Use str(exc) or type(exc).__name__, as _diagnostic_text does (server.py:109).
README.md:715 — the README and the flag help (server.py:1819) promise that any nonempty, readable .mcal file is offered. _MCAL_NAME_RE (protocol.py:941) silently skips some names: those with spaces, parentheses or non-ASCII characters, a leading _ or -, an upper-case .MCAL, or more than 133 characters. A hand-copied left glove (2).mcal never appears. Document the rule, and consider logging the names a scan skips.
launch.py:72 — when there is no folder, the bare name is passed through. That is harmless for mock and replay. But build_gmsl_source_factory takes the folder as a separate optional argument (launch.py:314), not from the ServerConfig the Begin check uses. A caller that leaves it out would give the real plugin a relative path. The plugin's constructor then throws (manus_hand_tracking_plugin.cpp:108), and both hands are lost. Raise an error on that path instead. The same throw happens when a file disappears after Begin, which deserves a sentence next to the README's "per-hand status" wording.
Tests: flaky (test_device_query.py:235) and release (:440) are dead code. Three test names no longer match what they test:
The post-scan re-checks in _handle_begin are untested.
Nits: client.py:779 calls /query a "sibling" of the control path, but the code replaces the whole path, so a station URL with a path prefix would miss. Also, client.py and mcal_catalog.py import the private _MCAL_NAME_RE; a public is_mcal_name() in protocol would be cleaner.
Conflicts
With main: a local 3-way merge has no conflicts. main is 2 commits ahead of the merge base, and no rebase is needed. GitHub's blocked state comes from branch protection, not from a conflict.
With open PRs (all by the same author as this one):
#1426 shares client.py, protocol.py and server.py, with a textual conflict in protocol.py and server.py. Whichever merges second rebases. If it also adds MessageType values, keep them clear of 11/12 and update test_message_type_wire_values once.
#1408 shares the README, client.py, protocol.py, server.py and test_protocol.py, on another base (conflicts not computed). If its calibration worker writes into this folder, its file names must match _MCAL_NAME_RE, or this listing will hide them.
#1405 (6 shared files, including launch.py) and #1437 (protocol.py) are on other bases. Conflicts were not computed; expect a rebase over the PluginRequest/MessageType changes.
Change-by-change (9 files)
File / area
What changed
Makes sense?
capture_server/README.md
Documents /query, QueryDevices/Devices (11/12), the MCAL folder and flag, ManusPluginConfig, query_devices() and the new plugin-config fields
⚠️ file-name rule missing; "per-hand status" is incomplete
capture_server/acquisition/isaac_capture.py
TrackingSelection gains manus_left/right_calibration; _manus_args() passes them as --left-calibration=/--right-calibration= in plugin_args
✅ matches the plugin's argv parser and the real and mock PluginConfig.plugin_args
capture_server/client.py
ManusPluginConfig, DeviceQueryError, query_devices() on its own connection; start(plugins=...) accepts ManusPluginConfig
⚠️ stale dispatch comment, empty timeout message, "sibling" comment
capture_server/launch.py
_calibration_path(); real and GMSL factories pass the folder to _tracking_adapter; mock and replay keep bare names
⚠️ the GMSL factory's separate folder argument
capture_server/mcal_catalog.py (new)
Scan policy (regular, nonempty, readable, no symlinks, name regex, 2000-file cap), selection_refusal(), resolve()
✅; name rule needs documenting
capture_server/protocol.py
MessageType 11/12, ManusCalibrationCatalogStatus, query request/reply types, optional calibration names on PluginRequest/PluginConfiguration (omitted when unset), DevicesReply in the server-message table
✅ additive and strict; ⚠️ the table is shared with the control path
capture_server/server.py
/query before owner admission; bounded scan pool and semaphore; Begin-time selection check with post-await re-checks; close() cancels query tasks; folder resolved in ServerConfig; flag wired into mock/real, replay and GMSL setups
⚠️ must-fix: shutdown during a Begin scan
tests/capture_server/test_device_query.py (new)
Protocol, scan, CLI, /query route, Begin selection, client and shutdown tests
✅ broad; ⚠️ some dead code, three stale test names
tests/capture_server/test_protocol.py
Wire-value list extended to 12
✅
Reviewed at dfe06f3 · 9 files · 2026-11-05 15:19 PST
… device-query review findings
The MCAL scan wait now stops when shutdown begins, so neither a query nor a
Begin can hold close() past its deadline, and a Begin cut short that way is
BUSY rather than a bad selection. query_devices() parses the reply directly
instead of through the control-connection table, falls back to the exception
type for an empty timeout message, and real hardware refuses a selected file
with no calibration directory. Document the offered-name rule with a public
is_mcal_name(), and tidy the device-query tests.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Gareth Morgan <gmorgan@nvidia.com>
Shutdown during a Begin's scan — _scan_mcal now stops waiting the moment shutdown begins (both for a free scan slot and for the result), so neither a query nor a Begin can hold close() open; the scan thread still finishes on its own and frees its slot. The query-task tracking is gone. The post-scan shutdown check now runs before the selection refusal, so a scan cut short by shutdown is BUSY, not INVALID_CONFIG. New test: shutdown while a Begin waits on a blocked scan (fails without the change).
Device-reply dispatch — DevicesReply is out of the control-connection parser table; query_devices() parses it directly, so a type-12 message on the control connection is a ProtocolError.
Timeout message — str(exc) or type(exc).__name__.
Offered-name rule — a public is_mcal_name() in protocol, used by the client, catalog and protocol; the README states the rule and that other names are skipped.
No folder on real hardware — _calibration_path raises for a selected file when real hardware has no calibration directory instead of passing a relative path through; the README notes that a file that disappears after Begin loses both hands.
Tests — removed the dead flaky/release code, renamed the three tests that no longer matched what they check, added tests for the name rule, the real-hardware error and the control-connection rejection. Not added: a dedicated test for the post-scan re-checks beyond the shutdown case.
Nit — the /query comment now says it is at the root path of the listener.
The must-fix item (`close()` timing out while a Begin waits on the MCAL scan) is fixed on `main` as merged in e0a1536b3: the scan stops waiting once the server is stopping and reports "the server is shutting down", and `test_close_returns_within_its_deadline_while_a_begin_waits_on_a_scan` is the Begin variant of the existing query test. I checked the code and the test names; I did not re-run the suite for this comment. The suggestions in that review are not addressed here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
GitHub Actions
Failure
AndreusNvidius
left a comment
NVdejung
left a comment
Summary
Adds a generic read-only query entry point to CaptureServer and uses it to list the MCAL files a capture host offers, so a client can discover them and select them by name. It is independent of calibration: it needs no worker, store or glove, and it stands on
main./queryroute. The existing listener serves/querybefore the control connection's owner admission, so a query works before capture and while capturing, without taking ownership or touching a session. It answers onlyQueryDevices(MessageType11) withDevices(12), one request and one reply per connection with bounded waits, and closes with 1002 on anything else, including every capture command. Values are numeric and additive; existing values and the protocol version are unchanged.mcal_status(READY,DISABLEDwhen the server has no folder,UNAVAILABLEwith a reason when the folder cannot be read) andmcal_files. A listing over the file limit is an error, never a truncated list.--manus-calibration-dir DIRnames a plain folder. Any nonempty, readable.mcalregular file directly in it is offered, whether placed by hand or written by a calibration tool; symbolic links are never offered. Scans run off the event loop on a small bounded pool.PluginRequestgains optionalleft_calibration/right_calibrationbasenames (omitted keeps the earlier wire form). The server checks them against the same scan the listing uses, after duplicate-Begin replay and before any hardware starts, then passes the resolved paths to the Manus plugin's own--left-calibration/--right-calibrationarguments. Whether a glove accepts a file is still the plugin's per-hand status.CaptureClient.query_devices()uses its own short-lived connection and raisesDeviceQueryErroron any failure (never an empty list);ManusPluginConfig(left_calibration, right_calibration)selects files instart(plugins=...).Behavior to know about
/querytreats the path as an ordinary control connection, so a query to it can briefly claim and release the owner slot; the client reportsDeviceQueryErrorwith the close code.Validation
tests/capture_server: 464 passed;mcal_catalog.pyandprotocol.pyfully covered.tests/recording(100% coverage required): 434 passed, 3 skipped.tests/recording_service: 47 passed, 100 skipped (real-episode tests needEGOCAP_REAL_EPISODE).SKIP=check-copyright-year pre-commit run --all-files: passed.🤖 Generated with Claude Code
Summary by CodeRabbit
--manus-calibration-diroption to configure where calibration files are found.