feat: Enhance pnpm configuration and implement mobile runtime parity#372
Merged
yacosta738 merged 8 commits intoMar 31, 2026
Merged
Conversation
…target - Move and expand pnpm dependency overrides to pnpm-workspace.yaml and package.json root - Add catalog and workspace package globs to pnpm-workspace.yaml for improved monorepo management - Enhance Makefile clean-all target to clean Rust, web, pnpm, and Gradle artifacts and caches
…ents, update state to propose phase
…id/ios - Redesign mobile clients to be onboarding-first, not local runtime hosts - Add RuntimeConnectionMethod, RuntimeConnectionTarget, RuntimeTrustState types - Update onboarding to guide users through endpoint/URL or pairing setup - Replace default RustCliBridge/host wiring with fail-closed behavior - Add client-first copy and UI states (TARGET_SELECTED, RECOVERY) - Remove Android local runtime packaging assumptions - Add 75+ tests for client-first behavior - Update onboarding and client-surfaces specs - Archive change with scope limitation note on real device smoke validation DALLAY-179
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTransitions mobile surfaces to a client-first model: adds shared runtime contracts, platform bridges and persistence, a MobileRuntimeCoordinator driving onboarding/readiness/session/approval flows, refactors Compose UI to consume coordinator state, and updates tooling/docs and workspace configs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (Compose)
participant Coord as MobileRuntimeCoordinator
participant Facade as RuntimeFacade
participant Persist as RuntimePersistence
participant UI as Chat / Config UI
App->>Coord: rememberPlatformRuntimeDependencies(initialSnapshot?)
Coord->>Persist: readLinkedRuntimeMetadata()
Persist-->>Coord: metadata?
Coord->>Facade: probeReadiness()
Facade-->>Coord: RuntimeReadinessSnapshot
Coord-->>App: expose coordinatorState
UI->>App: user action (start/resume/send/approve/disconnect)
App->>Coord: delegate action
Coord->>Facade: create/resume/send/submit/end...
Facade-->>Coord: RuntimeSession / RuntimeTurnResult / Failure / ApprovalPending
Coord->>Persist: saveActiveSessionId / saveLinkedRuntimeMetadata / clear...
Coord-->>App: updated coordinatorState (messages, sessions, pendingApproval)
App->>UI: render updated state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
✅ Contributor ReportUser: @yacosta738
Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-03-31 to 2026-03-31 |
Deploying corvus with
|
| Latest commit: |
3e82ab8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://20cc9d8d.corvus-42x.pages.dev |
| Branch Preview URL: | https://feature-dallay-179-mobile-ru.corvus-42x.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 41
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
clients/composeApp/build.gradle.kts (1)
44-74:⚠️ Potential issue | 🟡 MinorFix Detekt
UnusedPrivatePropertyerrors flagged by pipeline.The
by gettingdelegation pattern creates unused local vals. Switch togetByNameto configure source sets without introducing local variables:🔧 Proposed fix
sourceSets { - val commonMain by getting { + commonMain { dependencies { implementation(libs.compose.runtime) implementation(libs.compose.foundation) implementation(libs.compose.material3) implementation(libs.compose.ui) implementation(libs.compose.components.resources) implementation(libs.compose.ui.tooling.preview) implementation(libs.androidx.lifecycle.viewmodel.compose) implementation(libs.androidx.lifecycle.runtime.compose) implementation(compose.materialIconsExtended) } } - val commonTest by getting { dependencies { implementation(kotlin("test")) } } + commonTest { dependencies { implementation(kotlin("test")) } } - val jvmMain by getting { + jvmMain { dependencies { implementation(project(":agent-core-kmp")) implementation(compose.desktop.currentOs) implementation(libs.kotlinx.coroutines.swing) } } - val androidMain by getting { + androidMain { dependencies { implementation(libs.compose.ui.tooling.preview) implementation(libs.androidx.activity.compose) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/composeApp/build.gradle.kts` around lines 44 - 74, The unused private property warnings come from the "val <name> by getting" delegations; replace those with getByName(...) calls to avoid creating local vals. For each source set (commonMain, commonTest, jvmMain, androidMain) change "val commonMain by getting { ... }" style to "getByName(\"commonMain\") { ... }" (and similarly for "commonTest", "jvmMain", "androidMain") or "getByName(\"name\").apply { ... }" so you configure the source sets without introducing unused local variables; use the getByName function to locate and modify the source set blocks.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/onboarding/OnboardingScreen.kt (1)
152-162:⚠️ Potential issue | 🟠 MajorAddress the detekt blockers by deriving UI state from
OnboardingStepand trimming parameters.The added
primaryActionLabelcontributes toLongParameterList, and this function is already over method-size limits. Reduce inputs and usestep.progressIndex,step.totalSteps, andstep.actionLabeldirectly.♻️ Refactor sketch
fun OnboardingScreen( step: OnboardingStep, - currentStepIndex: Int, - totalSteps: Int, - isLastStep: Boolean, - primaryActionLabel: StringResource = - if (isLastStep) Res.string.button_start else Res.string.button_next, onSkip: () -> Unit, onNext: () -> Unit, modifier: Modifier = Modifier, ) { @@ - FuturisticProgressIndicator(currentStep = currentStepIndex, totalSteps = totalSteps) + FuturisticProgressIndicator( + currentStep = step.progressIndex, + totalSteps = step.totalSteps, + ) @@ - text = stringResource(primaryActionLabel), + text = stringResource(step.actionLabel),Also applies to: 248-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/onboarding/OnboardingScreen.kt` around lines 152 - 162, The OnboardingScreen function signature currently accepts redundant UI state (currentStepIndex, totalSteps, isLastStep, primaryActionLabel) causing a LongParameterList; instead derive UI values from the provided OnboardingStep (use step.progressIndex, step.totalSteps, step.actionLabel and compute isLastStep via step.progressIndex == step.totalSteps - 1) and remove the extra parameters (currentStepIndex, totalSteps, isLastStep, primaryActionLabel) from the OnboardingScreen declaration and all call sites; update internal uses to read from step.* (and keep onSkip, onNext, modifier) so the function is shorter and detekt-compliant.modules/agent-core-kmp/src/commonMain/kotlin/com/profiletailors/agent/core/CoreContracts.kt (1)
6-10:⚠️ Potential issue | 🟠 Major
CoreInvocationstill accepts raw session strings.That bypasses
SessionIdvalidation completely, so the new value class does not harden the public boundary yet. If UUID enforcement is intentional, the typed contract needs to start here.Minimal contract fix
data class CoreInvocation( val prompt: String, - val sessionId: String? = null, + val sessionId: SessionId? = null, val metadata: Map<String, String> = emptyMap(), val timeoutMs: Long? = null, )Also applies to: 15-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/agent-core-kmp/src/commonMain/kotlin/com/profiletailors/agent/core/CoreContracts.kt` around lines 6 - 10, CoreInvocation currently accepts a raw sessionId String which bypasses SessionId validation; change the sessionId property type on CoreInvocation from String? to SessionId? (and update any other data classes in the same file that take raw session strings) so the typed value class enforces UUID/validation at the public boundary; update call sites/constructors to wrap/parse strings into SessionId (or accept null) and adjust imports/serialization helpers if needed to compile.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (1)
149-157:⚠️ Potential issue | 🟡 MinorUse a UUID-shaped preview session ID.
The desktop preview snapshot now feeds
PreviewMobileRuntimeFacade, which wraps any non-blank ID inRuntimeSessionId."desktop-preview-session"will throw before the preview renders.Minimal fix
MobileBridgeSnapshot( runtimeAvailable = true, linkEstablished = true, sessionCapable = true, - sessionId = "desktop-preview-session", + sessionId = "550e8400-e29b-41d4-a716-000000000001", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt` around lines 149 - 157, The preview snapshot uses a plain string "desktop-preview-session" which RuntimeSessionId rejects; update defaultPreviewBridgeSnapshotFor to provide a UUID-shaped sessionId (e.g. use UUID.randomUUID().toString()) so PreviewMobileRuntimeFacade accepts it—modify the MobileBridgeSnapshot construction in defaultPreviewBridgeSnapshotFor to set sessionId to a UUID string instead of the literal, referencing defaultPreviewBridgeSnapshotFor, MobileBridgeSnapshot, PreviewMobileRuntimeFacade and RuntimeSessionId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.kt`:
- Around line 7-17: readLinkedRuntimeMetadata currently treats a present
targetId as sufficient and lets toTransportMode()/toTrustMode() silently coerce
null/unknown strings to defaults, which breaks the fail-closed model; update
readLinkedRuntimeMetadata to early-return null unless transport and trust
strings exist and successfully parse to non-null values and linkedAtEpochMs is
not MISSING_LINKED_AT: read KEY_TRANSPORT_MODE and KEY_TRUST_MODE into local
variables, if either is null or their toTransportMode()/toTrustMode() call
returns an unrecognized/null result then return null; also validate
sharedPreferences.getLong(KEY_LINKED_AT_EPOCH_MS, MISSING_LINKED_AT) !=
MISSING_LINKED_AT before constructing LinkedRuntimeMetadata so partial/stale
prefs never produce a trusted linked runtime.
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt`:
- Around line 17-22: The RuntimeCapabilities advertises approvalRequests = true
but the AndroidRuntimeBridge parser currently only produces AssistantMessage or
Failure; update the bridge's parsing logic so that when an approval payload is
detected it emits RuntimeEvent.ApprovalPending (not just AssistantMessage), e.g.
modify the response/event parsing functions in AndroidRuntimeBridge to recognize
approval-specific markers and return RuntimeEvent.ApprovalPending with the
approval details, and ensure any coordinator consumers handle
RuntimeEvent.ApprovalPending the same way they handle other events so the
approval card can be surfaced.
- Around line 77-79: The endSession method currently ignores a null return from
runCommand, causing MobileRuntimeCoordinator.endActiveSession() to treat
failures as success; update AndroidRuntimeBridge.endSession to check the result
of runCommand("__corvus_end_session__${sessionId.value}") and propagate failure
by throwing an exception (or returning/forwarding an error) when runCommand
returns null or indicates failure, including the sessionId and any error info in
the thrown exception so the MobileRuntimeCoordinator can detect and handle the
failure instead of clearing local state.
- Around line 104-128: The current code waits on process.waitFor(...) before
reading process.inputStream which can deadlock if the child fills the pipe;
change the flow in AndroidRuntimeBridge: start a background reader thread or
submit a Callable/Future that consumes process.inputStream.bufferedReader().use
{ it.readText() } immediately after starting the ProcessBuilder (before calling
waitFor), store the result into a String (or StringBuilder) and then call
process.waitFor(config.defaultTimeoutMs, ...); after waitFor returns join/get
the reader result (with timeout/interrupt handling), ensure the reader closes
streams and handle process.destroyForcibly() as before, and only return the
collected trimmed output if exitValue()==0 and the reader completed
successfully.
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt`:
- Around line 14-30: createAndroidRuntimeFacade currently always returns
FailClosedRuntimeFacade when initialBridgeSnapshot is null, which prevents the
app from reading persisted connection metadata and wiring AndroidRuntimeBridge;
update createAndroidRuntimeFacade to, when initialBridgeSnapshot is null,
attempt to load persisted connection/endpoint metadata and, if present and
valid, construct and return the appropriate runtime that wires
AndroidRuntimeBridge (or AndroidRuntimeFacade) instead of immediately returning
FailClosedRuntimeFacade; keep the PreviewMobileRuntimeFacade branch for non-null
initialBridgeSnapshot and fall back to FailClosedRuntimeFacade only if no
persisted connection can be loaded or validation fails, referencing
createAndroidRuntimeFacade, PreviewMobileRuntimeFacade, FailClosedRuntimeFacade,
and AndroidRuntimeBridge to locate the code to change.
In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt`:
- Around line 112-122: AppChatContent currently takes coordinatorState plus
eight separate callbacks causing a LongParameterList; replace this with a single
bindings holder (e.g., AppChatBindings) that contains the coordinatorState and
all callbacks as properties, then change AppChatContent signature to accept that
single bindings object and update all call sites to construct/pass the bindings
instance; update any usages inside AppChatContent to reference
bindings.coordinatorState and bindings.onSendMessage etc., and add the new
bindings data class (or value class) near related UI code so future changes to
ChatWorkspace only require adjusting the bindings type.
- Around line 49-53: The onboarding gate currently uses platform.isMobile which
causes desktop to skip onboarding; update the remember block for
shouldShowOnboarding (referencing shouldShowOnboarding, platform.isMobile,
onboardingState.status, MobileOnboardingStatus.SESSION_READY, and
AppChatContent) to remove the platform.isMobile guard so the UI shows onboarding
whenever onboardingState.status != MobileOnboardingStatus.SESSION_READY (and
additionally include any existing "target not configured" check if your runtime
has that flag) — i.e., stop gating onboarding on platform type so desktop
follows the client-first onboarding flow.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt`:
- Around line 46-84: The current logic clears persistedSessionId inside the
recoveryOverride when persistedSessionId != null && activeSessionId == null, but
because sessionLookup may be a failure the code can erase resume state on
transient listResumableSessions() errors; change the condition/order to avoid
clearing the persisted ID when sessionLookup.isFailure (or check
sessionLookup.isSuccess and that sessions do not contain persistedSessionId)
before calling persistence.clearActiveSessionId(), i.e. update the
recoveryOverride branch that calls persistence.clearActiveSessionId() (and the
activeSessionId decision) to first handle sessionLookup.isFailure and only clear
the persisted id when the session lookup succeeded and the persisted id is truly
absent.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/RuntimeContracts.kt`:
- Around line 249-268: The MobileRuntimeFacade interface currently defines only
synchronous methods (probeReadiness, createSession, listResumableSessions,
resumeSession, endSession, sendMessage, submitApproval) which blocks the UI;
change these method signatures to suspend functions and update all
implementations (notably AndroidRuntimeBridge) to perform blocking work inside
withContext(Dispatchers.IO) or a dedicated IO dispatcher (replace direct
Process.waitFor and blocking I/O calls with coroutine-dispatched equivalents),
and update all call sites (e.g., App.kt Compose callbacks and LaunchedEffect
usages) to call the now-suspending functions from coroutines so RuntimeSession,
RuntimeTurnResult, RuntimeReadinessSnapshot flows remain intact without blocking
the main thread.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatComponents.kt`:
- Around line 564-579: The TRUST_PENDING copy is hard-coded to "runtime
endpoint" but must vary by onboarding method; update the strings in both the
status mapping and bridgeStateDescription by checking the onboarding method on
MobileBridgeUiState (e.g., inspect bridgeState.onboardingState.selectedMethod or
onboardingState.supportedMethods/trustMethod) and return method-specific copy
(e.g., if companion/trusted companion use neutral or "Connect to companion
device", if runtime use "runtime endpoint") with a neutral fallback like
"Configure runtime connection" to avoid directing companion flows to an
endpoint.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.kt`:
- Around line 126-141: The ChatWorkspace function signature is exposing many
individual fields that duplicate ChatUiState and BridgeActions; replace the long
parameter list with a single state model (e.g., ChatUiState) and a single
actions interface/object (e.g., BridgeActions) so the composable consumes state
and invokes actions instead of many callbacks; update ChatWorkspace and the
other similar function (the one at lines ~280-286) to accept those models,
adjust call sites to pass the panel/state and actions objects, and remove the
now-redundant individual parameters to eliminate duplication and satisfy Detekt.
- Around line 167-192: The remember block for creating ChatWorkspaceActions
currently omits bridgeState and onSendMessage from its dependency list, so the
captured ::sendMessage closure can become stale; update the remember(...) call
to include bridgeState and onSendMessage along with the existing dependencies so
ChatWorkspaceActions (and its onSend = ::sendMessage) are recreated when bridge
readiness or the send callback changes, ensuring sendMessage sees the current
bridgeState and onSendMessage.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.kt`:
- Around line 30-34: ConfigPanel currently has too many parameters and
responsibilities; create a small UI state data class (e.g., ConfigPanelState)
that groups runtime/session inputs (bridgeState: MobileBridgeUiState,
resumableSessions: List<RuntimeSession>, activeSessionId, targetLabel) and
replace those parameters with a single state object in the ConfigPanel
signature, then extract the diagnostics section and the actions section into two
smaller composables (e.g., ConfigDiagnostics and ConfigActions) and move related
logic from ConfigPanel into them to reduce long method and long parameter list
violations; update callers to construct the new ConfigPanelState and pass it to
ConfigPanel, and ensure ConfigDiagnostics and ConfigActions accept only the
subset of fields they need from ConfigPanelState.
- Around line 104-117: The "Runtime target" card currently falls back to
bridgeState.platformName and the safe diagnostics hardcode "Transport: Runtime
endpoint" and a 30s timeout which misrepresents unconfigured clients; update the
diagnosticsCard usage (where targetLabel is read and where
buildSafeDiagnosticLines is called) so that if targetLabel == null the subtitle
becomes "Not configured" (instead of bridgeState.platformName) and the details
generation uses the actual bridgeState.connectionMethod (and omits or adjusts
the timeout text unless a runtime endpoint is configured). Also apply the same
change to the other occurrence noted (the block around lines 218-226) so both
cards show "Not configured" and the real connectionMethod when no targetLabel
exists.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/onboarding/OnboardingScreen.kt`:
- Line 135: The mapping for MobileOnboardingStatus.SESSION_READY returns
OnboardingDefaults.steps[3] which still has status = SESSION_PENDING; change the
mapping so SESSION_READY returns a step instance with its status set to
MobileOnboardingStatus.SESSION_READY (e.g. create or copy
OnboardingDefaults.steps[3] with status = MobileOnboardingStatus.SESSION_READY)
so the returned step accurately reflects the ready state.
In
`@clients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/ComposeAppCommonTest.kt`:
- Around line 264-278: The test should enforce the client-first contract by
asserting the expected client-first values instead of legacy defaults: update
the test function `should persist linked metadata with shared transport and
trust defaults` (and similarly the test around lines 497-514) to read the
persisted metadata via
`FakeMobileRuntimePersistence.readLinkedRuntimeMetadata()` and assert that
`linkedMetadata?.transportMode` and `linkedMetadata?.trustMode` match the
client-first enums (not
`RuntimeTransportMode.CLI_BRIDGE`/`RuntimeTrustMode.BRIDGE_LINKED`), and ensure
`MobileRuntimeCoordinator` is constructed/triggered the same way so the
coordinator behavior is validated; keep assertions for `targetId`
(`"mobile-runtime"`) and replace the transport/trust assertions with the
client-first expected enum values to prevent accepting legacy defaults.
In
`@clients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimePackagingCommonTest.kt`:
- Around line 51-58: The helper selectPackagedRuntimeExecutableForTest currently
uses multiple early returns causing a Detekt ReturnCount violation; collapse it
into a single expression by computing a trimmed non-empty directory and the
first matching executable from preferredFileNames (using runtimeDirectory,
availableEntries, preferredFileNames) in one chained expression and return
either the joined path "$directory/$executableName" or null without multiple
return statements; update the function body to a single-expression return that
produces null when no directory or no executable is found.
In
`@clients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/ui/chat/ClientFirstCopyTest.kt`:
- Around line 59-60: The test computes description using
bridgeStateDescription(bridgeState) but never asserts it; add an assertion that
compares the description variable to the expected description string/value in
the ClientFirstCopyTest.kt test (alongside the existing headline assertion).
Locate where headline = bridgeStateHeadline(bridgeState) and description =
bridgeStateDescription(bridgeState) are defined and add an assertion (e.g.,
assertEquals or the project's assertion helper) for description to validate the
description path.
In
`@clients/composeApp/src/iosMain/kotlin/com/profiletailors/corvus/MainViewController.kt`:
- Around line 5-7: Suppress the Kotlin lint rule FunctionNaming on the iOS entry
point by adding the `@Suppress`("FunctionNaming") annotation to the
MainViewController declaration so the PascalCase name used for UIKit interop is
allowed (similar to AppAndroidPreview); locate the MainViewController function
and apply `@Suppress`("FunctionNaming") immediately above it to silence the lint
for this platform-specific entry point.
In
`@clients/composeApp/src/iosMain/kotlin/com/profiletailors/corvus/runtime/IosPersistence.kt`:
- Around line 32-33: The readActiveSessionId() implementation should guard
against invalid persisted values by returning null if RuntimeSessionId
construction/parsing fails; wrap the call that converts
defaults.stringForKey(KEY_ACTIVE_SESSION_ID) into RuntimeSessionId in a safe
parse (e.g., runCatching or try/catch) and return null on exception or invalid
format so corrupted or manually edited storage cannot throw on startup; update
the readActiveSessionId() method accordingly and reference RuntimeSessionId,
defaults.stringForKey, and KEY_ACTIVE_SESSION_ID when making the change.
- Line 13: The stored Long is being retrieved with a Kotlin cast that returns
null because defaults.objectForKey(KEY_LINKED_AT_EPOCH_MS) returns an NSNumber
on iOS; update the linkedAtEpochMs assignment to first cast the result of
defaults.objectForKey(KEY_LINKED_AT_EPOCH_MS) to NSNumber and read its long
value (e.g., via longLongValue) so the timestamp persists (mirror the approach
used in AndroidPersistence).
In
`@clients/composeApp/src/jvmMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.jvm.kt`:
- Around line 77-97: The JVM build currently reports ENDPOINT_URL and
LOCAL_HOST_ADVANCED in jvmSupportedConnectionMethods even though the non-preview
path builds a plain JvmRuntimeFacade with InMemoryMobileRuntimePersistence and
therefore cannot persist or actually execute those connection methods; remove
those two methods from jvmSupportedConnectionMethods so the advertised
platformSupportedConnectionMethods reflect what JvmRuntimeFacade actually
supports, and instead surface/add those advanced methods only when creating a
PreviewMobileRuntimeFacade (via createJvmRuntimeFacade(initialBridgeSnapshot) /
PreviewMobileRuntimeFacade) so PlatformRuntimeDependencies constructed for the
preview path includes the extra capabilities while the default JvmRuntimeFacade
path does not.
In `@Makefile`:
- Line 103: The Makefile target invokes a root-level Gradle task
`:cleanAllWebApps` which is inconsistent with other calls that use the project
variable; update the invocation at `@$(GRADLEW) :cleanAllWebApps` to call the
module-scoped task using the same variable, i.e. change it to `@$(GRADLEW)
$(WEB_MODULE):cleanAllWebApps` so it matches the other uses of
`$(WEB_MODULE):cleanAllWebApps` and avoids failing when no root alias exists.
In
`@modules/agent-core-kmp/src/androidMain/kotlin/com/profiletailors/agent/core/AndroidRuntimeBridge.kt`:
- Around line 17-23: The class-level default MobileRuntimeCapabilities in
AndroidRuntimeBridge exposes resumableSessionList and approvalRequests as true
even when __corvus_probe__ fails; change the probe/fallback behavior so that
when probe/readiness is false the returned capabilities are an empty/zeroed
MobileRuntimeCapabilities (all flags false) instead of using the class default.
Update the code paths that build the probe snapshot (the probe function and the
property override of capabilities in AndroidRuntimeBridge, and the similar logic
around lines 48-53) to return a zeroed MobileRuntimeCapabilities on
failure/readiness=false so UI features aren’t enabled until a successful probe.
- Around line 76-78: endSession currently calls
runCommand("__corvus_end_session__${sessionId.value}") and ignores its result;
change it to capture the return value from runCommand, check for null (and any
non-success indicator if the return type exposes one), and only consider the
session ended when that result indicates success—if runCommand returns null or a
failure, surface the error (e.g. throw, return/propagate an error, or log and
avoid clearing local session state) so the client isn't misled; update the
override of endSession (and any callers) to handle the failure case accordingly.
- Around line 96-127: The runCommand function currently calls
process.waitFor(...) before draining output which can deadlock if the child
fills the OS pipe; start background readers to continuously consume
process.inputStream (and process.errorStream if not already merged) as soon as
the process is started (the ProcessBuilder already sets
redirectErrorStream(true) so at least read process.inputStream) into a
buffer/string in a separate thread or coroutine, then call
process.waitFor(config.defaultTimeoutMs, TimeUnit.MILLISECONDS), join/await the
reader(s) so all output is captured, and only then inspect exitValue() and
return the buffered trimmed text; ensure the reader threads are started right
after process.start() and are stopped/cleaned up on timeout or forced destroy.
In
`@modules/agent-core-kmp/src/commonMain/kotlin/com/profiletailors/agent/core/MobileRuntimeFacade.kt`:
- Around line 6-22: The MobileRuntimeFacade interface has several methods that
perform blocking IO and must be converted to suspend to avoid main-thread
blocking; update MobileRuntimeFacade so that probeReadiness(),
createSession(metadata: Map<String, String> = emptyMap()),
listResumableSessions(), resumeSession(sessionId: SessionId),
endSession(sessionId: SessionId), sendMessage(sessionId: SessionId, prompt:
String), and submitApproval(requestId: String, decision: MobileApprovalDecision,
sessionId: SessionId) are declared as suspend functions, and then update all
implementations (and any callers such as places invoking these methods from
LaunchedEffect) to use coroutine scopes/dispatchers as needed so the blocking
ProcessBuilder.waitFor()/delegated I/O runs off the UI thread.
In
`@modules/agent-core-kmp/src/commonMain/kotlin/com/profiletailors/agent/core/MobileRuntimePersistence.kt`:
- Around line 3-15: AndroidPersistence currently writes sensitive values
(KEY_ACTIVE_SESSION_ID and auth-related fields saved by
saveActiveSessionId/saveLinkedRuntimeMetadata/readActiveSessionId/readLinkedRuntimeMetadata)
to plain SharedPreferences; replace that storage with EncryptedSharedPreferences
from AndroidX Security. Initialize a MasterKey
(MasterKey.Builder(...).setKeyScheme(MasterKey.KeyScheme.AES256_GCM).build())
and create EncryptedSharedPreferences via
EncryptedSharedPreferences.create(...), then use that instance wherever
AndroidPersistence currently uses SharedPreferences for the session ID and
linked runtime metadata keys; preserve the same preference keys (e.g.,
KEY_ACTIVE_SESSION_ID) and null semantics, and ensure reads/writes call the
encrypted prefs instance. Ensure imports and gradle deps for
androidx.security.crypto are added.
In
`@modules/agent-core-kmp/src/jvmMain/kotlin/com/profiletailors/agent/core/RustCliBridge.kt`:
- Around line 102-143: The separator-based wire format is fragile: replace raw
delimiters with a structured encoding (e.g., JSON or Base64-encoded payload) for
all fields sent and received so arbitrary characters don't break parsing. Update
sendMessage and submitApproval to encode their concatenated payloads (encode
prompt, requestId, decision.name, sessionId.value) before passing to
runTurnCommand/runMobileCommand, and update listResumableSessions (and
parseSessionResult) to decode the encoded session payloads returned by
runMobileCommand instead of splitting on '|' or U+001F; locate these changes
around the methods listResumableSessions, sendMessage, submitApproval,
runTurnCommand/runMobileCommand and parseSessionResult to ensure
encoding/decoding is symmetric. Ensure require/error messages still validate
decoded structure and handle decode failures gracefully.
- Around line 145-175: runMobileCommand and runTurnCommand call
invoke(CoreInvocation) which currently waits for the child process to exit
before stdout is drained, causing deadlocks on large output; update the bridge
so invoke streams/drains the child process stdout while the process is running
(or provide a streaming variant of invoke) so that output is consumed
continuously instead of waiting for exit, then have runMobileCommand and
runTurnCommand use that streaming behavior (referencing invoke, CoreInvocation,
CoreResult, runMobileCommand, runTurnCommand, and parseTurnResult) to avoid
pipe-buffer blocking.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/archive-report.md`:
- Around line 35-48: The markdown is missing required blank lines before the
subheadings; add a single blank line before the "Onboarding Spec Changes"
heading and another blank line before the "Client-Surfaces Spec Changes" heading
so each heading is separated from the preceding content per MD022; locate these
headings by their exact text ("Onboarding Spec Changes" and "Client-Surfaces
Spec Changes") in archive-report.md and insert one empty line immediately above
each.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/design.md`:
- Around line 29-35: The archived design text incorrectly asserts that the
shared readiness contract no longer defaults to CLI_BRIDGE/BRIDGE_LINKED and
that persisted metadata reflects the selected client method, but the Android
persistence and common tests still fall back to those legacy modes; update the
wording in this file to either state that the archive reflects intended design
but implementation still falls back to CLI_BRIDGE/BRIDGE_LINKED (marking the
divergence as follow-up work) or soften the claim to match shipped behavior, and
add a short note referencing the relevant implementation areas
(RuntimeContracts.kt, CoreContracts.kt and enums
RuntimeTransportMode/RuntimeTrustMode) so readers know tests and persistence
still assume legacy modes until a follow-up change.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/exploration.md`:
- Around line 1-6: Change the second-level heading "## Exploration: Re-evaluate
mobile runtime parity under client-first product direction" to a top-level
heading by replacing the leading "##" with "#" (i.e., update the heading
"Exploration: Re-evaluate mobile runtime parity under client-first product
direction"), and ensure the file ends with a single trailing newline character
so the document satisfies markdownlint MD041 and MD047.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/proposal.md`:
- Around line 11-23: Add a blank line before the "In Scope" and "Out of Scope"
headings so each heading is surrounded by blank lines per markdownlint MD022;
locate the headings "In Scope" and "Out of Scope" in the proposal.md content and
insert an empty line directly above each heading line to ensure proper markdown
spacing.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/smoke-validation-report.md`:
- Around line 17-25: The MD022 lint failure is due to missing blank lines after
the "#### Android" and "#### iOS" headings in smoke-validation-report.md; edit
the document to insert an empty line immediately after each "#### Android" and
"#### iOS" heading so the list blocks that follow are separated by a blank line,
then re-run the markdown linter to confirm MD022 is resolved.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/tasks.md`:
- Around line 5-12: The markdown headings "Invalidated prior work" and "Reusable
completed work" need blank lines above them to satisfy MD022; edit the tasks.md
content and insert a single blank line immediately before the heading
"Invalidated prior work" and another blank line immediately before the heading
"Reusable completed work" so each heading is separated from the preceding
content by one blank line.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/verify-report.md`:
- Around line 1-7: Change the document's first heading from "## Verification
Report" to a top-level heading by replacing "## Verification Report" with "#
Verification Report" so the file's first line is a level-1 heading (per
markdownlint MD041); locate the heading text "Verification Report" in the file
(verify-report.md) and update that line only.
- Around line 50-84: Add language specifiers to the fenced code blocks in
verify-report.md: update the block that begins with "```" followed by "Command:"
to use a language (e.g., ```text or ```bash) and do the same for the block that
begins with "```" followed by "Commands:" (and any other unlabeled fences such
as the XML summary block) so markdownlint MD040 is satisfied; locate the
triple-backtick fences surrounding the "Command:" output and the "Commands:"/XML
summary output and prepend the appropriate language token (text or bash for
shell output, xml for XML snippets).
In `@openspec/specs/client-surfaces/spec.md`:
- Around line 109-125: The document currently mixes new client-first rules for
composeApp with legacy CLI-bridge/mobile-parity rules; update this single file
so composeApp is consistently defined as client-first by removing or reconciling
the legacy sections named "registry", "transport", "onboarding-alignment",
"mobile-web parity", "CLI-bridge output", and "background-session" (and the
content currently spanning lines roughly 187-289) so they no longer mandate
CLI-bridge behaviors (chat/session/approval) for composeApp; ensure the
remaining text (the composeApp bullet list and any "composeApp client surfaces"
statements) clearly specify approved transports, onboarding readiness
validation, and allowed connection paths (runtime URL/endpoint, pairing/trusted
companion as applicable) and verify markdown headings and references are updated
to avoid conflicting rules.
In `@openspec/specs/onboarding/spec.md`:
- Around line 75-84: The docs currently define conflicting onboarding contracts
for composeApp (desktop/Android/iOS) — reconcile the canonical onboarding
sequence and the newer readiness-based mobile completion/transport sections so
there is one authoritative onboarding contract: update the composeApp onboarding
text to state a single supported flow (e.g., readiness-based client onboarding
that supports connecting to an existing runtime by URL/endpoint as the default
for this milestone), remove or edit any language that still requires "create or
resume a first session" before onboarding completion if that contradicts the new
mobile contracts, and align mobile-specific guidance (pairing/trusted companion,
iOS approved client path, and prohibition of local runtime as default) across
the affected paragraphs (notably the original canonical sequence and the mobile
sections referenced around lines 142-164) so the doc is technically consistent
with the code changes.
In `@pnpm-workspace.yaml`:
- Around line 6-32: The root catalog lists happy-dom@20.8.8 while the
clients/web catalog uses 20.8.9; update the root catalog entry for "happy-dom"
to match 20.8.9 (or harmonize both to the chosen version) and ensure both
workspace catalogs reference the identical version; additionally, remove
duplicate overlapping "catalog" definitions or consolidate them into a single
canonical catalog (or add a clear comment/docstring describing the intended
precedence) so the two catalog sections cannot drift in the future.
---
Outside diff comments:
In `@clients/composeApp/build.gradle.kts`:
- Around line 44-74: The unused private property warnings come from the "val
<name> by getting" delegations; replace those with getByName(...) calls to avoid
creating local vals. For each source set (commonMain, commonTest, jvmMain,
androidMain) change "val commonMain by getting { ... }" style to
"getByName(\"commonMain\") { ... }" (and similarly for "commonTest", "jvmMain",
"androidMain") or "getByName(\"name\").apply { ... }" so you configure the
source sets without introducing unused local variables; use the getByName
function to locate and modify the source set blocks.
In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt`:
- Around line 149-157: The preview snapshot uses a plain string
"desktop-preview-session" which RuntimeSessionId rejects; update
defaultPreviewBridgeSnapshotFor to provide a UUID-shaped sessionId (e.g. use
UUID.randomUUID().toString()) so PreviewMobileRuntimeFacade accepts it—modify
the MobileBridgeSnapshot construction in defaultPreviewBridgeSnapshotFor to set
sessionId to a UUID string instead of the literal, referencing
defaultPreviewBridgeSnapshotFor, MobileBridgeSnapshot,
PreviewMobileRuntimeFacade and RuntimeSessionId.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/onboarding/OnboardingScreen.kt`:
- Around line 152-162: The OnboardingScreen function signature currently accepts
redundant UI state (currentStepIndex, totalSteps, isLastStep,
primaryActionLabel) causing a LongParameterList; instead derive UI values from
the provided OnboardingStep (use step.progressIndex, step.totalSteps,
step.actionLabel and compute isLastStep via step.progressIndex ==
step.totalSteps - 1) and remove the extra parameters (currentStepIndex,
totalSteps, isLastStep, primaryActionLabel) from the OnboardingScreen
declaration and all call sites; update internal uses to read from step.* (and
keep onSkip, onNext, modifier) so the function is shorter and detekt-compliant.
In
`@modules/agent-core-kmp/src/commonMain/kotlin/com/profiletailors/agent/core/CoreContracts.kt`:
- Around line 6-10: CoreInvocation currently accepts a raw sessionId String
which bypasses SessionId validation; change the sessionId property type on
CoreInvocation from String? to SessionId? (and update any other data classes in
the same file that take raw session strings) so the typed value class enforces
UUID/validation at the public boundary; update call sites/constructors to
wrap/parse strings into SessionId (or accept null) and adjust
imports/serialization helpers if needed to compile.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21afac85-a8ea-448c-9f62-878db7a4bd09
⛔ Files ignored due to path filters (3)
openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/android-launch.pngis excluded by!**/*.pngopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/ios-launch.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (54)
Makefileclients/androidApp/build.gradle.ktsclients/composeApp/build.gradle.ktsclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/MainActivity.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/IosRuntimeCompanionDiagnostics.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/RuntimeContracts.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatComponents.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ChatWorkspace.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanel.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/chat/MobileOnboardingModels.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/ui/onboarding/OnboardingScreen.ktclients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/ComposeAppCommonTest.ktclients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/OnboardingDefaultsTest.ktclients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimePackagingCommonTest.ktclients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependenciesCommonTest.ktclients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/ui/chat/ClientFirstCopyTest.ktclients/composeApp/src/commonTest/kotlin/com/profiletailors/corvus/ui/chat/ConfigPanelTest.ktclients/composeApp/src/iosMain/kotlin/com/profiletailors/corvus/MainViewController.ktclients/composeApp/src/iosMain/kotlin/com/profiletailors/corvus/runtime/IosPersistence.ktclients/composeApp/src/iosMain/kotlin/com/profiletailors/corvus/runtime/IosRuntimeBridge.ktclients/composeApp/src/iosMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.ios.ktclients/composeApp/src/jvmMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.jvm.ktclients/composeApp/src/jvmTest/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependenciesJvmTest.ktclients/web/package.jsonclients/web/pnpm-workspace.yamlmodules/agent-core-kmp/src/androidMain/kotlin/com/profiletailors/agent/core/AndroidRuntimeBridge.ktmodules/agent-core-kmp/src/commonMain/kotlin/com/profiletailors/agent/core/CoreContracts.ktmodules/agent-core-kmp/src/commonMain/kotlin/com/profiletailors/agent/core/MobileRuntimeFacade.ktmodules/agent-core-kmp/src/commonMain/kotlin/com/profiletailors/agent/core/MobileRuntimePersistence.ktmodules/agent-core-kmp/src/commonTest/kotlin/com/profiletailors/agent/core/AgentKernelTest.ktmodules/agent-core-kmp/src/commonTest/kotlin/com/profiletailors/agent/core/CoreContractsTest.ktmodules/agent-core-kmp/src/iosMain/kotlin/com/profiletailors/agent/core/IosRuntimeBridge.ktmodules/agent-core-kmp/src/jvmMain/kotlin/com/profiletailors/agent/core/RustCliBridge.ktmodules/agent-core-kmp/src/jvmTest/kotlin/com/profiletailors/agent/core/RustCliBridgeTest.ktopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/archive-report.mdopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/design.mdopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/exploration.mdopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/proposal.mdopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/smoke-validation-report.mdopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/specs/client-surfaces/spec.mdopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/specs/onboarding/spec.mdopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/state.yamlopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/tasks.mdopenspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/verify-report.mdopenspec/specs/client-surfaces/spec.mdopenspec/specs/onboarding/spec.mdpackage.jsonpnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- clients/web/package.json
Comment on lines
+1
to
+7
| ## Verification Report | ||
|
|
||
| **Change**: 2026-03-29-mobile-runtime-parity-requirements | ||
| **Version**: N/A | ||
| **Verification Date**: 2026-03-30 (final) | ||
|
|
||
| --- |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add top-level heading.
Per markdownlint MD041, the first line should be a top-level heading (#), not ##.
📝 Suggested fix
-## Verification Report
+# Verification Report📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ## Verification Report | |
| **Change**: 2026-03-29-mobile-runtime-parity-requirements | |
| **Version**: N/A | |
| **Verification Date**: 2026-03-30 (final) | |
| --- | |
| # Verification Report | |
| **Change**: 2026-03-29-mobile-runtime-parity-requirements | |
| **Version**: N/A | |
| **Verification Date**: 2026-03-30 (final) | |
| --- |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/verify-report.md`
around lines 1 - 7, Change the document's first heading from "## Verification
Report" to a top-level heading by replacing "## Verification Report" with "#
Verification Report" so the file's first line is a level-1 heading (per
markdownlint MD041); locate the heading text "Verification Report" in the file
(verify-report.md) and update that line only.
Comment on lines
+50
to
+84
| ``` | ||
| Command: | ||
| - bash ./scripts/gradlew.sh :agent-core-kmp:build :composeApp:build | ||
|
|
||
| Result: | ||
| - :agent-core-kmp:build SUCCESS | ||
| - :composeApp:build SUCCESS | ||
|
|
||
| All spotlessKotlinCheck violations have been fixed with './gradlew spotlessApply'. | ||
| ``` | ||
|
|
||
| **Tests**: ✅ 75 passed / ❌ 0 failed / ⚠️ 0 skipped | ||
| ``` | ||
| Commands: | ||
| - bash ./scripts/gradlew.sh :agent-core-kmp:jvmTest | ||
| - bash ./scripts/gradlew.sh :composeApp:jvmTest | ||
|
|
||
| XML summary: | ||
| - Total tests: 75 | ||
| - Failures: 0 | ||
| - Errors: 0 | ||
| - Skipped tests in XML reports: 0 | ||
|
|
||
| Suite inventory: | ||
| AgentKernelTest: 3 tests | ||
| CoreContractsTest: 15 tests | ||
| RustCliBridgeTest: 16 tests | ||
| ComposeAppCommonTest: 15 tests | ||
| OnboardingDefaultsTest: 9 tests | ||
| AndroidRuntimePackagingCommonTest: 3 tests | ||
| PlatformRuntimeDependenciesCommonTest: 4 tests | ||
| PlatformRuntimeDependenciesJvmTest: 2 tests | ||
| ClientFirstCopyTest: 6 tests | ||
| ConfigPanelTest: 2 tests | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language specifiers to code fences.
Per markdownlint MD040, fenced code blocks should have a language specified. Use text or bash for the build/test output blocks.
📝 Suggested fix
-```
+```text
Command:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| Command: | |
| - bash ./scripts/gradlew.sh :agent-core-kmp:build :composeApp:build | |
| Result: | |
| - :agent-core-kmp:build SUCCESS | |
| - :composeApp:build SUCCESS | |
| All spotlessKotlinCheck violations have been fixed with './gradlew spotlessApply'. | |
| ``` | |
| **Tests**: ✅ 75 passed / ❌ 0 failed / ⚠️ 0 skipped | |
| ``` | |
| Commands: | |
| - bash ./scripts/gradlew.sh :agent-core-kmp:jvmTest | |
| - bash ./scripts/gradlew.sh :composeApp:jvmTest | |
| XML summary: | |
| - Total tests: 75 | |
| - Failures: 0 | |
| - Errors: 0 | |
| - Skipped tests in XML reports: 0 | |
| Suite inventory: | |
| AgentKernelTest: 3 tests | |
| CoreContractsTest: 15 tests | |
| RustCliBridgeTest: 16 tests | |
| ComposeAppCommonTest: 15 tests | |
| OnboardingDefaultsTest: 9 tests | |
| AndroidRuntimePackagingCommonTest: 3 tests | |
| PlatformRuntimeDependenciesCommonTest: 4 tests | |
| PlatformRuntimeDependenciesJvmTest: 2 tests | |
| ClientFirstCopyTest: 6 tests | |
| ConfigPanelTest: 2 tests | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 50-50: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 62-62: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 62-62: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/archive/2026-03-30-mobile-runtime-parity-requirements/verify-report.md`
around lines 50 - 84, Add language specifiers to the fenced code blocks in
verify-report.md: update the block that begins with "```" followed by "Command:"
to use a language (e.g., ```text or ```bash) and do the same for the block that
begins with "```" followed by "Commands:" (and any other unlabeled fences such
as the XML summary block) so markdownlint MD040 is satisfied; locate the
triple-backtick fences surrounding the "Command:" output and the "Commands:"/XML
summary output and prepend the appropriate language token (text or bash for
shell output, xml for XML snippets).
Comment on lines
+109
to
+125
| Each surface MUST use exactly one approved transport for all runtime communication, and its onboarding | ||
| flow MUST validate readiness only through that approved transport. | ||
|
|
||
| For the composeApp client surfaces in this milestone: | ||
| - Desktop MUST be treated as a client-first surface. | ||
| - Android MUST be treated as a client-first surface. | ||
| - iOS MUST be treated as a client-first surface. | ||
| - Desktop and Android MUST NOT assume a locally installed `corvus` binary, packaged executable, or | ||
| immediate local process execution as the default path. | ||
| - iOS MUST NOT imply that local `corvus` execution is the expected default path. | ||
| - Desktop MUST support connecting to an existing runtime through runtime URL or endpoint | ||
| configuration. | ||
| - Android MUST support connecting to an existing runtime through runtime URL or endpoint | ||
| configuration. | ||
| - iOS MUST expose only the approved client connection path or paths supported on iOS for this | ||
| milestone, which MAY include runtime URL or endpoint configuration and MAY include pairing or a | ||
| trusted companion flow. |
Contributor
There was a problem hiding this comment.
This spec is still internally split between client-first and CLI-bridge/mobile-parity rules.
The new milestone text makes composeApp client-first and explicitly drops chat/session/approval parity from acceptance, but the unchanged registry, transport, onboarding-alignment, mobile-web parity, CLI-bridge output, and background-session sections still define composeApp as a CLI-bridge surface with mandatory chat/session/approval behavior. Please update or remove the legacy sections in the same change so this file stays a single source of truth.
As per coding guidelines, **/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
Also applies to: 187-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/specs/client-surfaces/spec.md` around lines 109 - 125, The document
currently mixes new client-first rules for composeApp with legacy
CLI-bridge/mobile-parity rules; update this single file so composeApp is
consistently defined as client-first by removing or reconciling the legacy
sections named "registry", "transport", "onboarding-alignment", "mobile-web
parity", "CLI-bridge output", and "background-session" (and the content
currently spanning lines roughly 187-289) so they no longer mandate CLI-bridge
behaviors (chat/session/approval) for composeApp; ensure the remaining text (the
composeApp bullet list and any "composeApp client surfaces" statements) clearly
specify approved transports, onboarding readiness validation, and allowed
connection paths (runtime URL/endpoint, pairing/trusted companion as applicable)
and verify markdown headings and references are updated to avoid conflicting
rules.
Comment on lines
+75
to
+84
| - Desktop composeApp MUST treat connection to an existing runtime by URL or endpoint as a supported | ||
| default onboarding path for this milestone. | ||
| - Android composeApp MUST treat connection to an existing runtime by URL or endpoint as a supported | ||
| default onboarding path for this milestone. | ||
| - Desktop, Android, and iOS composeApp MUST guide pairing or trusted companion flows only on the | ||
| platforms where those flows are approved and supported in this milestone. | ||
| - iOS composeApp MUST guide the approved iOS client connection path for this milestone and MUST NOT | ||
| imply that local `corvus` execution is the default trust model. | ||
| - Desktop, Android, and iOS composeApp MUST NOT frame local binary installation, local process | ||
| spawning, or immediate local runtime execution as the normal onboarding step. |
Contributor
There was a problem hiding this comment.
Compose onboarding now has two different mobile completion/transport contracts.
These new sections move desktop/Android/iOS to readiness-based client onboarding, but the unchanged canonical sequence above still requires every chat surface to create or resume a first session before onboarding is complete, and the later mobile terminology/retry text still assumes bridge-only recovery. Please reconcile those older sections in the same change so composeApp has one authoritative onboarding contract.
As per coding guidelines, **/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
Also applies to: 142-149, 151-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/specs/onboarding/spec.md` around lines 75 - 84, The docs currently
define conflicting onboarding contracts for composeApp (desktop/Android/iOS) —
reconcile the canonical onboarding sequence and the newer readiness-based mobile
completion/transport sections so there is one authoritative onboarding contract:
update the composeApp onboarding text to state a single supported flow (e.g.,
readiness-based client onboarding that supports connecting to an existing
runtime by URL/endpoint as the default for this milestone), remove or edit any
language that still requires "create or resume a first session" before
onboarding completion if that contradicts the new mobile contracts, and align
mobile-specific guidance (pairing/trusted companion, iOS approved client path,
and prohibition of local runtime as default) across the affected paragraphs
(notably the original canonical sequence and the mobile sections referenced
around lines 142-164) so the doc is technically consistent with the code
changes.
Comment on lines
+6
to
+32
| catalog: | ||
| '@astrojs/check': 0.9.8 | ||
| '@astrojs/starlight': 0.38.2 | ||
| '@biomejs/biome': 2.4.9 | ||
| '@playwright/test': 1.58.2 | ||
| '@tailwindcss/postcss': 4.2.2 | ||
| '@tsconfig/node22': 22.0.5 | ||
| '@types/node': 25.5.0 | ||
| '@vitejs/plugin-vue': 6.0.5 | ||
| '@vitest/coverage-v8': 4.0.18 | ||
| '@vue/compiler-dom': 3.5.30 | ||
| '@vue/test-utils': 2.4.6 | ||
| '@vue/tsconfig': 0.9.1 | ||
| astro: 6.0.8 | ||
| astro-vtbot: 2.1.12 | ||
| happy-dom: 20.8.8 | ||
| portless: 0.6.0 | ||
| postcss: 8.5.8 | ||
| sharp: 0.34.5 | ||
| tailwind-merge: 3.5.0 | ||
| tailwindcss: 4.2.2 | ||
| typescript: 5.9.3 | ||
| vite: 6.4.1 | ||
| vitest: 4.0.18 | ||
| vue: 3.5.31 | ||
| vue-i18n: 11.3.0 | ||
| vue-tsc: 3.2.6 |
Contributor
There was a problem hiding this comment.
Version mismatch: happy-dom is 20.8.8 here but 20.8.9 in clients/web/pnpm-workspace.yaml.
This inconsistency could lead to unexpected behavior depending on which workspace resolution takes precedence. Align versions across both catalog definitions.
Additionally, maintaining two separate catalog sections (root and clients/web) with overlapping entries creates drift risk. Consider consolidating to a single source of truth or documenting the intended hierarchy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pnpm-workspace.yaml` around lines 6 - 32, The root catalog lists
happy-dom@20.8.8 while the clients/web catalog uses 20.8.9; update the root
catalog entry for "happy-dom" to match 20.8.9 (or harmonize both to the chosen
version) and ensure both workspace catalogs reference the identical version;
additionally, remove duplicate overlapping "catalog" definitions or consolidate
them into a single canonical catalog (or add a clear comment/docstring
describing the intended precedence) so the two catalog sections cannot drift in
the future.
Comment on lines
+249
to
+268
| interface MobileRuntimeFacade { | ||
| val capabilities: RuntimeCapabilities | ||
|
|
||
| fun probeReadiness(): RuntimeReadinessSnapshot | ||
|
|
||
| fun createSession(metadata: Map<String, String> = emptyMap()): RuntimeSession | ||
|
|
||
| fun listResumableSessions(): List<RuntimeSession> | ||
|
|
||
| fun resumeSession(sessionId: RuntimeSessionId): RuntimeSession | ||
|
|
||
| fun endSession(sessionId: RuntimeSessionId) | ||
|
|
||
| fun sendMessage(sessionId: RuntimeSessionId, prompt: String): RuntimeTurnResult | ||
|
|
||
| fun submitApproval( | ||
| requestId: String, | ||
| decision: RuntimeApprovalDecision, | ||
| sessionId: RuntimeSessionId, | ||
| ): RuntimeTurnResult |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the relevant files mentioned in the review
find . -type f -name "App.kt" | head -20Repository: dallay/corvus
Length of output: 132
🏁 Script executed:
# Find AndroidRuntimeBridge.kt
find . -type f -name "AndroidRuntimeBridge.kt"Repository: dallay/corvus
Length of output: 260
🏁 Script executed:
# Examine RuntimeContracts.kt to see full context and line numbers
wc -l clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/RuntimeContracts.ktRepository: dallay/corvus
Length of output: 155
🏁 Script executed:
# Get the exact content of MobileRuntimeFacade interface
sed -n '249,268p' clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/RuntimeContracts.ktRepository: dallay/corvus
Length of output: 649
🏁 Script executed:
# Check App.kt to see how it invokes MobileRuntimeFacade methods
wc -l ./clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktRepository: dallay/corvus
Length of output: 136
🏁 Script executed:
# Read App.kt to find Compose callback usage
cat -n ./clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktRepository: dallay/corvus
Length of output: 7531
🏁 Script executed:
# Check AndroidRuntimeBridge for blocking implementation
wc -l ./clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktRepository: dallay/corvus
Length of output: 162
🏁 Script executed:
# Read AndroidRuntimeBridge to check for Process.waitFor() and blocking I/O
cat -n ./clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktRepository: dallay/corvus
Length of output: 7091
🏁 Script executed:
# Check MobileRuntimeCoordinator to see if it wraps the facade calls
find . -type f -name "*RuntimeCoordinator*"Repository: dallay/corvus
Length of output: 161
🏁 Script executed:
# Look at how MobileRuntimeCoordinator calls the facade
fd -type f -name "MobileRuntimeCoordinator.kt" -exec cat -n {} \;Repository: dallay/corvus
Length of output: 227
🏁 Script executed:
# Get the MobileRuntimeCoordinator file
cat -n ./clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.ktRepository: dallay/corvus
Length of output: 15026
🏁 Script executed:
# Check if there are any suspend functions or async patterns in the runtime module
rg "suspend fun|async" clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/ -A 2Repository: dallay/corvus
Length of output: 39
🏁 Script executed:
# Double-check that no suspend variants exist for these facade methods
rg "suspend fun (probeReadiness|createSession|sendMessage|submitApproval|listResumableSessions|resumeSession|endSession)" clients/composeApp/src/commonMain/kotlin/Repository: dallay/corvus
Length of output: 39
MobileRuntimeFacade contract enforces synchronous blocking calls, causing UI stalls.
All methods in the interface—probeReadiness(), createSession(), sendMessage(), etc.—are synchronous with no suspend variants. AndroidRuntimeBridge implements these with Process.waitFor() and blocking I/O (line 118). App.kt invokes them directly from Compose callbacks and LaunchedEffect (lines 38–46, 68, 81–87), which blocks the main thread. Convert the facade methods to suspend functions and dispatch blocking I/O to an appropriate coroutine context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/RuntimeContracts.kt`
around lines 249 - 268, The MobileRuntimeFacade interface currently defines only
synchronous methods (probeReadiness, createSession, listResumableSessions,
resumeSession, endSession, sendMessage, submitApproval) which blocks the UI;
change these method signatures to suspend functions and update all
implementations (notably AndroidRuntimeBridge) to perform blocking work inside
withContext(Dispatchers.IO) or a dedicated IO dispatcher (replace direct
Process.waitFor and blocking I/O calls with coroutine-dispatched equivalents),
and update all call sites (e.g., App.kt Compose callbacks and LaunchedEffect
usages) to call the now-suspending functions from coroutines so RuntimeSession,
RuntimeTurnResult, RuntimeReadinessSnapshot flows remain intact without blocking
the main thread.
- AndroidPersistence: fail-closed on null transport/trust/linkedAt - AndroidRuntimeBridge: zeroed capabilities on probe failure, approval events, fixed endSession error propagation, background reader to avoid deadlock - PlatformRuntimeDependencies.android: load persisted metadata before fail-closed - App.kt: remove platform.isMobile gating for onboarding, UUID for preview session - MobileRuntimeCoordinator: clear persisted ID only when lookup succeeds DALLAY-179
- Add 'Each surface MUST use exactly one transport for all runtime communication' - Add Scenario: Web surfaces use HTTP Gateway (test expects this phrase) - Add Scenario: Mobile surfaces use CLI bridge (test expects this phrase) DALLAY-179
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (1)
112-143: 🛠️ Refactor suggestion | 🟠 MajorCollapse parameters into a bindings holder to fix Detekt
LongParameterList.This was flagged in a previous review. Extract a data class:
Suggested refactor
data class ChatBindings( val coordinatorState: MobileRuntimeCoordinatorState, val onRetryBridge: () -> Unit, val onLinkSurface: () -> Unit, val onStartSession: () -> Unit, val onResumeSession: (String) -> Unit, val onSendMessage: (String) -> Unit, val onDisconnectReset: () -> Unit, val onApprove: () -> Unit, val onDeny: () -> Unit, ) `@Composable` private fun AppChatContent(platform: Platform, bindings: ChatBindings) { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt` around lines 112 - 143, The AppChatContent composable has a long parameter list; collapse those parameters into a single bindings holder by introducing a data class ChatBindings that contains coordinatorState: MobileRuntimeCoordinatorState and all callback lambdas (onRetryBridge, onLinkSurface, onStartSession, onResumeSession, onSendMessage, onDisconnectReset, onApprove, onDeny), then change the signature of AppChatContent(platform: Platform, bindings: ChatBindings) and update the call sites to pass a ChatBindings instance while forwarding bindings.* into ChatWorkspace (e.g., use bindings.coordinatorState, bindings.onSendMessage, etc.) to satisfy Detekt LongParameterList.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.kt`:
- Around line 7-23: Extract the nullable parsing / fail-closed checks out of
readLinkedRuntimeMetadata into a small helper to satisfy Detekt ReturnCount:
create a private helper (e.g., parseLinkedRuntimeMetadataComponents or
buildLinkedRuntimeMetadataOrNull) that reads sharedPreferences for
KEY_TARGET_ID, KEY_TRANSPORT_MODE, KEY_TRUST_MODE and KEY_LINKED_AT_EPOCH_MS,
converts transportStr via toTransportMode() and trustStr via toTrustMode(), and
returns either a LinkedRuntimeMetadata or null if any value is missing/invalid
(using MISSING_LINKED_AT sentinel for the long). Then have
readLinkedRuntimeMetadata call that helper and return its result, removing the
multiple early returns from the original method.
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt`:
- Around line 119-170: Refactor runCommand(prompt: String) to reduce early
returns by extracting process startup and output-draining into helpers:
implement startProcess(prompt: String): Process? (wrap ProcessBuilder
construction and start in try/catch returning null on IOException) and
drainOutputWithTimeout(process: Process): String? (spawn reader thread, waitFor
with timeout, join reader, handle destroyForcibly and interrupts, and return the
collected output or null on timeout/read failure); then have runCommand call
startProcess, call drainOutputWithTimeout, and finally return the output only if
process.exitValue() == 0 (otherwise return null) so all failure paths funnel
through these helper results rather than multiple early returns in runCommand
itself.
- Around line 181-228: The function turnFrom currently returns multiple
RuntimeTurnResult instances in different branches; refactor to build a single
RuntimeEvent early and return one RuntimeTurnResult at the end. Specifically,
inside turnFrom compute an event variable using the existing checks (rawOutput
== null → RuntimeEvent.Failure(...), approval_request_id and approval_message
present → RuntimeEvent.ApprovalPending wrapping a RuntimeApprovalRequest,
otherwise → RuntimeEvent.AssistantMessage using
values["assistant_message"]?.takeIf { it.isNotBlank() } ?: fallback), then
return RuntimeTurnResult(sessionId = sessionId, events = listOf(event)). Keep
existing helpers like toValues() and the same field values but centralize the
single return.
- Around line 75-76: The requireNotNull call inside createSession that wraps
runCommand("__corvus_create_session__") currently throws a generic exception
when null; change it to provide a descriptive message (e.g.,
requireNotNull(runCommand("__corvus_create_session__")) {
"runCommand('__corvus_create_session__') returned null in
createSession(metadata=$metadata)" }) so the thrown IllegalArgumentException
includes context; apply the same pattern to the other similar occurrence (the
requireNotNull around runCommand in the other method at the same file) by adding
explicit messages that reference the invoked command name and the surrounding
function (e.g., sessionFrom / createSession) to aid debugging.
- Around line 140-153: The readerThread swallowing all exceptions makes failures
invisible; update the Thread's catch block in AndroidRuntimeBridge (the
readerThread that reads process.inputStream into outputBuilder) to log
exceptions instead of silently ignoring them—capture the caught Exception and
call the existing logger (or Android Log) at debug level with a clear message
and the exception, ensuring the thread still terminates gracefully if needed;
reference the readerThread, outputBuilder, and process.inputStream symbols when
locating and changing the catch block.
- Around line 96-101: In endSession, don't throw IllegalStateException directly;
call Kotlin's error(...) with the same failure message when
runCommand("__corvus_end_session__${sessionId.value}") returns null so the
runtimeUnavailable branch uses error() instead of throw; update the endSession
method (referencing endSession, RuntimeSessionId, and runCommand) to produce the
identical message via error("Failed to end session ${sessionId.value}: runtime
unavailable").
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt`:
- Around line 25-39: Replace the broad catch (e: Exception) around the
AndroidRuntimeBridge() construction with specific I/O-related exception handling
(e.g., catch java.io.IOException and any other expected runtime bridge
construction exceptions) so Detekt's TooGenericExceptionCaught is avoided; for
each specific catch, construct the same FailClosedRuntimeFacade using the caught
exception's message (refer to AndroidRuntimeBridge, FailClosedRuntimeFacade, and
RuntimeCapabilities) and import the specific exception types instead of
Exception.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt`:
- Around line 37-110: refresh() is too long/complex—extract two helpers to
simplify: implement computeActiveSessionId(persistedSessionId: SessionId?,
sessions: List<Session>, readiness: Readiness): SessionId? that encapsulates the
when block that determines activeSessionId (including saving persisted ID when
adopting readiness.activeSessionId), and implement
computeRecoveryOverride(persistedSessionId: SessionId?, activeSessionId:
SessionId?, sessionLookup: Result<List<Session>>, linkedMetadata:
LinkedRuntimeMetadata?, readiness: Readiness): MobileRecoveryKind? that
encapsulates the when block that computes recoveryOverride (including clearing
persisted ID when appropriate). Replace the inlined when blocks in refresh()
with calls to these functions and use their return values when building
state.copy; keep all persistence side-effects (persistence.saveActiveSessionId,
persistence.clearActiveSessionId) inside the helper functions so refresh()
becomes a thin coordinator.
- Around line 363-364: The literal padding width "12" used in previewSessionId
should be extracted to a named constant to avoid a magic number; add a private
constant (e.g., PREVIEW_SESSION_ID_PAD or PREVIEW_SESSION_PAD) near the top of
the file or inside the class/companion object and replace the hardcoded 12 in
previewSessionId(index: Int) with that constant so RuntimeSessionId(...) uses
index.toString().padStart(PREVIEW_SESSION_PAD, '0').
- Around line 18-20: The isChatReady property in MobileRuntimeCoordinator uses a
fragile string comparison (bridgeSnapshot.toOnboardingState().status.name ==
"SESSION_READY"); change it to compare the enum directly by comparing
bridgeSnapshot.toOnboardingState().status to the enum constant (e.g.,
Status.SESSION_READY or the actual enum type constant used for onboarding
status) so the check doesn't break if the enum name changes—update the reference
in the isChatReady initializer and add the proper import if required.
In `@openspec/specs/client-surfaces/spec.md`:
- Around line 211-222: The spec currently contradicts itself: the milestone
exclusion list forbids "tool approval handling" while the Mobile-Web Parity
section still mandates approve/deny UI controls; update the document to resolve
this by either removing "tool approval handling" from the exclusion list or by
adding a scoped exception to Mobile-Web Parity that exempts milestone composeApp
surfaces from requiring approve/deny controls; reference the existing
implementation symbols AndroidRuntimeBridge.submitApproval and
MobileRuntimeCoordinator.submitApproval when deciding which approach to keep so
reviewers know the runtime-level approval flow is already implemented.
- Around line 63-76: Add a blank line immediately above every "#### Scenario:"
heading (e.g., "#### Scenario: Web surfaces use HTTP Gateway" and "####
Scenario: Mobile surfaces use CLI bridge") in
openspec/specs/client-surfaces/spec.md to satisfy MD022; ensure the same fix is
applied in the other referred blocks (lines around the other Scenario headings
noted in the review) so each scenario heading is preceded by an empty line.
---
Duplicate comments:
In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt`:
- Around line 112-143: The AppChatContent composable has a long parameter list;
collapse those parameters into a single bindings holder by introducing a data
class ChatBindings that contains coordinatorState: MobileRuntimeCoordinatorState
and all callback lambdas (onRetryBridge, onLinkSurface, onStartSession,
onResumeSession, onSendMessage, onDisconnectReset, onApprove, onDeny), then
change the signature of AppChatContent(platform: Platform, bindings:
ChatBindings) and update the call sites to pass a ChatBindings instance while
forwarding bindings.* into ChatWorkspace (e.g., use bindings.coordinatorState,
bindings.onSendMessage, etc.) to satisfy Detekt LongParameterList.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 896bf9da-cab1-4d94-b43d-e25a9db2d936
📒 Files selected for processing (6)
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.ktopenspec/specs/client-surfaces/spec.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: sonar
- GitHub Check: dependency-review
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (3)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Enforce null safety (no !!), structured concurrency, and non-blocking suspend code.
Prefer idiomatic Kotlin (expression bodies, sealed types, value classes when justified).
Verify tests follow TDD intent and use backtick test names where applicable.
Files:
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktopenspec/specs/client-surfaces/spec.md
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/specs/client-surfaces/spec.md
🧠 Learnings (1)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
openspec/specs/client-surfaces/spec.md
🪛 GitHub Actions: Scan with Detekt
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.kt
[error] 7-7: detekt: Function readLinkedRuntimeMetadata has 5 return statements which exceeds the limit of 2. [ReturnCount]
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt
[error] 27-27: detekt: The caught exception is too generic. Prefer catching specific exceptions to the case that is currently handled. [TooGenericExceptionCaught]
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt
[error] 37-37: detekt: The function refresh is too long (64). The maximum length is 60. [LongMethod]
[error] 37-37: detekt: The function refresh appears to be too complex based on Cyclomatic Complexity (complexity: 23). Defined complexity threshold for methods is set to '15' [CyclomaticComplexMethod]
[error] 364-364: detekt: This expression contains a magic number. Consider defining it to a well named constant. [MagicNumber]
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt
[error] 119-119: detekt: Function runCommand has 4 return statements which exceeds the limit of 2. [ReturnCount]
[error] 181-181: detekt: Function turnFrom has 3 return statements which exceeds the limit of 2. [ReturnCount]
[error] 99-99: detekt: Use check() or error() instead of throwing an IllegalStateException. [UseCheckOrError]
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt
[error] 113-113: detekt: The function AppChatContent has too many parameters (threshold 6). [LongParameterList]
🪛 markdownlint-cli2 (0.22.0)
openspec/specs/client-surfaces/spec.md
[warning] 65-65: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 71-71: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 141-141: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 148-148: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 155-155: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 223-223: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 230-230: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 246-246: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 253-253: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 275-275: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 281-281: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 305-305: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (8)
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.kt (2)
25-36: LGTM — persistence methods are correct.The save/clear implementations properly handle nullable
linkedAtEpochMs(remove vs put) and session ID lifecycle.Also applies to: 38-57
59-66: LGTM — fail-closed parsing correctly returns null for unrecognized values.The private constants and extension functions enforce strict enum matching, preventing silent coercion of corrupt/stale data.
Also applies to: 69-83
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt (1)
64-85: LGTM — composable wiring is correct.The
rememberkeys properly capturecontextandinitialBridgeSnapshot, and the persistence selection (in-memory for preview, SharedPreferences for production) aligns with the client-first model.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt (2)
112-162: LGTM — session lifecycle and chat methods are correct.State mutations are properly guarded with
runCatching, persistence is synchronized, and failures triggerrefresh()for recovery.Also applies to: 164-175, 177-195
238-261: LGTM — in-memory persistence and preview facade implementations are sound.
PreviewMobileRuntimeFacadecorrectly simulates approval flows when prompts contain trigger keywords, supporting UI development without a real runtime.Also applies to: 263-361
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (2)
44-47: State snapshot pattern is functional but consider exception safety.If
coordinator.block()throws, the UI state won't reflect any partial changes. This is likely fine for fail-fast behavior, but worth noting for debugging purposes.
49-54: LGTM — onboarding gate now applies to all platforms.The fix from past review is correctly applied:
shouldShowOnboardingno longer checksplatform.isMobile, ensuring desktop follows the client-first flow.clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt (1)
14-73: LGTM — probe parsing and capability detection are sound.The
toValues()andbooleanValue()helpers correctly parse k=v output, andprobeReadiness()appropriately zeros capabilities on probe failure.Also applies to: 230-246
|
- AndroidPersistence: extract parseLinkedRuntimeMetadataComponents helper for Detekt - AndroidRuntimeBridge: refactor runCommand with startProcess/drainOutputWithTimeout helpers, add descriptive messages to requireNotNull calls, use error() in endSession, log reader thread exceptions - PlatformRuntimeDependencies.android: use specific exception handlers (IOException, IllegalStateException) instead of generic Exception - MobileRuntimeCoordinator: extract computeActiveSessionId and computeRecoveryOverride helpers, add PREVIEW_SESSION_PAD constant, fix isChatReady to use MobileOnboardingStatus enum - App.kt: collapse long parameter list into ChatBindings data class - client-surfaces/spec.md: add blank lines before Scenario headings, clarify approval handling milestone exception DALLAY-179
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (5)
openspec/specs/client-surfaces/spec.md (3)
71-75:⚠️ Potential issue | 🟠 MajorUnify composeApp transport rules across platform sections.
Line 71-Line 75 mandates RustCliBridge as the mobile primary transport, while Line 137-Line 139 allows iOS “approved path(s)” that may be endpoint/pairing/trusted-companion, and Line 174-Line 178 restates CLI bridge path as approved onboarding transport. This is still internally split for iOS/companion flows. Define per-platform transport authority once (matrix + requirement) and remove conflicting wording.
As per coding guidelines,
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.Also applies to: 123-140, 174-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/specs/client-surfaces/spec.md` around lines 71 - 75, The "Scenario: Mobile surfaces use CLI bridge" rule conflicts with later iOS/companion wording about "approved path(s)" and the CLI bridge onboarding note; unify transport authority by defining a single per-platform transport matrix and requirement and then remove or replace the conflicting snippets. Update the spec to introduce a single "Per-platform transport authority" section (or matrix) that lists mobile platforms (composeApp/desktop/Android/iOS) and their primary approved transport (RustCliBridge for mobile where applicable and explicit approved iOS companion/endpoint/pairing exceptions if any), then change the "Scenario: Mobile surfaces use CLI bridge", the iOS "approved path(s)" text, and the CLI onboarding paragraph to reference that matrix instead of duplicating or contradicting rules.
65-65:⚠️ Potential issue | 🟡 MinorFix MD022: add blank lines before scenario headings.
#### Scenario:headings in these ranges are missing required surrounding blank lines. This is a lint failure and should be fixed in this PR.As per coding guidelines,
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.Also applies to: 71-71, 141-141, 148-148, 155-155, 228-228, 235-235, 251-251, 258-258, 280-280, 286-286, 310-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/specs/client-surfaces/spec.md` at line 65, The Markdown lint MD022 errors are caused by missing blank lines before "#### Scenario:" headings; update the spec.md by inserting a single blank line immediately above each "#### Scenario:" heading (e.g., "#### Scenario: Web surfaces use HTTP Gateway" and the other "#### Scenario:" occurrences) so that each header is preceded by an empty line, then save and re-run the linter to confirm the MD022 warnings are resolved.
201-226:⚠️ Potential issue | 🟠 MajorResolve milestone scope vs mandatory parity contradiction.
Line 211-Line 226 says composeApp is not required to deliver chat/session/approval UI parity this milestone, but Line 318-Line 339 still makes chat/session/tool-approval parity mandatory for mobile-web acceptance. This keeps acceptance criteria ambiguous and can cause release-gating disputes. Pick one contract (or add an explicit milestone-scoped exception in the parity section) and keep it single-source.
As per coding guidelines,
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.Also applies to: 318-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/specs/client-surfaces/spec.md` around lines 201 - 226, The spec currently contradicts itself: the composeApp "MUST NOT" list forbids chat/session/approval parity for the milestone while a later parity section mandates chat/session/tool-approval parity for mobile-web acceptance; resolve by picking one contract and making it single-source—either remove the parity requirement from the mobile-web acceptance section or add an explicit milestone-scoped exception there referencing the earlier exception about runtime-level approval (AndroidRuntimeBridge.submitApproval and MobileRuntimeCoordinator.submitApproval) and clarifying that UI/UX parity for chat/session/tool-approval is deferred for this milestone; update the wording in the parity section to reference composeApp and the two runtime bridge methods to keep the doc technically aligned and unambiguous.clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt (1)
52-63: 🧹 Nitpick | 🔵 TrivialGeneric
Exceptioncatch triggers Detekt.The fallback catch is intentional for unexpected exceptions, but Detekt flags it. Suppress with annotation or narrow to
RuntimeExceptionif truly unexpected errors should propagate.Suppress option
+ `@Suppress`("TooGenericExceptionCaught") } catch (e: Exception) { // Fall back to fail-closed for any other unexpected exceptions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt` around lines 52 - 63, The catch is too generic and triggers Detekt; either narrow the catch to RuntimeException or suppress the rule. Replace "catch (e: Exception)" with "catch (e: RuntimeException)" so only runtime errors are caught and others propagate, keeping the existing FailClosedRuntimeFacade(...) code, or add a suppression annotation (e.g., `@Suppress`("TooGenericExceptionCaught") or the project’s Detekt rule id) on the enclosing function/block and keep the current catch; reference FailClosedRuntimeFacade, RuntimeCapabilities and the catch block when making the change.clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt (1)
129-133: 🧹 Nitpick | 🔵 Trivial
runCommandstill has 3 return statements (Detekt limit is 2).Refactor to single return with nullable chaining:
Suggested fix
private fun runCommand(prompt: String): String? { - val process = startProcess(prompt) ?: return null - val output = drainOutputWithTimeout(process) ?: return null - return if (process.exitValue() == 0) output else null + val process = startProcess(prompt) + val output = process?.let { drainOutputWithTimeout(it) } + return output?.takeIf { process?.exitValue() == 0 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt` around lines 129 - 133, The function runCommand currently returns in three places; refactor it to a single return by chaining nullables: replace the multiple early returns with a single expression that starts from startProcess(prompt) and uses let (or safe-call ?. and nested let) to call drainOutputWithTimeout(process) and then check process.exitValue() == 0, returning the output string or null accordingly; reference runCommand, startProcess, drainOutputWithTimeout and process.exitValue() when applying this change so the final implementation is a one-line single return using nullable chaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.kt`:
- Around line 73-87: The parsing helpers toTransportMode() and toTrustMode()
only map legacy enum names and must be extended to cover all current
RuntimeTransportMode and RuntimeTrustMode enum values; update both functions to
include mappings for ENDPOINT_URL, TRUSTED_COMPANION, LOCAL_HOST_ADVANCED (for
transport) and PAIRING_REQUIRED, TRUSTED_COMPANION_ESTABLISHED (for trust) so
that when saveLinkedRuntimeMetadata writes those enum names they are parsed back
to the correct enum instances instead of returning null; modify the when
branches in toTransportMode() and toTrustMode() to return the appropriate
RuntimeTransportMode and RuntimeTrustMode constants for these additional names
(and keep existing legacy mappings).
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt`:
- Around line 14-23: The AndroidRuntimeBridge class has too many functions;
extract the parsing utilities to reduce method count by moving toValues,
booleanValue, sessionFrom, and turnFrom out of the AndroidRuntimeBridge class
and into a separate helper (e.g., AndroidRuntimeBridgeParser) or as top-level
private functions; update AndroidRuntimeBridge to call these new functions and
keep existing signatures (ensure visibility and package placement allow access),
and run Detekt to confirm the class now falls below the function threshold.
- Line 114: The code uses the ASCII unit separator literal '\u001f' inline when
building command strings (e.g., in the call to
runCommand("__corvus_send_message__${sessionId.value}\u001f$prompt" inside
turnFrom) which is unclear; define a named constant (e.g., UNIT_SEPARATOR or
PARAM_DELIM) with a short comment documenting that this delimiter convention
separates command parameters, replace the inline '\u001f' occurrences in
AndroidRuntimeBridge (and the second occurrence noted around the same file) with
that constant, and ensure the constant is placed near the class (companion
object or top-level) so other methods can reuse it for consistency.
- Line 178: The call readerThread.join(1000) uses a magic number; replace it
with a named constant (e.g., READER_THREAD_JOIN_TIMEOUT_MS) declared near the
AndroidRuntimeBridge class (companion object or top-level) and use that constant
in the readerThread.join call so the timeout is self-documenting and easy to
adjust; ensure the constant is in milliseconds and update any related comments
or usages (reference: readerThread.join and AndroidRuntimeBridge).
- Around line 165-167: The catch block in AndroidRuntimeBridge that currently
does `catch (e: Exception) { android.util.Log.d("AndroidRuntimeBridge", "Reader
thread exception: ${e.message}", e) }` should be annotated to suppress Detekt's
generic exception rule; add a suppression annotation such as
`@Suppress`("TooGenericExceptionCaught") on the enclosing function or the smallest
scope that contains this catch (e.g., the reader thread function in class
AndroidRuntimeBridge) so the generic Exception catch remains but Detekt will be
satisfied.
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt`:
- Line 23: Replace the hardcoded preference key string in the prefs.getString
call (currently "corvus.mobile.runtime.target" used to populate targetId) with
the shared constant to avoid duplication; use the existing
AndroidPersistence.KEY_TARGET_ID (or extract a single shared constant and
reference it from both places) so
prefs.getString(AndroidPersistence.KEY_TARGET_ID, null) is used instead of the
literal string.
In `@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt`:
- Around line 63-66: The use of remember around shouldShowOnboarding causes
potential stale evaluation; remove the remember(...) wrapper and compute the
boolean directly by evaluating onboardingState.status !=
MobileOnboardingStatus.SESSION_READY so shouldShowOnboarding is recomputed on
every composition and no longer depends on the remember lifecycle; update the
declaration of shouldShowOnboarding in App.kt accordingly.
In
`@clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt`:
- Around line 25-38: The MobileRuntimeCoordinator class exceeds Detekt's
function count limit; to satisfy the linter, extract the small helper
nextMessageId out of the class as a private top-level function (or alternatively
move applyTurnResult into a separate extension function) so the coordinator's
method count drops below the threshold while preserving behavior; update
references in MobileRuntimeCoordinator to call the new top-level nextMessageId
(or the extension applyTurnResult) and run the linter to confirm the
TooManyFunctions warning is resolved.
- Around line 96-101: In MobileRuntimeCoordinator where you're handling the case
`persistedSessionId == null && readiness.activeSessionId != null`, remove the
redundant requireNotNull call and rely on Kotlin's smart-cast of
`readiness.activeSessionId` (use it directly as `readiness.activeSessionId` to
assign to `runtimeSessionId` and pass into
`persistence.saveActiveSessionId(runtimeSessionId)`), keeping the when branch
logic intact.
---
Duplicate comments:
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt`:
- Around line 129-133: The function runCommand currently returns in three
places; refactor it to a single return by chaining nullables: replace the
multiple early returns with a single expression that starts from
startProcess(prompt) and uses let (or safe-call ?. and nested let) to call
drainOutputWithTimeout(process) and then check process.exitValue() == 0,
returning the output string or null accordingly; reference runCommand,
startProcess, drainOutputWithTimeout and process.exitValue() when applying this
change so the final implementation is a one-line single return using nullable
chaining.
In
`@clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt`:
- Around line 52-63: The catch is too generic and triggers Detekt; either narrow
the catch to RuntimeException or suppress the rule. Replace "catch (e:
Exception)" with "catch (e: RuntimeException)" so only runtime errors are caught
and others propagate, keeping the existing FailClosedRuntimeFacade(...) code, or
add a suppression annotation (e.g., `@Suppress`("TooGenericExceptionCaught") or
the project’s Detekt rule id) on the enclosing function/block and keep the
current catch; reference FailClosedRuntimeFacade, RuntimeCapabilities and the
catch block when making the change.
In `@openspec/specs/client-surfaces/spec.md`:
- Around line 71-75: The "Scenario: Mobile surfaces use CLI bridge" rule
conflicts with later iOS/companion wording about "approved path(s)" and the CLI
bridge onboarding note; unify transport authority by defining a single
per-platform transport matrix and requirement and then remove or replace the
conflicting snippets. Update the spec to introduce a single "Per-platform
transport authority" section (or matrix) that lists mobile platforms
(composeApp/desktop/Android/iOS) and their primary approved transport
(RustCliBridge for mobile where applicable and explicit approved iOS
companion/endpoint/pairing exceptions if any), then change the "Scenario: Mobile
surfaces use CLI bridge", the iOS "approved path(s)" text, and the CLI
onboarding paragraph to reference that matrix instead of duplicating or
contradicting rules.
- Line 65: The Markdown lint MD022 errors are caused by missing blank lines
before "#### Scenario:" headings; update the spec.md by inserting a single blank
line immediately above each "#### Scenario:" heading (e.g., "#### Scenario: Web
surfaces use HTTP Gateway" and the other "#### Scenario:" occurrences) so that
each header is preceded by an empty line, then save and re-run the linter to
confirm the MD022 warnings are resolved.
- Around line 201-226: The spec currently contradicts itself: the composeApp
"MUST NOT" list forbids chat/session/approval parity for the milestone while a
later parity section mandates chat/session/tool-approval parity for mobile-web
acceptance; resolve by picking one contract and making it single-source—either
remove the parity requirement from the mobile-web acceptance section or add an
explicit milestone-scoped exception there referencing the earlier exception
about runtime-level approval (AndroidRuntimeBridge.submitApproval and
MobileRuntimeCoordinator.submitApproval) and clarifying that UI/UX parity for
chat/session/tool-approval is deferred for this milestone; update the wording in
the parity section to reference composeApp and the two runtime bridge methods to
keep the doc technically aligned and unambiguous.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59d3294c-6b9c-4cf1-92e4-a8a8b5a3b2f1
📒 Files selected for processing (6)
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.ktopenspec/specs/client-surfaces/spec.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: sonar
- GitHub Check: dependency-review
- GitHub Check: pr-checks
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (3)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: Enforce null safety (no !!), structured concurrency, and non-blocking suspend code.
Prefer idiomatic Kotlin (expression bodies, sealed types, value classes when justified).
Verify tests follow TDD intent and use backtick test names where applicable.
Files:
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt
**/*
⚙️ CodeRabbit configuration file
**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.
Files:
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.ktopenspec/specs/client-surfaces/spec.mdclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.ktclients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.ktclients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt
**/*.{md,mdx}
⚙️ CodeRabbit configuration file
**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.
Files:
openspec/specs/client-surfaces/spec.md
🧠 Learnings (3)
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Applied to files:
openspec/specs/client-surfaces/spec.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks
Applied to files:
openspec/specs/client-surfaces/spec.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths
Applied to files:
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt
🪛 GitHub Actions: Scan with Detekt
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt
[error] 52-52: detekt [TooGenericExceptionCaught]: Caught exception is too generic; prefer catching specific exceptions.
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt
[error] 14-14: detekt [TooManyFunctions]: Class 'AndroidRuntimeBridge' with '14' functions detected. Defined threshold inside classes is set to '11'.
[error] 165-165: detekt [TooGenericExceptionCaught]: Caught exception is too generic; prefer catching specific exceptions.
[error] 129-129: detekt [ReturnCount]: Function runCommand has 3 return statements; limit is 2.
clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt
[error] 25-25: detekt [TooManyFunctions]: Class 'MobileRuntimeCoordinator' with '11' functions detected. Defined threshold inside classes is set to '11'.
🪛 markdownlint-cli2 (0.22.0)
openspec/specs/client-surfaces/spec.md
[warning] 65-65: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 71-71: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 141-141: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 148-148: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 155-155: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 228-228: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 235-235: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 251-251: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 258-258: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 280-280: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 286-286: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 310-310: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (8)
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidPersistence.kt (1)
5-61: LGTM on persistence implementation.The fail-closed validation in
parseLinkedRuntimeMetadataComponentscorrectly requires all fields (targetId, transportMode, trustMode, linkedAtEpochMs) to be present and valid before returning metadata. The helper extraction addresses the prior DetektReturnCountconcern.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/runtime/MobileRuntimeCoordinator.kt (2)
40-89: LGTM on coordinator refresh logic.The extracted
computeActiveSessionIdandcomputeRecoveryOverridehelpers address prior Detekt complexity concerns. The recovery ordering now correctly handlessessionLookup.isFailurebefore clearing the persisted session ID.
263-286: LGTM on in-memory persistence.Simple, correct implementation for preview/test scenarios.
clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/PlatformRuntimeDependencies.android.kt (1)
89-109: LGTM on platform dependencies wiring.Correctly selects
InMemoryMobileRuntimePersistencefor preview mode andAndroidPersistencefor production, matching the iOS implementation pattern.clients/composeApp/src/commonMain/kotlin/com/profiletailors/corvus/App.kt (2)
28-38: LGTM on ChatBindings consolidation.Addresses prior
LongParameterListconcern cleanly by grouping coordinator state and callbacks.
109-125: LGTM on AppOnboardingContent.Clean delegation to
runtimeOnboardingStepandOnboardingScreen.clients/composeApp/src/androidMain/kotlin/com/profiletailors/corvus/runtime/AndroidRuntimeBridge.kt (2)
25-73: LGTM on probeReadiness implementation.Correctly parses
k=voutput, derives capabilities from probe response, and falls back to all-false snapshot on failure.
155-180: LGTM on concurrent output draining.Spawning the reader thread before
waitForprevents the I/O deadlock from the prior review. Timeout handling withdestroyForciblyandinterruptis correct.
- AndroidPersistence: add ENDPOINT_URL, TRUSTED_COMPANION, LOCAL_HOST_ADVANCED to toTransportMode(); add PAIRING_REQUIRED, TRUSTED_COMPANION_ESTABLISHED to toTrustMode(); expose keys as internal - AndroidRuntimeBridge: extract parsing helpers to top-level, add UNIT_SEPARATOR and READER_THREAD_JOIN_TIMEOUT_MS constants, replace magic number, add Suppress annotation, refactor runCommand to single expression - PlatformRuntimeDependencies.android: use RuntimeException instead of Exception, use AndroidPersistence.KEY_TARGET_ID constant - App.kt: remove remember wrapper from shouldShowOnboarding - MobileRuntimeCoordinator: remove requireNotNull in computeActiveSessionId, extract nextMessageId to top-level - client-surfaces/spec.md: add milestone exception to tool approval parity scenario DALLAY-179
yacosta738
deleted the
feature/dallay-179-mobile-runtime-parity-client-first
branch
Merged
This was referenced Apr 9, 2026
Merged
Merged
Merged
This was referenced Apr 25, 2026
8 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces significant changes to the Android and Compose multiplatform client, focusing on a "client-first" onboarding and runtime connection model. The Android app no longer assumes a packaged local runtime by default; instead, it requires explicit endpoint configuration or a trusted companion for runtime connectivity. Additionally, the runtime bridge and persistence layers have been refactored to support this new model, and the onboarding and chat UI flows have been updated to reflect these changes.
Android Runtime Model and Build Changes:
Runtime Bridge and Persistence Implementation:
AndroidPersistencefor storing runtime metadata and session IDs inSharedPreferences, supporting the new client-first onboarding and session flow.AndroidRuntimeBridge, a facade for interacting with the runtime via command execution, supporting session management and approval flows.Compose App and Onboarding Flow Refactor:
Appto use aMobileRuntimeCoordinatorfor state management, replacing previous bridge snapshot logic. The onboarding flow now reflects the client-first model, showing onboarding screens when no session or connection is established and handling actions such as linking, session creation, and approval requests.Build and Dependency Updates:
agent-core-kmp, ensuring shared logic is available across platforms.Makefileclean targets to perform deeper, multi-language cleans, including Gradle, Rust, web, and pnpm artifacts.Closes: #274