From 1b0f09defd2fbba4476368b9b9c5663cecd49e3d Mon Sep 17 00:00:00 2001 From: Travis Gockel Date: Sun, 12 Jul 2026 18:27:32 -0600 Subject: [PATCH] test: fix stale malformed-label e2e test, split by failure site Since repo builds started re-merging inherited labels into the cache tag (stamp_repo_image_labels), a malformed org.outrig.mcp label hard-errors during ensure_image itself, so the old malformed_mcp_label_is_hard_error test panicked in its fixture helper before reaching the runtime-read assertion. The build-time failure is the better behavior; the test now asserts it directly. The runtime-read path is still reachable: the ImageSourceRef::Image branch only probes/pulls and never re-validates labels, so a raw (non-built) image ref with a malformed label passes ensure_image and fails only at embedded::merged_mcp. A new e2e case covers it, building the raw fixture with image::build_standalone (which stamps nothing beyond the Dockerfile's own labels) under a fixed tag rebuilt each run. merged_mcp reads labels off the handle's image tag without exec'ing, so the test uses Container::attach instead of a full start/bootstrap/stop cycle, and both malformed-label fixtures are bare alpine + LABEL -- no node toolchain, no shadow. Also dedupes the Build-source ImageConfig literal behind a shared fallible try_ensure_built_image helper and adds a non-e2e unit test pinning the hard-error site (merged_mcp_config_to_labels rejects a malformed inherited label) in plain cargo test. --- crates/outrig/src/container/embedded.rs | 9 ++ crates/outrig/tests/embedded_image.rs | 113 ++++++++++++++------ plan/done/0083-malformed-label-e2e-stale.md | 50 +++++++++ plan/todo/0083-malformed-label-e2e-stale.md | 29 ----- plan/todo/README.md | 1 - 5 files changed, 142 insertions(+), 60 deletions(-) create mode 100644 plan/done/0083-malformed-label-e2e-stale.md delete mode 100644 plan/todo/0083-malformed-label-e2e-stale.md diff --git a/crates/outrig/src/container/embedded.rs b/crates/outrig/src/container/embedded.rs index 057c172..dd8cd10 100644 --- a/crates/outrig/src/container/embedded.rs +++ b/crates/outrig/src/container/embedded.rs @@ -672,6 +672,15 @@ mod tests { assert_eq!(labels[LABEL_SCHEMA], LABEL_SCHEMA_VERSION); } + #[test] + fn merged_mcp_config_labels_reject_malformed_inherited_label() { + let inherited = BTreeMap::from([(LABEL_MCP.to_string(), r#"{"fs": ["#.to_string())]); + + let err = merged_mcp_config_to_labels("img", &inherited, &BTreeMap::new()).unwrap_err(); + + assert!(matches!(err, OutrigError::EmbeddedImageConfigParse { .. })); + } + #[test] fn standalone_image_labels_read_metadata_and_mcp() { let mut mcp = BTreeMap::new(); diff --git a/crates/outrig/tests/embedded_image.rs b/crates/outrig/tests/embedded_image.rs index 4c4e3c0..8eb843a 100644 --- a/crates/outrig/tests/embedded_image.rs +++ b/crates/outrig/tests/embedded_image.rs @@ -51,6 +51,35 @@ fn mcp_label_json(mcp: &BTreeMap) -> String { serde_json::to_string(mcp).expect("serialize mcp label json") } +/// Tempdir image context whose Dockerfile carries only a malformed +/// `org.outrig.mcp` label on the alpine base -- the parse failure under test +/// never touches the layer contents, so no tooling is installed. +fn malformed_label_context() -> tempfile::TempDir { + let ctx = tempfile::tempdir().expect("tempdir image context"); + let dockerfile = format!( + "FROM docker.io/library/alpine:latest\n{}", + label_line(embedded::LABEL_MCP, r#"{"fs": ["#), + ); + std::fs::write(ctx.path().join("Dockerfile"), dockerfile).expect("write Dockerfile"); + ctx +} + +/// `ensure_image` over a Build-source config rooted at `project_dir` (which +/// must contain a `Dockerfile`). Returns the result so tests can assert on +/// build-time failures. +async fn try_ensure_built_image(project_dir: &Path) -> outrig::error::Result { + let cfg = ImageConfig { + image_name: None, + dockerfile: Some("Dockerfile".into()), + context: Some(".".into()), + build_args: BTreeMap::new(), + security: Default::default(), + mcp: BTreeMap::new(), + sidecars: BTreeMap::new(), + }; + Ok(image::ensure_image(&cfg, project_dir, false).await?.tag) +} + /// Build an alpine + filesystem-MCP fixture image carrying `labels` (stamped via /// Dockerfile `LABEL`, mirroring what `outrig image build` stamps via /// `--label`). An empty slice builds a label-free image. @@ -65,20 +94,9 @@ async fn ensure_image_with_labels(labels: &[(&str, &str)]) -> ImageTag { dockerfile.push_str(&label_line(key, value)); } std::fs::write(ctx.path().join("Dockerfile"), dockerfile).expect("write Dockerfile"); - - let cfg = ImageConfig { - image_name: None, - dockerfile: Some("Dockerfile".into()), - context: Some(".".into()), - build_args: BTreeMap::new(), - security: Default::default(), - mcp: BTreeMap::new(), - sidecars: BTreeMap::new(), - }; - image::ensure_image(&cfg, ctx.path(), false) + try_ensure_built_image(ctx.path()) .await .expect("ensure embedded fixture image") - .tag } /// Convenience over [`ensure_image_with_labels`] for the common case: stamp just @@ -91,19 +109,9 @@ async fn ensure_image_with_mcp(mcp: &BTreeMap) -> ImageTa /// Build a label-free image (the committed mcp-fs fixture has the tool binaries /// but no `org.outrig.*` labels) -- runtime must fall back to repo config. async fn ensure_label_free_image() -> ImageTag { - let cfg = ImageConfig { - image_name: None, - dockerfile: Some("Dockerfile".into()), - context: Some(".".into()), - build_args: BTreeMap::new(), - security: Default::default(), - mcp: BTreeMap::new(), - sidecars: BTreeMap::new(), - }; - image::ensure_image(&cfg, &fixture_mcp_fs_dir(), false) + try_ensure_built_image(&fixture_mcp_fs_dir()) .await .expect("ensure mcp-fs fixture image") - .tag } async fn start_and_bootstrap(image: &ImageTag, host_ws: &Path) -> Container { @@ -237,19 +245,64 @@ async fn missing_label_falls_back_to_config() { } #[tokio::test] -async fn malformed_mcp_label_is_hard_error() { +async fn malformed_mcp_label_fails_ensure_image() { common::init_tracing(); let _guard = E2E_LOCK.lock().await; - let image = ensure_image_with_labels(&[(embedded::LABEL_MCP, r#"{"fs": ["#)]).await; - let host_ws = tempfile::tempdir().expect("tempdir host_ws"); - let container = start_and_bootstrap(&image, host_ws.path()).await; + // Repo builds re-merge inherited labels into the cache tag + // (stamp_repo_image_labels), so a malformed org.outrig.mcp label fails the + // build itself rather than the later runtime read. + let ctx = malformed_label_context(); - let err = embedded::merged_mcp(&container, &BTreeMap::new()) + let err = try_ensure_built_image(ctx.path()) .await - .expect_err("malformed org.outrig.mcp label should fail"); + .expect_err("malformed org.outrig.mcp label should fail ensure_image"); assert!(matches!(err, OutrigError::EmbeddedImageConfigParse { .. })); +} - container.stop(Duration::from_secs(2)).await.expect("stop"); +#[tokio::test] +async fn malformed_mcp_label_on_raw_image_fails_runtime_read() { + common::init_tracing(); + let _guard = E2E_LOCK.lock().await; + // A raw (non-built) image ref skips the build path's label re-merge, so a + // malformed label must still hard-error at the runtime read. The standalone + // build stamps nothing beyond the Dockerfile's own labels, and the fixed + // tag is rebuilt on every run, so nothing accumulates in local storage. + let ctx = malformed_label_context(); + let tag = ImageTag("outrig-raw-malformed:e2e".to_string()); + image::build_standalone( + ctx.path(), + Path::new("Dockerfile"), + Path::new("."), + &tag, + false, + &BTreeMap::new(), + ) + .await + .expect("build raw fixture image"); + + // ensure_image on an image-ref config only probes/pulls -- it must not + // re-validate labels, so the malformed image passes here. + let cfg = ImageConfig { + image_name: Some(tag.0.clone()), + dockerfile: None, + context: None, + build_args: BTreeMap::new(), + security: Default::default(), + mcp: BTreeMap::new(), + sidecars: BTreeMap::new(), + }; + let image = image::ensure_image(&cfg, ctx.path(), false) + .await + .expect("raw image ref should pass ensure_image without label validation") + .tag; + + // merged_mcp reads labels off the handle's image tag without ever exec'ing + // into the container, so an attached handle stands in for a running one. + let container = Container::attach("raw-malformed-runtime-read", image, None, None); + let err = embedded::merged_mcp(&container, &BTreeMap::new()) + .await + .expect_err("malformed org.outrig.mcp label should fail the runtime read"); + assert!(matches!(err, OutrigError::EmbeddedImageConfigParse { .. })); } #[tokio::test] diff --git a/plan/done/0083-malformed-label-e2e-stale.md b/plan/done/0083-malformed-label-e2e-stale.md new file mode 100644 index 0000000..463bedd --- /dev/null +++ b/plan/done/0083-malformed-label-e2e-stale.md @@ -0,0 +1,50 @@ +# 0083 -- Fix stale `malformed_mcp_label_is_hard_error` e2e test + +## Goal + +Repair the stale e2e test. `cargo test -p outrig --features e2e --test embedded_image +malformed_mcp_label` fails on trunk (verified against a clean tree during task 0079): the +test builds an image whose Dockerfile stamps a malformed `org.outrig.mcp` label and +expects `ensure_image` to succeed so the *runtime* read (`embedded::merged_mcp`) can fail. +Since the build path started re-merging inherited labels into the cache tag +(`merged_mcp_config_to_labels` at `image.rs:658`), the malformed label hard-errors during +`ensure_image` itself and the test's `.expect("ensure embedded fixture image")` panics +before the assertion runs. + +## Deliverables + +The behavior is arguably better (fail at build), so the fix is probably to update the +test to assert `ensure_image` fails with `EmbeddedImageConfigParse` -- and add a separate +runtime-read case using a raw (non-built) image ref if that path still needs coverage. + +## Acceptance + +- `cargo test -p outrig --features e2e --test embedded_image malformed_mcp_label` passes + on a clean tree. +- The runtime-read failure path (`embedded::merged_mcp`) either keeps coverage via a + raw-image-ref case or is shown to be unreachable. + +## Dependencies + +None (follows up on completed task 0079). + +## Decisions + +- Split the stale test into two, both keeping the `malformed_mcp_label` filter substring: + `malformed_mcp_label_fails_ensure_image` (build path hard-errors during the label + re-merge) and `malformed_mcp_label_on_raw_image_fails_runtime_read` (raw image ref + passes `ensure_image` untouched, fails at `embedded::merged_mcp`). The old name was + dropped -- it collided with the unit test `embedded_mcp_malformed_label_is_hard_error`. +- The runtime-read path is reachable only via the `ImageSourceRef::Image` branch, which + never re-validates labels; the raw fixture is built with `image::build_standalone` + (stamps nothing beyond the Dockerfile's own labels) rather than a hand-rolled buildah + invocation, under a fixed tag rebuilt each run so local storage doesn't accumulate. +- `merged_mcp` only reads labels off the handle's image tag, so the runtime-read test + uses `Container::attach` instead of a full start/bootstrap/stop cycle, and both + malformed-label fixtures are bare `FROM alpine` + `LABEL` (no node toolchain, no + shadow) -- the parse failure never touches layer contents. +- Runtime coverage stays library-level in `crates/outrig/tests/embedded_image.rs` (it + exercises `embedded::merged_mcp` directly); the CLI test file keeps its own raw-image + CLI cases. +- Bonus non-e2e regression test: `merged_mcp_config_labels_reject_malformed_inherited_label` + pins the new hard-error site (`merged_mcp_config_to_labels`) in plain `cargo test`. diff --git a/plan/todo/0083-malformed-label-e2e-stale.md b/plan/todo/0083-malformed-label-e2e-stale.md deleted file mode 100644 index ace6c0a..0000000 --- a/plan/todo/0083-malformed-label-e2e-stale.md +++ /dev/null @@ -1,29 +0,0 @@ -# 0083 -- Fix stale `malformed_mcp_label_is_hard_error` e2e test - -## Goal - -Repair the stale e2e test. `cargo test -p outrig --features e2e --test embedded_image -malformed_mcp_label` fails on trunk (verified against a clean tree during task 0079): the -test builds an image whose Dockerfile stamps a malformed `org.outrig.mcp` label and -expects `ensure_image` to succeed so the *runtime* read (`embedded::merged_mcp`) can fail. -Since the build path started re-merging inherited labels into the cache tag -(`merged_mcp_config_to_labels` at `image.rs:658`), the malformed label hard-errors during -`ensure_image` itself and the test's `.expect("ensure embedded fixture image")` panics -before the assertion runs. - -## Deliverables - -The behavior is arguably better (fail at build), so the fix is probably to update the -test to assert `ensure_image` fails with `EmbeddedImageConfigParse` -- and add a separate -runtime-read case using a raw (non-built) image ref if that path still needs coverage. - -## Acceptance - -- `cargo test -p outrig --features e2e --test embedded_image malformed_mcp_label` passes - on a clean tree. -- The runtime-read failure path (`embedded::merged_mcp`) either keeps coverage via a - raw-image-ref case or is shown to be unreachable. - -## Dependencies - -None (follows up on completed task 0079). diff --git a/plan/todo/README.md b/plan/todo/README.md index 619d140..a9c8e25 100644 --- a/plan/todo/README.md +++ b/plan/todo/README.md @@ -8,7 +8,6 @@ See [`.claude/CLAUDE.md`](../../.claude/CLAUDE.md) for the workflow conventions. | Task | Title | Dependencies | |------|----------------------------------------------------|--------------| -| 0083 | Fix stale `malformed_mcp_label_is_hard_error` test | -- | | 0084 | REPL slash-command dispatcher | -- | | 0085 | from_image_config sidecar translation | -- | | 0086 | Sidecar startup and clean-sweep performance | -- |