iris_x_hermes/docs/18-code-review.md

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 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 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 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 (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 — 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 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.