-
-
Notifications
You must be signed in to change notification settings - Fork 3
fix(notifications): stop announcing aborted, failed or stray idles as completed #234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -39,6 +39,7 @@ data class RuntimeActivityState( | |||||||||||||||||||
| val completedSessionIds: Set<String> = emptySet(), | ||||||||||||||||||||
| /** Sessions whose current run has ended; late stream events must not resurrect them. */ | ||||||||||||||||||||
| val settledSessionIds: Set<String> = emptySet(), | ||||||||||||||||||||
| val mutedSessionIds: Set<String> = emptySet(), | ||||||||||||||||||||
| val permissions: List<PermissionRequest> = emptyList(), | ||||||||||||||||||||
| val logs: List<RuntimeEventLog> = emptyList(), | ||||||||||||||||||||
| val streamError: String? = null, | ||||||||||||||||||||
|
|
@@ -189,11 +190,22 @@ class RuntimeActivityRepository( | |||||||||||||||||||
| activeSessionIds = current.activeSessionIds + sessionId, | ||||||||||||||||||||
| completedSessionIds = current.completedSessionIds - sessionId, | ||||||||||||||||||||
| settledSessionIds = current.settledSessionIds - sessionId, | ||||||||||||||||||||
| mutedSessionIds = current.mutedSessionIds - sessionId, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| persistUnread() | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| fun markSessionAborted(sessionId: String) { | ||||||||||||||||||||
| if (sessionId.isBlank()) return | ||||||||||||||||||||
| mutableState.update { current -> | ||||||||||||||||||||
| current.copy( | ||||||||||||||||||||
| activeSessionIds = current.activeSessionIds - sessionId, | ||||||||||||||||||||
| mutedSessionIds = current.mutedSessionIds + sessionId, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
|
Comment on lines
+202
to
+205
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · medium] Suggestion:
Suggested change
|
||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** Records that a run finished, leaving the chat unread until it is opened. */ | ||||||||||||||||||||
| fun markSessionFinished( | ||||||||||||||||||||
| sessionId: String, | ||||||||||||||||||||
|
|
@@ -251,14 +263,18 @@ class RuntimeActivityRepository( | |||||||||||||||||||
| } | ||||||||||||||||||||
| is OpenCodeEvent.SessionIdle -> { | ||||||||||||||||||||
| markRuntimeIdle(event.sessionId) | ||||||||||||||||||||
| var muted = false | ||||||||||||||||||||
| mutableState.update { current -> | ||||||||||||||||||||
| muted = event.sessionId in current.mutedSessionIds | ||||||||||||||||||||
| current.copy( | ||||||||||||||||||||
| activeSessionIds = current.activeSessionIds - event.sessionId, | ||||||||||||||||||||
| completedSessionIds = current.completedSessionIds + event.sessionId, | ||||||||||||||||||||
| settledSessionIds = current.settledSessionIds + event.sessionId, | ||||||||||||||||||||
| mutedSessionIds = current.mutedSessionIds - event.sessionId, | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [maintainability · low] |
||||||||||||||||||||
| ) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| appendLog(messages.eventCompleted, null, event.sessionId) | ||||||||||||||||||||
| if (muted) return | ||||||||||||||||||||
| parentResolutionOf(target, event.sessionId).onSuccess { parentId -> | ||||||||||||||||||||
| if (parentId == null) { | ||||||||||||||||||||
| onSessionIdle?.invoke(event.sessionId, sessionTitle(target, event.sessionId), target.id) | ||||||||||||||||||||
|
|
@@ -301,6 +317,7 @@ class RuntimeActivityRepository( | |||||||||||||||||||
| } else { | ||||||||||||||||||||
| current.settledSessionIds - event.sessionId | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| mutedSessionIds = current.mutedSessionIds - event.sessionId, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| if (event.status != "idle") { | ||||||||||||||||||||
|
|
@@ -314,6 +331,7 @@ class RuntimeActivityRepository( | |||||||||||||||||||
| current.copy( | ||||||||||||||||||||
| activeSessionIds = current.activeSessionIds - sessionId, | ||||||||||||||||||||
| settledSessionIds = current.settledSessionIds + sessionId, | ||||||||||||||||||||
| mutedSessionIds = current.mutedSessionIds + sessionId, | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -328,6 +328,7 @@ class ChatViewModel( | |
| * events arrive. Deriving it from events alone left every chat on the idle marker. | ||
| */ | ||
| private val onRunStateChanged: (String, Boolean) -> Unit = { _, _ -> }, | ||
| private val onSessionAborted: (String) -> Unit = {}, | ||
| private val draftRepo: DraftRepository? = null, | ||
| /** | ||
| * Starts the periodic connection probe. It runs an unbounded polling loop, which a virtual | ||
|
|
@@ -793,6 +794,7 @@ class ChatViewModel( | |
| _uiState.update { it.copy(attachments = emptyList(), imagePreviews = emptyList()) } | ||
| return | ||
| } | ||
| val interrupting = _uiState.value.isRunning | ||
|
|
||
| val userMessage = | ||
| ChatMessage( | ||
|
|
@@ -848,6 +850,7 @@ class ChatViewModel( | |
| } | ||
| refreshContextUsage(targetSessionId) | ||
| } | ||
| if (interrupting) onSessionAborted(targetSessionId) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · low] |
||
| currentBackend.sendMessage( | ||
|
Comment on lines
+853
to
854
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · high] |
||
| targetSessionId, | ||
| PromptRequest( | ||
|
|
@@ -1395,6 +1398,7 @@ class ChatViewModel( | |
| val currentBackend = backend ?: return | ||
| val sessionId = _uiState.value.sessionId ?: return | ||
| viewModelScope.launch { | ||
| onSessionAborted(sessionId) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · medium] |
||
| runCatching { currentBackend.abortSession(sessionId) } | ||
|
Comment on lines
+1401
to
1402
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · high] |
||
| .onSuccess { | ||
| _uiState.update { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,7 @@ import com.yugahashimoto.andcode.core.api.OpenCodeTodo | |||||
| import com.yugahashimoto.andcode.core.api.PromptAttachment | ||||||
| import com.yugahashimoto.andcode.core.api.QuestionRequest | ||||||
| import com.yugahashimoto.andcode.runtime.PermissionResponse | ||||||
| import kotlinx.coroutines.CancellationException | ||||||
| import kotlinx.coroutines.CoroutineScope | ||||||
| import kotlinx.coroutines.Dispatchers | ||||||
| import kotlinx.coroutines.Job | ||||||
|
|
@@ -69,6 +70,7 @@ class ClaudeCodeRuntime( | |||||
| private class SessionProcess( | ||||||
| val process: Process, | ||||||
| val readerJob: Job, | ||||||
| val parser: ClaudeStreamJsonParser, | ||||||
| val permissionMode: ClaudePermissionMode, | ||||||
| val directory: String, | ||||||
| val model: String?, | ||||||
|
|
@@ -280,6 +282,7 @@ class ClaudeCodeRuntime( | |||||
| ): Result<Unit> = | ||||||
| runCatching { | ||||||
| val session = ensureProcess(sessionId, directory.ifBlank { "/workspace" }, permissionMode, model, effort) | ||||||
| session.parser.beginTurn() | ||||||
| recordUserMessage(sessionId, prompt, attachments) | ||||||
| session.process.outputStream.apply { | ||||||
| write((json.encodeToString(JsonObject.serializer(), userMessage(prompt, attachments)) + "\n").toByteArray()) | ||||||
|
|
@@ -382,20 +385,33 @@ class ClaudeCodeRuntime( | |||||
| val requestedModel = model | ||||||
| val readerJob = | ||||||
| scope.launch { | ||||||
| runCatching { | ||||||
| process.inputStream.bufferedReader().forEachLine { line -> | ||||||
| handleLine(sessionId, parser, line, requestedModel) | ||||||
| } | ||||||
| } | ||||||
| val streamFailure = | ||||||
| runCatching { | ||||||
| process.inputStream.bufferedReader().forEachLine { line -> | ||||||
| handleLine(sessionId, parser, line, requestedModel) | ||||||
| } | ||||||
| }.exceptionOrNull() | ||||||
| .takeUnless { it is CancellationException } | ||||||
| messageStore.flush() | ||||||
| // A CLI that exits mid-turn would otherwise leave the chat spinning forever. | ||||||
| events.tryEmit(OpenCodeEvent.SessionIdle(sessionId)) | ||||||
| synchronized(this@ClaudeCodeRuntime) { | ||||||
| if (sessions[sessionId]?.process === process) sessions.remove(sessionId) | ||||||
| } | ||||||
| // stop() cancels this job; the abort path owns the state transitions, and an idle | ||||||
| // emitted here would announce the killed run as completed. | ||||||
| if (!isActive) return@launch | ||||||
| // A CLI that exits before its result line would otherwise leave the chat spinning | ||||||
| // forever; that is a failure, not a completion. A finished turn already emitted its | ||||||
| // own idle, and repeating it on process exit would re-announce the old run. | ||||||
| if (!parser.turnFinished) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [other · low] |
||||||
| val exitCode = runCatching { process.exitValue() }.getOrNull() | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [maintainability · low] |
||||||
| events.tryEmit( | ||||||
| OpenCodeEvent.SessionError(sessionId, messages.processExited(exitCode, streamFailure?.message)), | ||||||
| ) | ||||||
|
Comment on lines
+407
to
+409
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · low]
Comment on lines
+407
to
+409
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · low] |
||||||
| events.tryEmit(OpenCodeEvent.SessionIdle(sessionId)) | ||||||
| } | ||||||
|
Comment on lines
+405
to
+411
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · high] |
||||||
| } | ||||||
|
|
||||||
| return SessionProcess(process, readerJob, effectiveMode, directory, model, effort) | ||||||
| return SessionProcess(process, readerJob, parser, permissionMode, directory, model, effort) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · high] Suggestion:
Suggested change
|
||||||
| .also { sessions[sessionId] = it } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,11 @@ interface ClaudeMessages { | |
|
|
||
| fun signInExited(exitCode: Int): String | ||
|
|
||
| fun processExited( | ||
| exitCode: Int?, | ||
| detail: String?, | ||
| ): String | ||
|
|
||
| /** English fallbacks for unit tests and any construction path without a [Context]. */ | ||
| companion object Default : ClaudeMessages { | ||
| override val runtimeMissing = "The Linux environment is not installed yet" | ||
|
|
@@ -29,6 +34,14 @@ interface ClaudeMessages { | |
| override val updateFailed = "Claude Code update failed" | ||
|
|
||
| override fun signInExited(exitCode: Int) = "Claude Code sign-in stopped (exit code $exitCode)" | ||
|
|
||
| override fun processExited( | ||
| exitCode: Int?, | ||
| detail: String?, | ||
| ): String { | ||
| val cause = detail ?: exitCode?.let { "exit code $it" } ?: "process exited" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [maintainability · low] |
||
| return "Claude Code stopped before finishing the turn ($cause)" | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -41,4 +54,12 @@ class AndroidClaudeMessages(private val context: Context) : ClaudeMessages { | |
| override val updateFailed get() = context.getString(R.string.claude_error_update_failed) | ||
|
|
||
| override fun signInExited(exitCode: Int): String = context.getString(R.string.claude_error_sign_in_exit, exitCode) | ||
|
|
||
| override fun processExited( | ||
| exitCode: Int?, | ||
| detail: String?, | ||
| ): String { | ||
| val cause = detail ?: exitCode?.let { "exit code $it" } ?: "process exited" | ||
| return context.getString(R.string.claude_error_process_exited, cause) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [other · low] |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,14 @@ class ClaudeStreamJsonParser( | |
|
|
||
| private var currentMessageId: String? = null | ||
|
|
||
| @Volatile | ||
| var turnFinished: Boolean = false | ||
| private set | ||
|
Comment on lines
+48
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [maintainability · low]
Comment on lines
+48
to
+50
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [maintainability · low] |
||
|
|
||
| fun beginTurn() { | ||
| turnFinished = false | ||
| } | ||
|
Comment on lines
+52
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · medium]
Comment on lines
+52
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [bug · medium] |
||
|
|
||
| /** Tool calls that have not received a matching tool_result from Claude Code yet. */ | ||
| private val openTools = linkedMapOf<String, OpenTool>() | ||
| private val messagesById = linkedMapOf<String, OpenCodeMessage>() | ||
|
|
@@ -157,6 +165,7 @@ class ClaudeStreamJsonParser( | |
| } | ||
|
|
||
| private fun parseResult(root: JsonObject): Parsed { | ||
| turnFinished = true | ||
| val claudeSessionId = root.string("session_id") | ||
| val isError = root["is_error"]?.jsonPrimitive?.contentOrNull == "true" | ||
| val subtype = root.string("subtype") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[bug · medium]
markSessionAbortedは呼び出し元(ChatViewModel.abort / 割り込み送信)でabortSessionやsendMessageの成否に関わらず先に実行されます。abort が失敗したりランタイムが中断を無視して run が実際には継続している場合でも、ここで即座にactiveSessionIdsから除去され、drawer は停止状態を表示する一方、後続のSessionIdleが muted 扱いとなり完了通知も抑止されます。実際に run の終了が確認できてから muted へ移す(または abort 失敗時に状態を戻す)など、実行終了を確定させてから状態を変更する設計を検討してください。