From 7d9009adef2cf7ffec5aab34504d607b5f7da366 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 16:04:48 +0000 Subject: [PATCH 1/7] Split core's public-API snapshot, and give the runner one of its own The one snapshot covered two different promises at once: the frozen `devlaunch_core::api` tier, where any diff is a breaking change, and the binary surface that is reachable but never promised. A diff of nine hundred rows of internal churn with one removed `api` function in it reads as routine, so the promise stopped being checked in practice. Three files now: `public-api.api.txt` (the promise, 37 rows), `public-api.rest.txt` (the tripwire), and `devlaunch-runner/public-api.txt`, whose surface until now entered core's snapshot as a single unexpanded glob row -- so a removed `Runner` method moved nothing and passed CI. `scripts/public-api-snapshots.sh` owns the classification and the pins; tests in both crates hold the checked-in files to it. --- rust/devlaunch-core/public-api.api.txt | 37 ++++ .../{public-api.txt => public-api.rest.txt} | 37 ---- .../tests/public_api_snapshots.rs | 99 +++++++++ rust/devlaunch-runner/public-api.txt | 202 ++++++++++++++++++ .../tests/public_api_snapshot.rs | 70 ++++++ scripts/public-api-snapshots.sh | 97 +++++++++ 6 files changed, 505 insertions(+), 37 deletions(-) create mode 100644 rust/devlaunch-core/public-api.api.txt rename rust/devlaunch-core/{public-api.txt => public-api.rest.txt} (98%) create mode 100644 rust/devlaunch-core/tests/public_api_snapshots.rs create mode 100644 rust/devlaunch-runner/public-api.txt create mode 100644 rust/devlaunch-runner/tests/public_api_snapshot.rs create mode 100755 scripts/public-api-snapshots.sh diff --git a/rust/devlaunch-core/public-api.api.txt b/rust/devlaunch-core/public-api.api.txt new file mode 100644 index 00000000..12764ab0 --- /dev/null +++ b/rust/devlaunch-core/public-api.api.txt @@ -0,0 +1,37 @@ +pub mod devlaunch_core::api +pub enum devlaunch_core::api::LaunchVerb +pub devlaunch_core::api::LaunchVerb::Attach +pub devlaunch_core::api::LaunchVerb::Attach::command: core::option::Option +pub devlaunch_core::api::LaunchVerb::Code +pub devlaunch_core::api::LaunchVerb::Dotfiles +pub devlaunch_core::api::LaunchVerb::Recreate +pub devlaunch_core::api::LaunchVerb::Reset +pub devlaunch_core::api::LaunchVerb::Restart +pub devlaunch_core::api::LaunchVerb::Up +pub enum devlaunch_core::api::SpecIdentity<'a> +pub devlaunch_core::api::SpecIdentity::ExistingName(&'a str) +pub devlaunch_core::api::SpecIdentity::PathLeaf(&'a str) +pub devlaunch_core::api::SpecIdentity::RepoLabel(alloc::string::String) +pub devlaunch_core::api::SpecIdentity::Workspace(alloc::string::String) +pub enum devlaunch_core::api::WorkspaceSpec<'a> +pub devlaunch_core::api::WorkspaceSpec::ExistingIdOrName(&'a str) +pub devlaunch_core::api::WorkspaceSpec::HostPath(&'a str) +pub devlaunch_core::api::WorkspaceSpec::OwnerRepo +pub devlaunch_core::api::WorkspaceSpec::OwnerRepo::branch: core::option::Option<&'a str> +pub devlaunch_core::api::WorkspaceSpec::OwnerRepo::owner: &'a str +pub devlaunch_core::api::WorkspaceSpec::OwnerRepo::repo: &'a str +pub devlaunch_core::api::WorkspaceSpec::Path(&'a str) +pub devlaunch_core::api::WorkspaceSpec::SshUrl(&'a str) +pub devlaunch_core::api::WorkspaceSpec::Url(&'a str) +pub struct devlaunch_core::api::CommandContext<'r> +pub struct devlaunch_core::api::DevcontainerPath(_) +pub struct devlaunch_core::api::Launch<'a, 'r, 'l> +pub const devlaunch_core::api::HANDOFF_VAR: &str +pub const devlaunch_core::api::PREWARM_VAR: &str +pub fn devlaunch_core::api::enriched_listing(&mut devlaunch_core::flows::listing::CommandContext<'_>, &devlaunch_core::flows::listing::DlView<'_>, devlaunch_core::flows::listing::Sizes) -> core::result::Result, devlaunch_core::clients::devpod::ListingUnreadable> +pub fn devlaunch_core::api::identity(&str) -> core::result::Result, devlaunch_core::domain::workspace_id::UnsafeName> +pub fn devlaunch_core::api::json_document(&[devlaunch_core::flows::listing::ListedWorkspace]) -> serde_json::value::Value +pub fn devlaunch_core::api::parse(&str) -> devlaunch_core::domain::spec::WorkspaceSpec<'_> +pub fn devlaunch_core::api::resolve_devcontainer_ref(&str) -> core::result::Result +pub fn devlaunch_core::api::workspace_delete(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, core::option::Option<&std::path::Path>, &str, devlaunch_core::flows::lifecycle::Insistence, &mut dyn devlaunch_core::notices::Notices) -> core::result::Result +pub fn devlaunch_core::api::workspace_stop(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &str) -> core::result::Result diff --git a/rust/devlaunch-core/public-api.txt b/rust/devlaunch-core/public-api.rest.txt similarity index 98% rename from rust/devlaunch-core/public-api.txt rename to rust/devlaunch-core/public-api.rest.txt index a861eff6..aa56fa98 100644 --- a/rust/devlaunch-core/public-api.txt +++ b/rust/devlaunch-core/public-api.rest.txt @@ -1,14 +1,4 @@ pub mod devlaunch_core -pub mod devlaunch_core::api -pub enum devlaunch_core::api::LaunchVerb -pub devlaunch_core::api::LaunchVerb::Attach -pub devlaunch_core::api::LaunchVerb::Attach::command: core::option::Option -pub devlaunch_core::api::LaunchVerb::Code -pub devlaunch_core::api::LaunchVerb::Dotfiles -pub devlaunch_core::api::LaunchVerb::Recreate -pub devlaunch_core::api::LaunchVerb::Reset -pub devlaunch_core::api::LaunchVerb::Restart -pub devlaunch_core::api::LaunchVerb::Up impl core::clone::Clone for devlaunch_core::flows::launch::LaunchVerb pub fn devlaunch_core::flows::launch::LaunchVerb::clone(&self) -> devlaunch_core::flows::launch::LaunchVerb impl core::cmp::Eq for devlaunch_core::flows::launch::LaunchVerb @@ -17,11 +7,6 @@ pub fn devlaunch_core::flows::launch::LaunchVerb::eq(&self, &devlaunch_core::flo impl core::fmt::Debug for devlaunch_core::flows::launch::LaunchVerb pub fn devlaunch_core::flows::launch::LaunchVerb::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::flows::launch::LaunchVerb -pub enum devlaunch_core::api::SpecIdentity<'a> -pub devlaunch_core::api::SpecIdentity::ExistingName(&'a str) -pub devlaunch_core::api::SpecIdentity::PathLeaf(&'a str) -pub devlaunch_core::api::SpecIdentity::RepoLabel(alloc::string::String) -pub devlaunch_core::api::SpecIdentity::Workspace(alloc::string::String) impl<'a> core::clone::Clone for devlaunch_core::domain::spec::SpecIdentity<'a> pub fn devlaunch_core::domain::spec::SpecIdentity<'a>::clone(&self) -> devlaunch_core::domain::spec::SpecIdentity<'a> impl<'a> core::cmp::Eq for devlaunch_core::domain::spec::SpecIdentity<'a> @@ -30,16 +15,6 @@ pub fn devlaunch_core::domain::spec::SpecIdentity<'a>::eq(&self, &devlaunch_core impl<'a> core::fmt::Debug for devlaunch_core::domain::spec::SpecIdentity<'a> pub fn devlaunch_core::domain::spec::SpecIdentity<'a>::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl<'a> core::marker::StructuralPartialEq for devlaunch_core::domain::spec::SpecIdentity<'a> -pub enum devlaunch_core::api::WorkspaceSpec<'a> -pub devlaunch_core::api::WorkspaceSpec::ExistingIdOrName(&'a str) -pub devlaunch_core::api::WorkspaceSpec::HostPath(&'a str) -pub devlaunch_core::api::WorkspaceSpec::OwnerRepo -pub devlaunch_core::api::WorkspaceSpec::OwnerRepo::branch: core::option::Option<&'a str> -pub devlaunch_core::api::WorkspaceSpec::OwnerRepo::owner: &'a str -pub devlaunch_core::api::WorkspaceSpec::OwnerRepo::repo: &'a str -pub devlaunch_core::api::WorkspaceSpec::Path(&'a str) -pub devlaunch_core::api::WorkspaceSpec::SshUrl(&'a str) -pub devlaunch_core::api::WorkspaceSpec::Url(&'a str) impl<'a> core::clone::Clone for devlaunch_core::domain::spec::WorkspaceSpec<'a> pub fn devlaunch_core::domain::spec::WorkspaceSpec<'a>::clone(&self) -> devlaunch_core::domain::spec::WorkspaceSpec<'a> impl<'a> core::cmp::Eq for devlaunch_core::domain::spec::WorkspaceSpec<'a> @@ -49,12 +24,10 @@ impl<'a> core::fmt::Debug for devlaunch_core::domain::spec::WorkspaceSpec<'a> pub fn devlaunch_core::domain::spec::WorkspaceSpec<'a>::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl<'a> core::marker::Copy for devlaunch_core::domain::spec::WorkspaceSpec<'a> impl<'a> core::marker::StructuralPartialEq for devlaunch_core::domain::spec::WorkspaceSpec<'a> -pub struct devlaunch_core::api::CommandContext<'r> impl<'r> devlaunch_core::flows::listing::CommandContext<'r> pub fn devlaunch_core::flows::listing::CommandContext<'r>::git(&self) -> devlaunch_core::clients::git::Git<'r> pub fn devlaunch_core::flows::listing::CommandContext<'r>::new(&'r dyn devlaunch_runner::Runner) -> Self pub fn devlaunch_core::flows::listing::CommandContext<'r>::workspaces(&mut self) -> core::result::Result, devlaunch_core::clients::devpod::ListingUnreadable> -pub struct devlaunch_core::api::DevcontainerPath(_) impl devlaunch_core::domain::spec::DevcontainerPath pub fn devlaunch_core::domain::spec::DevcontainerPath::as_str(&self) -> &str impl core::clone::Clone for devlaunch_core::domain::spec::DevcontainerPath @@ -65,19 +38,9 @@ pub fn devlaunch_core::domain::spec::DevcontainerPath::eq(&self, &devlaunch_core impl core::fmt::Debug for devlaunch_core::domain::spec::DevcontainerPath pub fn devlaunch_core::domain::spec::DevcontainerPath::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result impl core::marker::StructuralPartialEq for devlaunch_core::domain::spec::DevcontainerPath -pub struct devlaunch_core::api::Launch<'a, 'r, 'l> impl<'a, 'r, 'l> devlaunch_core::flows::launch::Launch<'a, 'r, 'l> pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::new(&'a mut devlaunch_core::flows::listing::CommandContext<'r>, &'a mut devlaunch_core::flows::lifecycle::Refresh<'l>, &'a mut dyn devlaunch_core::flows::launch::ColdMachinery<'r>, &'a dyn devlaunch_core::flows::launch::Provision, &'a devlaunch_core::flows::launch::Host, &'a mut dyn core::ops::function::FnMut(&str), &'a mut dyn devlaunch_core::notices::Notices) -> Self pub fn devlaunch_core::flows::launch::Launch<'a, 'r, 'l>::run(&mut self, &str, &devlaunch_core::flows::launch::LaunchVerb, core::option::Option<&devlaunch_core::domain::spec::DevcontainerPath>) -> core::result::Result -pub const devlaunch_core::api::HANDOFF_VAR: &str -pub const devlaunch_core::api::PREWARM_VAR: &str -pub fn devlaunch_core::api::enriched_listing(&mut devlaunch_core::flows::listing::CommandContext<'_>, &devlaunch_core::flows::listing::DlView<'_>, devlaunch_core::flows::listing::Sizes) -> core::result::Result, devlaunch_core::clients::devpod::ListingUnreadable> -pub fn devlaunch_core::api::identity(&str) -> core::result::Result, devlaunch_core::domain::workspace_id::UnsafeName> -pub fn devlaunch_core::api::json_document(&[devlaunch_core::flows::listing::ListedWorkspace]) -> serde_json::value::Value -pub fn devlaunch_core::api::parse(&str) -> devlaunch_core::domain::spec::WorkspaceSpec<'_> -pub fn devlaunch_core::api::resolve_devcontainer_ref(&str) -> core::result::Result -pub fn devlaunch_core::api::workspace_delete(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &devlaunch_core::flows::workspace_clone::WorkspaceCloneManager<'_>, &mut devlaunch_core::domain::metadata::MetadataStorage, core::option::Option<&std::path::Path>, &str, devlaunch_core::flows::lifecycle::Insistence, &mut dyn devlaunch_core::notices::Notices) -> core::result::Result -pub fn devlaunch_core::api::workspace_stop(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &str) -> core::result::Result pub mod devlaunch_core::clients pub mod devlaunch_core::clients::devpod pub enum devlaunch_core::clients::devpod::ContainerState diff --git a/rust/devlaunch-core/tests/public_api_snapshots.rs b/rust/devlaunch-core/tests/public_api_snapshots.rs new file mode 100644 index 00000000..8d3c1daf --- /dev/null +++ b/rust/devlaunch-core/tests/public_api_snapshots.rs @@ -0,0 +1,99 @@ +//! The two files core's public-API snapshot splits into, and the invariant that +//! keeps them worth reading. +//! +//! `public-api.api.txt` is the promise: every row is a declaration under +//! `devlaunch_core::api`, the surface an external consumer is entitled to +//! depend on, so a diff there is a breaking change by definition. +//! `public-api.rest.txt` is the tripwire: the binary surface, reachable but +//! never promised, regenerated freely whenever a refactor moves it. +//! +//! One snapshot for both tiers is what the split replaced, and the reason is +//! the signal: a diff that is nine hundred rows of internal churn and one +//! removed `api` function reads as routine, and the one row that mattered goes +//! through review unremarked. +//! +//! The classification lives in `scripts/public-api-snapshots.sh` and nowhere +//! else — the CI job runs that script rather than re-implementing its filter. +//! These tests are the other half of that: they hold the *checked-in* files to +//! the rule, so a hand-edited snapshot, or a regeneration that put the promise +//! in the file nobody reads, fails here rather than passing quietly. + +const PROMISE: &str = include_str!("../public-api.api.txt"); +const REST: &str = include_str!("../public-api.rest.txt"); + +/// The rows of a snapshot: `cargo public-api` writes one declaration per line +/// and nothing else, so a blank line is noise from an editor rather than API. +fn rows(snapshot: &str) -> Vec<&str> { + snapshot + .lines() + .map(str::trim_end) + .filter(|row| !row.is_empty()) + .collect() +} + +/// Whether a row declares something under `devlaunch_core::api`. +/// +/// The path has to end there or continue with `::` — the same boundary the +/// regeneration script's `devlaunch_core::api\b` means, spelled out because a +/// future `devlaunch_core::apiary` must not be mistaken for the promise. +fn names_api(row: &str) -> bool { + const PATH: &str = "devlaunch_core::api"; + row.match_indices(PATH).any(|(at, _)| { + !row[at + PATH.len()..] + .chars() + .next() + .is_some_and(|next| next.is_alphanumeric() || next == '_') + }) +} + +#[test] +fn every_promised_row_is_an_api_declaration() { + let strays: Vec<&str> = rows(PROMISE) + .into_iter() + .filter(|row| !names_api(row)) + .collect(); + assert!( + strays.is_empty(), + "public-api.api.txt is the frozen promise and holds only \ + devlaunch_core::api declarations; these rows are something else: {strays:#?}" + ); +} + +#[test] +fn no_row_of_the_rest_is_an_api_declaration() { + let promised: Vec<&str> = rows(REST).into_iter().filter(|row| names_api(row)).collect(); + assert!( + promised.is_empty(), + "these devlaunch_core::api declarations are in the freely regenerated \ + file, where a breaking change to them would read as routine churn: {promised:#?}" + ); +} + +#[test] +fn the_two_files_share_no_row() { + let promise = rows(PROMISE); + let duplicated: Vec<&str> = rows(REST) + .into_iter() + .filter(|row| promise.contains(row)) + .collect(); + assert!( + duplicated.is_empty(), + "the split is a partition: a row belongs to exactly one file, and these \ + are in both: {duplicated:#?}" + ); +} + +#[test] +fn each_file_is_anchored_on_a_row_every_generation_produces() { + // Two rows `cargo public-api -p devlaunch-core` cannot omit while the crate + // exists and still declares `pub mod api`. Their absence means a truncated + // or misdirected snapshot -- an empty file passes every filter test above. + assert!( + rows(PROMISE).contains(&"pub mod devlaunch_core::api"), + "the promise file does not contain the api module's own row" + ); + assert!( + rows(REST).contains(&"pub mod devlaunch_core"), + "the rest file does not contain the crate root's row" + ); +} diff --git a/rust/devlaunch-runner/public-api.txt b/rust/devlaunch-runner/public-api.txt new file mode 100644 index 00000000..3bb4d926 --- /dev/null +++ b/rust/devlaunch-runner/public-api.txt @@ -0,0 +1,202 @@ +pub mod devlaunch_runner +pub mod devlaunch_runner::interrupt +pub struct devlaunch_runner::interrupt::Registration +impl core::fmt::Debug for devlaunch_runner::interrupt::Registration +pub fn devlaunch_runner::interrupt::Registration::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::ops::drop::Drop for devlaunch_runner::interrupt::Registration +pub fn devlaunch_runner::interrupt::Registration::drop(&mut self) +pub unsafe fn devlaunch_runner::interrupt::cleanup_and_exit(i32) -> never +pub fn devlaunch_runner::interrupt::register_dir(&std::path::Path) -> core::option::Option +pub fn devlaunch_runner::interrupt::register_file(&std::path::Path) -> core::option::Option +pub enum devlaunch_runner::DetachOutcome +pub devlaunch_runner::DetachOutcome::NotStarted(devlaunch_runner::OsFailure) +pub devlaunch_runner::DetachOutcome::ProgramNotFound +pub devlaunch_runner::DetachOutcome::Started +pub devlaunch_runner::DetachOutcome::Started::pid: u32 +impl core::clone::Clone for devlaunch_runner::DetachOutcome +pub fn devlaunch_runner::DetachOutcome::clone(&self) -> devlaunch_runner::DetachOutcome +impl core::cmp::Eq for devlaunch_runner::DetachOutcome +impl core::cmp::PartialEq for devlaunch_runner::DetachOutcome +pub fn devlaunch_runner::DetachOutcome::eq(&self, &devlaunch_runner::DetachOutcome) -> bool +impl core::fmt::Debug for devlaunch_runner::DetachOutcome +pub fn devlaunch_runner::DetachOutcome::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::Copy for devlaunch_runner::DetachOutcome +impl core::marker::StructuralPartialEq for devlaunch_runner::DetachOutcome +pub enum devlaunch_runner::EnvBase +pub devlaunch_runner::EnvBase::Empty +pub devlaunch_runner::EnvBase::Parent +impl core::clone::Clone for devlaunch_runner::EnvBase +pub fn devlaunch_runner::EnvBase::clone(&self) -> devlaunch_runner::EnvBase +impl core::cmp::Eq for devlaunch_runner::EnvBase +impl core::cmp::PartialEq for devlaunch_runner::EnvBase +pub fn devlaunch_runner::EnvBase::eq(&self, &devlaunch_runner::EnvBase) -> bool +impl core::default::Default for devlaunch_runner::EnvBase +pub fn devlaunch_runner::EnvBase::default() -> devlaunch_runner::EnvBase +impl core::fmt::Debug for devlaunch_runner::EnvBase +pub fn devlaunch_runner::EnvBase::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::Copy for devlaunch_runner::EnvBase +impl core::marker::StructuralPartialEq for devlaunch_runner::EnvBase +pub enum devlaunch_runner::Exit +pub devlaunch_runner::Exit::Code(i32) +pub devlaunch_runner::Exit::Signal(i32) +impl devlaunch_runner::Exit +pub fn devlaunch_runner::Exit::is_success(self) -> bool +impl core::clone::Clone for devlaunch_runner::Exit +pub fn devlaunch_runner::Exit::clone(&self) -> devlaunch_runner::Exit +impl core::cmp::Eq for devlaunch_runner::Exit +impl core::cmp::PartialEq for devlaunch_runner::Exit +pub fn devlaunch_runner::Exit::eq(&self, &devlaunch_runner::Exit) -> bool +impl core::fmt::Debug for devlaunch_runner::Exit +pub fn devlaunch_runner::Exit::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::Copy for devlaunch_runner::Exit +impl core::marker::StructuralPartialEq for devlaunch_runner::Exit +pub enum devlaunch_runner::Outcome +pub devlaunch_runner::Outcome::NotStarted(devlaunch_runner::OsFailure) +pub devlaunch_runner::Outcome::ProgramNotFound +pub devlaunch_runner::Outcome::Ran +pub devlaunch_runner::Outcome::Ran::exit: devlaunch_runner::Exit +pub devlaunch_runner::Outcome::Ran::io: T +pub devlaunch_runner::Outcome::TimedOut +impl devlaunch_runner::Outcome +pub fn devlaunch_runner::Outcome::succeeded(&self) -> bool +impl core::clone::Clone for devlaunch_runner::Outcome +pub fn devlaunch_runner::Outcome::clone(&self) -> devlaunch_runner::Outcome +impl core::cmp::Eq for devlaunch_runner::Outcome +impl core::cmp::PartialEq for devlaunch_runner::Outcome +pub fn devlaunch_runner::Outcome::eq(&self, &devlaunch_runner::Outcome) -> bool +impl core::marker::StructuralPartialEq for devlaunch_runner::Outcome +impl core::fmt::Debug for devlaunch_runner::Outcome +pub fn devlaunch_runner::Outcome::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +pub enum devlaunch_runner::StdinPlan +pub devlaunch_runner::StdinPlan::File(std::path::PathBuf) +pub devlaunch_runner::StdinPlan::Inherit +pub devlaunch_runner::StdinPlan::Null +impl core::clone::Clone for devlaunch_runner::StdinPlan +pub fn devlaunch_runner::StdinPlan::clone(&self) -> devlaunch_runner::StdinPlan +impl core::cmp::Eq for devlaunch_runner::StdinPlan +impl core::cmp::PartialEq for devlaunch_runner::StdinPlan +pub fn devlaunch_runner::StdinPlan::eq(&self, &devlaunch_runner::StdinPlan) -> bool +impl core::default::Default for devlaunch_runner::StdinPlan +pub fn devlaunch_runner::StdinPlan::default() -> devlaunch_runner::StdinPlan +impl core::fmt::Debug for devlaunch_runner::StdinPlan +pub fn devlaunch_runner::StdinPlan::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_runner::StdinPlan +pub struct devlaunch_runner::CapturedText +pub devlaunch_runner::CapturedText::stderr: alloc::string::String +pub devlaunch_runner::CapturedText::stdout: alloc::string::String +impl core::clone::Clone for devlaunch_runner::CapturedText +pub fn devlaunch_runner::CapturedText::clone(&self) -> devlaunch_runner::CapturedText +impl core::cmp::Eq for devlaunch_runner::CapturedText +impl core::cmp::PartialEq for devlaunch_runner::CapturedText +pub fn devlaunch_runner::CapturedText::eq(&self, &devlaunch_runner::CapturedText) -> bool +impl core::default::Default for devlaunch_runner::CapturedText +pub fn devlaunch_runner::CapturedText::default() -> devlaunch_runner::CapturedText +impl core::fmt::Debug for devlaunch_runner::CapturedText +pub fn devlaunch_runner::CapturedText::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_runner::CapturedText +pub struct devlaunch_runner::EnvSpec +pub devlaunch_runner::EnvSpec::base: devlaunch_runner::EnvBase +pub devlaunch_runner::EnvSpec::entries: alloc::collections::btree::map::BTreeMap +impl devlaunch_runner::EnvSpec +pub fn devlaunch_runner::EnvSpec::and(self, impl core::convert::Into, impl core::convert::Into) -> Self +pub fn devlaunch_runner::EnvSpec::empty() -> Self +pub fn devlaunch_runner::EnvSpec::inherited() -> Self +impl core::clone::Clone for devlaunch_runner::EnvSpec +pub fn devlaunch_runner::EnvSpec::clone(&self) -> devlaunch_runner::EnvSpec +impl core::cmp::Eq for devlaunch_runner::EnvSpec +impl core::cmp::PartialEq for devlaunch_runner::EnvSpec +pub fn devlaunch_runner::EnvSpec::eq(&self, &devlaunch_runner::EnvSpec) -> bool +impl core::default::Default for devlaunch_runner::EnvSpec +pub fn devlaunch_runner::EnvSpec::default() -> devlaunch_runner::EnvSpec +impl core::fmt::Debug for devlaunch_runner::EnvSpec +pub fn devlaunch_runner::EnvSpec::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_runner::EnvSpec +pub struct devlaunch_runner::Invocation +pub devlaunch_runner::Invocation::args: alloc::vec::Vec +pub devlaunch_runner::Invocation::cwd: core::option::Option +pub devlaunch_runner::Invocation::env: devlaunch_runner::EnvSpec +pub devlaunch_runner::Invocation::program: alloc::string::String +impl devlaunch_runner::Invocation +pub fn devlaunch_runner::Invocation::argv(&self) -> alloc::vec::Vec +pub fn devlaunch_runner::Invocation::new(impl core::convert::Into) -> Self +pub fn devlaunch_runner::Invocation::with_arg(self, impl core::convert::Into) -> Self +pub fn devlaunch_runner::Invocation::with_args(self, I) -> Self where I: core::iter::traits::collect::IntoIterator, S: core::convert::Into +pub fn devlaunch_runner::Invocation::with_cwd(self, impl core::convert::Into) -> Self +pub fn devlaunch_runner::Invocation::with_env(self, devlaunch_runner::EnvSpec) -> Self +pub fn devlaunch_runner::Invocation::with_var(self, impl core::convert::Into, impl core::convert::Into) -> Self +impl core::clone::Clone for devlaunch_runner::Invocation +pub fn devlaunch_runner::Invocation::clone(&self) -> devlaunch_runner::Invocation +impl core::cmp::Eq for devlaunch_runner::Invocation +impl core::cmp::PartialEq for devlaunch_runner::Invocation +pub fn devlaunch_runner::Invocation::eq(&self, &devlaunch_runner::Invocation) -> bool +impl core::convert::From for devlaunch_runner::SpawnSpec +pub fn devlaunch_runner::SpawnSpec::from(devlaunch_runner::Invocation) -> Self +impl core::default::Default for devlaunch_runner::Invocation +pub fn devlaunch_runner::Invocation::default() -> devlaunch_runner::Invocation +impl core::fmt::Debug for devlaunch_runner::Invocation +pub fn devlaunch_runner::Invocation::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_runner::Invocation +pub struct devlaunch_runner::OsFailure +pub devlaunch_runner::OsFailure::errno: core::option::Option +pub devlaunch_runner::OsFailure::kind: core::io::error::ErrorKind +impl core::clone::Clone for devlaunch_runner::OsFailure +pub fn devlaunch_runner::OsFailure::clone(&self) -> devlaunch_runner::OsFailure +impl core::cmp::Eq for devlaunch_runner::OsFailure +impl core::cmp::PartialEq for devlaunch_runner::OsFailure +pub fn devlaunch_runner::OsFailure::eq(&self, &devlaunch_runner::OsFailure) -> bool +impl core::convert::From<&core::io::error::Error> for devlaunch_runner::OsFailure +pub fn devlaunch_runner::OsFailure::from(&core::io::error::Error) -> Self +impl core::fmt::Debug for devlaunch_runner::OsFailure +pub fn devlaunch_runner::OsFailure::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::Copy for devlaunch_runner::OsFailure +impl core::marker::StructuralPartialEq for devlaunch_runner::OsFailure +pub struct devlaunch_runner::ProcessRunner +impl devlaunch_runner::ProcessRunner +pub fn devlaunch_runner::ProcessRunner::new() -> Self +impl core::clone::Clone for devlaunch_runner::ProcessRunner +pub fn devlaunch_runner::ProcessRunner::clone(&self) -> devlaunch_runner::ProcessRunner +impl core::default::Default for devlaunch_runner::ProcessRunner +pub fn devlaunch_runner::ProcessRunner::default() -> devlaunch_runner::ProcessRunner +impl core::fmt::Debug for devlaunch_runner::ProcessRunner +pub fn devlaunch_runner::ProcessRunner::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::Copy for devlaunch_runner::ProcessRunner +impl devlaunch_runner::Runner for devlaunch_runner::ProcessRunner +pub fn devlaunch_runner::ProcessRunner::capture(&self, &devlaunch_runner::SpawnSpec) -> devlaunch_runner::Outcome +pub fn devlaunch_runner::ProcessRunner::detach(&self, &devlaunch_runner::Invocation) -> devlaunch_runner::DetachOutcome +pub fn devlaunch_runner::ProcessRunner::passthrough(&self, &devlaunch_runner::SpawnSpec) -> devlaunch_runner::Outcome +pub fn devlaunch_runner::ProcessRunner::session(&self, &devlaunch_runner::SpawnSpec, &mut dyn core::ops::function::FnMut(&str)) -> devlaunch_runner::Outcome +pub struct devlaunch_runner::SpawnSpec +pub devlaunch_runner::SpawnSpec::invocation: devlaunch_runner::Invocation +pub devlaunch_runner::SpawnSpec::own_group: bool +pub devlaunch_runner::SpawnSpec::stdin: devlaunch_runner::StdinPlan +pub devlaunch_runner::SpawnSpec::timeout: core::option::Option +impl devlaunch_runner::SpawnSpec +pub fn devlaunch_runner::SpawnSpec::args(&self) -> &[alloc::string::String] +pub fn devlaunch_runner::SpawnSpec::leading_its_own_group(self) -> Self +pub fn devlaunch_runner::SpawnSpec::new(devlaunch_runner::Invocation) -> Self +pub fn devlaunch_runner::SpawnSpec::program(&self) -> &str +pub fn devlaunch_runner::SpawnSpec::with_stdin_file(self, impl core::convert::Into) -> Self +pub fn devlaunch_runner::SpawnSpec::with_stdin_null(self) -> Self +pub fn devlaunch_runner::SpawnSpec::with_timeout(self, core::time::Duration) -> Self +impl core::clone::Clone for devlaunch_runner::SpawnSpec +pub fn devlaunch_runner::SpawnSpec::clone(&self) -> devlaunch_runner::SpawnSpec +impl core::cmp::Eq for devlaunch_runner::SpawnSpec +impl core::cmp::PartialEq for devlaunch_runner::SpawnSpec +pub fn devlaunch_runner::SpawnSpec::eq(&self, &devlaunch_runner::SpawnSpec) -> bool +impl core::convert::From for devlaunch_runner::SpawnSpec +pub fn devlaunch_runner::SpawnSpec::from(devlaunch_runner::Invocation) -> Self +impl core::default::Default for devlaunch_runner::SpawnSpec +pub fn devlaunch_runner::SpawnSpec::default() -> devlaunch_runner::SpawnSpec +impl core::fmt::Debug for devlaunch_runner::SpawnSpec +pub fn devlaunch_runner::SpawnSpec::fmt(&self, &mut core::fmt::Formatter<'_>) -> core::fmt::Result +impl core::marker::StructuralPartialEq for devlaunch_runner::SpawnSpec +pub trait devlaunch_runner::Runner +pub fn devlaunch_runner::Runner::capture(&self, &devlaunch_runner::SpawnSpec) -> devlaunch_runner::Outcome +pub fn devlaunch_runner::Runner::detach(&self, &devlaunch_runner::Invocation) -> devlaunch_runner::DetachOutcome +pub fn devlaunch_runner::Runner::passthrough(&self, &devlaunch_runner::SpawnSpec) -> devlaunch_runner::Outcome +pub fn devlaunch_runner::Runner::session(&self, &devlaunch_runner::SpawnSpec, &mut dyn core::ops::function::FnMut(&str)) -> devlaunch_runner::Outcome +impl devlaunch_runner::Runner for devlaunch_runner::ProcessRunner +pub fn devlaunch_runner::ProcessRunner::capture(&self, &devlaunch_runner::SpawnSpec) -> devlaunch_runner::Outcome +pub fn devlaunch_runner::ProcessRunner::detach(&self, &devlaunch_runner::Invocation) -> devlaunch_runner::DetachOutcome +pub fn devlaunch_runner::ProcessRunner::passthrough(&self, &devlaunch_runner::SpawnSpec) -> devlaunch_runner::Outcome +pub fn devlaunch_runner::ProcessRunner::session(&self, &devlaunch_runner::SpawnSpec, &mut dyn core::ops::function::FnMut(&str)) -> devlaunch_runner::Outcome diff --git a/rust/devlaunch-runner/tests/public_api_snapshot.rs b/rust/devlaunch-runner/tests/public_api_snapshot.rs new file mode 100644 index 00000000..26d724ae --- /dev/null +++ b/rust/devlaunch-runner/tests/public_api_snapshot.rs @@ -0,0 +1,70 @@ +//! This crate's public surface is checked in, because it is the one an +//! external `Runner` implementor actually sees. +//! +//! Until the split it had no snapshot of its own: the whole crate entered +//! `devlaunch-core`'s snapshot as a single unexpanded row -- the glob re-export +//! `pub use devlaunch_core::runner::<>` -- so removing a +//! trait method, or changing what `Outcome` can be, moved nothing in any +//! checked-in file and passed CI silently. +//! +//! These tests do not police the whole snapshot -- the CI job's diff against a +//! fresh `cargo public-api` run does that. They hold the file to being *this* +//! crate's seam: present, and naming the declarations a caller implements +//! against, so a truncated or misdirected regeneration fails here instead of +//! being committed as the new truth. + +const SNAPSHOT: &str = include_str!("../public-api.txt"); + +fn rows(snapshot: &str) -> Vec<&str> { + snapshot + .lines() + .map(str::trim_end) + .filter(|row| !row.is_empty()) + .collect() +} + +fn has_row_starting(snapshot: &str, prefix: &str) -> bool { + rows(snapshot).into_iter().any(|row| row.starts_with(prefix)) +} + +#[test] +fn the_seam_carries_a_snapshot_of_its_own() { + assert!( + has_row_starting(SNAPSHOT, "pub mod devlaunch_runner"), + "devlaunch-runner's snapshot does not describe devlaunch-runner: {:#?}", + rows(SNAPSHOT).first() + ); +} + +#[test] +fn the_snapshot_pins_the_trait_an_implementor_writes_against() { + assert!( + rows(SNAPSHOT).contains(&"pub trait devlaunch_runner::Runner"), + "the Runner trait is missing from the snapshot" + ); + for method in ["capture", "passthrough", "session", "detach"] { + assert!( + has_row_starting(SNAPSHOT, &format!("pub fn devlaunch_runner::Runner::{method}(")), + "Runner::{method} is missing from the snapshot; every method of this \ + trait is a promise to whoever implements it" + ); + } +} + +#[test] +fn the_snapshot_pins_what_a_run_can_turn_out_to_be() { + assert!( + has_row_starting(SNAPSHOT, "pub enum devlaunch_runner::Outcome"), + "the Outcome enum is missing from the snapshot" + ); + for variant in ["Ran", "ProgramNotFound", "TimedOut", "NotStarted"] { + assert!( + has_row_starting( + SNAPSHOT, + &format!("pub devlaunch_runner::Outcome::{variant}") + ), + "Outcome::{variant} is missing from the snapshot; the set of ways a \ + run can end is what a caller matches exhaustively on" + ); + } +} diff --git a/scripts/public-api-snapshots.sh b/scripts/public-api-snapshots.sh new file mode 100755 index 00000000..8a172983 --- /dev/null +++ b/scripts/public-api-snapshots.sh @@ -0,0 +1,97 @@ +#!/usr/bin/env bash +# +# Regenerate the three checked-in `cargo public-api` snapshots -- and, wherever +# they are checked, be the one definition of how the surface is classified. +# +# There are three files because there are three different promises: +# +# rust/devlaunch-core/public-api.api.txt the frozen promise. Every row is a +# declaration under `devlaunch_core::api`, the tier an external consumer +# is entitled to depend on, so a diff here is a breaking change by +# definition and wants a reviewer who reads it that way. +# rust/devlaunch-core/public-api.rest.txt the tripwire. The binary surface -- +# `flows::`, `domain::`, `clients::` -- which is reachable but never +# promised: regenerate it freely, and read a diff for the accidental `pub` +# rather than for a broken contract. +# rust/devlaunch-runner/public-api.txt the process seam, as an external +# `Runner` implementor sees it. It had no snapshot until the split: the +# whole crate entered core's as one unexpanded glob row, so a removed +# trait method moved nothing and passed CI. +# +# The classification is one `grep` and it lives here, in the script CI runs, +# because the alternative is two copies of it -- one in the workflow, one in +# whatever regenerates the files -- drifting until the promise file quietly +# stops holding the promise. `.github/workflows/ci.yml`'s `public-api` job runs +# this into a scratch tree and diffs the result against what the repo carries; +# a developer runs it with no argument to accept a deliberate change. +# +# Usage: +# scripts/public-api-snapshots.sh # rewrite the checked-in files +# scripts/public-api-snapshots.sh DEST # write them under DEST instead +# scripts/public-api-snapshots.sh --print-pin +# +# Needs a nightly toolchain (cargo-public-api's rustdoc-JSON backend is +# nightly-only; the crates themselves still build on the stable pin) and the +# pinned cargo-public-api. This repository's devcontainer carries neither, so +# this is a host command: +# +# rustup toolchain install nightly +# cargo install cargo-public-api --locked \ +# --version "$(scripts/public-api-snapshots.sh --print-pin)" +# +set -euo pipefail + +# Pinned, and pinned here so the workflow installs what this script demands +# rather than the two agreeing by coincidence: cargo-public-api's rendering +# moves between releases, and a snapshot generated by a different one is a +# whole-file diff that says nothing. Bump it together with regenerated files. +PIN=0.52.0 + +# `-ss` omits blanket and auto-trait impls. Those rows move when *rustdoc* +# moves -- `UnsafeUnpin` appeared with a nightly, not with a crate change -- and +# a tripwire that fires on toolchain drift teaches people to update snapshots +# unread. Derived impls (Clone, Debug, serde) stay in: losing one is a real +# break. +FLAGS=(-ss) + +# The boundary matters: `\b` is what keeps a future `devlaunch_core::apiary` +# out of the promise file. `rust/devlaunch-core/tests/public_api_snapshots.rs` +# holds the checked-in files to this same rule from the other side. +API_ROW='devlaunch_core::api\b' + +if [[ "${1:-}" == "--print-pin" ]]; then + echo "$PIN" + exit 0 +fi + +repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +dest="${1:-$repo_root/rust}" +mkdir -p "$dest/devlaunch-core" "$dest/devlaunch-runner" + +if ! installed="$(cargo public-api --version 2>/dev/null)"; then + echo "cargo-public-api is not installed. cargo install cargo-public-api --locked --version $PIN" >&2 + exit 1 +fi +if [[ "$installed" != "cargo-public-api $PIN" ]]; then + echo "cargo-public-api is '$installed', and these snapshots are rendered by $PIN." >&2 + echo "Install the pin, or bump PIN in this script and regenerate everything with it." >&2 + exit 1 +fi + +cd "$repo_root/rust" + +core="$(cargo public-api -p devlaunch-core "${FLAGS[@]}")" + +# Two greps rather than one pass with a fallthrough, so the two files are +# complements by construction. Either coming out empty means the filter no +# longer matches the crate, which is a broken split rather than a small API. +if ! printf '%s\n' "$core" | grep -E "$API_ROW" >"$dest/devlaunch-core/public-api.api.txt"; then + echo "no rows matched $API_ROW: devlaunch-core no longer declares an 'api' module?" >&2 + exit 1 +fi +if ! printf '%s\n' "$core" | grep -Ev "$API_ROW" >"$dest/devlaunch-core/public-api.rest.txt"; then + echo "every row matched $API_ROW: the split has nothing left to classify?" >&2 + exit 1 +fi + +cargo public-api -p devlaunch-runner "${FLAGS[@]}" >"$dest/devlaunch-runner/public-api.txt" From 97c8c879ee8df03028b969ce7505506ea0337b78 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 16:20:15 +0000 Subject: [PATCH 2/7] Check all three snapshots from one script, and document regenerating them The public-api job ran `cargo public-api` itself and diffed one file. It now runs scripts/public-api-snapshots.sh into a scratch tree and diffs all three, so the filter that decides which row is a promise -- along with the `-ss` flag and the pinned cargo-public-api version, which the install step now reads from the script -- exists once rather than once in the workflow and once wherever the files get regenerated. Regeneration was documented only in the workflow's error string, which is a thing people learn by breaking. README gains "The public-API snapshots": what a diff in each file means, the one command, and the two prerequisites the devcontainer does not carry. A pytest doc guard holds the wiring -- three files checked in, the old combined one gone, no classification of its own in ci.yml, the pin read from the script, and the README section present. --- .github/workflows/ci.yml | 71 +++++++----- CHANGELOG.md | 12 ++ README.md | 40 +++++++ rust/devlaunch-core/src/lib.rs | 10 +- .../tests/public_api_snapshots.rs | 5 +- .../tests/public_api_snapshot.rs | 9 +- test/test_public_api_snapshots_doc.py | 103 ++++++++++++++++++ 7 files changed, 216 insertions(+), 34 deletions(-) create mode 100644 test/test_public_api_snapshots_doc.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02eaabe2..77bb669d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -376,24 +376,30 @@ jobs: env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} - # The API-freeze tripwire (#251 §7, #262). devlaunch-core's crate docs promise - # two tiers of `pub` — the frozen wf API behind `api`, and binary surface that - # is reachable but not promised — and this job is what makes that promise - # checkable rather than prose: rust/devlaunch-core/public-api.txt is the whole - # public surface as `cargo public-api` renders it, and any change to it fails - # here until the new snapshot is committed. That turns every surface change - # into a reviewed diff: an addition to `api` is a deliberate PR, a removal a - # visible breaking change, and an accidental `pub` a red tick instead of a - # silent promise. + # The API-freeze tripwire (#251 §7, #262, re-scoped by #312). devlaunch-core's + # crate docs promise two tiers of `pub` — the frozen wf API behind `api`, and + # binary surface that is reachable but not promised — and this job is what + # makes that promise checkable rather than prose: any change to a checked-in + # `cargo public-api` snapshot fails here until the new one is committed. That + # turns every surface change into a reviewed diff: an addition to `api` is a + # deliberate PR, a removal a visible breaking change, and an accidental `pub` + # a red tick instead of a silent promise. # - # Nightly because cargo-public-api's rustdoc-JSON backend needs it; the crate - # itself still builds on the stable pin everywhere else. `-ss` (omit blanket - # and auto-trait impls) rather than the full listing, because auto-trait rows - # move when *rustdoc* moves — `UnsafeUnpin` appeared with a nightly, not with - # a crate change — and a tripwire that fires on toolchain drift teaches people - # to update the snapshot unread. Derived impls (Clone, Debug, serde) stay in: - # losing one is a real API break. cargo-public-api is pinned for the same - # reason; bump it together with a freshly generated snapshot. + # Three snapshots, because one file could not tell those tiers apart. It held + # both, so a breaking `api` change arrived as one row inside two thousand of + # internal churn and read as routine; and `devlaunch-runner` — the trait an + # external implementor writes against — entered it as a single unexpanded glob + # row, so removing a `Runner` method moved nothing at all. Now: + # devlaunch-core/public-api.api.txt is the promise (a diff there is a breaking + # change by definition), devlaunch-core/public-api.rest.txt the tripwire over + # the binary surface, and devlaunch-runner/public-api.txt the process seam. + # + # Nightly because cargo-public-api's rustdoc-JSON backend needs it; the crates + # themselves still build on the stable pin everywhere else. The pinned version + # and the `-ss` rationale live in the regeneration script, which is also what + # this job runs: the split filter that decides which row is a promise exists + # once, in scripts/public-api-snapshots.sh, rather than once there and once + # here in a workflow nobody regenerates from. public-api: runs-on: ubuntu-latest timeout-minutes: 20 @@ -404,19 +410,28 @@ jobs: - uses: Swatinem/rust-cache@v2 with: workspaces: rust - - name: Install cargo-public-api (pinned) - run: cargo install cargo-public-api --locked --version 0.52.0 - - name: The public surface is the snapshot the repo carries - working-directory: rust + - name: Install cargo-public-api (the pin the script names) run: | set -euo pipefail - cargo public-api -p devlaunch-core -ss > /tmp/public-api.txt - if ! diff -u devlaunch-core/public-api.txt /tmp/public-api.txt; then - echo "::error::devlaunch-core's public API changed. If the change is" \ - "deliberate, regenerate the snapshot and commit it:" \ - "cd rust && cargo +nightly public-api -p devlaunch-core -ss" \ - "> devlaunch-core/public-api.txt (needs cargo install" \ - "cargo-public-api --locked --version 0.52.0 on a nightly toolchain)." + cargo install cargo-public-api --locked \ + --version "$(scripts/public-api-snapshots.sh --print-pin)" + - name: The public surface is the snapshots the repo carries + run: | + set -euo pipefail + scripts/public-api-snapshots.sh "$RUNNER_TEMP/public-api" + changed=0 + for snapshot in devlaunch-core/public-api.api.txt \ + devlaunch-core/public-api.rest.txt \ + devlaunch-runner/public-api.txt; do + diff -u "rust/$snapshot" "$RUNNER_TEMP/public-api/$snapshot" || changed=1 + done + if [ "$changed" -ne 0 ]; then + echo "::error::A public surface changed. If the change is deliberate," \ + "regenerate the snapshots and commit them:" \ + "scripts/public-api-snapshots.sh (needs a nightly toolchain and the" \ + "cargo-public-api the script pins; see 'The public-API snapshots' in" \ + "README.md). A diff in devlaunch-core/public-api.api.txt is a" \ + "breaking change to the promised API — say so in the PR." exit 1 fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 92a78160..988d2a97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **The public-API freeze is three snapshots instead of one, so its diff means something + again.** `devlaunch-core`'s snapshot splits into `public-api.api.txt` — the frozen + `devlaunch_core::api` promise, 37 rows, where any diff is a breaking change by definition — + and `public-api.rest.txt`, the tripwire over the binary surface that a refactor may move + freely. `devlaunch-runner` gets one of its own: the trait an external `Runner` implementor + writes against used to enter core's snapshot as a single unexpanded glob row, so removing a + method from it moved nothing and passed CI. `scripts/public-api-snapshots.sh` regenerates all + three and is what CI runs, so the filter deciding which row is a promise, the `-ss` flag and + the pinned `cargo-public-api` exist in one place; see "The public-API snapshots" in README.md. + ## [0.6.0] - 2026-08-22 ### Added diff --git a/README.md b/README.md index 1d2a095f..18103dcd 100644 --- a/README.md +++ b/README.md @@ -1874,6 +1874,46 @@ an installed release, the wheel's binary: DEVLAUNCH_DL_CMD='cargo run -q --manifest-path rust/Cargo.toml -p dl --bin dl --' pixi run pytest ``` +### The public-API snapshots + +Three files under `rust/` are the crates' public surface as `cargo public-api` renders it, and +CI's `public-api` job fails until a change to any of them is committed. They are not one file +because they are not one promise: + +| File | What a diff means | +| --- | --- | +| `devlaunch-core/public-api.api.txt` | **A breaking change.** The `devlaunch_core::api` tier is the frozen contract; a row leaving it breaks a consumer. | +| `devlaunch-core/public-api.rest.txt` | Routine. The binary surface — `flows::`, `domain::`, `clients::` — is reachable but never promised; read the diff for the accidental `pub`. | +| `devlaunch-runner/public-api.txt` | The process seam an external `Runner` implementor writes against. | + +The runner had no snapshot of its own until #338: its whole surface entered core's as the single +unexpanded row `pub use devlaunch_core::runner::<>`, so removing a trait +method moved nothing and passed. And core's one file mixed the two tiers, which is worse than it +sounds — a breaking `api` change arrives as one row inside two thousand of internal churn, and +reads as routine. + +Regenerate all three with one command, from anywhere in the checkout: + +```bash +scripts/public-api-snapshots.sh +``` + +That script is also what CI runs — into a scratch tree, then diffing — so the filter that decides +which row is a promise, the `-ss` flag and the pinned `cargo-public-api` version exist in exactly +one place. Two prerequisites, and this repository's devcontainer has neither, so it is a host +command: a nightly toolchain (cargo-public-api's rustdoc-JSON backend is nightly-only; the crates +themselves still build on the stable pin) and the pinned tool. + +```bash +rustup toolchain install nightly +cargo install cargo-public-api --locked --version "$(scripts/public-api-snapshots.sh --print-pin)" +``` + +Committing a regenerated `public-api.api.txt` is committing a breaking change, so say which one in +the pull request. `rust/devlaunch-core/tests/public_api_snapshots.rs` holds the two core files to +the split itself — every promised row is an `api` declaration and none of the others is — so a +hand-edited snapshot fails in the Rust suite rather than in review. + ### Coverage: two numbers, and neither is the other The crates that ship and the harness that judges them are measured separately, because they are diff --git a/rust/devlaunch-core/src/lib.rs b/rust/devlaunch-core/src/lib.rs index 3ebef95c..2580bef7 100644 --- a/rust/devlaunch-core/src/lib.rs +++ b/rust/devlaunch-core/src/lib.rs @@ -39,9 +39,13 @@ //! So a `pub` item here is not automatically part of the promised API. Only //! what [`api`] re-exports is. The distinction is enforced by keeping [`api`] //! the single re-export point, by the doc note above on every binary-surface -//! item, and by the `cargo public-api` snapshot in `public-api.txt`, which CI -//! diffs on every pull request: any change to the crate's public surface is a -//! committed, reviewed diff or a red tick. +//! item, and by two `cargo public-api` snapshots that CI diffs on every pull +//! request: any change to the crate's public surface is a committed, reviewed +//! diff or a red tick. The two tiers get a file each — `public-api.api.txt` +//! for the promise, `public-api.rest.txt` for the binary surface — so that a +//! breaking change is a diff in the small file rather than one row inside two +//! thousand of routine churn. `scripts/public-api-snapshots.sh` regenerates +//! them; see "The public-API snapshots" in README.md. // `runner` is pub so `devlaunch-test-support` can implement the trait; that // crate is dev-only and never shipped. diff --git a/rust/devlaunch-core/tests/public_api_snapshots.rs b/rust/devlaunch-core/tests/public_api_snapshots.rs index 8d3c1daf..419c6d4d 100644 --- a/rust/devlaunch-core/tests/public_api_snapshots.rs +++ b/rust/devlaunch-core/tests/public_api_snapshots.rs @@ -61,7 +61,10 @@ fn every_promised_row_is_an_api_declaration() { #[test] fn no_row_of_the_rest_is_an_api_declaration() { - let promised: Vec<&str> = rows(REST).into_iter().filter(|row| names_api(row)).collect(); + let promised: Vec<&str> = rows(REST) + .into_iter() + .filter(|row| names_api(row)) + .collect(); assert!( promised.is_empty(), "these devlaunch_core::api declarations are in the freely regenerated \ diff --git a/rust/devlaunch-runner/tests/public_api_snapshot.rs b/rust/devlaunch-runner/tests/public_api_snapshot.rs index 26d724ae..f00b9dd7 100644 --- a/rust/devlaunch-runner/tests/public_api_snapshot.rs +++ b/rust/devlaunch-runner/tests/public_api_snapshot.rs @@ -24,7 +24,9 @@ fn rows(snapshot: &str) -> Vec<&str> { } fn has_row_starting(snapshot: &str, prefix: &str) -> bool { - rows(snapshot).into_iter().any(|row| row.starts_with(prefix)) + rows(snapshot) + .into_iter() + .any(|row| row.starts_with(prefix)) } #[test] @@ -44,7 +46,10 @@ fn the_snapshot_pins_the_trait_an_implementor_writes_against() { ); for method in ["capture", "passthrough", "session", "detach"] { assert!( - has_row_starting(SNAPSHOT, &format!("pub fn devlaunch_runner::Runner::{method}(")), + has_row_starting( + SNAPSHOT, + &format!("pub fn devlaunch_runner::Runner::{method}(") + ), "Runner::{method} is missing from the snapshot; every method of this \ trait is a promise to whoever implements it" ); diff --git a/test/test_public_api_snapshots_doc.py b/test/test_public_api_snapshots_doc.py new file mode 100644 index 00000000..89ad1fe2 --- /dev/null +++ b/test/test_public_api_snapshots_doc.py @@ -0,0 +1,103 @@ +"""The public-API snapshots have one owner, and it is findable (#312, #338). + +Three files are checked in now instead of one -- the promised `api` tier, the +binary-surface tripwire, and the runner crate's own seam -- and what makes that +worth having is a single definition of which row belongs where. That definition +is a `grep` in ``scripts/public-api-snapshots.sh``. Copy it into the workflow +"just to check" and the copies drift: the day they disagree, the promise file +still exists and still passes, while the promise itself has quietly moved into +the file reviewers skim. + +So this guards the wiring rather than the surface -- CI's own diff is what +guards the surface. Each check below is a thing somebody could plausibly write, +with the reason it must not be written. +""" + +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +SCRIPT = REPO_ROOT / "scripts" / "public-api-snapshots.sh" +CI_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "ci.yml" +README = REPO_ROOT / "README.md" +RUST = REPO_ROOT / "rust" + +# The three files, and the promise each one carries. +SNAPSHOTS = ( + RUST / "devlaunch-core" / "public-api.api.txt", + RUST / "devlaunch-core" / "public-api.rest.txt", + RUST / "devlaunch-runner" / "public-api.txt", +) +# The classification itself: the pattern that decides "promise" from "rest". +API_ROW_PATTERN = "devlaunch_core::api\\b" + + +def ci_job(name: str) -> str: + """One job of ci.yml, as text. + + A slice rather than a parse: there is no YAML parser in this project's + dependencies, and the jobs are the only things at that indentation. + """ + text = CI_WORKFLOW.read_text(encoding="utf-8") + start = text.index(f"\n {name}:\n") + rest = text[start + 1 :] + end = rest.find("\n\n ") + return rest if end == -1 else rest[:end] + + +@pytest.mark.unit +def test_every_snapshot_the_split_promises_is_checked_in(): + missing = [str(path.relative_to(REPO_ROOT)) for path in SNAPSHOTS if not path.is_file()] + assert not missing, f"the split names these files and the repo does not carry them: {missing}" + + +@pytest.mark.unit +def test_the_one_snapshot_the_split_replaced_is_gone(): + combined = RUST / "devlaunch-core" / "public-api.txt" + assert not combined.exists(), ( + "the pre-split snapshot is still here; a second file describing the same " + "surface is one nobody regenerates and everybody trusts" + ) + + +@pytest.mark.unit +def test_the_classification_lives_in_the_script_alone(): + assert API_ROW_PATTERN in SCRIPT.read_text(encoding="utf-8"), ( + "the split filter is not in the regeneration script" + ) + assert "devlaunch_core::api" not in CI_WORKFLOW.read_text(encoding="utf-8"), ( + "ci.yml classifies rows itself; it must run the script instead, or the two " + "definitions of 'promised' will drift apart" + ) + + +@pytest.mark.unit +def test_ci_checks_all_three_snapshots_by_running_the_script(): + job = ci_job("public-api") + assert "scripts/public-api-snapshots.sh" in job, "the public-api job does not run the script" + for path in SNAPSHOTS: + relative = str(path.relative_to(RUST)) + assert relative in job, f"the public-api job never diffs {relative}" + + +@pytest.mark.unit +def test_ci_installs_the_version_the_script_pins(): + job = ci_job("public-api") + assert "--print-pin" in job, ( + "ci.yml names a cargo-public-api version of its own; the pin belongs to the " + "script that renders the snapshots, since a different renderer makes a " + "whole-file diff that says nothing" + ) + + +@pytest.mark.unit +def test_regenerating_is_documented_outside_the_ci_error_string(): + readme = README.read_text(encoding="utf-8") + assert "### The public-API snapshots" in readme, ( + "README has no section on the snapshots; a red tick that only explains " + "itself in a workflow's error string is a thing people learn by breaking" + ) + assert "scripts/public-api-snapshots.sh" in readme, ( + "the README section does not name the command that regenerates them" + ) From d55075874d2d403c50eb56b7c0fa4874ce413f7c Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 16:22:02 +0000 Subject: [PATCH 3/7] codespell: implementor -> implementer --- .github/workflows/ci.yml | 2 +- CHANGELOG.md | 2 +- README.md | 2 +- rust/devlaunch-runner/tests/public_api_snapshot.rs | 4 ++-- scripts/public-api-snapshots.sh | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 77bb669d..8f9c26e8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -388,7 +388,7 @@ jobs: # Three snapshots, because one file could not tell those tiers apart. It held # both, so a breaking `api` change arrived as one row inside two thousand of # internal churn and read as routine; and `devlaunch-runner` — the trait an - # external implementor writes against — entered it as a single unexpanded glob + # external implementer writes against — entered it as a single unexpanded glob # row, so removing a `Runner` method moved nothing at all. Now: # devlaunch-core/public-api.api.txt is the promise (a diff there is a breaking # change by definition), devlaunch-core/public-api.rest.txt the tripwire over diff --git a/CHANGELOG.md b/CHANGELOG.md index 988d2a97..8573fe6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 again.** `devlaunch-core`'s snapshot splits into `public-api.api.txt` — the frozen `devlaunch_core::api` promise, 37 rows, where any diff is a breaking change by definition — and `public-api.rest.txt`, the tripwire over the binary surface that a refactor may move - freely. `devlaunch-runner` gets one of its own: the trait an external `Runner` implementor + freely. `devlaunch-runner` gets one of its own: the trait an external `Runner` implementer writes against used to enter core's snapshot as a single unexpanded glob row, so removing a method from it moved nothing and passed CI. `scripts/public-api-snapshots.sh` regenerates all three and is what CI runs, so the filter deciding which row is a promise, the `-ss` flag and diff --git a/README.md b/README.md index 18103dcd..b3cdf693 100644 --- a/README.md +++ b/README.md @@ -1884,7 +1884,7 @@ because they are not one promise: | --- | --- | | `devlaunch-core/public-api.api.txt` | **A breaking change.** The `devlaunch_core::api` tier is the frozen contract; a row leaving it breaks a consumer. | | `devlaunch-core/public-api.rest.txt` | Routine. The binary surface — `flows::`, `domain::`, `clients::` — is reachable but never promised; read the diff for the accidental `pub`. | -| `devlaunch-runner/public-api.txt` | The process seam an external `Runner` implementor writes against. | +| `devlaunch-runner/public-api.txt` | The process seam an external `Runner` implementer writes against. | The runner had no snapshot of its own until #338: its whole surface entered core's as the single unexpanded row `pub use devlaunch_core::runner::<>`, so removing a trait diff --git a/rust/devlaunch-runner/tests/public_api_snapshot.rs b/rust/devlaunch-runner/tests/public_api_snapshot.rs index f00b9dd7..603a720b 100644 --- a/rust/devlaunch-runner/tests/public_api_snapshot.rs +++ b/rust/devlaunch-runner/tests/public_api_snapshot.rs @@ -1,5 +1,5 @@ //! This crate's public surface is checked in, because it is the one an -//! external `Runner` implementor actually sees. +//! external `Runner` implementer actually sees. //! //! Until the split it had no snapshot of its own: the whole crate entered //! `devlaunch-core`'s snapshot as a single unexpanded row -- the glob re-export @@ -39,7 +39,7 @@ fn the_seam_carries_a_snapshot_of_its_own() { } #[test] -fn the_snapshot_pins_the_trait_an_implementor_writes_against() { +fn the_snapshot_pins_the_trait_an_implementer_writes_against() { assert!( rows(SNAPSHOT).contains(&"pub trait devlaunch_runner::Runner"), "the Runner trait is missing from the snapshot" diff --git a/scripts/public-api-snapshots.sh b/scripts/public-api-snapshots.sh index 8a172983..e53d02a6 100755 --- a/scripts/public-api-snapshots.sh +++ b/scripts/public-api-snapshots.sh @@ -14,7 +14,7 @@ # promised: regenerate it freely, and read a diff for the accidental `pub` # rather than for a broken contract. # rust/devlaunch-runner/public-api.txt the process seam, as an external -# `Runner` implementor sees it. It had no snapshot until the split: the +# `Runner` implementer sees it. It had no snapshot until the split: the # whole crate entered core's as one unexpanded glob row, so a removed # trait method moved nothing and passed CI. # From 37c282ddcd73650f349d0b12e3510de84d3e3a6f Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 16:53:28 +0000 Subject: [PATCH 4/7] Never leave a snapshot at zero bytes, and let DEST be relative Two bugs the review found by running the script, both reproduced here before fixing. The shell truncates a redirect target before the command on its right runs, so when the script's own "no rows matched" guard fired it had already emptied the file it was complaining about. Reproduced with `pub mod api` demoted to `mod api`: the checked-in public-api.api.txt went 3376 bytes -> 0, and only one of the four tests over those files would have noticed. All three snapshots are now staged in a temp directory and moved into place only once every one of them generated; the same run against the same demotion now prints the same message and leaves the three files byte-identical to HEAD. A relative DEST was silently broken: `mkdir -p` ran in the caller's CWD and the writes ran after `cd rust`. Reproduced from a scratch directory: `mkdir` created the tree beside the caller and the redirect then died with "No such file or directory" -- reported, misleadingly, as a missing `api` module. `dest` is now absolutised before the `cd`, and the same invocation writes all three files where asked. Two things ride along in the same file: `--print-files`, so the workflow stops keeping its own copy of the list, and a header that states what the path filter cannot see. Both are described in the commit that uses them. --- scripts/public-api-snapshots.sh | 92 ++++++++++++++++++++++++++------- 1 file changed, 74 insertions(+), 18 deletions(-) diff --git a/scripts/public-api-snapshots.sh b/scripts/public-api-snapshots.sh index e53d02a6..054e75c7 100755 --- a/scripts/public-api-snapshots.sh +++ b/scripts/public-api-snapshots.sh @@ -5,30 +5,49 @@ # # There are three files because there are three different promises: # -# rust/devlaunch-core/public-api.api.txt the frozen promise. Every row is a -# declaration under `devlaunch_core::api`, the tier an external consumer -# is entitled to depend on, so a diff here is a breaking change by -# definition and wants a reviewer who reads it that way. -# rust/devlaunch-core/public-api.rest.txt the tripwire. The binary surface -- -# `flows::`, `domain::`, `clients::` -- which is reachable but never -# promised: regenerate it freely, and read a diff for the accidental `pub` -# rather than for a broken contract. +# rust/devlaunch-core/public-api.api.txt the promise, as far as a path match +# can see it: every row is a declaration *at* `devlaunch_core::api`, the +# tier an external consumer is entitled to depend on. A diff here is a +# deliberate change to that tier -- a removal or a changed signature is a +# break -- and wants a reviewer who reads it that way. The converse does +# not hold; see the limit below. +# rust/devlaunch-core/public-api.rest.txt the tripwire, and today also the +# promised types' behaviour. Mostly the binary surface -- `flows::`, +# `domain::`, `clients::` -- which is reachable but never promised, so +# most of a diff here is routine and read for the accidental `pub`. But +# see the limit below before reading a diff that touches a promised type +# as routine. # rust/devlaunch-runner/public-api.txt the process seam, as an external # `Runner` implementer sees it. It had no snapshot until the split: the # whole crate entered core's as one unexpanded glob row, so a removed # trait method moved nothing and passed CI. # +# The limit, measured rather than assumed: `cargo public-api` renders inherent +# methods and trait impls only at a type's *canonical* path, never at the path +# it is re-exported under. So `api::Launch::run` is rendered +# `devlaunch_core::flows::launch::Launch::run` and this classifier cannot see +# it. Of the 79 rows the generator emits for the `api` section, the match keeps +# 37; the other 42 -- `Launch::new`, `Launch::run`, `CommandContext::new`, +# `DevcontainerPath::as_str` and every derived `Clone`/`Debug`/`PartialEq` on +# the promised types -- land in the rest file. Renaming `Launch::run` therefore +# leaves the promise file byte-identical. Two consequences worth carrying: +# a diff in the rest file that touches a promised type is a contract change +# too, and this classifier is not the whole guard. Widening it is +# https://github.com/blooop/devlaunch/issues/352. +# # The classification is one `grep` and it lives here, in the script CI runs, # because the alternative is two copies of it -- one in the workflow, one in # whatever regenerates the files -- drifting until the promise file quietly -# stops holding the promise. `.github/workflows/ci.yml`'s `public-api` job runs -# this into a scratch tree and diffs the result against what the repo carries; -# a developer runs it with no argument to accept a deliberate change. +# stops holding even what it does hold. `.github/workflows/ci.yml`'s +# `public-api` job runs this into a scratch tree and diffs the result against +# what the repo carries; a developer runs it with no argument to accept a +# deliberate change. # # Usage: # scripts/public-api-snapshots.sh # rewrite the checked-in files # scripts/public-api-snapshots.sh DEST # write them under DEST instead # scripts/public-api-snapshots.sh --print-pin +# scripts/public-api-snapshots.sh --print-files # # Needs a nightly toolchain (cargo-public-api's rustdoc-JSON backend is # nightly-only; the crates themselves still build on the stable pin) and the @@ -51,7 +70,7 @@ PIN=0.52.0 # moves -- `UnsafeUnpin` appeared with a nightly, not with a crate change -- and # a tripwire that fires on toolchain drift teaches people to update snapshots # unread. Derived impls (Clone, Debug, serde) stay in: losing one is a real -# break. +# break. Note where they stay -- the rest file, per the limit above. FLAGS=(-ss) # The boundary matters: `\b` is what keeps a future `devlaunch_core::apiary` @@ -59,14 +78,36 @@ FLAGS=(-ss) # holds the checked-in files to this same rule from the other side. API_ROW='devlaunch_core::api\b' -if [[ "${1:-}" == "--print-pin" ]]; then +# Every file this script writes, relative to `rust/` -- which is also every +# file CI diffs. Emitted rather than restated there, for the same reason the +# pin is: a fourth snapshot added here should not need a workflow edit to be +# checked, and a workflow that lists them itself is a list that can fall behind. +API_FILE=devlaunch-core/public-api.api.txt +REST_FILE=devlaunch-core/public-api.rest.txt +RUNNER_FILE=devlaunch-runner/public-api.txt +FILES=("$API_FILE" "$REST_FILE" "$RUNNER_FILE") + +case "${1:-}" in +--print-pin) echo "$PIN" exit 0 -fi + ;; +--print-files) + printf '%s\n' "${FILES[@]}" + exit 0 + ;; +esac repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" dest="${1:-$repo_root/rust}" -mkdir -p "$dest/devlaunch-core" "$dest/devlaunch-runner" +for file in "${FILES[@]}"; do + mkdir -p "$dest/$(dirname "$file")" +done +# Absolute from here on. Everything below runs after a `cd` into `rust/`, which +# would otherwise re-anchor a relative DEST: the `mkdir` above lands beside the +# caller and the writes land somewhere else entirely -- or, as it happened, +# fail at the redirect and report it as a missing `api` module. +dest="$(cd "$dest" && pwd)" if ! installed="$(cargo public-api --version 2>/dev/null)"; then echo "cargo-public-api is not installed. cargo install cargo-public-api --locked --version $PIN" >&2 @@ -78,6 +119,18 @@ if [[ "$installed" != "cargo-public-api $PIN" ]]; then exit 1 fi +# Stage every file, and move them into place only once all three exist. The +# shell truncates a redirect target *before* the command on its right runs, so +# writing the destinations directly means a failed generation -- or a guard +# below firing -- empties a checked-in snapshot on the way to reporting the +# problem. That is a script whose whole job is to write those files leaving +# them at zero bytes, and only one of the tests over them notices. +staging="$(mktemp -d)" +trap 'rm -rf "$staging"' EXIT +for file in "${FILES[@]}"; do + mkdir -p "$staging/$(dirname "$file")" +done + cd "$repo_root/rust" core="$(cargo public-api -p devlaunch-core "${FLAGS[@]}")" @@ -85,13 +138,16 @@ core="$(cargo public-api -p devlaunch-core "${FLAGS[@]}")" # Two greps rather than one pass with a fallthrough, so the two files are # complements by construction. Either coming out empty means the filter no # longer matches the crate, which is a broken split rather than a small API. -if ! printf '%s\n' "$core" | grep -E "$API_ROW" >"$dest/devlaunch-core/public-api.api.txt"; then +if ! printf '%s\n' "$core" | grep -E "$API_ROW" >"$staging/$API_FILE"; then echo "no rows matched $API_ROW: devlaunch-core no longer declares an 'api' module?" >&2 exit 1 fi -if ! printf '%s\n' "$core" | grep -Ev "$API_ROW" >"$dest/devlaunch-core/public-api.rest.txt"; then +if ! printf '%s\n' "$core" | grep -Ev "$API_ROW" >"$staging/$REST_FILE"; then echo "every row matched $API_ROW: the split has nothing left to classify?" >&2 exit 1 fi +cargo public-api -p devlaunch-runner "${FLAGS[@]}" >"$staging/$RUNNER_FILE" -cargo public-api -p devlaunch-runner "${FLAGS[@]}" >"$dest/devlaunch-runner/public-api.txt" +for file in "${FILES[@]}"; do + mv "$staging/$file" "$dest/$file" +done From 895d034bc8c95203932f8bf18a6913dc456ec8be Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 16:53:52 +0000 Subject: [PATCH 5/7] Say what the promise file cannot see, and take the file list from the script The review measured the gap: `cargo public-api` renders inherent methods and trait impls only at a type's canonical path, so a filter on the `api` path cannot see them. Reproduced independently -- the generator emits the `api` section as 79 contiguous rows (2..80, ending where `pub mod devlaunch_core::clients` starts) and the filter keeps 37, the count the checked-in file has. The 42 that fall through include `Launch::new`, `Launch::run`, `CommandContext::new` and `DevcontainerPath::as_str`. Renaming `Launch::run` was confirmed to leave public-api.api.txt byte-identical while diffing the rest file twice, since the api re-export makes those rows render twice. The file is as #312 decided ("37 lines today") and is not changed here. What was wrong was the prose: lib.rs, the README table, the CHANGELOG entry, the job comment and the test's own module doc all asserted the converse -- that a break to the promise shows up in the small file. They now state the direction that holds, name what lands in the rest file instead, and point at #352, which widens the classifier. Shipping a fresh overclaim in the PR that exists to restore a guard's signal would be self-defeating. Also from the review: the workflow no longer keeps its own copy of the three snapshot paths (`--print-files`, next to `--print-pin`), and the Python guard stops enforcing that copy -- it now asserts the diff loop names no path of its own, and that the docs state the limit and name the ticket. Its ci.yml slicing ended at the first blank line inside a job, so it now ends at the next sibling key and asserts the slice contains the job; and the ban on classifying rows in the workflow applies to settings rather than comments, so the job may explain itself. Every one of those is perturbation-tested: a cosmetic blank line inside the job passes where the old slicing fails, a hardcoded list fails, a grep in a setting fails while the same string in a comment passes, and dropping either the limit or the ticket number from the docs fails. --- .github/workflows/ci.yml | 34 +++--- CHANGELOG.md | 10 +- README.md | 43 ++++--- rust/devlaunch-core/src/lib.rs | 24 +++- .../tests/public_api_snapshots.rs | 21 +++- test/test_public_api_snapshots_doc.py | 115 ++++++++++++++---- 6 files changed, 181 insertions(+), 66 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8f9c26e8..f318dc0f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -390,16 +390,23 @@ jobs: # internal churn and read as routine; and `devlaunch-runner` — the trait an # external implementer writes against — entered it as a single unexpanded glob # row, so removing a `Runner` method moved nothing at all. Now: - # devlaunch-core/public-api.api.txt is the promise (a diff there is a breaking - # change by definition), devlaunch-core/public-api.rest.txt the tripwire over - # the binary surface, and devlaunch-runner/public-api.txt the process seam. + # devlaunch-core/public-api.api.txt is the promise as a path match can see it, + # devlaunch-core/public-api.rest.txt the tripwire over the binary surface, and + # devlaunch-runner/public-api.txt the process seam. + # + # "As a path match can see it" is the honest scope, and it is narrower than it + # reads: cargo public-api renders methods and impls only at a type's canonical + # path, so a promised type's constructors, methods and derived impls are in + # the rest file, and renaming `api::Launch::run` leaves the promise file + # byte-identical. A diff in the rest file touching a promised type is a + # contract change too. Widening the classifier is #352. # # Nightly because cargo-public-api's rustdoc-JSON backend needs it; the crates - # themselves still build on the stable pin everywhere else. The pinned version - # and the `-ss` rationale live in the regeneration script, which is also what - # this job runs: the split filter that decides which row is a promise exists - # once, in scripts/public-api-snapshots.sh, rather than once there and once - # here in a workflow nobody regenerates from. + # themselves still build on the stable pin everywhere else. The pinned + # version, the `-ss` rationale, the split filter and the list of files to + # check all live in the regeneration script, which is also what this job runs + # — so what CI checks cannot drift from what a developer regenerates, and a + # fourth snapshot needs no workflow edit to be covered. public-api: runs-on: ubuntu-latest timeout-minutes: 20 @@ -420,18 +427,17 @@ jobs: set -euo pipefail scripts/public-api-snapshots.sh "$RUNNER_TEMP/public-api" changed=0 - for snapshot in devlaunch-core/public-api.api.txt \ - devlaunch-core/public-api.rest.txt \ - devlaunch-runner/public-api.txt; do + while read -r snapshot; do diff -u "rust/$snapshot" "$RUNNER_TEMP/public-api/$snapshot" || changed=1 - done + done < <(scripts/public-api-snapshots.sh --print-files) if [ "$changed" -ne 0 ]; then echo "::error::A public surface changed. If the change is deliberate," \ "regenerate the snapshots and commit them:" \ "scripts/public-api-snapshots.sh (needs a nightly toolchain and the" \ "cargo-public-api the script pins; see 'The public-API snapshots' in" \ - "README.md). A diff in devlaunch-core/public-api.api.txt is a" \ - "breaking change to the promised API — say so in the PR." + "README.md). A diff in devlaunch-core/public-api.api.txt is a change" \ + "to the promised API — say which, and note that a promised type's" \ + "methods and impls diff in the rest file instead (#352)." exit 1 fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 8573fe6c..0a5c87f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - **The public-API freeze is three snapshots instead of one, so its diff means something - again.** `devlaunch-core`'s snapshot splits into `public-api.api.txt` — the frozen - `devlaunch_core::api` promise, 37 rows, where any diff is a breaking change by definition — + again.** `devlaunch-core`'s snapshot splits into `public-api.api.txt` — the 37 declarations + written at the `devlaunch_core::api` path, where a diff is a change to the promised contract — and `public-api.rest.txt`, the tripwire over the binary surface that a refactor may move - freely. `devlaunch-runner` gets one of its own: the trait an external `Runner` implementer + freely. That guard is one-way, and the README says so: `cargo public-api` renders methods and + impls only at a type's canonical path, so a promised type's constructors, methods and derived + impls diff in the *rest* file (renaming `api::Launch::run` leaves the promise file + byte-identical). Widening the classifier is #352. + `devlaunch-runner` gets one of its own: the trait an external `Runner` implementer writes against used to enter core's snapshot as a single unexpanded glob row, so removing a method from it moved nothing and passed CI. `scripts/public-api-snapshots.sh` regenerates all three and is what CI runs, so the filter deciding which row is a promise, the `-ss` flag and diff --git a/README.md b/README.md index b3cdf693..aa7bc7e8 100644 --- a/README.md +++ b/README.md @@ -1882,37 +1882,52 @@ because they are not one promise: | File | What a diff means | | --- | --- | -| `devlaunch-core/public-api.api.txt` | **A breaking change.** The `devlaunch_core::api` tier is the frozen contract; a row leaving it breaks a consumer. | -| `devlaunch-core/public-api.rest.txt` | Routine. The binary surface — `flows::`, `domain::`, `clients::` — is reachable but never promised; read the diff for the accidental `pub`. | +| `devlaunch-core/public-api.api.txt` | **A change to the promised contract** — a removal or a changed signature breaks a consumer, an addition is a deliberate widening. Holds the 37 declarations written *at* the `devlaunch_core::api` path, and only those. | +| `devlaunch-core/public-api.rest.txt` | Mostly routine — the binary surface (`flows::`, `domain::`, `clients::`) is reachable but never promised, so read it for the accidental `pub`. **But** the promised types' methods and impls are in here too (see below), and a diff touching one of those is a contract change. | | `devlaunch-runner/public-api.txt` | The process seam an external `Runner` implementer writes against. | +**The promise file holds declarations, not behaviour.** `cargo public-api` renders inherent methods +and trait impls only at a type's *canonical* path, never at the path it is re-exported under, so +the classifier cannot see them: `api::Launch`'s only constructor and only method are rendered +`flows::launch::Launch::{new, run}` and land in the rest file, along with `CommandContext::new`, +`DevcontainerPath::as_str` and every derived `Clone`/`Debug`/`PartialEq` on the promised types — 42 +of the 79 rows the generator emits for the `api` section. Measured consequence: renaming +`api::Launch::run` leaves `public-api.api.txt` byte-identical. The guard is therefore one-way — a +diff in the promise file is a change to the promise, but not every change to the promise diffs it. +Widening the classifier is [#352](https://github.com/blooop/devlaunch/issues/352). + The runner had no snapshot of its own until #338: its whole surface entered core's as the single unexpanded row `pub use devlaunch_core::runner::<>`, so removing a trait method moved nothing and passed. And core's one file mixed the two tiers, which is worse than it -sounds — a breaking `api` change arrives as one row inside two thousand of internal churn, and -reads as routine. +sounds — a change to the promised declarations arrives as one row inside two thousand of internal +churn, and reads as routine. -Regenerate all three with one command, from anywhere in the checkout: +Regenerate all three with one command, from the repository root (or by absolute path from +anywhere — the script resolves the checkout from its own location): ```bash scripts/public-api-snapshots.sh ``` -That script is also what CI runs — into a scratch tree, then diffing — so the filter that decides -which row is a promise, the `-ss` flag and the pinned `cargo-public-api` version exist in exactly -one place. Two prerequisites, and this repository's devcontainer has neither, so it is a host -command: a nightly toolchain (cargo-public-api's rustdoc-JSON backend is nightly-only; the crates -themselves still build on the stable pin) and the pinned tool. +That script is also what CI runs — into a scratch tree, then diffing the files it names via +`--print-files` — so the filter that decides which row is a promise, the `-ss` flag, the pinned +`cargo-public-api` version and the list of snapshots all exist in exactly one place. Two +prerequisites, and this repository's devcontainer has neither, so it is a host command: a nightly +toolchain (cargo-public-api's rustdoc-JSON backend is nightly-only; the crates themselves still +build on the stable pin) and the pinned tool. ```bash rustup toolchain install nightly cargo install cargo-public-api --locked --version "$(scripts/public-api-snapshots.sh --print-pin)" ``` -Committing a regenerated `public-api.api.txt` is committing a breaking change, so say which one in -the pull request. `rust/devlaunch-core/tests/public_api_snapshots.rs` holds the two core files to -the split itself — every promised row is an `api` declaration and none of the others is — so a -hand-edited snapshot fails in the Rust suite rather than in review. +Committing a regenerated `public-api.api.txt` is committing a change to the promised contract, so +say which one in the pull request — and if the change was to a promised type's methods or impls, +the diff to point at is in `public-api.rest.txt`. `rust/devlaunch-core/tests/public_api_snapshots.rs` +holds the two core files to the split itself — every promised row is an `api` declaration and none +of the others is — so a hand-edited snapshot fails in the Rust suite rather than in review. The +script writes through a staging directory and moves the files into place only once all three +generated, so a failed run leaves the checked-in snapshots exactly as they were. ### Coverage: two numbers, and neither is the other diff --git a/rust/devlaunch-core/src/lib.rs b/rust/devlaunch-core/src/lib.rs index 2580bef7..a636c5c1 100644 --- a/rust/devlaunch-core/src/lib.rs +++ b/rust/devlaunch-core/src/lib.rs @@ -42,10 +42,26 @@ //! item, and by two `cargo public-api` snapshots that CI diffs on every pull //! request: any change to the crate's public surface is a committed, reviewed //! diff or a red tick. The two tiers get a file each — `public-api.api.txt` -//! for the promise, `public-api.rest.txt` for the binary surface — so that a -//! breaking change is a diff in the small file rather than one row inside two -//! thousand of routine churn. `scripts/public-api-snapshots.sh` regenerates -//! them; see "The public-API snapshots" in README.md. +//! for declarations at the [`api`] path, `public-api.rest.txt` for the rest — +//! so that a change to the promised tier's *declarations* is a diff in a +//! 37-row file rather than one row inside two thousand. +//! +//! **What that file does not cover, and it is not a small gap.** +//! `cargo public-api` renders inherent methods and trait impls only at a +//! type's canonical path, never at the path it is re-exported under. So +//! [`api::Launch`]'s only constructor and only method are rendered +//! `flows::launch::Launch::{new, run}` and land in `public-api.rest.txt`, as do +//! `CommandContext::new`, `DevcontainerPath::as_str` and every derived +//! `Clone`/`Debug`/`PartialEq` on the promised types: 42 of the 79 rows the +//! generator emits for the `api` section. Renaming `api::Launch::run` — an +//! unambiguous break — leaves `public-api.api.txt` byte-identical. So the +//! sound direction is one-way: a diff in the promise file *is* a change to the +//! promise, but a change to the promise need not diff it, and a diff in the +//! rest file that touches a promised type is a contract change too. Widening +//! the classifier is . +//! +//! `scripts/public-api-snapshots.sh` regenerates both; see "The public-API +//! snapshots" in README.md. // `runner` is pub so `devlaunch-test-support` can implement the trait; that // crate is dev-only and never shipped. diff --git a/rust/devlaunch-core/tests/public_api_snapshots.rs b/rust/devlaunch-core/tests/public_api_snapshots.rs index 419c6d4d..53b7b345 100644 --- a/rust/devlaunch-core/tests/public_api_snapshots.rs +++ b/rust/devlaunch-core/tests/public_api_snapshots.rs @@ -1,17 +1,28 @@ //! The two files core's public-API snapshot splits into, and the invariant that //! keeps them worth reading. //! -//! `public-api.api.txt` is the promise: every row is a declaration under -//! `devlaunch_core::api`, the surface an external consumer is entitled to -//! depend on, so a diff there is a breaking change by definition. -//! `public-api.rest.txt` is the tripwire: the binary surface, reachable but -//! never promised, regenerated freely whenever a refactor moves it. +//! `public-api.api.txt` is the promise as a path match can see it: every row is +//! a declaration written *at* `devlaunch_core::api`, the surface an external +//! consumer is entitled to depend on, so a diff there is a change to that +//! contract. `public-api.rest.txt` is the tripwire: the binary surface, +//! reachable but never promised, regenerated freely whenever a refactor moves +//! it. //! //! One snapshot for both tiers is what the split replaced, and the reason is //! the signal: a diff that is nine hundred rows of internal churn and one //! removed `api` function reads as routine, and the one row that mattered goes //! through review unremarked. //! +//! The tests below hold the partition, which is not the same as holding the +//! promise. `cargo public-api` renders methods and impls only at a type's +//! canonical path, so `api::Launch::{new, run}` and every derived impl on a +//! promised type are in the rest file — 42 of the 79 rows the generator emits +//! for the `api` section — and renaming `Launch::run` diffs neither of these +//! two files in the place a reader would look. Deliberately not asserted here: +//! widens the classifier, and +//! its red is that rename, so a test pinning today's classification of those +//! rows would have to be deleted to let the fix land. +//! //! The classification lives in `scripts/public-api-snapshots.sh` and nowhere //! else — the CI job runs that script rather than re-implementing its filter. //! These tests are the other half of that: they hold the *checked-in* files to diff --git a/test/test_public_api_snapshots_doc.py b/test/test_public_api_snapshots_doc.py index 89ad1fe2..7319e356 100644 --- a/test/test_public_api_snapshots_doc.py +++ b/test/test_public_api_snapshots_doc.py @@ -2,17 +2,21 @@ Three files are checked in now instead of one -- the promised `api` tier, the binary-surface tripwire, and the runner crate's own seam -- and what makes that -worth having is a single definition of which row belongs where. That definition -is a `grep` in ``scripts/public-api-snapshots.sh``. Copy it into the workflow -"just to check" and the copies drift: the day they disagree, the promise file -still exists and still passes, while the promise itself has quietly moved into -the file reviewers skim. +worth having is a single definition of which row belongs where, and which files +exist at all. Both live in ``scripts/public-api-snapshots.sh``. Copy either into +the workflow "just to check" and the copies drift: the day they disagree, the +promise file still exists and still passes, while the promise itself has +quietly moved into the file reviewers skim, or a fourth snapshot is generated +and never diffed. So this guards the wiring rather than the surface -- CI's own diff is what -guards the surface. Each check below is a thing somebody could plausibly write, -with the reason it must not be written. +guards the surface -- plus the one documentation claim that a reader would act +on: what the promise file does *not* cover. Each check below is a thing +somebody could plausibly write, with the reason it must not be written. """ +import re +import subprocess from pathlib import Path import pytest @@ -23,33 +27,52 @@ README = REPO_ROOT / "README.md" RUST = REPO_ROOT / "rust" -# The three files, and the promise each one carries. -SNAPSHOTS = ( - RUST / "devlaunch-core" / "public-api.api.txt", - RUST / "devlaunch-core" / "public-api.rest.txt", - RUST / "devlaunch-runner" / "public-api.txt", -) # The classification itself: the pattern that decides "promise" from "rest". API_ROW_PATTERN = "devlaunch_core::api\\b" +# The ticket that widens the classifier to cover a promised type's methods and +# impls. Named in the docs so the gap is a known follow-up rather than folklore. +WIDENING_TICKET = "352" + + +def script_files() -> list[str]: + """The snapshots the script says it writes, as it says them. + + Asking the script rather than listing them here for the same reason CI asks + it: this file is one of the places a stale copy could hide. + """ + printed = subprocess.run( + [str(SCRIPT), "--print-files"], + capture_output=True, + text=True, + check=True, + ) + return printed.stdout.split() def ci_job(name: str) -> str: """One job of ci.yml, as text. A slice rather than a parse: there is no YAML parser in this project's - dependencies, and the jobs are the only things at that indentation. + dependencies. The slice ends at the next line that starts a sibling key at + the jobs' own indentation -- two spaces then non-space -- rather than at the + first blank line, which a cosmetic blank line inside a job's `run:` block + would otherwise be mistaken for. """ text = CI_WORKFLOW.read_text(encoding="utf-8") - start = text.index(f"\n {name}:\n") - rest = text[start + 1 :] - end = rest.find("\n\n ") - return rest if end == -1 else rest[:end] + start = text.index(f"\n {name}:\n") + 1 + sibling = re.compile(r"^ \S", re.MULTILINE) + following = sibling.search(text, start + 1) + job = text[start : following.start()] if following else text[start:] + # A slice that lost the job's own body would make every assertion below + # vacuous, and a slice that ran to EOF would make them all trivially true. + assert "runs-on:" in job, f"the {name} job slice does not contain the job" + return job @pytest.mark.unit -def test_every_snapshot_the_split_promises_is_checked_in(): - missing = [str(path.relative_to(REPO_ROOT)) for path in SNAPSHOTS if not path.is_file()] - assert not missing, f"the split names these files and the repo does not carry them: {missing}" +def test_every_snapshot_the_script_writes_is_checked_in(): + missing = [name for name in script_files() if not (RUST / name).is_file()] + assert not missing, f"the script writes these files and the repo does not carry them: {missing}" @pytest.mark.unit @@ -66,19 +89,37 @@ def test_the_classification_lives_in_the_script_alone(): assert API_ROW_PATTERN in SCRIPT.read_text(encoding="utf-8"), ( "the split filter is not in the regeneration script" ) - assert "devlaunch_core::api" not in CI_WORKFLOW.read_text(encoding="utf-8"), ( + # Settings, not commentary: the job's comment block explains the tiers and + # names the api path to do it, which is where that explanation belongs. + settings = "\n".join( + line for line in ci_job("public-api").splitlines() if not line.lstrip().startswith("#") + ) + assert "devlaunch_core::api" not in settings, ( "ci.yml classifies rows itself; it must run the script instead, or the two " "definitions of 'promised' will drift apart" ) @pytest.mark.unit -def test_ci_checks_all_three_snapshots_by_running_the_script(): +def test_ci_takes_the_file_list_from_the_script_rather_than_repeating_it(): job = ci_job("public-api") assert "scripts/public-api-snapshots.sh" in job, "the public-api job does not run the script" - for path in SNAPSHOTS: - relative = str(path.relative_to(RUST)) - assert relative in job, f"the public-api job never diffs {relative}" + assert "--print-files" in job, ( + "the public-api job does not ask the script which files to diff; a list it " + "keeps itself is a list a fourth snapshot can fall off" + ) + # The machinery, not the prose: the error message may well name the promise + # file, because telling the developer which diff means what is its whole + # job. What must not name one is the check itself -- a `diff` line with a + # path in it is a list of files to compare that lives here. + comparisons = [line for line in job.splitlines() if "diff -u" in line] + assert comparisons, "the public-api job compares nothing" + for line in comparisons: + duplicated = [name for name in script_files() if name in line] + assert not duplicated, ( + f"the diff in ci.yml names snapshot paths of its own ({duplicated}); " + "--print-files is there so it does not have to" + ) @pytest.mark.unit @@ -101,3 +142,25 @@ def test_regenerating_is_documented_outside_the_ci_error_string(): assert "scripts/public-api-snapshots.sh" in readme, ( "the README section does not name the command that regenerates them" ) + + +@pytest.mark.unit +def test_the_docs_say_what_the_promise_file_does_not_cover(): + """The overclaim this section is one edit away from becoming again. + + `cargo public-api` renders methods and impls only at a type's canonical + path, so `api::Launch::run` is in the *rest* file and renaming it leaves the + promise file byte-identical. A guard that is trusted and silently does not + fire is worse than no guard, so the limit is documented where the guard is, + and the ticket that closes it is named. + """ + for path in (README, SCRIPT, RUST / "devlaunch-core" / "src" / "lib.rs"): + text = path.read_text(encoding="utf-8") + assert "canonical" in text, ( + f"{path.name} describes the promise file without the canonical-path limit " + "that decides what it can see" + ) + assert WIDENING_TICKET in text, ( + f"{path.name} states the limit without naming issue #{WIDENING_TICKET}, " + "which is what turns a known gap into a tracked one" + ) From b89cca9ea0a7fc2aa80dd3aafa50f693f0780194 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 21:09:51 +0000 Subject: [PATCH 6/7] A green tick must mean compared, not merely unchanged Three follow-ups from the re-review. The `--print-files` loop could report success having diffed nothing: `set -euo pipefail` does not see a process substitution fail, and a `while read` over no input runs zero times leaving `changed=0`. Reproduced against the shipped shell with two stubs -- one printing nothing, one exiting 3 -- both green. There is now a `checked` counter and an explicit refusal, in the same spirit as the `gate` job's "this gate covers no jobs" check, and both stubs go red. That is pinned by tests that run ci.yml's own step text rather than a paraphrase of it, against a stubbed generator: a control where every snapshot matches, one where a snapshot differs, and one where the list is empty. Deleting the counter block -- the shape shipped in 895d034 -- fails the third; deleting only the increment fails the control; making the diff compare a file with itself fails the second. Staging moves inside the destination. `/tmp` is a different filesystem from a checkout here (device 81 against 66306), so each `mv` was a copy-and-unlink that could truncate a destination; same-filesystem staging makes every move a rename. Destinations are also checked writable before anything is generated, because the mixed set the review reproduced -- first two moves applied, third refused -- passes all four partition invariants and is caught only by CI. That case now refuses in 66ms having generated nothing. The README claim that "a failed run leaves the checked-in snapshots exactly as they were" was true of a failed generation only, so it now says which failures leave what, including the one window that remains: a crash between renames leaves whole files but a mixed set. --- .github/workflows/ci.yml | 14 ++++ README.md | 14 +++- scripts/public-api-snapshots.sh | 23 +++++- test/test_public_api_snapshots_doc.py | 105 ++++++++++++++++++++++++++ 4 files changed, 152 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f318dc0f..ddd3552e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -427,9 +427,23 @@ jobs: set -euo pipefail scripts/public-api-snapshots.sh "$RUNNER_TEMP/public-api" changed=0 + checked=0 while read -r snapshot; do + checked=$((checked + 1)) diff -u "rust/$snapshot" "$RUNNER_TEMP/public-api/$snapshot" || changed=1 done < <(scripts/public-api-snapshots.sh --print-files) + # Count what was compared, in the same spirit as the `gate` job's + # "this gate covers no jobs" check. `set -e` does not see a process + # substitution fail, and a loop that runs zero times leaves changed=0 + # — so without this, a --print-files that printed nothing (or exited + # non-zero) would report success having diffed not one file, which is + # the exact shape of tripwire this job exists to stop being. + if [ "$checked" -eq 0 ]; then + echo "::error::--print-files listed no snapshots, so this job compared" \ + "nothing. The list lives in scripts/public-api-snapshots.sh; a green" \ + "tick here would mean the public surface is unchecked, not unchanged." + exit 1 + fi if [ "$changed" -ne 0 ]; then echo "::error::A public surface changed. If the change is deliberate," \ "regenerate the snapshots and commit them:" \ diff --git a/README.md b/README.md index aa7bc7e8..d098a1d8 100644 --- a/README.md +++ b/README.md @@ -1925,9 +1925,17 @@ Committing a regenerated `public-api.api.txt` is committing a change to the prom say which one in the pull request — and if the change was to a promised type's methods or impls, the diff to point at is in `public-api.rest.txt`. `rust/devlaunch-core/tests/public_api_snapshots.rs` holds the two core files to the split itself — every promised row is an `api` declaration and none -of the others is — so a hand-edited snapshot fails in the Rust suite rather than in review. The -script writes through a staging directory and moves the files into place only once all three -generated, so a failed run leaves the checked-in snapshots exactly as they were. +of the others is — so a hand-edited snapshot fails in the Rust suite rather than in review. + +**What a failed run leaves behind**, precisely, because "nothing" would be a claim rather than a +fact. The script checks every destination is writable before it generates anything, then writes +into a staging directory *inside* the destination and moves the files into place only once all +three exist. So a run that fails while generating — a compile error, a guard firing, a Ctrl-C — +leaves the checked-in snapshots byte-identical. Staging on the same filesystem makes each move a +rename rather than a copy, so no file is ever seen half-written. What is *not* atomic is the set of +three: a crash between renames leaves some files new and some old, each one whole. CI's +regenerate-and-diff is what catches that, since a mixed set still satisfies every invariant the +tests over these files can check. ### Coverage: two numbers, and neither is the other diff --git a/scripts/public-api-snapshots.sh b/scripts/public-api-snapshots.sh index 054e75c7..118c1e24 100755 --- a/scripts/public-api-snapshots.sh +++ b/scripts/public-api-snapshots.sh @@ -119,13 +119,34 @@ if [[ "$installed" != "cargo-public-api $PIN" ]]; then exit 1 fi +# Refuse before generating rather than half-way through applying. Two minutes +# of rustdoc followed by "permission denied" on the third move is the worst +# version of this: measured, it leaves two files updated and one stale, and a +# mixed set satisfies every invariant the tests over these files can check -- +# the partition holds on any complementary pair -- so only CI's +# regenerate-and-diff would notice. +for file in "${FILES[@]}"; do + if [[ ! -w "$dest/$(dirname "$file")" ]]; then + echo "cannot write $dest/$(dirname "$file"): nothing generated, nothing changed." >&2 + exit 1 + fi +done + # Stage every file, and move them into place only once all three exist. The # shell truncates a redirect target *before* the command on its right runs, so # writing the destinations directly means a failed generation -- or a guard # below firing -- empties a checked-in snapshot on the way to reporting the # problem. That is a script whose whole job is to write those files leaving # them at zero bytes, and only one of the tests over them notices. -staging="$(mktemp -d)" +# +# Staged *inside* `$dest`, not in `/tmp`: a real checkout is a different +# filesystem from `/tmp` (measured: device 66306 against 81), which makes every +# `mv` a copy-and-unlink rather than a rename, so an interrupted move can leave +# a destination half-written. Same filesystem means each move is a rename, and +# a rename either happened or did not. The set of three is still not one atomic +# act -- a crash between renames leaves some new and some old, each file whole +# -- and CI's diff is what catches that. +staging="$(mktemp -d "$dest/.staging.XXXXXX")" trap 'rm -rf "$staging"' EXIT for file in "${FILES[@]}"; do mkdir -p "$staging/$(dirname "$file")" diff --git a/test/test_public_api_snapshots_doc.py b/test/test_public_api_snapshots_doc.py index 7319e356..d75c5433 100644 --- a/test/test_public_api_snapshots_doc.py +++ b/test/test_public_api_snapshots_doc.py @@ -15,6 +15,7 @@ somebody could plausibly write, with the reason it must not be written. """ +import os import re import subprocess from pathlib import Path @@ -27,6 +28,8 @@ README = REPO_ROOT / "README.md" RUST = REPO_ROOT / "rust" +# The step whose shell the executable checks at the bottom of this file run. +CI_CHECK_STEP = "The public surface is the snapshots the repo carries" # The classification itself: the pattern that decides "promise" from "rest". API_ROW_PATTERN = "devlaunch_core::api\\b" # The ticket that widens the classifier to cover a promised type's methods and @@ -164,3 +167,105 @@ def test_the_docs_say_what_the_promise_file_does_not_cover(): f"{path.name} states the limit without naming issue #{WIDENING_TICKET}, " "which is what turns a known gap into a tracked one" ) + + +def ci_step_script(job: str, step_name: str) -> str: + """The shell of one step of a job, dedented and runnable. + + The checks below run the workflow's own text rather than a paraphrase of + it, because a paraphrase is another copy: the day the two disagree, the + test still passes and the thing that runs on a runner is the other one. + """ + lines = job.splitlines() + at = next(i for i, line in enumerate(lines) if line.strip() == f"- name: {step_name}") + run = next(i for i in range(at, len(lines)) if lines[i].strip() == "run: |") + body: list[str] = [] + indent = None + for line in lines[run + 1 :]: + if not line.strip(): + body.append("") + continue + here = len(line) - len(line.lstrip()) + if indent is None: + indent = here + elif here < indent: + break + body.append(line[indent:]) + assert body, f"the {step_name!r} step has no shell to run" + return "\n".join(body) + + +def run_the_ci_check(tmp_path: Path, listed: list[str], differing: str | None = None): + """Run ci.yml's check step in a fake checkout, against a stubbed script. + + The stub stands in for the regeneration -- there is no nightly toolchain in + this environment, and none is needed to test the *checking*. It writes + whatever it claims to write, so the step's diff has something real to + compare; ``listed`` is what its ``--print-files`` prints. + """ + root = tmp_path / "checkout" + (root / "scripts").mkdir(parents=True) + stub = root / "scripts" / "public-api-snapshots.sh" + stub.write_text( + "#!/usr/bin/env bash\n" + "set -euo pipefail\n" + f"listed=({' '.join(listed)})\n" + 'if [ "${1:-}" = "--print-files" ]; then\n' + ' [ ${#listed[@]} -eq 0 ] || printf "%s\\n" "${listed[@]}"\n' + " exit 0\n" + "fi\n" + 'dest="$1"\n' + 'for file in ${listed[@]+"${listed[@]}"}; do\n' + ' mkdir -p "$dest/$(dirname "$file")"\n' + ' echo generated > "$dest/$file"\n' + "done\n", + encoding="utf-8", + ) + stub.chmod(0o755) + for name in listed: + checked_in = root / "rust" / name + checked_in.parent.mkdir(parents=True, exist_ok=True) + checked_in.write_text("drifted\n" if name == differing else "generated\n", encoding="utf-8") + runner_temp = tmp_path / "runner-temp" + runner_temp.mkdir() + return subprocess.run( + ["bash", "-c", ci_step_script(ci_job("public-api"), CI_CHECK_STEP)], + cwd=root, + env={"PATH": os.environ["PATH"], "RUNNER_TEMP": str(runner_temp)}, + capture_output=True, + text=True, + check=False, + ) + + +@pytest.mark.unit +def test_the_ci_check_passes_when_every_snapshot_matches(tmp_path): + """The control: without it, the two failures below prove only a broken harness.""" + done = run_the_ci_check(tmp_path, ["a/one.txt", "b/two.txt", "c/three.txt"]) + assert done.returncode == 0, done.stdout + done.stderr + + +@pytest.mark.unit +def test_the_ci_check_fails_when_a_snapshot_differs(tmp_path): + done = run_the_ci_check(tmp_path, ["a/one.txt", "b/two.txt"], differing="b/two.txt") + assert done.returncode != 0, "a changed surface passed the check" + + +@pytest.mark.unit +def test_the_ci_check_fails_when_it_compared_nothing(tmp_path): + """A job that compares no file must not report that nothing changed. + + `set -euo pipefail` does not see a process substitution fail, and a + `while read` over no input runs its body zero times and leaves the + changed-flag at 0 -- so an empty file list, or a `--print-files` that + exited non-zero, reported success having diffed nothing at all. + """ + done = run_the_ci_check(tmp_path, []) + assert done.returncode != 0, ( + "the public-api job passed having compared nothing; a tripwire that " + "reports success without checking is the failure this ticket is about" + ) + assert "compared" in done.stdout + done.stderr, ( + "the failure does not say that nothing was compared, so whoever hits it " + "will look for a surface change that is not there" + ) From 07227647cd46d16edc5ef7d2dfe5ee1e016a1455 Mon Sep 17 00:00:00 2001 From: Austin Gregg-Smith Date: Sat, 22 Aug 2026 21:28:37 +0000 Subject: [PATCH 7/7] Ignore the snapshot script's staging directory The staging directory now lives beside its destination inside the repo so the moves into place are renames. Every exit path the script controls removes it, but a SIGKILL can leave one untracked where git add -A would sweep it in. Refs #338 --- .gitignore | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.gitignore b/.gitignore index c56bcbf6..3165c6c8 100644 --- a/.gitignore +++ b/.gitignore @@ -194,3 +194,10 @@ rust/target/ # `rust-coverage` CI job). The instrumented target directory is already covered by # the `target/` rule above. rust/lcov.info + +# `scripts/public-api-snapshots.sh` stages the regenerated snapshots beside their +# destination — same filesystem, so the moves into place are renames rather than +# copy+unlink. The directory is removed on every exit path the script controls; +# a SIGKILL is the one that can leave it behind, and this keeps that leftover out +# of a `git add -A`. +.staging.*