From d65983c9f870ac6ff932267b6190f3de662ed4e0 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Sat, 22 Aug 2026 12:06:20 +0530 Subject: [PATCH 1/6] Fixed binary-file crash, unified intent semantics, and flagged empty changes --- .gitignore | 1 + src/adapter/git.rs | 32 +++++++++++++++++++------------- src/adapter/jj.rs | 30 ++++++++++++++++++------------ src/dispute.rs | 27 ++++++++++++++++++++++----- src/docket.rs | 6 +++--- src/main.rs | 32 ++++++++++++++++++++++++++++---- tests/dispute_docket_test.rs | 8 ++++---- 7 files changed, 95 insertions(+), 41 deletions(-) diff --git a/.gitignore b/.gitignore index 281953e..4670528 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ *.env !fixtures/repo/head/secrets/.env *.DS_Store +.useit/ diff --git a/src/adapter/git.rs b/src/adapter/git.rs index d7fcc35..5210e17 100644 --- a/src/adapter/git.rs +++ b/src/adapter/git.rs @@ -1,7 +1,7 @@ //! Native Git adapter for extracting in-memory snapshots and adjudicating 3-way merges. use crate::change::{Change, Snapshot, Source}; -use crate::dispute::{Docket, Severity, Verdict}; +use crate::dispute::{Dispute, Docket, Severity, Verdict}; use crate::engine::Engine; use crate::policy::MeaningPolicy; use crate::visibility::VisibilityPolicy; @@ -222,14 +222,6 @@ impl GitAdapter { .any(|d| d.kind == crate::dispute::Kind::Visibility && d.severity == Severity::High); disputes.extend(vis_disputes); - let verdict = if cloaked { - Verdict::Cloaked - } else if visibility_policy.embargo_until.is_some() { - Verdict::Embargoed - } else { - meaning_policy.evaluate(&disputes) - }; - let mut touched_paths: Vec = base_snapshot .files .keys() @@ -240,19 +232,33 @@ impl GitAdapter { touched_paths.sort(); touched_paths.dedup(); - let scope = if touched_paths.is_empty() { - "no files changed".to_string() + if touched_paths.is_empty() { + disputes.push(Dispute::empty_change()); + } + + let verdict = if cloaked { + Verdict::Cloaked + } else if visibility_policy.embargo_until.is_some() { + Verdict::Embargoed } else { - touched_paths.join(", ") + meaning_policy.evaluate(&disputes) }; + let intent = options.intent.clone().unwrap_or_else(|| { + if touched_paths.is_empty() { + "no files changed".to_string() + } else { + touched_paths.join(", ") + } + }); + let docket = Docket { change: change.name, source: format!("git: {merge_base_sha:.7} (base) vs {head_sha:.7} (head)"), base: change.base_ref, head: change.head_ref, disputes, - scope, + intent, authors, verdict, embargo: visibility_policy.embargo_note(), diff --git a/src/adapter/jj.rs b/src/adapter/jj.rs index 48822fb..1099bef 100644 --- a/src/adapter/jj.rs +++ b/src/adapter/jj.rs @@ -294,14 +294,6 @@ impl JjAdapter { .any(|d| d.kind == Kind::Visibility && d.severity == Severity::High); disputes.extend(vis_disputes); - let verdict = if cloaked { - Verdict::Cloaked - } else if visibility_policy.embargo_until.is_some() { - Verdict::Embargoed - } else { - meaning_policy.evaluate(&disputes) - }; - let mut touched_paths: Vec = base_snapshot .files .keys() @@ -312,12 +304,26 @@ impl JjAdapter { touched_paths.sort(); touched_paths.dedup(); - let scope = if touched_paths.is_empty() { - "no files changed".to_string() + if touched_paths.is_empty() { + disputes.push(Dispute::empty_change()); + } + + let verdict = if cloaked { + Verdict::Cloaked + } else if visibility_policy.embargo_until.is_some() { + Verdict::Embargoed } else { - touched_paths.join(", ") + meaning_policy.evaluate(&disputes) }; + let intent = options.intent.clone().unwrap_or_else(|| { + if touched_paths.is_empty() { + "no files changed".to_string() + } else { + touched_paths.join(", ") + } + }); + let docket = Docket { change: change.name, source: format!( @@ -328,7 +334,7 @@ impl JjAdapter { base: change.base_ref, head: change.head_ref, disputes, - scope, + intent, authors, verdict, embargo: visibility_policy.embargo_note(), diff --git a/src/dispute.rs b/src/dispute.rs index b09646a..1cf2ad2 100644 --- a/src/dispute.rs +++ b/src/dispute.rs @@ -2,7 +2,7 @@ //! //! A [`Dispute`] represents a point of disagreement (either structural meaning //! or visibility violation). A [`Docket`] is the complete, rendered adjudication -//! record containing disputes, verdict, scope, and embargo metadata. +//! record containing disputes, verdict, intent, and embargo metadata. use serde::{Deserialize, Serialize}; @@ -54,6 +54,22 @@ pub struct Dispute { pub detail: String, } +impl Dispute { + /// Low-severity notice that a change contains no file differences. + /// + /// Does not affect blocking or review thresholds; it only makes the + /// empty change visible on the docket instead of passing silently. + pub fn empty_change() -> Dispute { + Dispute { + id: "D000".into(), + location: "-".into(), + kind: Kind::Meaning, + severity: Severity::Low, + detail: "no file differences between base and head".into(), + } + } +} + /// The final adjudication verdict for a change. #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "lowercase")] @@ -81,8 +97,9 @@ pub struct Docket { pub head: String, /// Collection of detected disputes. pub disputes: Vec, - /// Stated scope or intent of the change. - pub scope: String, + /// Stated intent of the change, or a summary of touched paths when none was given. + #[serde(alias = "scope")] + pub intent: String, /// Change author handles or agent identifiers. pub authors: Vec, /// Resulting adjudication verdict. @@ -144,7 +161,7 @@ impl Docket { )); } out.push('\n'); - out.push_str(&format!(" scope: {}\n", self.scope)); + out.push_str(&format!(" intent: {}\n", self.intent)); out.push_str(&format!(" authors: {}\n", self.authors.join(", "))); out.push('\n'); if self.disputes.is_empty() { @@ -233,7 +250,7 @@ mod tests { detail: "private path .env touched".into(), }, ], - scope: "auth refactor".into(), + intent: "auth refactor".into(), authors: vec!["@alice".into(), "@bob".into()], verdict: Verdict::Adjudicated, embargo: Some("patch held for maintainers until 2026-12-31".into()), diff --git a/src/docket.rs b/src/docket.rs index 70476b3..e61d96f 100644 --- a/src/docket.rs +++ b/src/docket.rs @@ -80,7 +80,7 @@ mod tests { base: "main".into(), head: "feature/test-docket".into(), disputes: vec![], - scope: "testing save and load".into(), + intent: "testing save and load".into(), authors: vec!["@tester".into()], verdict: Verdict::Adjudicated, embargo: None, @@ -91,7 +91,7 @@ mod tests { assert_eq!(loaded.change, original.change); assert_eq!(loaded.source, original.source); - assert_eq!(loaded.scope, original.scope); + assert_eq!(loaded.intent, original.intent); assert_eq!(loaded.verdict, original.verdict); let _ = std::fs::remove_file(path); @@ -105,7 +105,7 @@ mod tests { base: "main".into(), head: "feature/toml-test".into(), disputes: vec![], - scope: "toml format".into(), + intent: "toml format".into(), authors: vec!["@coder".into()], verdict: Verdict::Embargoed, embargo: Some("patch held for maintainers until 2026-12-31".into()), diff --git a/src/main.rs b/src/main.rs index 9d4c8d0..0eb5bea 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,7 +5,7 @@ use clap::{Parser, Subcommand}; use oot::adapter::{GitAdapter, GitAdjudicateOptions, JjAdapter, JjAdjudicateOptions}; use oot::change::{Change, Snapshot, Source}; -use oot::dispute::{Docket, Kind, Severity, Verdict}; +use oot::dispute::{Dispute, Docket, Kind, Severity, Verdict}; use oot::docket; use oot::engine::Engine; use oot::policy::MeaningPolicy; @@ -202,6 +202,21 @@ fn main() -> anyhow::Result { let mut disputes = eng.diff_snapshots(&change.base, &change.head)?; disputes.extend(vis_disputes); + let mut touched_paths: Vec = change + .base + .files + .keys() + .chain(change.head.files.keys()) + .filter(|p| change.base.files.get(*p) != change.head.files.get(*p)) + .cloned() + .collect(); + touched_paths.sort(); + touched_paths.dedup(); + + if touched_paths.is_empty() { + disputes.push(Dispute::empty_change()); + } + let verdict = if cloaked { Verdict::Cloaked } else if visibility_policy.embargo_until.is_some() { @@ -210,7 +225,13 @@ fn main() -> anyhow::Result { meaning_policy.evaluate(&disputes) }; - let scope = change.intent.clone().unwrap_or_else(|| "auto".into()); + let intent = change.intent.clone().unwrap_or_else(|| { + if touched_paths.is_empty() { + "no files changed".into() + } else { + touched_paths.join(", ") + } + }); let docket = Docket { change: change.name.clone(), @@ -218,7 +239,7 @@ fn main() -> anyhow::Result { base: change.base_ref.clone(), head: change.head_ref.clone(), disputes, - scope, + intent, authors: change.authors.clone(), verdict, embargo: visibility_policy.embargo_note(), @@ -260,7 +281,10 @@ fn load_dir( if p.is_dir() { load_dir(root, &p, files)?; } else { - let content = std::fs::read_to_string(&p)?; + // Read as bytes and convert lossily so binary files (images, + // lockfiles, etc.) are tracked as changed without failing the run. + let bytes = std::fs::read(&p)?; + let content = String::from_utf8_lossy(&bytes).into_owned(); let rel = p .strip_prefix(root) .unwrap_or(&p) diff --git a/tests/dispute_docket_test.rs b/tests/dispute_docket_test.rs index b5f82e3..5b5aafb 100644 --- a/tests/dispute_docket_test.rs +++ b/tests/dispute_docket_test.rs @@ -31,7 +31,7 @@ fn sample_docket() -> Docket { detail: "private path secrets/.env touched by @alice/@bob".into(), }, ], - scope: "Authentication and session management".into(), + intent: "Authentication and session management".into(), authors: vec!["@alice".into(), "@bob".into()], verdict: Verdict::Embargoed, embargo: Some("patch held for maintainers until 2026-09-01".into()), @@ -53,7 +53,7 @@ fn test_dispute_classification_and_counts() { base: "main".into(), head: "docs".into(), disputes: vec![], - scope: "doc updates".into(), + intent: "doc updates".into(), authors: vec!["@writer".into()], verdict: Verdict::Adjudicated, embargo: None, @@ -77,7 +77,7 @@ fn test_docket_json_serialization_roundtrip() { assert_eq!(deserialized.base, original.base); assert_eq!(deserialized.head, original.head); assert_eq!(deserialized.disputes.len(), original.disputes.len()); - assert_eq!(deserialized.scope, original.scope); + assert_eq!(deserialized.intent, original.intent); assert_eq!(deserialized.authors, original.authors); assert_eq!(deserialized.verdict, original.verdict); assert_eq!(deserialized.embargo, original.embargo); @@ -95,7 +95,7 @@ fn test_docket_toml_serialization_roundtrip() { assert_eq!(deserialized.base, original.base); assert_eq!(deserialized.head, original.head); assert_eq!(deserialized.disputes.len(), original.disputes.len()); - assert_eq!(deserialized.scope, original.scope); + assert_eq!(deserialized.intent, original.intent); assert_eq!(deserialized.authors, original.authors); assert_eq!(deserialized.verdict, original.verdict); assert_eq!(deserialized.embargo, original.embargo); From c6cf34eba3f9f5bab402809599b6e425f99859de Mon Sep 17 00:00:00 2001 From: kridaydave Date: Sat, 22 Aug 2026 12:28:04 +0530 Subject: [PATCH 2/6] Addressed review: shared adjudication tail, verdict-safe empty notice, regression tests --- TODO.md | 20 ++++- src/adapter/git.rs | 40 +++------- src/adapter/jj.rs | 40 +++------- src/dispute.rs | 53 ++++++++++++- src/main.rs | 41 +++------- tests/cli_test.rs | 141 +++++++++++++++++++++++++++++++++++ tests/dispute_docket_test.rs | 19 +++++ 7 files changed, 260 insertions(+), 94 deletions(-) diff --git a/TODO.md b/TODO.md index e3dbc6c..9f647cf 100644 --- a/TODO.md +++ b/TODO.md @@ -8,4 +8,22 @@ every change. Fixed by aligning the code with its own documented contract: only paths *touched* by a change (added, removed, or content-modified vs base) are checked. See `test_visibility_policy_only_flags_touched_private_paths`. -No open items. +## Binary change detection is lossy + +Snapshots store file contents as `String`, so non-UTF8 files go through +`String::from_utf8_lossy`, which collapses every invalid byte sequence to +U+FFFD. Two *different* binaries can therefore compare equal and register as +unchanged — a governance docket could then say "no files changed" for a +change that did alter a binary. This affects both `load_dir` +(src/main.rs) and the git adapter's `extract_snapshot` (src/adapter/git.rs). + +**Trigger:** when binary artifacts (images, lockfiles, compiled blobs) +become first-class inputs to adjudication, or any real docket shows a +false "no files changed". + +**Right fix:** store raw bytes (or a content hash) in `Snapshot.files` and +convert to text only at tree-sitter parse time, so touched-path detection +compares bytes exactly. + +Pinned by the lossy-conversion note in `load_dir` until then. + diff --git a/src/adapter/git.rs b/src/adapter/git.rs index 5210e17..90f84a2 100644 --- a/src/adapter/git.rs +++ b/src/adapter/git.rs @@ -1,7 +1,7 @@ //! Native Git adapter for extracting in-memory snapshots and adjudicating 3-way merges. use crate::change::{Change, Snapshot, Source}; -use crate::dispute::{Dispute, Docket, Severity, Verdict}; +use crate::dispute::{finalize_adjudication, Docket, Severity}; use crate::engine::Engine; use crate::policy::MeaningPolicy; use crate::visibility::VisibilityPolicy; @@ -222,35 +222,15 @@ impl GitAdapter { .any(|d| d.kind == crate::dispute::Kind::Visibility && d.severity == Severity::High); disputes.extend(vis_disputes); - let mut touched_paths: Vec = base_snapshot - .files - .keys() - .chain(head_snapshot.files.keys()) - .filter(|p| base_snapshot.files.get(*p) != head_snapshot.files.get(*p)) - .cloned() - .collect(); - touched_paths.sort(); - touched_paths.dedup(); - - if touched_paths.is_empty() { - disputes.push(Dispute::empty_change()); - } - - let verdict = if cloaked { - Verdict::Cloaked - } else if visibility_policy.embargo_until.is_some() { - Verdict::Embargoed - } else { - meaning_policy.evaluate(&disputes) - }; - - let intent = options.intent.clone().unwrap_or_else(|| { - if touched_paths.is_empty() { - "no files changed".to_string() - } else { - touched_paths.join(", ") - } - }); + let (disputes, intent, verdict) = finalize_adjudication( + disputes, + &base_snapshot.files, + &head_snapshot.files, + options.intent.clone(), + cloaked, + visibility_policy.embargo_until.is_some(), + meaning_policy, + ); let docket = Docket { change: change.name, diff --git a/src/adapter/jj.rs b/src/adapter/jj.rs index 1099bef..c4b5f78 100644 --- a/src/adapter/jj.rs +++ b/src/adapter/jj.rs @@ -6,7 +6,7 @@ //! caller's working copy. use crate::change::{Change, Snapshot, Source}; -use crate::dispute::{Dispute, Docket, Kind, Severity, Verdict}; +use crate::dispute::{finalize_adjudication, Dispute, Docket, Kind, Severity}; use crate::engine::Engine; use crate::policy::MeaningPolicy; use crate::visibility::VisibilityPolicy; @@ -294,35 +294,15 @@ impl JjAdapter { .any(|d| d.kind == Kind::Visibility && d.severity == Severity::High); disputes.extend(vis_disputes); - let mut touched_paths: Vec = base_snapshot - .files - .keys() - .chain(head_snapshot.files.keys()) - .filter(|p| base_snapshot.files.get(*p) != head_snapshot.files.get(*p)) - .cloned() - .collect(); - touched_paths.sort(); - touched_paths.dedup(); - - if touched_paths.is_empty() { - disputes.push(Dispute::empty_change()); - } - - let verdict = if cloaked { - Verdict::Cloaked - } else if visibility_policy.embargo_until.is_some() { - Verdict::Embargoed - } else { - meaning_policy.evaluate(&disputes) - }; - - let intent = options.intent.clone().unwrap_or_else(|| { - if touched_paths.is_empty() { - "no files changed".to_string() - } else { - touched_paths.join(", ") - } - }); + let (disputes, intent, verdict) = finalize_adjudication( + disputes, + &base_snapshot.files, + &head_snapshot.files, + options.intent.clone(), + cloaked, + visibility_policy.embargo_until.is_some(), + meaning_policy, + ); let docket = Docket { change: change.name, diff --git a/src/dispute.rs b/src/dispute.rs index 1cf2ad2..9fb267d 100644 --- a/src/dispute.rs +++ b/src/dispute.rs @@ -57,8 +57,9 @@ pub struct Dispute { impl Dispute { /// Low-severity notice that a change contains no file differences. /// - /// Does not affect blocking or review thresholds; it only makes the - /// empty change visible on the docket instead of passing silently. + /// Purely informational: [`finalize_adjudication`] evaluates the verdict + /// before appending this notice, so it never reaches blocking or review + /// thresholds regardless of the meaning policy. pub fn empty_change() -> Dispute { Dispute { id: "D000".into(), @@ -70,6 +71,54 @@ impl Dispute { } } +/// Shared adjudication tail used by every entry point (dir snapshots, git, jj). +/// +/// Computes touched paths, evaluates the verdict, appends the empty-change +/// notice, and resolves the docket intent. The verdict is evaluated *before* +/// the notice is appended so the notice cannot influence thresholds. +/// +/// Returns `(disputes, intent, verdict)`. +pub fn finalize_adjudication( + mut disputes: Vec, + base: &std::collections::HashMap, + head: &std::collections::HashMap, + user_intent: Option, + cloaked: bool, + embargo_active: bool, + meaning_policy: &crate::policy::MeaningPolicy, +) -> (Vec, String, Verdict) { + let mut touched_paths: Vec = base + .keys() + .chain(head.keys()) + .filter(|p| base.get(*p) != head.get(*p)) + .cloned() + .collect(); + touched_paths.sort(); + touched_paths.dedup(); + + let verdict = if cloaked { + Verdict::Cloaked + } else if embargo_active { + Verdict::Embargoed + } else { + meaning_policy.evaluate(&disputes) + }; + + if touched_paths.is_empty() { + disputes.push(Dispute::empty_change()); + } + + let intent = user_intent.unwrap_or_else(|| { + if touched_paths.is_empty() { + "no files changed".to_string() + } else { + touched_paths.join(", ") + } + }); + + (disputes, intent, verdict) +} + /// The final adjudication verdict for a change. #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "lowercase")] diff --git a/src/main.rs b/src/main.rs index 0eb5bea..c31d8a2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,7 +5,7 @@ use clap::{Parser, Subcommand}; use oot::adapter::{GitAdapter, GitAdjudicateOptions, JjAdapter, JjAdjudicateOptions}; use oot::change::{Change, Snapshot, Source}; -use oot::dispute::{Dispute, Docket, Kind, Severity, Verdict}; +use oot::dispute::{finalize_adjudication, Docket, Kind, Severity, Verdict}; use oot::docket; use oot::engine::Engine; use oot::policy::MeaningPolicy; @@ -202,36 +202,15 @@ fn main() -> anyhow::Result { let mut disputes = eng.diff_snapshots(&change.base, &change.head)?; disputes.extend(vis_disputes); - let mut touched_paths: Vec = change - .base - .files - .keys() - .chain(change.head.files.keys()) - .filter(|p| change.base.files.get(*p) != change.head.files.get(*p)) - .cloned() - .collect(); - touched_paths.sort(); - touched_paths.dedup(); - - if touched_paths.is_empty() { - disputes.push(Dispute::empty_change()); - } - - let verdict = if cloaked { - Verdict::Cloaked - } else if visibility_policy.embargo_until.is_some() { - Verdict::Embargoed - } else { - meaning_policy.evaluate(&disputes) - }; - - let intent = change.intent.clone().unwrap_or_else(|| { - if touched_paths.is_empty() { - "no files changed".into() - } else { - touched_paths.join(", ") - } - }); + let (disputes, intent, verdict) = finalize_adjudication( + disputes, + &change.base.files, + &change.head.files, + intent.clone(), + cloaked, + visibility_policy.embargo_until.is_some(), + &meaning_policy, + ); let docket = Docket { change: change.name.clone(), diff --git a/tests/cli_test.rs b/tests/cli_test.rs index 41c5016..d1ddf6b 100644 --- a/tests/cli_test.rs +++ b/tests/cli_test.rs @@ -221,6 +221,147 @@ fn test_cli_exit_code_zero_for_adjudicated() { let _ = std::fs::remove_dir_all(&temp_root); } +#[test] +fn test_cli_binary_file_does_not_crash() { + let bin = get_bin_path(); + let temp_root = std::env::temp_dir().join(format!("oot_cli_bin_{}", std::process::id())); + let base_dir = temp_root.join("base"); + let head_dir = temp_root.join("head"); + + std::fs::create_dir_all(&base_dir).unwrap(); + std::fs::create_dir_all(&head_dir).unwrap(); + + std::fs::write(base_dir.join("lib.rs"), "fn ok() {}").unwrap(); + std::fs::write(head_dir.join("lib.rs"), "fn ok() {}").unwrap(); + // Non-UTF8 blob: pre-fix, this failed the whole run with a UTF-8 error. + std::fs::write(head_dir.join("blob.bin"), [0xFFu8, 0xFE, 0x00, 0xD8]).unwrap(); + + let output = Command::new(&bin) + .args([ + "adjudicate", + "--change", + "assets/drop-binary", + "--source", + "git", + "--base", + base_dir.to_str().unwrap(), + "--head", + head_dir.to_str().unwrap(), + "--authors", + "@tester", + ]) + .output() + .expect("Failed to execute oot CLI"); + + assert_eq!( + output.status.code(), + Some(0), + "binary files must be tracked without failing the run" + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("OOT DOCKET")); + assert!(stdout.contains("ADJUDICATED")); + assert!(stdout.contains("blob.bin")); + + let _ = std::fs::remove_dir_all(&temp_root); +} + +#[test] +fn test_cli_empty_change_is_flagged_but_not_blocked() { + let bin = get_bin_path(); + let temp_root = std::env::temp_dir().join(format!("oot_cli_empty_{}", std::process::id())); + let base_dir = temp_root.join("base"); + let head_dir = temp_root.join("head"); + let policy_path = temp_root.join("low_blocks.toml"); + + std::fs::create_dir_all(&base_dir).unwrap(); + std::fs::create_dir_all(&head_dir).unwrap(); + + // Identical snapshots: nothing changed. + std::fs::write(base_dir.join("lib.rs"), "fn ok() {}").unwrap(); + std::fs::write(head_dir.join("lib.rs"), "fn ok() {}").unwrap(); + + // Even a policy that blocks on low severity must not block an empty + // change: the D000 notice never reaches MeaningPolicy evaluation. + std::fs::write(&policy_path, "block_on = [\"low\"]\nreview_on = [\"low\"]").unwrap(); + + let output = Command::new(&bin) + .args([ + "adjudicate", + "--change", + "chore/noop", + "--source", + "git", + "--base", + base_dir.to_str().unwrap(), + "--head", + head_dir.to_str().unwrap(), + "--policy", + policy_path.to_str().unwrap(), + "--authors", + "@tester", + ]) + .output() + .expect("Failed to execute oot CLI"); + + assert_eq!(output.status.code(), Some(0)); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("ADJUDICATED")); + assert!(stdout.contains("no file differences between base and head")); + assert!(stdout.contains("intent: no files changed")); + + let _ = std::fs::remove_dir_all(&temp_root); +} + +#[test] +fn test_cli_intent_wins_over_touched_paths() { + let bin = get_bin_path(); + let temp_root = std::env::temp_dir().join(format!("oot_cli_intent_{}", std::process::id())); + let base_dir = temp_root.join("base"); + let head_dir = temp_root.join("head"); + + std::fs::create_dir_all(&base_dir).unwrap(); + std::fs::create_dir_all(&head_dir).unwrap(); + + std::fs::write(base_dir.join("lib.rs"), "fn ok() {}").unwrap(); + std::fs::write(head_dir.join("lib.rs"), "fn ok(\"now\") {}").unwrap(); + + let run = |extra: &[&str]| { + let mut args = vec![ + "adjudicate".to_string(), + "--change".into(), + "feat/intent".into(), + "--source".into(), + "git".into(), + "--base".into(), + base_dir.to_string_lossy().into_owned(), + "--head".into(), + head_dir.to_string_lossy().into_owned(), + "--authors".into(), + "@tester".into(), + ]; + args.extend(extra.iter().map(|s| s.to_string())); + Command::new(&bin) + .args(&args) + .output() + .expect("Failed to execute oot CLI") + }; + + // User intent wins over the touched-path fallback. + let with_intent = run(&["--intent", "clarify ok"]); + assert_eq!(with_intent.status.code(), Some(0)); + let stdout = String::from_utf8_lossy(&with_intent.stdout); + assert!(stdout.contains("intent: clarify ok")); + + // Without --intent, the docket falls back to touched paths. + let without_intent = run(&[]); + assert_eq!(without_intent.status.code(), Some(0)); + let stdout = String::from_utf8_lossy(&without_intent.stdout); + assert!(stdout.contains("intent: lib.rs")); + + let _ = std::fs::remove_dir_all(&temp_root); +} + #[test] fn test_cli_repo_visibility_policy_flags_env() { let bin = get_bin_path(); diff --git a/tests/dispute_docket_test.rs b/tests/dispute_docket_test.rs index 5b5aafb..749c3c1 100644 --- a/tests/dispute_docket_test.rs +++ b/tests/dispute_docket_test.rs @@ -182,3 +182,22 @@ fn test_docket_render_verdicts() { assert!(rendered_adjudicated.contains("verdict: ▶ ADJUDICATED \n")); assert!(rendered_adjudicated.contains("dispute: none")); } + +#[test] +fn test_docket_loads_legacy_scope_field() { + // Old saved dockets carry "scope"; the serde alias must keep them loadable. + let legacy_json = r#"{ + "change": "feature/legacy", + "source": "git", + "base": "main", + "head": "feature/legacy", + "disputes": [], + "scope": "pre-rename intent", + "authors": ["@old"], + "verdict": "adjudicated", + "embargo": null + }"#; + + let docket: Docket = serde_json::from_str(legacy_json).expect("legacy docket must load"); + assert_eq!(docket.intent, "pre-rename intent"); +} From b9154be25330bc55077520d8d5afa8b6df97c3c4 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Sat, 22 Aug 2026 12:43:59 +0530 Subject: [PATCH 3/6] Stored snapshot contents as raw bytes so distinct binaries never compare equal --- TODO.md | 27 +++++-------- src/adapter/git.rs | 3 +- src/adapter/jj.rs | 33 +++++++++++++-- src/change.rs | 8 ++-- src/dispute.rs | 4 +- src/engine/mod.rs | 47 +++++++++++++--------- src/main.rs | 11 +++-- tests/cli_test.rs | 43 ++++++++++++++++++++ tests/engine_test.rs | 84 +++++++++++++++++++-------------------- tests/git_adapter_test.rs | 2 +- tests/jj_adapter_test.rs | 9 +++-- 11 files changed, 170 insertions(+), 101 deletions(-) diff --git a/TODO.md b/TODO.md index 9f647cf..de29c07 100644 --- a/TODO.md +++ b/TODO.md @@ -8,22 +8,13 @@ every change. Fixed by aligning the code with its own documented contract: only paths *touched* by a change (added, removed, or content-modified vs base) are checked. See `test_visibility_policy_only_flags_touched_private_paths`. -## Binary change detection is lossy - -Snapshots store file contents as `String`, so non-UTF8 files go through -`String::from_utf8_lossy`, which collapses every invalid byte sequence to -U+FFFD. Two *different* binaries can therefore compare equal and register as -unchanged — a governance docket could then say "no files changed" for a -change that did alter a binary. This affects both `load_dir` -(src/main.rs) and the git adapter's `extract_snapshot` (src/adapter/git.rs). - -**Trigger:** when binary artifacts (images, lockfiles, compiled blobs) -become first-class inputs to adjudication, or any real docket shows a -false "no files changed". - -**Right fix:** store raw bytes (or a content hash) in `Snapshot.files` and -convert to text only at tree-sitter parse time, so touched-path detection -compares bytes exactly. - -Pinned by the lossy-conversion note in `load_dir` until then. +## ~~Binary change detection is lossy~~ RESOLVED 2026-08-22 + +`Snapshot.files` now stores raw bytes (`HashMap>`). Change +detection compares bytes exactly, so two distinct binaries never compare +equal even when their lossy text collapses to the same U+FFFD sequence. +Text conversion happens only in the structural engine at parse time +(`as_text`, src/engine/mod.rs). All three ingestion paths (dir, git, jj) +store unconverted bytes. Pinned by +`test_cli_distinct_binaries_are_not_collapsed`. diff --git a/src/adapter/git.rs b/src/adapter/git.rs index 90f84a2..949dff9 100644 --- a/src/adapter/git.rs +++ b/src/adapter/git.rs @@ -158,8 +158,7 @@ impl GitAdapter { .with_context(|| format!("Failed to fetch blob {blob_sha} for {path}"))?; if blob_output.status.success() { - let content = String::from_utf8_lossy(&blob_output.stdout).into_owned(); - files.insert(path.to_string(), content); + files.insert(path.to_string(), blob_output.stdout); } } } diff --git a/src/adapter/jj.rs b/src/adapter/jj.rs index c4b5f78..1ae7341 100644 --- a/src/adapter/jj.rs +++ b/src/adapter/jj.rs @@ -89,8 +89,8 @@ impl JjAdapter { if !output.status.success() { return Err(anyhow!( - "jj {:?} failed: {}", - args, + "jj {} failed: {}", + args.join(" "), String::from_utf8_lossy(&output.stderr).trim() )); } @@ -98,6 +98,30 @@ impl JjAdapter { Ok(String::from_utf8_lossy(&output.stdout).into_owned()) } + /// Run a read-only jj command and return its raw stdout bytes. + /// + /// Used for file content so binary files keep exact bytes. + fn run_bytes(&self, args: &[&str]) -> Result> { + let mut full: Vec<&str> = vec!["--ignore-working-copy", "--no-pager", "--quiet"]; + full.extend_from_slice(args); + + let output = Command::new("jj") + .args(&full) + .current_dir(&self.repo_root) + .output() + .with_context(|| format!("Failed to run jj {:?}", args))?; + + if !output.status.success() { + return Err(anyhow!( + "jj {} failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr).trim() + )); + } + + Ok(output.stdout) + } + /// Resolve a revset to exactly one commit ID. /// /// Revset symbols resolve by priority (tag, then bookmark, then commit/change ID), @@ -204,8 +228,9 @@ impl JjAdapter { let mut conflicted = Vec::new(); for path in listing.lines().map(str::trim).filter(|l| !l.is_empty()) { - let content = self.run(&["file", "show", "-r", rev, "--", path])?; - if content + let content = self.run_bytes(&["file", "show", "-r", rev, "--", path])?; + let text = String::from_utf8_lossy(&content); + if text .lines() .any(|l| l.starts_with("<<<<<<<") && l.contains("conflict")) { diff --git a/src/change.rs b/src/change.rs index 6e86dfa..2869593 100644 --- a/src/change.rs +++ b/src/change.rs @@ -43,12 +43,14 @@ impl std::str::FromStr for Source { /// A snapshot is a mapping of relative file paths to their contents. /// +/// Contents are stored as raw bytes so binary files compare exactly; +/// text conversion happens only when the structural engine parses a file. /// Oot never assumes these files exist on a physical filesystem; /// they can be ingested from git, Jujutsu, or an agent's memory isolate. #[derive(Debug, Clone, Default)] pub struct Snapshot { - /// Map of file path (relative to repo root) to UTF-8 file content. - pub files: HashMap, + /// Map of file path (relative to repo root) to raw file content. + pub files: HashMap>, } /// A Change is the core unit Oot adjudicates: a content-addressed delta @@ -95,7 +97,7 @@ mod tests { fn test_change_and_snapshot_creation() { let mut snap = Snapshot::default(); snap.files - .insert("src/lib.rs".into(), "pub fn test() {}".into()); + .insert("src/lib.rs".into(), "pub fn test() {}".as_bytes().to_vec()); let change = Change { name: "test-change".into(), diff --git a/src/dispute.rs b/src/dispute.rs index 9fb267d..f5debf3 100644 --- a/src/dispute.rs +++ b/src/dispute.rs @@ -80,8 +80,8 @@ impl Dispute { /// Returns `(disputes, intent, verdict)`. pub fn finalize_adjudication( mut disputes: Vec, - base: &std::collections::HashMap, - head: &std::collections::HashMap, + base: &std::collections::HashMap>, + head: &std::collections::HashMap>, user_intent: Option, cloaked: bool, embargo_active: bool, diff --git a/src/engine/mod.rs b/src/engine/mod.rs index a79893d..0926a29 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -45,19 +45,19 @@ impl Engine { let Some(config) = self.config_for(path) else { continue; }; - let base_src = base.files.get(path); - let head_src = head.files.get(path); + let base_src = base.files.get(path).map(|v| as_text(v)); + let head_src = head.files.get(path).map(|v| as_text(v)); match (base_src, head_src) { (Some(b), Some(h)) => { let (base_fns, mut dupes) = extract_functions( - parse_source(&mut parser, &config.language, b).as_ref(), - b, + parse_source(&mut parser, &config.language, &b).as_ref(), + &b, config, ); let (head_fns, head_dupes) = extract_functions( - parse_source(&mut parser, &config.language, h).as_ref(), - h, + parse_source(&mut parser, &config.language, &h).as_ref(), + &h, config, ); dupes.extend(head_dupes); @@ -158,8 +158,8 @@ impl Engine { } (None, Some(h)) => { let summary = file_function_summary( - parse_source(&mut parser, &config.language, h).as_ref(), - h, + parse_source(&mut parser, &config.language, &h).as_ref(), + &h, config, ); disputes.push(meaning( @@ -201,26 +201,26 @@ impl Engine { let Some(config) = self.config_for(path) else { continue; }; - let b_file = base.files.get(path); - let o_file = ours.files.get(path); - let t_file = theirs.files.get(path); + let b_file = base.files.get(path).map(|v| as_text(v)); + let o_file = ours.files.get(path).map(|v| as_text(v)); + let t_file = theirs.files.get(path).map(|v| as_text(v)); match (b_file, o_file, t_file) { // File exists in all three (Some(b_src), Some(o_src), Some(t_src)) => { let (b_fns, mut dupes) = extract_functions( - parse_source(&mut parser, &config.language, b_src).as_ref(), - b_src, + parse_source(&mut parser, &config.language, &b_src).as_ref(), + &b_src, config, ); let (o_fns, o_dupes) = extract_functions( - parse_source(&mut parser, &config.language, o_src).as_ref(), - o_src, + parse_source(&mut parser, &config.language, &o_src).as_ref(), + &o_src, config, ); let (t_fns, t_dupes) = extract_functions( - parse_source(&mut parser, &config.language, t_src).as_ref(), - t_src, + parse_source(&mut parser, &config.language, &t_src).as_ref(), + &t_src, config, ); dupes.extend(o_dupes); @@ -412,8 +412,8 @@ impl Engine { // File added only in incoming (None, None, Some(t)) => { let summary = file_function_summary( - parse_source(&mut parser, &config.language, t).as_ref(), - t, + parse_source(&mut parser, &config.language, &t).as_ref(), + &t, config, ); disputes.push(meaning( @@ -445,6 +445,15 @@ fn parse_source(parser: &mut Parser, language: &Language, source: &str) -> Optio parser.parse(source, None) } +/// Lossily convert raw snapshot bytes to text for parsing. +/// +/// Only the structural engine touches this; change detection compares bytes, +/// so two distinct binary files never compare equal even if their lossy +/// text collapses. +fn as_text(bytes: &[u8]) -> std::borrow::Cow<'_, str> { + String::from_utf8_lossy(bytes) +} + fn meaning(n: &mut i32, path: &str, row: usize, detail: String, severity: Severity) -> Dispute { let id = format!("D{:03}", n); *n += 1; diff --git a/src/main.rs b/src/main.rs index c31d8a2..7da79c3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -248,11 +248,11 @@ fn exit_code_for(verdict: Verdict) -> std::process::ExitCode { } } -/// Recursively read files in a directory into a HashMap of relative paths to contents. +/// Recursively read files in a directory into a HashMap of relative paths to raw contents. fn load_dir( root: &std::path::Path, dir: &std::path::Path, - files: &mut std::collections::HashMap, + files: &mut std::collections::HashMap>, ) -> anyhow::Result<()> { for entry in std::fs::read_dir(dir)? { let entry = entry?; @@ -260,16 +260,15 @@ fn load_dir( if p.is_dir() { load_dir(root, &p, files)?; } else { - // Read as bytes and convert lossily so binary files (images, - // lockfiles, etc.) are tracked as changed without failing the run. + // Store raw bytes so binary files (images, lockfiles, etc.) are + // tracked with exact content; text conversion happens at parse time. let bytes = std::fs::read(&p)?; - let content = String::from_utf8_lossy(&bytes).into_owned(); let rel = p .strip_prefix(root) .unwrap_or(&p) .to_string_lossy() .replace('\\', "/"); - files.insert(rel, content); + files.insert(rel, bytes); } } Ok(()) diff --git a/tests/cli_test.rs b/tests/cli_test.rs index d1ddf6b..e89ed4b 100644 --- a/tests/cli_test.rs +++ b/tests/cli_test.rs @@ -362,6 +362,49 @@ fn test_cli_intent_wins_over_touched_paths() { let _ = std::fs::remove_dir_all(&temp_root); } +#[test] +fn test_cli_distinct_binaries_are_not_collapsed() { + let bin = get_bin_path(); + let temp_root = std::env::temp_dir().join(format!("oot_cli_bincmp_{}", std::process::id())); + let base_dir = temp_root.join("base"); + let head_dir = temp_root.join("head"); + + std::fs::create_dir_all(&base_dir).unwrap(); + std::fs::create_dir_all(&head_dir).unwrap(); + + // Two different invalid-UTF8 bytes both lossy-convert to U+FFFD, but raw + // byte comparison must still see them as different content. + std::fs::write(base_dir.join("blob.bin"), [0x80u8]).unwrap(); + std::fs::write(head_dir.join("blob.bin"), [0x81u8]).unwrap(); + + let output = Command::new(&bin) + .args([ + "adjudicate", + "--change", + "assets/touch-binary", + "--source", + "git", + "--base", + base_dir.to_str().unwrap(), + "--head", + head_dir.to_str().unwrap(), + "--authors", + "@tester", + ]) + .output() + .expect("Failed to execute oot CLI"); + + assert_eq!(output.status.code(), Some(0)); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + !stdout.contains("no file differences"), + "changed binary must register as touched, got:\n{stdout}" + ); + assert!(stdout.contains("intent: blob.bin")); + + let _ = std::fs::remove_dir_all(&temp_root); +} + #[test] fn test_cli_repo_visibility_policy_flags_env() { let bin = get_bin_path(); diff --git a/tests/engine_test.rs b/tests/engine_test.rs index 12c6e6f..6c86d89 100644 --- a/tests/engine_test.rs +++ b/tests/engine_test.rs @@ -14,7 +14,7 @@ fn authenticate(user: &str, pass: &str) -> bool { user == "admin" && pass == "secret" } "# - .to_string(), + .as_bytes().to_vec(), ); let mut head = Snapshot::default(); @@ -25,7 +25,7 @@ fn authenticate(user: &str, pass: &str) -> bool { user == "admin" && pass == "secure_password_v2" } "# - .to_string(), + .as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -55,7 +55,7 @@ fn legacy_multiply(a: i32, b: i32) -> i32 { a * b } "# - .to_string(), + .as_bytes().to_vec(), ); let mut head = Snapshot::default(); @@ -70,7 +70,7 @@ fn subtract(a: i32, b: i32) -> i32 { a - b } "# - .to_string(), + .as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -111,11 +111,11 @@ pub fn verify_signature() -> bool { let mut base = Snapshot::default(); base.files - .insert("src/crypto.rs".to_string(), source.to_string()); + .insert("src/crypto.rs".to_string(), source.as_bytes().to_vec()); let mut head = Snapshot::default(); head.files - .insert("src/crypto.rs".to_string(), source.to_string()); + .insert("src/crypto.rs".to_string(), source.as_bytes().to_vec()); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -133,36 +133,36 @@ fn test_engine_unsupported_extension_filtering() { let mut base = Snapshot::default(); base.files.insert( "README.md".to_string(), - "# Project\nInitial README".to_string(), + "# Project\nInitial README".as_bytes().to_vec(), ); base.files.insert( "config.toml".to_string(), - "title = 'Old Config'".to_string(), + "title = 'Old Config'".as_bytes().to_vec(), ); base.files.insert( "scripts/run.sh".to_string(), - "echo 'Running old script'".to_string(), + "echo 'Running old script'".as_bytes().to_vec(), ); base.files.insert( "style.css".to_string(), - "body { color: black; }".to_string(), + "body { color: black; }".as_bytes().to_vec(), ); let mut head = Snapshot::default(); head.files.insert( "README.md".to_string(), - "# Project\nUpdated README with more docs".to_string(), + "# Project\nUpdated README with more docs".as_bytes().to_vec(), ); head.files.insert( "config.toml".to_string(), - "title = 'New Config'".to_string(), + "title = 'New Config'".as_bytes().to_vec(), ); head.files.insert( "scripts/run.sh".to_string(), - "echo 'Running new script'".to_string(), + "echo 'Running new script'".as_bytes().to_vec(), ); head.files - .insert("style.css".to_string(), "body { color: blue; }".to_string()); + .insert("style.css".to_string(), "body { color: blue; }".as_bytes().to_vec()); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -179,14 +179,14 @@ fn test_engine_syntax_error_handling() { let mut base = Snapshot::default(); base.files.insert( "src/broken.rs".to_string(), - "fn valid_base() -> i32 { 42 }".to_string(), + "fn valid_base() -> i32 { 42 }".as_bytes().to_vec(), ); let mut head = Snapshot::default(); // Incomplete / invalid Rust syntax head.files.insert( "src/broken.rs".to_string(), - "fn broken_syntax( { !!! %%% invalid rust code @@@ }}}".to_string(), + "fn broken_syntax( { !!! %%% invalid rust code @@@ }}}".as_bytes().to_vec(), ); // Engine should handle syntax errors gracefully without panicking @@ -206,13 +206,13 @@ fn test_engine_file_added_and_removed() { let mut base = Snapshot::default(); base.files.insert( "src/old_module.rs".to_string(), - "fn old_util() {}".to_string(), + "fn old_util() {}".as_bytes().to_vec(), ); let mut head = Snapshot::default(); head.files.insert( "src/new_module.rs".to_string(), - "fn new_util() {}".to_string(), + "fn new_util() {}".as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -238,13 +238,13 @@ fn test_engine_rename_is_not_remove_add() { let mut base = Snapshot::default(); base.files.insert( "src/auth.rs".to_string(), - "fn verify_user(user: &str) -> bool { user.len() > 3 }".to_string(), + "fn verify_user(user: &str) -> bool { user.len() > 3 }".as_bytes().to_vec(), ); let mut head = Snapshot::default(); head.files.insert( "src/auth.rs".to_string(), - "fn check_user(user: &str) -> bool { user.len() > 3 }".to_string(), + "fn check_user(user: &str) -> bool { user.len() > 3 }".as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -271,7 +271,7 @@ fn test_engine_3way_rename_is_not_conflict() { let snap = |s: &str| { let mut x = Snapshot::default(); - x.files.insert("src/lib.rs".to_string(), s.to_string()); + x.files.insert("src/lib.rs".to_string(), s.as_bytes().to_vec()); x }; @@ -294,7 +294,7 @@ fn test_engine_added_file_summary_lists_functions() { let mut head = Snapshot::default(); head.files.insert( "src/newstuff.rs".to_string(), - "fn alpha() {}\nfn beta() {}\nfn gamma() {}\nfn delta() {}\n".to_string(), + "fn alpha() {}\nfn beta() {}\nfn gamma() {}\nfn delta() {}\n".as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -313,18 +313,18 @@ fn test_engine_multiple_files_and_functions() { let mut base = Snapshot::default(); base.files.insert( "src/a.rs".to_string(), - "fn fa1() {}\nfn fa2() {}\n".to_string(), + "fn fa1() {}\nfn fa2() {}\n".as_bytes().to_vec(), ); base.files - .insert("src/b.rs".to_string(), "fn fb1() {}\n".to_string()); + .insert("src/b.rs".to_string(), "fn fb1() {}\n".as_bytes().to_vec()); let mut head = Snapshot::default(); head.files.insert( "src/a.rs".to_string(), - "fn fa1() { println!(\"modified\"); }\nfn fa2() {}\nfn fa3() {}\n".to_string(), + "fn fa1() { println!(\"modified\"); }\nfn fa2() {}\nfn fa3() {}\n".as_bytes().to_vec(), ); head.files - .insert("src/b.rs".to_string(), "fn fb1() {}\n".to_string()); + .insert("src/b.rs".to_string(), "fn fb1() {}\n".as_bytes().to_vec()); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -354,7 +354,7 @@ func (s *Store) Name() string { return s.title } "# - .to_string(), + .as_bytes().to_vec(), ); let mut head = Snapshot::default(); @@ -371,7 +371,7 @@ func (s *Store) Name() string { return s.title } "# - .to_string(), + .as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -408,14 +408,14 @@ func (s *Store) Total() int { let mut base = Snapshot::default(); base.files - .insert("total.go".to_string(), base_src.to_string()); + .insert("total.go".to_string(), base_src.as_bytes().to_vec()); let mut ours = Snapshot::default(); ours.files - .insert("total.go".to_string(), ours_src.to_string()); + .insert("total.go".to_string(), ours_src.as_bytes().to_vec()); let mut theirs = Snapshot::default(); theirs .files - .insert("total.go".to_string(), theirs_src.to_string()); + .insert("total.go".to_string(), theirs_src.as_bytes().to_vec()); let disputes = engine .diff_3way(&base, &ours, &theirs) @@ -445,7 +445,7 @@ class Client { } } "# - .to_string(), + .as_bytes().to_vec(), ); let mut head = Snapshot::default(); @@ -462,7 +462,7 @@ class Client { } } "# - .to_string(), + .as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -488,7 +488,7 @@ const fetchUser = async (id) => { return { id }; }; "# - .to_string(), + .as_bytes().to_vec(), ); let mut head = Snapshot::default(); @@ -499,7 +499,7 @@ const fetchUser = async (id) => { return { id, cached: false }; }; "# - .to_string(), + .as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -533,14 +533,14 @@ export const formatPrice = (cents) => { let mut base = Snapshot::default(); base.files - .insert("src/price.mjs".to_string(), base_src.to_string()); + .insert("src/price.mjs".to_string(), base_src.as_bytes().to_vec()); let mut ours = Snapshot::default(); ours.files - .insert("src/price.mjs".to_string(), ours_src.to_string()); + .insert("src/price.mjs".to_string(), ours_src.as_bytes().to_vec()); let mut theirs = Snapshot::default(); theirs .files - .insert("src/price.mjs".to_string(), theirs_src.to_string()); + .insert("src/price.mjs".to_string(), theirs_src.as_bytes().to_vec()); // Covers both the 3-way path for JavaScript and .mjs extension routing. let disputes = engine @@ -560,12 +560,12 @@ fn test_engine_dotless_filename_is_not_source() { // A file literally named `go` with no extension must not be parsed as Go. let mut base = Snapshot::default(); base.files - .insert("tools/go".to_string(), "func NotReally() {}".to_string()); + .insert("tools/go".to_string(), "func NotReally() {}".as_bytes().to_vec()); let mut head = Snapshot::default(); head.files.insert( "tools/go".to_string(), - "func DefinitelyChanged() {}".to_string(), + "func DefinitelyChanged() {}".as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -595,7 +595,7 @@ func (b B) Name() string { return "B" } "# - .to_string(), + .as_bytes().to_vec(), ); let mut head = Snapshot::default(); @@ -612,7 +612,7 @@ func (b B) Name() string { return "B" } "# - .to_string(), + .as_bytes().to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); diff --git a/tests/git_adapter_test.rs b/tests/git_adapter_test.rs index e72bf10..d22b879 100644 --- a/tests/git_adapter_test.rs +++ b/tests/git_adapter_test.rs @@ -116,7 +116,7 @@ fn test_git_adapter_snapshot_extraction() { assert!(snapshot.files.contains_key("README.md")); assert_eq!( snapshot.files.get("src/lib.rs").unwrap(), - "pub fn compute() -> i32 { 42 }\n\npub fn auth() -> bool { true }\n" + "pub fn compute() -> i32 { 42 }\n\npub fn auth() -> bool { true }\n".as_bytes() ); } diff --git a/tests/jj_adapter_test.rs b/tests/jj_adapter_test.rs index 9e89136..cff71c3 100644 --- a/tests/jj_adapter_test.rs +++ b/tests/jj_adapter_test.rs @@ -150,10 +150,11 @@ fn test_jj_extract_snapshot() { let (_repo, adapter) = setup_base_and_head(false); let snap = adapter.extract_snapshot("@-").expect("snapshot extracts"); - let content = snap - .files - .get("src/lib.rs") - .expect("src/lib.rs present in head snapshot"); + let content = String::from_utf8_lossy( + snap.files + .get("src/lib.rs") + .expect("src/lib.rs present in head snapshot"), + ); assert!(content.contains("hello world")); assert!(!content.contains("<<<<<<<"), "clean commit has no markers"); } From 2f3e8d5c08733014828a31348662ab313430e2a6 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Sat, 22 Aug 2026 12:44:40 +0530 Subject: [PATCH 4/6] Formatted test fixtures --- tests/engine_test.rs | 75 ++++++++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 23 deletions(-) diff --git a/tests/engine_test.rs b/tests/engine_test.rs index 6c86d89..e0a3c36 100644 --- a/tests/engine_test.rs +++ b/tests/engine_test.rs @@ -14,7 +14,8 @@ fn authenticate(user: &str, pass: &str) -> bool { user == "admin" && pass == "secret" } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let mut head = Snapshot::default(); @@ -25,7 +26,8 @@ fn authenticate(user: &str, pass: &str) -> bool { user == "admin" && pass == "secure_password_v2" } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -55,7 +57,8 @@ fn legacy_multiply(a: i32, b: i32) -> i32 { a * b } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let mut head = Snapshot::default(); @@ -70,7 +73,8 @@ fn subtract(a: i32, b: i32) -> i32 { a - b } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -151,7 +155,9 @@ fn test_engine_unsupported_extension_filtering() { let mut head = Snapshot::default(); head.files.insert( "README.md".to_string(), - "# Project\nUpdated README with more docs".as_bytes().to_vec(), + "# Project\nUpdated README with more docs" + .as_bytes() + .to_vec(), ); head.files.insert( "config.toml".to_string(), @@ -161,8 +167,10 @@ fn test_engine_unsupported_extension_filtering() { "scripts/run.sh".to_string(), "echo 'Running new script'".as_bytes().to_vec(), ); - head.files - .insert("style.css".to_string(), "body { color: blue; }".as_bytes().to_vec()); + head.files.insert( + "style.css".to_string(), + "body { color: blue; }".as_bytes().to_vec(), + ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -186,7 +194,9 @@ fn test_engine_syntax_error_handling() { // Incomplete / invalid Rust syntax head.files.insert( "src/broken.rs".to_string(), - "fn broken_syntax( { !!! %%% invalid rust code @@@ }}}".as_bytes().to_vec(), + "fn broken_syntax( { !!! %%% invalid rust code @@@ }}}" + .as_bytes() + .to_vec(), ); // Engine should handle syntax errors gracefully without panicking @@ -238,13 +248,17 @@ fn test_engine_rename_is_not_remove_add() { let mut base = Snapshot::default(); base.files.insert( "src/auth.rs".to_string(), - "fn verify_user(user: &str) -> bool { user.len() > 3 }".as_bytes().to_vec(), + "fn verify_user(user: &str) -> bool { user.len() > 3 }" + .as_bytes() + .to_vec(), ); let mut head = Snapshot::default(); head.files.insert( "src/auth.rs".to_string(), - "fn check_user(user: &str) -> bool { user.len() > 3 }".as_bytes().to_vec(), + "fn check_user(user: &str) -> bool { user.len() > 3 }" + .as_bytes() + .to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -271,7 +285,8 @@ fn test_engine_3way_rename_is_not_conflict() { let snap = |s: &str| { let mut x = Snapshot::default(); - x.files.insert("src/lib.rs".to_string(), s.as_bytes().to_vec()); + x.files + .insert("src/lib.rs".to_string(), s.as_bytes().to_vec()); x }; @@ -294,7 +309,9 @@ fn test_engine_added_file_summary_lists_functions() { let mut head = Snapshot::default(); head.files.insert( "src/newstuff.rs".to_string(), - "fn alpha() {}\nfn beta() {}\nfn gamma() {}\nfn delta() {}\n".as_bytes().to_vec(), + "fn alpha() {}\nfn beta() {}\nfn gamma() {}\nfn delta() {}\n" + .as_bytes() + .to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -321,7 +338,9 @@ fn test_engine_multiple_files_and_functions() { let mut head = Snapshot::default(); head.files.insert( "src/a.rs".to_string(), - "fn fa1() { println!(\"modified\"); }\nfn fa2() {}\nfn fa3() {}\n".as_bytes().to_vec(), + "fn fa1() { println!(\"modified\"); }\nfn fa2() {}\nfn fa3() {}\n" + .as_bytes() + .to_vec(), ); head.files .insert("src/b.rs".to_string(), "fn fb1() {}\n".as_bytes().to_vec()); @@ -354,7 +373,8 @@ func (s *Store) Name() string { return s.title } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let mut head = Snapshot::default(); @@ -371,7 +391,8 @@ func (s *Store) Name() string { return s.title } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -445,7 +466,8 @@ class Client { } } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let mut head = Snapshot::default(); @@ -462,7 +484,8 @@ class Client { } } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -488,7 +511,8 @@ const fetchUser = async (id) => { return { id }; }; "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let mut head = Snapshot::default(); @@ -499,7 +523,8 @@ const fetchUser = async (id) => { return { id, cached: false }; }; "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); @@ -559,8 +584,10 @@ fn test_engine_dotless_filename_is_not_source() { // A file literally named `go` with no extension must not be parsed as Go. let mut base = Snapshot::default(); - base.files - .insert("tools/go".to_string(), "func NotReally() {}".as_bytes().to_vec()); + base.files.insert( + "tools/go".to_string(), + "func NotReally() {}".as_bytes().to_vec(), + ); let mut head = Snapshot::default(); head.files.insert( @@ -595,7 +622,8 @@ func (b B) Name() string { return "B" } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let mut head = Snapshot::default(); @@ -612,7 +640,8 @@ func (b B) Name() string { return "B" } "# - .as_bytes().to_vec(), + .as_bytes() + .to_vec(), ); let disputes = engine.diff_snapshots(&base, &head).expect("Diff failed"); From d91a0a6994b9952eb623a236faf3f68e8b708116 Mon Sep 17 00:00:00 2001 From: kridaydave Date: Sat, 22 Aug 2026 13:13:58 +0530 Subject: [PATCH 5/6] Hardened from final review: jj runner dedup, symlink skip, D000 structural exclusion, loud blob errors --- src/adapter/git.rs | 8 +++++-- src/adapter/jj.rs | 19 +--------------- src/dispute.rs | 18 +++++++++++---- src/main.rs | 5 ++++ src/policy.rs | 6 +++-- tests/cli_test.rs | 48 ++++++++++++++++++++++++++++++++++++++- tests/git_adapter_test.rs | 10 +++++++- tests/jj_adapter_test.rs | 22 ++++++++++++++++++ 8 files changed, 108 insertions(+), 28 deletions(-) diff --git a/src/adapter/git.rs b/src/adapter/git.rs index 949dff9..f8a6550 100644 --- a/src/adapter/git.rs +++ b/src/adapter/git.rs @@ -157,9 +157,13 @@ impl GitAdapter { .output() .with_context(|| format!("Failed to fetch blob {blob_sha} for {path}"))?; - if blob_output.status.success() { - files.insert(path.to_string(), blob_output.stdout); + if !blob_output.status.success() { + return Err(anyhow!( + "Failed to read blob {blob_sha} for {path}: {}", + String::from_utf8_lossy(&blob_output.stderr).trim() + )); } + files.insert(path.to_string(), blob_output.stdout); } } } diff --git a/src/adapter/jj.rs b/src/adapter/jj.rs index 1ae7341..88ad42f 100644 --- a/src/adapter/jj.rs +++ b/src/adapter/jj.rs @@ -78,24 +78,7 @@ impl JjAdapter { /// Run a read-only jj command and return its stdout. fn run(&self, args: &[&str]) -> Result { - let mut full: Vec<&str> = vec!["--ignore-working-copy", "--no-pager", "--quiet"]; - full.extend_from_slice(args); - - let output = Command::new("jj") - .args(&full) - .current_dir(&self.repo_root) - .output() - .with_context(|| format!("Failed to run jj {:?}", args))?; - - if !output.status.success() { - return Err(anyhow!( - "jj {} failed: {}", - args.join(" "), - String::from_utf8_lossy(&output.stderr).trim() - )); - } - - Ok(String::from_utf8_lossy(&output.stdout).into_owned()) + Ok(String::from_utf8_lossy(&self.run_bytes(args)?).into_owned()) } /// Run a read-only jj command and return its raw stdout bytes. diff --git a/src/dispute.rs b/src/dispute.rs index f5debf3..2e7b1e8 100644 --- a/src/dispute.rs +++ b/src/dispute.rs @@ -54,15 +54,22 @@ pub struct Dispute { pub detail: String, } +/// Sentinel id for the empty-change notice. +/// +/// The notice is informational only; policy evaluation and meaning counts +/// skip it by id so a persisted docket can never re-block on it. +pub const EMPTY_CHANGE_ID: &str = "D000"; + impl Dispute { /// Low-severity notice that a change contains no file differences. /// /// Purely informational: [`finalize_adjudication`] evaluates the verdict - /// before appending this notice, so it never reaches blocking or review - /// thresholds regardless of the meaning policy. + /// before appending this notice, and [`MeaningPolicy::evaluate`] plus + /// [`Docket::meaning_count`] skip the sentinel, so it never reaches + /// blocking or review thresholds regardless of the meaning policy. pub fn empty_change() -> Dispute { Dispute { - id: "D000".into(), + id: EMPTY_CHANGE_ID.into(), location: "-".into(), kind: Kind::Meaning, severity: Severity::Low, @@ -159,10 +166,13 @@ pub struct Docket { impl Docket { /// Return the number of meaning-related disputes. + /// + /// Excludes the empty-change notice, which is informational rather than + /// a detected dispute. pub fn meaning_count(&self) -> usize { self.disputes .iter() - .filter(|d| d.kind == Kind::Meaning) + .filter(|d| d.kind == Kind::Meaning && d.id != EMPTY_CHANGE_ID) .count() } diff --git a/src/main.rs b/src/main.rs index 7da79c3..4112270 100644 --- a/src/main.rs +++ b/src/main.rs @@ -257,6 +257,11 @@ fn load_dir( for entry in std::fs::read_dir(dir)? { let entry = entry?; let p = entry.path(); + // Never follow symlinks: a loop would recurse forever and an + // escaping link would ingest content from outside the snapshot. + if entry.file_type()?.is_symlink() { + continue; + } if p.is_dir() { load_dir(root, &p, files)?; } else { diff --git a/src/policy.rs b/src/policy.rs index 26e7965..16ff9de 100644 --- a/src/policy.rs +++ b/src/policy.rs @@ -34,10 +34,12 @@ impl MeaningPolicy { /// Evaluate only meaning disputes against blocking thresholds. /// - /// Visibility disputes are judged elsewhere (e.g. via [`crate::visibility::VisibilityPolicy`]). + /// The empty-change notice (`EMPTY_CHANGE_ID`) never blocks: it is + /// informational and excluded here so a saved docket re-evaluated under + /// any policy behaves the same as at adjudication time. pub fn evaluate(&self, disputes: &[Dispute]) -> Verdict { for d in disputes { - if d.kind != Kind::Meaning { + if d.kind != Kind::Meaning || d.id == crate::dispute::EMPTY_CHANGE_ID { continue; } let s = d.severity.as_str(); diff --git a/tests/cli_test.rs b/tests/cli_test.rs index e89ed4b..738766b 100644 --- a/tests/cli_test.rs +++ b/tests/cli_test.rs @@ -307,7 +307,10 @@ fn test_cli_empty_change_is_flagged_but_not_blocked() { assert_eq!(output.status.code(), Some(0)); let stdout = String::from_utf8_lossy(&output.stdout); assert!(stdout.contains("ADJUDICATED")); - assert!(stdout.contains("no file differences between base and head")); + // The notice is informational: it renders as a dispute line but does not + // inflate the detected-disputes count and cannot block under any policy. + assert!(stdout.contains("meaning: 0 disputes detected")); + assert!(stdout.contains("dispute-01: no file differences between base and head")); assert!(stdout.contains("intent: no files changed")); let _ = std::fs::remove_dir_all(&temp_root); @@ -405,6 +408,49 @@ fn test_cli_distinct_binaries_are_not_collapsed() { let _ = std::fs::remove_dir_all(&temp_root); } +#[test] +fn test_cli_symlink_loop_is_skipped() { + let bin = get_bin_path(); + let temp_root = std::env::temp_dir().join(format!("oot_cli_link_{}", std::process::id())); + let base_dir = temp_root.join("base"); + let head_dir = temp_root.join("head"); + + std::fs::create_dir_all(&base_dir).unwrap(); + std::fs::create_dir_all(&head_dir).unwrap(); + + std::fs::write(base_dir.join("lib.rs"), "fn ok() {}").unwrap(); + std::fs::write(head_dir.join("lib.rs"), "fn ok() {}").unwrap(); + + // A symlink pointing at the snapshot root itself: following it would + // recurse forever. Symlinks must be skipped, not followed. + #[cfg(unix)] + std::os::unix::fs::symlink(&base_dir, base_dir.join("loop")).unwrap(); + + let output = Command::new(&bin) + .args([ + "adjudicate", + "--change", + "chore/link", + "--source", + "git", + "--base", + base_dir.to_str().unwrap(), + "--head", + head_dir.to_str().unwrap(), + "--authors", + "@tester", + ]) + .output() + .expect("Failed to execute oot CLI"); + + assert_eq!(output.status.code(), Some(0)); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("ADJUDICATED")); + assert!(stdout.contains("intent: no files changed")); + + let _ = std::fs::remove_dir_all(&temp_root); +} + #[test] fn test_cli_repo_visibility_policy_flags_env() { let bin = get_bin_path(); diff --git a/tests/git_adapter_test.rs b/tests/git_adapter_test.rs index d22b879..4cc29d1 100644 --- a/tests/git_adapter_test.rs +++ b/tests/git_adapter_test.rs @@ -43,6 +43,10 @@ impl TempGitRepo { } fn write_file(&self, rel_path: &str, content: &str) { + self.write_bytes(rel_path, content.as_bytes()); + } + + fn write_bytes(&self, rel_path: &str, content: &[u8]) { let full = self.path.join(rel_path); if let Some(parent) = full.parent() { fs::create_dir_all(parent).expect("failed to create parent dir"); @@ -106,18 +110,22 @@ fn test_git_adapter_snapshot_extraction() { "pub fn compute() -> i32 { 42 }\n\npub fn auth() -> bool { true }\n", ); repo.write_file("README.md", "# Test Repo\n"); + // Invalid UTF-8: pins byte-exact blob storage (lossy conversion would + // turn 0xFF into U+FFFD and this assertion would fail). + repo.write_bytes("assets/blob.bin", &[0xFF, 0x00, 0x81]); let c1 = repo.commit("initial commit"); let adapter = GitAdapter::new(&repo.path).expect("valid git repo"); let snapshot = adapter.extract_snapshot(&c1).expect("extract snapshot"); - assert_eq!(snapshot.files.len(), 2); + assert_eq!(snapshot.files.len(), 3); assert!(snapshot.files.contains_key("src/lib.rs")); assert!(snapshot.files.contains_key("README.md")); assert_eq!( snapshot.files.get("src/lib.rs").unwrap(), "pub fn compute() -> i32 { 42 }\n\npub fn auth() -> bool { true }\n".as_bytes() ); + assert_eq!(snapshot.files.get("assets/blob.bin").unwrap(), &[0xFF, 0x00, 0x81]); } #[test] diff --git a/tests/jj_adapter_test.rs b/tests/jj_adapter_test.rs index cff71c3..8cfdb1e 100644 --- a/tests/jj_adapter_test.rs +++ b/tests/jj_adapter_test.rs @@ -159,6 +159,28 @@ fn test_jj_extract_snapshot() { assert!(!content.contains("<<<<<<<"), "clean commit has no markers"); } +#[test] +fn test_jj_extract_snapshot_binary_exact_bytes() { + if !jj_available() { + return; + } + let repo = init_repo(false); + write_lib(&repo.path, "hello"); + jj(&repo.path, &["commit", "-m", "base"]); + + // Invalid UTF-8: pins byte-exact file storage through `jj file show` + // (lossy conversion would turn 0xFF into U+FFFD and fail this assert). + std::fs::write(repo.path.join("assets.bin"), [0xFFu8, 0x00, 0x81]).unwrap(); + jj(&repo.path, &["commit", "-m", "add binary"]); + + let adapter = JjAdapter::new(&repo.path).expect("adapter should discover jj repo"); + let snap = adapter.extract_snapshot("@").expect("snapshot extracts"); + assert_eq!( + snap.files.get("assets.bin").expect("binary present"), + &[0xFF, 0x00, 0x81] + ); +} + #[test] fn test_jj_adjudicate_3way_clean_unilateral_change() { if !jj_available() { From 108e10c94ce591d132de4a3e5cff5dd85b7afc1b Mon Sep 17 00:00:00 2001 From: kridaydave Date: Sat, 22 Aug 2026 13:14:12 +0530 Subject: [PATCH 6/6] Formatted --- tests/git_adapter_test.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/git_adapter_test.rs b/tests/git_adapter_test.rs index 4cc29d1..a5b1f24 100644 --- a/tests/git_adapter_test.rs +++ b/tests/git_adapter_test.rs @@ -125,7 +125,10 @@ fn test_git_adapter_snapshot_extraction() { snapshot.files.get("src/lib.rs").unwrap(), "pub fn compute() -> i32 { 42 }\n\npub fn auth() -> bool { true }\n".as_bytes() ); - assert_eq!(snapshot.files.get("assets/blob.bin").unwrap(), &[0xFF, 0x00, 0x81]); + assert_eq!( + snapshot.files.get("assets/blob.bin").unwrap(), + &[0xFF, 0x00, 0x81] + ); } #[test]