Frostwire Code Reviewer

FrostWire Code Reviewer

Systematic review framework for FrostWire code changes. Covers correctness, performance, safety, security, code style, documentation, testing, and cross-platform compatibility. Complements the frostwire-engineer skill — that skill defines the rules, this skill enforces them.

Review current source and real call paths, not a historical bug list. A review request is read-only unless fixes are authorized. Historical examples below retain their diagnostic context, not a claim that each defect is still present.

When to Use This Skill

• Before commit: self-review your own diff
• During PR review: review another agent's or contributor's changes
• Security audit: deep-dive on code that handles untrusted input
• Pre-release gate: final review before tagging a release
• Post-fix verification: confirm a bug fix won't regress

Step 0: Determine Project Context

Before reviewing, determine which module(s) the change touches. Each module has different constraints:

Verify these values against current Gradle source/target, minSdk, desugaring, dependency declarations/resolution, and R8 configuration. Language support, runtime API availability, and repository policy are separate checks; version examples are not permanent pins.

Module: common/

Compiled by BOTH desktop and Android. This is the most restrictive target.

When reviewing common/ changes, ALWAYS check:

• Does the code import anything from java.awt, javax.swing, java.net.http, java.sql, java.nio.file?
• Does the code use System.getProperty("user.home") or other desktop-only system properties?
• Does the code compile against Java 17 (not Java 19)?

Module: desktop/

Module: android/

Step 1: Understand the Change

1. Read the commit message / PR description — what is the change trying to do?
2. Identify the threat model — does the changed code handle untrusted input? (remote peer data, user-supplied strings, network responses, file contents, deserialized objects)
3. Identify the blast radius — what modules are affected? common/ affects both desktop and Android.
4. Check the diff size — large diffs (>500 lines) need sectioned review. Ask the author to split if needed.
5. Trace production wiring — establish revision, requested scope, entry points, supported roles, callers, serializers, persistence, teardown, and tests, not only changed methods.
6. Check shared-brain ownership — search frostwire as existing agent gubatron for related findings, contracts, and file claims. Apply the operating rules below before edits or new findings; distinguish resolved, latent, conditional, and current defects.

Step 2: Review by Category

Walk each category. For each finding, assign a severity:

Give each finding path:line, trigger/preconditions, violated invariant, user impact, minimal remedy, and defensive validation. Severity follows impact and reachability, not dramatic wording. Label evidence as source-confirmed, locally observed, measured, estimated, or conditional; operation counts are not measured latency. A source-confirmed defect need not be exercised against a live service.

1. Cross-Platform Compatibility

Most important for common/ changes. If code is in common/, it must compile and run on Android.

Checklist

• No desktop-only or policy-forbidden imports — grep the diff for import java.awt, import javax.swing, import java.net.http, import java.sql, import java.nio.file. If found in common/, BLOCK unless an explicit repository policy exception is approved; Files/Path availability on API 26 is not the reason for the policy.
• No System.getProperty("user.home") in common/ — it is not Android app storage. Use injected File paths; resolve Context.getFilesDir() in Android-specific code.
• No JDBC in common/ — java.sql.Connection, DriverManager, PreparedStatement, ResultSet must not couple shared code to desktop JDBC implementations. Android uses android.database.sqlite.SQLiteDatabase. Abstract behind an interface (like LocalIndex).
• Java 17 compatible — sealed classes are a Java 17 language feature, not automatically forbidden. Record patterns and pattern-matching switch are not standard Java 17 features. Separately verify Android runtime/desugaring support, including java.util.random.RandomGenerator; use established Random/SecureRandom APIs where needed.
• No ProcessBuilder in common/ — Android can't spawn JVM subprocesses. If a feature needs subprocess launch, put the launcher in desktop/ only.
• OkHttp over java.net.http — when writing HTTP client code in common/, use OkHttp (available on both platforms), not java.net.http.HttpClient (Java 11+ desktop only).
• Android minSdk = 26 — all Build.VERSION.SDK_INT < 26 guards are dead code. Don't add them.
• Test on both targets — coordinator runs ./gradlew compileJava from desktop/ AND ./gradlew compilePlus1DebugJavaWithJavac from android/; compilation alone is not runtime verification.

2. Correctness

Checklist

• Happy path works — does the code produce the correct result for normal input?
• Null safety — are all parameters, return values, and cross-thread boundaries null-checked? Especially: deserialized objects, Intent extras, JSON fields, network responses, cursor columns.
• Empty/edge cases — empty lists, empty strings, zero, negative numbers, max values, empty arrays, single-element collections, empty Optional.
• Boundary conditions — off-by-one errors, <= vs <, inclusive vs exclusive ranges, Integer.MAX_VALUE wrap, Math.abs(Long.MIN_VALUE) returns negative.
• Integer overflow — long cutoff = now - threshold may overflow for arbitrary long domains; threshold > now alone means a negative result, not necessarily overflow. Validate domains and use checked arithmetic/bounded comparisons. Math.multiplyExact for multiplications that could overflow. Integer.compareUnsigned for sequence numbers that wrap, with explicit wrap/window semantics.
• Exception handling — does the code catch the right exceptions? Does it fail closed (safe default) not crashed? Are resources cleaned up in finally or try-with-resources?
• Concurrency — if the code crosses thread boundaries, is shared state protected? Are singletons thread-safe? Are volatile/atomic variables used correctly? Is the synchronized lock object correct?
• Resource leaks — are all Cursor, Connection, InputStream, OutputStream, Reader, Writer, PreparedStatement, ResultSet closed via try-with-resources? Are native handles (jlibtorrent SWIG objects) released deterministically?
• Transaction atomicity — if multiple DB operations must be atomic, are they wrapped in a transaction? (db.beginTransaction() / setTransactionSuccessful() / endTransaction())
• Defensive copies — are byte arrays cloned on POJO boundaries? (return bytes.clone() on getters, this.x = x.clone() in constructors). Are mutable collections exposed via defensive copies?
• UTF-8 handling — are string lengths measured in bytes (for network/DB) or chars (for display)? String.getBytes(UTF_8).length vs String.length().

FrostWire-specific correctness checks

• BTEngine off UI thread — JNI calls to jlibtorrent (BTEngine, SessionManager, Ed25519, TorrentInfo) must NEVER happen on the EDT (desktop) or main thread (Android). Use a worker such as ThreadExecutor or SystemUtils.postToHandler(MISC, ...). GUIMediator.safeInvokeLater() schedules ON the EDT and is only for the UI result, not the expensive work.
• Android lifecycle — are Activity/Fragment/Service callbacks guarded against null/destroyed state? Is getContext() null-checked?
• SharedPreferences thread safety — OnPreferenceChangeListener callbacks run on the main thread. Any BTEngine/DB call inside must be dispatched to a background thread.
• Search input sanitization — LocalIndex.search() is called with keywords from remote peers (via RelaySearchService). All search input must be treated as untrusted. FTS5 queries must be sanitized. LIKE queries must escape wildcards.

3. Security

Code that handles untrusted input must be hardened against malicious actors.

Threat Model Questions

1. Where does input come from? — remote peer (signed request), network response (HTTP), user input (search bar), file on disk (torrent, config), DHT (untrusted)
2. What can a malicious actor control? — keywords in search requests, JSON payloads, HTTP response bodies, file paths, info hashes, public keys, DHT items
3. What is the impact of misuse? — data leak (entire local index via % wildcard), crash (malformed JSON), resource exhaustion (large payload), injection (SQL, FTS5, command)

Checklist

• SQL injection — all queries use parameterized ? placeholders, never string concatenation with user input.
• FTS5 injection — FTS5 MATCH expressions are sanitized. Reserved words (OR, AND, NOT, NEAR) are quoted. Historical stripping of non-alphanumeric chars needs Unicode-safe token handling and fixtures, not an assumption that punctuation removal alone is safe or preserves search semantics.
• LIKE wildcard injection — % and _ in user input are escaped with ESCAPE ''. Without this, a remote peer can send % to match the entire local index in one query.
• Path traversal — file paths from untrusted sources are validated. No ../../etc/passwd. Use File.getCanonicalPath() for root/candidate and verify containment by path component, not naive string prefix (/allowed-other is not inside /allowed); account for symlink races where relevant.
• Deserialization — JSON parsing is wrapped in try-catch. Malformed JSON returns empty/null, not crash.
• Signature verification — remote requests are Ed25519-signed. Verify BEFORE processing. Timestamp skew is freshness, not replay prevention; require bounded authenticated requester/nonce replay state as well. Rate limiting is per authenticated source, with cheap ingress budgets before crypto.
• Auth token — control API endpoints (except /health) require X-IceBridge-Token header. The token is generated server-side, not client-supplied.
• Input length caps — cap all untrusted inputs before materialization, parsing, or scheduling. Historical examples: keywords ≤256 chars, JSON ≤16MB, file lists ≤10,000 entries; verify current per-protocol limits and byte/decoded-expansion/aggregate budgets. Reading a whole body then checking length is not a bound.
• No secrets in logs — private keys, auth tokens, passwords, BIP39 mnemonics are NEVER logged. Truncate only already-redacted nonsecret payloads to 200 chars; truncation does not redact secrets.
• No secrets in commits — git diff --cached reviewed before commit. No .pem, .key, .p12, .crt files containing secrets. No hardcoded API keys or passwords; public certificate fixtures require explicit provenance/review.
• Integer overflow as attack — Math.abs(Long.MIN_VALUE) and manual -Long.MIN_VALUE are both negative; a - b may already overflow. Validate timestamp domains and use checked subtraction with rejection on overflow or bounded comparisons, not a manual sign-flip workaround.
• Rate limiting — per-source rate limiting on all incoming peer requests. Sliding window; bound limiter-key cardinality and expiry as well as QPS.
• Error messages don't leak — rejection responses to remote peers must not reveal the rejection reason (helps attackers tune bypasses). Log details locally only.

4. Performance

Checklist

• No O(n²) on hot paths — search results, UI lists, peer directories. If iterating a collection inside another iteration, consider a Set/Map lookup instead.
• No DB calls on UI thread — all SQLiteDatabase operations are on background threads. synchronized(db) blocks must be short.
• No network on UI thread — all HTTP, rUDP, DHT operations are off the main thread.
• Batch DB operations — multiple INSERTs use beginTransaction()/endTransaction() not individual auto-commits.
• Cursor management — Cursors are closed via try-with-resources. Large result sets are paginated (LIMIT).
• Memory bounds — cap untrusted collections/bodies before materialization. Reserve inbound queue capacity before acceptance; never silently evict accepted work. Reject/backpressure unaccepted work and give accepted expiry/cancellation an explicit outcome.
• Thread pool sizing — daemon threads are marked setDaemon(true). Named threads for debugging.
• Lazy initialization — expensive resources are loaded on first use, not at startup.
• Static final for constants — regex patterns, Gson instances, Logger instances are private static final, not created per-call.
• Connection reuse — OkHttp OkHttpClient instances are reused (they have connection pools). Don't create a new client per request.

FrostWire-specific performance checks

• jlibtorrent Ed25519 over JDK — IdentityKeys.generate() uses com.frostwire.jlibtorrent.Ed25519.createKeypair(seed). Historical native speedup reports were 50-100x versus JDK KeyPairGenerator; remeasure the actual provider/runtime/workload rather than treating that ratio as universal.
• Polling intervals appropriate for mobile — historical guidance used a 300ms foreground transport poller and DHT advertising every 15-30 min on mobile (versus 5 min desktop). Verify current delivery/battery constraints; consider adaptive screen/background intervals rather than treating these values as fixed requirements.

5. Safety

Checklist

• Fail closed, not crashed — native code, deserialization, network calls are wrapped in appropriate exception handling with a safe default, avoiding crash-on-every-startup loops. Catching Java exceptions cannot contain native process crashes.
• Race conditions — coordinate close() with resource ownership and in-flight use through appropriate synchronization or atomic state. Serialize database close against database operations where required, but never make transport cancellation wait for the lock held by the blocking I/O it must interrupt. Test concurrent use/close and cancellation, not one mandatory locking pattern.
• Shutdown ordering — components are shut down in reverse order of startup. Listeners removed before transports closed. Transports closed before servers.
• Native init wrapped — try/catch around recoverable native initialization failures (Python, ffmpeg, jlibtorrent .so load). Corrupted binaries are a real-world occurrence; handle relevant linkage errors without claiming protection from a native crash.
• Synchronous cleanup before async — if state is cleaned up and then an async callback fires, the callback must see the cleaned state, not stale data.
• Timeout audit — every network/IO operation has a timeout. No Thread.sleep(Long.MAX_VALUE) without a shutdown path. No infinite Object.wait() without a notify. Verify dependency timeout units and use one monotonic absolute deadline covering queueing, serial sends, retries, and reads, not a timer started after them.

Android-specific safety checks

• Memory leaks — no static references to Activity/Fragment/View. Non-static inner classes holding implicit outer reference to Activity = leak. Use static inner classes with WeakReference or standalone classes.
• Listener/Receiver cleanup — BroadcastReceiver, ContentObserver, Cursor registered in onCreate/onResume must be unregistered in onDestroy/onPause.
• Background execution limits — Android 8+ restricts background execution. WorkManager handles eligible deferrable persistent jobs; permitted foreground services cover appropriate user-visible work. Doze restricts execution/network and processes may die; it does not specifically kill ScheduledExecutorService. Document lifecycle ownership and any scheduling migration.
• Battery impact — polling intervals should be adaptive. GPS, DHT, and network polling drain battery. Consider WorkManager with NetworkType.CONNECTED constraints.

6. Code Style (gubatron + aldenml)

The code must follow the FrostWire house style. See frostwire-engineer skill for the full spec.

Formatting

• Spotless check passes — coordinator runs ./gradlew spotlessCheck from desktop/ (enforces google-java-format). Fix with scoped/authorized ./gradlew spotlessApply; verify ratchetFrom against origin/master and do not format another owner's changes.
• No wildcard imports — Spotless removes unused imports but doesn't collapse wildcards. Ensure no import java.util.*.
• File header — use the applicable FrostWire license/header for original code; preserve upstream license notices for imported or derived code. Record actual authorship, not automatic maintainer attribution. Check compatibility rather than mechanically replacing every header with GPL v3.
• Logging — com.frostwire.util.Logger only. Never System.out, System.err, printStackTrace(), SLF4J, or java.util.logging in library code; intentional headless CLI UX is qualified below.
• No redundant comments — code should be self-explanatory. Method names describe what they do at the caller level; retain concise non-obvious invariant/API documentation required by the documentation checklist.
• No @SuppressWarnings("unused") — delete dead code, don't silence the compiler.
• No magic numbers — named constants for buffer sizes, port numbers, table names, DHT key prefixes.
• Reuse before building — search com.frostwire.util.* before writing any utility.
• Commit message format — [scope] imperative description (#issue). Scopes: [android], [desktop], [common], [all], [test], [docs], [build].
• Commit message model suffix — every commit message ends with the exact LLM model identifier used to author it, for example openai/gpt-5.6-luna.
• One change per commit — don't mix features, refactors, and formatting in the same commit.
• Changelog updated — desktop/changelog.txt and/or android/changelog.txt updated for user-facing changes. common/ changes update BOTH.
• UI/i18n parity — desktop user-facing strings use I18n.tr; Android locale keys, placeholders/plurals, and affected themes remain consistent.

Git History Review

• Commits are granular — one logical change per commit. A branch with 35+ granular commits is normal.
• No formatting noise mixed with product fixes — formatting changes go in their own commit.
• Branch is rebased, not merged — review the established linear-history convention; git fetch origin master then git rebase origin/master are examples only when explicitly authorized. Never perform history changes just because this review checklist mentions them.
• Force-push with lease — if a force-push is explicitly authorized, use git push --force-with-lease, never --force. A review does not authorize any push.

7. Documentation

Code must be documented well enough that a new contributor can understand it without reading the implementation.

Checklist

• Class javadoc — every public class has a javadoc explaining what it does, its role in the system, and key design decisions.
• Public method javadoc — every public method documents applicable @param, @return, @throws contracts. For non-obvious methods, include a brief explanation of the algorithm or approach.
• Thread safety — if a class is thread-safe, document how (e.g., "all public methods are synchronized on the internal db lock"). If not, document which thread must call it.
• Security notes — if a method handles untrusted input, document the sanitization performed. Example: "Sanitizes LIKE wildcards (% and _) to prevent wildcard injection from remote peer search requests."
• Design notes — non-obvious design decisions are documented inline. Why FTS5 with external content? Why bind to 0.0.0.0 on mobile?
• Working examples — where the API is non-trivial, include a code example in the javadoc:

  /**
   * Open a local index backed by SQLite + FTS5.
   *
   * <p>Example:
   * <pre>{@code
   * AndroidLocalIndex index = AndroidLocalIndex.open(context);
   * index.upsert(torrent);
   * List<LocalSharedTorrent> results = index.search("ubuntu", 10);
   * index.close();
   * }</pre>
   */
  

• Constants documented — non-obvious constant values have a comment explaining the choice.
• No outdated docs — if the code changed, the docs changed too. No stale @link references to moved classes.

8. Testing

All new code must be tested. Bug fixes must include regression tests.

Checklist

• New public methods have tests — at least one happy-path test per method.
• Edge cases tested — null input, empty input, max values, boundary conditions, concurrent access.
• Security tests — if the code handles untrusted input, write a defensive local test that proves rejection. Example: search_percentWildcard_doesNotMatchAll. Do not reproduce attacks against live peers/services.
• Bug fix includes regression test — the test must FAIL without the fix and PASS with it. The test name should describe the bug: searchLike_wildcardInjection_leaksEntireIndex.
• Mutation testing mindset — not just "does the test pass" but "if I delete this line, does the test fail?" A test that passes regardless of the implementation is worthless.
• Test isolation — each test sets up its own state (@Before/@BeforeEach) and cleans up (@After/@AfterEach). Tests don't depend on execution order. Each Robolectric test uses a unique DB name to avoid cross-test contamination; reset process-wide singletons and close resources.
• Real fixtures over synthetic — use actual torrent metadata, real search responses, real DHT items. Synthetic data misses the bugs that ship.
• Integration test coverage — unit tests cover individual methods, but does the wiring work? If you added a new component, write a test that exercises the full path (start server → send request → get response).
• Robolectric for Android — historical runner example: @RunWith(RobolectricTestRunner.class) with @Config(sdk = 34). Verify current configuration and limitations (FTS5/native jlibtorrent support is not guaranteed); document fallbacks and device/runtime verification gaps.
• Test naming — methodName_scenario_expectedResult (e.g., search_byTorrentName_returnsMatch, upsert_replacesExisting).
• Test compile check — ./gradlew compilePlus1DebugUnitTestJavaWithJavac (Android) or ./gradlew compileTestJava (desktop) passes.
• Test execution — at least the affected test class runs and passes. Don't claim "all tests pass" without running them.
• Behavior, not labels — inspect assertions and production wiring. A test name, mocked codec, regex/source-string assertion, or green suite cannot establish behavior it never executes. Prefer focused JUnit 5 behavior tests where established; use the existing Android runner where needed.

Regression test pattern for bug fixes

@Test
public void searchLike_percentWildcard_doesNotLeakEntireIndex() {
    // Local fixture for wildcard injection: "%" must not match all torrents.
    index.upsert(makeTorrent("a001", "Alpha", 100, 1));
    index.upsert(makeTorrent("b002", "Beta", 200, 1));

    List<LocalSharedTorrent> results = index.search("%", 100);
    assertEquals("Percent wildcard must not match all torrents", 0, results.size());
}

9. Dependency & Build Review

When adding or changing dependencies:

Checklist

• License compatibility — GPL v3 compatible? (Apache 2.0, MIT, BSD are OK. LGPL, EPL need care. GPL-incompatible = BLOCK.)
• APK size impact — check the current variant's assemble task before/after (./gradlew assembleDebug where available). Historical Netty addition was ~2MB and accepted for that feature; measure current APK/AAB and ABI impact rather than inheriting that approval.
• Transitive dependency count — run ./gradlew dependencies to see what comes along. Avoid dependencies that pull in 50+ transitive jars.
• CVE/advisory check — check the dependency version against known CVEs. Use OWASP Dependency Check or manual search.
• Actively maintained — last commit < 1 year ago? Issues being responded to?
• Same version on desktop and Android — prefer alignment for shared dependencies; where shipped versions differ (e.g., OkHttp desktop 4.12.0, Android 5.3.2), verify the shared API/runtime contract and both resolved graphs rather than pretending versions match.
• ProGuard/R8 keep rules — Gson-reflected classes, reflection-based code, and JNI signatures need appropriate keep rules in proguard-rules.pro; inspect multidex-config.txt separately for its main-dex role, not as a substitute for R8 rules. If adding Netty, check if R8 strips needed classes and test the minified artifact.

10. Wire Protocol & Schema Compatibility

When changing serialized data that crosses process/network boundaries:

Checklist

• Wire protocol version — RemoteSearchRequest, RemoteSearchResponse, SearchPayloadCodec: does the change break communication with older peers? Is the version field bumped? Explicitly define accepted/rejected versions; never preserve an insecure downgrade solely for connectivity.
• Canonical bytes — if canonicalBytes() changes, signatures from old peers will fail verification. Preserve field ordering and version/domain rules; appending fields is not automatically signature-compatible. Test old/new encodings and explicit rejection where needed.
• DB schema migration — if the SQLite schema changes: is SCHEMA_VERSION bumped? Does onUpgrade() handle the old→new path? Is the upgrade path tested? Does it preserve existing data?
• BIP39 mnemonic compatibility — if IdentityKeys serialization changes, old mnemonics must still restore correctly. Test with a known mnemonic.
• DHT item format — BEP 44/46 items: are they backward-compatible? Old clients receiving new-format items should ignore unknown fields where the version contract permits, otherwise reject cleanly, not crash or reinterpret signed bytes.

11. Build Verification

Code must compile and tests must pass on all affected targets. Only the coordinator runs Gradle in the shared tree; workers report required gates rather than starting competing builds. Verify current task names before execution.

Commands

Run each command from the named target directory.

Known test failure history (revalidate, not a blanket flaky dismissal)

• InternetArchiveSearchPatternTest — historically archive.org timeout
• MagnetDLSearchPatternTest — historically site moved, TLS cert changed
• TelluridePlaylistTests — historically YouTube source name changed

These records identify prior external-service failures, not proof that a fresh failure is unrelated. Capture current output, compare the baseline/environment, and determine whether changed code or a deterministic assertion is responsible before excluding a failure.

Verification evidence levels

• Source review establishes inspected call paths/invariants, not execution or passing tests.
• Isolated compilation/tests establish only the listed current sources and exercised behavior. Name cached classes, mocks, stubs, native libraries, and classpath dependencies; stale build outputs cannot prove current full-source integration.
• Full-source target gates compile and test the current affected modules, including shared callers. Report exact commands, revision, environment, failures/skips, and missing gates; a green subset is not the full module suite.
• Runtime/deployment evidence remains separate: local SQL is not Android CursorWindow/FTS proof; loopback/simulation is not WAN/EC2 capacity proof. Report delivered work, tail latency, and resource plateaus, not attempted sends or tests paced below a limiter.

Deploy & Artifact Verification

• "up-to-date" ≠ latest commit — gradle up-to-date only means the artifact matches that checkout's sources. Verify the deploy host's git rev-parse HEAD and the artifact's own version banner (e.g. IceBridge software version = 1.1.0 line); a missing banner block once proved a stale jar despite a "successful" build (MentisDB #896).
• Suite width matches change width — for common/ constants, shared helpers, or wire behavior, require the FULL module suite, not only the package suite. Deterministic failures shipped to CI because only the relay package ran (MentisDB #902).
• Constant/label changes audit — when a public constant value or user-visible label changes, grep every test asserting the old value before approving.

Review Output Format

When completing a review, produce a structured report with findings first, ordered by severity. Retain context, summary, verification, and recommendation, but do not bury defects beneath an overview. Each finding includes trigger, evidence, impact, minimal fix, defensive validation, and open/fixed/verified/blocked status. If none, say so and list residual risks.

## Code Review: [commit/PR description]

### Findings

#### BLOCK
- [file:line] Trigger/preconditions, violated invariant, evidence, impact, minimal fix, defensive validation, status.

#### HIGH
- [file:line] Description of the high-severity issue with the same evidence fields.

#### MEDIUM
- [file:line] Description of the medium issue.

#### LOW
- [file:line] Style nit or minor suggestion.

#### INFO
- Positive observations, accepted costs, or qualified follow-ups; no defect claim.

### Context
- Module(s): [desktop / android / common]
- Threat model: [trusted input / remote peer / user input / file]
- Blast radius: [desktop-only / android-only / both via common]
- Revision, scope, assumptions, and non-findings: [...]

### Summary
[1-2 sentence summary of what the change does and whether it's ready]

### Verification
- [ ] Compiles on [desktop/android/both]: exact command, revision, environment
- [ ] Tests pass on [desktop/android/both]: exact suites/counts, failures/skips, missing gates
- [ ] Spotless check passes (`./gradlew spotlessCheck`)
- [ ] Changelog updated
- [ ] No secrets in diff
- [ ] common/ code is Android-compatible and respects shared-code policy (including java.nio.file)
- Evidence level: [source / isolated tests with dependency caveats / full-source gates / device or deployment]
- Residual risks and unverified claims: [...]

### Recommendation
[APPROVE / REQUEST CHANGES / BLOCK] for [explicit scope]

Quick-Reference: Top 30 Most Common Findings

Based on the FrostWire codebase history + MentisDB frostwire chain lessons. Revalidate each example against current source; historical constants, versions, and fixes are not current-defect or topology claims:

1. common/ uses desktop-only API or violates shared-code policy — java.net.http, java.awt, desktop JDBC, or policy-forbidden java.nio.file. BLOCK. Historical review targets: IdentityKeys, IceBridgeTokens, IceBridgeHostCache (Files); Files exists on API 26, so distinguish policy from availability.
2. LIKE wildcard injection — remote peer sends %, matches entire local index. Escape with ESCAPE ''.
3. No transaction in multi-step DB writes — crash mid-write leaves DB inconsistent. Wrap in beginTransaction().
4. close() race condition — resource ownership and concurrent use/close are not coordinated. Database serialization and interruptible transport cancellation need different synchronization; do not require cancellation to wait behind blocking I/O.
5. Math.abs(Long.MIN_VALUE) is negative — timestamp skew bypass. Manual sign flip also overflows; validate domains and use checked arithmetic/bounded comparisons. Freshness alone does not prevent replay.
6. jlibtorrent on UI thread — StrictMode violation or EDT freeze. Always background.
7. Missing defensive byte[].clone() — shared mutable state across threads.
8. Resource leak — Cursor/Connection not in try-with-resources.
9. System.out instead of Logger — against house style (except intentional headless CLI UX in IceBridgeServer.main).
10. No regression test for bug fix — the bug will come back.
11. Secrets in logs — auth tokens, private keys, full JSON payloads logged on error. Use [set] / never print tokens after generate-token path is done.
12. Spotless violations — ./gradlew spotlessCheck fails. Fix with authorized/scoped ./gradlew spotlessApply.
13. ScheduledExecutorService on Android — in-process timers do not survive process death or bypass Doze restrictions. Use WorkManager for eligible deferrable persistent tasks, not as an automatic replacement for every live transport timer.
14. Memory leak via static Activity reference — Android-specific. Use WeakReference or static inner class.
15. Wire protocol break — canonical bytes changed without version/domain handling. Historical IdentityRecord v3 retained v1/v2 reads; preserve safe advertised compatibility, but explicitly reject insecure legacy wire paths rather than downgrading.
16. Self-discovery / co-located ports — PeerDiscovery must skip own Ed25519 pub + loopback; configurable ports for co-located standalone.
17. Async transport race — register response listeners before send; async fakes must deliver off-thread.
18. rUDP without app-level fragmentation — IP frags drop whole datagrams; use DATA_FRAG/DATA_END + byte-equal reassembly tests.
19. ProcessBuilder.inheritIO under Gradle — historical subprocess tests stalled on IO handling; redirect to temp files and diagnose actual inherited/redirected streams rather than assuming every inherited stream is a pipe. Tests use IdentityKeys.generate(0), never PoW 20.
20. App multi-hop re-sign vs requester verify — re-signing with forwarder key while verify checks requesterPub always fails. Dual-envelope (origin query signature separate from hop attribution) addresses this; authenticate maximum routing budget too. Flag mismatched re-sign paths as BLOCK.
21. HELLO_ACK without identity — historical empty ACK left initiator remotePub null, breaking mesh RELAY one way (#875). HELLO-shaped pub/ts/sig and setRemotePub fixed that wiring, but legacy shape/signature alone is not fresh endpoint key-possession proof; require the current bound handshake.
22. Dual connect() overwrite — second initiator session replaces authenticated responder session on sessionsByAddress. connect must reuse or explicitly reconcile simultaneous-open ownership (MentisDB #875).
23. Unauthenticated RELAY_RESPONSE — spoofable sourcePub into inbound queue / amp. Authenticated session/packet + rate limit required; CID/address lookup is not authentication. Local clients use /poll delivery (MentisDB #876).
24. Mesh flood without bounds — fan-out × hop TTL × payload without MAX_APP_PAYLOAD, rate limit, or TTL cap is amplification BLOCK. Verify current configured budgets, not historical fixed topology numbers.
25. **DHT list Co

…(truncated)