Build and pin the real bread-module-host binary for integration tests #7

Merged

Breadway merged 1 commit from fix/module-host-test-harness into main

2026-09-29 12:55:35 +08:00

Breadway commented

2026-09-29 08:45:08 +08:00

Owner

Follow-up to the post-merge review comment on #6 (the round-3 CI fix commits 5059771/af38653): those got CI green, but the reviewer flagged three real fragility gaps in ipc_integration.rs's nested cargo build -p bread-module-host helper:

module_host_sandbox.rs never called it at all, relying on ipc_integration happening to build first in target order. Its own header note still claimed cargo test --workspace already built the binary, which the af38653 commit message itself disproved.
The nested build always assumed the dev profile and default target/{debug,release} layout, so --release, a custom --target-dir, or a cross-compiling --target would miss it.
A miss falls through to a bare PATH lookup, which can silently succeed against a stale, separately installed ~/.local/bin/bread-module-host instead of the binary the test run just built, meaning a test could pass while exercising the wrong code.

What changed

Moved the build-and-locate helper into breadd/tests/common/mod.rs, used by both ipc_integration.rs and module_host_sandbox.rs, and fixed the latter's stale note.
The helper runs cargo build -p bread-module-host --locked --message-format=json and reads the path back out of the compiler-artifact message's executable field, instead of guessing a location. This is correct under any profile, CARGO_TARGET_DIR, or CARGO_BUILD_TARGET, since those are ordinary environment variables cargo already honours and the nested Command inherits from the test process.
Both test harnesses now pass that exact path to the breadd they spawn through a new BREAD_MODULE_HOST_BIN env var, which resolve_module_host_binary checks first, ahead of its normal current_exe()-relative resolution and before ever reaching the PATH fallback.
Added a cargo build --workspace step to check.yml before the test step, as cheap extra insurance alongside the in-test fix (the review's "cheaper alternative, or in addition").
Added a unit test pinning that the env var override wins outright over the normal resolution path.

Verification

Locally from a completely fresh CARGO_TARGET_DIR: cargo build --workspace --locked, cargo clippy --workspace --all-targets --locked -- -D warnings, cargo fmt --all -- --check, and cargo test --workspace --locked, all clean (163 breadd unit tests, 37 ipc_integration tests, 2 module_host_sandbox tests).
Specifically reproduced the exact scenario the review called out: cargo test -p breadd --locked --test module_host_sandbox run alone, from a brand-new empty target dir, with nothing else having built bread-module-host first. Confirmed the binary actually gets built at that fresh target dir (debug/bread-module-host) by this helper, and both tests pass.
Repeated the full cargo build, cargo clippy and cargo test --workspace --locked runs inside this repo's own pinned CI container via ci/build.sh (the same one CI uses), all green.

No merge; leaving this for review.

Relates to ECO-34.

Follow-up to the post-merge review comment on #6 (the round-3 CI fix commits 5059771/af38653): those got CI green, but the reviewer flagged three real fragility gaps in `ipc_integration.rs`'s nested `cargo build -p bread-module-host` helper: 1. `module_host_sandbox.rs` never called it at all, relying on `ipc_integration` happening to build first in target order. Its own header note still claimed `cargo test --workspace` already built the binary, which the af38653 commit message itself disproved. 2. The nested build always assumed the dev profile and default `target/{debug,release}` layout, so `--release`, a custom `--target-dir`, or a cross-compiling `--target` would miss it. 3. A miss falls through to a bare `PATH` lookup, which can silently succeed against a stale, separately **installed** `~/.local/bin/bread-module-host` instead of the binary the test run just built, meaning a test could pass while exercising the wrong code. ## What changed - Moved the build-and-locate helper into `breadd/tests/common/mod.rs`, used by both `ipc_integration.rs` and `module_host_sandbox.rs`, and fixed the latter's stale note. - The helper runs `cargo build -p bread-module-host --locked --message-format=json` and reads the path back out of the `compiler-artifact` message's `executable` field, instead of guessing a location. This is correct under any profile, `CARGO_TARGET_DIR`, or `CARGO_BUILD_TARGET`, since those are ordinary environment variables cargo already honours and the nested `Command` inherits from the test process. - Both test harnesses now pass that exact path to the `breadd` they spawn through a new `BREAD_MODULE_HOST_BIN` env var, which `resolve_module_host_binary` checks first, ahead of its normal `current_exe()`-relative resolution and before ever reaching the `PATH` fallback. - Added a `cargo build --workspace` step to `check.yml` before the test step, as cheap extra insurance alongside the in-test fix (the review's "cheaper alternative, or in addition"). - Added a unit test pinning that the env var override wins outright over the normal resolution path. ## Verification - Locally from a completely fresh `CARGO_TARGET_DIR`: `cargo build --workspace --locked`, `cargo clippy --workspace --all-targets --locked -- -D warnings`, `cargo fmt --all -- --check`, and `cargo test --workspace --locked`, all clean (163 breadd unit tests, 37 `ipc_integration` tests, 2 `module_host_sandbox` tests). - Specifically reproduced the exact scenario the review called out: `cargo test -p breadd --locked --test module_host_sandbox` run alone, from a brand-new empty target dir, with nothing else having built `bread-module-host` first. Confirmed the binary actually gets built at that fresh target dir (`debug/bread-module-host`) by this helper, and both tests pass. - Repeated the full `cargo build`, `cargo clippy` and `cargo test --workspace --locked` runs inside this repo's own pinned CI container via `ci/build.sh` (the same one CI uses), all green. No merge; leaving this for review. Relates to ECO-34.

Breadway added 1 commit

2026-09-29 08:45:08 +08:00

fix: build and pin the real bread-module-host binary for integration tests

check / check (push) Successful in 1m58s

Details

4a1cd540a3

Follow-up to the post-merge review on PR #6 (5059771/af38653): those
commits fixed the immediate CI failure, but ipc_integration.rs's nested
`cargo build -p bread-module-host` was fragile in three ways. It guessed a
target/{debug,release} location instead of reading the path cargo actually
used, it silently fell back to a bare PATH lookup (which can pick up a
stale, separately installed ~/.local/bin/bread-module-host) under a
different profile or --target-dir, and module_host_sandbox.rs never called
it at all, relying on ipc_integration happening to build first.

- Moved the build-and-locate helper into breadd/tests/common/mod.rs,
  shared by both integration test files, and fixed
  module_host_sandbox.rs's now-wrong "cargo test --workspace already built
  it" note.
- The helper now runs `cargo build -p bread-module-host --message-format=json`
  and reads the path back out of the compiler-artifact message's
  `executable` field, instead of guessing. This is correct under any
  profile, CARGO_TARGET_DIR, or CARGO_BUILD_TARGET, since those are
  environment variables cargo already honours on its own and the nested
  build inherits from the test process.
- Both test harnesses now pass that exact path to the breadd they spawn
  through a new BREAD_MODULE_HOST_BIN env var, which
  resolve_module_host_binary checks first, ahead of its normal
  current_exe()-relative resolution. A test run now never falls through to
  guessing or PATH.
- Added a `cargo build --workspace` step to check.yml before the test step,
  as cheap extra insurance alongside the in-test fix.
- Added a unit test pinning that the env var override wins outright.

Relates to ECO-34.

Breadway merged commit 80535a2d5c into main

2026-09-29 12:55:35 +08:00

Breadway deleted branch fix/module-host-test-harness

2026-09-29 12:55:35 +08:00

Breadway referenced this pull request from a commit

2026-09-29 12:55:36 +08:00

Merge pull request 'Build and pin the real bread-module-host binary for integration tests' (#7) from fix/module-host-test-harness into main

Breadway referenced this pull request

2026-10-09 15:34:17 +08:00

feat(bug): status check and opt-in fixed notifications #26