From bbb870ab5d6fb78aedaf82b1af198a1b312d569c Mon Sep 17 00:00:00 2001 From: Acture Date: Sat, 3 Oct 2026 00:52:11 +0800 Subject: [PATCH 1/8] feat: versioned grading records and evidence-based rescoring (P-678) grade and run write one versioned grading record to --output: the evidence (case results, matching decisions, submission and test-bundle fingerprints) and append-only score revisions. rescore adds a revision from that evidence without running anything, and refuses evidence whose tests, submissions, matching or assignment changed. summarize, report, export, grades-push, the database (schema v2) and the Python API read a named revision. --- CLAUDE.md | 10 +- README.md | 32 +- crates/scriptmark-py/src/lib.rs | 304 ++++- crates/scriptmark/src/db/mod.rs | 174 ++- crates/scriptmark/src/db/results.rs | 235 ++-- crates/scriptmark/src/db/schema.rs | 25 +- crates/scriptmark/src/display.rs | 64 + crates/scriptmark/src/export.rs | 4 +- crates/scriptmark/src/lib.rs | 1 + crates/scriptmark/src/main.rs | 518 ++++++-- crates/scriptmark/src/matching.rs | 10 +- crates/scriptmark/src/models/result.rs | 29 + crates/scriptmark/src/record.rs | 1165 +++++++++++++++++ crates/scriptmark/src/runner/answers.rs | 2 +- crates/scriptmark/src/runner/frozen.rs | 2 +- crates/scriptmark/src/tui/ui.rs | 35 +- crates/scriptmark/tests/examples.rs | 70 +- crates/scriptmark/tests/input_equivalence.rs | 3 + crates/scriptmark/tests/matching.rs | 6 +- crates/scriptmark/tests/record.rs | 531 ++++++++ examples/bundles/rescoring/assignment.toml | 20 + examples/bundles/rescoring/regrade.toml | 15 + .../rescoring/submissions/alice_text.py | 6 + .../bundles/rescoring/submissions/bob_text.py | 7 + .../bundles/rescoring/tests/test_title.toml | 17 + .../bundles/rescoring/tests/test_words.toml | 27 + python/scriptmark/__init__.py | 4 + 27 files changed, 3011 insertions(+), 305 deletions(-) create mode 100644 crates/scriptmark/src/record.rs create mode 100644 crates/scriptmark/tests/record.rs create mode 100644 examples/bundles/rescoring/assignment.toml create mode 100644 examples/bundles/rescoring/regrade.toml create mode 100644 examples/bundles/rescoring/submissions/alice_text.py create mode 100644 examples/bundles/rescoring/submissions/bob_text.py create mode 100644 examples/bundles/rescoring/tests/test_title.toml create mode 100644 examples/bundles/rescoring/tests/test_words.toml diff --git a/CLAUDE.md b/CLAUDE.md index 17a1fef..40d3f1a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,8 +32,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co succeeds, stage `notes` in this repository and commit/push its new gitlink. A failed push must never leave a published parent pointer to unavailable notes. - The README contains the complete command sequence. Keep original histories and - resolve divergence without overwriting either side. The migration provenance and - P-673 uncommitted-document handoff are in `notes/scriptmark/MIGRATION.md`. + resolve divergence without overwriting either side. - Linear is the execution home. Keep runnable examples, code, measurements and raw artifacts here; maintain narrative/design documentation on the notes branch. Historical code paths inside imported plans describe their original revision. @@ -63,7 +62,8 @@ default Python is newer than the PyO3 version supports; see the P-676 validation Single `scriptmark` crate (lib + bin) with `scriptmark-py` as separate cdylib for PyO3. -Data flows: TOML specs + student files → Runner → Results → Display/DB/HTML. +Data flows: TOML specs + student files → Runner → grading record (evidence + score +revisions) → Display/DB/HTML/Canvas. `rescore` adds a revision from saved evidence. ``` models/ Data models, TOML spec parsing, grading policies @@ -71,7 +71,8 @@ discovery Student file discovery + ZIP extraction runner/ PythonExecutor (subprocess), orchestrator, sandbox (setrlimit), parametrize, expander, oracle, linter checker/ Checker trait (8 impls) + Rhai + Python checkers -db/ SQLite (rusqlite bundled): students, sessions, results, similarity +record Grading record: versioned evidence, score revisions, rescore reuse checks +db/ SQLite (rusqlite bundled): students, sessions (= revisions), results, similarity canvas/ Canvas LMS API client (reqwest + rustls): roster pull, grades push tui/ ratatui terminal UI: students/sessions/similarity tabs scriptmark-py PyO3 bindings: grade, run, discover, load_spec (maturin, separate crate) @@ -100,6 +101,7 @@ Zero and withheld grades stay distinct. See the notes' teacher contract for poli - `crates/scriptmark/src/runner/orchestrator.rs` — Runs vars→setup→expand→oracle→execute pipeline per student, tokio parallel. - `crates/scriptmark/src/models/spec.rs` — All TOML spec structs (TestSpec, TestCase, SetupStep, Parametrize, Oracle, LintConfig, CheckMethod). - `crates/scriptmark/src/discovery.rs` — File discovery + ZIP archive extraction with size/count limits. +- `crates/scriptmark/src/record.rs` — The grading record every consumer reads (`--output`): evidence with submission/spec fingerprints, append-only score revisions, `check()` refusing evidence whose tests, submissions or matching changed. - `crates/scriptmark/src/grading.rs` — GradingPolicy dispatch (templates + Rhai formulas). - `crates/scriptmark/src/main.rs` — All CLI command handlers. - `crates/scriptmark-py/src/lib.rs` — PyO3 bindings: grade(), run(), discover(), load_spec(), StudentResult, TestSpec classes. diff --git a/README.md b/README.md index bc68077..bbbaee1 100644 --- a/README.md +++ b/README.md @@ -66,9 +66,7 @@ and git push Push the notes successfully **before** updating the code repository's gitlink. If a fast-forward fails, resolve the divergence while preserving both versions. Do not force-push or discard notes. Runtime code, examples and artifacts stay in this repository; -documentation edits belong on the notes branch. Original documentation history and -the P-673 worktree handoff are recorded in -[the migration record](https://github.com/Acture/obsidian-vault/blob/project/scriptmark/scriptmark/MIGRATION.md). +documentation edits belong on the notes branch. Only when explicitly reproducing an older code revision, restore its recorded notes with `git submodule update --init --checkout -- notes` from a clean notes checkout. @@ -112,8 +110,17 @@ never pushed — instead of scoring it as 0. # Grade with roster and database storage; points and any curve come from assignment.toml scriptmark grade submissions/ -t tests/ -r roster.csv --db grades.db -a archive/ -# Run tests only (raw JSON output) -scriptmark run submissions/ -t tests/ -o results.json +# Run tests only: the grading record's evidence, with no score revision yet +scriptmark run submissions/ -t tests/ -o output/results.json + +# Score the saved evidence again after editing points or the curve in assignment.toml. +# Nothing runs; revision 2 is added beside revision 1, and whose grade changed is shown. +# Changed specs, teacher files, submissions or [matching] rules are refused: grade again. +scriptmark rescore output/results.json + +# Read any revision: summarize, export grades as CSV, report +scriptmark summarize output/results.json --revision 1 +scriptmark export output/results.json --revision 2 -o grades.csv # Preview student/file/function matching; edit assignment.toml to resolve candidates scriptmark match submissions/ -t tests/ -o output/matches.json @@ -126,7 +133,7 @@ scriptmark grade late/ -t tests/ --replay output/results.cases.json -o output/la scriptmark similarity submissions/ --threshold 0.8 # Generate HTML report -scriptmark report results.json -o report.html +scriptmark report output/results.json -o report.html # Canvas LMS: find a course, fetch an assignment, grade it offline, push grades back export CANVAS_TOKEN=... CANVAS_URL=https://canvas.university.edu @@ -134,7 +141,7 @@ scriptmark canvas courses scriptmark canvas assignments --course-id 12345 scriptmark canvas fetch --course-id 12345 --assignment-id 67890 -o canvas/hw1 scriptmark grade --canvas canvas/hw1 -t tests/ -scriptmark grades-push --course-id 12345 --assignment-id 67890 output/results.json +scriptmark grades-push --course-id 12345 --assignment-id 67890 output/results.json --revision 2 # Or just pull the roster scriptmark roster-pull --course-id 12345 @@ -149,14 +156,21 @@ scriptmark tui grades.db import scriptmark # One-shot grading, under the assignment.toml beside tests/ (or pass assignment=...). -# freeze= keeps the generated inputs; replay= grades on ones kept earlier. -results = scriptmark.grade(["submissions/"], "tests/", freeze="output/cases.json") +# freeze= keeps the generated inputs; replay= grades on ones kept earlier; output= writes +# the grading record. +results = scriptmark.grade( + ["submissions/"], "tests/", freeze="output/cases.json", output="output/results.json" +) for r in results: if r.grade is None: print(f"{r.student_id}: withheld ({r.reason})") else: print(f"{r.student_id}: {r.grade} ({r.score}/{r.max} points)") +# Score the record again under the policy as it is now, without running anything +change = scriptmark.rescore("output/results.json") # {"revision": 2, "changes": [...]} +first = scriptmark.load_record("output/results.json", revision=1) + # Discover student files (convenience view — drops non-submitters and orphan files) # Keys are rendered student keys: a bare 学号 once a roster confirms it, otherwise # `local:` — the prefix means nothing has vouched for that filename token yet. diff --git a/crates/scriptmark-py/src/lib.rs b/crates/scriptmark-py/src/lib.rs index 0dc1b27..64d3811 100644 --- a/crates/scriptmark-py/src/lib.rs +++ b/crates/scriptmark-py/src/lib.rs @@ -1,14 +1,16 @@ use std::collections::HashMap; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::Arc; use pyo3::prelude::*; use pyo3::types::PyDict; +use scriptmark::assignment::Declared; use scriptmark::discovery::{LocalInputOptions, load_local_input}; use scriptmark::export::word; -use scriptmark::grading::grade_all; +use scriptmark::grading::Policy; use scriptmark::models::{AssignmentInput, GradeOutcome, StudentReport, TestSpec}; +use scriptmark::record::{self, Evidence, Record, RecordError}; use scriptmark::runner::frozen::{self, Frozen, FrozenError, Generation}; use scriptmark::runner::generation::draws_a_seed; use scriptmark::runner::orchestrator::{RunOptions, run_all}; @@ -281,14 +283,79 @@ impl Freezing<'_> { None => Ok(()), } } + + /// The file holding the inputs a batch used: the one written, else the one replayed. + fn holder(&self) -> PyResult> { + self.freeze + .or(self.replay) + .map(|p| absolute(Path::new(p))) + .transpose() + } +} + +fn value_error(e: impl std::fmt::Display) -> PyErr { + pyo3::exceptions::PyValueError::new_err(e.to_string()) +} + +/// A grading record that cannot be read: missing is the OS error, anything else a ValueError. +fn record_error(e: RecordError) -> PyErr { + match &e { + RecordError::Io(_, io) if io.kind() == std::io::ErrorKind::NotFound => { + pyo3::exceptions::PyFileNotFoundError::new_err(e.to_string()) + } + RecordError::Io(..) => pyo3::exceptions::PyOSError::new_err(e.to_string()), + RecordError::Invalid(..) => value_error(e), + } +} + +/// A path as an absolute one, as the CLI records it, so that a record written here can be +/// rescored from anywhere. +fn absolute(path: &Path) -> PyResult { + std::path::absolute(path) + .map_err(|e| pyo3::exceptions::PyOSError::new_err(format!("{}: {e}", path.display()))) +} + +/// Refuse to write a record over one holding rescored revisions, before anything runs. +fn check_output(output: Option<&str>) -> PyResult<()> { + match output { + Some(path) => record::check_replaceable(Path::new(path)).map_err(value_error), + None => Ok(()), + } +} + +fn write_record(record: &Record, output: Option<&str>) -> PyResult<()> { + match output { + Some(path) => record + .write(Path::new(path)) + .map_err(|e| pyo3::exceptions::PyOSError::new_err(format!("{path}: {e}"))), + None => Ok(()), + } +} + +/// Load the assignment and specs from a tests directory given as an absolute path. +fn declared_for( + tests: &Path, + assignment: Option<&Path>, +) -> PyResult<(Declared, Vec, Policy)> { + let mut declared = scriptmark::assignment::load(assignment, tests) + .map_err(|e| value_error(format!("{e:#}")))?; + let specs = load_specs_from_dir(tests).map_err(spec_error)?; + let policy = + scriptmark::assignment::settle(&mut declared.assignment, &declared.grading, &specs) + .map_err(|e| value_error(format!("{e:#}")))?; + declared.matching.validate(&specs).map_err(value_error)?; + Ok((declared, specs, policy)) } /// Run tests for all students, returning a list of raw result dicts. /// /// `freeze` writes generated inputs and oracle answers; `replay` verifies and reuses -/// a frozen bundle without recomputing its answers. +/// a frozen bundle without recomputing its answers. `output` writes the grading record, +/// unscored, for `rescore` to score. #[pyfunction] -#[pyo3(signature = (submissions, tests, *, timeout=10, python="python3", assignment=None, freeze=None, replay=None))] +#[pyo3(signature = (submissions, tests, *, timeout=10, python="python3", assignment=None, freeze=None, replay=None, output=None))] +// Each is a keyword argument of the Python API; a struct would not be one. +#[allow(clippy::too_many_arguments)] fn run( submissions: Vec, tests: String, @@ -297,22 +364,28 @@ fn run( assignment: Option, freeze: Option, replay: Option, + output: Option, ) -> PyResult { - let specs = load_specs_from_dir(Path::new(&tests)).map_err(spec_error)?; - let mut declared = - scriptmark::assignment::load(assignment.as_deref().map(Path::new), Path::new(&tests)) - .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; - scriptmark::assignment::settle(&mut declared.assignment, &declared.grading, &specs) - .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; + check_output(output.as_deref())?; + let tests = absolute(Path::new(&tests))?; + let assignment = assignment.map(|p| absolute(Path::new(&p))).transpose()?; + let (declared, specs, _) = declared_for(&tests, assignment.as_deref())?; let freezing = Freezing { freeze: freeze.as_deref(), replay: replay.as_deref(), }; - let (results, inputs) = - run_grading(&submissions, specs, timeout, python, &freezing, &declared)?; + let (record, inputs) = run_grading( + &submissions, + &tests, + specs, + timeout, + python, + &freezing, + &declared, + )?; freezing.save(&inputs)?; - let json_val = serde_json::to_value(&results) - .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; + write_record(&record, output.as_deref())?; + let json_val = serde_json::to_value(&record.evidence.students).map_err(value_error)?; Python::with_gil(|py| json_to_py(py, &json_val)) } @@ -320,11 +393,14 @@ fn run( /// policy — `assignment.toml`, given or found beside the tests directory, exactly as the /// CLI reads it. /// -/// `freeze` and `replay` are as for `run`. +/// `freeze` and `replay` are as for `run`. `output` writes the grading record, with this +/// grade as its first revision. /// /// Returns a list of StudentResult objects. #[pyfunction] -#[pyo3(signature = (submissions, tests, *, timeout=10, python="python3", assignment=None, freeze=None, replay=None))] +#[pyo3(signature = (submissions, tests, *, timeout=10, python="python3", assignment=None, freeze=None, replay=None, output=None))] +// Each is a keyword argument of the Python API; a struct would not be one. +#[allow(clippy::too_many_arguments)] fn grade( submissions: Vec, tests: String, @@ -333,50 +409,147 @@ fn grade( assignment: Option, freeze: Option, replay: Option, + output: Option, ) -> PyResult> { - let value_error = |e: anyhow::Error| pyo3::exceptions::PyValueError::new_err(format!("{e:#}")); - let mut declared = - scriptmark::assignment::load(assignment.as_deref().map(Path::new), Path::new(&tests)) - .map_err(value_error)?; - let specs = load_specs_from_dir(Path::new(&tests)).map_err(spec_error)?; - let policy = - scriptmark::assignment::settle(&mut declared.assignment, &declared.grading, &specs) - .map_err(value_error)?; + check_output(output.as_deref())?; + let tests = absolute(Path::new(&tests))?; + let assignment = assignment.map(|p| absolute(Path::new(&p))).transpose()?; + let (declared, specs, policy) = declared_for(&tests, assignment.as_deref())?; let freezing = Freezing { freeze: freeze.as_deref(), replay: replay.as_deref(), }; - let (mut reports, inputs) = - run_grading(&submissions, specs, timeout, python, &freezing, &declared)?; - grade_all(&mut reports, &declared.assignment.items, &policy).map_err(value_error)?; + let (mut record, inputs) = run_grading( + &submissions, + &tests, + specs, + timeout, + python, + &freezing, + &declared, + )?; + let revision = record + .score(&declared.assignment.items, &policy) + .map_err(|e| value_error(format!("{e:#}")))?; freezing.save(&inputs)?; + write_record(&record, output.as_deref())?; - reports.sort_by(|a, b| a.student_id.cmp(&b.student_id)); - Ok(reports + let view = record.view(Some(revision)).map_err(value_error)?; + Ok(view + .reports .into_iter() .map(|r| PyStudentResult { inner: r }) .collect()) } -/// Shared logic: discover submissions, run the specs through the orchestrator. Returns the -/// reports and the inputs they were graded on, for `Freezing::save`. +/// Read a grading record as one of its revisions scored it: the latest unless `revision` +/// names one, and unscored evidence when nothing has scored it yet. +#[pyfunction] +#[pyo3(signature = (record, *, revision=None))] +fn load_record(record: String, revision: Option) -> PyResult> { + let record = Record::load(Path::new(&record)).map_err(record_error)?; + let view = record.view(revision).map_err(value_error)?; + Ok(view + .reports + .into_iter() + .map(|r| PyStudentResult { inner: r }) + .collect()) +} + +/// Score a grading record again under the policy as it is now, and add that as a new +/// revision. Nothing runs. The record is refused unless its assignment, submissions, +/// matching and tests are still what they were. +/// +/// `assignment` names the assignment.toml holding the policy; by default the one the +/// record was graded with, or one beside its tests directory. Returns +/// `{"revision": n, "changes": [...]}`, every student whose grade changed. +#[pyfunction] +#[pyo3(signature = (record, *, assignment=None))] +fn rescore(py: Python<'_>, record: String, assignment: Option) -> PyResult { + let path = Path::new(&record); + let mut saved = Record::load(path).map_err(record_error)?; + let inputs = saved.evidence.inputs.clone(); + let scriptmark::record::Source::Local { dirs, roster } = &inputs.source else { + return Err(value_error(format!( + "{record} was graded from a Canvas bundle: rescore it with `scriptmark rescore {record}`" + ))); + }; + let assignment = match assignment { + Some(p) => Some(absolute(Path::new(&p))?), + None => inputs.assignment.clone(), + }; + let (declared, specs, policy) = declared_for(&inputs.tests, assignment.as_deref())?; + let roster = roster + .as_deref() + .map(scriptmark::roster::load_roster) + .transpose() + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; + let input = discover_input(dirs, &declared, roster.as_ref())?; + saved + .check(&record::Current { + assignment: &input.assignment, + attempt_policy: input.attempt_policy, + matching: &declared.matching, + specs: &specs, + students: &input.students, + }) + .map_err(|e| value_error(format!("{e:#}")))?; + let previous = saved.latest().cloned(); + let revision = saved + .score(&input.assignment.items, &policy) + .map_err(|e| value_error(format!("{e:#}")))?; + let changes = record::diff( + previous.as_ref(), + saved.revision(revision).map_err(value_error)?, + ); + saved + .write(path) + .map_err(|e| pyo3::exceptions::PyOSError::new_err(format!("{record}: {e}")))?; + let json_val = serde_json::json!({ "revision": revision, "changes": changes }); + json_to_py(py, &json_val) +} + +/// The local input, as grading finds it, refusing one with errors. +fn discover_input( + dirs: &[PathBuf], + declared: &Declared, + roster: Option<&scriptmark::roster::Roster>, +) -> PyResult { + let paths: Vec<&Path> = dirs.iter().map(PathBuf::as_path).collect(); + let input = load_local_input( + &paths, + LocalInputOptions { + assignment: declared.assignment.clone(), + roster, + attempt_policy: declared.attempt_policy, + matching: Some(&declared.matching), + }, + ) + .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; + let errors: Vec = input.errors().map(ToString::to_string).collect(); + if !errors.is_empty() { + return Err(value_error(errors.join("; "))); + } + Ok(input) +} + +/// Shared logic: discover submissions, run the specs through the orchestrator, and record +/// what they found, unscored. Returns the record and the inputs it was graded on, for +/// `Freezing::save`. /// /// A seed drawn with nowhere to keep it could never be replayed, and a `freeze` path that /// cannot be written would lose the inputs after the whole class ran: both are refused /// before any student runs. fn run_grading( submissions: &[String], + tests: &Path, specs: Vec, timeout: u64, python: &str, freezing: &Freezing, - declared: &scriptmark::assignment::Declared, -) -> PyResult<(Vec, Frozen)> { - declared - .matching - .validate(&specs) - .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; + declared: &Declared, +) -> PyResult<(Record, Frozen)> { let generation = match freezing.replay { Some(path) => Generation::Replay(Frozen::load(Path::new(path)).map_err(frozen_error)?), None => Generation::fresh(), @@ -385,7 +558,7 @@ fn run_grading( && freezing.freeze.is_none() && let Some((spec, case)) = draws_a_seed(&specs) { - return Err(pyo3::exceptions::PyValueError::new_err(format!( + return Err(value_error(format!( "case '{case}' in '{spec}' draws a random seed: pass freeze='cases.json' to keep it, or declare seed = N" ))); } @@ -393,21 +566,14 @@ fn run_grading( frozen::writable(Path::new(path)) .map_err(|e| pyo3::exceptions::PyOSError::new_err(format!("{path}: {e}")))?; } - let paths: Vec<&Path> = submissions.iter().map(|p| Path::new(p.as_str())).collect(); - let input = load_local_input( - &paths, - LocalInputOptions { - assignment: declared.assignment.clone(), - attempt_policy: declared.attempt_policy, - matching: Some(&declared.matching), - ..Default::default() - }, - ) - .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; - let errors: Vec = input.errors().map(ToString::to_string).collect(); - if !errors.is_empty() { - return Err(pyo3::exceptions::PyValueError::new_err(errors.join("; "))); - } + let dirs = submissions + .iter() + .map(|p| absolute(Path::new(p))) + .collect::>>()?; + let input = discover_input(&dirs, declared, None)?; + let spec_versions = record::spec_versions(&specs).map_err(|e| value_error(format!("{e:#}")))?; + let versions = record::submission_versions(&input.students) + .map_err(|e| pyo3::exceptions::PyOSError::new_err(format!("{e:#}")))?; let executor = Arc::new(PythonExecutor::with_python_cmd(python)); let options = RunOptions { @@ -420,14 +586,38 @@ fn run_grading( let rt = tokio::runtime::Runtime::new() .map_err(|e| pyo3::exceptions::PyRuntimeError::new_err(e.to_string()))?; - rt.block_on(async { + let (mut reports, inputs) = rt.block_on(async { let bundles = prepare(specs, &generation, executor.clone(), timeout) .await - .map_err(|e| pyo3::exceptions::PyValueError::new_err(e.to_string()))?; + .map_err(value_error)?; let inputs = Frozen::of(&bundles); let reports = run_all(&input.students, bundles.into(), executor, &options).await; - Ok((reports, inputs)) + Ok::<_, PyErr>((reports, inputs)) + })?; + record::seal(&mut reports, &input.students, versions) + .map_err(|e| value_error(format!("{e:#}")))?; + let bundle = record::bundle_version( + spec_versions, + timeout, + options.python.clone(), + &inputs, + freezing.holder()?, + ); + let record = Record::new(Evidence { + scriptmark: env!("CARGO_PKG_VERSION").to_string(), + assignment: (&input.assignment).into(), + inputs: record::Inputs { + tests: tests.to_path_buf(), + assignment: declared.path.as_deref().map(absolute).transpose()?, + source: record::Source::Local { dirs, roster: None }, + }, + attempt_policy: input.attempt_policy, + matching: declared.matching.clone(), + bundle, + students: reports, }) + .map_err(|e| value_error(format!("{e:#}")))?; + Ok((record, inputs)) } /// Convert serde_json::Value to a Python object. @@ -471,5 +661,7 @@ fn _scriptmark(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(load_spec, m)?)?; m.add_function(wrap_pyfunction!(run, m)?)?; m.add_function(wrap_pyfunction!(grade, m)?)?; + m.add_function(wrap_pyfunction!(load_record, m)?)?; + m.add_function(wrap_pyfunction!(rescore, m)?)?; Ok(()) } diff --git a/crates/scriptmark/src/db/mod.rs b/crates/scriptmark/src/db/mod.rs index efa3969..629b993 100644 --- a/crates/scriptmark/src/db/mod.rs +++ b/crates/scriptmark/src/db/mod.rs @@ -23,11 +23,15 @@ pub enum DbError { DuplicateStudent(String), #[error( "this database is schema version {found}, and this build reads only {expected}; \ - databases from before per-item grading are not read — use a new file" + databases from before grading records are not read — use a new file" )] Version { found: i64, expected: i64 }, #[error("unreadable stored value: {0}")] Stored(String), + #[error("'{0}' has no grade: a session stores a score revision")] + Unscored(String), + #[error("{0}")] + Record(String), } pub struct Database { @@ -68,6 +72,17 @@ mod tests { use super::*; + /// Revision 1 of evidence named after the assignment. + fn of(assignment: &str) -> SessionOf<'_> { + SessionOf { + assignment, + evidence: assignment, + revision: 1, + bundle: "{}", + grading_policy: "{}", + } + } + #[test] fn test_open_memory() { let db = Database::open_memory().unwrap(); @@ -111,7 +126,7 @@ mod tests { ..graded("alice", 95.0) }]; - let session_id = db.save_session("hw5", &reports, None).unwrap(); + let session_id = db.save_session(&of("hw5"), &reports).unwrap().id; assert!(session_id > 0); let sessions = db.list_sessions().unwrap(); @@ -132,8 +147,8 @@ mod tests { let report1 = vec![graded("alice", 80.0)]; let report2 = vec![graded("alice", 95.0)]; - db.save_session("hw5", &report1, None).unwrap(); - db.save_session("hw8", &report2, None).unwrap(); + db.save_session(&of("hw5"), &report1).unwrap(); + db.save_session(&of("hw8"), &report2).unwrap(); let history = db.get_student_history("alice").unwrap(); assert_eq!(history.len(), 2); @@ -142,7 +157,7 @@ mod tests { #[test] fn test_similarity_save_and_query() { let db = Database::open_memory().unwrap(); - let session_id = db.save_session("hw5", &[], None).unwrap(); + let session_id = db.save_session(&of("hw5"), &[]).unwrap().id; let pairs = vec![SimilarityPair { student_a: "alice".to_string(), @@ -188,7 +203,7 @@ mod tests { let db = Database::open_memory().unwrap(); let reports = vec![graded("alice", 80.0), graded("alice", 95.0)]; - let err = db.save_session("hw5", &reports, None).unwrap_err(); + let err = db.save_session(&of("hw5"), &reports).unwrap_err(); assert!(matches!(err, DbError::DuplicateStudent(id) if id == "alice")); } @@ -200,7 +215,7 @@ mod tests { SubmissionOutcome::NotSubmitted, Reason::NotSubmitted, )]; - let session_id = db.save_session("hw5", &reports, None).unwrap(); + let session_id = db.save_session(&of("hw5"), &reports).unwrap().id; // A student who was never graded must not come back as a zero. let row = &db.get_results(session_id).unwrap()[0]; @@ -222,7 +237,7 @@ mod tests { db.import_roster(&Roster::from_pairs(&[("alice", "Alice Smith")])) .unwrap(); let reports = vec![graded("local:alice", 88.0)]; - let session_id = db.save_session("hw5", &reports, None).unwrap(); + let session_id = db.save_session(&of("hw5"), &reports).unwrap().id; let results = db.get_results(session_id).unwrap(); assert_eq!(results[0].student_name.as_deref(), Some("Alice Smith")); @@ -246,7 +261,7 @@ mod tests { .unwrap(); let reports = vec![graded("local:alice", 88.0)]; - let session_id = db.save_session("hw5", &reports, None).unwrap(); + let session_id = db.save_session(&of("hw5"), &reports).unwrap().id; let results = db.get_results(session_id).unwrap(); assert_eq!(results.len(), 1, "one stored result must yield one row"); @@ -258,7 +273,7 @@ mod tests { let db = Database::open_memory().unwrap(); db.import_roster(&Roster::from_pairs(&[("alice", "Alice Smith")])) .unwrap(); - db.save_session("hw5", &[graded("local:alice", 70.0)], None) + db.save_session(&of("hw5"), &[graded("local:alice", 70.0)]) .unwrap(); // Whichever form the teacher copies out of the summary must find the run. @@ -300,7 +315,7 @@ mod tests { ), ]; - db.save_session("hw5", &reports, None).unwrap(); + db.save_session(&of("hw5"), &reports).unwrap(); let sessions = db.list_sessions().unwrap(); assert_eq!(sessions[0].student_count, 2); // A missing grade is not a zero, so it must not halve the mean. @@ -315,7 +330,7 @@ mod tests { SubmissionOutcome::Executable, Reason::TeacherFault, )]; - db.save_session("hw5", &reports, None).unwrap(); + db.save_session(&of("hw5"), &reports).unwrap(); assert_eq!(db.list_sessions().unwrap()[0].avg_grade, None); } @@ -338,9 +353,8 @@ mod tests { SubmissionOutcome::Executable, Reason::EnvironmentFault, ), - StudentReport::new("dan", SubmissionOutcome::Executable), ]; - let session_id = db.save_session("hw5", &reports, None).unwrap(); + let session_id = db.save_session(&of("hw5"), &reports).unwrap().id; let rows = db.get_results(session_id).unwrap(); let row = |id: &str| rows.iter().find(|r| r.student_id == id).unwrap(); @@ -352,8 +366,7 @@ mod tests { assert_eq!(row("carol").state, RowState::Withheld); assert_eq!(row("carol").final_grade, None); assert_eq!(row("carol").reason, Some(Reason::EnvironmentFault)); - assert_eq!(row("dan").state, RowState::Unscored); - // Graded rows come first, withheld and unscored after. + // Graded rows come first, withheld after. assert!(rows[..2].iter().all(|r| r.state == RowState::Graded)); } @@ -363,7 +376,7 @@ mod tests { let path = dir.path().join("grades.db"); Database::open(&path) .unwrap() - .save_session("hw5", &[], None) + .save_session(&of("hw5"), &[]) .unwrap(); let reopened = Database::open(&path).unwrap(); assert_eq!(reopened.list_sessions().unwrap().len(), 1); @@ -386,11 +399,134 @@ mod tests { fn test_an_unknown_stored_state_is_an_error_not_a_dropped_row() { let db = Database::open_memory().unwrap(); let session_id = db - .save_session("hw5", &[graded("alice", 90.0)], None) - .unwrap(); + .save_session(&of("hw5"), &[graded("alice", 90.0)]) + .unwrap() + .id; db.conn .execute("UPDATE results SET reason = 'no_such_reason'", []) .unwrap(); assert!(db.get_results(session_id).is_err()); } + + #[test] + fn test_a_revision_is_saved_once_and_found_again() { + let db = Database::open_memory().unwrap(); + let reports = [graded("alice", 90.0)]; + let first = db.save_session(&of("hw5"), &reports).unwrap(); + let again = db.save_session(&of("hw5"), &reports).unwrap(); + assert!(first.created); + assert_eq!( + again, + Saved { + id: first.id, + created: false + } + ); + let second = SessionOf { + revision: 2, + ..of("hw5") + }; + assert!(db.save_session(&second, &reports).unwrap().created); + let sessions = db.list_sessions().unwrap(); + assert_eq!( + sessions.iter().map(|s| s.revision).collect::>(), + [2, 1], + "newest first, even within one second" + ); + assert!(sessions.iter().all(|s| s.evidence == "hw5")); + } + + #[test] + fn test_an_unscored_report_is_not_a_session() { + let db = Database::open_memory().unwrap(); + let reports = [StudentReport::new("dan", SubmissionOutcome::Executable)]; + let err = db.save_session(&of("hw5"), &reports).unwrap_err(); + assert!(matches!(err, DbError::Unscored(id) if id == "dan")); + assert!( + db.list_sessions().unwrap().is_empty(), + "nothing half-written" + ); + } + + #[test] + fn test_identity_fault_and_evidence_version_read_back() { + let db = Database::open_memory().unwrap(); + // The roster says otherwise; the record's own name wins. + db.import_roster(&Roster::from_pairs(&[("alice", "Someone Else")])) + .unwrap(); + let report = StudentReport { + student_name: Some("Alice Wu".into()), + canvas_user_id: Some(4242), + test_results: vec![TestResult { + item_id: "q".into(), + file: Some("/subs/alice_q.py".into()), + cases: vec![CaseResult { + case_name: "one".into(), + status: TestStatus::Error, + fault: Some(Fault::Teacher), + cause: Some(Cause::TeacherImport), + ..Default::default() + }], + }], + submission: Some(SubmissionVersion { + attempt: Some(2), + submitted_at: Some("2026-10-01T08:00:00Z".into()), + files: vec![FileVersion { + path: "/subs/alice_q.py".into(), + sha256: "ab".into(), + }], + archives: Vec::new(), + }), + ..withheld("alice", SubmissionOutcome::Executable, Reason::TeacherFault) + }; + let session = db + .save_session( + &SessionOf { + revision: 3, + ..of("hw5") + }, + std::slice::from_ref(&report), + ) + .unwrap() + .id; + + let row = &db.get_results(session).unwrap()[0]; + assert_eq!(row.student_name.as_deref(), Some("Alice Wu")); + assert_eq!(row.canvas_user_id, Some(4242)); + let stored = db.get_student_details(session, "alice").unwrap().unwrap(); + let case = &stored.test_results[0].cases[0]; + assert_eq!( + (case.fault, case.cause), + (Some(Fault::Teacher), Some(Cause::TeacherImport)) + ); + assert_eq!(stored.submission, report.submission); + assert_eq!(stored.grade, report.grade); + let (session, _) = &db.get_student_history("alice").unwrap()[0]; + assert_eq!((session.evidence.as_str(), session.revision), ("hw5", 3)); + } + + #[test] + fn test_a_database_from_before_grading_records_is_refused() { + let dir = tempfile::tempdir().unwrap(); + let old = dir.path().join("v1.db"); + rusqlite::Connection::open(&old) + .unwrap() + .execute_batch( + "CREATE TABLE sessions (id INTEGER PRIMARY KEY); PRAGMA user_version = 1;", + ) + .unwrap(); + let err = Database::open(&old) + .err() + .expect("a version 1 database is refused"); + assert!( + matches!( + err, + DbError::Version { + found: 1, + expected: 2 + } + ), + "{err}" + ); + } } diff --git a/crates/scriptmark/src/db/results.rs b/crates/scriptmark/src/db/results.rs index fbc86bf..4d39837 100644 --- a/crates/scriptmark/src/db/results.rs +++ b/crates/scriptmark/src/db/results.rs @@ -1,27 +1,49 @@ -use rusqlite::Row; +use rusqlite::{OptionalExtension, Row}; use crate::models::{GradeOutcome, Reason, StudentReport}; +use crate::record::Record; use super::{Database, DbError}; -/// A grading session row. +/// A grading session: one score revision of one grading record's evidence. #[derive(Debug, Clone)] pub struct Session { pub id: i64, pub assignment: String, - pub spec_title: Option, - pub grading_policy: Option, + /// The digest of the evidence the revision scored. + pub evidence: String, + pub revision: u32, + /// The test bundle's version, as the record holds it (JSON). + pub bundle: String, + /// The revision's items and grading policy (JSON). + pub grading_policy: String, pub student_count: i64, /// Over graded students only; `None` when nobody was graded. pub avg_grade: Option, pub created_at: String, } -/// Whether a stored result carries a grade. +/// What a session is a session of. +#[derive(Debug, Clone, Copy)] +pub struct SessionOf<'a> { + pub assignment: &'a str, + pub evidence: &'a str, + pub revision: u32, + pub bundle: &'a str, + pub grading_policy: &'a str, +} + +/// A stored session, and whether this save stored it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct Saved { + pub id: i64, + /// `false` when the revision was already saved, as session `id`. + pub created: bool, +} + +/// Whether a stored result is a number. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum RowState { - /// Saved from results that were never scored. - Unscored, Graded, Withheld, } @@ -31,6 +53,7 @@ pub enum RowState { pub struct ResultRow { pub student_id: String, pub student_name: Option, + pub canvas_user_id: Option, pub pass_rate: f64, pub state: RowState, /// Why withheld, or why a graded 0 is a policy 0. @@ -71,9 +94,14 @@ impl ResultRow { } } -const SESSION_COLUMNS: &str = "s.id, s.assignment, s.spec_title, s.grading_policy, s.student_count, s.avg_grade, s.created_at"; -const RESULT_COLUMNS: &str = "r.student_id, st.name, r.pass_rate, r.grade, r.reason, r.score, \ - r.max_score, r.raw_grade, r.final_grade, r.lint_score, r.total_cases, r.passed_cases"; +const SESSION_COLUMNS: &str = "s.id, s.assignment, s.evidence, s.revision, s.bundle, \ + s.grading_policy, s.student_count, s.avg_grade, s.created_at"; +/// The name the record stored, else the roster's. +const RESULT_COLUMNS: &str = "r.student_id, COALESCE(r.student_name, st.name), r.canvas_user_id, \ + r.pass_rate, r.grade, r.reason, r.score, r.max_score, r.raw_grade, r.final_grade, \ + r.lint_score, r.total_cases, r.passed_cases"; +/// Newest first; a revision saved in the same second as its predecessor still sorts after it. +const NEWEST_FIRST: &str = "s.created_at DESC, s.id DESC"; /// A run made without --roster leaves keys unconfirmed, so the id carries a `local:` prefix /// that students.id never does. Match either form. (substr, not ltrim: ltrim strips a @@ -88,11 +116,13 @@ fn session_at(row: &Row, at: usize) -> rusqlite::Result { Ok(Session { id: row.get(at)?, assignment: row.get(at + 1)?, - spec_title: row.get(at + 2)?, - grading_policy: row.get(at + 3)?, - student_count: row.get(at + 4)?, - avg_grade: row.get(at + 5)?, - created_at: row.get(at + 6)?, + evidence: row.get(at + 2)?, + revision: row.get(at + 3)?, + bundle: row.get(at + 4)?, + grading_policy: row.get(at + 5)?, + student_count: row.get(at + 6)?, + avg_grade: row.get(at + 7)?, + created_at: row.get(at + 8)?, }) } @@ -105,43 +135,65 @@ fn result_at(row: &Row, at: usize) -> rusqlite::Result { Box::new(DbError::Stored(what)), ) }; - let state = match row.get::<_, Option>(at + 3)?.as_deref() { - None => RowState::Unscored, - Some("graded") => RowState::Graded, - Some("withheld") => RowState::Withheld, - Some(other) => return Err(bad(3, format!("grade state '{other}'"))), + let state = match row.get::<_, String>(at + 4)?.as_str() { + "graded" => RowState::Graded, + "withheld" => RowState::Withheld, + other => return Err(bad(4, format!("grade state '{other}'"))), }; let reason = row - .get::<_, Option>(at + 4)? + .get::<_, Option>(at + 5)? .map(|text| { serde_json::from_value::(serde_json::Value::String(text.clone())) - .map_err(|_| bad(4, format!("reason '{text}'"))) + .map_err(|_| bad(5, format!("reason '{text}'"))) }) .transpose()?; + let canvas_user_id = row + .get::<_, Option>(at + 2)? + .map(|id| u64::try_from(id).map_err(|_| bad(2, format!("Canvas user id {id}")))) + .transpose()?; Ok(ResultRow { student_id: row.get(at)?, student_name: row.get(at + 1)?, - pass_rate: row.get(at + 2)?, + canvas_user_id, + pass_rate: row.get(at + 3)?, state, reason, - score: row.get(at + 5)?, - max_score: row.get(at + 6)?, - raw_grade: row.get(at + 7)?, - final_grade: row.get(at + 8)?, - lint_score: row.get(at + 9)?, - total_cases: row.get(at + 10)?, - passed_cases: row.get(at + 11)?, + score: row.get(at + 6)?, + max_score: row.get(at + 7)?, + raw_grade: row.get(at + 8)?, + final_grade: row.get(at + 9)?, + lint_score: row.get(at + 10)?, + total_cases: row.get(at + 11)?, + passed_cases: row.get(at + 12)?, }) } impl Database { - /// Save a grading session with all student reports. Returns session ID. + /// Save revision `revision` of `record` as a session. A revision already saved is found, + /// not saved twice. + pub fn save_revision(&self, record: &Record, revision: u32) -> Result { + let invalid = |e: anyhow::Error| DbError::Record(format!("{e:#}")); + let policy = &record.revision(revision).map_err(invalid)?.policy; + let view = record.view(Some(revision)).map_err(invalid)?; + self.save_session( + &SessionOf { + assignment: &record.evidence.assignment.name, + evidence: &record.digest, + revision, + bundle: &serde_json::to_string(&record.evidence.bundle)?, + grading_policy: &serde_json::to_string(policy)?, + }, + &view.reports, + ) + } + + /// Save scored reports as a session, in one transaction. Returns the session it already + /// has when this revision of this evidence was saved before. pub fn save_session( &self, - assignment: &str, + of: &SessionOf, reports: &[StudentReport], - grading_policy_json: Option<&str>, - ) -> Result { + ) -> Result { // Two reports for one student would be merged by UNIQUE(session_id, student_id) // while student_count still claimed both — the silent overwrite this model exists // to prevent. Refuse before writing anything. @@ -151,6 +203,20 @@ impl Database { return Err(DbError::DuplicateStudent(report.student_id.clone())); } } + if let Some(report) = reports.iter().find(|r| r.grade.is_none()) { + return Err(DbError::Unscored(report.student_id.clone())); + } + if let Some(id) = self + .conn + .query_row( + "SELECT id FROM sessions WHERE evidence = ?1 AND revision = ?2", + rusqlite::params![of.evidence, of.revision], + |row| row.get(0), + ) + .optional()? + { + return Ok(Saved { id, created: false }); + } // Average over students who actually have a grade: ungraded students would // otherwise drag the mean toward zero. @@ -160,55 +226,68 @@ impl Database { .collect(); let avg = (!graded.is_empty()).then(|| graded.iter().sum::() / graded.len() as f64); - self.conn.execute( - "INSERT INTO sessions (assignment, student_count, avg_grade, grading_policy) - VALUES (?1, ?2, ?3, ?4)", - rusqlite::params![assignment, reports.len() as i64, avg, grading_policy_json], - )?; - let session_id = self.conn.last_insert_rowid(); - - let mut stmt = self.conn.prepare( - "INSERT INTO results - (session_id, student_id, pass_rate, grade, reason, score, max_score, raw_grade, - final_grade, lint_score, total_cases, passed_cases, details) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13)", + let tx = self.conn.unchecked_transaction()?; + tx.execute( + "INSERT INTO sessions + (assignment, evidence, revision, bundle, grading_policy, student_count, avg_grade) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)", + rusqlite::params![ + of.assignment, + of.evidence, + of.revision, + of.bundle, + of.grading_policy, + reports.len() as i64, + avg + ], )?; - - for report in reports { - let grade = report.grade.as_ref(); - let (state, score, raw_grade) = match grade.map(|g| &g.outcome) { - None => (None, None, None), - Some(GradeOutcome::Graded { - score, raw_grade, .. - }) => (Some("graded"), Some(*score), Some(*raw_grade)), - Some(GradeOutcome::Withheld { .. }) => (Some("withheld"), None, None), - }; - stmt.execute(rusqlite::params![ - session_id, - report.student_id, - report.pass_rate(), - state, - grade - .and_then(|g| g.reason()) - .map(|r| crate::export::word(&r)), - score, - grade.map(|g| g.max), - raw_grade, - report.final_grade(), - report.lint_score(), - report.total_cases() as i64, - report.total_passed() as i64, - serde_json::to_string(report)?, - ])?; + let session_id = tx.last_insert_rowid(); + { + let mut stmt = tx.prepare( + "INSERT INTO results + (session_id, student_id, student_name, canvas_user_id, pass_rate, grade, reason, + score, max_score, raw_grade, final_grade, lint_score, total_cases, + passed_cases, details) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14, ?15)", + )?; + for report in reports { + let grade = report.grade.as_ref().expect("checked above"); + let (state, score, raw_grade) = match &grade.outcome { + GradeOutcome::Graded { + score, raw_grade, .. + } => ("graded", Some(*score), Some(*raw_grade)), + GradeOutcome::Withheld { .. } => ("withheld", None, None), + }; + stmt.execute(rusqlite::params![ + session_id, + report.student_id, + report.student_name, + report.canvas_user_id.map(|id| id as i64), + report.pass_rate(), + state, + grade.reason().map(|r| crate::export::word(&r)), + score, + grade.max, + raw_grade, + report.final_grade(), + report.lint_score(), + report.total_cases() as i64, + report.total_passed() as i64, + serde_json::to_string(report)?, + ])?; + } } - - Ok(session_id) + tx.commit()?; + Ok(Saved { + id: session_id, + created: true, + }) } /// List all sessions. pub fn list_sessions(&self) -> Result, DbError> { let mut stmt = self.conn.prepare(&format!( - "SELECT {SESSION_COLUMNS} FROM sessions s ORDER BY s.created_at DESC" + "SELECT {SESSION_COLUMNS} FROM sessions s ORDER BY {NEWEST_FIRST}" ))?; let rows = stmt.query_map([], |row| session_at(row, 0))?; Ok(rows.collect::>()?) @@ -267,10 +346,10 @@ impl Database { JOIN sessions s ON r.session_id = s.id {JOIN_STUDENT} WHERE r.student_id IN (?1, 'local:' || ?1, ?2) - ORDER BY s.created_at DESC" + ORDER BY {NEWEST_FIRST}" ))?; let rows = stmt.query_map(rusqlite::params![bare, student_id], |row| { - Ok((session_at(row, 0)?, result_at(row, 7)?)) + Ok((session_at(row, 0)?, result_at(row, 9)?)) })?; Ok(rows.collect::>()?) } diff --git a/crates/scriptmark/src/db/schema.rs b/crates/scriptmark/src/db/schema.rs index 109fdd6..f36cb48 100644 --- a/crates/scriptmark/src/db/schema.rs +++ b/crates/scriptmark/src/db/schema.rs @@ -2,9 +2,9 @@ use rusqlite::Connection; use super::DbError; -/// The schema this build writes. A database at any other version — including one made -/// before grades were scored per item, which has none — is refused, not upgraded. -pub const VERSION: i64 = 1; +/// The schema this build writes. A database at any other version — one made before grading +/// records, or before grades were scored per item — is refused, not upgraded. +pub const VERSION: i64 = 2; pub fn migrate(conn: &Connection) -> Result<(), DbError> { let version: i64 = conn.query_row("PRAGMA user_version", [], |row| row.get(0))?; @@ -29,24 +29,33 @@ pub fn migrate(conn: &Connection) -> Result<(), DbError> { created_at TEXT DEFAULT (datetime('now')) ); + -- One score revision of one grading record's evidence. CREATE TABLE IF NOT EXISTS sessions ( id INTEGER PRIMARY KEY AUTOINCREMENT, assignment TEXT NOT NULL, - spec_title TEXT, - grading_policy TEXT, + -- The record's evidence digest, and which of its revisions this is. + evidence TEXT NOT NULL, + revision INTEGER NOT NULL, + -- The test bundle's version: spec and source digests, seeds, answers (JSON). + bundle TEXT NOT NULL, + -- The revision's items and grading policy (JSON). + grading_policy TEXT NOT NULL, student_count INTEGER NOT NULL DEFAULT 0, -- NULL when nobody was graded. avg_grade REAL, - created_at TEXT DEFAULT (datetime('now')) + created_at TEXT DEFAULT (datetime('now')), + UNIQUE(evidence, revision) ); CREATE TABLE IF NOT EXISTS results ( id INTEGER PRIMARY KEY AUTOINCREMENT, session_id INTEGER NOT NULL REFERENCES sessions(id), student_id TEXT NOT NULL, + -- As the record holds them; the roster table only fills a missing name. + student_name TEXT, + canvas_user_id INTEGER, pass_rate REAL NOT NULL, - -- 'graded' or 'withheld'; NULL when the results were never scored. - grade TEXT CHECK (grade IN ('graded', 'withheld')), + grade TEXT NOT NULL CHECK (grade IN ('graded', 'withheld')), -- Why withheld, or why a graded 0 is a policy 0. reason TEXT, score REAL, diff --git a/crates/scriptmark/src/display.rs b/crates/scriptmark/src/display.rs index e5c8b83..5504e46 100644 --- a/crates/scriptmark/src/display.rs +++ b/crates/scriptmark/src/display.rs @@ -4,6 +4,7 @@ use scriptmark::export; use scriptmark::models::{ Fault, GradeOutcome, ItemOutcome, StudentReport, SubmissionOutcome, TestStatus, }; +use scriptmark::record::{Change, GradeState, Standing}; /// Display a summary table of all student results. pub fn display_summary(reports: &[&StudentReport], title: &str) { @@ -263,3 +264,66 @@ pub fn display_stats(reports: &[&StudentReport]) { println!(" {unscored:>4} not scored"); } } + +/// What a rescore changed: every student whose grade differs from the previous revision. +pub fn display_changes(previous: Option, revision: u32, changes: &[Change], students: usize) { + let Some(previous) = previous else { + println!( + "\n{} revision {revision} is the first score of this evidence", + "Rescored:".bold() + ); + return; + }; + println!( + "\n{} revision {revision} against revision {previous}: {} of {students} students changed", + "Rescored:".bold(), + changes.len().to_string().yellow() + ); + if changes.is_empty() { + return; + } + let mut table = Table::new(); + table + .load_preset(UTF8_FULL) + .set_content_arrangement(ContentArrangement::Dynamic) + .set_header(vec![ + Cell::new("ID").fg(Color::Cyan), + Cell::new(format!("Revision {previous}")).fg(Color::White), + Cell::new(format!("Revision {revision}")).fg(Color::Yellow), + ]); + for change in changes { + table.add_row(vec![ + Cell::new(&change.student_id), + Cell::new(change.before.as_ref().map(standing).unwrap_or_default()), + Cell::new(standing(&change.after)), + ]); + } + println!("{table}"); +} + +/// A grade in one cell: `87.5 (35/40, raw 87.5)`, or `withheld: teacher_fault`. +fn standing(standing: &Standing) -> String { + let points = |x: f64| export::number(x, export::POINTS_DECIMALS); + match (standing.state, standing.final_grade) { + (GradeState::Graded, Some(grade)) => { + let mut text = format!( + "{} ({}/{}, raw {})", + export::number(grade, export::POINTS_DECIMALS), + standing.score.map(points).unwrap_or_default(), + points(standing.max), + standing.raw_grade.map(points).unwrap_or_default() + ); + if let Some(reason) = standing.reason { + text.push_str(&format!(", {}", export::word(&reason))); + } + text + } + _ => format!( + "withheld: {}", + standing + .reason + .map(|r| export::word(&r)) + .unwrap_or_default() + ), + } +} diff --git a/crates/scriptmark/src/export.rs b/crates/scriptmark/src/export.rs index 2db6527..bf296a0 100644 --- a/crates/scriptmark/src/export.rs +++ b/crates/scriptmark/src/export.rs @@ -36,7 +36,7 @@ pub fn grades_to_push(reports: &[StudentReport]) -> Result { for report in reports { let Some(grade) = &report.grade else { bail!( - "{} has no grade: these are `run` results; score them with `grade`", + "{} has no grade: the record has no score revision; score it with `scriptmark rescore`", report.student_id ); }; @@ -88,7 +88,7 @@ pub fn write_grades_csv( for report in reports { let Some(grade) = &report.grade else { bail!( - "{} has no grade: these are `run` results; score them with `grade`", + "{} has no grade: the record has no score revision; score it with `scriptmark rescore`", report.student_id ); }; diff --git a/crates/scriptmark/src/lib.rs b/crates/scriptmark/src/lib.rs index 5651d19..8df43ff 100644 --- a/crates/scriptmark/src/lib.rs +++ b/crates/scriptmark/src/lib.rs @@ -7,6 +7,7 @@ pub mod export; pub mod grading; pub mod input; pub mod matching; +pub mod record; pub mod roster; pub mod similarity; pub mod spec_loader; diff --git a/crates/scriptmark/src/main.rs b/crates/scriptmark/src/main.rs index 3e30ce1..1643a60 100644 --- a/crates/scriptmark/src/main.rs +++ b/crates/scriptmark/src/main.rs @@ -13,6 +13,7 @@ use scriptmark::models::{ AssignmentInput, DiagnosticSeverity, StudentKey, StudentReport, StudentSubmission, SubmissionOutcome, TestSpec, }; +use scriptmark::record::{self, Evidence, Record, View}; use scriptmark::roster::load_roster; use scriptmark::runner::frozen::{self, Frozen, Generation}; use scriptmark::runner::generation::SeedSource; @@ -39,8 +40,12 @@ enum Commands { Run(RunArgs), /// Preview student, file and function matching without running student programs Match(MatchArgs), - /// Summarize existing results (re-analyze without re-running) + /// Score a grading record again under the current policy, without running anything + Rescore(RescoreArgs), + /// Summarize a grading record as one of its revisions scored it Summarize(SummarizeArgs), + /// Write a revision's grades as CSV + Export(ExportArgs), /// Canvas LMS: browse courses, and fetch an assignment for grading #[command(subcommand)] Canvas(CanvasCommand), @@ -195,14 +200,53 @@ struct MatchArgs { python: String, } +/// Which score revision of a grading record to read. +#[derive(clap::Args)] +struct RevisionArg { + /// The score revision to read; the latest by default + #[arg(long, value_name = "N")] + revision: Option, +} + +#[derive(Parser)] +struct RescoreArgs { + /// The grading record, as `grade` or `run` wrote it. The new revision is added to it. + record: PathBuf, + + /// The assignment.toml holding the policy to score under. Defaults to the one the + /// record was graded with, or one beside its tests directory. + #[arg(long)] + assignment: Option, + + /// Save the new revision to SQLite database + #[arg(long)] + db: Option, +} + #[derive(Parser)] struct SummarizeArgs { - /// Path to results JSON file, as `grade` wrote it + /// Path to the grading record, as `grade`, `run` or `rescore` wrote it results: PathBuf, /// Path to roster CSV #[arg(short, long)] roster: Option, + + #[command(flatten)] + revision: RevisionArg, +} + +#[derive(Parser)] +struct ExportArgs { + /// Path to the grading record + results: PathBuf, + + /// Where to write the grades + #[arg(short, long, default_value = "grades.csv")] + output: PathBuf, + + #[command(flatten)] + revision: RevisionArg, } #[derive(Subcommand)] @@ -292,8 +336,12 @@ struct GradesPushArgs { #[arg(long)] assignment_id: u64, - /// Path to results JSON file (from scriptmark grade) + /// Path to the grading record results: PathBuf, + + /// The score revision to push. Required when the record holds more than one. + #[arg(long, value_name = "N")] + revision: Option, } #[derive(Parser)] @@ -317,9 +365,12 @@ struct SimilarityArgs { #[derive(Parser)] struct ReportArgs { - /// Path to results JSON file (from scriptmark grade) + /// Path to the grading record results: PathBuf, + #[command(flatten)] + revision: RevisionArg, + /// Output HTML report path #[arg(short, long, default_value = "report.html")] output: PathBuf, @@ -380,7 +431,7 @@ fn build_local_input( submissions: &[PathBuf], declared: &Declared, roster_path: Option<&PathBuf>, - preview: bool, + purpose: Purpose, ) -> Result { let (assignment, attempt_policy) = (declared.assignment.clone(), declared.attempt_policy); @@ -406,7 +457,7 @@ fn build_local_input( // itself about who a 学号 belongs to would attribute somebody's work to the wrong name. // Stop before running anything rather than producing results nobody should act on. let errors: Vec = input.errors().map(|d| d.to_string()).collect(); - if !preview && !errors.is_empty() { + if purpose != Purpose::Preview && !errors.is_empty() { anyhow::bail!( "refusing to grade: {} problem(s) with the input\n {}", errors.len(), @@ -501,7 +552,9 @@ async fn main() -> Result<()> { Commands::Grade(args) => cmd_grade(args).await, Commands::Run(args) => cmd_run(args).await, Commands::Match(args) => cmd_match(args), + Commands::Rescore(args) => cmd_rescore(args), Commands::Summarize(args) => cmd_summarize(args), + Commands::Export(args) => cmd_export(args), Commands::Canvas(cmd) => cmd_canvas(cmd).await, Commands::RosterPull(args) => cmd_roster_pull(args).await, Commands::GradesPush(args) => cmd_grades_push(args).await, @@ -512,13 +565,31 @@ async fn main() -> Result<()> { } } -/// Read a results file `grade` or `run` wrote. A file from before grades were scored per -/// item is refused rather than reinterpreted: what its numbers meant is not recoverable. -fn parse_results(content: &str) -> Result> { - serde_json::from_str(content).context( - "failed to parse the results file; results written before per-item grading are not \ - read — grade the submissions again", - ) +/// A revision of the grading record at `path`: the latest unless one is named, and the +/// unscored evidence when nothing has scored it yet. +fn load_view(path: &Path, revision: Option) -> Result<(Record, View)> { + let record = Record::load(path)?; + let view = record.view(revision)?; + Ok((record, view)) +} + +/// The revision a view shows, for consumers that need grades. +fn scored(view: &View, path: &Path) -> Result { + view.revision.with_context(|| { + format!( + "{} has no score revision yet: score it with `scriptmark rescore {}`", + path.display(), + path.display() + ) + }) +} + +/// How a summary names what it shows. +fn shown(path: &Path, view: &View) -> String { + match view.revision { + Some(n) => format!("{} (revision {n} of {})", path.display(), view.of), + None => format!("{} (unscored)", path.display()), + } } /// A fault or cause as the snake_case word the JSON results use; empty when absent. @@ -533,8 +604,8 @@ fn label(value: Option) -> String { /// be prepared stops the run before any student is graded, and so do fresh inputs that /// would replace other inputs frozen beside `output`. /// -/// Returns the reports and the inputs they were graded on, for `save_frozen` once the -/// results are written. +/// Returns the reports, the inputs they were graded on, for `save_frozen` once the +/// results are written, and the interpreter that ran them. async fn run_bundles( students: &[StudentSubmission], specs: Vec, @@ -542,7 +613,7 @@ async fn run_bundles( timeout: u64, output: &Path, options: &FrozenArgs, -) -> Result<(Vec, Frozen)> { +) -> Result<(Vec, Frozen, String)> { let generation = match &options.replay { Some(path) => Generation::Replay(Frozen::load(path)?), None => Generation::fresh(), @@ -575,10 +646,11 @@ async fn run_bundles( } } run_options.python = executor.python_cmd().to_string(); + let python = run_options.python.clone(); // Units run in their own process groups, so the terminal's Ctrl-C reaches only the // grader: take them down with it rather than leave them running to their timeouts. tokio::select! { - reports = orchestrator::run_all(students, bundles.into(), executor, &run_options) => Ok((reports, inputs)), + reports = orchestrator::run_all(students, bundles.into(), executor, &run_options) => Ok((reports, inputs, python)), _ = tokio::signal::ctrl_c() => { scriptmark::runner::python::kill_all_units(); anyhow::bail!("interrupted: every running unit was stopped") @@ -611,6 +683,27 @@ struct Batch { specs: Vec, policy: Policy, matching: scriptmark::matching::Config, + /// Where it all was found, for the grading record. + inputs: record::Inputs, +} + +/// What a batch is prepared for. +#[derive(Clone, Copy, PartialEq, Eq)] +enum Purpose { + /// Grading: an input with errors is refused, and a Canvas bundle keeps its record of + /// what was graded. + Grade, + /// A matching preview: an input with errors is shown, not refused. + Preview, + /// Checking evidence against the input as it is now: errors are refused, and nothing is + /// written. + Verify, +} + +/// A path as an absolute one, without resolving links: what was found under it is recorded, +/// and must be found again under the same name whatever the working directory. +fn absolute(path: &Path) -> Result { + std::path::absolute(path).with_context(|| format!("cannot resolve {}", path.display())) } /// Load the assignment and the specs, settle the items and the policy against each other, @@ -618,13 +711,22 @@ struct Batch { fn prepare_batch( submissions: &[PathBuf], canvas: Option<&PathBuf>, - tests_dir: &std::path::Path, + tests_dir: &Path, assignment_path: Option<&PathBuf>, roster: Option<&PathBuf>, - preview: bool, + purpose: Purpose, ) -> Result { - let mut declared = assignment::load(assignment_path.map(PathBuf::as_path), tests_dir)?; - let specs = load_specs_from_dir(tests_dir).context("Failed to load test specifications")?; + let tests_dir = absolute(tests_dir)?; + let submissions = submissions + .iter() + .map(|p| absolute(p)) + .collect::>>()?; + let canvas = canvas.map(|p| absolute(p)).transpose()?; + let roster = roster.map(|p| absolute(p)).transpose()?; + let assignment_path = assignment_path.map(|p| absolute(p)).transpose()?; + + let mut declared = assignment::load(assignment_path.as_deref(), &tests_dir)?; + let specs = load_specs_from_dir(&tests_dir).context("Failed to load test specifications")?; println!("Loaded {} test specs", specs.len()); let policy = assignment::settle(&mut declared.assignment, &declared.grading, &specs)?; @@ -639,39 +741,116 @@ fn prepare_batch( // Names, roster membership and submission state all come from the model, so there is // no separate roster merge afterwards. - let input = match canvas { - Some(bundle) => build_canvas_input(bundle, &declared, preview)?, - None => build_local_input(submissions, &declared, roster, preview)?, + let (input, source) = match &canvas { + Some(bundle) => ( + build_canvas_input(bundle, &declared, purpose)?, + record::Source::Canvas { + bundle: bundle.clone(), + }, + ), + None => ( + build_local_input(&submissions, &declared, roster.as_ref(), purpose)?, + record::Source::Local { + dirs: submissions, + roster, + }, + ), }; Ok(Batch { input, specs, policy, matching: declared.matching, + inputs: record::Inputs { + tests: tests_dir, + assignment: declared.path.map(|p| absolute(&p)).transpose()?, + source, + }, }) } +/// Refuse to write a run's record over one holding rescored revisions — before anything +/// runs, so a refused run costs nothing. +fn check_output(output: &Path) -> Result<()> { + record::check_replaceable(output) + .map_err(anyhow::Error::msg) + .context("refusing to replace the grading record") +} + +/// Run the batch and record what it found, unscored. The submissions are fingerprinted +/// before the run and checked after it, and the specs before they are prepared. +async fn execute( + input: &AssignmentInput, + specs: Vec, + inputs: record::Inputs, + run_options: RunOptions, + timeout: u64, + output: &Path, + frozen_args: &FrozenArgs, +) -> Result<(Record, Frozen)> { + let spec_versions = record::spec_versions(&specs)?; + let versions = record::submission_versions(&input.students)?; + let matching = run_options.matching.clone(); + let (mut reports, frozen, python) = run_bundles( + &input.students, + specs, + run_options, + timeout, + output, + frozen_args, + ) + .await?; + record::seal(&mut reports, &input.students, versions)?; + let bundle = record::bundle_version( + spec_versions, + timeout, + python, + &frozen, + (!frozen.is_empty()).then(|| frozen::beside(output)), + ); + let record = Record::new(Evidence { + scriptmark: env!("CARGO_PKG_VERSION").to_string(), + assignment: (&input.assignment).into(), + inputs, + attempt_policy: input.attempt_policy, + matching, + bundle, + students: reports, + })?; + Ok((record, frozen)) +} + +/// Write a grading record where `output` says, its directory made first. +fn write_record(record: &Record, output: &Path) -> Result<()> { + record + .write(output) + .with_context(|| format!("failed to write {}", output.display())) +} + async fn cmd_grade(args: GradeArgs) -> Result<()> { + check_output(&args.output)?; let Batch { input, specs, policy, matching, + inputs, } = prepare_batch( &args.submissions, args.canvas.as_ref(), &args.tests_dir, args.assignment.as_ref(), args.roster.as_ref(), - false, + Purpose::Grade, )?; - let (mut reports, inputs) = run_bundles( - &input.students, + let (mut record, frozen) = execute( + &input, specs, + inputs, RunOptions { python: args.python, - matching, + matching: matching.clone(), concurrency: args .concurrency .map(|n| usize::try_from(n).unwrap_or(usize::MAX)), @@ -683,26 +862,23 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { .await?; let items = &input.assignment.items; - grading::grade_all(&mut reports, items, &policy)?; - reports.sort_by(|a, b| a.student_id.cmp(&b.student_id)); + let revision = record.score(items, &policy)?; + let view = record.view(Some(revision))?; + let reports = &view.reports; // Display let report_refs: Vec<_> = reports.iter().collect(); display::display_summary(&report_refs, &args.tests_dir.display().to_string()); display::display_failures(&report_refs); display::display_stats(&report_refs); - for warning in grading::diagnostics(&reports, items) { + for warning in grading::diagnostics(reports, items) { eprintln!(" warning: {warning}"); } - // Save raw results - if let Some(parent) = args.output.parent() { - std::fs::create_dir_all(parent)?; - } - let json = serde_json::to_string_pretty(&reports)?; - std::fs::write(&args.output, &json)?; + // The grading record: the evidence and its first revision. + write_record(&record, &args.output)?; println!("\nResults saved to {}", args.output.display()); - save_frozen(&inputs, &args.output)?; + save_frozen(&frozen, &args.output)?; // Archive: the evidence per case, and the grades per student. if let Some(archive_dir) = &args.archive { @@ -714,21 +890,17 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { .unwrap_or("results"); let archive_path = archive_dir.join(format!("archive_{stem}.{}", args.format)); let grades_path = archive_dir.join(format!("grades_{stem}.csv")); - if !inputs.is_empty() { + if !frozen.is_empty() { let cases_path = archive_dir.join(format!("cases_{stem}.json")); - inputs.write(&cases_path)?; + frozen.write(&cases_path)?; println!("Inputs written to {}", cases_path.display()); } - scriptmark::export::write_grades_csv( - &reports, - items, - std::fs::File::create(&grades_path)?, - )?; + scriptmark::export::write_grades_csv(reports, items, std::fs::File::create(&grades_path)?)?; println!("Grades written to {}", grades_path.display()); match args.format.as_str() { "json" => { - std::fs::write(&archive_path, &json)?; + std::fs::write(&archive_path, record.to_json())?; } "csv" => { let mut wtr = csv::Writer::from_path(&archive_path)?; @@ -746,7 +918,7 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { "fault", "cause", ])?; - for report in &reports { + for report in reports { let state = label(Some(report.submission_state)); let mut rows = 0usize; for test_result in &report.test_results { @@ -806,45 +978,55 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { println!("Archived to {}", archive_path.display()); } - // 9. Save to database if --db specified if let Some(db_path) = &args.db { - let database = - scriptmark::db::Database::open(db_path).context("Failed to open database")?; - - if let Some(roster) = &input.roster { - database - .import_roster(roster) - .context("Failed to import roster")?; - } + save_to_db(db_path, &record, revision, input.roster.as_ref())?; + } - let session_id = database - .save_session( - &input.assignment.name, - &reports, - Some(&serde_json::to_string(&serde_json::json!({ - "grading": policy.config(), - "items": items, - }))?), - ) - .context("Failed to save session to database")?; + 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::roster::Roster>, +) -> 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")?; + } + let saved = database + .save_revision(record, revision) + .context("Failed to save session to database")?; + if saved.created { + println!( + "Saved to database: {} (session #{}, revision {revision})", + db_path.display(), + saved.id + ); + } else { println!( - "Saved to database: {} (session #{})", + "Revision {revision} is already in {} as session #{}", db_path.display(), - session_id + saved.id ); } - Ok(()) } async fn cmd_run(args: RunArgs) -> Result<()> { + check_output(&args.output)?; // The policy is settled even though nothing is scored: a run whose results cannot be // graded should say so now, not after the class has run. let Batch { input, specs, matching, + inputs, .. } = prepare_batch( &args.submissions, @@ -852,16 +1034,17 @@ async fn cmd_run(args: RunArgs) -> Result<()> { &args.tests_dir, args.assignment.as_ref(), args.roster.as_ref(), - false, + Purpose::Grade, )?; - // A JSON array, the same shape `grade` writes; unscored until graded. - let (results, inputs) = run_bundles( - &input.students, + // The same grading record `grade` writes, with no revision until it is scored. + let (record, frozen) = execute( + &input, specs, + inputs, RunOptions { python: args.python, - matching, + matching: matching.clone(), concurrency: args .concurrency .map(|n| usize::try_from(n).unwrap_or(usize::MAX)), @@ -872,18 +1055,19 @@ async fn cmd_run(args: RunArgs) -> Result<()> { ) .await?; - if let Some(parent) = args.output.parent() { - std::fs::create_dir_all(parent)?; - } - let json = serde_json::to_string_pretty(&results)?; - std::fs::write(&args.output, &json)?; - println!("Results saved to {}", args.output.display()); - save_frozen(&inputs, &args.output)?; + write_record(&record, &args.output)?; + println!( + "Results saved to {}; score them with `scriptmark rescore {}`", + args.output.display(), + args.output.display() + ); + save_frozen(&frozen, &args.output)?; Ok(()) } fn cmd_match(args: MatchArgs) -> Result<()> { + check_output(&args.output)?; let Batch { input, specs, @@ -895,7 +1079,7 @@ fn cmd_match(args: MatchArgs) -> Result<()> { &args.tests_dir, args.assignment.as_ref(), args.roster.as_ref(), - true, + Purpose::Preview, )?; let preview = scriptmark::matching::preview(input, &specs, &matching, &args.python)?; if let Some(parent) = args.output.parent() { @@ -911,13 +1095,74 @@ fn cmd_match(args: MatchArgs) -> Result<()> { Ok(()) } +/// Score a grading record's evidence again under the policy as it is now, as a new +/// revision. Nothing runs: the record is refused unless its assignment, submissions, matching +/// and tests are still what they were, because otherwise its evidence describes something +/// that no longer exists. +fn cmd_rescore(args: RescoreArgs) -> Result<()> { + let mut record = Record::load(&args.record)?; + let inputs = record.evidence.inputs.clone(); + let (submissions, canvas, roster) = match &inputs.source { + record::Source::Local { dirs, roster } => (dirs.clone(), None, roster.clone()), + record::Source::Canvas { bundle } => (Vec::new(), Some(bundle.clone()), None), + }; + let assignment = args.assignment.clone().or(inputs.assignment.clone()); + let Batch { + input, + specs, + policy, + matching, + .. + } = prepare_batch( + &submissions, + canvas.as_ref(), + &inputs.tests, + assignment.as_ref(), + roster.as_ref(), + Purpose::Verify, + )?; + record.check(&record::Current { + assignment: &input.assignment, + attempt_policy: input.attempt_policy, + matching: &matching, + specs: &specs, + students: &input.students, + })?; + + let items = &input.assignment.items; + let previous = record.latest().cloned(); + let revision = record.score(items, &policy)?; + let view = record.view(Some(revision))?; + + let report_refs: Vec<_> = view.reports.iter().collect(); + display::display_summary(&report_refs, &shown(&args.record, &view)); + display::display_stats(&report_refs); + for warning in grading::diagnostics(&view.reports, items) { + eprintln!(" warning: {warning}"); + } + let current = record.revision(revision)?; + display::display_changes( + previous.as_ref().map(|p| p.revision), + revision, + &record::diff(previous.as_ref(), current), + view.reports.len(), + ); + + write_record(&record, &args.record)?; + 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())?; + } + Ok(()) +} + fn cmd_summarize(args: SummarizeArgs) -> Result<()> { - let content = std::fs::read_to_string(&args.results).context("Failed to read results file")?; - let mut reports = parse_results(&content)?; + 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")?; - for report in reports.iter_mut() { + 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:` // prefix, would match nothing. `name_of` answers only when the key is @@ -929,18 +1174,35 @@ fn cmd_summarize(args: SummarizeArgs) -> Result<()> { } } - // Shown as `grade` scored them. Scoring again under another policy is P-678's regrade, - // which records what changed; a summary that silently re-scored could not. - reports.sort_by(|a, b| a.student_id.cmp(&b.student_id)); - - let report_refs: Vec<_> = reports.iter().collect(); - display::display_summary(&report_refs, &args.results.display().to_string()); + // Shown as the revision scored them: a summary never scores. `rescore` does, and + // records what changed. + let report_refs: Vec<_> = view.reports.iter().collect(); + display::display_summary(&report_refs, &shown(&args.results, &view)); display::display_failures(&report_refs); display::display_stats(&report_refs); Ok(()) } +fn cmd_export(args: ExportArgs) -> Result<()> { + let (_, view) = load_view(&args.results, args.revision.revision)?; + let revision = scored(&view, &args.results)?; + if let Some(parent) = args.output.parent().filter(|p| !p.as_os_str().is_empty()) { + std::fs::create_dir_all(parent)?; + } + scriptmark::export::write_grades_csv( + &view.reports, + &view.items, + std::fs::File::create(&args.output) + .with_context(|| format!("failed to create {}", args.output.display()))?, + )?; + println!( + "Grades of revision {revision} written to {}", + args.output.display() + ); + Ok(()) +} + /// Build the input for a `--canvas ` run. /// /// The declared `assignment.toml` is loaded and handed to `normalize`: it carries the @@ -952,7 +1214,7 @@ fn cmd_summarize(args: SummarizeArgs) -> Result<()> { fn build_canvas_input( bundle: &std::path::Path, declared: &Declared, - preview: bool, + purpose: Purpose, ) -> Result { use scriptmark::canvas::bundle; @@ -990,7 +1252,7 @@ fn build_canvas_input( report_input(&input); let errors: Vec = input.errors().map(|d| d.to_string()).collect(); - if !preview && !errors.is_empty() { + if purpose != Purpose::Preview && !errors.is_empty() { anyhow::bail!( "refusing to grade: {} problem(s) with the input\n {}", errors.len(), @@ -999,8 +1261,8 @@ fn build_canvas_input( } // The record of which attempt was graded, and of every file's provenance, outlives the - // process that produced it. - if !preview { + // process that produced it. Checking a record against the bundle writes nothing. + if purpose == Purpose::Grade { bundle::save_input(bundle, &input) .with_context(|| format!("failed to write {}", bundle::input_path(bundle).display()))?; } @@ -1117,24 +1379,53 @@ async fn cmd_roster_pull(args: RosterPullArgs) -> Result<()> { } async fn cmd_grades_push(args: GradesPushArgs) -> Result<()> { - let client = scriptmark::canvas::CanvasClient::new(&args.canvas_url) - .context("Failed to create Canvas client (is CANVAS_TOKEN set?)")?; - - let content = std::fs::read_to_string(&args.results).context("Failed to read results file")?; - let reports = parse_results(&content)?; + // Everything about what to push is settled before Canvas is contacted. + let record = Record::load(&args.results)?; + if args.revision.is_none() && record.revisions.len() > 1 { + anyhow::bail!( + "{} holds {} score revisions; name the one to push with --revision", + args.results.display(), + record.revisions.len() + ); + } + // The record says which Canvas assignment its evidence belongs to; pushing it anywhere + // else would publish one assignment's grades as another's. + let graded_for = &record.evidence.assignment; + for (flag, recorded, what) in [ + (args.course_id, graded_for.canvas_course_id, "course"), + ( + args.assignment_id, + graded_for.canvas_assignment_id, + "assignment", + ), + ] { + if let Some(recorded) = recorded + && recorded != flag + { + anyhow::bail!( + "refusing to push: {} was graded for Canvas {what} {recorded}, not {flag}", + args.results.display() + ); + } + } + let view = record.view(args.revision)?; + let revision = scored(&view, &args.results)?; // Only graded students are pushed — a real 0 included, a withheld grade never. The // Canvas user id is the one import recorded: parsing student_id as an integer would // either fail for every 学号 or, worse, post to whichever user held that number. let scriptmark::export::PushSet { grades, skipped } = - scriptmark::export::grades_to_push(&reports)?; + scriptmark::export::grades_to_push(&view.reports)?; for (why, n) in &skipped { println!("Skipping {n} student(s): {why}"); } + let client = scriptmark::canvas::CanvasClient::new(&args.canvas_url) + .context("Failed to create Canvas client (is CANVAS_TOKEN set?)")?; println!( - "Pushing {} grades to Canvas assignment {}...", + "Pushing {} grades of revision {revision} (evidence {}) to Canvas assignment {}...", grades.len(), + &record.digest[..12], args.assignment_id ); let results = client @@ -1232,8 +1523,8 @@ fn cmd_similarity(args: SimilarityArgs) -> Result<()> { } fn cmd_report(args: ReportArgs) -> Result<()> { - let content = std::fs::read_to_string(&args.results).context("Failed to read results file")?; - let reports = parse_results(&content)?; + let (_, view) = load_view(&args.results, args.revision.revision)?; + let reports = view.reports; let similarity = if let Some(sim_dir) = &args.similarity_dir { let mut submissions: std::collections::HashMap> = @@ -1299,15 +1590,17 @@ fn cmd_db(cmd: DbCommand) -> Result<()> { } use owo_colors::OwoColorize; println!( - "{:>4} {:<20} {:>8} {:>8} Date", - "ID", "Assignment", "Students", "Avg" + "{:>4} {:<20} {:>3} {:<12} {:>8} {:>8} Date", + "ID", "Assignment", "Rev", "Evidence", "Students", "Avg" ); - println!("{}", "-".repeat(70)); + println!("{}", "-".repeat(90)); for s in &sessions { println!( - "{:>4} {:<20} {:>8} {:>7} {}", + "{:>4} {:<20} {:>3} {:<12} {:>8} {:>7} {}", s.id.to_string().cyan(), s.assignment, + s.revision, + &s.evidence[..s.evidence.len().min(12)], s.student_count, s.avg_grade .map(|a| format!("{a:.1}")) @@ -1331,10 +1624,10 @@ fn cmd_db(cmd: DbCommand) -> Result<()> { use owo_colors::OwoColorize; println!("History for {} ({}):\n", name.bold(), student_id.cyan()); println!( - "{:<15} {:>24} {:>10} {:>8}/{:<8} Date", - "Assignment", "Grade", "Pass Rate", "Passed", "Total" + "{:<15} {:>3} {:>24} {:>10} {:>8}/{:<8} Date", + "Assignment", "Rev", "Grade", "Pass Rate", "Passed", "Total" ); - println!("{}", "-".repeat(90)); + println!("{}", "-".repeat(96)); for (session, result) in &history { let grade_color = match result.fraction() { Some(f) if f >= 0.9 => "\x1b[32m", @@ -1344,8 +1637,9 @@ fn cmd_db(cmd: DbCommand) -> Result<()> { }; let grade_text = result.grade_text(); println!( - "{:<15} {}{:>24}\x1b[0m {:>9.1}% {:>8}/{} {}", + "{:<15} {:>3} {}{:>24}\x1b[0m {:>9.1}% {:>8}/{} {}", session.assignment, + session.revision, grade_color, grade_text, result.pass_rate, diff --git a/crates/scriptmark/src/matching.rs b/crates/scriptmark/src/matching.rs index 468d7f3..368f5a6 100644 --- a/crates/scriptmark/src/matching.rs +++ b/crates/scriptmark/src/matching.rs @@ -10,7 +10,7 @@ use serde::{Deserialize, Serialize}; use crate::models::{FileOrigin, StudentFile, StudentSubmission, Target, TestSpec, normalize_key}; -#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] #[serde(default, deny_unknown_fields)] pub struct Config { pub students: Vec, @@ -19,7 +19,7 @@ pub struct Config { pub overrides: Vec, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] pub enum StudentRule { /// Zero-based component of the directory path relative to the scanned root. @@ -30,7 +30,7 @@ pub enum StudentRule { Regex { regex: String }, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct OwnerOverride { /// Relative path within a scanned root, or an absolute path. @@ -38,7 +38,7 @@ pub struct OwnerOverride { pub student: String, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct ItemRule { pub id: String, @@ -50,7 +50,7 @@ pub struct ItemRule { pub functions: BTreeMap>, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct ItemOverride { /// A rendered student key (`local:alice`, `canvas:123`, or a confirmed number). diff --git a/crates/scriptmark/src/models/result.rs b/crates/scriptmark/src/models/result.rs index fb2571c..28fca74 100644 --- a/crates/scriptmark/src/models/result.rs +++ b/crates/scriptmark/src/models/result.rs @@ -210,6 +210,10 @@ pub struct StudentReport { /// failed test case and scored. #[serde(default)] pub error: Option, + /// What was graded: the attempt and the content of its files, so that evidence is never + /// reused for a submission that has since changed. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub submission: Option, /// The grade and how it was reached. `None` until scored: `run` writes evidence only. #[serde(default, skip_serializing_if = "Option::is_none")] pub grade: Option, @@ -229,6 +233,7 @@ impl StudentReport { excused: false, lint: None, error: None, + submission: None, grade: None, } } @@ -284,6 +289,30 @@ impl StudentReport { } } +/// The selected attempt of a submission, as its bytes stood when it was graded. Identity, +/// delivery state and excusal are the report's own fields; this holds only what they lack. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct SubmissionVersion { + /// `None` when nothing was received. + pub attempt: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub submitted_at: Option, + /// Every runnable file of the attempt, by path. + pub files: Vec, + /// The archives those files were extracted from: a replaced archive is a changed + /// submission even while an earlier extraction still sits on disk. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub archives: Vec, +} + +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct FileVersion { + pub path: std::path::PathBuf, + pub sha256: String, +} + /// What the style check found. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(tag = "outcome", rename_all = "snake_case")] diff --git a/crates/scriptmark/src/record.rs b/crates/scriptmark/src/record.rs new file mode 100644 index 0000000..8c7159d --- /dev/null +++ b/crates/scriptmark/src/record.rs @@ -0,0 +1,1165 @@ +//! The grading record: the evidence a batch was graded on, and every score revision reached +//! from it, in one versioned file. +//! +//! Evidence is what execution produced — each student's case results and matching +//! decisions, and the submissions and test bundle they came from. It is never rewritten. A +//! revision is that evidence scored under one policy. Rescoring appends a revision and runs +//! nothing, and it refuses evidence whose submissions, matching or tests have changed since: +//! that evidence no longer describes them. + +use std::collections::{BTreeMap, BTreeSet}; +use std::io::Write; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result, bail}; +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +use crate::grading::{self, Policy}; +use crate::matching::{self, ItemMatch}; +use crate::models::{ + Assignment, AttemptPolicy, Check, FileOrigin, FileVersion, Grade, GradeOutcome, GradingConfig, + GradingItem, Reason, StudentReport, StudentSubmission, SubmissionVersion, TestSpec, +}; +use crate::runner::answers::{digest, fingerprint}; +use crate::runner::frozen::{Frozen, scratch}; + +/// The layout of the file. A newer one is refused by number, never half-read. +pub const FORMAT: u32 = 1; + +/// How many differences a refusal lists before it only counts the rest. +const SHOWN: usize = 20; + +/// Evidence and the revisions scored from it. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Record { + pub format: u32, + pub evidence: Evidence, + /// SHA-256 of `evidence`. Revisions and database sessions name the evidence by it, and a + /// record whose evidence no longer hashes to it is refused. + pub digest: String, + /// Oldest first, numbered from 1. Rescoring appends; nothing rewrites one. + pub revisions: Vec, +} + +/// What a batch was graded on, and what running it found. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Evidence { + /// The build that ran the students. + pub scriptmark: String, + pub assignment: AssignmentId, + /// Where the batch's inputs were found; `rescore` looks there unless told otherwise. + pub inputs: Inputs, + pub attempt_policy: AttemptPolicy, + pub matching: matching::Config, + pub bundle: BundleVersion, + /// One per student, sorted by `student_id`, ungraded. + pub students: Vec, +} + +/// Which assignment this is, after the input settled it. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct AssignmentId { + pub name: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub canvas_course_id: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub canvas_assignment_id: Option, +} + +impl From<&Assignment> for AssignmentId { + fn from(assignment: &Assignment) -> Self { + Self { + name: assignment.name.clone(), + canvas_course_id: assignment.canvas_course_id, + canvas_assignment_id: assignment.canvas_assignment_id, + } + } +} + +impl std::fmt::Display for AssignmentId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "'{}'", self.name)?; + if let (Some(course), Some(assignment)) = (self.canvas_course_id, self.canvas_assignment_id) + { + write!(f, " (Canvas course {course}, assignment {assignment})")?; + } + Ok(()) + } +} + +/// Where a batch's inputs are, as absolute paths. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Inputs { + pub tests: PathBuf, + /// The `assignment.toml` read, when there was one. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub assignment: Option, + pub source: Source, +} + +/// Where the submissions came from. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] +pub enum Source { + Local { + dirs: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + roster: Option, + }, + Canvas { + bundle: PathBuf, + }, +} + +/// The test bundle a batch ran. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct BundleVersion { + /// SHA-256 over the timeout and every spec's digest and sources, in order: the bundle's + /// version. + pub digest: String, + /// The `--timeout` every call ran under, where a spec did not set its own. + pub timeout: u64, + /// The interpreter that ran the students. Provenance only: rescoring never runs it. + pub python: String, + /// In load order, which is also the order lint looks for a `[lint]` spec in. + pub specs: Vec, + /// The frozen inputs and answers the batch used, when it had any. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub frozen: Option, +} + +/// One test spec as it ran. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct SpecVersion { + pub name: String, + /// SHA-256 of the spec as loaded: every case, check, timeout and path it declares. + pub digest: String, + /// SHA-256 of every teacher file the spec reads, by absolute path. + pub sources: BTreeMap, + /// Each template's seed, by template name. + #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] + pub seeds: BTreeMap, + /// The checksum of the spec's frozen reference answers, when it computes any. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub answers: Option, +} + +/// Frozen inputs and answers, linked rather than copied: their file is where `--replay` +/// reads them. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct FrozenLink { + /// The file holding them, when there is one. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub path: Option, + /// SHA-256 of their JSON, as `Frozen::to_json` writes it. + pub digest: String, +} + +/// The evidence scored under one policy. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Revision { + pub revision: u32, + /// The build that scored it: a grading fix can change grades under an identical policy. + pub scriptmark: String, + /// The evidence it was scored from: the record's `digest`. + pub evidence: String, + pub policy: ScoringPolicy, + /// One per student, by `student_id`. + pub grades: BTreeMap, + /// Accidental edits must not quietly publish other grades. + pub checksum: String, +} + +/// Everything a score depends on besides the evidence. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct ScoringPolicy { + pub items: Vec, + pub grading: GradingConfig, + pub derived_items: bool, +} + +/// The reports as one revision scored them. +#[derive(Debug, Clone)] +pub struct View { + /// `None` for evidence nothing has scored yet. + pub revision: Option, + /// How many revisions the record holds. + pub of: usize, + pub reports: Vec, + /// The items the revision scored, in declaration order; empty when unscored. + pub items: Vec, +} + +#[derive(Debug, thiserror::Error)] +pub enum RecordError { + #[error("cannot read grading record {path}: {error}", path = .0.display(), error = .1)] + Io(PathBuf, std::io::Error), + #[error("{path} is not a grading record this build can read: {why}", path = .0.display(), why = .1)] + Invalid(PathBuf, String), +} + +impl Record { + /// 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 { + evidence + .students + .sort_by(|a, b| a.student_id.cmp(&b.student_id)); + if let Some(pair) = evidence + .students + .windows(2) + .find(|pair| pair[0].student_id == pair[1].student_id) + { + bail!( + "two reports share student id '{}'; a grading record cannot tell them apart", + pair[0].student_id + ); + } + if let Some(report) = evidence.students.iter().find(|r| r.grade.is_some()) { + bail!( + "the evidence for '{}' already carries a grade", + report.student_id + ); + } + let digest = evidence_digest(&evidence); + Ok(Record { + format: FORMAT, + evidence, + digest, + revisions: Vec::new(), + }) + } + + /// The format first, then the strict body, so a newer record is refused by its number + /// rather than by a field this build has never heard of — and a results list from before + /// grading records by what it is. + pub fn from_json(text: &str) -> Result { + let value: Value = serde_json::from_str(text).map_err(|e| e.to_string())?; + if value.is_array() { + return Err( + "it is a list of reports from before grading records; grade the submissions again" + .into(), + ); + } + match value.get("format").and_then(Value::as_u64) { + None => Err("it has no format".into()), + Some(n) if n == u64::from(FORMAT) => { + let record: Record = serde_json::from_value(value).map_err(|e| e.to_string())?; + record.verify()?; + Ok(record) + } + Some(other) => Err(format!( + "it is format {other}, and this build reads format {FORMAT}" + )), + } + } + + pub fn load(path: &Path) -> Result { + let text = + std::fs::read_to_string(path).map_err(|e| RecordError::Io(path.to_path_buf(), e))?; + Record::from_json(&text).map_err(|e| RecordError::Invalid(path.to_path_buf(), e)) + } + + pub fn to_json(&self) -> String { + serde_json::to_string_pretty(self).expect("a grading record is plain JSON") + "\n" + } + + /// Write through a temporary file and a rename: the record is the only copy of its + /// revisions, and a half-written one would lose them all. + pub fn write(&self, path: &Path) -> std::io::Result<()> { + let mut file = scratch(path)?; + file.write_all(self.to_json().as_bytes())?; + file.persist(path).map_err(|e| e.error)?; + Ok(()) + } + + /// What the stored digests and numbering claim must hold. + fn verify(&self) -> Result<(), String> { + if evidence_digest(&self.evidence) != self.digest { + return Err( + "its evidence does not match its digest: it was edited or is incomplete".into(), + ); + } + let students = &self.evidence.students; + if students + .windows(2) + .any(|pair| pair[0].student_id >= pair[1].student_id) + { + return Err("its students are not one per id, in order".into()); + } + if let Some(report) = students.iter().find(|r| r.grade.is_some()) { + return Err(format!( + "the evidence for '{}' carries a grade", + report.student_id + )); + } + let ids: BTreeSet<&str> = students.iter().map(|r| r.student_id.as_str()).collect(); + for (i, revision) in self.revisions.iter().enumerate() { + let n = revision.revision; + if usize::try_from(n).ok() != Some(i + 1) { + return Err("its revisions are not numbered from 1 in order".into()); + } + if revision.evidence != self.digest { + return Err(format!("revision {n} was scored from other evidence")); + } + if revision.checksum != revision_checksum(revision) { + return Err(format!("revision {n} failed its checksum")); + } + if revision + .grades + .keys() + .map(String::as_str) + .collect::>() + != ids + { + return Err(format!( + "revision {n} does not grade exactly the record's students" + )); + } + } + Ok(()) + } + + pub fn latest(&self) -> Option<&Revision> { + self.revisions.last() + } + + pub fn revision(&self, n: u32) -> Result<&Revision> { + usize::try_from(n) + .ok() + .and_then(|n| n.checked_sub(1)) + .and_then(|i| self.revisions.get(i)) + .with_context(|| match self.revisions.len() { + 0 => format!("there is no revision {n}: nothing has scored this evidence yet"), + len => format!("there is no revision {n}: the record holds revisions 1 to {len}"), + }) + } + + /// The reports as revision `n` scored them: the latest when `n` is `None`, and unscored + /// evidence when nothing has scored it yet. + pub fn view(&self, n: Option) -> Result { + let revision = match n { + Some(n) => Some(self.revision(n)?), + None => self.latest(), + }; + let mut reports = self.evidence.students.clone(); + if let Some(revision) = revision { + for report in &mut reports { + report.grade = revision.grades.get(&report.student_id).cloned(); + } + } + Ok(View { + revision: revision.map(|r| r.revision), + of: self.revisions.len(), + reports, + items: revision.map(|r| r.policy.items.clone()).unwrap_or_default(), + }) + } + + /// Score the evidence under `policy` as a new revision, and return its number. Runs + /// nothing. Refused when the latest revision already used this policy on this build: it + /// would only repeat it. + pub fn score(&mut self, items: &[GradingItem], policy: &Policy) -> Result { + let scoring = ScoringPolicy { + items: items.to_vec(), + grading: policy.config().clone(), + derived_items: policy.derived_items(), + }; + let build = env!("CARGO_PKG_VERSION"); + if let Some(latest) = self.latest() + && latest.policy == scoring + && latest.scriptmark == build + { + bail!( + "nothing to rescore: revision {} already scored this evidence under the same policy", + latest.revision + ); + } + let mut reports = self.evidence.students.clone(); + grading::grade_all(&mut reports, items, policy)?; + let grades = reports + .into_iter() + .map(|r| { + let grade = r.grade.expect("grade_all grades every report"); + (r.student_id, grade) + }) + .collect(); + let n = u32::try_from(self.revisions.len() + 1).context("too many revisions")?; + let mut revision = Revision { + revision: n, + scriptmark: build.to_string(), + evidence: self.digest.clone(), + policy: scoring, + grades, + checksum: String::new(), + }; + revision.checksum = revision_checksum(&revision); + self.revisions.push(revision); + Ok(n) + } + + /// Whether the evidence still describes the batch as it is now: the same assignment, + /// submissions, matching and tests. Only then may a new policy score it. Refuses with + /// every difference found. Reads files; runs nothing. + pub fn check(&self, current: &Current) -> Result<()> { + let evidence = &self.evidence; + let mut changed = Vec::new(); + let assignment = AssignmentId::from(current.assignment); + if assignment != evidence.assignment { + changed.push(format!( + "the assignment is {assignment}, not {}", + evidence.assignment + )); + } + if current.attempt_policy != evidence.attempt_policy { + changed.push(format!( + "the attempt policy is {}, not {}", + word(¤t.attempt_policy), + word(&evidence.attempt_policy) + )); + } + if *current.matching != evidence.matching { + changed.push("the [matching] rules changed".into()); + } + let specs = spec_versions(current.specs)?; + let same_specs = spec_changes(&evidence.bundle.specs, &specs, &mut changed); + student_changes(evidence, current, same_specs, &mut changed)?; + if changed.is_empty() { + return Ok(()); + } + let more = changed.len().saturating_sub(SHOWN); + changed.truncate(SHOWN); + if more > 0 { + changed.push(format!("… and {more} more")); + } + bail!( + "refusing to rescore: the evidence no longer describes this batch, so only grading it \ + again can:\n {}", + changed.join("\n ") + ) + } +} + +/// A batch as it stands now, found without running anything. +pub struct Current<'a> { + pub assignment: &'a Assignment, + pub attempt_policy: AttemptPolicy, + pub matching: &'a matching::Config, + pub specs: &'a [TestSpec], + pub students: &'a [StudentSubmission], +} + +/// Records the spec differences; whether the specs are the same ones, in the same order. +fn spec_changes(recorded: &[SpecVersion], now: &[SpecVersion], changed: &mut Vec) -> bool { + let names = |specs: &[SpecVersion]| specs.iter().map(|s| s.name.clone()).collect::>(); + if names(recorded) != names(now) { + changed.push(format!( + "the test specs are [{}], not [{}]", + names(now).join(", "), + names(recorded).join(", ") + )); + return false; + } + for (was, now) in recorded.iter().zip(now) { + if was.digest != now.digest { + changed.push(format!("test spec '{}' changed", now.name)); + } + let paths: BTreeSet<&String> = was.sources.keys().chain(now.sources.keys()).collect(); + for path in paths { + if was.sources.get(path) != now.sources.get(path) { + changed.push(format!( + "{path}, which test spec '{}' reads, changed", + now.name + )); + } + } + } + true +} + +fn student_changes( + evidence: &Evidence, + current: &Current, + same_specs: bool, + changed: &mut Vec, +) -> Result<()> { + let recorded: BTreeMap<&str, &StudentReport> = evidence + .students + .iter() + .map(|r| (r.student_id.as_str(), r)) + .collect(); + let now: BTreeMap = current + .students + .iter() + .map(|s| (s.key().to_string(), s)) + .collect(); + for id in recorded.keys().filter(|id| !now.contains_key(**id)) { + changed.push(format!("student {id} is no longer in the input")); + } + for (id, student) in &now { + let Some(report) = recorded.get(id.as_str()) else { + changed.push(format!("student {id} was not graded")); + continue; + }; + if report.canvas_user_id != student.identity.canvas_user_id { + changed.push(format!("{id}'s Canvas user id changed")); + } + if report.submission_state != student.outcome() { + changed.push(format!( + "{id}'s submission is now {}, not {}", + word(&student.outcome()), + word(&report.submission_state) + )); + } + if report.excused != student.is_excused() { + changed.push(format!( + "{id} is {}excused now", + if student.is_excused() { + "" + } else { + "no longer " + } + )); + } + if report.submission.as_ref() != Some(&submission_version(student)?) { + changed.push(format!("{id}'s submitted files changed")); + } + if same_specs { + for spec in current.specs { + let now = matching::item_match(current.matching, student, spec); + let was = report.matches.iter().find(|m| m.item == now.item); + if !was.is_some_and(|was| same_static_match(was, &now)) { + changed.push(format!( + "{id}'s file for '{}' is matched differently", + now.item + )); + } + } + } + } + Ok(()) +} + +/// The decisions made before anything ran. Function decisions are made while running and +/// are evidence, not something to compare against. +fn same_static_match(a: &ItemMatch, b: &ItemMatch) -> bool { + a.student == b.student + && a.item == b.item + && a.file == b.file + && a.origin == b.origin + && a.owner == b.owner +} + +/// Each spec's version as loaded, in load order. Reads files; runs nothing. +pub fn spec_versions(specs: &[TestSpec]) -> Result> { + specs + .iter() + .map(|spec| { + Ok(SpecVersion { + name: spec.meta.name.clone(), + digest: digest(&serde_json::to_vec(spec)?), + sources: sources(spec).map_err(anyhow::Error::msg)?, + seeds: BTreeMap::new(), + answers: None, + }) + }) + .collect() +} + +/// Every teacher file a spec reads, hashed: imports, data files, Python checkers and +/// reference implementations, and every other `.py` file beside a module, checker or +/// reference — the harness puts their directory on `sys.path`, so a helper there is +/// imported without being declared. +fn sources(spec: &TestSpec) -> Result, String> { + let mut files: BTreeSet = spec + .meta + .data_files + .iter() + .map(|p| spec.dir.join(p)) + .collect(); + let mut python: BTreeSet = spec.meta.imports.iter().map(PathBuf::from).collect(); + for case in spec + .cases + .iter() + .chain(spec.scenarios.iter().flat_map(|s| &s.steps)) + { + if let Some(Ok(Check::Python(script))) = case.check.as_ref().map(|c| c.resolve()) { + python.insert(script.into()); + } + for oracle in case + .oracle + .iter() + .chain(case.parametrize.iter().map(|p| &p.oracle)) + { + if let Some(reference) = &oracle.reference { + python.insert(reference.into()); + } + } + } + let dirs: BTreeSet<&Path> = python + .iter() + .map(|module| { + module + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or(Path::new(".")) + }) + .collect(); + for dir in dirs { + let failed = |e: std::io::Error| format!("cannot list '{}': {e}", dir.display()); + for entry in std::fs::read_dir(dir).map_err(failed)? { + let path = entry.map_err(failed)?.path(); + if path.extension().is_some_and(|e| e == "py") && path.is_file() { + files.insert(path); + } + } + } + files.extend(python); + let mut hashes = BTreeMap::new(); + for path in files { + fingerprint(&path, &mut hashes, &mut BTreeSet::new())?; + } + Ok(hashes) +} + +/// The bundle a batch ran: each spec's version with the seeds and answers it was prepared +/// with, and a link to the frozen file that holds them. +pub fn bundle_version( + mut specs: Vec, + timeout: u64, + python: String, + frozen: &Frozen, + frozen_path: Option, +) -> BundleVersion { + for spec in &mut specs { + if let Some(templates) = frozen.specs.get(&spec.name) { + spec.seeds = templates + .iter() + .filter_map(|(name, made)| made.seed.map(|seed| (name.clone(), seed))) + .collect(); + } + spec.answers = frozen.answers.get(&spec.name).map(|a| a.checksum.clone()); + } + let semantics: Vec<(&String, &String, &BTreeMap)> = specs + .iter() + .map(|s| (&s.name, &s.digest, &s.sources)) + .collect(); + BundleVersion { + digest: digest(&serde_json::to_vec(&(timeout, semantics)).expect("plain JSON")), + timeout, + python, + frozen: (!frozen.is_empty()).then(|| FrozenLink { + path: frozen_path, + digest: digest(frozen.to_json().as_bytes()), + }), + specs, + } +} + +/// What `student` hands in now: the selected attempt, the SHA-256 of each of its files, and +/// of the archives they came out of. +pub fn submission_version(student: &StudentSubmission) -> Result { + let hash = |path: &Path| -> Result { + let bytes = + std::fs::read(path).with_context(|| format!("cannot read {}", path.display()))?; + Ok(FileVersion { + path: path.to_path_buf(), + sha256: digest(&bytes), + }) + }; + let mut files = student + .files() + .iter() + .map(|f| hash(&f.path)) + .collect::>>()?; + files.sort(); + let archives: BTreeSet<&PathBuf> = student + .files() + .iter() + .filter_map(|f| match &f.origin { + FileOrigin::Archive { archive, .. } => Some(archive), + _ => None, + }) + .collect(); + let attempt = student.selected_attempt(); + Ok(SubmissionVersion { + attempt: attempt.map(|a| a.attempt), + submitted_at: attempt.and_then(|a| a.submitted_at.clone()), + files, + archives: archives + .into_iter() + .map(|p| hash(p)) + .collect::>()?, + }) +} + +/// Each student's submission version, in input order. +pub fn submission_versions(students: &[StudentSubmission]) -> Result> { + students.iter().map(submission_version).collect() +} + +/// Put on each report — `run_all` returns them in input order — the version its student's +/// submission had before the run, refusing when one changed meanwhile: its cases may have +/// judged two versions. +pub fn seal( + reports: &mut [StudentReport], + students: &[StudentSubmission], + before: Vec, +) -> Result<()> { + let after = submission_versions(students)?; + if reports.len() != students.len() || before.len() != students.len() { + bail!("the reports do not match the students one to one"); + } + for ((report, student), (was, now)) in reports + .iter_mut() + .zip(students) + .zip(before.into_iter().zip(after)) + { + if report.student_id != student.key().to_string() { + bail!( + "the report for '{}' is not in its student's place", + report.student_id + ); + } + if was != now { + bail!( + "{}'s submission changed while it was being graded; grade again", + report.student_id + ); + } + report.submission = Some(was); + } + Ok(()) +} + +/// Whether a run may write its record over `path`. A record holding rescored revisions — +/// grades kept nowhere else — is never replaced; anything else is, as before. +pub fn check_replaceable(path: &Path) -> Result<(), String> { + let Ok(text) = std::fs::read_to_string(path) else { + return Ok(()); + }; + match Record::from_json(&text) { + Ok(record) if record.revisions.len() > 1 => Err(format!( + "{} holds {} score revisions, which it alone records; move it aside, or write this \ + run to another file", + path.display(), + record.revisions.len() + )), + _ => Ok(()), + } +} + +/// How a grade stands, in a few words. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum GradeState { + Graded, + Withheld, +} + +/// A grade and the numbers behind it, for comparing two revisions. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct Standing { + pub state: GradeState, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reason: Option, + pub max: f64, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub score: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub raw_grade: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub final_grade: Option, +} + +impl Standing { + pub fn of(grade: &Grade) -> Self { + let (state, score, raw_grade) = match grade.outcome { + GradeOutcome::Graded { + score, raw_grade, .. + } => (GradeState::Graded, Some(score), Some(raw_grade)), + GradeOutcome::Withheld { .. } => (GradeState::Withheld, None, None), + }; + Self { + state, + reason: grade.reason(), + max: grade.max, + score, + raw_grade, + final_grade: grade.final_grade(), + } + } +} + +/// One student whose grade differs between two revisions. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct Change { + pub student_id: String, + /// `None` when there was no earlier revision. + pub before: Option, + pub after: Standing, +} + +/// The students whose grade `after` changed from `before`, by id. +pub fn diff(before: Option<&Revision>, after: &Revision) -> Vec { + after + .grades + .iter() + .filter_map(|(id, grade)| { + let now = Standing::of(grade); + let was = before.and_then(|b| b.grades.get(id)).map(Standing::of); + (was.as_ref() != Some(&now)).then(|| Change { + student_id: id.clone(), + before: was, + after: now, + }) + }) + .collect() +} + +fn evidence_digest(evidence: &Evidence) -> String { + digest(&serde_json::to_vec(evidence).expect("evidence is plain JSON")) +} + +fn revision_checksum(revision: &Revision) -> String { + digest( + &serde_json::to_vec(&( + revision.revision, + &revision.scriptmark, + &revision.evidence, + &revision.policy, + &revision.grades, + )) + .expect("a revision is plain JSON"), + ) +} + +fn word(value: &T) -> String { + crate::export::word(value) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::models::{CaseResult, Cause, Fault, SubmissionOutcome, TestResult, TestStatus}; + + /// A student with `passed` and `failed` cases on the one item, `sum`. + fn report(id: &str, passed: usize, failed: usize) -> StudentReport { + let case = |name: String, status: TestStatus| CaseResult { + case_name: name, + status, + fault: (status != TestStatus::Passed).then_some(Fault::Student), + cause: (status != TestStatus::Passed).then_some(Cause::Wrong), + ..Default::default() + }; + let cases = (0..passed) + .map(|i| case(format!("pass {i}"), TestStatus::Passed)) + .chain((0..failed).map(|i| case(format!("fail {i}"), TestStatus::Failed))) + .collect(); + StudentReport { + test_results: vec![TestResult { + item_id: "sum".into(), + file: None, + cases, + }], + ..StudentReport::new(id, SubmissionOutcome::Executable) + } + } + + fn evidence(students: Vec) -> Evidence { + let frozen = Frozen { + format: crate::runner::frozen::FORMAT, + scriptmark: "test".into(), + specs: BTreeMap::new(), + answers: BTreeMap::new(), + }; + Evidence { + scriptmark: "test".into(), + assignment: AssignmentId { + name: "hw".into(), + canvas_course_id: None, + canvas_assignment_id: None, + }, + inputs: Inputs { + tests: "/hw/tests".into(), + assignment: None, + source: Source::Local { + dirs: vec!["/hw/submissions".into()], + roster: None, + }, + }, + attempt_policy: AttemptPolicy::default(), + matching: matching::Config::default(), + bundle: bundle_version(Vec::new(), 10, "python3".into(), &frozen, None), + students, + } + } + + fn record() -> Record { + Record::new(evidence(vec![report("bob", 1, 1), report("alice", 2, 0)])).unwrap() + } + + /// One item, `sum`, worth `points`. + fn policy(points: u32) -> (Vec, Policy) { + ( + vec![GradingItem { + points, + ..GradingItem::new("sum") + }], + Policy::compile(GradingConfig::default(), false).unwrap(), + ) + } + + fn rescored(record: &mut Record, points: u32) -> u32 { + let (items, policy) = policy(points); + record.score(&items, &policy).unwrap() + } + + #[test] + fn a_record_round_trips_with_every_revision() { + let mut record = record(); + assert_eq!(rescored(&mut record, 10), 1); + assert_eq!(rescored(&mut record, 20), 2); + let read = Record::from_json(&record.to_json()).unwrap(); + assert_eq!(read.digest, record.digest); + assert_eq!(read.revisions, record.revisions); + assert_eq!(read.to_json(), record.to_json()); + } + + #[test] + fn students_are_kept_in_id_order_and_never_twice() { + assert_eq!( + record() + .evidence + .students + .iter() + .map(|r| r.student_id.as_str()) + .collect::>(), + ["alice", "bob"] + ); + let err = + Record::new(evidence(vec![report("bob", 1, 0), report("bob", 0, 1)])).unwrap_err(); + assert!(err.to_string().contains("share student id 'bob'"), "{err}"); + } + + #[test] + fn evidence_carrying_a_grade_is_not_evidence() { + let mut record = record(); + rescored(&mut record, 10); + let graded = record.view(None).unwrap().reports; + let err = Record::new(evidence(graded)).unwrap_err(); + assert!(err.to_string().contains("already carries a grade"), "{err}"); + } + + #[test] + fn an_edited_record_is_refused() { + let mut record = record(); + rescored(&mut record, 10); + let edit = |change: &dyn Fn(&mut Value)| { + let mut value: Value = serde_json::from_str(&record.to_json()).unwrap(); + change(&mut value); + Record::from_json(&value.to_string()).unwrap_err() + }; + + let err = edit(&|v| { + v["evidence"]["students"][1]["test_results"][0]["cases"][1]["status"] = "passed".into() + }); + assert!(err.contains("does not match its digest"), "{err}"); + + let err = edit(&|v| v["revisions"][0]["grades"]["bob"]["final_grade"] = 100.0.into()); + assert!(err.contains("revision 1 failed its checksum"), "{err}"); + + let err = edit(&|v| v["revisions"][0]["revision"] = 2.into()); + assert!(err.contains("not numbered from 1"), "{err}"); + + let err = edit(&|v| { + v["evidence"]["students"][0]["grade"] = v["revisions"][0]["grades"]["alice"].clone() + }); + assert!(err.contains("does not match its digest"), "{err}"); + } + + #[test] + fn results_from_before_grading_records_and_newer_records_are_refused_by_what_they_are() { + let err = Record::from_json("[]").unwrap_err(); + assert!(err.contains("before grading records"), "{err}"); + + let mut value: Value = serde_json::from_str(&record().to_json()).unwrap(); + value["format"] = 2.into(); + value["something_new"] = true.into(); + let err = Record::from_json(&value.to_string()).unwrap_err(); + assert!(err.contains("it is format 2"), "{err}"); + + let err = Record::from_json("{}").unwrap_err(); + assert!(err.contains("no format"), "{err}"); + } + + #[test] + fn rescoring_appends_and_refuses_to_repeat_the_latest_revision() { + let mut record = record(); + rescored(&mut record, 10); + let (items, policy) = policy(10); + let err = record.score(&items, &policy).unwrap_err(); + assert!( + err.to_string() + .contains("revision 1 already scored this evidence"), + "{err}" + ); + rescored(&mut record, 20); + // Going back to an earlier policy is a new decision, and a new revision. + assert_eq!(rescored(&mut record, 10), 3); + assert_eq!(record.revisions.len(), 3); + assert!(record.revisions.iter().all(|r| r.evidence == record.digest)); + } + + #[test] + fn a_view_shows_one_revision_and_unscored_evidence_before_any() { + let mut record = record(); + let unscored = record.view(None).unwrap(); + assert_eq!((unscored.revision, unscored.of), (None, 0)); + assert!(unscored.reports.iter().all(|r| r.grade.is_none())); + assert!(unscored.items.is_empty()); + let err = record.view(Some(1)).unwrap_err(); + assert!(err.to_string().contains("nothing has scored"), "{err}"); + + rescored(&mut record, 10); + rescored(&mut record, 20); + let grade = |view: &View, id: &str| { + view.reports + .iter() + .find(|r| r.student_id == id) + .unwrap() + .final_grade() + }; + let first = record.view(Some(1)).unwrap(); + let latest = record.view(None).unwrap(); + assert_eq!( + (first.revision, latest.revision, latest.of), + (Some(1), Some(2), 2) + ); + assert_eq!(first.items[0].points, 10); + assert_eq!(latest.items[0].points, 20); + assert_eq!(grade(&first, "bob"), Some(50.0)); + assert_eq!(grade(&latest, "bob"), Some(50.0)); + let err = record.view(Some(3)).unwrap_err(); + assert!(err.to_string().contains("revisions 1 to 2"), "{err}"); + } + + #[test] + fn a_diff_names_every_student_whose_grade_moved() { + let mut record = record(); + rescored(&mut record, 10); + let first = diff(None, &record.revisions[0]); + assert_eq!(first.len(), 2, "everyone is new in the first revision"); + assert!(first.iter().all(|c| c.before.is_none())); + + // Points alone do not move a proportional grade, but they move the score. + rescored(&mut record, 20); + let changes = diff(Some(&record.revisions[0]), &record.revisions[1]); + assert_eq!(changes.len(), 2); + let bob = changes.iter().find(|c| c.student_id == "bob").unwrap(); + assert_eq!(bob.before.as_ref().unwrap().score, Some(5.0)); + assert_eq!(bob.after.score, Some(10.0)); + assert_eq!(bob.after.final_grade, Some(50.0)); + + let (items, _) = policy(20); + let all_or_nothing = Policy::compile(GradingConfig::default(), false).unwrap(); + let items: Vec = items + .into_iter() + .map(|i| GradingItem { + aggregation: crate::models::Aggregation::AllOrNothing, + ..i + }) + .collect(); + record.score(&items, &all_or_nothing).unwrap(); + let changes = diff(Some(&record.revisions[1]), &record.revisions[2]); + assert_eq!( + changes + .iter() + .map(|c| (c.student_id.as_str(), c.after.final_grade)) + .collect::>(), + [("bob", Some(0.0))], + "alice passed everything either way" + ); + } + + #[test] + fn a_record_with_rescored_revisions_is_never_replaced() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("results.json"); + assert!(check_replaceable(&path).is_ok(), "nothing there"); + std::fs::write(&path, "not a record").unwrap(); + assert!(check_replaceable(&path).is_ok()); + + let mut record = record(); + record.write(&path).unwrap(); + assert!(check_replaceable(&path).is_ok(), "unscored"); + rescored(&mut record, 10); + record.write(&path).unwrap(); + assert!(check_replaceable(&path).is_ok(), "only its own first grade"); + rescored(&mut record, 20); + record.write(&path).unwrap(); + let err = check_replaceable(&path).unwrap_err(); + assert!(err.contains("holds 2 score revisions"), "{err}"); + } + + #[test] + fn sources_cover_references_checkers_data_and_undeclared_helpers() { + let dir = tempfile::tempdir().unwrap(); + let at = |name: &str, content: &str| { + let path = dir.path().join(name); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(path, content).unwrap(); + }; + at("teacher/support.py", "from helper import X\n"); + at("teacher/helper.py", "X = 1\n"); + at("teacher/notes.txt", "not python"); + at("solutions/ref.py", "def answer(x):\n return x\n"); + at("check.py", "print('{}')\n"); + at("data/poem.txt", "words\n"); + let spec = crate::spec_loader::load_spec_str( + "[meta]\nname = 'q'\nfile = 'q.py'\nfunction = 'f'\nlanguage = 'python'\n\ + imports = ['teacher/support.py']\ndata_files = ['data/']\n\ + [[cases]]\nname = 'one'\nargs = [1]\nexpect = 1\ncheck = { python = 'check.py' }\n\ + [[cases]]\nname = 'many'\n[cases.parametrize]\nargs = { x = 'int(0, 3)' }\n\ + samples = [[1]]\n[cases.parametrize.oracle]\nreference = 'solutions/ref.py'\n\ + function = 'answer'\n", + dir.path(), + ) + .unwrap(); + let hashed: Vec = sources(&spec) + .unwrap() + .into_keys() + .map(|p| { + p.strip_prefix(&*dir.path().to_string_lossy()) + .unwrap() + .to_string() + }) + .collect(); + for expected in [ + "/teacher/support.py", + "/teacher/helper.py", + "/solutions/ref.py", + "/check.py", + "/data/poem.txt", + ] { + assert!( + hashed.iter().any(|p| p == expected), + "{expected} not in {hashed:?}" + ); + } + assert!( + !hashed.iter().any(|p| p.ends_with("notes.txt")), + "{hashed:?}" + ); + } +} diff --git a/crates/scriptmark/src/runner/answers.rs b/crates/scriptmark/src/runner/answers.rs index 0f63151..9ec564c 100644 --- a/crates/scriptmark/src/runner/answers.rs +++ b/crates/scriptmark/src/runner/answers.rs @@ -109,7 +109,7 @@ impl Contract { } } -fn fingerprint( +pub(crate) fn fingerprint( path: &Path, files: &mut BTreeMap, parents: &mut BTreeSet, diff --git a/crates/scriptmark/src/runner/frozen.rs b/crates/scriptmark/src/runner/frozen.rs index 555b3d4..6ffa527 100644 --- a/crates/scriptmark/src/runner/frozen.rs +++ b/crates/scriptmark/src/runner/frozen.rs @@ -142,7 +142,7 @@ impl Frozen { /// A temporary file beside `path`, its directory made first. Its mode is left to the umask, /// as `std::fs::write` leaves it, rather than tempfile's owner-only default. -fn scratch(path: &Path) -> std::io::Result { +pub(crate) fn scratch(path: &Path) -> std::io::Result { let dir = path .parent() .filter(|p| !p.as_os_str().is_empty()) diff --git a/crates/scriptmark/src/tui/ui.rs b/crates/scriptmark/src/tui/ui.rs index 9f6b7f2..5c9b612 100644 --- a/crates/scriptmark/src/tui/ui.rs +++ b/crates/scriptmark/src/tui/ui.rs @@ -32,8 +32,10 @@ fn draw_header(f: &mut Frame, area: Rect, app: &App) { .and_then(|i| app.sessions.get(i)) .map(|s| { format!( - " | {} | {} students | avg {}", + " | {} | revision {} of evidence {} | {} students | avg {}", s.assignment, + s.revision, + evidence_short(&s.evidence), s.student_count, s.avg_grade .map(|a| format!("{a:.1}")) @@ -257,13 +259,21 @@ fn draw_detail(f: &mut Frame, area: Rect, app: &App) { } fn draw_sessions(f: &mut Frame, area: Rect, app: &App) { - let header = Row::new(vec!["ID", "Assignment", "Students", "Avg Grade", "Date"]) - .style( - Style::default() - .fg(Color::DarkGray) - .add_modifier(Modifier::BOLD), - ) - .bottom_margin(1); + let header = Row::new(vec![ + "ID", + "Assignment", + "Revision", + "Evidence", + "Students", + "Avg Grade", + "Date", + ]) + .style( + Style::default() + .fg(Color::DarkGray) + .add_modifier(Modifier::BOLD), + ) + .bottom_margin(1); let rows: Vec = app .sessions @@ -278,6 +288,8 @@ fn draw_sessions(f: &mut Frame, area: Rect, app: &App) { Row::new(vec![ Cell::from(s.id.to_string()).style(Style::default().fg(Color::Cyan)), Cell::from(s.assignment.as_str()), + Cell::from(s.revision.to_string()), + Cell::from(evidence_short(&s.evidence)).style(Style::default().fg(Color::DarkGray)), Cell::from(s.student_count.to_string()), Cell::from( s.avg_grade @@ -295,6 +307,8 @@ fn draw_sessions(f: &mut Frame, area: Rect, app: &App) { [ Constraint::Min(4), Constraint::Min(20), + Constraint::Min(8), + Constraint::Min(12), Constraint::Min(10), Constraint::Min(10), Constraint::Min(20), @@ -311,6 +325,11 @@ fn draw_sessions(f: &mut Frame, area: Rect, app: &App) { f.render_widget(table, area); } +/// Enough of an evidence digest to tell two apart at a glance. +fn evidence_short(evidence: &str) -> &str { + &evidence[..evidence.len().min(12)] +} + fn draw_similarity(f: &mut Frame, area: Rect, app: &App) { let header = Row::new(vec![ "Score", diff --git a/crates/scriptmark/tests/examples.rs b/crates/scriptmark/tests/examples.rs index d29bafe..32a1a42 100644 --- a/crates/scriptmark/tests/examples.rs +++ b/crates/scriptmark/tests/examples.rs @@ -3,7 +3,7 @@ //! reference for the three fixture kinds P-674 names — a pure function, a shared object, //! and files read and written — for generated cases (P-675), and for scoring them: //! declared points, all or nothing under a curve, and items derived when nothing is -//! declared. +//! declared — and for rescoring the evidence under another policy (P-678). use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -246,6 +246,74 @@ async fn test_the_number_of_draws_never_changes_an_items_points() { } } +/// The rescoring example, run as its comments say: graded, then rescored under +/// `regrade.toml` with no interpreter on PATH, so nothing can have run. +#[test] +fn test_rescoring_example() { + let source = examples().join("bundles/rescoring"); + let dir = tempfile::tempdir().unwrap(); + for name in ["assignment.toml", "regrade.toml", "tests", "submissions"] { + copy(&source.join(name), &dir.path().join(name)); + } + let scriptmark = |args: &[&str], path: Option<&str>| { + let mut command = std::process::Command::new(env!("CARGO_BIN_EXE_scriptmark")); + command.current_dir(dir.path()).args(args); + if let Some(path) = path { + command.env("PATH", path); + } + let output = command.output().unwrap(); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + }; + scriptmark( + &[ + "grade", + "submissions", + "-t", + "tests", + "-o", + "out/results.json", + ], + None, + ); + scriptmark( + &[ + "rescore", + "out/results.json", + "--assignment", + "regrade.toml", + ], + Some(""), + ); + + let record = scriptmark::record::Record::load(&dir.path().join("out/results.json")).unwrap(); + let grades = |revision: u32| { + let reports = record.view(Some(revision)).unwrap().reports; + ( + graded(student(&reports, "alice")).2, + graded(student(&reports, "bob")).2, + ) + }; + // bob counts one of four word cases right: 1.5 of 6 points, then 0 of 4 all or nothing. + assert_eq!(grades(1), (100.0, 55.0)); + assert_eq!(grades(2), (100.0, 60.0)); +} + +fn copy(from: &Path, to: &Path) { + if from.is_dir() { + std::fs::create_dir_all(to).unwrap(); + for entry in std::fs::read_dir(from).unwrap() { + let entry = entry.unwrap(); + copy(&entry.path(), &to.join(entry.file_name())); + } + } else { + std::fs::copy(from, to).unwrap(); + } +} + #[test] fn test_the_standalone_example_spec_still_loads() { load_spec(&examples().join("python/test_larger_number.toml")).unwrap_or_else(|e| panic!("{e}")); diff --git a/crates/scriptmark/tests/input_equivalence.rs b/crates/scriptmark/tests/input_equivalence.rs index 26d2073..e9c83da 100644 --- a/crates/scriptmark/tests/input_equivalence.rs +++ b/crates/scriptmark/tests/input_equivalence.rs @@ -343,4 +343,7 @@ fn test_results_written_before_per_item_grading_are_refused() { // they are not read as if they were today's results. let raw = std::fs::read_to_string(fixture_root().join("legacy_results.json")).unwrap(); assert!(serde_json::from_str::>(&raw).is_err()); + // Nor is it a grading record: a list of reports is refused by what it is. + let err = scriptmark::record::Record::from_json(&raw).unwrap_err(); + assert!(err.contains("before grading records"), "{err}"); } diff --git a/crates/scriptmark/tests/matching.rs b/crates/scriptmark/tests/matching.rs index f7724f1..2e677cd 100644 --- a/crates/scriptmark/tests/matching.rs +++ b/crates/scriptmark/tests/matching.rs @@ -7,6 +7,7 @@ use std::process::{Command, Output}; use scriptmark::discovery::{LocalInputOptions, load_local_input}; use scriptmark::matching::{Config, OwnerOverride, State}; use scriptmark::models::{Cause, StudentReport}; +use scriptmark::record::Record; const SPEC: &str = r#" [meta] @@ -75,8 +76,11 @@ impl Bench { fn grades(&self) -> Vec { self.successful("grade"); - serde_json::from_slice(&std::fs::read(self.path().join("out/result.json")).unwrap()) + Record::load(&self.path().join("out/result.json")) + .unwrap() + .view(None) .unwrap() + .reports } } diff --git a/crates/scriptmark/tests/record.rs b/crates/scriptmark/tests/record.rs new file mode 100644 index 0000000..7e694bc --- /dev/null +++ b/crates/scriptmark/tests/record.rs @@ -0,0 +1,531 @@ +//! P-678: the grading record. `grade` and `run` write one; every consumer reads a revision of +//! it; `rescore` adds a revision from the saved evidence without running anything, and +//! refuses evidence whose tests, submissions or matching have changed. + +use std::path::Path; +use std::process::{Command, Output}; + +use scriptmark::record::Record; + +const SPEC: &str = r#" +[meta] +name = "sum" +file = "sum.py" +function = "add" +language = "python" +imports = ["teacher/support.py"] + +[[cases]] +name = "small" +args = [1, 2] +expect = 3 +check = { function = "same" } + +[[cases]] +name = "negative" +args = [-1, -2] +expect = -3 +"#; + +const ASSIGNMENT: &str = r#" +[assignment] +name = "sums" + +[[items]] +id = "sum" +points = 10 +aggregation = "proportional" +"#; + +/// alice is right; bob adds absolute values, so he passes one case of two. +fn bench() -> tempfile::TempDir { + let dir = tempfile::tempdir().unwrap(); + for (name, content) in [ + ("tests/test_sum.toml", SPEC), + // The checker's helper is imported, not declared: it still decides the grade. + ( + "tests/teacher/support.py", + "from helper import equal\n\ndef same(result, expected):\n return equal(result, expected)\n", + ), + ( + "tests/teacher/helper.py", + "def equal(a, b):\n return a == b\n", + ), + ("assignment.toml", ASSIGNMENT), + ( + "submissions/alice_sum.py", + "def add(a, b):\n return a + b\n", + ), + ( + "submissions/bob_sum.py", + "def add(a, b):\n return abs(a) + abs(b)\n", + ), + ] { + write(dir.path(), name, content); + } + dir +} + +fn write(dir: &Path, name: &str, content: &str) { + let path = dir.join(name); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(path, content).unwrap(); +} + +fn scriptmark(dir: &Path, args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_scriptmark")) + .current_dir(dir) + .args(args) + .output() + .unwrap() +} + +/// `rescore` with no interpreter anywhere on PATH: it must not need one. +fn rescore(dir: &Path, args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_scriptmark")) + .current_dir(dir) + .env("PATH", "") + .args(["rescore", "out/results.json"]) + .args(args) + .output() + .unwrap() +} + +fn text(output: &Output) -> String { + format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ) +} + +fn succeeded(output: Output) -> Output { + assert!(output.status.success(), "{}", text(&output)); + output +} + +fn refused(output: Output, why: &str) { + assert!( + !output.status.success(), + "expected a refusal: {}", + text(&output) + ); + assert!( + text(&output).contains(why), + "no '{why}' in:\n{}", + text(&output) + ); +} + +const GRADE: [&str; 6] = [ + "grade", + "submissions", + "-t", + "tests", + "-o", + "out/results.json", +]; + +fn record(dir: &Path) -> Record { + Record::load(&dir.join("out/results.json")).unwrap_or_else(|e| panic!("{e}")) +} + +/// Each student's final grade as revision `n` scored it. +fn grades(dir: &Path, n: Option) -> Vec<(String, Option)> { + record(dir) + .view(n) + .unwrap() + .reports + .iter() + .map(|r| (r.student_id.clone(), r.final_grade())) + .collect() +} + +fn graded(alice: f64, bob: f64) -> Vec<(String, Option)> { + vec![ + ("local:alice".into(), Some(alice)), + ("local:bob".into(), Some(bob)), + ] +} + +fn all_or_nothing(dir: &Path) { + write( + dir, + "assignment.toml", + &ASSIGNMENT.replace("proportional", "all_or_nothing"), + ); +} + +#[test] +fn grade_writes_a_record_whose_first_revision_every_consumer_reads() { + let temp = bench(); + let dir = temp.path(); + succeeded(scriptmark(dir, &GRADE)); + + let record = record(dir); + assert_eq!(record.revisions.len(), 1); + assert_eq!(grades(dir, None), graded(100.0, 50.0)); + let evidence = &record.evidence; + assert_eq!(evidence.assignment.name, "sums"); + assert!(evidence.inputs.tests.is_absolute()); + let sources = &evidence.bundle.specs[0].sources; + assert!( + sources.keys().any(|p| p.ends_with("teacher/helper.py")), + "{sources:?}" + ); + for student in &evidence.students { + let submission = student.submission.as_ref().expect("fingerprinted"); + assert_eq!(submission.attempt, Some(1)); + assert_eq!(submission.files.len(), 1); + assert!(submission.files[0].path.is_absolute()); + assert_eq!(submission.files[0].sha256.len(), 64); + } + + let summary = succeeded(scriptmark(dir, &["summarize", "out/results.json"])); + assert!( + text(&summary).contains("(revision 1 of 1)"), + "{}", + text(&summary) + ); + succeeded(scriptmark( + dir, + &["export", "out/results.json", "-o", "out/grades.csv"], + )); + let csv = std::fs::read_to_string(dir.join("out/grades.csv")).unwrap(); + assert!(csv.contains("local:bob,,,graded,,5,10,50,50,"), "{csv}"); + succeeded(scriptmark( + dir, + &["report", "out/results.json", "-o", "out/report.html"], + )); + assert!(dir.join("out/report.html").exists()); +} + +#[test] +fn rescoring_runs_nothing_and_keeps_the_earlier_revision() { + let temp = bench(); + let dir = temp.path(); + succeeded(scriptmark(dir, &GRADE)); + let before = record(dir); + + all_or_nothing(dir); + let output = succeeded(rescore(dir, &[])); + // The count is coloured; the words around it are not. + for words in ["revision 2 against revision 1: ", " of 2 students changed"] { + assert!(text(&output).contains(words), "{}", text(&output)); + } + let after = record(dir); + let changes = scriptmark::record::diff(Some(&after.revisions[0]), &after.revisions[1]); + assert_eq!( + changes + .iter() + .map(|c| c.student_id.as_str()) + .collect::>(), + ["local:bob"] + ); + + assert_eq!(after.digest, before.digest, "the evidence is untouched"); + assert_eq!(after.revisions[0], before.revisions[0]); + assert_eq!(grades(dir, Some(1)), graded(100.0, 50.0)); + assert_eq!(grades(dir, None), graded(100.0, 0.0)); + + let summary = succeeded(scriptmark( + dir, + &["summarize", "out/results.json", "--revision", "1"], + )); + assert!( + text(&summary).contains("(revision 1 of 2)"), + "{}", + text(&summary) + ); + succeeded(scriptmark( + dir, + &[ + "export", + "out/results.json", + "--revision", + "2", + "-o", + "out/grades.csv", + ], + )); + let csv = std::fs::read_to_string(dir.join("out/grades.csv")).unwrap(); + assert!(csv.contains("local:bob,,,graded,,0,10,0,0,"), "{csv}"); + + refused(rescore(dir, &[]), "nothing to rescore: revision 2"); + refused( + scriptmark(dir, &["summarize", "out/results.json", "--revision", "3"]), + "there is no revision 3", + ); +} + +#[test] +fn rescoring_refuses_evidence_whose_tests_submissions_or_matching_changed() { + type Change = fn(&Path); + let changes: [(&str, Change, &str); 7] = [ + ( + "spec", + |dir| { + let spec = std::fs::read_to_string(dir.join("tests/test_sum.toml")).unwrap(); + write( + dir, + "tests/test_sum.toml", + &spec.replace("expect = -3", "expect = 3"), + ); + }, + "test spec 'sum' changed", + ), + ( + "undeclared helper", + |dir| { + write( + dir, + "tests/teacher/helper.py", + "def equal(a, b):\n return True\n", + ) + }, + "teacher/helper.py, which test spec 'sum' reads, changed", + ), + ( + "student file", + |dir| { + write( + dir, + "submissions/bob_sum.py", + "def add(a, b):\n return a + b\n", + ) + }, + "local:bob's submitted files changed", + ), + ( + "new student", + |dir| { + write( + dir, + "submissions/carol_sum.py", + "def add(a, b):\n return a + b\n", + ) + }, + "student local:carol was not graded", + ), + ( + "removed student", + |dir| std::fs::remove_file(dir.join("submissions/bob_sum.py")).unwrap(), + "student local:bob is no longer in the input", + ), + ( + "matching", + |dir| { + let assignment = std::fs::read_to_string(dir.join("assignment.toml")).unwrap(); + write( + dir, + "assignment.toml", + &(assignment + + "[[matching.items]]\nid = 'sum'\nfiles = ['sum.py', 'add.py']\n"), + ); + }, + "the [matching] rules changed", + ), + ( + "assignment", + |dir| { + let assignment = std::fs::read_to_string(dir.join("assignment.toml")).unwrap(); + write( + dir, + "assignment.toml", + &assignment.replace("name = \"sums\"", "name = \"sums, again\""), + ); + }, + "the assignment is 'sums, again', not 'sums'", + ), + ]; + for (what, change, why) in changes { + let temp = bench(); + let dir = temp.path(); + succeeded(scriptmark(dir, &GRADE)); + let saved = std::fs::read(dir.join("out/results.json")).unwrap(); + all_or_nothing(dir); + change(dir); + let output = rescore(dir, &[]); + assert!(!output.status.success(), "{what}: {}", text(&output)); + assert!( + text(&output).contains(why), + "{what}: no '{why}' in\n{}", + text(&output) + ); + assert_eq!( + std::fs::read(dir.join("out/results.json")).unwrap(), + saved, + "{what}: a refused rescore writes nothing" + ); + } +} + +#[test] +fn a_run_is_scored_by_its_first_rescore() { + let temp = bench(); + let dir = temp.path(); + succeeded(scriptmark( + dir, + &[ + "run", + "submissions", + "-t", + "tests", + "-o", + "out/results.json", + ], + )); + assert!(record(dir).revisions.is_empty()); + let summary = succeeded(scriptmark(dir, &["summarize", "out/results.json"])); + assert!(text(&summary).contains("(unscored)"), "{}", text(&summary)); + refused( + scriptmark(dir, &["export", "out/results.json"]), + "has no score revision yet", + ); + + let output = succeeded(rescore(dir, &[])); + assert!( + text(&output).contains("revision 1 is the first score"), + "{}", + text(&output) + ); + assert_eq!(grades(dir, None), graded(100.0, 50.0)); +} + +#[test] +fn a_record_with_rescored_revisions_is_never_replaced() { + let temp = bench(); + let dir = temp.path(); + succeeded(scriptmark(dir, &GRADE)); + // One revision is the grade's own: grading again replaces it, as it always has. + succeeded(scriptmark(dir, &GRADE)); + all_or_nothing(dir); + succeeded(rescore(dir, &[])); + let saved = std::fs::read(dir.join("out/results.json")).unwrap(); + + refused(scriptmark(dir, &GRADE), "holds 2 score revisions"); + refused( + scriptmark( + dir, + &[ + "match", + "submissions", + "-t", + "tests", + "-o", + "out/results.json", + ], + ), + "holds 2 score revisions", + ); + assert_eq!(std::fs::read(dir.join("out/results.json")).unwrap(), saved); + succeeded(scriptmark( + dir, + &[ + "grade", + "submissions", + "-t", + "tests", + "-o", + "out/again.json", + ], + )); +} + +#[test] +fn grades_push_names_its_revision_and_keeps_to_the_records_assignment() { + let temp = bench(); + let dir = temp.path(); + let with_canvas = ASSIGNMENT.replace( + "name = \"sums\"", + "name = \"sums\"\ncanvas_course_id = 7\ncanvas_assignment_id = 8", + ); + write(dir, "assignment.toml", &with_canvas); + succeeded(scriptmark(dir, &GRADE)); + write( + dir, + "assignment.toml", + &with_canvas.replace("proportional", "all_or_nothing"), + ); + succeeded(rescore(dir, &[])); + + // Each refusal comes before Canvas is contacted, or any token is needed. + let push = |extra: &[&str]| { + let mut args = vec![ + "grades-push", + "--canvas-url", + "http://127.0.0.1:9", + "--course-id", + "7", + "out/results.json", + ]; + args.extend_from_slice(extra); + Command::new(env!("CARGO_BIN_EXE_scriptmark")) + .current_dir(dir) + .env_remove("CANVAS_TOKEN") + .args(args) + .output() + .unwrap() + }; + refused( + push(&["--assignment-id", "8"]), + "name the one to push with --revision", + ); + refused( + push(&["--assignment-id", "9", "--revision", "2"]), + "was graded for Canvas assignment 8, not 9", + ); + refused( + push(&["--assignment-id", "8", "--revision", "3"]), + "there is no revision 3", + ); +} + +#[test] +fn database_sessions_are_revisions_of_one_evidence() { + let temp = bench(); + let dir = temp.path(); + let mut args = GRADE.to_vec(); + args.extend(["--db", "grades.db"]); + succeeded(scriptmark(dir, &args)); + all_or_nothing(dir); + succeeded(rescore(dir, &["--db", "grades.db"])); + + let digest = record(dir).digest; + let db = scriptmark::db::Database::open(&dir.join("grades.db")).unwrap(); + let sessions = db.list_sessions().unwrap(); + assert_eq!( + sessions + .iter() + .map(|s| (s.revision, s.evidence.as_str())) + .collect::>(), + [(2, digest.as_str()), (1, digest.as_str())] + ); + let bob = |session: i64| { + db.get_results(session) + .unwrap() + .into_iter() + .find(|r| r.student_id == "local:bob") + .unwrap() + .final_grade + }; + assert_eq!(bob(sessions[0].id), Some(0.0)); + assert_eq!(bob(sessions[1].id), Some(50.0)); + let listed = succeeded(scriptmark(dir, &["db", "sessions", "--db", "grades.db"])); + assert!(text(&listed).contains(&digest[..12]), "{}", text(&listed)); +} + +#[test] +fn results_from_before_grading_records_are_refused_by_every_reader() { + let temp = bench(); + let dir = temp.path(); + write(dir, "out/results.json", "[]"); + for args in [ + &["summarize", "out/results.json"][..], + &["export", "out/results.json"], + &["report", "out/results.json"], + &["rescore", "out/results.json"], + ] { + refused(scriptmark(dir, args), "before grading records"); + } +} diff --git a/examples/bundles/rescoring/assignment.toml b/examples/bundles/rescoring/assignment.toml new file mode 100644 index 0000000..7b73a44 --- /dev/null +++ b/examples/bundles/rescoring/assignment.toml @@ -0,0 +1,20 @@ +# Grade once, then change the policy and score the same evidence again: +# +# scriptmark grade submissions -t tests -o out/results.json +# scriptmark rescore out/results.json --assignment regrade.toml +# +# The rescore runs nothing. It adds revision 2 to out/results.json, keeps revision 1, and +# prints whose grade changed. Editing a spec, a teacher file or a submission first makes it +# refuse: that evidence no longer describes the batch. +[assignment] +name = "text" + +[[items]] +id = "words" +points = 6 +aggregation = "proportional" + +[[items]] +id = "title" +points = 4 +aggregation = "proportional" diff --git a/examples/bundles/rescoring/regrade.toml b/examples/bundles/rescoring/regrade.toml new file mode 100644 index 0000000..4a9e260 --- /dev/null +++ b/examples/bundles/rescoring/regrade.toml @@ -0,0 +1,15 @@ +# The same assignment under another policy: counting words is now all or nothing, and the +# title case is worth more. Only [[items]] and [grading] may differ from assignment.toml; +# a changed name, attempt policy or [matching] rule is a different batch, graded afresh. +[assignment] +name = "text" + +[[items]] +id = "words" +points = 4 +aggregation = "all_or_nothing" + +[[items]] +id = "title" +points = 6 +aggregation = "proportional" diff --git a/examples/bundles/rescoring/submissions/alice_text.py b/examples/bundles/rescoring/submissions/alice_text.py new file mode 100644 index 0000000..0863e0b --- /dev/null +++ b/examples/bundles/rescoring/submissions/alice_text.py @@ -0,0 +1,6 @@ +def count_words(text): + return len(text.split()) + + +def title_case(text): + return text.title() diff --git a/examples/bundles/rescoring/submissions/bob_text.py b/examples/bundles/rescoring/submissions/bob_text.py new file mode 100644 index 0000000..e81438a --- /dev/null +++ b/examples/bundles/rescoring/submissions/bob_text.py @@ -0,0 +1,7 @@ +def count_words(text): + # Splits on single spaces only: extra spaces, newlines and "" all miscount. + return len(text.split(" ")) + + +def title_case(text): + return text.title() diff --git a/examples/bundles/rescoring/tests/test_title.toml b/examples/bundles/rescoring/tests/test_title.toml new file mode 100644 index 0000000..f213f5e --- /dev/null +++ b/examples/bundles/rescoring/tests/test_title.toml @@ -0,0 +1,17 @@ +# Title case: every word capitalised. + +[meta] +name = "title" +file = "text.py" +function = "title_case" +language = "python" + +[[cases]] +name = "two words" +args = ["hello world"] +expect = "Hello World" + +[[cases]] +name = "already titled" +args = ["Hello"] +expect = "Hello" diff --git a/examples/bundles/rescoring/tests/test_words.toml b/examples/bundles/rescoring/tests/test_words.toml new file mode 100644 index 0000000..3059837 --- /dev/null +++ b/examples/bundles/rescoring/tests/test_words.toml @@ -0,0 +1,27 @@ +# Counting words: whitespace of any kind and length separates them. + +[meta] +name = "words" +file = "text.py" +function = "count_words" +language = "python" + +[[cases]] +name = "two words" +args = ["hello world"] +expect = 2 + +[[cases]] +name = "extra spaces" +args = [" hello world "] +expect = 2 + +[[cases]] +name = "empty" +args = [""] +expect = 0 + +[[cases]] +name = "lines" +args = ["one\ntwo\nthree"] +expect = 3 diff --git a/python/scriptmark/__init__.py b/python/scriptmark/__init__.py index d288605..a188e0d 100644 --- a/python/scriptmark/__init__.py +++ b/python/scriptmark/__init__.py @@ -4,7 +4,9 @@ discover, grade, load_input, + load_record, load_spec, + rescore, run, StudentResult, TestSpec, @@ -14,7 +16,9 @@ "discover", "grade", "load_input", + "load_record", "load_spec", + "rescore", "run", "StudentResult", "TestSpec", From 8e85e5e12d2d07100c4885ba8545acacc277fe98 Mon Sep 17 00:00:00 2001 From: Acture Date: Sat, 3 Oct 2026 01:01:08 +0800 Subject: [PATCH 2/8] fix: address review of grading records (P-678) The assignment name is a label, so declaring assignment.toml after grading with derived items still rescores; Canvas ids and the attempt policy stay part of the evidence. Database sessions keep each revision's checksum and refuse a diverged copy's revision under the same number. --format is validated before anything runs, a non-UTF-8 path is refused before the run instead of panicking after it, and a Finder .DS_Store in a data directory is not fingerprinted. Tests cover the Canvas bundle path, a replaced archive behind a stale extraction, attempt policy, re-matched files and a session that fails part way. --- README.md | 17 ++- crates/scriptmark/src/db/mod.rs | 38 +++++ crates/scriptmark/src/db/results.rs | 28 +++- crates/scriptmark/src/db/schema.rs | 4 +- crates/scriptmark/src/main.rs | 26 +++- crates/scriptmark/src/record.rs | 51 ++++--- crates/scriptmark/tests/record.rs | 223 +++++++++++++++++++++++++++- 7 files changed, 340 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index bbbaee1..ac0e7fd 100644 --- a/README.md +++ b/README.md @@ -113,14 +113,15 @@ scriptmark grade submissions/ -t tests/ -r roster.csv --db grades.db -a archive/ # Run tests only: the grading record's evidence, with no score revision yet scriptmark run submissions/ -t tests/ -o output/results.json -# Score the saved evidence again after editing points or the curve in assignment.toml. -# Nothing runs; revision 2 is added beside revision 1, and whose grade changed is shown. -# Changed specs, teacher files, submissions or [matching] rules are refused: grade again. +# Score saved evidence under assignment.toml as it is now, without running anything: +# a `run` record gets revision 1, a graded one its next. Edit points or the curve and +# rescore again; earlier revisions are kept, and whose grade changed is shown. Changed +# specs, teacher files, submissions or [matching] rules are refused: grade again. scriptmark rescore output/results.json -# Read any revision: summarize, export grades as CSV, report +# Read the latest revision, or any other with --revision N scriptmark summarize output/results.json --revision 1 -scriptmark export output/results.json --revision 2 -o grades.csv +scriptmark export output/results.json -o grades.csv # Preview student/file/function matching; edit assignment.toml to resolve candidates scriptmark match submissions/ -t tests/ -o output/matches.json @@ -141,7 +142,8 @@ scriptmark canvas courses scriptmark canvas assignments --course-id 12345 scriptmark canvas fetch --course-id 12345 --assignment-id 67890 -o canvas/hw1 scriptmark grade --canvas canvas/hw1 -t tests/ -scriptmark grades-push --course-id 12345 --assignment-id 67890 output/results.json --revision 2 +# pushes the record's only revision; name one with --revision N once it holds several +scriptmark grades-push --course-id 12345 --assignment-id 67890 output/results.json # Or just pull the roster scriptmark roster-pull --course-id 12345 @@ -167,7 +169,8 @@ for r in results: else: print(f"{r.student_id}: {r.grade} ({r.score}/{r.max} points)") -# Score the record again under the policy as it is now, without running anything +# After editing the policy, score the record again without running anything +# (records graded from a Canvas bundle are rescored with the CLI) change = scriptmark.rescore("output/results.json") # {"revision": 2, "changes": [...]} first = scriptmark.load_record("output/results.json", revision=1) diff --git a/crates/scriptmark/src/db/mod.rs b/crates/scriptmark/src/db/mod.rs index 629b993..534f2ea 100644 --- a/crates/scriptmark/src/db/mod.rs +++ b/crates/scriptmark/src/db/mod.rs @@ -30,6 +30,11 @@ pub enum DbError { Stored(String), #[error("'{0}' has no grade: a session stores a score revision")] Unscored(String), + #[error( + "session #{session} already holds another revision {revision} of this evidence, \ + scored from a different copy of the record; refusing to mix them" + )] + Conflict { session: i64, revision: u32 }, #[error("{0}")] Record(String), } @@ -78,6 +83,7 @@ mod tests { assignment, evidence: assignment, revision: 1, + checksum: "c1", bundle: "{}", grading_policy: "{}", } @@ -529,4 +535,36 @@ mod tests { "{err}" ); } + + #[test] + fn test_another_revision_under_a_saved_number_is_refused() { + let db = Database::open_memory().unwrap(); + let reports = [graded("alice", 90.0)]; + let id = db.save_session(&of("hw5"), &reports).unwrap().id; + let other = SessionOf { + checksum: "c2", + ..of("hw5") + }; + let err = db.save_session(&other, &reports).unwrap_err(); + assert!( + matches!(err, DbError::Conflict { session, revision: 1 } if session == id), + "{err}" + ); + } + + #[test] + fn test_a_session_that_fails_part_way_leaves_nothing() { + let db = Database::open_memory().unwrap(); + db.conn + .execute_batch( + "CREATE TRIGGER fail BEFORE INSERT ON results WHEN NEW.student_id = 'bob' + BEGIN SELECT RAISE(ABORT, 'disk full'); END;", + ) + .unwrap(); + let reports = [graded("alice", 90.0), graded("bob", 80.0)]; + assert!(db.save_session(&of("hw5"), &reports).is_err()); + assert!(db.list_sessions().unwrap().is_empty(), "no half session"); + db.conn.execute_batch("DROP TRIGGER fail").unwrap(); + assert!(db.save_session(&of("hw5"), &reports).unwrap().created); + } } diff --git a/crates/scriptmark/src/db/results.rs b/crates/scriptmark/src/db/results.rs index 4d39837..7108609 100644 --- a/crates/scriptmark/src/db/results.rs +++ b/crates/scriptmark/src/db/results.rs @@ -29,6 +29,8 @@ pub struct SessionOf<'a> { pub assignment: &'a str, pub evidence: &'a str, pub revision: u32, + /// The revision's checksum. + pub checksum: &'a str, pub bundle: &'a str, pub grading_policy: &'a str, } @@ -173,22 +175,24 @@ impl Database { /// not saved twice. pub fn save_revision(&self, record: &Record, revision: u32) -> Result { let invalid = |e: anyhow::Error| DbError::Record(format!("{e:#}")); - let policy = &record.revision(revision).map_err(invalid)?.policy; + let entry = record.revision(revision).map_err(invalid)?; let view = record.view(Some(revision)).map_err(invalid)?; self.save_session( &SessionOf { assignment: &record.evidence.assignment.name, evidence: &record.digest, revision, + checksum: &entry.checksum, bundle: &serde_json::to_string(&record.evidence.bundle)?, - grading_policy: &serde_json::to_string(policy)?, + grading_policy: &serde_json::to_string(&entry.policy)?, }, &view.reports, ) } /// Save scored reports as a session, in one transaction. Returns the session it already - /// has when this revision of this evidence was saved before. + /// has when this revision of this evidence was saved before, and refuses one saved under + /// the same number with other grades. pub fn save_session( &self, of: &SessionOf, @@ -206,15 +210,21 @@ impl Database { if let Some(report) = reports.iter().find(|r| r.grade.is_none()) { return Err(DbError::Unscored(report.student_id.clone())); } - if let Some(id) = self + if let Some((id, checksum)) = self .conn .query_row( - "SELECT id FROM sessions WHERE evidence = ?1 AND revision = ?2", + "SELECT id, checksum FROM sessions WHERE evidence = ?1 AND revision = ?2", rusqlite::params![of.evidence, of.revision], - |row| row.get(0), + |row| Ok((row.get::<_, i64>(0)?, row.get::<_, String>(1)?)), ) .optional()? { + if checksum != of.checksum { + return Err(DbError::Conflict { + session: id, + revision: of.revision, + }); + } return Ok(Saved { id, created: false }); } @@ -229,12 +239,14 @@ impl Database { let tx = self.conn.unchecked_transaction()?; tx.execute( "INSERT INTO sessions - (assignment, evidence, revision, bundle, grading_policy, student_count, avg_grade) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7)", + (assignment, evidence, revision, checksum, bundle, grading_policy, student_count, + avg_grade) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)", rusqlite::params![ of.assignment, of.evidence, of.revision, + of.checksum, of.bundle, of.grading_policy, reports.len() as i64, diff --git a/crates/scriptmark/src/db/schema.rs b/crates/scriptmark/src/db/schema.rs index f36cb48..c66df03 100644 --- a/crates/scriptmark/src/db/schema.rs +++ b/crates/scriptmark/src/db/schema.rs @@ -33,9 +33,11 @@ pub fn migrate(conn: &Connection) -> Result<(), DbError> { CREATE TABLE IF NOT EXISTS sessions ( id INTEGER PRIMARY KEY AUTOINCREMENT, assignment TEXT NOT NULL, - -- The record's evidence digest, and which of its revisions this is. + -- The record's evidence digest, which of its revisions this is, and that + -- revision's checksum: two copies of a record can number different revisions alike. evidence TEXT NOT NULL, revision INTEGER NOT NULL, + checksum TEXT NOT NULL, -- The test bundle's version: spec and source digests, seeds, answers (JSON). bundle TEXT NOT NULL, -- The revision's items and grading policy (JSON). diff --git a/crates/scriptmark/src/main.rs b/crates/scriptmark/src/main.rs index 1643a60..1c96338 100644 --- a/crates/scriptmark/src/main.rs +++ b/crates/scriptmark/src/main.rs @@ -126,8 +126,8 @@ struct GradeArgs { archive: Option, /// Archive format - #[arg(short, long, default_value = "csv")] - format: String, + #[arg(short, long, value_enum, default_value_t = ArchiveFormat::Csv)] + format: ArchiveFormat, /// Save results to SQLite database #[arg(long)] @@ -137,6 +137,15 @@ struct GradeArgs { frozen: FrozenArgs, } +/// How `--archive` writes the evidence: refused when spelled wrong, before anything runs. +#[derive(Clone, Copy, PartialEq, Eq, clap::ValueEnum)] +enum ArchiveFormat { + /// The grading record itself + Json, + /// One row per case + Csv, +} + #[derive(Parser)] struct RunArgs { /// Directories containing student submissions @@ -888,7 +897,11 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { .file_name() .and_then(|n| n.to_str()) .unwrap_or("results"); - let archive_path = archive_dir.join(format!("archive_{stem}.{}", args.format)); + let extension = match args.format { + ArchiveFormat::Json => "json", + ArchiveFormat::Csv => "csv", + }; + let archive_path = archive_dir.join(format!("archive_{stem}.{extension}")); let grades_path = archive_dir.join(format!("grades_{stem}.csv")); if !frozen.is_empty() { let cases_path = archive_dir.join(format!("cases_{stem}.json")); @@ -898,11 +911,11 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { scriptmark::export::write_grades_csv(reports, items, std::fs::File::create(&grades_path)?)?; println!("Grades written to {}", grades_path.display()); - match args.format.as_str() { - "json" => { + match args.format { + ArchiveFormat::Json => { std::fs::write(&archive_path, record.to_json())?; } - "csv" => { + ArchiveFormat::Csv => { let mut wtr = csv::Writer::from_path(&archive_path)?; wtr.write_record([ "student_name", @@ -973,7 +986,6 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { } wtr.flush()?; } - other => anyhow::bail!("unknown archive format '{other}': use json or csv"), } println!("Archived to {}", archive_path.display()); } diff --git a/crates/scriptmark/src/record.rs b/crates/scriptmark/src/record.rs index 8c7159d..1e35e36 100644 --- a/crates/scriptmark/src/record.rs +++ b/crates/scriptmark/src/record.rs @@ -80,17 +80,6 @@ impl From<&Assignment> for AssignmentId { } } -impl std::fmt::Display for AssignmentId { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "'{}'", self.name)?; - if let (Some(course), Some(assignment)) = (self.canvas_course_id, self.canvas_assignment_id) - { - write!(f, " (Canvas course {course}, assignment {assignment})")?; - } - Ok(()) - } -} - /// Where a batch's inputs are, as absolute paths. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] @@ -231,7 +220,7 @@ impl Record { report.student_id ); } - let digest = evidence_digest(&evidence); + let digest = evidence_digest(&evidence).context("the evidence cannot be recorded")?; Ok(Record { format: FORMAT, evidence, @@ -271,6 +260,7 @@ impl Record { } pub fn to_json(&self) -> String { + // `new` already serialised the evidence, and a revision is numbers and words. serde_json::to_string_pretty(self).expect("a grading record is plain JSON") + "\n" } @@ -285,7 +275,7 @@ impl Record { /// What the stored digests and numbering claim must hold. fn verify(&self) -> Result<(), String> { - if evidence_digest(&self.evidence) != self.digest { + if evidence_digest(&self.evidence).map_err(|e| e.to_string())? != self.digest { return Err( "its evidence does not match its digest: it was edited or is incomplete".into(), ); @@ -414,11 +404,16 @@ impl Record { pub fn check(&self, current: &Current) -> Result<()> { let evidence = &self.evidence; let mut changed = Vec::new(); - let assignment = AssignmentId::from(current.assignment); - if assignment != evidence.assignment { + // The name is a label — the record keeps the one it was graded under — but the + // Canvas ids say whose grades these are. + let (was, now) = (&evidence.assignment, AssignmentId::from(current.assignment)); + if (now.canvas_course_id, now.canvas_assignment_id) + != (was.canvas_course_id, was.canvas_assignment_id) + { changed.push(format!( - "the assignment is {assignment}, not {}", - evidence.assignment + "the assignment's Canvas ids are {}, not {}", + canvas_ids(&now), + canvas_ids(was) )); } if current.attempt_policy != evidence.attempt_policy { @@ -579,7 +574,7 @@ pub fn spec_versions(specs: &[TestSpec]) -> Result> { /// Every teacher file a spec reads, hashed: imports, data files, Python checkers and /// reference implementations, and every other `.py` file beside a module, checker or /// reference — the harness puts their directory on `sys.path`, so a helper there is -/// imported without being declared. +/// imported without being declared. A Finder `.DS_Store` in a data directory is not. fn sources(spec: &TestSpec) -> Result, String> { let mut files: BTreeSet = spec .meta @@ -629,6 +624,8 @@ fn sources(spec: &TestSpec) -> Result, String> { for path in files { fingerprint(&path, &mut hashes, &mut BTreeSet::new())?; } + // Finder leaves these in any folder it opens; no test reads one. + hashes.retain(|path, _| !path.ends_with("/.DS_Store")); Ok(hashes) } @@ -670,6 +667,10 @@ pub fn bundle_version( /// of the archives they came out of. pub fn submission_version(student: &StudentSubmission) -> Result { let hash = |path: &Path| -> Result { + // Taken before anything runs: a path the record cannot hold must not cost a run. + if path.to_str().is_none() { + bail!("cannot record {}: its path is not UTF-8", path.display()); + } let bytes = std::fs::read(path).with_context(|| format!("cannot read {}", path.display()))?; Ok(FileVersion { @@ -827,8 +828,8 @@ pub fn diff(before: Option<&Revision>, after: &Revision) -> Vec { .collect() } -fn evidence_digest(evidence: &Evidence) -> String { - digest(&serde_json::to_vec(evidence).expect("evidence is plain JSON")) +fn evidence_digest(evidence: &Evidence) -> serde_json::Result { + Ok(digest(&serde_json::to_vec(evidence)?)) } fn revision_checksum(revision: &Revision) -> String { @@ -844,6 +845,15 @@ fn revision_checksum(revision: &Revision) -> String { ) } +fn canvas_ids(assignment: &AssignmentId) -> String { + let id = |id: Option| id.map_or_else(|| "none".to_string(), |id| id.to_string()); + format!( + "course {} and assignment {}", + id(assignment.canvas_course_id), + id(assignment.canvas_assignment_id) + ) +} + fn word(value: &T) -> String { crate::export::word(value) } @@ -1126,6 +1136,7 @@ mod tests { at("solutions/ref.py", "def answer(x):\n return x\n"); at("check.py", "print('{}')\n"); at("data/poem.txt", "words\n"); + at("data/.DS_Store", "finder"); let spec = crate::spec_loader::load_spec_str( "[meta]\nname = 'q'\nfile = 'q.py'\nfunction = 'f'\nlanguage = 'python'\n\ imports = ['teacher/support.py']\ndata_files = ['data/']\n\ diff --git a/crates/scriptmark/tests/record.rs b/crates/scriptmark/tests/record.rs index 7e694bc..029fed4 100644 --- a/crates/scriptmark/tests/record.rs +++ b/crates/scriptmark/tests/record.rs @@ -261,7 +261,7 @@ fn rescoring_runs_nothing_and_keeps_the_earlier_revision() { #[test] fn rescoring_refuses_evidence_whose_tests_submissions_or_matching_changed() { type Change = fn(&Path); - let changes: [(&str, Change, &str); 7] = [ + let changes: [(&str, Change, &str); 9] = [ ( "spec", |dir| { @@ -326,16 +326,45 @@ fn rescoring_refuses_evidence_whose_tests_submissions_or_matching_changed() { "the [matching] rules changed", ), ( - "assignment", + "Canvas ids", |dir| { let assignment = std::fs::read_to_string(dir.join("assignment.toml")).unwrap(); write( dir, "assignment.toml", - &assignment.replace("name = \"sums\"", "name = \"sums, again\""), + &assignment.replace( + "name = \"sums\"", + "name = \"sums\"\ncanvas_course_id = 7\ncanvas_assignment_id = 8", + ), ); }, - "the assignment is 'sums, again', not 'sums'", + "the assignment's Canvas ids are course 7 and assignment 8, not course none", + ), + ( + "attempt policy", + |dir| { + let assignment = std::fs::read_to_string(dir.join("assignment.toml")).unwrap(); + write( + dir, + "assignment.toml", + &assignment.replace( + "name = \"sums\"", + "name = \"sums\"\nattempt_policy = \"earliest\"", + ), + ); + }, + "the attempt policy is earliest, not latest", + ), + ( + "file match", + |dir| { + std::fs::rename( + dir.join("submissions/alice_sum.py"), + dir.join("submissions/alice_add.py"), + ) + .unwrap() + }, + "local:alice's file for 'sum' is matched differently", ), ]; for (what, change, why) in changes { @@ -529,3 +558,189 @@ fn results_from_before_grading_records_are_refused_by_every_reader() { refused(scriptmark(dir, args), "before grading records"); } } + +#[test] +fn the_assignment_name_is_a_label_so_declaring_items_later_rescores() { + let temp = bench(); + let dir = temp.path(); + // Graded with items derived from the specs, under the folder's name... + std::fs::remove_file(dir.join("assignment.toml")).unwrap(); + succeeded(scriptmark(dir, &GRADE)); + let derived = record(dir).evidence.assignment.name; + assert!(record(dir).revisions[0].policy.derived_items); + // ...then weighed, as the note `grade` printed suggests, under a name of its own. + write( + dir, + "assignment.toml", + &ASSIGNMENT + .replace("name = \"sums\"", "name = \"Homework 3\"") + .replace("proportional", "all_or_nothing"), + ); + succeeded(rescore(dir, &[])); + let record = record(dir); + assert_eq!(record.evidence.assignment.name, derived); + assert!(!record.revisions[1].policy.derived_items); + assert_eq!(grades(dir, None), graded(100.0, 0.0)); +} + +#[test] +fn a_replaced_archive_is_a_changed_submission_even_behind_a_stale_extraction() { + let temp = bench(); + let dir = temp.path(); + let zip_of = |content: &str| { + std::fs::remove_file(dir.join("submissions/bob_sum.py")).ok(); + let file = std::fs::File::create(dir.join("submissions/bob_hw.zip")).unwrap(); + let mut zip = zip::ZipWriter::new(file); + zip.start_file("sum.py", zip::write::SimpleFileOptions::default()) + .unwrap(); + std::io::Write::write_all(&mut zip, content.as_bytes()).unwrap(); + zip.finish().unwrap(); + }; + zip_of("def add(a, b):\n return abs(a) + abs(b)\n"); + succeeded(scriptmark(dir, &GRADE)); + let bob = record(dir) + .evidence + .students + .into_iter() + .find(|r| r.student_id == "local:bob") + .unwrap(); + assert_eq!(bob.submission.unwrap().archives.len(), 1); + + // The extraction from the first archive stays on disk; the archive itself changed. + zip_of("def add(a, b):\n return a + b\n"); + all_or_nothing(dir); + refused(rescore(dir, &[]), "local:bob's submitted files changed"); +} + +/// A Canvas bundle as `canvas fetch` leaves one, for course 7, assignment 8. Ada tried +/// twice — wrong, then right — Ben once, wrongly, and Cy, who handed nothing in, is excused. +fn canvas_bundle(dir: &Path) { + let bundle = dir.join("canvas/hw"); + let mut manifest = serde_json::Map::new(); + for (id, source) in [ + (1001, "def add(a, b):\n return 0\n"), + (1002, "def add(a, b):\n return a + b\n"), + (1003, "def add(a, b):\n return abs(a) + abs(b)\n"), + ] { + let path = bundle.join(format!("attachments/{id}/sum.py")); + write( + dir, + path.strip_prefix(dir).unwrap().to_str().unwrap(), + source, + ); + manifest.insert( + id.to_string(), + serde_json::json!({ "stored": { "path": path, "size": source.len() } }), + ); + } + let attempt = |n: u32, attachment: u64| { + serde_json::json!({ + "user_id": 11, "attempt": n, "workflow_state": "submitted", + "submitted_at": format!("2026-10-0{n}T08:00:00Z"), "submission_type": "online_upload", + "attachments": [{ "id": attachment, "filename": "sum.py" }], + }) + }; + let payload = serde_json::json!({ + "course_id": 7, "assignment_id": 8, "assignment_name": "sums", + "users": [ + { "id": 11, "name": "Ada", "sis_user_id": "2024001" }, + { "id": 12, "name": "Ben", "sis_user_id": "2024002" }, + { "id": 13, "name": "Cy", "sis_user_id": "2024003" }, + ], + "submissions": [ + { + "id": 1, "user_id": 11, "attempt": 2, "workflow_state": "submitted", + "submitted_at": "2026-10-02T08:00:00Z", "submission_type": "online_upload", + "attachments": [{ "id": 1002, "filename": "sum.py" }], + "submission_history": [attempt(1, 1001), attempt(2, 1002)], + }, + { + "id": 2, "user_id": 12, "attempt": 1, "workflow_state": "submitted", + "submitted_at": "2026-10-01T09:00:00Z", "submission_type": "online_upload", + "attachments": [{ "id": 1003, "filename": "sum.py" }], + }, + { "id": 3, "user_id": 13, "workflow_state": "unsubmitted", "excused": true }, + ], + }); + write( + dir, + "canvas/hw/canvas-payload.json", + &serde_json::to_string_pretty(&payload).unwrap(), + ); + write( + dir, + "canvas/hw/attachments.json", + &serde_json::to_string_pretty(&manifest).unwrap(), + ); +} + +#[test] +fn a_canvas_record_rescores_from_its_bundle_and_refuses_another_attempt_or_excusal() { + let temp = bench(); + let dir = temp.path(); + canvas_bundle(dir); + let grade = [ + "grade", + "--canvas", + "canvas/hw", + "-t", + "tests", + "-o", + "out/results.json", + ]; + succeeded(scriptmark(dir, &grade)); + let evidence = record(dir).evidence; + assert_eq!( + ( + evidence.assignment.canvas_course_id, + evidence.assignment.canvas_assignment_id + ), + (Some(7), Some(8)) + ); + let ada = evidence + .students + .iter() + .find(|r| r.canvas_user_id == Some(11)) + .unwrap(); + assert_eq!(ada.submission.as_ref().unwrap().attempt, Some(2)); + assert_eq!( + grades(dir, None) + .into_iter() + .map(|(_, grade)| grade) + .collect::>(), + [Some(100.0), Some(50.0), None], + "Ada on her second attempt; Cy excused" + ); + + // Rescoring reads the bundle and writes nothing into it. + std::fs::remove_file(dir.join("canvas/hw/input.json")).unwrap(); + all_or_nothing(dir); + succeeded(rescore(dir, &[])); + assert!(!dir.join("canvas/hw/input.json").exists()); + assert_eq!( + grades(dir, None) + .into_iter() + .map(|(_, grade)| grade) + .collect::>(), + [Some(100.0), Some(0.0), None] + ); + + write( + dir, + "assignment.toml", + &ASSIGNMENT.replace( + "name = \"sums\"", + "name = \"sums\"\nattempt_policy = \"earliest\"", + ), + ); + let output = rescore(dir, &[]); + refused(output, "the attempt policy is earliest, not latest"); + write(dir, "assignment.toml", ASSIGNMENT); + + let payload_path = dir.join("canvas/hw/canvas-payload.json"); + let mut payload: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&payload_path).unwrap()).unwrap(); + payload["submissions"][1]["excused"] = true.into(); + std::fs::write(&payload_path, payload.to_string()).unwrap(); + refused(rescore(dir, &[]), "2024002 is excused now"); +} From 303afe09632f5856f0342207d8f886a840a429bf Mon Sep 17 00:00:00 2001 From: Acture Date: Sat, 3 Oct 2026 01:04:42 +0800 Subject: [PATCH 3/8] feat: save any revision of a grading record to the database (P-678) db save [--revision N] stores a revision as a session after the fact, so the database, like summarize, export and grades-push, can take a named revision rather than only the one grade or rescore just made. --- README.md | 1 + crates/scriptmark/src/main.rs | 26 ++++++++++++++++ crates/scriptmark/tests/record.rs | 49 +++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+) diff --git a/README.md b/README.md index ac0e7fd..fbe9c0a 100644 --- a/README.md +++ b/README.md @@ -122,6 +122,7 @@ scriptmark rescore output/results.json # Read the latest revision, or any other with --revision N scriptmark summarize output/results.json --revision 1 scriptmark export output/results.json -o grades.csv +scriptmark db save output/results.json --revision 1 --db grades.db # Preview student/file/function matching; edit assignment.toml to resolve candidates scriptmark match submissions/ -t tests/ -o output/matches.json diff --git a/crates/scriptmark/src/main.rs b/crates/scriptmark/src/main.rs index 1c96338..023d99d 100644 --- a/crates/scriptmark/src/main.rs +++ b/crates/scriptmark/src/main.rs @@ -411,6 +411,16 @@ enum DbAction { #[arg(default_value = "scriptmark.db")] path: PathBuf, }, + /// Save a revision of a grading record as a session + Save { + /// The grading record + record: PathBuf, + #[command(flatten)] + revision: RevisionArg, + /// Database file path + #[arg(long, default_value = "scriptmark.db")] + db: PathBuf, + }, /// Import a roster CSV into the database ImportRoster { /// Roster CSV file @@ -1576,6 +1586,22 @@ fn cmd_db(cmd: DbCommand) -> Result<()> { println!("Database initialized: {}", path.display()); Ok(()) } + DbAction::Save { + record, + revision, + db, + } => { + 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()) + } DbAction::ImportRoster { roster, db } => { let database = scriptmark::db::Database::open(&db).context("Failed to open database")?; diff --git a/crates/scriptmark/tests/record.rs b/crates/scriptmark/tests/record.rs index 029fed4..5a08f2f 100644 --- a/crates/scriptmark/tests/record.rs +++ b/crates/scriptmark/tests/record.rs @@ -544,6 +544,55 @@ fn database_sessions_are_revisions_of_one_evidence() { assert!(text(&listed).contains(&digest[..12]), "{}", text(&listed)); } +#[test] +fn any_revision_can_be_saved_to_the_database_later_and_only_once() { + let temp = bench(); + let dir = temp.path(); + let save = |extra: &[&str]| { + let mut args = vec!["db", "save", "out/results.json", "--db", "grades.db"]; + args.extend_from_slice(extra); + scriptmark(dir, &args) + }; + succeeded(scriptmark( + dir, + &[ + "run", + "submissions", + "-t", + "tests", + "-o", + "out/results.json", + ], + )); + refused(save(&[]), "has no score revision yet"); + succeeded(rescore(dir, &[])); + all_or_nothing(dir); + succeeded(rescore(dir, &[])); + + let first = succeeded(save(&["--revision", "1"])); + assert!( + text(&first).contains("session #1, revision 1"), + "{}", + text(&first) + ); + let again = succeeded(save(&["--revision", "1"])); + assert!( + text(&again).contains("Revision 1 is already in grades.db as session #1"), + "{}", + text(&again) + ); + succeeded(save(&[])); + let db = scriptmark::db::Database::open(&dir.join("grades.db")).unwrap(); + assert_eq!( + db.list_sessions() + .unwrap() + .iter() + .map(|s| s.revision) + .collect::>(), + [2, 1] + ); +} + #[test] fn results_from_before_grading_records_are_refused_by_every_reader() { let temp = bench(); From 9a2ecb90aa2f24dc7cdb4192581e2e2cde6f8e78 Mon Sep 17 00:00:00 2001 From: Acture Date: Sat, 3 Oct 2026 01:05:27 +0800 Subject: [PATCH 4/8] docs: link published P-678 grading record notes --- notes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes b/notes index 16f98fb..e8b9ff5 160000 --- a/notes +++ b/notes @@ -1 +1 @@ -Subproject commit 16f98fbfc61e8d32c1e381c5d9f5c57089f75387 +Subproject commit e8b9ff5f844d752e837fc54a569781abf670fcc0 From 2912e6f140dbabe85f4b221bae9dcf7cd64ea377 Mon Sep 17 00:00:00 2001 From: Acture Date: Sat, 3 Oct 2026 03:45:40 +0800 Subject: [PATCH 5/8] feat: --force replaces a record holding rescored revisions (P-678) grade, run and match refused to write over such a record with no way past but moving it. --force now grades afresh and replaces it once the run succeeds, saying what it discards; Python's grade/run take force=True. It is long-only, and --format gives up its -f, so -f means nothing and cannot be typed by accident. --fresh still only replaces frozen inputs. --- README.md | 3 +++ crates/scriptmark-py/src/lib.rs | 29 ++++++++++++--------- crates/scriptmark/src/main.rs | 43 ++++++++++++++++++++++++------- crates/scriptmark/src/record.rs | 8 +++--- crates/scriptmark/tests/record.rs | 25 +++++++++++++++++- 5 files changed, 82 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index fbe9c0a..80a18ec 100644 --- a/README.md +++ b/README.md @@ -119,6 +119,9 @@ scriptmark run submissions/ -t tests/ -o output/results.json # specs, teacher files, submissions or [matching] rules are refused: grade again. scriptmark rescore output/results.json +# Grade afresh over a record that holds rescored revisions, discarding them +scriptmark grade submissions/ -t tests/ --force + # Read the latest revision, or any other with --revision N scriptmark summarize output/results.json --revision 1 scriptmark export output/results.json -o grades.csv diff --git a/crates/scriptmark-py/src/lib.rs b/crates/scriptmark-py/src/lib.rs index 64d3811..a0477c2 100644 --- a/crates/scriptmark-py/src/lib.rs +++ b/crates/scriptmark-py/src/lib.rs @@ -315,11 +315,14 @@ fn absolute(path: &Path) -> PyResult { .map_err(|e| pyo3::exceptions::PyOSError::new_err(format!("{}: {e}", path.display()))) } -/// Refuse to write a record over one holding rescored revisions, before anything runs. -fn check_output(output: Option<&str>) -> PyResult<()> { - match output { - Some(path) => record::check_replaceable(Path::new(path)).map_err(value_error), - None => Ok(()), +/// Refuse to write a record over one holding rescored revisions, before anything runs, +/// unless `force` says to discard them. +fn check_output(output: Option<&str>, force: bool) -> PyResult<()> { + match output.map(|path| record::check_replaceable(Path::new(path))) { + Some(Err(why)) if !force => Err(value_error(format!( + "{why}: move it aside, write elsewhere, or pass force=True to discard them" + ))), + _ => Ok(()), } } @@ -351,9 +354,9 @@ fn declared_for( /// /// `freeze` writes generated inputs and oracle answers; `replay` verifies and reuses /// a frozen bundle without recomputing its answers. `output` writes the grading record, -/// unscored, for `rescore` to score. +/// unscored, for `rescore` to score; `force=True` replaces one holding rescored revisions. #[pyfunction] -#[pyo3(signature = (submissions, tests, *, timeout=10, python="python3", assignment=None, freeze=None, replay=None, output=None))] +#[pyo3(signature = (submissions, tests, *, timeout=10, python="python3", assignment=None, freeze=None, replay=None, output=None, force=false))] // Each is a keyword argument of the Python API; a struct would not be one. #[allow(clippy::too_many_arguments)] fn run( @@ -365,8 +368,9 @@ fn run( freeze: Option, replay: Option, output: Option, + force: bool, ) -> PyResult { - check_output(output.as_deref())?; + check_output(output.as_deref(), force)?; let tests = absolute(Path::new(&tests))?; let assignment = assignment.map(|p| absolute(Path::new(&p))).transpose()?; let (declared, specs, _) = declared_for(&tests, assignment.as_deref())?; @@ -393,12 +397,12 @@ fn run( /// policy — `assignment.toml`, given or found beside the tests directory, exactly as the /// CLI reads it. /// -/// `freeze` and `replay` are as for `run`. `output` writes the grading record, with this -/// grade as its first revision. +/// `freeze`, `replay` and `force` are as for `run`. `output` writes the grading record, with +/// this grade as its first revision. /// /// Returns a list of StudentResult objects. #[pyfunction] -#[pyo3(signature = (submissions, tests, *, timeout=10, python="python3", assignment=None, freeze=None, replay=None, output=None))] +#[pyo3(signature = (submissions, tests, *, timeout=10, python="python3", assignment=None, freeze=None, replay=None, output=None, force=false))] // Each is a keyword argument of the Python API; a struct would not be one. #[allow(clippy::too_many_arguments)] fn grade( @@ -410,8 +414,9 @@ fn grade( freeze: Option, replay: Option, output: Option, + force: bool, ) -> PyResult> { - check_output(output.as_deref())?; + check_output(output.as_deref(), force)?; let tests = absolute(Path::new(&tests))?; let assignment = assignment.map(|p| absolute(Path::new(&p))).transpose()?; let (declared, specs, policy) = declared_for(&tests, assignment.as_deref())?; diff --git a/crates/scriptmark/src/main.rs b/crates/scriptmark/src/main.rs index 023d99d..23296a0 100644 --- a/crates/scriptmark/src/main.rs +++ b/crates/scriptmark/src/main.rs @@ -126,7 +126,7 @@ struct GradeArgs { archive: Option, /// Archive format - #[arg(short, long, value_enum, default_value_t = ArchiveFormat::Csv)] + #[arg(long, value_enum, default_value_t = ArchiveFormat::Csv)] format: ArchiveFormat, /// Save results to SQLite database @@ -135,6 +135,11 @@ struct GradeArgs { #[command(flatten)] frozen: FrozenArgs, + + /// Replace the grading record at --output even when it holds rescored revisions, + /// discarding them. Long form only: there is no -f. + #[arg(long)] + force: bool, } /// How `--archive` writes the evidence: refused when spelled wrong, before anything runs. @@ -189,6 +194,11 @@ struct RunArgs { #[command(flatten)] frozen: FrozenArgs, + + /// Replace the grading record at --output even when it holds rescored revisions, + /// discarding them. Long form only: there is no -f. + #[arg(long)] + force: bool, } #[derive(Parser)] @@ -207,6 +217,11 @@ struct MatchArgs { assignment: Option, #[arg(long, default_value = "python3")] python: String, + + /// Replace the grading record at --output even when it holds rescored revisions, + /// discarding them. Long form only: there is no -f. + #[arg(long)] + force: bool, } /// Which score revision of a grading record to read. @@ -789,11 +804,21 @@ fn prepare_batch( } /// Refuse to write a run's record over one holding rescored revisions — before anything -/// runs, so a refused run costs nothing. -fn check_output(output: &Path) -> Result<()> { - record::check_replaceable(output) - .map_err(anyhow::Error::msg) - .context("refusing to replace the grading record") +/// runs, so a refused run costs nothing — unless `--force` says to discard them. Even then +/// the record is replaced only once the run has succeeded. +fn check_output(output: &Path, force: bool) -> Result<()> { + match record::check_replaceable(output) { + Ok(()) => Ok(()), + Err(why) if force => { + eprintln!(" note: {why}; --force replaces it once this run succeeds"); + Ok(()) + } + Err(why) => Err(anyhow::anyhow!( + "{why}: move it aside, write this run elsewhere with --output, or pass --force to \ + discard them" + )) + .context("refusing to replace the grading record"), + } } /// Run the batch and record what it found, unscored. The submissions are fingerprinted @@ -847,7 +872,7 @@ fn write_record(record: &Record, output: &Path) -> Result<()> { } async fn cmd_grade(args: GradeArgs) -> Result<()> { - check_output(&args.output)?; + check_output(&args.output, args.force)?; let Batch { input, specs, @@ -1041,7 +1066,7 @@ fn save_to_db( } async fn cmd_run(args: RunArgs) -> Result<()> { - check_output(&args.output)?; + check_output(&args.output, args.force)?; // The policy is settled even though nothing is scored: a run whose results cannot be // graded should say so now, not after the class has run. let Batch { @@ -1089,7 +1114,7 @@ async fn cmd_run(args: RunArgs) -> Result<()> { } fn cmd_match(args: MatchArgs) -> Result<()> { - check_output(&args.output)?; + check_output(&args.output, args.force)?; let Batch { input, specs, diff --git a/crates/scriptmark/src/record.rs b/crates/scriptmark/src/record.rs index 1e35e36..d6384aa 100644 --- a/crates/scriptmark/src/record.rs +++ b/crates/scriptmark/src/record.rs @@ -743,16 +743,16 @@ pub fn seal( Ok(()) } -/// Whether a run may write its record over `path`. A record holding rescored revisions — -/// grades kept nowhere else — is never replaced; anything else is, as before. +/// Whether a run may write its record over `path` without losing anything: not when it +/// holds rescored revisions, grades kept nowhere else. Anything else is replaced, as before. +/// The caller says what to do instead, and may be told to replace it anyway. pub fn check_replaceable(path: &Path) -> Result<(), String> { let Ok(text) = std::fs::read_to_string(path) else { return Ok(()); }; match Record::from_json(&text) { Ok(record) if record.revisions.len() > 1 => Err(format!( - "{} holds {} score revisions, which it alone records; move it aside, or write this \ - run to another file", + "{} holds {} score revisions, which it alone records", path.display(), record.revisions.len() )), diff --git a/crates/scriptmark/tests/record.rs b/crates/scriptmark/tests/record.rs index 5a08f2f..e42d3ce 100644 --- a/crates/scriptmark/tests/record.rs +++ b/crates/scriptmark/tests/record.rs @@ -422,7 +422,7 @@ fn a_run_is_scored_by_its_first_rescore() { } #[test] -fn a_record_with_rescored_revisions_is_never_replaced() { +fn a_record_with_rescored_revisions_is_replaced_only_by_force() { let temp = bench(); let dir = temp.path(); succeeded(scriptmark(dir, &GRADE)); @@ -459,6 +459,29 @@ fn a_record_with_rescored_revisions_is_never_replaced() { "out/again.json", ], )); + + // There is no short form to type by accident. + let mut short = GRADE.to_vec(); + short.push("-f"); + refused(scriptmark(dir, &short), "unexpected argument '-f'"); + assert_eq!(std::fs::read(dir.join("out/results.json")).unwrap(), saved); + + // --force grades afresh over it, says what it discards, and composes with --fresh. + let mut force = GRADE.to_vec(); + force.extend(["--force", "--fresh"]); + let output = succeeded(scriptmark(dir, &force)); + assert!( + text(&output).contains("holds 2 score revisions, which it alone records; --force"), + "{}", + text(&output) + ); + let replaced = record(dir); + assert_eq!(replaced.revisions.len(), 1); + assert_eq!( + grades(dir, None), + graded(100.0, 0.0), + "under the current policy" + ); } #[test] From 5958577b60ca503214f172aeeb8965e67c68c8b1 Mon Sep 17 00:00:00 2001 From: Acture Date: Sat, 3 Oct 2026 03:46:06 +0800 Subject: [PATCH 6/8] docs: link published P-678 --force notes --- notes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes b/notes index e8b9ff5..2a02a4f 160000 --- a/notes +++ b/notes @@ -1 +1 @@ -Subproject commit e8b9ff5f844d752e837fc54a569781abf670fcc0 +Subproject commit 2a02a4f0b1f2370f39a8d46fc825d703a1172774 From a2a87a6404be5db542ecd3d4e782aed134377a94 Mon Sep 17 00:00:00 2001 From: Acture Date: Sat, 3 Oct 2026 23:04:55 +0800 Subject: [PATCH 7/8] refactor: the record is the only JSON; --archive writes tables (P-678) --format json only copied the record --output already holds, and an unknown format failed after the whole class had run. --archive now always writes its CSV tables (per case, per student) beside the frozen inputs. --- crates/scriptmark/src/main.rs | 158 +++++++++++++----------------- crates/scriptmark/tests/record.rs | 27 +++++ 2 files changed, 95 insertions(+), 90 deletions(-) diff --git a/crates/scriptmark/src/main.rs b/crates/scriptmark/src/main.rs index 23296a0..b1a765d 100644 --- a/crates/scriptmark/src/main.rs +++ b/crates/scriptmark/src/main.rs @@ -121,14 +121,11 @@ struct GradeArgs { #[arg(long, default_value = "python3")] python: String, - /// Archive results to this directory (JSON/CSV) + /// Also write CSV tables to this directory: one row per case, and one per student's + /// grade, beside the frozen inputs. The JSON is the record at --output. #[arg(short, long)] archive: Option, - /// Archive format - #[arg(long, value_enum, default_value_t = ArchiveFormat::Csv)] - format: ArchiveFormat, - /// Save results to SQLite database #[arg(long)] db: Option, @@ -142,15 +139,6 @@ struct GradeArgs { force: bool, } -/// How `--archive` writes the evidence: refused when spelled wrong, before anything runs. -#[derive(Clone, Copy, PartialEq, Eq, clap::ValueEnum)] -enum ArchiveFormat { - /// The grading record itself - Json, - /// One row per case - Csv, -} - #[derive(Parser)] struct RunArgs { /// Directories containing student submissions @@ -932,11 +920,7 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { .file_name() .and_then(|n| n.to_str()) .unwrap_or("results"); - let extension = match args.format { - ArchiveFormat::Json => "json", - ArchiveFormat::Csv => "csv", - }; - let archive_path = archive_dir.join(format!("archive_{stem}.{extension}")); + let archive_path = archive_dir.join(format!("archive_{stem}.csv")); let grades_path = archive_dir.join(format!("grades_{stem}.csv")); if !frozen.is_empty() { let cases_path = archive_dir.join(format!("cases_{stem}.json")); @@ -946,82 +930,76 @@ async fn cmd_grade(args: GradeArgs) -> Result<()> { scriptmark::export::write_grades_csv(reports, items, std::fs::File::create(&grades_path)?)?; println!("Grades written to {}", grades_path.display()); - match args.format { - ArchiveFormat::Json => { - std::fs::write(&archive_path, record.to_json())?; + let mut wtr = csv::Writer::from_path(&archive_path)?; + wtr.write_record([ + "student_name", + "student_id", + "submission_state", + "item_id", + "case_name", + "status", + "actual", + "expected", + "message", + "elapsed_ms", + "fault", + "cause", + ])?; + for report in reports { + let state = label(Some(report.submission_state)); + let mut rows = 0usize; + for test_result in &report.test_results { + for case in &test_result.cases { + rows += 1; + wtr.write_record([ + report.student_name.as_deref().unwrap_or(""), + &report.student_id, + &state, + &test_result.item_id, + &case.case_name, + &format!("{:?}", case.status), + case.actual.as_deref().unwrap_or(""), + case.expected.as_deref().unwrap_or(""), + case.failure + .as_ref() + .map(|f| f.message.as_str()) + .unwrap_or(""), + &case.elapsed_ms.map(|ms| ms.to_string()).unwrap_or_default(), + &label(case.fault), + &label(case.cause), + ])?; + } } - ArchiveFormat::Csv => { - let mut wtr = csv::Writer::from_path(&archive_path)?; + // Every student gets at least one row, so the CSV covers the same cohort + // as the JSON archive rather than quietly dropping non-submitters. Its + // message says why there is no grade. + if rows == 0 { + let why = report + .error + .clone() + .unwrap_or_else(|| label(report.grade.as_ref().and_then(|g| g.reason()))); wtr.write_record([ - "student_name", - "student_id", - "submission_state", - "item_id", - "case_name", - "status", - "actual", - "expected", - "message", - "elapsed_ms", - "fault", - "cause", - ])?; - for report in reports { - let state = label(Some(report.submission_state)); - let mut rows = 0usize; - for test_result in &report.test_results { - for case in &test_result.cases { - rows += 1; - wtr.write_record([ - report.student_name.as_deref().unwrap_or(""), - &report.student_id, - &state, - &test_result.item_id, - &case.case_name, - &format!("{:?}", case.status), - case.actual.as_deref().unwrap_or(""), - case.expected.as_deref().unwrap_or(""), - case.failure - .as_ref() - .map(|f| f.message.as_str()) - .unwrap_or(""), - &case.elapsed_ms.map(|ms| ms.to_string()).unwrap_or_default(), - &label(case.fault), - &label(case.cause), - ])?; - } - } - // Every student gets at least one row, so the CSV covers the same cohort - // as the JSON archive rather than quietly dropping non-submitters. Its - // message says why there is no grade. - if rows == 0 { - let why = report.error.clone().unwrap_or_else(|| { - label(report.grade.as_ref().and_then(|g| g.reason())) - }); - wtr.write_record([ - report.student_name.as_deref().unwrap_or(""), - &report.student_id, - &state, - "", - "", - if report.error.is_some() { - "Error".to_string() - } else { - format!("{:?}", report.status()) - } - .as_str(), - "", - "", - &why, - "", - "", - "", - ])?; + report.student_name.as_deref().unwrap_or(""), + &report.student_id, + &state, + "", + "", + if report.error.is_some() { + "Error".to_string() + } else { + format!("{:?}", report.status()) } - } - wtr.flush()?; + .as_str(), + "", + "", + &why, + "", + "", + "", + ])?; } } + wtr.flush()?; println!("Archived to {}", archive_path.display()); } diff --git a/crates/scriptmark/tests/record.rs b/crates/scriptmark/tests/record.rs index e42d3ce..793e3ef 100644 --- a/crates/scriptmark/tests/record.rs +++ b/crates/scriptmark/tests/record.rs @@ -200,6 +200,33 @@ fn grade_writes_a_record_whose_first_revision_every_consumer_reads() { assert!(dir.join("out/report.html").exists()); } +#[test] +fn the_archive_is_tables_and_the_json_is_the_record() { + let temp = bench(); + let dir = temp.path(); + let mut args = GRADE.to_vec(); + args.extend(["--archive", "out/archive"]); + succeeded(scriptmark(dir, &args)); + let cases = std::fs::read_to_string(dir.join("out/archive/archive_tests.csv")).unwrap(); + assert!(cases.starts_with("student_name,student_id,"), "{cases}"); + let grades = std::fs::read_to_string(dir.join("out/archive/grades_tests.csv")).unwrap(); + assert!( + grades.contains("local:bob,,,graded,,5,10,50,50,"), + "{grades}" + ); + let files: Vec<_> = std::fs::read_dir(dir.join("out/archive")) + .unwrap() + .map(|e| e.unwrap().file_name().into_string().unwrap()) + .collect(); + assert!( + files.iter().all(|f| f.ends_with(".csv")), + "no second copy of the record: {files:?}" + ); + + args.extend(["--format", "json"]); + refused(scriptmark(dir, &args), "unexpected argument '--format'"); +} + #[test] fn rescoring_runs_nothing_and_keeps_the_earlier_revision() { let temp = bench(); From 6b51dc5283bc9c807cf9785acc12c7d317ee8da8 Mon Sep 17 00:00:00 2001 From: Acture Date: Sat, 3 Oct 2026 23:05:21 +0800 Subject: [PATCH 8/8] docs: link published P-678 archive notes --- notes | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/notes b/notes index 2a02a4f..b64db70 160000 --- a/notes +++ b/notes @@ -1 +1 @@ -Subproject commit 2a02a4f0b1f2370f39a8d46fc825d703a1172774 +Subproject commit b64db70d71a2b08b50bc636244da07bcd80266dc