From 9bbabf997fce5b568c1b8dba187de15de075c2e4 Mon Sep 17 00:00:00 2001 From: Acture Date: Wed, 7 Oct 2026 21:56:42 +0800 Subject: [PATCH 1/5] feat: import local rosters and explicit submissions (OSS-155) --- CLAUDE.md | 2 + README.md | 50 ++ examples/bundles/local_import/README.md | 33 ++ examples/bundles/local_import/assignment.toml | 18 + examples/bundles/local_import/manifest.toml | 22 + examples/bundles/local_import/roster.csv | 5 + .../local_import/submissions/001_work.py | 2 + .../local_import/submissions/002_work.py | 2 + .../bundles/local_import/tests/double.toml | 10 + src/cli/Cargo.toml | 1 + src/cli/src/main.rs | 80 ++- src/cli/tests/local_import.rs | 501 ++++++++++++++++++ src/core/Cargo.toml | 4 +- src/core/src/assignment.rs | 3 + src/core/src/discovery.rs | 196 +++---- src/core/src/input/local.rs | 207 ++++++++ src/core/src/input/mod.rs | 2 + src/core/src/input/table.rs | 268 ++++++++++ src/core/src/models/config.rs | 2 + src/core/src/models/submission.rs | 18 + src/core/src/roster.rs | 146 +---- 21 files changed, 1315 insertions(+), 257 deletions(-) create mode 100644 examples/bundles/local_import/README.md create mode 100644 examples/bundles/local_import/assignment.toml create mode 100644 examples/bundles/local_import/manifest.toml create mode 100644 examples/bundles/local_import/roster.csv create mode 100644 examples/bundles/local_import/submissions/001_work.py create mode 100644 examples/bundles/local_import/submissions/002_work.py create mode 100644 examples/bundles/local_import/tests/double.toml create mode 100644 src/cli/tests/local_import.rs create mode 100644 src/core/src/input/local.rs create mode 100644 src/core/src/input/table.rs diff --git a/CLAUDE.md b/CLAUDE.md index 1191f3a..26fcace 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -114,6 +114,7 @@ Zero and withheld grades stay distinct. See the notes' teacher contract for poli - `src/core/src/runner/orchestrator.rs` — Runs prepared bundles per student, tokio parallel. - `src/core/src/models/spec.rs` — TOML test specification types. - `src/core/src/discovery.rs` — File discovery and archive extraction with size/count limits. +- `src/core/src/input/local.rs` and `input/table.rs` — Assignment-relative local sources, explicit student/file manifests and CSV/XLSX roster mappings. Spreadsheet student IDs must be text; preserve row/sheet diagnostics. - `src/core/src/record.rs` — Versioned grading evidence and score revisions, including reuse checks. - `src/core/src/grading.rs` — GradingPolicy dispatch (templates + Rhai formulas). - `src/cli/src/main.rs` — CLI command handlers and application orchestration. @@ -126,6 +127,7 @@ Zero and withheld grades stay distinct. See the notes' teacher contract for poli - Integration tests spawn real Python processes — need `python3` available - `scriptmark-core` exposes grading models and operations; CLI adapters import it directly, without compatibility re-exports. - Core's `test-support` feature exposes shared graded/withheld report fixtures for adapter tests; it is enabled only by the CLI's dev-dependency. +- `[input]` in `assignment.toml` configures either submission paths plus a mapped CSV/XLSX roster, or `[[input.students]]` with exact files/archives. `grade`, `run` and `match` resolve the same input; rescore reconstructs it from saved CLI overrides and the recorded assignment file. File and function matching remains owned by `matching`. - Publish both Rust packages with `cargo publish --workspace`, which orders workspace dependencies before their consumers. - Platform-specific code uses `#[cfg(target_os = "macos")]` / `#[cfg(target_os = "linux")]` for rlimit types diff --git a/README.md b/README.md index fbc6523..c85c253 100644 --- a/README.md +++ b/README.md @@ -201,6 +201,56 @@ scriptmark tui grades.db ## TOML Test Specs +### Local input configuration + +`grade`, `run` and `match` can read submission paths and a CSV/XLSX roster from +`assignment.toml`. Configured paths are relative to that file. See the +[runnable local-import example](examples/bundles/local_import/README.md). + +```toml +[input] +submissions = ["submissions"] + +[input.roster] +path = "roster.xlsx" +sheet = "学生" +header_row = 2 + +[input.roster.columns] +student_id = "学号" +name = "姓名" +# canvas_user_id = "Canvas ID" # optional; distinct from the student number +``` + +Use `.csv` and omit `sheet` for CSV. Header rows and numeric column selectors start +at 1; a column selector can also be an exact heading. Without a mapping, the default +layout is `name,student_id` or `name,unused,student_id[,canvas_user_id]`. XLSX student +IDs must be text cells; numeric cells are rejected because their original leading +zeros cannot be established. Workbook files with several sheets need `sheet`. + +For an explicit class and file list, replace `[input]`/`[input.roster]` with: + +```toml +[[input.students]] +student_id = "00123" +name = "张三" +files = ["handins/answer.py"] # individual files or archives; [] means no submission +``` + +An explicit list cannot be combined with a roster table or submission directory. +It uses the same ownership and item matching component. Invalid identities/paths +stop grading and remain visible in `match` diagnostics. An identical repeated roster +row is reported and merged; conflicting rows are refused. CLI submission paths and +`--roster` override configured paths in table mode. To use only configured sources: + +```fish +scriptmark match -t tests/ --assignment assignment.toml +scriptmark grade -t tests/ --assignment assignment.toml +scriptmark export output/results.json -o grades.xlsx +``` + +### Test definitions + Teachers can configure student ownership, item file patterns, function aliases and per-student overrides in `assignment.toml`. Preview decisions and candidates with `scriptmark match`; unresolved conflicts withhold grades. See diff --git a/examples/bundles/local_import/README.md b/examples/bundles/local_import/README.md new file mode 100644 index 0000000..6690531 --- /dev/null +++ b/examples/bundles/local_import/README.md @@ -0,0 +1,33 @@ +# Local roster or explicit submission list + +From the repository root: + +```fish +cargo run -p scriptmark -- match -t examples/bundles/local_import/tests -o output/local-matches.json +cargo run -p scriptmark -- grade -t examples/bundles/local_import/tests -o output/local.json +cargo run -p scriptmark -- export output/local.json -o output/local-grades.xlsx + +# The same class, with every submitted file assigned explicitly. +cargo run -p scriptmark -- grade -t examples/bundles/local_import/tests --assignment examples/bundles/local_import/manifest.toml -o output/manifest.json +``` + +`001` earns full points, `002` earns zero, and `003` remains ungraded because no work +was submitted. Student numbers remain text, including their leading zeros. + +`assignment.toml` selects row 2 as the CSV header and maps its Chinese headings. +To use an Excel roster, save the same table as `roster.xlsx`, keep the student ID +column as **text**, change `input.roster.path`, and set `input.roster.sheet` to the +worksheet name. Multiple-sheet workbooks require an explicit sheet. Columns accept +either exact header names or one-based column numbers. + +Paths in either configuration are relative to that configuration file. CLI paths +are relative to the working directory; positional submissions and `--roster` override +the corresponding configured paths. The explicit `input.students` list is a separate +input mode and cannot be combined with a roster table or submission directories. +It accepts individual files and archives; `files = []` records a non-submitter. +File-to-item and function matching still use the normal `[matching]` rules. + +Run `match` to see import diagnostics without executing student code. Invalid rows, +numeric Excel student IDs and invalid/duplicate submission paths stop grading, with +the source file, worksheet and row where available. Repeated identical roster IDs +are reported and merged; conflicting identities stop grading. diff --git a/examples/bundles/local_import/assignment.toml b/examples/bundles/local_import/assignment.toml new file mode 100644 index 0000000..fac8765 --- /dev/null +++ b/examples/bundles/local_import/assignment.toml @@ -0,0 +1,18 @@ +[assignment] +name = "Local import example" + +[input] +submissions = ["submissions"] + +[input.roster] +path = "roster.csv" +header_row = 2 + +[input.roster.columns] +student_id = "学号" +name = "姓名" + +[[items]] +id = "double" +points = 10 +aggregation = "proportional" diff --git a/examples/bundles/local_import/manifest.toml b/examples/bundles/local_import/manifest.toml new file mode 100644 index 0000000..802d409 --- /dev/null +++ b/examples/bundles/local_import/manifest.toml @@ -0,0 +1,22 @@ +[assignment] +name = "Local import example" + +[[input.students]] +student_id = "001" +name = "张三" +files = ["submissions/001_work.py"] + +[[input.students]] +student_id = "002" +name = "李四" +files = ["submissions/002_work.py"] + +[[input.students]] +student_id = "003" +name = "王五" +files = [] + +[[items]] +id = "double" +points = 10 +aggregation = "proportional" diff --git a/examples/bundles/local_import/roster.csv b/examples/bundles/local_import/roster.csv new file mode 100644 index 0000000..4fbad1b --- /dev/null +++ b/examples/bundles/local_import/roster.csv @@ -0,0 +1,5 @@ +示例班级 +姓名,学号 +张三,001 +李四,002 +王五,003 diff --git a/examples/bundles/local_import/submissions/001_work.py b/examples/bundles/local_import/submissions/001_work.py new file mode 100644 index 0000000..8c70e01 --- /dev/null +++ b/examples/bundles/local_import/submissions/001_work.py @@ -0,0 +1,2 @@ +def double(x: int) -> int: + return x * 2 diff --git a/examples/bundles/local_import/submissions/002_work.py b/examples/bundles/local_import/submissions/002_work.py new file mode 100644 index 0000000..6fe66e3 --- /dev/null +++ b/examples/bundles/local_import/submissions/002_work.py @@ -0,0 +1,2 @@ +def double(x: int) -> int: + return x diff --git a/examples/bundles/local_import/tests/double.toml b/examples/bundles/local_import/tests/double.toml new file mode 100644 index 0000000..c64cf7c --- /dev/null +++ b/examples/bundles/local_import/tests/double.toml @@ -0,0 +1,10 @@ +[meta] +name = "double" +file = "work.py" +function = "double" +language = "python" + +[[cases]] +name = "positive" +args = [3] +expect = 6 diff --git a/src/cli/Cargo.toml b/src/cli/Cargo.toml index ed58f79..a9920e7 100644 --- a/src/cli/Cargo.toml +++ b/src/cli/Cargo.toml @@ -32,6 +32,7 @@ ratatui = { workspace = true } crossterm = { workspace = true } [dev-dependencies] +rust_xlsxwriter = { workspace = true } scriptmark-core = { workspace = true, features = ["test-support"] } tempfile = "3" zip = { workspace = true } diff --git a/src/cli/src/main.rs b/src/cli/src/main.rs index bbe59b0..2869f92 100644 --- a/src/cli/src/main.rs +++ b/src/cli/src/main.rs @@ -83,7 +83,7 @@ struct FrozenArgs { #[derive(Parser)] struct GradeArgs { /// Directories containing student submissions - #[arg(required_unless_present = "canvas", conflicts_with = "canvas")] + #[arg(conflicts_with = "canvas")] submissions: Vec, /// Grade from a Canvas bundle written by `scriptmark canvas fetch`. @@ -100,7 +100,7 @@ struct GradeArgs { #[arg(short, long, default_value = "output/results.json")] output: PathBuf, - /// Path to roster CSV (name,_,student_id) + /// Roster CSV or XLSX; configure its layout in assignment.toml [input.roster] #[arg(short, long)] roster: Option, @@ -142,7 +142,7 @@ struct GradeArgs { #[derive(Parser)] struct RunArgs { /// Directories containing student submissions - #[arg(required_unless_present = "canvas", conflicts_with = "canvas")] + #[arg(conflicts_with = "canvas")] submissions: Vec, /// Grade from a Canvas bundle written by `scriptmark canvas fetch`. @@ -159,7 +159,7 @@ struct RunArgs { #[arg(short, long, default_value = "output/results.json")] output: PathBuf, - /// Path to roster CSV (name,_,student_id) + /// Roster CSV or XLSX; configure its layout in assignment.toml [input.roster] #[arg(short, long)] roster: Option, @@ -191,7 +191,7 @@ struct RunArgs { #[derive(Parser)] struct MatchArgs { - #[arg(required_unless_present = "canvas", conflicts_with = "canvas")] + #[arg(conflicts_with = "canvas")] submissions: Vec, #[arg(long)] canvas: Option, @@ -454,23 +454,18 @@ enum DbAction { fn build_local_input( submissions: &[PathBuf], declared: &Declared, - roster_path: Option<&PathBuf>, + resolved: &scriptmark_core::input::local::Resolved, purpose: Purpose, ) -> Result { let (assignment, attempt_policy) = (declared.assignment.clone(), declared.attempt_policy); - 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(), + roster: resolved.roster.as_ref(), attempt_policy, - matching: Some(&declared.matching), + matching: Some(&resolved.matching), }, ) .context("Failed to discover submissions")?; @@ -757,6 +752,11 @@ fn prepare_batch( // Names, roster membership and submission state all come from the model, so there is // no separate roster merge afterwards. + if canvas.is_some() && !declared.input.is_empty() { + anyhow::bail!( + "--canvas cannot be combined with assignment.toml [input]; use a separate local assignment configuration" + ); + } let (input, source) = match &canvas { Some(bundle) => ( build_canvas_input(bundle, &declared, purpose)?, @@ -764,13 +764,23 @@ fn prepare_batch( bundle: bundle.clone(), }, ), - None => ( - build_local_input(&submissions, &declared, roster.as_ref(), purpose)?, - record::Source::Local { - dirs: submissions, - roster, - }, - ), + None => { + let resolved = declared.input.resolve( + declared.path.as_deref(), + &submissions, + roster.as_deref(), + &declared.matching, + )?; + let input = build_local_input(&resolved.paths, &declared, &resolved, purpose)?; + declared.matching = resolved.matching; + ( + input, + record::Source::Local { + dirs: submissions, + roster, + }, + ) + } }; Ok(Batch { input, @@ -1520,14 +1530,28 @@ fn cmd_db(cmd: DbCommand) -> Result<()> { } => { let (saved, view) = load_view(&record, revision.revision)?; let revision = scored(&view, &record)?; - // The roster the record was graded with, when it had one, names its students. - let roster = match &saved.evidence.inputs.source { - record::Source::Local { - roster: Some(path), .. - } => Some(load_roster(path).context("Failed to load roster")?), - _ => None, - }; - save_to_db(&db, &saved, revision, roster.as_ref()) + // Use the identities frozen in the record, not a table that may have changed. + let roster = scriptmark_core::roster::Roster::from_entries( + saved + .evidence + .students + .iter() + .filter_map(|student| { + let key = StudentKey::parse(&student.student_id); + if matches!(key, StudentKey::Extracted(_)) { + return None; + } + Some(scriptmark_core::roster::RosterEntry { + key, + name: student.student_name.clone(), + canvas_user_id: student.canvas_user_id, + source: scriptmark_core::roster::RosterSource::Supplied, + location: None, + }) + }) + .collect(), + ); + save_to_db(&db, &saved, revision, Some(&roster)) } DbAction::ImportRoster { roster, db } => { let database = diff --git a/src/cli/tests/local_import.rs b/src/cli/tests/local_import.rs new file mode 100644 index 0000000..b071cc8 --- /dev/null +++ b/src/cli/tests/local_import.rs @@ -0,0 +1,501 @@ +//! The same class through CSV, XLSX or a TOML file list, including the saved-record flow. + +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +use rust_xlsxwriter::Workbook; +use scriptmark_core::assignment; +use scriptmark_core::discovery::{LocalInputOptions, load_local_input}; +use scriptmark_core::input::table::{self, Column, Columns, Options}; +use scriptmark_core::models::{AssignmentInput, DiagnosticKind, SubmissionOutcome}; +use scriptmark_core::record::Record; + +fn write(root: &Path, name: &str, content: &str) -> PathBuf { + let path = root.join(name); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, content).unwrap(); + path +} + +fn setup() -> tempfile::TempDir { + let dir = tempfile::tempdir().unwrap(); + write( + dir.path(), + "submissions/001_work.py", + "def double(x):\n return x * 2\n", + ); + write( + dir.path(), + "submissions/002_work.py", + "def double(x):\n return x\n", + ); + write( + dir.path(), + "tests/double.toml", + "[meta]\nname = 'double'\nfile = 'work.py'\nfunction = 'double'\nlanguage = 'python'\n[[cases]]\nname = 'positive'\nargs = [3]\nexpect = 6\n", + ); + write( + dir.path(), + "roster.csv", + "class roster\n姓名,学号\n张三,001\n李四,002\n王五,003\n", + ); + let mut book = Workbook::new(); + book.add_worksheet() + .set_name("说明") + .unwrap() + .write_string(0, 0, "choose the roster worksheet") + .unwrap(); + let sheet = book.add_worksheet(); + sheet.set_name("学生").unwrap(); + sheet.write_string(0, 0, "class roster").unwrap(); + for (i, (name, id)) in [ + ("姓名", "学号"), + ("张三", "001"), + ("李四", "002"), + ("王五", "003"), + ] + .into_iter() + .enumerate() + { + sheet.write_string(i as u32 + 1, 0, name).unwrap(); + sheet.write_string(i as u32 + 1, 1, id).unwrap(); + } + book.save(dir.path().join("roster.xlsx")).unwrap(); + for format in ["csv", "xlsx"] { + let sheet = if format == "xlsx" { + "sheet = '学生'\n" + } else { + "" + }; + write( + dir.path(), + &format!("{format}.toml"), + &format!( + "[assignment]\nname = 'local example'\n[input]\nsubmissions = ['submissions']\n[input.roster]\npath = 'roster.{format}'\n{sheet}header_row = 2\n[input.roster.columns]\nstudent_id = '学号'\nname = '姓名'\n" + ), + ); + } + write( + dir.path(), + "manifest.toml", + "[assignment]\nname = 'local example'\n[[input.students]]\nstudent_id = '001'\nname = '张三'\nfiles = ['submissions/001_work.py']\n[[input.students]]\nstudent_id = '002'\nname = '李四'\nfiles = ['submissions/002_work.py']\n[[input.students]]\nstudent_id = '003'\nname = '王五'\nfiles = []\n", + ); + dir +} + +fn input(dir: &Path, config: &str) -> AssignmentInput { + let declared = assignment::load(Some(&dir.join(config)), &dir.join("tests")).unwrap(); + let resolved = declared + .input + .resolve(declared.path.as_deref(), &[], None, &declared.matching) + .unwrap(); + load_local_input( + &resolved.paths, + LocalInputOptions { + assignment: declared.assignment, + roster: resolved.roster.as_ref(), + attempt_policy: declared.attempt_policy, + matching: Some(&resolved.matching), + }, + ) + .unwrap() +} + +fn run(dir: &Path, args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_scriptmark")) + .current_dir(dir) + .args(args) + .output() + .unwrap() +} + +fn success(dir: &Path, args: &[&str]) { + let output = run(dir, args); + assert!( + output.status.success(), + "{args:?}\n{}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +fn options() -> Options { + Options { + sheet: None, + header_row: Some(2), + columns: Some(Columns { + student_id: Column::Name("学号".into()), + name: Some(Column::Name("姓名".into())), + canvas_user_id: None, + }), + } +} + +#[test] +fn three_sources_preserve_identity_files_and_non_submitters() { + let dir = setup(); + let inputs: Vec = ["csv.toml", "xlsx.toml", "manifest.toml"] + .iter() + .map(|file| input(dir.path(), file)) + .collect(); + let signature = |input: &AssignmentInput| -> Vec<_> { + input + .students + .iter() + .map(|student| { + ( + student.identity.clone(), + student.outcome(), + student + .selected_attempt() + .map(|attempt| { + attempt + .files + .iter() + .map(|file| file.path.clone()) + .collect::>() + }) + .unwrap_or_default(), + ) + }) + .collect() + }; + assert_eq!(signature(&inputs[0]), signature(&inputs[1])); + assert_eq!(signature(&inputs[0]), signature(&inputs[2])); + assert!(inputs.iter().all(|input| input.errors().next().is_none())); + assert_eq!( + inputs[0] + .with_outcome(SubmissionOutcome::NotSubmitted) + .count(), + 1 + ); + assert_eq!(inputs[0].students[0].identity.key.to_string(), "001"); +} + +#[test] +fn all_sources_grade_export_rescore_and_save_without_reloading_the_roster() { + let dir = setup(); + let mut grades = Vec::new(); + for format in ["csv", "xlsx", "manifest"] { + let config = format!("{format}.toml"); + let record = format!("out/{format}.json"); + success( + dir.path(), + &[ + "grade", + "-t", + "tests", + "--assignment", + &config, + "-o", + &record, + ], + ); + success( + dir.path(), + &["export", &record, "-o", &format!("out/{format}.csv")], + ); + let before = Record::load(&dir.path().join(&record)).unwrap(); + grades.push( + before + .view(None) + .unwrap() + .reports + .iter() + .map(|report| (report.student_id.clone(), report.final_grade())) + .collect::>(), + ); + let content = std::fs::read_to_string(dir.path().join(&config)).unwrap(); + write( + dir.path(), + &config, + &(content + "\n[grading]\nscale = 20\n"), + ); + success(dir.path(), &["rescore", &record]); + assert_eq!( + Record::load(&dir.path().join(&record)) + .unwrap() + .revisions + .len(), + 2 + ); + std::fs::rename( + dir.path().join(&config), + dir.path().join(format!("{config}.saved")), + ) + .unwrap(); + success( + dir.path(), + &["db", "save", &record, "--db", "out/grades.db"], + ); + } + assert_eq!(grades[0], grades[1]); + assert_eq!(grades[0], grades[2]); + let db = scriptmark::db::Database::open(&dir.path().join("out/grades.db")).unwrap(); + assert_eq!( + db.get_student("001").unwrap().unwrap().name.as_deref(), + Some("张三") + ); +} + +#[test] +fn table_mapping_errors_and_numeric_ids_name_the_source() { + let dir = setup(); + let mut configured = options(); + configured.columns.as_mut().unwrap().student_id = Column::Name("wrong heading".into()); + let error = table::load(&dir.path().join("roster.csv"), &configured).unwrap_err(); + assert!(format!("{error:#}").contains("missing column 'wrong heading'")); + let error = table::load(&dir.path().join("roster.xlsx"), &options()).unwrap_err(); + assert!(format!("{error:#}").contains("multiple worksheets")); + let mut book = Workbook::new(); + let sheet = book.add_worksheet(); + sheet.set_name("学生").unwrap(); + sheet.write_string(0, 0, "title").unwrap(); + sheet.write_string(1, 0, "姓名").unwrap(); + sheet.write_string(1, 1, "学号").unwrap(); + sheet.write_string(2, 0, "张三").unwrap(); + sheet.write_number(2, 1, 123.0).unwrap(); + book.save(dir.path().join("numbers.xlsx")).unwrap(); + let roster = table::load(&dir.path().join("numbers.xlsx"), &options()).unwrap(); + let diagnostic = roster.errors().next().unwrap(); + assert_eq!( + diagnostic.location.as_ref().unwrap().sheet.as_deref(), + Some("学生") + ); + assert_eq!(diagnostic.location.as_ref().unwrap().row, Some(3)); + assert!(diagnostic.to_string().contains("text cell")); + assert!(diagnostic.to_string().contains("numbers.xlsx [学生]:3")); +} + +#[test] +fn duplicate_missing_and_malformed_rows_remain_visible() { + let dir = setup(); + write( + dir.path(), + "roster.csv", + "title\n姓名,学号\n张三,001\n张三,001\n另一个人,001\n没有学号,\n缺列\n", + ); + let roster = table::load(&dir.path().join("roster.csv"), &options()).unwrap(); + assert_eq!(roster.entries.len(), 1); + let rows: Vec = roster + .errors() + .map(|diagnostic| diagnostic.location.as_ref().unwrap().row.unwrap()) + .collect(); + assert!( + rows.contains(&5) && rows.contains(&6) && rows.contains(&7), + "{rows:?}" + ); + let output = run( + dir.path(), + &["grade", "-t", "tests", "--assignment", "csv.toml"], + ); + assert!(!output.status.success()); + assert!(!dir.path().join("output/results.json").exists()); +} + +#[test] +fn manifest_paths_are_explicit_and_bad_paths_are_not_missing_submissions() { + let dir = setup(); + write( + dir.path(), + "submissions/999_extra.py", + "raise RuntimeError('must not be imported')\n", + ); + assert_eq!(input(dir.path(), "manifest.toml").student_count(), 3); + let original = std::fs::read_to_string(dir.path().join("manifest.toml")).unwrap(); + write( + dir.path(), + "manifest.toml", + &original.replace("submissions/001_work.py", "absent.py"), + ); + let imported = input(dir.path(), "manifest.toml"); + let diagnostic = imported.errors().next().unwrap(); + assert!( + matches!(&diagnostic.kind, DiagnosticKind::InvalidSubmissionPath { student, .. } if student == "001") + ); + assert!(diagnostic.location.as_ref().unwrap().row.is_some()); + let output = run( + dir.path(), + &["grade", "-t", "tests", "--assignment", "manifest.toml"], + ); + assert!(!output.status.success()); + success( + dir.path(), + &["match", "-t", "tests", "--assignment", "manifest.toml"], + ); +} + +#[test] +fn config_paths_survive_another_working_directory_and_changes_refuse_rescore() { + let dir = setup(); + let elsewhere = tempfile::tempdir().unwrap(); + let tests = dir.path().join("tests"); + let config = dir.path().join("xlsx.toml"); + let output = dir.path().join("result.json"); + success( + elsewhere.path(), + &[ + "grade", + "-t", + tests.to_str().unwrap(), + "--assignment", + config.to_str().unwrap(), + "-o", + output.to_str().unwrap(), + ], + ); + let original = std::fs::read_to_string(&config).unwrap(); + std::fs::write( + &config, + original + .replace("roster.xlsx", "roster.csv") + .replace("sheet = '学生'\n", ""), + ) + .unwrap(); + write( + dir.path(), + "roster.csv", + "title\n姓名,学号\n张三,099\n李四,002\n王五,003\n", + ); + let result = run(elsewhere.path(), &["rescore", output.to_str().unwrap()]); + assert!(!result.status.success()); + assert_eq!(Record::load(&output).unwrap().revisions.len(), 1); +} + +#[test] +fn explicit_archive_ownership_and_conflicts_use_the_existing_matcher() { + use std::io::Write; + let dir = setup(); + let archive = dir.path().join("arbitrary.zip"); + let mut zip = zip::ZipWriter::new(std::fs::File::create(&archive).unwrap()); + zip.start_file("work.py", zip::write::SimpleFileOptions::default()) + .unwrap(); + zip.write_all(b"def double(x: int) -> int:\n return x * 2\n") + .unwrap(); + zip.finish().unwrap(); + let original = std::fs::read_to_string(dir.path().join("manifest.toml")).unwrap(); + write( + dir.path(), + "manifest.toml", + &original.replace("submissions/001_work.py", "arbitrary.zip"), + ); + let imported = input(dir.path(), "manifest.toml"); + assert!(imported.errors().next().is_none()); + assert_eq!(imported.students[0].identity.key.to_string(), "001"); + assert_eq!( + imported.students[0].selected_attempt().unwrap().files.len(), + 1 + ); + success( + dir.path(), + &["run", "-t", "tests", "--assignment", "manifest.toml"], + ); + let config = std::fs::read_to_string(dir.path().join("manifest.toml")).unwrap(); + write( + dir.path(), + "manifest.toml", + &config.replace("submissions/002_work.py", "arbitrary.zip"), + ); + let imported = input(dir.path(), "manifest.toml"); + assert!( + imported + .errors() + .any(|diagnostic| diagnostic.to_string().contains("listed more than once")) + ); + write( + dir.path(), + "manifest.toml", + &(config + "\n[[matching.owners]]\npath = 'arbitrary.zip'\nstudent = '003'\n"), + ); + assert!( + input(dir.path(), "manifest.toml") + .errors() + .any(|diagnostic| matches!(diagnostic.kind, DiagnosticKind::AmbiguousOwner { .. })) + ); +} + +#[test] +fn mapped_columns_and_cli_overrides_do_not_silently_mix_input_modes() { + let dir = setup(); + write( + dir.path(), + "other.csv", + "名单\n编号,姓名\n001,张三\n002,李四\n003,王五\n", + ); + let options = Options { + sheet: None, + header_row: Some(2), + columns: Some(Columns { + student_id: Column::Number(1), + name: Some(Column::Name("姓名".into())), + canvas_user_id: None, + }), + }; + assert_eq!( + table::load(&dir.path().join("other.csv"), &options) + .unwrap() + .entries[0] + .key + .to_string(), + "001" + ); + let config = std::fs::read_to_string(dir.path().join("csv.toml")) + .unwrap() + .replace("'学号'", "'编号'"); + write(dir.path(), "csv.toml", &config); + success( + dir.path(), + &[ + "grade", + "submissions", + "-t", + "tests", + "--assignment", + "csv.toml", + "--roster", + "other.csv", + ], + ); + let output = run( + dir.path(), + &[ + "grade", + "submissions", + "-t", + "tests", + "--assignment", + "manifest.toml", + ], + ); + assert!(!output.status.success()); + assert!(String::from_utf8_lossy(&output.stderr).contains("do not combine")); +} + +#[test] +fn the_shipped_local_import_example_runs_as_documented() { + let dir = tempfile::tempdir().unwrap(); + let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../examples/bundles/local_import"); + for file in ["assignment.toml", "manifest.toml"] { + success( + dir.path(), + &[ + "grade", + "-t", + root.join("tests").to_str().unwrap(), + "--assignment", + root.join(file).to_str().unwrap(), + "-o", + file, + ], + ); + let record = Record::load(&dir.path().join(file)).unwrap(); + let reports = record.view(None).unwrap().reports; + assert_eq!( + reports + .iter() + .map(|report| report.final_grade()) + .collect::>(), + [Some(100.0), Some(0.0), None] + ); + } +} diff --git a/src/core/Cargo.toml b/src/core/Cargo.toml index 548a725..b8586ea 100644 --- a/src/core/Cargo.toml +++ b/src/core/Cargo.toml @@ -26,6 +26,7 @@ anyhow = { workspace = true } regex = { workspace = true } zip = { workspace = true } rust_xlsxwriter = { workspace = true } +calamine = { workspace = true } libc = { workspace = true } rand = { workspace = true } rand_chacha = { workspace = true } @@ -37,6 +38,3 @@ bzip2-rs = "0.1.2" lzma-rs = "0.3.0" tempfile = "3" sha2 = "0.10" - -[dev-dependencies] -calamine = { workspace = true } diff --git a/src/core/src/assignment.rs b/src/core/src/assignment.rs index b26dd9b..5b0da70 100644 --- a/src/core/src/assignment.rs +++ b/src/core/src/assignment.rs @@ -16,6 +16,7 @@ pub struct Declared { pub attempt_policy: AttemptPolicy, pub grading: GradingConfig, pub matching: crate::matching::Config, + pub input: crate::input::local::Config, /// Where it was read from; `None` when there was no file. pub path: Option, } @@ -46,6 +47,7 @@ pub fn load(explicit: Option<&Path>, tests_dir: &Path) -> Result { attempt_policy: AttemptPolicy::default(), grading: GradingConfig::default(), matching: crate::matching::Config::default(), + input: crate::input::local::Config::default(), path: None, }); }; @@ -62,6 +64,7 @@ pub fn load(explicit: Option<&Path>, tests_dir: &Path) -> Result { attempt_policy: config.assignment.attempt_policy, grading: config.grading, matching: config.matching, + input: config.input, path: Some(path), }) } diff --git a/src/core/src/discovery.rs b/src/core/src/discovery.rs index 5280d2e..45b5d44 100644 --- a/src/core/src/discovery.rs +++ b/src/core/src/discovery.rs @@ -66,6 +66,14 @@ fn extract_archives( let extract_root = dir.join(EXTRACT_DIR); let mut extracted = Vec::new(); + if dir.is_file() { + let target = dir + .parent() + .unwrap_or(Path::new(".")) + .join(EXTRACT_DIR) + .join(dir.file_name().expect("a file has a name")); + return crate::archive::expand(dir, &target, &crate::archive::is_gradeable, diagnostics); + } let Ok(entries) = std::fs::read_dir(dir) else { return extracted; }; @@ -126,7 +134,7 @@ impl Default for LocalInputOptions<'_> { } } -/// Scan directories of student submissions and build the unified input. +/// Scan submission directories or explicitly listed files/archives into the unified input. /// /// 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 @@ -149,14 +157,17 @@ pub fn load_local_input( for path in paths { let path = path.as_ref(); - if !path.is_dir() { - return Err(DiscoveryError::NotADirectory(path.to_path_buf())); + if !path.is_dir() && !path.is_file() { + return Err(DiscoveryError::InvalidPath(path.to_path_buf())); } } let mut origins: BTreeMap = BTreeMap::new(); let mut extra_dirs: Vec = Vec::new(); for path in paths { + if path.as_ref().is_file() && crate::archive::format_of(path.as_ref()).is_none() { + continue; + } 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()) @@ -175,6 +186,7 @@ pub fn load_local_input( let mut all_paths: Vec = paths .iter() + .filter(|p| p.as_ref().is_dir()) .map(|p| p.as_ref().to_path_buf()) .chain(extra_dirs) .collect(); @@ -222,11 +234,15 @@ pub fn load_local_input( index += 1; } + let mut files: std::collections::BTreeSet = paths + .iter() + .filter(|p| p.as_ref().is_file()) + .map(|p| p.as_ref().to_path_buf()) + .collect(); for dir_path in &all_paths { let entries = std::fs::read_dir(dir_path) .map_err(|e| DiscoveryError::IoError(dir_path.clone(), e))?; - let mut files: Vec = Vec::new(); for entry in entries { // 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` @@ -235,7 +251,9 @@ pub fn load_local_input( // 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(meta) if meta.is_file() => { + files.insert(path); + } Ok(_) => {} Err(e) => diagnostics.push(InputDiagnostic::warning( DiagnosticKind::UnreadableDirEntry { @@ -252,91 +270,88 @@ pub fn load_local_input( )), } } - files.sort(); - - let is_extracted = dir_path.components().any(|c| c.as_os_str() == EXTRACT_DIR); - - for path in files { - let Some(filename) = path.file_name().and_then(|n| n.to_str()) else { - continue; - }; - if crate::archive::is_noise(filename) { - continue; - } - - let ext = path - .extension() - .and_then(|e| e.to_str()) - .unwrap_or("") - .to_lowercase(); - // A file out of an archive takes its key from the archive's name; the files - // within it are named by the student. - let default_key = if let Some(FileOrigin::Archive { archive, .. }) = origins.get(&path) - { - archive - .file_name() - .and_then(|n| n.to_str()) - .and_then(extract_sid) - } else { - extract_sid(filename) - }; - let owner_path = match origins.get(&path) { - Some(FileOrigin::Archive { archive, .. }) => archive, - _ => &path, - }; - let relative = paths - .iter() - .filter_map(|root| owner_path.strip_prefix(root.as_ref()).ok()) - .min_by_key(|p| p.components().count()) - .unwrap_or(owner_path); - let owner = matching - .owner(relative, owner_path, default_key) - .map_err(|e| DiscoveryError::Matching(e.to_string()))?; - if owner.state == crate::matching::State::Ambiguous { - diagnostics.push(InputDiagnostic::error(DiagnosticKind::AmbiguousOwner { - path: path.clone(), - decision: owner.clone(), - })); - } - let key = owner.selected.clone(); - // Even an empty/unreadable archive records receipt under the teacher's owner rule. - if !is_extracted && crate::archive::format_of(&path).is_some() { - match key { - Some(key) => { - seen_keys.insert(key); - } - None => unmatched.push(UnmatchedArtifact { - path: path.clone(), - reason: UnmatchedReason::NoStudentKey, - }), - } - continue; - } - - let language = detect_language(&ext); + } + for path in files { + let is_extracted = path.components().any(|c| c.as_os_str() == EXTRACT_DIR); + let Some(filename) = path.file_name().and_then(|n| n.to_str()) else { + continue; + }; + if crate::archive::is_noise(filename) { + continue; + } - match (key, language) { - (Some(key), Some(language)) => { - seen_keys.insert(key.clone()); - let origin = origins.get(&path).cloned().unwrap_or(FileOrigin::Direct); - let mut file = StudentFile::direct(path.clone(), language).with_origin(origin); - file.owner = Some(owner); - by_key.entry(key).or_default().push(file); - } - (Some(key), None) => { - // Owner known, type unusable: the student submitted, just not code. - diagnostics.push(crate::archive::archive_or_ignored(&key, &path)); + let ext = path + .extension() + .and_then(|e| e.to_str()) + .unwrap_or("") + .to_lowercase(); + // A file out of an archive takes its key from the archive's name; the files + // within it are named by the student. + let default_key = if let Some(FileOrigin::Archive { archive, .. }) = origins.get(&path) { + archive + .file_name() + .and_then(|n| n.to_str()) + .and_then(extract_sid) + } else { + extract_sid(filename) + }; + let owner_path = match origins.get(&path) { + Some(FileOrigin::Archive { archive, .. }) => archive, + _ => &path, + }; + let relative = paths + .iter() + .filter(|root| root.as_ref().is_dir()) + .filter_map(|root| owner_path.strip_prefix(root.as_ref()).ok()) + .min_by_key(|p| p.components().count()) + .unwrap_or(owner_path); + let owner = matching + .owner(relative, owner_path, default_key) + .map_err(|e| DiscoveryError::Matching(e.to_string()))?; + if owner.state == crate::matching::State::Ambiguous { + diagnostics.push(InputDiagnostic::error(DiagnosticKind::AmbiguousOwner { + path: path.clone(), + decision: owner.clone(), + })); + } + let key = owner.selected.clone(); + // Even an empty/unreadable archive records receipt under the teacher's owner rule. + if !is_extracted && crate::archive::format_of(&path).is_some() { + match key { + Some(key) => { seen_keys.insert(key); } - (None, language) => unmatched.push(UnmatchedArtifact { + None => unmatched.push(UnmatchedArtifact { path: path.clone(), - reason: if language.is_some() { - UnmatchedReason::NoStudentKey - } else { - UnmatchedReason::UnsupportedType - }, + reason: UnmatchedReason::NoStudentKey, }), } + continue; + } + + 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); + let mut file = StudentFile::direct(path.clone(), language).with_origin(origin); + file.owner = Some(owner); + by_key.entry(key).or_default().push(file); + } + (Some(key), None) => { + // Owner known, type unusable: the student submitted, just not code. + diagnostics.push(crate::archive::archive_or_ignored(&key, &path)); + seen_keys.insert(key); + } + (None, language) => unmatched.push(UnmatchedArtifact { + path: path.clone(), + reason: if language.is_some() { + UnmatchedReason::NoStudentKey + } else { + UnmatchedReason::UnsupportedType + }, + }), } } @@ -442,8 +457,8 @@ pub fn load_local_input( pub enum DiscoveryError { #[error("invalid matching rules: {0}")] Matching(String), - #[error("not a directory: {0}")] - NotADirectory(std::path::PathBuf), + #[error("not a submission file or directory: {0}")] + InvalidPath(std::path::PathBuf), #[error("IO error reading {0}: {1}")] IoError(std::path::PathBuf, std::io::Error), } @@ -1056,12 +1071,15 @@ mod tests { } #[test] - fn test_not_a_directory_is_a_hard_error() { + fn test_explicit_files_are_accepted_but_missing_paths_are_an_error() { let dir = tempfile::tempdir().unwrap(); let file = dir.path().join("a.py"); std::fs::write(&file, "pass").unwrap(); - let err = load_local_input(&[&file], LocalInputOptions::default()).unwrap_err(); - assert!(matches!(err, DiscoveryError::NotADirectory(_))); + let input = load_local_input(&[&file], LocalInputOptions::default()).unwrap(); + assert_eq!(input.student_count(), 1); + let missing = dir.path().join("missing.py"); + let err = load_local_input(&[&missing], LocalInputOptions::default()).unwrap_err(); + assert!(matches!(err, DiscoveryError::InvalidPath(_))); } } diff --git a/src/core/src/input/local.rs b/src/core/src/input/local.rs new file mode 100644 index 0000000..b8a677c --- /dev/null +++ b/src/core/src/input/local.rs @@ -0,0 +1,207 @@ +//! Resolve teacher-owned local inputs before discovering or executing student files. + +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result, bail}; +use serde::{Deserialize, Serialize}; + +use super::table; +use crate::matching::{self, OwnerOverride}; +use crate::models::{DiagnosticKind, InputDiagnostic, SourceLocation, normalize_key}; +use crate::roster::{Roster, RosterEntry}; + +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(default, deny_unknown_fields)] +pub struct Config { + pub submissions: Vec, + pub roster: Option, + pub students: Vec>, +} + +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(default, deny_unknown_fields)] +pub struct RosterConfig { + pub path: Option, + pub sheet: Option, + pub header_row: Option, + pub columns: Option, +} + +impl RosterConfig { + pub fn options(&self) -> table::Options { + table::Options { + sheet: self.sheet.clone(), + header_row: self.header_row, + columns: self.columns.clone(), + } + } +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Student { + pub student_id: String, + pub name: Option, + pub canvas_user_id: Option, + #[serde(default)] + pub files: Vec, +} + +pub struct Resolved { + pub paths: Vec, + pub roster: Option, + pub matching: matching::Config, +} + +impl Config { + pub fn is_empty(&self) -> bool { + self.submissions.is_empty() && self.roster.is_none() && self.students.is_empty() + } + + /// CLI paths use the working directory; configured paths use assignment.toml's directory. + pub fn resolve( + &self, + assignment: Option<&Path>, + submissions: &[PathBuf], + roster_override: Option<&Path>, + matching: &matching::Config, + ) -> Result { + let base = assignment.and_then(Path::parent).unwrap_or(Path::new(".")); + let resolve = |path: &Path| -> Result { + if path.as_os_str().is_empty() { + bail!("an input path cannot be empty"); + } + std::path::absolute(base.join(path)).context("cannot resolve local input path") + }; + if !self.students.is_empty() { + if !self.submissions.is_empty() + || !submissions.is_empty() + || self.roster.is_some() + || roster_override.is_some() + { + bail!( + "input.students is an explicit roster and file list; do not combine it with submission directories or a roster table" + ); + } + let config_path = assignment.context("input.students requires an assignment.toml")?; + let source = std::fs::read_to_string(config_path)?; + let mut entries = Vec::new(); + let mut diagnostics = Vec::new(); + let mut paths = Vec::new(); + let mut matching = matching.clone(); + for owner in &mut matching.owners { + owner.path = resolve(&owner.path)?; + } + let mut owners: BTreeMap = BTreeMap::new(); + for entry in &self.students { + let row = source + .get(..entry.span().start) + .context("invalid source location in input.students")? + .bytes() + .filter(|b| *b == b'\n') + .count() + 1; + let location = SourceLocation::row(config_path, row); + let student = entry.get_ref(); + let id = normalize_key(&student.student_id); + if let Err(error) = table::valid_student_id(&id) { + diagnostics.push( + InputDiagnostic::error(DiagnosticKind::UnusableRosterRow { + reason: error.to_string(), + }) + .at(location), + ); + continue; + } + let mut roster_entry = RosterEntry::new( + &id, + student + .name + .as_ref() + .map(|name| name.trim().to_owned()) + .filter(|name| !name.is_empty()), + ); + roster_entry.canvas_user_id = student.canvas_user_id; + roster_entry.location = Some(location.clone()); + entries.push(roster_entry); + if student.canvas_user_id == Some(0) { + diagnostics.push( + InputDiagnostic::error(DiagnosticKind::UnusableRosterRow { + reason: "canvas_user_id must be positive".into(), + }) + .at(location.clone()), + ); + } + for file in &student.files { + let result = (|| -> Result { + let path = resolve(file)?; + if !std::fs::metadata(&path) + .with_context(|| format!("cannot read {}", path.display()))? + .is_file() + { + bail!( + "expected a file or archive; list individual files in input.students.files" + ); + } + let canonical = path.canonicalize()?; + if let Some(other) = owners.get(&canonical) { + bail!("file is listed more than once (first assigned to '{other}')"); + } + owners.insert(canonical, id.clone()); + Ok(path) + })(); + match result { + Ok(path) => { + matching.owners.push(OwnerOverride { + path: path.clone(), + student: id.clone(), + }); + paths.push(path); + } + Err(error) => diagnostics.push( + InputDiagnostic::error(DiagnosticKind::InvalidSubmissionPath { + path: file.clone(), + student: id.clone(), + reason: format!("{error:#}"), + }) + .at(location.clone()), + ), + } + } + } + return Ok(Resolved { + paths, + roster: Some(Roster::with_diagnostics(entries, diagnostics)), + matching, + }); + } + let paths = if submissions.is_empty() { + self.submissions + .iter() + .map(|path| resolve(path)) + .collect::>>()? + } else { + submissions.to_vec() + }; + if paths.is_empty() { + bail!("provide submissions, input.submissions, or input.students in assignment.toml"); + } + let roster_config = self.roster.clone().unwrap_or_default(); + let roster_path = match roster_override { + Some(path) => Some(path.to_path_buf()), + None => roster_config.path.as_deref().map(resolve).transpose()?, + }; + if self.roster.is_some() && roster_path.is_none() { + bail!("input.roster needs a path, or --roster"); + } + let roster = roster_path + .as_deref() + .map(|path| table::load(path, &roster_config.options())) + .transpose()?; + Ok(Resolved { + paths, + roster, + matching: matching.clone(), + }) + } +} diff --git a/src/core/src/input/mod.rs b/src/core/src/input/mod.rs index ac95938..627175b 100644 --- a/src/core/src/input/mod.rs +++ b/src/core/src/input/mod.rs @@ -10,3 +10,5 @@ //! - Canvas payloads: [`canvas::normalize`] pub mod canvas; +pub mod local; +pub mod table; diff --git a/src/core/src/input/table.rs b/src/core/src/input/table.rs new file mode 100644 index 0000000..965b1ca --- /dev/null +++ b/src/core/src/input/table.rs @@ -0,0 +1,268 @@ +//! Roster tables share one typed reader and one identity validator. + +use std::path::Path; + +use anyhow::{Context, Result, bail}; +use calamine::{Data, Reader, Xlsx}; +use serde::{Deserialize, Serialize}; + +use crate::models::{DiagnosticKind, InputDiagnostic, SourceLocation, StudentKey, normalize_key}; +use crate::roster::{Roster, RosterEntry, RosterSource}; + +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] +#[serde(default, deny_unknown_fields)] +pub struct Options { + pub sheet: Option, + /// One-based physical row containing the column headings. + pub header_row: Option, + pub columns: Option, +} + +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Columns { + pub student_id: Column, + pub name: Option, + pub canvas_user_id: Option, +} + +/// A header name, or a one-based column number. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(untagged)] +pub enum Column { + Name(String), + Number(usize), +} + +impl Column { + fn index(&self, header: &[Data]) -> Result { + match self { + Self::Number(n) if *n > 0 && *n <= header.len() => Ok(n - 1), + Self::Number(n) => bail!("column {n} is outside the header; columns start at 1"), + Self::Name(name) => { + let matches: Vec = header + .iter() + .enumerate() + .filter_map(|(i, cell)| { + matches!(cell, Data::String(value) if value.trim() == name.trim()) + .then_some(i) + }) + .collect(); + match matches.as_slice() { + [i] if !name.trim().is_empty() => Ok(*i), + [] => bail!("missing column '{name}'"), + _ => bail!("column '{name}' is empty or appears more than once"), + } + } + } + } +} + +struct Table { + sheet: Option, + rows: Vec<(usize, Vec)>, +} + +fn read(path: &Path, options: &Options) -> Result { + match path + .extension() + .and_then(|s| s.to_str()) + .map(str::to_ascii_lowercase) + .as_deref() + { + Some("csv") => { + if options.sheet.is_some() { + bail!("CSV has no worksheets; remove input.roster.sheet"); + } + let mut reader = csv::ReaderBuilder::new() + .has_headers(false) + .flexible(true) + .from_path(path)?; + let mut rows = Vec::new(); + for record in reader.records() { + let record = record?; + let row = record.position().map_or(1, |p| p.line() as usize); + rows.push(( + row, + record.iter().map(|s| Data::String(s.to_owned())).collect(), + )); + } + Ok(Table { sheet: None, rows }) + } + Some("xlsx") => { + let mut book: Xlsx<_> = calamine::open_workbook(path)?; + let sheet = match &options.sheet { + Some(sheet) => sheet.clone(), + None => match book.sheet_names().as_slice() { + [sheet] => sheet.clone(), + [] => bail!("workbook has no worksheets"), + _ => bail!("workbook has multiple worksheets; set input.roster.sheet"), + }, + }; + let range = book.worksheet_range(&sheet)?; + let (row_offset, col_offset) = range.start().unwrap_or((0, 0)); + let rows = range + .rows() + .enumerate() + .map(|(i, row)| { + let mut cells = vec![Data::Empty; col_offset as usize]; + cells.extend_from_slice(row); + (row_offset as usize + i + 1, cells) + }) + .collect(); + Ok(Table { + sheet: Some(sheet), + rows, + }) + } + _ => bail!("roster must be a .csv or .xlsx file"), + } +} + +fn student_id(cell: Option<&Data>) -> Result { + match cell { + None | Some(Data::Empty) => Ok(String::new()), + Some(Data::String(value)) => Ok(normalize_key(value)), + _ => bail!( + "student_id must be a text cell; numeric cells cannot reliably preserve leading zeros" + ), + } +} + +fn canvas_id(cell: Option<&Data>) -> Result> { + let value = match cell { + None | Some(Data::Empty) => return Ok(None), + Some(Data::String(s)) if s.trim().is_empty() => return Ok(None), + Some(Data::String(s)) => normalize_key(s) + .parse::() + .context("canvas_user_id must be a positive integer")?, + Some(Data::Int(n)) if *n > 0 => u64::try_from(*n)?, + Some(Data::Float(n)) + if n.is_finite() && *n > 0.0 && n.fract() == 0.0 && *n <= 9_007_199_254_740_991.0 => + { + *n as u64 + } + _ => bail!("canvas_user_id must be an exactly representable positive integer"), + }; + if value == 0 { + bail!("canvas_user_id must be positive"); + } + Ok(Some(value)) +} + +pub(crate) fn valid_student_id(value: &str) -> Result<()> { + if value.is_empty() { + bail!("student_id is empty"); + } + if let Some(prefix) = crate::models::RESERVED_KEY_PREFIXES + .iter() + .find(|p| value.starts_with(**p)) + { + bail!("student id '{value}' starts with the reserved prefix '{prefix}'"); + } + Ok(()) +} + +/// Read either format. Without a mapping, use name/id or name/unused/id/Canvas-id. +pub fn load(path: &Path, options: &Options) -> Result { + let table = + read(path, options).with_context(|| format!("cannot read roster {}", path.display()))?; + let header_row = options.header_row.unwrap_or(1); + let header = table + .rows + .iter() + .find(|(row, _)| *row == header_row) + .map(|(_, cells)| cells) + .with_context(|| { + format!( + "{} {:?}: header row {header_row} is missing (rows start at 1)", + path.display(), + table.sheet + ) + })?; + let mapping = options + .columns + .as_ref() + .map(|columns| -> Result<(usize, Option, Option)> { + let id = columns.student_id.index(header)?; + let name = columns + .name + .as_ref() + .map(|column| column.index(header)) + .transpose()?; + let canvas = columns + .canvas_user_id + .as_ref() + .map(|column| column.index(header)) + .transpose()?; + let mut indexes = vec![id]; + indexes.extend(name); + indexes.extend(canvas); + indexes.sort_unstable(); + if indexes.windows(2).any(|pair| pair[0] == pair[1]) { + bail!("each roster field must use a different column"); + } + Ok((id, name, canvas)) + }) + .transpose() + .with_context(|| format!("{} {:?}:{header_row}", path.display(), table.sheet))?; + let mut entries = Vec::new(); + let mut diagnostics = Vec::new(); + for (row, cells) in &table.rows { + if *row <= header_row { + continue; + } + let location = SourceLocation { + file: Some(path.to_path_buf()), + sheet: table.sheet.clone(), + row: Some(*row), + }; + let parsed = (|| -> Result { + if cells.len() != header.len() { + bail!( + "row has {} columns; header has {}", + cells.len(), + header.len() + ); + } + if header.len() < 2 && mapping.is_none() { + bail!("need at least 2 columns, or an explicit column mapping"); + } + let (id, name, canvas) = mapping.unwrap_or(( + if header.len() >= 3 { 2 } else { 1 }, + Some(0), + (header.len() >= 4).then_some(3), + )); + let number = student_id(cells.get(id))?; + let uid = canvas_id(canvas.and_then(|i| cells.get(i)))?; + let key = if number.is_empty() { + StudentKey::CanvasUser(uid.context("row has no student id or Canvas user id")?) + } else { + valid_student_id(&number)?; + StudentKey::Number(number) + }; + let name = match name.and_then(|i| cells.get(i)) { + None | Some(Data::Empty) => None, + Some(Data::String(s)) => (!s.trim().is_empty()).then(|| s.trim().to_owned()), + _ => bail!("name must be a text cell"), + }; + Ok(RosterEntry { + key, + source: RosterSource::Supplied, + name, + canvas_user_id: uid, + location: Some(location.clone()), + }) + })(); + match parsed { + Ok(entry) => entries.push(entry), + Err(error) => diagnostics.push( + InputDiagnostic::error(DiagnosticKind::UnusableRosterRow { + reason: error.to_string(), + }) + .at(location), + ), + } + } + Ok(Roster::with_diagnostics(entries, diagnostics)) +} diff --git a/src/core/src/models/config.rs b/src/core/src/models/config.rs index 844059e..1a4fbca 100644 --- a/src/core/src/models/config.rs +++ b/src/core/src/models/config.rs @@ -136,6 +136,8 @@ pub enum AttemptPolicy { pub struct AssignmentConfig { pub assignment: AssignmentInfo, #[serde(default)] + pub input: crate::input::local::Config, + #[serde(default)] pub matching: crate::matching::Config, /// Expected student files. #[serde(default)] diff --git a/src/core/src/models/submission.rs b/src/core/src/models/submission.rs index ddc5edb..f998bfa 100644 --- a/src/core/src/models/submission.rs +++ b/src/core/src/models/submission.rs @@ -536,6 +536,12 @@ pub enum DiagnosticSeverity { )] #[serde(rename_all = "snake_case")] pub enum DiagnosticKind { + #[error("invalid submission path '{path}' for '{student}': {reason}")] + InvalidSubmissionPath { + path: PathBuf, + student: String, + reason: String, + }, #[error("ambiguous owner of '{path}': {decision:?}; add a matching.owners override")] AmbiguousOwner { path: PathBuf, @@ -684,6 +690,18 @@ impl InputDiagnostic { impl fmt::Display for InputDiagnostic { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if let Some(location) = &self.location { + if let Some(file) = &location.file { + write!(f, "{}", file.display())?; + } + if let Some(sheet) = &location.sheet { + write!(f, " [{sheet}]")?; + } + if let Some(row) = location.row { + write!(f, ":{row}")?; + } + write!(f, ": ")?; + } write!(f, "{}", self.kind) } } diff --git a/src/core/src/roster.rs b/src/core/src/roster.rs index b5e88c8..4f0f013 100644 --- a/src/core/src/roster.rs +++ b/src/core/src/roster.rs @@ -206,148 +206,20 @@ fn merge_by_key(entries: Vec) -> (Vec, Vec Result { - let content = - std::fs::read_to_string(path).map_err(|e| RosterError::IoError(path.to_path_buf(), e))?; - - // Strip UTF-8 BOM if present - let content = content.strip_prefix('\u{feff}').unwrap_or(&content); - - let mut reader = csv::ReaderBuilder::new() - .has_headers(true) - .flexible(true) - .from_reader(content.as_bytes()); - - // The width the file declares. A row that does not match it has shifted — usually an - // unescaped separator in a name — and its columns no longer mean what their position - // says, so it must not be read positionally. - let header_len = match reader.headers() { - Ok(header) => header.len(), - Err(e) => return Err(RosterError::CsvError(path.to_path_buf(), e)), - }; - - let mut entries = Vec::new(); - let mut diagnostics = Vec::new(); - - 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_number = if record.len() >= 3 { - record.get(2).unwrap_or("") - } else if record.len() >= 2 { - 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; - }; - - 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; - } - - // Column 3 is the Canvas user id, which `roster-pull` now writes so that grade push - // never has to guess it. Read for every row, independently of the key decision - // below: a student with both a 学号 and a Canvas id needs both kept. - let canvas_user_id = record - .get(3) - .map(normalize_key) - .filter(|value| !value.is_empty()) - .and_then(|value| value.parse::().ok()); - - if student_number.is_empty() { - // A Canvas enrollee with no SIS id is still a member. They are keyed by their - // Canvas id — but only from a row whose width matches the header, because a - // shifted row puts somebody's 学号 in this column, and keying on it would - // invent a Canvas user that does not exist. - if let Some(id) = canvas_user_id.filter(|_| record.len() == header_len) { - entries.push(RosterEntry { - key: StudentKey::CanvasUser(id), - source: RosterSource::Supplied, - name: (!name.is_empty()).then_some(name), - canvas_user_id: Some(id), - location: Some(location), - }); - continue; - } - - diagnostics.push( - InputDiagnostic::warning(DiagnosticKind::UnusableRosterRow { - reason: if name.is_empty() { - "blank row".to_string() - } else if record.len() != header_len { - format!( - "'{name}' has no student id, and the row has {} column(s) where the \ - header declares {header_len} — it has probably shifted", - record.len() - ) - } 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, - location: Some(location), + let mut diagnostic = InputDiagnostic::warning(DiagnosticKind::DuplicateRosterEntry { + key: key.to_string(), + count, }); + diagnostic.location = merged[first_seen[&key]].location.clone(); + diagnostics.push(diagnostic); } - Ok(Roster::with_diagnostics(entries, diagnostics)) + (merged, diagnostics) } -#[derive(Debug, thiserror::Error)] -pub enum RosterError { - #[error("IO error reading {0}: {1}")] - IoError(std::path::PathBuf, std::io::Error), - #[error("CSV parse error in {0}: {1}")] - CsvError(std::path::PathBuf, csv::Error), +/// Load a roster using the default table layout. Assignment inputs can configure columns. +pub fn load_roster(path: &Path) -> anyhow::Result { + crate::input::table::load(path, &crate::input::table::Options::default()) } #[cfg(test)] From 4fff8220782003d95084a1fe42d2e183f89c54d4 Mon Sep 17 00:00:00 2001 From: Acture Date: Wed, 7 Oct 2026 22:01:44 +0800 Subject: [PATCH 2/5] docs: reference local-import notes (OSS-155) --- notes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes b/notes index 08d9bbf..62a0f18 160000 --- a/notes +++ b/notes @@ -1 +1 @@ -Subproject commit 08d9bbf6db8bda52b8560541b810085c2aeb5ec8 +Subproject commit 62a0f187ca9de77ad09b1e156fb10844b2e6ea24 From 92e39ce3d4e6ae2b89d0e74d6228dd65284d64c9 Mon Sep 17 00:00:00 2001 From: Acture Date: Wed, 7 Oct 2026 22:47:58 +0800 Subject: [PATCH 3/5] fix: close review gaps in local roster and manifest import (OSS-155) Review of the local-import branch found inputs that were refused, misread or dropped without a trace: - Blank roster rows (Excel's `,,` export, empty separator rows) are skipped instead of refusing the whole run; a table with no usable student row is an error located to its file and sheet. - `summarize --roster` and `db import-roster` read tables through the same `[input.roster]` layout via `--assignment`, print diagnostics with their severity and refuse a roster with errors instead of naming nobody. - Explicitly listed files are never skipped as noise, and matching rules see only their file name, not the directories above them (a level-0 directory rule keyed explicit files as `local:/`). - DB saves import the identities frozen in the record for grade, rescore and db save alike, and a missing name no longer erases a stored one. - Header/mapping errors use the `file [sheet]:row` location; each repeated roster row is located; an unreadable `input.submissions` entry names assignment.toml; `.xlsm` opens as a workbook, `.xls`/`.ods` get a clear message and other names are read as CSV as before. --- CLAUDE.md | 2 +- README.md | 27 ++- examples/bundles/local_import/README.md | 14 +- src/cli/src/db/roster.rs | 7 +- src/cli/src/main.rs | 145 +++++++++------- src/cli/tests/local_import.rs | 222 +++++++++++++++++++++++- src/core/src/discovery.rs | 10 +- src/core/src/input/local.rs | 69 +++++--- src/core/src/input/table.rs | 81 +++++---- src/core/src/models/submission.rs | 29 ++-- src/core/src/record.rs | 24 +++ src/core/src/roster.rs | 100 ++++++++++- 12 files changed, 576 insertions(+), 154 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 26fcace..98d7f3f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -127,7 +127,7 @@ Zero and withheld grades stay distinct. See the notes' teacher contract for poli - Integration tests spawn real Python processes — need `python3` available - `scriptmark-core` exposes grading models and operations; CLI adapters import it directly, without compatibility re-exports. - Core's `test-support` feature exposes shared graded/withheld report fixtures for adapter tests; it is enabled only by the CLI's dev-dependency. -- `[input]` in `assignment.toml` configures either submission paths plus a mapped CSV/XLSX roster, or `[[input.students]]` with exact files/archives. `grade`, `run` and `match` resolve the same input; rescore reconstructs it from saved CLI overrides and the recorded assignment file. File and function matching remains owned by `matching`. +- `[input]` in `assignment.toml` configures either submission paths plus a mapped CSV/XLSX roster, or `[[input.students]]` with exact files/archives. `grade`, `run` and `match` resolve the same input; rescore reconstructs it from saved CLI overrides and the recorded assignment file. `summarize --roster` and `db import-roster` read tables through `Config::roster_table` with `--assignment`'s layout. DB saves import the identities frozen in the record (`Record::roster`). File and function matching remains owned by `matching`. - Publish both Rust packages with `cargo publish --workspace`, which orders workspace dependencies before their consumers. - Platform-specific code uses `#[cfg(target_os = "macos")]` / `#[cfg(target_os = "linux")]` for rlimit types diff --git a/README.md b/README.md index c85c253..d619768 100644 --- a/README.md +++ b/README.md @@ -222,11 +222,14 @@ name = "姓名" # canvas_user_id = "Canvas ID" # optional; distinct from the student number ``` -Use `.csv` and omit `sheet` for CSV. Header rows and numeric column selectors start -at 1; a column selector can also be an exact heading. Without a mapping, the default -layout is `name,student_id` or `name,unused,student_id[,canvas_user_id]`. XLSX student -IDs must be text cells; numeric cells are rejected because their original leading -zeros cannot be established. Workbook files with several sheets need `sheet`. +`.xlsx` and `.xlsm` are read as workbooks; `.xls`/`.ods` must be saved as one of +those first, and any other file is read as CSV (omit `sheet`). Header rows and numeric +column selectors start at 1; a column selector can also be an exact heading. Without a +mapping, the default layout is `name,student_id` or +`name,unused,student_id[,canvas_user_id]`. XLSX student IDs must be text cells; numeric +cells are rejected because their original leading zeros cannot be established. Workbook +files with several sheets need `sheet`. Blank rows are skipped; a table with no usable +student row below the header is refused rather than graded as an empty class. For an explicit class and file list, replace `[input]`/`[input.roster]` with: @@ -240,8 +243,9 @@ files = ["handins/answer.py"] # individual files or archives; [] means no submi An explicit list cannot be combined with a roster table or submission directory. It uses the same ownership and item matching component. Invalid identities/paths stop grading and remain visible in `match` diagnostics. An identical repeated roster -row is reported and merged; conflicting rows are refused. CLI submission paths and -`--roster` override configured paths in table mode. To use only configured sources: +row is reported at each repeat and merged; conflicting rows are refused. CLI submission +paths and `--roster` override configured paths in table mode. To use only configured +sources: ```fish scriptmark match -t tests/ --assignment assignment.toml @@ -249,6 +253,15 @@ scriptmark grade -t tests/ --assignment assignment.toml scriptmark export output/results.json -o grades.xlsx ``` +`summarize --roster` and `db import-roster` read a table with the default layout, or +with the `[input.roster]` layout of `--assignment`, which also supplies the path when +no roster file is given. A roster with errors is refused there as in grading: + +```fish +scriptmark summarize output/results.json --assignment assignment.toml +scriptmark db import-roster --assignment assignment.toml --db grades.db +``` + ### Test definitions Teachers can configure student ownership, item file patterns, function aliases and diff --git a/examples/bundles/local_import/README.md b/examples/bundles/local_import/README.md index 6690531..07c1ae8 100644 --- a/examples/bundles/local_import/README.md +++ b/examples/bundles/local_import/README.md @@ -28,6 +28,14 @@ It accepts individual files and archives; `files = []` records a non-submitter. File-to-item and function matching still use the normal `[matching]` rules. Run `match` to see import diagnostics without executing student code. Invalid rows, -numeric Excel student IDs and invalid/duplicate submission paths stop grading, with -the source file, worksheet and row where available. Repeated identical roster IDs -are reported and merged; conflicting identities stop grading. +numeric Excel student IDs, a roster with no usable student row and invalid/duplicate +submission paths stop grading, with the source file, worksheet and row where available. +Blank rows are skipped. Each repeat of an identical roster ID is reported and merged; +conflicting identities stop grading. + +The same mapping names students outside grading: + +```fish +cargo run -p scriptmark -- summarize output/local.json --assignment examples/bundles/local_import/assignment.toml +cargo run -p scriptmark -- db import-roster --assignment examples/bundles/local_import/assignment.toml --db output/grades.db +``` diff --git a/src/cli/src/db/roster.rs b/src/cli/src/db/roster.rs index 41f59b3..36f781b 100644 --- a/src/cli/src/db/roster.rs +++ b/src/cli/src/db/roster.rs @@ -19,9 +19,10 @@ impl Database { let mut stmt = self.conn.prepare( "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. + -- A roster that does not know a value must not erase one stored earlier: + -- a CSV roster carries no Canvas id, which grade push needs, and a record + -- graded without names would blank those an import had stored. + name = COALESCE(excluded.name, students.name), canvas_id = COALESCE(excluded.canvas_id, students.canvas_id)", )?; let mut stored = std::collections::BTreeSet::new(); diff --git a/src/cli/src/main.rs b/src/cli/src/main.rs index 2869f92..fac41d5 100644 --- a/src/cli/src/main.rs +++ b/src/cli/src/main.rs @@ -10,11 +10,11 @@ use scriptmark_core::assignment::{self, Declared}; use scriptmark_core::discovery::{LocalInputOptions, load_local_input}; use scriptmark_core::grading::{self, Policy}; use scriptmark_core::models::{ - AssignmentInput, DiagnosticSeverity, StudentKey, StudentReport, StudentSubmission, - SubmissionOutcome, TestSpec, + AssignmentInput, DiagnosticSeverity, InputDiagnostic, StudentKey, StudentReport, + StudentSubmission, SubmissionOutcome, TestSpec, }; use scriptmark_core::record::{self, Evidence, Record, View}; -use scriptmark_core::roster::load_roster; +use scriptmark_core::roster::Roster; use scriptmark_core::runner::frozen::{self, Frozen, Generation}; use scriptmark_core::runner::generation::SeedSource; use scriptmark_core::runner::orchestrator::{self, RunOptions}; @@ -240,10 +240,15 @@ struct SummarizeArgs { /// Path to the grading record, as `grade`, `run` or `rescore` wrote it results: PathBuf, - /// Path to roster CSV + /// Roster CSV or XLSX whose names label the students #[arg(short, long)] roster: Option, + /// Read the roster with this assignment's [input.roster] layout, and from its path when + /// --roster is absent + #[arg(long)] + assignment: Option, + #[command(flatten)] revision: RevisionArg, } @@ -426,10 +431,15 @@ enum DbAction { #[arg(long, default_value = "scriptmark.db")] db: PathBuf, }, - /// Import a roster CSV into the database + /// Import a roster CSV or XLSX into the database ImportRoster { - /// Roster CSV file - roster: PathBuf, + /// Roster CSV or XLSX file + #[arg(required_unless_present = "assignment")] + roster: Option, + /// Read the roster with this assignment's [input.roster] layout, and from its path + /// when no roster file is given + #[arg(long)] + assignment: Option, /// Database file path #[arg(long, default_value = "scriptmark.db")] db: PathBuf, @@ -487,11 +497,8 @@ fn build_local_input( 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. +/// Print the import summary and every anomaly the adapters recorded. fn report_input(input: &AssignmentInput) { - use owo_colors::OwoColorize; - let counts = [ (SubmissionOutcome::Executable, "executable"), (SubmissionOutcome::SubmittedEmpty, "submitted but empty"), @@ -549,9 +556,25 @@ fn report_input(input: &AssignmentInput) { } } - let errors = input.diagnostics_of(DiagnosticSeverity::Error).count(); - let warnings = input.diagnostics_of(DiagnosticSeverity::Warning).count(); - for diagnostic in &input.diagnostics { + print_diagnostics(&input.diagnostics); +} + +/// Print every diagnostic under its severity. Adapters never print themselves; diagnostics +/// reach a terminal only through here. +fn print_diagnostics(diagnostics: &[InputDiagnostic]) { + use owo_colors::OwoColorize; + + let count = |severity| { + diagnostics + .iter() + .filter(|d| d.severity == severity) + .count() + }; + let (errors, warnings) = ( + count(DiagnosticSeverity::Error), + count(DiagnosticSeverity::Warning), + ); + for diagnostic in diagnostics { match diagnostic.severity { DiagnosticSeverity::Error => println!(" {} {diagnostic}", "error:".red()), DiagnosticSeverity::Warning => println!(" {} {diagnostic}", "warning:".yellow()), @@ -563,6 +586,28 @@ fn report_input(input: &AssignmentInput) { } } +/// The roster a command names outside grading: `roster`, or `[input.roster].path` of +/// `assignment`, read with that assignment's layout. Its errors refuse it, as in grading. +fn roster_table(roster: Option<&Path>, assignment: Option<&Path>) -> Result { + let input = match assignment { + Some(path) => { + scriptmark_core::spec_loader::load_assignment_config(path) + .with_context(|| format!("Failed to load {}", path.display()))? + .input + } + None => Default::default(), + }; + let roster = input + .roster_table(assignment, roster)? + .context("give a roster file, or --assignment with an [input.roster] path")?; + print_diagnostics(&roster.diagnostics); + let errors = roster.errors().count(); + if errors > 0 { + anyhow::bail!("refusing the roster: {errors} problem(s)"); + } + Ok(roster) +} + #[tokio::main] async fn main() -> Result<()> { let cli = Cli::parse(); @@ -754,7 +799,12 @@ fn prepare_batch( // no separate roster merge afterwards. if canvas.is_some() && !declared.input.is_empty() { anyhow::bail!( - "--canvas cannot be combined with assignment.toml [input]; use a separate local assignment configuration" + "a Canvas bundle is the whole input, but {} also configures [input]; grade the bundle with an assignment file that has no [input]", + declared + .path + .as_deref() + .unwrap_or(Path::new("assignment.toml")) + .display() ); } let (input, source) = match &canvas { @@ -940,26 +990,20 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { } if let Some(db_path) = &args.db { - save_to_db(db_path, &record, revision, input.roster.as_ref())?; + save_to_db(db_path, &record, revision)?; } Ok(()) } -/// Save a revision as a database session, importing the roster it was graded with first. -/// Saving one revision twice finds the session it already has. -fn save_to_db( - db_path: &Path, - record: &Record, - revision: u32, - roster: Option<&scriptmark_core::roster::Roster>, -) -> Result<()> { +/// Save a revision as a database session, importing the roster it was graded with first — +/// as the record froze it, not a table that may have changed since. Saving one revision +/// twice finds the session it already has. +fn save_to_db(db_path: &Path, record: &Record, revision: u32) -> Result<()> { let database = scriptmark::db::Database::open(db_path).context("Failed to open database")?; - if let Some(roster) = roster { - database - .import_roster(roster) - .context("Failed to import roster")?; - } + database + .import_roster(&record.roster()) + .context("Failed to import roster")?; let saved = database .save_revision(record, revision) .context("Failed to save session to database")?; @@ -1113,7 +1157,7 @@ fn cmd_rescore(args: RescoreArgs) -> Result<()> { println!("\nRevision {revision} added to {}", args.record.display()); if let Some(db_path) = &args.db { - save_to_db(db_path, &record, revision, input.roster.as_ref())?; + save_to_db(db_path, &record, revision)?; } Ok(()) } @@ -1121,8 +1165,8 @@ fn cmd_rescore(args: RescoreArgs) -> Result<()> { fn cmd_summarize(args: SummarizeArgs) -> Result<()> { let (_, mut view) = load_view(&args.results, args.revision.revision)?; - if let Some(roster_path) = &args.roster { - let roster = load_roster(roster_path).context("Failed to load roster")?; + if args.roster.is_some() || args.assignment.is_some() { + let roster = roster_table(args.roster.as_deref(), args.assignment.as_deref())?; for report in view.reports.iter_mut() { // `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:` @@ -1530,39 +1574,18 @@ fn cmd_db(cmd: DbCommand) -> Result<()> { } => { let (saved, view) = load_view(&record, revision.revision)?; let revision = scored(&view, &record)?; - // Use the identities frozen in the record, not a table that may have changed. - let roster = scriptmark_core::roster::Roster::from_entries( - saved - .evidence - .students - .iter() - .filter_map(|student| { - let key = StudentKey::parse(&student.student_id); - if matches!(key, StudentKey::Extracted(_)) { - return None; - } - Some(scriptmark_core::roster::RosterEntry { - key, - name: student.student_name.clone(), - canvas_user_id: student.canvas_user_id, - source: scriptmark_core::roster::RosterSource::Supplied, - location: None, - }) - }) - .collect(), - ); - save_to_db(&db, &saved, revision, Some(&roster)) + save_to_db(&db, &saved, revision) } - DbAction::ImportRoster { roster, db } => { + DbAction::ImportRoster { + roster, + assignment, + db, + } => { + let roster = roster_table(roster.as_deref(), assignment.as_deref())?; let database = scriptmark::db::Database::open(&db).context("Failed to open database")?; - let roster_csv = scriptmark_core::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_csv) + .import_roster(&roster) .context("Failed to import roster")?; println!("Imported {} students into {}", count, db.display()); Ok(()) diff --git a/src/cli/tests/local_import.rs b/src/cli/tests/local_import.rs index b071cc8..31fb32e 100644 --- a/src/cli/tests/local_import.rs +++ b/src/cli/tests/local_import.rs @@ -84,10 +84,19 @@ fn setup() -> tempfile::TempDir { } fn input(dir: &Path, config: &str) -> AssignmentInput { + input_with(dir, config, &[]) +} + +fn input_with(dir: &Path, config: &str, submissions: &[PathBuf]) -> AssignmentInput { let declared = assignment::load(Some(&dir.join(config)), &dir.join("tests")).unwrap(); let resolved = declared .input - .resolve(declared.path.as_deref(), &[], None, &declared.matching) + .resolve( + declared.path.as_deref(), + submissions, + None, + &declared.matching, + ) .unwrap(); load_local_input( &resolved.paths, @@ -243,8 +252,14 @@ fn table_mapping_errors_and_numeric_ids_name_the_source() { let dir = setup(); let mut configured = options(); configured.columns.as_mut().unwrap().student_id = Column::Name("wrong heading".into()); - let error = table::load(&dir.path().join("roster.csv"), &configured).unwrap_err(); - assert!(format!("{error:#}").contains("missing column 'wrong heading'")); + let error = format!( + "{:#}", + table::load(&dir.path().join("roster.csv"), &configured).unwrap_err() + ); + assert!( + error.contains("roster.csv:2: missing column 'wrong heading'"), + "{error}" + ); let error = table::load(&dir.path().join("roster.xlsx"), &options()).unwrap_err(); assert!(format!("{error:#}").contains("multiple worksheets")); let mut book = Workbook::new(); @@ -344,7 +359,12 @@ fn config_paths_survive_another_working_directory_and_changes_refuse_rescore() { output.to_str().unwrap(), ], ); - let original = std::fs::read_to_string(&config).unwrap(); + // With only the policy changed, the record rescores from anywhere: it names its + // assignment absolutely. + let original = std::fs::read_to_string(&config).unwrap() + "\n[grading]\nscale = 20\n"; + std::fs::write(&config, &original).unwrap(); + success(elsewhere.path(), &["rescore", output.to_str().unwrap()]); + assert_eq!(Record::load(&output).unwrap().revisions.len(), 2); std::fs::write( &config, original @@ -359,7 +379,12 @@ fn config_paths_survive_another_working_directory_and_changes_refuse_rescore() { ); let result = run(elsewhere.path(), &["rescore", output.to_str().unwrap()]); assert!(!result.status.success()); - assert_eq!(Record::load(&output).unwrap().revisions.len(), 1); + let stderr = String::from_utf8_lossy(&result.stderr); + assert!( + stderr.contains("student 001 is no longer in the input"), + "{stderr}" + ); + assert_eq!(Record::load(&output).unwrap().revisions.len(), 2); } #[test] @@ -499,3 +524,190 @@ fn the_shipped_local_import_example_runs_as_documented() { ); } } + +#[test] +fn rosters_read_outside_grading_use_the_same_mapping_and_refuse_errors() { + let dir = setup(); + write( + dir.path(), + "plain.toml", + "[assignment]\nname = 'local example'\n", + ); + success( + dir.path(), + &[ + "grade", + "submissions", + "-t", + "tests", + "--assignment", + "plain.toml", + "-o", + "out/plain.json", + ], + ); + // The mapped table names the students, by --roster or by the assignment's own path. + for args in [ + &["--roster", "roster.xlsx", "--assignment", "xlsx.toml"][..], + &["--assignment", "csv.toml"][..], + ] { + let output = run( + dir.path(), + &[&["summarize", "out/plain.json"][..], args].concat(), + ); + assert!(output.status.success(), "{args:?}"); + assert!(String::from_utf8_lossy(&output.stdout).contains("张三")); + } + // Read with the default layout, the title row is taken for the header: refused, not a + // summary that silently names nobody. + let output = run( + dir.path(), + &["summarize", "out/plain.json", "--roster", "roster.csv"], + ); + assert!(!output.status.success()); + assert!(String::from_utf8_lossy(&output.stdout).contains("roster.csv:3: roster row unusable")); + + let output = run( + dir.path(), + &["db", "import-roster", "roster.csv", "--db", "out/roster.db"], + ); + assert!(!output.status.success()); + success( + dir.path(), + &[ + "db", + "import-roster", + "--assignment", + "xlsx.toml", + "--db", + "out/roster.db", + ], + ); + let name = |id: &str| { + scriptmark::db::Database::open(&dir.path().join("out/roster.db")) + .unwrap() + .get_student(id) + .unwrap() + .unwrap() + .name + }; + assert_eq!(name("001").as_deref(), Some("张三")); + + // A record that knows the students but not their names keeps the imported names. + let nameless = std::fs::read_to_string(dir.path().join("manifest.toml")) + .unwrap() + .replace("name = '张三'\n", "") + .replace("name = '李四'\n", "") + .replace("name = '王五'\n", ""); + write(dir.path(), "nameless.toml", &nameless); + success( + dir.path(), + &[ + "grade", + "-t", + "tests", + "--assignment", + "nameless.toml", + "-o", + "out/nameless.json", + "--db", + "out/roster.db", + ], + ); + assert_eq!(name("001").as_deref(), Some("张三")); +} + +#[test] +fn explicit_files_are_never_noise_and_rules_see_only_their_names() { + let dir = setup(); + write( + dir.path(), + "submissions/__main__.py", + "def double(x):\n return x * 2\n", + ); + let manifest = std::fs::read_to_string(dir.path().join("manifest.toml")) + .unwrap() + .replace("submissions/001_work.py", "submissions/__main__.py"); + write(dir.path(), "manifest.toml", &manifest); + let imported = input(dir.path(), "manifest.toml"); + assert!(imported.errors().next().is_none()); + assert_eq!( + imported.students[0].selected_attempt().unwrap().files.len(), + 1 + ); + + // A directory rule reads the directories below a scanned root. An explicit file has + // none, so the filename default decides — never a directory above the file. + write( + dir.path(), + "directory.toml", + "[assignment]\nname = 'local example'\n[[matching.students]]\nkind = 'directory'\nlevel = 0\n", + ); + let file = dir.path().join("submissions/002_work.py"); + let imported = input_with(dir.path(), "directory.toml", &[file]); + assert!(imported.errors().next().is_none()); + assert_eq!(imported.students[0].identity.key.to_string(), "local:002"); +} + +#[test] +fn blank_rows_are_skipped_and_an_empty_table_is_refused() { + let dir = setup(); + write( + dir.path(), + "roster.csv", + "title\n姓名,学号\n张三,001\n,\n李四,002\n王五,003\n,\n,\n", + ); + success( + dir.path(), + &["grade", "-t", "tests", "--assignment", "csv.toml"], + ); + + // A header row set below the data leaves nobody on the roster: refused, not a run in + // which every student is merely "not on the roster". + let roster = table::load( + &dir.path().join("roster.csv"), + &Options { + header_row: Some(6), + columns: None, + ..options() + }, + ) + .unwrap(); + assert!( + roster + .errors() + .any(|d| matches!(d.kind, DiagnosticKind::EmptyRoster { header_row: 6 })) + ); + write(dir.path(), "roster.csv", "title\n姓名,学号\n,\n"); + let output = run( + dir.path(), + &["grade", "-t", "tests", "--assignment", "csv.toml"], + ); + assert!(!output.status.success()); + assert!( + String::from_utf8_lossy(&output.stderr) + .contains("roster has no usable student rows below header row 2") + ); +} + +#[test] +fn a_configured_submission_path_that_does_not_exist_names_the_assignment() { + let dir = setup(); + let config = std::fs::read_to_string(dir.path().join("csv.toml")) + .unwrap() + .replace( + "submissions = ['submissions']", + "submissions = ['submisions']", + ); + write(dir.path(), "csv.toml", &config); + let output = run( + dir.path(), + &["grade", "-t", "tests", "--assignment", "csv.toml"], + ); + assert!(!output.status.success()); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("csv.toml: cannot read input.submissions entry 'submisions'"), + "{stderr}" + ); +} diff --git a/src/core/src/discovery.rs b/src/core/src/discovery.rs index 45b5d44..3fbb2bf 100644 --- a/src/core/src/discovery.rs +++ b/src/core/src/discovery.rs @@ -234,11 +234,13 @@ pub fn load_local_input( index += 1; } - let mut files: std::collections::BTreeSet = paths + // Files named explicitly were chosen by the teacher, so they are never noise. + let explicit: std::collections::BTreeSet = paths .iter() .filter(|p| p.as_ref().is_file()) .map(|p| p.as_ref().to_path_buf()) .collect(); + let mut files = explicit.clone(); for dir_path in &all_paths { let entries = std::fs::read_dir(dir_path) .map_err(|e| DiscoveryError::IoError(dir_path.clone(), e))?; @@ -276,7 +278,7 @@ pub fn load_local_input( let Some(filename) = path.file_name().and_then(|n| n.to_str()) else { continue; }; - if crate::archive::is_noise(filename) { + if crate::archive::is_noise(filename) && !explicit.contains(&path) { continue; } @@ -299,12 +301,14 @@ pub fn load_local_input( Some(FileOrigin::Archive { archive, .. }) => archive, _ => &path, }; + // Rules see a path below a scanned directory. An explicit file or archive has no such + // root, so they see its name alone, never the directories above it. let relative = paths .iter() .filter(|root| root.as_ref().is_dir()) .filter_map(|root| owner_path.strip_prefix(root.as_ref()).ok()) .min_by_key(|p| p.components().count()) - .unwrap_or(owner_path); + .unwrap_or_else(|| Path::new(owner_path.file_name().unwrap_or_default())); let owner = matching .owner(relative, owner_path, default_key) .map_err(|e| DiscoveryError::Matching(e.to_string()))?; diff --git a/src/core/src/input/local.rs b/src/core/src/input/local.rs index b8a677c..78ad1bd 100644 --- a/src/core/src/input/local.rs +++ b/src/core/src/input/local.rs @@ -67,13 +67,7 @@ impl Config { roster_override: Option<&Path>, matching: &matching::Config, ) -> Result { - let base = assignment.and_then(Path::parent).unwrap_or(Path::new(".")); - let resolve = |path: &Path| -> Result { - if path.as_os_str().is_empty() { - bail!("an input path cannot be empty"); - } - std::path::absolute(base.join(path)).context("cannot resolve local input path") - }; + let resolve = |path: &Path| relative_to(assignment, path); if !self.students.is_empty() { if !self.submissions.is_empty() || !submissions.is_empty() @@ -178,7 +172,17 @@ impl Config { let paths = if submissions.is_empty() { self.submissions .iter() - .map(|path| resolve(path)) + .map(|path| { + let resolved = resolve(path)?; + std::fs::metadata(&resolved).with_context(|| { + format!( + "{}: cannot read input.submissions entry '{}'", + assignment.unwrap_or(Path::new("assignment.toml")).display(), + path.display() + ) + })?; + Ok(resolved) + }) .collect::>>()? } else { submissions.to_vec() @@ -186,22 +190,45 @@ impl Config { if paths.is_empty() { bail!("provide submissions, input.submissions, or input.students in assignment.toml"); } - let roster_config = self.roster.clone().unwrap_or_default(); - let roster_path = match roster_override { - Some(path) => Some(path.to_path_buf()), - None => roster_config.path.as_deref().map(resolve).transpose()?, - }; - if self.roster.is_some() && roster_path.is_none() { - bail!("input.roster needs a path, or --roster"); - } - let roster = roster_path - .as_deref() - .map(|path| table::load(path, &roster_config.options())) - .transpose()?; Ok(Resolved { paths, - roster, + roster: self.roster_table(assignment, roster_override)?, matching: matching.clone(), }) } + + /// The table roster: `path` from the command line when given, otherwise + /// `input.roster.path`, read with the `[input.roster]` layout either way. + pub fn roster_table( + &self, + assignment: Option<&Path>, + path: Option<&Path>, + ) -> Result> { + if !self.students.is_empty() { + bail!("input.students lists the class itself; it has no roster table"); + } + let layout = self.roster.clone().unwrap_or_default(); + let path = match path { + Some(path) => Some(path.to_path_buf()), + None => layout + .path + .as_deref() + .map(|path| relative_to(assignment, path)) + .transpose()?, + }; + if self.roster.is_some() && path.is_none() { + bail!("input.roster needs a path, or --roster"); + } + path.map(|path| table::load(&path, &layout.options())) + .transpose() + } +} + +/// A configured path, relative to the directory of the assignment file that names it. +fn relative_to(assignment: Option<&Path>, path: &Path) -> Result { + if path.as_os_str().is_empty() { + bail!("an input path cannot be empty"); + } + let base = assignment.and_then(Path::parent).unwrap_or(Path::new(".")); + std::path::absolute(base.join(path)).context("cannot resolve local input path") } diff --git a/src/core/src/input/table.rs b/src/core/src/input/table.rs index 965b1ca..c6401a2 100644 --- a/src/core/src/input/table.rs +++ b/src/core/src/input/table.rs @@ -70,26 +70,7 @@ fn read(path: &Path, options: &Options) -> Result
{ .map(str::to_ascii_lowercase) .as_deref() { - Some("csv") => { - if options.sheet.is_some() { - bail!("CSV has no worksheets; remove input.roster.sheet"); - } - let mut reader = csv::ReaderBuilder::new() - .has_headers(false) - .flexible(true) - .from_path(path)?; - let mut rows = Vec::new(); - for record in reader.records() { - let record = record?; - let row = record.position().map_or(1, |p| p.line() as usize); - rows.push(( - row, - record.iter().map(|s| Data::String(s.to_owned())).collect(), - )); - } - Ok(Table { sheet: None, rows }) - } - Some("xlsx") => { + Some("xlsx" | "xlsm") => { let mut book: Xlsx<_> = calamine::open_workbook(path)?; let sheet = match &options.sheet { Some(sheet) => sheet.clone(), @@ -115,7 +96,37 @@ fn read(path: &Path, options: &Options) -> Result
{ rows, }) } - _ => bail!("roster must be a .csv or .xlsx file"), + Some("xls" | "xlsb" | "ods" | "numbers") => { + bail!("save the roster as .xlsx or .csv; this spreadsheet format is not read") + } + // Anything else is read as CSV, as rosters always were: `roster-pull -o` takes any name. + _ => { + if options.sheet.is_some() { + bail!("CSV has no worksheets; remove input.roster.sheet"); + } + let mut reader = csv::ReaderBuilder::new() + .has_headers(false) + .flexible(true) + .from_path(path)?; + let mut rows = Vec::new(); + for record in reader.records() { + let record = record?; + let row = record.position().map_or(1, |p| p.line() as usize); + rows.push(( + row, + record.iter().map(|s| Data::String(s.to_owned())).collect(), + )); + } + Ok(Table { sheet: None, rows }) + } + } +} + +fn is_blank(cell: &Data) -> bool { + match cell { + Data::Empty => true, + Data::String(value) => value.trim().is_empty(), + _ => false, } } @@ -168,6 +179,11 @@ pub fn load(path: &Path, options: &Options) -> Result { let table = read(path, options).with_context(|| format!("cannot read roster {}", path.display()))?; let header_row = options.header_row.unwrap_or(1); + let at = |row: Option| SourceLocation { + file: Some(path.to_path_buf()), + sheet: table.sheet.clone(), + row, + }; let header = table .rows .iter() @@ -175,9 +191,8 @@ pub fn load(path: &Path, options: &Options) -> Result { .map(|(_, cells)| cells) .with_context(|| { format!( - "{} {:?}: header row {header_row} is missing (rows start at 1)", - path.display(), - table.sheet + "{}: header row {header_row} is missing (rows start at 1)", + at(None) ) })?; let mapping = options @@ -205,18 +220,16 @@ pub fn load(path: &Path, options: &Options) -> Result { Ok((id, name, canvas)) }) .transpose() - .with_context(|| format!("{} {:?}:{header_row}", path.display(), table.sheet))?; + .with_context(|| at(Some(header_row)).to_string())?; let mut entries = Vec::new(); let mut diagnostics = Vec::new(); for (row, cells) in &table.rows { - if *row <= header_row { + // A row with nothing in it is spacing, not a record: spreadsheets keep empty + // separator rows, and Excel's CSV export writes them as `,,`. + if *row <= header_row || cells.iter().all(is_blank) { continue; } - let location = SourceLocation { - file: Some(path.to_path_buf()), - sheet: table.sheet.clone(), - row: Some(*row), - }; + let location = at(Some(*row)); let parsed = (|| -> Result { if cells.len() != header.len() { bail!( @@ -264,5 +277,11 @@ pub fn load(path: &Path, options: &Options) -> Result { ), } } + // A wrong header row or worksheet leaves nobody on the roster; grading against it would + // turn every student into "not on the roster" and lose every 缺交. + if entries.is_empty() { + diagnostics + .push(InputDiagnostic::error(DiagnosticKind::EmptyRoster { header_row }).at(at(None))); + } Ok(Roster::with_diagnostics(entries, diagnostics)) } diff --git a/src/core/src/models/submission.rs b/src/core/src/models/submission.rs index f998bfa..782bd8f 100644 --- a/src/core/src/models/submission.rs +++ b/src/core/src/models/submission.rs @@ -547,6 +547,8 @@ pub enum DiagnosticKind { path: PathBuf, decision: crate::matching::Decision, }, + #[error("roster has no usable student rows below header row {header_row}")] + EmptyRoster { header_row: usize }, #[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}")] @@ -647,6 +649,22 @@ impl SourceLocation { } } +/// `file [sheet]:row`, with whichever parts are known. +impl fmt::Display for SourceLocation { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if let Some(file) = &self.file { + write!(f, "{}", file.display())?; + } + if let Some(sheet) = &self.sheet { + write!(f, " [{sheet}]")?; + } + if let Some(row) = self.row { + write!(f, ":{row}")?; + } + Ok(()) + } +} + #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] pub struct InputDiagnostic { pub severity: DiagnosticSeverity, @@ -691,16 +709,7 @@ impl InputDiagnostic { impl fmt::Display for InputDiagnostic { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { if let Some(location) = &self.location { - if let Some(file) = &location.file { - write!(f, "{}", file.display())?; - } - if let Some(sheet) = &location.sheet { - write!(f, " [{sheet}]")?; - } - if let Some(row) = location.row { - write!(f, ":{row}")?; - } - write!(f, ": ")?; + write!(f, "{location}: ")?; } write!(f, "{}", self.kind) } diff --git a/src/core/src/record.rs b/src/core/src/record.rs index d6384aa..3af8fdc 100644 --- a/src/core/src/record.rs +++ b/src/core/src/record.rs @@ -198,6 +198,30 @@ pub enum RecordError { } impl Record { + /// The roster members this record was graded with, as the evidence froze them — not as + /// a table that may since have changed. Students known only by a token taken from a + /// file name are not roster members. + pub fn roster(&self) -> crate::roster::Roster { + crate::roster::Roster::from_entries( + self.evidence + .students + .iter() + .filter_map(|student| { + let key = crate::models::StudentKey::parse(&student.student_id); + (!matches!(key, crate::models::StudentKey::Extracted(_))).then(|| { + crate::roster::RosterEntry { + key, + name: student.student_name.clone(), + canvas_user_id: student.canvas_user_id, + source: crate::roster::RosterSource::Supplied, + location: None, + } + }) + }) + .collect(), + ) + } + /// Unscored evidence. Students are sorted by id; two with one id, or one already graded, /// are refused: a revision could not tell them apart. pub fn new(mut evidence: Evidence) -> Result { diff --git a/src/core/src/roster.rs b/src/core/src/roster.rs index 4f0f013..433cf25 100644 --- a/src/core/src/roster.rs +++ b/src/core/src/roster.rs @@ -165,7 +165,9 @@ fn contradiction(kept: &RosterEntry, other: &RosterEntry) -> Option { 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(); + // The location of every row after the first, each of which is the one to delete. + let mut repeats: std::collections::BTreeMap>> = + Default::default(); let mut conflicted: std::collections::BTreeSet = Default::default(); let mut diagnostics = Vec::new(); @@ -192,7 +194,10 @@ fn merge_by_key(entries: Vec) -> (Vec, Vec) -> (Vec, Vec> = roster + .diagnostics + .iter() + .map(|d| d.location.as_ref().and_then(|l| l.row)) + .collect(); + assert_eq!(rows, vec![Some(4), Some(5)]); + assert!(roster.diagnostics.iter().all(|d| matches!( + &d.kind, + DiagnosticKind::DuplicateRosterEntry { count, .. } if *count == 3 + ))); } #[test] @@ -360,6 +395,53 @@ mod tests { assert_eq!(location.row, Some(2)); } + #[test] + fn test_blank_rows_are_spacing_not_records() { + let dir = tempfile::tempdir().unwrap(); + let path = write( + &dir, + "name,class,student_id\nAlice,A,2024010001\n,,\n , ,\nBob,B,2024010002\n,,\n,,\n", + ); + + let roster = load_roster(&path).unwrap(); + assert_eq!(roster.len(), 2); + assert!(roster.diagnostics.is_empty()); + } + + #[test] + fn test_a_roster_with_nobody_on_it_is_an_error() { + let dir = tempfile::tempdir().unwrap(); + let path = write(&dir, "name,class,student_id\n,,\n"); + + let roster = load_roster(&path).unwrap(); + let errors: Vec<_> = roster.errors().collect(); + assert_eq!(errors.len(), 1); + assert!(matches!( + errors[0].kind, + DiagnosticKind::EmptyRoster { header_row: 1 } + )); + assert_eq!( + errors[0].to_string().split(": ").next(), + Some(path.to_str().unwrap()) + ); + } + + #[test] + fn test_any_other_extension_is_read_as_csv_but_binary_spreadsheets_are_refused() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("roster.txt"); + std::fs::write(&path, "name,class,student_id\nAlice,A,2024010001\n").unwrap(); + assert_eq!(load_roster(&path).unwrap().len(), 1); + + let xls = dir.path().join("roster.xls"); + std::fs::write(&xls, [0xd0, 0xcf, 0x11, 0xe0]).unwrap(); + let error = format!("{:#}", load_roster(&xls).unwrap_err()); + assert!( + error.contains("save the roster as .xlsx or .csv"), + "{error}" + ); + } + #[test] fn test_unusable_rows_are_reported_rather_than_dropped_silently() { let dir = tempfile::tempdir().unwrap(); From 70c48d596f81e16dd6eff456d729606fe297b97e Mon Sep 17 00:00:00 2001 From: Acture Date: Wed, 7 Oct 2026 22:53:05 +0800 Subject: [PATCH 4/5] fix: read a given roster beside an explicit list; document explicit files (OSS-155) `summarize`/`db import-roster` with a manifest assignment rejected even an explicitly given roster file; only a missing path is refused now. Explicit files bypassing the noise filter and seeing rules by file name alone are now documented in the README, the example and the notes. --- README.md | 4 +++- examples/bundles/local_import/README.md | 2 ++ notes | 2 +- src/cli/tests/local_import.rs | 25 +++++++++++++++++++++++++ src/core/src/input/local.rs | 6 +++--- 5 files changed, 34 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index d619768..4fd1601 100644 --- a/README.md +++ b/README.md @@ -241,7 +241,9 @@ files = ["handins/answer.py"] # individual files or archives; [] means no submi ``` An explicit list cannot be combined with a roster table or submission directory. -It uses the same ownership and item matching component. Invalid identities/paths +It uses the same ownership and item matching component. A file named explicitly, in +the list or as a CLI path, is never skipped as noise (`.*`, `__*`), and student rules +see only its file name, never the directories above it. Invalid identities/paths stop grading and remain visible in `match` diagnostics. An identical repeated roster row is reported at each repeat and merged; conflicting rows are refused. CLI submission paths and `--roster` override configured paths in table mode. To use only configured diff --git a/examples/bundles/local_import/README.md b/examples/bundles/local_import/README.md index 07c1ae8..1494add 100644 --- a/examples/bundles/local_import/README.md +++ b/examples/bundles/local_import/README.md @@ -25,6 +25,8 @@ are relative to the working directory; positional submissions and `--roster` ove the corresponding configured paths. The explicit `input.students` list is a separate input mode and cannot be combined with a roster table or submission directories. It accepts individual files and archives; `files = []` records a non-submitter. +A listed file is graded whatever its name, even one a directory scan would skip as +noise (`.*`, `__*`). File-to-item and function matching still use the normal `[matching]` rules. Run `match` to see import diagnostics without executing student code. Invalid rows, diff --git a/notes b/notes index 62a0f18..785624f 160000 --- a/notes +++ b/notes @@ -1 +1 @@ -Subproject commit 62a0f187ca9de77ad09b1e156fb10844b2e6ea24 +Subproject commit 785624fda41ce4229b2ad8cedd6c354853af5200 diff --git a/src/cli/tests/local_import.rs b/src/cli/tests/local_import.rs index 31fb32e..7140b71 100644 --- a/src/cli/tests/local_import.rs +++ b/src/cli/tests/local_import.rs @@ -558,6 +558,31 @@ fn rosters_read_outside_grading_use_the_same_mapping_and_refuse_errors() { assert!(output.status.success(), "{args:?}"); assert!(String::from_utf8_lossy(&output.stdout).contains("张三")); } + // An explicit list has no table of its own, but a roster file given beside it is read. + write(dir.path(), "names.csv", "name,student_id\n张三,001\n"); + let output = run( + dir.path(), + &[ + "summarize", + "out/plain.json", + "--roster", + "names.csv", + "--assignment", + "manifest.toml", + ], + ); + assert!(output.status.success()); + assert!(String::from_utf8_lossy(&output.stdout).contains("张三")); + let output = run( + dir.path(), + &[ + "summarize", + "out/plain.json", + "--assignment", + "manifest.toml", + ], + ); + assert!(String::from_utf8_lossy(&output.stderr).contains("no roster table to read")); // Read with the default layout, the title row is taken for the header: refused, not a // summary that silently names nobody. let output = run( diff --git a/src/core/src/input/local.rs b/src/core/src/input/local.rs index 78ad1bd..e61e524 100644 --- a/src/core/src/input/local.rs +++ b/src/core/src/input/local.rs @@ -204,12 +204,12 @@ impl Config { assignment: Option<&Path>, path: Option<&Path>, ) -> Result> { - if !self.students.is_empty() { - bail!("input.students lists the class itself; it has no roster table"); - } let layout = self.roster.clone().unwrap_or_default(); let path = match path { Some(path) => Some(path.to_path_buf()), + None if !self.students.is_empty() => { + bail!("input.students lists the class itself; it has no roster table to read") + } None => layout .path .as_deref() From def60b9de40a56df2f76264d392238752daf3f3b Mon Sep 17 00:00:00 2001 From: Acture Date: Thu, 8 Oct 2026 23:18:12 +0800 Subject: [PATCH 5/5] fix: refuse a run whose archive cannot be expanded where it belongs (OSS-155) A read-only submission directory, or a file in the way of the extraction directory, was only a warning: the archive's owner was graded as having submitted nothing, which `missing = "zero"` turns into a zero for an environment fault. Preparing the extraction directory now fails as an error, so grading stops; a corrupt archive stays a warning against its owner. --- src/core/src/archive.rs | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/src/core/src/archive.rs b/src/core/src/archive.rs index 559c342..a623eb6 100644 --- a/src/core/src/archive.rs +++ b/src/core/src/archive.rs @@ -337,14 +337,16 @@ pub(crate) fn expand( Err(e) if e.kind() != std::io::ErrorKind::NotFound => Err(e), _ => std::fs::create_dir_all(target), }; + // Not the student's archive at fault but where it is expanded (a read-only directory, a + // file in the way): an error, so the run stops instead of grading the owner as empty. if let Err(e) = fresh { - diagnostics.push(unreadable( - archive, - format!( + diagnostics.push(InputDiagnostic::error(DiagnosticKind::ArchiveUnreadable { + archive: archive.to_path_buf(), + reason: format!( "cannot prepare extraction directory {}: {e}", target.display() ), - )); + })); return Vec::new(); } @@ -772,6 +774,30 @@ mod tests { ); } + #[test] + fn test_an_extraction_directory_that_cannot_be_prepared_is_an_error() { + let dir = tempfile::tempdir().unwrap(); + let archive = zip_with(dir.path(), "hw.zip", &[("lab5.py", b"x=1")]); + // A file where the extraction directory belongs. + let target = dir.path().join("out"); + std::fs::write(&target, "in the way").unwrap(); + let mut diagnostics = Vec::new(); + + let files = expand(&archive, &target, &is_gradeable, &mut diagnostics); + + assert!(files.is_empty()); + assert_eq!(diagnostics.len(), 1); + assert_eq!( + diagnostics[0].severity, + crate::models::DiagnosticSeverity::Error + ); + assert!(matches!( + &diagnostics[0].kind, + DiagnosticKind::ArchiveUnreadable { reason, .. } + if reason.contains("cannot prepare extraction directory") + )); + } + #[test] fn test_an_empty_archive_says_so() { let dir = tempfile::tempdir().unwrap();