英数かななどの特殊キーがendevだと送れないので、HID入力をhidrawで転送するように変更#1
Conversation
WalkthroughRaspberry Pi の /dev/hidraw をパースして入力レポートを生成・フォワードし、hidapp 経由で BlueZ に複数 report ID 対応の通知を行う実装および、それを検証する Vagrant E2E、ツール群、CI/Makefile、ドキュメントの追加・更新です。 ChangesHIDraw入力フォワーディング実装
BlueZ / HID 複数 report ID 対応
E2E / ツールチェーン / ドキュメント / CI
🎯 4 (Complex) | ⏱️ ~60 分 Sequence Diagram sequenceDiagram
participant HIDraw as /dev/hidraw
participant Forwarder as input.Forwarder
participant App as hidapp.App
participant DBus as DBusDaemon
HIDraw->>Forwarder: read bytes (poll/read)
Forwarder->>Forwarder: ParseDescriptor -> Report{ID,Data}
Forwarder->>App: send(input.Report)
App->>DBus: SendInputReport(InputReport{ID,Data})
DBus->>DBus: route to characteristic for report ID
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@internal/bluez/hid.go`:
- Around line 537-541: In notifyInputLocked, do not silently return nil when an
InputReport.ID is missing from app.inputReportPath; instead return a descriptive
error (e.g., using fmt.Errorf or errors.New) referencing the missing report ID
so callers can detect and handle the missing mapping. Update the function
notifyInputLocked to construct and return that error when ok == false (and
ensure callers of notifyInputLocked handle/propagate the returned error).
In `@internal/input/descriptor.go`:
- Around line 57-63: In the item parsing branch where itemType == 1 && tag == 8
(the Report ID item), add a guard after validating len(value) that rejects a
report ID of 0x00 by returning an error (e.g., fmt.Errorf("HID report ID must
not be 0x00")) instead of accepting it; keep setting reportID = value[0] and
descriptor.UsesReportID = true only for non-zero values so invalid descriptors
with report ID 0x00 are refused.
In `@internal/input/forwarder_other.go`:
- Around line 15-20: The current Forwarder methods (Descriptor and Run) always
return nil, hiding that this file targets non-Linux; change them to return an
explicit unsupported-OS error instead. Define a package-level error variable
(e.g., ErrUnsupportedOS = errors.New("forwarder: unsupported OS")) and have
Forwarder.Descriptor() return (Descriptor{}, ErrUnsupportedOS) and
Forwarder.Run(...) return ErrUnsupportedOS so callers can detect unsupported
environments; update imports to include "errors" if needed and reference the
symbols Forwarder.Descriptor, Forwarder.Run, and ErrUnsupportedOS.
In `@scripts/hid-e2e.sh`:
- Line 74: The global kill line `pkill -x python3` stops all python3 processes
and destabilizes tests; change it to target only the test-launched process by
either recording and killing its PID when you start it (save the background
process PID and use `kill $PID || true`) or by using a specific process name
variable (replace `pkill -x python3` with `pkill -x "$TEST_PROCESS_NAME" ||
true`) that matches the exact command started by the script; update any start
logic to export that PID or set TEST_PROCESS_NAME so the tear-down only
terminates the intended process.
In `@tools/hci-proxy.py`:
- Around line 57-61: write_all_fd currently assumes os.write never raises; when
called (e.g., write_all_fd(vhci_fd, packet)) on a non‑blocking fd a
BlockingIOError will crash the process—wrap the os.write loop in exception
handling to handle BlockingIOError and retry until the full buffer is written:
on BlockingIOError (or EAGAIN) wait for the fd to become writable (use
select.select or poll) then continue the loop, also handle InterruptedError by
retrying the write, and ensure the memoryview slice (view = view[written:]) is
only applied after a successful write; update function write_all_fd accordingly
so writes to non‑blocking vhci_fd do not abort the program.
- Around line 111-121: The bridge() loop forks a child for each connection
(os.fork()) but never reaps children, causing zombies; add child-reaping in the
parent by installing a SIGCHLD handler that calls os.waitpid(-1, os.WNOHANG) in
a loop (or call waitpid periodically) so exited children from raw_proxy() are
collected, or alternatively perform a non-blocking waitpid loop after accept();
ensure the handler is registered once at module startup and that bridge()
continues accepting connections in the parent.
- Around line 70-78: The code makes sockets non-blocking in connect_tcp
(connection.setblocking(False)) but later calls sendall() without handling
BlockingIOError, causing crashes under congestion; add a retrying non-blocking
send helper (e.g., send_all_nonblocking or retry_send) that mirrors the recv
BlockingIOError handling: catch BlockingIOError, wait for writability with
select.select (or similar) until deadline, retry partial sends, propagate other
exceptions, and replace the raw sendall() calls with this helper; ensure the
helper uses the same timeout/deadline logic and integrates with existing
last_error/timeout behavior used in connect_tcp.
In `@Vagrantfile`:
- Around line 29-31: The Vagrantfile currently downloads and extracts the Go
tarball stored in variables go_archive and tmp_dir without verification; modify
the install sequence to fetch or define the expected SHA256 checksum (e.g., from
the official Go release checksum file or a known constant), compute the
downloaded file's SHA256 (for "${tmp_dir}/${go_archive}"), compare it to the
expected value, and only proceed to run rm -rf /usr/local and tar -C /usr/local
-xzf "${tmp_dir}/${go_archive}" if the checksums match; if the checksum
verification fails, abort with a clear error message and nonzero exit status.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 267c7eff-1c6f-4683-a6a3-69c9c21f7bfb
📒 Files selected for processing (30)
.dockerignore.gitignoreMakefileREADME.ja.mdREADME.mdVagrantfilecompose.yamldocker/hid-e2e/Dockerfiledocker/hid-e2e/run.shdocs/internal/.gitignoreexamples/config.rpi.yamlgo.modinternal/bluez/hid.gointernal/bluez/hid_test.gointernal/config/config.gointernal/config/config_test.gointernal/hidapp/app.gointernal/hidapp/app_test.gointernal/hidreport/report.gointernal/hidreport/report_test.gointernal/input/descriptor.gointernal/input/descriptor_test.gointernal/input/forwarder_linux.gointernal/input/forwarder_other.gointernal/rpiapp/app_test.goscripts/hid-e2e.shtools/bluez-agent.pytools/bluez-pair.pytools/hci-proxy.pytools/hidraw-cuse.c
💤 Files with no reviewable changes (6)
- .dockerignore
- internal/hidreport/report.go
- internal/hidreport/report_test.go
- compose.yaml
- docker/hid-e2e/run.sh
- docker/hid-e2e/Dockerfile
GitHub Actionsの入口をmake ciに変更し、Goの通常検査に加えてrace test、ビルド、go.mod/go.sumの整合性、shell/Python/Vagrantfile/C helperの構文検査をCIで落とせるようにする。
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@Makefile`:
- Line 1: Add two make targets named all and clean and include them in the
existing .PHONY list so make's default behavior and cleanup are explicit;
implement all as the default build target that depends on the primary build
steps (reuse existing targets like build, fmt, lint as appropriate) and
implement clean to remove generated artifacts (build outputs, temp files, vendor
modules if applicable), ensuring .PHONY: includes all and clean alongside the
existing targets.
In `@scripts/hid-e2e.sh`:
- Around line 109-115: The btmgmt_cmd function currently swallows failures by
appending "|| true" to the script invocation; remove the "|| true" so failures
propagate, and ensure btmgmt_cmd (and the other occurrences around the block
referenced) returns non-zero on error so the caller can exit immediately—update
callers or the main script flow to check the btmgmt_cmd exit status (or use set
-e) so that any failure running script -qfec btmgmt (writing to /tmp/btmgmt.log)
aborts the run instead of continuing with the invalid state.
In `@tools/hci-proxy.py`:
- Around line 95-103: connect_tcp currently calls connection.connect() as a
blocking call so the overall deadline can be exceeded; before each connect
attempt compute remaining = deadline - time.monotonic(), set that as the
per-socket timeout (e.g., connection.settimeout(max(0, remaining))) so the
connect raises socket.timeout when it exceeds the remaining time, catch
socket.timeout and other exceptions, close the socket on failure and continue
retrying until deadline, and only call connection.setblocking(False) after a
successful connect; update function connect_tcp to use per-attempt
remaining-time socket timeouts and clean up sockets on errors.
- Around line 149-153: ループ内の upstream.connect(unix_path) が OSError
を捕捉しておらず接続失敗でプロセスが落ちるので、acceptループ（server.accept() を呼ぶ箇所）内で upstream.connect を
try/except OSError で囲み、例外時は client と upstream のソケットを安全に close してログ出力後に continue
して次の接続を待つようにしてください；また pid = os.fork() や以降の子/親処理は upstream.connect
に成功した後にのみ実行されるよう順序を保ってください（関数/変数参照: upstream.connect, server.accept, client,
upstream, pid, os.fork）。
In `@Vagrantfile`:
- Line 38: printf 'export PATH=/usr/local/go/bin:$PATH\\n' >/etc/profile.d/go.sh
currently writes the literal backslash-n into the file; change the write so it
emits a real trailing newline instead of the two characters "\n". Locate the
printf invocation (the line starting with printf 'export
PATH=/usr/local/go/bin:$PATH\\n') and replace it with a form that prints a
newline (for example by using printf with a format specifier and the PATH string
as an argument, or by switching to a command that interprets escapes) so
/etc/profile.d/go.sh contains a proper newline-terminated export line.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e96c774d-2ff5-419c-8bd0-5c5a674c3cc2
📒 Files selected for processing (22)
.github/workflows/ci.yml.golangci.ymlMakefileREADME.ja.mdREADME.mdVagrantfilecmd/kbd-hid/main.gointernal/bluez/agent.gointernal/bluez/hid.gointernal/bluez/hid_test.gointernal/bluez/properties.gointernal/config/config.gointernal/execx/runner.gointernal/hidapp/app.gointernal/input/descriptor.gointernal/input/descriptor_test.gointernal/input/forwarder_other.gointernal/localapp/app.gointernal/rpiapp/app.gointernal/state/state.goscripts/hid-e2e.shtools/hci-proxy.py
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@internal/input/forwarder_linux.go`:
- Around line 107-109: The comparison uses int `size` (from unix.IoctlGetInt)
against `math.MaxUint32`, which can break on 32-bit Linux; explicitly convert
`size` to an unsigned 64-bit value before comparing and also guard against
negative `size` values. Update the check around `size` in the code that
constructs the `Descriptor` (the block that returns `Descriptor{},
fmt.Errorf("%w: %s has %d bytes", errDescriptorSizeOutOfRange, device, size)`)
to first return an error if `size < 0`, then compare using `uint64(size) >
math.MaxUint32` when deciding whether to return `errDescriptorSizeOutOfRange`.
In `@tools/bluez-agent.py`:
- Line 97: The Agent instance created with Agent(bus.raw, AGENT_PATH) is not
stored, so Python may GC the dbus.service.Object and unexport it; modify main()
to assign the result to a persistent variable (e.g., agent = Agent(bus.raw,
AGENT_PATH)) and keep that reference in main scope for the duration of the main
loop so the agent remains alive and exported on D-Bus; ensure the variable is
not shadowed or scoped locally-only (keep it in main's scope or module-level if
needed).
In `@tools/stubs/dbus/mainloop/glib.pyi`:
- Around line 3-4: DBusGMainLoop のコンストラクタ定義が実APIと異なり set_as_default
を必須にしているため、型チェックで引数なしインスタンス化が誤検出されます。スタブの DBusGMainLoop.__init__ の
set_as_default パラメータをオプショナルにしデフォルト値 False
を与えて、引数なしでもインスタンス化できるように変更してください（参照シンボル: DBusGMainLoop, __init__,
set_as_default）。
In `@tools/stubs/dbus/service.pyi`:
- Around line 14-19: method デコレータの型シグネチャで in_signature と out_signature
が必須になっているため、実APIと一致させるためにこれらをオプショナルに変更してください（デフォルト値 None を与える）。具体的には
tools/stubs/dbus/service.pyi 内の method の定義を修正し、dbus_interface
は必須のままにしておき、in_signature: Optional[str] = None および out_signature: Optional[str]
= None のようにデフォルトを設定して型注釈を更新してください。
🪄 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: ASSERTIVE
Plan: Pro
Run ID: bc3a9eb2-eda3-4e5d-884f-4cc869df8686
⛔ Files ignored due to path filters (1)
tools/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/ci.yml.gitignoreMakefileREADME.ja.mdREADME.mdVagrantfileinternal/input/forwarder_linux.goscripts/hid-e2e.shtools/.gitignoretools/README.ja.mdtools/README.mdtools/bluez-agent.pytools/bluez-pair.pytools/hci-proxy.pytools/lib/bluez_dbus.pytools/pyproject.tomltools/pyrightconfig.jsontools/stubs/dbus/__init__.pyitools/stubs/dbus/mainloop/__init__.pyitools/stubs/dbus/mainloop/glib.pyitools/stubs/dbus/service.pyitools/stubs/gi/__init__.pyitools/stubs/gi/repository/GLib.pyitools/stubs/gi/repository/__init__.pyi
| if size > math.MaxUint32 { | ||
| return Descriptor{}, fmt.Errorf("%w: %s has %d bytes", errDescriptorSizeOutOfRange, device, size) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/input/forwarder_linux.go | head -120 | tail -20Repository: RarkHopper/RpiKeyboardSwitcher
Length of output: 958
🏁 Script executed:
cat -n internal/input/forwarder_linux.go | head -110 | tail -25Repository: RarkHopper/RpiKeyboardSwitcher
Length of output: 925
🏁 Script executed:
grep -n "math.MaxUint32\|math.MaxInt\|import" internal/input/forwarder_linux.go | head -20Repository: RarkHopper/RpiKeyboardSwitcher
Length of output: 156
🏁 Script executed:
cd internal/input && head -10 forwarder_linux.goRepository: RarkHopper/RpiKeyboardSwitcher
Length of output: 158
32bit Linux での型比較が失敗する可能性があります。
size は int 型（unix.IoctlGetInt() の戻り値）で、32bit 環境では 32bit 符号付きです。math.MaxUint32 (4294967295) は 32bit int で表現できない値のため、比較時にアーキテクチャ依存の問題が発生します。明示的に uint64 に変換して比較してください。
修正案
- if size > math.MaxUint32 {
+ if uint64(size) > math.MaxUint32 {
return Descriptor{}, fmt.Errorf("%w: %s has %d bytes", errDescriptorSizeOutOfRange, device, size)
}🤖 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 `@internal/input/forwarder_linux.go` around lines 107 - 109, The comparison
uses int `size` (from unix.IoctlGetInt) against `math.MaxUint32`, which can
break on 32-bit Linux; explicitly convert `size` to an unsigned 64-bit value
before comparing and also guard against negative `size` values. Update the check
around `size` in the code that constructs the `Descriptor` (the block that
returns `Descriptor{}, fmt.Errorf("%w: %s has %d bytes",
errDescriptorSizeOutOfRange, device, size)`) to first return an error if `size <
0`, then compare using `uint64(size) > math.MaxUint32` when deciding whether to
return `errDescriptorSizeOutOfRange`.
|
|
||
| dbus.mainloop.glib.DBusGMainLoop(set_as_default=True) | ||
| bus = system_bus() | ||
| Agent(bus.raw, AGENT_PATH) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
dbus-python の dbus.service.Object は、Python側で参照を保持しないと D-Bus 上のエクスポートが失われる可能性がありますか？
💡 Result:
はい、dbus-python の dbus.service.Object を使用する場合、Python 側でそのオブジェクトへの参照を保持し続ける必要があります。 Python 側でオブジェクトへの参照がなくなると、Python のガベージコレクションによってそのオブジェクトが破棄される可能性があります。オブジェクトが破棄されると、D-Bus 上でのエクスポートも自動的に解除され、クライアントからのアクセスができなくなります [1][2]。 そのため、D-Bus サービスとしてオブジェクトを公開し続ける必要がある場合は、メインループの実行中、そのオブジェクトインスタンスをグローバル変数やリスト、あるいはサービスを管理するクラスの属性などに格納し、生存期間を維持するように設計してください [3]。 もしオブジェクトを明示的に D-Bus から削除したい場合は、dbus.service.Object が提供する remove_from_connection メソッドを呼び出すことで、安全にエクスポートを解除することができます [2][4]。
Citations:
- 1: https://askubuntu.com/questions/226018/reference-counting-on-exported-dbus-objects
- 2: https://stackoverflow.com/questions/17446414/dbus-object-lifecycle
- 3: https://stackoverflow.com/questions/34482691/register-a-hello-world-dbus-service-object-and-method-using-python
- 4: https://dbus.freedesktop.org/doc/dbus-python/dbus.service.html
🏁 Script executed:
find . -name "bluez-agent.py" -type fRepository: RarkHopper/RpiKeyboardSwitcher
Length of output: 97
🏁 Script executed:
cat -n ./tools/bluez-agent.pyRepository: RarkHopper/RpiKeyboardSwitcher
Length of output: 4895
エージェント インスタンスへの参照を保持してください
Agent(bus.raw, AGENT_PATH) の戻り値を変数に保存していないため、Python のガベージコレクションによって dbus.service.Object が破棄され、D-Bus 上のエクスポートが失われるリスクがあります。main() のスコープで参照を保持し、メインループ実行中にエージェントが有効な状態を維持してください。
修正案
- Agent(bus.raw, AGENT_PATH)
+ agent = Agent(bus.raw, AGENT_PATH)📝 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.
| Agent(bus.raw, AGENT_PATH) | |
| agent = Agent(bus.raw, AGENT_PATH) |
🤖 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 `@tools/bluez-agent.py` at line 97, The Agent instance created with
Agent(bus.raw, AGENT_PATH) is not stored, so Python may GC the
dbus.service.Object and unexport it; modify main() to assign the result to a
persistent variable (e.g., agent = Agent(bus.raw, AGENT_PATH)) and keep that
reference in main scope for the duration of the main loop so the agent remains
alive and exported on D-Bus; ensure the variable is not shadowed or scoped
locally-only (keep it in main's scope or module-level if needed).
| class DBusGMainLoop: | ||
| def __init__(self, set_as_default: bool) -> None: ... |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# スタブファイルの確認
cat -n tools/stubs/dbus/mainloop/glib.pyiRepository: RarkHopper/RpiKeyboardSwitcher
Length of output: 217
🏁 Script executed:
# dbus-python の公式ドキュメントやソースコードの確認
# まず、リポジトリ内に dbus-python の参考資料がないか探索
find . -type f \( -name "*.md" -o -name "*.rst" -o -name "*.txt" \) | xargs grep -l "DBusGMainLoop" 2>/dev/null | head -5Repository: RarkHopper/RpiKeyboardSwitcher
Length of output: 56
🌐 Web query:
dbus-python DBusGMainLoop constructor default parameter set_as_default
💡 Result:
The DBusGMainLoop constructor in the dbus-python library accepts a set_as_default parameter [1][2]. When set to True, this parameter configures the GLib main loop as the default main loop for all subsequent D-Bus connections created in the process [1][3]. This is the recommended approach for integrating D-Bus with a GLib-based main loop (such as those used in GTK or GNOME applications) [1][2]. The standard usage pattern is: from dbus.mainloop.glib import DBusGMainLoop DBusGMainLoop(set_as_default=True) By calling this, you ensure that when you instantiate a bus connection (e.g., dbus.SessionBus or dbus.SystemBus), it automatically uses the GLib main loop to handle asynchronous calls, signals, and object exports [3]. If set_as_default is not used, or if no main loop is configured, these operations may raise a RuntimeError or fail to function correctly [3]. Note that importing the older dbus.glib module is deprecated because it sets the default main loop as a side effect of being imported, which is considered non-obvious and poor API design [1][2]. New programs should explicitly use the DBusGMainLoop class as shown above [1][2].
Citations:
- 1: https://dbus.freedesktop.org/doc/dbus-python/dbus.glib.html
- 2: https://github.com/zyga/dbus-python/blob/master/dbus/glib.py
- 3: https://dbus.freedesktop.org/doc/dbus-python/dbus.html
🌐 Web query:
site:github.com dbus-python dbus/mainloop/glib.py DBusGMainLoop __init__
💡 Result:
In the dbus-python library, DBusGMainLoop is a class used to integrate the D-Bus event loop with the GLib main loop [1][2]. This integration is necessary for applications that need to perform asynchronous D-Bus operations, such as receiving signals, exporting objects, or making non-blocking method calls [2][3]. The init method of DBusGMainLoop accepts an optional argument, set_as_default [1][2]. Usage: To use it, you should import the class and instantiate it before connecting to the D-Bus [2]. The recommended pattern is: from dbus.mainloop.glib import DBusGMainLoop DBusGMainLoop(set_as_default=True) When set_as_default is set to True, it configures the GLib main loop as the default main loop for all subsequent D-Bus connections created in the process [1][2]. If you do not set this, or if you need to use different main loops for different connections, you can instantiate DBusGMainLoop without arguments and pass the resulting object to the constructor of a D-Bus connection (e.g., dbus.SessionBus(mainloop=dbus_loop)) [2]. Important Considerations: 1. Initialization Order: It is critical to initialize the DBusGMainLoop before creating any D-Bus connections or importing modules that might implicitly create them, as D-Bus connections must be attached to a main loop upon creation to handle asynchronous events [2][3]. 2. Deprecation of dbus.glib: You should avoid importing the older dbus.glib module, as it is deprecated [1][2]. Importing it triggers a side effect that sets the default main loop, which is considered non-obvious and poor practice [1]. Always use the explicit import from dbus.mainloop.glib [1][2].
Citations:
- 1: https://github.com/zyga/dbus-python/blob/master/dbus/glib.py
- 2: https://github.com/warsaw/dbus-python/blob/master/doc/tutorial.txt
- 3: Need to set up DBusGMainLoop before importing networkmanager when connecting signal seveas/python-networkmanager#42
🌐 Web query:
dbus-python PyPI package DBusGMainLoop set_as_default parameter default value
💡 Result:
In the dbus-python library, the set_as_default parameter for the DBusGMainLoop constructor defaults to False [1][2]. When set_as_default is set to True, it configures the GLib main loop as the default main loop implementation for D-Bus, which is a common requirement for applications using D-Bus with GLib-based frameworks [1][2]. If left at its default value of False, the main loop must be explicitly associated with D-Bus connections, typically by passing the loop instance to the Bus constructor [2].
Citations:
- 1: https://dbus.freedesktop.org/doc/dbus-python/dbus.glib.html
- 2: https://github.com/warsaw/dbus-python/blob/master/doc/tutorial.txt
DBusGMainLoop の引数定義を実API に合わせてください。
set_as_default パラメータは実装では デフォルト値が False で、引数なしでもインスタンス化できます。現在のスタブ定義では必須になっているため、引数なし呼び出しが型検査で誤検知されます。
修正案
class DBusGMainLoop:
- def __init__(self, set_as_default: bool) -> None: ...
+ def __init__(self, set_as_default: bool = False) -> None: ...📝 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.
| class DBusGMainLoop: | |
| def __init__(self, set_as_default: bool) -> None: ... | |
| class DBusGMainLoop: | |
| def __init__(self, set_as_default: bool = False) -> None: ... |
🤖 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 `@tools/stubs/dbus/mainloop/glib.pyi` around lines 3 - 4, DBusGMainLoop
のコンストラクタ定義が実APIと異なり set_as_default を必須にしているため、型チェックで引数なしインスタンス化が誤検出されます。スタブの
DBusGMainLoop.__init__ の set_as_default パラメータをオプショナルにしデフォルト値 False
を与えて、引数なしでもインスタンス化できるように変更してください（参照シンボル: DBusGMainLoop, __init__,
set_as_default）。
| def method( | ||
| dbus_interface: str, | ||
| *, | ||
| in_signature: str, | ||
| out_signature: str, | ||
| ) -> Callable[[Callable[_P, _R]], Callable[_P, _R]]: ... |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# ファイルの確認
cat -n tools/stubs/dbus/service.pyi | head -30Repository: RarkHopper/RpiKeyboardSwitcher
Length of output: 621
🏁 Script executed:
# dbus-python の実装を確認するため、ライブラリの情報を調査
# dbus-python の method デコレータのシグネチャを確認
rg "def method" tools/stubs/dbus/ -A 10Repository: RarkHopper/RpiKeyboardSwitcher
Length of output: 396
🌐 Web query:
dbus-python dbus.service.method decorator signature parameters default values
💡 Result:
In the dbus-python library, the dbus.service.method decorator is used to export class methods as D-Bus methods [1][2]. Its signature is defined as follows [2]: dbus.service.method(dbus_interface, in_signature=None, out_signature=None, async_callbacks=None, sender_keyword=None, path_keyword=None, destination_keyword=None, message_keyword=None, connection_keyword=None, byte_arrays=False, rel_path_keyword=None, kwargs) Parameters and Default Values: dbus_interface (required): The D-Bus interface name to which the method belongs [2]. in_signature (default: None): A string representing the D-Bus signature of the input arguments [2]. out_signature (default: None): A string representing the D-Bus signature of the return value [2]. async_callbacks (default: None): Used for asynchronous method handling [2]. sender_keyword (default: None): If provided, the sender's unique name is passed to the method as a keyword argument with this name [2]. path_keyword (default: None): If provided, the destination object path is passed to the method as a keyword argument with this name [2]. destination_keyword (default: None): If provided, the destination bus name is passed to the method as a keyword argument with this name [2]. message_keyword (default: None): If provided, the underlying D-Bus message object is passed to the method as a keyword argument with this name [2]. connection_keyword (default: None): If provided, the dbus.connection.Connection object is passed to the method as a keyword argument with this name [2]. byte_arrays (default: False): If True, byte arrays are handled as such rather than as lists of bytes [2]. rel_path_keyword (default: None): If provided, the destination object path relative to the exported object path is passed to the method as a keyword argument with this name [2]. kwargs: Additional keyword arguments may be accepted depending on the implementation [2]. Note that while some modern Python D-Bus libraries (such as dbus-next or dbus-fast) use Python type annotations to determine signatures [3][4][5], the standard dbus-python library relies on these explicit decorator parameters [2].
Citations:
- 1: https://github.com/warsaw/dbus-python/blob/master/dbus/service.py
- 2: https://dbus.freedesktop.org/doc/dbus-python/dbus.service.html
- 3: https://github.com/altdesktop/python-dbus-next/blob/master/dbus_next/service.py
- 4: https://python-dbus-next.readthedocs.io/en/latest/high-level-service/index.html
- 5: https://dbus-fast.readthedocs.io/en/stable/high-level-service/
🏁 Script executed:
# dbus-python の公式ソースコードで method 関数の実装を確認
curl -s https://raw.githubusercontent.com/warsaw/dbus-python/master/dbus/service.py | head -200 | grep -A 30 "^def method"Repository: RarkHopper/RpiKeyboardSwitcher
Length of output: 56
🌐 Web query:
dbus-python method decorator dbus_interface required parameter
💡 Result:
In the dbus-python library, the dbus_interface parameter is a required argument for the @dbus.service.method and @dbus.service.signal decorators [1]. When defining a D-Bus service object by inheriting from dbus.service.Object, you must use these decorators to export methods or signals [1]. The dbus_interface parameter specifies the D-Bus interface name (e.g., 'com.example.Sample') under which the method or signal will be exposed on the bus [1]. Example usage: @dbus.service.method(dbus_interface='com.example.Sample', in_signature='v', out_signature='s') def MyMethod(self, var): return str(var) If this parameter is omitted, the decorator will not correctly associate the method with the intended D-Bus interface, as it is a mandatory part of the decorator's signature [1]. Note that other Python D-Bus libraries, such as dbus-next or dasbus, have different APIs and decorator requirements [2][3][4].
Citations:
- 1: https://dbus.freedesktop.org/doc/dbus-python/dbus.service.html
- 2: https://python-dbus-next.readthedocs.io/en/latest/high-level-service/index.html
- 3: https://dbus-fast.readthedocs.io/en/stable/high-level-service/service-interface.html
- 4: https://dasbus.readthedocs.io/en/latest/api/dasbus.server.interface.html
method デコレータのシグネチャで in_signature / out_signature の仕様が実APIと異なります。
in_signature / out_signature は実APIではオプショナルパラメータ（デフォルト値 None）ですが、スタブでは必須になっています。これらをオプショナルに変更してください。
ただし、dbus_interface は実APIでは必須パラメータなので、デフォルト値を追加すべきではありません。
修正提案
def method(
dbus_interface: str,
*,
- in_signature: str,
- out_signature: str,
+ in_signature: str | None = None,
+ out_signature: str | None = None,
) -> Callable[[Callable[_P, _R]], Callable[_P, _R]]: ...🤖 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 `@tools/stubs/dbus/service.pyi` around lines 14 - 19, method デコレータの型シグネチャで
in_signature と out_signature が必須になっているため、実APIと一致させるためにこれらをオプショナルに変更してください（デフォルト値
None を与える）。具体的には tools/stubs/dbus/service.pyi 内の method の定義を修正し、dbus_interface
は必須のままにしておき、in_signature: Optional[str] = None および out_signature: Optional[str]
= None のようにデフォルトを設定して型注釈を更新してください。
RarkHopper
force-pushed
the
a-shinagawa/hidraw-forwarding
branch
from
67cceca to
edcea81
Compare
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)
.github/workflows/ci.yml (1)
18-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGitHub Actions はコミットSHAで固定してください。
タグ参照（
@v6、@v8.1.0など）は、サプライチェーン観点で改ざん耐性が弱いです。主要なアクションは40桁のSHA値で固定することで、セキュリティが大幅に向上します。現在以下の4箇所でタグ参照が使われています：
- 18行目：
actions/checkout@v6- 21行目：
actions/setup-go@v6- 29行目：
astral-sh/setup-uv@v8.1.0- 35行目：
hashicorp/setup-packer@v3.1.0各アクションの最新コミットSHAを確認して置き換えてください。
🤖 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/ci.yml at line 18, GitHub Actions のワークフローでタグ参照を使っている箇所（actions/checkout@v6, actions/setup-go@v6, astral-sh/setup-uv@v8.1.0, hashicorp/setup-packer@v3.1.0）をそれぞれ対応する公式リポジトリで最新のリリースコミットの40文字SHAで置き換えてください（例: uses: actions/checkout@<40-char-SHA>）；各アクションのリポジトリで該当ブランチ/タグの最新コミットSHAを取得して、.github/workflows/ci.yml の該当エントリをタグ指定からフルSHA指定に更新してください。
🤖 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 `@packer/cloud-init/user-data`:
- Around line 4-5: Replace the build-time password-based SSH settings so the
final image disables password authentication: set ssh_pwauth to false and ensure
the vagrant user is locked (lock_passwd: true) and does not retain a known
password; remove or clear any plaintext password/hash fields for the user and
keep only the ssh-authorized-keys entry so SSH uses key-based auth only. Update
the user block(s) where ssh_pwauth, lock_passwd, and the vagrant account are
defined (references: ssh_pwauth, lock_passwd, user: vagrant) and apply the same
changes to the other occurrences noted (lines ~19-20 and ~24-25).
In `@scripts/hid-e2e.sh`:
- Around line 406-421: The script's main() only calls print_logs on the specific
empty-MAC branch, so other failures (with set -e) exit without collecting VM
logs; add a global ERR trap that invokes print_logs (or a wrapper like on_error
that calls print_logs and preserves exit code) so any premature exit runs log
collection. Update the script to register trap 'print_logs' (or trap 'on_error'
where on_error calls print_logs) for ERR and optionally EXIT near the top
(before main/run) so functions like main, start_vms, prepare_central,
prepare_peripheral, pair_central_with_peripheral, etc., always produce logs on
failure.
- Around line 332-335: The script assigns hidraw_path from a find result and
then unconditionally runs hidraw_path="/dev/$(basename "$hidraw_path")", which
will produce a misleading result when find returns empty; modify the logic
around the hidraw_path variable to check whether the find result is non-empty
(e.g., test if hidraw_path is empty or null) before running basename, and if
empty print a clear error message (including context like "no hidraw device
found for uhid") to stderr and exit with a non-zero status; keep the assignment
to event_path as-is and reference the hidraw_path variable name and the basename
conversion ("/dev/$(basename \"$hidraw_path\")") when making the change.
In `@scripts/provision-e2e-vm.sh`:
- Around line 38-39: The curl download command using "${go_archive}" and
"${tmp_dir}/${go_archive}" should include retry and timeout flags to avoid
transient network failures; update the curl invocations (the line that runs curl
-fsSL "https://go.dev/dl/${go_archive}" -o "${tmp_dir}/${go_archive}" and the
similar occurrence around lines 46-47) to add sensible options such as --retry,
--retry-connrefused, --retry-delay and a total timeout like --max-time (or
--connect-timeout) so downloads are retried and don't hang indefinitely, and
keep the existing sha256sum verification step unchanged.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Line 18: GitHub Actions のワークフローでタグ参照を使っている箇所（actions/checkout@v6,
actions/setup-go@v6, astral-sh/setup-uv@v8.1.0,
hashicorp/setup-packer@v3.1.0）をそれぞれ対応する公式リポジトリで最新のリリースコミットの40文字SHAで置き換えてください（例:
uses:
actions/checkout@<40-char-SHA>）；各アクションのリポジトリで該当ブランチ/タグの最新コミットSHAを取得して、.github/workflows/ci.yml
の該当エントリをタグ指定からフルSHA指定に更新してください。
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a4132dd4-54a2-44ae-ad49-55b0ba7497a6
📒 Files selected for processing (16)
.github/workflows/ci.yml.gitignore.gitmodulesMakefileREADME.ja.mdREADME.mdVagrantfilepacker/cloud-init/meta-datapacker/cloud-init/network-configpacker/cloud-init/user-datapacker/e2e-utm.pkr.hclscripts/hid-e2e.shscripts/install-packer-utm-plugin.shscripts/install-vagrant-utm-plugin.shscripts/provision-e2e-vm.shthird_party/vagrant_utm
| ssh_pwauth: true | ||
|
|
There was a problem hiding this comment.
既知パスワードによるSSHログインを最終イメージに残さないでください。
ssh_pwauth: true と既知の vagrant パスワード（lock_passwd: false）の組み合わせだと、ボックス配布後もパスワード認証経路が残ります。ビルド時に必要でも、最終イメージではパスワード認証を無効化し、アカウントをロックして鍵認証のみに寄せてください。
Also applies to: 19-20, 24-25
🤖 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 `@packer/cloud-init/user-data` around lines 4 - 5, Replace the build-time
password-based SSH settings so the final image disables password authentication:
set ssh_pwauth to false and ensure the vagrant user is locked (lock_passwd:
true) and does not retain a known password; remove or clear any plaintext
password/hash fields for the user and keep only the ssh-authorized-keys entry so
SSH uses key-based auth only. Update the user block(s) where ssh_pwauth,
lock_passwd, and the vagrant account are defined (references: ssh_pwauth,
lock_passwd, user: vagrant) and apply the same changes to the other occurrences
noted (lines ~19-20 and ~24-25).
| event_path="$(cat /tmp/hid-e2e-event.path)" | ||
| hidraw_path="$(find /sys/devices/virtual/misc/uhid -maxdepth 3 -type d -name 'hidraw*' | sort | tail -1)" | ||
| hidraw_path="/dev/$(basename "$hidraw_path")" | ||
|
|
There was a problem hiding this comment.
hidraw検出失敗時の明示的チェックを追加してください
Line 333-335 は find が空でも hidraw_path="/dev/$(basename "$hidraw_path")" を実行します。未検出時に原因が見えづらい失敗になります。空チェックで即時に明確なエラーにしてください。
修正案
event_path="$(cat /tmp/hid-e2e-event.path)"
hidraw_path="$(find /sys/devices/virtual/misc/uhid -maxdepth 3 -type d -name 'hidraw*' | sort | tail -1)"
+if [ -z "$hidraw_path" ]; then
+ echo "hidraw device not found under /sys/devices/virtual/misc/uhid" >&2
+ exit 1
+fi
hidraw_path="/dev/$(basename "$hidraw_path")"📝 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.
| event_path="$(cat /tmp/hid-e2e-event.path)" | |
| hidraw_path="$(find /sys/devices/virtual/misc/uhid -maxdepth 3 -type d -name 'hidraw*' | sort | tail -1)" | |
| hidraw_path="/dev/$(basename "$hidraw_path")" | |
| event_path="$(cat /tmp/hid-e2e-event.path)" | |
| hidraw_path="$(find /sys/devices/virtual/misc/uhid -maxdepth 3 -type d -name 'hidraw*' | sort | tail -1)" | |
| if [ -z "$hidraw_path" ]; then | |
| echo "hidraw device not found under /sys/devices/virtual/misc/uhid" >&2 | |
| exit 1 | |
| fi | |
| hidraw_path="/dev/$(basename "$hidraw_path")" |
🤖 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 `@scripts/hid-e2e.sh` around lines 332 - 335, The script assigns hidraw_path
from a find result and then unconditionally runs hidraw_path="/dev/$(basename
"$hidraw_path")", which will produce a misleading result when find returns
empty; modify the logic around the hidraw_path variable to check whether the
find result is non-empty (e.g., test if hidraw_path is empty or null) before
running basename, and if empty print a clear error message (including context
like "no hidraw device found for uhid") to stderr and exit with a non-zero
status; keep the assignment to event_path as-is and reference the hidraw_path
variable name and the basename conversion ("/dev/$(basename \"$hidraw_path\")")
when making the change.
| main() { | ||
| start_vms | ||
| prepare_central | ||
| prepare_peripheral | ||
| mac="$(peripheral_address)" | ||
| if [ -z "$mac" ]; then | ||
| print_logs >&2 || true | ||
| fail "peripheral address was empty" | ||
| fi | ||
| pair_central_with_peripheral "$mac" | ||
| wait_for_central_evdev_keyboard "$mac" | ||
| start_central_input_capture | ||
| trigger_peripheral_hidraw_report | ||
| verify_central_key_a_input | ||
| log "passed: virtual HCI pair, BLE HID notification, hidraw report, and evdev KEY_A press/release" | ||
| } |
There was a problem hiding this comment.
失敗時ログ収集が一部経路で実行されません
Line 406-421 の main では、print_logs が Line 411 の空MAC分岐でしか呼ばれません。set -e による他の失敗経路では即終了し、解析に必要なVMログが欠落します。ERR トラップで常時回収してください。
修正案
main() {
+ trap 'print_logs >&2 || true' ERR
start_vms
prepare_central
prepare_peripheral
mac="$(peripheral_address)"
if [ -z "$mac" ]; then
- print_logs >&2 || true
fail "peripheral address was empty"
fi
pair_central_with_peripheral "$mac"📝 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.
| main() { | |
| start_vms | |
| prepare_central | |
| prepare_peripheral | |
| mac="$(peripheral_address)" | |
| if [ -z "$mac" ]; then | |
| print_logs >&2 || true | |
| fail "peripheral address was empty" | |
| fi | |
| pair_central_with_peripheral "$mac" | |
| wait_for_central_evdev_keyboard "$mac" | |
| start_central_input_capture | |
| trigger_peripheral_hidraw_report | |
| verify_central_key_a_input | |
| log "passed: virtual HCI pair, BLE HID notification, hidraw report, and evdev KEY_A press/release" | |
| } | |
| main() { | |
| trap 'print_logs >&2 || true' ERR | |
| start_vms | |
| prepare_central | |
| prepare_peripheral | |
| mac="$(peripheral_address)" | |
| if [ -z "$mac" ]; then | |
| fail "peripheral address was empty" | |
| fi | |
| pair_central_with_peripheral "$mac" | |
| wait_for_central_evdev_keyboard "$mac" | |
| start_central_input_capture | |
| trigger_peripheral_hidraw_report | |
| verify_central_key_a_input | |
| log "passed: virtual HCI pair, BLE HID notification, hidraw report, and evdev KEY_A press/release" | |
| } |
🤖 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 `@scripts/hid-e2e.sh` around lines 406 - 421, The script's main() only calls
print_logs on the specific empty-MAC branch, so other failures (with set -e)
exit without collecting VM logs; add a global ERR trap that invokes print_logs
(or a wrapper like on_error that calls print_logs and preserves exit code) so
any premature exit runs log collection. Update the script to register trap
'print_logs' (or trap 'on_error' where on_error calls print_logs) for ERR and
optionally EXIT near the top (before main/run) so functions like main,
start_vms, prepare_central, prepare_peripheral, pair_central_with_peripheral,
etc., always produce logs on failure.
| curl -fsSL "https://go.dev/dl/${go_archive}" -o "${tmp_dir}/${go_archive}" | ||
| printf '%s %s\n' "${go_linux_arm64_sha256}" "${tmp_dir}/${go_archive}" | sha256sum -c - |
There was a problem hiding this comment.
外部ダウンロードにリトライとタイムアウトを入れてください。
SHA検証はできていますが、curl が単発実行のため一時的なネットワーク不調でE2Eビルドが落ちやすいです。
修正例
-curl -fsSL "https://go.dev/dl/${go_archive}" -o "${tmp_dir}/${go_archive}"
+curl -fsSL --retry 5 --retry-delay 2 --connect-timeout 10 --max-time 300 \
+ "https://go.dev/dl/${go_archive}" -o "${tmp_dir}/${go_archive}"
@@
-curl -fsSL "https://github.com/astral-sh/uv/releases/download/${uv_version}/${uv_archive}" -o "${tmp_dir}/${uv_archive}"
+curl -fsSL --retry 5 --retry-delay 2 --connect-timeout 10 --max-time 300 \
+ "https://github.com/astral-sh/uv/releases/download/${uv_version}/${uv_archive}" -o "${tmp_dir}/${uv_archive}"Also applies to: 46-47
🤖 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 `@scripts/provision-e2e-vm.sh` around lines 38 - 39, The curl download command
using "${go_archive}" and "${tmp_dir}/${go_archive}" should include retry and
timeout flags to avoid transient network failures; update the curl invocations
(the line that runs curl -fsSL "https://go.dev/dl/${go_archive}" -o
"${tmp_dir}/${go_archive}" and the similar occurrence around lines 46-47) to add
sensible options such as --retry, --retry-connrefused, --retry-delay and a total
timeout like --max-time (or --connect-timeout) so downloads are retried and
don't hang indefinitely, and keep the existing sha256sum verification step
unchanged.
1 participant
Summary by CodeRabbit
新機能
改善
変更