Skip to content

refactor: add RNS/LXMF binding interfaces and Chaquopy implementations#669

Closed
torlando-tech wants to merge 6 commits intomainfrom
feat/rns-lxmf-binding-interfaces
Closed

refactor: add RNS/LXMF binding interfaces and Chaquopy implementations#669
torlando-tech wants to merge 6 commits intomainfrom
feat/rns-lxmf-binding-interfaces

Conversation

@torlando-tech
Copy link
Copy Markdown
Owner

Summary

  • Phase 1: 15 binding interfaces in :reticulum module (bindings/rns/ + bindings/lxmf/) mirroring reticulum-kt's live-object API shape. Add DestinationType.LINK.
  • Phase 2: Expand rns_api.py with thin pass-through methods for all RNS/LXMF operations (Identity, Destination, Link, Transport, LXMF Router, Message).
  • Phase 3: 12 Chaquopy implementation classes in app/service/rns/ wrapping live Python objects via PyObject with proper close() lifecycle.

Zero runtime impact — not wired into DI yet. Callback bridging deferred to Phase 4.

Architecture

RnsIdentity (interface)          — live object with sign(), encrypt()
  ├── ChaquopyRnsIdentity        — wraps PyObject (now)
  └── reticulum-kt Identity      — native Kotlin (later)

Identity (data class)            — snapshot: hash + publicKey
  └── created from RnsIdentity.toSnapshot()

Interfaces are blocking (not suspend) to match reticulum-kt — callers dispatch to Dispatchers.IO.

Test plan

  • :reticulum:compileDebugKotlin passes
  • :app:compileNoSentryDebugKotlin passes
  • :reticulum:testDebugUnitTest passes (no regressions)
  • Python syntax check passes
  • CI build passes

🤖 Generated with Claude Code

Strangler Fig Phase 1-3: Create the binding interface layer that mirrors
reticulum-kt's API shape, enabling a mechanical swap from Chaquopy to
native Kotlin later.

Phase 1 - Binding interfaces (15 files in :reticulum bindings/):
  RNS layer: RnsReticulum, RnsIdentity, RnsIdentityProvider,
    RnsDestination, RnsDestinationProvider, RnsLink, RnsLinkProvider,
    RnsTransport, RnsAnnounceHandler, RnsInterfaceInfo
  LXMF layer: LxmfRouter, LxmfMessage, LxmfMessageState,
    LxmfMessageFactory, LxmfAppDataParser
  Add DestinationType.LINK to match reticulum-kt

Phase 2 - Python thin layer:
  Expand rns_api.py with binding methods for all RNS/LXMF operations
  (Identity, Destination, Link, Transport, LXMF Router, Message)

Phase 3 - Chaquopy implementations (12 files in app/service/rns/):
  ChaquopyRns* and ChaquopyLxmf* classes wrapping live Python objects
  via PyObject with proper close() lifecycle management

Not wired into DI yet — zero runtime impact. Callback bridging
(TODOs in Destination, Link, Router, Message) deferred to Phase 4.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 12, 2026

Greptile Summary

This PR implements Phase 1–3 of a strangler-fig migration, introducing 15 Kotlin binding interfaces in the :reticulum module and 12 Chaquopy wrapper classes in app/service/rns/ that delegate to an expanded rns_api.py. The Python file grows from ~26 to 365 lines of thin pass-throughs covering Identity, Destination, Link, Transport, LXMF Router, and Message operations. As stated in the description, nothing is wired into DI yet, so there is zero runtime impact.

Key observations:

  • All previously reviewed issues (LOG_DEBUG hardcoding, pyAspects leak, null-crash in LxmfMessageFactory, placeholder callback TypeError, registerRequestHandler null generator, unconditional callback clearing in ChaquopyRnsLink) have been resolved correctly.
  • parsePropagationDict in ChaquopyLxmfRouter correctly iterates the dict in a single pass with try/finally closing each K/V — a good pattern that should be applied to the fields getter in ChaquopyLxmfMessage as well (see inline comment).
  • rns_api.py link_teardown accepts a reason parameter but does not forward it to link.teardown(), silently dropping the caller's intent.
  • The create_destination docstring documents incorrect numeric constants for direction (claims IN=1, OUT=2; actual RNS values are 0x11=17, 0x12=18) and dest_type (off-by-one from real RNS constants).
  • The interface-only files (RnsIdentity, RnsDestination, RnsLink, etc.) are clean and well-documented.

Confidence Score: 4/5

  • Safe to merge — no runtime impact yet; remaining issues are minor correctness and documentation concerns for future phases.
  • All blocking issues from the prior review round have been addressed. The two new bugs (link_teardown dropping reason, incorrect docstring constants) are low-severity and isolated to the Python bridge. The ChaquopyLxmfMessage.fields double-iteration is a PyObject lifecycle quality issue but not a hard crash. The PR is not wired into DI so none of these can cause regressions today.
  • python/rns_api.py (docstring + reason drop) and app/.../ChaquopyLxmfMessage.kt (fields PyObject leak) need attention before Phase 4 integration.

Important Files Changed

Filename Overview
python/rns_api.py Core Python bridge expanded with 300+ lines of thin pass-throughs; two issues: link_teardown silently drops the reason argument, and the create_destination docstring documents incorrect numeric constants for both direction and dest_type.
app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyLxmfMessage.kt New Chaquopy wrapper for live LXMF messages; the fields getter's associate-then-close pattern iterates the dict twice, leaking the PyObjects created by the first iteration as the finally block closes a different set of wrappers.
app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyRnsDestination.kt New Chaquopy wrapper for RNS.Destination; correctly guards both setLinkEstablishedCallback (skips non-null callback until Phase 4) and registerRequestHandler (no-op until Phase 4), addressing prior review feedback.
app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyLxmfRouter.kt New Chaquopy wrapper for LXMF router; parsePropagationDict correctly iterates dict entries in a single pass with try/finally closing each K/V PyObject pair, addressing the pattern flagged in prior review.
app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyRnsTransport.kt New Chaquopy wrapper for RNS transport; getInterfaces() closes all dict K/V PyObjects but each item from asList() is not explicitly closed (pre-existing issue flagged in prior review). All other methods correctly close return values.
app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyLxmfMessageFactory.kt Factory implementation correctly throws UnsupportedOperationException for Phase 4, avoiding the prior issue of passing null Python Destination objects that would crash immediately.

Sequence Diagram

sequenceDiagram
    participant KCaller as Kotlin Caller (IO Dispatcher)
    participant KBinding as Binding Interface<br/>(RnsIdentity / RnsDestination / RnsLink)
    participant ChImpl as Chaquopy Impl<br/>(ChaquopyRns* / ChaquopyLxmf*)
    participant PyApi as rns_api.py<br/>(RnsApi instance)
    participant RNS as RNS / LXMF<br/>(Python library)

    KCaller->>KBinding: sign(message)
    KBinding->>ChImpl: sign(message)
    ChImpl->>PyApi: callAttr("identity_sign", pyIdentity, message)
    PyApi->>RNS: identity.sign(bytes(message))
    RNS-->>PyApi: signature bytes
    PyApi-->>ChImpl: PyObject (bytes)
    ChImpl->>ChImpl: result.toJava(ByteArray) + result.close()
    ChImpl-->>KCaller: ByteArray

    KCaller->>KBinding: create(identity, direction, type, appName, aspects)
    KBinding->>ChImpl: create(...)
    ChImpl->>PyApi: callAttr("create_destination", pyIdentity, 0x11, 0, appName, pyAspects)
    Note over ChImpl: pyAspects closed in finally
    PyApi->>RNS: RNS.Destination(identity, direction, dest_type, app_name, *aspects)
    RNS-->>PyApi: live Destination object
    PyApi-->>ChImpl: PyObject (Destination)
    ChImpl-->>KCaller: ChaquopyRnsDestination (wraps PyObject)

    KCaller->>KBinding: getPropagationState()
    KBinding->>ChImpl: getPropagationState()
    ChImpl->>PyApi: callAttr("lxmf_get_propagation_state")
    PyApi->>RNS: lxmf_router.propagation_transfer_state
    RNS-->>PyApi: dict {state, state_name, progress, messages_received}
    PyApi-->>ChImpl: PyObject (dict)
    ChImpl->>ChImpl: parsePropagationDict — iterate once, close each K/V
    ChImpl-->>KCaller: PropagationState(data class)
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: python/rns_api.py
Line: 161-162

Comment:
**Incorrect constant values documented in `create_destination` docstring**

The docstring documents the wrong numeric values for both `direction` and `dest_type`. Kotlin's `ChaquopyRnsDestinationProvider` passes the actual RNS constants:

- `Direction.IN → 0x11` (17), `Direction.OUT → 0x12` (18)
- `DestinationType.SINGLE → 0`, `GROUP → 1`, `PLAIN → 2`, `LINK → 3`

But the docstring claims `IN=1, OUT=2` and `SINGLE=1, GROUP=2, PLAIN=3, LINK=4`, both off from the real values. A future maintainer reading the docstring and writing a direct Python caller would pass the wrong integers.

```suggestion
            direction: int (RNS.Destination.IN=0x11 (17) or RNS.Destination.OUT=0x12 (18))
            dest_type: int (SINGLE=0, GROUP=1, PLAIN=2, LINK=3)
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: python/rns_api.py
Line: 257-258

Comment:
**`reason` parameter silently dropped in `link_teardown`**

`reason` is accepted as a parameter (and passed by the Kotlin caller via `link_teardown(pyLink, reason)`) but `link.teardown()` is called without it. `RNS.Link.teardown()` does accept an optional `reason` integer — silently dropping it means the remote peer always receives the default reason code regardless of what the Kotlin side requested.

```suggestion
    def link_teardown(link, reason=0):
        link.teardown(reason)
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyLxmfMessage.kt
Line: 85-95

Comment:
**`asMap()` double-iteration leaks PyObjects from `associate` lambda**

The `associate { (k, v) -> k.toInt() to v.toJava(...) }` call materialises one set of `PyObject` key/value wrappers and extracts the primitives from them. The `finally` block then iterates `pyDict` a **second time**, materialising a fresh set of wrappers and closing those. Chaquopy creates a new wrapper instance on each access into the dict view, so the original set from `associate` is never explicitly closed — it leaks until the JVM GC eventually finalises them.

The fix is a single, manual loop so each wrapper is closed in the same iteration it was opened:

```kotlin
val pyDict = result.asMap() as Map<PyObject, PyObject>
buildMap {
    for ((k, v) in pyDict) {
        try {
            put(k.toInt(), v.toJava(Any::class.java) as Any)
        } finally {
            k.close()
            v.close()
        }
    }
}
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: d0cd739

Comment thread app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyRnsDestination.kt Outdated
Comment thread app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyLxmfMessageFactory.kt Outdated
Comment thread app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyRnsTransport.kt Outdated
The createLinkCallback parameter is intentionally unused now — it will
be wired when we bridge Python callbacks to Kotlin in Phase 4.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@sentry
Copy link
Copy Markdown
Contributor

sentry bot commented Mar 12, 2026

Codecov Report

❌ Patch coverage is 0% with 312 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
python/rns_api.py 0.00% 312 Missing ⚠️

📢 Thoughts on this report? Let us know!

torlando-tech and others added 2 commits March 11, 2026 23:24
- Fix RNS constant mapping: IN=0x11, OUT=0x12, SINGLE=0, GROUP=1, PLAIN=2, LINK=3
- Remove broken createLinkCallback that returned api as callable (would TypeError)
- Replace null source/dest in LxmfMessageFactory with UnsupportedOperationException
- Close intermediate PyObject keys/values in getInterfaces() and fields getter

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines +81 to +91
responseGenerator: (path: String, data: ByteArray?, requestId: ByteArray) -> ByteArray?,
) {
// TODO: Bridge Python request handler callback to Kotlin lambda
api
.callAttr(
"destination_register_request_handler",
pyDestination,
path,
null as Any?, // Will wire callback in Phase 4
)?.close()
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Calling registerRequestHandler will cause a runtime crash because it passes a null callback to a Python function that does not permit None as a value.
Severity: HIGH

Suggested Fix

The Kotlin implementation of registerRequestHandler should be updated to not pass null for the response_generator. If this functionality is not yet implemented, it should throw an UnsupportedOperationException or be stubbed in a way that does not cause a crash.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location:
app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyRnsDestination.kt#L79-L91

Potential issue: The `registerRequestHandler` function in `ChaquopyRnsDestination.kt`
passes a `null` value for the `response_generator` parameter to the underlying Python
API. This `null` is converted to `None` in Python. The Python
`Destination.register_request_handler` function explicitly checks if this parameter is
callable and raises a `ValueError` if it is not, as is the case with `None`. This will
cause a runtime crash whenever this public API method is invoked.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment thread app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyRnsLink.kt
@torlando-tech
Copy link
Copy Markdown
Owner Author

@greptileai

…tubs

- registerRequestHandler: skip Python call (RNS validates callable, raises ValueError for None)
- setClosedCallback/setPacketCallback: only call Python to clear (null); skip for non-null (Phase 4)

Same pattern as setLinkEstablishedCallback fix.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines +108 to +113
// Close all PyObject keys and values from the dict view
for ((k, v) in dict) {
k.close()
v.close()
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The code closes individual keys and values from a Python dictionary, which can include singleton objects like True or False, potentially causing runtime crashes.
Severity: CRITICAL

Suggested Fix

Remove the loop that iterates through the dictionary entries and calls k.close() and v.close(). The individual keys and values should not be closed. Only the top-level PyObject container returned from the Python call should be closed, which is already handled correctly.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location:
app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyRnsTransport.kt#L108-L113

Potential issue: The `getInterfaces()` method retrieves a list of dictionaries from
Python. A `finally` block then iterates through each dictionary's entries and calls
`close()` on every key and value `PyObject`. Some of these values are Python singletons,
such as the boolean `True` and `False`. According to Chaquopy documentation, closing a
`PyObject` that wraps a singleton will invalidate all other references to that singleton
across the entire Python runtime. This can lead to non-deterministic crashes when any
subsequent Python code attempts to access the invalidated object.

Comment thread app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyRnsLink.kt
Comment thread python/rns_api.py
Comment thread app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyLxmfRouter.kt Outdated
…ded LOG_DEBUG

- ChaquopyLxmfRouter: close dict keys/values after extracting primitives
- ChaquopyRnsDestinationProvider: close pyAspects after passing to create_destination
- rns_api.py: parameterize loglevel (default LOG_WARNING instead of LOG_DEBUG)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@torlando-tech
Copy link
Copy Markdown
Owner Author

@greptileai

Comment on lines +85 to +95
val pyDict = result.asMap() as Map<PyObject, PyObject>
try {
pyDict.entries.associate { (k, v) ->
k.toInt() to (v.toJava(Any::class.java) as Any)
}
} finally {
// Close all PyObject keys and values from the dict view
for ((k, v) in pyDict) {
k.close()
v.close()
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asMap() double-iteration leaks PyObjects from associate lambda

The associate { (k, v) -> k.toInt() to v.toJava(...) } call materialises one set of PyObject key/value wrappers and extracts the primitives from them. The finally block then iterates pyDict a second time, materialising a fresh set of wrappers and closing those. Chaquopy creates a new wrapper instance on each access into the dict view, so the original set from associate is never explicitly closed — it leaks until the JVM GC eventually finalises them.

The fix is a single, manual loop so each wrapper is closed in the same iteration it was opened:

val pyDict = result.asMap() as Map<PyObject, PyObject>
buildMap {
    for ((k, v) in pyDict) {
        try {
            put(k.toInt(), v.toJava(Any::class.java) as Any)
        } finally {
            k.close()
            v.close()
        }
    }
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/com/lxmf/messenger/service/rns/ChaquopyLxmfMessage.kt
Line: 85-95

Comment:
**`asMap()` double-iteration leaks PyObjects from `associate` lambda**

The `associate { (k, v) -> k.toInt() to v.toJava(...) }` call materialises one set of `PyObject` key/value wrappers and extracts the primitives from them. The `finally` block then iterates `pyDict` a **second time**, materialising a fresh set of wrappers and closing those. Chaquopy creates a new wrapper instance on each access into the dict view, so the original set from `associate` is never explicitly closed — it leaks until the JVM GC eventually finalises them.

The fix is a single, manual loop so each wrapper is closed in the same iteration it was opened:

```kotlin
val pyDict = result.asMap() as Map<PyObject, PyObject>
buildMap {
    for ((k, v) in pyDict) {
        try {
            put(k.toInt(), v.toJava(Any::class.java) as Any)
        } finally {
            k.close()
            v.close()
        }
    }
}
```

How can I resolve this? If you propose a fix, please make it concise.

@torlando-tech torlando-tech changed the title feat: add RNS/LXMF binding interfaces and Chaquopy implementations refactor: add RNS/LXMF binding interfaces and Chaquopy implementations Mar 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant