diff --git a/pkg-r/R/chat.R b/pkg-r/R/chat.R index c4afbde8..40740efd 100644 --- a/pkg-r/R/chat.R +++ b/pkg-r/R/chat.R @@ -113,14 +113,7 @@ commons_server <- function(id, client, ...) { prewarm_on_idle(client) - chat <- shinychat::chat_server(id, client = client, ...) - # shinychat owns the conversation identity (it sets the client's - # `conversation_id` binding, which ellmer stamps on its spans); commons - # only needs to know that a restore happened. - chat$history$on_restore(function(values) { - client$queue_restore_reminder() - }) - chat + shinychat::chat_server(id, client = client, ...) } # An error escaping a later::later() callback would stop the app, so diff --git a/pkg-r/R/commons.R b/pkg-r/R/commons.R index e1634aa0..3570f60c 100644 --- a/pkg-r/R/commons.R +++ b/pkg-r/R/commons.R @@ -302,7 +302,13 @@ Commons <- R6::R6Class( }, set_turns = function(value) { - private$restore_reminder_pending <- FALSE + # History that is not a prefix of the current turns may come from a + # previous session, where the R state behind its tool calls is gone. + if (length(value) == 0) { + private$restore_reminder_pending <- FALSE + } else if (!turns_are_prefix(value, self$get_turns())) { + private$restore_reminder_pending <- TRUE + } super$set_turns(value) }, @@ -416,11 +422,6 @@ Commons <- R6::R6Class( private$corpus }, - queue_restore_reminder = function() { - private$restore_reminder_pending <- TRUE - invisible(self) - }, - prewarm = function() { # A direct call is typically warming caches ahead of deployment, so # failures propagate: a cold cache should fail the deploy. diff --git a/pkg-r/R/turn-reminder.R b/pkg-r/R/turn-reminder.R index 334aa6b9..9b611665 100644 --- a/pkg-r/R/turn-reminder.R +++ b/pkg-r/R/turn-reminder.R @@ -35,3 +35,26 @@ append_restored_conversation_reminder <- function(inputs) { list(ContentTurnReminder(text = restored_conversation_turn_reminder)) ) } + +# shinychat reconstructs stored turns, so compare their roles and text rather +# than the resulting objects. +turns_are_prefix <- function(value, current) { + if (length(value) > length(current)) { + return(FALSE) + } + identical( + vapply(value, turn_text_signature, character(1)), + vapply(current[seq_along(value)], turn_text_signature, character(1)) + ) +} + +turn_text_signature <- function(turn) { + texts <- vapply( + turn@contents, + function(content) { + if (S7::S7_inherits(content, ellmer::ContentText)) content@text else "" + }, + character(1) + ) + paste(c(turn@role, texts), collapse = "\x1f") +} diff --git a/pkg-r/tests/testthat/test-chat.R b/pkg-r/tests/testthat/test-chat.R index 359307b2..f1f3043c 100644 --- a/pkg-r/tests/testthat/test-chat.R +++ b/pkg-r/tests/testthat/test-chat.R @@ -43,33 +43,6 @@ test_that("commons_server runs under shiny::testServer without error", { succeed() }) -test_that("commons_server queues a restore reminder when history is restored", { - skip_if_not_installed("shiny") - skip_if_not_installed("shinychat") - - shiny::testServer( - function(input, output, session) { - agent <- test_agent() - chat <- commons_server("chat", client = agent) - }, - { - controller <- shinychat:::get_session_chat_bookmark_info( - session, - "chat.history-controller" - ) - controller$restore_app_state(list()) - expect_true(agent$.__enclos_env__$private$restore_reminder_pending) - - chat$clear() - expect_false(agent$.__enclos_env__$private$restore_reminder_pending) - - controller$restore_app_state(list()) - controller$new_chat() - expect_false(agent$.__enclos_env__$private$restore_reminder_pending) - } - ) -}) - test_that("commons_theme() bundles the commons chat assets", { theme <- commons_theme() diff --git a/pkg-r/tests/testthat/test-commons.R b/pkg-r/tests/testthat/test-commons.R index fb68592a..a2915413 100644 --- a/pkg-r/tests/testthat/test-commons.R +++ b/pkg-r/tests/testthat/test-commons.R @@ -662,9 +662,20 @@ test_that("Claude 5 user turns contain one hidden reminder", { ) }) +foreign_turns <- function() { + list( + ellmer::UserTurn("An earlier question."), + ellmer::AssistantTurn( + list(ellmer::ContentText("An earlier answer.")), + tokens = c(0, 0, 0), + cost = 0 + ) + ) +} + test_that("restored conversations add one hidden reminder to the next turn", { agent <- test_agent() - agent$queue_restore_reminder() + agent$set_turns(foreign_turns()) unused_stream <- agent$stream_async("Do not consume this stream.") expect_s3_class(unused_stream, "coro_generator_instance") @@ -700,13 +711,35 @@ test_that("restored conversations add one hidden reminder to the next turn", { test_that("replacing restored history clears its queued reminder", { agent <- test_agent() - agent$queue_restore_reminder() + agent$set_turns(foreign_turns()) + expect_true(agent$.__enclos_env__$private$restore_reminder_pending) agent$set_turns(list()) expect_false(agent$.__enclos_env__$private$restore_reminder_pending) }) +test_that("set_turns queues the restore reminder for foreign history only", { + agent <- test_agent() + stream_citations_fixture(agent, "First answer.", split_at = 5) + turns <- agent$get_turns() + + agent$set_turns(turns[1]) + expect_false(agent$.__enclos_env__$private$restore_reminder_pending) + + agent$set_turns(turns[1]) + expect_false(agent$.__enclos_env__$private$restore_reminder_pending) + + agent$set_turns(foreign_turns()) + expect_true(agent$.__enclos_env__$private$restore_reminder_pending) + + agent$set_turns(foreign_turns()) + expect_true(agent$.__enclos_env__$private$restore_reminder_pending) + + agent$set_turns(list()) + expect_false(agent$.__enclos_env__$private$restore_reminder_pending) +}) + test_that("stream_async projects citations without touching stored turns", { path <- withr::local_tempfile(fileext = ".md") writeLines("Canopy cover is always acre-weighted for reporting.", path)