From fd70a2be5857d0255469c92d98504e4da54e4892 Mon Sep 17 00:00:00 2001 From: skaltman Date: Wed, 12 Aug 2026 15:27:21 -0700 Subject: [PATCH 01/17] Replace JSONL review log with markdown files --- R/trajectory-review-log.R | 533 +++++++++++++++++++++++++++++++------- R/trajectory-review.R | 87 ++++--- man/trajectory_review.Rd | 30 +-- 3 files changed, 499 insertions(+), 151 deletions(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 141c2dd..52d3dcb 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -1,37 +1,125 @@ -new_review_event <- function( +read_review_state <- function(review_dir) { + if (!dir.exists(review_dir)) { + return(list(flags = character(), notes = list())) + } + + files <- list.files( + review_dir, + pattern = "^conversation-.*[.]md$", + full.names = TRUE + ) + documents <- list() + invalid <- character() + + for (file in files) { + document <- read_review_document(file) + if (is.null(document)) { + invalid <- c(invalid, file) + } else { + documents[[length(documents) + 1]] <- document + } + } + + if (length(invalid) > 0) { + cli::cli_warn(c( + "Ignoring {cli::qty(invalid)}malformed trajectory review + document{?s}.", + i = "{.file {invalid}}" + )) + } + + flags <- unlist(lapply(documents, review_document_flags), use.names = FALSE) + notes <- unlist(lapply(documents, review_document_notes), recursive = FALSE) + list( + flags = flags %||% character(), + notes = notes %||% list() + ) +} + +write_conversation_review <- function( + review_dir, trajectories, - key, - action, - user, + conversation, + flags, + notes, source = trajectory_source(trajectories), - note = NULL + call = rlang::caller_env() ) { - exchange <- key$exchange - record <- list( - schema_version = 1L, - event_id = new_review_event_id(), - time = review_timestamp(), - user = user, - source = source, - conversation = names(trajectories)[[key$conversation]], - exchange = exchange, - action = action, - note = note + id <- names(trajectories)[[conversation]] + notes <- Filter(\(note) identical(note$conversation, id), notes) + conversation_flags <- review_conversation_flags( + id, + trajectories[[conversation]], + flags ) + file <- review_document_path(review_dir, id) + + if ( + !conversation_flags$conversation && + length(conversation_flags$exchanges) == 0 && + length(notes) == 0 + ) { + if (file.exists(file) && !file.remove(file)) { + cli::cli_abort( + "Could not remove empty trajectory review {.file {file}}.", + call = call + ) + } + return(invisible(file)) + } - if (!is.null(exchange)) { - turns <- split_exchanges(trajectories[[key$conversation]])[[exchange]] - provenance <- exchange_provenance(turns) - record$question <- turns[[1]]@text - record$tag <- if (is.na(provenance$tag)) "none" else provenance$tag + if (!dir.exists(review_dir)) { + created <- dir.create(review_dir, recursive = TRUE, showWarnings = FALSE) + if (!created && !dir.exists(review_dir)) { + cli::cli_abort( + "Could not create trajectory review directory {.path {review_dir}}.", + call = call + ) + } } - record + markdown <- review_document( + id, + trajectories[[conversation]], + conversation_flags, + notes, + source + ) + temporary <- tempfile( + pattern = ".commons-review-", + tmpdir = review_dir, + fileext = ".md" + ) + on.exit(unlink(temporary), add = TRUE) + + tryCatch( + writeLines(markdown, temporary, useBytes = TRUE), + error = function(error) { + cli::cli_abort( + "Could not write trajectory review {.file {file}}.", + parent = error, + call = call + ) + } + ) + if (!file.rename(temporary, file)) { + cli::cli_abort( + "Could not replace trajectory review {.file {file}}.", + call = call + ) + } + + invisible(file) } -new_review_event_id <- function() { - suffix <- paste(sample(c(letters, 0:9), 12, replace = TRUE), collapse = "") - paste0(format(Sys.time(), "%Y%m%dt%H%M%OS6", tz = "UTC"), "-", suffix) +new_review_note <- function(trajectories, key, user, note) { + list( + time = review_timestamp(), + user = user, + conversation = names(trajectories)[[key$conversation]], + exchange = key$exchange, + note = note + ) } review_timestamp <- function() { @@ -50,103 +138,354 @@ trajectory_source <- function(trajectories) { attr(trajectories, "source") %||% list(kind = "unknown") } -append_review_record <- function(file, record) { - line <- jsonlite::toJSON(drop_nulls(record), auto_unbox = TRUE) - cat(line, "\n", file = file, sep = "", append = TRUE) +review_document <- function( + id, + turns, + flags, + notes, + source, + updated_at = review_timestamp() +) { + metadata <- list( + generated_by = "commons::trajectory_review", + schema_version = 1L, + conversation = id, + source = source, + updated_at = updated_at, + flags = flags, + notes = lapply(notes, review_note_metadata) + ) + frontmatter <- yaml::as.yaml(metadata, indent.mapping.sequence = TRUE) + body <- review_document_body(id, turns, flags, notes, source, updated_at) + + paste0("---\n", frontmatter, "---\n\n", body, "\n") } -read_review_records <- function(file) { - if (!file.exists(file)) { - return(list()) - } +review_document_body <- function(id, turns, flags, notes, source, updated_at) { + exchanges <- split_exchanges(turns) + conversation_notes <- Filter(\(note) is.null(note$exchange), notes) + last_active <- attr(turns, "last_active") %||% as.POSIXct(NA) + source_kind <- source$kind %||% "unknown" + metadata <- c( + sprintf("- Conversation: `%s`", id), + sprintf("- Source: `%s`", source_kind), + if (!is.na(last_active)) { + sprintf("- Last active: %s", format(last_active, "%Y-%m-%dT%H:%M:%S%z")) + }, + sprintf("- Review updated: %s", updated_at) + ) - records <- list() - invalid <- integer() - lines <- readLines(file, warn = FALSE) - for (i in seq_along(lines)) { - record <- tryCatch( - jsonlite::fromJSON(lines[[i]], simplifyVector = FALSE), - error = function(err) NULL + sections <- c( + "# Trajectory review", + "", + "> Generated by `commons::trajectory_review()`. Manual edits are not supported.", + "", + metadata, + "", + "## Conversation review", + "", + sprintf("**Flagged:** %s", review_yes_no(flags$conversation)), + review_notes_markdown(conversation_notes, level = 3L) + ) + + for (i in seq_along(exchanges)) { + exchange_notes <- Filter(\(note) identical(note$exchange, i), notes) + sections <- c( + sections, + review_exchange_markdown( + exchanges[[i]], + i, + i %in% flags$exchanges, + exchange_notes + ) ) - if (!is_review_record(record)) { - invalid <- c(invalid, i) - next + } + + paste(sections, collapse = "\n") +} + +review_exchange_markdown <- function(exchange, number, flagged, notes) { + provenance <- exchange_provenance(exchange) + assistant_text <- unlist(lapply(exchange, turn_text), use.names = FALSE) + tool_sections <- review_tools_markdown(exchange) + citation_section <- review_citations_markdown(provenance$citations) + + c( + "", + sprintf("## Exchange %d", number), + "", + sprintf("**Trust:** %s", review_trust_label(provenance$tag)), + "", + sprintf("**Flagged:** %s", review_yes_no(flagged)), + "", + "### User", + "", + exchange[[1]]@text, + tool_sections, + if (length(assistant_text) > 0) { + c( + "", + "### Assistant", + "", + assistant_text + ) + }, + citation_section, + review_notes_markdown(notes, level = 3L) + ) +} + +review_tools_markdown <- function(exchange) { + sections <- character() + for (turn in exchange) { + for (content in turn@contents) { + if (S7::S7_inherits(content, ellmer::ContentToolRequest)) { + sections <- c( + sections, + "", + sprintf("### Tool call: `%s`", content@name), + "", + "#### Arguments", + "", + "```json", + as.character(jsonlite::toJSON( + content@arguments, + auto_unbox = TRUE, + pretty = TRUE, + null = "null" + )), + "```" + ) + } else if (S7::S7_inherits(content, ellmer::ContentToolResult)) { + request <- content@request + name <- if (is.null(request)) "unknown" else request@name + sections <- c( + sections, + "", + sprintf("### Tool result: `%s`", name), + "", + truncate_review_tool_result(review_tool_result_markdown(content)) + ) + } } - records[[length(records) + 1]] <- record } + sections +} + +review_tool_result_markdown <- function(content) { + display <- content@extra$display + if (is.null(display)) { + display <- viewer_tool_display(content@request, content@value) + } + markdown <- display$markdown %||% NULL + if (rlang::is_string(markdown) && nzchar(markdown)) { + return(markdown) + } + format_review_tool_result(content@value) +} - if (length(invalid) > 0) { - cli::cli_warn( - "Ignoring {cli::qty(invalid)}invalid review record{?s} on line{?s} - {invalid} of {.file {file}}." +format_review_tool_result <- function(value) { + if (is.null(value)) { + return("(empty result)") + } + if (is.data.frame(value)) { + return(df_to_markdown(value)) + } + if (is.character(value)) { + return(paste(value, collapse = "\n")) + } + if (is.atomic(value)) { + return(paste(format(value, trim = TRUE), collapse = ", ")) + } + paste(utils::capture.output(print(value)), collapse = "\n") +} + +truncate_review_tool_result <- function( + result, + max_lines = 100L, + max_chars = 20000L +) { + lines <- strsplit(result, "\n", fixed = TRUE)[[1]] + truncated <- length(lines) > max_lines + lines <- utils::head(lines, max_lines) + result <- paste(lines, collapse = "\n") + + if (nchar(result, type = "chars") > max_chars) { + result <- substr(result, 1L, max_chars) + truncated <- TRUE + } + if (truncated) { + result <- paste(result, "... (tool result truncated)", sep = "\n\n") + } + result +} + +review_citations_markdown <- function(citations) { + if (length(citations) == 0) { + return(character()) + } + + items <- unlist(lapply(citations, function(citation) { + reason <- citation$reason + c( + sprintf("- %s", normalize_citation(citation$quote)), + if (!is.na(reason)) sprintf(" - Reason: %s", flatten_inline(reason)) ) + })) + c("", "### Citations", "", items) +} + +review_notes_markdown <- function(notes, level) { + if (length(notes) == 0) { + return(character()) } - records + + heading <- paste0(strrep("#", level), " Review notes") + rendered <- unlist(lapply(notes, function(note) { + text <- paste0("> ", gsub("\n", "\n> ", note$note, fixed = TRUE)) + c( + "", + text, + ">", + sprintf("> _%s, %s_", note$user, note$time) + ) + })) + c("", heading, rendered) } -is_review_record <- function(record) { - if ( - !is.list(record) || - !rlang::is_string(record$conversation) || - !rlang::is_string(record$action) || - !record$action %in% c("flag", "unflag", "note") - ) { - return(FALSE) +review_note_metadata <- function(note) { + metadata <- list( + user = note$user, + time = note$time, + text = note$note + ) + if (!is.null(note$exchange)) { + metadata$exchange <- note$exchange } - exchange <- record$exchange + metadata +} + +review_conversation_flags <- function(id, turns, flags) { + exchanges <- seq_along(split_exchanges(turns)) + list( + conversation = review_key(id) %in% flags, + exchanges = exchanges[vapply( + exchanges, + \(exchange) review_key(id, exchange) %in% flags, + logical(1) + )] + ) +} + +read_review_document <- function(file) { + lines <- tryCatch( + readLines(file, warn = FALSE, encoding = "UTF-8"), + error = function(error) character() + ) + if (length(lines) < 3 || !identical(lines[[1]], "---")) { + return(NULL) + } + + end <- which(lines[-1] == "---") + if (length(end) == 0) { + return(NULL) + } + end <- end[[1]] + 1L + metadata <- tryCatch( + yaml::yaml.load(paste(lines[seq.int(2L, end - 1L)], collapse = "\n")), + error = function(error) NULL + ) + if (!is_review_document(metadata)) { + return(NULL) + } + metadata +} + +is_review_document <- function(document) { + is.list(document) && + identical(document$generated_by, "commons::trajectory_review") && + identical(document$schema_version, 1L) && + rlang::is_string(document$conversation) && + is.list(document$source) && + rlang::is_string(document$updated_at) && + is_review_flags(document$flags) && + is.list(document$notes) && + all(vapply(document$notes, is_review_note, logical(1))) +} + +is_review_flags <- function(flags) { if ( - !is.null(exchange) && - (!is.numeric(exchange) || - length(exchange) != 1 || - !is.finite(exchange) || - exchange < 1 || - exchange != trunc(exchange)) + !is.list(flags) || + !is.logical(flags$conversation) || + length(flags$conversation) != 1 || + is.na(flags$conversation) ) { return(FALSE) } - !identical(record$action, "note") || rlang::is_string(record$note) + exchanges <- unlist(flags$exchanges, use.names = FALSE) + length(exchanges) == 0 || + all(vapply(exchanges, is_review_exchange, logical(1))) +} + +is_review_note <- function(note) { + is.list(note) && + rlang::is_string(note$user) && + rlang::is_string(note$time) && + rlang::is_string(note$text) && + (is.null(note$exchange) || is_review_exchange(note$exchange)) } -actionable_review_records <- function(records) { - note_indices <- integer() - latest_flag_indices <- integer() +is_review_exchange <- function(exchange) { + is.numeric(exchange) && + length(exchange) == 1 && + is.finite(exchange) && + exchange >= 1 && + exchange == trunc(exchange) +} - for (i in seq_along(records)) { - record <- records[[i]] - if (identical(record$action, "note")) { - note_indices <- c(note_indices, i) - } else { - key <- review_key(record$conversation, record$exchange) - latest_flag_indices[[key]] <- i - } +review_document_flags <- function(document) { + flags <- if (document$flags$conversation) { + review_key(document$conversation) + } else { + character() } + exchanges <- unlist(document$flags$exchanges, use.names = FALSE) + c( + flags, + vapply( + exchanges, + \(exchange) review_key(document$conversation, exchange), + character(1) + ) + ) +} - active_flag_indices <- unname(latest_flag_indices) - active_flag_indices <- active_flag_indices[vapply( - records[active_flag_indices], - function(record) identical(record$action, "flag"), - logical(1) - )] - records[sort(c(note_indices, active_flag_indices))] +review_document_notes <- function(document) { + lapply(document$notes, function(note) { + list( + time = note$time, + user = note$user, + conversation = document$conversation, + exchange = note$exchange, + note = note$text + ) + }) } -review_flags <- function(records) { - active <- Filter( - function(record) identical(record$action, "flag"), - actionable_review_records(records) - ) - vapply( - active, - function(record) review_key(record$conversation, record$exchange), - character(1) - ) +review_document_path <- function(review_dir, id) { + encoded <- utils::URLencode(id, reserved = TRUE) + file.path(review_dir, sprintf("conversation-%s.md", encoded)) } -review_notes <- function(records) { - Filter( - function(record) identical(record$action, "note"), - records - ) +review_trust_label <- function(tag) { + if (is.na(tag)) { + return("No data tool") + } + switch(tag, A = "Verified (A)", B = "Cited (B)", C = "Untrusted (C)") +} + +review_yes_no <- function(value) { + if (value) "Yes" else "No" } review_key <- function(id, exchange = NULL) { diff --git a/R/trajectory-review.R b/R/trajectory-review.R index c750b0f..79b3a0c 100644 --- a/R/trajectory-review.R +++ b/R/trajectory-review.R @@ -12,13 +12,14 @@ #' Transcripts are reviewable rather than live: conversations and questions #' can be flagged for review and annotated with notes. Notes apply to the #' whole conversation, or to a single question-and-answer exchange selected -#' in the transcript. Flags and notes land in `review_file`, one JSON record -#' per line, and are restored when the viewer reopens. +#' in the transcript. Flags and notes land in `review_dir`, one generated +#' Markdown document per reviewed conversation, and are restored when the +#' viewer reopens. #' -#' New review records use schema version 1 and include a unique event id, UTC -#' timestamp, reviewer username, trajectory source, conversation id, optional -#' exchange number, action, and optional note. Exchange-level records also -#' snapshot the question and trust tag. +#' Each Markdown document contains the complete reviewer-visible conversation +#' and its tool activity. YAML frontmatter stores active flags and note history +#' so the reviewer can restore its state. Tool results are limited to 100 lines +#' or 20,000 characters in the rendered document. #' #' Trajectories carry no record of how each answer was tagged when it was #' produced, so the viewer derives trust levels from the tool calls in the @@ -35,16 +36,15 @@ #' #' @param trajectories A named list of conversations, as returned by #' [trajectory_read()]. -#' @param review_file Path of the JSONL file that review actions append to: -#' flags, unflags, and feedback notes, each with a timestamp, the -#' conversation id, and (for questions) the exchange number. Created on -#' first use; flags and notes recorded here are restored when the viewer -#' reopens. Defaults to `COMMONS_REVIEW_FILE` when set. +#' @param review_dir Directory where review actions write one generated +#' Markdown document per conversation. Created on first use; active flags +#' and note history recorded here are restored when the viewer reopens. +#' Defaults to `COMMONS_REVIEW_DIR` when set. #' #' @details -#' A single reviewer app writes all of its review events to `review_file`. For a -#' deployed app, point `COMMONS_REVIEW_FILE` at persistent storage: files in a -#' Posit Connect app's working directory are replaced on redeployment. +#' A single reviewer app writes all of its review documents to `review_dir`. +#' For a deployed app, point `COMMONS_REVIEW_DIR` at persistent storage: files +#' in a Posit Connect app's working directory are replaced on redeployment. #' File-backed review apps should use one Connect process because separate #' processes do not coordinate file writes or in-memory review state. #' @@ -61,21 +61,21 @@ #' @export trajectory_review <- function( trajectories = trajectory_read(), - review_file = Sys.getenv( - "COMMONS_REVIEW_FILE", - unset = "commons-review.jsonl" + review_dir = Sys.getenv( + "COMMONS_REVIEW_DIR", + unset = "commons-reviews" ) ) { check_viewer_packages() check_trajectories(trajectories) - rlang::check_string(review_file) + rlang::check_string(review_dir) source <- trajectory_source(trajectories) trajectories <- drop_side_conversations(trajectories) summary <- summarize_trajectories(trajectories) questions <- summarize_questions(trajectories) shiny::shinyApp( viewer_ui(summary), - viewer_server(trajectories, summary, questions, review_file, source) + viewer_server(trajectories, summary, questions, review_dir, source) ) } @@ -85,7 +85,8 @@ check_viewer_packages <- function(call = rlang::caller_env()) { "htmltools", "plotly", "shiny", - "shinychat" + "shinychat", + "yaml" ) missing <- pkgs[!vapply(pkgs, requireNamespace, logical(1), quietly = TRUE)] @@ -475,13 +476,13 @@ viewer_server <- function( trajectories, summary, questions, - review_file, + review_dir, source = trajectory_source(trajectories) ) { # Share review state across sessions in the documented single process. - review_records <- read_review_records(review_file) - app_flags <- shiny::reactiveVal(review_flags(review_records)) - app_notes <- shiny::reactiveVal(review_notes(review_records)) + review_state <- read_review_state(review_dir) + app_flags <- shiny::reactiveVal(review_state$flags) + app_notes <- shiny::reactiveVal(review_state$notes) function(input, output, session) { flags <- app_flags @@ -622,15 +623,20 @@ viewer_server <- function( } review <- selection_review_key(key, summary) flagged <- review %in% flags() - record <- new_review_event( + next_flags <- if (flagged) { + setdiff(flags(), review) + } else { + union(flags(), review) + } + write_conversation_review( + review_dir, trajectories, - key, - action = if (flagged) "unflag" else "flag", - user = user, - source = source + key$conversation, + next_flags, + notes(), + source ) - append_review_record(review_file, record) - flags(if (flagged) setdiff(flags(), review) else union(flags(), review)) + flags(next_flags) }) shiny::observeEvent(input$exchange_select, { @@ -681,16 +687,19 @@ viewer_server <- function( if (is.null(key) || !nzchar(note)) { return() } - record <- new_review_event( + next_notes <- c( + notes(), + list(new_review_note(trajectories, key, user, note)) + ) + write_conversation_review( + review_dir, trajectories, - key, - action = "note", - user = user, - source = source, - note = note + key$conversation, + flags(), + next_notes, + source ) - append_review_record(review_file, record) - notes(c(notes(), list(record))) + notes(next_notes) bslib::update_submit_textarea( "review_note", value = "", diff --git a/man/trajectory_review.Rd b/man/trajectory_review.Rd index 82fc141..4feb021 100644 --- a/man/trajectory_review.Rd +++ b/man/trajectory_review.Rd @@ -6,18 +6,17 @@ \usage{ trajectory_review( trajectories = trajectory_read(), - review_file = Sys.getenv("COMMONS_REVIEW_FILE", unset = "commons-review.jsonl") + review_dir = Sys.getenv("COMMONS_REVIEW_DIR", unset = "commons-reviews") ) } \arguments{ \item{trajectories}{A named list of conversations, as returned by \code{\link[=trajectory_read]{trajectory_read()}}.} -\item{review_file}{Path of the JSONL file that review actions append to: -flags, unflags, and feedback notes, each with a timestamp, the -conversation id, and (for questions) the exchange number. Created on -first use; flags and notes recorded here are restored when the viewer -reopens. Defaults to \code{COMMONS_REVIEW_FILE} when set.} +\item{review_dir}{Directory where review actions write one generated +Markdown document per conversation. Created on first use; active flags +and note history recorded here are restored when the viewer reopens. +Defaults to \code{COMMONS_REVIEW_DIR} when set.} } \value{ A \code{\link[shiny:shinyApp]{shiny::shinyApp()}} object. Calling \code{trajectory_review()} at the @@ -36,13 +35,14 @@ chat UI would show. Transcripts are reviewable rather than live: conversations and questions can be flagged for review and annotated with notes. Notes apply to the whole conversation, or to a single question-and-answer exchange selected -in the transcript. Flags and notes land in \code{review_file}, one JSON record -per line, and are restored when the viewer reopens. +in the transcript. Flags and notes land in \code{review_dir}, one generated +Markdown document per reviewed conversation, and are restored when the +viewer reopens. -New review records use schema version 1 and include a unique event id, UTC -timestamp, reviewer username, trajectory source, conversation id, optional -exchange number, action, and optional note. Exchange-level records also -snapshot the question and trust tag. +Each Markdown document contains the complete reviewer-visible conversation +and its tool activity. YAML frontmatter stores active flags and note history +so the reviewer can restore its state. Tool results are limited to 100 lines +or 20,000 characters in the rendered document. Trajectories carry no record of how each answer was tagged when it was produced, so the viewer derives trust levels from the tool calls in the @@ -58,9 +58,9 @@ shinychat's conversation-title generation, and completions with no user turn—are excluded from the viewer. } \details{ -A single reviewer app writes all of its review events to \code{review_file}. For a -deployed app, point \code{COMMONS_REVIEW_FILE} at persistent storage: files in a -Posit Connect app's working directory are replaced on redeployment. +A single reviewer app writes all of its review documents to \code{review_dir}. +For a deployed app, point \code{COMMONS_REVIEW_DIR} at persistent storage: files +in a Posit Connect app's working directory are replaced on redeployment. File-backed review apps should use one Connect process because separate processes do not coordinate file writes or in-memory review state. } From 43c5a72124399d161773a9749ba5d37f4ece53c0 Mon Sep 17 00:00:00 2001 From: skaltman Date: Wed, 12 Aug 2026 15:27:35 -0700 Subject: [PATCH 02/17] Update gitignore patterns for commons-reviews directory --- .Rbuildignore | 2 +- .gitignore | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.Rbuildignore b/.Rbuildignore index 48833c1..68bab75 100644 --- a/.Rbuildignore +++ b/.Rbuildignore @@ -14,5 +14,5 @@ CLAUDE.md ^pkgdown$ ^\.github$ inst/hex/ -^commons-review\.jsonl$ +^commons-reviews$ inst/manifest.json diff --git a/.gitignore b/.gitignore index 900a359..350e2e9 100644 --- a/.gitignore +++ b/.gitignore @@ -14,4 +14,4 @@ CLAUDE.local.md AGENTS.override.md inst/hex/output .shinychat/ -commons-review.jsonl +commons-reviews/ From 45ceeb386829ad08bd2dc71a647ba98544c7bb2c Mon Sep 17 00:00:00 2001 From: skaltman Date: Wed, 12 Aug 2026 15:27:44 -0700 Subject: [PATCH 03/17] Refactor tool result formatting and simplify flags structure in trajectory review log --- R/trajectory-review-log.R | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 52d3dcb..fa6e5d8 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -152,7 +152,10 @@ review_document <- function( conversation = id, source = source, updated_at = updated_at, - flags = flags, + flags = list( + conversation = flags$conversation, + exchanges = as.list(flags$exchanges) + ), notes = lapply(notes, review_note_metadata) ) frontmatter <- yaml::as.yaml(metadata, indent.mapping.sequence = TRUE) @@ -264,7 +267,7 @@ review_tools_markdown <- function(exchange) { "", sprintf("### Tool result: `%s`", name), "", - truncate_review_tool_result(review_tool_result_markdown(content)) + truncate_review_tool_result(format_review_tool_result(content@value)) ) } } @@ -272,18 +275,6 @@ review_tools_markdown <- function(exchange) { sections } -review_tool_result_markdown <- function(content) { - display <- content@extra$display - if (is.null(display)) { - display <- viewer_tool_display(content@request, content@value) - } - markdown <- display$markdown %||% NULL - if (rlang::is_string(markdown) && nzchar(markdown)) { - return(markdown) - } - format_review_tool_result(content@value) -} - format_review_tool_result <- function(value) { if (is.null(value)) { return("(empty result)") From 7be4e1877272c9148e4e9194d4692795db6fa5cb Mon Sep 17 00:00:00 2001 From: skaltman Date: Wed, 12 Aug 2026 15:28:01 -0700 Subject: [PATCH 04/17] Update tests for markdown format --- .../testthat/_snaps/trajectory-review-log.md | 7 +- .../trajectory-review-log/review-document.md | 86 ++++++++ tests/testthat/fixtures/review-v1.jsonl | 5 - tests/testthat/test-trajectory-review-log.R | 198 +++++++++++++++--- tests/testthat/test-trajectory-review.R | 36 +--- 5 files changed, 272 insertions(+), 60 deletions(-) create mode 100644 tests/testthat/_snaps/trajectory-review-log/review-document.md delete mode 100644 tests/testthat/fixtures/review-v1.jsonl diff --git a/tests/testthat/_snaps/trajectory-review-log.md b/tests/testthat/_snaps/trajectory-review-log.md index dccadc2..dc2d05d 100644 --- a/tests/testthat/_snaps/trajectory-review-log.md +++ b/tests/testthat/_snaps/trajectory-review-log.md @@ -1,8 +1,9 @@ -# read_review_records ignores malformed records +# review state ignores unrelated Markdown and warns on bad reviews Code - records <- read_review_records(review_file) + state <- read_review_state(review_dir) Condition Warning: - Ignoring invalid review record on line 1 of ''. + Ignoring malformed trajectory review document. + i '/conversation-bad.md' diff --git a/tests/testthat/_snaps/trajectory-review-log/review-document.md b/tests/testthat/_snaps/trajectory-review-log/review-document.md new file mode 100644 index 0000000..e81ad5c --- /dev/null +++ b/tests/testthat/_snaps/trajectory-review-log/review-document.md @@ -0,0 +1,86 @@ +--- +generated_by: commons::trajectory_review +schema_version: 1 +conversation: conv/1 +source: + kind: connect + server: https://connect.example.com + content_guid: 00000000-0000-0000-0000-000000000001 +updated_at: '2026-08-12T16:10:00Z' +flags: + conversation: yes + exchanges: + - 1 +notes: + - user: sara + time: '2026-08-12T16:00:00Z' + text: |- + Check the governed measure. + The SQL is otherwise correct. + exchange: 1 + - user: lee + time: '2026-08-12T16:05:00Z' + text: Review the complete conversation. +--- + +# Trajectory review + +> Generated by `commons::trajectory_review()`. Manual edits are not supported. + +- Conversation: `conv/1` +- Source: `connect` +- Last active: 2026-08-11T09:30:00+0000 +- Review updated: 2026-08-12T16:10:00Z + +## Conversation review + +**Flagged:** Yes + +### Review notes + +> Review the complete conversation. +> +> _lee, 2026-08-12T16:05:00Z_ + +## Exchange 1 + +**Trust:** Cited (B) + +**Flagged:** Yes + +### User + +How many orders? + +### Tool call: `run_sql` + +#### Arguments + +```json +{ + "sql": "select count(*) from orders" +} +``` + +### Tool result: `run_sql` + +6 + +### Assistant + +There are 6 orders. + +Orders are rows. + +### Citations + +- Orders are rows. + - Reason: definition + +### Review notes + +> Check the governed measure. +> The SQL is otherwise correct. +> +> _sara, 2026-08-12T16:00:00Z_ + diff --git a/tests/testthat/fixtures/review-v1.jsonl b/tests/testthat/fixtures/review-v1.jsonl deleted file mode 100644 index ceb7c76..0000000 --- a/tests/testthat/fixtures/review-v1.jsonl +++ /dev/null @@ -1,5 +0,0 @@ -{"schema_version":1,"event_id":"event-1","time":"2026-08-01T16:00:00Z","user":"sara","source":{"kind":"connect","server":"https://connect.example.com","content_guid":"00000000-0000-0000-0000-000000000001"},"conversation":"conv1","action":"flag"} -{"schema_version":1,"event_id":"event-2","time":"2026-08-01T16:01:00Z","user":"sara","source":{"kind":"connect","server":"https://connect.example.com","content_guid":"00000000-0000-0000-0000-000000000001"},"conversation":"conv1","exchange":1,"action":"note","note":"Use the governed orders measure.","question":"How many orders?","tag":"A"} -{"schema_version":1,"event_id":"event-3","time":"2026-08-01T16:02:00Z","user":"sara","source":{"kind":"connect","server":"https://connect.example.com","content_guid":"00000000-0000-0000-0000-000000000001"},"conversation":"conv1","action":"unflag"} -{"schema_version":1,"event_id":"event-4","time":"2026-08-01T16:03:00Z","user":"lee","source":{"kind":"connect","server":"https://connect.example.com","content_guid":"00000000-0000-0000-0000-000000000001"},"conversation":"conv1","exchange":1,"action":"flag","question":"How many orders?","tag":"A"} -{"schema_version":1,"event_id":"event-5","time":"2026-08-01T16:04:00Z","user":"lee","source":{"kind":"connect","server":"https://connect.example.com","content_guid":"00000000-0000-0000-0000-000000000001"},"conversation":"conv1","action":"note","note":"Review the complete conversation."} diff --git a/tests/testthat/test-trajectory-review-log.R b/tests/testthat/test-trajectory-review-log.R index c5e7b10..1ed0b99 100644 --- a/tests/testthat/test-trajectory-review-log.R +++ b/tests/testthat/test-trajectory-review-log.R @@ -1,38 +1,188 @@ -test_that("actionable_review_records returns notes and active flags", { - reviews <- actionable_review_records( - read_review_records(test_path("fixtures", "review-v1.jsonl")) +review_test_turns <- function() { + request <- ellmer::ContentToolRequest( + id = "call-1", + name = "run_sql", + arguments = list(sql = "select count(*) from orders") ) - - expect_equal( - vapply(reviews, function(record) record$event_id, character(1)), - c("event-2", "event-4", "event-5") + turns <- list( + ellmer::UserTurn("How many orders?"), + ellmer::AssistantTurn(list(request)), + ellmer::UserTurn(list(ellmer::ContentToolResult( + value = "6", + request = request + ))), + ellmer::AssistantTurn( + "There are 6 orders.\n\nOrders are rows." + ) ) - expect_equal( - reviews[[1]][c("user", "question", "tag", "source")], + attr(turns, "last_active") <- as.POSIXct( + "2026-08-11 09:30:00", + tz = "UTC" + ) + turns +} + +review_test_notes <- function(id = "conv/1") { + list( list( + time = "2026-08-12T16:00:00Z", user = "sara", - question = "How many orders?", - tag = "A", - source = list( - kind = "connect", - server = "https://connect.example.com", - content_guid = "00000000-0000-0000-0000-000000000001" - ) + conversation = id, + exchange = 1L, + note = "Check the governed measure.\nThe SQL is otherwise correct." + ), + list( + time = "2026-08-12T16:05:00Z", + user = "lee", + conversation = id, + exchange = NULL, + note = "Review the complete conversation." ) ) +} + +test_that("review_document renders a self-contained trajectory review", { + markdown <- review_document( + "conv/1", + review_test_turns(), + list(conversation = TRUE, exchanges = 1L), + review_test_notes(), + source = list( + kind = "connect", + server = "https://connect.example.com", + content_guid = "00000000-0000-0000-0000-000000000001" + ), + updated_at = "2026-08-12T16:10:00Z" + ) + + file <- withr::local_tempfile(fileext = ".md") + writeLines(markdown, file, useBytes = TRUE) + expect_snapshot_file(file, "review-document.md") + expect_match(markdown, "exchanges:\n - 1", fixed = TRUE) }) -test_that("read_review_records ignores malformed records", { - review_file <- withr::local_tempfile( - lines = c( - '{"conversation":"conv1","exchange":1.9,"action":"flag"}', - '{"conversation":"conv1","exchange":1,"action":"flag"}' +test_that("review state round trips through YAML frontmatter", { + review_dir <- withr::local_tempdir() + id <- "conv/1" + file <- review_document_path(review_dir, id) + writeLines( + review_document( + id, + review_test_turns(), + list(conversation = TRUE, exchanges = 1L), + review_test_notes(id), + source = list(kind = "unknown"), + updated_at = "2026-08-12T16:10:00Z" + ), + file + ) + + state <- read_review_state(review_dir) + + expect_equal(state$flags, c("conv/1", "conv/1#1")) + expect_equal(basename(file), "conversation-conv%2F1.md") + expect_length(state$notes, 2) + expect_equal( + state$notes[[1]][c("conversation", "exchange", "user", "note")], + list( + conversation = id, + exchange = 1L, + user = "sara", + note = "Check the governed measure.\nThe SQL is otherwise correct." ) ) +}) + +test_that("review state ignores unrelated Markdown and warns on bad reviews", { + review_dir <- withr::local_tempdir() + writeLines("# Personal notes", file.path(review_dir, "notes.md")) + invalid <- file.path(review_dir, "conversation-bad.md") + writeLines("# Not generated by commons", invalid) expect_snapshot( - records <- read_review_records(review_file), - transform = \(x) gsub(review_file, "", x, fixed = TRUE) + state <- read_review_state(review_dir), + transform = \(x) gsub(review_dir, "", x, fixed = TRUE) + ) + expect_equal(state, list(flags = character(), notes = list())) +}) + +test_that("conversation reviews are created atomically and removed when empty", { + parent <- withr::local_tempdir() + review_dir <- file.path(parent, "reviews") + trajectories <- list(`conv/1` = review_test_turns()) + source <- list(kind = "unknown") + + write_conversation_review( + review_dir, + trajectories, + conversation = 1L, + flags = "conv/1#1", + notes = list(), + source = source + ) + file <- review_document_path(review_dir, "conv/1") + + expect_identical(dir.exists(review_dir), TRUE) + expect_identical(file.exists(file), TRUE) + expect_length(list.files(review_dir, pattern = "^[.]commons-review-"), 0) + + unknown <- review_document_path(review_dir, "unknown") + writeLines( + review_document( + "unknown", + review_test_turns(), + list(conversation = TRUE, exchanges = integer()), + list(), + source, + updated_at = "2026-08-12T16:10:00Z" + ), + unknown + ) + unknown_contents <- readBin(unknown, "raw", n = file.info(unknown)$size) + + write_conversation_review( + review_dir, + trajectories, + conversation = 1L, + flags = character(), + notes = list(), + source = source + ) + + expect_identical(file.exists(file), FALSE) + expect_identical(file.exists(unknown), TRUE) + expect_identical( + readBin(unknown, "raw", n = file.info(unknown)$size), + unknown_contents + ) +}) + +test_that("conversation reviews with notes remain after their final unflag", { + review_dir <- withr::local_tempdir() + trajectories <- list(conv1 = review_test_turns()) + notes <- review_test_notes("conv1")[1] + + write_conversation_review( + review_dir, + trajectories, + conversation = 1L, + flags = character(), + notes = notes + ) + + state <- read_review_state(review_dir) + expect_equal(state$flags, character()) + expect_equal(state$notes, notes) +}) + +test_that("tool results are capped by lines and characters", { + expect_equal( + truncate_review_tool_result(paste(1:101, collapse = "\n"), max_lines = 2), + "1\n2\n\n... (tool result truncated)" + ) + expect_equal( + truncate_review_tool_result("abcdef", max_chars = 3), + "abc\n\n... (tool result truncated)" ) - expect_length(records, 1) + expect_equal(truncate_review_tool_result("short"), "short") }) diff --git a/tests/testthat/test-trajectory-review.R b/tests/testthat/test-trajectory-review.R index 370b86e..b72fa91 100644 --- a/tests/testthat/test-trajectory-review.R +++ b/tests/testthat/test-trajectory-review.R @@ -355,10 +355,10 @@ test_that("the viewer filters conversations and follows selection", { trajectories <- list(conv1 = early, conv2 = late) summary <- summarize_trajectories(trajectories) questions <- summarize_questions(trajectories) - review_file <- withr::local_tempfile(fileext = ".jsonl") + review_dir <- withr::local_tempdir() shiny::testServer( - viewer_server(trajectories, summary, questions, review_file), + viewer_server(trajectories, summary, questions, review_dir), { session$setInputs( group_by = "conversation", @@ -401,7 +401,7 @@ test_that("the viewer filters conversations and follows selection", { ) }) -test_that("flags and notes append to and restore from the review file", { +test_that("flags and notes write to and restore from review documents", { skip_if_not_installed("shiny") skip_if_not_installed("bslib") skip_if_not_installed("shinychat") @@ -411,8 +411,8 @@ test_that("flags and notes append to and restore from the review file", { trajectories <- list(conv1 = turns) summary <- summarize_trajectories(trajectories) questions <- summarize_questions(trajectories) - review_file <- withr::local_tempfile(fileext = ".jsonl") - server <- viewer_server(trajectories, summary, questions, review_file) + review_dir <- withr::local_tempdir() + server <- viewer_server(trajectories, summary, questions, review_dir) shiny::testServer( server, @@ -452,30 +452,10 @@ test_that("flags and notes append to and restore from the review file", { expect_length(notes(), 2) }) - records <- lapply(readLines(review_file), jsonlite::fromJSON) + restored <- read_review_state(review_dir) + expect_equal(restored$flags, c("conv1", "conv1#1")) expect_equal( - vapply(records, function(r) r$action, character(1)), - c("flag", "flag", "note", "note") - ) - expect_equal(records[[2]]$conversation, "conv1") - expect_equal(records[[2]]$exchange, 1) - expect_equal(records[[3]]$note, "Wrong join, should use orders.") - expect_null(records[[4]]$exchange) - expect_equal( - records[[2]][c("schema_version", "user", "source", "question", "tag")], - list( - schema_version = 1L, - user = "unknown", - source = list(kind = "unknown"), - question = "One?", - tag = "none" - ) - ) - - restored <- read_review_records(review_file) - expect_equal(review_flags(restored), c("conv1", "conv1#1")) - expect_equal( - vapply(review_notes(restored), `[[`, character(1), "note"), + vapply(restored$notes, `[[`, character(1), "note"), c("Wrong join, should use orders.", "Reviewed end to end; looks fine.") ) }) From 8ce6704ab5871fa612e8bce9740183a6ed6b3e9e Mon Sep 17 00:00:00 2001 From: skaltman Date: Wed, 12 Aug 2026 17:45:21 -0700 Subject: [PATCH 05/17] Preserve assistant/tool order --- R/trajectory-review-log.R | 80 ++++++++++++--------- tests/testthat/test-trajectory-review-log.R | 48 ++++++++++++- 2 files changed, 95 insertions(+), 33 deletions(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index fa6e5d8..066792f 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -209,8 +209,7 @@ review_document_body <- function(id, turns, flags, notes, source, updated_at) { review_exchange_markdown <- function(exchange, number, flagged, notes) { provenance <- exchange_provenance(exchange) - assistant_text <- unlist(lapply(exchange, turn_text), use.names = FALSE) - tool_sections <- review_tools_markdown(exchange) + contents <- review_exchange_contents_markdown(exchange) citation_section <- review_citations_markdown(provenance$citations) c( @@ -224,57 +223,74 @@ review_exchange_markdown <- function(exchange, number, flagged, notes) { "### User", "", exchange[[1]]@text, - tool_sections, - if (length(assistant_text) > 0) { - c( - "", - "### Assistant", - "", - assistant_text - ) - }, + contents, citation_section, review_notes_markdown(notes, level = 3L) ) } -review_tools_markdown <- function(exchange) { +review_exchange_contents_markdown <- function(exchange) { sections <- character() + assistant_open <- FALSE + for (turn in exchange) { for (content in turn@contents) { - if (S7::S7_inherits(content, ellmer::ContentToolRequest)) { + if ( + identical(turn@role, "assistant") && + S7::S7_inherits(content, ellmer::ContentText) + ) { + if (!assistant_open) { + sections <- c(sections, "", "### Assistant", "") + } + sections <- c(sections, content@text) + assistant_open <- TRUE + } else if (S7::S7_inherits(content, ellmer::ContentToolRequest)) { sections <- c( sections, - "", - sprintf("### Tool call: `%s`", content@name), - "", - "#### Arguments", - "", - "```json", - as.character(jsonlite::toJSON( - content@arguments, - auto_unbox = TRUE, - pretty = TRUE, - null = "null" - )), - "```" + review_tool_request_markdown(content) ) + assistant_open <- FALSE } else if (S7::S7_inherits(content, ellmer::ContentToolResult)) { - request <- content@request - name <- if (is.null(request)) "unknown" else request@name sections <- c( sections, - "", - sprintf("### Tool result: `%s`", name), - "", - truncate_review_tool_result(format_review_tool_result(content@value)) + review_tool_result_markdown(content) ) + assistant_open <- FALSE } } } sections } +review_tool_request_markdown <- function(request) { + c( + "", + sprintf("### Tool call: `%s`", request@name), + "", + "#### Arguments", + "", + "```json", + as.character(jsonlite::toJSON( + request@arguments, + auto_unbox = TRUE, + pretty = TRUE, + null = "null" + )), + "```" + ) +} + +review_tool_result_markdown <- function(result) { + request <- result@request + name <- if (is.null(request)) "unknown" else request@name + c( + "", + sprintf("### Tool result: `%s`", name), + "", + truncate_review_tool_result(format_review_tool_result(result@value)) + ) +} + format_review_tool_result <- function(value) { if (is.null(value)) { return("(empty result)") diff --git a/tests/testthat/test-trajectory-review-log.R b/tests/testthat/test-trajectory-review-log.R index 1ed0b99..150514e 100644 --- a/tests/testthat/test-trajectory-review-log.R +++ b/tests/testthat/test-trajectory-review-log.R @@ -58,7 +58,53 @@ test_that("review_document renders a self-contained trajectory review", { file <- withr::local_tempfile(fileext = ".md") writeLines(markdown, file, useBytes = TRUE) expect_snapshot_file(file, "review-document.md") - expect_match(markdown, "exchanges:\n - 1", fixed = TRUE) +}) + +test_that("review documents preserve assistant and tool content order", { + first <- ellmer::ContentToolRequest( + id = "call-1", + name = "first_tool", + arguments = list() + ) + second <- ellmer::ContentToolRequest( + id = "call-2", + name = "second_tool", + arguments = list() + ) + exchange <- list( + ellmer::UserTurn("Check both sources."), + ellmer::AssistantTurn(list( + ellmer::ContentText("I'll check both sources."), + first, + ellmer::ContentText("The first call is queued."), + second + )), + ellmer::UserTurn(list( + ellmer::ContentToolResult("first result", request = first), + ellmer::ContentToolResult("second result", request = second) + )), + ellmer::AssistantTurn("Both checks are complete.") + ) + + markdown <- review_exchange_markdown( + exchange, + number = 1L, + flagged = FALSE, + notes = list() + ) + + expected <- c( + "I'll check both sources.", + "### Tool call: `first_tool`", + "The first call is queued.", + "### Tool call: `second_tool`", + "### Tool result: `first_tool`", + "first result", + "### Tool result: `second_tool`", + "second result", + "Both checks are complete." + ) + expect_equal(markdown[markdown %in% expected], expected) }) test_that("review state round trips through YAML frontmatter", { From d3adfa159087e73aa6414ff0904fe81a4027e209 Mon Sep 17 00:00:00 2001 From: skaltman Date: Wed, 12 Aug 2026 22:07:46 -0700 Subject: [PATCH 06/17] Replace file atomically with backup and encode conversation IDs as hex --- R/trajectory-review-log.R | 39 +++++++- R/trajectory-review.R | 6 +- man/trajectory_review.Rd | 6 +- tests/testthat/_snaps/commons.new.md | 90 +++++++++++++++++++ .../trajectory-review-log/review-document.md | 2 + tests/testthat/test-trajectory-review-log.R | 45 +++++++++- 6 files changed, 179 insertions(+), 9 deletions(-) create mode 100644 tests/testthat/_snaps/commons.new.md diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 066792f..95777c6 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -102,14 +102,42 @@ write_conversation_review <- function( ) } ) - if (!file.rename(temporary, file)) { + replace_review_file(temporary, file, call = call) + + invisible(file) +} + +replace_review_file <- function( + temporary, + file, + call = rlang::caller_env() +) { + backup <- tempfile( + pattern = ".commons-review-backup-", + tmpdir = dirname(file), + fileext = ".md" + ) + on.exit(unlink(backup), add = TRUE) + + had_file <- file.exists(file) + if (had_file && !file.rename(file, backup)) { cli::cli_abort( - "Could not replace trajectory review {.file {file}}.", + "Could not prepare to replace trajectory review {.file {file}}.", call = call ) } - invisible(file) + if (file.rename(temporary, file)) { + return(invisible(file)) + } + + if (had_file) { + file.rename(backup, file) + } + cli::cli_abort( + "Could not replace trajectory review {.file {file}}.", + call = call + ) } new_review_note <- function(trajectories, key, user, note) { @@ -182,6 +210,8 @@ review_document_body <- function(id, turns, flags, notes, source, updated_at) { "# Trajectory review", "", "> Generated by `commons::trajectory_review()`. Manual edits are not supported.", + "> Agents: parse the YAML frontmatter for review state and use the Markdown", + "> body as the human-readable conversation, tool activity, and review notes.", "", metadata, "", @@ -480,7 +510,8 @@ review_document_notes <- function(document) { } review_document_path <- function(review_dir, id) { - encoded <- utils::URLencode(id, reserved = TRUE) + bytes <- as.integer(charToRaw(enc2utf8(id))) + encoded <- paste(sprintf("%02x", bytes), collapse = "") file.path(review_dir, sprintf("conversation-%s.md", encoded)) } diff --git a/R/trajectory-review.R b/R/trajectory-review.R index 79b3a0c..45abdd7 100644 --- a/R/trajectory-review.R +++ b/R/trajectory-review.R @@ -18,8 +18,10 @@ #' #' Each Markdown document contains the complete reviewer-visible conversation #' and its tool activity. YAML frontmatter stores active flags and note history -#' so the reviewer can restore its state. Tool results are limited to 100 lines -#' or 20,000 characters in the rendered document. +#' so the reviewer can restore its state and agents can identify flagged +#' conversations, exchanges, and reviewer notes. The Markdown body is the +#' human-readable transcript for joint human-agent review. Tool results are +#' limited to 100 lines or 20,000 characters in the rendered document. #' #' Trajectories carry no record of how each answer was tagged when it was #' produced, so the viewer derives trust levels from the tool calls in the diff --git a/man/trajectory_review.Rd b/man/trajectory_review.Rd index 4feb021..888b383 100644 --- a/man/trajectory_review.Rd +++ b/man/trajectory_review.Rd @@ -41,8 +41,10 @@ viewer reopens. Each Markdown document contains the complete reviewer-visible conversation and its tool activity. YAML frontmatter stores active flags and note history -so the reviewer can restore its state. Tool results are limited to 100 lines -or 20,000 characters in the rendered document. +so the reviewer can restore its state and agents can identify flagged +conversations, exchanges, and reviewer notes. The Markdown body is the +human-readable transcript for joint human-agent review. Tool results are +limited to 100 lines or 20,000 characters in the rendered document. Trajectories carry no record of how each answer was tagged when it was produced, so the viewer derives trust levels from the tool calls in the diff --git a/tests/testthat/_snaps/commons.new.md b/tests/testthat/_snaps/commons.new.md new file mode 100644 index 0000000..7dc920f --- /dev/null +++ b/tests/testthat/_snaps/commons.new.md @@ -0,0 +1,90 @@ +# commons() validates its inputs + + Code + commons(client = "not a chat", data_sources = test_source()) + Condition + Error in `commons()`: + ! `client` must be an , e.g. from `ellmer::chat_anthropic()`. + +--- + + Code + commons(client = test_client(), data_sources = "not a source") + Condition + Error in `commons()`: + ! `data_sources` must be a `data_source()` or a named list of them. + +--- + + Code + commons(client = test_client(), data_sources = test_source(), context_layer = "not context") + Message + duckdb is storing downloaded extensions and secrets under ~/.duckdb: + i /Users/saraa/.duckdb + This persists across sessions and is shared with the DuckDB CLI and other clients. + i Run duckdb(shared_home = FALSE) to use a temporary directory instead. + i See ?duckdb_storage for details and alternatives. + Condition + Error in `commons()`: + ! `context_layer` must be a `context_layer()` or `NULL`. + +--- + + Code + commons(client = test_client(), data_sources = test_source(), semantic_layer = list()) + Message + duckdb is storing downloaded extensions and secrets under ~/.duckdb: + i /Users/saraa/.duckdb + This persists across sessions and is shared with the DuckDB CLI and other clients. + i Run duckdb(shared_home = FALSE) to use a temporary directory instead. + i See ?duckdb_storage for details and alternatives. + Condition + Error in `commons()`: + ! `semantic_layer` must be a `semantic_layer()`. + +--- + + Code + commons(client = test_client(), data_sources = test_source(), log = TRUE, + share_with = 1) + Message + duckdb is storing downloaded extensions and secrets under ~/.duckdb: + i /Users/saraa/.duckdb + This persists across sessions and is shared with the DuckDB CLI and other clients. + i Run duckdb(shared_home = FALSE) to use a temporary directory instead. + i See ?duckdb_storage for details and alternatives. + Condition + Error in `commons()`: + ! `share_with` must be a character vector of Connect usernames. + +# commons() errors on injection parameters matching no name + + Code + commons(client = test_client(), data_sources = list(sales_db = test_source()), + semantic_layer = layer) + Message + duckdb is storing downloaded extensions and secrets under ~/.duckdb: + i /Users/saraa/.duckdb + This persists across sessions and is shared with the DuckDB CLI and other clients. + i Run duckdb(shared_home = FALSE) to use a temporary directory instead. + i See ?duckdb_storage for details and alternatives. + Condition + Error in `initialize()`: + ! Measure "region_revenue" has undocumented argument `warehouse` matching no data source. + i Available sources: "sales_db". + +--- + + Code + commons(client = test_client(), data_sources = test_source(), semantic_layer = layer) + Message + duckdb is storing downloaded extensions and secrets under ~/.duckdb: + i /Users/saraa/.duckdb + This persists across sessions and is shared with the DuckDB CLI and other clients. + i Run duckdb(shared_home = FALSE) to use a temporary directory instead. + i See ?duckdb_storage for details and alternatives. + Condition + Error in `initialize()`: + ! Measure "region_revenue" has undocumented argument `warehouse` matching no data source. + i `data_sources` has no named sources. + diff --git a/tests/testthat/_snaps/trajectory-review-log/review-document.md b/tests/testthat/_snaps/trajectory-review-log/review-document.md index e81ad5c..35118c1 100644 --- a/tests/testthat/_snaps/trajectory-review-log/review-document.md +++ b/tests/testthat/_snaps/trajectory-review-log/review-document.md @@ -26,6 +26,8 @@ notes: # Trajectory review > Generated by `commons::trajectory_review()`. Manual edits are not supported. +> Agents: parse the YAML frontmatter for review state and use the Markdown +> body as the human-readable conversation, tool activity, and review notes. - Conversation: `conv/1` - Source: `connect` diff --git a/tests/testthat/test-trajectory-review-log.R b/tests/testthat/test-trajectory-review-log.R index 150514e..a0a4915 100644 --- a/tests/testthat/test-trajectory-review-log.R +++ b/tests/testthat/test-trajectory-review-log.R @@ -126,7 +126,7 @@ test_that("review state round trips through YAML frontmatter", { state <- read_review_state(review_dir) expect_equal(state$flags, c("conv/1", "conv/1#1")) - expect_equal(basename(file), "conversation-conv%2F1.md") + expect_equal(basename(file), "conversation-636f6e762f31.md") expect_length(state$notes, 2) expect_equal( state$notes[[1]][c("conversation", "exchange", "user", "note")], @@ -203,6 +203,49 @@ test_that("conversation reviews are created atomically and removed when empty", ) }) +test_that("conversation reviews replace an existing file", { + review_dir <- withr::local_tempdir() + trajectories <- list(conv1 = review_test_turns()) + + write_conversation_review( + review_dir, + trajectories, + conversation = 1L, + flags = "conv1", + notes = list() + ) + write_conversation_review( + review_dir, + trajectories, + conversation = 1L, + flags = "conv1#1", + notes = list() + ) + + state <- read_review_state(review_dir) + expect_equal(state$flags, "conv1#1") + expect_length( + list.files(review_dir, pattern = "^[.]commons-review-backup-"), + 0 + ) +}) + +test_that("conversation ids map to distinct safe filenames", { + review_dir <- withr::local_tempdir() + ids <- c("x/y", "x%2Fy") + files <- vapply( + ids, + \(id) review_document_path(review_dir, id), + character(1) + ) + + expect_length(unique(files), 2) + expect_equal( + basename(files), + c("conversation-782f79.md", "conversation-7825324679.md") + ) +}) + test_that("conversation reviews with notes remain after their final unflag", { review_dir <- withr::local_tempdir() trajectories <- list(conv1 = review_test_turns()) From 84edf1224f885402a882c2381b1ee8197f8ceae2 Mon Sep 17 00:00:00 2001 From: skaltman Date: Wed, 12 Aug 2026 22:13:34 -0700 Subject: [PATCH 07/17] Consolidate tests --- R/trajectory-review-log.R | 29 +++++----- tests/testthat/test-trajectory-review-log.R | 60 ++++++--------------- 2 files changed, 30 insertions(+), 59 deletions(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 95777c6..dea788c 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -112,26 +112,27 @@ replace_review_file <- function( file, call = rlang::caller_env() ) { - backup <- tempfile( - pattern = ".commons-review-backup-", - tmpdir = dirname(file), - fileext = ".md" - ) - on.exit(unlink(backup), add = TRUE) - - had_file <- file.exists(file) - if (had_file && !file.rename(file, backup)) { - cli::cli_abort( - "Could not prepare to replace trajectory review {.file {file}}.", - call = call + backup <- NULL + if (file.exists(file)) { + backup <- tempfile( + pattern = ".commons-review-backup-", + tmpdir = dirname(file), + fileext = ".md" ) + on.exit(unlink(backup), add = TRUE) + if (!file.rename(file, backup)) { + cli::cli_abort( + "Could not replace trajectory review {.file {file}}.", + call = call + ) + } } if (file.rename(temporary, file)) { - return(invisible(file)) + return(invisible()) } - if (had_file) { + if (!is.null(backup)) { file.rename(backup, file) } cli::cli_abort( diff --git a/tests/testthat/test-trajectory-review-log.R b/tests/testthat/test-trajectory-review-log.R index a0a4915..68ef1d1 100644 --- a/tests/testthat/test-trajectory-review-log.R +++ b/tests/testthat/test-trajectory-review-log.R @@ -126,7 +126,10 @@ test_that("review state round trips through YAML frontmatter", { state <- read_review_state(review_dir) expect_equal(state$flags, c("conv/1", "conv/1#1")) - expect_equal(basename(file), "conversation-636f6e762f31.md") + expect_length( + unique(c(file, review_document_path(review_dir, "conv%2F1"))), + 2 + ) expect_length(state$notes, 2) expect_equal( state$notes[[1]][c("conversation", "exchange", "user", "note")], @@ -152,7 +155,7 @@ test_that("review state ignores unrelated Markdown and warns on bad reviews", { expect_equal(state, list(flags = character(), notes = list())) }) -test_that("conversation reviews are created atomically and removed when empty", { +test_that("conversation reviews are created, replaced, and removed", { parent <- withr::local_tempdir() review_dir <- file.path(parent, "reviews") trajectories <- list(`conv/1` = review_test_turns()) @@ -172,6 +175,16 @@ test_that("conversation reviews are created atomically and removed when empty", expect_identical(file.exists(file), TRUE) expect_length(list.files(review_dir, pattern = "^[.]commons-review-"), 0) + write_conversation_review( + review_dir, + trajectories, + conversation = 1L, + flags = "conv/1", + notes = list(), + source = source + ) + expect_equal(read_review_state(review_dir)$flags, "conv/1") + unknown <- review_document_path(review_dir, "unknown") writeLines( review_document( @@ -203,49 +216,6 @@ test_that("conversation reviews are created atomically and removed when empty", ) }) -test_that("conversation reviews replace an existing file", { - review_dir <- withr::local_tempdir() - trajectories <- list(conv1 = review_test_turns()) - - write_conversation_review( - review_dir, - trajectories, - conversation = 1L, - flags = "conv1", - notes = list() - ) - write_conversation_review( - review_dir, - trajectories, - conversation = 1L, - flags = "conv1#1", - notes = list() - ) - - state <- read_review_state(review_dir) - expect_equal(state$flags, "conv1#1") - expect_length( - list.files(review_dir, pattern = "^[.]commons-review-backup-"), - 0 - ) -}) - -test_that("conversation ids map to distinct safe filenames", { - review_dir <- withr::local_tempdir() - ids <- c("x/y", "x%2Fy") - files <- vapply( - ids, - \(id) review_document_path(review_dir, id), - character(1) - ) - - expect_length(unique(files), 2) - expect_equal( - basename(files), - c("conversation-782f79.md", "conversation-7825324679.md") - ) -}) - test_that("conversation reviews with notes remain after their final unflag", { review_dir <- withr::local_tempdir() trajectories <- list(conv1 = review_test_turns()) From 1620a63276a24b469c7ba683c5d4b1519fed77d7 Mon Sep 17 00:00:00 2001 From: skaltman Date: Wed, 12 Aug 2026 22:31:40 -0700 Subject: [PATCH 08/17] Reduce default max_lines from 100 to 50 in truncate_review_tool_result --- R/trajectory-review-log.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index dea788c..8452cfe 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -340,7 +340,7 @@ format_review_tool_result <- function(value) { truncate_review_tool_result <- function( result, - max_lines = 100L, + max_lines = 50L, max_chars = 20000L ) { lines <- strsplit(result, "\n", fixed = TRUE)[[1]] From 96a13940be7754781dce0d6cdbc6942d3ca50ea4 Mon Sep 17 00:00:00 2001 From: skaltman Date: Thu, 13 Aug 2026 14:51:05 -0700 Subject: [PATCH 09/17] Update trajectory_review documentation: reduce tool result line limit and expand configuration details --- R/trajectory-review.R | 24 +++++++++++++++++++----- man/trajectory_review.Rd | 24 +++++++++++++++++++----- 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/R/trajectory-review.R b/R/trajectory-review.R index 45abdd7..cedbcb5 100644 --- a/R/trajectory-review.R +++ b/R/trajectory-review.R @@ -21,7 +21,7 @@ #' so the reviewer can restore its state and agents can identify flagged #' conversations, exchanges, and reviewer notes. The Markdown body is the #' human-readable transcript for joint human-agent review. Tool results are -#' limited to 100 lines or 20,000 characters in the rendered document. +#' limited to 50 lines or 20,000 characters in the rendered document. #' #' Trajectories carry no record of how each answer was tagged when it was #' produced, so the viewer derives trust levels from the tool calls in the @@ -45,10 +45,21 @@ #' #' @details #' A single reviewer app writes all of its review documents to `review_dir`. -#' For a deployed app, point `COMMONS_REVIEW_DIR` at persistent storage: files -#' in a Posit Connect app's working directory are replaced on redeployment. -#' File-backed review apps should use one Connect process because separate -#' processes do not coordinate file writes or in-memory review state. +#' Pass `review_dir` directly, set `COMMONS_REVIEW_DIR` for the current R +#' process with `Sys.setenv()`, or add it to `.Renviron` to keep the setting +#' across local R sessions. Without either, reviews land in `commons-reviews` +#' relative to the app's working directory. +#' +#' On Posit Connect, configure `COMMONS_REVIEW_DIR` to name writable persistent +#' storage available to the reviewer app. The default working-directory path +#' is replaced when the content is redeployed. File-backed review apps should +#' use one Connect process because separate processes do not coordinate file +#' writes or in-memory review state. +#' +#' All sessions of one reviewer app share the same `review_dir`, flags, and +#' notes; review state is not separated by user. Notes record `session$user`, +#' the login information supplied by the Shiny host, or `"unknown"` when it is +#' unavailable. Flags do not record who changed them. #' #' @return A [shiny::shinyApp()] object. Calling `trajectory_review()` at the #' console launches the reviewer; the result can also be served as the last @@ -59,6 +70,9 @@ #' trajectory_review() #' #' trajectory_review(trajectory_read(from = "2026-07-01")) +#' +#' Sys.setenv(COMMONS_REVIEW_DIR = "/path/to/persistent/reviews") +#' trajectory_review() #' } #' @export trajectory_review <- function( diff --git a/man/trajectory_review.Rd b/man/trajectory_review.Rd index 888b383..f6bba67 100644 --- a/man/trajectory_review.Rd +++ b/man/trajectory_review.Rd @@ -44,7 +44,7 @@ and its tool activity. YAML frontmatter stores active flags and note history so the reviewer can restore its state and agents can identify flagged conversations, exchanges, and reviewer notes. The Markdown body is the human-readable transcript for joint human-agent review. Tool results are -limited to 100 lines or 20,000 characters in the rendered document. +limited to 50 lines or 20,000 characters in the rendered document. Trajectories carry no record of how each answer was tagged when it was produced, so the viewer derives trust levels from the tool calls in the @@ -61,15 +61,29 @@ turn—are excluded from the viewer. } \details{ A single reviewer app writes all of its review documents to \code{review_dir}. -For a deployed app, point \code{COMMONS_REVIEW_DIR} at persistent storage: files -in a Posit Connect app's working directory are replaced on redeployment. -File-backed review apps should use one Connect process because separate -processes do not coordinate file writes or in-memory review state. +Pass \code{review_dir} directly, set \code{COMMONS_REVIEW_DIR} for the current R +process with \code{Sys.setenv()}, or add it to \code{.Renviron} to keep the setting +across local R sessions. Without either, reviews land in \code{commons-reviews} +relative to the app's working directory. + +On Posit Connect, configure \code{COMMONS_REVIEW_DIR} to name writable persistent +storage available to the reviewer app. The default working-directory path +is replaced when the content is redeployed. File-backed review apps should +use one Connect process because separate processes do not coordinate file +writes or in-memory review state. + +All sessions of one reviewer app share the same \code{review_dir}, flags, and +notes; review state is not separated by user. Notes record \code{session$user}, +the login information supplied by the Shiny host, or \code{"unknown"} when it is +unavailable. Flags do not record who changed them. } \examples{ \dontrun{ trajectory_review() trajectory_review(trajectory_read(from = "2026-07-01")) + +Sys.setenv(COMMONS_REVIEW_DIR = "/path/to/persistent/reviews") +trajectory_review() } } From 30463c3fcaea8f01bb7d542babced31695b6e3f8 Mon Sep 17 00:00:00 2001 From: skaltman Date: Fri, 14 Aug 2026 11:32:49 -0700 Subject: [PATCH 10/17] Add pin-backed storage and download functionality for trajectory reviews --- R/trajectory-review-log.R | 145 +++++++++++++++++++- R/trajectory-review.R | 59 ++++++-- man/trajectory_review.Rd | 32 ++++- tests/testthat/test-trajectory-review-log.R | 38 +++++ tests/testthat/test-trajectory-review.R | 16 ++- 5 files changed, 264 insertions(+), 26 deletions(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 8452cfe..2699b99 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -3,11 +3,7 @@ read_review_state <- function(review_dir) { return(list(flags = character(), notes = list())) } - files <- list.files( - review_dir, - pattern = "^conversation-.*[.]md$", - full.names = TRUE - ) + files <- review_document_files(review_dir) documents <- list() invalid <- character() @@ -36,6 +32,145 @@ read_review_state <- function(review_dir) { ) } +review_store <- function( + review_dir, + review_board = NULL, + review_pin = NULL, + call = rlang::caller_env() +) { + if (xor(is.null(review_board), is.null(review_pin))) { + cli::cli_abort( + "{.arg review_board} and {.arg review_pin} must be supplied together.", + call = call + ) + } + if (!is.null(review_pin)) { + rlang::check_string(review_pin, call = call) + if (!requireNamespace("pins", quietly = TRUE)) { + cli::cli_abort( + "{.fn trajectory_review} requires {.pkg pins} for pin-backed review storage.", + call = call + ) + } + } + list( + review_dir = review_dir, + board = review_board, + pin = review_pin + ) +} + +hydrate_review_store <- function(store, call = rlang::caller_env()) { + if (is.null(store$board) || !pins::pin_exists(store$board, store$pin)) { + return(invisible(store)) + } + + files <- tryCatch( + { + versions <- pins::pin_versions(store$board, store$pin) + pins::pin_download( + store$board, + store$pin, + version = versions$version[[1]] + ) + }, + error = function(error) { + cli::cli_abort( + "Could not download trajectory reviews from pin {.val {store$pin}}.", + parent = error, + call = call + ) + } + ) + unlink(review_document_files(store$review_dir)) + dir.create(store$review_dir, recursive = TRUE, showWarnings = FALSE) + reviews <- files[grepl("^conversation-.*[.]md$", basename(files))] + if (length(reviews) > 0) { + file.copy(reviews, store$review_dir, overwrite = TRUE) + } + invisible(store) +} + +sync_review_store <- function(store, call = rlang::caller_env()) { + if (is.null(store$board)) { + return(invisible(store)) + } + + staging <- tempfile("commons-review-pin-") + dir.create(staging) + on.exit(unlink(staging, recursive = TRUE), add = TRUE) + writeLines( + "Generated by commons::trajectory_review().", + file.path(staging, "README.txt") + ) + files <- review_document_files(store$review_dir) + if (length(files) > 0) { + file.copy(files, staging) + } + + tryCatch( + pins::pin_upload( + store$board, + staging, + name = store$pin, + title = "Commons trajectory reviews", + description = "Markdown review documents generated by commons." + ), + error = function(error) { + cli::cli_abort( + "Could not upload trajectory reviews to pin {.val {store$pin}}.", + parent = error, + call = call + ) + } + ) + invisible(store) +} + +write_review_archive <- function( + review_dir, + file, + call = rlang::caller_env() +) { + staging <- tempfile("commons-reviews-") + dir.create(file.path(staging, "commons-reviews"), recursive = TRUE) + on.exit(unlink(staging, recursive = TRUE), add = TRUE) + + files <- review_document_files(review_dir) + if (length(files) > 0) { + file.copy(files, file.path(staging, "commons-reviews")) + } + + old <- setwd(staging) + on.exit(setwd(old), add = TRUE) + tryCatch( + utils::tar( + file, + files = "commons-reviews", + compression = "gzip", + tar = "internal" + ), + error = function(error) { + cli::cli_abort( + "Could not create the trajectory review archive.", + parent = error, + call = call + ) + } + ) +} + +review_document_files <- function(review_dir) { + if (!dir.exists(review_dir)) { + return(character()) + } + list.files( + review_dir, + pattern = "^conversation-.*[.]md$", + full.names = TRUE + ) +} + write_conversation_review <- function( review_dir, trajectories, diff --git a/R/trajectory-review.R b/R/trajectory-review.R index cedbcb5..13c443a 100644 --- a/R/trajectory-review.R +++ b/R/trajectory-review.R @@ -41,7 +41,12 @@ #' @param review_dir Directory where review actions write one generated #' Markdown document per conversation. Created on first use; active flags #' and note history recorded here are restored when the viewer reopens. -#' Defaults to `COMMONS_REVIEW_DIR` when set. +#' Defaults to `COMMONS_REVIEW_DIR` when set. When `review_board` and +#' `review_pin` are supplied, this directory is a local cache of the pin. +#' @param review_board Optional pins board used for durable review storage, +#' such as `pins::board_connect()`. Must be supplied with `review_pin`. +#' @param review_pin Name of a pin containing the review documents. Must be +#' supplied with `review_board`. #' #' @details #' A single reviewer app writes all of its review documents to `review_dir`. @@ -50,11 +55,21 @@ #' across local R sessions. Without either, reviews land in `commons-reviews` #' relative to the app's working directory. #' -#' On Posit Connect, configure `COMMONS_REVIEW_DIR` to name writable persistent -#' storage available to the reviewer app. The default working-directory path -#' is replaced when the content is redeployed. File-backed review apps should -#' use one Connect process because separate processes do not coordinate file -#' writes or in-memory review state. +#' On Posit Connect, use a Connect pin for durable storage across deployments: +#' +#' ``` +#' trajectory_review( +#' trajectories, +#' review_board = pins::board_connect(), +#' review_pin = "agent-reviews" +#' ) +#' ``` +#' +#' The reviewer downloads the pin at startup and uploads a new pin version +#' after each review action. Without a pin, the default working-directory path +#' is replaced when the content is redeployed. Review apps should use one +#' Connect process because separate processes do not coordinate writes or +#' in-memory review state. #' #' All sessions of one reviewer app share the same `review_dir`, flags, and #' notes; review state is not separated by user. Notes record `session$user`, @@ -80,18 +95,22 @@ trajectory_review <- function( review_dir = Sys.getenv( "COMMONS_REVIEW_DIR", unset = "commons-reviews" - ) + ), + review_board = NULL, + review_pin = NULL ) { check_viewer_packages() check_trajectories(trajectories) rlang::check_string(review_dir) + store <- review_store(review_dir, review_board, review_pin) + hydrate_review_store(store) source <- trajectory_source(trajectories) trajectories <- drop_side_conversations(trajectories) summary <- summarize_trajectories(trajectories) questions <- summarize_questions(trajectories) shiny::shinyApp( viewer_ui(summary), - viewer_server(trajectories, summary, questions, review_dir, source) + viewer_server(trajectories, summary, questions, store, source) ) } @@ -418,6 +437,11 @@ viewer_ui <- function(summary) { "Trust Level", trust_choices("conversation") ), + shiny::downloadButton( + "download_reviews", + "Download reviews", + icon = shiny::icon("download") + ), shiny::uiOutput("entries") ), trust_timeline_card(), @@ -492,11 +516,11 @@ viewer_server <- function( trajectories, summary, questions, - review_dir, + store, source = trajectory_source(trajectories) ) { # Share review state across sessions in the documented single process. - review_state <- read_review_state(review_dir) + review_state <- read_review_state(store$review_dir) app_flags <- shiny::reactiveVal(review_state$flags) app_notes <- shiny::reactiveVal(review_state$notes) @@ -517,6 +541,15 @@ viewer_server <- function( }) user <- review_user(session) + output$download_reviews <- shiny::downloadHandler( + filename = function() { + sprintf("commons-reviews-%s.tar.gz", Sys.Date()) + }, + content = function(file) { + write_review_archive(store$review_dir, file) + } + ) + output$review_ready <- shiny::renderText({ if (is.null(review_selection())) "" else "ready" }) @@ -645,13 +678,14 @@ viewer_server <- function( union(flags(), review) } write_conversation_review( - review_dir, + store$review_dir, trajectories, key$conversation, next_flags, notes(), source ) + sync_review_store(store) flags(next_flags) }) @@ -708,13 +742,14 @@ viewer_server <- function( list(new_review_note(trajectories, key, user, note)) ) write_conversation_review( - review_dir, + store$review_dir, trajectories, key$conversation, flags(), next_notes, source ) + sync_review_store(store) notes(next_notes) bslib::update_submit_textarea( "review_note", diff --git a/man/trajectory_review.Rd b/man/trajectory_review.Rd index f6bba67..e5d7785 100644 --- a/man/trajectory_review.Rd +++ b/man/trajectory_review.Rd @@ -6,7 +6,9 @@ \usage{ trajectory_review( trajectories = trajectory_read(), - review_dir = Sys.getenv("COMMONS_REVIEW_DIR", unset = "commons-reviews") + review_dir = Sys.getenv("COMMONS_REVIEW_DIR", unset = "commons-reviews"), + review_board = NULL, + review_pin = NULL ) } \arguments{ @@ -16,7 +18,14 @@ trajectory_review( \item{review_dir}{Directory where review actions write one generated Markdown document per conversation. Created on first use; active flags and note history recorded here are restored when the viewer reopens. -Defaults to \code{COMMONS_REVIEW_DIR} when set.} +Defaults to \code{COMMONS_REVIEW_DIR} when set. When \code{review_board} and +\code{review_pin} are supplied, this directory is a local cache of the pin.} + +\item{review_board}{Optional pins board used for durable review storage, +such as \code{pins::board_connect()}. Must be supplied with \code{review_pin}.} + +\item{review_pin}{Name of a pin containing the review documents. Must be +supplied with \code{review_board}.} } \value{ A \code{\link[shiny:shinyApp]{shiny::shinyApp()}} object. Calling \code{trajectory_review()} at the @@ -66,11 +75,20 @@ process with \code{Sys.setenv()}, or add it to \code{.Renviron} to keep the sett across local R sessions. Without either, reviews land in \code{commons-reviews} relative to the app's working directory. -On Posit Connect, configure \code{COMMONS_REVIEW_DIR} to name writable persistent -storage available to the reviewer app. The default working-directory path -is replaced when the content is redeployed. File-backed review apps should -use one Connect process because separate processes do not coordinate file -writes or in-memory review state. +On Posit Connect, use a Connect pin for durable storage across deployments: + +\if{html}{\out{
}}\preformatted{trajectory_review( + trajectories, + review_board = pins::board_connect(), + review_pin = "agent-reviews" +) +}\if{html}{\out{
}} + +The reviewer downloads the pin at startup and uploads a new pin version +after each review action. Without a pin, the default working-directory path +is replaced when the content is redeployed. Review apps should use one +Connect process because separate processes do not coordinate writes or +in-memory review state. All sessions of one reviewer app share the same \code{review_dir}, flags, and notes; review state is not separated by user. Notes record \code{session$user}, diff --git a/tests/testthat/test-trajectory-review-log.R b/tests/testthat/test-trajectory-review-log.R index 68ef1d1..3e5b557 100644 --- a/tests/testthat/test-trajectory-review-log.R +++ b/tests/testthat/test-trajectory-review-log.R @@ -142,6 +142,44 @@ test_that("review state round trips through YAML frontmatter", { ) }) +test_that("review archives contain generated documents", { + review_dir <- withr::local_tempdir() + writeLines("review one", file.path(review_dir, "conversation-one.md")) + writeLines("not a review", file.path(review_dir, "notes.md")) + archive <- tempfile(fileext = ".tar.gz") + + write_review_archive(review_dir, archive) + + expect_equal( + utils::untar(archive, list = TRUE), + "commons-reviews/conversation-one.md" + ) +}) + +test_that("pin-backed review stores sync and restore documents", { + skip_if_not_installed("pins") + board <- pins::board_folder(withr::local_tempdir(), versioned = TRUE) + source_dir <- withr::local_tempdir() + restored_dir <- withr::local_tempdir() + store <- review_store(source_dir, board, "agent-reviews") + writeLines("review one", file.path(source_dir, "conversation-one.md")) + + sync_review_store(store) + hydrate_review_store(review_store(restored_dir, board, "agent-reviews")) + + expect_equal( + readLines(file.path(restored_dir, "conversation-one.md")), + "review one" + ) + + unlink(file.path(source_dir, "conversation-one.md")) + sync_review_store(store) + hydrate_review_store(review_store(restored_dir, board, "agent-reviews")) + + expect_length(review_document_files(restored_dir), 0) + expect_length(pins::pin_versions(board, "agent-reviews")$version, 2) +}) + test_that("review state ignores unrelated Markdown and warns on bad reviews", { review_dir <- withr::local_tempdir() writeLines("# Personal notes", file.path(review_dir, "notes.md")) diff --git a/tests/testthat/test-trajectory-review.R b/tests/testthat/test-trajectory-review.R index b72fa91..b445893 100644 --- a/tests/testthat/test-trajectory-review.R +++ b/tests/testthat/test-trajectory-review.R @@ -358,7 +358,12 @@ test_that("the viewer filters conversations and follows selection", { review_dir <- withr::local_tempdir() shiny::testServer( - viewer_server(trajectories, summary, questions, review_dir), + viewer_server( + trajectories, + summary, + questions, + review_store(review_dir) + ), { session$setInputs( group_by = "conversation", @@ -412,7 +417,12 @@ test_that("flags and notes write to and restore from review documents", { summary <- summarize_trajectories(trajectories) questions <- summarize_questions(trajectories) review_dir <- withr::local_tempdir() - server <- viewer_server(trajectories, summary, questions, review_dir) + server <- viewer_server( + trajectories, + summary, + questions, + review_store(review_dir) + ) shiny::testServer( server, @@ -621,6 +631,8 @@ test_that("viewer_ui uses bslib's resizable review sidebar", { html <- as.character(viewer_ui(list())) expect_match(html, "shiny-date-range-input") + expect_match(html, "download_reviews") + expect_match(html, "Download reviews") expect_match(html, "bslib-sidebar-layout") expect_match(html, "sidebar-right") expect_match(html, "data-resizable") From d9481e5f883b8ca28d21a7b96779d89025b08b46 Mon Sep 17 00:00:00 2001 From: skaltman Date: Fri, 14 Aug 2026 14:18:50 -0700 Subject: [PATCH 11/17] Simplify trajectory review document generation and improve markdown formatting --- R/trajectory-review-log.R | 106 +++++++++++++++++++------------------- R/trajectory-review.R | 41 +++++++++------ man/trajectory_review.Rd | 29 ++++++++--- 3 files changed, 99 insertions(+), 77 deletions(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 2699b99..94c2956 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -177,7 +177,6 @@ write_conversation_review <- function( conversation, flags, notes, - source = trajectory_source(trajectories), call = rlang::caller_env() ) { id <- names(trajectories)[[conversation]] @@ -217,8 +216,7 @@ write_conversation_review <- function( id, trajectories[[conversation]], conversation_flags, - notes, - source + notes ) temporary <- tempfile( pattern = ".commons-review-", @@ -298,63 +296,52 @@ review_user <- function(session) { user } -trajectory_source <- function(trajectories) { - attr(trajectories, "source") %||% list(kind = "unknown") -} - review_document <- function( id, turns, flags, notes, - source, updated_at = review_timestamp() ) { + last_active <- attr(turns, "last_active") %||% as.POSIXct(NA) metadata <- list( generated_by = "commons::trajectory_review", schema_version = 1L, - conversation = id, - source = source, - updated_at = updated_at, - flags = list( - conversation = flags$conversation, - exchanges = as.list(flags$exchanges) - ), - notes = lapply(notes, review_note_metadata) + conversation = id + ) + if (!is.na(last_active)) { + metadata$last_active <- format(last_active, "%Y-%m-%dT%H:%M:%S%z") + } + metadata <- c( + metadata, + list( + updated_at = updated_at, + flags = list( + conversation = flags$conversation, + exchanges = as.list(flags$exchanges) + ), + notes = lapply(notes, review_note_metadata) + ) ) frontmatter <- yaml::as.yaml(metadata, indent.mapping.sequence = TRUE) - body <- review_document_body(id, turns, flags, notes, source, updated_at) + body <- review_document_body(id, turns, flags, notes) paste0("---\n", frontmatter, "---\n\n", body, "\n") } -review_document_body <- function(id, turns, flags, notes, source, updated_at) { +review_document_body <- function(id, turns, flags, notes) { exchanges <- split_exchanges(turns) conversation_notes <- Filter(\(note) is.null(note$exchange), notes) - last_active <- attr(turns, "last_active") %||% as.POSIXct(NA) - source_kind <- source$kind %||% "unknown" - metadata <- c( - sprintf("- Conversation: `%s`", id), - sprintf("- Source: `%s`", source_kind), - if (!is.na(last_active)) { - sprintf("- Last active: %s", format(last_active, "%Y-%m-%dT%H:%M:%S%z")) - }, - sprintf("- Review updated: %s", updated_at) - ) sections <- c( - "# Trajectory review", - "", - "> Generated by `commons::trajectory_review()`. Manual edits are not supported.", - "> Agents: parse the YAML frontmatter for review state and use the Markdown", - "> body as the human-readable conversation, tool activity, and review notes.", + sprintf("# %s", flatten_inline(id)), "", - metadata, + "", "", - "## Conversation review", + "## Conversation-level review", "", sprintf("**Flagged:** %s", review_yes_no(flags$conversation)), - review_notes_markdown(conversation_notes, level = 3L) + review_notes_markdown(conversation_notes) ) for (i in seq_along(exchanges)) { @@ -385,13 +372,13 @@ review_exchange_markdown <- function(exchange, number, flagged, notes) { sprintf("**Trust:** %s", review_trust_label(provenance$tag)), "", sprintf("**Flagged:** %s", review_yes_no(flagged)), + review_notes_markdown(notes), "", "### User", "", - exchange[[1]]@text, + review_blockquote(exchange[[1]]@text), contents, - citation_section, - review_notes_markdown(notes, level = 3L) + citation_section ) } @@ -408,7 +395,7 @@ review_exchange_contents_markdown <- function(exchange) { if (!assistant_open) { sections <- c(sections, "", "### Assistant", "") } - sections <- c(sections, content@text) + sections <- c(sections, review_blockquote(content@text)) assistant_open <- TRUE } else if (S7::S7_inherits(content, ellmer::ContentToolRequest)) { sections <- c( @@ -431,9 +418,11 @@ review_exchange_contents_markdown <- function(exchange) { review_tool_request_markdown <- function(request) { c( "", - sprintf("### Tool call: `%s`", request@name), - "", - "#### Arguments", + "
", + sprintf( + "Tool call: %s", + request@name + ), "", "```json", as.character(jsonlite::toJSON( @@ -442,7 +431,9 @@ review_tool_request_markdown <- function(request) { pretty = TRUE, null = "null" )), - "```" + "```", + "", + "
" ) } @@ -451,9 +442,15 @@ review_tool_result_markdown <- function(result) { name <- if (is.null(request)) "unknown" else request@name c( "", - sprintf("### Tool result: `%s`", name), + "
", + sprintf( + "Tool result: %s", + name + ), + "", + truncate_review_tool_result(format_review_tool_result(result@value)), "", - truncate_review_tool_result(format_review_tool_result(result@value)) + "
" ) } @@ -508,22 +505,26 @@ review_citations_markdown <- function(citations) { c("", "### Citations", "", items) } -review_notes_markdown <- function(notes, level) { +review_notes_markdown <- function(notes) { if (length(notes) == 0) { - return(character()) + return(c("", "**Review notes:** None")) } - heading <- paste0(strrep("#", level), " Review notes") rendered <- unlist(lapply(notes, function(note) { - text <- paste0("> ", gsub("\n", "\n> ", note$note, fixed = TRUE)) c( "", - text, + review_blockquote(note$note), ">", sprintf("> _%s, %s_", note$user, note$time) ) })) - c("", heading, rendered) + c("", "**Review notes:**", rendered) +} + +review_blockquote <- function(text) { + lines <- strsplit(text, "\n", fixed = TRUE)[[1]] + lines <- ifelse(nzchar(lines), paste0("> ", lines), ">") + paste(lines, collapse = "\n") } review_note_metadata <- function(note) { @@ -579,7 +580,6 @@ is_review_document <- function(document) { identical(document$generated_by, "commons::trajectory_review") && identical(document$schema_version, 1L) && rlang::is_string(document$conversation) && - is.list(document$source) && rlang::is_string(document$updated_at) && is_review_flags(document$flags) && is.list(document$notes) && diff --git a/R/trajectory-review.R b/R/trajectory-review.R index 13c443a..4c33bed 100644 --- a/R/trajectory-review.R +++ b/R/trajectory-review.R @@ -20,8 +20,9 @@ #' and its tool activity. YAML frontmatter stores active flags and note history #' so the reviewer can restore its state and agents can identify flagged #' conversations, exchanges, and reviewer notes. The Markdown body is the -#' human-readable transcript for joint human-agent review. Tool results are -#' limited to 50 lines or 20,000 characters in the rendered document. +#' human-readable transcript for joint human-agent review. Tool calls and +#' results are collapsed for easier scanning, and tool results are limited to +#' 50 lines or 20,000 characters in the rendered document. #' #' Trajectories carry no record of how each answer was tagged when it was #' produced, so the viewer derives trust levels from the tool calls in the @@ -55,21 +56,33 @@ #' across local R sessions. Without either, reviews land in `commons-reviews` #' relative to the app's working directory. #' -#' On Posit Connect, use a Connect pin for durable storage across deployments: +#' On Posit Connect, use a versioned Connect pin for durable storage across +#' deployments: #' #' ``` #' trajectory_review( #' trajectories, -#' review_board = pins::board_connect(), +#' review_dir = file.path(tempdir(), "commons-reviews"), +#' review_board = pins::board_connect( +#' auth = "envvar", +#' use_cache_on_failure = FALSE +#' ), #' review_pin = "agent-reviews" #' ) #' ``` #' #' The reviewer downloads the pin at startup and uploads a new pin version -#' after each review action. Without a pin, the default working-directory path -#' is replaced when the content is redeployed. Review apps should use one -#' Connect process because separate processes do not coordinate writes or -#' in-memory review state. +#' after each review action. Connect supplies `CONNECT_SERVER` and +#' `CONNECT_API_KEY` to running content when its default API-key integration is +#' enabled, so `auth = "envvar"` does not require embedding credentials. +#' Without a pin, the default working-directory path is replaced when the +#' content is redeployed. Review apps should use one Connect process because +#' separate processes do not coordinate writes or in-memory review state. +#' +#' The Connect API key's user must have editor access to the content whose +#' trajectories are read. The pin is owned by the reviewer app's publisher. +#' Download the generated documents with the app's **Download reviews** button +#' or [pins::pin_download()] when they need to be reviewed outside the app. #' #' All sessions of one reviewer app share the same `review_dir`, flags, and #' notes; review state is not separated by user. Notes record `session$user`, @@ -104,13 +117,12 @@ trajectory_review <- function( rlang::check_string(review_dir) store <- review_store(review_dir, review_board, review_pin) hydrate_review_store(store) - source <- trajectory_source(trajectories) trajectories <- drop_side_conversations(trajectories) summary <- summarize_trajectories(trajectories) questions <- summarize_questions(trajectories) shiny::shinyApp( viewer_ui(summary), - viewer_server(trajectories, summary, questions, store, source) + viewer_server(trajectories, summary, questions, store) ) } @@ -516,8 +528,7 @@ viewer_server <- function( trajectories, summary, questions, - store, - source = trajectory_source(trajectories) + store ) { # Share review state across sessions in the documented single process. review_state <- read_review_state(store$review_dir) @@ -682,8 +693,7 @@ viewer_server <- function( trajectories, key$conversation, next_flags, - notes(), - source + notes() ) sync_review_store(store) flags(next_flags) @@ -746,8 +756,7 @@ viewer_server <- function( trajectories, key$conversation, flags(), - next_notes, - source + next_notes ) sync_review_store(store) notes(next_notes) diff --git a/man/trajectory_review.Rd b/man/trajectory_review.Rd index e5d7785..ebd702d 100644 --- a/man/trajectory_review.Rd +++ b/man/trajectory_review.Rd @@ -52,8 +52,9 @@ Each Markdown document contains the complete reviewer-visible conversation and its tool activity. YAML frontmatter stores active flags and note history so the reviewer can restore its state and agents can identify flagged conversations, exchanges, and reviewer notes. The Markdown body is the -human-readable transcript for joint human-agent review. Tool results are -limited to 50 lines or 20,000 characters in the rendered document. +human-readable transcript for joint human-agent review. Tool calls and +results are collapsed for easier scanning, and tool results are limited to +50 lines or 20,000 characters in the rendered document. Trajectories carry no record of how each answer was tagged when it was produced, so the viewer derives trust levels from the tool calls in the @@ -75,20 +76,32 @@ process with \code{Sys.setenv()}, or add it to \code{.Renviron} to keep the sett across local R sessions. Without either, reviews land in \code{commons-reviews} relative to the app's working directory. -On Posit Connect, use a Connect pin for durable storage across deployments: +On Posit Connect, use a versioned Connect pin for durable storage across +deployments: \if{html}{\out{
}}\preformatted{trajectory_review( trajectories, - review_board = pins::board_connect(), + review_dir = file.path(tempdir(), "commons-reviews"), + review_board = pins::board_connect( + auth = "envvar", + use_cache_on_failure = FALSE + ), review_pin = "agent-reviews" ) }\if{html}{\out{
}} The reviewer downloads the pin at startup and uploads a new pin version -after each review action. Without a pin, the default working-directory path -is replaced when the content is redeployed. Review apps should use one -Connect process because separate processes do not coordinate writes or -in-memory review state. +after each review action. Connect supplies \code{CONNECT_SERVER} and +\code{CONNECT_API_KEY} to running content when its default API-key integration is +enabled, so \code{auth = "envvar"} does not require embedding credentials. +Without a pin, the default working-directory path is replaced when the +content is redeployed. Review apps should use one Connect process because +separate processes do not coordinate writes or in-memory review state. + +The Connect API key's user must have editor access to the content whose +trajectories are read. The pin is owned by the reviewer app's publisher. +Download the generated documents with the app's \strong{Download reviews} button +or \code{\link[pins:pin_download]{pins::pin_download()}} when they need to be reviewed outside the app. All sessions of one reviewer app share the same \code{review_dir}, flags, and notes; review state is not separated by user. Notes record \code{session$user}, From 74980683f1b8506f17e75e6f9734ee786ad5ab46 Mon Sep 17 00:00:00 2001 From: skaltman Date: Fri, 14 Aug 2026 15:32:14 -0700 Subject: [PATCH 12/17] Make review_dir optional with smart defaults and improve markdown formatting --- R/trajectory-review-log.R | 52 +++++++++++++++++++++++++------------ R/trajectory-review.R | 54 ++++++++++++++++++--------------------- man/trajectory_review.Rd | 50 ++++++++++++++++++------------------ 3 files changed, 85 insertions(+), 71 deletions(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 94c2956..19268fe 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -33,7 +33,7 @@ read_review_state <- function(review_dir) { } review_store <- function( - review_dir, + review_dir = NULL, review_board = NULL, review_pin = NULL, call = rlang::caller_env() @@ -44,6 +44,11 @@ review_store <- function( call = call ) } + review_dir <- resolve_review_dir( + review_dir, + pin_backed = !is.null(review_pin), + call = call + ) if (!is.null(review_pin)) { rlang::check_string(review_pin, call = call) if (!requireNamespace("pins", quietly = TRUE)) { @@ -60,6 +65,21 @@ review_store <- function( ) } +resolve_review_dir <- function( + review_dir, + pin_backed, + call = rlang::caller_env() +) { + if (!is.null(review_dir)) { + rlang::check_string(review_dir, call = call) + return(review_dir) + } + if (pin_backed) { + return(tempfile("commons-reviews-")) + } + Sys.getenv("COMMONS_REVIEW_DIR", unset = "commons-reviews") +} + hydrate_review_store <- function(store, call = rlang::caller_env()) { if (is.null(store$board) || !pins::pin_exists(store$board, store$pin)) { return(invisible(store)) @@ -418,11 +438,9 @@ review_exchange_contents_markdown <- function(exchange) { review_tool_request_markdown <- function(request) { c( "", - "
", - sprintf( - "Tool call: %s", - request@name - ), + sprintf("### Tool call: `%s`", request@name), + "", + "#### Arguments", "", "```json", as.character(jsonlite::toJSON( @@ -431,26 +449,26 @@ review_tool_request_markdown <- function(request) { pretty = TRUE, null = "null" )), - "```", - "", - "
" + "```" ) } review_tool_result_markdown <- function(result) { request <- result@request name <- if (is.null(request)) "unknown" else request@name + if (identical(name, "search_pool")) { + return(c( + "", + sprintf("### Tool result: `%s`", name), + "", + "_Discovery result omitted from the review log._" + )) + } c( "", - "
", - sprintf( - "Tool result: %s", - name - ), - "", - truncate_review_tool_result(format_review_tool_result(result@value)), + sprintf("### Tool result: `%s`", name), "", - "
" + truncate_review_tool_result(format_review_tool_result(result@value)) ) } diff --git a/R/trajectory-review.R b/R/trajectory-review.R index 4c33bed..03e58b0 100644 --- a/R/trajectory-review.R +++ b/R/trajectory-review.R @@ -12,17 +12,17 @@ #' Transcripts are reviewable rather than live: conversations and questions #' can be flagged for review and annotated with notes. Notes apply to the #' whole conversation, or to a single question-and-answer exchange selected -#' in the transcript. Flags and notes land in `review_dir`, one generated -#' Markdown document per reviewed conversation, and are restored when the -#' viewer reopens. +#' in the transcript. Flags and notes are stored as one generated Markdown +#' document per reviewed conversation and are restored when the viewer +#' reopens. #' #' Each Markdown document contains the complete reviewer-visible conversation #' and its tool activity. YAML frontmatter stores active flags and note history #' so the reviewer can restore its state and agents can identify flagged #' conversations, exchanges, and reviewer notes. The Markdown body is the -#' human-readable transcript for joint human-agent review. Tool calls and -#' results are collapsed for easier scanning, and tool results are limited to -#' 50 lines or 20,000 characters in the rendered document. +#' human-readable transcript for joint human-agent review. Search-pool results +#' are omitted because later tool calls record any selected measure; other tool +#' results are limited to 50 lines or 20,000 characters. #' #' Trajectories carry no record of how each answer was tagged when it was #' produced, so the viewer derives trust levels from the tool calls in the @@ -39,19 +39,19 @@ #' #' @param trajectories A named list of conversations, as returned by #' [trajectory_read()]. -#' @param review_dir Directory where review actions write one generated -#' Markdown document per conversation. Created on first use; active flags -#' and note history recorded here are restored when the viewer reopens. -#' Defaults to `COMMONS_REVIEW_DIR` when set. When `review_board` and -#' `review_pin` are supplied, this directory is a local cache of the pin. +#' @param review_dir Optional directory where review actions write generated +#' Markdown documents. For local review, defaults to `COMMONS_REVIEW_DIR` +#' when set and otherwise to `commons-reviews` in the working directory. +#' Pin-backed review uses an automatically managed temporary cache unless +#' this argument is supplied. #' @param review_board Optional pins board used for durable review storage, #' such as `pins::board_connect()`. Must be supplied with `review_pin`. #' @param review_pin Name of a pin containing the review documents. Must be #' supplied with `review_board`. #' #' @details -#' A single reviewer app writes all of its review documents to `review_dir`. -#' Pass `review_dir` directly, set `COMMONS_REVIEW_DIR` for the current R +#' Without a pin, a single reviewer app writes all review documents to +#' `review_dir`. Pass it directly, set `COMMONS_REVIEW_DIR` for the current R #' process with `Sys.setenv()`, or add it to `.Renviron` to keep the setting #' across local R sessions. Without either, reviews land in `commons-reviews` #' relative to the app's working directory. @@ -62,7 +62,6 @@ #' ``` #' trajectory_review( #' trajectories, -#' review_dir = file.path(tempdir(), "commons-reviews"), #' review_board = pins::board_connect( #' auth = "envvar", #' use_cache_on_failure = FALSE @@ -71,22 +70,23 @@ #' ) #' ``` #' -#' The reviewer downloads the pin at startup and uploads a new pin version -#' after each review action. Connect supplies `CONNECT_SERVER` and -#' `CONNECT_API_KEY` to running content when its default API-key integration is -#' enabled, so `auth = "envvar"` does not require embedding credentials. -#' Without a pin, the default working-directory path is replaced when the -#' content is redeployed. Review apps should use one Connect process because -#' separate processes do not coordinate writes or in-memory review state. +#' The reviewer uses an automatically managed temporary local cache, downloads +#' the pin at startup, and uploads a new pin version after each review action. +#' Connect supplies `CONNECT_SERVER` and `CONNECT_API_KEY` to running content +#' when its default API-key integration is enabled, so `auth = "envvar"` does +#' not require embedding credentials. Without a pin, the default +#' working-directory path is replaced when the content is redeployed. Review +#' apps should use one Connect process because separate processes do not +#' coordinate writes or in-memory review state. #' #' The Connect API key's user must have editor access to the content whose #' trajectories are read. The pin is owned by the reviewer app's publisher. #' Download the generated documents with the app's **Download reviews** button #' or [pins::pin_download()] when they need to be reviewed outside the app. #' -#' All sessions of one reviewer app share the same `review_dir`, flags, and -#' notes; review state is not separated by user. Notes record `session$user`, -#' the login information supplied by the Shiny host, or `"unknown"` when it is +#' All sessions of one reviewer app share the same flags and notes; review +#' state is not separated by user. Notes record `session$user`, the login +#' information supplied by the Shiny host, or `"unknown"` when it is #' unavailable. Flags do not record who changed them. #' #' @return A [shiny::shinyApp()] object. Calling `trajectory_review()` at the @@ -105,16 +105,12 @@ #' @export trajectory_review <- function( trajectories = trajectory_read(), - review_dir = Sys.getenv( - "COMMONS_REVIEW_DIR", - unset = "commons-reviews" - ), + review_dir = NULL, review_board = NULL, review_pin = NULL ) { check_viewer_packages() check_trajectories(trajectories) - rlang::check_string(review_dir) store <- review_store(review_dir, review_board, review_pin) hydrate_review_store(store) trajectories <- drop_side_conversations(trajectories) diff --git a/man/trajectory_review.Rd b/man/trajectory_review.Rd index ebd702d..421567b 100644 --- a/man/trajectory_review.Rd +++ b/man/trajectory_review.Rd @@ -6,7 +6,7 @@ \usage{ trajectory_review( trajectories = trajectory_read(), - review_dir = Sys.getenv("COMMONS_REVIEW_DIR", unset = "commons-reviews"), + review_dir = NULL, review_board = NULL, review_pin = NULL ) @@ -15,11 +15,11 @@ trajectory_review( \item{trajectories}{A named list of conversations, as returned by \code{\link[=trajectory_read]{trajectory_read()}}.} -\item{review_dir}{Directory where review actions write one generated -Markdown document per conversation. Created on first use; active flags -and note history recorded here are restored when the viewer reopens. -Defaults to \code{COMMONS_REVIEW_DIR} when set. When \code{review_board} and -\code{review_pin} are supplied, this directory is a local cache of the pin.} +\item{review_dir}{Optional directory where review actions write generated +Markdown documents. For local review, defaults to \code{COMMONS_REVIEW_DIR} +when set and otherwise to \code{commons-reviews} in the working directory. +Pin-backed review uses an automatically managed temporary cache unless +this argument is supplied.} \item{review_board}{Optional pins board used for durable review storage, such as \code{pins::board_connect()}. Must be supplied with \code{review_pin}.} @@ -44,17 +44,17 @@ chat UI would show. Transcripts are reviewable rather than live: conversations and questions can be flagged for review and annotated with notes. Notes apply to the whole conversation, or to a single question-and-answer exchange selected -in the transcript. Flags and notes land in \code{review_dir}, one generated -Markdown document per reviewed conversation, and are restored when the -viewer reopens. +in the transcript. Flags and notes are stored as one generated Markdown +document per reviewed conversation and are restored when the viewer +reopens. Each Markdown document contains the complete reviewer-visible conversation and its tool activity. YAML frontmatter stores active flags and note history so the reviewer can restore its state and agents can identify flagged conversations, exchanges, and reviewer notes. The Markdown body is the -human-readable transcript for joint human-agent review. Tool calls and -results are collapsed for easier scanning, and tool results are limited to -50 lines or 20,000 characters in the rendered document. +human-readable transcript for joint human-agent review. Search-pool results +are omitted because later tool calls record any selected measure; other tool +results are limited to 50 lines or 20,000 characters. Trajectories carry no record of how each answer was tagged when it was produced, so the viewer derives trust levels from the tool calls in the @@ -70,8 +70,8 @@ shinychat's conversation-title generation, and completions with no user turn—are excluded from the viewer. } \details{ -A single reviewer app writes all of its review documents to \code{review_dir}. -Pass \code{review_dir} directly, set \code{COMMONS_REVIEW_DIR} for the current R +Without a pin, a single reviewer app writes all review documents to +\code{review_dir}. Pass it directly, set \code{COMMONS_REVIEW_DIR} for the current R process with \code{Sys.setenv()}, or add it to \code{.Renviron} to keep the setting across local R sessions. Without either, reviews land in \code{commons-reviews} relative to the app's working directory. @@ -81,7 +81,6 @@ deployments: \if{html}{\out{
}}\preformatted{trajectory_review( trajectories, - review_dir = file.path(tempdir(), "commons-reviews"), review_board = pins::board_connect( auth = "envvar", use_cache_on_failure = FALSE @@ -90,22 +89,23 @@ deployments: ) }\if{html}{\out{
}} -The reviewer downloads the pin at startup and uploads a new pin version -after each review action. Connect supplies \code{CONNECT_SERVER} and -\code{CONNECT_API_KEY} to running content when its default API-key integration is -enabled, so \code{auth = "envvar"} does not require embedding credentials. -Without a pin, the default working-directory path is replaced when the -content is redeployed. Review apps should use one Connect process because -separate processes do not coordinate writes or in-memory review state. +The reviewer uses an automatically managed temporary local cache, downloads +the pin at startup, and uploads a new pin version after each review action. +Connect supplies \code{CONNECT_SERVER} and \code{CONNECT_API_KEY} to running content +when its default API-key integration is enabled, so \code{auth = "envvar"} does +not require embedding credentials. Without a pin, the default +working-directory path is replaced when the content is redeployed. Review +apps should use one Connect process because separate processes do not +coordinate writes or in-memory review state. The Connect API key's user must have editor access to the content whose trajectories are read. The pin is owned by the reviewer app's publisher. Download the generated documents with the app's \strong{Download reviews} button or \code{\link[pins:pin_download]{pins::pin_download()}} when they need to be reviewed outside the app. -All sessions of one reviewer app share the same \code{review_dir}, flags, and -notes; review state is not separated by user. Notes record \code{session$user}, -the login information supplied by the Shiny host, or \code{"unknown"} when it is +All sessions of one reviewer app share the same flags and notes; review +state is not separated by user. Notes record \code{session$user}, the login +information supplied by the Shiny host, or \code{"unknown"} when it is unavailable. Flags do not record who changed them. } \examples{ From ea6b283fbf02d66f6dcdfdfd43f5e6a25628e8a7 Mon Sep 17 00:00:00 2001 From: skaltman Date: Fri, 14 Aug 2026 15:59:05 -0700 Subject: [PATCH 13/17] Remove `source` field from review documents and update review document structure --- tests/testthat/_snaps/commons.new.md | 90 ------------------- .../trajectory-review-log/review-document.md | 42 ++++----- tests/testthat/test-trajectory-review-log.R | 71 +++++++++++---- 3 files changed, 70 insertions(+), 133 deletions(-) delete mode 100644 tests/testthat/_snaps/commons.new.md diff --git a/tests/testthat/_snaps/commons.new.md b/tests/testthat/_snaps/commons.new.md deleted file mode 100644 index 7dc920f..0000000 --- a/tests/testthat/_snaps/commons.new.md +++ /dev/null @@ -1,90 +0,0 @@ -# commons() validates its inputs - - Code - commons(client = "not a chat", data_sources = test_source()) - Condition - Error in `commons()`: - ! `client` must be an , e.g. from `ellmer::chat_anthropic()`. - ---- - - Code - commons(client = test_client(), data_sources = "not a source") - Condition - Error in `commons()`: - ! `data_sources` must be a `data_source()` or a named list of them. - ---- - - Code - commons(client = test_client(), data_sources = test_source(), context_layer = "not context") - Message - duckdb is storing downloaded extensions and secrets under ~/.duckdb: - i /Users/saraa/.duckdb - This persists across sessions and is shared with the DuckDB CLI and other clients. - i Run duckdb(shared_home = FALSE) to use a temporary directory instead. - i See ?duckdb_storage for details and alternatives. - Condition - Error in `commons()`: - ! `context_layer` must be a `context_layer()` or `NULL`. - ---- - - Code - commons(client = test_client(), data_sources = test_source(), semantic_layer = list()) - Message - duckdb is storing downloaded extensions and secrets under ~/.duckdb: - i /Users/saraa/.duckdb - This persists across sessions and is shared with the DuckDB CLI and other clients. - i Run duckdb(shared_home = FALSE) to use a temporary directory instead. - i See ?duckdb_storage for details and alternatives. - Condition - Error in `commons()`: - ! `semantic_layer` must be a `semantic_layer()`. - ---- - - Code - commons(client = test_client(), data_sources = test_source(), log = TRUE, - share_with = 1) - Message - duckdb is storing downloaded extensions and secrets under ~/.duckdb: - i /Users/saraa/.duckdb - This persists across sessions and is shared with the DuckDB CLI and other clients. - i Run duckdb(shared_home = FALSE) to use a temporary directory instead. - i See ?duckdb_storage for details and alternatives. - Condition - Error in `commons()`: - ! `share_with` must be a character vector of Connect usernames. - -# commons() errors on injection parameters matching no name - - Code - commons(client = test_client(), data_sources = list(sales_db = test_source()), - semantic_layer = layer) - Message - duckdb is storing downloaded extensions and secrets under ~/.duckdb: - i /Users/saraa/.duckdb - This persists across sessions and is shared with the DuckDB CLI and other clients. - i Run duckdb(shared_home = FALSE) to use a temporary directory instead. - i See ?duckdb_storage for details and alternatives. - Condition - Error in `initialize()`: - ! Measure "region_revenue" has undocumented argument `warehouse` matching no data source. - i Available sources: "sales_db". - ---- - - Code - commons(client = test_client(), data_sources = test_source(), semantic_layer = layer) - Message - duckdb is storing downloaded extensions and secrets under ~/.duckdb: - i /Users/saraa/.duckdb - This persists across sessions and is shared with the DuckDB CLI and other clients. - i Run duckdb(shared_home = FALSE) to use a temporary directory instead. - i See ?duckdb_storage for details and alternatives. - Condition - Error in `initialize()`: - ! Measure "region_revenue" has undocumented argument `warehouse` matching no data source. - i `data_sources` has no named sources. - diff --git a/tests/testthat/_snaps/trajectory-review-log/review-document.md b/tests/testthat/_snaps/trajectory-review-log/review-document.md index 35118c1..e86f182 100644 --- a/tests/testthat/_snaps/trajectory-review-log/review-document.md +++ b/tests/testthat/_snaps/trajectory-review-log/review-document.md @@ -2,10 +2,7 @@ generated_by: commons::trajectory_review schema_version: 1 conversation: conv/1 -source: - kind: connect - server: https://connect.example.com - content_guid: 00000000-0000-0000-0000-000000000001 +last_active: 2026-08-11T09:30:00+0000 updated_at: '2026-08-12T16:10:00Z' flags: conversation: yes @@ -23,22 +20,15 @@ notes: text: Review the complete conversation. --- -# Trajectory review +# conv/1 -> Generated by `commons::trajectory_review()`. Manual edits are not supported. -> Agents: parse the YAML frontmatter for review state and use the Markdown -> body as the human-readable conversation, tool activity, and review notes. + -- Conversation: `conv/1` -- Source: `connect` -- Last active: 2026-08-11T09:30:00+0000 -- Review updated: 2026-08-12T16:10:00Z - -## Conversation review +## Conversation-level review **Flagged:** Yes -### Review notes +**Review notes:** > Review the complete conversation. > @@ -50,9 +40,16 @@ notes: **Flagged:** Yes +**Review notes:** + +> Check the governed measure. +> The SQL is otherwise correct. +> +> _sara, 2026-08-12T16:00:00Z_ + ### User -How many orders? +> How many orders? ### Tool call: `run_sql` @@ -70,19 +67,12 @@ How many orders? ### Assistant -There are 6 orders. - -Orders are rows. +> There are 6 orders. +> +> Orders are rows. ### Citations - Orders are rows. - Reason: definition -### Review notes - -> Check the governed measure. -> The SQL is otherwise correct. -> -> _sara, 2026-08-12T16:00:00Z_ - diff --git a/tests/testthat/test-trajectory-review-log.R b/tests/testthat/test-trajectory-review-log.R index 3e5b557..c31a074 100644 --- a/tests/testthat/test-trajectory-review-log.R +++ b/tests/testthat/test-trajectory-review-log.R @@ -47,11 +47,6 @@ test_that("review_document renders a self-contained trajectory review", { review_test_turns(), list(conversation = TRUE, exchanges = 1L), review_test_notes(), - source = list( - kind = "connect", - server = "https://connect.example.com", - content_guid = "00000000-0000-0000-0000-000000000001" - ), updated_at = "2026-08-12T16:10:00Z" ) @@ -94,19 +89,41 @@ test_that("review documents preserve assistant and tool content order", { ) expected <- c( - "I'll check both sources.", + "> I'll check both sources.", "### Tool call: `first_tool`", - "The first call is queued.", + "> The first call is queued.", "### Tool call: `second_tool`", "### Tool result: `first_tool`", "first result", "### Tool result: `second_tool`", "second result", - "Both checks are complete." + "> Both checks are complete." ) expect_equal(markdown[markdown %in% expected], expected) }) +test_that("search_pool results are omitted from review documents", { + request <- ellmer::ContentToolRequest( + id = "call-1", + name = "search_pool", + arguments = list(query = "revenue") + ) + result <- ellmer::ContentToolResult( + "A long catalog of matching measures", + request = request + ) + + expect_equal( + review_tool_result_markdown(result), + c( + "", + "### Tool result: `search_pool`", + "", + "_Discovery result omitted from the review log._" + ) + ) +}) + test_that("review state round trips through YAML frontmatter", { review_dir <- withr::local_tempdir() id <- "conv/1" @@ -117,7 +134,6 @@ test_that("review state round trips through YAML frontmatter", { review_test_turns(), list(conversation = TRUE, exchanges = 1L), review_test_notes(id), - source = list(kind = "unknown"), updated_at = "2026-08-12T16:10:00Z" ), file @@ -156,6 +172,32 @@ test_that("review archives contain generated documents", { ) }) +test_that("review directories resolve by storage mode", { + expect_equal( + withr::with_envvar( + c(COMMONS_REVIEW_DIR = "/custom/reviews"), + resolve_review_dir(NULL, pin_backed = FALSE) + ), + "/custom/reviews" + ) + expect_equal( + withr::with_envvar( + c(COMMONS_REVIEW_DIR = NA), + resolve_review_dir(NULL, pin_backed = FALSE) + ), + "commons-reviews" + ) + + cache <- resolve_review_dir(NULL, pin_backed = TRUE) + expect_equal(dirname(cache), tempdir()) + expect_match(basename(cache), "^commons-reviews-") + expect_identical(dir.exists(cache), FALSE) + expect_equal( + resolve_review_dir("/explicit/reviews", pin_backed = TRUE), + "/explicit/reviews" + ) +}) + test_that("pin-backed review stores sync and restore documents", { skip_if_not_installed("pins") board <- pins::board_folder(withr::local_tempdir(), versioned = TRUE) @@ -197,15 +239,13 @@ test_that("conversation reviews are created, replaced, and removed", { parent <- withr::local_tempdir() review_dir <- file.path(parent, "reviews") trajectories <- list(`conv/1` = review_test_turns()) - source <- list(kind = "unknown") write_conversation_review( review_dir, trajectories, conversation = 1L, flags = "conv/1#1", - notes = list(), - source = source + notes = list() ) file <- review_document_path(review_dir, "conv/1") @@ -218,8 +258,7 @@ test_that("conversation reviews are created, replaced, and removed", { trajectories, conversation = 1L, flags = "conv/1", - notes = list(), - source = source + notes = list() ) expect_equal(read_review_state(review_dir)$flags, "conv/1") @@ -230,7 +269,6 @@ test_that("conversation reviews are created, replaced, and removed", { review_test_turns(), list(conversation = TRUE, exchanges = integer()), list(), - source, updated_at = "2026-08-12T16:10:00Z" ), unknown @@ -242,8 +280,7 @@ test_that("conversation reviews are created, replaced, and removed", { trajectories, conversation = 1L, flags = character(), - notes = list(), - source = source + notes = list() ) expect_identical(file.exists(file), FALSE) From 07d46d4e5e4333915df4b446dcf8085d80e7b808 Mon Sep 17 00:00:00 2001 From: skaltman Date: Fri, 14 Aug 2026 15:59:22 -0700 Subject: [PATCH 14/17] Update trajectory iteration guidance to clarify reviewed conversations priority --- inst/skills/commons/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/inst/skills/commons/SKILL.md b/inst/skills/commons/SKILL.md index dda52d4..fb911cf 100644 --- a/inst/skills/commons/SKILL.md +++ b/inst/skills/commons/SKILL.md @@ -8,7 +8,7 @@ description: Building, maintaining, and iterating on a self-service data agent w Read the relevant reference below before starting the corresponding task. - [Extract commons context from existing data artifacts](references/extracting-from-artifacts.md) - turn a trusted Shiny app, Quarto report, R script, or SQL file into measures, dictionary edits, and free-text context, each carrying provenance back to its source. -- [Iterate on a commons agent from trajectories](references/iterating-from-trajectories.md) - improve an existing agent by reading logged trajectories, finding recurring low-trust or slow answers, and proposing semantic-layer or context-layer changes. +- [Iterate on a commons agent from trajectories](references/iterating-from-trajectories.md) - improve an existing agent from reviewed conversations when available, or from raw logged trajectories otherwise, then propose semantic-layer or context-layer changes. ## Agent project layout From 3f06456f5acdd7d1244ac6a14ee497afee591c70 Mon Sep 17 00:00:00 2001 From: skaltman Date: Fri, 14 Aug 2026 15:59:32 -0700 Subject: [PATCH 15/17] Add guidance for using reviewed trajectories and trajectory review setup --- .../references/iterating-from-trajectories.md | 74 ++++++++++++++++--- 1 file changed, 64 insertions(+), 10 deletions(-) diff --git a/inst/skills/commons/references/iterating-from-trajectories.md b/inst/skills/commons/references/iterating-from-trajectories.md index db6396c..92c10bf 100644 --- a/inst/skills/commons/references/iterating-from-trajectories.md +++ b/inst/skills/commons/references/iterating-from-trajectories.md @@ -20,25 +20,79 @@ The answer uses SQL with little documentation and the agent had to inspect table 1. Find the agent definition. Search for `commons(`, `data_source(`, `semantic_layer(`, `measure(`, and `context_layer(`. Identify where the semantic layer and context layer are constructed. If they are wrapped in project helpers, follow those helpers. Note that measure arguments without `@param` documentation are never seen by the model: an argument named after a data source receives that source's connection, and any other undocumented argument keeps its default. A new measure that queries a database should take the connection this way rather than referencing a global; other objects it needs (a pins board, an API client) should come from a default written as a call, e.g. `board = pins::board_connect()`. -2. Load trajectories. - Use `commons::trajectory_read()`. With no arguments it resolves automatically: on Posit Connect it reads this content's own traces, in a deployed project it reads the deployment's traces from Connect (requires the `CONNECT_API_KEY` environment variable and editor access to the content), and otherwise it reads local trace files. You can also pass a Connect content GUID, a content URL, or a directory of OTLP trace files. If resolution is unclear, inspect the project for `log =`, `COMMONS_TRACES_DIR`, `OTEL_*` environment variables, or deployment setup. +2. Choose the evidence path. + Do not always begin by loading raw trajectories. First look for a `commons-reviews/` directory, a downloaded review archive, or reviewer configuration that names a review pin. - When the store is large, subset: `n` keeps the `n` most recent conversations, and `from`/`to` keep conversations with chat activity in a time window, e.g. `trajectory_read(n = 25, from = Sys.Date() - 7)`. +### Path 1: Reviewed trajectories -3. Read the conversations. - Each trajectory is a list of ellmer turns, named by conversation id. +Use review documents whenever they are available. They contain the conversations and exchanges a reviewer flagged or annotated, so treat them as the primary qualitative evidence rather than starting over from the raw log. + +- Read each `conversation-*.md` file. Parse its YAML frontmatter for the conversation id, active conversation and exchange flags, and reviewer notes. Read the Markdown body for the transcript, trust labels, tool calls, tool results, and notes in context. +- Treat flags as requests for attention, not conclusions. Use review notes to understand what the reviewer wants investigated. +- Treat tool results as excerpts because the Markdown renderer truncates long results. +- Use the latest pin version unless the user asks to compare review state over time. +- Supplement the reviews with raw trajectories only when needed to quantify how common a theme is, inspect a truncated result, or analyze conversations that were not reviewed. + +When a review pin is configured but not present locally, ask for its name or a downloaded archive. With access to the same Connect account, retrieve it read-only: ```r -trajectories <- commons::trajectory_read() -turns <- trajectories[[1]] +board <- pins::board_connect() +pins::pin_download(board, "") +``` + +### Path 2: Raw trajectories + +When no review documents exist, analyze raw logs now; do not make reviewer setup a prerequisite. +Use `commons::trajectory_read()`. With no arguments it resolves automatically: on Posit Connect it reads this content's own traces, in a deployed project it reads the deployment's traces from Connect, and otherwise it reads local trace files. You can also pass a Connect content GUID, a content URL, or a directory of OTLP trace files. Connect reads require `CONNECT_API_KEY` and editor access to the content. + +If resolution is unclear, inspect the project for `log =`, `COMMONS_TRACES_DIR`, `OTEL_*` environment variables, or deployment setup. When the store is large, use `n`, `from`, or `to`, for example: + +```r +trajectories <- commons::trajectory_read( + n = 25, + from = Sys.Date() - 7 +) + +turns <- trajectories[[1]] print(turns) ``` -4. Analyze themes. +If traces are absent, confirm that the agent runs with `log = TRUE` while OpenTelemetry tracing is active. + +### Set up trajectory review + +When the project has no review workflow, recommend setting one up for future iterations after completing the current raw-log analysis. + +For local review: + +```r +commons::trajectory_review( + commons::trajectory_read() # set n to limit the number of trajectories +) +``` + +For a deployed reviewer, back review state with a versioned Connect pin so it +survives restarts and redeployments. `trajectory_review()` manages its temporary +working cache automatically: + +```r +commons::trajectory_review( + commons::trajectory_read(""), + review_board = pins::board_connect( + auth = "envvar", + use_cache_on_failure = FALSE + ), + review_pin = "agent-reviews" +) +``` + +Use one Connect process because separate processes do not coordinate review writes. The reviewer can export a `commons-reviews` archive with **Download reviews**. Prepare local reviewer code when the user asks, but obtain explicit confirmation before deploying content or creating external resources. + +3. Analyze themes. Group conversations by the business concept being asked about, not by exact wording. Note which themes already hit Path A, which are documented Path B, and which are exploratory Path B. -5. Propose changes. +4. Propose changes. Present the highest-value changes first. For each proposal, note the theme and current typical path, how many questions are described by that theme, and the recommended change. Prefer semantic-layer edits when the question is a stable governed metric. Prefer context-layer edits when the issue is table choice, grain, filters, joins, caveats, terminology, or reusable SQL shape. @@ -49,7 +103,7 @@ print(turns) * **extension** — same concept, superset behavior: edit the existing measure, adding the `@param` and the provenance tag. Never create `revenue2`. * **conflict** — same concept but a different computation, or a contradiction with an existing dictionary caveat: surface to the user with both sides; do not resolve silently. -6. Wait before editing. +5. Wait before editing. Do not make semantic-layer or context-layer edits until the user chooses which proposed changes to apply. The data scientist should confirm any new business definition, canonical table, exclusion rule, or SQL pattern. When a proposal is accepted, new measures and context land in the agent project layout defined in `SKILL.md`: measures in `measures/`, free-text in `context/`. Because these changes are born from trajectory analysis rather than an artifact, they carry the self-referencing provenance `trajectory analysis ()` — a `#' @provenance` tag on measures, YAML frontmatter on context files. From 1d795164cb91204e0bacef45780735f8620599c5 Mon Sep 17 00:00:00 2001 From: skaltman Date: Fri, 14 Aug 2026 16:06:50 -0700 Subject: [PATCH 16/17] Replace immediate syncs after each review action with debounced syncing to batch updates --- R/trajectory-review-log.R | 58 ++++++++++++++++++++- R/trajectory-review.R | 27 +++++++--- man/trajectory_review.Rd | 9 ++-- tests/testthat/test-trajectory-review-log.R | 4 ++ tests/testthat/test-trajectory-review.R | 41 +++++++++++++++ 5 files changed, 127 insertions(+), 12 deletions(-) diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 19268fe..7da6097 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -91,7 +91,7 @@ hydrate_review_store <- function(store, call = rlang::caller_env()) { pins::pin_download( store$board, store$pin, - version = versions$version[[1]] + version = latest_review_pin_version(store, versions) ) }, error = function(error) { @@ -111,6 +111,32 @@ hydrate_review_store <- function(store, call = rlang::caller_env()) { invisible(store) } +latest_review_pin_version <- function(store, versions) { + if ("active" %in% names(versions)) { + active <- which(versions$active %in% TRUE) + if (length(active) > 0) { + return(versions$version[[active[[1]]]]) + } + } + + if ( + inherits(store$board, "pins_board_folder") && + !is.null(store$board$path) + ) { + paths <- file.path(store$board$path, store$pin, versions$version) + modified <- file.info(paths)$mtime + if (any(!is.na(modified))) { + return(versions$version[[which.max(modified)]]) + } + } + + if ("created" %in% names(versions) && any(!is.na(versions$created))) { + return(versions$version[[which.max(versions$created)]]) + } + + versions$version[[nrow(versions)]] +} + sync_review_store <- function(store, call = rlang::caller_env()) { if (is.null(store$board)) { return(invisible(store)) @@ -147,6 +173,28 @@ sync_review_store <- function(store, call = rlang::caller_env()) { invisible(store) } +request_review_store_sync <- function(store, state, revision) { + if (is.null(store$board)) { + return(invisible(store)) + } + state$dirty <- TRUE + revision(revision() + 1L) + invisible(store) +} + +flush_review_store_sync <- function( + store, + state, + call = rlang::caller_env() +) { + if (!state$dirty) { + return(invisible(store)) + } + sync_review_store(store, call = call) + state$dirty <- FALSE + invisible(store) +} + write_review_archive <- function( review_dir, file, @@ -673,7 +721,13 @@ review_trust_label <- function(tag) { if (is.na(tag)) { return("No data tool") } - switch(tag, A = "Verified (A)", B = "Cited (B)", C = "Untrusted (C)") + switch( + tag, + A = "Verified (A)", + B = "Cited (B)", + C = "Untrusted (C)", + sprintf("Unknown (%s)", tag) + ) } review_yes_no <- function(value) { diff --git a/R/trajectory-review.R b/R/trajectory-review.R index 03e58b0..ff90b83 100644 --- a/R/trajectory-review.R +++ b/R/trajectory-review.R @@ -71,10 +71,11 @@ #' ``` #' #' The reviewer uses an automatically managed temporary local cache, downloads -#' the pin at startup, and uploads a new pin version after each review action. -#' Connect supplies `CONNECT_SERVER` and `CONNECT_API_KEY` to running content -#' when its default API-key integration is enabled, so `auth = "envvar"` does -#' not require embedding credentials. Without a pin, the default +#' the pin at startup, and uploads a new pin version after a brief pause in +#' review activity. Pending changes are also uploaded when a reviewer session +#' ends. Connect supplies `CONNECT_SERVER` and `CONNECT_API_KEY` to running +#' content when its default API-key integration is enabled, so `auth = "envvar"` +#' does not require embedding credentials. Without a pin, the default #' working-directory path is replaced when the content is redeployed. Review #' apps should use one Connect process because separate processes do not #' coordinate writes or in-memory review state. @@ -520,6 +521,8 @@ trust_choices <- function(group_by) { } } +review_sync_delay_ms <- 1000L + viewer_server <- function( trajectories, summary, @@ -530,10 +533,22 @@ viewer_server <- function( review_state <- read_review_state(store$review_dir) app_flags <- shiny::reactiveVal(review_state$flags) app_notes <- shiny::reactiveVal(review_state$notes) + sync_state <- rlang::env(dirty = FALSE) function(input, output, session) { flags <- app_flags notes <- app_notes + sync_revision <- shiny::reactiveVal(0L) + debounced_sync_revision <- shiny::debounce( + shiny::reactive(sync_revision()), + millis = review_sync_delay_ms + ) + shiny::observeEvent( + debounced_sync_revision(), + flush_review_store_sync(store, sync_state), + ignoreInit = TRUE + ) + session$onSessionEnded(\() flush_review_store_sync(store, sync_state)) selected <- shiny::reactiveVal(NULL) selected_transcript <- shiny::reactive({ key <- selected() @@ -691,8 +706,8 @@ viewer_server <- function( next_flags, notes() ) - sync_review_store(store) flags(next_flags) + request_review_store_sync(store, sync_state, sync_revision) }) shiny::observeEvent(input$exchange_select, { @@ -754,8 +769,8 @@ viewer_server <- function( flags(), next_notes ) - sync_review_store(store) notes(next_notes) + request_review_store_sync(store, sync_state, sync_revision) bslib::update_submit_textarea( "review_note", value = "", diff --git a/man/trajectory_review.Rd b/man/trajectory_review.Rd index 421567b..bbfbce7 100644 --- a/man/trajectory_review.Rd +++ b/man/trajectory_review.Rd @@ -90,10 +90,11 @@ deployments: }\if{html}{\out{}} The reviewer uses an automatically managed temporary local cache, downloads -the pin at startup, and uploads a new pin version after each review action. -Connect supplies \code{CONNECT_SERVER} and \code{CONNECT_API_KEY} to running content -when its default API-key integration is enabled, so \code{auth = "envvar"} does -not require embedding credentials. Without a pin, the default +the pin at startup, and uploads a new pin version after a brief pause in +review activity. Pending changes are also uploaded when a reviewer session +ends. Connect supplies \code{CONNECT_SERVER} and \code{CONNECT_API_KEY} to running +content when its default API-key integration is enabled, so \code{auth = "envvar"} +does not require embedding credentials. Without a pin, the default working-directory path is replaced when the content is redeployed. Review apps should use one Connect process because separate processes do not coordinate writes or in-memory review state. diff --git a/tests/testthat/test-trajectory-review-log.R b/tests/testthat/test-trajectory-review-log.R index c31a074..e03fa8c 100644 --- a/tests/testthat/test-trajectory-review-log.R +++ b/tests/testthat/test-trajectory-review-log.R @@ -124,6 +124,10 @@ test_that("search_pool results are omitted from review documents", { ) }) +test_that("unexpected trust tags remain visible", { + expect_equal(review_trust_label("D"), "Unknown (D)") +}) + test_that("review state round trips through YAML frontmatter", { review_dir <- withr::local_tempdir() id <- "conv/1" diff --git a/tests/testthat/test-trajectory-review.R b/tests/testthat/test-trajectory-review.R index b445893..50c6796 100644 --- a/tests/testthat/test-trajectory-review.R +++ b/tests/testthat/test-trajectory-review.R @@ -470,6 +470,47 @@ test_that("flags and notes write to and restore from review documents", { ) }) +test_that("pin-backed review sync batches actions and flushes on session end", { + skip_if_not_installed("shiny") + skip_if_not_installed("bslib") + skip_if_not_installed("shinychat") + skip_if_not_installed("htmltools") + + syncs <- 0L + local_mocked_bindings( + sync_review_store = function(...) syncs <<- syncs + 1L + ) + turns <- list(ellmer::UserTurn("One?"), ellmer::AssistantTurn("1.")) + trajectories <- list(conv1 = turns) + summary <- summarize_trajectories(trajectories) + questions <- summarize_questions(trajectories) + store <- list( + review_dir = withr::local_tempdir(), + board = TRUE, + pin = "agent-reviews" + ) + + shiny::testServer( + viewer_server(trajectories, summary, questions, store), + { + session$setInputs(group_by = "conversation", trust = "all", entry_1 = 1) + session$setInputs(flag_toggle = 1) + session$setInputs(flag_toggle = 2) + + expect_identical(syncs, 0L) + session$elapse(review_sync_delay_ms - 1L) + expect_identical(syncs, 0L) + session$elapse(1L) + expect_identical(syncs, 1L) + + session$setInputs(flag_toggle = 3) + expect_identical(syncs, 1L) + session$close() + expect_identical(syncs, 2L) + } + ) +}) + timeline_question <- function(tag, date) { list(tag = tag, last_active = as.POSIXct(paste(date, "09:00:00"))) } From a6b457d712023975d5185f5926684e155e16fb53 Mon Sep 17 00:00:00 2001 From: skaltman Date: Fri, 14 Aug 2026 16:14:26 -0700 Subject: [PATCH 17/17] Suppress messages in test_source helper function --- tests/testthat/helper-commons.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/helper-commons.R b/tests/testthat/helper-commons.R index 644e5ca..e639803 100644 --- a/tests/testthat/helper-commons.R +++ b/tests/testthat/helper-commons.R @@ -17,7 +17,7 @@ test_sales <- function() { } test_source <- function() { - data_source(sales = test_sales()) + suppressMessages(data_source(sales = test_sales())) } test_client <- function() {