Introduce xbot-shell interactive CLI with dynamic service management#27
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an IPython/Rich interactive xbot-shell with dynamic ServiceProxy and register parsing; core ServiceInterface synchronization and lifecycle dispatch improvements; packaging and CI for the shell; extensive READMEs and runnable GPIO/LCD examples. ChangesInteractive Shell Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.idea/vcs.xml (1)
5-6: ⚡ Quick winAvoid committing VCS mappings for generated build dependency folders.
These mappings point to
cmake-build-debug/_deps/*, which are local/generated paths and tend to create noisy, machine-specific IDE config drift. Prefer keeping only stable repo paths in shared.idea/vcs.xml.Proposed cleanup
- <mapping directory="$PROJECT_DIR$/cmake-build-debug/_deps/crow-src" vcs="Git" /> - <mapping directory="$PROJECT_DIR$/cmake-build-debug/_deps/spdlog-src" vcs="Git" />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.idea/vcs.xml around lines 5 - 6, Remove the machine-specific VCS mappings for generated build dependency folders from .idea/vcs.xml by deleting the mapping entries that reference "cmake-build-debug/_deps/crow-src" and "cmake-build-debug/_deps/spdlog-src"; keep only stable repo paths in vcs.xml (or make .idea/vcs.xml untracked/ignored for local IDE settings) so generated _deps mappings are not committed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xbot_service_interface/examples/lcd_hello.py`:
- Around line 143-151: The main loop is busy-waiting because time.sleep is
commented out; restore a short sleep or yield to avoid CPU spin by adding a
small delay (e.g., 0.01s) inside the loop where xbot.ok() / svc.connected and
lcd_write(svc, f"Counter: {counter}", line=0) run. Modify the loop around
xbot.ok(), svc.connected, lcd_write and counter to call time.sleep(0.01) (or use
an appropriate configurable interval or event/condition wait) after the
work/update to throttle the loop and reduce CPU usage.
In `@xbot_service_interface/pyproject.toml`:
- Line 47: The `[project.optional-dependencies].shell` entry currently allows
older versions; update the `shell` extras in
xbot_service_interface/pyproject.toml (the `shell = [...]` array) to match the
documented minimums by raising ipython to at least 9.13.0 and rich to at least
15.0.0 (leave heatshrink2 as-is), i.e. change the version specifiers for ipython
and rich in the `shell` extras to `ipython>=9.13.0` and `rich>=15.0.0`.
In `@xbot_service_interface/README.md`:
- Around line 115-117: The fenced code blocks showing console/output and service
snippets (for example the block starting with "→ Discovered: EchoService (id=1)
at 192.168.1.5:4242 — connect(1)", the block with "%services / %connect
EchoService / %connect 1 as svc", and the block with "svc.send_<TAB> ...
svc.registers.<TAB>") are missing language identifiers; update each
triple-backtick fence to include a language tag (use "text") so they become
```text ... ``` to satisfy markdown lint and improve readability.
In `@xbot_service_interface/xbot_service_interface/shell.py`:
- Around line 185-194: wait_connected has a race and accumulates callbacks:
after checking iface._connected you register a persistent lambda via
iface.on_connected and then wait, so if the connection happens between the
initial check and registration the call can still time out, and repeated calls
leave callbacks attached. Fix by registering a one-shot callback (capture the
unsubscribe handle returned by iface.on_connected and call it inside the
callback, or register a named callback and remove it explicitly after it runs),
then re-check iface._connected immediately after registering (and before
ev.wait) so you can short-circuit if connection already occurred; use the
existing symbols wait_connected, _iface, on_connected and the local ev to
implement the one-shot callback and post-registration re-check.
---
Nitpick comments:
In @.idea/vcs.xml:
- Around line 5-6: Remove the machine-specific VCS mappings for generated build
dependency folders from .idea/vcs.xml by deleting the mapping entries that
reference "cmake-build-debug/_deps/crow-src" and
"cmake-build-debug/_deps/spdlog-src"; keep only stable repo paths in vcs.xml (or
make .idea/vcs.xml untracked/ignored for local IDE settings) so generated _deps
mappings are not committed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d0005ab-40e1-4204-aa21-63ed292bc63d
📒 Files selected for processing (9)
.idea/vcs.xmlxbot_service_interface/README.mdxbot_service_interface/examples/gpio.jsonxbot_service_interface/examples/lcd_hello.pyxbot_service_interface/pyproject.tomlxbot_service_interface/requirements.txtxbot_service_interface/xbot_service_interface/interface.pyxbot_service_interface/xbot_service_interface/manager.pyxbot_service_interface/xbot_service_interface/shell.py
ClemensElflein
changed the title
- lcd_hello.py: restore time.sleep(0.01) to prevent CPU spin - shell.py wait_connected: register callback before checking _connected to close race; one-shot _cb removes itself to prevent accumulation; timeout path also cleans up the callback - pyproject.toml: raise shell extras to ipython>=9.13.0, rich>=15.0.0 - README.md: add `text` language tag to three bare fenced code blocks - .idea/vcs.xml: remove generated cmake-build-debug/_deps VCS mappings
ClemensElflein
force-pushed
the
feature/ipython
branch
from
1168ad5 to
b78fbba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xbot_service_interface/xbot_service_interface/shell.py`:
- Around line 197-206: In wait_connected(), you register _cb with
iface.on_connected but when iface._connected is already True you call ev.set()
without removing the registered _cb, causing accumulation in
iface._connected_callbacks; after calling ev.set() in the branch that re-checks
iface._connected, remove _cb from iface._connected_callbacks (use the same safe
removal pattern with try/except ValueError as used later) so the callback is
cleaned up regardless of whether it was fired or the event was set immediately;
refer to wait_connected(), iface.on_connected, iface._connected, _cb, and
iface._connected_callbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d6431ff2-1e5d-448d-aee0-9100ba9d864d
📒 Files selected for processing (4)
xbot_service_interface/README.mdxbot_service_interface/examples/lcd_hello.pyxbot_service_interface/pyproject.tomlxbot_service_interface/xbot_service_interface/shell.py
Makes it unambiguous that this is the Python implementation alongside the C++ libxbot-service-interface. PyPI project name updated to match: xbot-service-interface → xbot-service-interface-py. The importable package (xbot_service_interface) is unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xbot_service_interface_py/pyproject.toml`:
- Line 6: Update README.md to use the new distribution name: replace any
occurrences of pip install xbot-service-interface and pip install
"xbot-service-interface[shell]" with pip install xbot-service-interface-py and
pip install "xbot-service-interface-py[shell]"; edit
xbot_service_interface_py/README.md (search for the old package strings) and
update both the plain install and extras install commands so they match the new
name declared in pyproject.toml.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c76e75ad-46d1-4bc8-a523-0d09fefd1221
📒 Files selected for processing (29)
xbot_service_interface_py/.gitignorexbot_service_interface_py/README.mdxbot_service_interface_py/examples/echo_no_schema.pyxbot_service_interface_py/examples/echo_service.jsonxbot_service_interface_py/examples/echo_with_schema.pyxbot_service_interface_py/examples/gpio.jsonxbot_service_interface_py/examples/lcd_hello.pyxbot_service_interface_py/pyproject.tomlxbot_service_interface_py/requirements.txtxbot_service_interface_py/tests/__init__.pyxbot_service_interface_py/tests/conftest.pyxbot_service_interface_py/tests/test_datatypes.pyxbot_service_interface_py/tests/test_discovery.pyxbot_service_interface_py/tests/test_interface.pyxbot_service_interface_py/tests/test_io.pyxbot_service_interface_py/tests/test_rpc.pyxbot_service_interface_py/tests/test_schema.pyxbot_service_interface_py/tests/test_serialization.pyxbot_service_interface_py/xbot_service_interface/__init__.pyxbot_service_interface_py/xbot_service_interface/datatypes.pyxbot_service_interface_py/xbot_service_interface/discovery.pyxbot_service_interface_py/xbot_service_interface/exceptions.pyxbot_service_interface_py/xbot_service_interface/interface.pyxbot_service_interface_py/xbot_service_interface/io.pyxbot_service_interface_py/xbot_service_interface/manager.pyxbot_service_interface_py/xbot_service_interface/py.typedxbot_service_interface_py/xbot_service_interface/schema.pyxbot_service_interface_py/xbot_service_interface/serialization.pyxbot_service_interface_py/xbot_service_interface/shell.py
💤 Files with no reviewable changes (7)
- xbot_service_interface_py/requirements.txt
- xbot_service_interface_py/xbot_service_interface/manager.py
- xbot_service_interface_py/examples/gpio.json
- xbot_service_interface_py/xbot_service_interface/interface.py
- xbot_service_interface_py/xbot_service_interface/shell.py
- xbot_service_interface_py/examples/lcd_hello.py
- xbot_service_interface_py/README.md
Tests on 3.10–3.12 for every push. Build wheel+sdist after tests pass. Publish to PyPI on v* tags via OIDC trusted publisher (no API token needed).
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xbot_service_interface_py/pyproject.toml (1)
6-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRename is not fully propagated to user-facing install instructions.
After changing distribution name to
xbot-service-interface-py, downstream guidance still referencesxbot-service-interface[shell](e.g.,xbot_service_interface_py/xbot_service_interface/shell.py:786-802,xbot_service_interface_py/README.md:8-14), which will mislead users when shell deps are missing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/pyproject.toml` at line 6, The README and any user-facing installation references still use the old package name/extras (xbot-service-interface[shell]); update all references to the renamed distribution "xbot-service-interface-py" and the extras syntax so installation instructions are accurate (for example update the README.md lines and any docstrings or error/help text in xbot_service_interface_py/xbot_service_interface/shell.py that mention "xbot-service-interface[shell]") — ensure pip examples, CI badges, and user-facing error/help messages uniformly show the new package name and, if extras are needed, use the correct extras syntax for the renamed package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/python.yaml:
- Around line 18-19: The workflow uses floating tags for third-party actions;
replace each action reference with its immutable commit SHA to prevent
unexpected changes: update occurrences of actions/checkout@v4,
actions/setup-python@v5, actions/upload-artifact@v4,
actions/download-artifact@v4, and pypa/gh-action-pypi-publish@release/v1 to the
corresponding pinned commit SHAs (find the latest release commit SHAs in each
action's GitHub repo) so the job definitions using those identifiers use the SHA
instead of the tag.
- Line 18: The checkout step is using actions/checkout@v4 without disabling
credential persistence; update the checkout steps that use "uses:
actions/checkout@v4" to add a "with:" block containing "persist-credentials:
false" (and add it to each occurrence of that checkout line), so credentials are
not persisted to the runner workspace.
- Around line 3-7: Add explicit least-privilege GitHub Actions permissions and
stop persisting the GITHUB_TOKEN to checked-out repo steps: add a top-level or
per-job permissions block that narrows scopes (e.g., for test/build jobs set
contents: read only; for the publish job set id-token: write and contents:
read), and update every actions/checkout@v4 step to include with:
persist-credentials: false to avoid exposing tokens to the workspace; ensure the
publish job retains id-token: write while adding contents: read so publishing is
scoped correctly.
In `@xbot_service_interface_py/pyproject.toml`:
- Line 10: The project's pyproject.toml sets project.license = "MIT" (SPDX
string) which requires setuptools >= 77 but build-system.requires currently
lists an older minimum; either change project.license to the TOML table form
(e.g., project.license = { text = "MIT" }) to remain compatible with older
setuptools, or update build-system.requires to ensure setuptools>=77; update the
relevant entries for project.license and build-system.requires accordingly so
the chosen approach is consistent.
---
Outside diff comments:
In `@xbot_service_interface_py/pyproject.toml`:
- Line 6: The README and any user-facing installation references still use the
old package name/extras (xbot-service-interface[shell]); update all references
to the renamed distribution "xbot-service-interface-py" and the extras syntax so
installation instructions are accurate (for example update the README.md lines
and any docstrings or error/help text in
xbot_service_interface_py/xbot_service_interface/shell.py that mention
"xbot-service-interface[shell]") — ensure pip examples, CI badges, and
user-facing error/help messages uniformly show the new package name and, if
extras are needed, use the correct extras syntax for the renamed package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80400f5f-f05f-46f7-8631-def20d6bea6f
📒 Files selected for processing (2)
.github/workflows/python.yamlxbot_service_interface_py/pyproject.toml
There was a problem hiding this comment.
🧹 Nitpick comments (3)
xbot_service_interface_py/README.md (3)
62-62: ⚡ Quick winClarify IncompatibleServiceError handling behavior.
The statement that
IncompatibleServiceErroris "logged" doesn't clarify whether the interface connection attempt continues or is blocked entirely. Consider specifying that the interface will not connect when schema validation fails.📝 Proposed clarification
-The schema file is validated against the service advertisement: if type or version mismatches, `IncompatibleServiceError` is logged and the interface is not connected. +The schema file is validated against the service advertisement: if type or version mismatches, `IncompatibleServiceError` is logged and the interface will not connect to the service.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/README.md` at line 62, Clarify that when schema validation against the service advertisement fails and an IncompatibleServiceError is raised, the interface connection attempt is aborted and the interface will not connect (i.e., the connection is blocked/terminated), and update the README sentence to explicitly state that behavior referencing IncompatibleServiceError and "service advertisement" so readers know the interface will not proceed with connection on validation failure.
18-25: 💤 Low valueAdd language identifier to fenced code block.
The architecture diagram uses a fenced code block without a language identifier. Consider adding
textor removing the fence if plain text formatting is intended. As per coding guidelines, markdownlint flags this as MD040.📝 Proposed fix
-``` +```text XbotServiceIo — owns the UDP socket and discovery listener └── ServiceInterface — represents one remote service ├── registers — send/read configuration registers🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/README.md` around lines 18 - 25, Update the fenced architecture diagram block so it includes a language identifier (e.g., add ```text before the diagram) or remove the triple-backtick fence if you want plain paragraph text; modify the README.md fenced block containing the "XbotServiceIo — owns the UDP socket and discovery listener" diagram so the top fence becomes ```text (or delete the fence lines) to satisfy markdownlint MD040.
27-27: ⚡ Quick winClarify callback dispatch mechanism.
The statement "All callbacks are dispatched from those threads" may be misleading. Based on the core library enhancements (layer 1), lifecycle callbacks (
on_connected,on_disconnected) are dispatched on dedicated daemon threads, not directly from the IO/discovery threads. This distinction matters for understanding threading behavior and potential deadlock scenarios.📝 Proposed clarification
-`XbotServiceIo` runs two background threads: one for multicast service discovery (UDP 233.255.255.0:4242) and one for the data/control socket. All callbacks are dispatched from those threads. +`XbotServiceIo` runs two background threads: one for multicast service discovery (UDP 233.255.255.0:4242) and one for the data/control socket. Lifecycle callbacks (`on_connected`, `on_disconnected`) are dispatched on dedicated daemon threads to avoid blocking the IO thread.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/README.md` at line 27, Update the README text describing XbotServiceIo so it accurately states which threads dispatch which callbacks: clarify that multicast discovery and data/control IO run on their own background threads (the "IO/discovery threads") while lifecycle callbacks (on_connected, on_disconnected) are dispatched on separate dedicated daemon threads instead of directly on the IO/discovery threads; mention this distinction briefly and note that other event callbacks continue to be dispatched from the IO/discovery threads and that the dedicated daemoning of lifecycle callbacks reduces risk of deadlocks between IO and lifecycle handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@xbot_service_interface_py/README.md`:
- Line 62: Clarify that when schema validation against the service advertisement
fails and an IncompatibleServiceError is raised, the interface connection
attempt is aborted and the interface will not connect (i.e., the connection is
blocked/terminated), and update the README sentence to explicitly state that
behavior referencing IncompatibleServiceError and "service advertisement" so
readers know the interface will not proceed with connection on validation
failure.
- Around line 18-25: Update the fenced architecture diagram block so it includes
a language identifier (e.g., add ```text before the diagram) or remove the
triple-backtick fence if you want plain paragraph text; modify the README.md
fenced block containing the "XbotServiceIo — owns the UDP socket and discovery
listener" diagram so the top fence becomes ```text (or delete the fence lines)
to satisfy markdownlint MD040.
- Line 27: Update the README text describing XbotServiceIo so it accurately
states which threads dispatch which callbacks: clarify that multicast discovery
and data/control IO run on their own background threads (the "IO/discovery
threads") while lifecycle callbacks (on_connected, on_disconnected) are
dispatched on separate dedicated daemon threads instead of directly on the
IO/discovery threads; mention this distinction briefly and note that other event
callbacks continue to be dispatched from the IO/discovery threads and that the
dedicated daemoning of lifecycle callbacks reduces risk of deadlocks between IO
and lifecycle handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd0081cf-b9c3-44e1-84f5-63e0d20a2bd0
📒 Files selected for processing (1)
xbot_service_interface_py/README.md
… name in shell error, bump setuptools>=77 for SPDX license
Member
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
xbot_service_interface_py/README.md (1)
72-74: ⚡ Quick winAvoid documenting private attribute access in public examples.
Using
echo._active_schemain README encourages reliance on an internal field. Prefer a documented public accessor/property for schema introspection in examples.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/README.md` around lines 72 - 74, The README example uses the private attribute echo._active_schema; replace this with a documented public accessor on the Echo/echo instance (e.g., a property or method such as Echo.active_schema or Echo.get_schema()) and update the example to call that accessor (e.g., echo.active_schema) instead of accessing _active_schema directly; implement the accessor on the Echo class if it doesn't exist and ensure it returns the same schema object used by the internal _active_schema so public examples no longer reference private fields.xbot_service_interface_py/xbot_service_interface/shell.py (2)
424-425: 💤 Low valuePreserve exception chain with
raise ... from.When re-raising exceptions, use
raise ... from err(orfrom Noneif intentionally suppressing) to preserve the exception chain for debugging.Suggested fix for this and similar patterns (lines 462, 478, 491)
except ImportError: - raise RuntimeError( + raise RuntimeError( "heatshrink2 not installed. Run: pip install heatshrink2") + ) from NoneAnd similarly:
- raise ValueError( + raise ValueError( f"Expected one of {list(enum_def['values'].keys())} or an integer, got {raw!r}") + ) from None - raise ValueError(f"Cannot parse list: {e}") + raise ValueError(f"Cannot parse list: {e}") from e - raise ValueError(f"Cannot read file {path!r}: {e}") + raise ValueError(f"Cannot read file {path!r}: {e}") from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/xbot_service_interface/shell.py` around lines 424 - 425, Change the plain re-raises to preserve the exception chain by using "raise RuntimeError(... ) from err" (or "from None" if you must suppress) inside the corresponding except blocks; specifically update the RuntimeError for the heatshrink2 install check and the similar raise sites (the other raises at the same pattern) so they use the caught exception variable (e.g., err) in the "from" clause to retain the original traceback.
309-313: 💤 Low valueRemove unnecessary
fprefix from string literals.These f-strings have no placeholders.
Suggested fix
- _print(f" [dim]cleared[/dim]", plain=" cleared") + _print(" [dim]cleared[/dim]", plain=" cleared") changed = True else: - _print(f" [yellow]Cannot clear required register — skipped[/yellow]", + _print("[yellow]Cannot clear required register — skipped[/yellow]", plain=" Cannot clear required register — skipped")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xbot_service_interface_py/xbot_service_interface/shell.py` around lines 309 - 313, Remove the unnecessary f-string prefixes on the two _print calls that have no placeholders: update the calls to _print(" [dim]cleared[/dim]", plain=" cleared") and _print(" [yellow]Cannot clear required register — skipped[/yellow]", plain=" Cannot clear required register — skipped") in the block containing the local variable changed (the branch that sets changed = True and the else branch), i.e., remove the leading "f" from those string literals so they are plain strings..github/workflows/python.yaml (1)
27-30: ⚡ Quick winAdd a shell smoke-check to CI for the new CLI path.
Line 28 installs only
.[dev], so the new shell dependency path is not validated in CI.Suggested minimal update
- - name: Install - run: pip install -e ".[dev]" + - name: Install + run: pip install -e ".[dev,shell]" - name: Test run: pytest + - name: Shell smoke test + run: python -c "from xbot_service_interface.shell import main; print(main.__name__)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/python.yaml around lines 27 - 30, The CI currently only runs the "Install" step which does pip install -e ".[dev]" and then "Test" runs pytest, so the new shell/CLI extras path isn't validated; add a new step (e.g., name it "Shell smoke-check") immediately after the "Install" step that installs the shell extras (for example pip install -e ".[shell]" or pip install -e ".[dev,shell]" as appropriate) and runs a minimal smoke command against the CLI (for example run the CLI with --version or --help) to ensure the new path is installed and the entrypoint works before running "Test"/pytest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xbot_service_interface_py/examples/lcd_hello.py`:
- Line 24: Replace the hardcoded BIND_IP value used in lcd_hello.py (BIND_IP)
with a portable default like "0.0.0.0" and add support for overriding it via an
environment variable or CLI option; update initialization to read from
os.environ.get("BIND_IP") or a parsed argument first and fall back to "0.0.0.0"
so the example runs out-of-the-box on any machine while still allowing custom
bind addresses.
- Around line 143-151: The loop in the example updates the LCD far too
frequently (every 10ms); change the sleep cadence in the main loop that calls
lcd_write (the while xbot.ok() block using svc, counter and lcd_write) from
time.sleep(0.01) to a more realistic interval like time.sleep(1.0) so the
counter updates roughly once per second and reduces I2C/service load.
---
Nitpick comments:
In @.github/workflows/python.yaml:
- Around line 27-30: The CI currently only runs the "Install" step which does
pip install -e ".[dev]" and then "Test" runs pytest, so the new shell/CLI extras
path isn't validated; add a new step (e.g., name it "Shell smoke-check")
immediately after the "Install" step that installs the shell extras (for example
pip install -e ".[shell]" or pip install -e ".[dev,shell]" as appropriate) and
runs a minimal smoke command against the CLI (for example run the CLI with
--version or --help) to ensure the new path is installed and the entrypoint
works before running "Test"/pytest.
In `@xbot_service_interface_py/README.md`:
- Around line 72-74: The README example uses the private attribute
echo._active_schema; replace this with a documented public accessor on the
Echo/echo instance (e.g., a property or method such as Echo.active_schema or
Echo.get_schema()) and update the example to call that accessor (e.g.,
echo.active_schema) instead of accessing _active_schema directly; implement the
accessor on the Echo class if it doesn't exist and ensure it returns the same
schema object used by the internal _active_schema so public examples no longer
reference private fields.
In `@xbot_service_interface_py/xbot_service_interface/shell.py`:
- Around line 424-425: Change the plain re-raises to preserve the exception
chain by using "raise RuntimeError(... ) from err" (or "from None" if you must
suppress) inside the corresponding except blocks; specifically update the
RuntimeError for the heatshrink2 install check and the similar raise sites (the
other raises at the same pattern) so they use the caught exception variable
(e.g., err) in the "from" clause to retain the original traceback.
- Around line 309-313: Remove the unnecessary f-string prefixes on the two
_print calls that have no placeholders: update the calls to _print("
[dim]cleared[/dim]", plain=" cleared") and _print(" [yellow]Cannot clear
required register — skipped[/yellow]", plain=" Cannot clear required register
— skipped") in the block containing the local variable changed (the branch that
sets changed = True and the else branch), i.e., remove the leading "f" from
those string literals so they are plain strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 12bcb75d-4ca3-46d9-86fa-e8c74db5ab75
📒 Files selected for processing (32)
.github/workflows/python.yamlxbot_service_interface/README.mdxbot_service_interface/requirements.txtxbot_service_interface_py/.gitignorexbot_service_interface_py/README.mdxbot_service_interface_py/examples/echo_no_schema.pyxbot_service_interface_py/examples/echo_service.jsonxbot_service_interface_py/examples/echo_with_schema.pyxbot_service_interface_py/examples/gpio.jsonxbot_service_interface_py/examples/lcd_hello.pyxbot_service_interface_py/pyproject.tomlxbot_service_interface_py/requirements.txtxbot_service_interface_py/tests/__init__.pyxbot_service_interface_py/tests/conftest.pyxbot_service_interface_py/tests/test_datatypes.pyxbot_service_interface_py/tests/test_discovery.pyxbot_service_interface_py/tests/test_interface.pyxbot_service_interface_py/tests/test_io.pyxbot_service_interface_py/tests/test_rpc.pyxbot_service_interface_py/tests/test_schema.pyxbot_service_interface_py/tests/test_serialization.pyxbot_service_interface_py/xbot_service_interface/__init__.pyxbot_service_interface_py/xbot_service_interface/datatypes.pyxbot_service_interface_py/xbot_service_interface/discovery.pyxbot_service_interface_py/xbot_service_interface/exceptions.pyxbot_service_interface_py/xbot_service_interface/interface.pyxbot_service_interface_py/xbot_service_interface/io.pyxbot_service_interface_py/xbot_service_interface/manager.pyxbot_service_interface_py/xbot_service_interface/py.typedxbot_service_interface_py/xbot_service_interface/schema.pyxbot_service_interface_py/xbot_service_interface/serialization.pyxbot_service_interface_py/xbot_service_interface/shell.py
💤 Files with no reviewable changes (2)
- xbot_service_interface/README.md
- xbot_service_interface/requirements.txt
PyPI account not yet activated. Releases now upload dist artifacts and include ready-to-copy pip install commands in the release body. README updated with git+https install instructions until PyPI is live.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Documentation
Examples
Tests
Chores