diff --git a/.changepacks/changepack_log_build_identity_race.json b/.changepacks/changepack_log_build_identity_race.json new file mode 100644 index 0000000..bdee90a --- /dev/null +++ b/.changepacks/changepack_log_build_identity_race.json @@ -0,0 +1,7 @@ +{ + "changes": { + "crates/devup-mcp/Cargo.toml": "Patch" + }, + "note": "Stop failing a build-identity test for something that is not a defect. The test compared the -dirty suffix baked into the binary at compile time against a git status it ran itself at test time, which are two observations of the working tree taken at two different moments; an untracked file appearing in between, a probe script or a scratch log, made a correctly behaving binary report clean while the test's own git call reported dirty, and the assertion failed with no bug to find. It failed that way during the line-box investigation. The build script now publishes what it actually observed, DEVUP_MCP_GIT_DIRTY as true, false or unknown when git could not be asked, together with DEVUP_MCP_BUILD_ID_SOURCE recording whether the identity came from git or from an injected DEVUP_MCP_BUILD_ID, and the test asserts against those build-time facts instead. What stays under test is the chain that can actually break: the git observation, the suffix git_identity derives from it, and --version printing the baked value faithfully; an injected identity carries whatever suffix its caller chose and an unknown observation says nothing about this plumbing, so neither is asserted. The run-time comparison remains as a note on stderr, because a tree that moved between building and running is information about the run rather than a fault in the binary.", + "date": "2026-09-12T21:40:00+09:00" +} diff --git a/crates/devup-mcp/build.rs b/crates/devup-mcp/build.rs index 79cec8b..aea21a3 100644 --- a/crates/devup-mcp/build.rs +++ b/crates/devup-mcp/build.rs @@ -22,12 +22,51 @@ fn main() { println!("cargo:rerun-if-changed={}", path.display()); } - let build_id = env::var("DEVUP_MCP_BUILD_ID") + let overridden = env::var("DEVUP_MCP_BUILD_ID") .ok() - .filter(|value| safe(value)) + .filter(|value| safe(value)); + let source = if overridden.is_some() { "env" } else { "git" }; + let build_id = overridden .or_else(git_build_id) .unwrap_or_else(|| "source-unknown".to_owned()); println!("cargo:rustc-env=DEVUP_MCP_BUILD_ID={build_id}"); + + // What the working tree looked like AT BUILD TIME, and where the build id + // came from. Without these, a test can only compare the baked `-dirty` + // suffix against a fresh `git status`, which is a different observation at + // a different time: an untracked file appearing after compilation - a probe + // script, a scratch log - makes the binary say clean while the test's own + // git call says dirty, and the test fails for a reason that is not a defect. + // That happened. Recording the build-time facts lets the test check the + // plumbing that can actually break (git observation -> `git_identity` + // suffix -> `--version` output) without asserting that the tree stood still. + println!( + "cargo:rustc-env=DEVUP_MCP_BUILD_ID_SOURCE={}", + if build_id == "source-unknown" { + "unknown" + } else { + source + } + ); + println!( + "cargo:rustc-env=DEVUP_MCP_GIT_DIRTY={}", + match git_dirty() { + Some(true) => "true", + Some(false) => "false", + None => "unknown", + } + ); +} + +/// Whether the working tree had changes when this build ran, or `None` when git +/// could not be asked. Separate from [`git_build_id`] so the observation can be +/// published on its own rather than only surviving as a `-dirty` suffix. +fn git_dirty() -> Option { + let status = Command::new("git") + .args(["status", "--porcelain=v1", "--untracked-files=normal"]) + .output() + .ok()?; + status.status.success().then_some(!status.stdout.is_empty()) } fn git_build_id() -> Option { diff --git a/crates/devup-mcp/tests/cli.rs b/crates/devup-mcp/tests/cli.rs index def25f2..ef9c968 100644 --- a/crates/devup-mcp/tests/cli.rs +++ b/crates/devup-mcp/tests/cli.rs @@ -47,14 +47,6 @@ fn version_flag_reports_the_installed_binary_version() { #[test] fn version_build_id_reports_the_repository_dirty_state() { - let repository = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../.."); - let status = Command::new("git") - .args(["status", "--porcelain=v1", "--untracked-files=normal"]) - .current_dir(repository) - .output() - .expect("inspect repository status"); - assert!(status.status.success()); - let output = Command::new(env!("CARGO_BIN_EXE_devup-mcp")) .arg("--version") .output() @@ -67,7 +59,61 @@ fn version_build_id_reports_the_repository_dirty_state() { .and_then(|(_, value)| value.strip_suffix(')')) .expect("version output includes a parenthesized build ID"); - assert_eq!(build_id.ends_with("-dirty"), !status.stdout.is_empty()); + // Compare the baked suffix against what the build script OBSERVED, not + // against a fresh `git status`. Those are two observations at two different + // times: the suffix is fixed at compile time, and an untracked file + // appearing afterwards - a probe script, a scratch log - makes a run-time + // git call disagree with a binary that is behaving correctly. This test + // failed exactly that way during the line-box investigation. + // + // What remains under test is the chain that can actually break: the build + // script's git observation, `git_identity`'s suffix, and `--version` + // printing the baked value faithfully. + match ( + env!("DEVUP_MCP_BUILD_ID_SOURCE"), + env!("DEVUP_MCP_GIT_DIRTY"), + ) { + ("git", "true") => assert!( + build_id.ends_with("-dirty"), + "the build script saw a dirty tree, so --version must say so: {build_id}" + ), + ("git", "false") => assert!( + !build_id.ends_with("-dirty"), + "the build script saw a clean tree, so --version must not claim dirty: {build_id}" + ), + // An injected DEVUP_MCP_BUILD_ID carries whatever suffix its caller + // chose, and "unknown" means git could not be asked at build time. + // Neither says anything about this plumbing, so neither is asserted. + (source, dirty) => { + assert!( + !build_id.is_empty(), + "a build ID is still required (source={source}, gitDirty={dirty})" + ); + } + } + + // Advisory only: if the tree moved between compiling and running, say so + // rather than failing. A mismatch here is information about the run, not a + // defect in the binary. + let repository = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("../.."); + if let Ok(status) = Command::new("git") + .args(["status", "--porcelain=v1", "--untracked-files=normal"]) + .current_dir(repository) + .output() + && status.status.success() + { + let now_dirty = !status.stdout.is_empty(); + if env!("DEVUP_MCP_GIT_DIRTY") == "true" && !now_dirty + || env!("DEVUP_MCP_GIT_DIRTY") == "false" && now_dirty + { + eprintln!( + "note: the working tree changed between build and run \ + (build saw dirty={}, now dirty={now_dirty}); the baked build ID is \ + still correct for the build that produced it", + env!("DEVUP_MCP_GIT_DIRTY") + ); + } + } } #[test]