git-ents.gitmain
⌘K
foforge
commit c7dd37f
feat: give checks static dependencies and composite aggregates

A check body now holds an optional command, a reserved sandbox image, and the names of sibling checks that must pass first. The graph is fully static: validated and topologically sorted when the set is written, so the worker only ever walks a fixed order. A check whose dependency did not pass is recorded skipped without running, and a command-less composite derives its outcome from its dependencies alone. The image field is rejected until the Sprite sandbox can honor one, so it lands in the format now without a later data migration. The command’s move to an Option subtree is an incompatible format change: an existing check set must be re-added (acceptable pre-1.0, per the module’s migration note.

feat: add optional command, image, and depends to the checks format feat: add checks::order write-time graph validation and topological sort feat: add Status::Skipped for a check whose dependency did not pass feat: settle checks in dependency order and derive composite outcomes in the worker feat: add --image and --depends to git ents checks add feat: render a check’s image and dependencies on the web configuration card Assisted-by: Claude:claude-fable-5 EOF )

Joseph D. Carpinelli · 1 month ago

Reviews

No reviews of this commit yet — record a verdict below.

Start a review

verdict

crates/git-ents-server/src/checks.rs @@ -186,10 +186,14 @@ /// Run the checks for one queued push in its repository's Sprite, advancing the /// recorded run as it goes: `running` while the Sprite is prepared, then each -/// check flipped to its result as it finishes. An infra failure (an unreachable -/// Sprite, a tree that will not sync) finalizes the run as `error` rather than -/// leaving it stuck at `running`, then returns `Err`. Returns `Ok` even when a -/// check fails — a failing check is a recorded result, not an error. +/// check flipped to its result as it finishes. Checks settle in the dependency +/// order `checks::order` fixed at write time: a check whose dependency did not +/// pass is recorded `skipped` without touching the Sprite, and a composite (no +/// command) derives its status from its dependencies alone. An infra failure +/// (an unreachable Sprite, a tree that will not sync, a check set that fails +/// re-validation) finalizes the run as `error` rather than leaving it stuck at +/// `running`, then returns `Err`. Returns `Ok` even when a check fails — a +/// failing check is a recorded result, not an error. fn process_job(job: &Job) -> Result<(), String> { let runnable = checks::load(&job.repo).map_err(|e| format!("could not read checks: {e}"))?; if runnable.is_empty() { @@ -197,6 +201,19 @@ } let mut outcomes = statuses(&runnable, Status::Running); + // Re-validate defensively: the CLI rejects an invalid graph before it is + // pushed, but a hand-crafted push could still land one. Indices into + // `runnable`/`outcomes` rather than borrows, so outcomes stay mutable. + let ordered: Vec<usize> = match checks::order(&runnable) { + Ok(ordered) => ordered + .iter() + .filter_map(|check| runnable.iter().position(|c| c.name == check.name)) + .collect(), + Err(e) => { + finalize_error(&job.repo, job.new, &mut outcomes); + return Err(format!("invalid check set: {e}")); + } + }; let sprite = sprite_name(&job.repo); if let Err(e) = ensure_auth().and_then(|()| ensure_sprite(&sprite)) { finalize_error(&job.repo, job.new, &mut outcomes); @@ -214,18 +231,70 @@ return Err(e); } - for (index, check) in runnable.iter().enumerate() { - let result = run_one(&sprite, check); - if let Some(outcome) = outcomes.get_mut(index) { - outcome.status = result.status; - outcome.duration_secs = Some(result.duration_secs); - outcome.recording = Some(result.recording); + for index in ordered { + let Some(check) = runnable.get(index) else { + continue; + }; + // Topological order guarantees every dependency settled already. + let deps: Vec<Status> = check + .depends + .iter() + .filter_map(|dep| { + outcomes + .iter() + .find(|outcome| outcome.name == *dep) + .map(|outcome| outcome.status) + }) + .collect(); + let all_pass = deps.iter().all(|status| *status == Status::Pass); + match &check.command { + Some(command) if all_pass => { + let result = run_one(&sprite, &check.name, command); + if let Some(outcome) = outcomes.get_mut(index) { + outcome.status = result.status; + outcome.duration_secs = Some(result.duration_secs); + outcome.recording = Some(result.recording); + } + } + Some(_) => { + eprintln!("checks: SKIP {} (a dependency did not pass)", check.name); + if let Some(outcome) = outcomes.get_mut(index) { + outcome.status = Status::Skipped; + } + } + None => { + let status = derive_composite(&deps); + eprintln!( + "checks: {} {} (composite)", + status.to_string().to_uppercase(), + check.name + ); + if let Some(outcome) = outcomes.get_mut(index) { + outcome.status = status; + } + } } advance(&job.repo, job.new, &outcomes); } Ok(()) } +/// A composite check's status, derived from its dependencies' settled +/// statuses: `pass` when everything passed, `fail` when anything failed or +/// errored, `skipped` when nothing failed but something was skipped. +fn derive_composite(deps: &[Status]) -> Status { + if deps.iter().all(|status| *status == Status::Pass) { + Status::Pass + } else if deps + .iter() + .any(|status| matches!(status, Status::Fail | Status::Error)) + { + Status::Fail + } else { + Status::Skipped + } +} + /// Advance the recorded run for `new` to `outcomes`; a recording hiccup is /// logged but never derails the worker. fn advance(repo: &Path, new: ObjectId, outcomes: &[RunOutcome]) { @@ -444,35 +513,23 @@ /// exactly what a developer running the check by hand would see — and logging /// a `PASS`/`FAIL` line. Returns its outcome; a check that exceeds /// [`CHECK_TIMEOUT`] or cannot be captured is [`Status::Error`]. -fn run_one(sprite: &str, check: &Check) -> RunResult { +fn run_one(sprite: &str, name: &str, command: &str) -> RunResult { let start = Instant::now(); let pair = match native_pty_system().openpty(CHECK_PTY_SIZE) { Ok(pair) => pair, Err(e) => { - eprintln!( - "checks: ERROR {} (could not allocate a pty: {e})", - check.name - ); + eprintln!("checks: ERROR {name} (could not allocate a pty: {e})"); return finish(Status::Error, start, &[]); } }; let mut cmd = CommandBuilder::new("sprite"); cmd.args([ - "exec", - "--tty", - "-s", - sprite, - "--dir", - WORKDIR, - "--", - "sh", - "-c", - &check.command, + "exec", "--tty", "-s", sprite, "--dir", WORKDIR, "--", "sh", "-c", command, ]); let mut child = match pair.slave.spawn_command(cmd) { Ok(child) => child, Err(e) => { - eprintln!("checks: ERROR {} (could not run: {e})", check.name); + eprintln!("checks: ERROR {name} (could not run: {e})"); return finish(Status::Error, start, &[]); } }; @@ -482,7 +539,7 @@ let master = pair.master; let Ok(mut reader) = master.try_clone_reader() else { - eprintln!("checks: ERROR {} (could not read the pty)", check.name); + eprintln!("checks: ERROR {name} (could not read the pty)"); let _killed = child.kill(); return finish(Status::Error, start, &[]); }; @@ -524,10 +581,7 @@ drop(master); if timed_out { - eprintln!( - "checks: ERROR {} (timed out after {CHECK_TIMEOUT:?})", - check.name - ); + eprintln!("checks: ERROR {name} (timed out after {CHECK_TIMEOUT:?})"); let _killed = child.kill(); return finish(Status::Error, start, &events); } @@ -535,19 +589,16 @@ let status = match child.wait() { Ok(status) => status, Err(e) => { - eprintln!( - "checks: ERROR {} (could not wait on the sprite CLI: {e})", - check.name - ); + eprintln!("checks: ERROR {name} (could not wait on the sprite CLI: {e})"); return finish(Status::Error, start, &events); } }; if status.success() { - eprintln!("checks: PASS {}", check.name); + eprintln!("checks: PASS {name}"); finish(Status::Pass, start, &events) } else { - eprintln!("checks: FAIL {} ({})", check.name, check.command); + eprintln!("checks: FAIL {name} ({command})"); finish(Status::Fail, start, &events) } } @@ -624,6 +675,29 @@ assert_eq!(refs, vec!["refs/heads/main", "refs/heads/feature"]); } + #[test] + fn composite_status_derives_from_its_dependencies() { + assert_eq!( + derive_composite(&[Status::Pass, Status::Pass]), + Status::Pass + ); + assert_eq!( + derive_composite(&[Status::Pass, Status::Fail]), + Status::Fail + ); + assert_eq!( + derive_composite(&[Status::Error, Status::Skipped]), + Status::Fail + ); + assert_eq!( + derive_composite(&[Status::Pass, Status::Skipped]), + Status::Skipped + ); + // Vacuously all-pass: a composite with no dependencies never validates, + // but the derivation itself is total. + assert_eq!(derive_composite(&[]), Status::Pass); + } + #[test] fn pending_jobs_groups_by_repo_and_drops_malformed() { let queue = tempfile::tempdir().unwrap();
crates/git-ents/src/checks.rs @@ -14,9 +14,12 @@ //! `checks/<name>` and `results/<name>` moved from bare blobs to subtrees //! (`CheckBody`/[`Outcome`]) so a run's outcome can carry more than one field //! (a duration, a log URL), and a run's [`Status`] moved from a bare string to -//! a closed enum. Each is an incompatible format change: data written in a -//! prior layout no longer loads and must be re-recorded. Acceptable pre-1.0 -//! (see the format compatibility rules in `git_store`'s module docs). +//! a closed enum. [`CheckBody::command`] then moved from a required blob to an +//! `Option` subtree when checks gained `image` and `depends`, so a composite +//! check can exist without a command. Each is an incompatible format change: +//! data written in a prior layout no longer loads and must be re-recorded. +//! Acceptable pre-1.0 (see the format compatibility rules in `git_store`'s +//! module docs). use std::path::Path; @@ -30,8 +33,14 @@ /// identity, so it is not duplicated inside the body. #[derive(Debug, Clone, PartialEq, Eq, Facet)] struct CheckBody { - /// The shell command run for the check (e.g. `cargo fmt --check`). - command: String, + /// The shell command run for the check (e.g. `cargo fmt --check`), or + /// `None` for a composite check that only aggregates its `depends`. + command: Option<String>, + /// The sandbox image the command runs in; `None` uses the default. + image: Option<String>, + /// Names of sibling checks that must pass before this one runs. Stored as + /// `None` when empty so an independent check stays a minimal tree. + depends: Option<Vec<String>>, } /// One configured check, assembled from its map key and [`CheckBody`] at load. @@ -39,8 +48,13 @@ pub struct Check { /// The name it is stored under. pub name: String, - /// The shell command run for the check (e.g. `cargo fmt --check`). - pub command: String, + /// The shell command run for the check (e.g. `cargo fmt --check`), or + /// `None` for a composite check that only aggregates its dependencies. + pub command: Option<String>, + /// The sandbox image the command runs in; `None` uses the default. + pub image: Option<String>, + /// Names of sibling checks that must pass before this one runs. + pub depends: Vec<String>, } /// Load the configured checks recorded at [`CHECKS_REF`] in `repo`. @@ -52,6 +66,8 @@ git_store::Store::open(repo)?.load_map(CHECKS_REF, |name, body: CheckBody| Check { name, command: body.command, + image: body.image, + depends: body.depends.unwrap_or_default(), }) } @@ -66,6 +82,12 @@ check.name.clone(), CheckBody { command: check.command.clone(), + image: check.image.clone(), + depends: if check.depends.is_empty() { + None + } else { + Some(check.depends.clone()) + }, }, ) }, @@ -73,6 +95,85 @@ ) } +/// Validate `checks` as a static dependency graph and return them in an order +/// that runs every check after its dependencies — Kahn's topological sort, +/// with ties broken by name so the order is deterministic. +/// +/// Rejected here, at write time, so the worker only ever walks a fixed order: +/// a `depends` entry naming no configured check, a duplicate or self edge, a +/// check with neither a command nor dependencies, and any dependency cycle +/// (reported with its member names). A check that sets an `image` is also +/// rejected until the Sprite sandbox can honor one — the field exists in the +/// format now so supporting it later is not a data migration. +pub fn order(checks: &[Check]) -> Result<Vec<&Check>, String> { + let mut by_name: std::collections::BTreeMap<&str, &Check> = std::collections::BTreeMap::new(); + for check in checks { + if by_name.insert(check.name.as_str(), check).is_some() { + return Err(format!("check {} is defined twice", check.name)); + } + } + let mut blocking: std::collections::BTreeMap<&str, usize> = std::collections::BTreeMap::new(); + for check in checks { + if check.command.is_none() && check.depends.is_empty() { + return Err(format!( + "check {} has neither a command nor dependencies", + check.name + )); + } + if check.image.is_some() { + return Err(format!( + "check {} sets an image, which the checks sandbox does not support yet", + check.name + )); + } + let mut seen = std::collections::BTreeSet::new(); + for dep in &check.depends { + if !by_name.contains_key(dep.as_str()) { + return Err(format!( + "check {} depends on unknown check {dep}", + check.name + )); + } + if dep == &check.name { + return Err(format!("check {} depends on itself", check.name)); + } + if !seen.insert(dep.as_str()) { + return Err(format!("check {} lists dependency {dep} twice", check.name)); + } + } + blocking.insert(check.name.as_str(), check.depends.len()); + } + + let mut ordered = Vec::with_capacity(checks.len()); + while ordered.len() < checks.len() { + let ready: Vec<&str> = blocking + .iter() + .filter_map(|(name, blockers)| (*blockers == 0).then_some(*name)) + .collect(); + if ready.is_empty() { + let cycle: Vec<&str> = blocking.keys().copied().collect(); + return Err(format!( + "check dependencies form a cycle: {}", + cycle.join(", ") + )); + } + for name in ready { + let _ready = blocking.remove(name); + if let Some(check) = by_name.get(name) { + ordered.push(*check); + } + for (blocked, blockers) in blocking.iter_mut() { + if let Some(check) = by_name.get(blocked) + && check.depends.iter().any(|dep| dep == name) + { + *blockers = blockers.saturating_sub(1); + } + } + } + } + Ok(ordered) +} + /// The namespace under which a commit's check runs are recorded: one ref, /// `refs/meta/runs/<commit>`, per checked commit, holding the *log* of every /// run against it. Definitions live on [`CHECKS_REF`]; this is their history. @@ -95,6 +196,8 @@ /// An infrastructure failure (an unreachable sandbox, a timeout) kept the /// check from completing. Error, + /// The check never ran because a dependency did not pass. + Skipped, } impl std::fmt::Display for Status { @@ -105,6 +208,7 @@ Self::Pass => "pass", Self::Fail => "fail", Self::Error => "error", + Self::Skipped => "skipped", }) } } @@ -270,7 +374,25 @@ fn check(name: &str, command: &str) -> Check { Check { name: name.to_owned(), - command: command.to_owned(), + command: Some(command.to_owned()), + image: None, + depends: Vec::new(), + } + } + + fn composite(name: &str, depends: &[&str]) -> Check { + Check { + name: name.to_owned(), + command: None, + image: None, + depends: depends.iter().map(|dep| (*dep).to_owned()).collect(), + } + } + + fn dependent(name: &str, command: &str, depends: &[&str]) -> Check { + Check { + depends: depends.iter().map(|dep| (*dep).to_owned()).collect(), + ..check(name, command) } } @@ -310,10 +432,11 @@ #[test] fn loads_the_on_disk_checks_format() { - // A fixture written as the real `checks/<name>/command` subtree layout - // (a struct value, not a bare blob) must keep loading, guarding the - // checks document's shape against an incompatible change to data - // already on a ref. + // A fixture written as the real `checks/<name>/command/some` subtree + // layout (the `Option`-wrapped command, with `image`/`depends` omitted + // entirely) must keep loading, with the missing optional fields unset — + // guarding the checks document's shape against an incompatible change + // to data already on a ref. let repo = unique_repo(); write_checks_doc( &repo, @@ -449,5 +572,81 @@ fn displays_lowercase_status_words() { assert_eq!(Status::Queued.to_string(), "queued"); assert_eq!(Status::Pass.to_string(), "pass"); + assert_eq!(Status::Skipped.to_string(), "skipped"); + } + + #[test] + fn store_then_load_round_trips_image_and_depends() { + let repo = unique_repo(); + let written = vec![ + Check { + image: Some("rust:1.88".to_owned()), + ..check("fmt", "cargo fmt --check") + }, + dependent("test", "cargo nextest run", &["fmt"]), + composite("ci", &["fmt", "test"]), + ]; + store(&repo, &written).unwrap(); + let mut loaded = load(&repo).unwrap(); + loaded.sort_by(|a, b| a.name.cmp(&b.name)); + let mut expected = written; + expected.sort_by(|a, b| a.name.cmp(&b.name)); + assert_eq!(loaded, expected); + let _ = std::fs::remove_dir_all(&repo); + } + + #[test] + fn order_runs_dependencies_first() { + let checks = vec![ + composite("ci", &["test", "fmt"]), + dependent("test", "cargo nextest run", &["fmt"]), + check("fmt", "cargo fmt --check"), + ]; + let names: Vec<&str> = order(&checks) + .unwrap() + .iter() + .map(|c| c.name.as_str()) + .collect(); + assert_eq!(names, vec!["fmt", "test", "ci"]); + } + + #[test] + fn order_rejects_a_cycle() { + let checks = vec![ + dependent("a", "true", &["b"]), + dependent("b", "true", &["a"]), + check("fmt", "cargo fmt --check"), + ]; + let err = order(&checks).unwrap_err(); + assert!(err.contains("cycle"), "unexpected error: {err}"); + assert!(err.contains('a') && err.contains('b')); + } + + #[test] + fn order_rejects_an_unknown_dependency() { + let checks = vec![dependent("test", "cargo nextest run", &["fmt"])]; + let err = order(&checks).unwrap_err(); + assert!(err.contains("unknown check fmt"), "unexpected error: {err}"); + } + + #[test] + fn order_rejects_self_and_duplicate_edges() { + let selfish = vec![dependent("a", "true", &["a"])]; + assert!(order(&selfish).unwrap_err().contains("itself")); + let doubled = vec![ + check("fmt", "true"), + dependent("a", "true", &["fmt", "fmt"]), + ]; + assert!(order(&doubled).unwrap_err().contains("twice")); + } + + #[test] + fn order_rejects_an_empty_check() { + let checks = vec![composite("hollow", &[])]; + let err = order(&checks).unwrap_err(); + assert!( + err.contains("neither a command nor dependencies"), + "unexpected error: {err}" + ); } }
crates/git-ents/src/main.rs @@ -187,8 +187,16 @@ Add { /// Name to record the check under (`checks/<name>`). name: Option<String>, - /// Command the check runs (e.g. `cargo fmt --check`). + /// Command the check runs (e.g. `cargo fmt --check`); omit for a + /// composite check that only aggregates its dependencies. command: Option<String>, + /// Sandbox image the command runs in (reserved: the Sprite sandbox + /// does not honor an image yet, so setting one is rejected). + #[arg(long)] + image: Option<String>, + /// Check that must pass before this one runs (repeatable). + #[arg(long = "depends", value_name = "CHECK")] + depends: Vec<String>, /// Remote whose `refs/meta/checks` to update. #[arg(default_value = "origin")] remote: String, @@ -342,8 +350,10 @@ ChecksAction::Add { name, command, + image, + depends, remote, - } => add_check(name, command, &remote), + } => add_check(name, command, image, depends, &remote), ChecksAction::Remove { name, remote } => remove::<Checks>(&name, &remote), ChecksAction::Debug { remote } => checks_debug(&remote), ChecksAction::Runs { remote } => checks_runs(&remote), @@ -627,7 +637,17 @@ } fn value(item: &Check) -> String { - item.command.clone() + let mut value = item + .command + .clone() + .unwrap_or_else(|| "(composite)".to_owned()); + if let Some(image) = &item.image { + value.push_str(&format!(" [image: {image}]")); + } + if !item.depends.is_empty() { + value.push_str(&format!(" [needs: {}]", item.depends.join(", "))); + } + value } } @@ -684,11 +704,27 @@ } /// Add `name` running `command` to `remote`'s set, replacing any check already -/// recorded under that name, and push the update. Prompts for either field -/// left `None` when run at an interactive terminal. -fn add_check(name: Option<String>, command: Option<String>, remote: &str) -> Result<(), String> { +/// recorded under that name, and push the update. Prompts for any field left +/// unset when run at an interactive terminal. The whole set is validated as a +/// dependency graph (`checks::order`) before it is stored, so a cycle or a +/// dangling dependency never lands on the remote. +fn add_check( + name: Option<String>, + command: Option<String>, + image: Option<String>, + depends: Vec<String>, + remote: &str, +) -> Result<(), String> { let name = interactive::text_or(name, "Check name")?; - let command = interactive::text_or(command, "Command")?; + let command = interactive::optional_text_or(command, "Command (empty for a composite)")?; + let depends = if depends.is_empty() { + parse_depends(interactive::optional_text_or( + None, + "Depends on (comma-separated, empty for none)", + )?) + } else { + depends + }; let repo = repo()?; let expected = sync(remote, CHECKS_REF)?; let mut checks = checks::load(&repo).map_err(|error| error.to_string())?; @@ -696,13 +732,30 @@ checks.push(Check { name: name.clone(), command, + image, + depends, }); + let _ordered = checks::order(&checks)?; checks::store(&repo, &checks).map_err(|error| error.to_string())?; push_signed(remote, CHECKS_REF, expected.as_deref())?; println!("recorded check {name}"); Ok(()) } +/// Split an interactive comma-separated dependency reply into names, dropping +/// empty segments; `None` (no reply) is no dependencies. +fn parse_depends(reply: Option<String>) -> Vec<String> { + reply + .map(|value| { + value + .split(',') + .map(|name| name.trim().to_owned()) + .filter(|name| !name.is_empty()) + .collect() + }) + .unwrap_or_default() +} + /// Set this machine up to produce the signed pushes the server requires: /// ensure a signing key exists, then record the SSH signing config /// (SSH-format signatures, the key, and "sign when the server asks" so pushes
crates/git-ents/src/testutil.rs @@ -286,18 +286,24 @@ /// Lay the checks document out at [`crate::checks::CHECKS_REF`] as the real /// on-disk format: a bare scalar-keyed map at the ref's tree root, one -/// `<name>/command` blob per configured check (the map value is a `CheckBody` -/// subtree, not a bare blob, and the map itself is the whole document — no -/// wrapper struct). Asserts the loader still reads the format independent of -/// the writer. +/// `<name>/command/some` blob per configured check (the map value is a +/// `CheckBody` subtree whose `command` is the `Option` tree encoding, and the +/// map itself is the whole document — no wrapper struct), with the optional +/// `image`/`depends` fields omitted entirely — asserting the loader fills a +/// check's missing optional fields as unset, independent of the writer. pub(crate) fn write_checks_doc(repo: &Path, checks: &[(&str, &str)]) { let mut entries = String::new(); for (name, command) in checks { let command_blob = git_with_stdin(repo, &["hash-object", "-w", "--stdin"], command); + let some_tree = git_with_stdin( + repo, + &["mktree"], + &format!("100644 blob {command_blob}\tsome\n"), + ); let check_tree = git_with_stdin( repo, &["mktree"], - &format!("100644 blob {command_blob}\tcommand\n"), + &format!("040000 tree {some_tree}\tcommand\n"), ); entries.push_str(&format!("040000 tree {check_tree}\t{name}\n")); }
crates/git-ents-server/src/web/render.rs @@ -26,8 +26,25 @@ } } -/// A check renders structurally: its name is the key, its command the value. -impl Render for Check {} +/// A check's name is the key and its command the value — `(composite)` for a +/// check with none — with its image and dependencies appended as ` · `-joined +/// annotations rather than the raw `Option`/`Vec` the structural walk would +/// print. +impl Render for Check { + fn render(&self) -> Markup { + let mut value = self + .command + .clone() + .unwrap_or_else(|| "(composite)".to_owned()); + if let Some(image) = &self.image { + value.push_str(&format!(" · image {image}")); + } + if !self.depends.is_empty() { + value.push_str(&format!(" · needs {}", self.depends.join(", "))); + } + row(&self.name, &value) + } +} /// Config renders structurally: each field becomes a keyed row. impl Render for Config {}