diff --git a/src/artifacts.rs b/src/artifacts.rs index d4266acb..d9223588 100644 --- a/src/artifacts.rs +++ b/src/artifacts.rs @@ -887,6 +887,48 @@ impl ArtifactRunWriter { self.run.path() } + /// Reads one manifested artifact while sibling invocations may still own + /// scratch directories. The caller must retain exclusive access to this + /// writer throughout the read; no manifest update or append can interleave. + /// Run-wide checkpoint/resume/finalization boundaries still use their full + /// tree audits and require every scratch and poisoned append to be resolved. + pub(crate) fn read_authenticated(&self, relative: &Path, max_bytes: usize) -> Result> { + self.run_lock.verify(&self.run)?; + let result = (|| -> Result> { + let relative = validate_artifact_relative_path(relative)?; + if self + .outstanding_scratches + .keys() + .any(|scratch| artifact_path_starts_with(&relative, scratch)) + { + bail!("artifact read path overlaps an outstanding scratch directory"); + } + if self.poisoned_appends.contains(&relative) { + bail!("artifact read path is poisoned by an unrecovered prior append"); + } + let record = self + .files + .get(&relative) + .context("artifact read path is not registered in the writer manifest")?; + let max_bytes = u64::try_from(max_bytes).context("artifact read limit overflow")?; + if record.bytes > max_bytes { + bail!("artifact file exceeds its configured {max_bytes} byte limit"); + } + + // Preserve the selected path's privacy and directory/lock identity + // checks without auditing unrelated, actively written scratch trees. + verify_writer_evidence(&self.writer_evidence, &self.run_root, &self.run)?; + let before = verify_private_artifact_read_path(&self.run, &relative)?; + let contents = read_and_verify_record_with_limit(&self.run, record, max_bytes)?; + if verify_private_artifact_read_path(&self.run, &relative)? != before { + bail!("artifact file identity changed during authenticated read"); + } + verify_writer_evidence(&self.writer_evidence, &self.run_root, &self.run)?; + Ok(contents) + })(); + finish_with_artifact_lock_verification(result, self.run_lock.verify(&self.run)) + } + /// Reserves one owner-private, child-writable scratch directory directly /// beneath this run. Only a canonical single-component name is accepted so /// cleanup can remain handle-relative and identity-bound. A launcher must diff --git a/src/artifacts/part2.rs b/src/artifacts/part2.rs index a1ddce96..759cb9b4 100644 --- a/src/artifacts/part2.rs +++ b/src/artifacts/part2.rs @@ -180,10 +180,37 @@ fn verify_manifest_contents<'a>( } fn read_and_verify_record(run: &SafeRoot, record: &ArtifactFileRecord) -> Result> { + read_and_verify_record_with_limit(run, record, MAX_ARTIFACT_FILE_BYTES) +} + +fn verify_private_artifact_read_path(run: &SafeRoot, relative: &Path) -> Result { + ensure_private_directory(run.path())?; + let mut directory = run.path().to_path_buf(); + if let Some(parent) = relative.parent() { + for component in parent.components() { + directory.push(component); + ensure_private_directory(&directory)?; + ensure_same_device( + &fs::symlink_metadata(&directory)?, + run.identity().device, + &directory, + )?; + } + } + let path = run.path().join(relative); + let identity = ensure_private_regular_file(&path)?; + ensure_same_device(&fs::symlink_metadata(&path)?, run.identity().device, &path)?; + Ok(identity) +} + +fn read_and_verify_record_with_limit( + run: &SafeRoot, + record: &ArtifactFileRecord, + max_bytes: u64, +) -> Result> { let (_parent, _file_name) = artifact_parent_and_name(run, &record.path, false)?; ensure_private_regular_file(&run.path().join(&record.path))?; - let contents = - BoundedRegularReader::read_relative(run.path(), &record.path, MAX_ARTIFACT_FILE_BYTES)?; + let contents = BoundedRegularReader::read_relative(run.path(), &record.path, max_bytes)?; if u64::try_from(contents.len()).unwrap_or(u64::MAX) != record.bytes || sha256_hex(&contents) != record.sha256 { diff --git a/src/artifacts/tests.rs b/src/artifacts/tests.rs index 3df8da93..b4bf6bf9 100644 --- a/src/artifacts/tests.rs +++ b/src/artifacts/tests.rs @@ -11,6 +11,101 @@ fn sha256_matches_standard_vector() { ); } +#[test] +fn authenticated_file_read_refuses_its_own_poisoned_append_without_requiring_run_quiescence() { + let (_temp, repo) = committed_repo(); + let mut writer = ArtifactRunWriter::reserve( + &repo, + RunArtifactFamily::Supervise, + RunId::new("authenticated-read-poison").unwrap(), + "supervise", + ) + .unwrap(); + let transcript = Path::new("logs/child-c.jsonl"); + let sibling = Path::new("logs/child-a.jsonl"); + for path in [transcript, sibling] { + writer + .write_bytes( + path, + b"completed\n", + ArtifactFileDisposition::PrivateEvidence, + ) + .unwrap(); + } + // Even if disk bytes happen to match the old manifest, an unrecovered + // append must not be treated as durable, authenticated evidence. + writer.poisoned_appends.insert(transcript.to_path_buf()); + let error = writer.read_authenticated(transcript, 1024).unwrap_err(); + assert!(error.to_string().contains("poisoned")); + assert!(writer.resume_binding().is_err()); + + writer.poisoned_appends.remove(transcript); + writer.poisoned_appends.insert(sibling.to_path_buf()); + assert_eq!( + writer.read_authenticated(transcript, 1024).unwrap(), + b"completed\n" + ); + assert!(writer.resume_binding().is_err()); + let run = writer.run_dir().to_path_buf(); + let error = writer.finalize(transcript, false).unwrap_err(); + assert!(error.to_string().contains("poisoned append")); + assert!(!run.join(FINALIZATION_MARKER).exists()); +} + +#[cfg(unix)] +#[test] +fn authenticated_file_read_preserves_private_path_and_writer_lock_custody() { + use std::os::unix::fs::symlink; + + let (_temp, repo) = committed_repo(); + let mut writer = ArtifactRunWriter::reserve( + &repo, + RunArtifactFamily::Supervise, + RunId::new("authenticated-read-custody").unwrap(), + "supervise", + ) + .unwrap(); + let relative = Path::new("logs/child-c.jsonl"); + writer + .write_bytes( + relative, + b"completed\n", + ArtifactFileDisposition::PrivateEvidence, + ) + .unwrap(); + let path = writer.run_dir().join(relative); + let parent = path.parent().unwrap(); + fs::set_permissions(parent, fs::Permissions::from_mode(0o755)).unwrap(); + assert!(writer + .read_authenticated(relative, 1024) + .unwrap_err() + .to_string() + .contains("owner-private")); + fs::set_permissions(parent, fs::Permissions::from_mode(0o700)).unwrap(); + fs::set_permissions(&path, fs::Permissions::from_mode(0o644)).unwrap(); + assert!(writer + .read_authenticated(relative, 1024) + .unwrap_err() + .to_string() + .contains("owner-private")); + fs::set_permissions(&path, fs::Permissions::from_mode(0o600)).unwrap(); + let saved = parent.join("saved.jsonl"); + fs::rename(&path, &saved).unwrap(); + symlink(&saved, &path).unwrap(); + assert!(writer.read_authenticated(relative, 1024).is_err()); + fs::remove_file(&path).unwrap(); + fs::rename(saved, &path).unwrap(); + assert_eq!( + writer.read_authenticated(relative, 1024).unwrap(), + b"completed\n" + ); + + let lock_path = writer.run_dir().join(RUN_LOCK_FILE); + fs::rename(&lock_path, writer.run_dir().join("saved.lock")).unwrap(); + fs::write(lock_path, b"replacement").unwrap(); + assert!(writer.read_authenticated(relative, 1024).is_err()); +} + #[test] fn supervisor_invocation_scratch_names_require_the_full_canonical_grammar() { let valid = [ diff --git a/src/supervise/assignment_execution.rs b/src/supervise/assignment_execution.rs index decaaa2d..a017ab25 100644 --- a/src/supervise/assignment_execution.rs +++ b/src/supervise/assignment_execution.rs @@ -269,25 +269,13 @@ pub(super) fn read_authenticated_review_transcript( { bail!("review transcript artifact path is not canonical relative form"); } - let before = writer - .resume_binding() - .context("failed to authenticate retained transcript artifact manifest before read")?; - let transcript_path = writer.run_dir().join(transcript_relative); // Authenticate and decode the complete retained evidence under the supervisor-report cap. // Only the later review-lens request is excerpted to its smaller, independent request limit. - let bytes = read_bounded_regular_file_nofollow(&transcript_path, MAX_SUPERVISOR_REPORT_BYTES) - .with_context(|| { - format!( - "failed to read retained child transcript {}", - transcript_path.display() - ) - })?; - let after = writer - .resume_binding() - .context("failed to authenticate retained transcript artifact manifest after read")?; - if before != after { - bail!("retained transcript artifact manifest changed while it was read"); - } + // The shared writer remains locked by the caller. Authenticate the exact bytes + // before allowing review to read them, independently of sibling scratch state. + let bytes = writer + .read_authenticated(transcript_relative, MAX_SUPERVISOR_REPORT_BYTES) + .context("failed to authenticate retained transcript artifact manifest before read")?; String::from_utf8(bytes).context("child transcript evidence is not valid UTF-8") } diff --git a/src/supervise/tests/plan_runtime.rs b/src/supervise/tests/plan_runtime.rs index d1325627..e07ff4e2 100644 --- a/src/supervise/tests/plan_runtime.rs +++ b/src/supervise/tests/plan_runtime.rs @@ -5239,6 +5239,32 @@ fn stacked_review_lenses_execute_every_configured_boundary_and_aggregate() { assert!(report.review_lens_usage.iter().all(|usage| { usage.observation == RoleUsageObservation::ProcessObserved && usage.usage.is_some() })); + let lens_usage = report + .review_lens_usage + .iter() + .map(|row| (row.lens_id.as_str(), row.usage)) + .collect::>(); + assert_eq!( + lens_usage, + BTreeMap::from([ + ( + "diff-security", + Some(Usage { + input_tokens: 100, + output_tokens: 20, + total_tokens: 120, + }), + ), + ( + "report-consistency", + Some(Usage { + input_tokens: 200, + output_tokens: 40, + total_tokens: 240, + }), + ), + ]) + ); assert_eq!( report.review_lens_total_usage, Some(Usage { diff --git a/src/supervise/tests/run_artifacts.rs b/src/supervise/tests/run_artifacts.rs index 19d6564b..3b8c3317 100644 --- a/src/supervise/tests/run_artifacts.rs +++ b/src/supervise/tests/run_artifacts.rs @@ -5142,6 +5142,58 @@ fn retained_review_transcript_refuses_input_above_supervisor_report_limit() { ))); } +#[test] +fn retained_review_transcript_authenticates_while_sibling_invocation_scratch_is_open() { + let (_temp, repo_path) = injected_repository(); + let mut writer = ArtifactRunWriter::reserve( + &repo_path, + RunArtifactFamily::Supervise, + RunId::new("review-transcript-live-sibling").unwrap(), + "supervise-test", + ) + .expect("reserve shared artifact writer"); + let transcript_relative = Path::new("logs/child-c.jsonl"); + let transcript = "completed child-c transcript\n"; + writer + .write_bytes( + transcript_relative, + transcript.as_bytes(), + ArtifactFileDisposition::PrivateEvidence, + ) + .expect("retain child-c transcript"); + let mut journal = None; + let mut autonomy_kpis = AutonomyKpiCollector::default(); + let artifacts = Mutex::new(SharedSupervisorArtifacts { + writer: &mut writer, + journal: &mut journal, + autonomy_kpis: &mut autonomy_kpis, + checkpoint: None, + }); + let sibling_scratch = with_supervisor_artifacts(&artifacts, |writer, _| { + writer.create_supervisor_invocation_scratch_dir("capture-assignment-0000-attempt-01") + }) + .expect("hold child-a invocation scratch open"); + fs::write(sibling_scratch.path().join("stdout.jsonl"), b"in flight") + .expect("sibling is still producing evidence"); + let authenticated = with_supervisor_artifacts(&artifacts, |writer, _| { + assert!( + writer.resume_binding().is_err(), + "checkpoint must still refuse live scratch" + ); + read_authenticated_review_transcript(writer, transcript_relative) + }); + with_supervisor_artifacts(&artifacts, |writer, _| { + writer.discard_scratch(&sibling_scratch)?; + writer.resume_binding()?; + Ok(()) + }) + .expect("quiescent writer becomes resumable"); + assert_eq!( + authenticated.expect("parent review authenticates child-c independently"), + transcript + ); +} + #[test] fn retained_review_transcript_manifest_authentication_refuses_tampering() { let (_temp, repo_path) = injected_repository(); @@ -5168,14 +5220,35 @@ fn retained_review_transcript_manifest_authentication_refuses_tampering() { transcript ); + for tampered in [b"truncated".to_vec(), vec![b'f'; transcript.len()]] { + fs::write(writer.run_dir().join(transcript_relative), tampered) + .expect("tamper transcript fixture"); + let error = read_authenticated_review_transcript(&mut writer, transcript_relative) + .expect_err("length or hash mismatch must be refused before review reads it"); + assert!(error + .to_string() + .contains("failed to authenticate retained transcript artifact manifest before read")); + assert!(format!("{error:#}").contains("digest/length does not match its manifest")); + } + + let unregistered = Path::new("logs/forged.jsonl"); + fs::write(writer.run_dir().join(unregistered), b"forged transcript") + .expect("write unregistered transcript"); + let error = read_authenticated_review_transcript(&mut writer, unregistered) + .expect_err("unregistered transcript must be refused before read"); + assert!(error.to_string().contains("before read")); + assert!(format!("{error:#}").contains("not registered")); + + let scratch = writer.create_scratch_dir("still-writing").unwrap(); fs::write( - writer.run_dir().join(transcript_relative), - b"tampered transcript", + scratch.path().join("stdout.jsonl"), + b"unfinished transcript", ) - .expect("tamper transcript fixture"); - let error = read_authenticated_review_transcript(&mut writer, transcript_relative) - .expect_err("tampered transcript must be refused"); - assert!(error - .to_string() - .contains("failed to authenticate retained transcript artifact manifest before read")); + .unwrap(); + let error = + read_authenticated_review_transcript(&mut writer, Path::new("still-writing/stdout.jsonl")) + .expect_err("live scratch transcript must be refused before read"); + assert!(error.to_string().contains("before read")); + assert!(format!("{error:#}").contains("outstanding scratch")); + writer.discard_scratch(&scratch).unwrap(); } diff --git a/src/supervise/tests/scheduler.rs b/src/supervise/tests/scheduler.rs index a40d8ea1..db8e6f5f 100644 --- a/src/supervise/tests/scheduler.rs +++ b/src/supervise/tests/scheduler.rs @@ -901,7 +901,9 @@ fn semantic_block_claims_follow_actual_dispatch_order_with_overlap_scan_ahead() skip_without_containment!(); #[derive(Default)] struct BlockState { + child_a_started: bool, child_c_started: bool, + child_c_parent_review_started: bool, } let (temp, repo_path) = injected_repository(); @@ -927,7 +929,9 @@ fn semantic_block_claims_follow_actual_dispatch_order_with_overlap_scan_ahead() let runner = { let assignments = assignments.clone(); let state = Arc::clone(&state); - move |command: &ExternalAgentCommand| { + move |command: &ExternalAgentCommand, + cancellation: &ProcessCancellation, + _review_runtime: Option>| { let id = injected_command_assignment_id(command); if let Some(assignment) = assignments .iter() @@ -939,6 +943,16 @@ fn semantic_block_claims_follow_actual_dispatch_order_with_overlap_scan_ahead() &command.output_last_message, &injected_auditor_report(assignment, &child), ); + if assignment.id == "child-c" { + let (lock, condvar) = &*state; + let mut block = lock.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + assert!( + block.child_a_started, + "sibling invocation scratch must be live" + ); + block.child_c_parent_review_started = true; + condvar.notify_all(); + } return injected_verified_run(command); } let (lock, condvar) = &*state; @@ -946,12 +960,37 @@ fn semantic_block_claims_follow_actual_dispatch_order_with_overlap_scan_ahead() if id == "child-c" { block.child_c_started = true; condvar.notify_all(); + // C may reach its runner before A's thread is scheduled. Do + // not let it complete until A owns live invocation scratches. + block = wait_for_fixture_state( + condvar, + block, + |block| block.child_a_started, + "semantic_block_claims_follow_actual_dispatch_order_with_overlap_scan_ahead", + "semantic-block-dispatch-order", + "child-a holds invocation scratch", + Duration::from_secs(120), + ); } else if id == "child-a" { - while !block.child_c_started { - block = condvar - .wait(block) + block.child_a_started = true; + condvar.notify_all(); + // Keep A's invocation scratch live until C has authenticated its + // completed transcript and entered parent review. Cancellation + // also releases A if authentication fails, so the old behavior + // produces a failed report rather than a deadlocked fixture. + let deadline = Instant::now() + Duration::from_secs(120); + while !block.child_c_parent_review_started && !cancellation.is_cancelled() { + if Instant::now() >= deadline { + if !block.child_c_started { + panic!("child-c runner did not start"); + } + panic!("child-c parent review did not start"); + } + (block, _) = condvar + .wait_timeout(block, Duration::from_millis(25)) .unwrap_or_else(|poisoned| poisoned.into_inner()); } + assert!(block.child_c_started || cancellation.is_cancelled()); } drop(block); let assignment = assignments @@ -967,7 +1006,7 @@ fn semantic_block_claims_follow_actual_dispatch_order_with_overlap_scan_ahead() #[cfg(target_os = "linux")] let _selected_fixture = selected_concurrent_injected_fixture(&plan, &options, &temp) .expect("hold explicit selected concurrent fixture"); - run_supervisor_plan_with_concurrent_runner( + run_supervisor_plan_with_concurrent_cancellable_runner( plan, SupervisorConsultantPlan::default(), options, @@ -977,6 +1016,7 @@ fn semantic_block_claims_follow_actual_dispatch_order_with_overlap_scan_ahead() } .expect("run deterministic semantic Block scheduling"); assert!(report.success, "unexpected failed report: {report:#?}"); + assert!(state.0.lock().unwrap().child_c_parent_review_started); assert_eq!( report .released_semantic_intents