oficina / docs Public

ci: fix round 1 -- reorder so no_host_imports can't pass by skipping, one clippy invocation, add cargo fmt

Two Important review findings, same underlying failure mode as before:
a check that reports success without checking.

no_host_imports read the wasm32 artifact off disk, but the gate ran
cargo test before scripts/build-wasm.sh built it -- the check only
worked because a different test happened to build the artifact as a
side effect first. Reordered the gate so build-wasm.sh always runs
first, and taught no_host_imports a DOCS_REQUIRE_WASM=1 mode (set by
the gate) where a missing artifact is a failure, not a skip. A bare
local `cargo test` without the var still skips on a fresh clone.

The gate also ran its own looser clippy (no --all-targets, no
-D clippy::disallowed_methods) before cargo test, and build-wasm.sh's
stricter clippy after -- so a banned method planted in a test file
sailed through the fast check and was only caught ~40s later. Deleted
the gate's clippy line; reordering already puts build-wasm.sh's
correct clippy first, so there is exactly one clippy invocation and no
flags to drift apart.

Also: cargo fmt --check, matching gofmt's role on the Go side (needed
rustup component add rustfmt, and one mechanical reformat of three
test files); and a documented tradeoff that npm ci needs the registry
reachable at run time.

Re-proved all three induced failures against the reordered gate. The
banned-method case now fails in 0.63s instead of 3.57s, because it's
caught by the only clippy pass instead of a second one after the
first let it through.

Paul Campbell pushed by paul@keymail.dev 1 month ago 3bc95ca741f5366ed78dd713843ce4ec86f4da82 parent 01e0e6f

5 files changed, +94 −22

.amadan/README.md +8 −0
.amadan/ci.d/10-gate +26 −5
core/tests/no_host_imports.rs +19 −2
core/tests/wasm_trap_containment.rs +34 −10
core/tests/wire_compat.rs +7 −5

CI passed — run details

Intent

full ledger: core-wire-compat

diff --git a/.amadan/README.md b/.amadan/README.md

index f7efe12..b5eba52 100644

--- a/.amadan/README.md

+++ b/.amadan/README.md

@@ -15,6 +15,14 @@ never carries over — the gate runs `npm ci` itself, against

`fixtures/package-lock.json`, so the installed `yjs` is always exactly

13.6.27 rather than whatever a bare `npm install` would resolve.

+**Known tradeoff:** that `npm ci` needs the npm registry reachable at run

+time. It is the right fix for the fresh-worktree-per-branch problem above,

+and the lockfile is committed so the *version* resolved never drifts, but

+it does mean the gate is not fully offline-capable the way the Rust and Go

+steps are once their caches are warm. If the registry is ever unreachable

+from the runner, that is the failure this line produces — not a bug in

+the gate itself.

## The runner

Jobs are claimed by an `amadan runner` on the machine that has one. Two

diff --git a/.amadan/ci.d/10-gate b/.amadan/ci.d/10-gate

index effe869..e4f385a 100755

--- a/.amadan/ci.d/10-gate

+++ b/.amadan/ci.d/10-gate

@@ -27,14 +27,35 @@ export CGO_ENABLED=0

echo "=== fixtures: node dependencies"

(cd fixtures && npm ci)

-echo "=== rust: build, clippy, test"

-cargo build --release --manifest-path core/Cargo.toml

-cargo clippy --manifest-path core/Cargo.toml -- -D warnings

-cargo test --manifest-path core/Cargo.toml

+echo "=== rust: fmt"

+cargo fmt --manifest-path core/Cargo.toml --check

-echo "=== wasm: the artifact the host embeds"

+# build-wasm.sh is the ONLY clippy invocation in this gate (it runs

+# --all-targets -D warnings -D clippy::disallowed_methods) and it runs

+# BEFORE cargo test, for two reasons that used to be two separate bugs:

+# 1. A second, looser clippy invocation here previously let a banned

+# method planted in a test file sail through this fast check and get

+# caught only ~40s later, inside build-wasm.sh's own clippy run --

+# defeating the point of failing fast. One invocation, no drift.

+# 2. core/tests/no_host_imports.rs asserts zero wasm imports by reading

+# the wasm32 artifact off disk; it only exists after this line. Running

+# cargo test first made the assertion's own presence into a race —

+# it silently reported PASS-via-SKIP unless some other test happened

+# to build the artifact as a side effect first. Building here removes

+# the race instead of hoping to win it.

+echo "=== wasm: clippy, then the artifact the host embeds"

./scripts/build-wasm.sh

+echo "=== rust: build, test"

+# DOCS_REQUIRE_WASM=1 turns core/tests/no_host_imports.rs's "artifact

+# missing" branch into a failure instead of a skip. It is always set here

+# because the wasm artifact was just built above; unset, a bare local

+# `cargo test` still skips gracefully on a fresh clone.

+export DOCS_REQUIRE_WASM=1

+cargo build --release --manifest-path core/Cargo.toml

+cargo test --manifest-path core/Cargo.toml

echo "=== the embedded wasm must match a fresh build"

if ! cmp -s core/target/wasm32-unknown-unknown/release/docs_core.wasm internal/core/testdata/docs_core.wasm; then

echo "gate: internal/core/testdata/docs_core.wasm is stale. Re-copy it." >&2

diff --git a/core/tests/no_host_imports.rs b/core/tests/no_host_imports.rs

index d9e849e..551c2b5 100644

--- a/core/tests/no_host_imports.rs

+++ b/core/tests/no_host_imports.rs

@@ -4,10 +4,25 @@ use std::path::Path;

fn no_host_imports() {

let wasm_path = "target/wasm32-unknown-unknown/release/docs_core.wasm";

- // Skip if artifact not yet built

+ // DOCS_REQUIRE_WASM=1 is set by the CI gate, which always builds the

+ // wasm artifact before running this test — so under the gate a missing

+ // artifact is a real failure, not something to shrug off. Left unset,

+ // a bare `cargo test` on a fresh clone (no wasm build yet) still skips,

+ // so local iteration on the native tests isn't forced to build wasm

+ // first.

+ let require_wasm = std::env::var("DOCS_REQUIRE_WASM").as_deref() == Ok("1");

if !Path::new(wasm_path).exists() {

+ if require_wasm {

+ panic!(

+ "FAILED (DOCS_REQUIRE_WASM=1): wasm artifact not found at {}. \

+ The gate must build it before running this test.",

+ wasm_path

+ );

+ }

eprintln!(

- "SKIPPED: wasm artifact not found at {}. Run ./scripts/build-wasm.sh first.",

+ "SKIPPED (DOCS_REQUIRE_WASM unset): wasm artifact not found at {}. \

+ Run ./scripts/build-wasm.sh first.",

wasm_path

);

return;

@@ -36,4 +51,6 @@ fn no_host_imports() {

imports.join("\n ")

);

}

+ eprintln!("PASSED (checked {} bytes, 0 imports)", wasm_bytes.len());

}

diff --git a/core/tests/wasm_trap_containment.rs b/core/tests/wasm_trap_containment.rs

index 3750fa3..55dd9e1 100644

--- a/core/tests/wasm_trap_containment.rs

+++ b/core/tests/wasm_trap_containment.rs

@@ -104,7 +104,9 @@ impl Harness {

let engine = Engine::new(&config);

let module = Module::new(&engine, wasm_bytes).expect("module should parse and validate");

let mut store = Store::new(&engine, ());

- store.set_fuel(u64::MAX / 2).expect("fuel metering is enabled");

+ store

+ .set_fuel(u64::MAX / 2)

+ .expect("fuel metering is enabled");

// Zero imports (core/tests/no_host_imports.rs enforces this), so an

// empty Linker is enough to instantiate.

let linker = Linker::<()>::new(&engine);

@@ -185,7 +187,9 @@ impl Harness {

Err(e) => return (Err(e), 0),

};

let before = self.store.get_fuel().expect("fuel metering is enabled");

- self.store.set_fuel(APPLY_FUEL_BUDGET).expect("fuel metering is enabled");

+ self.store

+ .set_fuel(APPLY_FUEL_BUDGET)

+ .expect("fuel metering is enabled");

let result = self.apply.call(&mut self.store, (handle, ptr, len));

let after = self.store.get_fuel().unwrap_or(0);

let consumed = APPLY_FUEL_BUDGET.saturating_sub(after);

@@ -229,8 +233,13 @@ impl Harness {

return Ok(Vec::new());

}

let out_ptr = self.alloc.call(&mut self.store, len)?;

- assert_ne!(out_ptr, 0, "docs_core_alloc failed for {len} bytes (text_read buffer)");

- let rc = self.text_read.call(&mut self.store, (handle, out_ptr, len))?;

+ assert_ne!(

+ out_ptr, 0,

+ "docs_core_alloc failed for {len} bytes (text_read buffer)"

+ );

+ let rc = self

+ .text_read

+ .call(&mut self.store, (handle, out_ptr, len))?;

assert_eq!(

rc, len as i32,

"docs_core_text_read returned {rc} bytes written, expected {len} \

@@ -369,10 +378,15 @@ fn trap_on_one_document_does_not_corrupt_others_or_wedge_the_instance() {

assert_ne!(handle, 0);

let (result, fuel) = h.apply(handle, bytes);

let rc = result.expect("applying a known-good fixture must not trap");

- assert_eq!(rc, 0, "control document {name} failed to apply its own fixture");

+ assert_eq!(

+ rc, 0,

+ "control document {name} failed to apply its own fixture"

+ );

eprintln!("control {name}: fuel consumed = {fuel}");

let expected = expected_text(name);

- let got = h.read_text(handle).expect("reading a freshly-seeded control must not trap");

+ let got = h

+ .read_text(handle)

+ .expect("reading a freshly-seeded control must not trap");

assert_eq!(

got,

expected.as_bytes(),

@@ -407,7 +421,10 @@ fn trap_on_one_document_does_not_corrupt_others_or_wedge_the_instance() {

match result {

Ok(0) => oks += 1,

Ok(rc) => {

- assert!(rc < 0, "apply returned a positive non-zero code {rc} at trial {i}");

+ assert!(

+ rc < 0,

+ "apply returned a positive non-zero code {rc} at trial {i}"

+ );

clean_errors += 1;

}

Err(e) => {

@@ -533,10 +550,14 @@ fn borrow_leak_repro_trapping_unicode_mutation() {

.expect("doc_new must not trap");

let (control_result, _) = h.apply(control, hello);

assert_eq!(control_result.unwrap(), 0);

- let control_content = h.read_text(control).expect("reading the freshly-seeded control");

+ let control_content = h

+ .read_text(control)

+ .expect("reading the freshly-seeded control");

assert_eq!(control_content, hello_expected.as_bytes());

- let handle = h.doc_new("corrupted-unicode", 7).expect("doc_new must not trap");

+ let handle = h

+ .doc_new("corrupted-unicode", 7)

+ .expect("doc_new must not trap");

assert_ne!(handle, 0);

let (result, _fuel) = h.apply(handle, &trapping);

eprintln!("borrow_leak_repro: apply(trapping mutation) = {result:?}");

@@ -568,7 +589,10 @@ fn borrow_leak_repro_trapping_unicode_mutation() {

let after = h

.doc_new("after-corruption", 99)

.expect("doc_new must keep working after the corrupted apply");

- assert_ne!(after, 0, "doc_new returned a null handle after the corrupted apply");

+ assert_ne!(

+ after, 0,

+ "doc_new returned a null handle after the corrupted apply"

+ );

let (after_result, _) = h.apply(after, hello);

assert_eq!(

after_result.expect("a brand-new document must still apply a good update"),

diff --git a/core/tests/wire_compat.rs b/core/tests/wire_compat.rs

index 3343b9a..3e8e1bd 100644

--- a/core/tests/wire_compat.rs

+++ b/core/tests/wire_compat.rs

@@ -41,9 +41,9 @@ fn any_to_json(any: &Any) -> Value {

Any::Undefined => unreachable!(

"Any::Undefined in a delta attribute — not representable in the fixture JSON"

- Any::Buffer(_) => unreachable!(

- "Any::Buffer in a delta attribute — not representable in the fixture JSON"

- ),

+ Any::Buffer(_) => {

+ unreachable!("Any::Buffer in a delta attribute — not representable in the fixture JSON")

+ }

Any::Array(items) => Value::Array(items.iter().map(any_to_json).collect()),

Any::Map(map) => {

let mut entries: Vec<(&String, &Any)> = map.iter().collect();

@@ -166,7 +166,10 @@ fn abi_round_trips_a_fixture() {

fn abi_rejects_garbage_without_panicking() {

let handle = docs_core::abi::doc_new("abi-rejects-garbage-guid", 43);

let rc = docs_core::abi::apply_slice(handle, &[0xff, 0xff, 0xff, 0xff]);

- assert!(rc < 0, "garbage input should return an error code, got {rc}");

+ assert!(

+ rc < 0,

+ "garbage input should return an error code, got {rc}"

+ );

docs_core::abi::doc_free(handle);

}

@@ -186,4 +189,3 @@ fn abi_rejects_garbage_without_panicking() {

// exactly the property finding 1 and 5 need proven. So the mutation fuzz

// runs the real compiled wasm module under `wasmi` instead, where traps

// are always safely observable as an `Err` rather than a process crash.