refactor: land the standards-audit findings in batches#381
Open
AprilNEA wants to merge 17 commits into
Open
Conversation
…ners Every allow/expect in the workspace now states its reason inline (the four stragglers had it in a loose comment or not at all), and the one remaining dashed section banner is reduced to plain prose.
CARGO_PKG_VERSION is cargo-provided and always valid semver; falling back to 0.0.0 would silently make every release look like an upgrade. Panic with a reasoned expect instead of hiding the impossible case.
Expose the etcetera-backed home_dir from openlogi-core and use it for the launchd plist location instead of reading $HOME by hand — one home resolution policy for the whole workspace.
The receiver and feature listeners run in a void callback with no error channel, so every unrecognised byte used to degenerate into a silent return — losing whole events over an identity-only field: - Unifying/Bolt 0x41 device-connection: an unknown kind nibble dropped the event entirely; on the Unifying path arrival notifications are the only device source, so the device never appeared at all. Kind now folds to Unknown via num_enum(default) (identity-only by policy). - Pairing-info register reads no longer fail the whole read over the kind nibble either, matching the listener policy. - Bolt 0x54 pairing status: an unknown error code turned a failure into a session timeout; PairingError now carries the raw code in a num_enum(catch_all) Other variant. Same treatment for the 0x4e passkey press type. - Bolt 0x4d passkey: trim the NUL padding before decoding. - 0x1d4b WirelessDeviceStatus broadcast: the (re)connection signal was dropped if any of its three bytes was unrecognised; all fields now decode infallibly with catch_all Other variants. - smartshift_enhanced: an unknown wheel mode silently reported Ratchet; it is now Hidpp20Error::UnsupportedResponse, matching 0x2110 and hires_wheel (the pinned fallback test asserts the error instead). openlogi-hid's discovery kind mapping is simplified to the now- infallible From.
A corrupt Bolt passkey notification used to flow through from_utf8_lossy and parse().unwrap_or(0), rendering an all-Left click sequence that can never authenticate — with no diagnostic anywhere. The 0x4d decoder now NUL-trims and validates ASCII digits once, producing the digits and their numeric value together; a malformed payload fails the pairing session explicitly (surfaced over IPC as the generic transport-failure message, so the wire format is unchanged). The DPI write path clamped an out-of-range u32 to u16::MAX and wrote that to the device. The awaitable path now rejects it as the same OutOfRange feature error the device itself would return; the fire-and-forget path logs and skips the write.
The lighting color was a raw String parsed independently in two places with divergent silent fallbacks: the agent's parse_hex fell back to black (lights off) while the GUI's fell back to white and didn't strip a '#' prefix. A hand-edited config degraded differently on each side of the IPC with no diagnostic. openlogi-core now owns a validated Rgb newtype (exactly 6 hex digits) that serializes as the same hex string the field used to hold, so TOML files and the bincode wire format are unchanged — the pinned wire bytes in wire_format.rs prove it. Config load folds an unparseable value to white with the same documented per-field tolerance as the brightness clamp (failing the load would discard the whole user config). Every consumer — agent write path, GUI swatches and keyboard glow, CLI diag — now uses the typed components; both hand-rolled hex parsers are gone.
The hand-rolled temp-file writer used a fixed .toml.tmp name (a concurrent-save collision surface) and never fsynced the directory, so the rename could vanish on a crash. atomic-write-file — already the asset cache's writer — covers both; the 0600 mode on every save is preserved.
inject.rs simulated three whole backends as inline cfg-gated modules
(~1700 lines in one file). Each moves verbatim to inject/{macos,linux,
windows}.rs, matching the hook crate's layout; shared code and the
public API are unchanged.
asset/mod.rs, windows/mod.rs, and cmd/diag/mod.rs all carry real logic (the asset resolver, the window registry, the diag device-selection policy) — per the module-layout rule they are foo.rs files with their children in the sibling directory. Pure renames.
The ipc_client's inline pacing module (with its own tests) becomes a file; the duplicated file_url helper collapses to one copy; two one-line single-caller wrappers in app.rs are inlined; the hotspot geometry sorts with total_cmp instead of NaN-tolerant unwrap_or.
The hand-rolled env::temp_dir()+pid directories leaked on assertion failure; tempfile::tempdir cleans up on drop, as the core and xtask tests already do.
…oad.rs The lazy load-state types have no AppState dependency; state.rs keeps the state itself.
SyncOutcome/AssetControl/run_asset_sync are a self-contained subsystem; main.rs keeps startup wiring only.
app.rs interleaved the home gallery, the device-detail tabs, and the shared card/label primitives in one ~1900-line file. Each concern moves to its own module under app/; AppView, routing, and the capability- gated DetailTab logic stay in app.rs. Pure code motion.
detect_swipe and SwipeAccumulator are input-processing runtime, not part of the Action vocabulary the rest of binding.rs defines.
The hand-rolled env::temp_dir()+pid directories leaked on assertion failure; tempfile::tempdir cleans up on drop.
Brings openlogi-core to parity with openlogi-hid, which already enforces rustdoc coverage at compile time.
This was referenced Jul 13, 2026
Greptile SummaryThis PR lands a broad standards-audit refactor across HID handling, config, GUI structure, and docs. The main changes are:
Confidence Score: 4/5The changed config and pairing boundaries need fixes before merging.
crates/openlogi-core/src/config.rs; crates/openlogi-hid/src/pairing.rs Important Files Changed
Reviews (1): Last reviewed commit: "docs(core): document the remaining publi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+290
to
+292
| Ok(String::deserialize(deserializer)? | ||
| .parse() | ||
| .unwrap_or(Rgb::WHITE)) |
There was a problem hiding this comment.
When config.toml contains a common hand-edited value like color = "#ff00aa", this deserializes the raw string directly and Rgb::from_str rejects it because it is not exactly six characters. The load then silently stores Rgb::WHITE, so the next save overwrites the user's chosen lighting color with white.
Comment on lines
399
to
404
| let method = match pairing_auth { | ||
| Some(auth) if auth & 0x01 != 0 => PasskeyMethod::Keyboard(passkey), | ||
| Some(auth) if auth & 0x01 != 0 => PasskeyMethod::Keyboard(digits), | ||
| _ => PasskeyMethod::Pointer { | ||
| clicks: passkey_to_clicks(&passkey), | ||
| passkey, | ||
| clicks: passkey_to_clicks(value), | ||
| passkey: digits, | ||
| }, |
There was a problem hiding this comment.
When a keyboard-like Bolt device reaches this path without auth bit 0x01 set, the session can still have been opened with keyboard entropy, but this branch falls back to the pointer method and renders only the 10-bit click sequence. The user is shown the wrong authentication flow, so the receiver waits for the keyboard passkey and pairing times out or fails.
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
Lands the findings of a workspace-wide audit against the newly codified agent-guide standards (AGENTS.md, #379): silent wire-value drops, boundary-validation gaps, hand-rolled logic with mature in-tree replacements, and module-structure debt. Seventeen focused commits, one batch each, ordered fixes-first — intended for rebase-merge (every commit is release-quality conventional).
Changes
Behavior fixes
Unknown(num_enum(default)) instead of hiding the device — on the Unifying path arrival notifications are the only device source. Bolt 0x54 unknown pairing-error codes carry the raw byte (catch_all Other(u8)) instead of degenerating into a session timeout; same for the 0x4e press type. The 0x1d4b reconnection broadcast decodes infallibly.smartshift_enhancedreportsUnsupportedResponsefor an unknown wheel mode instead of silently claiming Ratchet, matching 0x2110/hires_wheel.parse().unwrap_or(0). Out-of-range DPI writes are rejected asOutOfRange(awaitable path) or logged-and-skipped (fire-and-forget path) instead of clampingu16::MAXonto the device.Rgbnewtype serialized as the same hex string — the two divergent hand-rolled parsers (agent fell back to black, GUI to white) are gone. The pinned bytes inwire_format.rsare unchanged, proving IPC compatibility.Dependency replacements
atomic-write-file(adds the directory fsync and randomized temp name the hand-rolled writer lacked; 0600 preserved).$HOMEresolution via the etcetera-backedpaths::home_dir().env::temp_dir()+pid test dirs →tempfile::tempdir().Structure (pure code motion)
inject/{macos,linux,windows}.rs(1691 → 682 lines), matching the hook crate's layout.app.rssplits along its three seams intoapp/{home,detail,widgets}.rs(1909 → 683);asset/mod.rs→asset.rs,windows/mod.rs→windows.rsper the module-layout rule; the ipc_clientpacingmodule and the asset-sync orchestration get their own files;Load/LazyDeviceData→state/load.rs; duplicatedfile_urlcollapsed; NaN-tolerant sorts →total_cmp.binding/swipe.rs;cmd/diag/mod.rs→diag.rs(cli).Docs & hygiene
# after documenting 140 flagged items.reason; the last section-banner comments are gone; the updater fails fast on an impossible invalid embedded version.Testing
devenv tasks run openlogi:check(fmt --check, workspace clippy-D warnings, full workspace tests incl. doc-tests) — green on the final tree; each commit was additionally verified with targeted clippy + tests before landing.cargo clippy --target x86_64-pc-windows-gnucross-lint on the touched cfg-shared crates (core, inject) — clean; Linux paths rely on CI as the authoritative check.wire_format.rs) pass with unchanged pinned bytes; noPROTOCOL_VERSIONbump needed.Follow-ups (tracked separately, out of scope here)
.ok()worst).PROTOCOL_VERSIONbump.