feat: v0.2 enhancements — LSP, multi-base diffs, directory overrides, analytics #5
+25
−1
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,164 @@ | ||||||||
| # Plan: Fix xtask Test Failures | ||||||||
|
|
||||||||
| **Created:** 2026-04-04 | ||||||||
| **Updated:** 2026-04-04 | ||||||||
| **Priority:** P0 (blocking CI / PR #5) | ||||||||
| **Status:** ready-for-execution | ||||||||
|
|
||||||||
| ## Problem | ||||||||
|
|
||||||||
| 3 tests in `xtask` fail, blocking CI and PR #5 merge:``` | ||||||||
| tests::run_with_args_dispatches_ci_with_fake_cargo - FAIL | ||||||||
| tests::run_with_args_dispatches_conform_quick - FAIL | ||||||||
| tests::run_with_args_dispatches_mutants_with_fake_cargo - FAIL | ||||||||
| ``` | ||||||||
|
|
||||||||
| ## Root Cause Analysis | ||||||||
|
|
||||||||
| ### Issue 1: Missing Binary for Conformance Tests | ||||||||
|
|
||||||||
| `run_with_args_dispatches_conform_quick` calls `conform::run_conformance(true)` which runs conformance tests. Tests 3 (Survivability) and 9 (Tool error code) need the `diffguard` binary, but: | ||||||||
|
|
||||||||
| - `CARGO_BIN_EXE_diffguard` is set by `cargo test` to point to the xtask binary, not diffguard | ||||||||
| - `cargo_bin_path()` returns wrong path | ||||||||
| - `run_diffguard()` fails to find the binary | ||||||||
|
|
||||||||
| ### Issue 2: Mutex Poisoning Cascade | ||||||||
|
|
||||||||
| When `run_with_args_dispatches_conform_quick` panics, subsequent tests that use `ENV_LOCK.lock().unwrap()` fail with `PoisonError`. | ||||||||
|
|
||||||||
| The panic happens in `main.rs::tests` but doesn't use `ENV_LOCK`. However, the panic still affects test state. | ||||||||
|
|
||||||||
| ### Issue 3: `cargo_bin_path()` Resolution | ||||||||
|
|
||||||||
| In `conform_real.rs`, `cargo_bin_path()` prefers `CARGO_BIN_EXE_diffguard` env var, but when running `cargo test -p xtask`, this points to the xtask test binary, not diffguard. | ||||||||
|
|
||||||||
| ## Solution | ||||||||
|
|
||||||||
| ### Task 1: Fix Binary Path Resolution | ||||||||
|
|
||||||||
| **File:** `xtask/src/conform_real.rs` | ||||||||
| **Lines:** 1296-1330 | ||||||||
|
|
||||||||
| Modify `cargo_bin_path()` to: | ||||||||
| 1. Check for `CARGO_BIN_EXE_diffguard` env var first | ||||||||
| 2. Fall back to workspace-relative `target/debug/diffguard` path | ||||||||
| 3. Build binary if missing | ||||||||
|
|
||||||||
| ```rust | ||||||||
| fn cargo_bin_path() -> String { | ||||||||
| // Prefer env var if set and valid (points to actual diffguard binary) | ||||||||
| if let Ok(bin) = std::env::var("CARGO_BIN_EXE_diffguard") { | ||||||||
| // Verify it's the diffguard binary, not xtask | ||||||||
| if bin.contains("diffguard") && !bin.contains("xtask") { | ||||||||
| return bin; | ||||||||
| } | ||||||||
| } | ||||||||
| // Fall back to workspace-relative path | ||||||||
| workspace_root() | ||||||||
| .join("target") | ||||||||
| .join("debug") | ||||||||
| .join(if cfg!(windows) { "diffguard.exe" } else { "diffguard" }) | ||||||||
| .to_str() | ||||||||
| .unwrap() | ||||||||
| .to_string() | ||||||||
| } | ||||||||
| ``` | ||||||||
|
Comment on lines
+48
to
+66
There was a problem hiding this comment. Proposed binary path validation has critical flaws. The string-based check on line 53 is fragile and can fail in valid scenarios:
Consider a path-based approach instead: 🔧 Proposed fix using Path-based validation fn cargo_bin_path() -> String {
// Prefer env var if set and valid (points to actual diffguard binary)
if let Ok(bin) = std::env::var("CARGO_BIN_EXE_diffguard") {
- // Verify it's the diffguard binary, not xtask
- if bin.contains("diffguard") && !bin.contains("xtask") {
- return bin;
+ // Verify it's actually the diffguard binary by checking the filename
+ let path = std::path::Path::new(&bin);
+ if let Some(filename) = path.file_name().and_then(|n| n.to_str()) {
+ let expected = if cfg!(windows) { "diffguard.exe" } else { "diffguard" };
+ if filename == expected && path.exists() {
+ return bin;
+ }
}
}
// Fall back to workspace-relative path
workspace_root()
.join("target")
.join("debug")
.join(if cfg!(windows) { "diffguard.exe" } else { "diffguard" })
- .to_str()
- .unwrap()
- .to_string()
+ .to_string_lossy()
+ .to_string()
}🤖 Prompt for AI Agents |
||||||||
|
|
||||||||
| ### Task 2: Add Build Step in `run_conformance` | ||||||||
|
|
||||||||
| **File:** `xtask/src/conform_real.rs` | ||||||||
|
|
||||||||
| At the start of `run_conformance()`, call `ensure_diffguard_built()`: | ||||||||
|
|
||||||||
| ```rust | ||||||||
| pub fn run_conformance(quick: bool) -> Result<()> { | ||||||||
| ensure_diffguard_built()?; // Add this line | ||||||||
| // ... rest of function | ||||||||
| } | ||||||||
| ``` | ||||||||
|
|
||||||||
| ### Task 3: Implement `ensure_diffguard_built` | ||||||||
|
|
||||||||
| ```rust | ||||||||
| fn ensure_diffguard_built() -> Result<()> { | ||||||||
| let binary = cargo_bin_path(); | ||||||||
| if !std::path::Path::new(&binary).exists() { | ||||||||
| eprintln!("Building diffguard binary for conformance tests..."); | ||||||||
| let status = Command::new("cargo") | ||||||||
| .args(["build", "--bin", "diffguard"]) | ||||||||
| .current_dir(workspace_root()) | ||||||||
| .status() | ||||||||
| .context("cargo build --bin diffguard")?; | ||||||||
| if !status.success() { | ||||||||
| bail!("Failed to build diffguard binary"); | ||||||||
| } | ||||||||
| } | ||||||||
| Ok(()) | ||||||||
| } | ||||||||
| ``` | ||||||||
|
|
||||||||
| ### Task 4: Handle Poisoned Mutex | ||||||||
|
|
||||||||
| **Files:** `xtask/src/main.rs`, `xtask/src/conform_real.rs` | ||||||||
|
|
||||||||
| Replace `.unwrap()` with recovery pattern: | ||||||||
|
|
||||||||
| ```rust | ||||||||
| // Before: | ||||||||
| let _guard = ENV_LOCK.lock().unwrap(); | ||||||||
|
|
||||||||
| // After: | ||||||||
| let _guard = ENV_LOCK.lock().unwrap_or_else(|e| { | ||||||||
| // Clear poison and continue | ||||||||
| e.into_inner() | ||||||||
| }); | ||||||||
| ``` | ||||||||
|
|
||||||||
| This allows tests to continue even if a previous test panicked while holding the lock. | ||||||||
|
|
||||||||
| ### Task 5: Fix Test Isolation | ||||||||
|
|
||||||||
| **File:** `xtask/src/main.rs` | ||||||||
|
|
||||||||
| The test `run_with_args_dispatches_conform_quick` should not panic when conformance fails. It should return a result or handle the error gracefully: | ||||||||
|
|
||||||||
| ```rust | ||||||||
| #[test] | ||||||||
| fn run_with_args_dispatches_conform_quick() { | ||||||||
| // Build first | ||||||||
| let binary = workspace_root() | ||||||||
| .join("target") | ||||||||
| .join("debug") | ||||||||
| .join(if cfg!(windows) { "diffguard.exe" } else { "diffguard" }); | ||||||||
| if !binary.exists() { | ||||||||
| let status = Command::new("cargo") | ||||||||
| .args(["build", "--bin", "diffguard"]) | ||||||||
| .status() | ||||||||
| .expect("cargo build"); | ||||||||
| assert!(status.success(), "Failed to build diffguard"); | ||||||||
| } | ||||||||
|
|
||||||||
| run_with_args(["xtask", "conform", "--quick"]).expect("run conform"); | ||||||||
| } | ||||||||
|
Comment on lines
+126
to
+143
There was a problem hiding this comment. 🧹 Nitpick | 🔵 Trivial Consider using a test setup function instead of inline build logic. The test still uses ♻️ Optional refactor for better test isolation// In a test utils module or at top of test module
fn ensure_test_binary_built() -> std::path::PathBuf {
let binary = workspace_root()
.join("target")
.join("debug")
.join(if cfg!(windows) { "diffguard.exe" } else { "diffguard" });
if !binary.exists() {
let status = Command::new("cargo")
.args(["build", "--bin", "diffguard"])
.status()
.expect("cargo build");
assert!(status.success(), "Failed to build diffguard");
}
binary
}
#[test]
fn run_with_args_dispatches_conform_quick() {
let _binary = ensure_test_binary_built();
run_with_args(["xtask", "conform", "--quick"]).expect("run conform");
}This makes the setup reusable across multiple tests that need the binary. 🤖 Prompt for AI Agents |
||||||||
| ``` | ||||||||
|
|
||||||||
| ## Verification Steps | ||||||||
|
|
||||||||
| 1. Run `cargo build --bin diffguard` | ||||||||
| 2. Run `cargo test -p xtask -- --test-threads=1` | ||||||||
| 3. Run `cargo run -p xtask -- conform` | ||||||||
| 4. Run `cargo run -p xtask -- ci` | ||||||||
|
|
||||||||
| ## Success Criteria | ||||||||
|
|
||||||||
| - [ ] All 13 xtask tests pass | ||||||||
| - [ ] All 15 conformance tests pass | ||||||||
| - [ ] `cargo run -p xtask -- ci` passes | ||||||||
| - [ ] No changes to actual diffguard logic (only test harness) | ||||||||
|
|
||||||||
| ## Notes | ||||||||
|
|
||||||||
| - This is blocking PR #5 | ||||||||
| - The fix is isolated to xtask test infrastructure | ||||||||
| - No production code changes needed | ||||||||
|
There was a problem hiding this comment. Add trailing newline. The file should end with a single newline character per markdown conventions. 📝 Proposed fix - No production code changes needed
+📝 Committable suggestion
Suggested change
🧰 Tools🪛 markdownlint-cli2 (0.22.0)[warning] 164-164: Files should end with a single newline character (MD047, single-trailing-newline) 🤖 Prompt for AI Agents |
||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix markdown formatting for the code fence.
The code fence should be surrounded by blank lines and have a language specified for proper rendering.
📝 Proposed fix for markdown formatting
🧰 Tools
🪛 LanguageTool
[typographical] ~11-~11: To join two clauses or introduce examples, consider using an em dash.
Context: ..._with_args_dispatches_ci_with_fake_cargo - FAIL tests::run_with_args_dispatches_con...
(DASH_RULE)
[typographical] ~12-~12: To join two clauses or introduce examples, consider using an em dash.
Context: ...::run_with_args_dispatches_conform_quick - FAIL tests::run_with_args_dispatches_mut...
(DASH_RULE)
[typographical] ~13-~13: To join two clauses or introduce examples, consider using an em dash.
Context: ..._args_dispatches_mutants_with_fake_cargo - FAIL ``` ## Root Cause Analysis ### Is...
(DASH_RULE)
🪛 markdownlint-cli2 (0.22.0)
[warning] 14-14: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents