Upgrade to AppFunctions 1.0.0-alpha10#24
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the application to the new androidx.appfunctions version 1.0.0-alpha10 architecture, replacing the legacy configuration with the new AppFunctionService entry points. It introduces BaseChatAppFunctionService, BaseBuiltInAppFunctionService, and BaseFakeAppFunctionService, registers them in the manifests, and adds AppFunctionExceptionFormatter to format exceptions for LLM outputs. Feedback focuses on ensuring cooperative coroutine cancellation by rethrowing CancellationException instead of swallowing or wrapping it in BaseChatAppFunctionService and AgentOrchestrator. Additionally, it is recommended to avoid using reflection with Class.forName when launching MainActivity in BaseChatAppFunctionService.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| val sentMessageId = | ||
| try { | ||
| messageRepository.send( | ||
| text = messageBody, | ||
| recipientIds = listOf(endpointValue), | ||
| imageUris = imageUris?.map { it.toString() }, | ||
| ) | ||
| } catch (e: Exception) { | ||
| throw AppFunctionAppUnknownException("Failed to send message: ${e.message}") | ||
| } |
There was a problem hiding this comment.
Swallowing or wrapping CancellationException in a generic AppFunctionAppUnknownException can break cooperative coroutine cancellation. In Kotlin coroutines, CancellationException should always be rethrown to allow the coroutine machinery to cancel the job properly.
| val sentMessageId = | |
| try { | |
| messageRepository.send( | |
| text = messageBody, | |
| recipientIds = listOf(endpointValue), | |
| imageUris = imageUris?.map { it.toString() }, | |
| ) | |
| } catch (e: Exception) { | |
| throw AppFunctionAppUnknownException("Failed to send message: ${e.message}") | |
| } | |
| val sentMessageId = | |
| try { | |
| messageRepository.send( | |
| text = messageBody, | |
| recipientIds = listOf(endpointValue), | |
| imageUris = imageUris?.map { it.toString() }, | |
| ) | |
| } catch (e: kotlinx.coroutines.CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| throw AppFunctionAppUnknownException("Failed to send message: ${e.message}") | |
| } |
| return PendingIntent.getActivity( | ||
| this, | ||
| 0, | ||
| Intent(this, Class.forName("com.example.chatapp.MainActivity")) | ||
| .apply { putExtra("nav_route", "call/${recipient.id}") }, | ||
| PendingIntent.FLAG_ONE_SHOT or PendingIntent.FLAG_IMMUTABLE, | ||
| ) |
There was a problem hiding this comment.
Using Class.forName to resolve MainActivity is fragile and uses reflection, which can be slow and flagged by static analysis or R8. Since MainActivity is registered in the manifest, you can use Intent.setClassName directly with the class name string. This is more idiomatic and avoids reflection.
val intent = Intent().setClassName(this, "com.example.chatapp.MainActivity")
.apply { putExtra("nav_route", "call/${recipient.id}") }
return PendingIntent.getActivity(
this,
0,
intent,
PendingIntent.FLAG_ONE_SHOT or PendingIntent.FLAG_IMMUTABLE,
)| is ExecuteAppFunctionResult.Error -> { | ||
| val exception = executionResult.exception | ||
| val appException = AppFunctionExceptionFormatter.getAppFunctionException(exception) | ||
| if (appException != null) { | ||
| results.add( | ||
| ToolOutput( | ||
| functionId = toolCall.functionId, | ||
| callId = toolCall.callId, | ||
| result = | ||
| AppFunctionExceptionFormatter.format( | ||
| appException, | ||
| toolCall.functionId, | ||
| ), | ||
| ), | ||
| ) | ||
| } else { | ||
| throw IllegalStateException( | ||
| "Tool execution failed for ${toolCall.functionId}: ${exception.message}", | ||
| exception, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
If the tool execution is cancelled, executionResult.exception will be a CancellationException. Wrapping CancellationException in an IllegalStateException breaks cooperative coroutine cancellation. You should check if the exception is a CancellationException and rethrow it directly.
is ExecuteAppFunctionResult.Error -> {
val exception = executionResult.exception
if (exception is kotlinx.coroutines.CancellationException) {
throw exception
}
val appException = AppFunctionExceptionFormatter.getAppFunctionException(exception)
if (appException != null) {
results.add(
ToolOutput(
functionId = toolCall.functionId,
callId = toolCall.callId,
result =
AppFunctionExceptionFormatter.format(
appException,
toolCall.functionId,
),
),
)
} else {
throw IllegalStateException(
"Tool execution failed for ${toolCall.functionId}: ${exception.message}",
exception,
)
}
}
| @@ -28,7 +28,7 @@ android { | |||
| defaultConfig { | |||
| applicationId = "com.example.chatapp.wear" | |||
| minSdk = 30 | |||
There was a problem hiding this comment.
nit/optional: Can we just bump this to 36? then we don't need all the RequiresApi (?)
| @@ -47,7 +47,7 @@ class AppFunctionsViewModelTest { | |||
| fun toggleFunction_doesNotCrash() = | |||
| runTest { | |||
| // Use a potentially real ID from the app | |||
There was a problem hiding this comment.
nit: seems this is fixed?
| android:name="com.example.appfunctions.agent.data.FakeAppFunctionService" | ||
| android:permission="android.permission.BIND_APP_FUNCTION_SERVICE" | ||
| android:exported="true" | ||
| tools:targetApi="36"> |
There was a problem hiding this comment.
Multiple services won't work on Android 36 and is a 37+ feature
| } | ||
| } | ||
| } | ||
| object AppFunctionsIds { |
There was a problem hiding this comment.
These are available from ChatAppFunctionService too
| recipientsRepo: RecipientsRepository, | ||
| callMgr: CallManager, | ||
| ) : BaseChatAppFunctionService() { | ||
| init { |
There was a problem hiding this comment.
I'll suggest use AppFunctionManager API to test so that the service doesn't need to be faked. That will be our testing suggestion for app developer as well.
In general, the testing is done as following
- Use UiAutomation to adopt shell permission to obtain EXECUTE_APP_FUNCTION permission
- Use Jetpack AppFunctionManager to search AppFunctions from your target app package name. Then developers can assert the functions are exposed correctly.
- Use Jetpack AppFunctionManager to execute it.
This will ensure that the testing is e2e and represent how agent will use it. The mocking test has a problem that if developer setup the manifest or something incorrectly, it would not catch it.
There was a problem hiding this comment.
Actually, these are covered in https://github.com/android/appfunctions/blob/main/ChatApp/app/src/androidTest/kotlin/com/example/chatapp/appfunctions/AppFunctionInstrumentationTest.kt. So I think we can delete this file safely
| ) | ||
| is ExecuteAppFunctionResult.Error -> { | ||
| val exception = executionResult.exception | ||
| val appException = AppFunctionExceptionFormatter.getAppFunctionException(exception) |
| @Test | ||
| fun `invoke returns Error when AppFunctionManager is null`() = runTest { | ||
| val useCaseWithNullManager = ExecuteAppFunctionUseCase(null, convertAppFunctionDataToJsonUseCase) | ||
| val function = mockk<AppFunctionMetadata>(relaxed = true) |
3 participants
• Upgraded both
agentandChatAppmodules to AppFunctions 1.0.0-alpha10.• Refactored exception handling in
AgentOrchestratorto act on errors directly instead of surfacing them as unhandled exceptions.• Cleaned up obsolete repository classes and refactored base service architectures.