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/ diff --git a/R/trajectory-review-log.R b/R/trajectory-review-log.R index 141c2dd..7da6097 100644 --- a/R/trajectory-review-log.R +++ b/R/trajectory-review-log.R @@ -1,37 +1,355 @@ -new_review_event <- function( +read_review_state <- function(review_dir) { + if (!dir.exists(review_dir)) { + return(list(flags = character(), notes = list())) + } + + files <- review_document_files(review_dir) + 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() + ) +} + +review_store <- function( + review_dir = NULL, + 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 + ) + } + 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)) { + 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 + ) +} + +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)) + } + + files <- tryCatch( + { + versions <- pins::pin_versions(store$board, store$pin) + pins::pin_download( + store$board, + store$pin, + version = latest_review_pin_version(store, versions) + ) + }, + 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) +} + +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)) + } + + 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) +} + +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, + 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, - key, - action, - user, - source = trajectory_source(trajectories), - note = NULL + conversation, + flags, + notes, + 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 (!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 + ) + } + } + + markdown <- review_document( + id, + trajectories[[conversation]], + conversation_flags, + notes + ) + 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 + ) + } ) + replace_review_file(temporary, file, call = call) - 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 + invisible(file) +} + +replace_review_file <- function( + temporary, + file, + call = rlang::caller_env() +) { + 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()) } - record + if (!is.null(backup)) { + file.rename(backup, file) + } + cli::cli_abort( + "Could not replace trajectory review {.file {file}}.", + call = call + ) } -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() { @@ -46,109 +364,376 @@ review_user <- function(session) { user } -trajectory_source <- function(trajectories) { - attr(trajectories, "source") %||% list(kind = "unknown") +review_document <- function( + id, + turns, + flags, + notes, + 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 + ) + 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) + + paste0("---\n", frontmatter, "---\n\n", body, "\n") } -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_body <- function(id, turns, flags, notes) { + exchanges <- split_exchanges(turns) + conversation_notes <- Filter(\(note) is.null(note$exchange), notes) + + sections <- c( + sprintf("# %s", flatten_inline(id)), + "", + "", + "", + "## Conversation-level review", + "", + sprintf("**Flagged:** %s", review_yes_no(flags$conversation)), + review_notes_markdown(conversation_notes) + ) + + 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 + ) + ) + } + + paste(sections, collapse = "\n") +} + +review_exchange_markdown <- function(exchange, number, flagged, notes) { + provenance <- exchange_provenance(exchange) + contents <- review_exchange_contents_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)), + review_notes_markdown(notes), + "", + "### User", + "", + review_blockquote(exchange[[1]]@text), + contents, + citation_section + ) } -read_review_records <- function(file) { - if (!file.exists(file)) { - return(list()) +review_exchange_contents_markdown <- function(exchange) { + sections <- character() + assistant_open <- FALSE + + for (turn in exchange) { + for (content in turn@contents) { + if ( + identical(turn@role, "assistant") && + S7::S7_inherits(content, ellmer::ContentText) + ) { + if (!assistant_open) { + sections <- c(sections, "", "### Assistant", "") + } + sections <- c(sections, review_blockquote(content@text)) + assistant_open <- TRUE + } else if (S7::S7_inherits(content, ellmer::ContentToolRequest)) { + sections <- c( + sections, + review_tool_request_markdown(content) + ) + assistant_open <- FALSE + } else if (S7::S7_inherits(content, ellmer::ContentToolResult)) { + sections <- c( + sections, + 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 + 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)) + ) +} + +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") +} - 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 +truncate_review_tool_result <- function( + result, + max_lines = 50L, + 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)) ) - if (!is_review_record(record)) { - invalid <- c(invalid, i) - next - } - records[[length(records) + 1]] <- record + })) + c("", "### Citations", "", items) +} + +review_notes_markdown <- function(notes) { + if (length(notes) == 0) { + return(c("", "**Review notes:** None")) } - if (length(invalid) > 0) { - cli::cli_warn( - "Ignoring {cli::qty(invalid)}invalid review record{?s} on line{?s} - {invalid} of {.file {file}}." + rendered <- unlist(lapply(notes, function(note) { + c( + "", + review_blockquote(note$note), + ">", + sprintf("> _%s, %s_", note$user, note$time) ) + })) + 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) { + metadata <- list( + user = note$user, + time = note$time, + text = note$note + ) + if (!is.null(note$exchange)) { + metadata$exchange <- note$exchange } - records + metadata } -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_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) } - exchange <- record$exchange + 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) && + 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))) } -actionable_review_records <- function(records) { - note_indices <- integer() - latest_flag_indices <- integer() +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)) +} - 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 - } +is_review_exchange <- function(exchange) { + is.numeric(exchange) && + length(exchange) == 1 && + is.finite(exchange) && + exchange >= 1 && + exchange == trunc(exchange) +} + +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) { + bytes <- as.integer(charToRaw(enc2utf8(id))) + encoded <- paste(sprintf("%02x", bytes), collapse = "") + 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)", + sprintf("Unknown (%s)", tag) ) } +review_yes_no <- function(value) { + if (value) "Yes" else "No" +} + review_key <- function(id, exchange = NULL) { paste(c(id, exchange), collapse = "#") } diff --git a/R/trajectory-review.R b/R/trajectory-review.R index c750b0f..ff90b83 100644 --- a/R/trajectory-review.R +++ b/R/trajectory-review.R @@ -12,13 +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_file`, one JSON record -#' per line, 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. #' -#' 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 and agents can identify flagged +#' conversations, exchanges, and reviewer notes. The Markdown body is the +#' 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 @@ -35,18 +39,56 @@ #' #' @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 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 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. -#' File-backed review apps should use one Connect process because separate -#' processes do not coordinate file writes or in-memory review state. +#' 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. +#' +#' On Posit Connect, use a versioned Connect pin for durable storage across +#' deployments: +#' +#' ``` +#' trajectory_review( +#' trajectories, +#' review_board = pins::board_connect( +#' auth = "envvar", +#' use_cache_on_failure = FALSE +#' ), +#' review_pin = "agent-reviews" +#' ) +#' ``` +#' +#' The reviewer uses an automatically managed temporary local cache, downloads +#' 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. +#' +#' 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 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 @@ -57,25 +99,27 @@ #' 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( trajectories = trajectory_read(), - review_file = Sys.getenv( - "COMMONS_REVIEW_FILE", - unset = "commons-review.jsonl" - ) + review_dir = NULL, + review_board = NULL, + review_pin = NULL ) { check_viewer_packages() check_trajectories(trajectories) - rlang::check_string(review_file) - source <- trajectory_source(trajectories) + store <- review_store(review_dir, review_board, review_pin) + hydrate_review_store(store) 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, store) ) } @@ -85,7 +129,8 @@ check_viewer_packages <- function(call = rlang::caller_env()) { "htmltools", "plotly", "shiny", - "shinychat" + "shinychat", + "yaml" ) missing <- pkgs[!vapply(pkgs, requireNamespace, logical(1), quietly = TRUE)] @@ -401,6 +446,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(), @@ -471,21 +521,34 @@ trust_choices <- function(group_by) { } } +review_sync_delay_ms <- 1000L + viewer_server <- function( trajectories, summary, questions, - review_file, - source = trajectory_source(trajectories) + store ) { # 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(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() @@ -500,6 +563,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" }) @@ -622,15 +694,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( + store$review_dir, trajectories, - key, - action = if (flagged) "unflag" else "flag", - user = user, - source = source + key$conversation, + next_flags, + notes() ) - append_review_record(review_file, record) - flags(if (flagged) setdiff(flags(), review) else union(flags(), review)) + flags(next_flags) + request_review_store_sync(store, sync_state, sync_revision) }) shiny::observeEvent(input$exchange_select, { @@ -681,16 +758,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( + store$review_dir, trajectories, - key, - action = "note", - user = user, - source = source, - note = note + key$conversation, + flags(), + next_notes ) - append_review_record(review_file, record) - notes(c(notes(), list(record))) + notes(next_notes) + request_review_store_sync(store, sync_state, sync_revision) bslib::update_submit_textarea( "review_note", value = "", 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 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. diff --git a/man/trajectory_review.Rd b/man/trajectory_review.Rd index 82fc141..bbfbce7 100644 --- a/man/trajectory_review.Rd +++ b/man/trajectory_review.Rd @@ -6,18 +6,26 @@ \usage{ trajectory_review( trajectories = trajectory_read(), - review_file = Sys.getenv("COMMONS_REVIEW_FILE", unset = "commons-review.jsonl") + review_dir = NULL, + review_board = NULL, + review_pin = NULL ) } \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}{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}.} + +\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 @@ -36,13 +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_file}, one JSON record -per line, 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. -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 and agents can identify flagged +conversations, exchanges, and reviewer notes. The Markdown body is the +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 @@ -58,16 +70,52 @@ 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. -File-backed review apps should use one Connect process because separate -processes do not coordinate file writes or in-memory review state. +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. + +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( + auth = "envvar", + use_cache_on_failure = FALSE + ), + review_pin = "agent-reviews" +) +}\if{html}{\out{
}} + +The reviewer uses an automatically managed temporary local cache, downloads +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. + +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 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() } } 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..e86f182 --- /dev/null +++ b/tests/testthat/_snaps/trajectory-review-log/review-document.md @@ -0,0 +1,78 @@ +--- +generated_by: commons::trajectory_review +schema_version: 1 +conversation: conv/1 +last_active: 2026-08-11T09:30:00+0000 +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. +--- + +# conv/1 + + + +## Conversation-level review + +**Flagged:** Yes + +**Review notes:** + +> Review the complete conversation. +> +> _lee, 2026-08-12T16:05:00Z_ + +## Exchange 1 + +**Trust:** Cited (B) + +**Flagged:** Yes + +**Review notes:** + +> Check the governed measure. +> The SQL is otherwise correct. +> +> _sara, 2026-08-12T16:00:00Z_ + +### 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 + 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/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() { diff --git a/tests/testthat/test-trajectory-review-log.R b/tests/testthat/test-trajectory-review-log.R index c5e7b10..e03fa8c 100644 --- a/tests/testthat/test-trajectory-review-log.R +++ b/tests/testthat/test-trajectory-review-log.R @@ -1,38 +1,326 @@ -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") + ) + 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." + ) + ) + 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", + 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(), + 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") +}) + +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("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( - vapply(reviews, function(record) record$event_id, character(1)), - c("event-2", "event-4", "event-5") + review_tool_result_markdown(result), + c( + "", + "### Tool result: `search_pool`", + "", + "_Discovery result omitted from the review log._" + ) + ) +}) + +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" + file <- review_document_path(review_dir, id) + writeLines( + review_document( + id, + review_test_turns(), + list(conversation = TRUE, exchanges = 1L), + review_test_notes(id), + 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_length( + unique(c(file, review_document_path(review_dir, "conv%2F1"))), + 2 + ) + expect_length(state$notes, 2) expect_equal( - reviews[[1]][c("user", "question", "tag", "source")], + state$notes[[1]][c("conversation", "exchange", "user", "note")], list( + conversation = id, + exchange = 1L, user = "sara", - question = "How many orders?", - tag = "A", - source = list( - kind = "connect", - server = "https://connect.example.com", - content_guid = "00000000-0000-0000-0000-000000000001" - ) + note = "Check the governed measure.\nThe SQL is otherwise correct." ) ) }) -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 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("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) + 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")) + 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, replaced, and removed", { + parent <- withr::local_tempdir() + review_dir <- file.path(parent, "reviews") + trajectories <- list(`conv/1` = review_test_turns()) + + write_conversation_review( + review_dir, + trajectories, + conversation = 1L, + flags = "conv/1#1", + notes = list() + ) + 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) + + write_conversation_review( + review_dir, + trajectories, + conversation = 1L, + flags = "conv/1", + notes = list() + ) + expect_equal(read_review_state(review_dir)$flags, "conv/1") + + unknown <- review_document_path(review_dir, "unknown") + writeLines( + review_document( + "unknown", + review_test_turns(), + list(conversation = TRUE, exchanges = integer()), + list(), + 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() + ) + + 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..50c6796 100644 --- a/tests/testthat/test-trajectory-review.R +++ b/tests/testthat/test-trajectory-review.R @@ -355,10 +355,15 @@ 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_store(review_dir) + ), { session$setInputs( group_by = "conversation", @@ -401,7 +406,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 +416,13 @@ 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_store(review_dir) + ) shiny::testServer( server, @@ -452,31 +462,52 @@ 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") + vapply(restored$notes, `[[`, character(1), "note"), + c("Wrong join, should use orders.", "Reviewed end to end; looks fine.") ) - 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" - ) +}) + +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" ) - restored <- read_review_records(review_file) - expect_equal(review_flags(restored), c("conv1", "conv1#1")) - expect_equal( - vapply(review_notes(restored), `[[`, character(1), "note"), - c("Wrong join, should use orders.", "Reviewed end to end; looks fine.") + 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) + } ) }) @@ -641,6 +672,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")