Split the API snapshots: runner crate, api-vs-rest

.github/workflows/ci.yml

@@ -385,24 +385,37 @@ 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 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 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, 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
@@ -413,19 +426,41 @@ 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
+          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:" \
+              "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 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
 

.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.*

CHANGELOG.md

@@ -7,6 +7,22 @@ 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 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. 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
+  the pinned `cargo-public-api` exist in one place; see "The public-API snapshots" in README.md.
+
 ### Fixed
 
 - **A `kill` or a closed terminal now runs the same cleanup Ctrl-C does.** Only

README.md

@@ -2174,6 +2174,69 @@ 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 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::<<devlaunch_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 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 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 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 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.
+
+**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
 
 The crates that ship and the harness that judges them are measured separately, because they are

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<alloc::string::String>
+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<alloc::vec::Vec<devlaunch_core::flows::listing::ListedWorkspace>, devlaunch_core::clients::devpod::ListingUnreadable>
+pub fn devlaunch_core::api::identity(&str) -> core::result::Result<devlaunch_core::domain::spec::SpecIdentity<'_>, 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<devlaunch_core::domain::spec::DevcontainerPath, devlaunch_core::domain::spec::DevcontainerRefError>
+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<devlaunch_core::flows::lifecycle::LifecycleNotice>) -> core::result::Result<devlaunch_core::flows::lifecycle::DeleteOutcome, devlaunch_core::clients::devpod::NotRun>
+pub fn devlaunch_core::api::workspace_stop(&mut devlaunch_core::flows::listing::CommandContext<'_>, &mut devlaunch_core::flows::lifecycle::Refresh<'_>, &str) -> core::result::Result<devlaunch_core::flows::lifecycle::StopOutcome, devlaunch_core::clients::devpod::NotRun>