Skip to content

feat(adb): SMS read/send/delete/mark bridge via ContentProvider - #1

Merged
ygpark80 merged 2 commits into
masterfrom
feat/adb-sms-bridge
Jul 23, 2026
Merged

ygpark80 merged 2 commits into
masterfrom
feat/adb-sms-bridge

Conversation

@ygpark80

@ygpark80 ygpark80 commented Jul 23, 2026 •

Copy link
Copy Markdown

Adds AdbMessagesProvider — a ContentProvider folded into QUIK that relays read / send / delete / mark-read over adb shell content and on-device automation, reusing QUIK's own MessageRepository / ConversationRepository.

Why this shape

Deleting/writing SMS on Android is allowed only for the default SMS app (OS-enforced). Because QUIK is your permanent default messenger, it already holds those rights at all times — so this bridge needs no default-role juggling and normal texting keeps working. (A standalone helper like adbsms has to seize the default role, which disables messaging while active.)

Docs live in the provider's KDoc header; the usage + security summary is inlined below.

Interface

Authority ${applicationId}.adb — content://dev.octoshrimpy.quik.adb (release), ...debug.adb (debug). Swap to match your build.

Read (_id, id, threadId, contentId, address, body, date, dateSent, read, seen, type, boxId, subId, locked):

adb shell content query --uri content://dev.octoshrimpy.quik.adb/messages            # all, newest first
adb shell content query --uri content://dev.octoshrimpy.quik.adb/messages/137        # one
adb shell content query --uri "content://dev.octoshrimpy.quik.adb/messages?threadId=42&limit=20"
adb shell content query --uri content://dev.octoshrimpy.quik.adb/conversations       # threads

Send (real SMS via QUIK's send path; optional --bind subId:i:<n>):

adb shell content insert --uri content://dev.octoshrimpy.quik.adb/messages \
    --bind address:s:+821012345678 --bind body:s:hello_from_adb

Delete (removed from Realm and the system SMS provider):

adb shell content delete --uri content://dev.octoshrimpy.quik.adb/messages/137
adb shell content delete --uri content://dev.octoshrimpy.quik.adb/messages --where "id IN (1,2,3)"

Mark read / unread (per-thread, matching the repository):

adb shell content update --uri content://dev.octoshrimpy.quik.adb/messages/137 --bind read:i:1
adb shell content update --uri content://dev.octoshrimpy.quik.adb/messages/137 --bind read:i:0

Security

The provider must stay exported (the shell uid is a separate process, so adb can only reach an exported provider). Left unguarded that would let any installed app read/send/delete SMS with no permission of its own — a route to leak OTPs or send SMS silently. So every call is gated fail-closed by Binder.getCallingUid() (kernel-supplied, unspoofable), uniformly across read/send/delete/mark. Only two callers are allowed:

  • QUIK itself (same uid).
  • shell (uid 2000), so adb works — already gated by USB debugging + an authorized host.

Every other caller is rejected with SecurityException. This is an adb-only bridge by design — it is deliberately not exposed to other apps (on-device automation belongs inside QUIK as a feature, not behind an app-facing provider). There is no URL token either — a query-parameter token leaks into logcat/dumpsys, and the uid gate is stronger and uniform.

Verified E2E

Physical Galaxy S24 Ultra (Android 17), QUIK set as default SMS app:

  • read — queried the real 891-message store via adb.
  • delete — removed a message from both Realm and content://sms/{id} ("No result found" after).
  • send — sent a real SMS (self round-trip produced sent + received rows; also in content://sms/sent).
  • caller gate — after reinstall, the shell uid still reads through the gate (no SecurityException, real data returned).

Note: system-provider deletes only land when the default-SMS role is settled; flipping the role immediately before a delete no-ops silently (test-harness artifact — not a concern when QUIK is the standing default).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added an ADB-accessible content provider for reading and managing SMS messages and conversations.
    • Supports querying, sending, deleting, and marking messages as read or unread.
    • Access is restricted to the app and Android shell processes.
  • Chores

    • Added automated daily upstream synchronization with conflict reporting.

@codeant-ai

codeant-ai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown

🤖 CodeAnt AI — Review Status

Status Commit Started (UTC) Finished (UTC)
✅ Reviewed your PR a64176d Jul 23, 2026 · 08:16 08:22

Updated in place by CodeAnt AI · last 5 reviews

@codeant-ai

codeant-ai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c0664f8-9666-4640-aeab-89a00322a2f5

📥 Commits

Reviewing files that changed from the base of the PR and between a64176d and 3b640b8.

📒 Files selected for processing (4)
  • .github/workflows/sync-upstream.yml
  • presentation/src/main/AndroidManifest.xml
  • presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
  • presentation/src/main/java/com/moez/QKSMS/injection/AppComponent.kt

📝 Walkthrough

Walkthrough

Adds an exported ADB-accessible SMS ContentProvider with repository-backed queries and mutations, Dagger injection, and UID checks. Also adds a scheduled GitHub Actions workflow that merges upstream changes into master and reports conflicts through an issue.

Changes

ADB SMS bridge

Layer / File(s) Summary
Provider registration and initialization
presentation/src/main/AndroidManifest.xml, presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt, presentation/src/main/java/com/moez/QKSMS/injection/AppComponent.kt
Registers the exported provider, initializes its authority and URI matcher, injects message and conversation repositories, and restricts callers to the app UID or shell UID.
Message and conversation queries
presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
Adds filtered, sorted message queries; non-archived conversation queries; cursor column mappings; and MIME type handling.
Message mutations and conversation refreshes
presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
Adds message insertion, deletion, read-state updates, numeric selection parsing, and conversation updates for affected threads.

Upstream synchronization

Layer / File(s) Summary
Scheduled upstream merge workflow
.github/workflows/sync-upstream.yml
Adds daily and manual synchronization that merges and pushes upstream/master, or creates an issue when merge conflicts occur.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ADB
  participant AdbMessagesProvider
  participant MessageRepository
  participant ConversationRepository
  ADB->>AdbMessagesProvider: query, insert, update, or delete request
  AdbMessagesProvider->>MessageRepository: read or mutate messages
  AdbMessagesProvider->>ConversationRepository: refresh affected conversations
  AdbMessagesProvider-->>ADB: cursor, URI, or mutation count
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the new ADB SMS bridge ContentProvider and its read, send, delete, and mark-read functionality.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/adb-sms-bridge

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Checkov (3.3.8)
.github/workflows/sync-upstream.yml

Traceback (most recent call last):
File "/usr/local/bin/checkov", line 2, in
from checkov.main import Checkov
ModuleNotFoundError: No module named 'checkov'


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

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

@codeant-ai codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Jul 23, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt`:
- Around line 231-233: The parseIdsFromSelection function must only accept
complete supported predicates: id = <n> or id IN (<n>, ...), using matchEntire
rather than extracting arbitrary digits. Reject unsupported or compound
selections (unless selectionArgs are handled), and deduplicate the returned IDs
before delete() or update() uses them.
- Around line 87-92: Make the lazy initialization in ensureReady thread-safe:
mark injected as volatile and synchronize the check-and-inject sequence so only
one caller invokes appComponent.inject(this), with the flag published after
injection completes. Ensure provider methods access messageRepo and
conversationRepo only after this synchronized initialization.
- Around line 95-103: The AdbMessagesProvider.checkToken authorization must
default-deny when no token is configured: remove the open-access return and
reject non-shell callers, while preserving token validation for configured
tokens. Update ADB_BRIDGE.md at lines 70-79 to document the required production
token lifecycle and explicitly describe any supported Android shell exception.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3727f4ce-281e-40e6-aa23-e2dad29be0ff

📥 Commits

Reviewing files that changed from the base of the PR and between 01474f0 and a64176d.

📒 Files selected for processing (4)
  • ADB_BRIDGE.md
  • presentation/src/main/AndroidManifest.xml
  • presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
  • presentation/src/main/java/com/moez/QKSMS/injection/AppComponent.kt

Comment on lines +87 to +92
/** Dagger graph isn't ready at provider onCreate (before Application.onCreate); inject lazily. */
private fun ensureReady() {
if (!injected) {
appComponent.inject(this)
injected = true
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate and inspect the target file and related component/provider lifecycle.
printf 'Files matching AdbMessagesProvider.kt:\n'
fd -a 'AdbMessagesProvider\.kt$' . || true

printf '\nTarget file outline:\n'
ast-grep outline presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt || true

printf '\nRelevant source with line numbers:\n'
cat -n presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt | sed -n '1,180p'

printf '\nReferences to appComponent/injected/inject(this) in provider:\n'
rg -n "appComponent|`@Inject`|`@Component`|interface Adb|class Adb|ensureReady|injected" presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt

Repository: circlesac/quik

Length of output: 9953


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Remaining provider methods:\n'
cat -n presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt | sed -n '179,250p'

printf '\nDagger/appComponent definitions:\n'
rg -n "appComponent|interface .*Component|`@Component`|AppComponent|inject\\(AdbMessagesProvider" -S .

printf '\nContentProvider lifecycle/threading usage in repo:\n'
fd -a 'AndroidManifest.xml|Manifest.java|AndroidManifest\\.xml' . -x sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' sh {} | sed -n '1,220p'

printf '\nAndroid manifest provider registration snippets:\n'
rg -n "AdbMessagesProvider|adb|ContentProviderProvider|android:multiprocess|android:exported" -S ./presentation ./app || true

printf '\nKotlin/dependency versions relevant for lateinit/lateinit semantics:\n'
rg -n "kotlin|dagger|hilt|androidx|AndroidManifest|provider" -S build.gradle* settings.gradle* || true

Repository: circlesac/quik

Length of output: 28122


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'AppComponentManager:\n'
cat -n presentation/src/main/java/com/moez/QKSMS/injection/AppComponentManager.kt

printf '\nQKApplication lifecycle init order:\n'
cat -n presentation/src/main/java/com/moez/QKSMS/common/QKApplication.kt | sed -n '1,140p'

printf '\nInjection component around provider target:\n'
cat -n presentation/src/main/java/com/moez/QKSMS/injection/AppComponent.kt | sed -n '50,80p'

printf '\nRepository properties/types:\n'
rg -n "class .*MessageRepository|class .*ConversationRepository|lateinit var|MessageRepository =|ConversationRepository =" presentation/src/main/java -S .

Repository: circlesac/quik

Length of output: 50370


Synchronize the lazy injection flag before provider operations.

ensureReady() is called from multiple provider methods, but injected is a plain mutable property. A concurrent caller can observe true without a happens-before edge to Dagger’s field assignments and call messageRepo/conversationRepo while they are still uninitialized. Mark it @Volatile and guard the injection in a synchronization block, or use a thread-safe lazy holder, before accessing the repositories.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt` around
lines 87 - 92, Make the lazy initialization in ensureReady thread-safe: mark
injected as volatile and synchronize the check-and-inject sequence so only one
caller invokes appComponent.inject(this), with the flag published after
injection completes. Ensure provider methods access messageRepo and
conversationRepo only after this synchronized initialization.

Comment thread presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt Outdated
Comment on lines +231 to +233
/** Pull the numeric ids out of a `where` clause like `id IN (1, 2, 3)` or `id = 5`. */
private fun parseIdsFromSelection(selection: String?): List<Long> =
selection?.let { Regex("\\d+").findAll(it).map { m -> m.value.toLong() }.toList() } ?: emptyList()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Strictly parse supported ID predicates.

This extracts digits from any selection: threadId = 42 becomes message ID 42, and compound clauses can collect unrelated numeric values. delete() and update() can therefore mutate unintended messages. This is a destructive targeting bug, not SQL injection.

Accept only complete id = <n> / id IN (<n>, ...) clauses with matchEntire, reject unsupported selections (or honor selectionArgs), and deduplicate IDs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt` around
lines 231 - 233, The parseIdsFromSelection function must only accept complete
supported predicates: id = <n> or id IN (<n>, ...), using matchEntire rather
than extracting arbitrary digits. Reject unsupported or compound selections
(unless selectionArgs are handled), and deduplicate the returned IDs before
delete() or update() uses them.

Comment on lines +88 to +93
private fun ensureReady() {
if (!injected) {
appComponent.inject(this)
injected = true
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: ensureReady() is not thread-safe for a Binder-exposed ContentProvider: two concurrent calls can race on injected, leading to duplicate injection or visibility issues where one thread observes injected = true before injected fields are safely visible. Guard initialization with synchronization (or another thread-safe one-time init primitive) so injection and flag publication are atomic. [race condition]

Severity Level: Major ⚠️
❌ Concurrent adb calls can crash provider initialization.
⚠️ On-device automation unreliable under parallel bridge usage.
Steps of Reproduction ✅
1. Register the adb bridge provider as seen in
`presentation/src/main/AndroidManifest.xml:258-260`, where `.adb.AdbMessagesProvider` is
declared as a "provider" with `android:authorities="${applicationId}.adb"` and exported
for external access (also documented in `ADB_BRIDGE.md:68-78`).

2. From a host or on-device automation, issue two concurrent commands, for example `adb
shell content query --uri content://dev.octoshrimpy.quik.adb/messages` and `adb shell
content insert --uri content://dev.octoshrimpy.quik.adb/messages ...`, which are
dispatched into `AdbMessagesProvider.query()` (AdbMessagesProvider.kt:105-158) and
`insert()` (160-177) on separate binder threads.

3. Both entrypoints immediately call `ensureReady()` at AdbMessagesProvider.kt:88-93,
which checks the non-volatile `injected` flag at line 72 and, if false, executes
`appComponent.inject(this)` at line 90 to initialize the `@Inject lateinit var` fields
`messageRepo` and `conversationRepo` defined at lines 69-70.

4. Because `ensureReady()` is unsynchronized, two threads can race: writes to `injected`
and the injected fields may be observed out of order, allowing a third thread to see
`injected == true` while `messageRepo` / `conversationRepo` are not safely published, then
dereference `messageRepo` in `insert()` (line 172) or `delete()` (line 194), causing
intermittent initialization errors or crashes in the adb bridge under parallel usage.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
**Line:** 88:93
**Comment:**
	*Race Condition: `ensureReady()` is not thread-safe for a Binder-exposed `ContentProvider`: two concurrent calls can race on `injected`, leading to duplicate injection or visibility issues where one thread observes `injected = true` before injected fields are safely visible. Guard initialization with synchronization (or another thread-safe one-time init primitive) so injection and flag publication are atomic.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +124 to +127
val limit = uri.getQueryParameter("limit")?.toIntOrNull() ?: results.size

MatrixCursor(MESSAGE_COLUMNS).apply {
results.take(limit).forEach { m ->

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: limit is accepted as any integer, but take() throws IllegalArgumentException for negative values; a request like ?limit=-1 will crash the provider. Clamp limit to a non-negative range before passing it to take. [logic error]

Severity Level: Major ⚠️
❌ Negative limit crashes adb message queries unexpectedly.
⚠️ Automation scripts fail when misconfigured query parameters.
Steps of Reproduction ✅
1. Use the documented adb SMS bridge from `ADB_BRIDGE.md:20-34` but supply a negative
limit, e.g. `adb shell content query --uri
"content://dev.octoshrimpy.quik.adb/messages?limit=-1"`; this enters
`AdbMessagesProvider.query()` at
`presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt:105-158`.

2. Inside `query()`, Realm results are loaded into `results` (lines 113-123). The code
computes `limit` from the URI at line 124: `val limit =
uri.getQueryParameter("limit")?.toIntOrNull() ?: results.size`.

3. With `?limit=-1`, `getQueryParameter("limit")` returns the string `"-1"`,
`toIntOrNull()` yields `-1`, and therefore `limit` is set to `-1` instead of defaulting to
`results.size`.

4. At line 127, `results.take(limit)` calls Kotlin's `List.take(n)` with `n = -1`, which
throws `IllegalArgumentException` for a negative count, causing the ContentProvider to
crash and the adb `content query` operation to fail unexpectedly.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
**Line:** 124:127
**Comment:**
	*Logic Error: `limit` is accepted as any integer, but `take()` throws `IllegalArgumentException` for negative values; a request like `?limit=-1` will crash the provider. Clamp `limit` to a non-negative range before passing it to `take`.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

if (ids.isEmpty()) return 0

// Look up affected threads before deleting so we can refresh those conversations after.
val threadIds = ids.mapNotNull { messageRepo.getUnmanagedMessage(it)?.threadId }.distinct()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: Resolving thread IDs by calling getUnmanagedMessage per ID creates one Realm lookup per message, which scales poorly for bulk operations (id IN (...)). Fetch all needed messages in one query to avoid N+1 database access in delete/update paths. [performance]

Severity Level: Major ⚠️
⚠️ Bulk delete/update operations slow for large ID sets.
⚠️ On-device automation suffers delays on multi-message actions.
Steps of Reproduction ✅
1. Use the bridge bulk delete pattern from `ADB_BRIDGE.md:49-57`, e.g. `adb shell content
delete --uri content://dev.octoshrimpy.quik.adb/messages --where "id IN
(1,2,3,...,1000)"`, which reaches `AdbMessagesProvider.delete()` at
`presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt:179-199`.

2. `delete()` parses the IDs from the selection string via
`parseIdsFromSelection(selection)` (lines 183-186), producing a `List<Long>` with many
message IDs for large operations.

3. To compute affected threads, `delete()` executes `val threadIds = ids.mapNotNull {
messageRepo.getUnmanagedMessage(it)?.threadId }.distinct()` at line 191. Each
`getUnmanagedMessage(it)` call opens a new Realm instance and queries by ID, as
implemented in `MessageRepositoryImpl.getUnmanagedMessage()` at
`data/src/main/java/com/moez/QKSMS/repository/MessageRepositoryImpl.kt:13-16`.

4. For N IDs, this results in N separate Realm get/open operations before
`messageRepo.deleteMessages(ids)` at line 194, causing N+1 database queries and degrading
performance noticeably when automation or scripts perform large multi-message
delete/update actions.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
**Line:** 191:191
**Comment:**
	*Performance: Resolving thread IDs by calling `getUnmanagedMessage` per ID creates one Realm lookup per message, which scales poorly for bulk operations (`id IN (...)`). Fetch all needed messages in one query to avoid N+1 database access in delete/update paths.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +215 to +222
if (ids.isEmpty()) return 0

// read/unread operate per-thread in the repository, so resolve threads from the message ids.
val threadIds = ids.mapNotNull { messageRepo.getUnmanagedMessage(it)?.threadId }.distinct()

if (read != 0) messageRepo.markRead(threadIds) else messageRepo.markUnread(threadIds)
conversationRepo.updateConversations(threadIds)
return ids.size

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: update() returns the requested ID count, not the number of rows actually modified; nonexistent IDs still report success, which breaks ContentProvider row-count semantics and can mislead callers into thinking updates were applied. Return the real affected-row count based on existing/resolved messages. [api mismatch]

Severity Level: Major ⚠️
⚠️ Callers misinterpret update success, masking stale IDs.
⚠️ Bridge status checks unreliable for failed read/unread.
Steps of Reproduction ✅
1. Invoke the read/unread bridge as documented in `ADB_BRIDGE.md:59-64`, but with an ID
that does not exist, e.g. `adb shell content update --uri
content://dev.octoshrimpy.quik.adb/messages/999999 --bind read:i:1`. This enters
`AdbMessagesProvider.update()` at
`presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt:201-223`.

2. `update()` resolves the IDs from either the URI or the selection at lines 210-213; for
`/messages/999999`, it sets `ids = listOf(ContentUris.parseId(uri))`, yielding a single ID
`999999L`.

3. It then constructs `threadIds` via `ids.mapNotNull {
messageRepo.getUnmanagedMessage(it)?.threadId }.distinct()` at line 218. Since
`messageRepo.getUnmanagedMessage(999999L)` (implemented in
`MessageRepositoryImpl.kt:13-16`) returns `null` when the message is missing, `threadIds`
becomes an empty list and `messageRepo.markRead(threadIds)` / `markUnread(threadIds)` at
line 220 perform no state changes.

4. Despite no rows being modified, `update()` returns `ids.size` at line 222 (in this case
`1`), violating ContentProvider semantics by reporting success based on requested IDs
rather than actual affected messages, and misleading callers that rely on the return count
to detect stale or nonexistent IDs.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
**Line:** 215:222
**Comment:**
	*Api Mismatch: `update()` returns the requested ID count, not the number of rows actually modified; nonexistent IDs still report success, which breaks ContentProvider row-count semantics and can mislead callers into thinking updates were applied. Return the real affected-row count based on existing/resolved messages.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Comment on lines +232 to +233
private fun parseIdsFromSelection(selection: String?): List<Long> =
selection?.let { Regex("\\d+").findAll(it).map { m -> m.value.toLong() }.toList() } ?: emptyList()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The ID parser extracts every digit sequence from the full selection string, so unrelated numbers (phone numbers, timestamps, other predicates) are treated as message IDs and can cause unintended delete/update of wrong rows. Parse only validated id predicates (or use structured selectionArgs) instead of global \\d+ scraping. [incorrect variable usage]

Severity Level: Critical 🚨
❌ Wrong SMS messages deleted due to misparsed selection.
⚠️ Threads marked read/unread incorrectly from complex predicates.
Steps of Reproduction ✅
1. Follow the documented delete interface in `ADB_BRIDGE.md:49-57` but include additional
numeric predicates, e.g. `adb shell content delete --uri
content://dev.octoshrimpy.quik.adb/messages --where "threadId = 42 AND date <
1234567890"`. This routes to `AdbMessagesProvider.delete()` in
`presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt:179-199`.

2. For `/messages` URIs, `delete()` computes `ids` via `parseIdsFromSelection(selection)`
at lines 183-186, passing the entire selection string `threadId = 42 AND date <
1234567890`.

3. `parseIdsFromSelection()` at lines 232-233 uses `Regex("\\d+")` across the raw
selection and maps every digit sequence (`"42"`, `"1234567890"`) to `Long`s, without
checking that they belong to an `id` predicate; the resulting `ids` list therefore
contains `42L` and `1234567890L`.

4. `messageRepo.deleteMessages(ids)` at line 194 subsequently deletes messages whose IDs
match these unrelated numbers, and `conversationRepo.updateConversations(threadIds)` at
line 195 refreshes threads accordingly, leading to unintended deletion or state changes in
messages whose IDs happen to equal thread IDs, timestamps, or other numeric values
embedded in complex where clauses.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** presentation/src/main/java/com/moez/QKSMS/adb/AdbMessagesProvider.kt
**Line:** 232:233
**Comment:**
	*Incorrect Variable Usage: The ID parser extracts every digit sequence from the full `selection` string, so unrelated numbers (phone numbers, timestamps, other predicates) are treated as message IDs and can cause unintended delete/update of wrong rows. Parse only validated `id` predicates (or use structured `selectionArgs`) instead of global `\\d+` scraping.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

@ygpark80
ygpark80 force-pushed the feat/adb-sms-bridge branch 2 times, most recently from f101381 to b4d11a9 Compare July 23, 2026 08:57
Add AdbMessagesProvider, a ContentProvider folded into QUIK that relays
read / send / delete / mark-read over `adb shell content`, reusing QUIK's
own MessageRepository/ConversationRepository. Because QUIK is the permanent
default SMS app it already holds full read+write rights on the system SMS
provider, so no default-role juggling is needed (unlike a standalone adbsms
helper, which seizes the role and disables normal messaging while active).

Authority `<applicationId>.adb`. Endpoints: /messages, /messages/#,
/conversations. Usage lives in the provider's KDoc header.

Security: the provider must stay exported (shell is a separate uid, so adb
can only reach an exported provider), which would otherwise let any app
read/send/delete SMS with no permission of its own. Every call is gated
fail-closed by Binder.getCallingUid() (kernel-supplied, unspoofable): only
QUIK's own uid and the shell uid (adb; already gated by USB debugging + an
authorized host) may call; everything else is rejected with
SecurityException. This is an adb-only bridge by design — deliberately not
exposed to other apps (on-device automation belongs inside QUIK as a
feature, not behind an app-facing provider). No URL token, which would
leak into logcat/dumpsys.

Verified E2E on a physical device (Galaxy S24 Ultra, Android 17), QUIK as
default SMS app: read returned the real message store; delete removed a row
from both Realm and content://sms/{id}; send produced a real SMS (self
round-trip: sent + received rows, also in content://sms/sent); after
reinstall the shell uid still reads through the caller gate. Deletes only
reach the system provider once the default-SMS role is settled (a role flip
immediately before a delete no-ops silently — a test-harness artifact, not
a concern when QUIK is the standing default).

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@ygpark80
ygpark80 force-pushed the feat/adb-sms-bridge branch from b4d11a9 to fb67883 Compare July 23, 2026 09:08
Daily (and on-demand) job that merges octoshrimpy/quik master into this
fork's master, carrying the fork's additive changes (the adb SMS bridge).
The bridge is a new file + two tiny additive edits, so the merge is
normally conflict-free; on a conflict the job aborts and opens an issue.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@ygpark80
ygpark80 merged commit 008ef28 into master Jul 23, 2026
1 check passed
@ygpark80
ygpark80 deleted the feat/adb-sms-bridge branch July 23, 2026 10:09
@ygpark80
ygpark80 restored the feat/adb-sms-bridge branch July 24, 2026 11:14
@coderabbitai coderabbitai Bot mentioned this pull request Aug 12, 2026
6 tasks done
@ygpark80
ygpark80 deleted the feat/adb-sms-bridge branch October 8, 2026 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant