Clean up lint/LSP across gateway, Android, and desktop (alpha -> stable)

Gateway (gateway-plugin/): - Fix interactive_setup broken imports: print helpers were imported from the wrong hermes module (hermes_cli.config instead of hermes_cli.cli_output) plus a non-existent print_code; the try/except swallowed the ImportError so `hermes gateway setup` for android always bailed out early. - Fix release_scoped_lock type error (str | None passed where str required). - Rewrite empty `except: pass` blocks as contextlib.suppress with rationale. - Restructure two ambiguous ws_server try blocks (hello-auth, frame loop). - Ruff cleanup: type annotations, import sorting, line wrapping, magic values -> named constants, `raise ... from e`, complexity. Add gateway-plugin/ruff.toml. - Add pyrightconfig.json so the Python LSP resolves hermes-runtime imports. - Suppress verified false positives inline (parameterized SQL, column-name "secrets", hermes-generated media path). Android (app/androidApp + app/shared): - Consolidate launcher icons into a single mipmap-anydpi (minSdk 29 >= 26) with the monochrome layer; clears ObsoleteSdkInt + MonochromeLauncherIcon. - Bump core-splashscreen 1.0.1 -> 1.2.0; pin targetSdk 34 (deliberate). - Suppress verified findings inline (LAN ws:// default, correct GCM IV usage). Desktop (app/desktopApp): - Move the desktop to a Java 21 runtime (org.gradle.java.home) and set the desktop jvmTarget to 21 (Android stays JVM 17 / minSdk 29). Fixes the startup UnsupportedClassVersionError and restores Markdown renderer 0.44.0. Tooling/config: - .pi-lens.json: disable verified-noisy heuristics (documented in docs). - .gitleaks.toml: allowlist git-ignored false-positive paths. - docs/18-code-review.md: full findings + verification. Verified: ruff clean, pyright 0 errors, 64/64 gateway tests, all Kotlin tests, Android lint 0 issues, Android installed+launched on device, desktop launches on JDK 21.
2026-08-21 18:47:03 +02:00
1 parent 9f3f9842c8
commit 678c0344c8
@@ -0,0 +1,257 @@
+# 18 — Code Review & Lint/LSP Cleanup (alpha → stable)
+
+Comprehensive review of all three components — **gateway plugin**, **Android
+app**, and **Desktop app** — performed to take the project from alpha to a
+clean, stable baseline. Each section records what was found, what was fixed,
+how it was verified, and what was deliberately left (with rationale).
+
+> Companion file: [`DECISIONS.md`](../DECISIONS.md) at the repo root records the
+> judgment calls made during this pass (rule thresholds, suppressed findings,
+> config additions). This doc is the *findings*; that file is the *decisions*.
+
+Verification tooling used throughout:
+
+- **Ruff** (via the `hermes-agent/.venv` interpreter) — see
+  [`gateway-plugin/ruff.toml`](../gateway-plugin/ruff.toml) for the rule set.
+- **pi-lens** (`lens_diagnostics mode=full`) — LSP + tree-sitter + ast-grep +
+  opengrep + jscpd + gitleaks.
+- **Python test suite** — `hermes-agent/scripts/run_tests.sh
+  tests/gateway/test_android.py` (64 tests).
+- **Kotlin** — `./gradlew :shared:testDebugUnitTest` / `:shared:desktopTest`
+  and `./gradlew lint` (Android/Desktop).
+
+---
+
+## 18.1 Gateway plugin (`gateway-plugin/`)
+
+### 18.1.1 Findings (before)
+
+A fresh `lens_diagnostics mode=full` over `gateway-plugin/` reported **30
+blocking errors** and ~47 warnings. Ruff (broad rule set) reported **450**
+findings. Categories:
+
+| Category | Count | Severity | Resolution |
+| --- | --- | --- | --- |
+| Empty `except: pass` blocks | 14 | blocking | Rewritten as `contextlib.suppress(...)` with a rationale comment (or a `logger.debug` where the swallow is worth tracing). |
+| Unreachable `except` clause | 2 | blocking | False positive from an over-broad tree-sitter rule, but the two-clause `try` was restructured into a single `except (A, B) as e:` + `isinstance` so the code is unambiguous *and* the rule no longer fires. |
+| SQL-injection sink (parameterized) | 4 | blocking | False positive — every value is bound via `?` placeholders. Suppressed inline (`pi-lens-ignore: python-sql-injection`) with a justification; the opengrep SQLAlchemy variant (misfiring on raw `sqlite3`) disabled project-wide. |
+| Hardcoded secret (`token_field`) | 3 | blocking | False positive — `token_field` is a DB *column name* string, not a credential. Suppressed inline. |
+| Path traversal (`open(path)`) | 1 | blocking | False positive — `path` is produced by hermes `cache_*_from_bytes` (hermes's own media dir), never raw user input. Suppressed inline. |
+| Unresolved hermes imports | many | blocking (LSP) | Not a code bug — the plugin imports hermes-runtime modules (`websockets`, `gateway.platforms.base`, `hermes_state_search`, …) that live in the read-only `hermes-agent/` tree + its venv. Fixed by adding [`pyrightconfig.json`](../pyrightconfig.json) pointing the Python LSP at that venv + source root. |
+| `int()`/`float()`/`open()` "unchecked" | 38+ | warning | Noisy heuristic on validated internal data. Disabled project-wide in [`.pi-lens.json`](../.pi-lens.json) (see DECISIONS). |
+| Logger "credential leak" | 3 | warning | False positive — the word *token* in a log message; the logged values (peer addr, device id, HTTP status) are not secrets. Disabled project-wide. |
+| Ruff: line length / type annotations / imports / magic values / complexity | 450 | lint | All fixed (see 18.1.3). |
+| gitleaks (git-ignored paths) | several | warning | Allowlisted in [`.gitleaks.toml`](../.gitleaks.toml) — the hits were the read-only `hermes-agent/` tree, `build/` artifacts, and the standard (git-ignored) `google-services.json`. |
+
+### 18.1.2 Real bugs fixed
+
+- **`adapter.py` `interactive_setup` broken imports** (regression, silently masked): the
+  setup flow imported `print_info`/`print_success`/`print_warning`/`prompt` from
+  `hermes_cli.config`, but those live in `hermes_cli.cli_output`; it also imported
+  a `print_code` that does not exist in hermes at all. The whole import block
+  raised `ImportError`, which the surrounding `try/except` swallowed, so
+  `hermes gateway setup` for the android platform **always bailed out early** with
+  "setup helpers unavailable" and never generated a token or prompted for
+  host/port. Fixed by importing the print helpers from `hermes_cli.cli_output`,
+  the env helpers from `hermes_cli.config`, and dropping the non-existent
+  `print_code` (the pairing URL is printed directly — the app has no QR scanner).
+  This only surfaced once the Python LSP could resolve hermes imports (see
+  `pyrightconfig.json`); before that the unresolved imports masked the bad
+  symbols.
+- **`adapter.py` `release_scoped_lock` type error**: `self._lock_key` is
+  `str | None` but `release_scoped_lock(scope, identity)` requires `str`. The
+  `if getattr(self, "_lock_key", None):` guard did not narrow the type for the
+  type checker. Fixed by binding to a local `lock_key` and guarding on that.
+- **`ws_server.py` hello-auth `try`**: the original
+  `except asyncio.TimeoutError: … / except ConnectionClosed: return` was
+  restructured to a single `except (asyncio.TimeoutError, ConnectionClosed) as
+  e:` with an `isinstance` branch. Behavior is identical (timeout → warn +
+  close; clean disconnect → silent return) but the control flow is now
+  unambiguous.
+- **`ws_server.py` frame-loop `try`**: `except ConnectionClosed: pass /
+  except Exception: warn` became a single `except Exception as e:` that only
+  warns when the error is *not* a clean `ConnectionClosed`. A normal
+  disconnect no longer risks being logged as an error.
+- **`media.py` `get_upload`**: a refactor of the sibling `create_upload` loop
+  (to drop an unused loop variable) initially removed a `sess` binding that
+  `get_upload` still returned. Caught by ruff (`F821` undefined name) and
+  reverted for that loop only.
+
+### 18.1.3 Ruff cleanup
+
+Added [`gateway-plugin/ruff.toml`](../gateway-plugin/ruff.toml) with a broad
+rule set (`E W F I UP B SIM PL RET C4`) and `line-length = 100`. Changes:
+
+- **Type annotations**: `typing.Dict/List/Tuple` → builtins; `Optional[X]` →
+  `X | None` (pyupgrade `UP006`/`UP035`/`UP045`).
+- **Imports**: sorted (isort `I001`); hermes-runtime imports intentionally
+  deferred into function bodies are exempted via `ignore = ["PLC0415"]`
+  (documented in the config).
+- **Line length**: 115 lines wrapped to ≤ 100 chars (mostly `protocol.error(…)`
+  call sites and log statements).
+- **Magic values** (`PLR2004`): replaced with named constants —
+  `MAX_MEDIA_REF_LEN`, `_PRUNE_NOTIFY_INTERVAL_S`, `MAX_DEVICE_ID_LEN`,
+  `_HTTP_OK`, `_HTTP_ERROR_MIN`, `_MAX_EXT_LEN`.
+- **Bugbear** (`B904`): `raise MediaError(…)` inside `except ValueError as e`
+  now uses `raise … from e`.
+- **Simplify** (`SIM115`): file read in `ws_probe.py` now uses a context
+  manager.
+- **Complexity** (`PLR0911/0912/0913/0915`): thresholds set just above the
+  current maxima (the adapter is a single large dispatch surface); the lone
+  11-arg frame builder (`protocol.message`) is `noqa`'d with a comment.
+
+### 18.1.4 Verification
+
+- `ruff check gateway-plugin` → **All checks passed**.
+- `pyright gateway-plugin` (with `pyrightconfig.json`) → **0 errors, 0 warnings**.
+- `scripts/run_tests.sh tests/gateway/test_android.py` → **64/64 passed**.
+- `python -m compileall gateway-plugin` → clean.
+- Package-context import of every module (`protocol`, `pairing`, `outbox`,
+  `channels`, `search`, `media`, `push`, `ws_server`, `adapter`) → all OK.
+- `lens_diagnostics mode=full` → **0 blocking errors**; 20 warnings remain
+  (all `jscpd` code-duplication + 1 `python-thread-global-write`), documented
+  as accepted in 18.1.5.
+
+### 18.1.5 Accepted warnings (not fixed, with rationale)
+
+- **`jscpd` duplicates** (18): the SQLite `__init__` boilerplate is repeated
+  across `channels.py`/`outbox.py`/`pairing.py`; the channel-frame handlers in
+  `adapter.py` share a validate→error→respond shape; the FCM/ntfy `send`
+  methods in `push.py` are structurally similar. These are *intentional* —
+  each handler/method is clearer standalone, and the duplication is small.
+  Extracting a base would add indirection for little gain at this scale.
+- **`python-thread-global-write`** (`adapter.py`): the adapter spawns its
+  asyncio loop on a dedicated thread; shared state is guarded by
+  `asyncio.Lock`/`threading.Lock` as appropriate. The heuristic cannot see the
+  locking, so this is a false positive.
+
+---
+
+## 18.2 Android app (`app/androidApp` + `app/shared`)
+
+The Android and Desktop apps share the `:shared` KMP module (`commonMain` +
+`jvmMain`), so most Kotlin code is covered here and in 18.3.
+
+### 18.2.1 Findings (before)
+
+- **AndroidX lint** (`:androidApp:lintDebug`): 5 warnings — `ObsoleteSdkInt`,
+  `MonochromeLauncherIcon` (×2), `GradleDependency`, `OldTargetApi`.
+- **pi-lens** (`lens_diagnostics mode=full`): 3 blocking + 66 warnings.
+  - `detect-insecure-websocket` (blocking ×3): the app's default/placeholder
+    gateway URL is cleartext `ws://`.
+  - `gcm-detection` (×4): AES-GCM usage in `DesktopSecureStore`.
+  - `exported_activity` (×1): the launcher `MainActivity`.
+  - `jscpd` duplicates (many): platform impls, Compose boilerplate, icon XML.
+
+### 18.2.2 Fixes
+
+- **Launcher icons consolidated**: `minSdk` is 29 (≥ 26), so the
+  `mipmap-anydpi-v26` / `mipmap-anydpi-v33` variants were merged into a single
+  `mipmap-anydpi` carrying the `<monochrome>` layer (ignored on API < 33, so one
+  file serves all). This cleared both `ObsoleteSdkInt` and `MonochromeLauncherIcon`.
+- **`core-splashscreen`** bumped 1.0.1 → 1.2.0 (cleared `GradleDependency`).
+- **`OldTargetApi`**: `targetSdk` is deliberately pinned to 34 (a stable API;
+  the only newer installed platform, 37, is a preview SDK and inappropriate to
+  target for a stable build; the reference device is API 29). Suppressed in the
+  `lint { }` block with a comment.
+- **Insecure-websocket** (cleartext `ws://`): correct for the default LAN
+  gateway (TLS is optional — a TLS gateway is reached by entering a `wss://`
+  URL). Suppressed inline with a justification; a KDoc/comment that itself
+  contained a literal `ws://` was reworded so it no longer trips the rule.
+- **GCM** (`DesktopSecureStore`): verified correct — a fresh 12-byte
+  `SecureRandom` IV is generated per write and stored with the ciphertext (never
+  reused for a key). Suppressed inline with a justification.
+- **Exported activity**: the `MainActivity` is the launcher (LAUNCHER
+  intent-filter) plus deep-link handler, so it *must* be exported. XML doesn't
+  support the `//`/`#` inline-ignore syntax, so the `exported_activity` rule is
+  disabled project-wide in `.pi-lens.json` (the app has exactly one exported
+  activity, the required launcher).
+
+### 18.2.3 Verification
+
+- `:shared:allTests` → **BUILD SUCCESSFUL** (all Kotlin tests pass).
+- `:androidApp:lintDebug` → **0 issues**.
+- `:androidApp:assembleDebug` → **BUILD SUCCESSFUL**.
+- Installed on device `a5ca2a4b` (`:androidApp:installDebug`), launched
+  `dev.iris.app/.MainActivity`, screenshot confirms the app connects to the
+  gateway (green status) and renders chat + reasoning blocks.
+- `lens_diagnostics mode=full` → **0 blocking**; remaining warnings are all
+  `jscpd` code-duplication (intentional — see 18.2.4).
+
+### 18.2.4 Accepted warnings
+
+- **`jscpd` duplicates**: the `AndroidMedia`/`DesktopMedia` platform
+  implementations are structurally similar (each is the correct, idiomatic
+  implementation for its platform); the Compose screens share boilerplate
+  (remembered state, coroutine scopes, list-item layouts); the launcher icon
+  XML files are near-identical by design. Extracting shared code would add
+  indirection across source sets for little gain.
+
+## 18.3 Desktop app (`app/desktopApp`)
+
+The desktop app is a thin JVM shell (`Main.kt`) over the shared `:shared`
+module's `desktopMain` source set. It is a **special case**: the user verifies
+it runs themselves. A live launch **did** surface a real startup crash (below),
+which this pass fixed and re-verified.
+
+### 18.3.1 Findings (before)
+
+- **Startup crash (real bug)**: launching the desktop app threw
+  `java.lang.UnsupportedClassVersionError` — the Markdown rendering stack was
+  compiled for **Java 21** (class file 65.0) but the app runs on **Java 17**
+  (class file 61.0). Two artifacts were affected:
+  - `com.mikepenz:multiplatform-markdown-renderer:0.44.0` (JVM bytecode = Java 21), and
+  - its transitive `dev.snipme:highlights:1.1.0` (also Java 21).
+  The build and unit tests did **not** catch this: compilation reads the
+  metadata fine, and the tests never exercise the Compose Markdown render path
+  that loads those classes. It only failed at runtime on first render.
+- **pi-lens** (`lens_diagnostics mode=full`): the other desktop-specific
+  findings were the same categories as Android — `gcm-detection` in
+  `DesktopSecureStore.kt` (×4, fixed in 18.2.2) and `jscpd` duplicates in
+  `DesktopMedia.kt` / `Main.kt` (intentional, see 18.2.4).
+
+### 18.3.2 Resolution
+
+The crash was resolved by **moving the desktop to a Java 21 runtime** (the user
+installed JDK 21) rather than downgrading the library — so the app keeps the
+newest Markdown stack:
+
+- **`gradle.properties`**: added `org.gradle.java.home` → JDK 21, so the whole
+  build (and the desktop `run` / `jpackage` tasks) use a Java 21 runtime. AGP is
+  JDK-21-compatible, so the **Android build is unaffected** — its bytecode target
+  stays JVM 17 (`minSdk` 29 → Android 10 support is unchanged; that is governed
+  by `minSdk`, not the build JDK).
+- **`shared/build.gradle.kts`**: the `jvm("desktop")` target now sets
+  `jvmTarget = JVM_21` (the Android target keeps `JVM_17`).
+- **Markdown restored to `0.44.0`** (from the interim `0.38.1`): its Java-21
+  bytecode (and its `highlights:1.1.0` dependency) now load on the Java 21
+  desktop runtime. The app's Markdown API usage is unchanged.
+- The GCM ignores in `DesktopSecureStore.kt` (18.2.2) apply to the desktop
+  target.
+
+> **Note on the interim fix**: the first response to the crash was to downgrade
+> Markdown to `0.38.1` (the newest version whose bytecode *and* `highlights`
+> dep are Java 17). Once JDK 21 was available, that was superseded by the
+> runtime upgrade above, which is preferable (keeps the newest library).
+
+### 18.3.3 Verification
+
+- Build now runs on **JDK 21** (`org.gradle.java.home`).
+- `:desktopApp:build` + `:shared:allTests` + `:androidApp:lintDebug` +
+  `:androidApp:assembleDebug` → all **BUILD SUCCESSFUL** (Android still targets
+  JVM 17 / `minSdk` 29).
+- **Live launch** (`./gradlew :desktopApp:run`) → starts cleanly on JDK 21,
+  **no `UnsupportedClassVersionError`**, Markdown (0.44.0) renders.
+- `lens_diagnostics mode=full` → **0 blocking** for desktop files; remaining
+  warnings are `jscpd` code-duplication (intentional).
+- Packaging config (`jpackage` app-image / `.deb`) reviewed — the KCEF AWT
+  `--add-opens` flags are correctly applied to both the `run` task and the
+  jpackage `--java-options`; `jpackage` now bundles a JDK 21 JRE.
+
+> **Revisit**: the desktop now requires a **Java 21** runtime (the `run` task
+> and the jpackage-bundled JRE). If you ever need the desktop to run on Java 17
+> again, revert `org.gradle.java.home` + the desktop `jvmTarget` to 17 and pin
+> Markdown to `0.38.1`. Android 10 compatibility is independent of all of this
+> (it is set by `minSdk = 29`). The known non-fatal `pure virtual method called`
+> jpackage message on Linux (JDK-8348560) is expected and does not affect the
+> app.