diff --git a/CHANGELOG.md b/CHANGELOG.md index c4ccc8a..3f6c065 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,22 @@ ## Unreleased +- Stop untranslated prose from hiding behind reference context. A Notes, + Bibliography, References, Works Cited or Index section title, a leading + `p. 12.`, or an `in English:` gloss now only excuses an unchanged block + shorter than 120 characters; longer unchanged or near-copied text is checked + like any other prose. A long citation that should stay verbatim needs + explicit acceptance with `bookforge correct`. +- Hold saved and cached results to the same source-copy rule as fresh output. + A cache hit that fresh output could not pass is treated as a miss. On + resume, previously accepted provider output that contains a blocking copy + moves to `needs_review` with its text, usage and provenance unchanged, and + is not retranslated automatically. Human-accepted segments are never + rechecked and are never shared across jobs. +- Keep human corrections out of the double-check pass on resume. The stored + correction was protected, but the rebuilt EPUB could still pick up an + automated rewrite of the same segment. + - Accept batch translations whose only source-copy finding is an editorial warning for a title, heading, or short proper-name line. Previously the report called these warnings while the parser still rejected the blocks. diff --git a/crates/bookforge-cli/src/commands/resume.rs b/crates/bookforge-cli/src/commands/resume.rs index 1931bd5..1eff8ea 100644 --- a/crates/bookforge-cli/src/commands/resume.rs +++ b/crates/bookforge-cli/src/commands/resume.rs @@ -565,6 +565,20 @@ async fn run_inner( } }; glossary.run_config.guidance_by_segment = store.load_retry_guidance(&job.id)?; + // Recheck accepted provider output only after the saved input and prompt + // contract passed preflight, and before it can seed translation context. + let revalidated = super::translate::revalidate_stored_translations( + store, + &job.id, + &segments, + snapshot.source_language.as_deref(), + &snapshot.target_language, + )?; + if print_stdout && revalidated > 0 { + println!( + "Review: {revalidated} saved segment(s) contain untranslated source-language prose; kept for review." + ); + } let context_cfg = snapshot_context_run_config(snapshot); let context_registry: Option> = if context_cfg.enabled() { let registry = Arc::new(ContextRegistry::new(&segments)); @@ -820,9 +834,24 @@ async fn run_inner( if double_check_runtime.settings.double_check.mode != DoubleCheckMode::Off && !snapshot.finalize.double_check_complete { + // Human acceptance stays authoritative during optional automated + // correction. The store refuses to overwrite a corrected row, but the + // EPUB is rebuilt from in-memory translations, so corrected segments + // must never enter the double-check pass at all. + let human_corrected = store + .load_terminal_segment_translations(&job.id)? + .into_iter() + .filter(|saved| saved.human_corrected) + .map(|saved| saved.segment_id) + .collect::>(); + let reviewable_segments = segments + .iter() + .filter(|segment| !human_corrected.contains(&segment.id.0)) + .cloned() + .collect::>(); let changed_segment_ids = match double_check_after_resume( &job, - &segments, + &reviewable_segments, &mut translations, &double_check_run_config, snapshot, @@ -1229,6 +1258,11 @@ async fn double_check_after_resume( progress: Arc, stop_cancel_token: tokio_util::sync::CancellationToken, ) -> Result> { + // Nothing reviewable (e.g. every segment is human-accepted): construct no + // provider and spend nothing. + if segments.is_empty() { + return Ok(Vec::new()); + } let double_check = &settings.double_check; let provider_name = double_check .provider @@ -1518,7 +1552,7 @@ mod tests { SegmentSource, SegmentTextRun, }, }; - use bookforge_store::{CreateJob, SaveNeedsReview, SaveTranslation}; + use bookforge_store::{CreateJob, SaveManualCorrection, SaveNeedsReview, SaveTranslation}; use std::{ io::Write, path::{Path, PathBuf}, @@ -2325,9 +2359,129 @@ mod tests { ); } + #[tokio::test] + async fn resume_revalidates_saved_copies_and_keeps_explicit_acceptance() { + let chapter = r#" + +Endnotes + +

Endnotes

+

This deliberately long English paragraph explains why the narrator returned to the village after the winter. The people had kept every letter and remembered the promises, but their circumstances had changed and they needed an answer before the next meeting.

+ +"#; + let mut settings = TranslationProfile::V1Fast.resolve(); + settings.batch.enabled = false; + settings.segmentation.max_segment_tokens = 1000; + settings.double_check.mode = DoubleCheckMode::Off; + let mut fixture = resume_fixture_with_chapter(settings, 1, chapter, |segment| { + segment + .source + .blocks + .iter() + .any(|block| block.text.chars().count() > 120) + }); + let segment = fixture.segments[0].clone(); + let blocks = segment + .source + .blocks + .iter() + .map(|block| BlockTranslation { + block_id: block.block_id.clone(), + text: block.text.clone(), + }) + .collect::>(); + fixture + .store + .save_translation(SaveTranslation { + job_id: &fixture.job.id, + segment_id: &segment.id.0, + translated_text: &segment.source.text, + blocks: &blocks, + // Revalidation follows the saved provider, which can differ + // from the job's primary provider after a fallback. + provider: "deepseek", + model: "legacy-provider", + prompt_version: "v1", + input_tokens: Some(91), + input_cached_tokens: Some(17), + output_tokens: Some(61), + tokens_estimated: false, + }) + .unwrap(); + + run_fixture(&mut fixture).await.unwrap(); + + let records = fixture + .store + .load_terminal_segment_translations(&fixture.job.id) + .unwrap(); + assert_eq!(records[0].status, "needs_review"); + assert_eq!(records[0].blocks, blocks); + assert_eq!(records[0].provider, "deepseek"); + let summary = fixture.store.summary(&fixture.job.id).unwrap().unwrap(); + assert_eq!( + ( + summary.needs_review, + summary.input_tokens, + summary.output_tokens + ), + (1, 91, 61) + ); + assert_eq!( + fixture.store.segment_records(&fixture.job.id).unwrap()[0].attempts, + 1, + "a flagged copy is kept for review, not retranslated" + ); + assert!(fixture.snapshot.output_path.exists()); + + fixture + .store + .save_manual_correction(SaveManualCorrection { + job_id: &fixture.job.id, + segment_id: &segment.id.0, + translated_text: &segment.source.text, + blocks: &blocks, + }) + .unwrap(); + let accepted = fixture + .store + .load_terminal_segment_translations(&fixture.job.id) + .unwrap(); + // Every segment is human-accepted, so optional double-check must not + // build its provider. This one points at a closed local port with no + // API key: reaching it at all would fail the resume. + let double_check = &mut fixture.snapshot.settings.double_check; + double_check.mode = DoubleCheckMode::Full; + double_check.provider = Some("openai-compatible".into()); + double_check.base_url = Some("http://127.0.0.1:9/v1".into()); + double_check.api_key_env = Some("BOOKFORGE_TEST_UNSET_DOUBLE_CHECK_KEY".into()); + fixture.snapshot.finalize.double_check_complete = false; + run_fixture(&mut fixture).await.unwrap(); + assert_eq!( + fixture + .store + .load_terminal_segment_translations(&fixture.job.id) + .unwrap(), + accepted + ); + assert!(accepted[0].human_corrected); + assert_eq!(accepted[0].status, "succeeded"); + } + fn resume_fixture(settings: ResolvedRunSettings, segment_count: usize) -> ResumeFixture { + resume_fixture_with_chapter(settings, segment_count, RESUME_CHAPTER_ONE, |_| true) + } + + /// [`resume_fixture`] with a custom first chapter, keeping the first + /// `segment_count` segments that `select` accepts. + fn resume_fixture_with_chapter( + settings: ResolvedRunSettings, + segment_count: usize, + chapter_one: &str, + select: impl Fn(&Segment) -> bool, + ) -> ResumeFixture { let tempdir = tempfile::tempdir().expect("tempdir should be created"); - let input = fixture_input(); + let input = fixture_input_with_chapter(chapter_one); let output = tempdir.path().join("translated.epub"); let events = tempdir.path().join("events.jsonl"); let store = JobStore::open(tempdir.path().join("jobs.sqlite")).expect("store should open"); @@ -2354,6 +2508,7 @@ mod tests { ); let segments = all_segments .into_iter() + .filter(|segment| select(segment)) .take(segment_count) .collect::>(); let prompt_version = "v1"; @@ -2644,6 +2799,10 @@ mod tests { // Builds a synthetic EPUB so resume tests exercise the EPUB path in CI // without relying on local, gitignored book fixtures. fn fixture_input() -> PathBuf { + fixture_input_with_chapter(RESUME_CHAPTER_ONE) + } + + fn fixture_input_with_chapter(chapter_one: &str) -> PathBuf { let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) .expect("clock should be after epoch") @@ -2652,11 +2811,11 @@ mod tests { "bookforge-resume-fixture-{}-{nanos}.epub", std::process::id() )); - build_resume_epub(&path); + build_resume_epub(&path, chapter_one); path } - fn build_resume_epub(path: &Path) { + fn build_resume_epub(path: &Path, chapter_one: &str) { let file = std::fs::File::create(path).expect("fixture EPUB should be creatable"); let mut zip = ZipWriter::new(file); let stored = SimpleFileOptions::default().compression_method(CompressionMethod::Stored); @@ -2669,7 +2828,7 @@ mod tests { zip.start_file("content.opf", deflated).unwrap(); zip.write_all(RESUME_OPF.as_bytes()).unwrap(); zip.start_file("chapter1.xhtml", deflated).unwrap(); - zip.write_all(RESUME_CHAPTER_ONE.as_bytes()).unwrap(); + zip.write_all(chapter_one.as_bytes()).unwrap(); zip.start_file("chapter2.xhtml", deflated).unwrap(); zip.write_all(RESUME_CHAPTER_TWO.as_bytes()).unwrap(); zip.finish().unwrap(); diff --git a/crates/bookforge-cli/src/commands/translate/cache.rs b/crates/bookforge-cli/src/commands/translate/cache.rs index c3a1ec7..85766d8 100644 --- a/crates/bookforge-cli/src/commands/translate/cache.rs +++ b/crates/bookforge-cli/src/commands/translate/cache.rs @@ -1,7 +1,11 @@ +use std::collections::HashMap; + use anyhow::Result; -use bookforge_core::segment::{Segment, SegmentStatus}; -use bookforge_llm::SegmentTranslation; -use bookforge_store::{JobStore, SaveCachedTranslation}; +use bookforge_core::finding::{EngineFinding, QaFindingKind}; +use bookforge_core::ir::QaFindingSeverity; +use bookforge_core::segment::{BlockTranslation, Segment, SegmentStatus}; +use bookforge_llm::{SegmentTranslation, blocking_source_copy_error, should_validate_source_copy}; +use bookforge_store::{JobStore, SaveCachedTranslation, SegmentStatus as StoredStatus}; #[derive(Clone, Copy)] pub struct CacheContext<'a> { @@ -62,6 +66,21 @@ pub fn apply_cached_translations( let Some(hit) = hits.get(&segment.id.0) else { continue; }; + // Cached output may predate the current source-copy rule. A hit that + // fresh batch output could not pass becomes an ordinary miss; the job + // that produced it is untouched. The lookup never offers manual + // corrections, so this only ever rejects provider output. + if !stored_source_copies( + segment, + &hit.blocks, + cache.provider, + cache.source_lang, + cache.target_lang, + ) + .is_empty() + { + continue; + } cache.store.save_cached_translation(SaveCachedTranslation { job_id: cache.job_id, segment_id: &segment.id.0, @@ -90,6 +109,105 @@ pub fn apply_cached_translations( Ok(cached) } +/// Blocking source copies among a stored result's `blocks`, judged by the +/// rule batch acceptance applies to fresh output. +fn stored_source_copies( + segment: &Segment, + blocks: &[BlockTranslation], + provider: &str, + source_lang: Option<&str>, + target_lang: &str, +) -> Vec { + if !should_validate_source_copy(provider, source_lang, target_lang) { + return Vec::new(); + } + let by_id = blocks + .iter() + .map(|block| (&block.block_id, block.text.as_str())) + .collect::>(); + segment + .source + .blocks + .iter() + .filter_map(|source| { + let translation = by_id.get(&source.block_id)?; + let message = blocking_source_copy_error( + &source.kind, + &source.text, + translation, + segment.metadata.section_title.as_deref(), + )?; + Some( + EngineFinding::new(QaFindingKind::SourceCopyUnchanged, message) + .with_block_id(source.block_id.0.clone()) + .with_severity(QaFindingSeverity::Error), + ) + }) + .collect() +} + +/// Recheck provider output accepted by an earlier run against the current +/// source-copy rule. A blocking copy moves its segment to `needs_review` +/// with the saved text, blocks, usage and provenance untouched; ordinary +/// resume does not retranslate it. Human-corrected segments are +/// authoritative and never rechecked. Returns how many were reclassified. +pub(crate) fn revalidate_stored_translations( + store: &JobStore, + job_id: &str, + segments: &[Segment], + source_lang: Option<&str>, + target_lang: &str, +) -> Result { + let by_id = segments + .iter() + .map(|segment| (segment.id.0.as_str(), segment)) + .collect::>(); + let mut reclassified = 0; + for saved in store.load_terminal_segment_translations(job_id)? { + if saved.human_corrected + || !matches!( + saved.segment_status(), + StoredStatus::Succeeded | StoredStatus::SkippedCached + ) + { + continue; + } + let Some(segment) = by_id.get(saved.segment_id.as_str()) else { + continue; + }; + let copies = stored_source_copies( + segment, + &saved.blocks, + &saved.provider, + source_lang, + target_lang, + ); + if copies.is_empty() { + continue; + } + let error = format!( + "saved translation failed source-copy revalidation: {}", + copies + .iter() + .map(|finding| format!( + "block {}: {}", + finding.block_id.as_deref().unwrap_or_default(), + finding.message + )) + .collect::>() + .join("; ") + ); + reclassified += usize::from(store.mark_stored_translation_for_review( + job_id, + &saved.segment_id, + &saved.translated_text, + &error, + &copies, + )?); + } + Ok(reclassified) +} + pub fn pending_segments_for_job( store: &JobStore, job_id: &str, @@ -106,3 +224,6 @@ pub fn pending_segments_for_job( .cloned() .collect()) } + +#[cfg(test)] +mod tests; diff --git a/crates/bookforge-cli/src/commands/translate/cache/tests.rs b/crates/bookforge-cli/src/commands/translate/cache/tests.rs new file mode 100644 index 0000000..b106656 --- /dev/null +++ b/crates/bookforge-cli/src/commands/translate/cache/tests.rs @@ -0,0 +1,366 @@ +use super::*; +use bookforge_core::{ + ir::{BlockId, SectionId}, + segment::{SegmentBlock, SegmentId, SegmentMetadata, SegmentSource}, +}; +use bookforge_store::{CreateJob, JobRecord, SaveManualCorrection, SaveTranslation}; + +const SOURCE: &str = "This deliberately long English paragraph explains why the narrator returned to the village after the winter. The people had kept every letter and remembered the promises, but their circumstances had changed and they needed an answer before the next meeting."; +const TRANSLATED: &str = "Questo lungo paragrafo spiega perché il narratore tornò al villaggio dopo l'inverno. Gli abitanti avevano conservato ogni lettera e ricordavano le promesse, ma la situazione era cambiata e avevano bisogno di una risposta prima della riunione successiva."; + +struct Fixture { + _dir: tempfile::TempDir, + store: JobStore, + original: JobRecord, + destination: JobRecord, + segments: Vec, +} + +impl Fixture { + /// An `original` job holding one saved provider result for a one-block + /// segment, and a `destination` job that could reuse it from the cache. + fn new(source: &str, translated: &str, provider: &str, target: &str) -> Self { + Self::with_block( + source, + translated, + provider, + target, + "paragraph", + "Endnotes", + ) + } + + fn with_block( + source: &str, + translated: &str, + provider: &str, + target: &str, + kind: &str, + section_title: &str, + ) -> Self { + let dir = tempfile::tempdir().unwrap(); + let input = dir.path().join("input.epub"); + std::fs::write(&input, b"synthetic cache fixture").unwrap(); + let store = JobStore::open(dir.path().join("jobs.sqlite")).unwrap(); + let make_job = |name: &str| { + store + .create_job(CreateJob { + input: &input, + output: &dir.path().join(name), + source_lang: Some("English"), + target_lang: target, + provider, + model: "fixture", + base_url: None, + api_key_env: None, + book_id: None, + series_id: None, + }) + .unwrap() + }; + let original = make_job("original.epub"); + let destination = make_job("destination.epub"); + let segments = vec![Segment { + id: SegmentId("seg_a".into()), + section_id: SectionId("notes".into()), + ordinal: 0, + block_ids: vec![BlockId("b_a".into())], + source: SegmentSource { + text: source.into(), + token_estimate: 70, + blocks: vec![SegmentBlock { + block_id: BlockId("b_a".into()), + kind: kind.into(), + text: source.into(), + text_runs: vec![], + protected_spans: vec![], + }], + }, + metadata: SegmentMetadata { + section_title: Some(section_title.into()), + ..Default::default() + }, + context: Default::default(), + constraints: Default::default(), + checksum: "identical-source".into(), + }]; + for job in [&original, &destination] { + store + .insert_segments(&job.id, &segments, "batch_v3", provider, "fixture", "ns") + .unwrap(); + } + store + .save_translation(SaveTranslation { + job_id: &original.id, + segment_id: "seg_a", + translated_text: translated, + blocks: &[BlockTranslation { + block_id: BlockId("b_a".into()), + text: translated.into(), + }], + provider, + model: "fixture", + prompt_version: "batch_v3", + input_tokens: Some(91), + input_cached_tokens: Some(17), + output_tokens: Some(61), + tokens_estimated: false, + }) + .unwrap(); + Self { + _dir: dir, + store, + original, + destination, + segments, + } + } + + fn cache(&self) -> CacheContext<'_> { + CacheContext { + store: &self.store, + job_id: &self.destination.id, + prompt_version: "batch_v3", + provider: &self.destination.provider, + model: "fixture", + source_lang: Some("English"), + target_lang: &self.destination.target_lang, + cache_namespace: "ns", + } + } + + fn apply_cache(&self) -> Vec { + apply_cached_translations(&self.segments, &self.segments, self.cache()).unwrap() + } + + fn revalidate(&self, job: &JobRecord) -> usize { + revalidate_stored_translations( + &self.store, + &job.id, + &self.segments, + Some("English"), + &job.target_lang, + ) + .unwrap() + } +} + +#[test] +fn copied_prose_cache_hits_are_misses_without_modifying_the_original_job() { + for prefix in ["", "p. 12. ", "The author writes in English: "] { + let source = format!("{prefix}{SOURCE}"); + let fixture = Fixture::new(&source, &source, "deepseek", "Italian"); + let before = fixture + .store + .load_terminal_segment_translations(&fixture.original.id) + .unwrap(); + + assert!(fixture.apply_cache().is_empty(), "{prefix}"); + assert_eq!( + fixture + .store + .resumable_segment_ids(&fixture.destination.id) + .unwrap(), + ["seg_a"] + ); + assert!( + fixture + .store + .load_block_translations(&fixture.destination.id) + .unwrap() + .is_empty() + ); + assert_eq!( + fixture + .store + .load_terminal_segment_translations(&fixture.original.id) + .unwrap(), + before + ); + } +} + +#[test] +fn translated_hits_still_reuse_cache_without_new_usage() { + let fixture = Fixture::new(SOURCE, TRANSLATED, "deepseek", "Italian"); + let hits = fixture.apply_cache(); + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].status, SegmentStatus::SkippedCached); + assert_eq!(hits[0].blocks[0].text, TRANSLATED); + let summary = fixture + .store + .summary(&fixture.destination.id) + .unwrap() + .unwrap(); + assert_eq!( + (summary.cached, summary.input_tokens, summary.output_tokens), + (1, 0, 0) + ); + assert_eq!(fixture.revalidate(&fixture.original), 0); +} + +#[test] +fn copies_fresh_batch_output_may_keep_remain_reusable() { + for (source, provider, target, kind, section) in [ + // A short citation in a reference section. + ( + "Example Author, p. 12.", + "deepseek", + "Italian", + "paragraph", + "Endnotes", + ), + // A heading, which batch acceptance grades as an editorial warning. + ( + "The Long Road Home", + "deepseek", + "Italian", + "heading", + "Chapter 1", + ), + // Runs where copy detection does not apply at all. + (SOURCE, "mock", "Italian", "paragraph", "Chapter 1"), + (SOURCE, "deepseek", "English", "paragraph", "Chapter 1"), + ] { + let fixture = Fixture::with_block(source, source, provider, target, kind, section); + assert_eq!(fixture.apply_cache().len(), 1, "{source} ({kind})"); + assert_eq!( + fixture.revalidate(&fixture.original), + 0, + "{source} ({kind})" + ); + } +} + +#[test] +fn resume_flags_saved_copies_once_and_preserves_all_translation_evidence() { + let fixture = Fixture::new(SOURCE, SOURCE, "deepseek", "Italian"); + let mut expected = fixture + .store + .load_terminal_segment_translations(&fixture.original.id) + .unwrap(); + assert_eq!(fixture.revalidate(&fixture.original), 1); + assert_eq!(fixture.revalidate(&fixture.original), 0); + expected[0].status = "needs_review".into(); + expected[0].error = Some( + "saved translation failed source-copy revalidation: block b_a: translation is \ + unchanged from the source-language prose" + .into(), + ); + assert_eq!( + fixture + .store + .load_terminal_segment_translations(&fixture.original.id) + .unwrap(), + expected + ); + assert!( + fixture + .store + .resumable_segment_ids(&fixture.original.id) + .unwrap() + .is_empty(), + "ordinary resume must not retranslate a flagged segment" + ); + let summary = fixture + .store + .summary(&fixture.original.id) + .unwrap() + .unwrap(); + assert_eq!( + ( + summary.needs_review, + summary.input_tokens, + summary.output_tokens + ), + (1, 91, 61) + ); + let findings = fixture + .store + .segment_qa_findings(&fixture.original.id) + .unwrap(); + assert!(findings.iter().any(|finding| { + finding.kind == "source_copy_unchanged" + && finding.severity == "error" + && finding.block_id.as_deref() == Some("b_a") + })); + assert_eq!( + fixture + .store + .get_job(&fixture.original.id) + .unwrap() + .unwrap() + .status, + "needs_review" + ); +} + +#[test] +fn explicitly_accepted_unchanged_citations_survive_resume_and_stay_job_local() { + let citation = "Example Author, A Deliberately Long Published Title Retained in Its Original Language: Essays on History, Memory, Literature and the Interpretation of Culture (London: Example Press, 2016), pages 425–452."; + let fixture = Fixture::new(citation, citation, "deepseek", "Italian"); + assert_eq!( + fixture.revalidate(&fixture.original), + 1, + "unreviewed long citations require review" + ); + fixture + .store + .save_manual_correction(SaveManualCorrection { + job_id: &fixture.original.id, + segment_id: "seg_a", + translated_text: citation, + blocks: &[BlockTranslation { + block_id: BlockId("b_a".into()), + text: citation.into(), + }], + }) + .unwrap(); + let accepted = fixture + .store + .load_terminal_segment_translations(&fixture.original.id) + .unwrap(); + assert!(accepted[0].human_corrected); + assert_eq!(accepted[0].status, "succeeded"); + assert_eq!(fixture.revalidate(&fixture.original), 0); + assert_eq!( + fixture + .store + .load_terminal_segment_translations(&fixture.original.id) + .unwrap(), + accepted + ); + assert!( + fixture.apply_cache().is_empty(), + "a reviewer's acceptance is not shared across jobs" + ); +} + +#[test] +fn resume_revalidates_results_restored_from_the_cache_too() { + let fixture = Fixture::new(SOURCE, SOURCE, "deepseek", "Italian"); + fixture + .store + .save_cached_translation(SaveCachedTranslation { + job_id: &fixture.destination.id, + segment_id: "seg_a", + translated_text: SOURCE, + blocks: &[BlockTranslation { + block_id: BlockId("b_a".into()), + text: SOURCE.into(), + }], + provider: "deepseek", + model: "fixture", + prompt_version: "batch_v3", + }) + .unwrap(); + assert_eq!(fixture.revalidate(&fixture.destination), 1); + assert_eq!( + fixture + .store + .segment_records(&fixture.destination.id) + .unwrap()[0] + .status, + "needs_review" + ); +} diff --git a/crates/bookforge-cli/src/commands/translate/mod.rs b/crates/bookforge-cli/src/commands/translate/mod.rs index 73f33fe..81f2ce3 100644 --- a/crates/bookforge-cli/src/commands/translate/mod.rs +++ b/crates/bookforge-cli/src/commands/translate/mod.rs @@ -57,7 +57,10 @@ mod settings; mod snapshot; pub use args::TranslateArgs; -pub(crate) use cache::{CacheContext, apply_cached_translations, pending_segments_for_job}; +pub(crate) use cache::{ + CacheContext, apply_cached_translations, pending_segments_for_job, + revalidate_stored_translations, +}; use checkpointing::finalize_writer; pub(crate) use engine::{CheckpointRunContext, run_checkpointed_translation}; use engine::{record_glossary_telemetry, run_checkpointed_translation_instrumented}; diff --git a/crates/bookforge-llm/src/batch.rs b/crates/bookforge-llm/src/batch.rs index 296e024..6675732 100644 --- a/crates/bookforge-llm/src/batch.rs +++ b/crates/bookforge-llm/src/batch.rs @@ -52,6 +52,7 @@ use planning::{ set_batch_output_override, split_batch_with_config, take_batch_output_override, token_estimate, }; pub use rendering::batch_item_validation_error; +pub use rendering::blocking_source_copy_error; pub use rendering::parse_batch_response; use rendering::{ batch_prompt_template, batch_response_item_count, parse_batch_response_with_validation, diff --git a/crates/bookforge-llm/src/batch/rendering.rs b/crates/bookforge-llm/src/batch/rendering.rs index 30b8362..1997fa8 100644 --- a/crates/bookforge-llm/src/batch/rendering.rs +++ b/crates/bookforge-llm/src/batch/rendering.rs @@ -117,6 +117,21 @@ pub(crate) fn intentionally_unchanged_block( > PROPER_NOUN_TOKEN_SHARE } +/// A source-copy error that blocks acceptance: the deterministic copy check +/// fired and the block is not one a translator would intentionally leave +/// unchanged. Warning-only copies (titles, headings, short proper-noun lines) +/// return `None`, exactly as batch acceptance treats them, so cache reuse and +/// resume revalidation hold stored output to the rule fresh output passed. +pub fn blocking_source_copy_error( + block_kind: &str, + source: &str, + translation: &str, + section_title: Option<&str>, +) -> Option { + crate::validation::source_copy_validation_error(source, translation, section_title) + .filter(|_| !intentionally_unchanged_block(block_kind, source, translation)) +} + /// Whitespace-collapsed text with marker tokens stripped, for length checks. fn normalized_line(text: &str) -> String { bookforge_core::marker::strip_marker_tokens(text) diff --git a/crates/bookforge-llm/src/lib.rs b/crates/bookforge-llm/src/lib.rs index 76c058a..88d35f4 100644 --- a/crates/bookforge-llm/src/lib.rs +++ b/crates/bookforge-llm/src/lib.rs @@ -14,9 +14,9 @@ mod validation; pub use batch::{ BatchItemFailure, BatchItemTranslation, BatchKind, BatchMode, BatchSizer, BatchTranslationResult, TranslationBatch, TranslationBatchItem, - account_for_batch_prompt_overhead, block_mismatch_findings, build_translation_batches, - collect_repair_items, parse_batch_response, split_batch, translate_batches_with_callback, - translate_batches_with_control, + account_for_batch_prompt_overhead, block_mismatch_findings, blocking_source_copy_error, + build_translation_batches, collect_repair_items, parse_batch_response, split_batch, + translate_batches_with_callback, translate_batches_with_control, }; pub use concurrency::{AdaptiveLimiter, PauseSignal, PauseState}; pub use double_check::{ @@ -44,3 +44,4 @@ pub use scheduler::{ translate_segments_with_control, }; pub use telemetry::{TelemetryLog, telemetry_summary}; +pub use validation::should_validate_source_copy; diff --git a/crates/bookforge-llm/src/validation.rs b/crates/bookforge-llm/src/validation.rs index 2d8d8a5..4fe5bf7 100644 --- a/crates/bookforge-llm/src/validation.rs +++ b/crates/bookforge-llm/src/validation.rs @@ -293,7 +293,7 @@ const ITALIAN_STOP_WORDS: &[&str] = &[ "una", "uno", "voi", ]; -pub(crate) fn should_validate_source_copy( +pub fn should_validate_source_copy( provider: &str, source_language: Option<&str>, target_language: &str, @@ -311,17 +311,22 @@ pub(crate) fn source_copy_validation_error( translation: &str, section_title: Option<&str>, ) -> Option { - if is_reference_section(section_title) { - return None; - } - let source_normalized = normalized_prose(source); // Identifiers, numeric headings, and ornamental separators have no // source-language prose to translate. Their preservation is checked by // the structural/protected-span validators, not the language-copy gate. - if !source_normalized.chars().any(char::is_alphabetic) - || looks_like_page_reference(&source_normalized) - || looks_like_bilingual_gloss(&source_normalized) + if !source_normalized.chars().any(char::is_alphabetic) { + return None; + } + // Reference context (a Notes/Bibliography/Index section, a `p. 12.` + // prefix, an `in English:` gloss) can justify leaving a short citation + // unchanged, but never substantial prose: from MIN_SOURCE_CHARS up the + // copy checks always run, so an untranslated paragraph cannot hide behind + // a heading. Long verbatim citations need explicit reviewer acceptance. + if source_normalized.chars().count() < MIN_SOURCE_CHARS + && (is_reference_section(section_title) + || looks_like_page_reference(&source_normalized) + || looks_like_bilingual_gloss(&source_normalized)) { return None; } @@ -1381,9 +1386,47 @@ mod tests { } #[test] - fn allows_reference_sections() { - assert!(source_copy_validation_error(SOURCE, SOURCE, Some("Endnotes")).is_none()); - assert!(source_copy_validation_error(SOURCE, SOURCE, Some("Bibliography")).is_none()); + fn reference_context_never_exempts_substantial_prose() { + for heading in [ + None, + Some("Notes"), + Some("Endnotes"), + Some("Bibliography"), + Some("References"), + Some("Works Cited"), + Some("Index"), + Some("Author's Note"), + Some("Notes to Chapter 1"), + ] { + for prefix in ["", "p. 12. ", "The author explains in English: "] { + let source = format!("{prefix}{SOURCE}"); + assert!( + source_copy_validation_error(&source, &source, heading).is_some(), + "{heading:?}: {prefix}" + ); + let near_copy = format!("{source} Traduzione."); + assert!( + source_copy_validation_error(&source, &near_copy, heading).is_some(), + "{heading:?}: {prefix}" + ); + } + } + } + + #[test] + fn reference_context_still_exempts_short_citations() { + let citation = "Example Author, A Published Title, p. 12."; + for heading in ["Notes", "Endnotes", "Bibliography", "Works Cited", "Index"] { + assert!( + source_copy_validation_error(citation, citation, Some(heading)).is_none(), + "{heading}" + ); + } + // Outside reference context the short line is still reported; batch + // acceptance then grades it by severity. + assert!(source_copy_validation_error(citation, citation, Some("Chapter 1")).is_some()); + let page = "p. 28. Brick by brick."; + assert!(source_copy_validation_error(page, page, None).is_none()); } /// Unspaced prose of a length the detector is meant to cover. `words` @@ -1426,19 +1469,29 @@ mod tests { } #[test] - fn allows_page_references_without_section_metadata() { + fn page_reference_prefix_does_not_exempt_untranslated_commentary() { let reference = "p. 28. Brick by brick, the fireplaces. For an overview of how this works, \ see Example Author, 'A Long Article Title That Should Remain in Its Published Language', \ Journal of Example Studies 41 (2016): 425-452."; - assert!(source_copy_validation_error(reference, reference, None).is_none()); + assert!(source_copy_validation_error(reference, reference, None).is_some()); + // Translating the commentary while keeping the cited title passes. + let translated = "p. 28. Mattone dopo mattone, i camini. Per una panoramica di come \ + funziona, si veda Example Author, 'A Long Article Title That Should Remain in Its \ + Published Language', Journal of Example Studies 41 (2016): 425-452."; + assert!(source_copy_validation_error(reference, translated, None).is_none()); } #[test] - fn allows_explicit_bilingual_glosses() { + fn bilingual_gloss_does_not_exempt_an_untranslated_paragraph() { let gloss = "“todo lugar é são paulo—em todo lugar resistência—resista brasil—a turquia \ está ao seu lado!” Or in English: “the whole world is São Paulo—resistance everywhere—\ Brazil, resist!—Turkey is by your side.”"; - assert!(source_copy_validation_error(gloss, gloss, Some("Chapter 8")).is_none()); + assert!(source_copy_validation_error(gloss, gloss, Some("Chapter 8")).is_some()); + // Keeping the Portuguese quotation while translating the English gloss passes. + let translated = "“todo lugar é são paulo—em todo lugar resistência—resista brasil—a \ + turquia está ao seu lado!” O in italiano: “il mondo intero è San Paolo—resistenza \ + ovunque—Brasile, resisti!—la Turchia è al tuo fianco.”"; + assert!(source_copy_validation_error(gloss, translated, Some("Chapter 8")).is_none()); } #[test] diff --git a/crates/bookforge-store/src/db/tests.rs b/crates/bookforge-store/src/db/tests.rs index 2f06f35..93113f5 100644 --- a/crates/bookforge-store/src/db/tests.rs +++ b/crates/bookforge-store/src/db/tests.rs @@ -6983,3 +6983,123 @@ fn summary_retries_include_all_statuses_without_crossing_jobs() { } assert!(store.summary("missing_job").unwrap().is_none()); } + +#[test] +fn revalidation_preserves_checkpoint_rows_and_does_not_override_newer_or_manual_results() { + let db_path = temp_path("revalidation.sqlite"); + let (store, job, _) = + build_seeded_store_with_translation(&db_path, "revalidation_ns", &["b_000000"]); + let evidence = || { + let conn = store.conn.borrow(); + [ + "SELECT * FROM translations WHERE job_id = ?1", + "SELECT * FROM translation_blocks WHERE job_id = ?1", + "SELECT attempts, tokens_input, tokens_input_cached, tokens_output, \ + tokens_estimated, translated_hash FROM segments WHERE job_id = ?1", + ] + .iter() + .map(|sql| { + let mut stmt = conn.prepare(sql).unwrap(); + let columns = stmt.column_count(); + stmt.query_map(params![job.id], |row| { + (0..columns) + .map(|i| row.get::<_, rusqlite::types::Value>(i)) + .collect::>>() + }) + .unwrap() + .collect::>>() + .unwrap() + }) + .collect::>() + }; + store + .record_segment_engine_findings( + &job.id, + "seg_a", + &[ + EngineFinding::new(QaFindingKind::SourceCopyUnchanged, "title unchanged") + .with_block_id("b_000000") + .with_severity(QaFindingSeverity::Warning), + ], + ) + .unwrap(); + let copy = [EngineFinding::new( + QaFindingKind::SourceCopyUnchanged, + "translation is unchanged from the source-language prose", + ) + .with_block_id("b_000000") + .with_severity(QaFindingSeverity::Error)]; + let error = "saved translation failed source-copy revalidation"; + let before = evidence(); + + assert!( + !store + .mark_stored_translation_for_review(&job.id, "seg_a", "stale output", error, ©) + .unwrap(), + "a stale reader must not reclassify newer output" + ); + assert!( + store + .mark_stored_translation_for_review(&job.id, "seg_a", "Tradotto", error, ©) + .unwrap() + ); + assert_eq!( + evidence(), + before, + "only review status, reason and findings may change" + ); + assert!( + !store + .mark_stored_translation_for_review(&job.id, "seg_a", "Tradotto", error, ©) + .unwrap(), + "reclassification happens once" + ); + let record = &store.segment_records(&job.id).unwrap()[0]; + assert_eq!( + (record.status.as_str(), record.error.as_deref()), + ("needs_review", Some(error)) + ); + assert!(store.resumable_segment_ids(&job.id).unwrap().is_empty()); + let findings = store.segment_qa_findings(&job.id).unwrap(); + let mut kept = findings + .iter() + .map(|finding| (finding.severity.as_str(), finding.message.as_str())) + .collect::>(); + kept.sort_unstable(); + assert_eq!( + kept, + [ + ( + "error", + "translation is unchanged from the source-language prose" + ), + ("warning", "title unchanged"), + ], + "revalidation appends to, never replaces, the segment's findings" + ); + + store + .save_manual_correction(SaveManualCorrection { + job_id: &job.id, + segment_id: "seg_a", + translated_text: "Tradotto", + blocks: &[BlockTranslation { + block_id: BlockId("b_000000".into()), + text: "Tradotto".into(), + }], + }) + .unwrap(); + let accepted = evidence(); + assert!( + !store + .mark_stored_translation_for_review(&job.id, "seg_a", "Tradotto", error, ©) + .unwrap(), + "human acceptance is authoritative" + ); + assert_eq!(evidence(), accepted); + assert_eq!( + store.segment_records(&job.id).unwrap()[0].status, + "succeeded" + ); + let _ = fs::remove_file(db_path); +} diff --git a/crates/bookforge-store/src/db/translations.rs b/crates/bookforge-store/src/db/translations.rs index 3f78204..e3fd624 100644 --- a/crates/bookforge-store/src/db/translations.rs +++ b/crates/bookforge-store/src/db/translations.rs @@ -272,6 +272,65 @@ impl JobStore { Ok(()) } + /// Move an accepted provider result to `needs_review` after a stricter + /// validation rule rejects it, appending `findings` to the segment's + /// existing ones. Saved text, block rows, attempts, usage and provenance + /// are untouched. The expected-text and human-correction guards stop a + /// stale reader from overriding a newer correction. Returns whether the + /// segment was reclassified. + pub fn mark_stored_translation_for_review( + &self, + job_id: &str, + segment_id: &str, + expected_text: &str, + error: &str, + findings: &[EngineFinding], + ) -> Result { + let mut conn = self.conn.borrow_mut(); + let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; + let changed = tx.execute( + &format!( + "UPDATE segments SET status = '{}', error = ?3 + WHERE job_id = ?1 AND id = ?2 AND status IN ({}) + AND EXISTS (SELECT 1 FROM translations t + WHERE t.job_id = ?1 AND t.segment_id = ?2 + AND t.human_corrected = 0 + AND t.translated_text = ?4)", + SegmentStatus::NeedsReview.as_db_text(), + SegmentStatus::sql_set(&[SegmentStatus::Succeeded, SegmentStatus::SkippedCached]) + ), + params![job_id, segment_id, error, expected_text], + )? != 0; + if changed { + for (index, finding) in findings.iter().enumerate() { + let hash = stable_hash(&format!( + "{job_id}\u{1f}revalidation\u{1f}{segment_id}\u{1f}{index}" + )); + // Findings are instrumentation, as in the checkpoint paths. + let _ = insert_qa_finding_row( + &tx, + NewQaFindingRow { + id: &format!("qaf_{}", &hash[..24]), + segment_id, + job_id, + severity: finding.severity.as_str(), + kind: finding.kind.as_str(), + message: &finding.message, + block_id: finding.block_id.as_deref(), + }, + ); + } + touch_job_unless_status_on( + &tx, + job_id, + JobStatus::NeedsReview, + &[JobStatus::Paused, JobStatus::Stopped], + )?; + } + tx.commit()?; + Ok(changed) + } + /// Save a cache-hit translation. Like the other model-write paths, this /// is one `IMMEDIATE` transaction that yields to frozen human corrections. pub fn save_cached_translation(&self, request: SaveCachedTranslation<'_>) -> Result<()> {