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.
This commit is contained in:
@@ -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:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<String> = 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<PathBuf>) {
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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<doctate_server::config::Config> {
|
||||
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"
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user