From 9a00b592f065d9df99996528217d26fda568b6f9 Mon Sep 17 00:00:00 2001 From: Brummel Date: Sun, 26 Apr 2026 22:09:08 +0200 Subject: [PATCH] fix: Preserve manual oneliner across delete and reset paths Consolidate the sticky-Manual rule for oneliner.json into a single helper paths::delete_oneliner_unless_manual that holds the per-case lock, reads the state, keeps OnelinerState::Manual, and removes any LLM-derived variant. Three previously-direct deletion paths now route through it: handle_delete_recording, reset_case_artefacts (admin single-reset), and bulk_reset (admin bulk action). Before this fix a clinician's manual override was wiped by a recording delete, letting the LLM auto-regen path overwrite it on the next view. Tests: 4 unit tests for the wrapper contract, integration tests for the recording-delete and case-reset endpoints (Manual preserved, Ready wiped), and a static-scan invariant that fails on direct remove_file(... ONELINER_FILENAME) calls outside paths.rs. --- server/src/paths.rs | 130 ++++++++++++++++++ server/src/routes/bulk.rs | 9 +- server/src/routes/case_actions.rs | 32 +++-- server/tests/delete_recording_test.rs | 67 ++++++++- server/tests/oneliner_disk_invariants_test.rs | 88 ++++++++++++ server/tests/reset_case_test.rs | 108 +++++++++++++++ 6 files changed, 421 insertions(+), 13 deletions(-) create mode 100644 server/tests/oneliner_disk_invariants_test.rs create mode 100644 server/tests/reset_case_test.rs diff --git a/server/src/paths.rs b/server/src/paths.rs index bd86166..656dca1 100644 --- a/server/src/paths.rs +++ b/server/src/paths.rs @@ -4,6 +4,9 @@ use doctate_common::TranscriptState; use doctate_common::oneliners::{ONELINER_FILENAME, OnelinerState}; use serde::{Deserialize, Serialize}; use time::OffsetDateTime; +use tracing::info; + +use crate::oneliner_locks::OnelinerLocks; /// Absolute on-disk location of a case directory. /// @@ -95,6 +98,46 @@ pub async fn write_oneliner_state(case_dir: &Path, state: &OnelinerState) -> std tokio::fs::rename(&tmp_path, &final_path).await } +/// Delete `case_dir/oneliner.json` unless it persists a manual override. +/// +/// Acquires the per-case oneliner lock, reads the current state, and +/// removes the file only when the on-disk state is missing, malformed, +/// or one of the LLM-derived variants (`Ready`, `Empty`, `Error`). +/// `OnelinerState::Manual` is preserved untouched — once a clinician +/// has set the oneliner by hand, it must survive every server-driven +/// artefact wipe (recording delete, admin reset). The only legitimate +/// path that may remove a manual oneliner is the user-initiated +/// `purge-closed`, which removes the entire case directory and is +/// therefore not routed through this helper. +/// +/// Returns `Ok(true)` if the file was deleted (or was already absent), +/// `Ok(false)` if a manual override was preserved. +pub async fn delete_oneliner_unless_manual( + case_dir: &Path, + locks: &OnelinerLocks, +) -> std::io::Result { + // Hold the per-case lock across read+decide+delete so a concurrent + // worker cannot flip Manual <-> Ready between our check and our + // remove. Worker write paths take the same lock around their own + // re-check window, so the two paths serialize cleanly. + let _guard = locks.lock_for(case_dir).await; + + let (state, _) = read_oneliner_state(case_dir).await; + if matches!(state, Some(OnelinerState::Manual { .. })) { + info!( + case_dir = %case_dir.display(), + "manual oneliner preserved across artefact wipe" + ); + return Ok(false); + } + + match tokio::fs::remove_file(case_dir.join(ONELINER_FILENAME)).await { + Ok(_) => Ok(true), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(true), + Err(e) => Err(e), + } +} + /// Load the `.transcript.txt` sidecar for a recording as a /// [`TranscriptState`]. /// @@ -111,3 +154,90 @@ pub async fn read_transcript_state(audio_stem_path: &Path) -> TranscriptState { let raw = tokio::fs::read_to_string(&transcript_path).await.ok(); TranscriptState::from_raw(raw.as_deref()) } + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + fn manual_state() -> OnelinerState { + OnelinerState::Manual { + text: "manuell-arzt".to_owned(), + set_at: "2026-04-26T10:00:00Z".to_owned(), + } + } + + fn ready_state() -> OnelinerState { + OnelinerState::Ready { + text: "knee, left, pain".to_owned(), + generated_at: "2026-04-26T10:00:00Z".to_owned(), + } + } + + #[tokio::test] + async fn delete_oneliner_unless_manual_keeps_manual() { + let dir = TempDir::new().unwrap(); + write_oneliner_state(dir.path(), &manual_state()) + .await + .unwrap(); + + let locks = OnelinerLocks::new(); + let removed = delete_oneliner_unless_manual(dir.path(), &locks) + .await + .unwrap(); + + assert!(!removed, "Manual must be preserved"); + let (state, _) = read_oneliner_state(dir.path()).await; + assert!( + matches!(state, Some(OnelinerState::Manual { ref text, .. }) if text == "manuell-arzt"), + "Manual state must remain identical on disk, got {state:?}" + ); + } + + #[tokio::test] + async fn delete_oneliner_unless_manual_removes_ready() { + let dir = TempDir::new().unwrap(); + write_oneliner_state(dir.path(), &ready_state()) + .await + .unwrap(); + + let locks = OnelinerLocks::new(); + let removed = delete_oneliner_unless_manual(dir.path(), &locks) + .await + .unwrap(); + + assert!(removed, "Ready must be deleted"); + assert!(!dir.path().join(ONELINER_FILENAME).exists()); + } + + #[tokio::test] + async fn delete_oneliner_unless_manual_handles_missing_file() { + let dir = TempDir::new().unwrap(); + let locks = OnelinerLocks::new(); + + let removed = delete_oneliner_unless_manual(dir.path(), &locks) + .await + .expect("missing file must not be an error"); + + assert!(removed, "absent file is reported as deleted"); + } + + #[tokio::test] + async fn delete_oneliner_unless_manual_handles_malformed_json() { + // Malformed JSON must not lock the case in place forever — the + // file is treated like a non-Manual state and removed so the + // pipeline can re-derive a fresh oneliner. + let dir = TempDir::new().unwrap(); + tokio::fs::write(dir.path().join(ONELINER_FILENAME), b"{ not json") + .await + .unwrap(); + + let locks = OnelinerLocks::new(); + let removed = delete_oneliner_unless_manual(dir.path(), &locks) + .await + .unwrap(); + + assert!(removed, "malformed file must be cleared"); + assert!(!dir.path().join(ONELINER_FILENAME).exists()); + } +} diff --git a/server/src/routes/bulk.rs b/server/src/routes/bulk.rs index 3b2c82c..d70a8fd 100644 --- a/server/src/routes/bulk.rs +++ b/server/src/routes/bulk.rs @@ -14,6 +14,7 @@ use crate::config::Config; use crate::csrf::{CsrfForm, HasCsrfToken}; use crate::error::AppError; use crate::events::{self, CaseEventKind, EventSender}; +use crate::oneliner_locks::OnelinerLocks; use crate::paths::{CloseMarker, write_close_marker}; use crate::routes::case_actions::{ build_analysis_input, reset_case_artefacts, write_input_create_new, @@ -45,6 +46,7 @@ pub async fn handle_bulk_action( State(config): State>, State(analyze_tx): State, State(events_tx): State, + State(locks): State, CsrfForm(form): CsrfForm, ) -> Result { // Bulk actions are admin-only across the board: the UI hides the @@ -78,7 +80,9 @@ pub async fn handle_bulk_action( .await } BulkAction::Close => bulk_close(&events_tx, &user.slug, &user_root, &form.case_ids).await, - BulkAction::Reset => bulk_reset(&events_tx, &user.slug, &user_root, &form.case_ids).await, + BulkAction::Reset => { + bulk_reset(&events_tx, &locks, &user.slug, &user_root, &form.case_ids).await + } } Ok(Redirect::to("/web/cases")) @@ -185,6 +189,7 @@ async fn bulk_close( async fn bulk_reset( events_tx: &EventSender, + locks: &OnelinerLocks, slug: &str, user_root: &std::path::Path, case_ids: &[String], @@ -202,7 +207,7 @@ async fn bulk_reset( skipped += 1; continue; }; - if let Err(e) = reset_case_artefacts(&case_dir).await { + if let Err(e) = reset_case_artefacts(&case_dir, locks).await { warn!(slug = %slug, case_id = %case_id, error = %e, "bulk-reset: failed"); skipped += 1; continue; diff --git a/server/src/routes/case_actions.rs b/server/src/routes/case_actions.rs index 52a6e45..8f342a3 100644 --- a/server/src/routes/case_actions.rs +++ b/server/src/routes/case_actions.rs @@ -12,7 +12,6 @@ use tokio::io::AsyncWriteExt; use tracing::{info, warn}; use doctate_common::TranscriptState; -use doctate_common::oneliners::ONELINER_FILENAME; use doctate_common::timestamp::{filename_stem_to_recorded_at, now_rfc3339}; use crate::analyze::{ @@ -24,8 +23,10 @@ use crate::config::Config; use crate::csrf::{CsrfForm, CsrfOnlyForm, HasCsrfToken}; use crate::error::AppError; use crate::events::{self, CaseEventKind, EventSender}; +use crate::oneliner_locks::OnelinerLocks; use crate::paths::{ - CLOSE_MARKER, CloseMarker, read_close_marker, read_transcript_state, write_close_marker, + CLOSE_MARKER, CloseMarker, delete_oneliner_unless_manual, read_close_marker, + read_transcript_state, write_close_marker, }; use crate::routes::user_web::locate_case_or_404; use crate::routes::web::validate_filename; @@ -297,18 +298,27 @@ pub(crate) async fn read_document(case_dir: &Path) -> Option { .ok() } -/// Reset a case to its raw audio: delete all derived artefacts -/// (transcripts, oneliner, analysis input, document) and rename every +/// Reset a case to its raw audio: delete derived artefacts +/// (transcripts, analysis input, document, non-Manual oneliner) and rename every /// `*.m4a.failed` back to `*.m4a` so the transcribe self-heal picks it /// up again. Idempotent: missing files are no-ops. -pub(crate) async fn reset_case_artefacts(case_dir: &Path) -> std::io::Result<()> { - for name in [DOCUMENT_FILE, ANALYSIS_INPUT_FILE, ONELINER_FILENAME] { +/// +/// `oneliner.json` is special: a [`OnelinerState::Manual`] override is +/// preserved (per the sticky-Manual rule). Routed through +/// [`delete_oneliner_unless_manual`] so the per-case lock serializes +/// the wipe with concurrent worker writes. +pub(crate) async fn reset_case_artefacts( + case_dir: &Path, + locks: &OnelinerLocks, +) -> std::io::Result<()> { + for name in [DOCUMENT_FILE, ANALYSIS_INPUT_FILE] { match tokio::fs::remove_file(case_dir.join(name)).await { Ok(_) => {} Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} Err(e) => return Err(e), } } + delete_oneliner_unless_manual(case_dir, locks).await?; let mut entries = tokio::fs::read_dir(case_dir).await?; while let Some(entry) = entries.next_entry().await? { let name = entry.file_name(); @@ -431,6 +441,7 @@ pub async fn handle_reset_case( user: AuthenticatedWebUser, State(config): State>, State(events_tx): State, + State(locks): State, CaseIdPath(case_id): CaseIdPath, CsrfForm(form): CsrfForm, ) -> Result { @@ -441,7 +452,7 @@ pub async fn handle_reset_case( let user_root = config.data_path.join(&user.slug); let case_dir = locate_case_or_404(&user_root, &case_id, &user.slug, "reset").await?; - reset_case_artefacts(&case_dir) + reset_case_artefacts(&case_dir, &locks) .await .map_err(|e| AppError::Internal(format!("reset: {e}")))?; @@ -574,6 +585,7 @@ pub async fn handle_delete_recording( user: AuthenticatedWebUser, State(config): State>, State(events_tx): State, + State(locks): State, CaseIdPath(case_id): CaseIdPath, CsrfForm(form): CsrfForm, ) -> Result { @@ -600,7 +612,11 @@ pub async fn handle_delete_recording( for p in &targets { remove_if_exists(p).await?; } - remove_if_exists(&case_dir.join(ONELINER_FILENAME)).await?; + // Oneliner is the only artefact carrying a possibly-manual override — + // route through the wrapper so a clinician's edit survives the wipe. + delete_oneliner_unless_manual(&case_dir, &locks) + .await + .map_err(|e| AppError::Internal(format!("oneliner: {e}")))?; info!( slug = %user.slug, diff --git a/server/tests/delete_recording_test.rs b/server/tests/delete_recording_test.rs index fe219bf..75de2be 100644 --- a/server/tests/delete_recording_test.rs +++ b/server/tests/delete_recording_test.rs @@ -10,11 +10,13 @@ mod common; use axum::http::StatusCode; +use doctate_common::oneliners::OnelinerState; use tower::util::ServiceExt; use common::{ ANALYSIS_INPUT_FILE, DOCUMENT_FILE, ONELINER_FILENAME, TestConfig, csrf_form_post, - login_with_csrf, paths, seed_case, seed_recording_with_sidecars, test_user, + login_with_csrf, paths, seed_case, seed_oneliner_ready, seed_oneliner_state, + seed_recording_with_sidecars, test_user, }; fn delete_body(filename: &str) -> String { @@ -30,8 +32,11 @@ async fn delete_recording_removes_file_and_sidecars_and_invalidates_derived() { let victim = seed_recording_with_sidecars(&case_dir, "2026-04-19T10-00-00Z", "delete me"); let survivor = seed_recording_with_sidecars(&case_dir, "2026-04-19T11-00-00Z", "keep me"); - // Derived artefacts the delete must also purge. - std::fs::write(case_dir.join(ONELINER_FILENAME), b"{}").unwrap(); + // Derived artefacts the delete must also purge. The oneliner is + // seeded as `Ready` (a real LLM-derived state) so the new sticky- + // Manual wrapper sees a non-Manual variant and proceeds with the + // delete — keeps the original contract „auto state must be wiped". + seed_oneliner_ready(&case_dir, "knee, left, pain"); std::fs::write(case_dir.join(DOCUMENT_FILE), b"# stale").unwrap(); std::fs::write(case_dir.join(ANALYSIS_INPUT_FILE), b"{}").unwrap(); @@ -258,3 +263,59 @@ async fn delete_recording_accepts_failed_suffix() { assert_eq!(resp.status().as_u16() / 100, 3); assert!(!case_dir.join("2026-04-19T10-00-00Z.m4a.failed").exists()); } + +#[tokio::test] +async fn delete_recording_preserves_manual_oneliner_override() { + // Bug regression: deleting a recording must NOT wipe a clinician's + // manual oneliner override. The Manual variant is sticky for the + // lifetime of the case — only `purge-closed` (whole-case delete) + // is allowed to remove it. + let cfg = TestConfig::new().with_user(test_user("dr_a")).build(); + let case_id = "77777777-7777-7777-7777-777777777777"; + let case_dir = seed_case(&cfg.data_path, "dr_a", case_id); + + let victim = seed_recording_with_sidecars(&case_dir, "2026-04-26T10-00-00Z", "raw text"); + seed_recording_with_sidecars(&case_dir, "2026-04-26T11-00-00Z", "more raw text"); + + let manual_text = "55 J., Knieschmerz links — manuell vom Arzt"; + seed_oneliner_state( + &case_dir, + &OnelinerState::Manual { + text: manual_text.into(), + set_at: "2026-04-26T12:00:00Z".into(), + }, + ); + + let (app, store) = doctate_server::create_router_and_session_store(cfg); + let (cookie, csrf) = login_with_csrf(&app, &store, "dr_a", "s").await; + + let resp = app + .clone() + .oneshot(csrf_form_post( + paths::recording_delete(case_id), + &cookie, + &csrf, + &delete_body(&victim), + )) + .await + .unwrap(); + assert_eq!( + resp.status().as_u16() / 100, + 3, + "delete should still redirect on success" + ); + + // Recording is gone… + assert!(!case_dir.join(&victim).exists(), "victim m4a deleted"); + // …but the Manual override is untouched. + let bytes = std::fs::read(case_dir.join(ONELINER_FILENAME)) + .expect("oneliner.json must still exist after recording delete"); + let on_disk: OnelinerState = + serde_json::from_slice(&bytes).expect("oneliner.json must still be valid JSON"); + match on_disk { + OnelinerState::Manual { text, .. } => { + assert_eq!(text, manual_text, "manual text must be byte-identical"); + } + other => panic!("expected Manual after recording delete, got {other:?}"), + } +} diff --git a/server/tests/oneliner_disk_invariants_test.rs b/server/tests/oneliner_disk_invariants_test.rs new file mode 100644 index 0000000..687bf70 --- /dev/null +++ b/server/tests/oneliner_disk_invariants_test.rs @@ -0,0 +1,88 @@ +//! Repo invariant: the only place allowed to issue a direct +//! `remove_file(... ONELINER_FILENAME)` (or the literal `"oneliner.json"`) +//! is `server/src/paths.rs`, which hosts the canonical wrapper +//! `delete_oneliner_unless_manual`. Every other call-site MUST go +//! through that wrapper so the sticky-Manual rule +//! (`OnelinerState::Manual { .. }` survives every server-driven wipe) +//! is enforced uniformly. +//! +//! Static, AST-free scan: a line in a non-whitelisted source file is a +//! violation iff it mentions `remove_file` together with either the +//! constant `ONELINER_FILENAME` or the literal `"oneliner.json"` — +//! pragmatic and good enough to catch the common regression pattern. +//! Comment lines (starting with `//` after trim) are ignored so this +//! file's own documentation does not self-trip. + +use std::fs; +use std::path::{Path, PathBuf}; + +/// Files that may legitimately call `remove_file(... oneliner.json)`. +/// Paths are relative to `server/src/`. +const REMOVE_WHITELIST: &[&str] = &["paths.rs"]; + +#[test] +fn no_direct_oneliner_remove_outside_paths_rs() { + let server_src = Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut rs_files = Vec::new(); + collect_rs_files(&server_src, &mut rs_files); + assert!( + !rs_files.is_empty(), + "expected to find at least one .rs file under {}", + server_src.display() + ); + + let mut violations: Vec = Vec::new(); + for path in &rs_files { + let rel = path.strip_prefix(&server_src).unwrap(); + let rel_str = rel.to_string_lossy().replace('\\', "/"); + if REMOVE_WHITELIST.iter().any(|w| rel_str == *w) { + continue; + } + let contents = match fs::read_to_string(path) { + Ok(c) => c, + Err(e) => panic!("failed to read {}: {e}", path.display()), + }; + for (idx, line) in contents.lines().enumerate() { + let trimmed = line.trim_start(); + if trimmed.starts_with("//") { + continue; + } + let mentions_remove = trimmed.contains("remove_file"); + let mentions_target = + trimmed.contains("ONELINER_FILENAME") || trimmed.contains("\"oneliner.json\""); + if mentions_remove && mentions_target { + violations.push(format!( + "{}:{}: direct remove_file on oneliner.json — \ + route through paths::delete_oneliner_unless_manual to honour \ + the sticky-Manual rule. Offending line: {}", + rel_str, + idx + 1, + line.trim() + )); + } + } + } + + assert!( + violations.is_empty(), + "Sticky-Manual oneliner invariant violated:\n - {}\n\n\ + If this is a new legitimate exception, extend REMOVE_WHITELIST in \ + tests/oneliner_disk_invariants_test.rs (and document why).", + violations.join("\n - ") + ); +} + +fn collect_rs_files(dir: &Path, out: &mut Vec) { + let entries = match fs::read_dir(dir) { + Ok(e) => e, + Err(e) => panic!("read_dir {}: {e}", dir.display()), + }; + for entry in entries.flatten() { + let path = entry.path(); + if path.is_dir() { + collect_rs_files(&path, out); + } else if path.extension().and_then(|s| s.to_str()) == Some("rs") { + out.push(path); + } + } +} diff --git a/server/tests/reset_case_test.rs b/server/tests/reset_case_test.rs new file mode 100644 index 0000000..71cd61f --- /dev/null +++ b/server/tests/reset_case_test.rs @@ -0,0 +1,108 @@ +//! Admin-only case reset endpoint: POST /web/cases/{case_id}/reset. +//! +//! Verifies the sticky-Manual rule for the reset path: a clinician-set +//! `OnelinerState::Manual` must survive an admin-triggered reset, while +//! all LLM-derived oneliner states (`Ready`, `Empty`, `Error`) are +//! wiped as before. The wider „transcripts removed, .m4a.failed +//! re-armed" contract is covered in `analyze_test.rs`. + +mod common; + +use axum::http::{StatusCode, header}; +use doctate_common::oneliners::OnelinerState; +use tower::util::ServiceExt; + +use common::{ + ONELINER_FILENAME, TestConfig, csrf_form_post, login_with_csrf, paths, seed_case, + seed_oneliner_ready, seed_oneliner_state, seed_recording, test_admin, +}; + +fn cfg_admin(label: &'static str) -> std::sync::Arc { + TestConfig::new() + .with_label(label) + .with_user(test_admin("dr_a")) + .build() +} + +#[tokio::test] +async fn reset_case_preserves_manual_oneliner_override() { + // Bug regression: an admin reset must NOT destroy a clinician's + // manual oneliner override. The Manual variant is sticky for the + // lifetime of the case. + let cfg = cfg_admin("reset-manual"); + let case_id = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"; + let dir = seed_case(&cfg.data_path, "dr_a", case_id); + seed_recording(&dir, "2026-04-26T10-00-00Z", Some("Knie links")); + + let manual_text = "55 J., Knieschmerz links — manuell vom Arzt"; + seed_oneliner_state( + &dir, + &OnelinerState::Manual { + text: manual_text.into(), + set_at: "2026-04-26T12:00:00Z".into(), + }, + ); + + let (app, store) = doctate_server::create_router_and_session_store(cfg); + let (cookie, csrf) = login_with_csrf(&app, &store, "dr_a", "s").await; + + let resp = app + .oneshot(csrf_form_post( + paths::case_reset(case_id), + &cookie, + &csrf, + "", + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::SEE_OTHER); + assert_eq!( + resp.headers().get(header::LOCATION).unwrap(), + &format!("/web/cases/{case_id}") + ); + + // Transcript was wiped (the rest of the reset semantics still works). + assert!(!dir.join("2026-04-26T10-00-00Z.transcript.txt").exists()); + // …but the Manual override is untouched. + let bytes = std::fs::read(dir.join(ONELINER_FILENAME)) + .expect("oneliner.json must still exist after reset"); + let on_disk: OnelinerState = + serde_json::from_slice(&bytes).expect("oneliner.json must still be valid JSON"); + match on_disk { + OnelinerState::Manual { text, .. } => { + assert_eq!(text, manual_text, "manual text must be byte-identical"); + } + other => panic!("expected Manual after reset, got {other:?}"), + } +} + +#[tokio::test] +async fn reset_case_removes_auto_oneliner() { + // Positive counterpart: a `Ready` oneliner (LLM-derived auto state) + // must still be wiped by reset — otherwise the wrapper would be + // protecting too much. + let cfg = cfg_admin("reset-auto"); + let case_id = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"; + let dir = seed_case(&cfg.data_path, "dr_a", case_id); + seed_recording(&dir, "2026-04-26T10-00-00Z", Some("Knie links")); + seed_oneliner_ready(&dir, "knee, left, pain"); + + let (app, store) = doctate_server::create_router_and_session_store(cfg); + let (cookie, csrf) = login_with_csrf(&app, &store, "dr_a", "s").await; + + let resp = app + .oneshot(csrf_form_post( + paths::case_reset(case_id), + &cookie, + &csrf, + "", + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::SEE_OTHER); + + assert!( + !dir.join(ONELINER_FILENAME).exists(), + "Ready oneliner must be wiped by reset" + ); +}