From 354eb694740deba0719c24676aa4562cc7a6434e Mon Sep 17 00:00:00 2001 From: Cedric ANTHONY Date: Mon, 31 Aug 2026 08:03:18 +0200 Subject: [PATCH] fix(vcs): ignore structural Jujutsu tree entries --- Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 2 +- herdr-plugin.toml | 2 +- src/vcs/jj.rs | 115 ++++++++++++++++++++++++++++++++++++++++-- tests/jj_status.rs | 41 +++++++++++++++ tests/test_release.py | 6 +-- 7 files changed, 159 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1e89c27..92ad509 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -415,7 +415,7 @@ checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" [[package]] name = "herdr-context" -version = "0.19.4" +version = "0.19.5" dependencies = [ "cap-fs-ext", "cap-primitives", diff --git a/Cargo.toml b/Cargo.toml index 76c4ce5..7369201 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "herdr-context" -version = "0.19.4" +version = "0.19.5" edition = "2024" rust-version = "1.97.1" autobenches = false diff --git a/README.md b/README.md index eaabaed..78ee946 100644 --- a/README.md +++ b/README.md @@ -335,7 +335,7 @@ test suite. ## Release status -Version `0.19.4` is the current release line. Tag `v0.19.4`, Cargo +Version `0.19.5` is the current release line. Tag `v0.19.5`, Cargo metadata, both manifests, and the minimum Herdr version are validated together; pushing a `v*` tag runs formatting, Clippy, the full test suite, and the contract checks, then publishes generated GitHub release notes. diff --git a/herdr-plugin.toml b/herdr-plugin.toml index 0848bf9..ba680c0 100644 --- a/herdr-plugin.toml +++ b/herdr-plugin.toml @@ -1,6 +1,6 @@ id = "herdr-context" name = "herdr-context" -version = "0.19.4" +version = "0.19.5" min_herdr_version = "0.8.0" description = "Per-tab project context dock" platforms = ["linux", "macos"] diff --git a/src/vcs/jj.rs b/src/vcs/jj.rs index 9f8c917..86c0d85 100644 --- a/src/vcs/jj.rs +++ b/src/vcs/jj.rs @@ -1,5 +1,5 @@ use std::fs; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use std::sync::atomic::AtomicBool; use std::time::Duration; @@ -265,10 +265,12 @@ fn parse_templated_diff(output: &[u8], stale: bool) -> Result Result Result Result { .map_err(|_| invalid_output("Jujutsu path is not valid UTF-8")) } +/// Borrowed view of a raw status template path; on non-unix platforms this +/// is where encoding validation stays fail-closed. +#[cfg(unix)] +fn status_path(path: &[u8]) -> Result<&Path, VcsError> { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + Ok(Path::new(OsStr::from_bytes(path))) +} + +#[cfg(not(unix))] +fn status_path(path: &[u8]) -> Result<&Path, VcsError> { + let path = + std::str::from_utf8(path).map_err(|_| invalid_output("Jujutsu path is not valid UTF-8"))?; + Ok(Path::new(path)) +} + +// Validates a raw status path without owning it: platform encoding always, +// then the VcsEntryStatus normalization invariants whenever the path is +// carried into an entry (target always, source only for renames/copies). +fn validate_status_path(path: &[u8], in_entry: bool) -> Result<(), VcsError> { + let path = status_path(path)?; + if !in_entry { + return Ok(()); + } + let mut components = path.components(); + let Some(first) = components.next() else { + return Err(invalid_output("Jujutsu status path is empty")); + }; + if !matches!(first, Component::Normal(_)) + || components.any(|component| !matches!(component, Component::Normal(_))) + { + return Err(invalid_output("Jujutsu status path is not normalized")); + } + Ok(()) +} + fn trim_ascii(mut value: &[u8]) -> &[u8] { while value.first().is_some_and(u8::is_ascii_whitespace) { value = &value[1..]; @@ -428,6 +475,66 @@ mod tests { } } + #[test] + fn parser_skips_structural_tree_records() { + for output in [ + b"A\0src\0src\0false\0false\0\0tree\0".as_slice(), + b"M\0src\0src\0false\0false\0tree\0tree\0".as_slice(), + b"D\0src\0src\0false\0false\0tree\0\0".as_slice(), + ] { + let snapshot = parse_templated_diff(output, false).expect("status"); + assert!(snapshot.entries().is_empty()); + } + } + + #[test] + fn parser_keeps_file_to_tree_as_type_changed() { + let snapshot = parse_templated_diff(b"M\0src\0src\0false\0false\0file\0tree\0", false) + .expect("status"); + assert_eq!(snapshot.entries().len(), 1); + assert_eq!(snapshot.entries()[0].kind(), VcsStatusKind::TypeChanged); + } + + #[test] + fn parser_validates_paths_before_structural_filtering() { + for output in [ + // Structural pair, but the target escapes the workspace root. + b"A\0src\0../outside\0false\0false\0\0tree\0".as_slice(), + // Structural rename with an empty source path. + b"R\0\0src\0false\0false\0tree\0tree\0".as_slice(), + // Structural rename with a non-normalized source path. + b"R\0../old\0src\0false\0false\0tree\0tree\0".as_slice(), + ] { + assert_eq!( + parse_templated_diff(output, false) + .expect_err("invalid output") + .kind(), + VcsErrorKind::InvalidData + ); + } + } + + #[test] + fn parser_bounds_records_before_structural_filtering() { + let record: &[u8] = b"A\0src\0src\0false\0false\0\0tree\0"; + let at_limit = record.repeat(super::MAX_STATUS_ENTRIES); + assert!( + parse_templated_diff(&at_limit, false) + .expect("status") + .entries() + .is_empty() + ); + + let mut over_limit = at_limit; + over_limit.extend_from_slice(record); + assert_eq!( + parse_templated_diff(&over_limit, false) + .expect_err("record limit exceeded") + .kind(), + VcsErrorKind::InvalidData + ); + } + #[test] fn workspace_root_parser_preserves_spaces_and_line_break_bytes() { assert_eq!( diff --git a/tests/jj_status.rs b/tests/jj_status.rs index 0771784..49a924f 100644 --- a/tests/jj_status.rs +++ b/tests/jj_status.rs @@ -195,6 +195,47 @@ fn mixed_non_conflicted_descendants_aggregate_as_modified() { ); } +#[test] +fn added_descendant_tree_records_mark_directories_modified() { + let temp = TempDir::new().expect("tempdir"); + fs::create_dir_all(temp.path().join("src/nested")).expect("directories"); + fs::write(temp.path().join("src/nested/added.rs"), []).expect("fixture file"); + let script = temp.path().join("fake-jj"); + executable( + &script, + "#!/bin/sh\nprintf 'A\\000\\000src\\000false\\000false\\000\\000tree\\000A\\000\\000src/nested\\000false\\000false\\000\\000tree\\000A\\000\\000src/nested/added.rs\\000false\\000false\\000\\000file\\000'\n", + ); + let mut service = + JjService::with_executable(script, JujutsuMode::Fresh, Duration::from_secs(1)); + let workspace = herdr_context::vcs::VcsWorkspace::new( + temp.path().to_path_buf(), + herdr_context::vcs::VcsBackendMetadata::new("jj", "Jujutsu", true).expect("metadata"), + ) + .expect("workspace"); + let snapshot = service.refresh_status(&workspace).expect("status"); + let mut tree = FilesTree::new(temp.path().to_path_buf()).expect("tree"); + tree.load_directory(Path::new("")).expect("root"); + tree.merge_status(&snapshot).expect("status overlay"); + tree.load_directory(Path::new("src")).expect("src"); + tree.load_directory(Path::new("src/nested")) + .expect("nested"); + + assert_eq!( + tree.node(Path::new("src")).expect("src").status(), + Some(VcsStatusKind::Modified) + ); + assert_eq!( + tree.node(Path::new("src/nested")).expect("nested").status(), + Some(VcsStatusKind::Modified) + ); + assert_eq!( + tree.node(Path::new("src/nested/added.rs")) + .expect("added row") + .status(), + Some(VcsStatusKind::Added) + ); +} + #[test] fn malformed_templated_output_is_rejected() { let temp = TempDir::new().expect("tempdir"); diff --git a/tests/test_release.py b/tests/test_release.py index c742db4..f83146e 100644 --- a/tests/test_release.py +++ b/tests/test_release.py @@ -19,15 +19,15 @@ class ReleaseContractTests(unittest.TestCase): def test_repository_contract_is_release_ready(self) -> None: contract = release.validate_repository(ROOT) - self.assertEqual(contract.version, "0.19.4") + self.assertEqual(contract.version, "0.19.5") self.assertEqual(contract.min_herdr_version, "0.8.0") self.assertEqual(len(contract.performance_metrics), 12) def test_trigger_tag_must_exactly_match_cargo_version(self) -> None: contract = release.validate_repository(ROOT) - release.validate_trigger_tag(contract, "v0.19.4") - with self.assertRaisesRegex(release.ReleaseError, "exactly v0.19.4"): + release.validate_trigger_tag(contract, "v0.19.5") + with self.assertRaisesRegex(release.ReleaseError, "exactly v0.19.5"): release.validate_trigger_tag(contract, "vtest") def test_failed_budget_requires_complete_risk_acceptance(self) -> None: