From 18301d138667fc8cab6670b370c307c7e9ecda91 Mon Sep 17 00:00:00 2001 From: Acture Date: Sun, 4 Oct 2026 00:46:11 +0800 Subject: [PATCH 1/3] fix: a replaced archive is graded from its new bytes (P-868) `archive::expand` skipped any entry whose output file already existed, so a student who re-uploaded `bob_hw.zip` under the same name was graded on the first upload's files, and an entry dropped from the new archive stayed in the extraction directory, where the scan picked it up as a student of its own. The Canvas loader shares `expand` and had the same fault. `expand` now clears its target before opening the archive, so the directory holds exactly what the archive yields now. A replacement that will not open grades nothing rather than its predecessor's files. The local target is named by the archive's file name instead of its stem. Clearing a stem-named directory would let `hw.zip` and `hw.tar` destroy each other's extraction, and the stem of `...zip` is `..`, which would delete the submissions directory itself. A record graded before this change lists `.scriptmark_extracted//` paths, so `rescore` refuses it as changed; grade again. --- crates/scriptmark/src/archive.rs | 130 ++++++++++++++++++++-- crates/scriptmark/src/canvas/bundle.rs | 46 ++++++++ crates/scriptmark/src/discovery.rs | 142 +++++++++++++++++++++++-- crates/scriptmark/src/models/result.rs | 2 +- crates/scriptmark/tests/record.rs | 57 +++++++--- 5 files changed, 344 insertions(+), 33 deletions(-) diff --git a/crates/scriptmark/src/archive.rs b/crates/scriptmark/src/archive.rs index 0470807..cde243b 100644 --- a/crates/scriptmark/src/archive.rs +++ b/crates/scriptmark/src/archive.rs @@ -273,8 +273,6 @@ impl<'a> Sink<'a> { self.total_bytes += size; self.file_count += 1; self.claimed.insert(out_path.clone(), name.to_string()); - // Provenance is recorded whether or not the bytes are written this run, so a cached - // extraction still traces every file back to the entry it came from. self.extracted.push(ExtractedFile { out_path: out_path.clone(), archive: self.archive.to_path_buf(), @@ -294,11 +292,8 @@ impl<'a> Sink<'a> { self.file_count -= 1; } - /// Write an accepted entry, unless it is already there from an earlier run. + /// Write an accepted entry. fn write(&mut self, name: &str, out_path: &Path, size: u64, read: &mut dyn Read) { - if out_path.exists() { - return; - } let mut buf = Vec::new(); let failure = match read.read_to_end(&mut buf) { Err(_) => Some("unreadable entry".to_string()), @@ -319,8 +314,10 @@ fn unreadable(archive: &Path, reason: String) -> InputDiagnostic { /// Expand `archive` into `target`, writing only the entries `wanted` accepts. /// -/// Entries are flattened onto their base names. Already-extracted files are not rewritten, -/// but their provenance is recorded again, so a cached extraction still traces back. +/// Entries are flattened onto their base names. `target` belongs to this archive alone and +/// is rebuilt on every call: whatever an earlier expansion left there is removed first, so +/// it holds exactly what the archive yields now. A student who re-uploads under the same +/// name is graded on the new bytes, and an entry they dropped does not linger to be graded. pub(crate) fn expand( archive: &Path, target: &Path, @@ -332,10 +329,19 @@ pub(crate) fn expand( }; let before = diagnostics.len(); - if let Err(e) = std::fs::create_dir_all(target) { + // Cleared before the archive is opened, so a replacement that will not open grades + // nothing rather than its predecessor's files. + let fresh = match std::fs::remove_dir_all(target) { + Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(e), + _ => std::fs::create_dir_all(target), + }; + if let Err(e) = fresh { diagnostics.push(unreadable( archive, - format!("cannot create extraction directory: {e}"), + format!( + "cannot prepare extraction directory {}: {e}", + target.display() + ), )); return Vec::new(); } @@ -798,4 +804,108 @@ mod tests { DiagnosticKind::ArchiveEmpty { .. } )); } + + fn listing(dir: &Path) -> Vec { + let mut names: Vec = std::fs::read_dir(dir) + .unwrap() + .map(|e| e.unwrap().file_name().into_string().unwrap()) + .collect(); + names.sort(); + names + } + + /// P-868: a student re-uploads under the same name. Same entry name, same size — nothing + /// short of the bytes tells the two apart, and the bytes are what gets graded. + #[test] + fn test_a_replaced_archive_is_expanded_from_its_new_bytes() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out"); + let mut diagnostics = Vec::new(); + let archive = zip_with(dir.path(), "hw.zip", &[("src/lab5.py", b"x=1")]); + expand(&archive, &target, &is_gradeable, &mut diagnostics); + + zip_with(dir.path(), "hw.zip", &[("src/lab5.py", b"x=2")]); + let files = expand(&archive, &target, &is_gradeable, &mut diagnostics); + + assert_eq!(files.len(), 1); + assert_eq!(std::fs::read(&files[0].out_path).unwrap(), b"x=2"); + assert!(diagnostics.is_empty(), "got {diagnostics:?}"); + } + + /// The extraction directory is listed, not just the entries `expand` returns, so a file + /// the new archive no longer holds must not be left there to be graded. + #[test] + fn test_an_entry_dropped_or_renamed_in_the_new_archive_leaves_nothing_behind() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out"); + let mut diagnostics = Vec::new(); + let archive = zip_with( + dir.path(), + "hw.zip", + &[("lab5.py", b"x=1"), ("util.py", b"y=1"), ("old.py", b"z=1")], + ); + expand(&archive, &target, &is_gradeable, &mut diagnostics); + assert_eq!(listing(&target), ["lab5.py", "old.py", "util.py"]); + + // util.py is gone; old.py is now new.py. + let archive = zip_with( + dir.path(), + "hw.zip", + &[("lab5.py", b"x=1"), ("new.py", b"z=1")], + ); + let files = expand(&archive, &target, &is_gradeable, &mut diagnostics); + + assert_eq!(listing(&target), ["lab5.py", "new.py"]); + let entries: Vec<&str> = files.iter().map(|f| f.entry.as_str()).collect(); + assert_eq!(entries, ["lab5.py", "new.py"]); + } + + #[test] + fn test_an_unchanged_archive_expands_to_the_same_files_every_time() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out"); + let archive = zip_with( + dir.path(), + "hw.zip", + &[("src/lab5.py", b"x=1"), ("util.py", b"y=1")], + ); + let expanded = || { + let mut diagnostics = Vec::new(); + let files: Vec<(PathBuf, String, Vec)> = + expand(&archive, &target, &is_gradeable, &mut diagnostics) + .into_iter() + .map(|f| { + let bytes = std::fs::read(&f.out_path).unwrap(); + (f.out_path, f.entry, bytes) + }) + .collect(); + assert!(diagnostics.is_empty(), "got {diagnostics:?}"); + files + }; + + let first = expanded(); + assert_eq!(first.len(), 2); + assert_eq!(expanded(), first); + } + + /// A replacement that cannot be opened is reported, and grades nothing — least of all + /// the files its predecessor left. + #[test] + fn test_an_unreadable_replacement_leaves_nothing_of_the_old_extraction() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("out"); + let archive = zip_with(dir.path(), "hw.zip", &[("lab5.py", b"x=1")]); + expand(&archive, &target, &is_gradeable, &mut Vec::new()); + + std::fs::write(&archive, b"not a zip").unwrap(); + let mut diagnostics = Vec::new(); + let files = expand(&archive, &target, &is_gradeable, &mut diagnostics); + + assert!(files.is_empty()); + assert!(matches!( + &diagnostics[..], + [d] if matches!(d.kind, DiagnosticKind::ArchiveUnreadable { .. }) + )); + assert!(listing(&target).is_empty()); + } } diff --git a/crates/scriptmark/src/canvas/bundle.rs b/crates/scriptmark/src/canvas/bundle.rs index 1ed367d..4651097 100644 --- a/crates/scriptmark/src/canvas/bundle.rs +++ b/crates/scriptmark/src/canvas/bundle.rs @@ -482,4 +482,50 @@ mod tests { assert_eq!(entry.expanded[0].entry, "src/Lab5.py"); assert!(entry.expanded[0].path.is_file()); } + + /// P-868: a re-fetch replaces the attachment's bytes at the same path — a download + /// whose size did not match, or a hand-repaired bundle. The next load expands the new + /// archive, not what the old one left beside it. + #[test] + fn test_a_replaced_zip_attachment_is_expanded_from_its_new_bytes() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + let zip_path = root.join("attachments/3/work.zip"); + std::fs::create_dir_all(zip_path.parent().unwrap()).unwrap(); + std::fs::write(root.join(PAYLOAD_FILE), "{}").unwrap(); + let store = |entries: &[(&str, &str)]| { + use std::io::Write as _; + let mut zip = zip::ZipWriter::new(std::fs::File::create(&zip_path).unwrap()); + for (name, body) in entries { + zip.start_file(*name, zip::write::SimpleFileOptions::default()) + .unwrap(); + zip.write_all(body.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + let manifest: Manifest = Manifest::from([( + "3".to_string(), + AttachmentRecord::Stored { + path: zip_path.clone(), + size: std::fs::metadata(&zip_path).unwrap().len(), + }, + )]); + std::fs::write( + root.join(MANIFEST_FILE), + serde_json::to_string(&manifest).unwrap(), + ) + .unwrap(); + }; + store(&[("src/Lab5.py", "wrong"), ("src/util.py", "u")]); + load(root).unwrap(); + + store(&[("src/Lab5.py", "right")]); + let (_, downloads, diagnostics) = load(root).unwrap(); + + assert!(diagnostics.is_empty(), "got {diagnostics:?}"); + let entry = downloads.get(&3).unwrap().as_ref().unwrap(); + assert_eq!(entry.expanded.len(), 1); + let lab5 = &entry.expanded[0].path; + assert_eq!(std::fs::read_to_string(lab5).unwrap(), "right"); + assert!(!lab5.with_file_name("util.py").exists()); + } } diff --git a/crates/scriptmark/src/discovery.rs b/crates/scriptmark/src/discovery.rs index 3ffe805..b1b72d4 100644 --- a/crates/scriptmark/src/discovery.rs +++ b/crates/scriptmark/src/discovery.rs @@ -55,7 +55,10 @@ fn extract_sid(filename: &str) -> Option { Some(sid) } -/// Expand every archive in a directory into `{EXTRACT_DIR}/{archive_stem}/`. +/// Expand every archive in a directory into `{EXTRACT_DIR}/{archive file name}/`. +/// +/// Named by the whole file name, not the stem: each expansion clears its directory, so +/// `hw.zip` and `hw.tar` must not share one, and the stem of `...zip` is `..`. fn extract_archives( dir: &Path, diagnostics: &mut Vec, @@ -90,11 +93,8 @@ fn extract_archives( archives.sort(); for archive in archives { - let stem = archive - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or("unknown"); - let target = extract_root.join(stem); + let name = archive.file_name().expect("a directory entry has a name"); + let target = extract_root.join(name); extracted.extend(crate::archive::expand( &archive, &target, @@ -178,7 +178,7 @@ pub fn load_local_input( .map(|p| p.as_ref().to_path_buf()) .chain(extra_dirs) .collect(); - // Walk nested submissions; never revisit the extraction cache or directory symlinks. + // Walk nested submissions; never revisit the extraction directory or directory symlinks. let mut index = 0; while index < all_paths.len() { let dir = &all_paths[index]; @@ -269,7 +269,7 @@ pub fn load_local_input( .and_then(|e| e.to_str()) .unwrap_or("") .to_lowercase(); - // Inside an extraction directory the archive stem carries the key; the files + // A file out of an archive takes its key from the archive's name; the files // within it are named by the student. let default_key = if let Some(FileOrigin::Archive { archive, .. }) = origins.get(&path) { @@ -543,16 +543,140 @@ mod tests { } ); - // A second run reuses the extraction and must still report where the file came from. + // A second run extracts again and must report the same file, from the same place. let again = scan(dir.path()); let bob_again = again .students .iter() .find(|s| s.key().raw() == "bob") .expect("bob"); + assert_eq!(bob_again.files()[0].path, bob.files()[0].path); assert_eq!(bob_again.files()[0].origin, bob.files()[0].origin); } + fn zip_at(path: &Path, entries: &[(&str, &str)]) { + use std::io::Write as _; + let mut zip = zip::ZipWriter::new(std::fs::File::create(path).unwrap()); + for (entry, body) in entries { + zip.start_file(*entry, zip::write::SimpleFileOptions::default()) + .unwrap(); + zip.write_all(body.as_bytes()).unwrap(); + } + zip.finish().unwrap(); + } + + fn bodies(student: &StudentSubmission) -> Vec<(String, String)> { + student + .files() + .iter() + .map(|f| (f.file_name(), std::fs::read_to_string(&f.path).unwrap())) + .collect() + } + + /// P-868: the student replaces `bob_hw.zip`, fixing `sum.py` and renaming `util.py`. + /// The scan grades the new bytes, and the old `util.py` does not linger in the + /// extraction to be picked up as a submission of its own. + #[test] + fn test_a_replaced_archive_is_scanned_from_its_new_contents() { + let dir = tempfile::tempdir().unwrap(); + let zip_path = dir.path().join("bob_hw.zip"); + zip_at(&zip_path, &[("sum.py", "wrong"), ("util.py", "u")]); + let first = scan(dir.path()); + assert_eq!( + bodies(&first.students[0]), + [ + ("sum.py".into(), "wrong".into()), + ("util.py".into(), "u".into()) + ] + ); + + zip_at(&zip_path, &[("sum.py", "right"), ("helpers.py", "u")]); + let input = scan(dir.path()); + + let keys: Vec = input.students.iter().map(|s| s.key().raw()).collect(); + assert_eq!(keys, ["bob"]); + assert_eq!( + bodies(&input.students[0]), + [ + ("helpers.py".into(), "u".into()), + ("sum.py".into(), "right".into()) + ] + ); + assert!(input.unmatched.is_empty(), "got {:?}", input.unmatched); + } + + /// `bob_hw.zip` and `bob_hw.tar` share a stem. Each archive gets its own extraction, so + /// refreshing one never deletes or relabels the other's files. + #[test] + fn test_archives_sharing_a_stem_keep_separate_extractions() { + let dir = tempfile::tempdir().unwrap(); + let zip_path = dir.path().join("bob_hw.zip"); + zip_at(&zip_path, &[("lab.py", "from zip")]); + let tar_path = dir.path().join("bob_hw.tar"); + { + let mut tar = tar::Builder::new(std::fs::File::create(&tar_path).unwrap()); + let mut header = tar::Header::new_gnu(); + header.set_size(8); + header.set_mode(0o644); + header.set_cksum(); + tar.append_data(&mut header, "lab.py", &b"from tar"[..]) + .unwrap(); + tar.finish().unwrap(); + } + + for _ in 0..2 { + let input = scan(dir.path()); + let bob = &input.students[0]; + let mut got: Vec<(PathBuf, String)> = bob + .files() + .iter() + .map(|f| match &f.origin { + FileOrigin::Archive { archive, .. } => { + (archive.clone(), std::fs::read_to_string(&f.path).unwrap()) + } + other => panic!("not from an archive: {other:?}"), + }) + .collect(); + got.sort(); + assert_eq!( + got, + [ + (tar_path.clone(), "from tar".to_string()), + (zip_path.clone(), "from zip".to_string()) + ] + ); + } + } + + /// The stem of `...zip` is `..` and of `..zip` is `.`, so an extraction directory named + /// after the stem would be the scanned directory itself, or every archive's extraction + /// at once. Refreshing that would delete the submissions. + #[test] + fn test_extraction_stays_inside_its_own_directory_whatever_the_archive_is_called() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("alice_sum.py"), "a").unwrap(); + zip_at(&dir.path().join("bob_hw.zip"), &[("sum.py", "b")]); + zip_at(&dir.path().join("...zip"), &[("x.py", "x")]); + zip_at(&dir.path().join("..zip"), &[("y.py", "y")]); + + for _ in 0..2 { + let input = scan(dir.path()); + let bob = input + .students + .iter() + .find(|s| s.key().raw() == "bob") + .expect("bob"); + assert_eq!(bodies(bob), [("sum.py".into(), "b".into())]); + } + assert_eq!( + std::fs::read_to_string(dir.path().join("alice_sum.py")).unwrap(), + "a" + ); + // Nothing was extracted beside the submissions. + assert!(!dir.path().join("x.py").exists()); + assert!(!dir.path().join("y.py").exists()); + } + /// Characterisation test for the zip-bomb guards, written before P-670 lifts this loop /// into a shared `expand_archive` that Canvas attachments also feed. An oversized entry /// must be reported and skipped *without* taking its archive-mates down with it — the diff --git a/crates/scriptmark/src/models/result.rs b/crates/scriptmark/src/models/result.rs index 28fca74..8dd7b8d 100644 --- a/crates/scriptmark/src/models/result.rs +++ b/crates/scriptmark/src/models/result.rs @@ -301,7 +301,7 @@ pub struct SubmissionVersion { /// Every runnable file of the attempt, by path. pub files: Vec, /// The archives those files were extracted from: a replaced archive is a changed - /// submission even while an earlier extraction still sits on disk. + /// submission even when the files graded out of it are byte-identical. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub archives: Vec, } diff --git a/crates/scriptmark/tests/record.rs b/crates/scriptmark/tests/record.rs index 793e3ef..b6cc39f 100644 --- a/crates/scriptmark/tests/record.rs +++ b/crates/scriptmark/tests/record.rs @@ -682,20 +682,27 @@ fn the_assignment_name_is_a_label_so_declaring_items_later_rescores() { assert_eq!(grades(dir, None), graded(100.0, 0.0)); } +/// bob hands in `bob_hw.zip` instead of `bob_sum.py`. +fn bob_zips(dir: &Path, entries: &[(&str, &str)]) { + std::fs::remove_file(dir.join("submissions/bob_sum.py")).ok(); + let file = std::fs::File::create(dir.join("submissions/bob_hw.zip")).unwrap(); + let mut zip = zip::ZipWriter::new(file); + for (name, content) in entries { + zip.start_file(*name, zip::write::SimpleFileOptions::default()) + .unwrap(); + std::io::Write::write_all(&mut zip, content.as_bytes()).unwrap(); + } + zip.finish().unwrap(); +} + +const WRONG: &str = "def add(a, b):\n return abs(a) + abs(b)\n"; +const RIGHT: &str = "def add(a, b):\n return a + b\n"; + #[test] -fn a_replaced_archive_is_a_changed_submission_even_behind_a_stale_extraction() { +fn a_replaced_archive_is_a_changed_submission() { let temp = bench(); let dir = temp.path(); - let zip_of = |content: &str| { - std::fs::remove_file(dir.join("submissions/bob_sum.py")).ok(); - let file = std::fs::File::create(dir.join("submissions/bob_hw.zip")).unwrap(); - let mut zip = zip::ZipWriter::new(file); - zip.start_file("sum.py", zip::write::SimpleFileOptions::default()) - .unwrap(); - std::io::Write::write_all(&mut zip, content.as_bytes()).unwrap(); - zip.finish().unwrap(); - }; - zip_of("def add(a, b):\n return abs(a) + abs(b)\n"); + bob_zips(dir, &[("sum.py", WRONG)]); succeeded(scriptmark(dir, &GRADE)); let bob = record(dir) .evidence @@ -705,12 +712,36 @@ fn a_replaced_archive_is_a_changed_submission_even_behind_a_stale_extraction() { .unwrap(); assert_eq!(bob.submission.unwrap().archives.len(), 1); - // The extraction from the first archive stays on disk; the archive itself changed. - zip_of("def add(a, b):\n return a + b\n"); + bob_zips(dir, &[("sum.py", RIGHT)]); all_or_nothing(dir); refused(rescore(dir, &[]), "local:bob's submitted files changed"); } +/// P-868, as reported: bob fixes `sum.py` and re-uploads `bob_hw.zip` under the same name, +/// dropping a helper on the way. Grading again grades the new archive — and only it. +#[test] +fn a_replaced_archive_is_graded_from_its_new_contents() { + let temp = bench(); + let dir = temp.path(); + bob_zips(dir, &[("sum.py", WRONG), ("util.py", "")]); + succeeded(scriptmark(dir, &GRADE)); + assert_eq!(grades(dir, None), graded(100.0, 50.0)); + + bob_zips(dir, &[("sum.py", RIGHT)]); + succeeded(scriptmark(dir, &GRADE)); + // Exactly two students: the dropped util.py does not come back as a student "util". + assert_eq!(grades(dir, None), graded(100.0, 100.0)); + let bob = record(dir) + .evidence + .students + .into_iter() + .find(|r| r.student_id == "local:bob") + .unwrap(); + let files = bob.submission.unwrap().files; + assert_eq!(files.len(), 1); + assert_eq!(std::fs::read_to_string(&files[0].path).unwrap(), RIGHT); +} + /// A Canvas bundle as `canvas fetch` leaves one, for course 7, assignment 8. Ada tried /// twice — wrong, then right — Ben once, wrongly, and Cy, who handed nothing in, is excused. fn canvas_bundle(dir: &Path) { From c0b77cbbe32ccf507e3b11a932eb6ab44c300c16 Mon Sep 17 00:00:00 2001 From: Acture Date: Sun, 4 Oct 2026 00:50:34 +0800 Subject: [PATCH 2/3] fix: a failed write leaves no partial file in an extraction (P-868) The scan lists the extraction directory, so a truncated file left by a failed write (disk full) was graded as a student named after the file. Review follow-ups: the extraction root sits inside the scanned directory, not beside it; the odd-name test also pins where each archive lands. --- crates/scriptmark/src/archive.rs | 2 ++ crates/scriptmark/src/discovery.rs | 7 +++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/scriptmark/src/archive.rs b/crates/scriptmark/src/archive.rs index cde243b..559c342 100644 --- a/crates/scriptmark/src/archive.rs +++ b/crates/scriptmark/src/archive.rs @@ -300,6 +300,8 @@ impl<'a> Sink<'a> { Ok(_) => std::fs::write(out_path, &buf).err().map(|e| e.to_string()), }; if let Some(reason) = failure { + // A failed write can leave a truncated file, and the scan lists this directory. + std::fs::remove_file(out_path).ok(); self.rollback(name, out_path, size, reason); } } diff --git a/crates/scriptmark/src/discovery.rs b/crates/scriptmark/src/discovery.rs index b1b72d4..5280d2e 100644 --- a/crates/scriptmark/src/discovery.rs +++ b/crates/scriptmark/src/discovery.rs @@ -17,7 +17,7 @@ use crate::models::{ }; use crate::roster::Roster; -/// Directory archives are expanded into, beside the directory being scanned. +/// Directory archives are expanded into, inside the directory being scanned. const EXTRACT_DIR: &str = ".scriptmark_extracted"; /// Map file extensions to language identifiers. @@ -672,7 +672,10 @@ mod tests { std::fs::read_to_string(dir.path().join("alice_sum.py")).unwrap(), "a" ); - // Nothing was extracted beside the submissions. + // Each archive landed in its own directory, and nothing beside the submissions. + let extracted = dir.path().join(EXTRACT_DIR); + assert!(extracted.join("...zip/x.py").is_file()); + assert!(extracted.join("..zip/y.py").is_file()); assert!(!dir.path().join("x.py").exists()); assert!(!dir.path().join("y.py").exists()); } From 763e80553f5ff9e517b808685ac913ddc694e947 Mon Sep 17 00:00:00 2001 From: Acture Date: Sun, 4 Oct 2026 00:51:24 +0800 Subject: [PATCH 3/3] docs: link published P-868 notes --- notes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes b/notes index b64db70..f7bba71 160000 --- a/notes +++ b/notes @@ -1 +1 @@ -Subproject commit b64db70d71a2b08b50bc636244da07bcd80266dc +Subproject commit f7bba717190efbe26fe55b8bffe89981f588d7a8