diff --git a/.gitignore b/.gitignore index bdea6d5..c467443 100644 --- a/.gitignore +++ b/.gitignore @@ -179,3 +179,11 @@ templates .remember/ tests/geec/ tests_submission/ + +# P-669 test fixtures — the broad rules above (*.csv, submissions, templates, config.toml) +# must not swallow committed test data. +!crates/scriptmark/tests/fixtures/** +# ...but not what running the suite over them produces. +crates/scriptmark/tests/fixtures/**/__pycache__/ +crates/scriptmark/tests/fixtures/**/.scriptmark_extracted/ +crates/scriptmark/tests/fixtures/**/.DS_Store diff --git a/Cargo.toml b/Cargo.toml index a02419b..d661cb7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,7 @@ members = [ ] [workspace.package] -version = "0.2.0" +version = "0.3.0" edition = "2024" license = "GPL-3.0-or-later" authors = ["Acture "] diff --git a/README.md b/README.md index 389fd92..a8a62b4 100644 --- a/README.md +++ b/README.md @@ -67,8 +67,16 @@ results = scriptmark.grade(["submissions/"], "tests/") for r in results: print(f"{r.student_id}: {r.grade:.1f} ({r.passed}/{r.total})") -# Discover student files -subs = scriptmark.discover(["submissions/"]) # {'alice': ['path/to/alice_lab5.py'], ...} +# Discover student files (convenience view — drops non-submitters and orphan files) +# Keys are rendered student keys: a bare 学号 once a roster confirms it, otherwise +# `local:` — the prefix means nothing has vouched for that filename token yet. +subs = scriptmark.discover(["submissions/"]) # {'local:alice': ['path/to/alice_lab5.py'], ...} + +# The full input model: every student keeps an outcome, nothing is dropped +inp = scriptmark.load_input(["submissions/"], roster="roster.csv") +for s in inp["students"]: + print(s["identity"]["key"], s["state"]) # not_submitted | submitted_empty | executable +print(inp["unmatched"], inp["diagnostics"]) # Load and inspect a spec spec = scriptmark.load_spec("tests/test_lab5.toml") diff --git a/crates/scriptmark-py/src/lib.rs b/crates/scriptmark-py/src/lib.rs index 85710e3..82a9a47 100644 --- a/crates/scriptmark-py/src/lib.rs +++ b/crates/scriptmark-py/src/lib.rs @@ -4,9 +4,9 @@ use std::path::Path; use pyo3::prelude::*; use pyo3::types::PyDict; -use scriptmark::discovery::discover_submissions; +use scriptmark::discovery::{LocalInputOptions, load_local_input}; use scriptmark::grading::apply_grading; -use scriptmark::models::{StudentReport, TestSpec}; +use scriptmark::models::{AssignmentInput, StudentReport, TestSpec}; use scriptmark::runner::orchestrator::run_all; use scriptmark::runner::python::PythonExecutor; use scriptmark::spec_loader::load_specs_from_dir; @@ -118,25 +118,65 @@ impl PyStudentResult { /// Discover student submission files in the given directories. /// /// Returns a dict mapping student IDs to lists of file paths. +/// +/// This is a lossy convenience view: a dict keyed on student id cannot represent duplicate +/// identities, files with no identifiable owner, or roster members who did not submit. Use +/// `load_input()` when any of those matter. #[pyfunction] fn discover(paths: Vec) -> PyResult>> { - let path_refs: Vec<&Path> = paths.iter().map(|p| Path::new(p.as_str())).collect(); - let subs = discover_submissions(&path_refs, None) - .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; - - Ok(subs - .by_student - .into_iter() - .map(|(sid, files)| { - let paths = files - .into_iter() + let input = local_input(&paths)?; + + Ok(input + .students + .iter() + .filter(|s| !s.files().is_empty()) + .map(|student| { + let files = student + .files() + .iter() .map(|f| f.path.to_string_lossy().to_string()) .collect(); - (sid, paths) + (student.identity.key.to_string(), files) }) .collect()) } +/// Load the full unified input model as a dict. +/// +/// Unlike `discover()`, nothing is dropped: every student carries an outcome +/// (`executable`, `submitted_empty`, `received_unmatched`, `not_submitted`), unattributable +/// files appear under `unmatched`, and anomalies appear under `diagnostics`. +#[pyfunction] +#[pyo3(signature = (paths, *, roster=None))] +fn load_input(py: Python<'_>, paths: Vec, roster: Option) -> PyResult { + let roster = match roster { + Some(path) => Some( + scriptmark::roster::load_roster(Path::new(&path)) + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?, + ), + None => None, + }; + let path_refs: Vec<&Path> = paths.iter().map(|p| Path::new(p.as_str())).collect(); + let input = load_local_input( + &path_refs, + LocalInputOptions { + roster: roster.as_ref(), + ..Default::default() + }, + ) + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; + + let json_val = serde_json::to_value(&input) + .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; + json_to_py(py, &json_val) +} + +fn local_input(paths: &[String]) -> PyResult { + let path_refs: Vec<&Path> = paths.iter().map(|p| Path::new(p.as_str())).collect(); + load_local_input(&path_refs, LocalInputOptions::default()) + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string())) +} + /// Load a test specification from a TOML file. #[pyfunction] fn load_spec(path: String) -> PyResult { @@ -147,7 +187,7 @@ fn load_spec(path: String) -> PyResult { Ok(PyTestSpec { inner: spec }) } -/// Run tests for all students, returning raw results as dicts. +/// Run tests for all students, returning a list of raw result dicts. #[pyfunction] #[pyo3(signature = (submissions, tests, *, timeout=10, python="python3"))] fn run( @@ -175,7 +215,7 @@ fn grade( python: &str, policy: &str, ) -> PyResult> { - let results = run_grading(&submissions, &tests, timeout, python)?; + let mut reports = run_grading(&submissions, &tests, timeout, python)?; // Apply grading policy let grading_policy = @@ -184,7 +224,6 @@ fn grade( lower: 60.0, upper: 100.0, }); - let mut reports: Vec = results.into_values().collect(); apply_grading(&mut reports, &grading_policy); reports.sort_by(|a, b| a.student_id.cmp(&b.student_id)); @@ -200,10 +239,8 @@ fn run_grading( tests: &str, timeout: u64, python: &str, -) -> PyResult> { - let path_refs: Vec<&Path> = submissions.iter().map(|p| Path::new(p.as_str())).collect(); - let subs = discover_submissions(&path_refs, None) - .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; +) -> PyResult> { + let input = local_input(submissions)?; let specs = load_specs_from_dir(Path::new(tests)) .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; @@ -214,8 +251,7 @@ fn run_grading( let rt = tokio::runtime::Runtime::new() .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; - let results = rt.block_on(run_all(&subs, &specs, &executor, timeout, None)); - Ok(results) + Ok(rt.block_on(run_all(&input.students, &specs, &executor, timeout, None))) } /// Convert serde_json::Value to a Python object. @@ -255,6 +291,7 @@ fn _scriptmark(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_function(wrap_pyfunction!(discover, m)?)?; + m.add_function(wrap_pyfunction!(load_input, m)?)?; m.add_function(wrap_pyfunction!(load_spec, m)?)?; m.add_function(wrap_pyfunction!(run, m)?)?; m.add_function(wrap_pyfunction!(grade, m)?)?; diff --git a/crates/scriptmark/src/db/mod.rs b/crates/scriptmark/src/db/mod.rs index 618ada7..d55ee60 100644 --- a/crates/scriptmark/src/db/mod.rs +++ b/crates/scriptmark/src/db/mod.rs @@ -19,6 +19,8 @@ pub enum DbError { Json(#[from] serde_json::Error), #[error("IO error: {0}")] Io(#[from] std::io::Error), + #[error("two reports share student id '{0}'; refusing to overwrite one with the other")] + DuplicateStudent(String), } pub struct Database { @@ -52,9 +54,8 @@ impl Database { #[cfg(test)] mod tests { - use std::collections::HashMap; - use crate::models::*; + use crate::roster::Roster; use crate::similarity::SimilarityPair; use super::*; @@ -68,9 +69,7 @@ mod tests { #[test] fn test_roster_import_and_query() { let db = Database::open_memory().unwrap(); - let mut roster = HashMap::new(); - roster.insert("alice".to_string(), "Alice Smith".to_string()); - roster.insert("bob".to_string(), "Bob Jones".to_string()); + let roster = Roster::from_pairs(&[("alice", "Alice Smith"), ("bob", "Bob Jones")]); let count = db.import_roster(&roster).unwrap(); assert_eq!(count, 2); @@ -90,7 +89,7 @@ mod tests { student_id: "alice".to_string(), student_name: Some("Alice".to_string()), test_results: vec![TestResult { - spec_name: "test".to_string(), + item_id: "test".to_string(), cases: vec![CaseResult { case_name: "case1".to_string(), status: TestStatus::Passed, @@ -101,8 +100,7 @@ mod tests { }], }], final_grade: Some(95.0), - backend_name: None, - lint_score: None, + ..Default::default() }]; let session_id = db.save_session("hw5", &reports, None).unwrap(); @@ -116,7 +114,7 @@ mod tests { let results = db.get_results(session_id).unwrap(); assert_eq!(results.len(), 1); assert_eq!(results[0].student_id, "alice"); - assert!((results[0].final_grade - 95.0).abs() < 0.1); + assert_eq!(results[0].final_grade, Some(95.0)); } #[test] @@ -125,19 +123,13 @@ mod tests { let report1 = vec![StudentReport { student_id: "alice".to_string(), - student_name: None, - test_results: vec![], final_grade: Some(80.0), - backend_name: None, - lint_score: None, + ..Default::default() }]; let report2 = vec![StudentReport { student_id: "alice".to_string(), - student_name: None, - test_results: vec![], final_grade: Some(95.0), - backend_name: None, - lint_score: None, + ..Default::default() }]; db.save_session("hw5", &report1, None).unwrap(); @@ -171,16 +163,190 @@ mod tests { #[test] fn test_roster_upsert() { let db = Database::open_memory().unwrap(); - let mut roster = HashMap::new(); - roster.insert("alice".to_string(), "Alice V1".to_string()); - db.import_roster(&roster).unwrap(); - - roster.insert("alice".to_string(), "Alice V2".to_string()); - db.import_roster(&roster).unwrap(); + db.import_roster(&Roster::from_pairs(&[("alice", "Alice V1")])) + .unwrap(); + db.import_roster(&Roster::from_pairs(&[("alice", "Alice V2")])) + .unwrap(); let alice = db.get_student("alice").unwrap().unwrap(); assert_eq!(alice.name.as_deref(), Some("Alice V2")); assert_eq!(db.list_students().unwrap().len(), 1); } + + #[test] + fn test_import_roster_counts_rows_stored() { + let db = Database::open_memory().unwrap(); + // A repeated row is merged before it ever reaches the database. + let roster = Roster::from_pairs(&[("alice", "Alice"), ("alice", "Alice")]); + assert_eq!(roster.len(), 1); + assert_eq!(db.import_roster(&roster).unwrap(), 1); + } + + #[test] + fn test_duplicate_student_ids_are_refused_rather_than_merged() { + let db = Database::open_memory().unwrap(); + let reports = vec![ + StudentReport { + student_id: "alice".to_string(), + final_grade: Some(80.0), + ..Default::default() + }, + StudentReport { + student_id: "alice".to_string(), + final_grade: Some(95.0), + ..Default::default() + }, + ]; + + let err = db.save_session("hw5", &reports, None).unwrap_err(); + assert!(matches!(err, DbError::DuplicateStudent(id) if id == "alice")); + } + + #[test] + fn test_ungraded_students_read_back_as_none_not_zero() { + let db = Database::open_memory().unwrap(); + let reports = vec![StudentReport { + student_id: "absent".to_string(), + submission_state: Some(SubmissionOutcome::NotSubmitted), + ..Default::default() + }]; + let session_id = db.save_session("hw5", &reports, None).unwrap(); + + // A student who was never graded must not come back as a zero. + assert_eq!(db.get_results(session_id).unwrap()[0].final_grade, None); + assert_eq!( + db.get_student_history("absent").unwrap()[0].1.final_grade, + None + ); + } + + #[test] + fn test_an_unconfirmed_key_still_joins_to_its_roster_row() { + let db = Database::open_memory().unwrap(); + // A run made without --roster renders ids with a `local:` prefix; the roster + // imported afterwards holds the bare token. Both must resolve to one student — + // including a non-numeric one, which a character-set trim would have mangled. + db.import_roster(&Roster::from_pairs(&[("alice", "Alice Smith")])) + .unwrap(); + let reports = vec![StudentReport { + student_id: "local:alice".to_string(), + final_grade: Some(88.0), + ..Default::default() + }]; + let session_id = db.save_session("hw5", &reports, None).unwrap(); + + let results = db.get_results(session_id).unwrap(); + assert_eq!(results[0].student_name.as_deref(), Some("Alice Smith")); + + let history = db.get_student_history("alice").unwrap(); + assert_eq!(history.len(), 1); + assert_eq!(history[0].1.student_name.as_deref(), Some("Alice Smith")); + } + + #[test] + fn test_a_result_row_never_picks_up_a_second_students_name() { + let db = Database::open_memory().unwrap(); + // Both forms present in the students table: the join must resolve to exactly one. + db.import_roster(&Roster::from_pairs(&[("alice", "Alice Smith")])) + .unwrap(); + db.conn + .execute( + "INSERT INTO students (id, name) VALUES ('local:alice', 'Someone Else')", + [], + ) + .unwrap(); + + let reports = vec![StudentReport { + student_id: "local:alice".to_string(), + final_grade: Some(88.0), + ..Default::default() + }]; + let session_id = db.save_session("hw5", &reports, None).unwrap(); + + let results = db.get_results(session_id).unwrap(); + assert_eq!(results.len(), 1, "one stored result must yield one row"); + assert_eq!(results[0].student_name.as_deref(), Some("Alice Smith")); + } + + #[test] + fn test_history_accepts_the_id_form_the_tables_print() { + let db = Database::open_memory().unwrap(); + db.import_roster(&Roster::from_pairs(&[("alice", "Alice Smith")])) + .unwrap(); + db.save_session( + "hw5", + &[StudentReport { + student_id: "local:alice".to_string(), + final_grade: Some(70.0), + ..Default::default() + }], + None, + ) + .unwrap(); + + // Whichever form the teacher copies out of the summary must find the run. + for id in ["alice", "local:alice"] { + let history = db.get_student_history(id).unwrap(); + assert_eq!(history.len(), 1, "no history for '{id}'"); + assert_eq!(history[0].1.student_name.as_deref(), Some("Alice Smith")); + assert_eq!(db.get_student_name(id), "Alice Smith"); + } + } + + #[test] + fn test_a_csv_import_does_not_erase_a_stored_canvas_id() { + let db = Database::open_memory().unwrap(); + // First a Canvas-sourced import, which knows the Canvas id... + let mut from_canvas = Roster::from_pairs(&[("alice", "Alice")]); + from_canvas.entries[0].canvas_user_id = Some(4242); + db.import_roster(&from_canvas).unwrap(); + + // ...then a CSV, which never carries one. It must not null the id out, or grade + // push loses the only thing it can key on. + db.import_roster(&Roster::from_pairs(&[("alice", "Alice Wu")])) + .unwrap(); + + let stored = db.get_student("alice").unwrap().unwrap(); + assert_eq!(stored.name.as_deref(), Some("Alice Wu")); + assert_eq!(stored.canvas_id, Some(4242)); + } + + #[test] + fn test_an_errored_report_is_not_graded() { + let mut reports = vec![StudentReport { + student_id: "alice".to_string(), + submission_state: Some(SubmissionOutcome::Executable), + error: Some("grading task failed: panicked".to_string()), + ..Default::default() + }]; + assert!(!reports[0].is_gradeable()); + + crate::grading::apply_grading(&mut reports, &GradingPolicy::default()); + // An infrastructure failure must not become a defensible-looking number. + assert_eq!(reports[0].final_grade, None); + } + + #[test] + fn test_average_ignores_ungraded_students() { + let db = Database::open_memory().unwrap(); + let reports = vec![ + StudentReport { + student_id: "alice".to_string(), + final_grade: Some(90.0), + ..Default::default() + }, + StudentReport { + student_id: "absent".to_string(), + submission_state: Some(SubmissionOutcome::NotSubmitted), + ..Default::default() + }, + ]; + + db.save_session("hw5", &reports, None).unwrap(); + let sessions = db.list_sessions().unwrap(); + assert_eq!(sessions[0].student_count, 2); + // A missing grade is not a zero, so it must not halve the mean. + assert!((sessions[0].avg_grade - 90.0).abs() < 0.1); + } } diff --git a/crates/scriptmark/src/db/results.rs b/crates/scriptmark/src/db/results.rs index a664630..3dcb536 100644 --- a/crates/scriptmark/src/db/results.rs +++ b/crates/scriptmark/src/db/results.rs @@ -20,7 +20,8 @@ pub struct ResultRow { pub student_id: String, pub student_name: Option, pub pass_rate: f64, - pub final_grade: f64, + /// `None` when the student has no grade — which is not the same as a zero. + pub final_grade: Option, pub lint_score: Option, pub total_cases: i64, pub passed_cases: i64, @@ -34,10 +35,23 @@ impl Database { reports: &[StudentReport], grading_policy_json: Option<&str>, ) -> Result { - let avg = if reports.is_empty() { + // Two reports for one student would be merged by UNIQUE(session_id, student_id) + // while student_count still claimed both — the silent overwrite this model exists + // to prevent. Refuse before writing anything. + let mut seen = std::collections::BTreeSet::new(); + for report in reports { + if !seen.insert(report.student_id.as_str()) { + return Err(DbError::DuplicateStudent(report.student_id.clone())); + } + } + + // Average over students who actually have a grade: ungraded students would + // otherwise drag the mean toward zero. + let graded: Vec = reports.iter().filter_map(|r| r.final_grade).collect(); + let avg = if graded.is_empty() { 0.0 } else { - reports.iter().filter_map(|r| r.final_grade).sum::() / reports.len() as f64 + graded.iter().sum::() / graded.len() as f64 }; self.conn.execute( @@ -48,7 +62,7 @@ impl Database { let session_id = self.conn.last_insert_rowid(); let mut stmt = self.conn.prepare( - "INSERT OR REPLACE INTO results + "INSERT INTO results (session_id, student_id, pass_rate, final_grade, lint_score, total_cases, passed_cases, details) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)", )?; @@ -95,7 +109,15 @@ impl Database { let mut stmt = self.conn.prepare( "SELECT r.student_id, s.name, r.pass_rate, r.final_grade, r.lint_score, r.total_cases, r.passed_cases FROM results r - LEFT JOIN students s ON r.student_id = s.id + -- A run made without --roster leaves keys unconfirmed, so the id carries a + -- `local:` prefix that students.id never does. Match either form. (substr, + -- not ltrim: ltrim strips a character set, so 'local:alice' would become + -- 'ice'.) + LEFT JOIN students s + ON s.id = CASE + WHEN r.student_id LIKE 'local:%' THEN substr(r.student_id, 7) + ELSE r.student_id + END WHERE r.session_id = ?1 ORDER BY r.final_grade DESC", )?; @@ -104,7 +126,7 @@ impl Database { student_id: row.get(0)?, student_name: row.get(1)?, pass_rate: row.get::<_, f64>(2).unwrap_or(0.0), - final_grade: row.get::<_, f64>(3).unwrap_or(0.0), + final_grade: row.get(3)?, lint_score: row.get(4)?, total_cases: row.get::<_, i64>(5).unwrap_or(0), passed_cases: row.get::<_, i64>(6).unwrap_or(0), @@ -137,20 +159,30 @@ impl Database { } /// Get a student's history across all sessions. + /// Accepts the id in whichever form the teacher read off a table: a bare 学号, or the + /// `local:`-prefixed form a run made without a roster prints. pub fn get_student_history( &self, student_id: &str, ) -> Result, DbError> { + let bare = student_id + .strip_prefix("local:") + .unwrap_or(student_id) + .to_string(); let mut stmt = self.conn.prepare( "SELECT s.id, s.assignment, s.spec_title, s.grading_policy, s.student_count, s.avg_grade, s.created_at, r.student_id, st.name, r.pass_rate, r.final_grade, r.lint_score, r.total_cases, r.passed_cases FROM results r JOIN sessions s ON r.session_id = s.id - LEFT JOIN students st ON r.student_id = st.id - WHERE r.student_id = ?1 + LEFT JOIN students st + ON st.id = CASE + WHEN r.student_id LIKE 'local:%' THEN substr(r.student_id, 7) + ELSE r.student_id + END + WHERE r.student_id IN (?1, 'local:' || ?1, ?2) ORDER BY s.created_at DESC", )?; - let rows = stmt.query_map(rusqlite::params![student_id], |row| { + let rows = stmt.query_map(rusqlite::params![bare, student_id], |row| { Ok(( Session { id: row.get(0)?, @@ -165,7 +197,7 @@ impl Database { student_id: row.get(7)?, student_name: row.get(8)?, pass_rate: row.get::<_, f64>(9).unwrap_or(0.0), - final_grade: row.get::<_, f64>(10).unwrap_or(0.0), + final_grade: row.get(10)?, lint_score: row.get(11)?, total_cases: row.get::<_, i64>(12).unwrap_or(0), passed_cases: row.get::<_, i64>(13).unwrap_or(0), diff --git a/crates/scriptmark/src/db/roster.rs b/crates/scriptmark/src/db/roster.rs index a5725ef..d4a84f1 100644 --- a/crates/scriptmark/src/db/roster.rs +++ b/crates/scriptmark/src/db/roster.rs @@ -1,6 +1,5 @@ -use std::collections::HashMap; - use super::{Database, DbError}; +use crate::roster::Roster; /// A student record from the database. #[derive(Debug, Clone)] @@ -12,18 +11,30 @@ pub struct Student { } impl Database { - /// Import a roster (student_id -> name mapping). Upserts. - pub fn import_roster(&self, roster: &HashMap) -> Result { - let mut count = 0; + /// Import a roster. Upserts. + /// + /// Returns the number of rows actually stored. A [`Roster`] holds at most one row per + /// key, so this normally equals `roster.len()`. + pub fn import_roster(&self, roster: &Roster) -> Result { let mut stmt = self.conn.prepare( - "INSERT INTO students (id, name) VALUES (?1, ?2) - ON CONFLICT(id) DO UPDATE SET name = excluded.name", + "INSERT INTO students (id, name, canvas_id) VALUES (?1, ?2, ?3) + ON CONFLICT(id) DO UPDATE SET + name = excluded.name, + -- A CSV roster carries no Canvas id, so writing its NULL would erase one + -- an earlier Canvas import had stored, and grade push would lose it. + canvas_id = COALESCE(excluded.canvas_id, students.canvas_id)", )?; - for (id, name) in roster { - stmt.execute(rusqlite::params![id, name])?; - count += 1; + let mut stored = std::collections::BTreeSet::new(); + for entry in &roster.entries { + let id = entry.key.to_string(); + stmt.execute(rusqlite::params![ + id, + entry.name, + entry.canvas_user_id.map(|id| id as i64), + ])?; + stored.insert(id); } - Ok(count) + Ok(stored.len()) } /// Get a single student by ID. @@ -63,8 +74,10 @@ impl Database { } /// Get a student name, returning "N/A" if not found. + /// + /// Accepts the `local:`-prefixed form too — `students.id` always holds the bare key. pub fn get_student_name(&self, id: &str) -> String { - self.get_student(id) + self.get_student(id.strip_prefix("local:").unwrap_or(id)) .ok() .flatten() .and_then(|s| s.name) diff --git a/crates/scriptmark/src/discovery.rs b/crates/scriptmark/src/discovery.rs index ab7038e..4e5e3ac 100644 --- a/crates/scriptmark/src/discovery.rs +++ b/crates/scriptmark/src/discovery.rs @@ -1,11 +1,30 @@ -use std::collections::HashMap; +//! The local entry point: scan directories of student files and produce an +//! [`AssignmentInput`]. +//! +//! Every file that arrives is accounted for. A file whose owner can be identified but +//! whose type nothing runs becomes an `IgnoredFile` diagnostic against that student; a +//! file with no identifiable owner becomes an [`UnmatchedArtifact`]. Nothing is dropped on +//! the floor, and nothing is printed — anomalies are returned as diagnostics for the CLI +//! to render. + +use std::collections::BTreeMap; use std::io::Read as _; use std::path::{Path, PathBuf}; -use crate::models::{StudentFile, SubmissionSet}; +use crate::models::{ + Assignment, AssignmentInput, AttemptPolicy, DiagnosticKind, FileOrigin, InputDiagnostic, + InputSource, RosterMatch, SourceLocation, StudentFile, StudentIdentity, StudentKey, + StudentSubmission, SubmissionAttempt, UnmatchedArtifact, UnmatchedReason, normalize_key, +}; +use crate::roster::Roster; + +/// Directory archives are expanded into, beside the directory being scanned. +const EXTRACT_DIR: &str = ".scriptmark_extracted"; /// Map file extensions to language identifiers. -fn detect_language(ext: &str) -> Option<&'static str> { +/// +/// Shared with the Canvas adapter, which classifies downloaded attachments the same way. +pub(crate) fn detect_language(ext: &str) -> Option<&'static str> { match ext { "py" => Some("python"), "cpp" | "cc" | "cxx" => Some("cpp"), @@ -19,231 +38,389 @@ fn detect_language(ext: &str) -> Option<&'static str> { } } -/// Extract student ID from a filename. +/// Extract a student key from a filename. /// -/// Convention: `{student_id}_{rest}.ext` (e.g. `alice_Lab5_1.py` → `alice`) +/// Convention: `{key}_{rest}.ext` (e.g. `alice_Lab5_1.py` → `alice`). This split is a +/// placeholder — teacher-configurable matching is P-673 — so the key it yields is only +/// ever an unconfirmed [`crate::models::StudentKey::Extracted`] until a roster vouches +/// for it. fn extract_sid(filename: &str) -> Option { let stem = Path::new(filename).file_stem()?.to_str()?; - let sid = stem.split('_').next()?; + // Normalised here rather than at each use: `seen_keys`, `by_key` and the roster-merge + // coverage set are all keyed on this string, and a token with stray whitespace would + // otherwise group separately from the identity built out of it. + let sid = normalize_key(stem.split('_').next()?); if sid.is_empty() { return None; } - Some(sid.to_string()) + Some(sid) +} + +/// A file that came out of an archive, with the provenance needed to trace it back. +#[derive(Debug, Clone)] +struct ExtractedFile { + out_path: PathBuf, + archive: PathBuf, + /// The path *inside* the archive, before flattening. + entry: String, +} + +const MAX_FILE_SIZE: u64 = 5_000_000; // 5 MB per file +const MAX_TOTAL_SIZE: u64 = 50_000_000; // 50 MB total per archive +const MAX_FILE_COUNT: usize = 100; + +fn is_noise(name: &str) -> bool { + name.starts_with('.') || name.starts_with("__") +} + +fn skipped(archive: &Path, entry: &str, reason: String) -> InputDiagnostic { + InputDiagnostic::warning(DiagnosticKind::ArchiveEntrySkipped { + archive: archive.to_path_buf(), + entry: entry.to_string(), + reason, + }) } -/// Extract .zip archives in a directory to `.scriptmark_extracted/{archive_stem}/`. +/// Extract `.zip` archives in a directory to `{EXTRACT_DIR}/{archive_stem}/`. /// -/// Returns list of directories created. Skips archives that have already been extracted -/// (directory exists and is non-empty). Silently skips corrupt/unreadable archives. -fn extract_archives(dir: &Path) -> Vec { - let extract_root = dir.join(".scriptmark_extracted"); - let mut created = Vec::new(); - - let entries = match std::fs::read_dir(dir) { - Ok(e) => e, - Err(_) => return created, +/// Archives already extracted are not re-extracted, but their index is re-read so that +/// provenance survives a second run — otherwise a cached extraction would leave every file +/// it produced with no traceable origin. +fn extract_archives(dir: &Path, diagnostics: &mut Vec) -> Vec { + let extract_root = dir.join(EXTRACT_DIR); + let mut extracted = Vec::new(); + + let Ok(entries) = std::fs::read_dir(dir) else { + return extracted; }; - for entry in entries.flatten() { - let path = entry.path(); - if !path.is_file() { + // Sort: read_dir order is not stable across filesystems. + let mut archives: Vec = Vec::new(); + for entry in entries { + let Ok(path) = entry.map(|entry| entry.path()) else { continue; - } - let ext = path + }; + let is_zip = path .extension() .and_then(|e| e.to_str()) - .unwrap_or("") - .to_lowercase(); - if ext != "zip" { + .is_some_and(|e| e.eq_ignore_ascii_case("zip")); + if !is_zip { continue; } + match std::fs::metadata(&path) { + Ok(meta) if meta.is_file() => archives.push(path), + Ok(_) => {} + Err(e) => diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::UnreadableDirEntry { + dir: dir.to_path_buf(), + reason: format!("{}: {e}", path.display()), + }, + )), + } + } + archives.sort(); - let stem = path + for archive_path in archives { + let stem = archive_path .file_stem() .and_then(|s| s.to_str()) .unwrap_or("unknown"); let target = extract_root.join(stem); - // Skip if already extracted - if target.is_dir() - && std::fs::read_dir(&target) - .map(|mut d| d.next().is_some()) - .unwrap_or(false) - { - created.push(target); - continue; - } - - // Extract - let file = match std::fs::File::open(&path) { + let file = match std::fs::File::open(&archive_path) { Ok(f) => f, - Err(_) => continue, + Err(e) => { + diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::ArchiveUnreadable { + archive: archive_path.clone(), + reason: e.to_string(), + }, + )); + continue; + } }; let mut archive = match zip::ZipArchive::new(file) { Ok(a) => a, Err(e) => { - eprintln!("[WARN] Skipping corrupt archive {}: {e}", path.display()); + diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::ArchiveUnreadable { + archive: archive_path.clone(), + reason: e.to_string(), + }, + )); continue; } }; if let Err(e) = std::fs::create_dir_all(&target) { - eprintln!("[WARN] Cannot create extract dir {}: {e}", target.display()); + diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::ArchiveUnreadable { + archive: archive_path.clone(), + reason: format!("cannot create extraction directory: {e}"), + }, + )); continue; } - const MAX_FILE_SIZE: u64 = 5_000_000; // 5 MB per file - const MAX_TOTAL_SIZE: u64 = 50_000_000; // 50 MB total per archive - const MAX_FILE_COUNT: usize = 100; - let mut total_bytes: u64 = 0; let mut file_count: usize = 0; + // Flattening can map two in-archive paths onto one output name; remember who got + // there first so the loser is reported rather than silently dropped. + let mut claimed: BTreeMap = BTreeMap::new(); for i in 0..archive.len() { let mut entry = match archive.by_index(i) { Ok(e) => e, - Err(_) => continue, + Err(e) => { + // Unreadable metadata is still something that arrived; reporting it is + // what keeps "nothing is dropped on the floor" true. + diagnostics.push(skipped( + &archive_path, + &format!("entry #{i}"), + e.to_string(), + )); + continue; + } }; - if entry.is_dir() { continue; } - // Check uncompressed size before reading - if entry.size() > MAX_FILE_SIZE { - eprintln!( - "[WARN] Skipping oversized entry in {}: {} ({} bytes)", - path.display(), - entry.name(), - entry.size() - ); + let Some(name) = entry.enclosed_name() else { + // Path traversal attempt. + diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::ArchiveEntrySkipped { + archive: archive_path.clone(), + entry: entry.name().to_string(), + reason: "unsafe path".to_string(), + }, + )); + continue; + }; + let entry_name = name.to_string_lossy().into_owned(); + + let Some(filename) = name.file_name().map(|n| n.to_owned()) else { + continue; + }; + if is_noise(&filename.to_string_lossy()) { + continue; + } + + let out_path = target.join(&filename); + + if let Some(first) = claimed.get(&out_path) { + diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::ArchiveNameCollision { + archive: archive_path.clone(), + entry: format!("{entry_name} (already taken by {first})"), + }, + )); continue; } + // Every guard runs before the entry is recorded. Claiming the name first would + // let a rejected entry block the real submission from ever being extracted, and + // the accounting runs on a cached rerun too so the diagnostics do not vanish + // the second time a directory is scanned. + if entry.size() > MAX_FILE_SIZE { + diagnostics.push(skipped( + &archive_path, + &entry_name, + format!( + "{} bytes exceeds the {MAX_FILE_SIZE} byte limit", + entry.size() + ), + )); + continue; + } if total_bytes + entry.size() > MAX_TOTAL_SIZE { - eprintln!( - "[WARN] Archive {} exceeds total extraction limit ({}B), stopping", - path.display(), - MAX_TOTAL_SIZE - ); + diagnostics.push(skipped( + &archive_path, + &entry_name, + format!("archive exceeds the {MAX_TOTAL_SIZE} byte total"), + )); break; } - if file_count >= MAX_FILE_COUNT { - eprintln!( - "[WARN] Archive {} exceeds file count limit ({}), stopping", - path.display(), - MAX_FILE_COUNT - ); + diagnostics.push(skipped( + &archive_path, + &entry_name, + format!("archive exceeds the {MAX_FILE_COUNT} file limit"), + )); break; } - let name = match entry.enclosed_name() { - Some(n) => n.to_owned(), - None => continue, // skip path traversal attempts - }; - - // Flatten: extract to target/{filename} regardless of subdirectories in archive - let filename = match name.file_name() { - Some(n) => n.to_owned(), - None => continue, - }; - - // Skip __pycache__, .DS_Store, etc. - let fname_str = filename.to_string_lossy(); - if fname_str.starts_with('.') || fname_str.starts_with("__") { - continue; - } - - let out_path = target.join(&filename); + total_bytes += entry.size(); + file_count += 1; + claimed.insert(out_path.clone(), entry_name.clone()); + + // Provenance is recorded whether or not the bytes are written this run, so a + // cached extraction still traces back to its archive entry. + extracted.push(ExtractedFile { + out_path: out_path.clone(), + archive: archive_path.clone(), + entry: entry_name.clone(), + }); + + // Only the bytes are skipped when the file is already there — an entry that + // failed last run is retried, so its diagnostic recurs instead of vanishing on + // the second scan of a directory. if out_path.exists() { - continue; // don't overwrite + continue; } let mut buf = Vec::new(); - if entry.read_to_end(&mut buf).is_ok() { - let _ = std::fs::write(&out_path, &buf); - total_bytes += buf.len() as u64; - file_count += 1; + let failure = match entry.read_to_end(&mut buf) { + Err(_) => Some("unreadable entry".to_string()), + Ok(_) => std::fs::write(&out_path, &buf).err().map(|e| e.to_string()), + }; + if let Some(reason) = failure { + diagnostics.push(skipped(&archive_path, &entry_name, reason)); + // Roll the claim and the provenance back together; letting them drift is + // what lets a rejected entry block a real one. + extracted.pop(); + claimed.remove(&out_path); + total_bytes -= entry.size(); + file_count -= 1; } } - - created.push(target); } - created + extracted } -/// Scan directories for student submission files and group by student ID. +/// How to interpret a set of scanned directories. +pub struct LocalInputOptions<'a> { + pub assignment: Assignment, + /// The roster of record. Without one, membership is unknown rather than negative. + pub roster: Option<&'a Roster>, + pub attempt_policy: AttemptPolicy, +} + +impl Default for LocalInputOptions<'_> { + fn default() -> Self { + Self { + assignment: Assignment::default(), + roster: None, + attempt_policy: AttemptPolicy::Latest, + } + } +} + +/// Scan directories of student submissions and build the unified input. /// -/// Only includes files with recognized language extensions. -/// If `extensions` is provided, only includes files matching those extensions. -pub fn discover_submissions( +/// Local input has no notion of repeated attempts — inferring them from Canvas download +/// filename tokens would be a matching rule, which is P-673 — so every student gets +/// exactly one attempt. +pub fn load_local_input( paths: &[impl AsRef], - extensions: Option<&[&str]>, -) -> Result { - let mut by_student: HashMap> = HashMap::new(); + options: LocalInputOptions<'_>, +) -> Result { + let mut diagnostics: Vec = Vec::new(); + let mut unmatched: Vec = Vec::new(); + // BTreeMap: iteration order must not depend on hashing. + let mut by_key: BTreeMap> = BTreeMap::new(); + // A student who sent only unusable files still submitted something. + let mut seen_keys: std::collections::BTreeSet = Default::default(); + + for path in paths { + let path = path.as_ref(); + if !path.is_dir() { + return Err(DiscoveryError::NotADirectory(path.to_path_buf())); + } + } - // Phase 0: Extract archives in each submission directory + let mut origins: BTreeMap = BTreeMap::new(); let mut extra_dirs: Vec = Vec::new(); - for dir_path in paths { - let extracted = extract_archives(dir_path.as_ref()); - extra_dirs.extend(extracted); + for path in paths { + for extracted in extract_archives(path.as_ref(), &mut diagnostics) { + if let Some(parent) = extracted.out_path.parent() + && !extra_dirs.contains(&parent.to_path_buf()) + { + extra_dirs.push(parent.to_path_buf()); + } + origins.insert( + extracted.out_path, + FileOrigin::Archive { + archive: extracted.archive, + entry: extracted.entry, + }, + ); + } } - // Combine original paths + extracted subdirectories - let all_paths: Vec<&Path> = paths + let all_paths: Vec = paths .iter() - .map(|p| p.as_ref()) - .chain(extra_dirs.iter().map(|p| p.as_path())) + .map(|p| p.as_ref().to_path_buf()) + .chain(extra_dirs) .collect(); for dir_path in &all_paths { - if !dir_path.is_dir() { - return Err(DiscoveryError::NotADirectory(dir_path.to_path_buf())); - } - let entries = std::fs::read_dir(dir_path) - .map_err(|e| DiscoveryError::IoError(dir_path.to_path_buf(), e))?; + .map_err(|e| DiscoveryError::IoError(dir_path.clone(), e))?; + let mut files: Vec = Vec::new(); for entry in entries { - let entry = entry.map_err(|e| DiscoveryError::IoError(dir_path.to_path_buf(), e))?; - let path = entry.path(); - - if !path.is_file() { - continue; + // Not `path().is_file()`: that turns a metadata failure into "not a file", so + // a submission we merely failed to stat would be read as 缺交. `fs::metadata` + // follows symlinks exactly as `is_file()` did, but hands back the error. + // (`DirEntry::metadata` would not — on Unix it is `symlink_metadata`, which + // would silently stop grading symlinked submissions.) + match entry.map(|entry| entry.path()) { + Ok(path) => match std::fs::metadata(&path) { + Ok(meta) if meta.is_file() => files.push(path), + Ok(_) => {} + Err(e) => diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::UnreadableDirEntry { + dir: dir_path.clone(), + reason: format!("{}: {e}", path.display()), + }, + )), + }, + Err(e) => diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::UnreadableDirEntry { + dir: dir_path.clone(), + reason: e.to_string(), + }, + )), } + } + files.sort(); - let ext = match path.extension().and_then(|e| e.to_str()) { - Some(e) => e, - None => continue, - }; + let is_extracted = dir_path.components().any(|c| c.as_os_str() == EXTRACT_DIR); - // Filter by allowed extensions if specified - if let Some(allowed) = extensions - && !allowed.contains(&ext) - { + for path in files { + let Some(filename) = path.file_name().and_then(|n| n.to_str()) else { continue; - } - - let language = match detect_language(ext) { - Some(lang) => lang.to_string(), - None => continue, - }; - - let filename = match path.file_name().and_then(|n| n.to_str()) { - Some(n) => n, - None => continue, }; + if is_noise(filename) { + continue; + } - // For files inside .scriptmark_extracted/, prefer SID from the parent dir - // (which is the archive stem, e.g. "bob_12345_67890_Lab5") - let is_extracted = dir_path - .components() - .any(|c| c.as_os_str() == ".scriptmark_extracted"); + let ext = path + .extension() + .and_then(|e| e.to_str()) + .unwrap_or("") + .to_lowercase(); + // Archives are inputs to extraction, not submissions in their own right — but + // the upload still happened. Registering the owner here is what stops a + // truncated or empty archive being reported as 缺交. + if !is_extracted && ext == "zip" { + match extract_sid(filename) { + Some(key) => { + seen_keys.insert(key); + } + None => unmatched.push(UnmatchedArtifact { + path: path.clone(), + reason: UnmatchedReason::NoStudentKey, + }), + } + continue; + } - let sid = if is_extracted { - // Try parent dir first (archive stem has SID), then file + // Inside an extraction directory the archive stem carries the key; the files + // within it are named by the student. + let key = if is_extracted { dir_path .file_name() .and_then(|n| n.to_str()) @@ -253,24 +430,133 @@ pub fn discover_submissions( extract_sid(filename) }; - let sid = match sid { - Some(sid) => sid, - None => continue, - }; - - by_student - .entry(sid) - .or_default() - .push(StudentFile { path, language }); + let language = detect_language(&ext); + + match (key, language) { + (Some(key), Some(language)) => { + seen_keys.insert(key.clone()); + let origin = origins.get(&path).cloned().unwrap_or(FileOrigin::Direct); + by_key + .entry(key) + .or_default() + .push(StudentFile::direct(path.clone(), language).with_origin(origin)); + } + (Some(key), None) => { + // Owner known, type unusable: the student submitted, just not code. + diagnostics.push( + InputDiagnostic::info(DiagnosticKind::IgnoredFile { + key: key.clone(), + path: path.clone(), + }) + .at(SourceLocation::file(path.clone())), + ); + seen_keys.insert(key); + } + (None, language) => unmatched.push(UnmatchedArtifact { + path: path.clone(), + reason: if language.is_some() { + UnmatchedReason::NoStudentKey + } else { + UnmatchedReason::UnsupportedType + }, + }), + } } } - // Sort files within each student for deterministic ordering - for files in by_student.values_mut() { + let mut students: Vec = Vec::new(); + // Keyed on the value, not its rendering: `Display` prefixes are not escaped, so a 学号 + // that literally reads `canvas:5` would otherwise collide with `CanvasUser(5)`. + let mut covered: std::collections::BTreeSet = Default::default(); + let mut covered_canvas_ids: std::collections::BTreeSet = Default::default(); + + for key in &seen_keys { + let mut files = by_key.remove(key).unwrap_or_default(); files.sort_by(|a, b| a.path.cmp(&b.path)); + + let mut identity = StudentIdentity::extracted(key); + let roster_match = match options.roster { + None => RosterMatch::NoRoster, + Some(roster) => match roster.lookup(&identity.key) { + Some(i) => { + identity.confirm_number(); + identity.name = roster.entries[i].name.clone(); + identity.canvas_user_id = roster.entries[i].canvas_user_id; + covered.insert(identity.key.clone()); + covered_canvas_ids.extend(identity.canvas_user_id); + RosterMatch::Matched(i) + } + None => { + diagnostics.push(InputDiagnostic::warning(DiagnosticKind::NotOnRoster { + key: key.clone(), + })); + RosterMatch::NotInRoster + } + }, + }; + + let attempt = SubmissionAttempt::new(1).with_files(files); + students.push(StudentSubmission::received( + identity, + roster_match, + vec![attempt], + options.attempt_policy, + )); } - Ok(SubmissionSet { by_student }) + // A roster student who sent nothing must still appear — that is the whole point of + // having a roster of record. Duplicate rows for one number yield one student carrying + // every row it matched, not one student per row. + if let Some(roster) = options.roster { + for entry in &roster.entries { + if !covered.insert(entry.key.clone()) { + continue; + } + // A roster that names one person under both a 学号 and a Canvas id must not + // count them twice. + if entry + .canvas_user_id + .is_some_and(|id| covered_canvas_ids.contains(&id)) + { + continue; + } + covered_canvas_ids.extend(entry.canvas_user_id); + let Some(index) = roster.lookup(&entry.key) else { + continue; + }; + let mut identity = match &entry.key { + StudentKey::CanvasUser(id) => StudentIdentity::canvas_user(*id), + key => StudentIdentity::number(key.raw()), + }; + identity.name = entry.name.clone(); + identity.canvas_user_id = identity.canvas_user_id.or(entry.canvas_user_id); + students.push(StudentSubmission::not_submitted(identity, index)); + } + } + + let mut input = AssignmentInput { + assignment: options.assignment, + source: InputSource::Local { + scanned_dirs: paths.iter().map(|p| p.as_ref().to_path_buf()).collect(), + roster_path: options + .roster + .and_then(|r| r.entries.first()) + .and_then(|e| e.location.as_ref()) + .and_then(|l| l.file.clone()), + }, + roster: options.roster.cloned(), + students, + unmatched, + diagnostics, + }; + + if let Some(roster) = options.roster { + input.diagnostics.extend(roster.diagnostics.iter().cloned()); + } + let zero_padded = input.detect_zero_padded_variants(); + input.diagnostics.extend(zero_padded); + + Ok(input.sorted()) } #[derive(Debug, thiserror::Error)] @@ -284,6 +570,22 @@ pub enum DiscoveryError { #[cfg(test)] mod tests { use super::*; + use crate::models::{StudentKey, SubmissionOutcome}; + + fn scan(dir: &Path) -> AssignmentInput { + load_local_input(&[dir], LocalInputOptions::default()).unwrap() + } + + fn scan_with(dir: &Path, roster: &Roster) -> AssignmentInput { + load_local_input( + &[dir], + LocalInputOptions { + roster: Some(roster), + ..Default::default() + }, + ) + .unwrap() + } #[test] fn test_extract_sid() { @@ -305,42 +607,316 @@ mod tests { } #[test] - fn test_discover_extracts_zip_archives() { + fn test_discover_submissions() { let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("alice_Lab5.py"), "pass").unwrap(); + std::fs::write(dir.path().join("bob_Lab5.py"), "pass").unwrap(); + std::fs::write(dir.path().join("alice_Lab6.py"), "pass").unwrap(); + + let input = scan(dir.path()); + assert_eq!(input.student_count(), 2); + assert_eq!(input.students[0].files().len(), 2); // alice + assert_eq!(input.students[1].files().len(), 1); // bob + assert_eq!(input.languages(), vec!["python"]); + // With no roster, membership is unknown rather than negative. + assert!( + input + .students + .iter() + .all(|s| s.roster_match == RosterMatch::NoRoster) + ); + assert_eq!( + input.with_outcome(SubmissionOutcome::NotSubmitted).count(), + 0 + ); + } - // Normal .py file + #[test] + fn test_discover_extracts_zip_archives_and_records_provenance() { + let dir = tempfile::tempdir().unwrap(); std::fs::write(dir.path().join("alice_Lab5.py"), "pass").unwrap(); - // Create a .zip containing a .py file let zip_path = dir.path().join("bob_12345_67890_Lab5.zip"); let file = std::fs::File::create(&zip_path).unwrap(); let mut zip = zip::ZipWriter::new(file); - zip.start_file("Lab5.py", zip::write::SimpleFileOptions::default()) + zip.start_file("src/Lab5.py", zip::write::SimpleFileOptions::default()) .unwrap(); use std::io::Write; zip.write_all(b"def foo(): return 42").unwrap(); zip.finish().unwrap(); - let result = discover_submissions(&[dir.path()], None).unwrap(); + let input = scan(dir.path()); + assert_eq!(input.student_count(), 2); + + let bob = input + .students + .iter() + .find(|s| s.key().raw() == "bob") + .expect("bob"); + // The in-archive path survives flattening. + assert_eq!( + bob.files()[0].origin, + FileOrigin::Archive { + archive: zip_path.clone(), + entry: "src/Lab5.py".to_string(), + } + ); - // alice from .py, bob from extracted .zip - assert_eq!(result.student_count(), 2); - assert!(result.by_student.contains_key("alice")); - assert!(result.by_student.contains_key("bob")); + // A second run reuses the extraction and must still report where the file came from. + 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].origin, bob.files()[0].origin); } #[test] - fn test_discover_submissions() { + fn test_archive_name_collision_is_reported_not_dropped() { let dir = tempfile::tempdir().unwrap(); - std::fs::write(dir.path().join("alice_Lab5.py"), "pass").unwrap(); - std::fs::write(dir.path().join("bob_Lab5.py"), "pass").unwrap(); - std::fs::write(dir.path().join("alice_Lab6.py"), "pass").unwrap(); - std::fs::write(dir.path().join("notes.txt"), "ignore me").unwrap(); + let zip_path = dir.path().join("carol_Lab5.zip"); + let file = std::fs::File::create(&zip_path).unwrap(); + let mut zip = zip::ZipWriter::new(file); + use std::io::Write; + for folder in ["a", "b"] { + zip.start_file( + format!("{folder}/Lab5.py"), + zip::write::SimpleFileOptions::default(), + ) + .unwrap(); + zip.write_all(b"pass").unwrap(); + } + zip.finish().unwrap(); + + let input = scan(dir.path()); + assert!( + input + .diagnostics + .iter() + .any(|d| matches!(&d.kind, DiagnosticKind::ArchiveNameCollision { .. })), + "expected a collision diagnostic, got {:?}", + input.diagnostics + ); + } + + #[test] + fn test_unusable_file_makes_the_owner_submitted_empty() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("2024010005_notes.txt"), "hello").unwrap(); + + let roster = Roster::from_pairs(&[("2024010005", "Eve")]); + let input = scan_with(dir.path(), &roster); + + assert_eq!(input.student_count(), 1); + assert_eq!( + input.students[0].outcome(), + SubmissionOutcome::SubmittedEmpty + ); + assert!(input.unmatched.is_empty()); + assert!( + input + .diagnostics + .iter() + .any(|d| matches!(&d.kind, DiagnosticKind::IgnoredFile { .. })) + ); + } + + #[cfg(unix)] + #[test] + fn test_a_file_we_cannot_stat_is_reported_not_treated_as_absent() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("2024010001_lab1.py"), "pass").unwrap(); + // A dangling symlink: `metadata()` fails on it exactly as it would for a file + // whose metadata cannot be read. + std::os::unix::fs::symlink( + dir.path().join("gone.py"), + dir.path().join("2024010002_lab1.py"), + ) + .unwrap(); + + let input = scan(dir.path()); + + // The healthy submission is unaffected... + assert_eq!(input.student_count(), 1); + // ...and the one we could not stat leaves a trace rather than vanishing. + assert!( + input + .diagnostics + .iter() + .any(|d| matches!(&d.kind, DiagnosticKind::UnreadableDirEntry { .. })), + "expected an UnreadableDirEntry diagnostic, got {:?}", + input.diagnostics + ); + } + + #[cfg(unix)] + #[test] + fn test_a_symlinked_submission_is_still_graded() { + let dir = tempfile::tempdir().unwrap(); + // The target lives outside the scanned directory, so only the link is discovered. + let elsewhere = tempfile::tempdir().unwrap(); + let real = elsewhere.path().join("real.py"); + std::fs::write(&real, "pass").unwrap(); + std::os::unix::fs::symlink(&real, dir.path().join("2024010001_lab1.py")).unwrap(); + + // `is_file()` followed symlinks, so switching to a non-following stat would have + // quietly stopped grading these. + let input = scan(dir.path()); + assert_eq!(input.student_count(), 1); + assert_eq!(input.students[0].outcome(), SubmissionOutcome::Executable); + } + + #[test] + fn test_keyless_file_becomes_an_unmatched_artifact() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("_notes_v2.py"), "pass").unwrap(); + + let input = scan(dir.path()); + assert_eq!(input.student_count(), 0); + assert_eq!(input.unmatched.len(), 1); + assert_eq!(input.unmatched[0].reason, UnmatchedReason::NoStudentKey); + } + + #[test] + fn test_roster_students_who_did_not_submit_survive() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("2024010001_lab1.py"), "pass").unwrap(); + + let roster = Roster::from_pairs(&[("2024010001", "Alice"), ("2024010004", "Dan")]); + let input = scan_with(dir.path(), &roster); + + assert_eq!(input.student_count(), 2); + let dan = input + .students + .iter() + .find(|s| s.key().raw() == "2024010004") + .expect("the non-submitter must not disappear"); + assert_eq!(dan.outcome(), SubmissionOutcome::NotSubmitted); + assert_eq!(dan.identity.name.as_deref(), Some("Dan")); + assert_eq!(dan.roster_match, RosterMatch::Matched(1)); + } + + #[test] + fn test_submitter_absent_from_roster_is_kept_as_unmatched() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("9999999999_lab1.py"), "pass").unwrap(); + + let roster = Roster::from_pairs(&[("2024010001", "Alice")]); + let input = scan_with(dir.path(), &roster); + + let stranger = input + .students + .iter() + .find(|s| s.key().raw() == "9999999999") + .expect("stranger"); + assert_eq!(stranger.outcome(), SubmissionOutcome::ReceivedUnmatched); + // Unconfirmed by any roster, so the key stays a bare extracted token. + assert!(matches!(stranger.key(), StudentKey::Extracted(_))); + assert_eq!(stranger.identity.key.to_string(), "local:9999999999"); + } + + #[test] + fn test_leading_zero_pair_stays_distinct_and_is_flagged() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("0024010003_lab1.py"), "pass").unwrap(); + std::fs::write(dir.path().join("24010003_lab1.py"), "pass").unwrap(); + + let roster = Roster::from_pairs(&[("0024010003", "Carol"), ("24010003", "Dave")]); + let input = scan_with(dir.path(), &roster); + + assert_eq!(input.student_count(), 2); + assert_eq!(input.students[0].identity.name.as_deref(), Some("Carol")); + assert_eq!(input.students[1].identity.name.as_deref(), Some("Dave")); + assert!( + input + .diagnostics + .iter() + .any(|d| matches!(&d.kind, DiagnosticKind::SuspectedZeroPaddedVariant { .. })) + ); + } + + #[test] + fn test_a_repeated_roster_row_yields_one_student() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("2024010001_lab1.py"), "pass").unwrap(); + + // The same person listed twice — merged before the submission ever looks them up. + let roster = Roster::from_pairs(&[("2024010001", "Alice"), ("2024010001", "Alice")]); + let input = scan_with(dir.path(), &roster); + + assert_eq!(input.student_count(), 1); + assert_eq!(input.students[0].roster_match, RosterMatch::Matched(0)); + assert_eq!(input.students[0].identity.name.as_deref(), Some("Alice")); + assert_eq!(input.students[0].outcome(), SubmissionOutcome::Executable); + } + + #[test] + fn test_a_repeated_roster_row_does_not_spawn_a_phantom_non_submitter() { + let dir = tempfile::tempdir().unwrap(); + let roster = Roster::from_pairs(&[("2024010004", "Dan"), ("2024010004", "Dan")]); + let input = scan_with(dir.path(), &roster); + + // One row per person, whether or not they submitted — two would collide on + // student_id the moment anything tried to persist them. + assert_eq!(input.student_count(), 1); + assert_eq!(input.students[0].outcome(), SubmissionOutcome::NotSubmitted); + } + + #[test] + fn test_a_contradictory_roster_surfaces_as_an_error_not_a_warning() { + let dir = tempfile::tempdir().unwrap(); + let roster_path = dir.path().join("roster.csv"); + std::fs::write( + &roster_path, + "name,class,student_id\nAlice Wu,A,2024010001\nAlice Chen,B,2024010001\n", + ) + .unwrap(); + let roster = crate::roster::load_roster(&roster_path).unwrap(); + + let subs = dir.path().join("subs"); + std::fs::create_dir(&subs).unwrap(); + std::fs::write(subs.join("2024010001_lab1.py"), "pass").unwrap(); + let input = load_local_input( + &[&subs], + LocalInputOptions { + roster: Some(&roster), + ..Default::default() + }, + ) + .unwrap(); + + // The CLI refuses to grade while this is present — attributing this submission to + // either name would be a coin flip. + let errors: Vec<_> = input.errors().collect(); + assert_eq!(errors.len(), 1); + assert!(matches!( + &errors[0].kind, + DiagnosticKind::ConflictingRosterEntry { .. } + )); + } + + #[test] + fn test_output_is_byte_identical_across_runs() { + let dir = tempfile::tempdir().unwrap(); + for i in 0..12 { + std::fs::write(dir.path().join(format!("s{i:04}_lab1.py")), "pass").unwrap(); + } + std::fs::write(dir.path().join("_orphan.py"), "pass").unwrap(); + std::fs::write(dir.path().join("s0003_notes.txt"), "hi").unwrap(); + + let first = serde_json::to_string(&scan(dir.path())).unwrap(); + let second = serde_json::to_string(&scan(dir.path())).unwrap(); + assert_eq!(first, second); + } + + #[test] + fn test_not_a_directory_is_a_hard_error() { + let dir = tempfile::tempdir().unwrap(); + let file = dir.path().join("a.py"); + std::fs::write(&file, "pass").unwrap(); - let result = discover_submissions(&[dir.path()], None).unwrap(); - assert_eq!(result.student_count(), 2); - assert_eq!(result.by_student["alice"].len(), 2); - assert_eq!(result.by_student["bob"].len(), 1); - assert_eq!(result.languages(), vec!["python"]); + let err = load_local_input(&[&file], LocalInputOptions::default()).unwrap_err(); + assert!(matches!(err, DiscoveryError::NotADirectory(_))); } } diff --git a/crates/scriptmark/src/display.rs b/crates/scriptmark/src/display.rs index 25443ba..d7878a9 100644 --- a/crates/scriptmark/src/display.rs +++ b/crates/scriptmark/src/display.rs @@ -20,13 +20,18 @@ pub fn display_summary(reports: &[&StudentReport], title: &str) { ]); for report in reports { - let status = report.status(); - let (status_str, status_color) = match status { - TestStatus::Passed => ("PASSED", Color::Green), - TestStatus::Failed => ("FAILED", Color::Red), - TestStatus::Missing => ("MISSING", Color::DarkGrey), - TestStatus::Error => ("ERROR", Color::Red), - TestStatus::Timeout => ("TIMEOUT", Color::Yellow), + // An infrastructure failure has no test results, so `status()` would call it + // MISSING — indistinguishable from a student who simply never submitted. + let (status_str, status_color) = if report.error.is_some() { + ("ERROR", Color::Red) + } else { + match report.status() { + TestStatus::Passed => ("PASSED", Color::Green), + TestStatus::Failed => ("FAILED", Color::Red), + TestStatus::Missing => ("MISSING", Color::DarkGrey), + TestStatus::Error => ("ERROR", Color::Red), + TestStatus::Timeout => ("TIMEOUT", Color::Yellow), + } }; let grade_str = report @@ -60,7 +65,9 @@ pub fn display_summary(reports: &[&StudentReport], title: &str) { pub fn display_failures(reports: &[&StudentReport]) { let failed: Vec<_> = reports .iter() - .filter(|r| r.status() == TestStatus::Failed || r.status() == TestStatus::Error) + .filter(|r| { + r.error.is_some() || r.status() == TestStatus::Failed || r.status() == TestStatus::Error + }) .collect(); if failed.is_empty() { @@ -81,6 +88,10 @@ pub fn display_failures(reports: &[&StudentReport]) { .bold() ); + if let Some(error) = &report.error { + println!(" {} {error}", "ERROR".red().bold()); + } + for test_result in &report.test_results { for case in &test_result.cases { if case.status == TestStatus::Passed { @@ -97,7 +108,7 @@ pub fn display_failures(reports: &[&StudentReport]) { println!( " {} [{}] {}", status_str, - test_result.spec_name.dimmed(), + test_result.item_id.dimmed(), case.case_name ); diff --git a/crates/scriptmark/src/grading.rs b/crates/scriptmark/src/grading.rs index 3b4af3e..dd4b8ca 100644 --- a/crates/scriptmark/src/grading.rs +++ b/crates/scriptmark/src/grading.rs @@ -17,6 +17,13 @@ fn apply_template(reports: &mut [StudentReport], config: &TemplatePolicy) { let upper = config.upper; for report in reports.iter_mut() { + // A student who never submitted has no grade — not a zero. Turning any + // non-executable outcome into a number is P-677's decision to make, and a + // fabricated 0.0 here would be pushed straight to Canvas. + if !report.is_gradeable() { + report.final_grade = None; + continue; + } if report.status() == TestStatus::Missing { report.final_grade = Some(0.0); continue; @@ -60,6 +67,10 @@ fn apply_formula(reports: &mut [StudentReport], config: &FormulaPolicy) { let engine = Engine::new(); for report in reports.iter_mut() { + if !report.is_gradeable() { + report.final_grade = None; + continue; + } if report.status() == TestStatus::Missing { report.final_grade = Some(0.0); continue; @@ -115,14 +126,11 @@ mod tests { StudentReport { student_id: "test".to_string(), - student_name: None, test_results: vec![TestResult { - spec_name: "test".to_string(), + item_id: "test".to_string(), cases, }], - final_grade: None, - backend_name: None, - lint_score: None, + ..Default::default() } } @@ -189,16 +197,42 @@ mod tests { fn test_missing_student_gets_zero() { let mut reports = vec![StudentReport { student_id: "missing".to_string(), - student_name: None, - test_results: vec![], - final_grade: None, - backend_name: None, - lint_score: None, + ..Default::default() }]; apply_grading(&mut reports, &GradingPolicy::default()); assert_eq!(reports[0].final_grade, Some(0.0)); } + #[test] + fn test_non_submitter_is_left_ungraded_not_zeroed() { + use crate::models::SubmissionOutcome; + + for outcome in [ + SubmissionOutcome::NotSubmitted, + SubmissionOutcome::SubmittedEmpty, + SubmissionOutcome::ReceivedUnmatched, + ] { + let mut reports = vec![StudentReport { + student_id: "absent".to_string(), + submission_state: Some(outcome), + ..Default::default() + }]; + apply_grading(&mut reports, &GradingPolicy::default()); + assert_eq!( + reports[0].final_grade, None, + "{outcome:?} must not be turned into a number" + ); + } + + // Someone who did submit runnable code is graded as before. + let mut graded = vec![StudentReport { + submission_state: Some(SubmissionOutcome::Executable), + ..make_report(100) + }]; + apply_grading(&mut graded, &GradingPolicy::default()); + assert!(graded[0].final_grade.is_some()); + } + #[test] fn test_formula_basic() { let policy = GradingPolicy::Formula(FormulaPolicy { diff --git a/crates/scriptmark/src/input/canvas.rs b/crates/scriptmark/src/input/canvas.rs new file mode 100644 index 0000000..f381259 --- /dev/null +++ b/crates/scriptmark/src/input/canvas.rs @@ -0,0 +1,964 @@ +//! The Canvas entry point: turn Canvas API payloads into the unified input. +//! +//! [`normalize`] is pure — it performs no I/O. Fetching, pagination and attachment +//! download are P-670; this module defines the payload shapes that work lands in, so the +//! HTTP layer can be written and tested against exactly what normalisation consumes. +//! +//! The payload types live here rather than in [`crate::canvas::client`] on purpose: that +//! module's `CanvasSubmission` is the *grade-push response* shape, and its types are not +//! re-exported. + +use std::collections::{BTreeMap, HashMap}; +use std::path::{Path, PathBuf}; + +use serde::{Deserialize, Serialize}; + +use crate::discovery::detect_language; +use crate::models::{ + Assignment, AssignmentInput, Attachment, AttemptPolicy, DiagnosticKind, FileOrigin, + InputDiagnostic, InputSource, RosterMatch, SourceStatus, StudentFile, StudentIdentity, + StudentKey, StudentSubmission, SubmissionAttempt, is_reserved_key, normalize_key, +}; +use crate::roster::{Roster, RosterEntry, RosterSource}; + +/// A course user, as `GET /courses/:id/users` returns it. +/// +/// `sis_user_id` is `Option` and nothing else: a payload sending it unquoted is a +/// deserialisation error rather than a silent number→string coercion, which is what keeps +/// a 学号 like `0024010003` from arriving as `24010003`. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct CanvasUserPayload { + pub id: u64, + #[serde(default)] + pub name: Option, + #[serde(default)] + pub sortable_name: Option, + #[serde(default)] + pub sis_user_id: Option, + #[serde(default)] + pub login_id: Option, + #[serde(default)] + pub email: Option, +} + +/// An uploaded file, as Canvas reports it on a submission. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct CanvasAttachmentPayload { + pub id: u64, + #[serde(default)] + pub filename: Option, + #[serde(default)] + pub display_name: Option, + #[serde(default)] + pub content_type: Option, + #[serde(default)] + pub size: Option, + #[serde(default)] + pub url: Option, +} + +impl CanvasAttachmentPayload { + fn name(&self) -> String { + self.display_name + .clone() + .or_else(|| self.filename.clone()) + .unwrap_or_else(|| format!("attachment-{}", self.id)) + } +} + +/// A submission, as `GET /courses/:id/assignments/:id/submissions` returns it. +/// +/// Canvas emits a placeholder row for every enrolled student, so `attempt: None` with +/// `workflow_state: "unsubmitted"` means *nothing was handed in* — not an empty hand-in. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct CanvasSubmissionPayload { + #[serde(default)] + pub id: u64, + pub user_id: u64, + #[serde(default)] + pub attempt: Option, + #[serde(default)] + pub workflow_state: Option, + #[serde(default)] + pub submitted_at: Option, + #[serde(default)] + pub late: bool, + #[serde(default)] + pub missing: bool, + #[serde(default)] + pub excused: Option, + #[serde(default)] + pub submission_type: Option, + #[serde(default)] + pub body: Option, + #[serde(default)] + pub attachments: Vec, + /// Earlier attempts, when the request asked for them. + #[serde(default)] + pub submission_history: Vec, +} + +impl CanvasSubmissionPayload { + fn source_status(&self) -> SourceStatus { + SourceStatus { + workflow_state: self + .workflow_state + .clone() + .unwrap_or_else(|| "unknown".to_string()), + late: self.late, + missing: self.missing, + excused: self.excused.unwrap_or(false), + } + } + + fn has_text_body(&self) -> bool { + self.body.as_deref().is_some_and(|b| !b.trim().is_empty()) + } +} + +/// Everything one Canvas import fetched. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct CanvasPayload { + #[serde(default)] + pub course_id: Option, + #[serde(default)] + pub assignment_id: Option, + #[serde(default)] + pub assignment_name: Option, + #[serde(default)] + pub users: Vec, + #[serde(default)] + pub submissions: Vec, +} + +/// Where each downloaded attachment landed, keyed by attachment id. +/// +/// P-669 does no downloading, so this is supplied by the caller: the fixtures point it at +/// committed files, and P-670 fills it after fetching. An attachment that is missing from +/// it is reported as [`DiagnosticKind::PendingDownload`] and never makes a student +/// executable. +pub type DownloadedAttachments = HashMap; + +/// Turn Canvas payloads into the unified input. +/// +/// Canvas decides who is in the course. The roster is the union of course enrollment and +/// whatever the teacher supplied: enrollment settles membership, so somebody Canvas lists +/// is never called a stranger because a spreadsheet is out of date, and a non-submitter +/// still appears rather than vanishing. A supplied row Canvas has never heard of is kept +/// and flagged `NotEnrolled`, so a hand-maintained list cannot lose people either. +pub fn normalize( + payload: &CanvasPayload, + roster: Option<&Roster>, + downloads: &DownloadedAttachments, + policy: AttemptPolicy, +) -> AssignmentInput { + let mut diagnostics: Vec = Vec::new(); + + let users: BTreeMap = + payload.users.iter().map(|u| (u.id, u)).collect(); + + let roster = merged_roster(&payload.users, roster, &mut diagnostics); + + let mut students: Vec = Vec::new(); + // Keyed on the value, not its rendering — `Display` prefixes are not escaped. + let mut covered: std::collections::BTreeSet = Default::default(); + let mut covered_canvas_ids: std::collections::BTreeSet = Default::default(); + let mut seen_users: std::collections::BTreeSet = Default::default(); + + // Deterministic regardless of payload order. + let mut submissions: Vec<&CanvasSubmissionPayload> = payload.submissions.iter().collect(); + submissions.sort_by_key(|s| s.user_id); + + for submission in submissions { + if !seen_users.insert(submission.user_id) { + diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::DuplicateSubmissionRow { + canvas_user_id: submission.user_id, + }, + )); + continue; + } + let user = users.get(&submission.user_id).copied(); + let mut identity = identity_for(submission.user_id, user, &mut diagnostics); + + let attempts = attempts_of(submission, downloads, &mut diagnostics, &identity); + + // A placeholder row for someone the roster covers is handled by the roster merge + // below, which is the only producer of NotSubmitted. A placeholder row for someone + // the roster does not cover describes a non-event: nothing arrived and nobody is + // owed a grade. + if attempts.is_empty() { + continue; + } + + if submission.attachments.is_empty() && submission.has_text_body() { + diagnostics.push(InputDiagnostic::warning(DiagnosticKind::TextEntryOnly { + key: identity.key.raw(), + })); + } + + let roster_match = match roster.lookup(&identity.key) { + Some(i) => { + covered.insert(identity.key.clone()); + covered_canvas_ids.extend(identity.canvas_user_id); + if identity.name.is_none() { + identity.name = roster.entries[i].name.clone(); + } + RosterMatch::Matched(i) + } + None => { + diagnostics.push(InputDiagnostic::warning(DiagnosticKind::NotOnRoster { + key: identity.key.raw(), + })); + RosterMatch::NotInRoster + } + }; + + students.push(StudentSubmission::received( + identity, + roster_match, + attempts, + policy, + )); + } + + // Canvas knows who these people are even though the teacher's CSV only carries a + // number, so their Canvas identity is filled in from enrollment rather than lost. + let mut by_number: BTreeMap> = BTreeMap::new(); + for user in &payload.users { + if let Some(number) = user.sis_user_id.as_deref().map(normalize_key) + && !number.is_empty() + && !is_reserved_key(&number) + { + by_number.entry(number).or_default().push(user); + } + } + + for entry in &roster.entries { + if !covered.insert(entry.key.clone()) { + continue; + } + // A roster that names one person under both a 学号 and a Canvas id must not count + // them twice. + if entry + .canvas_user_id + .is_some_and(|id| covered_canvas_ids.contains(&id)) + { + continue; + } + covered_canvas_ids.extend(entry.canvas_user_id); + + let Some(index) = roster.lookup(&entry.key) else { + continue; + }; + let mut identity = match &entry.key { + StudentKey::CanvasUser(id) => StudentIdentity::canvas_user(*id), + key => StudentIdentity::number(key.raw()), + }; + identity.name = entry.name.clone(); + identity.canvas_user_id = identity.canvas_user_id.or(entry.canvas_user_id); + + // Enrich from enrollment. A Canvas-keyed row resolves by Canvas id; a 学号 row + // resolves by student number — never through `raw()`, which would compare a Canvas + // id against other people's SIS ids and walk straight through the namespace + // boundary `lookup` exists to hold. + let enrolled = match &entry.key { + StudentKey::CanvasUser(id) => users.get(id).copied(), + _ => entry + .student_number() + .and_then(|number| by_number.get(number)) + .filter(|candidates| candidates.len() == 1) + .map(|candidates| candidates[0]), + }; + if let Some(user) = enrolled { + identity.canvas_user_id = identity.canvas_user_id.or(Some(user.id)); + identity.sis_user_id = user.sis_user_id.as_deref().map(normalize_key); + identity.login_id = user.login_id.clone(); + identity.sortable_name = user.sortable_name.clone(); + identity.email = user.email.clone(); + if identity.name.is_none() { + identity.name = user.name.clone(); + } + } + students.push(StudentSubmission::not_submitted(identity, index)); + } + + diagnostics.extend(roster.diagnostics.iter().cloned()); + diagnostics.dedup(); + + let mut input = AssignmentInput { + assignment: Assignment { + name: payload.assignment_name.clone().unwrap_or_default(), + canvas_course_id: payload.course_id, + canvas_assignment_id: payload.assignment_id, + ..Assignment::default() + }, + source: InputSource::Canvas { + course_id: payload.course_id, + assignment_id: payload.assignment_id, + }, + roster: Some(roster), + students, + unmatched: Vec::new(), + diagnostics, + }; + + let zero_padded = input.detect_zero_padded_variants(); + input.diagnostics.extend(zero_padded); + + input.sorted() +} + +/// The roster of record on the Canvas path: course enrollment, plus any supplied row it +/// does not already cover. +/// +/// Canvas is authoritative about who is in the course — an enrollee carrying no SIS id is +/// keyed by their Canvas id rather than dropped, and a submitter Canvas knows about is +/// never called a stranger because a teacher's spreadsheet is out of date. A supplied row +/// for somebody Canvas has never heard of is still kept, marked as supplied and flagged, +/// so a hand-maintained list cannot silently lose people either. +fn merged_roster( + users: &[CanvasUserPayload], + supplied: Option<&Roster>, + diagnostics: &mut Vec, +) -> Roster { + let mut entries: Vec = users + .iter() + .map(|u| { + let number = u + .sis_user_id + .as_deref() + .map(normalize_key) + .filter(|n| !n.is_empty() && !is_reserved_key(n)); + RosterEntry { + key: match number { + Some(number) => StudentKey::Number(number), + None => StudentKey::CanvasUser(u.id), + }, + source: RosterSource::CanvasEnrollment, + name: u.name.clone(), + canvas_user_id: Some(u.id), + location: None, + } + }) + .collect(); + entries.sort_by(|a, b| (&a.key, a.canvas_user_id).cmp(&(&b.key, b.canvas_user_id))); + + let mut carried = Vec::new(); + if let Some(supplied) = supplied { + let enrolled: std::collections::BTreeSet<&StudentKey> = + entries.iter().map(|e| &e.key).collect(); + for entry in &supplied.entries { + if enrolled.contains(&entry.key) { + continue; + } + diagnostics.push(InputDiagnostic::warning(DiagnosticKind::NotEnrolled { + key: entry.key.raw(), + })); + carried.push(entry.clone()); + } + diagnostics.extend(supplied.diagnostics.iter().cloned()); + } + entries.extend(carried); + + Roster::from_entries(entries) +} + +fn identity_for( + user_id: u64, + user: Option<&CanvasUserPayload>, + diagnostics: &mut Vec, +) -> StudentIdentity { + let sis = user + .and_then(|u| u.sis_user_id.as_deref()) + .map(normalize_key) + .filter(|s| !s.is_empty()) + // The same rule the roster loader applies: a 学号 beginning with a reserved + // prefix would render as a key of another kind and stop round-tripping. + .filter(|s| !is_reserved_key(s)); + + let mut identity = match &sis { + // Canvas vouches for its own SIS id, so this is a confirmed 学号 even before a + // roster is consulted. + Some(number) => StudentIdentity::number(number), + None => { + diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::MissingStudentNumber { + canvas_user_id: user_id, + }, + )); + StudentIdentity::canvas_user(user_id) + } + }; + + identity.canvas_user_id = Some(user_id); + identity.sis_user_id = sis; + if let Some(user) = user { + identity.name = user.name.clone(); + identity.sortable_name = user.sortable_name.clone(); + identity.login_id = user.login_id.clone(); + identity.email = user.email.clone(); + } + identity +} + +/// Every attempt Canvas reported, newest information first resolved into our own shape. +fn attempts_of( + submission: &CanvasSubmissionPayload, + downloads: &DownloadedAttachments, + diagnostics: &mut Vec, + identity: &StudentIdentity, +) -> Vec { + let rows: Vec<&CanvasSubmissionPayload> = if submission.submission_history.is_empty() { + vec![submission] + } else { + submission.submission_history.iter().collect() + }; + + let mut attempts: Vec = Vec::new(); + for row in rows { + // No attempt number means Canvas's placeholder row: nothing was handed in. + let Some(number) = row.attempt else { + continue; + }; + + let mut attachments = Vec::new(); + let mut files = Vec::new(); + for payload in &row.attachments { + let name = payload.name(); + attachments.push(Attachment { + id: payload.id, + filename: name.clone(), + content_type: payload.content_type.clone(), + size: payload.size, + url: payload.url.clone(), + }); + + match downloads.get(&payload.id) { + Some(path) => { + let ext = Path::new(&name) + .extension() + .and_then(|e| e.to_str()) + .unwrap_or("") + .to_lowercase(); + if let Some(language) = detect_language(&ext) { + files.push(StudentFile::direct(path.clone(), language).with_origin( + FileOrigin::Attachment { + attempt: number, + attachment_id: payload.id, + }, + )); + } else { + diagnostics.push(InputDiagnostic::info(DiagnosticKind::IgnoredFile { + key: identity.key.raw(), + path: path.clone(), + })); + } + } + // Not downloaded, so there is nothing to run — the student is not made + // executable on the strength of an attachment we do not have. + None => { + diagnostics.push(InputDiagnostic::warning(DiagnosticKind::PendingDownload { + attachment_id: payload.id, + filename: name.clone(), + })) + } + } + } + + files.sort_by(|a, b| a.path.cmp(&b.path)); + attachments.sort_by(|a, b| a.filename.cmp(&b.filename)); + + attempts.push(SubmissionAttempt { + attempt: number, + submitted_at: row.submitted_at.clone(), + source_status: Some(row.source_status()), + attachments, + files, + }); + } + + attempts.sort_by_key(|a| a.attempt); + attempts +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::models::{StudentKey, SubmissionOutcome}; + + fn user(id: u64, sis: Option<&str>, name: &str) -> CanvasUserPayload { + CanvasUserPayload { + id, + name: Some(name.to_string()), + sortable_name: None, + sis_user_id: sis.map(str::to_string), + login_id: None, + email: None, + } + } + + fn attachment(id: u64, name: &str) -> CanvasAttachmentPayload { + CanvasAttachmentPayload { + id, + filename: Some(name.to_string()), + display_name: Some(name.to_string()), + content_type: Some("text/x-python".to_string()), + size: Some(4), + url: Some(format!("https://example.invalid/files/{id}")), + } + } + + fn submitted( + user_id: u64, + attempt: u32, + attachments: Vec, + ) -> CanvasSubmissionPayload { + CanvasSubmissionPayload { + id: user_id * 10 + attempt as u64, + user_id, + attempt: Some(attempt), + workflow_state: Some("submitted".to_string()), + submitted_at: Some(format!("2026-03-0{attempt}T00:00:00Z")), + late: false, + missing: false, + excused: None, + submission_type: Some("online_upload".to_string()), + body: None, + attachments, + submission_history: Vec::new(), + } + } + + fn placeholder(user_id: u64) -> CanvasSubmissionPayload { + CanvasSubmissionPayload { + id: 0, + user_id, + attempt: None, + workflow_state: Some("unsubmitted".to_string()), + submitted_at: None, + late: false, + missing: true, + excused: None, + submission_type: None, + body: None, + attachments: Vec::new(), + submission_history: Vec::new(), + } + } + + fn downloads(pairs: &[(u64, &str)]) -> DownloadedAttachments { + pairs + .iter() + .map(|(id, path)| (*id, PathBuf::from(path))) + .collect() + } + + #[test] + fn test_unsubmitted_placeholder_is_not_submitted_not_empty() { + let payload = CanvasPayload { + users: vec![user(1, Some("2024010004"), "Dan")], + submissions: vec![placeholder(1)], + ..Default::default() + }; + let input = normalize(&payload, None, &downloads(&[]), AttemptPolicy::Latest); + + assert_eq!(input.student_count(), 1); + assert_eq!(input.students[0].outcome(), SubmissionOutcome::NotSubmitted); + } + + #[test] + fn test_submitted_with_no_attachments_is_empty_not_missing() { + let payload = CanvasPayload { + users: vec![user(1, Some("2024010005"), "Eve")], + submissions: vec![submitted(1, 1, vec![])], + ..Default::default() + }; + let input = normalize(&payload, None, &downloads(&[]), AttemptPolicy::Latest); + + assert_eq!( + input.students[0].outcome(), + SubmissionOutcome::SubmittedEmpty + ); + } + + #[test] + fn test_latest_attempt_wins_whatever_the_history_order() { + let mut base = submitted(1, 2, vec![attachment(20, "lab1.py")]); + base.submission_history = vec![ + submitted(1, 2, vec![attachment(20, "lab1.py")]), + submitted(1, 1, vec![attachment(10, "draft.py")]), + ]; + let payload = CanvasPayload { + users: vec![user(1, Some("2024010002"), "Bob")], + submissions: vec![base.clone()], + ..Default::default() + }; + let files = downloads(&[(10, "/tmp/draft.py"), (20, "/tmp/lab1.py")]); + + let input = normalize(&payload, None, &files, AttemptPolicy::Latest); + let student = &input.students[0]; + assert_eq!(student.selected_attempt().unwrap().attempt, 2); + assert_eq!(student.files()[0].file_name(), "lab1.py"); + + // Reversed history must not change the answer. + let mut reversed = base; + reversed.submission_history.reverse(); + let payload = CanvasPayload { + users: vec![user(1, Some("2024010002"), "Bob")], + submissions: vec![reversed], + ..Default::default() + }; + let input = normalize(&payload, None, &files, AttemptPolicy::Latest); + assert_eq!(input.students[0].selected_attempt().unwrap().attempt, 2); + } + + #[test] + fn test_undownloaded_attachment_does_not_make_a_student_executable() { + let payload = CanvasPayload { + users: vec![user(1, Some("2024010001"), "Alice")], + submissions: vec![submitted(1, 1, vec![attachment(10, "lab1.py")])], + ..Default::default() + }; + let input = normalize(&payload, None, &downloads(&[]), AttemptPolicy::Latest); + + assert_eq!( + input.students[0].outcome(), + SubmissionOutcome::SubmittedEmpty + ); + assert!( + input + .diagnostics + .iter() + .any(|d| matches!(&d.kind, DiagnosticKind::PendingDownload { .. })) + ); + // The attachment is still recorded — we know it exists, we just don't have it. + assert_eq!(input.students[0].artifact_names(), vec!["lab1.py"]); + } + + #[test] + fn test_user_without_sis_id_is_kept_under_its_canvas_id() { + let payload = CanvasPayload { + users: vec![user(4242, None, "No SIS")], + submissions: vec![submitted(4242, 1, vec![attachment(10, "lab1.py")])], + ..Default::default() + }; + let input = normalize( + &payload, + None, + &downloads(&[(10, "/tmp/lab1.py")]), + AttemptPolicy::Latest, + ); + + assert_eq!(input.student_count(), 1); + assert_eq!(input.students[0].key(), &StudentKey::CanvasUser(4242)); + assert_eq!(input.students[0].identity.key.to_string(), "canvas:4242"); + assert!(input.diagnostics.iter().any(|d| matches!( + &d.kind, + DiagnosticKind::MissingStudentNumber { canvas_user_id } if *canvas_user_id == 4242 + ))); + // Enrollment is the roster when none is supplied, and Canvas says this person is + // enrolled — so they are a member, not a stranger. + assert_eq!(input.students[0].roster_match, RosterMatch::Matched(0)); + assert_eq!(input.students[0].outcome(), SubmissionOutcome::Executable); + } + + #[test] + fn test_a_sis_id_that_looks_like_a_rendered_key_is_not_used_as_a_number() { + // Would otherwise render as `local:alice` and parse back as an Extracted key, + // so Display would stop being reversible. + let payload = CanvasPayload { + users: vec![user(7, Some("local:alice"), "Odd")], + submissions: vec![placeholder(7)], + ..Default::default() + }; + let input = normalize(&payload, None, &downloads(&[]), AttemptPolicy::Latest); + + assert_eq!(input.students[0].key(), &StudentKey::CanvasUser(7)); + assert!(input.diagnostics.iter().any(|d| matches!( + &d.kind, + DiagnosticKind::MissingStudentNumber { canvas_user_id } if *canvas_user_id == 7 + ))); + } + + #[test] + fn test_numeric_sis_user_id_is_rejected_rather_than_coerced() { + // A lenient number→string coercion here would silently turn 0024010003 into + // 24010003 and merge two students. + let err = serde_json::from_str::(r#"{"id":1,"sis_user_id":24010003}"#) + .unwrap_err(); + assert!( + err.to_string().contains("invalid type"), + "expected a type error, got: {err}" + ); + } + + #[test] + fn test_canvas_enrollment_outranks_a_stale_supplied_roster() { + let payload = CanvasPayload { + users: vec![ + user(1, Some("2024010001"), "Alice"), + user(2, Some("9999999999"), "Late Add"), + ], + submissions: vec![ + submitted(1, 1, vec![attachment(10, "lab1.py")]), + submitted(2, 1, vec![attachment(20, "lab1.py")]), + ], + ..Default::default() + }; + // The teacher's CSV predates the late enrolment. + let roster = Roster::from_pairs(&[("2024010001", "Alice")]); + let files = downloads(&[(10, "/tmp/a.py"), (20, "/tmp/b.py")]); + + let input = normalize(&payload, Some(&roster), &files, AttemptPolicy::Latest); + + // Canvas is authoritative about who is in the course, so someone it says is + // enrolled is a member — not a stranger whose work goes ungraded. + for key in ["2024010001", "9999999999"] { + let student = input + .students + .iter() + .find(|s| s.key().raw() == key) + .unwrap_or_else(|| panic!("{key} missing")); + assert_eq!(student.outcome(), SubmissionOutcome::Executable, "{key}"); + } + } + + #[test] + fn test_a_supplied_row_canvas_has_never_heard_of_is_kept_and_flagged() { + let payload = CanvasPayload { + users: vec![user(1, Some("2024010001"), "Alice")], + submissions: vec![placeholder(1)], + ..Default::default() + }; + // Somebody the teacher tracks who was never enrolled — kept, so a hand-maintained + // list cannot silently lose people either. + let roster = Roster::from_pairs(&[("2024010001", "Alice"), ("2024019999", "Ghost")]); + let input = normalize( + &payload, + Some(&roster), + &downloads(&[]), + AttemptPolicy::Latest, + ); + + assert_eq!(input.student_count(), 2); + let ghost = input + .students + .iter() + .find(|s| s.key().raw() == "2024019999") + .expect("the supplied-only row must survive"); + assert_eq!(ghost.outcome(), SubmissionOutcome::NotSubmitted); + assert!( + input + .diagnostics + .iter() + .any(|d| matches!(&d.kind, DiagnosticKind::NotEnrolled { .. })), + "and must be flagged as not enrolled" + ); + } + + #[test] + fn test_enrollment_becomes_the_roster_when_none_is_supplied() { + let payload = CanvasPayload { + users: vec![ + user(1, Some("2024010001"), "Alice"), + user(2, Some("2024010004"), "Dan"), + ], + submissions: vec![ + submitted(1, 1, vec![attachment(10, "lab1.py")]), + placeholder(2), + ], + ..Default::default() + }; + let input = normalize( + &payload, + None, + &downloads(&[(10, "/tmp/a.py")]), + AttemptPolicy::Latest, + ); + + assert_eq!(input.student_count(), 2); + assert_eq!(input.roster.as_ref().unwrap().len(), 2); + let dan = input + .students + .iter() + .find(|s| s.key().raw() == "2024010004") + .unwrap(); + assert_eq!(dan.outcome(), SubmissionOutcome::NotSubmitted); + } + + #[test] + fn test_two_accounts_claiming_one_student_number_is_a_hard_error() { + let payload = CanvasPayload { + users: vec![ + user(1, Some("2024010001"), "Alice"), + user(2, Some("2024010001"), "Alice Chen"), + ], + submissions: vec![ + submitted(1, 1, vec![attachment(10, "lab1.py")]), + submitted(2, 1, vec![attachment(20, "lab1.py")]), + ], + ..Default::default() + }; + let input = normalize( + &payload, + None, + &downloads(&[(10, "/tmp/a.py"), (20, "/tmp/b.py")]), + AttemptPolicy::Latest, + ); + + // A student number identifies one person, so two enrolments claiming it cannot + // both be right — and picking one would grade somebody's work under another name. + let errors: Vec<_> = input.errors().collect(); + assert_eq!(errors.len(), 1); + assert!(matches!( + &errors[0].kind, + DiagnosticKind::ConflictingRosterEntry { key, .. } if key == "2024010001" + )); + } + + #[test] + fn test_sis_id_is_trimmed_the_same_way_the_csv_is() { + let payload = CanvasPayload { + users: vec![user(1, Some(" 2024010001 "), "Alice")], + submissions: vec![submitted(1, 1, vec![attachment(10, "lab1.py")])], + ..Default::default() + }; + let roster = Roster::from_pairs(&[("2024010001", "Alice")]); + let input = normalize( + &payload, + Some(&roster), + &downloads(&[(10, "/tmp/a.py")]), + AttemptPolicy::Latest, + ); + + assert_eq!(input.students[0].roster_match, RosterMatch::Matched(0)); + assert_eq!(input.students[0].outcome(), SubmissionOutcome::Executable); + } + + #[test] + fn test_a_contradictory_roster_is_reported_the_same_whatever_the_payload_order() { + let users = vec![ + user(1, Some("2024010001"), "Alice One"), + user(2, Some("2024010001"), "Alice Two"), + ]; + let roster = Roster::from_pairs(&[("2024010001", "Alice")]); + + let run = |users: Vec| { + serde_json::to_string(&normalize( + &CanvasPayload { + users, + ..Default::default() + }, + Some(&roster), + &downloads(&[]), + AttemptPolicy::Latest, + )) + .unwrap() + }; + + let mut reversed = users.clone(); + reversed.reverse(); + // Which account the payload happened to list first must not decide anything. + assert_eq!(run(users), run(reversed)); + } + + #[test] + fn test_a_canvas_id_never_resolves_against_someone_elses_student_number() { + // User 2024010001's Canvas id is the decimal text of another student's 学号. + let payload = CanvasPayload { + users: vec![ + user(2024010001, None, "No SIS"), + user(7, Some("2024010001"), "Alice"), + ], + submissions: vec![placeholder(2024010001), placeholder(7)], + ..Default::default() + }; + let input = normalize(&payload, None, &downloads(&[]), AttemptPolicy::Latest); + + let no_sis = input + .students + .iter() + .find(|s| s.key() == &StudentKey::CanvasUser(2024010001)) + .expect("the SIS-less enrollee"); + // Their record must not pick up Alice's identity across the namespace boundary. + assert_eq!(no_sis.identity.sis_user_id, None); + assert_eq!(no_sis.identity.name.as_deref(), Some("No SIS")); + } + + #[test] + fn test_a_repeated_submission_row_does_not_become_a_second_student() { + let payload = CanvasPayload { + users: vec![user(1, Some("2024010001"), "Alice")], + submissions: vec![ + submitted(1, 1, vec![attachment(10, "lab1.py")]), + submitted(1, 1, vec![attachment(10, "lab1.py")]), + ], + ..Default::default() + }; + let input = normalize( + &payload, + None, + &downloads(&[(10, "/tmp/a.py")]), + AttemptPolicy::Latest, + ); + + assert_eq!(input.student_count(), 1); + assert!( + input + .diagnostics + .iter() + .any(|d| matches!(&d.kind, DiagnosticKind::DuplicateSubmissionRow { .. })) + ); + } + + #[test] + fn test_text_entry_without_a_file_is_flagged() { + let mut submission = submitted(1, 1, vec![]); + submission.submission_type = Some("online_text_entry".to_string()); + submission.body = Some("my answer".to_string()); + let payload = CanvasPayload { + users: vec![user(1, Some("2024010006"), "Faye")], + submissions: vec![submission], + ..Default::default() + }; + let input = normalize(&payload, None, &downloads(&[]), AttemptPolicy::Latest); + + assert_eq!( + input.students[0].outcome(), + SubmissionOutcome::SubmittedEmpty + ); + assert!( + input + .diagnostics + .iter() + .any(|d| matches!(&d.kind, DiagnosticKind::TextEntryOnly { .. })) + ); + } + + #[test] + fn test_source_status_is_preserved_but_stays_out_of_the_file_list() { + let mut submission = submitted(1, 1, vec![attachment(10, "lab1.py")]); + submission.late = true; + let payload = CanvasPayload { + users: vec![user(1, Some("2024010001"), "Alice")], + submissions: vec![submission], + ..Default::default() + }; + let input = normalize( + &payload, + None, + &downloads(&[(10, "/tmp/a.py")]), + AttemptPolicy::Latest, + ); + + let status = input.students[0] + .selected_attempt() + .unwrap() + .source_status + .as_ref() + .unwrap(); + assert!(status.late); + assert_eq!(status.workflow_state, "submitted"); + } +} diff --git a/crates/scriptmark/src/input/mod.rs b/crates/scriptmark/src/input/mod.rs new file mode 100644 index 0000000..ac95938 --- /dev/null +++ b/crates/scriptmark/src/input/mod.rs @@ -0,0 +1,12 @@ +//! Entry-point adapters. +//! +//! Each adapter turns one source's material into the same [`crate::models::AssignmentInput`]. +//! There is deliberately no `InputSource` trait: the ticket asks for a common *output* +//! interface, and the shared type plus its validator is that interface. A one-method trait +//! would additionally force the async Canvas loader (P-670) and the synchronous local scan +//! into one shape, which buys nothing. +//! +//! - Local directories: [`crate::discovery::load_local_input`] +//! - Canvas payloads: [`canvas::normalize`] + +pub mod canvas; diff --git a/crates/scriptmark/src/lib.rs b/crates/scriptmark/src/lib.rs index 2d64a52..8370803 100644 --- a/crates/scriptmark/src/lib.rs +++ b/crates/scriptmark/src/lib.rs @@ -2,6 +2,7 @@ pub mod models; pub mod discovery; pub mod grading; +pub mod input; pub mod roster; pub mod similarity; pub mod spec_loader; diff --git a/crates/scriptmark/src/main.rs b/crates/scriptmark/src/main.rs index b13f4c9..81933ca 100644 --- a/crates/scriptmark/src/main.rs +++ b/crates/scriptmark/src/main.rs @@ -5,9 +5,12 @@ use std::path::PathBuf; use anyhow::{Context, Result}; use clap::{Parser, Subcommand}; -use scriptmark::discovery::discover_submissions; +use scriptmark::discovery::{LocalInputOptions, load_local_input}; use scriptmark::grading::apply_grading; -use scriptmark::models::{FormulaPolicy, GradingPolicy, TemplatePolicy}; +use scriptmark::models::{ + Assignment, AssignmentInput, AttemptPolicy, DiagnosticSeverity, FormulaPolicy, GradingItem, + GradingPolicy, StudentKey, SubmissionOutcome, TemplatePolicy, TestSpec, +}; use scriptmark::roster::load_roster; use scriptmark::runner::orchestrator; use scriptmark::runner::python::PythonExecutor; @@ -67,6 +70,10 @@ struct GradeArgs { #[arg(short, long)] roster: Option, + /// Path to assignment.toml. Defaults to one beside the tests directory. + #[arg(long)] + assignment: Option, + /// Grading template: none, linear, sqrt, log, strict (default: sqrt) #[arg(short = 'g', long, default_value = "sqrt")] grading: String, @@ -119,6 +126,14 @@ struct RunArgs { #[arg(short, long, default_value = "output/results.json")] output: PathBuf, + /// Path to roster CSV (name,_,student_id) + #[arg(short, long)] + roster: Option, + + /// Path to assignment.toml. Defaults to one beside the tests directory. + #[arg(long)] + assignment: Option, + /// Per-test timeout in seconds #[arg(long, default_value = "10")] timeout: u64, @@ -281,6 +296,155 @@ fn build_grading_policy(grading: &str, formula: Option<&str>, range: (f64, f64)) } } +/// Load `assignment.toml`, explicitly or from beside the tests directory. +/// +/// An explicit path that cannot be read or parsed is an error; an absent default is not. +fn load_assignment( + explicit: Option<&PathBuf>, + tests_dir: &std::path::Path, +) -> Result<(Assignment, AttemptPolicy)> { + let path = match explicit { + Some(path) => Some(path.clone()), + None => [tests_dir.parent(), Some(tests_dir)] + .into_iter() + .flatten() + .map(|dir| dir.join("assignment.toml")) + .find(|candidate| candidate.is_file()), + }; + + let Some(path) = path else { + // Fall back to the directory name, which is what the db session has always used. + let name = tests_dir + .parent() + .and_then(|p| p.file_name()) + .or_else(|| tests_dir.file_name()) + .and_then(|n| n.to_str()) + .unwrap_or("unknown"); + return Ok((Assignment::named(name), AttemptPolicy::default())); + }; + + let config = scriptmark::spec_loader::load_assignment_config(&path) + .with_context(|| format!("Failed to load {}", path.display()))?; + Ok(( + Assignment { + name: config.assignment.name, + canvas_course_id: config.assignment.canvas_course_id, + canvas_assignment_id: config.assignment.canvas_assignment_id, + items: config.items, + }, + config.assignment.attempt_policy, + )) +} + +/// Reconcile the declared grading items against the specs that were actually loaded. +/// +/// Undeclared items are derived from the specs, so `Assignment.items` is always populated +/// and every `TestResult.item_id` names one of them. A declared item with no spec, or a +/// spec naming no declared item, is reported rather than silently ignored. +fn reconcile_items(assignment: &mut Assignment, specs: &[TestSpec]) { + if assignment.items.is_empty() { + assignment.items = specs + .iter() + .map(|spec| GradingItem::new(&spec.meta.name)) + .collect(); + return; + } + + for spec in specs { + if assignment.item(&spec.meta.name).is_none() { + eprintln!( + " warning: test spec '{}' is not a declared grading item", + spec.meta.name + ); + } + } + for item in &assignment.items { + if !specs.iter().any(|spec| spec.meta.name == item.id) { + eprintln!(" warning: grading item '{}' has no test spec", item.id); + } + } +} + +/// Build the unified input from local directories. +fn build_local_input( + submissions: &[PathBuf], + tests_dir: &std::path::Path, + assignment_path: Option<&PathBuf>, + roster_path: Option<&PathBuf>, +) -> Result { + let (assignment, attempt_policy) = load_assignment(assignment_path, tests_dir)?; + + let roster = match roster_path { + Some(path) => Some(load_roster(path).context("Failed to load roster")?), + None => None, + }; + + let input = load_local_input( + submissions, + LocalInputOptions { + assignment, + roster: roster.as_ref(), + attempt_policy, + }, + ) + .context("Failed to discover submissions")?; + + report_input(&input); + + // An Error diagnostic means the input cannot be trusted — a roster that disagrees with + // itself about who a 学号 belongs to would attribute somebody's work to the wrong name. + // Stop before running anything rather than producing results nobody should act on. + let errors: Vec = input.errors().map(|d| d.to_string()).collect(); + if !errors.is_empty() { + anyhow::bail!( + "refusing to grade: {} problem(s) with the input\n {}", + errors.len(), + errors.join("\n ") + ); + } + + Ok(input) +} + +/// Print the import summary and every anomaly the adapters recorded. Adapters never print +/// themselves — this is the only place diagnostics reach a terminal. +fn report_input(input: &AssignmentInput) { + use owo_colors::OwoColorize; + + let counts = [ + (SubmissionOutcome::Executable, "executable"), + (SubmissionOutcome::SubmittedEmpty, "submitted but empty"), + (SubmissionOutcome::ReceivedUnmatched, "received, unmatched"), + (SubmissionOutcome::NotSubmitted, "not submitted"), + ]; + println!("Found {} students:", input.student_count()); + for (outcome, label) in counts { + let n = input.with_outcome(outcome).count(); + if n > 0 { + println!(" {n:>4} {label}"); + } + } + if !input.unmatched.is_empty() { + println!( + " {:>4} files with no identifiable owner", + input.unmatched.len() + ); + } + + let errors = input.diagnostics_of(DiagnosticSeverity::Error).count(); + let warnings = input.diagnostics_of(DiagnosticSeverity::Warning).count(); + for diagnostic in &input.diagnostics { + match diagnostic.severity { + DiagnosticSeverity::Error => println!(" {} {diagnostic}", "error:".red()), + DiagnosticSeverity::Warning => println!(" {} {diagnostic}", "warning:".yellow()), + DiagnosticSeverity::Info => println!(" {} {diagnostic}", "note:".dimmed()), + } + } + if errors + warnings > 0 { + println!(" ({errors} errors, {warnings} warnings)"); + } +} + fn parse_range(s: &str) -> Result<(f64, f64), String> { let parts: Vec<&str> = s.split(',').collect(); if parts.len() != 2 { @@ -309,32 +473,27 @@ async fn main() -> Result<()> { } async fn cmd_grade(args: GradeArgs) -> Result<()> { - // 1. Discover submissions - let submissions = discover_submissions( - &args - .submissions - .iter() - .map(|p| p.as_path()) - .collect::>(), - None, - ) - .context("Failed to discover submissions")?; - - println!( - "Found {} students in {} directories", - submissions.student_count(), - args.submissions.len() - ); + // 1. Build the unified input — names, roster membership and submission state all come + // from the model, so there is no separate roster merge afterwards. + let input = build_local_input( + &args.submissions, + &args.tests_dir, + args.assignment.as_ref(), + args.roster.as_ref(), + )?; // 2. Load test specs let specs = load_specs_from_dir(&args.tests_dir).context("Failed to load test specifications")?; println!("Loaded {} test specs", specs.len()); + let mut input = input; + reconcile_items(&mut input.assignment, &specs); + // 3. Run tests let executor = PythonExecutor::with_python_cmd(&args.python); - let mut results = orchestrator::run_all( - &submissions, + let mut reports = orchestrator::run_all( + &input.students, &specs, &executor, args.timeout, @@ -342,19 +501,8 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { ) .await; - // 4. Load roster and merge names - if let Some(roster_path) = &args.roster { - let roster = load_roster(roster_path).context("Failed to load roster")?; - for (sid, report) in results.iter_mut() { - if let Some(name) = roster.get(sid) { - report.student_name = Some(name.clone()); - } - } - } - - // 5. Apply grading policy + // 4. Apply grading policy let policy = build_grading_policy(&args.grading, args.formula.as_deref(), args.range); - let mut reports: Vec<_> = results.into_values().collect(); apply_grading(&mut reports, &policy); reports.sort_by(|a, b| a.student_id.cmp(&b.student_id)); @@ -391,7 +539,8 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { wtr.write_record([ "student_name", "student_id", - "spec_name", + "submission_state", + "item_id", "case_name", "status", "actual", @@ -400,12 +549,19 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { "elapsed_ms", ])?; for report in &reports { + let state = report + .submission_state + .map(|s| format!("{s:?}")) + .unwrap_or_default(); + let mut rows = 0usize; for test_result in &report.test_results { for case in &test_result.cases { + rows += 1; wtr.write_record([ report.student_name.as_deref().unwrap_or(""), &report.student_id, - &test_result.spec_name, + &state, + &test_result.item_id, &case.case_name, &format!("{:?}", case.status), case.actual.as_deref().unwrap_or(""), @@ -418,6 +574,28 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { ])?; } } + // Every student gets at least one row, so the CSV covers the same cohort + // as the JSON archive rather than quietly dropping non-submitters — and + // a spec that produced no cases at all out of the denominator too. + if rows == 0 { + wtr.write_record([ + report.student_name.as_deref().unwrap_or(""), + &report.student_id, + &state, + "", + "", + if report.error.is_some() { + "Error".to_string() + } else { + format!("{:?}", report.status()) + } + .as_str(), + "", + "", + report.error.as_deref().unwrap_or(""), + "", + ])?; + } } wtr.flush()?; } @@ -433,25 +611,14 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { let database = scriptmark::db::Database::open(db_path).context("Failed to open database")?; - // Import roster if we loaded one - if let Some(roster_path) = &args.roster - && let Ok(roster) = scriptmark::roster::load_roster(roster_path) - { - let _ = database.import_roster(&roster); + if let Some(roster) = &input.roster { + database + .import_roster(roster) + .context("Failed to import roster")?; } - // Derive assignment name: try parent dir name (e.g. "hw5" from "courses/geec/hw5/tests") - // then fall back to tests_dir name, then "unknown" - let assignment = args - .tests_dir - .parent() - .and_then(|p| p.file_name()) - .and_then(|n| n.to_str()) - .or_else(|| args.tests_dir.file_name().and_then(|n| n.to_str())) - .unwrap_or("unknown"); - let session_id = database - .save_session(assignment, &reports, None) + .save_session(&input.assignment.name, &reports, None) .context("Failed to save session to database")?; println!( @@ -465,29 +632,24 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { } async fn cmd_run(args: RunArgs) -> Result<()> { - let submissions = discover_submissions( - &args - .submissions - .iter() - .map(|p| p.as_path()) - .collect::>(), - None, - ) - .context("Failed to discover submissions")?; - - println!( - "Found {} students in {} directories", - submissions.student_count(), - args.submissions.len() - ); + let input = build_local_input( + &args.submissions, + &args.tests_dir, + args.assignment.as_ref(), + args.roster.as_ref(), + )?; let specs = load_specs_from_dir(&args.tests_dir).context("Failed to load test specifications")?; println!("Loaded {} test specs", specs.len()); + let mut input = input; + reconcile_items(&mut input.assignment, &specs); + let executor = PythonExecutor::with_python_cmd(&args.python); + // A JSON array, the same shape `grade` writes and `summarize` reads. let results = orchestrator::run_all( - &submissions, + &input.students, &specs, &executor, args.timeout, @@ -513,8 +675,13 @@ fn cmd_summarize(args: SummarizeArgs) -> Result<()> { if let Some(roster_path) = &args.roster { let roster = load_roster(roster_path).context("Failed to load roster")?; for report in reports.iter_mut() { - if let Some(name) = roster.get(&report.student_id) { - report.student_name = Some(name.clone()); + // `student_id` is a rendered key, so it is parsed back rather than compared as + // text — otherwise a run made without --roster, whose ids carry a `local:` + // prefix, would match nothing. `name_of` answers only when the key is + // unambiguous: with duplicate roster rows there is no single right name, and + // guessing one would hide the clash. + if let Some(name) = roster.name_of(&StudentKey::parse(&report.student_id)) { + report.student_name = Some(name.to_string()); } } } @@ -558,20 +725,22 @@ async fn cmd_grades_push(args: GradesPushArgs) -> Result<()> { let reports: Vec = serde_json::from_str(&content).context("Failed to parse results JSON")?; - // Build grades map: try to parse student_id as u64 (Canvas user ID) + // Only students who actually ran code get a score pushed. Parsing student_id as an + // integer would either fail for every 学号 or, worse, succeed and post to whichever + // Canvas user happened to hold that number. let mut grades = std::collections::HashMap::new(); + let mut skipped = 0usize; for report in &reports { - if let Some(grade) = report.final_grade { - if let Ok(uid) = report.student_id.parse::() { + match (report.final_grade, report.canvas_user_id) { + (Some(grade), Some(uid)) if report.is_gradeable() => { grades.insert(uid, grade); - } else { - eprintln!( - "Warning: cannot push grade for '{}' — student_id is not a Canvas user ID", - report.student_id - ); } + _ => skipped += 1, } } + if skipped > 0 { + println!("Skipping {skipped} students with no grade or no Canvas user id"); + } println!( "Pushing {} grades to Canvas assignment {}...", @@ -718,10 +887,13 @@ fn cmd_db(cmd: DbCommand) -> Result<()> { DbAction::ImportRoster { roster, db } => { let database = scriptmark::db::Database::open(&db).context("Failed to open database")?; - let roster_map = + let roster_csv = scriptmark::roster::load_roster(&roster).context("Failed to load roster CSV")?; + for diagnostic in &roster_csv.diagnostics { + println!(" warning: {diagnostic}"); + } let count = database - .import_roster(&roster_map) + .import_roster(&roster_csv) .context("Failed to import roster")?; println!("Imported {} students into {}", count, db.display()); Ok(()) @@ -773,18 +945,21 @@ fn cmd_db(cmd: DbCommand) -> Result<()> { ); println!("{}", "-".repeat(75)); for (session, result) in &history { - let grade_color = if result.final_grade >= 90.0 { - "\x1b[32m" - } else if result.final_grade >= 70.0 { - "\x1b[34m" - } else { - "\x1b[31m" + let grade_color = match result.final_grade { + Some(g) if g >= 90.0 => "\x1b[32m", + Some(g) if g >= 70.0 => "\x1b[34m", + Some(_) => "\x1b[31m", + None => "\x1b[2m", }; + let grade_text = result + .final_grade + .map(|g| format!("{g:.1}")) + .unwrap_or_else(|| "-".to_string()); println!( - "{:<15} {}{:>7.1}\x1b[0m {:>9.1}% {:>8}/{} {}", + "{:<15} {}{:>7}\x1b[0m {:>9.1}% {:>8}/{} {}", session.assignment, grade_color, - result.final_grade, + grade_text, result.pass_rate, result.passed_cases, result.total_cases, diff --git a/crates/scriptmark/src/models/config.rs b/crates/scriptmark/src/models/config.rs index 35bc5f5..ecce965 100644 --- a/crates/scriptmark/src/models/config.rs +++ b/crates/scriptmark/src/models/config.rs @@ -1,5 +1,7 @@ use serde::{Deserialize, Serialize}; +use crate::models::GradingItem; + /// Grading policy — how to convert pass rate to final grade. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(untagged)] @@ -66,6 +68,17 @@ fn default_language() -> String { "python".to_string() } +/// Which submission attempt to grade when a source reports several. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum AttemptPolicy { + /// Highest attempt number wins. + #[default] + Latest, + /// Lowest attempt number wins. + Earliest, +} + /// Assignment-level configuration (from assignment.toml). #[derive(Debug, Clone, Serialize, Deserialize)] pub struct AssignmentConfig { @@ -73,6 +86,10 @@ pub struct AssignmentConfig { /// Expected student files. #[serde(default)] pub files: Vec, + /// The items this assignment is marked on. Each `id` is a test spec's `[meta] name`. + /// Left empty, the items are derived from the specs that were loaded. + #[serde(default)] + pub items: Vec, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -80,6 +97,15 @@ pub struct AssignmentInfo { pub name: String, #[serde(default = "default_tests_dir")] pub tests_dir: String, + /// Canvas course id. Kept apart from the assignment id and from any student identity. + #[serde(default)] + pub canvas_course_id: Option, + /// Canvas assignment id. + #[serde(default)] + pub canvas_assignment_id: Option, + /// Which attempt to grade when the source reports several. + #[serde(default)] + pub attempt_policy: AttemptPolicy, } fn default_tests_dir() -> String { diff --git a/crates/scriptmark/src/models/result.rs b/crates/scriptmark/src/models/result.rs index 1aabfa4..b4e122f 100644 --- a/crates/scriptmark/src/models/result.rs +++ b/crates/scriptmark/src/models/result.rs @@ -1,5 +1,7 @@ use serde::{Deserialize, Serialize}; +use crate::models::SubmissionOutcome; + /// Status of a single test case or an overall student report. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] @@ -34,10 +36,13 @@ pub struct CaseResult { pub elapsed_ms: Option, } -/// Aggregated result for one test spec (one TOML file) for one student. +/// Aggregated result for one grading item for one student. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct TestResult { - pub spec_name: String, + /// The [`crate::models::GradingItem`] this evidence belongs to — the test spec's + /// `[meta] name`. Read from `spec_name` in results written before items were modelled. + #[serde(alias = "spec_name")] + pub item_id: String, pub cases: Vec, } @@ -77,11 +82,17 @@ impl TestResult { } /// Complete report for a single student across all test specs. -#[derive(Debug, Clone, Serialize, Deserialize)] +/// +/// New fields are `Option` rather than defaulted enums: a results file written before this +/// model existed must not claim a `submission_state` it never recorded. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] pub struct StudentReport { + /// `StudentKey`'s rendering — a bare 学号, or a `canvas:` / `local:` prefixed form that + /// can never be mistaken for one. pub student_id: String, #[serde(default)] pub student_name: Option, + #[serde(default)] pub test_results: Vec, #[serde(default)] pub final_grade: Option, @@ -90,6 +101,31 @@ pub struct StudentReport { /// Lint-based style score (0-100). Set by linter, used by grading. #[serde(default)] pub lint_score: Option, + /// Canvas user id, kept separate from `student_id` so grade push never has to guess it + /// by parsing the student id as an integer. + #[serde(default)] + pub canvas_user_id: Option, + /// How the submission arrived. `None` on records written before this field existed. + #[serde(default)] + pub submission_state: Option, + /// An infrastructure failure that stopped this student being graded at all — a panicked + /// task, not a wrong answer. Kept out of `test_results` so it can never be counted as a + /// failed test case and scored. + #[serde(default)] + pub error: Option, +} + +impl StudentReport { + /// True when the student actually had runnable code and nothing went wrong running it — + /// the only case a numeric grade means anything. Reports from before these fields + /// existed are graded as they were. + pub fn is_gradeable(&self) -> bool { + self.error.is_none() + && matches!( + self.submission_state, + None | Some(SubmissionOutcome::Executable) + ) + } } impl StudentReport { diff --git a/crates/scriptmark/src/models/submission.rs b/crates/scriptmark/src/models/submission.rs index d2b9ef4..38ffcb8 100644 --- a/crates/scriptmark/src/models/submission.rs +++ b/crates/scriptmark/src/models/submission.rs @@ -1,43 +1,1085 @@ -use std::collections::HashMap; +//! The unified input model. +//! +//! Both entry points — Canvas ([`crate::input::canvas`]) and local +//! ([`crate::discovery`]) — produce an [`AssignmentInput`], and everything downstream +//! reads only from it. Entry-specific material (Canvas workflow states, attachment URLs, +//! spreadsheet coordinates) is kept in clearly marked optional side fields so that it +//! never reaches the scoring path. + +use std::fmt; use std::path::PathBuf; use serde::{Deserialize, Serialize}; -/// A single file belonging to a student's submission. -#[derive(Debug, Clone, Serialize, Deserialize)] +use crate::models::config::AttemptPolicy; +use crate::roster::Roster; + +/// The only normalisation applied to an identity key. +/// +/// Strips a UTF-8 BOM and surrounding whitespace. Deliberately does *not* case-fold, +/// parse as a number, or touch zero padding — `"012345"` and `"12345"` are different +/// students and must stay that way. +pub fn normalize_key(raw: &str) -> String { + raw.strip_prefix('\u{feff}') + .unwrap_or(raw) + .trim() + .to_string() +} + +/// Strip leading zeros, for *comparison only*. Never used to build a key. +pub fn zero_stripped(key: &str) -> &str { + let trimmed = key.trim_start_matches('0'); + if trimmed.is_empty() { key } else { trimmed } +} + +/// The total identity key for a student. +/// +/// Every student in an [`AssignmentInput`] has exactly one, so a student can never be +/// keyless. The `Display` form is what lands in `StudentReport.student_id`, the database +/// and the CSV export — the prefixes make a Canvas id or an unconfirmed local token +/// impossible to mistake for a 学号. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum StudentKey { + /// A confirmed 学号 — matched in the roster, or taken from a Canvas `sis_user_id`. + Number(String), + /// A Canvas user carrying no SIS id. + CanvasUser(u64), + /// A token pulled off a local filename that no roster confirms. + Extracted(String), +} + +impl fmt::Display for StudentKey { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Number(number) => write!(f, "{number}"), + Self::CanvasUser(id) => write!(f, "canvas:{id}"), + Self::Extracted(token) => write!(f, "local:{token}"), + } + } +} + +/// Prefixes [`StudentKey`]'s `Display` uses to mark a Canvas-native or unconfirmed key. +/// A 学号 may not begin with one, or the rendering would stop being reversible. +pub const RESERVED_KEY_PREFIXES: [&str; 2] = ["local:", "canvas:"]; + +/// Whether this text could be mistaken for a rendered key of another kind. +pub fn is_reserved_key(text: &str) -> bool { + RESERVED_KEY_PREFIXES + .iter() + .any(|prefix| text.starts_with(prefix)) +} + +impl StudentKey { + /// The inverse of [`fmt::Display`], for reading a key back out of a results file, a + /// database row or a CSV column. + pub fn parse(rendered: &str) -> Self { + if let Some(id) = rendered.strip_prefix("canvas:") + && let Ok(id) = id.parse::() + { + return Self::CanvasUser(id); + } + match rendered.strip_prefix("local:") { + Some(token) => Self::Extracted(normalize_key(token)), + None => Self::Number(normalize_key(rendered)), + } + } + + /// The raw text behind the key, without the `Display` prefix. + pub fn raw(&self) -> String { + match self { + Self::Number(number) => number.clone(), + Self::CanvasUser(id) => id.to_string(), + Self::Extracted(token) => token.clone(), + } + } +} + +/// Who a submission belongs to. The separate id fields are kept apart on purpose: +/// 学号, Canvas user id, SIS id and login id are four different things. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct StudentIdentity { + pub key: StudentKey, + #[serde(default)] + pub student_number: Option, + #[serde(default)] + pub canvas_user_id: Option, + #[serde(default)] + pub sis_user_id: Option, + #[serde(default)] + pub login_id: Option, + #[serde(default)] + pub name: Option, + #[serde(default)] + pub sortable_name: Option, + #[serde(default)] + pub email: Option, +} + +impl StudentIdentity { + fn bare(key: StudentKey) -> Self { + Self { + key, + student_number: None, + canvas_user_id: None, + sis_user_id: None, + login_id: None, + name: None, + sortable_name: None, + email: None, + } + } + + /// A student identified by a confirmed 学号. + pub fn number(student_number: impl Into) -> Self { + let number = normalize_key(&student_number.into()); + Self { + student_number: Some(number.clone()), + ..Self::bare(StudentKey::Number(number)) + } + } + + /// A Canvas user with no SIS id to key on. + pub fn canvas_user(canvas_user_id: u64) -> Self { + Self { + canvas_user_id: Some(canvas_user_id), + ..Self::bare(StudentKey::CanvasUser(canvas_user_id)) + } + } + + /// A token extracted from a local filename, confirmed by nothing. + pub fn extracted(token: impl Into) -> Self { + let token = normalize_key(&token.into()); + Self::bare(StudentKey::Extracted(token)) + } + + /// Promote an unconfirmed token to a confirmed 学号 once a roster vouches for it. + pub fn confirm_number(&mut self) { + if let StudentKey::Extracted(token) = &self.key { + let number = token.clone(); + self.key = StudentKey::Number(number.clone()); + self.student_number = Some(number); + } + } +} + +/// A file a student uploaded, as the source describes it. Becomes a [`StudentFile`] only +/// once the bytes are actually on disk. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct Attachment { + pub id: u64, + pub filename: String, + #[serde(default)] + pub content_type: Option, + #[serde(default)] + pub size: Option, + #[serde(default)] + pub url: Option, +} + +/// Source-reported state for one attempt. Canvas fills this; local input leaves it `None` +/// rather than fabricating `late: false`, which would be a claim rather than a default. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct SourceStatus { + pub workflow_state: String, + #[serde(default)] + pub late: bool, + #[serde(default)] + pub missing: bool, + #[serde(default)] + pub excused: bool, +} + +/// Where a [`StudentFile`] came from, so every graded byte traces back to its origin. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum FileOrigin { + /// Found directly in a scanned directory. + #[default] + Direct, + /// Extracted from an archive. `entry` is the path *inside* the archive. + Archive { archive: PathBuf, entry: String }, + /// Downloaded from a Canvas attachment. + Attachment { attempt: u32, attachment_id: u64 }, +} + +/// A single runnable file belonging to a student's submission. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct StudentFile { pub path: PathBuf, pub language: String, + #[serde(default)] + pub origin: FileOrigin, +} + +impl StudentFile { + /// A file found directly on disk, with no archive or attachment behind it. + pub fn direct(path: impl Into, language: impl Into) -> Self { + Self { + path: path.into(), + language: language.into(), + origin: FileOrigin::Direct, + } + } + + pub fn with_origin(mut self, origin: FileOrigin) -> Self { + self.origin = origin; + self + } + + pub fn file_name(&self) -> String { + self.path + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_default() + } } -/// All student submissions for a grading session, grouped by student ID. -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct SubmissionSet { - /// student_id -> list of files - pub by_student: HashMap>, +/// One submission attempt. Local input produces exactly one; Canvas may produce several. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct SubmissionAttempt { + pub attempt: u32, + #[serde(default)] + pub submitted_at: Option, + #[serde(default)] + pub source_status: Option, + /// What the source says was uploaded. + #[serde(default)] + pub attachments: Vec, + /// What is actually on disk and runnable. + #[serde(default)] + pub files: Vec, } -impl SubmissionSet { - pub fn student_ids(&self) -> Vec { - let mut ids: Vec = self.by_student.keys().cloned().collect(); - ids.sort(); - ids +impl SubmissionAttempt { + pub fn new(attempt: u32) -> Self { + Self { + attempt, + submitted_at: None, + source_status: None, + attachments: Vec::new(), + files: Vec::new(), + } + } + + pub fn with_files(mut self, files: Vec) -> Self { + self.files = files; + self + } +} + +/// How the material was delivered. Orthogonal to [`RosterMatch`] — keeping the two axes +/// apart is what stops `(ReceivedUnmatched, Matched)` from being representable. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum SubmissionState { + /// On the roster, nothing received. + NotSubmitted, + /// Material received, but nothing runnable in it. + SubmittedEmpty, + /// At least one file in a recognised language. + Executable, +} + +/// Whether the submitter could be tied to a roster entry. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum RosterMatch { + /// Index into [`Roster::entries`]. + Matched(usize), + NotInRoster, + /// No roster was supplied at all, so membership is simply unknown. + NoRoster, +} + +/// The four outcomes the ticket requires be distinguishable. Computed from the two stored +/// axes rather than stored, so the two can never disagree. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum SubmissionOutcome { + NotSubmitted, + SubmittedEmpty, + ReceivedUnmatched, + Executable, +} + +/// Everything known about one student's participation in one assignment. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct StudentSubmission { + pub identity: StudentIdentity, + pub roster_match: RosterMatch, + pub state: SubmissionState, + pub attempts: Vec, + /// Index into `attempts`, chosen by [`AttemptPolicy`]. `None` iff nothing was received. + pub selected: Option, +} + +impl StudentSubmission { + /// A roster student from whom nothing arrived. + /// + /// Always `Matched`, never `NotInRoster`: a non-submitter only exists because a roster + /// vouches for them. + pub fn not_submitted(identity: StudentIdentity, roster_index: usize) -> Self { + Self { + identity, + roster_match: RosterMatch::Matched(roster_index), + state: SubmissionState::NotSubmitted, + attempts: Vec::new(), + selected: None, + } + } + + /// A student with received material. `state` is derived from the selected attempt, so + /// it can never contradict the files. + pub fn received( + identity: StudentIdentity, + roster_match: RosterMatch, + attempts: Vec, + policy: AttemptPolicy, + ) -> Self { + debug_assert!( + !attempts.is_empty(), + "received() needs at least one attempt; a student with nothing to show for \ + themselves is either not_submitted() or has one empty attempt" + ); + let selected = select_attempt(&attempts, policy); + let state = match selected.and_then(|i| attempts.get(i)) { + Some(attempt) if !attempt.files.is_empty() => SubmissionState::Executable, + Some(_) => SubmissionState::SubmittedEmpty, + None => SubmissionState::NotSubmitted, + }; + Self { + identity, + roster_match, + state, + attempts, + selected, + } + } + + /// One student, one attempt, a list of Python files — the shape most callers and tests + /// want when they already know who the files belong to. + pub fn from_files( + student_number: impl Into, + paths: &[impl AsRef], + ) -> Self { + let files: Vec = paths + .iter() + .map(|p| StudentFile::direct(p.as_ref().to_path_buf(), "python")) + .collect(); + Self::received( + StudentIdentity::number(student_number), + RosterMatch::NoRoster, + vec![SubmissionAttempt::new(1).with_files(files)], + AttemptPolicy::Latest, + ) + } + + pub fn key(&self) -> &StudentKey { + &self.identity.key + } + + pub fn selected_attempt(&self) -> Option<&SubmissionAttempt> { + self.selected.and_then(|i| self.attempts.get(i)) + } + + /// The runnable files of the selected attempt. Reading through `selected` means the + /// files can never belong to a different attempt than the one that was chosen. + pub fn files(&self) -> &[StudentFile] { + self.selected_attempt() + .map(|a| a.files.as_slice()) + .unwrap_or(&[]) + } + + pub fn outcome(&self) -> SubmissionOutcome { + if self.state != SubmissionState::NotSubmitted + && matches!(self.roster_match, RosterMatch::NotInRoster) + { + return SubmissionOutcome::ReceivedUnmatched; + } + match self.state { + SubmissionState::NotSubmitted => SubmissionOutcome::NotSubmitted, + SubmissionState::SubmittedEmpty => SubmissionOutcome::SubmittedEmpty, + SubmissionState::Executable => SubmissionOutcome::Executable, + } + } + + /// Names of the artifacts attributed to this student, whichever source they came from: + /// attachment names when the source reported any, on-disk basenames otherwise. + pub fn artifact_names(&self) -> Vec { + let Some(attempt) = self.selected_attempt() else { + return Vec::new(); + }; + let mut names: Vec = if attempt.attachments.is_empty() { + attempt.files.iter().map(StudentFile::file_name).collect() + } else { + attempt + .attachments + .iter() + .map(|a| a.filename.clone()) + .collect() + }; + names.sort(); + names + } + + /// Total ordering key: independent of `read_dir` order, and total even for duplicates. + fn sort_key(&self) -> (StudentKey, Option, PathBuf) { + ( + self.identity.key.clone(), + self.identity.canvas_user_id, + self.files() + .first() + .map(|f| f.path.clone()) + .unwrap_or_default(), + ) + } +} + +fn select_attempt(attempts: &[SubmissionAttempt], policy: AttemptPolicy) -> Option { + if attempts.is_empty() { + return None; + } + let pick = match policy { + // Highest attempt number wins. `submitted_at` is only a tie-break: the workspace + // has no date library, so comparing timestamps means a lexicographic string + // compare — correct for uniform UTC and silently wrong otherwise. + AttemptPolicy::Latest => attempts.iter().enumerate().max_by(|(_, a), (_, b)| { + a.attempt + .cmp(&b.attempt) + .then(a.submitted_at.cmp(&b.submitted_at)) + }), + AttemptPolicy::Earliest => attempts.iter().enumerate().min_by(|(_, a), (_, b)| { + a.attempt + .cmp(&b.attempt) + .then(a.submitted_at.cmp(&b.submitted_at)) + }), + }; + pick.map(|(i, _)| i) +} + +/// Material that arrived but could not be attributed to any student. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +pub struct UnmatchedArtifact { + pub path: PathBuf, + pub reason: UnmatchedReason, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum UnmatchedReason { + /// No student key could be extracted from the filename. + NoStudentKey, + /// A keyless file in a format no backend runs. + UnsupportedType, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum DiagnosticSeverity { + Info, + Warning, + Error, +} + +/// Anomalies found while building the input. Data, not a pre-rendered string: the text is +/// derived from the fields so the two cannot drift. +#[derive( + Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize, thiserror::Error, +)] +#[serde(rename_all = "snake_case")] +pub enum DiagnosticKind { + #[error("roster lists '{key}' in {count} identical rows; merged into one")] + DuplicateRosterEntry { key: String, count: usize }, + #[error("roster rows disagree about who '{key}' is: {detail}")] + ConflictingRosterEntry { key: String, detail: String }, + #[error("student numbers differ only by zero padding: {keys:?}; kept distinct")] + SuspectedZeroPaddedVariant { keys: Vec }, + #[error("Canvas user {canvas_user_id} has no sis_user_id; keyed by Canvas id")] + MissingStudentNumber { canvas_user_id: u64 }, + #[error("'{key}' submitted but is not on the roster")] + NotOnRoster { key: String }, + #[error("roster row unusable ({reason}); the student it names is not in the input")] + UnusableRosterRow { reason: String }, + #[error("ignored '{path}' for '{key}': not a supported submission file")] + IgnoredFile { key: String, path: PathBuf }, + #[error("archive '{archive}' entry '{entry}' collides with an already extracted name")] + ArchiveNameCollision { archive: PathBuf, entry: String }, + #[error("archive '{archive}' could not be read: {reason}")] + ArchiveUnreadable { archive: PathBuf, reason: String }, + #[error("archive '{archive}' entry '{entry}' skipped: {reason}")] + ArchiveEntrySkipped { + archive: PathBuf, + entry: String, + reason: String, + }, + #[error("attachment {attachment_id} ('{filename}') has not been downloaded")] + PendingDownload { + attachment_id: u64, + filename: String, + }, + #[error("'{key}' submitted a text entry with no gradeable file")] + TextEntryOnly { key: String }, + #[error( + "Canvas reported more than one submission row for user {canvas_user_id}; kept the first" + )] + DuplicateSubmissionRow { canvas_user_id: u64 }, + #[error("'{key}' is on the supplied roster but is not enrolled in the Canvas course")] + NotEnrolled { key: String }, + #[error("could not read an entry of '{dir}': {reason}")] + UnreadableDirEntry { dir: PathBuf, reason: String }, +} + +/// Where in the source an anomaly was found. `sheet`/`row` are for the spreadsheet +/// importer (P-672); the local and Canvas adapters only set `file`. +#[derive(Debug, Clone, Default, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +pub struct SourceLocation { + #[serde(default)] + pub file: Option, + #[serde(default)] + pub sheet: Option, + #[serde(default)] + pub row: Option, +} + +impl SourceLocation { + pub fn file(path: impl Into) -> Self { + Self { + file: Some(path.into()), + ..Self::default() + } + } + + pub fn row(path: impl Into, row: usize) -> Self { + Self { + file: Some(path.into()), + sheet: None, + row: Some(row), + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +pub struct InputDiagnostic { + pub severity: DiagnosticSeverity, + pub kind: DiagnosticKind, + #[serde(default)] + pub location: Option, +} + +impl InputDiagnostic { + pub fn warning(kind: DiagnosticKind) -> Self { + Self { + severity: DiagnosticSeverity::Warning, + kind, + location: None, + } + } + + pub fn info(kind: DiagnosticKind) -> Self { + Self { + severity: DiagnosticSeverity::Info, + kind, + location: None, + } + } + + /// Something the run cannot sensibly continue past. The CLI refuses to grade while any + /// of these is present rather than producing results nobody should trust. + pub fn error(kind: DiagnosticKind) -> Self { + Self { + severity: DiagnosticSeverity::Error, + kind, + location: None, + } + } + + pub fn at(mut self, location: SourceLocation) -> Self { + self.location = Some(location); + self + } +} + +impl fmt::Display for InputDiagnostic { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.kind) + } +} + +/// Which entry point produced an input. Recorded once, on the input as a whole — a +/// per-student copy would let a `Local` input hold Canvas-sourced students. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum InputSource { + Canvas { + #[serde(default)] + course_id: Option, + #[serde(default)] + assignment_id: Option, + }, + Local { + scanned_dirs: Vec, + #[serde(default)] + roster_path: Option, + }, +} + +/// One thing a student is marked on. +/// +/// `id` is the test spec's `[meta] name`, which is what `TestResult.item_id` carries — so +/// the assignment's declared items and the results reference the same identity rather than +/// two parallel notions of "a question". Scores, weights and how evidence inside an item +/// aggregates are P-677's; this is identity only. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +pub struct GradingItem { + pub id: String, + #[serde(default)] + pub title: Option, +} + +impl GradingItem { + pub fn new(id: impl Into) -> Self { + Self { + id: id.into(), + title: None, + } + } + + /// What to show a human: the title when the teacher gave one, else the id. + pub fn label(&self) -> &str { + self.title.as_deref().unwrap_or(&self.id) + } +} + +/// Assignment identity. Course id and assignment id are stored apart from the name, and +/// apart from any student identity. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct Assignment { + pub name: String, + #[serde(default)] + pub canvas_course_id: Option, + #[serde(default)] + pub canvas_assignment_id: Option, + /// The items this assignment is marked on, in declaration order. + #[serde(default)] + pub items: Vec, +} + +impl Assignment { + pub fn named(name: impl Into) -> Self { + Self { + name: name.into(), + ..Self::default() + } + } + + pub fn with_items(mut self, items: Vec) -> Self { + self.items = items; + self + } + + pub fn item(&self, id: &str) -> Option<&GradingItem> { + self.items.iter().find(|item| item.id == id) + } +} + +/// The unified contract. Whatever the entry point, this is what downstream reads. +/// +/// Binding files and functions to items is P-673; per-item scoring is P-677. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct AssignmentInput { + pub assignment: Assignment, + pub source: InputSource, + #[serde(default)] + pub roster: Option, + pub students: Vec, + #[serde(default)] + pub unmatched: Vec, + #[serde(default)] + pub diagnostics: Vec, +} + +/// The cross-source comparison shape. Canvas carries user ids and attempt timestamps that +/// local input simply does not have, so equivalence is asserted on this projection rather +/// than on the whole struct. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +pub struct ProjectedStudent { + pub key: String, + pub outcome: SubmissionOutcome, + pub artifacts: Vec, +} + +impl AssignmentInput { + pub fn new(assignment: Assignment, source: InputSource) -> Self { + Self { + assignment, + source, + roster: None, + students: Vec::new(), + unmatched: Vec::new(), + diagnostics: Vec::new(), + } + } + + /// Impose a total order on every vector. `read_dir` order is not stable across + /// filesystems, so without this any assertion on the output is flaky in CI. + pub fn sorted(mut self) -> Self { + self.students.sort_by_key(StudentSubmission::sort_key); + self.unmatched.sort(); + self.diagnostics + .sort_by(|a, b| (&a.location, &a.kind).cmp(&(&b.location, &b.kind))); + self } pub fn student_count(&self) -> usize { - self.by_student.len() + self.students.len() + } + + pub fn with_outcome( + &self, + outcome: SubmissionOutcome, + ) -> impl Iterator { + self.students.iter().filter(move |s| s.outcome() == outcome) } + /// Distinct languages across every selected attempt. pub fn languages(&self) -> Vec { let mut langs: Vec = self - .by_student - .values() - .flatten() + .students + .iter() + .flat_map(|s| s.files()) .map(|f| f.language.clone()) - .collect::>() - .into_iter() .collect(); langs.sort(); + langs.dedup(); langs } + + /// The cross-source comparison shape. + /// + /// Keyed on [`StudentKey::raw`], not its `Display` form: whether a given student number + /// counts as *confirmed* is a property of the source — Canvas vouches for its own SIS + /// ids, a local filename token vouches for nothing — so comparing the prefixed form + /// would report a difference in confidence as a difference in identity. + pub fn projection(&self) -> Vec { + let mut projected: Vec = self + .students + .iter() + .map(|s| ProjectedStudent { + key: s.identity.key.raw(), + outcome: s.outcome(), + artifacts: s.artifact_names(), + }) + .collect(); + projected.sort(); + projected + } + + pub fn diagnostics_of( + &self, + severity: DiagnosticSeverity, + ) -> impl Iterator { + self.diagnostics + .iter() + .filter(move |d| d.severity == severity) + } + + /// Anything that makes the input untrustworthy to grade from. + pub fn errors(&self) -> impl Iterator { + self.diagnostics_of(DiagnosticSeverity::Error) + } + + /// One `SuspectedZeroPaddedVariant` per group of keys that differ only by zero + /// padding. Keys are never merged — this only flags that an upstream export may have + /// stripped the padding. + pub fn detect_zero_padded_variants(&self) -> Vec { + let mut buckets: std::collections::BTreeMap> = Default::default(); + for student in &self.students { + // Only keys that denote a student number are comparable. A Canvas id is a + // separate namespace, so `CanvasUser(123)` beside `Number("00123")` is not a + // padding variant — warning about it would be a false positive. + let raw = match &student.identity.key { + StudentKey::Number(number) => number.clone(), + StudentKey::Extracted(token) => token.clone(), + StudentKey::CanvasUser(_) => continue, + }; + buckets + .entry(zero_stripped(&raw).to_string()) + .or_default() + .push(raw); + } + buckets + .into_values() + .filter_map(|mut keys| { + keys.sort(); + keys.dedup(); + (keys.len() > 1).then(|| { + InputDiagnostic::warning(DiagnosticKind::SuspectedZeroPaddedVariant { keys }) + }) + }) + .collect() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn executable_attempt(n: u32, file: &str) -> SubmissionAttempt { + SubmissionAttempt::new(n).with_files(vec![StudentFile::direct(file, "python")]) + } + + #[test] + fn test_key_display_cannot_be_confused_with_a_number() { + assert_eq!(StudentKey::Number("012345".into()).to_string(), "012345"); + assert_eq!(StudentKey::CanvasUser(12345).to_string(), "canvas:12345"); + assert_eq!( + StudentKey::Extracted("notes".into()).to_string(), + "local:notes" + ); + } + + #[test] + fn test_leading_zeros_are_never_stripped_from_a_key() { + let identity = StudentIdentity::number("0024010003"); + assert_eq!(identity.key, StudentKey::Number("0024010003".into())); + assert_eq!(identity.student_number.as_deref(), Some("0024010003")); + assert_ne!(identity.key, StudentIdentity::number("24010003").key); + } + + #[test] + fn test_normalize_key_trims_and_strips_bom_only() { + assert_eq!(normalize_key("\u{feff} 2024010001 "), "2024010001"); + assert_eq!(normalize_key("0024010003"), "0024010003"); + } + + #[test] + fn test_not_submitted_is_always_roster_matched() { + let s = StudentSubmission::not_submitted(StudentIdentity::number("2024010004"), 3); + assert_eq!(s.state, SubmissionState::NotSubmitted); + assert_eq!(s.roster_match, RosterMatch::Matched(3)); + assert_eq!(s.outcome(), SubmissionOutcome::NotSubmitted); + assert!(s.files().is_empty()); + } + + #[test] + fn test_outcome_separates_all_four_cases() { + let executable = StudentSubmission::received( + StudentIdentity::number("1"), + RosterMatch::Matched(0), + vec![executable_attempt(1, "1_lab.py")], + AttemptPolicy::Latest, + ); + assert_eq!(executable.outcome(), SubmissionOutcome::Executable); + + let empty = StudentSubmission::received( + StudentIdentity::number("2"), + RosterMatch::Matched(1), + vec![SubmissionAttempt::new(1)], + AttemptPolicy::Latest, + ); + assert_eq!(empty.outcome(), SubmissionOutcome::SubmittedEmpty); + + // Not on the roster wins over the delivery axis, whether or not the files run. + let unmatched_runnable = StudentSubmission::received( + StudentIdentity::extracted("9999"), + RosterMatch::NotInRoster, + vec![executable_attempt(1, "9999_lab.py")], + AttemptPolicy::Latest, + ); + assert_eq!( + unmatched_runnable.outcome(), + SubmissionOutcome::ReceivedUnmatched + ); + + let unmatched_empty = StudentSubmission::received( + StudentIdentity::extracted("8888"), + RosterMatch::NotInRoster, + vec![SubmissionAttempt::new(1)], + AttemptPolicy::Latest, + ); + assert_eq!( + unmatched_empty.outcome(), + SubmissionOutcome::ReceivedUnmatched + ); + + let absent = StudentSubmission::not_submitted(StudentIdentity::number("4"), 0); + assert_eq!(absent.outcome(), SubmissionOutcome::NotSubmitted); + } + + #[test] + fn test_latest_attempt_wins_and_files_follow_the_selection() { + let student = StudentSubmission::received( + StudentIdentity::number("2024010002"), + RosterMatch::Matched(0), + vec![ + executable_attempt(1, "first.py"), + executable_attempt(2, "second.py"), + ], + AttemptPolicy::Latest, + ); + assert_eq!(student.selected_attempt().unwrap().attempt, 2); + assert_eq!(student.files()[0].file_name(), "second.py"); + + // Order in the payload must not decide the selection. + let reversed = StudentSubmission::received( + StudentIdentity::number("2024010002"), + RosterMatch::Matched(0), + vec![ + executable_attempt(2, "second.py"), + executable_attempt(1, "first.py"), + ], + AttemptPolicy::Latest, + ); + assert_eq!(reversed.selected_attempt().unwrap().attempt, 2); + assert_eq!(reversed.files()[0].file_name(), "second.py"); + } + + #[test] + fn test_earliest_attempt_policy() { + let student = StudentSubmission::received( + StudentIdentity::number("2024010002"), + RosterMatch::Matched(0), + vec![ + executable_attempt(2, "second.py"), + executable_attempt(1, "first.py"), + ], + AttemptPolicy::Earliest, + ); + assert_eq!(student.selected_attempt().unwrap().attempt, 1); + } + + #[test] + fn test_artifact_names_prefer_attachments_then_fall_back_to_files() { + let mut attempt = executable_attempt(1, "/tmp/2024010001_lab1.py"); + assert_eq!( + StudentSubmission::received( + StudentIdentity::number("2024010001"), + RosterMatch::Matched(0), + vec![attempt.clone()], + AttemptPolicy::Latest, + ) + .artifact_names(), + vec!["2024010001_lab1.py"] + ); + + attempt.attachments.push(Attachment { + id: 1, + filename: "lab1.py".into(), + content_type: None, + size: None, + url: None, + }); + assert_eq!( + StudentSubmission::received( + StudentIdentity::number("2024010001"), + RosterMatch::Matched(0), + vec![attempt], + AttemptPolicy::Latest, + ) + .artifact_names(), + vec!["lab1.py"] + ); + } + + #[test] + fn test_zero_padded_variants_are_flagged_but_never_merged() { + let mut input = AssignmentInput::new( + Assignment::named("hw1"), + InputSource::Local { + scanned_dirs: vec![], + roster_path: None, + }, + ); + input.students = vec![ + StudentSubmission::not_submitted(StudentIdentity::number("0024010003"), 0), + StudentSubmission::not_submitted(StudentIdentity::number("24010003"), 1), + StudentSubmission::not_submitted(StudentIdentity::number("2024010001"), 2), + ]; + + let found = input.detect_zero_padded_variants(); + assert_eq!(found.len(), 1); + assert!(matches!( + &found[0].kind, + DiagnosticKind::SuspectedZeroPaddedVariant { keys } if keys == &["0024010003", "24010003"] + )); + // Both survive as separate students. + assert_eq!(input.students.len(), 3); + } + + #[test] + fn test_a_canvas_id_is_not_a_zero_padding_variant_of_a_student_number() { + let mut input = AssignmentInput::new( + Assignment::named("hw1"), + InputSource::Canvas { + course_id: None, + assignment_id: None, + }, + ); + input.students = vec![ + StudentSubmission::not_submitted(StudentIdentity::number("00123"), 0), + StudentSubmission::not_submitted(StudentIdentity::canvas_user(123), 1), + ]; + + // Separate namespaces — comparing their padding would be a false positive. + assert!(input.detect_zero_padded_variants().is_empty()); + } + + #[test] + fn test_reserved_prefixes_are_recognised() { + assert!(is_reserved_key("local:alice")); + assert!(is_reserved_key("canvas:5")); + assert!(!is_reserved_key("2024010001")); + assert!(!is_reserved_key("localhost")); + } + + #[test] + fn test_confirm_number_promotes_an_extracted_token() { + let mut identity = StudentIdentity::extracted("2024010001"); + assert!(identity.student_number.is_none()); + identity.confirm_number(); + assert_eq!(identity.key, StudentKey::Number("2024010001".into())); + assert_eq!(identity.student_number.as_deref(), Some("2024010001")); + } + + #[test] + fn test_grading_item_identity_is_the_spec_name() { + let assignment = Assignment::named("hw1").with_items(vec![ + GradingItem::new("find_larger_number"), + GradingItem { + id: "sum_pair".to_string(), + title: Some("第二题 求和".to_string()), + }, + ]); + + // A result references an item by the same id a test spec's [meta] name carries. + let result = crate::models::TestResult { + item_id: "sum_pair".to_string(), + cases: vec![], + }; + let item = assignment.item(&result.item_id).expect("declared item"); + assert_eq!(item.label(), "第二题 求和"); + // Without a title a human still gets something meaningful. + assert_eq!( + assignment.item("find_larger_number").unwrap().label(), + "find_larger_number" + ); + assert!(assignment.item("nope").is_none()); + } + + #[test] + fn test_results_written_as_spec_name_still_load() { + // The field was called `spec_name` before items were modelled. + let legacy = r#"{"spec_name": "find_larger_number", "cases": []}"#; + let result: crate::models::TestResult = serde_json::from_str(legacy).unwrap(); + assert_eq!(result.item_id, "find_larger_number"); + } + + #[test] + fn test_new_enums_serialise_snake_case() { + let json = serde_json::to_string(&SubmissionOutcome::ReceivedUnmatched).unwrap(); + assert_eq!(json, "\"received_unmatched\""); + let json = serde_json::to_string(&FileOrigin::Direct).unwrap(); + assert_eq!(json, "\"direct\""); + } } diff --git a/crates/scriptmark/src/report_template.html b/crates/scriptmark/src/report_template.html index 5d4e7c1..83f5006 100644 --- a/crates/scriptmark/src/report_template.html +++ b/crates/scriptmark/src/report_template.html @@ -107,15 +107,17 @@

ScriptMark Report

r._passed = (r.test_results||[]).reduce((s,t) => s + (t.cases||[]).filter(c => c.status==='passed').length, 0); r._failed = r._total - r._passed; r._rate = r._total > 0 ? (r._passed / r._total * 100) : 0; - r._grade = r.final_grade || 0; - r._status = r._total === 0 ? 'missing' : r._failed > 0 ? 'failed' : 'passed'; + r._graded = r.final_grade !== null && r.final_grade !== undefined; + r._grade = r._graded ? r.final_grade : null; + r._status = r.error ? 'error' : r._total === 0 ? 'missing' : r._failed > 0 ? 'failed' : 'passed'; }); const el = id => document.getElementById(id); const esc = s => String(s).replace(/&/g,'&').replace(//g,'>').replace(/"/g,'"'); const totalStudents = REPORTS.length; const passedStudents = REPORTS.filter(r => r._status==='passed').length; -const avgGrade = REPORTS.reduce((s,r) => s+r._grade, 0) / Math.max(totalStudents,1); +const gradedReports = REPORTS.filter(r => r._graded); +const avgGrade = gradedReports.reduce((s,r) => s+r._grade, 0) / Math.max(gradedReports.length,1); const totalCases = REPORTS.reduce((s,r) => s+r._total, 0); const totalPassed = REPORTS.reduce((s,r) => s+r._passed, 0); @@ -148,7 +150,7 @@

ScriptMark Report

const sp=document.createElement('span'); sp.textContent=k; l.appendChild(sp); }); } -drawChart('grade-chart','grade-labels',REPORTS.map(r=>r._grade),5,100,k=>k>=90?'var(--green)':k>=70?'var(--accent)':k>=60?'var(--yellow)':'var(--red)'); +drawChart('grade-chart','grade-labels',gradedReports.map(r=>r._grade),5,100,k=>k>=90?'var(--green)':k>=70?'var(--accent)':k>=60?'var(--yellow)':'var(--red)'); drawChart('pass-chart','pass-labels',REPORTS.map(r=>r._rate),10,100,k=>k>=80?'var(--green)':k>=50?'var(--yellow)':'var(--red)'); let sortKey='grade', sortDir=-1; @@ -156,15 +158,15 @@

ScriptMark Report

let d=[...REPORTS]; if(filter){const f=filter.toLowerCase();d=d.filter(r=>r.student_id.includes(f)||(r.student_name||'').toLowerCase().includes(f));} d.sort((a,b)=>{ - const va=sortKey==='name'?(a.student_name||''):sortKey==='id'?a.student_id:sortKey==='grade'?a._grade:sortKey==='rate'?a._rate:sortKey==='passed'?a._passed:sortKey==='failed'?a._failed:sortKey==='total'?a._total:a._status; - const vb=sortKey==='name'?(b.student_name||''):sortKey==='id'?b.student_id:sortKey==='grade'?b._grade:sortKey==='rate'?b._rate:sortKey==='passed'?b._passed:sortKey==='failed'?b._failed:sortKey==='total'?b._total:b._status; + const va=sortKey==='name'?(a.student_name||''):sortKey==='id'?a.student_id:sortKey==='grade'?(a._graded?a._grade:-1):sortKey==='rate'?a._rate:sortKey==='passed'?a._passed:sortKey==='failed'?a._failed:sortKey==='total'?a._total:a._status; + const vb=sortKey==='name'?(b.student_name||''):sortKey==='id'?b.student_id:sortKey==='grade'?(b._graded?b._grade:-1):sortKey==='rate'?b._rate:sortKey==='passed'?b._passed:sortKey==='failed'?b._failed:sortKey==='total'?b._total:b._status; return typeof va==='number'?(va-vb)*sortDir:String(va).localeCompare(String(vb))*sortDir; }); const tbody=el('student-table'); tbody.textContent=''; d.forEach(r=>{ const tr=document.createElement('tr'); tr.dataset.sid=r.student_id; - const gc=r._grade>=90?'var(--green)':r._grade>=70?'var(--accent)':r._grade>=60?'var(--yellow)':'var(--red)'; - [{t:r.student_name||'N/A',c:'name-col'},{t:r.student_id},{t:r._status,badge:true},{t:r._passed},{t:r._failed},{t:r._total},{t:r._rate.toFixed(1)+'%'},{t:r._grade.toFixed(1),style:`color:${gc};font-weight:700`}].forEach(col=>{ + const gc=!r._graded?'var(--text-dim)':r._grade>=90?'var(--green)':r._grade>=70?'var(--accent)':r._grade>=60?'var(--yellow)':'var(--red)'; + [{t:r.student_name||'N/A',c:'name-col'},{t:r.student_id},{t:r._status,badge:true},{t:r._passed},{t:r._failed},{t:r._total},{t:r._rate.toFixed(1)+'%'},{t:r._graded?r._grade.toFixed(1):'—',style:`color:${gc};font-weight:700`}].forEach(col=>{ const td=document.createElement('td'); if(col.c)td.className=col.c; if(col.style)td.setAttribute('style',col.style); if(col.badge){const sp=document.createElement('span');sp.className=`status-badge status-${col.t}`;sp.textContent=col.t;td.appendChild(sp);} else td.textContent=col.t; diff --git a/crates/scriptmark/src/roster.rs b/crates/scriptmark/src/roster.rs index 66e900b..9178fb0 100644 --- a/crates/scriptmark/src/roster.rs +++ b/crates/scriptmark/src/roster.rs @@ -1,11 +1,232 @@ -use std::collections::HashMap; use std::path::Path; -/// Load a roster CSV mapping student IDs to names. +use serde::{Deserialize, Serialize}; + +use crate::models::{ + DiagnosticKind, DiagnosticSeverity, InputDiagnostic, SourceLocation, StudentKey, normalize_key, +}; + +/// One roster row. /// -/// Expected format: `name,_,student_id` (header row skipped). -/// Handles UTF-8 BOM. -pub fn load_roster(path: &Path) -> Result, RosterError> { +/// The key is a [`StudentKey`], not a bare string, so a Canvas enrollment carrying no SIS +/// id is still a roster member — keyed by its Canvas id — rather than being dropped for +/// want of a student number. Student numbers are text: leading zeros survive, and nothing +/// is ever parsed as an integer. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum RosterSource { + /// A row the teacher supplied, from a CSV or an explicit config. + #[default] + Supplied, + /// A course enrollment Canvas reported. + CanvasEnrollment, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct RosterEntry { + pub key: StudentKey, + #[serde(default)] + pub source: RosterSource, + #[serde(default)] + pub name: Option, + #[serde(default)] + pub canvas_user_id: Option, + #[serde(default)] + pub location: Option, +} + +impl RosterEntry { + pub fn new(student_number: impl Into, name: Option) -> Self { + Self { + key: StudentKey::Number(normalize_key(&student_number.into())), + source: RosterSource::Supplied, + name, + canvas_user_id: None, + location: None, + } + } + + /// The 学号, when this row has one. + pub fn student_number(&self) -> Option<&str> { + match &self.key { + StudentKey::Number(number) => Some(number), + _ => None, + } + } +} + +/// The roster of record for an assignment. +/// +/// Rows are kept in a `Vec`, not a map: two rows carrying the same student number are both +/// retained and reported, rather than one silently overwriting the other. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct Roster { + pub entries: Vec, + #[serde(default)] + pub diagnostics: Vec, +} + +impl Roster { + pub fn from_entries(entries: Vec) -> Self { + Self::with_diagnostics(entries, Vec::new()) + } + + pub fn with_diagnostics( + entries: Vec, + mut diagnostics: Vec, + ) -> Self { + let (entries, merge_diagnostics) = merge_by_key(entries); + diagnostics.extend(merge_diagnostics); + Self { + entries, + diagnostics, + } + } + + /// Convenience for tests and for callers holding a plain id/name list. + pub fn from_pairs, V: AsRef>(pairs: &[(K, V)]) -> Self { + Self::from_entries( + pairs + .iter() + .map(|(id, name)| RosterEntry::new(id.as_ref(), Some(name.as_ref().to_string()))) + .collect(), + ) + } + + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } + + pub fn len(&self) -> usize { + self.entries.len() + } + + /// Exact-key lookup, returning the one row for that key. + /// + /// A student number is compared as written, after the same trim the loader applies — + /// never case-folded, never zero-stripped. An unconfirmed local token and a confirmed + /// 学号 denote the same thing, so they match the same row; a Canvas id is a separate + /// namespace and only ever matches a Canvas-keyed row. + /// + /// There is at most one row per key: a student number identifies one person, so rows + /// that merely repeat are merged on construction and rows that contradict each other + /// are refused. + pub fn lookup(&self, key: &StudentKey) -> Option { + let matches = |entry: &RosterEntry| match (&entry.key, key) { + (StudentKey::CanvasUser(a), StudentKey::CanvasUser(b)) => a == b, + (StudentKey::CanvasUser(_), _) | (_, StudentKey::CanvasUser(_)) => false, + (a, b) => a.raw() == b.raw(), + }; + self.entries.iter().position(matches) + } + + /// Look a student number up as written. + pub fn lookup_number(&self, number: &str) -> Option { + self.lookup(&StudentKey::Number(normalize_key(number))) + } + + /// Anything that makes this roster untrustworthy to grade from. + pub fn errors(&self) -> impl Iterator { + self.diagnostics + .iter() + .filter(|d| d.severity == DiagnosticSeverity::Error) + } + + pub fn name_of(&self, key: &StudentKey) -> Option<&str> { + self.lookup(key) + .and_then(|i| self.entries[i].name.as_deref()) + } +} + +/// What makes two rows for one key irreconcilable, if anything. +/// +/// Only non-empty values count: a row that simply does not know someone's name does not +/// contradict one that does. +fn contradiction(kept: &RosterEntry, other: &RosterEntry) -> Option { + match (kept.name.as_deref(), other.name.as_deref()) { + (Some(a), Some(b)) if a != b => return Some(format!("named '{a}' and '{b}'")), + _ => {} + } + match (kept.canvas_user_id, other.canvas_user_id) { + (Some(a), Some(b)) if a != b => Some(format!("Canvas ids {a} and {b}")), + _ => None, + } +} + +/// Collapse rows that share a key. +/// +/// A student number identifies one person, so repeated rows are the same person listed +/// twice — merged, and reported so the file gets cleaned up. Rows that disagree about who +/// that person is are a different matter: there is no answer to pick, so they are an +/// `Error` and the run stops rather than attributing someone's work to the wrong name. +/// +/// Rows from *different* sources are never in conflict: Canvas spelling a name differently +/// from the teacher's spreadsheet is ordinary, and enrollment wins because it comes first. +fn merge_by_key(entries: Vec) -> (Vec, Vec) { + let mut merged: Vec = Vec::new(); + let mut first_seen: std::collections::BTreeMap = Default::default(); + let mut repeats: std::collections::BTreeMap = Default::default(); + let mut conflicted: std::collections::BTreeSet = Default::default(); + let mut diagnostics = Vec::new(); + + for entry in entries { + let Some(&i) = first_seen.get(&entry.key) else { + first_seen.insert(entry.key.clone(), merged.len()); + merged.push(entry); + continue; + }; + + let kept = &mut merged[i]; + if kept.source == entry.source + && let Some(detail) = contradiction(kept, &entry) + { + if conflicted.insert(entry.key.clone()) { + let mut diagnostic = + InputDiagnostic::error(DiagnosticKind::ConflictingRosterEntry { + key: entry.key.to_string(), + detail, + }); + diagnostic.location = entry.location.clone(); + diagnostics.push(diagnostic); + } + continue; + } + + *repeats.entry(entry.key.clone()).or_insert(1) += 1; + // Fill in only what the kept row does not already know. + if kept.name.is_none() { + kept.name = entry.name; + } + if kept.canvas_user_id.is_none() { + kept.canvas_user_id = entry.canvas_user_id; + } + } + + for (key, count) in repeats { + if conflicted.contains(&key) { + continue; + } + diagnostics.push(InputDiagnostic::warning( + DiagnosticKind::DuplicateRosterEntry { + key: key.to_string(), + count, + }, + )); + } + + (merged, diagnostics) +} + +/// Load a roster CSV. +/// +/// Expected format: `name,_,student_id` (header row skipped), or `name,student_id`. +/// Handles a UTF-8 BOM. Column *mapping* — choosing which column is which — is P-672; +/// this stays positional on purpose. +/// +/// A row that parses but carries no usable student number is reported rather than skipped: +/// dropping it silently would take that student out of the roster of record, and with them +/// the `NotSubmitted` entry the model exists to preserve. +pub fn load_roster(path: &Path) -> Result { let content = std::fs::read_to_string(path).map_err(|e| RosterError::IoError(path.to_path_buf(), e))?; @@ -17,27 +238,69 @@ pub fn load_roster(path: &Path) -> Result, RosterError> .flexible(true) .from_reader(content.as_bytes()); - let mut roster = HashMap::new(); + let mut entries = Vec::new(); + let mut diagnostics = Vec::new(); - for result in reader.records() { + for (row, result) in reader.records().enumerate() { let record = result.map_err(|e| RosterError::CsvError(path.to_path_buf(), e))?; + // +2: one for the skipped header, one for 1-based line numbers. + let location = SourceLocation::row(path.to_path_buf(), row + 2); // Format: name, _, student_id (or name, student_id) let name = record.get(0).unwrap_or("").trim().to_string(); - let student_id = if record.len() >= 3 { - record.get(2).unwrap_or("").trim().to_string() + let student_number = if record.len() >= 3 { + record.get(2).unwrap_or("") } else if record.len() >= 2 { - record.get(1).unwrap_or("").trim().to_string() + record.get(1).unwrap_or("") } else { + diagnostics.push( + InputDiagnostic::warning(DiagnosticKind::UnusableRosterRow { + reason: format!("only {} column(s); need at least 2", record.len()), + }) + .at(location), + ); continue; }; - if !student_id.is_empty() { - roster.insert(student_id, name); + let student_number = normalize_key(student_number); + if let Some(prefix) = crate::models::RESERVED_KEY_PREFIXES + .iter() + .find(|p| student_number.starts_with(**p)) + { + diagnostics.push( + InputDiagnostic::warning(DiagnosticKind::UnusableRosterRow { + reason: format!( + "student id '{student_number}' starts with the reserved prefix '{prefix}'" + ), + }) + .at(location), + ); + continue; + } + if student_number.is_empty() { + diagnostics.push( + InputDiagnostic::warning(DiagnosticKind::UnusableRosterRow { + reason: if name.is_empty() { + "blank row".to_string() + } else { + format!("'{name}' has no student id") + }, + }) + .at(location), + ); + continue; } + + entries.push(RosterEntry { + key: StudentKey::Number(student_number), + source: RosterSource::Supplied, + name: (!name.is_empty()).then_some(name), + canvas_user_id: None, + location: Some(location), + }); } - Ok(roster) + Ok(Roster::with_diagnostics(entries, diagnostics)) } #[derive(Debug, thiserror::Error)] @@ -52,29 +315,163 @@ pub enum RosterError { mod tests { use super::*; + fn write(dir: &tempfile::TempDir, content: &str) -> std::path::PathBuf { + let path = dir.path().join("roster.csv"); + std::fs::write(&path, content).unwrap(); + path + } + #[test] fn test_load_roster() { let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("roster.csv"); - std::fs::write( - &path, + let path = write( + &dir, "name,class,student_id\nAlice,A,alice123\nBob,B,bob456\n", - ) - .unwrap(); + ); let roster = load_roster(&path).unwrap(); assert_eq!(roster.len(), 2); - assert_eq!(roster["alice123"], "Alice"); - assert_eq!(roster["bob456"], "Bob"); + assert_eq!(roster.lookup_number("alice123"), Some(0)); + assert_eq!(roster.entries[0].name.as_deref(), Some("Alice")); + assert_eq!(roster.entries[1].name.as_deref(), Some("Bob")); + assert!(roster.diagnostics.is_empty()); } #[test] fn test_load_roster_with_bom() { let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("roster.csv"); - std::fs::write(&path, "\u{feff}name,class,student_id\nAlice,A,alice123\n").unwrap(); + let path = write(&dir, "\u{feff}name,class,student_id\nAlice,A,alice123\n"); + + let roster = load_roster(&path).unwrap(); + assert_eq!(roster.entries[0].student_number(), Some("alice123")); + } + + #[test] + fn test_leading_zeros_survive_and_stay_distinct() { + let dir = tempfile::tempdir().unwrap(); + let path = write( + &dir, + "name,class,student_id\nCarol,C,0024010003\nDave,D,24010003\n", + ); + + let roster = load_roster(&path).unwrap(); + assert_eq!(roster.len(), 2); + assert_eq!(roster.lookup_number("0024010003"), Some(0)); + assert_eq!(roster.lookup_number("24010003"), Some(1)); + // Exact-text keys cannot collide, so this is not a duplicate. + assert!(roster.diagnostics.is_empty()); + } + + #[test] + fn test_repeated_identical_rows_are_merged_and_reported() { + let dir = tempfile::tempdir().unwrap(); + let path = write( + &dir, + "name,class,student_id\nAlice,A,2024010001\nAlice,B,2024010001\n", + ); + + let roster = load_roster(&path).unwrap(); + // One student number is one person, so a repeated row is that person listed twice. + assert_eq!(roster.len(), 1); + assert_eq!(roster.entries[0].name.as_deref(), Some("Alice")); + assert_eq!(roster.diagnostics.len(), 1); + assert_eq!(roster.diagnostics[0].severity, DiagnosticSeverity::Warning); + assert!(matches!( + &roster.diagnostics[0].kind, + DiagnosticKind::DuplicateRosterEntry { key, count } + if key == "2024010001" && *count == 2 + )); + } + + #[test] + fn test_a_row_that_only_knows_less_is_not_a_conflict() { + let dir = tempfile::tempdir().unwrap(); + // The second row has no name — it does not contradict the first, it just knows + // less, so the two merge. + let path = write( + &dir, + "name,class,student_id\n,A,2024010001\nAlice,B,2024010001\n", + ); + + let roster = load_roster(&path).unwrap(); + assert_eq!(roster.len(), 1); + assert_eq!(roster.entries[0].name.as_deref(), Some("Alice")); + assert!(roster.errors().next().is_none()); + } + + #[test] + fn test_rows_that_disagree_about_who_a_number_is_are_an_error() { + let dir = tempfile::tempdir().unwrap(); + let path = write( + &dir, + "name,class,student_id\nAlice Wu,A,2024010001\nAlice Chen,B,2024010001\n", + ); + + let roster = load_roster(&path).unwrap(); + // There is no answer to pick between them, so this is not a warning to skim past. + let errors: Vec<_> = roster.errors().collect(); + assert_eq!(errors.len(), 1); + assert!(matches!( + &errors[0].kind, + DiagnosticKind::ConflictingRosterEntry { key, .. } if key == "2024010001" + )); + // The line the clash was spotted on is named, so the file can be fixed. + assert_eq!(errors[0].location.as_ref().and_then(|l| l.row), Some(3)); + } + + #[test] + fn test_lookup_trims_but_does_not_otherwise_normalise() { + let roster = Roster::from_pairs(&[("2024010001", "Alice")]); + assert_eq!(roster.lookup_number(" 2024010001 "), Some(0)); + assert_eq!(roster.lookup_number("02024010001"), None); + assert_eq!(roster.lookup_number("missing"), None); + } + + #[test] + fn test_an_unconfirmed_local_token_matches_a_student_number_row() { + let roster = Roster::from_pairs(&[("2024010001", "Alice")]); + assert_eq!( + roster.lookup(&StudentKey::Extracted("2024010001".into())), + Some(0) + ); + // A Canvas id is a separate namespace and must not match a 学号 row. + assert_eq!(roster.lookup(&StudentKey::CanvasUser(2024010001)), None); + } + + #[test] + fn test_rows_record_their_source_line() { + let dir = tempfile::tempdir().unwrap(); + let path = write(&dir, "name,class,student_id\nAlice,A,2024010001\n"); + + let roster = load_roster(&path).unwrap(); + let location = roster.entries[0].location.as_ref().unwrap(); + assert_eq!(location.file.as_deref(), Some(path.as_path())); + assert_eq!(location.row, Some(2)); + } + + #[test] + fn test_unusable_rows_are_reported_rather_than_dropped_silently() { + let dir = tempfile::tempdir().unwrap(); + let path = write( + &dir, + "name,class,student_id\nBob Lin,,2024010002\nCarol,,\nsolo\n", + ); let roster = load_roster(&path).unwrap(); - assert_eq!(roster["alice123"], "Alice"); + assert_eq!(roster.len(), 1); + // Carol's blank id and the one-column row each leave a trace naming their line. + assert_eq!(roster.diagnostics.len(), 2); + assert!( + roster + .diagnostics + .iter() + .all(|d| matches!(&d.kind, DiagnosticKind::UnusableRosterRow { .. })) + ); + let rows: Vec> = roster + .diagnostics + .iter() + .map(|d| d.location.as_ref().and_then(|l| l.row)) + .collect(); + assert_eq!(rows, vec![Some(3), Some(4)]); } } diff --git a/crates/scriptmark/src/runner/oracle.rs b/crates/scriptmark/src/runner/oracle.rs index 064e71c..0da1d8b 100644 --- a/crates/scriptmark/src/runner/oracle.rs +++ b/crates/scriptmark/src/runner/oracle.rs @@ -15,10 +15,7 @@ pub async fn resolve_oracle( ) { if let Some(ref_path) = &oracle.reference { // Run teacher's reference implementation with same function + args - let ref_file = StudentFile { - path: Path::new(ref_path).to_path_buf(), - language: "python".to_string(), - }; + let ref_file = StudentFile::direct(Path::new(ref_path).to_path_buf(), "python"); let ref_spec = TestSpec { meta: spec.meta.clone(), vars: Default::default(), diff --git a/crates/scriptmark/src/runner/orchestrator.rs b/crates/scriptmark/src/runner/orchestrator.rs index 92ac470..a2460ba 100644 --- a/crates/scriptmark/src/runner/orchestrator.rs +++ b/crates/scriptmark/src/runner/orchestrator.rs @@ -2,8 +2,8 @@ use std::collections::HashMap; use std::sync::Arc; use crate::models::{ - CaseResult, FailureDetail, StudentFile, StudentReport, SubmissionSet, TestResult, TestSpec, - TestStatus, + CaseResult, FailureDetail, StudentFile, StudentReport, StudentSubmission, SubmissionState, + TestResult, TestSpec, TestStatus, }; use tokio::sync::Semaphore; @@ -12,14 +12,22 @@ use crate::runner::resolve::resolve_args; /// Run all test specs for all students in parallel. /// +/// Takes `&[StudentSubmission]` rather than the whole `AssignmentInput` so that the roster, +/// the unmatched artifacts and the diagnostics stay out of the runner. Canvas-only material +/// is kept out of the executor itself by `run_student`, which only ever sees a student id +/// and a file list. +/// +/// Returns one report per student **in input order**, including students with nothing to +/// run: a roster member who did not submit must not vanish from the results, and a +/// `HashMap` keyed on student id would additionally drop one of two retained duplicates. /// Concurrency is bounded by `max_concurrent` (defaults to number of CPUs). pub async fn run_all( - submissions: &SubmissionSet, + students: &[StudentSubmission], specs: &[TestSpec], executor: &PythonExecutor, timeout_secs: u64, max_concurrent: Option, -) -> HashMap { +) -> Vec { let concurrency = max_concurrent.unwrap_or_else(|| { std::thread::available_parallelism() .map(|n| n.get()) @@ -29,32 +37,60 @@ pub async fn run_all( let mut handles = Vec::new(); - for (sid, files) in &submissions.by_student { - let sid = sid.clone(); - let files = files.clone(); + for student in students { + let identity = student.identity.clone(); + let outcome = student.outcome(); + // Gate on the delivery axis, not the collapsed outcome: a submitter who is missing + // from the roster still has runnable code, and refusing to run it would hide the + // very output a teacher needs to resolve the mismatch. + let runnable = student.state == SubmissionState::Executable; + let files = student.files().to_vec(); let specs = specs.to_vec(); let sem = semaphore.clone(); let python_cmd = executor.python_cmd().to_string(); let timeout = timeout_secs; let handle = tokio::spawn(async move { - let _permit = sem.acquire().await.unwrap(); - let exec = PythonExecutor::with_python_cmd(&python_cmd); - let report = run_student(&exec, &sid, &files, &specs, timeout).await; - (sid, report) + let sid = identity.key.to_string(); + let mut report = if runnable { + let _permit = sem.acquire().await.unwrap(); + let exec = PythonExecutor::with_python_cmd(&python_cmd); + run_student(&exec, &sid, &files, &specs, timeout).await + } else { + // Nothing to run, but the student still gets a row. + StudentReport { + student_id: sid, + ..Default::default() + } + }; + report.student_name = identity.name.clone(); + report.canvas_user_id = identity.canvas_user_id; + report.submission_state = Some(outcome); + report }); - handles.push(handle); + handles.push((student, handle)); } - let mut results = HashMap::new(); - for handle in handles { - if let Ok((sid, report)) = handle.await { - results.insert(sid, report); + let mut reports = Vec::with_capacity(handles.len()); + for (student, handle) in handles { + match handle.await { + Ok(report) => reports.push(report), + // A panicked task must not make the student disappear — but it must not look + // like a failed test case either. Recorded as an error, so `is_gradeable()` + // withholds a grade rather than scoring an infrastructure failure. + Err(e) => reports.push(StudentReport { + student_id: student.identity.key.to_string(), + student_name: student.identity.name.clone(), + canvas_user_id: student.identity.canvas_user_id, + submission_state: Some(student.outcome()), + error: Some(format!("grading task failed: {e}")), + ..Default::default() + }), } } - results + reports } /// Run all test specs for a single student. @@ -226,7 +262,7 @@ async fn run_student( }; test_results.push(TestResult { - spec_name: spec.meta.name.clone(), + item_id: spec.meta.name.clone(), cases, }); } @@ -245,10 +281,9 @@ async fn run_student( StudentReport { student_id: sid.to_string(), - student_name: None, test_results, - final_grade: None, backend_name: Some("python".to_string()), lint_score, + ..Default::default() } } diff --git a/crates/scriptmark/src/runner/python.rs b/crates/scriptmark/src/runner/python.rs index b7c3228..bdaf652 100644 --- a/crates/scriptmark/src/runner/python.rs +++ b/crates/scriptmark/src/runner/python.rs @@ -599,7 +599,7 @@ impl PythonExecutor { }) .collect(); - scored.sort_by(|a, b| b.1.cmp(&a.1)); + scored.sort_by_key(|a| std::cmp::Reverse(a.1)); scored.first().map(|(f, _)| *f) } diff --git a/crates/scriptmark/src/tui/ui.rs b/crates/scriptmark/src/tui/ui.rs index 9b4eafe..30946a8 100644 --- a/crates/scriptmark/src/tui/ui.rs +++ b/crates/scriptmark/src/tui/ui.rs @@ -103,15 +103,18 @@ fn draw_student_list(f: &mut Frame, area: Rect, app: &App) { .iter() .enumerate() .map(|(i, r)| { - let grade_color = if r.final_grade >= 90.0 { - Color::Green - } else if r.final_grade >= 70.0 { - Color::Blue - } else if r.final_grade >= 60.0 { - Color::Yellow - } else { - Color::Red + let grade_color = match r.final_grade { + Some(g) if g >= 90.0 => Color::Green, + Some(g) if g >= 70.0 => Color::Blue, + Some(g) if g >= 60.0 => Color::Yellow, + Some(_) => Color::Red, + // Not graded at all — a dash, never a red zero. + None => Color::DarkGray, }; + let grade_text = r + .final_grade + .map(|g| format!("{g:.1}")) + .unwrap_or_else(|| "—".to_string()); let style = if i == app.selected { Style::default().bg(Color::DarkGray) @@ -122,7 +125,7 @@ fn draw_student_list(f: &mut Frame, area: Rect, app: &App) { Row::new(vec![ Cell::from(r.student_name.as_deref().unwrap_or("N/A")), Cell::from(r.student_id.as_str()), - Cell::from(format!("{:.1}", r.final_grade)).style(Style::default().fg(grade_color)), + Cell::from(grade_text).style(Style::default().fg(grade_color)), Cell::from(format!("{:.0}%", r.pass_rate)), Cell::from(format!("{}/{}", r.passed_cases, r.total_cases)), ]) @@ -166,7 +169,7 @@ fn draw_detail(f: &mut Frame, area: Rect, app: &App) { { for tr in &report.test_results { lines.push(Line::from(Span::styled( - format!("--- {} ---", tr.spec_name), + format!("--- {} ---", tr.item_id), Style::default().fg(Color::Cyan), ))); for case in &tr.cases { diff --git a/crates/scriptmark/tests/fixtures/hw1/canvas/assignment.json b/crates/scriptmark/tests/fixtures/hw1/canvas/assignment.json new file mode 100644 index 0000000..59c61cb --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/canvas/assignment.json @@ -0,0 +1,181 @@ +{ + "course_id": 5510, + "assignment_id": 88120, + "assignment_name": "hw1", + "users": [ + { + "id": 101, + "name": "Alice Wu", + "sortable_name": "Wu, Alice", + "sis_user_id": "2024010001", + "login_id": "awu" + }, + { + "id": 102, + "name": "Bob Lin", + "sortable_name": "Lin, Bob", + "sis_user_id": "2024010002", + "login_id": "blin" + }, + { + "id": 103, + "name": "Carol Zero", + "sortable_name": "Zero, Carol", + "sis_user_id": "0024010003", + "login_id": "czero" + }, + { + "id": 104, + "name": "Dave Nozero", + "sortable_name": "Nozero, Dave", + "sis_user_id": "24010003", + "login_id": "dnozero" + }, + { + "id": 105, + "name": "Dan Absent", + "sortable_name": "Absent, Dan", + "sis_user_id": "2024010004", + "login_id": "dabsent" + }, + { + "id": 106, + "name": "Eve Empty", + "sortable_name": "Empty, Eve", + "sis_user_id": "2024010005", + "login_id": "eempty" + } + ], + "submissions": [ + { + "id": 9101, + "user_id": 101, + "attempt": 1, + "workflow_state": "submitted", + "submitted_at": "2026-03-01T09:00:00Z", + "submission_type": "online_upload", + "attachments": [ + { + "id": 1001, + "filename": "lab1.py", + "display_name": "lab1.py", + "content_type": "text/x-python", + "size": 52, + "url": "https://canvas.invalid/files/1001/download" + } + ] + }, + { + "id": 9102, + "user_id": 102, + "attempt": 2, + "workflow_state": "submitted", + "submitted_at": "2026-03-02T22:10:00Z", + "late": true, + "submission_type": "online_upload", + "attachments": [ + { + "id": 1003, + "filename": "lab1.py", + "display_name": "lab1.py", + "content_type": "text/x-python", + "size": 52, + "url": "https://canvas.invalid/files/1003/download" + } + ], + "submission_history": [ + { + "id": 9102, + "user_id": 102, + "attempt": 2, + "workflow_state": "submitted", + "submitted_at": "2026-03-02T22:10:00Z", + "late": true, + "submission_type": "online_upload", + "attachments": [ + { + "id": 1003, + "filename": "lab1.py", + "display_name": "lab1.py", + "content_type": "text/x-python", + "size": 52, + "url": "https://canvas.invalid/files/1003/download" + } + ] + }, + { + "id": 9102, + "user_id": 102, + "attempt": 1, + "workflow_state": "submitted", + "submitted_at": "2026-03-01T08:00:00Z", + "submission_type": "online_upload", + "attachments": [ + { + "id": 1002, + "filename": "draft.py", + "display_name": "draft.py", + "content_type": "text/x-python", + "size": 52, + "url": "https://canvas.invalid/files/1002/download" + } + ] + } + ] + }, + { + "id": 9103, + "user_id": 103, + "attempt": 1, + "workflow_state": "submitted", + "submitted_at": "2026-03-01T10:00:00Z", + "submission_type": "online_upload", + "attachments": [ + { + "id": 1004, + "filename": "lab1.py", + "display_name": "lab1.py", + "content_type": "text/x-python", + "size": 52, + "url": "https://canvas.invalid/files/1004/download" + } + ] + }, + { + "id": 9104, + "user_id": 104, + "attempt": 1, + "workflow_state": "submitted", + "submitted_at": "2026-03-01T11:00:00Z", + "submission_type": "online_upload", + "attachments": [ + { + "id": 1005, + "filename": "lab1.py", + "display_name": "lab1.py", + "content_type": "text/x-python", + "size": 52, + "url": "https://canvas.invalid/files/1005/download" + } + ] + }, + { + "id": 0, + "user_id": 105, + "attempt": null, + "workflow_state": "unsubmitted", + "submitted_at": null, + "missing": true, + "attachments": [] + }, + { + "id": 9106, + "user_id": 106, + "attempt": 1, + "workflow_state": "submitted", + "submitted_at": "2026-03-01T12:00:00Z", + "submission_type": "online_upload", + "attachments": [] + } + ] +} diff --git a/crates/scriptmark/tests/fixtures/hw1/canvas/files/1001/lab1.py b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1001/lab1.py new file mode 100644 index 0000000..1e5de16 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1001/lab1.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return max(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/canvas/files/1002/draft.py b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1002/draft.py new file mode 100644 index 0000000..6d1d494 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1002/draft.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return min(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/canvas/files/1003/lab1.py b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1003/lab1.py new file mode 100644 index 0000000..1e5de16 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1003/lab1.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return max(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/canvas/files/1004/lab1.py b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1004/lab1.py new file mode 100644 index 0000000..1e5de16 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1004/lab1.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return max(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/canvas/files/1005/lab1.py b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1005/lab1.py new file mode 100644 index 0000000..1e5de16 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/canvas/files/1005/lab1.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return max(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/legacy_results.json b/crates/scriptmark/tests/fixtures/hw1/legacy_results.json new file mode 100644 index 0000000..437f16c --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/legacy_results.json @@ -0,0 +1,33 @@ +[ + { + "student_id": "alice", + "student_name": "Alice Wu", + "test_results": [ + { + "spec_name": "find_larger_number", + "cases": [ + { "case_name": "3 < 5", "status": "passed", "actual": "5", "expected": "5", "failure": null, "elapsed_ms": 12 } + ] + } + ], + "final_grade": 95.0, + "backend_name": "python", + "lint_score": null + }, + { + "student_id": "bob", + "student_name": "Bob Lin", + "test_results": [ + { + "spec_name": "find_larger_number", + "cases": [ + { "case_name": "3 < 5", "status": "failed", "actual": "3", "expected": "5", + "failure": { "message": "expected 5, got 3", "details": "" }, "elapsed_ms": 11 } + ] + } + ], + "final_grade": 60.0, + "backend_name": "python", + "lint_score": null + } +] diff --git a/crates/scriptmark/tests/fixtures/hw1/local/roster.csv b/crates/scriptmark/tests/fixtures/hw1/local/roster.csv new file mode 100644 index 0000000..59f22d7 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/local/roster.csv @@ -0,0 +1,8 @@ +name,class,student_id +Alice Wu,A,2024010001 +Alice Wu,B,2024010001 +Bob Lin,A,2024010002 +Carol Zero,A,0024010003 +Dave Nozero,A,24010003 +Dan Absent,B,2024010004 +Eve Empty,B,2024010005 diff --git a/crates/scriptmark/tests/fixtures/hw1/local/submissions/0024010003_lab1.py b/crates/scriptmark/tests/fixtures/hw1/local/submissions/0024010003_lab1.py new file mode 100644 index 0000000..1e5de16 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/local/submissions/0024010003_lab1.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return max(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010001_lab1.py b/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010001_lab1.py new file mode 100644 index 0000000..1e5de16 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010001_lab1.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return max(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010002_lab1.py b/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010002_lab1.py new file mode 100644 index 0000000..1e5de16 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010002_lab1.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return max(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010005_notes.txt b/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010005_notes.txt new file mode 100644 index 0000000..e16a9f7 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/local/submissions/2024010005_notes.txt @@ -0,0 +1 @@ +I ran out of time, sorry! diff --git a/crates/scriptmark/tests/fixtures/hw1/local/submissions/24010003_lab1.py b/crates/scriptmark/tests/fixtures/hw1/local/submissions/24010003_lab1.py new file mode 100644 index 0000000..1e5de16 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/local/submissions/24010003_lab1.py @@ -0,0 +1,2 @@ +def find_larger_number(a, b): + return max(a, b) diff --git a/crates/scriptmark/tests/fixtures/hw1/local/submissions/_scratch_v2.py b/crates/scriptmark/tests/fixtures/hw1/local/submissions/_scratch_v2.py new file mode 100644 index 0000000..b99f799 --- /dev/null +++ b/crates/scriptmark/tests/fixtures/hw1/local/submissions/_scratch_v2.py @@ -0,0 +1,2 @@ +def scratch(): + pass diff --git a/crates/scriptmark/tests/input_equivalence.rs b/crates/scriptmark/tests/input_equivalence.rs new file mode 100644 index 0000000..7f26698 --- /dev/null +++ b/crates/scriptmark/tests/input_equivalence.rs @@ -0,0 +1,318 @@ +//! The same assignment, carried through both entry points, must arrive as the same input. +//! +//! A bare `assert_eq!(local, canvas)` would be satisfied by two empty vectors — which is +//! exactly what a gitignored fixture tree produces in CI. So each side is first checked +//! against a hand-written expected table, then the two are compared. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use scriptmark::discovery::{LocalInputOptions, load_local_input}; +use scriptmark::input::canvas::{CanvasPayload, DownloadedAttachments, normalize}; +use scriptmark::models::{ + Assignment, AssignmentInput, AttemptPolicy, DiagnosticKind, ProjectedStudent, RosterMatch, + StudentReport, SubmissionOutcome, UnmatchedReason, +}; +use scriptmark::roster::{Roster, load_roster}; + +fn fixture_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/hw1") +} + +/// Copy the committed fixture somewhere writable. The local adapter expands archives beside +/// the directory it scans, and `#[test]` bodies run in parallel — neither belongs in the +/// source tree. +fn copy_dir(src: &Path, dst: &Path) { + std::fs::create_dir_all(dst).unwrap(); + for entry in std::fs::read_dir(src).unwrap() { + let entry = entry.unwrap(); + let target = dst.join(entry.file_name()); + if entry.path().is_dir() { + copy_dir(&entry.path(), &target); + } else { + std::fs::copy(entry.path(), &target).unwrap(); + } + } +} + +fn roster() -> Roster { + let path = fixture_root().join("local/roster.csv"); + load_roster(&path) + .unwrap_or_else(|e| panic!("fixture roster missing at {}: {e}", path.display())) +} + +fn local_input(dir: &tempfile::TempDir) -> AssignmentInput { + copy_dir(&fixture_root().join("local"), dir.path()); + let roster = roster(); + load_local_input( + &[dir.path().join("submissions")], + LocalInputOptions { + assignment: Assignment::named("hw1"), + roster: Some(&roster), + attempt_policy: AttemptPolicy::Latest, + }, + ) + .unwrap() +} + +fn canvas_input() -> AssignmentInput { + let root = fixture_root().join("canvas"); + let raw = std::fs::read_to_string(root.join("assignment.json")) + .unwrap_or_else(|e| panic!("fixture payload missing: {e}")); + let payload: CanvasPayload = serde_json::from_str(&raw).expect("fixture payload must parse"); + + // Standing in for P-670's downloader: the files these attachments would land in. + let downloads: DownloadedAttachments = HashMap::from([ + (1001, root.join("files/1001/lab1.py")), + (1002, root.join("files/1002/draft.py")), + (1003, root.join("files/1003/lab1.py")), + (1004, root.join("files/1004/lab1.py")), + (1005, root.join("files/1005/lab1.py")), + ]); + for path in downloads.values() { + assert!( + path.is_file(), + "fixture download missing: {}", + path.display() + ); + } + + normalize(&payload, Some(&roster()), &downloads, AttemptPolicy::Latest) +} + +/// What both sources must produce, written out rather than derived. +fn expected() -> Vec { + use SubmissionOutcome::*; + let rows: &[(&str, SubmissionOutcome, &[&str])] = &[ + // Two roster rows carry this number; the submitter matches both. + ("2024010001", Executable, &["lab1.py"]), + // Resubmitted — the second attempt is the one that counts. + ("2024010002", Executable, &["lab1.py"]), + // Differs from the next row only by zero padding, and stays a separate student. + ("0024010003", Executable, &["lab1.py"]), + ("24010003", Executable, &["lab1.py"]), + // On the roster, nothing received. + ("2024010004", NotSubmitted, &[]), + // Something arrived, nothing runnable in it. + ("2024010005", SubmittedEmpty, &[]), + ]; + let mut expected: Vec = rows + .iter() + .map(|(key, outcome, artifacts)| ProjectedStudent { + key: (*key).to_string(), + outcome: *outcome, + artifacts: artifacts.iter().map(|a| a.to_string()).collect(), + }) + .collect(); + expected.sort(); + expected +} + +/// Strip the student-number prefix the local fixture deliberately carries. +/// +/// A Canvas attachment is named by the student (`lab1.py`); a teacher's bulk download is +/// named `{sid}_lab1.py`. That is the one shape difference between the sources, and +/// reversing it is a matching rule — which is P-673 — so it lives here and not in the model. +fn canonical(projection: Vec) -> Vec { + projection + .into_iter() + .map(|mut student| { + let prefix = format!("{}_", student.key); + student.artifacts = student + .artifacts + .iter() + .map(|name| name.strip_prefix(&prefix).unwrap_or(name).to_string()) + .collect(); + student + }) + .collect() +} + +#[test] +fn test_local_material_matches_the_expected_table() { + let dir = tempfile::tempdir().unwrap(); + let input = local_input(&dir); + + assert_eq!(input.student_count(), 6); + assert_eq!(canonical(input.projection()), expected()); +} + +#[test] +fn test_canvas_material_matches_the_expected_table() { + let input = canvas_input(); + + assert_eq!(input.student_count(), 6); + assert_eq!(canonical(input.projection()), expected()); +} + +#[test] +fn test_both_entry_points_produce_equivalent_input() { + let dir = tempfile::tempdir().unwrap(); + let local = local_input(&dir); + let canvas = canvas_input(); + + // Neither side may be trivially empty — that is how this assertion goes vacuous. + assert_eq!(local.student_count(), 6); + assert_eq!(canvas.student_count(), 6); + assert_eq!( + canonical(local.projection()), + canonical(canvas.projection()) + ); +} + +#[test] +fn test_every_roster_member_is_present_on_both_sides() { + let dir = tempfile::tempdir().unwrap(); + let roster = roster(); + // Seven rows in the file; 2024010001 is listed twice and merges, so six people. + assert_eq!(roster.len(), 6); + + for input in [local_input(&dir), canvas_input()] { + for entry in &roster.entries { + assert!( + input.students.iter().any(|s| s.identity.key == entry.key), + "roster member {} vanished", + entry.key + ); + } + } +} + +#[test] +fn test_a_repeated_roster_row_is_merged_and_reported_on_both_sides() { + let dir = tempfile::tempdir().unwrap(); + for input in [local_input(&dir), canvas_input()] { + assert!( + input.diagnostics.iter().any(|d| matches!( + &d.kind, + DiagnosticKind::DuplicateRosterEntry { key, count } + if key == "2024010001" && *count == 2 + )), + "the repeated row must be reported so the file gets cleaned up" + ); + // One student number is one person, however many rows repeat it. + let alice: Vec<_> = input + .students + .iter() + .filter(|s| s.identity.key.raw() == "2024010001") + .collect(); + assert_eq!(alice.len(), 1); + // Which row index they land on is each source's own ordering, not a shared fact. + assert!(matches!(alice[0].roster_match, RosterMatch::Matched(_))); + // Merged, so the name is known — not withheld as it would be for a real clash. + assert_eq!(alice[0].identity.name.as_deref(), Some("Alice Wu")); + assert!(input.errors().next().is_none()); + } +} + +/// Each source reaches "received but unmatchable" by a different route, so it is asserted +/// per source rather than across them: locally a filename token no roster confirms, and on +/// Canvas a submission from somebody the course does not list. +#[test] +fn test_canvas_material_from_someone_not_enrolled_is_unmatched() { + let root = fixture_root().join("canvas"); + let raw = std::fs::read_to_string(root.join("assignment.json")).unwrap(); + let mut payload: CanvasPayload = serde_json::from_str(&raw).unwrap(); + + // Submitted, then dropped the course — Canvas no longer lists them. + payload.submissions.push( + serde_json::from_str( + r#"{"id": 9107, "user_id": 107, "attempt": 1, "workflow_state": "submitted", + "submitted_at": "2026-03-01T13:00:00Z", "submission_type": "online_upload", + "attachments": [{"id": 1006, "filename": "lab1.py", "display_name": "lab1.py"}]}"#, + ) + .unwrap(), + ); + let downloads: DownloadedAttachments = HashMap::from([(1006, root.join("files/1001/lab1.py"))]); + + let input = normalize(&payload, Some(&roster()), &downloads, AttemptPolicy::Latest); + let stranger = input + .students + .iter() + .find(|s| s.identity.canvas_user_id == Some(107)) + .expect("their work must not be discarded"); + assert_eq!(stranger.outcome(), SubmissionOutcome::ReceivedUnmatched); +} + +#[test] +fn test_only_the_local_side_can_have_orphan_files() { + let dir = tempfile::tempdir().unwrap(); + let local = local_input(&dir); + + assert_eq!(local.unmatched.len(), 1); + assert!(local.unmatched[0].path.ends_with("_scratch_v2.py")); + assert_eq!(local.unmatched[0].reason, UnmatchedReason::NoStudentKey); + + // Canvas attributes every attachment to a user, so it has no orphan class at all. + assert!(canvas_input().unmatched.is_empty()); +} + +#[test] +fn test_assignment_identity_is_kept_apart_from_student_identity() { + let canvas = canvas_input(); + assert_eq!(canvas.assignment.canvas_course_id, Some(5510)); + assert_eq!(canvas.assignment.canvas_assignment_id, Some(88120)); + + let alice = canvas + .students + .iter() + .find(|s| s.identity.key.raw() == "2024010001") + .unwrap(); + // 学号, Canvas user id, SIS id and login id are four separate fields. + assert_eq!(alice.identity.student_number.as_deref(), Some("2024010001")); + assert_eq!(alice.identity.canvas_user_id, Some(101)); + assert_eq!(alice.identity.sis_user_id.as_deref(), Some("2024010001")); + assert_eq!(alice.identity.login_id.as_deref(), Some("awu")); + + // A non-submitter keeps theirs too, although the teacher's CSV has no Canvas column — + // it is backfilled from enrollment rather than lost. + let dan = canvas + .students + .iter() + .find(|s| s.identity.key.raw() == "2024010004") + .unwrap(); + assert_eq!(dan.outcome(), SubmissionOutcome::NotSubmitted); + assert_eq!(dan.identity.canvas_user_id, Some(105)); +} + +#[test] +fn test_local_scan_leaves_no_artifacts_in_the_committed_fixture() { + let dir = tempfile::tempdir().unwrap(); + let _ = local_input(&dir); + assert!( + !fixture_root() + .join("local/submissions/.scriptmark_extracted") + .exists(), + "the committed fixture tree must not be written to" + ); +} + +#[test] +fn test_local_input_is_byte_identical_across_runs() { + let first = tempfile::tempdir().unwrap(); + let second = tempfile::tempdir().unwrap(); + + let a = serde_json::to_string(&local_input(&first)).unwrap(); + let b = serde_json::to_string(&local_input(&second)).unwrap(); + // Paths differ between the two temp dirs; everything else must not. + let normalise = + |s: String, dir: &tempfile::TempDir| s.replace(dir.path().to_str().unwrap(), "ROOT"); + assert_eq!(normalise(a, &first), normalise(b, &second)); +} + +#[test] +fn test_results_written_before_this_model_still_load() { + let raw = std::fs::read_to_string(fixture_root().join("legacy_results.json")).unwrap(); + let reports: Vec = serde_json::from_str(&raw).expect("legacy results must load"); + + assert_eq!(reports.len(), 2); + for report in &reports { + // A record that never carried a submission state must not claim one. + assert!(report.submission_state.is_none()); + assert!(report.canvas_user_id.is_none()); + // And it is still graded exactly as it was. + assert!(report.is_gradeable()); + } + assert_eq!(reports[0].student_id, "alice"); + assert_eq!(reports[0].final_grade, Some(95.0)); +} diff --git a/crates/scriptmark/tests/integration.rs b/crates/scriptmark/tests/integration.rs index c782350..5a7e344 100644 --- a/crates/scriptmark/tests/integration.rs +++ b/crates/scriptmark/tests/integration.rs @@ -1,10 +1,19 @@ -use std::collections::HashMap; use std::io::Write; +use scriptmark::grading::apply_grading; use scriptmark::models::*; use scriptmark::runner::orchestrator; use scriptmark::runner::python::PythonExecutor; +/// Reports come back as a list in input order, so tests look a student up by the id the +/// model renders rather than indexing a map. +fn by_id<'a>(reports: &'a [StudentReport], student_id: &str) -> &'a StudentReport { + reports + .iter() + .find(|r| r.student_id == student_id) + .unwrap_or_else(|| panic!("no report for '{student_id}'")) +} + fn setup_test_dir() -> tempfile::TempDir { let dir = tempfile::tempdir().unwrap(); @@ -73,10 +82,10 @@ async fn test_python_executor_correct_student() { let dir = setup_test_dir(); let executor = PythonExecutor::new(); - let files = vec![StudentFile { - path: dir.path().join("alice_lab5.py"), - language: "python".to_string(), - }]; + let files = vec![StudentFile::direct( + dir.path().join("alice_lab5.py"), + "python", + )]; let spec = test_spec(); @@ -112,10 +121,10 @@ async fn test_python_executor_buggy_student() { let dir = setup_test_dir(); let executor = PythonExecutor::new(); - let files = vec![StudentFile { - path: dir.path().join("bob_lab5.py"), - language: "python".to_string(), - }]; + let files = vec![StudentFile::direct( + dir.path().join("bob_lab5.py"), + "python", + )]; let spec = test_spec(); @@ -155,39 +164,23 @@ async fn test_orchestrator_runs_all_students() { let dir = setup_test_dir(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([ - ( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_lab5.py"), - language: "python".to_string(), - }], - ), - ( - "bob".to_string(), - vec![StudentFile { - path: dir.path().join("bob_lab5.py"), - language: "python".to_string(), - }], - ), - ]), - }; + let students = vec![ + StudentSubmission::from_files("alice", &[dir.path().join("alice_lab5.py")]), + StudentSubmission::from_files("bob", &[dir.path().join("bob_lab5.py")]), + ]; let specs = vec![test_spec()]; - let results = orchestrator::run_all(&submissions, &specs, &executor, 10, Some(2)).await; + let results = orchestrator::run_all(&students, &specs, &executor, 10, Some(2)).await; assert_eq!(results.len(), 2); - assert!(results.contains_key("alice")); - assert!(results.contains_key("bob")); - let alice = &results["alice"]; + let alice = by_id(&results, "alice"); assert_eq!(alice.status(), TestStatus::Passed); assert_eq!(alice.total_cases(), 4); assert_eq!(alice.total_passed(), 4); - let bob = &results["bob"]; + let bob = by_id(&results, "bob"); assert_eq!(bob.status(), TestStatus::Failed); assert_eq!(bob.total_cases(), 4); // bob returns min instead of max: fails on "3<5" and "negative", passes "equal zero" and "TypeError" @@ -250,19 +243,14 @@ expect = 10 .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_math.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_math.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + + let alice = by_id(&results, "alice"); assert_eq!(alice.total_cases(), 1); assert_eq!( alice.test_results[0].cases[0].status, @@ -304,18 +292,13 @@ expect = "hello world" .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_echo.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_echo.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!(alice.test_results[0].cases[0].status, TestStatus::Passed); } @@ -353,18 +336,13 @@ expect = 0.001 .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_config.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_config.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!( alice.test_results[0].cases[0].status, TestStatus::Passed, @@ -419,18 +397,13 @@ reference = "{}/solutions/lab5.py" .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_lab5.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_lab5.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!(alice.total_cases(), 10, "Should have 10 generated cases"); assert_eq!( @@ -476,18 +449,13 @@ rhai = "if a >= b { a } else { b }" .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_lab5.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_lab5.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!(alice.total_cases(), 5, "Should have 5 generated cases"); assert_eq!( @@ -544,18 +512,13 @@ expect = 60 .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_proc.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_proc.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!(alice.total_cases(), 1); assert_eq!( @@ -634,18 +597,13 @@ expect = 20.0 .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_hw.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_hw.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!(alice.total_cases(), 2); assert_eq!( @@ -711,18 +669,13 @@ expect = [0, 1, 2] .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_hw.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_hw.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!(alice.total_cases(), 1); assert_eq!( @@ -778,18 +731,13 @@ args = [] .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_hw.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_hw.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!( alice.test_results[0].cases[0].status, @@ -846,18 +794,13 @@ expect = 10 .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_hw.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_hw.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); assert_eq!( alice.test_results[0].cases[0].status, @@ -919,18 +862,13 @@ expect = 5 .unwrap(); let executor = PythonExecutor::new(); - let submissions = SubmissionSet { - by_student: HashMap::from([( - "alice".to_string(), - vec![StudentFile { - path: dir.path().join("alice_hw.py"), - language: "python".to_string(), - }], - )]), - }; - - let results = orchestrator::run_all(&submissions, &[spec], &executor, 10, Some(1)).await; - let alice = &results["alice"]; + let students = vec![StudentSubmission::from_files( + "alice", + &[dir.path().join("alice_hw.py")], + )]; + + let results = orchestrator::run_all(&students, &[spec], &executor, 10, Some(1)).await; + let alice = by_id(&results, "alice"); // With copy_refs=true (default), second case should still see original DATA assert_eq!( @@ -944,3 +882,70 @@ expect = 5 "length should still be 5 because DATA was deepcopied per case" ); } + +/// The seam between the input model and the results: `run_all` is the only place a +/// student's delivery outcome and Canvas id reach `StudentReport`, and it is what makes +/// "a non-submitter is never scored zero" work end to end. +#[tokio::test] +async fn test_run_all_stamps_identity_and_outcome_onto_every_report() { + let dir = setup_test_dir(); + let executor = PythonExecutor::new(); + + let mut alice = StudentSubmission::from_files("alice", &[dir.path().join("alice_lab5.py")]); + alice.roster_match = RosterMatch::Matched(0); + alice.identity.canvas_user_id = Some(101); + alice.identity.name = Some("Alice".to_string()); + + let mut dan_identity = StudentIdentity::number("dan"); + dan_identity.canvas_user_id = Some(105); + let absent = StudentSubmission::not_submitted(dan_identity, 1); + + let students = vec![alice, absent]; + let results = orchestrator::run_all(&students, &[test_spec()], &executor, 10, Some(2)).await; + + assert_eq!(results.len(), 2, "a non-submitter must still get a row"); + + let alice = by_id(&results, "alice"); + assert_eq!(alice.submission_state, Some(SubmissionOutcome::Executable)); + assert_eq!(alice.canvas_user_id, Some(101)); + assert_eq!(alice.student_name.as_deref(), Some("Alice")); + assert!(alice.is_gradeable()); + + let dan = by_id(&results, "dan"); + assert_eq!(dan.submission_state, Some(SubmissionOutcome::NotSubmitted)); + assert_eq!(dan.canvas_user_id, Some(105)); + assert!(dan.test_results.is_empty()); + assert!(!dan.is_gradeable()); + + // And the consumer honours it: no grade, rather than a zero that would be pushed to + // Canvas as if the student had earned it. + let mut graded = results; + apply_grading(&mut graded, &GradingPolicy::default()); + assert!(by_id(&graded, "alice").final_grade.is_some()); + assert_eq!(by_id(&graded, "dan").final_grade, None); +} + +/// A submitter the roster does not list still has runnable code, and a teacher needs that +/// output to work out why the two disagree. +#[tokio::test] +async fn test_a_submitter_absent_from_the_roster_is_still_executed() { + let dir = setup_test_dir(); + let executor = PythonExecutor::new(); + + let mut stranger = StudentSubmission::from_files("alice", &[dir.path().join("alice_lab5.py")]); + stranger.roster_match = RosterMatch::NotInRoster; + assert_eq!(stranger.outcome(), SubmissionOutcome::ReceivedUnmatched); + + let results = orchestrator::run_all(&[stranger], &[test_spec()], &executor, 10, Some(1)).await; + + assert_eq!(results[0].total_cases(), 4, "their tests must still run"); + assert_eq!(results[0].total_passed(), 4); + assert_eq!( + results[0].submission_state, + Some(SubmissionOutcome::ReceivedUnmatched) + ); + // Run, reported — but not graded until the identity clash is resolved. + let mut graded = results; + apply_grading(&mut graded, &GradingPolicy::default()); + assert_eq!(graded[0].final_grade, None); +} diff --git a/docs/plans/2026-09-21-p669-unified-input-model.md b/docs/plans/2026-09-21-p669-unified-input-model.md new file mode 100644 index 0000000..1998780 --- /dev/null +++ b/docs/plans/2026-09-21-p669-unified-input-model.md @@ -0,0 +1,420 @@ +# P-669 — Unified assignment / student / submission input model + +Linear: https://linear.app/acturea/issue/P-669 +Parent: P-663 · Milestone: Canvas 与本地提交可统一导入 + +Revision 4 — rewritten after an adversarial design review (6 blockers, 11 majors), then +corrected after two adversarial reviews of the implementation, and then after the ticket +owner overruled two decisions. **D9 and D10 below are superseded — see "Owner decisions" +at the end.** + +## Scope + +Build the typed input contract that both entry points (Canvas, local) produce and that +everything downstream consumes. **Not** in scope: Canvas HTTP fetching and attachment +download (P-670), XLSX / column mapping (P-672), teacher matching rules (P-673), the +teacher test package (P-674), per-item scoring and zero-vs-ungraded policy (P-677). + +## Decisions + +### D1 — Identity has one total key + +```rust +pub enum StudentKey { + Number(String), // confirmed 学号: roster hit, or Canvas sis_user_id + CanvasUser(u64), // Canvas user with no SIS id + Extracted(String), // token pulled off a local filename, unconfirmed +} +``` + +`Display` renders `2024010001`, `canvas:12345`, `local:notes` — the prefixed forms can +never be mistaken for a 学号. `StudentReport.student_id` carries that rendering, so the db +primary key, the CSV column and the TUI search key stay unambiguous. `student_number`, +`canvas_user_id`, `sis_user_id` and `login_id` remain separate optional provenance fields +on `StudentIdentity`; the key never replaces them. + +Ordering is `(StudentKey, canvas_user_id, first_path)` — total, and independent of +`read_dir` order, so duplicates sort stably. + +### D2 — Key comparison is exact text after a trim, on every adapter + +Trim ASCII/Unicode whitespace and strip a UTF-8 BOM. Never case-fold, never parse as an +integer, never strip zeros. `sis_user_id` deserializes as `Option` only: a payload +sending `"sis_user_id": 24010003` unquoted is a type error, not a silent coercion, and a +test pins that. + +`"012345"` and `"12345"` are different students and are never merged. Because keys are +exact text they cannot collide, so the diagnostic is named for what it is — a suspicion +that an upstream export dropped padding: + +> one pass over the sorted students, bucketed by zero-stripped key; every bucket holding +> more than one distinct raw key emits one `SuspectedZeroPaddedVariant { keys }` at +> Warning. Keys stay distinct. + +The asymmetric case (roster has `24010003`, submission carries `0024010003`) therefore +yields a `NotInRoster` submission **and** a `NotSubmitted` roster entry, plus that +diagnostic linking them. That is the 明确结果; auto-repair is deliberately not attempted. + +### D3 — The two axes are orthogonal; the ticket's four outcomes are computed + +Storing delivery state and roster state in one enum lets them contradict each other +(`ReceivedUnmatched` + `Matched`), and hides `SubmittedEmpty` whenever a non-roster student +submits nothing usable. Stored separately: + +```rust +enum SubmissionState { NotSubmitted, SubmittedEmpty, Executable } // delivery only +enum RosterMatch { Matched(usize), Ambiguous(Vec), NotInRoster, NoRoster } +``` + +`Executable` = at least one file with a recognised language (P-674 may tighten it). +The ticket's four outcomes come out of a method, so the pair can never disagree: + +```rust +fn outcome(&self) -> SubmissionOutcome // NotSubmitted | SubmittedEmpty | ReceivedUnmatched | Executable +``` + +`ReceivedUnmatched` dominates for *reporting*: anything received from someone not on the +roster is unmatched regardless of whether its files would run. Execution is gated on the +delivery axis instead, so an unmatched submitter's code still runs — a teacher needs that +output to resolve the clash — and `apply_grading` withholds the grade. + +`NotSubmitted` is produced only by `not_submitted()`, which always sets `Matched` or +`Ambiguous`, so `(NotSubmitted, NotInRoster)` does not arise in tree. `received()` carries a +`debug_assert!` against the empty-attempt case that would otherwise let a caller construct +it. + +### D4 — The runner takes `&[StudentSubmission]`, not `AssignmentInput` + +Handing the executor the whole `AssignmentInput` would put `workflow_state`, `late`, +`missing`, `excused`, attachment URLs and `login_id` inside the core scoring path — exactly +the 入口特有字段 the ticket bars. `run_all(&[StudentSubmission], &[TestSpec], …)` keeps +`AssignmentInput` a boundary object P-673/P-677 can extend without touching the runner, and +keeps the 12 integration-test sites to one line each. + +One report per input element, **in input order** (today's spawn order is HashMap-random). +A `JoinError` produces an error-state report rather than a student silently disappearing. + +### D5 — Nothing is silently dropped, and the anti-overwrite rule holds past RAM + +- Extracted key, not in roster → retained student, `outcome() == ReceivedUnmatched`. +- No extractable key → `unmatched: Vec`. +- Recognisable owner, unusable file type → `IgnoredFile` diagnostic; an owner with only + ignored files is `SubmittedEmpty`. +- Two archive entries flattening to one name → `ArchiveNameCollision`, not a silent skip. + +Retaining duplicates in memory is pointless if SQLite merges them again. `save_session` +currently writes `INSERT OR REPLACE` into a table with `UNIQUE(session_id, student_id)` +(`db/results.rs:51`, `db/schema.rs:36`), and `import_roster` upserts on +`id TEXT PRIMARY KEY` while counting rows *iterated* rather than stored. So: duplicate +`student_id`s are rejected before any write with a hard error (fail-fast, per CLAUDE.md), +and `import_roster` returns rows actually stored. No schema change. + +`avg_grade` currently divides a `filter_map(final_grade)` sum by `reports.len()` +(`db/results.rs:37-41`) — with ungraded students that average is wrong; divide by the +graded count. + +### D6 — A non-submitter is never given a numeric zero + +`apply_template` and `apply_formula` both open with +`if report.status() == TestStatus::Missing { final_grade = Some(0.0) }` +(`grading.rs:19-22`, `63-66`), and an empty `test_results` *is* `Missing`. Flowing +non-submitters through unchanged would write a hard 0.0 into the summary, the CSV, the db +average — and `cmd_grades_push` pushes every report with `final_grade.is_some()` +(`main.rs:562-574`), so a routine `grade` + `grades-push` would post zeros to Canvas for +students who never submitted. + +Rule: `apply_grading` skips any report whose `submission_state` is present and not +`Executable`, leaving `final_grade: None`. The guard is on `submission_state`, **not** on +`status() == Missing` — a student who did submit but whose specs produced nothing keeps +today's behaviour, and legacy reports (`submission_state: None`) are untouched. +`cmd_grades_push` filters on `Executable` and keys on `canvas_user_id`, never on +`student_id.parse::()`. Turning any non-executable outcome into a number is P-677. + +### D7 — Canvas payload types live in `input/canvas.rs`; `canvas/client.rs` is untouched + +`canvas/mod.rs` re-exports only `{CanvasClient, CanvasError}`, so its `CanvasUser` / +`CanvasSubmission` are unreachable from an adapter or a test; and `CanvasSubmission` is a +grade-push response shape (`{id, user_id, score, grade}`) with no `submission_history`, +`submission_type` or `body`. P-669 defines its own serde payload structs mirroring the API +JSON. P-670 maps its HTTP DTOs into them — it owns that wiring, not this ticket. + +The core stays source-neutral: `source_status: Option` (only Canvas fills +it), and `Attachment.url` / `content_type` / `size` are `Option` — `late: false` on a local +submission is a false statement, not a neutral default. + +### D8 — Attachments become files only once they are on disk + +`normalize()` is pure and download is P-670, so attachments have no local path of their +own. Signature: + +```rust +fn normalize(payload: &CanvasPayload, roster: Option<&Roster>, downloads: &HashMap) -> AssignmentInput +``` + +`downloads` maps attachment id → downloaded path. The fixture fills it from committed +files; P-670 fills it after downloading. An attachment absent from `downloads` yields a +`PendingDownload` diagnostic and does **not** make a student `Executable`. Provenance runs +the whole chain: `FileOrigin::Attachment { attempt, attachment_id }` sits beside `Direct` +and `Archive`. + +`SubmittedEmpty` = no attachments **and** no usable body. A Canvas `online_text_entry` +carrying a non-empty body but no file gets its own diagnostic rather than being mislabelled. + +### D9 — ~~The roster is the roster of record whenever one is supplied~~ (superseded) + +Canvas `users` supplies identity enrichment only (name, `sis_user_id`, `login_id`); it +never creates or removes membership. With `roster: None` on the Canvas path, enrollment +*is* the roster and `NoRoster` is not used. + +`pull_roster` collapses `sis_user_id` / `login_id` / stringified Canvas id into one key with +a silent `insert` (`canvas/client.rs:110-117`) and `save_roster_csv` writes no Canvas id — +that path is deliberately **not** migrated here; it moves in P-670, and until then it can +feed non-conforming keys into the model. + +### D10 — ~~Grading-item identity is the existing one; no new dead type~~ (superseded) + +The repo already has a grading unit with an identity and a live association: +`TestSpec.meta.name` → `TestResult.spec_name` (`orchestrator.rs:229`). Adding a parallel +`GradingItem` that nothing references would be dead code, and `points` pre-empts P-677. +P-669 records the association and stops there. + +`AssignmentInfo` does gain `canvas_course_id`, `canvas_assignment_id` and `attempt_policy`, +because the ticket demands course/assignment ids be stored separately — and those are wired +for real: `load_assignment_config` has zero callers today, so `cmd_grade` / `cmd_run` gain +`--assignment `, defaulting to `assignment.toml` beside the tests dir. An unread +config key is a knob that silently does nothing. + +`AttemptPolicy::Latest` = highest `attempt` integer; `submitted_at` is a documented +tie-break only. The workspace has no date library, so a timestamp comparison would be a +lexicographic string compare — correct only for uniform UTC and silently wrong otherwise. + +### D11 — Diagnostics are data, not pre-rendered strings + +`kind: DiagnosticKind` is a data-carrying enum deriving `Display` via `thiserror`, matching +how `RosterError` and `DiagnosticError` already work in these files; there is no separate +`message` field to drift from it. `InputSource` lives on `AssignmentInput` only — a +per-student copy would let an input claim `Local` while holding Canvas students. + +Fail-fast boundary: structural failures (`NotADirectory`, malformed CSV) stay `Err`; +per-record anomalies become diagnostics. Adapters and the validator never print — only +`main.rs` renders. + +### D12 — Extend, never rename + +`StudentFile` (6 uses in `runner/python.rs`, which P-673 owns) and +`StudentReport.student_id` (db schema, CSV headers, TUI, similarity) keep their names. +New `StudentReport` fields are `Option<_>` + `#[serde(default)]`; `submission_state` is +**not** a bare enum with a `Default`, because every legacy record would then claim a state +it never had. Every new enum gets `#[serde(rename_all = "snake_case")]` to match +`TestStatus`. + +`cmd_similarity` and `cmd_report` keep their own `split('_')` key extraction for now — a +recorded decision, not an oversight; routing them through the model is a follow-up. + +## Model (`models/submission.rs`) + +``` +Assignment { name, canvas_course_id: Option, canvas_assignment_id: Option } +StudentKey { Number(String) | CanvasUser(u64) | Extracted(String) } +StudentIdentity { key, student_number, canvas_user_id, sis_user_id, login_id, name, sortable_name, email } +Attachment { id, filename, content_type: Option, size: Option, url: Option } +SourceStatus { workflow_state, late, missing, excused } // Canvas only +SubmissionAttempt { attempt, submitted_at, source_status: Option, attachments, files } +StudentFile { path, language, origin: FileOrigin } +FileOrigin { Direct | Archive { archive, entry } | Attachment { attempt, attachment_id } } +SubmissionState { NotSubmitted | SubmittedEmpty | Executable } +RosterMatch { Matched(usize) | Ambiguous(Vec) | NotInRoster | NoRoster } +SubmissionOutcome { NotSubmitted | SubmittedEmpty | ReceivedUnmatched | Executable } // computed +StudentSubmission { identity, roster_match, state, attempts, selected: Option } +UnmatchedArtifact { path, reason } +InputSource { Canvas { course_id, assignment_id } | Local { scanned_dirs, roster_path } } +InputDiagnostic { severity, kind: DiagnosticKind, location: Option } +SourceLocation { file: Option, sheet: Option, row: Option } +AssignmentInput { assignment, source, roster: Option, students: Vec<_>, unmatched, diagnostics } +``` + +`files` is **not** a fourth parallel copy of the same bytes: `StudentFile`s live on the +attempt, and `StudentSubmission::files()` reads through `selected`. A submission whose +files come from attempt 1 while `selected` points at attempt 2 is therefore not +representable. + +`diagnostics` and `unmatched` are sorted by `(location, kind)` — `read_dir` order is not +stable across filesystems, so an unsorted vector would be flaky in CI. + +## Equivalence: a projection, asserted against an absolute table + +```rust +struct ProjectedStudent { key: String, outcome: SubmissionOutcome, artifacts: Vec } +``` + +`key` is `StudentKey::raw()`, not its `Display` form. Whether a student number counts as +*confirmed* is a property of the source — Canvas vouches for its own SIS ids, a local +filename token vouches for nothing — so comparing the prefixed form would report a +difference in confidence as a difference in identity. + +A cross-source `assert_eq!` alone can pass on two empty vectors — which is exactly what a +gitignored fixture tree produces in CI. So the test asserts each side against a +hand-written expected table **first**, checks `students.len()` on both, and only then +compares the two. + +The one shape difference is deliberate and documented in the test: the local fixture names +files `{sid}_{name}` (the real Canvas bulk-download convention) while a Canvas attachment +carries `{name}`, so the test strips a leading `{key}_` before comparing. That +canonicaliser lives in the test, not the model — it is a matching rule, and matching is +P-673. + +## Fixture design + +`crates/scriptmark/tests/fixtures/hw1/` — `.gitignore` gained +`!crates/scriptmark/tests/fixtures/**` (verified with `git add -n`: the bare `submissions`, +`*.csv`, `templates` and `config.toml` rules would otherwise swallow the tree, and the test +would then pass on two empty vectors in CI). + +| Key | Canvas | Local | Expected | +|---|---|---|---| +| `2024010001` | 1 attachment, downloaded | `2024010001_lab1.py` | `Executable`; roster has two rows for it → `RosterMatch::Ambiguous` + `DuplicateRosterEntry`, and no name is guessed | +| `2024010002` | attempts 1 and 2, different attachments | `2024010002_lab1.py` | `Executable`, `selected.attempt == 2` | +| `0024010003` / `24010003` | both present | both | distinct students, `SuspectedZeroPaddedVariant` | +| `2024010004` | placeholder row: `workflow_state: unsubmitted`, `attempt: null` | roster only | `NotSubmitted` | +| `2024010005` | `workflow_state: submitted`, `attempt: 1`, `attachments: []` | `2024010005_notes.txt` | `SubmittedEmpty` | +| `9999999999` | submitted, enrolled, absent from roster CSV | `9999999999_lab1.py` | `ReceivedUnmatched` | +| — | — | `_scratch_v2.py` | `unmatched` artifact | + +Only the local side can have orphan files: Canvas attributes every attachment to a user by +construction, so `unmatched` is asserted per side rather than compared across them. + +`_scratch_v2.py` — the leading underscore matters. `extract_sid` is `stem.split('_').next()` +and returns `None` only for an empty first token, as its own test pins; `scratch_v2.py` +would yield a student keyed `scratch`. The `_`-split rule is a placeholder P-673 replaces, +so these expectations are provisional. + +Canvas-only unit tests (unreachable from the equivalence fixture): `sis_user_id: null` → +retained under `CanvasUser`, `MissingStudentNumber`; unquoted `"sis_user_id": 24010003` → +deserialize error; no-roster → every entry `NoRoster`, zero `NotSubmitted`; a Canvas value +with a trailing space matching a trimmed CSV value. + +The local adapter test copies the fixture tree into a `tempfile::tempdir()` and builds the +zip there. `discover_submissions` writes `.scriptmark_extracted/` next to its input +(`discovery.rs:39`), which is gitignored and would accumulate invisibly in the source tree, +its stale-skip would freeze what later runs see, and parallel `#[test]` threads would race +on one extraction directory. + +Two further tests: a legacy `results.json` still deserializes with +`submission_state.is_none()`; and running the local adapter twice over the same tempdir +produces byte-identical `serde_json::to_string(&input)`. + +## Touch list + +| File | Change | +|---|---| +| `models/submission.rs` | The model above; delete `SubmissionSet` | +| `models/config.rs` | `AssignmentInfo` += canvas ids + `attempt_policy` | +| `models/result.rs` | `StudentReport` += `canvas_user_id`, `submission_state` (both `Option`); `derive(Default)` | +| `roster.rs` | `Roster { entries, diagnostics }`, `RosterEntry { key: StudentKey, … }`, `lookup() -> Unique/Ambiguous/Missing` | +| `discovery.rs` | Local adapter → `AssignmentInput`; archive manifest for provenance; no silent `continue` | +| `input/mod.rs`, `input/canvas.rs` | Canvas payload structs + pure `normalize()` | +| `grading.rs` | Skip non-`Executable`; two `StudentReport` literals | +| `runner/orchestrator.rs` | `run_all(&[StudentSubmission], …) -> Vec`, input order | +| `runner/oracle.rs` | `StudentFile::direct()` | +| `main.rs` | grade/run via adapter; `--assignment`; `cmd_summarize`; both import-roster sites; `cmd_grades_push` on `canvas_user_id`; print diagnostics | +| `db/roster.rs` | `import_roster(&Roster)`, count stored rows, populate `canvas_id` | +| `db/results.rs` | Reject duplicate `student_id` before write; fix `avg_grade` denominator | +| `db/mod.rs` | 2 `import_roster` call sites, 3 `StudentReport` literals | +| `scriptmark-py/src/lib.rs` | `run()` returns a list; `discover()` keeps its dict, documented lossy; add `load_input()` | +| `tests/integration.rs` | 12 `SubmissionSet` + 15 `StudentFile` literals, 28 map assertions | +| `crates/scriptmark/tests/input_equivalence.rs` | New | +| `.gitignore` | `!crates/scriptmark/tests/fixtures/**` | + +Ergonomics matter here — 40-odd construction sites get rewritten, and if building a +one-student input in a test is painful, P-673/P-674 will rewrite these files again. +So: `StudentFile::direct(path, language)`, `StudentSubmission::from_files(key, paths)`, +`derive(Default)` on `StudentReport`, `by_id(&[StudentReport], &str)`, and +`Roster::from_pairs(&[(id, name)])`. + +## Release note + +`models/mod.rs` does `pub use submission::*`, so `SubmissionSet`, `StudentFile`'s field set +and `run_all`'s signature are all public API of the crate published at 0.2.0, and +`scriptmark.run()` changes from dict to list. Bump both `Cargo.toml` and `pyproject.toml` +to 0.3.0 and update the README's Python example. + +## Verification — the exact CI commands + +``` +cargo fmt --check +cargo clippy --all-targets -- -D warnings +cargo test # workspace root, all members + doctests +``` + +`-p scriptmark` is not enough: it never compiles `scriptmark-py`, which calls both +`discover_submissions` and `run_all`. Locally `scriptmark-py` cannot build because the +machine's Python is 3.14 and pyo3 0.24 tops out at 3.13 (pre-existing, unrelated), so the +local substitute is +`PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1 cargo clippy --all-targets -p scriptmark-py -- -D warnings`. + +## Post-review corrections + +An adversarial review of the first commit found three defects that made a student vanish +or a whole run fail, and nine narrower gaps. All are fixed; the notes below record the ones +that changed a decision rather than just the code. + +- **A roster number is one student, however many rows name it.** `not_submitted` takes + every matching row (`Matched` or `Ambiguous`) instead of one index. Before, a duplicate + row for a student who had not submitted produced one student *per row*, two reports + sharing a `student_id`, and an aborted `grade --db` after the whole class had run. +- **`RosterEntry` is keyed by `StudentKey`, not a bare string.** A Canvas enrollee with no + SIS id could not be represented as a roster row at all, so a SIS-less non-submitter + disappeared while a diagnostic claimed they had been kept. +- **Execution is gated on `state`, not `outcome()`.** `ReceivedUnmatched` collapses the two + axes for *reporting*; gating on it meant an unmatched submitter's runnable code was never + executed. They now run and are reported, and `apply_grading` still withholds the grade. +- **An infrastructure failure is `StudentReport.error`, not a synthetic test case.** The + `JoinError` arm used to fabricate a `scriptmark/run` case, which scored like a genuine + 0% and was pushed to Canvas as a defensible-looking 60. +- **Archive entries are validated before they are recorded.** A rejected entry used to + reserve the flattened output name its owner's real file needed, and skip diagnostics + appeared only on a first scan — the "byte-identical across runs" invariant above did not + actually hold for a directory containing an archive. +- **An archive that yields nothing still registers its owner**, so a truncated upload is + `SubmittedEmpty` rather than 缺交; and `load_roster` reports the rows it cannot use. +- **Consumers resolve both id forms.** The db joins use `substr`, not `ltrim` — `ltrim` + strips a character *set*, so `local:alice` became `ice` — and `summarize` parses the key + back via `StudentKey::parse` rather than comparing text. +- **The CSV archive emits a row per student**, so it covers the same cohort as the JSON + archive instead of dropping every non-submitter. + +## Owner decisions (supersede D9 and D10) + +Both were judgement calls I made and the owner reversed. They are the ones P-670/P-672/ +P-673 should build on. + +### 评分项 is a type, not an implicit convention (supersedes D10) + +`GradingItem { id, title }` exists; `Assignment` holds `items`; `TestResult.item_id` +references it and reads `spec_name` from older results files. `id` is still the test spec's +`[meta] name` — the point is that the assignment now *declares* its items instead of the +concept living only in a string copied between two structs. + +`assignment.toml` may declare `[[items]]` to give them titles. Undeclared, they are derived +from the specs that loaded. A declared item with no spec, or a spec that is not a declared +item, is reported. `points` and weighting stay P-677's. + +### Canvas decides membership (supersedes D9) + +On the Canvas path the roster is the **union** of course enrollment and whatever the teacher +supplied, each row marked with its `RosterSource`. Enrollment decides membership — P-663 +and P-670 both put Canvas in charge of student attribution — so a student Canvas lists but a +stale CSV omits is a member, not a stranger. A supplied row Canvas has never heard of is +kept and flagged `NotEnrolled`, so a hand-maintained list cannot lose people either. + +Three consequences to design against: + +- A CSV row duplicated for somebody Canvas lists once resolves to that one enrollment; the + duplicate is reported as a data-entry error rather than making the match ambiguous. + Locally, where the CSV is the only source, it stays `Ambiguous`. +- `ReceivedUnmatched` is reachable on Canvas only for a submission from somebody the course + does not list. The two sources therefore reach it by different routes, and the fixture + asserts it per source rather than across them — the cross-source projection covers the six + students both sources genuinely agree on. +- An ambiguous key yields no `canvas_user_id`: with two accounts claiming one 学号, taking + either would make an identity depend on the order the payload happened to arrive in. diff --git a/pyproject.toml b/pyproject.toml index 7e8a94e..6e6f847 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "maturin" [project] name = "scriptmark" -version = "0.2.0" +version = "0.3.0" description = "Automated grading CLI for student programming assignments" readme = "README.md" license = { text = "GPL-3.0-or-later" } diff --git a/python/scriptmark/__init__.py b/python/scriptmark/__init__.py index 1f352f0..d288605 100644 --- a/python/scriptmark/__init__.py +++ b/python/scriptmark/__init__.py @@ -3,6 +3,7 @@ from scriptmark._scriptmark import ( discover, grade, + load_input, load_spec, run, StudentResult, @@ -12,6 +13,7 @@ __all__ = [ "discover", "grade", + "load_input", "load_spec", "run", "StudentResult",