Streamline install, diagnostics, and persistent read-only support#142
Open
Streamline install, diagnostics, and persistent read-only support#142
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Owner
Author
|
Release-draft drift after the latest push:
@coderabbitai resume |
|
No description provided. |
|
Key items logged from the session note:
🧠 Learnings used✅ Actions performedReviews resumed. |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
scripts/debug.sh (1)
64-64:⚠️ Potential issue | 🟡 MinorConfig parse failure prevents debug report generation.
If
/etc/default/bluetooth_2_usb_readonlyis malformed,load_readonly_config()callsfail()and the script exits immediately. This means operators can't get a debug report when investigating config issues.Consider wrapping in a subshell or pre-check:
-load_readonly_config +PARSE_ERROR=0 +if ! (load_readonly_config) 2>/dev/null; then + PARSE_ERROR=1 +fiThen use
PARSE_ERRORto gate the readonly_mode/bluetooth_state_persistent calls (lines 312-323) and show an error indicator in the report instead of failing completely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/debug.sh` at line 64, The call to load_readonly_config currently aborts the whole script via fail() on a malformed /etc/default/bluetooth_2_usb_readonly; change this to detect parse errors without exiting by invoking load_readonly_config in a guarded context (e.g., a subshell or try-style pre-check) and set a PARSE_ERROR flag when parsing fails instead of calling fail(); then update the later consumers readonly_mode and bluetooth_state_persistent to check PARSE_ERROR and emit an error indicator in the debug report if set (rather than relying on values from the failed parse).
🧹 Nitpick comments (10)
scripts/lib/boot.sh (1)
61-64: Heuristic for dwc2 builtin may misclassify loaded modules.If
/sys/module/dwc2exists, this returns "builtin", but that directory also exists when dwc2 is loaded as a module. The earlier checks for kernel config should catch the module case first, so this fallback is only reached when config evidence is unavailable. The logic order is correct, but consider adding a comment explaining this is a last-resort heuristic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lib/boot.sh` around lines 61 - 64, Add a short explanatory comment above the heuristic check using [[ -d /sys/module/dwc2 ]] in scripts/lib/boot.sh stating that this is a last-resort heuristic (reached only if earlier kernel config checks found no evidence) and that /sys/module/dwc2 may also exist when dwc2 is loaded as a module, so the check can misclassify module vs builtin; keep the existing logic but document the rationale and ordering to prevent future confusion.scripts/install_host_hidapi_udev_rule.sh (1)
38-43: Consider narrowing the udevadm trigger scope.
udevadm trigger --subsystem-match=usbre-enumerates all USB devices on the system. Since the rule targets a specific vendor/product (1d6b:0104), consider narrowing the trigger:install -m 0644 "${RULE_SRC}" "${RULE_DST}" udevadm control --reload-rules -udevadm trigger --subsystem-match=usb +udevadm trigger --subsystem-match=usb --attr-match=idVendor=1d6b --attr-match=idProduct=0104This avoids unnecessary enumeration of unrelated USB devices, though the current approach is functional and the impact is minor (only momentary).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/install_host_hidapi_udev_rule.sh` around lines 38 - 43, The udev trigger is too broad—replace the global "udevadm trigger --subsystem-match=usb" call with a scoped trigger that matches the vendor/product in your rule (use the same vendor/product values referenced by RULE_SRC/RULE_DST, e.g., idVendor=1d6b and idProduct=0104) so only that device class is re-enumerated; update the script to call udevadm trigger with --subsystem-match=usb plus appropriate --attr-match=idVendor=... and --attr-match=idProduct=... (or an equivalent attribute match) immediately after reloading rules.scripts/host_relay_test_capture.ps1 (1)
26-34: Empty catch block is intentional but could benefit from a comment.The static analysis flagged the empty catch, but here it's valid: if
py -3isn't available or fails, silently falling through to the final error message is the correct behavior. Consider adding a brief comment to document this intent and quiet future static analysis concerns.Suggested documentation
} else { try { & py -3 -c "import hid" *> $null if ($LASTEXITCODE -eq 0) { $PythonBin = "py -3" } } catch { + # Silently ignore if py launcher is unavailable; fall through to error } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/host_relay_test_capture.ps1` around lines 26 - 34, The empty catch block after the attempt to run "& py -3 -c \"import hid\" *> $null" is intentional to allow falling through when py -3 is unavailable; add a brief inline comment inside that catch (referencing the catch that follows the py invocation) stating that the exception is purposely ignored so the script can continue to the final error handling and to silence static-analysis warnings.src/bluetooth_2_usb/service_config.py (1)
192-200: Config is loaded twice when using--print-shell-command.
main()loads the config at line 185, thenbuild_shell_command()at line 194 reloads it internally. Consider passing the already-loaded config to avoid redundant file I/O and potential TOCTOU issues if the file changes between reads.♻️ Optional refactor
if args.print_shell_command: + argv = build_cli_argv(config, append_debug=args.append_debug) + command = shlex.split(args.executable) + argv print( - build_shell_command( - args.executable, - append_debug=args.append_debug, - env_file=DEFAULT_ENV_FILE, - ) + shlex.join(command) ) return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/service_config.py` around lines 192 - 200, main() currently loads configuration and then calls build_shell_command() which reloads the config again when args.print_shell_command is true; avoid double reads and TOCTOU by changing build_shell_command() to accept a config object (or optional config parameter) and passing the already-loaded config from main() into build_shell_command() instead of letting it re-open DEFAULT_ENV_FILE; update build_shell_command(signature) and its callers to accept and use the provided config when present, falling back to the existing file-read only if the parameter is None.src/bluetooth_2_usb/test_harness_capture.py (1)
728-731: Clarify whygrab_devicesparameter is unused.The
grab_devicesparameter is accepted but immediately deleted. If this is intentionally a no-op for the hidapi backend (since hidapi doesn't support exclusive access), consider documenting this or using_prefix to signal it's unused.def run_capture( scenario_name: str, timeout_sec: float = 5.0, device_substring: str = DEFAULT_DEVICE_SUBSTRING, keyboard_node: str | None = None, mouse_node: str | None = None, consumer_node: str | None = None, - grab_devices: bool = True, + grab_devices: bool = True, # Unused: hidapi backend cannot grab devices exclusively ) -> HarnessResult: - del grab_devices + del grab_devices # hidapi doesn't support exclusive device access🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/test_harness_capture.py` around lines 728 - 731, The parameter grab_devices in the function test_harness_capture (or whichever function declares "grab_devices: bool = True") is accepted then immediately deleted, which is confusing; update the declaration to indicate it's intentionally unused for the hidapi backend by either renaming the parameter to _grab_devices (or _grab_devices: bool = True) or removing the del grab_devices and replacing it with a brief comment such as "# grab_devices intentionally ignored for hidapi (no exclusive access)" so callers see it's a no-op; ensure references to grab_devices in the function signature (and any docstring) are updated accordingly.scripts/smoke_test.sh (1)
242-262: Consider handling Python invocation failures more gracefully.The inline Python script at lines 244-251 could fail if Python itself has issues, leaving
RELAYABLE_COUNTempty. The subsequent check at line 253 handles empty as 0, but the error case at 259-261 only triggers if the outer command fails. If the Python JSON parsing raises an exception, the script might produce confusing output.♻️ Optional: Add fallback for parse errors
RELAYABLE_COUNT="$( "${VENV_DIR}/bin/python" - "$LIST_DEVICES_JSON" <<'PY' import json import sys - -with open(sys.argv[1], encoding="utf-8") as handle: - devices = json.load(handle) -print(sum(1 for device in devices if device.get("relay_candidate"))) +try: + with open(sys.argv[1], encoding="utf-8") as handle: + devices = json.load(handle) + print(sum(1 for device in devices if device.get("relay_candidate"))) +except Exception: + print("") PY - )" + )" || RELAYABLE_COUNT=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/smoke_test.sh` around lines 242 - 262, The inline Python block that computes RELAYABLE_COUNT can fail and leave RELAYABLE_COUNT empty; update the invocation so parsing errors are detected and a safe default is used: run the Python snippet and check its exit status (or make the snippet catch exceptions and print "0" and exit non‑zero), then assign RELAYABLE_COUNT only if the command succeeded, otherwise set RELAYABLE_COUNT=0 and log the parsing failure via warn including contents of LIST_DEVICES_JSON and set EXIT_CODE=1; refer to VENV_DIR, LIST_DEVICES_JSON, RELAYABLE_COUNT, ok, soft_warn, warn, and EXIT_CODE when locating where to add the checks and error handling.src/bluetooth_2_usb/test_harness_capture_windows.py (1)
750-752: Document whywndprocreference is kept.The
del wndprocstatement at line 751 looks like it's discarding the reference, but actually thewndprocWNDPROC ctypes callback must stay alive for the duration of the window's lifetime. The variable is returned from_create_message_window()specifically to prevent garbage collection. A brief comment would clarify this non-obvious pattern.hwnd, wndproc = _create_message_window() - del wndproc + del wndproc # Prevent linter warning; reference kept alive in window class🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/test_harness_capture_windows.py` around lines 750 - 752, The code currently deletes the wndproc reference after calling _create_message_window(), but that callback must remain alive for the window's lifetime; find the del wndproc near the hwnd, wndproc = _create_message_window() call and replace it with a short comment explaining that the wndproc WNDPROC ctypes callback must be retained to prevent garbage collection (or alternatively retain the reference in a module-level variable or attach it to the hwnd object) so that the callback isn't freed while the window exists; keep the subsequent _register_raw_input(hwnd) call unchanged.src/bluetooth_2_usb/cli.py (2)
233-234: Nitpick: redundant.clear()on new Event.
asyncio.Event()is created in unset state by default, so line 234'srelaying_active.clear()is redundant. Harmless, but could be removed for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/cli.py` around lines 233 - 234, Remove the redundant call to relaying_active.clear(): since relaying_active is initialized via asyncio.Event() (which starts unset), delete the extraneous relaying_active.clear() line so only relaying_active = asyncio.Event() remains; this affects the relaying_active Event in the CLI setup where asyncio.Event() is created.
142-153: Minor observation: redundant event set calls.In
_signal_handler,_request_shutdown(sig_name)already callsshutdown_event.set()on line 144, then lines 150-153 call it again viacall_soon_threadsafe. The double-set is harmless sinceasyncio.Event.set()is idempotent, but the redundancy is slightly confusing. This doesn't impact correctness—just noting for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/cli.py` around lines 142 - 153, The code calls shutdown_event.set() twice for the same signal (once inside _request_shutdown and again inside _signal_handler via active_loop.call_soon_threadsafe), which is redundant; pick one approach and remove the duplicate: either remove the shutdown_event.set() from _request_shutdown so it only logs the signal and let _signal_handler perform the thread-safe set via active_loop.call_soon_threadsafe(shutdown_event.set) with the RuntimeError fallback, or keep _request_shutdown as the single setter and delete the call_soon_threadsafe block in _signal_handler; update _request_shutdown and _signal_handler accordingly to reference the same shutdown_event and active_loop symbols.src/bluetooth_2_usb/test_harness_common.py (1)
317-318: Consider explicit error message for invalid scenario names.
get_scenariodoes a bare dict lookup that raisesKeyErrorwith just the invalid name. For user-facing CLI scenarios, a more descriptive error might help:def get_scenario(name: str) -> ScenarioDefinition: if name not in SCENARIOS: raise ValueError(f"Unknown scenario '{name}'. Valid: {', '.join(SCENARIO_NAMES)}") return SCENARIOS[name]This is optional since the caller might already validate against
SCENARIO_NAMES.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bluetooth_2_usb/test_harness_common.py` around lines 317 - 318, Replace the bare dict lookup in get_scenario with an explicit validation: check if name is not in SCENARIOS and raise a ValueError with a descriptive message that includes the invalid name and the list of valid scenario names (use SCENARIO_NAMES), otherwise return SCENARIOS[name]; this keeps get_scenario clear and user-friendly for CLI callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/doc-consistency-review-playbook.md`:
- Around line 46-65: The checklist currently only runs --help on the listed .sh
scripts and misses macOS and Windows entrypoints; update the consistency review
to also invoke the host-capture wrappers (host_relay_test_capture.command and
host_relay_test_capture.ps1) so their --help/usage is compared to the docs;
modify the loop/inspection logic in docs/doc-consistency-review-playbook.md to
include those two wrapper filenames (and any other platform-specific entrypoints
referenced in docs like host_relay_test_capture) so argument/passthrough drift
on macOS/Windows is detected.
In `@docs/pi-cli-service-test-playbook.md`:
- Around line 257-275: The uninstall validation must also assert that the
persistent mount for /var/lib/bluetooth is disabled; update the test block that
runs sudo -n /opt/bluetooth_2_usb/scripts/uninstall.sh to additionally check the
mount unit (e.g. run systemctl is-enabled var-lib-bluetooth.mount || true and
systemctl show -P UnitFileState var-lib-bluetooth.mount or systemctl is-active
var-lib-bluetooth.mount || true) and echo a clear marker (like mount-disabled or
mount-enabled) so regressions leaving var-lib-bluetooth.mount enabled will fail;
reference uninstall.sh, bluetooth_2_usb.service and var-lib-bluetooth.mount when
adding these checks.
In `@docs/pi-host-relay-loopback-test-playbook.md`:
- Around line 198-204: Update the third diagnostic command so it runs with
elevated privileges: change the command string "journalctl -u
bluetooth_2_usb.service -n 100 --no-pager" to include sudo at the start so it
becomes "sudo journalctl -u bluetooth_2_usb.service -n 100 --no-pager"; this
ensures the journalctl invocation for the bluetooth_2_usb.service log is
copy-pasteable for non-root Pi users when following the playbook.
In `@scripts/install.sh`:
- Around line 96-100: Capture the validate command's exit code and only treat
the known "pre-reboot missing prerequisites" code as a warn; for any other
non-zero exit, fail the install. Concretely: run "${VENV_DIR}/bin/python" -m
bluetooth_2_usb --validate-env, save its exit code ($?), compare it against a
named constant (e.g., PRE_REBOOT_EXIT) that you add and set to the documented
value for the pre-reboot condition, call warn "Environment validation reports
missing runtime prerequisites until after reboot" only when the code equals
PRE_REBOOT_EXIT, call ok on zero, and call error + exit 1 for any other non-zero
code; keep existing ok/warn helper names and the command invocation unchanged.
In `@scripts/lib/boot.sh`:
- Around line 128-154: normalize_modules_load() currently writes changes to the
provided cmdline file in-place; add the same timestamped backup behavior as
normalize_dwc2_overlay() before running the Python rewrite: check that the
cmdline_file exists and is writable, create a backup copy with a precise
timestamp suffix (e.g., append YYYYMMDDHHMMSS or epoch) and only proceed if the
backup succeeded, so rollback is possible if the Python step corrupts
cmdline.txt; ensure this backup step is performed at the start of
normalize_modules_load() and uses the same naming convention as
normalize_dwc2_overlay() for consistency.
- Around line 97-126: normalize_dwc2_overlay currently overwrites the provided
config file in-place; before running the Python rewrite logic add conservative,
timestamped backup behavior so the original file can be restored if something
goes wrong: create a copy of config_file (e.g., appending a timestamp or .bak
with date/time) prior to invoking the python3 block, verify the copy succeeded
and only proceed on success, and leave the backup in the same directory;
reference the normalize_dwc2_overlay function and the config_file variable when
implementing the backup and any error checks so the installer will always
produce a timestamped backup before modifying config.txt.
In `@scripts/lib/readonly.sh`:
- Around line 118-140: The write_persist_mount_unit function currently
interpolates persist_spec directly into the unit file; validate/sanitize
persist_spec (in write_persist_mount_unit, using persist_mount_unit_name to
locate the unit) before the here-doc: ensure it contains no newlines and only
allowed characters (e.g. /^[A-Za-z0-9_\/\.-:]+$/ or a whitelist of safe chars),
or fail early with an error/log if it doesn't match, and escape or reject any
unsafe input so malformed or injected systemd unit content cannot be written.
In `@scripts/setup_persistent_bluetooth_state.sh`:
- Around line 84-99: The seeding block currently ignores a failed mkdir for
seed_lock_dir and continues, which can hide a stale lock; update the mkdir call
handling so that if mkdir "$seed_lock_dir" fails (i.e., the lock already exists
or mkdir errored) the script prints an explicit error to stderr (including the
seed_lock_dir and PERSIST_BLUETOOTH_DIR) and exits non‑zero instead of
proceeding; keep the existing cleanup_seed_lock/trap logic inside the successful
mkdir branch (the else branch should not continue to seeding or the bind mount).
In `@src/bluetooth_2_usb/test_harness_inject.py`:
- Around line 36-39: _mouse_capabilities currently only advertises relative
motion (EV_REL) so any BTN_* events in scenario.mouse_button_steps will fail;
update _mouse_capabilities to include an EV_KEY entry listing the mouse button
key codes you need (e.g., BTN_LEFT, BTN_RIGHT, BTN_MIDDLE and any other BTN_*
used by tests) so the virtual device advertises those buttons and write() calls
succeed; modify the return dict in _mouse_capabilities() to add ecodes.EV_KEY:
[ecodes.BTN_LEFT, ecodes.BTN_RIGHT, ...] matching the buttons referenced by the
scenarios.
---
Duplicate comments:
In `@scripts/debug.sh`:
- Line 64: The call to load_readonly_config currently aborts the whole script
via fail() on a malformed /etc/default/bluetooth_2_usb_readonly; change this to
detect parse errors without exiting by invoking load_readonly_config in a
guarded context (e.g., a subshell or try-style pre-check) and set a PARSE_ERROR
flag when parsing fails instead of calling fail(); then update the later
consumers readonly_mode and bluetooth_state_persistent to check PARSE_ERROR and
emit an error indicator in the debug report if set (rather than relying on
values from the failed parse).
---
Nitpick comments:
In `@scripts/host_relay_test_capture.ps1`:
- Around line 26-34: The empty catch block after the attempt to run "& py -3 -c
\"import hid\" *> $null" is intentional to allow falling through when py -3 is
unavailable; add a brief inline comment inside that catch (referencing the catch
that follows the py invocation) stating that the exception is purposely ignored
so the script can continue to the final error handling and to silence
static-analysis warnings.
In `@scripts/install_host_hidapi_udev_rule.sh`:
- Around line 38-43: The udev trigger is too broad—replace the global "udevadm
trigger --subsystem-match=usb" call with a scoped trigger that matches the
vendor/product in your rule (use the same vendor/product values referenced by
RULE_SRC/RULE_DST, e.g., idVendor=1d6b and idProduct=0104) so only that device
class is re-enumerated; update the script to call udevadm trigger with
--subsystem-match=usb plus appropriate --attr-match=idVendor=... and
--attr-match=idProduct=... (or an equivalent attribute match) immediately after
reloading rules.
In `@scripts/lib/boot.sh`:
- Around line 61-64: Add a short explanatory comment above the heuristic check
using [[ -d /sys/module/dwc2 ]] in scripts/lib/boot.sh stating that this is a
last-resort heuristic (reached only if earlier kernel config checks found no
evidence) and that /sys/module/dwc2 may also exist when dwc2 is loaded as a
module, so the check can misclassify module vs builtin; keep the existing logic
but document the rationale and ordering to prevent future confusion.
In `@scripts/smoke_test.sh`:
- Around line 242-262: The inline Python block that computes RELAYABLE_COUNT can
fail and leave RELAYABLE_COUNT empty; update the invocation so parsing errors
are detected and a safe default is used: run the Python snippet and check its
exit status (or make the snippet catch exceptions and print "0" and exit
non‑zero), then assign RELAYABLE_COUNT only if the command succeeded, otherwise
set RELAYABLE_COUNT=0 and log the parsing failure via warn including contents of
LIST_DEVICES_JSON and set EXIT_CODE=1; refer to VENV_DIR, LIST_DEVICES_JSON,
RELAYABLE_COUNT, ok, soft_warn, warn, and EXIT_CODE when locating where to add
the checks and error handling.
In `@src/bluetooth_2_usb/cli.py`:
- Around line 233-234: Remove the redundant call to relaying_active.clear():
since relaying_active is initialized via asyncio.Event() (which starts unset),
delete the extraneous relaying_active.clear() line so only relaying_active =
asyncio.Event() remains; this affects the relaying_active Event in the CLI setup
where asyncio.Event() is created.
- Around line 142-153: The code calls shutdown_event.set() twice for the same
signal (once inside _request_shutdown and again inside _signal_handler via
active_loop.call_soon_threadsafe), which is redundant; pick one approach and
remove the duplicate: either remove the shutdown_event.set() from
_request_shutdown so it only logs the signal and let _signal_handler perform the
thread-safe set via active_loop.call_soon_threadsafe(shutdown_event.set) with
the RuntimeError fallback, or keep _request_shutdown as the single setter and
delete the call_soon_threadsafe block in _signal_handler; update
_request_shutdown and _signal_handler accordingly to reference the same
shutdown_event and active_loop symbols.
In `@src/bluetooth_2_usb/service_config.py`:
- Around line 192-200: main() currently loads configuration and then calls
build_shell_command() which reloads the config again when
args.print_shell_command is true; avoid double reads and TOCTOU by changing
build_shell_command() to accept a config object (or optional config parameter)
and passing the already-loaded config from main() into build_shell_command()
instead of letting it re-open DEFAULT_ENV_FILE; update
build_shell_command(signature) and its callers to accept and use the provided
config when present, falling back to the existing file-read only if the
parameter is None.
In `@src/bluetooth_2_usb/test_harness_capture_windows.py`:
- Around line 750-752: The code currently deletes the wndproc reference after
calling _create_message_window(), but that callback must remain alive for the
window's lifetime; find the del wndproc near the hwnd, wndproc =
_create_message_window() call and replace it with a short comment explaining
that the wndproc WNDPROC ctypes callback must be retained to prevent garbage
collection (or alternatively retain the reference in a module-level variable or
attach it to the hwnd object) so that the callback isn't freed while the window
exists; keep the subsequent _register_raw_input(hwnd) call unchanged.
In `@src/bluetooth_2_usb/test_harness_capture.py`:
- Around line 728-731: The parameter grab_devices in the function
test_harness_capture (or whichever function declares "grab_devices: bool =
True") is accepted then immediately deleted, which is confusing; update the
declaration to indicate it's intentionally unused for the hidapi backend by
either renaming the parameter to _grab_devices (or _grab_devices: bool = True)
or removing the del grab_devices and replacing it with a brief comment such as
"# grab_devices intentionally ignored for hidapi (no exclusive access)" so
callers see it's a no-op; ensure references to grab_devices in the function
signature (and any docstring) are updated accordingly.
In `@src/bluetooth_2_usb/test_harness_common.py`:
- Around line 317-318: Replace the bare dict lookup in get_scenario with an
explicit validation: check if name is not in SCENARIOS and raise a ValueError
with a descriptive message that includes the invalid name and the list of valid
scenario names (use SCENARIO_NAMES), otherwise return SCENARIOS[name]; this
keeps get_scenario clear and user-friendly for CLI callers.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9b3321e-ce3b-460e-ad73-4534e87de246
📒 Files selected for processing (55)
.gitattributes.github/workflows/PyPI-publish.yml.github/workflows/ci.ymlAGENTS.mdCONTRIBUTING.mdMANIFEST.inREADME.mdbluetooth_2_usb.servicedocs/doc-consistency-review-playbook.mddocs/pi-cli-service-test-playbook.mddocs/pi-host-relay-loopback-test-playbook.mddocs/pi-manual-test-plan.mddocs/release-versioning-policy.mdpyproject.tomlrequirements-host-capture.txtrequirements.txtscripts/debug.shscripts/disable_readonly_overlayfs.shscripts/enable_readonly_overlayfs.shscripts/host_relay_test_capture.commandscripts/host_relay_test_capture.ps1scripts/host_relay_test_capture.shscripts/install.shscripts/install_host_hidapi_udev_rule.shscripts/lib/bluetooth.shscripts/lib/boot.shscripts/lib/common.shscripts/lib/install.shscripts/lib/readonly.shscripts/lib/report.shscripts/pi_relay_test_inject.shscripts/setup_persistent_bluetooth_state.shscripts/smoke_test.shscripts/uninstall.shsrc/bluetooth_2_usb/__init__.pysrc/bluetooth_2_usb/args.pysrc/bluetooth_2_usb/cli.pysrc/bluetooth_2_usb/evdev.pysrc/bluetooth_2_usb/inventory.pysrc/bluetooth_2_usb/relay.pysrc/bluetooth_2_usb/service_config.pysrc/bluetooth_2_usb/service_runner.pysrc/bluetooth_2_usb/test_harness.pysrc/bluetooth_2_usb/test_harness_capture.pysrc/bluetooth_2_usb/test_harness_capture_windows.pysrc/bluetooth_2_usb/test_harness_common.pysrc/bluetooth_2_usb/test_harness_inject.pysrc/bluetooth_2_usb/version.pytests/test_args.pytests/test_cli.pytests/test_relay.pytests/test_service_config.pytests/test_test_harness.pytests/test_version.pyudev/70-bluetooth_2_usb_hidapi.rules
💤 Files with no reviewable changes (1)
- .github/workflows/PyPI-publish.yml
✅ Files skipped from review due to trivial changes (13)
- requirements-host-capture.txt
- .gitattributes
- udev/70-bluetooth_2_usb_hidapi.rules
- scripts/host_relay_test_capture.command
- MANIFEST.in
- tests/test_args.py
- .github/workflows/ci.yml
- docs/release-versioning-policy.md
- docs/pi-manual-test-plan.md
- AGENTS.md
- scripts/disable_readonly_overlayfs.sh
- README.md
- scripts/lib/common.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- src/bluetooth_2_usb/version.py
- pyproject.toml
- scripts/lib/report.sh
- scripts/uninstall.sh
- scripts/lib/install.sh
- src/bluetooth_2_usb/init.py
- src/bluetooth_2_usb/args.py
Comment on lines
+46
to
+65
| Compare the docs against the current `--help` output of all managed scripts: | ||
|
|
||
| ```bash | ||
| for s in \ | ||
| scripts/install.sh \ | ||
| scripts/uninstall.sh \ | ||
| scripts/debug.sh \ | ||
| scripts/smoke_test.sh \ | ||
| scripts/pi_relay_test_inject.sh \ | ||
| scripts/host_relay_test_capture.sh \ | ||
| scripts/install_host_hidapi_udev_rule.sh \ | ||
| scripts/enable_readonly_overlayfs.sh \ | ||
| scripts/disable_readonly_overlayfs.sh \ | ||
| scripts/setup_persistent_bluetooth_state.sh | ||
| do | ||
| echo "==== $s" | ||
| bash "$s" --help | ||
| echo | ||
| done | ||
| ``` |
There was a problem hiding this comment.
Cover the macOS and Windows host-capture entrypoints too.
docs/pi-host-relay-loopback-test-playbook.md now documents scripts/host_relay_test_capture.command and scripts/host_relay_test_capture.ps1, but this checklist only inspects the .sh entrypoint. Drift in wrapper arguments or passthrough behavior on those platforms would currently escape the consistency review. As per coding guidelines, "**/*.md: Keep documentation aligned with real script interfaces, defaults, and managed paths`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/doc-consistency-review-playbook.md` around lines 46 - 65, The checklist
currently only runs --help on the listed .sh scripts and misses macOS and
Windows entrypoints; update the consistency review to also invoke the
host-capture wrappers (host_relay_test_capture.command and
host_relay_test_capture.ps1) so their --help/usage is compared to the docs;
modify the loop/inspection logic in docs/doc-consistency-review-playbook.md to
include those two wrapper filenames (and any other platform-specific entrypoints
referenced in docs like host_relay_test_capture) so argument/passthrough drift
on macOS/Windows is detected.
Comment on lines
+257
to
+275
| ## Uninstall validation | ||
|
|
||
| ```bash | ||
| PI_HOST="${PI_HOST:-your-pi-host}" | ||
|
|
||
| ssh -4 "$PI_HOST" ' | ||
| sudo -n /opt/bluetooth_2_usb/scripts/uninstall.sh | ||
| systemctl is-active bluetooth_2_usb.service || true | ||
| systemctl show -P LoadState bluetooth_2_usb.service | ||
| test -d /opt/bluetooth_2_usb && echo checkout-present || echo checkout-missing | ||
| ' | ||
| ``` | ||
|
|
||
| Expected outcome: | ||
|
|
||
| - service integration is removed | ||
| - checkout remains present | ||
| - persistent mount units are disabled | ||
| - runtime env files and wrapper are removed |
There was a problem hiding this comment.
Verify the persistent mount cleanup during uninstall.
The expected outcome says uninstall.sh disables the persistent mounts, but this block never checks var-lib-bluetooth.mount or the persistence mount after uninstall. A regression that leaves those mounts enabled would still pass the playbook. As per coding guidelines, "**/*.md: Keep documentation aligned with real script interfaces, defaults, and managed paths`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/pi-cli-service-test-playbook.md` around lines 257 - 275, The uninstall
validation must also assert that the persistent mount for /var/lib/bluetooth is
disabled; update the test block that runs sudo -n
/opt/bluetooth_2_usb/scripts/uninstall.sh to additionally check the mount unit
(e.g. run systemctl is-enabled var-lib-bluetooth.mount || true and systemctl
show -P UnitFileState var-lib-bluetooth.mount or systemctl is-active
var-lib-bluetooth.mount || true) and echo a clear marker (like mount-disabled or
mount-enabled) so regressions leaving var-lib-bluetooth.mount enabled will fail;
reference uninstall.sh, bluetooth_2_usb.service and var-lib-bluetooth.mount when
adding these checks.
Comment on lines
+198
to
+204
| Check on the Pi: | ||
|
|
||
| ```bash | ||
| systemctl is-active bluetooth_2_usb.service | ||
| sudo /opt/bluetooth_2_usb/venv/bin/python -m bluetooth_2_usb --list_devices --output json | ||
| journalctl -u bluetooth_2_usb.service -n 100 --no-pager | ||
| ``` |
There was a problem hiding this comment.
Run the log check with sudo.
journalctl -u bluetooth_2_usb.service is the only privileged triage command in this block without sudo, so a normal Pi user can hit a permission error right when debugging a timeout. Please make that line sudo journalctl ... to keep the playbook copy-pasteable. As per coding guidelines, "**/*.md: Keep commands in documentation copy-pasteable`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/pi-host-relay-loopback-test-playbook.md` around lines 198 - 204, Update
the third diagnostic command so it runs with elevated privileges: change the
command string "journalctl -u bluetooth_2_usb.service -n 100 --no-pager" to
include sudo at the start so it becomes "sudo journalctl -u
bluetooth_2_usb.service -n 100 --no-pager"; this ensures the journalctl
invocation for the bluetooth_2_usb.service log is copy-pasteable for non-root Pi
users when following the playbook.
Comment on lines
+96
to
+100
| if "${VENV_DIR}/bin/python" -m bluetooth_2_usb --validate-env; then | ||
| ok "Environment validation passed" | ||
| else | ||
| warn "Environment validation reports missing runtime prerequisites until after reboot" | ||
| fi |
There was a problem hiding this comment.
Only downgrade the known pre-reboot exit from --validate-env.
This branch warns on any non-zero exit, so real validation regressions after the venv rebuild still leave install green and tell the operator to reboot. Please warn only on the expected prerequisite exit and fail the install for everything else.
Suggested fix
if "${VENV_DIR}/bin/python" -m bluetooth_2_usb --validate-env; then
ok "Environment validation passed"
else
- warn "Environment validation reports missing runtime prerequisites until after reboot"
+ rc=$?
+ if [[ "$rc" -eq 3 ]]; then
+ warn "Environment validation reports missing runtime prerequisites until after reboot"
+ else
+ fail "Environment validation failed with exit code ${rc}"
+ fi
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install.sh` around lines 96 - 100, Capture the validate command's
exit code and only treat the known "pre-reboot missing prerequisites" code as a
warn; for any other non-zero exit, fail the install. Concretely: run
"${VENV_DIR}/bin/python" -m bluetooth_2_usb --validate-env, save its exit code
($?), compare it against a named constant (e.g., PRE_REBOOT_EXIT) that you add
and set to the documented value for the pre-reboot condition, call warn
"Environment validation reports missing runtime prerequisites until after
reboot" only when the code equals PRE_REBOOT_EXIT, call ok on zero, and call
error + exit 1 for any other non-zero code; keep existing ok/warn helper names
and the command invocation unchanged.
Comment on lines
+97
to
+126
| normalize_dwc2_overlay() { | ||
| local config_file="$1" | ||
| local overlay_line="$2" | ||
|
|
||
| require_commands python3 | ||
| python3 - "$config_file" "$overlay_line" <<'PY' | ||
| from pathlib import Path | ||
| import sys | ||
|
|
||
| config_path = Path(sys.argv[1]) | ||
| overlay_line = sys.argv[2] | ||
| lines = config_path.read_text(encoding="utf-8").splitlines() | ||
| filtered = [line for line in lines if not line.lstrip().startswith("dtoverlay=dwc2")] | ||
|
|
||
| inserted = False | ||
| result = [] | ||
| for line in filtered: | ||
| result.append(line) | ||
| if not inserted and line.strip() == "[all]": | ||
| result.append(overlay_line) | ||
| inserted = True | ||
|
|
||
| if not inserted: | ||
| if result and result[-1] != "": | ||
| result.append("") | ||
| result.extend(["[all]", overlay_line]) | ||
|
|
||
| config_path.write_text("\n".join(result) + "\n", encoding="utf-8") | ||
| PY | ||
| } |
There was a problem hiding this comment.
Missing backup before modifying config.txt.
Per coding guidelines, boot changes should leave timestamped backups. normalize_dwc2_overlay() overwrites config.txt in-place without creating a backup. If the installer calls this function, a malformed write could leave the system unbootable.
Consider adding backup logic:
normalize_dwc2_overlay() {
local config_file="$1"
local overlay_line="$2"
+ local backup_file="${config_file}.b2u-backup.$(date +%Y%m%d%H%M%S)"
require_commands python3
+ cp -p "$config_file" "$backup_file"
python3 - "$config_file" "$overlay_line" <<'PY'As per coding guidelines: "Boot changes should be conservative and leave timestamped backups."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| normalize_dwc2_overlay() { | |
| local config_file="$1" | |
| local overlay_line="$2" | |
| require_commands python3 | |
| python3 - "$config_file" "$overlay_line" <<'PY' | |
| from pathlib import Path | |
| import sys | |
| config_path = Path(sys.argv[1]) | |
| overlay_line = sys.argv[2] | |
| lines = config_path.read_text(encoding="utf-8").splitlines() | |
| filtered = [line for line in lines if not line.lstrip().startswith("dtoverlay=dwc2")] | |
| inserted = False | |
| result = [] | |
| for line in filtered: | |
| result.append(line) | |
| if not inserted and line.strip() == "[all]": | |
| result.append(overlay_line) | |
| inserted = True | |
| if not inserted: | |
| if result and result[-1] != "": | |
| result.append("") | |
| result.extend(["[all]", overlay_line]) | |
| config_path.write_text("\n".join(result) + "\n", encoding="utf-8") | |
| PY | |
| } | |
| normalize_dwc2_overlay() { | |
| local config_file="$1" | |
| local overlay_line="$2" | |
| local backup_file="${config_file}.b2u-backup.$(date +%Y%m%d%H%M%S)" | |
| require_commands python3 | |
| cp -p "$config_file" "$backup_file" | |
| python3 - "$config_file" "$overlay_line" <<'PY' | |
| from pathlib import Path | |
| import sys | |
| config_path = Path(sys.argv[1]) | |
| overlay_line = sys.argv[2] | |
| lines = config_path.read_text(encoding="utf-8").splitlines() | |
| filtered = [line for line in lines if not line.lstrip().startswith("dtoverlay=dwc2")] | |
| inserted = False | |
| result = [] | |
| for line in filtered: | |
| result.append(line) | |
| if not inserted and line.strip() == "[all]": | |
| result.append(overlay_line) | |
| inserted = True | |
| if not inserted: | |
| if result and result[-1] != "": | |
| result.append("") | |
| result.extend(["[all]", overlay_line]) | |
| config_path.write_text("\n".join(result) + "\n", encoding="utf-8") | |
| PY | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/boot.sh` around lines 97 - 126, normalize_dwc2_overlay currently
overwrites the provided config file in-place; before running the Python rewrite
logic add conservative, timestamped backup behavior so the original file can be
restored if something goes wrong: create a copy of config_file (e.g., appending
a timestamp or .bak with date/time) prior to invoking the python3 block, verify
the copy succeeded and only proceed on success, and leave the backup in the same
directory; reference the normalize_dwc2_overlay function and the config_file
variable when implementing the backup and any error checks so the installer will
always produce a timestamped backup before modifying config.txt.
Comment on lines
+128
to
+154
| normalize_modules_load() { | ||
| local cmdline_file="$1" | ||
| local modules="$2" | ||
|
|
||
| require_commands python3 | ||
| python3 - "$cmdline_file" "$modules" <<'PY' | ||
| from pathlib import Path | ||
| import sys | ||
|
|
||
| cmdline_path = Path(sys.argv[1]) | ||
| modules = sys.argv[2] | ||
| tokens = cmdline_path.read_text(encoding="utf-8").strip().split() | ||
| existing = [] | ||
| for token in tokens: | ||
| if token.startswith("modules-load="): | ||
| existing.extend(value for value in token.split("=", 1)[1].split(",") if value) | ||
|
|
||
| merged = [] | ||
| for value in [*existing, *modules.split(",")]: | ||
| if value and value not in merged: | ||
| merged.append(value) | ||
|
|
||
| tokens = [token for token in tokens if not token.startswith("modules-load=")] | ||
| tokens.append("modules-load=" + ",".join(merged)) | ||
| cmdline_path.write_text(" ".join(tokens) + "\n", encoding="utf-8") | ||
| PY | ||
| } |
There was a problem hiding this comment.
Missing backup before modifying cmdline.txt.
Same concern as normalize_dwc2_overlay(): normalize_modules_load() modifies cmdline.txt without a timestamped backup. A corrupted cmdline can prevent boot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/boot.sh` around lines 128 - 154, normalize_modules_load()
currently writes changes to the provided cmdline file in-place; add the same
timestamped backup behavior as normalize_dwc2_overlay() before running the
Python rewrite: check that the cmdline_file exists and is writable, create a
backup copy with a precise timestamp suffix (e.g., append YYYYMMDDHHMMSS or
epoch) and only proceed if the backup succeeded, so rollback is possible if the
Python step corrupts cmdline.txt; ensure this backup step is performed at the
start of normalize_modules_load() and uses the same naming convention as
normalize_dwc2_overlay() for consistency.
Comment on lines
+118
to
+140
| write_persist_mount_unit() { | ||
| local persist_spec="$1" | ||
| local mount_path="$2" | ||
| local fs_type="$3" | ||
| local unit_name | ||
|
|
||
| unit_name="$(persist_mount_unit_name "$mount_path")" | ||
| cat >"/etc/systemd/system/${unit_name}" <<EOF | ||
| [Unit] | ||
| Description=bluetooth_2_usb persistent storage mount | ||
| Before=local-fs.target bluetooth.service ${B2U_SERVICE_UNIT} | ||
|
|
||
| [Mount] | ||
| What=${persist_spec} | ||
| Where=${mount_path} | ||
| Type=${fs_type} | ||
| Options=defaults,noatime | ||
|
|
||
| [Install] | ||
| WantedBy=local-fs.target | ||
| EOF | ||
| chmod 0644 "/etc/systemd/system/${unit_name}" | ||
| } |
There was a problem hiding this comment.
Validate persist_spec before writing to systemd unit file.
write_persist_mount_unit() interpolates $persist_spec directly into a systemd unit file. If the spec contains newlines or special characters (unlikely from blkid but possible from user override), the unit file could be malformed or exploited.
Consider validating the spec format:
write_persist_mount_unit() {
local persist_spec="$1"
local mount_path="$2"
local fs_type="$3"
local unit_name
+ # Basic validation: spec should be a path-like string
+ if [[ ! "$persist_spec" =~ ^/[^$'\n']+$ ]]; then
+ fail "Invalid persist_spec format: ${persist_spec}"
+ fi
+
unit_name="$(persist_mount_unit_name "$mount_path")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lib/readonly.sh` around lines 118 - 140, The write_persist_mount_unit
function currently interpolates persist_spec directly into the unit file;
validate/sanitize persist_spec (in write_persist_mount_unit, using
persist_mount_unit_name to locate the unit) before the here-doc: ensure it
contains no newlines and only allowed characters (e.g. /^[A-Za-z0-9_\/\.-:]+$/
or a whitelist of safe chars), or fail early with an error/log if it doesn't
match, and escape or reject any unsafe input so malformed or injected systemd
unit content cannot be written.
Comment on lines
+84
to
+99
| if [[ -d /var/lib/bluetooth ]]; then | ||
| seed_lock_dir="${PERSIST_BLUETOOTH_DIR}/.b2u-seed.lock" | ||
| seed_marker="${PERSIST_BLUETOOTH_DIR}/.b2u-seeded" | ||
| if mkdir "$seed_lock_dir" 2>/dev/null; then | ||
| cleanup_seed_lock() { | ||
| rmdir "$seed_lock_dir" 2>/dev/null || true | ||
| } | ||
| trap cleanup_seed_lock EXIT | ||
| if [[ ! -e "$seed_marker" ]] && [[ -z "$(find "$PERSIST_BLUETOOTH_DIR" -mindepth 1 -maxdepth 1 ! -name '.b2u-seed.lock' ! -name '.b2u-seeded' ! -name '.b2u-persistent-state' 2>/dev/null | head -n 1)" ]]; then | ||
| cp -a /var/lib/bluetooth/. "$PERSIST_BLUETOOTH_DIR"/ | ||
| touch "$seed_marker" | ||
| fi | ||
| cleanup_seed_lock | ||
| trap - EXIT | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Fail if the seed lock already exists.
When mkdir "$seed_lock_dir" fails, the script silently skips seeding and still switches /var/lib/bluetooth over to the persistent bind mount. A stale lock from an interrupted run can therefore bring the system up against an empty or partially seeded state.
Suggested fix
seed_lock_dir="${PERSIST_BLUETOOTH_DIR}/.b2u-seed.lock"
seed_marker="${PERSIST_BLUETOOTH_DIR}/.b2u-seeded"
if mkdir "$seed_lock_dir" 2>/dev/null; then
cleanup_seed_lock() {
rmdir "$seed_lock_dir" 2>/dev/null || true
@@
cleanup_seed_lock
trap - EXIT
+ else
+ fail "Seed lock ${seed_lock_dir} already exists. Remove the stale lock or wait for the active migration to finish."
fi
fiAs per coding guidelines, "scripts/**/*.sh: Shell scripts should fail loudly on ambiguous or unsafe input" and "scripts/**/*.sh: Treat install/uninstall/read-only flows in shell scripts as production code."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [[ -d /var/lib/bluetooth ]]; then | |
| seed_lock_dir="${PERSIST_BLUETOOTH_DIR}/.b2u-seed.lock" | |
| seed_marker="${PERSIST_BLUETOOTH_DIR}/.b2u-seeded" | |
| if mkdir "$seed_lock_dir" 2>/dev/null; then | |
| cleanup_seed_lock() { | |
| rmdir "$seed_lock_dir" 2>/dev/null || true | |
| } | |
| trap cleanup_seed_lock EXIT | |
| if [[ ! -e "$seed_marker" ]] && [[ -z "$(find "$PERSIST_BLUETOOTH_DIR" -mindepth 1 -maxdepth 1 ! -name '.b2u-seed.lock' ! -name '.b2u-seeded' ! -name '.b2u-persistent-state' 2>/dev/null | head -n 1)" ]]; then | |
| cp -a /var/lib/bluetooth/. "$PERSIST_BLUETOOTH_DIR"/ | |
| touch "$seed_marker" | |
| fi | |
| cleanup_seed_lock | |
| trap - EXIT | |
| fi | |
| fi | |
| if [[ -d /var/lib/bluetooth ]]; then | |
| seed_lock_dir="${PERSIST_BLUETOOTH_DIR}/.b2u-seed.lock" | |
| seed_marker="${PERSIST_BLUETOOTH_DIR}/.b2u-seeded" | |
| if mkdir "$seed_lock_dir" 2>/dev/null; then | |
| cleanup_seed_lock() { | |
| rmdir "$seed_lock_dir" 2>/dev/null || true | |
| } | |
| trap cleanup_seed_lock EXIT | |
| if [[ ! -e "$seed_marker" ]] && [[ -z "$(find "$PERSIST_BLUETOOTH_DIR" -mindepth 1 -maxdepth 1 ! -name '.b2u-seed.lock' ! -name '.b2u-seeded' ! -name '.b2u-persistent-state' 2>/dev/null | head -n 1)" ]]; then | |
| cp -a /var/lib/bluetooth/. "$PERSIST_BLUETOOTH_DIR"/ | |
| touch "$seed_marker" | |
| fi | |
| cleanup_seed_lock | |
| trap - EXIT | |
| else | |
| fail "Seed lock ${seed_lock_dir} already exists. Remove the stale lock or wait for the active migration to finish." | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup_persistent_bluetooth_state.sh` around lines 84 - 99, The
seeding block currently ignores a failed mkdir for seed_lock_dir and continues,
which can hide a stale lock; update the mkdir call handling so that if mkdir
"$seed_lock_dir" fails (i.e., the lock already exists or mkdir errored) the
script prints an explicit error to stderr (including the seed_lock_dir and
PERSIST_BLUETOOTH_DIR) and exits non‑zero instead of proceeding; keep the
existing cleanup_seed_lock/trap logic inside the successful mkdir branch (the
else branch should not continue to seeding or the bind mount).
Comment on lines
+36
to
+39
| def _mouse_capabilities() -> dict[int, list[int]]: | ||
| return { | ||
| ecodes.EV_REL: [ecodes.REL_X, ecodes.REL_Y], | ||
| } |
There was a problem hiding this comment.
Advertise mouse button keys on the virtual mouse.
run_inject() replays scenario.mouse_button_steps, but _mouse_capabilities() only exposes EV_REL. Any BTN_* step will fail at write time and get reported as a generic access error.
Suggested fix
def _mouse_capabilities() -> dict[int, list[int]]:
return {
ecodes.EV_REL: [ecodes.REL_X, ecodes.REL_Y],
+ ecodes.EV_KEY: [ecodes.BTN_LEFT, ecodes.BTN_RIGHT, ecodes.BTN_MIDDLE],
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bluetooth_2_usb/test_harness_inject.py` around lines 36 - 39,
_mouse_capabilities currently only advertises relative motion (EV_REL) so any
BTN_* events in scenario.mouse_button_steps will fail; update
_mouse_capabilities to include an EV_KEY entry listing the mouse button key
codes you need (e.g., BTN_LEFT, BTN_RIGHT, BTN_MIDDLE and any other BTN_* used
by tests) so the virtual device advertises those buttons and write() calls
succeed; modify the return dict in _mouse_capabilities() to add ecodes.EV_KEY:
[ecodes.BTN_LEFT, ecodes.BTN_RIGHT, ...] matching the buttons referenced by the
scenarios.
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
/opt/bluetooth_2_usbworkflow--validate-env,smoke_test.sh, anddebug.shIncluded work
install.shthat applies the current checkout to the managed system installuninstall.shthat removes system integration while leaving the checkout in placebluetooth_2_usb.service, packaging, and virtualenv rebuild behavior around the managed installblack,ruff,shfmt,shellcheck, andyamllintRelated issues
Fixes #138
Refs #139 for persistent read-only deployment support.
Refs #140 for HID compatibility and runtime hardening.
Summary by CodeRabbit
New Features
Documentation
Chores