ARIA
a61b47a947
The plugin is named 'iris' (IrisAdapter, IRIS_HOME_CHANNEL, label Iris), but several docs still referred to it as the android platform/plugin and to the product as 'the Android app'. Rename name-mentions to iris/IRIS and product-mentions to 'Iris app'; keep legitimate OS references (androidApp, Android SDK, Android 10, androidx, test_android.py, ...). Also includes pi-lens markdown-lint autofixes (table spacing, trailing newlines) in the touched files.
258 lines
15 KiB
Markdown
258 lines
15 KiB
Markdown
# 18 — Code Review & Lint/LSP Cleanup (alpha → stable)
|
||
|
||
Comprehensive review of all three components — **gateway plugin**, **Iris app
|
||
(Android)**, 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 iris 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 Iris app — Android (`app/androidApp` + `app/shared`)
|
||
|
||
The Iris 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.
|