iris_x_hermes/docs/18-code-review.md

# 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.