fix(kotlin-sdk): address Android JNI review issues

[QuantumExplorer]

Summary

• split Android cargo feature flags into distinct argv entries so cargo-ndk passes shielded builds correctly
• wrap daemon-attached JNI callbacks in local frames, including per-entry frames for large persistence/event batches
• expand Kotlin SDK workflow path filters to include Rust workspace files and dependent crates

Verification

• bash -n packages/kotlin-sdk/build_android.sh
• confirmed feature argv expands as separate --features / shielded arguments
• cargo check -p rs-unified-sdk-jni --no-default-features
• cargo check -p rs-unified-sdk-jni --features shielded --no-default-features
• git diff --check

Gradle/Kotlin verification was not run locally because this machine has no Java runtime on PATH.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ddf1ad28-2bbd-43db-b2cd-e0b3acf39896

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-pr3999-review-issues

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Review complete (commit c373413)

[thepastaclaw]

Code Review

PR is a well-scoped defensive fix that wraps JNI callbacks in with_local_frame to prevent local-ref table overflow on Android's daemon-attached signer/persistence threads, plus a real bash argv-splitting fix in build_android.sh and expanded CI trigger paths. Three reviewers converged on the same issue: sign_async_trampoline uses manual push_local_frame/pop_local_frame instead of the RAII closure form used everywhere else in the PR — currently correct but not unwind-safe under the outer catch_unwind. Two minor nits on redundant per-address allocations and workflow trigger paths.

Source: reviewers opus for general, gpt-5.5 for general, opus for rust-quality, gpt-5.5 for rust-quality, opus for ffi-engineer, gpt-5.5 for ffi-engineer; verifier opus.

🟡 1 suggestion(s) | 💬 2 nitpick(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-unified-sdk-jni/src/signer.rs`:
- [SUGGESTION] packages/rs-unified-sdk-jni/src/signer.rs:84-116: Use `with_local_frame` in `sign_async_trampoline` for unwind-safe frame cleanup
  Every other trampoline in this PR (and `can_sign_trampoline` right below in the same file at line 145) uses `env.with_local_frame(cap, |env| { ... })`, which guarantees the frame is popped on every exit path — including a Rust unwind, since `with_local_frame` pops before resuming. `sign_async_trampoline` alone hand-brackets `push_local_frame(16)` with three separate `pop_local_frame` calls (lines 93, 111, 115). The outer `catch_unwind` at line 69 will swallow any panic between the push and the final pop, and the pushed JNI local frame stays on the daemon-attached Tokio worker for the lifetime of the thread. Because that thread is reused for every state-transition signature, repeated failures accumulate leaked frames on a live worker.

  The reason this isn't already using `with_local_frame` is that `Box::into_raw(pending)` must survive across the JNI dispatch so it can be reclaimed on failure. That's expressible in the closure form by returning `Result<(), Box<PendingSign>>` from the closure — the raw pointer itself is unaffected by the frame pop. See Codex's suggested rewrite below.

[thepastaclaw]

🟡 Suggestion: Use with_local_frame in sign_async_trampoline for unwind-safe frame cleanup

Every other trampoline in this PR (and can_sign_trampoline right below in the same file at line 145) uses env.with_local_frame(cap, |env| { ... }), which guarantees the frame is popped on every exit path — including a Rust unwind, since with_local_frame pops before resuming. sign_async_trampoline alone hand-brackets push_local_frame(16) with three separate pop_local_frame calls (lines 93, 111, 115). The outer catch_unwind at line 69 will swallow any panic between the push and the final pop, and the pushed JNI local frame stays on the daemon-attached Tokio worker for the lifetime of the thread. Because that thread is reused for every state-transition signature, repeated failures accumulate leaked frames on a live worker.

The reason this isn't already using with_local_frame is that Box::into_raw(pending) must survive across the JNI dispatch so it can be reclaimed on failure. That's expressible in the closure form by returning Result<(), Box<PendingSign>> from the closure — the raw pointer itself is unaffected by the frame pop. See Codex's suggested rewrite below.

Suggested change

	let token = Box::into_raw(pending) as jlong;
	let dispatched = env.with_local_frame(16, |env| -> Result<(), jni::errors::Error> {
	let jpubkey = env.byte_array_from_slice(pubkey)?;
	let jdata = env.byte_array_from_slice(payload)?;
	env.call_method(
	ctx.bridge.as_obj(),
	"signAsync",
	"([BI[BJ)V",
	&[
	(&jpubkey).into(),
	(key_type as jint).into(),
	(&jdata).into(),
	token.into(),
	],
	)?;
	Ok(())
	});
	if dispatched.is_err() {
	let _ = env.exception_clear();
	// Reclaim the token we just leaked and fail the request.
	return Err(Box::from_raw(token as *mut PendingSign));
	}

_{source: ['claude', 'codex']}

[thepastaclaw]

💬 Nitpick: user/friend pool identity arrays reallocated per address

Before the inner per-address with_local_frame(32, ...) was added, spec.user_identity_id and spec.friend_identity_id were pool-scoped and could be built once per pool and reused across every address in the pool. With the inner frame at line 528, both 32-byte arrays are rebuilt for every address entry — an extra pair of byte_array_from_slice upcalls and JVM allocations per address. The correctness win (bounded local-ref usage) is worth it, but if pools with many addresses are common the shared identity arrays could be hoisted to a pool-level frame while the per-address frame handles only the entry-specific refs. Not blocking; the current shape is simpler.

_{source: ['claude']}

[thepastaclaw]

💬 Nitpick: packages/rs-*/** already subsumes the three specific rs-* paths

The umbrella glob at line 11 (packages/rs-*/**) already matches packages/rs-unified-sdk-jni/**, packages/rs-sdk-ffi/**, and packages/rs-platform-wallet-ffi/**. GitHub Actions unions the patterns so this is harmless, but the redundancy suggests the three specific entries are load-bearing when they aren't. Drop lines 12–14.

Suggested change

	- 'packages/rs-*/**'
	- 'packages/rs-unified-sdk-jni/**'
	- 'packages/rs-sdk-ffi/**'
	- 'packages/rs-platform-wallet-ffi/**'
	- 'packages/rs-*/**'

_{source: ['claude']}