From 68916c5ad4813b2c4e4e70a3a7b6ebb249f5b8de Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 16 Oct 2026 21:03:19 +0900 Subject: [PATCH 1/3] Expose versioned reconciliation JSON to reporting consumers Evaluate claims once for the human and JSON renderers, preserving the existing human output and exit precedence while retaining partial evidence in the machine report. Review and OSCAL validate the schema and process status before using explicit supplying components. Pre-evaluation failures remain exit 2 with stderr diagnostics and no report. Remove reconciled_count and its prose tokens now that consumers read JSON. Validation: 51 check CLI tests, 141 Python tests, all-targets clippy, Ruff, and unchanged default check/review output against the main baseline. Remove drifted, test_a_drifted_report_publishes_no_component_definition, and test_a_report_this_reader_cannot_parse_is_could_not_look from the old human-report parser tests. Machine-contract tests now cover malformed data, unsupported versions, partial reports and explicit component identities. --- docs/DESIGN.md | 8 + docs/REFERENCE.md | 46 ++++++ src/check.rs | 330 +++++++++++++++++++++++++++---------- src/main.rs | 13 +- tests/check_cli.rs | 182 ++++++++++++++++++++ tests/test_uphold_check.py | 252 +++++++++++++++++++++++----- uphold_check.py | 207 +++++++++++++---------- 7 files changed, 814 insertions(+), 224 deletions(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 1e6aa7f..0896a98 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -169,6 +169,14 @@ component, here are the controls it implements. Its ecosystem Compliance-to-Policy) already turns that into policy for several engines and normalizes their results back, which a private format would forgo. +Rust evaluates reconciliation once into a report; the default human renderer +and the versioned `uphold check --json` renderer consume that same result. +Python review and OSCAL read the JSON contract, including explicit component +identities, rather than interpreting a human evidence line. A malformed report, +unknown schema version, unavailable evidence, or unexpected process status is +could-not-look. Review can still use the partial evidence from an evaluated +refusal; an export requires the whole reconciliation to hold. + The export reconciles first and emits only what held: a component definition is an assertion to an outside reader, and exporting an unreconciled one would publish a claim this tool had just failed to confirm. diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 4acc372..865eec4 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -3780,6 +3780,52 @@ PATH. Hook trust in Codex and user-level settings are outside this declaration check. A settings file that cannot be parsed in its JSON or TOML format is exit `2`, including when another settings file declares a valid hook. +`uphold check --json` renders the same evaluated reconciliation as the default +human report. It emits one JSON object on stdout, including partial evidence +when the result is refused or unavailable: + +```json +{ + "schema_version": 1, + "verdict": "clean", + "claims": [ + { + "principle": "explicit-unknown", + "rule": "catalog-tests", + "status": "reconciled", + "suppliers": [{"component": "local", "seam": "a hook installed here"}] + } + ], + "violations": [], + "unavailable": [], + "notes": [] +} +``` + +`schema_version` is an integer. `verdict` is `clean`, `violations`, or +`could_not_look`, corresponding to process status `0`, `1`, or `2`. `claims` +contains every evaluated declaration entry, in declaration order. Its `status` +is `reconciled`, `refused`, or `unavailable`; only reconciled claims have +suppliers. Each supplier names its `component` (`local` or `uphold`) explicitly +and carries the supplying `seam` as a description. Consumers route by component +and treat seam descriptions, notes and diagnostics as text for a person. +`violations`, `unavailable` and `notes` are arrays of diagnostic strings. +Additional fields may be added within a schema version. + +A missing or invalid policy or declaration, discovery failure, or other error +before evaluation produces no JSON: stdout is empty, the diagnostic is on +stderr, and the process exits `2`. `--coverage` and `--json` are separate modes +and cannot be combined. Exit precedence remains the same as the human mode: +a claim whose missing rule may be in unread configuration exits `2`; otherwise +a known violation exits `1` even if other configuration is unreadable. The +`unavailable` array records unread evidence regardless of the exit code. + +The Python review and OSCAL consumers read this JSON contract. They reject +unknown schema versions, malformed or inconsistent reports, unexpected process +statuses, and any unavailable evidence. Review accepts an evaluated refusal +and retains the claims that reconciled. OSCAL requires a clean report. Neither +consumer parses the human report's wording. + `--coverage` counts the direction the reconcile cannot — a rule firing under no claim is invisible to a reconcile. It reports and does not refuse: `0`, or `2` where a tier's configuration could not be read, with a count of `?` rather than diff --git a/src/check.rs b/src/check.rs index aea8fdf..a152f3d 100644 --- a/src/check.rs +++ b/src/check.rs @@ -28,7 +28,7 @@ use std::collections::{BTreeMap, BTreeSet}; use std::path::Path; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use crate::catalog; use crate::config::Policy; @@ -711,8 +711,63 @@ pub(crate) struct Supply { pub unestablished: Vec, } -/// `uphold check`, and `uphold check --coverage`. -pub(crate) fn run(root: &Path, policy: &Policy, coverage: bool) -> Result { +/// The versioned machine contract, independent of the human report's wording. +#[derive(Debug, Serialize)] +pub(crate) struct Report { + schema_version: u8, + pub(crate) verdict: Verdict, + pub(crate) claims: Vec, + violations: Vec, + unavailable: Vec, + notes: Vec, + #[serde(skip)] + misplaced_count: usize, + #[serde(skip)] + fatal: Option, +} + +#[derive(Debug, Clone, Copy, Serialize)] +#[serde(rename_all = "snake_case")] +pub(crate) enum Verdict { + Clean, + Violations, + CouldNotLook, +} + +impl Verdict { + const fn exit(self) -> Exit { + match self { + Self::Clean => Exit::Clean, + Self::Violations => Exit::Violations, + Self::CouldNotLook => Exit::Broken, + } + } +} + +#[derive(Debug, Serialize)] +pub(crate) struct EvaluatedClaim { + pub(crate) principle: String, + pub(crate) rule: String, + status: ClaimStatus, + suppliers: Vec, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +enum ClaimStatus { + Reconciled, + Refused, + Unavailable, +} + +#[derive(Debug, Serialize)] +struct Supplier { + component: &'static str, + seam: String, +} + +/// `uphold check`, its JSON form, and the separate coverage report. +pub(crate) fn run(root: &Path, policy: &Policy, coverage: bool, json: bool) -> Result { let path = root.join(DECLARATION); if !path.is_file() { return Err(Fatal::new(format!( @@ -724,125 +779,222 @@ pub(crate) fn run(root: &Path, policy: &Policy, coverage: bool) -> Result let text = std::fs::read_to_string(&path).map_err(|error| Fatal::at(&path, error))?; let declaration: Declaration = toml::from_str(&text).map_err(|error| Fatal::at(&path, error))?; - let installed = installed(root)?; let supply = suppliers(policy, &installed); - if coverage { return report_coverage(policy, &declaration, &installed, &supply); } + let report = evaluate(&path, &declaration, &installed, &supply)?; + if json { + println!( + "{}", + serde_json::to_string(&report).map_err(|error| Fatal::new(error.to_string()))? + ); + Ok(report.verdict.exit()) + } else { + report_human(&report) + } +} - let mut failures: Vec = Vec::new(); - let mut evidence: Vec = Vec::new(); - - for (index, claim) in declaration.enforce.iter().enumerate() { - let at = format!("enforce[{index}]"); - if claim.tier.is_some() { - return Err(Fatal::at( - &path, - format!( - "{at} carries a `tier`. The field is gone: a rule id resolves across \ - every seam at once, so a claim naming one no longer has to say which. \ - Drop the line." - ), - )); - } - let (Some(principle), Some(rule)) = (claim.principle.as_deref(), claim.rule.as_deref()) - else { - return Err(Fatal::at( - &path, - format!("{at}: `principle` and `rule` are both required"), - )); - }; - if principle.trim().is_empty() || rule.trim().is_empty() { - return Err(Fatal::at( - &path, - format!("{at}: `principle` and `rule` must not be blank"), - )); - } +fn validate_claim<'a>(path: &Path, index: usize, claim: &'a Claim) -> Result<(&'a str, &'a str)> { + let at = format!("enforce[{index}]"); + if claim.tier.is_some() { + return Err(Fatal::at( + path, + format!( + "{at} carries a `tier`. The field is gone: a rule id resolves across \ + every seam at once, so a claim naming one no longer has to say which. \ + Drop the line." + ), + )); + } + let (Some(principle), Some(rule)) = (claim.principle.as_deref(), claim.rule.as_deref()) else { + return Err(Fatal::at( + path, + format!("{at}: `principle` and `rule` are both required"), + )); + }; + if principle.trim().is_empty() || rule.trim().is_empty() { + return Err(Fatal::at( + path, + format!("{at}: `principle` and `rule` must not be blank"), + )); + } + Ok((principle, rule)) +} - let Some(record) = catalog::get(principle)? else { - failures.push(format!( +fn assess_claim( + index: usize, + principle: &str, + rule: &str, + supplied: bool, + unreadable: &[String], +) -> Result<(ClaimStatus, Option)> { + let at = format!("enforce[{index}]"); + let Some(record) = catalog::get(principle)? else { + return Ok(( + ClaimStatus::Refused, + Some(format!( "{at}: {}", catalog::unknown(principle, catalog::ids()?.len()) - )); - continue; - }; - if record.deprecated() { - failures.push(format!( + )), + )); + }; + if record.deprecated() { + return Ok(( + ClaimStatus::Refused, + Some(format!( "{at}: {principle:?} is deprecated; the catalog keeps it for redirects only" - )); - continue; - } - if record.refuses_automation() { - failures.push(format!( + )), + )); + } + if record.refuses_automation() { + return Ok(( + ClaimStatus::Refused, + Some(format!( "{at}: the {principle:?} record says enforcement.automatable = \"no\"; \ - no rule can be claimed to enforce it" - )); - continue; - } - - if let Some(by) = supply.supplied.get(rule) { - evidence.push(format!( - "{principle} <- {rule} enforced by {}", - by.join(", ") - )); - continue; - } - - if !installed.unreadable.is_empty() { - // A rule absent from what could be read is not an absent rule. The - // configuration that could not be inspected is exactly where it - // might be, so this is could-not-look and not a false claim. - return Err(Fatal::new(format!( + no rule can be claimed to enforce it" + )), + )); + } + if supplied { + return Ok((ClaimStatus::Reconciled, None)); + } + // A rule absent from an unreadable configuration is unknown, not false. + if !unreadable.is_empty() { + return Ok(( + ClaimStatus::Unavailable, + Some(format!( "{at}: no rule {rule:?} in what could be read, and {} \ - could not be read; cannot tell whether the claim holds", - installed.unreadable.join("; ") - ))); - } + could not be read; cannot tell whether the claim holds", + unreadable.join("; ") + )), + )); + } + let mut failure = format!("{at}: {principle:?} claims {rule:?}, which no seam here supplies"); + if let Some((_, went)) = REMOVED.iter().find(|(id, _)| *id == rule) { + failure.push_str("; "); + failure.push_str(went); + } + Ok((ClaimStatus::Refused, Some(failure))) +} - let mut failure = - format!("{at}: {principle:?} claims {rule:?}, which no seam here supplies"); - if let Some((_, went)) = REMOVED.iter().find(|(id, _)| *id == rule) { - failure.push_str("; "); - failure.push_str(went); +fn evaluate( + path: &Path, + declaration: &Declaration, + installed: &Installed, + supply: &Supply, +) -> Result { + // Invalid declarations are pre-evaluation failures: neither renderer emits a report. + let claims = declaration + .enforce + .iter() + .enumerate() + .map(|(index, claim)| validate_claim(path, index, claim)) + .collect::>>()?; + let mut report = Report { + schema_version: 1, + verdict: Verdict::Clean, + claims: Vec::new(), + violations: installed.misplaced.clone(), + unavailable: installed.unreadable.clone(), + notes: installed.how.clone(), + misplaced_count: installed.misplaced.len(), + fatal: None, + }; + for (index, (principle, rule)) in claims.into_iter().enumerate() { + let by = supply.supplied.get(rule); + let (status, diagnostic) = + assess_claim(index, principle, rule, by.is_some(), &installed.unreadable)?; + if let Some(diagnostic) = diagnostic { + if status == ClaimStatus::Unavailable { + if report.fatal.is_none() { + report.fatal = Some(diagnostic.clone()); + } + report.unavailable.push(diagnostic); + } else { + report.violations.push(diagnostic); + } } - failures.push(failure); + report.claims.push(EvaluatedClaim { + principle: principle.to_owned(), + rule: rule.to_owned(), + status, + suppliers: if status == ClaimStatus::Reconciled { + by.into_iter() + .flatten() + .map(|seam| Supplier { + component: if seam == "a hook installed here" { + "local" + } else { + "uphold" + }, + seam: seam.clone(), + }) + .collect() + } else { + Vec::new() + }, + }); } + // Preserve the reconcile's precedence: an indeterminate claim stops the run; + // otherwise a found violation outranks an unread runner configuration. + let exit = if report.fatal.is_some() { + Exit::Broken + } else { + verdict(report.violations.len(), report.unavailable.len()) + }; + report.verdict = match exit { + Exit::Clean => Verdict::Clean, + Exit::Violations => Verdict::Violations, + Exit::Broken => Verdict::CouldNotLook, + }; + Ok(report) +} - // A hook that can only fail where it is pinned is a defect in the file this - // reconcile reads, whatever it is claimed for, so it is refused beside the - // claims rather than inside them. - if !installed.misplaced.is_empty() { +fn report_human(report: &Report) -> Result { + if let Some(fatal) = &report.fatal { + return Err(Fatal::new(fatal.clone())); + } + if report.misplaced_count > 0 { eprintln!("hook configuration refused:"); - for misplaced in &installed.misplaced { + for misplaced in report.violations.iter().take(report.misplaced_count) { eprintln!("- {misplaced}"); } } - if !failures.is_empty() { + if report.violations.len() > report.misplaced_count { eprintln!("enforcement claims refused ({DECLARATION}):"); - for failure in &failures { + for failure in report.violations.iter().skip(report.misplaced_count) { eprintln!("- {failure}"); } } - if !failures.is_empty() || !installed.misplaced.is_empty() { + if !report.violations.is_empty() { return Ok(Exit::Violations); } - if !installed.unreadable.is_empty() { + if !report.unavailable.is_empty() { return Err(Fatal::new(format!( "runner configuration could not be read: {}", - installed.unreadable.join("; ") + report.unavailable.join("; ") ))); } - - println!("reconciled {} enforcement claims:", evidence.len()); - for line in &evidence { - println!(" {line}"); + println!("reconciled {} enforcement claims:", report.claims.len()); + for claim in &report.claims { + println!( + " {} <- {} enforced by {}", + claim.principle, + claim.rule, + claim + .suppliers + .iter() + .map(|supplier| supplier.seam.as_str()) + .collect::>() + .join(", ") + ); } - for note in &installed.how { + for note in &report.notes { println!(" note {note}"); } - Ok(Exit::Clean) + Ok(report.verdict.exit()) } /// The denominator the reconcile cannot see: rules that run and claim nothing. diff --git a/src/main.rs b/src/main.rs index 68947a0..21c5588 100644 --- a/src/main.rs +++ b/src/main.rs @@ -114,7 +114,7 @@ usage: uphold scan --text [FILE|-] run the host-identity rules over text uphold guard --stage STAGE run the guards that fire at STAGE uphold guard --text [FILE|-] run the text-capable guards over text - uphold check reconcile policy/upheld.toml against what runs + uphold check [--json] reconcile policy/upheld.toml against what runs uphold check --coverage which rules run here and carry no principle uphold audit --for-publication what a private->public flip would republish uphold init --owner OWNER write a first policy, claims and hook config @@ -361,10 +361,11 @@ fn run() -> Result { "guard" => guard_command(rest), "audit" => audit_command(rest), "check" => match rest { - [] => check_command(false), - [flag] if flag == "--coverage" => check_command(true), + [] => check_command(false, false), + [flag] if flag == "--coverage" => check_command(true, false), + [flag] if flag == "--json" => check_command(false, true), _ => Err(Fatal::new(format!( - "usage: uphold check [--coverage]\n\n{USAGE}" + "usage: uphold check [--coverage | --json]\n\n{USAGE}" ))), }, "hooks" => match rest { @@ -579,11 +580,11 @@ fn run() -> Result { /// The loader runs first and its answer is what the reconcile reads, which is /// the whole point of moving this in: `uphold_check.py` re-implemented /// `config::load` to answer the same question and was free to disagree with it. -fn check_command(coverage: bool) -> Result { +fn check_command(coverage: bool, json: bool) -> Result { let working = std::env::current_dir()?; let (root, policy_path) = discover(&working).ok_or_else(|| no_policy_here(&working))?; let policy = config::load(&root, &policy_path)?; - check::run(&root, &policy, coverage) + check::run(&root, &policy, coverage, json) } fn scan_command(arguments: &[OsString]) -> Result { diff --git a/tests/check_cli.rs b/tests/check_cli.rs index bad0b7b..e821fa1 100644 --- a/tests/check_cli.rs +++ b/tests/check_cli.rs @@ -1190,3 +1190,185 @@ fn a_claim_on_a_text_only_rule_is_credited_to_scan_text() { stdout(&present) ); } + +#[test] +fn json_and_human_output_share_the_evaluated_claims() { + let root = workspace(); + write(&root, "policy/principles.toml", GUARD_POLICY); + write(&root, ".pre-commit-config.yaml", PRE_COMMIT); + write( + &root, + "policy/upheld.toml", + "[[enforce]]\nprinciple = \"fail-safe-defaults\"\nrule = \"prevent-public-push\"\n\n[[enforce]]\nprinciple = \"explicit-unknown\"\nrule = \"my-own-check\"\n", + ); + let human = check(&root, &[]); + let machine = check(&root, &["--json"]); + assert_eq!(code(&human), 0, "{}", stderr(&human)); + assert_eq!(code(&machine), 0, "{}", stderr(&machine)); + assert!(machine.stderr.is_empty(), "{}", stderr(&machine)); + let report: serde_json::Value = serde_json::from_slice(&machine.stdout).unwrap(); + assert_eq!(report.pointer("/schema_version").unwrap(), 1); + assert_eq!(report.pointer("/verdict").unwrap(), "clean"); + let claims = report.pointer("/claims").unwrap().as_array().unwrap(); + assert_eq!(claims.len(), 2); + for claim in claims { + assert_eq!(claim.pointer("/status").unwrap(), "reconciled"); + let principle = claim.pointer("/principle").unwrap().as_str().unwrap(); + let rule = claim.pointer("/rule").unwrap().as_str().unwrap(); + let suppliers = claim.pointer("/suppliers").unwrap().as_array().unwrap(); + let seams: Vec<&str> = suppliers + .iter() + .map(|by| by.pointer("/seam").unwrap().as_str().unwrap()) + .collect(); + assert!(stdout(&human).contains(&format!( + "{principle} <- {rule} enforced by {}", + seams.join(", ") + ))); + } + assert_eq!( + claims + .first() + .unwrap() + .pointer("/suppliers/0/component") + .unwrap(), + "uphold" + ); + assert_eq!( + claims + .last() + .unwrap() + .pointer("/suppliers/0/component") + .unwrap(), + "local" + ); + assert_eq!( + report.pointer("/violations").unwrap(), + &serde_json::json!([]) + ); + assert_eq!( + report.pointer("/unavailable").unwrap(), + &serde_json::json!([]) + ); + for note in report.pointer("/notes").unwrap().as_array().unwrap() { + assert!(stdout(&human).contains(note.as_str().unwrap())); + } +} + +#[test] +fn json_refusal_keeps_the_claims_that_reconciled() { + let root = workspace(); + write(&root, "policy/principles.toml", GUARD_POLICY); + write(&root, ".pre-commit-config.yaml", PRE_COMMIT); + write( + &root, + "policy/upheld.toml", + "[[enforce]]\nprinciple = \"explicit-unknown\"\nrule = \"my-own-check\"\n\n[[enforce]]\nprinciple = \"fail-safe-defaults\"\nrule = \"missing-check\"\n", + ); + let human = check(&root, &[]); + let machine = check(&root, &["--json"]); + assert_eq!(code(&human), 1); + assert_eq!(human.stdout, Vec::::new()); + assert_eq!(code(&machine), 1, "{}", stderr(&machine)); + let report: serde_json::Value = serde_json::from_slice(&machine.stdout).unwrap(); + assert_eq!(report.pointer("/verdict").unwrap(), "violations"); + assert_eq!(report.pointer("/claims/0/status").unwrap(), "reconciled"); + assert_eq!(report.pointer("/claims/1/status").unwrap(), "refused"); + assert_eq!( + report.pointer("/claims/1/suppliers").unwrap(), + &serde_json::json!([]) + ); + let failure = report.pointer("/violations/0").unwrap().as_str().unwrap(); + assert!(failure.contains("missing-check")); + assert!(stderr(&human).contains(failure)); +} + +#[test] +fn json_unavailable_evidence_preserves_partial_claims_and_status_precedence() { + let root = workspace(); + write(&root, "policy/principles.toml", GUARD_POLICY); + write(&root, ".pre-commit-config.yaml", PRE_COMMIT); + write(&root, ".codex/hooks.json", "{ not json"); + let held = "[[enforce]]\nprinciple = \"explicit-unknown\"\nrule = \"my-own-check\"\n"; + // A supplied claim remains visible even when another configuration cannot be read. + for (extra, expected, status) in [ + ("", 2, "reconciled"), + ( + "\n[[enforce]]\nprinciple = \"fail-safe-defaults\"\nrule = \"missing-check\"\n", + 2, + "unavailable", + ), + ( + "\n[[enforce]]\nprinciple = \"not-a-principle\"\nrule = \"my-own-check\"\n", + 1, + "refused", + ), + ] { + write(&root, "policy/upheld.toml", &format!("{held}{extra}")); + let machine = check(&root, &["--json"]); + assert_eq!(code(&machine), expected, "{}", stderr(&machine)); + assert_eq!(code(&check(&root, &[])), expected); + let report: serde_json::Value = serde_json::from_slice(&machine.stdout).unwrap(); + assert_eq!(report.pointer("/claims/0/status").unwrap(), "reconciled"); + assert_eq!( + report + .pointer("/claims") + .unwrap() + .as_array() + .unwrap() + .last() + .unwrap() + .get("status") + .unwrap(), + status + ); + assert!( + report + .pointer("/unavailable") + .unwrap() + .as_array() + .unwrap() + .iter() + .any(|note| note.as_str().unwrap().contains(".codex/hooks.json")) + ); + assert_eq!( + report.pointer("/verdict").unwrap(), + if expected == 1 { + "violations" + } else { + "could_not_look" + } + ); + } +} + +#[test] +fn json_pre_evaluation_failures_have_no_report() { + let root = workspace(); + write(&root, "policy/principles.toml", GUARD_POLICY); + for declaration in [ + None, + Some("[[enforce]] this is not TOML"), + Some("[[enforce]]\nprinciple = \"explicit-unknown\"\n"), + ] { + if let Some(contents) = declaration { + write(&root, "policy/upheld.toml", contents); + } + let output = check(&root, &["--json"]); + assert_eq!(code(&output), 2); + assert_eq!(output.stdout, Vec::::new()); + assert_ne!(output.stderr, Vec::::new()); + } + write(&root, "policy/principles.toml", "[rule"); + let output = check(&root, &["--json"]); + assert_eq!(code(&output), 2); + assert_eq!(output.stdout, Vec::::new()); +} + +#[test] +fn json_is_a_reconciliation_format_and_not_a_coverage_format() { + let root = workspace(); + let output = check(&root, &["--coverage", "--json"]); + assert_eq!(code(&output), 2); + assert_eq!(output.stdout, Vec::::new()); + assert!(stderr(&output).contains("usage: uphold check")); +} diff --git a/tests/test_uphold_check.py b/tests/test_uphold_check.py index f7d9045..910971d 100644 --- a/tests/test_uphold_check.py +++ b/tests/test_uphold_check.py @@ -289,15 +289,7 @@ def test_a_refused_declaration_exports_nothing(self): @needs_the_engine class TheTwoReadersOfOneReport(unittest.TestCase): - """The binary decides which rules run; the export must carry that answer whole. - - Since the reconcile moved into the loader, this script no longer derives a - rule set of its own -- it PARSES the binary's report. That closes the old - disagreement and opens a quieter one: a parser that stops recognising the - report it reads produces an empty answer rather than an error, and an empty - answer here is a component definition with no components, published at exit - 0 over a repository whose reconcile had just succeeded on every claim. - """ + """The binary decides which rules run; its machine contract carries that answer.""" def setUp(self): self._directory = tempfile.TemporaryDirectory() @@ -341,50 +333,218 @@ def test_the_binary_and_the_export_name_the_same_rules(self): } self.assertEqual(rules, {"prevent-public-push", "no-merge-conflict-markers"}) - def test_a_report_this_reader_cannot_parse_is_could_not_look(self): - """One word of the report changes, and the reader recovers nothing. - - Before the count check this returned `{}`, which is indistinguishable - from a repository where no claim is supplied by anything. - """ + def test_human_wording_is_not_part_of_the_consumer_contract(self): real = uphold_check.engine - def drifted(root, *args): - answered = real(root, *args) - if args[:1] == ("check",): - answered.stdout = answered.stdout.replace("enforced by", "supplied by") - return answered - - with ( - mock.patch.object(uphold_check, "engine", drifted), - self.assertRaises(uphold_check.CouldNotLook) as raised, - ): - uphold_check.engine_suppliers(self.tmp) - self.assertIn("2 reconciled claim(s)", str(raised.exception)) - self.assertIn("0 evidence line(s)", str(raised.exception)) - - def test_a_drifted_report_publishes_no_component_definition(self): - """Could not look is exit 2 and no document, not exit 0 and an empty one.""" - real = uphold_check.engine + def human_wording(root, *args): + self.assertEqual(args, ("check", "--json")) + return real(root, *args) - def drifted(root, *args): - answered = real(root, *args) - if args[:1] == ("check",): - answered.stdout = answered.stdout.replace("enforced by", "supplied by") - return answered + with mock.patch.object(uphold_check, "engine", human_wording): + self.assertEqual( + uphold_check.engine_suppliers(self.tmp), + { + "prevent-public-push": ["uphold"], + "no-merge-conflict-markers": ["uphold"], + }, + ) + def test_malformed_json_publishes_no_component_definition(self): + failed = subprocess.CompletedProcess( + ["uphold", "check", "--json"], 0, "not JSON", "" + ) stdout = io.StringIO() with ( contextlib.chdir(self.tmp), contextlib.redirect_stdout(stdout), contextlib.redirect_stderr(io.StringIO()), - mock.patch.object(uphold_check, "engine", drifted), + mock.patch.object(uphold_check, "engine", return_value=failed), ): code = uphold_check.main(["--oscal"]) self.assertEqual(code, 2) self.assertEqual(stdout.getvalue().strip(), "") +class ReconciliationContract(unittest.TestCase): + def clean_report(self): + return { + "schema_version": 1, + "verdict": "clean", + "claims": [ + { + "principle": "explicit-unknown", + "rule": "my-check", + "status": "reconciled", + "suppliers": [{"component": "local", "seam": "wording can change"}], + } + ], + "violations": [], + "unavailable": [], + "notes": [], + } + + def suppliers(self, report, *, status=0, strict=True): + answered = subprocess.CompletedProcess( + ["uphold", "check", "--json"], status, json.dumps(report), "" + ) + with mock.patch.object(uphold_check, "engine", return_value=answered) as engine: + result = uphold_check.engine_suppliers(ROOT, strict=strict) + engine.assert_called_once_with(ROOT, "check", "--json") + return result + + def test_component_is_explicit_and_seam_wording_does_not_route_it(self): + report = self.clean_report() + self.assertEqual(self.suppliers(report), {"my-check": ["local"]}) + report["claims"][0]["suppliers"][0]["seam"] = "a hook installed here" + report["claims"][0]["suppliers"][0]["component"] = "uphold" + self.assertEqual(self.suppliers(report), {"my-check": ["uphold"]}) + + def test_empty_evaluated_claims_are_a_valid_clean_report(self): + report = self.clean_report() + report["claims"] = [] + self.assertEqual(self.suppliers(report), {}) + + def test_unknown_versions_including_boolean_and_float_are_rejected(self): + for version in (None, True, 1.0, 0, 2, "1"): + with ( + self.subTest(version=version), + self.assertRaises(uphold_check.CouldNotLook), + ): + report = self.clean_report() + report["schema_version"] = version + self.suppliers(report) + + def test_malformed_and_inconsistent_reports_fail_closed(self): + mutations = [ + lambda r: r.pop("claims"), + lambda r: r.update(claims={}), + lambda r: r.update(verdict="unknown"), + lambda r: r.update(verdict="violations", violations=["refused"]), + lambda r: r.update(unavailable=["missing config"]), + lambda r: r.update(violations=["refused"]), + lambda r: r.update(notes=[3]), + lambda r: r["claims"].append(None), + lambda r: r["claims"][0].update(rule=""), + lambda r: r["claims"][0].update(status="unknown"), + lambda r: r["claims"][0].update(status="refused", suppliers=[]), + lambda r: r["claims"][0].update(status="unavailable", suppliers=[]), + lambda r: r["claims"][0].update(suppliers=[]), + lambda r: r["claims"][0].update(suppliers=[None]), + lambda r: r["claims"][0]["suppliers"][0].update(component="other"), + lambda r: r["claims"][0]["suppliers"][0].update(seam=""), + ] + for mutate in mutations: + with ( + self.subTest(mutate=mutate), + self.assertRaises(uphold_check.CouldNotLook), + ): + report = self.clean_report() + mutate(report) + self.suppliers(report, strict=False) + for report in (None, [], True): + with ( + self.subTest(report=report), + self.assertRaises(uphold_check.CouldNotLook), + ): + self.suppliers(report) + + def test_malformed_json_and_duplicate_fields_fail_closed(self): + for payload in ("", "not JSON", '{"schema_version": 1, "schema_version": 1}'): + answered = subprocess.CompletedProcess(["uphold", "check"], 0, payload, "") + with ( + self.subTest(payload=payload), + mock.patch.object(uphold_check, "engine", return_value=answered), + self.assertRaises(uphold_check.CouldNotLook), + ): + uphold_check.engine_suppliers(ROOT, strict=False) + + def test_review_retains_partial_evidence_from_an_evaluated_refusal(self): + report = self.clean_report() + report.update(verdict="violations", violations=["a false claim"]) + report["claims"].append( + { + "principle": "explicit-unknown", + "rule": "missing", + "status": "refused", + "suppliers": [], + } + ) + self.assertEqual( + self.suppliers(report, status=1, strict=False), {"my-check": ["local"]} + ) + with self.assertRaises(uphold_check.Refused): + self.suppliers(report, status=1) + + def test_unavailable_evidence_never_becomes_an_ordinary_review_refusal(self): + for verdict, status, violations in ( + ("could_not_look", 2, []), + ("violations", 1, ["bad claim"]), + ): + report = self.clean_report() + report.update( + verdict=verdict, violations=violations, unavailable=["unread runner"] + ) + for strict in (False, True): + with ( + self.subTest(verdict=verdict, strict=strict), + self.assertRaises(uphold_check.CouldNotLook), + ): + self.suppliers(report, status=status, strict=strict) + + def test_a_refused_claim_cannot_borrow_a_reconciled_claims_rule(self): + report = self.clean_report() + report["claims"][0]["rule"] = "my-check" + report["claims"].append( + { + "principle": "conways-law", + "rule": "my-check", + "status": "refused", + "suppliers": [], + } + ) + report.update( + verdict="violations", violations=["conways-law cannot be automated"] + ) + refused = subprocess.CompletedProcess( + ["uphold", "check"], 1, json.dumps(report), "" + ) + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + exemptions = [ + f'{key} = "No subject in this scratch repository."' + for key, record in uphold_check.load_records().items() + if record["enforcement"]["automatable"] == "yes" + and key != "explicit-unknown" + ] + build( + root, + "\n".join( + [ + "[[enforce]]", + 'principle = "explicit-unknown"', + 'rule = "my-check"', + "[[enforce]]", + 'principle = "conways-law"', + 'rule = "my-check"', + "[review]", + "max_lines = 10000", + "[review.no_subject_here]", + *exemptions, + ] + ), + ) + output = io.StringIO() + with ( + contextlib.chdir(root), + contextlib.redirect_stdout(output), + contextlib.redirect_stderr(io.StringIO()), + mock.patch.object(uphold_check, "engine", return_value=refused), + ): + code = uphold_check.main(["--review"]) + self.assertEqual(code, 0) + self.assertIn("1 static rule(s)", output.getvalue()) + + class AnUnavailableEngine(unittest.TestCase): def test_unavailable_engine_prevents_review_emission_and_oscal(self): with tempfile.TemporaryDirectory() as directory: @@ -423,7 +583,21 @@ def test_unavailable_engine_prevents_review_emission_and_oscal(self): self.assertFalse((root / "REVIEW.md").exists()) def test_evaluated_refusal_remains_available_to_review_only(self): - refused = subprocess.CompletedProcess(["uphold", "check"], 1, "", "bad claim") + refused = subprocess.CompletedProcess( + ["uphold", "check", "--json"], + 1, + json.dumps( + { + "schema_version": 1, + "verdict": "violations", + "claims": [], + "violations": ["bad claim"], + "unavailable": [], + "notes": [], + } + ), + "", + ) with mock.patch.object(uphold_check, "engine", return_value=refused): self.assertEqual(uphold_check.engine_suppliers(ROOT, strict=False), {}) with self.assertRaises(uphold_check.Refused): diff --git a/uphold_check.py b/uphold_check.py index bb0be62..219bc9a 100755 --- a/uphold_check.py +++ b/uphold_check.py @@ -238,95 +238,122 @@ def declared_claims(declaration: dict) -> list[tuple[str, str]]: return claims -# The report `uphold check` prints, as the two fragments this reader keys on. -# Written down here rather than inline because the count check below is the only -# thing that notices when the binary's format and this reader drift apart, and a -# reader that has to be read alongside the format it parses should say what it -# expects in one place. -RECONCILED_PREFIX = "reconciled " -RECONCILED_SUFFIX = " enforcement claims:" -ENFORCED_BY = " enforced by " - +def report_object(pairs: list[tuple[str, object]]) -> dict: + """JSON objects cannot silently overwrite evidence through duplicate keys.""" + result = {} + for key, value in pairs: + if key in result: + raise ValueError(f"duplicate JSON field {key!r}") + result[key] = value + return result + + +def reconciliation_report(answered: subprocess.CompletedProcess) -> dict: + """Validate the versioned engine contract before any evidence is consumed.""" + try: + report = json.loads(answered.stdout, object_pairs_hook=report_object) + except (ValueError, TypeError) as error: + raise CouldNotLook( + f"uphold check --json returned malformed JSON: {error}" + ) from error + if not isinstance(report, dict) or type(report.get("schema_version")) is not int: + raise CouldNotLook("uphold check --json has no integer schema_version") + if report["schema_version"] != 1: + raise CouldNotLook( + f"unsupported uphold check schema version {report['schema_version']!r}" + ) + verdicts = {"clean": 0, "violations": 1, "could_not_look": 2} + verdict = report.get("verdict") + if not isinstance(verdict, str) or verdicts.get(verdict) != answered.returncode: + raise CouldNotLook( + "uphold check --json verdict disagrees with its process status" + ) + for field in ("violations", "unavailable", "notes"): + values = report.get(field) + if not isinstance(values, list) or not all( + isinstance(value, str) and value.strip() for value in values + ): + raise CouldNotLook(f"uphold check --json has malformed {field}") + claims = report.get("claims") + if not isinstance(claims, list): + raise CouldNotLook("uphold check --json has no claims array") + for claim in claims: + if not isinstance(claim, dict) or not all( + isinstance(claim.get(field), str) and claim[field].strip() + for field in ("principle", "rule") + ): + raise CouldNotLook("uphold check --json has a malformed claim") + status = claim.get("status") + if status not in ("reconciled", "refused", "unavailable"): + raise CouldNotLook("uphold check --json has an unknown claim status") + suppliers = claim.get("suppliers") + if not isinstance(suppliers, list) or bool(suppliers) != ( + status == "reconciled" + ): + raise CouldNotLook("uphold check --json has inconsistent claim suppliers") + for supplier in suppliers: + if ( + not isinstance(supplier, dict) + or supplier.get("component") not in ("local", "uphold") + or not isinstance(supplier.get("seam"), str) + or not supplier["seam"].strip() + ): + raise CouldNotLook("uphold check --json has a malformed supplier") + if status == "refused" and not report["violations"]: + raise CouldNotLook("uphold check --json omits the claim's violation") + if status == "unavailable" and not report["unavailable"]: + raise CouldNotLook( + "uphold check --json omits the claim's unavailable evidence" + ) + if ( + (verdict == "clean" and (report["violations"] or report["unavailable"])) + or (verdict == "violations" and not report["violations"]) + or (verdict == "could_not_look" and not report["unavailable"]) + ): + raise CouldNotLook("uphold check --json has inconsistent verdict evidence") + return report -def reconciled_count(line: str) -> int | None: - """The N of `reconciled N enforcement claims:`, or None for any other line.""" - line = line.strip() - if not (line.startswith(RECONCILED_PREFIX) and line.endswith(RECONCILED_SUFFIX)): - return None - count = line[len(RECONCILED_PREFIX) : -len(RECONCILED_SUFFIX)] - return int(count) if count.isdigit() else None +def engine_report(root: Path, *, strict: bool = True) -> dict: + """Read Rust's evaluated evidence through its versioned JSON contract. -def engine_suppliers(root: Path, *, strict: bool = True) -> dict[str, list[str]]: - """Which seams supply each rule, as the reconcile in the binary sees it. - - `strict` is the difference between the two callers. `--oscal` publishes an - assertion to an outside reader, so a declaration that does not reconcile has - nothing honest to export and the refusal travels. `--review` runs over a - declaration somebody is still writing: a claim naming a rule nothing - supplies is exactly the state it exists to help with, and refusing there - would take the review document away at the moment it is most wanted. The - evidence lines for the claims that DID hold are on stdout either way. - - The evidence lines are counted against the binary's own header, because this - is a PARSER of another program's report and a parser that stops recognising - it fails silently: every line skipped by the `continue` below, an empty - mapping returned, and `--oscal` publishing a component definition with no - components at all -- exit 0, to an outside reader, over a repository whose - reconcile had just succeeded on every claim. Nothing in the pipeline could - tell that apart from a repository that enforces nothing. A report this - reader could not read is could-not-look; see the `explicit-unknown` record. + OSCAL requires a clean reconciliation. Review also accepts an evaluated + refusal and retains evidence for claims that held. Neither can use a report + with unavailable evidence, even when a violation determines the exit code. """ - answered = engine(root, "check") - if answered.returncode == 2: - raise CouldNotLook(answered.stderr.strip() or "uphold check could not look") - if answered.returncode not in (0, 1): + answered = engine(root, "check", "--json") + if answered.returncode not in (0, 1, 2): raise CouldNotLook( f"uphold check exited unexpectedly with status {answered.returncode}: " f"{answered.stderr.strip() or 'no diagnostic was reported'}" ) - if answered.returncode != 0 and strict: + if answered.returncode == 2 and not answered.stdout.strip(): + # Loading/discovery/declaration failures happen before a report exists. + raise CouldNotLook(answered.stderr.strip() or "uphold check could not look") + report = reconciliation_report(answered) + if report["unavailable"]: + raise CouldNotLook( + "uphold check could not look: " + "; ".join(report["unavailable"]) + ) + if answered.returncode == 1 and strict: raise Refused( "the enforcement claims do not reconcile, so there is nothing " - f"honest to export:\n{answered.stderr.strip()}" + "honest to export:\n" + "\n".join(report["violations"]) ) + return report + + +def engine_suppliers(root: Path, *, strict: bool = True) -> dict[str, list[str]]: + """Fold reconciled claims into the components used by the OSCAL export.""" + report = engine_report(root, strict=strict) suppliers: dict[str, list[str]] = {} - reconciled: int | None = None - read = 0 - for line in answered.stdout.splitlines(): - if reconciled is None: - reconciled = reconciled_count(line) - if " <- " not in line or ENFORCED_BY not in line: + for claim in report["claims"]: + if claim["status"] != "reconciled": continue - read += 1 - _, rest = line.split(" <- ", 1) - rule, by = rest.split(ENFORCED_BY, 1) - # Folded back to the SEAM, because an OSCAL component is a thing that - # implements a control and the seam is that thing. `uphold check` names - # the evidence -- which stage, which scan -- and a component per stage - # would split one implementation across five. - seams = [] - for part in by.split(","): - part = part.strip() - if not part: - continue - seams.append("local" if part.startswith("a hook") else "uphold") - for seam in seams: - if seam not in suppliers.setdefault(rule.strip(), []): - suppliers[rule.strip()].append(seam) - - # Only where the binary said the reconcile HELD. A refused one prints its - # failures on stderr and nothing on stdout, and `--review` is meant to keep - # going over exactly that; there is no count there to check against, and - # requiring one would turn the state this mode exists for into exit 2. - if answered.returncode == 0 and read != reconciled: - raise CouldNotLook( - f"`uphold check` reported {reconciled if reconciled is not None else 'no'} " - f"reconciled claim(s) and this reader recovered {read} evidence line(s) " - "from the same report, so which seam supplies which rule is unknown. " - "The binary's report and this reader have drifted apart; the binary " - "is the authority, so fix the reader." - ) + for supplier in claim["suppliers"]: + seams = suppliers.setdefault(claim["rule"], []) + if supplier["component"] not in seams: + seams.append(supplier["component"]) return suppliers @@ -730,28 +757,28 @@ def run_review(argv: list[str]) -> int: (name, emit_target(root, name)) for name in settings["emit_controls"] ] claims = declared_claims(declaration) - suppliers = engine_suppliers(root, strict=False) + report = engine_report(root, strict=False) + reconciled = { + (claim["principle"], claim["rule"]) + for claim in report["claims"] + if claim["status"] == "reconciled" + } except CouldNotLook as error: print(f"uphold review could not look: {error}", file=sys.stderr) return 2 records = load_records() - # Filtered by `suppliers`, exactly as `active` is on the line below. A - # principle leaves the review document because a rule enforces it -- that is - # the `continue` in `review.route`, "a rule enforces it; a reviewer - # repeating it is noise" -- and a claim naming a rule no seam here supplies - # enforces nothing. Unfiltered, such a claim removed the principle from the - # document AND was absent from the "already active here" list the same - # document prints, so an `automatable = "yes"` record could be enforced by - # nothing and reviewed by nobody, with the page showing no trace of either. - # - # The reconcile refuses that claim outright; this mode has to survive it, - # because `--review` runs over a declaration a person is still writing. - claimed = {principle for principle, rule in claims if rule in suppliers} + # Partial reports identify the exact claims that held. A refused claim may + # name the same rule as a reconciled one, so the rule id alone is insufficient. + claimed = { + principle for principle, rule in claims if (principle, rule) in reconciled + } for_review, errors, stale = review_mod.route( records, claimed, settings["exempt"], settings["include_domains"] ) - active = sorted({rule for _, rule in claims if rule in suppliers}) + active = sorted( + {rule for principle, rule in claims if (principle, rule) in reconciled} + ) document = review_mod.render(for_review, active) control_errors, uncontrolled = review_mod.audit_controls( From 9a9b3c61e898703e84be9c7505837591c9ea58c3 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 16 Oct 2026 21:07:17 +0900 Subject: [PATCH 2/3] Document the reconciliation report interfaces --- src/check.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/check.rs b/src/check.rs index a152f3d..c76246a 100644 --- a/src/check.rs +++ b/src/check.rs @@ -726,6 +726,7 @@ pub(crate) struct Report { fatal: Option, } +/// The machine report names the same outcome that determines the process status. #[derive(Debug, Clone, Copy, Serialize)] #[serde(rename_all = "snake_case")] pub(crate) enum Verdict { @@ -744,6 +745,8 @@ impl Verdict { } } +/// A declaration entry retains its individual result and supplying seams, so a +/// refused or incomplete report can still identify the claims that held. #[derive(Debug, Serialize)] pub(crate) struct EvaluatedClaim { pub(crate) principle: String, From 30f3b0d733ad22574a72763afa65e83cc90e802c Mon Sep 17 00:00:00 2001 From: HackingGate Date: Fri, 16 Oct 2026 21:10:04 +0900 Subject: [PATCH 3/3] Exclude misplaced range hooks from partial reconciliation evidence A range-only hook declared exclusively at stages without a push range cannot supply an enforcement claim. Exclude that declaration before forming the local supplier set, while retaining misplaced diagnostics and independently valid declarations of the same id. JSON partial results now keep review from crediting an explicitly unusable hook. Validation: 52 check CLI tests cover invalid, mixed and independent supplying stages; 29 reporting tests include the review regression. All-targets clippy, Ruff and formatting pass. --- docs/REFERENCE.md | 4 +++ src/check.rs | 12 ++++++++ tests/check_cli.rs | 62 ++++++++++++++++++++++++++++++++++++++ tests/test_uphold_check.py | 30 ++++++++++++++++++ 4 files changed, 108 insertions(+) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index 865eec4..419f750 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -3820,6 +3820,10 @@ a claim whose missing rule may be in unread configuration exits `2`; otherwise a known violation exits `1` even if other configuration is unreadable. The `unavailable` array records unread evidence regardless of the exit code. +A range-only hook configured exclusively at stages without a push range does +not supply a claim. A valid declaration of the same id at another stage or +through another runner still supplies it independently. + The Python review and OSCAL consumers read this JSON contract. They reject unknown schema versions, malformed or inconsistent reports, unexpected process statuses, and any unavailable evidence. Review accepts an evaluated refusal diff --git a/src/check.rs b/src/check.rs index c76246a..b350487 100644 --- a/src/check.rs +++ b/src/check.rs @@ -340,6 +340,18 @@ fn pinned_ids(root: &Path) -> Result, Vec)>> { let mut every = BTreeSet::new(); for entry in repos { for hook in entry.hooks { + // A range-only hook at no push stage cannot supply a claim. Filter + // this declaration only: another declaration may install the same + // id at a valid stage, or another runner may supply it independently. + if RANGE_IDS.contains(&hook.id.as_str()) + && hook.stages.as_ref().is_some_and(|stages| { + !stages + .iter() + .any(|stage| RANGE_STAGES.contains(&stage.as_str())) + }) + { + continue; + } every.insert(hook.id); } } diff --git a/tests/check_cli.rs b/tests/check_cli.rs index e821fa1..9e3a272 100644 --- a/tests/check_cli.rs +++ b/tests/check_cli.rs @@ -1372,3 +1372,65 @@ fn json_is_a_reconciliation_format_and_not_a_coverage_format() { assert_eq!(output.stdout, Vec::::new()); assert!(stderr(&output).contains("usage: uphold check")); } + +#[test] +fn misplaced_range_hooks_do_not_supply_partial_claim_evidence() { + let root = workspace(); + write(&root, "policy/principles.toml", GUARD_POLICY); + write( + &root, + "policy/upheld.toml", + "[[enforce]]\nprinciple = 'explicit-unknown'\nrule = 'uphold-supply-chain'\n", + ); + for (stages, other_hook, lefthook, expected_status, expected_exit) in [ + ("[manual]", "", "", "refused", 1), + ("[pre-commit]", "", "", "refused", 1), + ("[]", "", "", "refused", 1), + ("[pre-push, manual]", "", "", "reconciled", 1), + ( + "[manual]", + " - id: uphold-supply-chain\n stages: [pre-push]\n", + "", + "reconciled", + 1, + ), + ( + "[manual]", + "", + "pre-push:\n commands:\n uphold-supply-chain:\n run: ./check.sh\n", + "reconciled", + 1, + ), + ("[pre-push]", "", "", "reconciled", 0), + ] { + let config = format!( + "repos:\n - repo: https://github.com/HackingGate/uphold\n rev: v2.0.0\n hooks:\n - id: uphold-supply-chain\n stages: {stages}\n{other_hook}" + ); + write(&root, ".pre-commit-config.yaml", &config); + write(&root, "lefthook.yml", lefthook); + let output = check(&root, &["--json"]); + assert_eq!( + code(&output), + expected_exit, + "{}{}", + stdout(&output), + stderr(&output) + ); + let report: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!( + report.pointer("/claims/0/status").unwrap(), + expected_status, + "{report}" + ); + let suppliers = report + .pointer("/claims/0/suppliers") + .unwrap() + .as_array() + .unwrap(); + assert_eq!( + suppliers.is_empty(), + expected_status == "refused", + "{report}" + ); + } +} diff --git a/tests/test_uphold_check.py b/tests/test_uphold_check.py index 910971d..3dbe530 100644 --- a/tests/test_uphold_check.py +++ b/tests/test_uphold_check.py @@ -333,6 +333,36 @@ def test_the_binary_and_the_export_name_the_same_rules(self): } self.assertEqual(rules, {"prevent-public-push", "no-merge-conflict-markers"}) + def test_misplaced_range_only_hook_does_not_silence_review(self): + exemptions = [ + f'{key} = "No subject in this scratch repository."' + for key, record in uphold_check.load_records().items() + if record["enforcement"]["automatable"] == "yes" + and key != "explicit-unknown" + ] + build( + self.tmp, + "\n".join( + [ + "[[enforce]]", + 'principle = "explicit-unknown"', + 'rule = "uphold-supply-chain"', + "[review]", + "max_lines = 10000", + "[review.no_subject_here]", + *exemptions, + ] + ), + **{ + ".pre-commit-config.yaml": "repos:\n - repo: https://github.com/HackingGate/uphold\n rev: v2.0.0\n hooks:\n - id: uphold-supply-chain\n stages: [manual]\n", + }, + ) + self.assertEqual(uphold_check.engine_suppliers(self.tmp, strict=False), {}) + result = run(self.tmp, "--review") + self.assertEqual(result.returncode, 1, result.stdout) + self.assertIn("explicit-unknown", result.stderr) + self.assertIn("no rule here claims it", result.stderr) + def test_human_wording_is_not_part_of_the_consumer_contract(self): real = uphold_check.engine