Skip to content

Tests: Make main's suites runnable and green again, and reject a stale-ABI cached executable - #1337

Merged
brandonpayton merged 6 commits into
mainfrom
fix-main-suites-and-vim-abi-cache
Aug 29, 2026
Merged

Tests: Make main's suites runnable and green again, and reject a stale-ABI cached executable#1337
brandonpayton merged 6 commits into
mainfrom
fix-main-suites-and-vim-abi-cache

Conversation

@mho22

@mho22 mho22 commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Why

Four defects stop main from running or passing its own test suites. A fifth lets a stale binary through the package cache into the one place the host trusts.

The exact-ABI vitest runner cannot enumerate. scripts/ci-vitest-evidence-classes.tsv is an exact-cover whitelist, so drift in either direction is a hard stop. #1322 deleted tests/package-system/fetch-binaries-allow-stale.test.ts and tests/package-system/package-source-publish-contract.test.ts but left their rows; three later PRs added test files without classifying them. scripts/ci-run-test-suite.sh vitest exact-abi-source has been unable to start since.

Two host tests throw before collecting. host/test/kernel-scratch-contract.test.ts and host/test/kernel-blocking-retry-snapshot.test.ts read Rust sources by path, and #1321 moved those modules from crates/kernel/src/ to crates/runtime-core/. The scratch contract's regex also needs pub fn from_syscall rather than pub(crate) fn, because the module now crosses a crate boundary.

A third host test calls a method that no longer exists. host/test/privileged-projection.test.ts calls getMountSetIdCapability, which is absent from host/src/vfs/vfs.ts.

cargo test -p xtask is red. local_build::tests::clean_cascades_to_products_embedding_the_package asserts that cleaning a package nothing depends on removes only that package, and it named ruby. #1336 ships Ruby as a shell lazy-archive, so ruby now has a dependent and its removal set is fourteen nodes: eight packages and six browser products.

A cached executable can declare any ABI and still be trusted. A vim built before an ABI bump reached a desktop built after it, and the host refused it at exec with refusing unsafe program artifact before execution. Three things stack to produce that. packages/registry/vim/vim-src/ is a gitignored, persistent build tree; the ABI marker comes from libc/glue/channel_syscall.c, which wasm32posix-cc compiles at link time and which is not a make dependency, so after a bump make saw src/vim up to date, relinked nothing, and build-vim.sh re-collected the previous binary. wasm_artifact_policy_failures_for then let it through: it required __abi_version to be exported but compared its value only for side modules, so the resolver cached a stale executable under the current ABI's key. And build-vim.sh was outside vim's cache identity, because build_input_digests_from_repo hashes build.toml.inputs and the git inputs but not script_path, and vim declared no inputs.

What changed

Five commits, each one defect, plus one follow-up review commit (see below).

The evidence registry drops the two rows for files #1322 deleted and gains rows for the three unclassified files. A file is source-only only when its whole import closure stays clear of the binary resolver, which the runner enforces separately from the cover check; tests/package-system/build-input-crate-closure.test.ts qualifies, the two browser tests do too.

The two snapshot contracts follow the modules to crates/runtime-core/. The privileged-projection test follows the set-ID capability out of VirtualPlatformIO.

The clean-cascade test reads its leaf out of the plan graph — the first package no other node depends on — instead of naming one. The assertion keeps its meaning and no longer breaks the next time an image embeds whichever package the test happened to name. Nine packages in the checked-in graph still qualify.

The artifact policy compares the __abi_version value, not just its presence. A required export declaring anything other than wasm_posix_shared::ABI_VERSION is now a policy failure, guarded to non-side-modules so the existing dylink path keeps owning those; both call sites, cache-entry validation and build-script-output validation, go through the shared helper. Five hardcoded __abi_version -> 0 wasm literals in build_deps.rs would have failed that check, so they now derive from the existing emit_wasm_build_script helper and track ABI_VERSION. Two cache-mutation tests built their "different bytes" by rewriting that same body constant, which the new check rejects before the generation assertion runs; they now append a custom section the policy ignores. build-vim.sh drops $SRC_DIR/src/vim before make, as build-netcat.sh already does, and vim declares its inputs. vim's revision stays 4 — the added inputs already move the cache key.

Follow-up review (commit bceead629)

A local review of this branch (by Claude, at the author's request) hardened
one test and documented one deliberately-narrow check.

The clean-cascade leaf test still named its candidate by reconstructing a
package node from each graph.packages name under a hardcoded wasm32
arch. A package planned only for wasm64 would produce a node present neither
as a graph key nor in any dependency edge, so the "nothing depends on it"
predicate was trivially satisfied and clean_removal_set ran against a node
the graph does not contain — the assert passing while verifying nothing. The
test now reads the leaf out of the graph's real node set (the Package nodes
already in graph.dependencies), so the candidate is honest for any planned
target, fully realizing the "read the leaf out of the graph" intent.

The follow-up also considered making the executable __abi_version check as
strict as the side-module branch (failing Missing/Invalid/Err, not only
Present(mismatch)). That change was reverted because it is wrong by design:
genuine absence is already reported by the required-exports check (a Missing
branch double-reports), and legitimate executables carry markers
artifact_identity classifies Invalid — extraction only succeeds for a
constant-thunk shape, which the instrumenter guarantees for side modules but
not for arbitrary executables (the spidermonkey disabled-fork fixture is
accepted precisely in this state). Present(mismatch) is therefore the only
reliable staleness signal for executables. A comment at the check now records
that rationale so the narrower branch is not "fixed" into a regression later.

Validation

cargo test -p xtask --target aarch64-apple-darwin — 555 pass, 0 fail. Red on main before this branch.

cargo test --workspace --exclude xtask --target aarch64-apple-darwin, inside scripts/dev-shell.sh — 1999 pass, 0 fail.

scripts/ci-run-test-suite.sh vitest exact-abi-source — the runner enumerates again and reports 208 of 212 files passing, 3139 tests. The 11 remaining failures, in test/exec-state-tracking.test.ts, test/spawn-pid-authority.test.ts and test/vfork-production-mechanism.test.ts, are an artifact of the worktree, not of this branch: those tests load real binaries out of local-binaries/, that tier was built at ABI 44 by an unrelated branch, and main is at ABI 43. Decoding __abi_version out of local-binaries/programs/wasm32/exec-child.wasm gives i32.const 44. Every failure message is the ABI policy rejecting those bytes.

Follow-up commit bceead629 re-validated via scripts/dev-shell.sh at --target aarch64-apple-darwin: cargo test -p xtask build_deps module (287 pass, 0 fail) and clean_cascades_to_products_embedding_the_package (pass).

Two of the five defects were confirmed against a clean origin/main checkout rather than only in the working worktree.

mho22 added 5 commits August 28, 2026 10:24
## Why

`scripts/ci-run-test-suite.sh vitest exact-abi-source` aborts on `main`
before it runs a single test:

    ci-run-test-suite: classified Vitest evidence is not a regular file:
      tests/package-system/fetch-binaries-allow-stale.test.ts

`scripts/ci-vitest-evidence-classes.tsv` must exactly cover the live test
file inventory, so a drift in either direction is a hard stop. Four
merges left it out of sync in both.

Two classified files no longer exist. #1322 ("Remove the remote binary
channel") deleted `fetch-binaries-allow-stale.test.ts` and
`package-source-publish-contract.test.ts` along with the scripts they
covered, but left their registry rows behind. That PR deliberately
excluded `.github/`, not `scripts/`.

Three live files were never classified:
`build-input-crate-closure.test.ts` (#1330),
`browser-cors-proxy-service-worker-parity.test.ts` and
`browser-mitm-ca-env.test.ts` (the git-over-HTTPS clone pair).

## What changed

Drop the two rows for deleted files. Add the three live files, keeping
the file sorted by repository-relative path as its header requires.

All three are `source-only`: each reads repository sources and resolves
no artifact. The two browser tests import only from `host/src/`.
`build-input-crate-closure.test.ts` parses `build.toml` and `Cargo.toml`
and describes itself as "a pure file-parse check" — it does not follow
its neighbour `build-input-import-closure.test.ts`, which is
`prepared-product` because it resolves `programs/coreutils.wasm`.

## Validation

    bash scripts/ci-run-test-suite.sh vitest exact-abi-source

Now enumerates and runs: 212 files, 206 passed, 1 skipped. All three
newly-classified files run and pass in the group. It reached the test
phase for the first time since #1322.

The five remaining failures are unrelated to this change and predate it:
two read Rust sources that #1321 moved to `crates/runtime-core/`, and
three depend on an ABI-matched kernel artifact.
## Why

Two contract tests fail to collect on `main`:

    Error: ENOENT: no such file or directory, open
      '<repo>/crates/kernel/src/spawn.rs'
    Error: ENOENT: no such file or directory, open
      '<repo>/crates/kernel/src/blocked_retry.rs'

#1321 ("Extract runtime-core crate for the Rust-first runtime split")
moved 41 modules from `crates/kernel/src/` to `crates/runtime-core/src/`.
Both tests read their Rust source by path, and neither path was updated,
so the whole file throws before a single case runs.

`kernel-blocking-retry-snapshot.test.ts` also pins the declaration of
`BlockingRetryOperation::from_syscall` by regex. The move changed its
visibility from `pub(crate)` to `pub` — it now crosses a crate boundary
— so the match returned `undefined` and the assertion reported
`expected undefined to be defined` rather than naming the real cause.

`wasm_api.rs` stayed in `crates/kernel/`, so the three references to it
are still correct and are left alone.

## What changed

Point the two `readFileSync` URLs at `crates/runtime-core/src/`, and
match `pub fn from_syscall` as the source now declares it.

`pub(crate)` is not a state the module can return to: `crates/kernel`
consumes it across the crate boundary, so it must stay `pub`.

## Validation

    npx vitest run test/kernel-scratch-contract.test.ts \
      test/kernel-blocking-retry-snapshot.test.ts
    Test Files  2 passed (2)
         Tests  163 passed (163)

Both files failed to collect before this change.
## Why

Three cases in `host/test/privileged-projection.test.ts` fail on `main`:

    TypeError: io.getMountSetIdCapability is not a function
    AssertionError: expected [Function] to throw an error

Commit 48b6692 ("Packages: Boot login from package-backed images",
#1307) moved the set-ID mount policy out of `VirtualPlatformIO` and into
the free function `resolveMountSetIdCapability` in
`host/src/vfs/memory-fs.ts`. It removed the `getMountSetIdCapability`
method and the constructor-side validation, but left three call sites in
the test.

So the test still asked the constructor to reject a candidate backend,
and the constructor no longer rejects anything — `toThrow` received
`undefined`.

## What changed

Point the three call sites at `resolveMountSetIdCapability`. The two
rejection cases pass the mount config directly instead of building a
`VirtualPlatformIO` around it, which is what the function now takes.

`VirtualPlatformIO` stays imported: the file still constructs one to
`lstat` the projected programs.

    npx vitest run test/privileged-projection.test.ts
    Test Files  1 passed (1)
         Tests  12 passed (12)
## Why

A vim built before an ABI bump reached a desktop built after it, and
the host refused it at exec:

  refusing unsafe program artifact before execution: ABI 43, expected 44

Three defects stack to produce that.

`packages/registry/vim/vim-src/` is a gitignored, persistent build
tree. The ABI marker comes from `libc/glue/channel_syscall.c`, which
`wasm32posix-cc` compiles at link time; it is not a `make` dependency.
So after an ABI bump `make` still saw `src/vim` up to date, relinked
nothing, and `build-vim.sh` re-collected the previous binary on every
rebuild.

`wasm_artifact_policy_failures_for` then let that binary through. It
required `__abi_version` to be *exported*, but compared its *value*
only for side modules (`dylink_section_count > 0`). An executable
declaring any ABI passed, and the resolver cached it under the current
ABI's key -- the one place the host trusts.

`build-vim.sh` was also outside vim's cache identity.
`build_input_digests_from_repo` hashes `build.toml.inputs` and the git
inputs; `script_path` is not hashed. vim declared no `inputs`, so
editing its build script invalidated nothing.

## What changed

Compare the value, not just the presence. A required `__abi_version`
export that declares anything other than `wasm_posix_shared::ABI_VERSION`
is now a policy failure, guarded to non-side-modules so the existing
dylink path keeps owning those. Both call sites -- cache-entry
validation and build-script-output validation -- go through the shared
helper, so both gain the check.

Five hardcoded `__abi_version -> 0` wasm literals in `build_deps.rs`
would have failed that check. They now derive from the existing
`emit_wasm_build_script` helper and `minimal_executable_wasm`, so they
track `ABI_VERSION` instead of pinning zero. `wasm_exporting_names`
gained an inner `wasm_exporting_names_declaring_abi(names, abi)` form
so the new test can declare a stale one deliberately.

Two cache-mutation tests produced their "different bytes" by rewriting
that same body constant. The rewritten body no longer passes the
artifact policy, so validation would fire before the generation check
they assert on. They now append a custom section the policy ignores.

Relink unconditionally: `build-vim.sh` drops `$SRC_DIR/src/vim` before
`make`, as `build-netcat.sh` already does.

Declare vim's `inputs` (`build-vim.sh`, `bundle-runtime.sh`). 66 of the
84 `build.toml` files already declare them; vim was one of the 18 that
did not. `revision` stays 4 -- the added inputs already move the cache
key, so bumping it would only force a second, redundant rebuild.

## Validation

`cargo test -p xtask --target aarch64-apple-darwin` -- 542 pass. The
two `local_build::tests::*checked_in_authority*` failures are
pre-existing on `origin/main` at `2df2f0f13`: their expected package
lists still hold 75 names, and the checked-in supported set now
projects 77.
…ing it

## Why

`cargo test -p xtask` is red on `main` at `888e628d5`:

  local_build::tests::clean_cascades_to_products_embedding_the_package

Its second half asserts that cleaning a package nothing depends on
removes only that package. It named `ruby`, with a comment recording
that `ruby` sat in no `depends_on`, no product composition, and no
`root_mirror_packages` when the test was written.

`888e628d5` (#1336) ships Ruby as a shell lazy-archive. `ruby` now has
a dependent, so the removal set is 14 nodes: eight packages and six
browser products.

## What changed

Take the leaf from the plan graph -- the first package no other node
depends on -- rather than naming one. The assertion keeps its meaning:
`clean_removal_set` on a package with no dependents returns only that
package. It no longer breaks the next time an image embeds whichever
package the test happened to name.

## Validation

`cargo test -p xtask --target aarch64-apple-darwin` -- 555 pass, 0
fail. Nine packages in the checked-in graph still qualify as leaves.
@brandonpayton

Copy link
Copy Markdown
Member

Kudos on getting the 1337 PR, @mho22 ;)

The clean-cascade leaf test built its candidate node by reconstructing a
package node from each package name under a hardcoded wasm32 arch. A
package planned only for wasm64 would then produce a node present neither
as a graph key nor in any dependency edge, so the leaf predicate was
trivially satisfied and clean_removal_set ran against a node the graph
does not contain. The assert still passed but no longer verified real
clean-cascade behavior -- the robustness the "read the leaf out of the
graph" rewrite was meant to guarantee.

Read the leaf out of the graph's actual node set instead: iterate the
Package nodes already in graph.dependencies and pick one nothing depends
on. The candidate is now honest for any planned target.

Also document why the executable __abi_version check in build_deps is
deliberately narrower than the side-module branch: executable markers
are not guaranteed to be a constant thunk artifact_identity can extract,
so Missing/Invalid/Err are legitimate states for a valid executable and
only Present(mismatch) is a reliable staleness signal. Genuine absence of
the export is already reported by the required-exports check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@brandonpayton

Copy link
Copy Markdown
Member

Follow-up review commit: bceead629

I (Claude, reviewing this branch locally at Brandon's request) reviewed the
diff, found two candidate issues, and pushed one fix plus one clarifying
comment. Recording the reasoning here so the trail is legible.

1. Clean-cascade leaf test now reads the leaf from the graph's real node set

The leaf test built its candidate node by reconstructing a package node from
each graph.packages name under a hardcoded wasm32 arch. A package
planned only for wasm64 would then produce a node present neither as a graph
key nor in any dependency edge, so the "nothing depends on it" predicate was
trivially satisfied and clean_removal_set ran against a node the graph does
not actually contain. The assert would still pass while verifying nothing —
the exact robustness the "read the leaf out of the graph" rewrite was meant to
guarantee.

Fix: iterate the Package nodes already in graph.dependencies and pick one
nothing depends on, so the candidate is a node the graph truly contains for
any planned target.

2. Executable __abi_version check — investigated, intentionally left as-is (comment only)

I first suspected the executable branch was too permissive next to the
side-module branch: it only fails Present(mismatch) and ignores
Missing/Invalid/Err. Making it strict broke two existing tests, and they
were right to break:

  • Genuine absence of __abi_version is already reported by the
    required-exports check, so a Missing branch double-reports.
  • The spidermonkey disabled-fork fixture legitimately carries a marker that
    artifact_identity classifies Invalid (extraction only succeeds for a
    constant thunk / call-delegate shape — guaranteed for side modules by the
    instrumenter, not for arbitrary executables), and the test expects
    acceptance.

So Present(mismatch) really is the only reliable staleness signal for
executables, and the narrow check is correct by design. I reverted the
behavior change and instead added a comment at the check documenting why
it is deliberately narrower than the side-module branch, so the next reader
doesn't repeat my mistake.

Validation

Run via scripts/dev-shell.sh with --target aarch64-apple-darwin (xtask is
a host tool):

  • cargo test -p xtask build_deps module — 287 pass, 0 fail
  • clean_cascades_to_products_embedding_the_package — pass

@brandonpayton
brandonpayton merged commit 614d692 into main Aug 29, 2026
@brandonpayton
brandonpayton deleted the fix-main-suites-and-vim-abi-cache branch August 29, 2026 05:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants