From e03c06d8b458e23670ec6b8629711dbc0a939601 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 25 Aug 2026 15:53:43 -0700 Subject: [PATCH 1/6] =?UTF-8?q?chore(wip):=20batch=20#360=20#361=20#363=20?= =?UTF-8?q?#364=20#367=20=E2=80=94=20stub=20for=20lane=20ownership?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 16dfaf62..1f30e435 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ edition = "2021" # the ROOT manifest (`[workspace.package].version`), so it MUST be set here for a # release to fire (§3.6). The library crates (dig-node-core/dig-runtime/dig-wallet) # keep their own independent versions — only the released binary tracks the workspace version. -version = "0.154.0" +version = "0.155.0" # Release hardening, matching digstore: keep integer-overflow checks ON in release. # The node parses untrusted serialized input and does offset/length arithmetic over From 79275fdf9610dd5d066e3ad071ea8341300c994d Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 25 Aug 2026 16:07:36 -0700 Subject: [PATCH 2/6] refactor(wallet): delete the dead oracle-first ChainTransport constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ChainTransport::with_sources` had no callers anywhere in the workspace, was `pub` + `#[must_use]`, and built `peer_reads: None` — a transport whose `peak_height` takes `chia-query`'s router path, which asks `api.coinset.org` FIRST. It read like the obvious way to build a transport over the node's own fabric while quietly being the one shape NC-12 exists to prevent: the node's headline chain fact decided by a single third party. Deleted rather than repaired: a constructor kept "in case someone needs it" is how it comes back, and the only production construction (`sage/service.rs:173`) already chains `with_peer_reads`. Makes the deletion durable with a fail-closed source guard: the constructors that may build `peer_reads: None` are a CLOSED enumeration of two (`new`, and the `#[cfg(test)]` `with_client`), and a third one fails the suite by name. Proved load-bearing by reintroducing `with_sources` — the guard reports it. Closes #360 Co-Authored-By: Claude --- Cargo.lock | 2 +- crates/dig-wallet/src/sage/chain.rs | 69 +++++++++++++++++++++++++---- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 34313c77..e68e3987 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3012,7 +3012,7 @@ dependencies = [ [[package]] name = "dig-node-service" -version = "0.154.0" +version = "0.155.0" dependencies = [ "async-trait", "axum", diff --git a/crates/dig-wallet/src/sage/chain.rs b/crates/dig-wallet/src/sage/chain.rs index e411b630..1e4cbd6d 100644 --- a/crates/dig-wallet/src/sage/chain.rs +++ b/crates/dig-wallet/src/sage/chain.rs @@ -106,15 +106,6 @@ impl ChainTransport { } } - /// A transport reading through `sources` — the node's one registry-owned fabric. - #[must_use] - pub fn with_sources(sources: Arc) -> Self { - Self { - sources, - peer_reads: None, - } - } - /// Serve arbitrary coin reads from this node's OWN peers, corroborated and cached in `db`. /// /// Without this the two arbitrary reads fall through to the third-party oracle, which a node @@ -708,4 +699,64 @@ mod corroborated_peak_tests { ); assert_eq!(reported, None); } + + /// **Every oracle-first `ChainTransport` in this file is one of two NAMED constructors, and + /// the list is closed.** + /// + /// A transport built with `peer_reads: None` answers [`ChainTransport::peak_height`] from + /// `chia-query`'s router, whose first move is to ask `api.coinset.org` — one third party + /// deciding the node's headline chain fact. Exactly two constructors are allowed to produce + /// that shape and neither is reachable from production: `new`, which every real caller + /// immediately chains `with_peer_reads` onto, and the `#[cfg(test)]` `with_client`. + /// + /// This guard exists because `with_sources` was a THIRD one (dig-node#360) — `pub`, `#[must_use]`, + /// named as though it were the ordinary way to build a transport over the node's own fabric, + /// and silently oracle-first. It had no callers, so nothing failed; it was deleted, and this is + /// what makes the deletion durable rather than a one-time tidy-up. + /// + /// **Fail-closed by construction:** a new site is a FAILURE, never a silent pass. Adding a + /// legitimate one means naming it here, which is the review moment the ticket asks for. + /// + /// The needle is the struct-literal FIELD form (trailing comma), not the prose form, so the + /// doc comments that discuss this shape are not counted as sites. And it is assembled at run + /// time from two fragments so this test cannot match ITSELF — + /// a source-scanning guard that finds its own needle reports a site that does not exist and + /// passes for the wrong reason. + #[test] + fn the_oracle_first_constructors_are_a_closed_enumeration_of_two() { + const ALLOWED: [&str; 2] = ["new", "with_client"]; + + let source = include_str!("chain.rs"); + let needle = ["peer_reads", ": None,"].concat(); + + let mut found: Vec<&str> = Vec::new(); + for (offset, _) in source.match_indices(&needle) { + // Walk back to the nearest `fn ` and read the identifier that follows it. The nearest + // preceding `fn` is the enclosing one for every construction site in this file. + let before = &source[..offset]; + let fn_at = before + .rfind("fn ") + .expect("a construction site outside any function"); + let name = before[fn_at + 3..] + .split(|c: char| !c.is_alphanumeric() && c != '_') + .next() + .expect("a `fn` with no name"); + found.push(name); + } + + assert!( + !found.is_empty(), + "the scan matched nothing, so it proves nothing — the needle no longer describes how \ + an oracle-first transport is written, and this guard has gone vacuous" + ); + + let unlisted: Vec<&&str> = found.iter().filter(|n| !ALLOWED.contains(n)).collect(); + assert!( + unlisted.is_empty(), + "these constructors build an oracle-first ChainTransport and are not on the closed \ + list {ALLOWED:?}: {unlisted:?} — a transport whose peak_height is one HTTPS \ + endpoint's opinion (NC-12, dig-node#360). Attach peer reads, or add the name here \ + deliberately." + ); + } } From 3c16e9d53180daccb42e241539a23242bfe166c0 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 25 Aug 2026 17:08:01 -0700 Subject: [PATCH 3/6] fix(test): stop the serve harness leaking a ~57 MB temp tree per node `content_serve.rs` built its temp path by hand -- `env::temp_dir().join(format! ("dig-node-serve-test-{pid}-{n}"))` -- and nothing ever removed it. Each node seeds a real compiled `.dig` module and warms a cache, so a run cost ~57 MB per node. 1,123 trees reached 62.5 GB and took the dev machine to 81 MB free on a 1.9 TB disk, producing a machine-wide ENOSPC that stopped an unrelated lane mid-build. It is self-concealing: it grows fastest when the suite runs most, so it reads like a build-cache problem (the first diagnosis blamed worktree `target/` dirs, which were not the cause). - A `NodeCache` RAII guard OWNS the tree (`tempfile::TempDir`), so removal happens in `Drop` -- including on an unwind. Ownership is the point, not the deletion: a cleanup line at the end of a test is skipped by every failing assertion, i.e. exactly the runs a developer repeats. - Both leaking sites in the file are covered (`dig-node-serve-test-*` and `dig-node-origin-test-*`); the latter now shares the guard. - The node's detached axum task still holds `wallet.sqlite` open when the test body returns, and Windows will not unlink an open file, so `TempDir::drop` cannot remove that last ~1 MB. `Drop` cannot cancel the task (it cannot `.await`), so the residue is BOUNDED rather than pretended away: a once-per- process sweep removes trees idle for 15 minutes, which no live run can be. Measured, same machine, full suite: per-run residue 970 MB -> 17 MB, and the 62.5 GB accumulation is gone. Two tests hold it: one asserts the tree is gone after a normal drop AND after a `catch_unwind` panic; one asserts the sweep removes an abandoned tree while sparing a live one and a stranger's directory. Each was proved load-bearing by reverting only its own fix. Closes #361 Co-Authored-By: Claude --- .../dig-node-service/tests/content_serve.rs | 253 ++++++++++++++++-- 1 file changed, 228 insertions(+), 25 deletions(-) diff --git a/crates/dig-node-service/tests/content_serve.rs b/crates/dig-node-service/tests/content_serve.rs index fc6b5127..4db453fd 100644 --- a/crates/dig-node-service/tests/content_serve.rs +++ b/crates/dig-node-service/tests/content_serve.rs @@ -29,12 +29,118 @@ fn env_guard() -> Arc> { .clone() } -static SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); - /// RAII release of the env-serialization lock (held for the whole test). #[must_use] struct EnvHold(#[allow(dead_code)] tokio::sync::OwnedMutexGuard<()>); +/// A node's temp tree, **owned** — removed when the test's binding drops, including on panic. +/// +/// # Why this is a type and not a `PathBuf` plus a cleanup line (dig-node#361) +/// +/// This harness used to build its path by hand, `env::temp_dir().join(format!("dig-node-serve-\ +/// test-{pid}-{n}"))`, and nothing ever removed it. Each node seeds a real compiled `.dig` module +/// and warms a cache, so a run costs ~57 MB. **1,123 of them reached 62.5 GB and took the dev +/// machine to 81 MB free on a 1.9 TB disk**, producing a machine-wide `ENOSPC` that stopped an +/// unrelated lane mid-build. It is self-concealing: it grows fastest exactly when the suite runs +/// most, so it surfaces during heavy development and reads like a build-cache problem. +/// +/// **Ownership is the point, not the deletion.** A `remove_dir_all` at the end of each test would +/// be skipped by every failing assertion — the runs most worth diagnosing leak, and a panic +/// unwinding past a deferred cleanup placed after an `.await` leaks whenever anything catches it. +/// `TempDir` removes the tree in `Drop`, which unwinding runs. +struct NodeCache { + /// The whole per-node tree (`//`): cache, state dir, everything. + dir: tempfile::TempDir, + /// `/cache`, the path handed to `DIG_NODE_CACHE` and to [`seed_module`]. + cache: PathBuf, +} + +/// Every temp tree this harness has ever created carries this prefix — the guard's own name for +/// itself, and what [`sweep_stale_trees`] matches on. +const TREE_PREFIX: &str = "dig-node-serve-test-"; + +/// A tree untouched for this long cannot belong to a live run. +/// +/// Only the `content_serve` binary creates these, and it finishes in ~2.5 minutes, so a tree idle +/// for fifteen has no owner. Chosen as a safety margin rather than a tuning knob: being too eager +/// deletes a concurrent lane's working directory and manufactures a flaky failure somewhere else, +/// while being too patient costs only one more run's worth of small files. +const STALE_AFTER: std::time::Duration = std::time::Duration::from_secs(15 * 60); + +/// Remove trees left by EARLIER runs, once per test process. +/// +/// # Why a sweep is needed at all when the guard is RAII +/// +/// The per-node [`NodeCache`] drop removes the ~57 MB cache tree, which is the whole of the disk +/// cost. What it cannot always remove is the node's `wallet.sqlite` (~428 KB with its `-wal` and +/// `-shm`): the axum task serving the node is detached and still holds the handle when the test +/// body returns, and **Windows refuses to unlink an open file**. `TempDir::drop` swallows that +/// error, so the tree survives with its database in it. +/// +/// Fixing that inside `Drop` is not possible honestly — cancelling the server task needs the +/// runtime to poll it, and `Drop` cannot `.await`. So the residue is **bounded** instead of +/// pretended away, which dig-node#361 names as the acceptable second option: one run's worth of +/// small files at any time, rather than unbounded growth. The 62.5 GB / `ENOSPC` failure the +/// ticket was filed for is removed by the guard; this keeps the entry COUNT flat. +fn sweep_stale_trees() { + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| sweep_trees_in(&std::env::temp_dir(), STALE_AFTER)); +} + +/// The sweep itself, over an explicit directory and threshold. +/// +/// Split out from [`sweep_stale_trees`] so it is reachable from a test: the `Once` fires before +/// the first [`NodeCache`], and a `Once` cannot be made to fire twice, so a test that could only +/// call the wrapper could never observe what it did. +fn sweep_trees_in(root: &Path, stale_after: std::time::Duration) { + let Ok(entries) = std::fs::read_dir(root) else { + return; + }; + for entry in entries.flatten() { + if !entry.file_name().to_string_lossy().starts_with(TREE_PREFIX) { + continue; + } + // Unreadable metadata is treated as RECENT, so an entry this code cannot judge is left + // alone. The fail-safe direction is keeping a stranger's directory, never removing it. + let recently_touched = entry + .metadata() + .and_then(|m| m.modified()) + .and_then(|t| t.elapsed().map_err(std::io::Error::other)) + .map_or(true, |age| age < stale_after); + if recently_touched { + continue; + } + // Best effort by design: a tree whose database is still open elsewhere simply stays, and + // the next run tries again. + let _ = std::fs::remove_dir_all(entry.path()); + } +} + +impl NodeCache { + /// A fresh, uniquely-named tree with its `cache` subdirectory created. + fn new() -> Self { + sweep_stale_trees(); + let dir = tempfile::Builder::new() + .prefix(TREE_PREFIX) + .tempdir() + .expect("temp dir for a serve-test node"); + let cache = dir.path().join("cache"); + std::fs::create_dir_all(&cache).expect("cache dir"); + Self { dir, cache } + } + + /// The tree root — the node's `DIG_NODE_STATE_DIR`. + fn root(&self) -> &Path { + self.dir.path() + } +} + +impl AsRef for NodeCache { + fn as_ref(&self) -> &Path { + &self.cache + } +} + /// Compile a REAL public `.dig` module (the SAME `digstore_stage::stage_and_compile` engine the node /// depends on) with a `PublicManifest` section. Returns `(root, module_bytes)`. fn compile_public_module(store_id: Bytes32, files: &[(String, Vec)]) -> (Bytes32, Vec) { @@ -70,8 +176,8 @@ fn compile_public_module(store_id: Bytes32, files: &[(String, Vec)]) -> (Byt /// Seed a compiled module into the node's on-disk cache at its canonical `(store, root)` path /// (`/modules//.module`) so the local-first serve finds it. -fn seed_module(cache: &Path, store_hex: &str, root_hex: &str, bytes: &[u8]) { - let dir = cache.join("modules").join(store_hex); +fn seed_module(cache: impl AsRef, store_hex: &str, root_hex: &str, bytes: &[u8]) { + let dir = cache.as_ref().join("modules").join(store_hex); std::fs::create_dir_all(&dir).unwrap(); std::fs::write(dir.join(format!("{root_hex}.module")), bytes).unwrap(); } @@ -98,7 +204,7 @@ async fn mock_upstream_all_miss() -> String { /// Start the service app on an ephemeral loopback port with a unique temp cache, the pin OFF (so the /// serve is hermetic — no coinset), and the given upstream. Returns the bound addr, the cache dir (to /// seed a module into), and the env-serialization hold. -async fn start_server(upstream: &str) -> (SocketAddr, PathBuf, EnvHold) { +async fn start_server(upstream: &str) -> (SocketAddr, NodeCache, EnvHold) { let hold = env_guard().lock_owned().await; let (addr, cache) = spawn_node(upstream).await; (addr, cache, EnvHold(hold)) @@ -108,19 +214,12 @@ async fn start_server(upstream: &str) -> (SocketAddr, PathBuf, EnvHold) { /// lock, so a single test can stand up two nodes (a reader and the gateway it falls back to) inside /// one hold. Each node captures its own cache dir at construction, so the process-global /// `DIG_NODE_CACHE` may be repointed between calls. -async fn spawn_node(upstream: &str) -> (SocketAddr, PathBuf) { - let unique = SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); - let base = std::env::temp_dir().join(format!( - "dig-node-serve-test-{}-{}", - std::process::id(), - unique - )); - let cache = base.join("cache"); - std::fs::create_dir_all(&cache).unwrap(); - std::env::set_var("DIG_NODE_CACHE", &cache); +async fn spawn_node(upstream: &str) -> (SocketAddr, NodeCache) { + let cache = NodeCache::new(); + std::env::set_var("DIG_NODE_CACHE", &cache.cache); // Isolate the #501 control-token/paired-token state dir per test (identity-independent), so a // host with a real machine state dir can't defeat the temp isolation. - std::env::set_var("DIG_NODE_STATE_DIR", &base); + std::env::set_var("DIG_NODE_STATE_DIR", cache.root()); // Hermetic: disable the chain-anchored pin so the serve resolves against the requested root with // NO coinset call (the node-side gate only; a real deploy leaves the pin ON). std::env::set_var("DIG_NODE_PIN", "off"); @@ -653,15 +752,11 @@ async fn drive_s_get( use tower::ServiceExt; let hold = env_guard().lock_owned().await; - let unique = SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); - let base = std::env::temp_dir().join(format!( - "dig-node-origin-test-{}-{}", - std::process::id(), - unique - )); - std::fs::create_dir_all(base.join("cache")).unwrap(); - std::env::set_var("DIG_NODE_CACHE", base.join("cache")); - std::env::set_var("DIG_NODE_STATE_DIR", &base); + // Owned for the body of this helper, so the tree goes when it returns OR panics. The same + // hand-rolled leak as `spawn_node` had (`dig-node-origin-test-*`), fixed the same way. + let base = NodeCache::new(); + std::env::set_var("DIG_NODE_CACHE", &base.cache); + std::env::set_var("DIG_NODE_STATE_DIR", base.root()); std::env::set_var("DIG_NODE_PIN", "off"); let config = dig_node_service::Config { upstream: "http://127.0.0.1:1/unreachable".to_string(), @@ -928,3 +1023,111 @@ async fn health_reports_the_peer_tier_as_unattached_before_the_peer_network_star "a live node with no peer network must say so rather than leaving it unstated" ); } + +/// **The temp tree is removed when the guard drops — including when the drop is a PANIC unwinding +/// out of a failing test.** +/// +/// This is dig-node#361's regression, and the panic half is the half that matters. The leak was +/// not "someone forgot the cleanup line"; it was that a cleanup line cannot run on the failing +/// runs, which are exactly the runs a developer repeats. 1,123 leaked trees reached 62.5 GB and +/// took the machine to 81 MB free. +/// +/// **Asserted on the path, after the guard is gone.** Asserting that `Drop` was *reached* would +/// pass for a `Drop` that reached a `remove_dir_all` and ignored its error, which is precisely the +/// silent failure mode of the type being tested. +/// +/// No node is started here on purpose: this pins the GUARD, and a live node's open `wallet.sqlite` +/// handle is a separate, bounded residue that [`sweep_stale_trees`] owns. +#[test] +fn the_temp_tree_is_removed_on_drop_and_on_panic() { + let cache = NodeCache::new(); + let normal = cache.root().to_path_buf(); + std::fs::write( + normal.join("cache").join("seeded.bin"), + b"57 MB stands in here", + ) + .unwrap(); + assert!( + normal.is_dir(), + "the fixture never existed, so nothing is proven" + ); + drop(cache); + assert!( + !normal.exists(), + "a guard dropped normally left its tree at {}", + normal.display() + ); + + // The panic path. `catch_unwind` is what makes it observable from inside a passing test; the + // unwind is real, and the guard is dropped by it and not by us. + let leaked = std::sync::Arc::new(std::sync::Mutex::new(PathBuf::new())); + let seen = std::sync::Arc::clone(&leaked); + let outcome = std::panic::catch_unwind(move || { + let cache = NodeCache::new(); + *seen.lock().unwrap() = cache.root().to_path_buf(); + std::fs::write(cache.as_ref().join("seeded.bin"), b"and here").unwrap(); + panic!("a failing assertion, which is when the old harness leaked"); + }); + + assert!( + outcome.is_err(), + "the fixture did not panic, so it proves nothing" + ); + let path = leaked.lock().unwrap().clone(); + assert!( + !path.as_os_str().is_empty(), + "the closure never built a tree, so the assertion below is vacuous" + ); + assert!( + !path.exists(), + "a panic unwound past the guard and left its tree at {} -- this is the ENOSPC defect", + path.display() + ); +} + +/// **The sweep removes an abandoned tree and leaves a live one alone.** +/// +/// Both halves are the test. A sweep that removes everything would pass a +/// "the stale one is gone" assertion while deleting a concurrent lane's working directory +/// mid-run — turning a disk-hygiene fix into a flaky-failure generator in an unrelated repo. So +/// the fresh tree is not decoration: it is the actor that distinguishes this sweep from the +/// nearest wrong one. +/// +/// Run over a scratch directory rather than the real `Temp`, so it cannot delete anything real, +/// and with an explicit threshold rather than [`STALE_AFTER`], so it does not take fifteen +/// minutes to be true. +#[test] +fn the_sweep_removes_an_abandoned_tree_and_spares_a_live_one() { + let scratch = tempfile::tempdir().unwrap(); + let threshold = std::time::Duration::from_millis(300); + + let abandoned = scratch.path().join(format!("{TREE_PREFIX}abandoned")); + std::fs::create_dir_all(abandoned.join("cache")).unwrap(); + std::fs::write(abandoned.join("wallet.sqlite"), b"residue").unwrap(); + + // An unrelated directory that merely shares the temp dir. The sweep must be selective by + // PREFIX as well as by age -- it is pointed at a directory full of other people's files. + let stranger = scratch.path().join("someone-elses-work"); + std::fs::create_dir_all(&stranger).unwrap(); + + std::thread::sleep(threshold * 4); + + let live = scratch.path().join(format!("{TREE_PREFIX}live")); + std::fs::create_dir_all(live.join("cache")).unwrap(); + + sweep_trees_in(scratch.path(), threshold); + + assert!( + !abandoned.exists(), + "the abandoned tree survived, so the count grows without bound" + ); + assert!( + live.is_dir(), + "the sweep deleted a tree young enough to belong to a running test -- this would break \ + a concurrent lane rather than tidy up after this one" + ); + assert!( + stranger.is_dir(), + "the sweep removed a directory that is not one of ours at all" + ); +} From 1970f41e07db6caf3f399e81522eab2569cbe671 Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 25 Aug 2026 17:14:37 -0700 Subject: [PATCH 4/6] ci: report the version comparison the gate made, on every run The version-increment gate held both numbers and reported only pass/fail. A stale bump is invisible everywhere else -- `git status`, the diff, fmt, clippy and the test suite are all green when `main` has taken your version number, and a rebase can drop the bump commit entirely as "already upstream". It went stale three times on one branch (PR #354) across a five-round gate, and each time the lane had to rediscover at merge what the comparison even was. Writes a table to `$GITHUB_STEP_SUMMARY` on EVERY run, success or failure: the base ref actually compared (short SHA of `main`, plus the head SHA), a row per manifest with base, head and verdict, the must-match row where both exist, and the overall verdict spelled out. Being able to READ the comparison while the gate is green is the point -- that is what turns a merge-time surprise into something a long-lived review round can notice. What the gate ENFORCES is unchanged: same predicate, same exit code, same error annotations. `${GITHUB_STEP_SUMMARY:-/dev/null}` keeps the script runnable off Actions, which is how the three cases below were exercised. Bounded to dig-node deliberately (#364, CLAUDE.md 1.3c rule zero). 25 other repos carry a byte-identical copy and 6 carry a diverged one; propagating is a named follow-up with that finite list, not "every repo". Verified by running the extracted script unpiped against scratch repos: passing (RC=0), the stale-bump failure with `main` holding the same version (RC=1), and the both-manifests-agree case (RC=0) -- each producing its summary. Closes #364 Co-Authored-By: Claude --- .../workflows/ensure-version-increment.yml | 51 ++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ensure-version-increment.yml b/.github/workflows/ensure-version-increment.yml index 3c68abaf..1444f06b 100644 --- a/.github/workflows/ensure-version-increment.yml +++ b/.github/workflows/ensure-version-increment.yml @@ -5,6 +5,13 @@ # - BOTH present -> both must increase AND equal each other on this branch # - neither present -> nothing to enforce (passes) # Version files added new on this branch (absent on base) pass. Modeled on Chia-Network/cadt. +# +# The comparison is written to the job summary on EVERY run, pass or fail (dig-node#364). A stale +# bump is invisible everywhere else: `git status`, the diff, fmt, clippy and the test suite are all +# green when `main` has taken your version number, and a rebase can drop the bump commit entirely as +# "already upstream". This gate is the only thing that holds both numbers, so it is the only place +# the comparison can be surfaced -- and being able to read it WHILE the gate is green is what turns +# a merge-time surprise into something a long-lived review round can notice. name: Check Version Increment on: @@ -56,6 +63,22 @@ jobs: # strictly-greater semver-ish compare using sort -V; true when $2 > $1 greater() { [ "$1" != "$2" ] && [ "$(printf '%s\n%s\n' "$1" "$2" | sort -V | tail -1)" = "$2" ]; } + # One row of the comparison table, to the job summary AND the log. + # `${GITHUB_STEP_SUMMARY:-/dev/null}` so the script stays runnable outside Actions. + SUMMARY="${GITHUB_STEP_SUMMARY:-/dev/null}" + row() { printf '| %s | `%s` | `%s` | %s |\n' "$1" "${2:-(absent)}" "${3:-(absent)}" "$4" >> "$SUMMARY"; } + + base_sha=$(git -C base-repo rev-parse --short HEAD) + head_sha=$(git -C branch-repo rev-parse --short HEAD) + { + echo "### Version increment" + echo + echo "Comparing this branch against \`main\` at \`$base_sha\` (head \`$head_sha\`)." + echo + echo "| manifest | base | head | verdict |" + echo "| --- | --- | --- | --- |" + } >> "$SUMMARY" + B=base-repo H=branch-repo has_pkg=false; has_cargo=false @@ -73,12 +96,16 @@ jobs: bp=$(pkg_version "$B"); hp=$(pkg_version "$H") echo "package.json: base='$bp' head='$hp'" if [ -n "$bp" ]; then - if ! greater "$bp" "$hp"; then + if greater "$bp" "$hp"; then + row "package.json" "$bp" "$hp" "increments" + else echo "::error::package.json version must be incremented ($bp -> $hp) before merging." + row "package.json" "$bp" "$hp" "**does not increment**" fail=1 fi else echo "package.json is new on this branch (no base version) — OK." + row "package.json" "" "$hp" "new on this branch" fi fi @@ -86,12 +113,16 @@ jobs: bc=$(cargo_version "$B"); hc=$(cargo_version "$H") echo "Cargo.toml: base='$bc' head='$hc'" if [ -n "$bc" ]; then - if ! greater "$bc" "$hc"; then + if greater "$bc" "$hc"; then + row "Cargo.toml" "$bc" "$hc" "increments" + else echo "::error::Cargo.toml version must be incremented ($bc -> $hc) before merging." + row "Cargo.toml" "$bc" "$hc" "**does not increment**" fail=1 fi else echo "Cargo.toml is new on this branch (no base version) — OK." + row "Cargo.toml" "" "$hc" "new on this branch" fi fi @@ -99,8 +130,24 @@ jobs: hp=$(pkg_version "$H"); hc=$(cargo_version "$H") if [ -n "$hp" ] && [ -n "$hc" ] && [ "$hp" != "$hc" ]; then echo "::error::package.json ($hp) and Cargo.toml ($hc) versions must match each other." + row "both manifests" "$hp" "$hc" "**disagree with each other**" fail=1 + elif [ -n "$hp" ] && [ -n "$hc" ]; then + row "both manifests" "$hp" "$hc" "agree" fi fi + # The verdict, spelled out. A summary showing only rows would leave a reader inferring + # the outcome from the rows -- which is the inference this whole change exists to remove. + { + echo + if [ "$fail" -eq 0 ]; then + echo "**PASS** — the version increments over \`main\`." + else + echo "**FAIL** — see the rows above. If this branch has been open a while, \`main\`" + echo "has probably taken your version number, or a rebase dropped the bump commit as" + echo "\"already upstream\". Re-bump against \`$base_sha\`." + fi + } >> "$SUMMARY" + exit $fail From 4555604e6c7a0713deaafe4bd8b11fa8d07d682c Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 25 Aug 2026 17:28:56 -0700 Subject: [PATCH 5/6] test(wallet): execute the sweep guard's worked examples instead of asserting them A doc example in `sources.rs` was exactly backwards -- it claimed the attribute-inline shape failed loudly, when it measures `sites=[2]` and drops the construction -- and it survived FOUR adversarial gate rounds. Everything else in that file was measured; only the examples were prose, and that is where the false statement lived. Reviewers read a doc example as documentation rather than as a claim to test, so the remedy is to make the claim executable. The worked examples are now `json` blocks carrying their fixture AND their expected `sites` / `ended_inside_a_test_item`, and a test extracts every one of them from this file's own source and runs the real classifier over it. Doing this as a rustdoc doctest is not possible and would have been worse than prose: rustdoc does not run doctests on `#[cfg(test)]` items, and this whole module is one, so a ```rust example would compile in nobody's build while reading exactly like a passing test. Fail-closed twice over: an unterminated block is an error, and the example COUNT is pinned, so deleting an inconvenient example -- the cheapest way to green -- fails rather than passing quietly. Found while doing it: the trailing-comment example asserted `sites=[5]` and `sites=[]` for "the same fixture" WITHOUT ever writing that fixture down, so its numbers were unreproducible by construction. The fixture is now explicit and the measured values are `[6]` and `[]`. A claim whose input is missing cannot be checked by anyone, which is worse than a claim that is merely wrong. No classifier heuristic is added. Five rounds established that the durable artifact is the fail-closed flag plus an honestly open enumeration, and `sweep` is untouched -- only the file's statements ABOUT it are now enforced. Each of the four examples was proved load-bearing: altering any expected value, and flipping any documented flag, turns the suite red naming that example. Closes #363 Co-Authored-By: Claude --- crates/dig-wallet/src/sage/sources.rs | 192 ++++++++++++++++++++++++-- 1 file changed, 181 insertions(+), 11 deletions(-) diff --git a/crates/dig-wallet/src/sage/sources.rs b/crates/dig-wallet/src/sage/sources.rs index 74d5008d..c7915348 100644 --- a/crates/dig-wallet/src/sage/sources.rs +++ b/crates/dig-wallet/src/sage/sources.rs @@ -502,14 +502,37 @@ mod sole_owner_tests { /// the rest of a test module is read as production. Also noisy, also loud. /// * When the attribute is on its OWN line and the item follows on one line, that item line is /// the clearing line and is judged like any other — so a `CONSTRUCTOR` on it is reported as - /// PRODUCTION and fails here. Measured on this fixture: - /// `["#[cfg(test)]", "fn f() { ChiaQuery::new(c); }", "fn later() { ChiaQuery::new(c); }"]` - /// yields `sites=[2, 3]`. Loud, and answered by moving it into a column-0 test module — the - /// same trade as the indented attribute above. Note the ATTRIBUTE-INLINE form behaves - /// differently and correctly: `#[cfg(test)] fn f() { ChiaQuery::new(c); }` latches on its own - /// line, and a latching line is never passed to [`ends_a_column_0_item`], so that - /// construction is dropped as the test code it is (`sites=[2]` for the same fixture, the - /// remaining site being the production line below). + /// PRODUCTION and fails here. Loud, and answered by moving it into a column-0 test module — + /// the same trade as the indented attribute above. + /// + /// ```json + /// { + /// "name": "attribute on its own line: the gated item is reported as production", + /// "lines": [ + /// "#[cfg(test)]", + /// "fn f() { ChiaQuery::new(c); }", + /// "fn later() { ChiaQuery::new(c); }" + /// ], + /// "sites": [2, 3], + /// "ended_inside_a_test_item": false + /// } + /// ``` + /// + /// The ATTRIBUTE-INLINE form behaves differently and correctly: it latches on its own line, + /// and a latching line is never passed to [`ends_a_column_0_item`], so that construction is + /// dropped as the test code it is. Only the production line below it remains. + /// + /// ```json + /// { + /// "name": "attribute inline: the gated construction is correctly dropped", + /// "lines": [ + /// "#[cfg(test)] fn f() { ChiaQuery::new(c); }", + /// "fn later() { ChiaQuery::new(c); }" + /// ], + /// "sites": [2], + /// "ended_inside_a_test_item": false + /// } + /// ``` /// * A column-0 `#[cfg(test)]` item that never appears to close is not silent either: /// [`Swept::ended_inside_a_test_item`] reports it and the assertion REFUSES. When this /// classifier cannot tell, it says so instead of returning nothing. @@ -525,9 +548,50 @@ mod sole_owner_tests { /// latches the sweep on text that is not code. It needs a source file that quotes Rust /// attributes at column 0. /// * **A terminator carrying trailing content** — `} // done`. [`ends_a_column_0_item`] judges - /// the raw line, so the comment stops it ending anything. Measured: the same fixture with a - /// bare `}` gives `sites=[5]`, and with `} // done` gives `sites=[]`. `rustfmt` preserves - /// the trailing comment byte-for-byte, so formatting does not suppress it. + /// the raw line, so the comment stops it ending anything and the latch runs on over the + /// production item below. `rustfmt` preserves the trailing comment byte-for-byte, so + /// formatting does not suppress it. + /// + /// The two fixtures below differ in ONE character-run — line 4's trailing comment — and that + /// is the whole demonstration: the constructor on line 6 is seen with a bare terminator and + /// silently dropped with a commented one, `ended_inside_a_test_item` reporting `false` in + /// both cases. The production item is written over three lines deliberately; a one-line + /// `fn later() { … }` would itself end at column 0 and clear the latch, so the drop would not + /// occur and the example would demonstrate nothing. + /// + /// ```json + /// { + /// "name": "bare terminator: the production construction below is seen", + /// "lines": [ + /// "#[cfg(test)]", + /// "mod tests {", + /// " fn f() { ChiaQuery::new(c); }", + /// "}", + /// "fn later() {", + /// " ChiaQuery::new(c);", + /// "}" + /// ], + /// "sites": [6], + /// "ended_inside_a_test_item": false + /// } + /// ``` + /// + /// ```json + /// { + /// "name": "terminator with a trailing comment: the same construction is dropped, silently", + /// "lines": [ + /// "#[cfg(test)]", + /// "mod tests {", + /// " fn f() { ChiaQuery::new(c); }", + /// "} // done", + /// "fn later() {", + /// " ChiaQuery::new(c);", + /// "}" + /// ], + /// "sites": [], + /// "ended_inside_a_test_item": false + /// } + /// ``` /// /// **What bounds the damage**, in both cases: the mis-latch lasts only until the next column-0 /// line ending in `}` or `;`, which in ordinary Rust is the end of the next item. So each drops @@ -729,6 +793,112 @@ mod sole_owner_tests { ); } + /// **Every worked example in this module's documentation is EXECUTED, and a wrong one fails + /// CI.** + /// + /// # Why this exists (dig-node#363) + /// + /// A doc example in this file was exactly backwards — it claimed the attribute-inline shape + /// failed loudly, when it measures `sites=[2]` and drops the construction — and it survived + /// **four adversarial gate rounds**. Everything else in the file was measured; only the + /// examples were prose, and that is precisely where the false statement lived. Reviewers read + /// a doc example as documentation rather than as a claim to test, so the fix is not a sharper + /// reviewer: it is making the claim executable. + /// + /// The same pass found a second, quieter failure of the same kind. The trailing-comment + /// example asserted `sites=[5]` and `sites=[]` for "the same fixture" **without ever writing + /// that fixture down**, so its numbers were unreproducible by construction. The reconstructed + /// fixture measures `[6]` and `[]`. A claim whose input is missing cannot be checked by + /// anybody, which is a worse failure than a claim that is merely wrong. + /// + /// # This is a documentation harness, NOT another classifier heuristic + /// + /// Five rounds established that the durable artifact here is the fail-closed flag plus an + /// honestly OPEN enumeration of mis-read shapes — not a better parser. Nothing below changes + /// what [`sweep`] does. It only makes the file's own statements about `sweep` fail when they + /// are false. + /// + /// # Why an extracted JSON block and not a `#[doc]` example + /// + /// `rustdoc` does not run doctests on `#[cfg(test)]` items, and this whole module is one — so + /// a ```` ```rust ```` example here would compile in nobody's build and read exactly like a + /// passing test. That is the cfg-gated-and-unfalsifiable trap, and it would leave the file in + /// the state the ticket was filed about while looking fixed. + #[test] + fn every_worked_example_in_the_docs_is_executed_against_the_real_classifier() { + /// One worked example, in the shape the doc blocks are written in. + #[derive(serde::Deserialize)] + struct Example { + name: String, + lines: Vec, + sites: Vec, + ended_inside_a_test_item: bool, + } + + // This module's own source. Reading the FILE rather than a copied fixture is what ties the + // assertions to the sentences a reader actually sees. + let source = include_str!("sources.rs"); + + let mut examples = Vec::new(); + let mut collecting: Option = None; + for raw in source.lines() { + // Strip the doc-comment prefix so what remains is the JSON as rendered. + let Some(doc) = raw.trim_start().strip_prefix("///") else { + continue; + }; + let doc = doc.trim(); + match (&mut collecting, doc) { + (None, "```json") => collecting = Some(String::new()), + (Some(_), "```") => { + let body = collecting.take().expect("collecting"); + examples.push(serde_json::from_str::(&body).unwrap_or_else(|e| { + panic!("a worked example is not valid JSON: {e}\n{body}") + })); + } + (Some(buf), line) => { + buf.push_str(line); + buf.push('\n'); + } + (None, _) => {} + } + } + + assert!( + collecting.is_none(), + "an unterminated ```json block -- the extractor stopped mid-example, so the examples \ + after it were never checked" + ); + + // Fail-closed on the COUNT, not merely on emptiness. Deleting an inconvenient example is + // the cheapest way to make this test green, and it is the one edit this must not permit + // silently; adding one deliberately means updating this number, which is the review moment. + const WORKED_EXAMPLES: usize = 4; + assert_eq!( + examples.len(), + WORKED_EXAMPLES, + "the docs carry a different number of worked examples than this harness expects -- if \ + that is deliberate, say so here; if it is not, an example has been lost and its claim \ + is unchecked again" + ); + + for example in examples { + let swept = sweep(&example.lines.join("\n")); + assert_eq!( + swept.sites, example.sites, + "the documented `sites` for \"{}\" is not what the classifier measures -- the \ + documentation is wrong, not the classifier (this ticket does not change `sweep`)", + example.name + ); + assert_eq!( + swept.ended_inside_a_test_item, example.ended_inside_a_test_item, + "the documented fail-closed flag for \"{}\" is not what the classifier measures; \ + a shape documented as SILENT that in fact reports itself -- or the reverse -- \ + misstates the one bound this guard offers", + example.name + ); + } + } + /// A file the sweep could not finish reading is REFUSED, never reported clean. /// /// An unterminated `#[cfg(test)]` item means every line after it was assumed to be test code From 753118a4a0ebf3b2e6758522668bb3eac12e43da Mon Sep 17 00:00:00 2001 From: Michael Taylor Date: Tue, 25 Aug 2026 18:20:41 -0700 Subject: [PATCH 6/6] chore(wallet): bump dig-wallet to 0.36.0 for the removed pub constructor `ChainTransport::with_sources` was `pub`, so deleting it is a breaking change to this crate's surface even though it had no callers. SemVer minor on 0.x. Co-Authored-By: Claude --- Cargo.lock | 2 +- crates/dig-wallet/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e68e3987..7a8e6cda 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3274,7 +3274,7 @@ dependencies = [ [[package]] name = "dig-wallet" -version = "0.35.0" +version = "0.36.0" dependencies = [ "async-trait", "axum", diff --git a/crates/dig-wallet/Cargo.toml b/crates/dig-wallet/Cargo.toml index 49e859f7..f198ec96 100644 --- a/crates/dig-wallet/Cargo.toml +++ b/crates/dig-wallet/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "dig-wallet" -version = "0.35.0" +version = "0.36.0" edition = "2021" license = "GPL-2.0-only" description = "DIG Browser built-in Chia wallet sidecar: a local axum server (using digstore-chain + chia-wallet-sdk over coinset.org) that serves a Sage-mirroring wallet UI. Native Rust so BLS signing works; the browser opens it at 127.0.0.1."