diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index eed1d73..d1e9b2f 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -4585,7 +4585,11 @@ the repository's own policy: ```toml # policy/principles.toml [decide] -adopt = ["unix-composability", "defense-in-depth", "economy-of-mechanism"] +adopt = [ + { id = "unix-composability", why = "every gate here is a command a script can run" }, + "defense-in-depth", + "economy-of-mechanism", +] [[decide.order]] prefer = "defense-in-depth" @@ -4598,7 +4602,13 @@ over = "defense-in-depth" why = "Each layer is a separate command that can be run and tested alone." ``` -`adopt` is catalog ids. Every pair of adopted records that names the other in +`adopt` is catalog ids. An entry is the id alone, or a table `{ id, why }` +whose `why` is this repository's reason for adopting the record; both forms +mix in one list. The record says what it holds and is the same text for every +consumer, so the reason belongs to the consumer. `why` is optional, where an +order's is required, because an adoption is reviewable from the record alone. +It is printed and never parsed: a ruling cites the records and the orders, and +reads an adoption's `why` as context. Every pair of adopted records that names the other in `conflicts_with` takes exactly one `[[decide.order]]`: `prefer` is the record whose answer wins where the two disagree, `over` the one it wins over, and `why` the reason, for whoever reviews the order. `conflicts_with` is symmetric, so @@ -4610,7 +4620,7 @@ what a repository rules by is one command: ```text adopted 3 record(s) for design rulings ([decide]): - unix-composability + unix-composability: every gate here is a command a script can run defense-in-depth economy-of-mechanism order defense-in-depth over economy-of-mechanism: A second control is ... @@ -4618,11 +4628,14 @@ adopted 3 record(s) for design rulings ([decide]): ``` Exit `1`, under `design rulings refused ([decide]):`, for an adopted id the -catalog does not define or keeps only as deprecated, an adopted conflict with no +catalog does not define or keeps only as deprecated, an adoption `why` that is +the record's `summary` or `claim` copied, an adopted conflict with no order, an order naming a record the repository does not adopt, an order over two records that do not conflict, and a second order over a pair already ordered. Exit `2`, at load and so from every command that reads the policy, for a table whose shape is wrong: an unknown key, an `order` missing `prefer`, `over` or -`why` or holding a blank one, an id adopted twice, and `[decide]` in a bundled +`why` or holding a blank one, an id adopted twice, an adopt entry that is +neither an id nor a table, an adopt table with a key other than `id` and +`why` or a blank `why`, and `[decide]` in a bundled set or an `inherit.paths` file, since which records a repository rules by is that repository's own declaration. `uphold schema` describes the table. diff --git a/docs/schema/principles.schema.json b/docs/schema/principles.schema.json index 4449727..97899b7 100644 --- a/docs/schema/principles.schema.json +++ b/docs/schema/principles.schema.json @@ -1,5 +1,18 @@ { "$defs": { + "Adopt": { + "anyOf": [ + { + "description": "A record id.", + "type": "string" + }, + { + "$ref": "#/$defs/Reasoned", + "description": "A record id with this repository's reason for adopting it." + } + ], + "description": "One `[decide] adopt` entry: a record id, written alone or as `{ id, why }`.\n\nThe `why` is the consumer's fact, not the catalog's: the record says what it\nholds, and this says why this repository rules by it. It is printed by\n`uphold check` and never parsed, so it is context for a ruling and never a\ncitation in one. It is optional, where an order's is required: an adoption\nis reviewable from the record alone, an order is not.\n\nSerialized in the form it was written, so a string entry stays a string in\n`uphold check --json`." + }, "Collect": { "description": "How this command's subjects are found.", "oneOf": [ @@ -45,9 +58,9 @@ "properties": { "adopt": { "default": [], - "description": "Catalog record ids, each at most once.", + "description": "Catalog record ids, each at most once. An entry is an id, or a table\n`{ id, why }` whose `why` is this repository's reason for adopting it.", "items": { - "type": "string" + "$ref": "#/$defs/Adopt" }, "type": "array" }, @@ -379,6 +392,23 @@ } ] }, + "Reasoned": { + "additionalProperties": false, + "description": "The table form of an adopt entry.", + "properties": { + "id": { + "type": "string" + }, + "why": { + "description": "Why this repository adopts the record, in its own words. Refused at\nload when blank; `uphold check` refuses one that copies the record's\n`summary` or `claim`.", + "type": "string" + } + }, + "required": [ + "id" + ], + "type": "object" + }, "Remote": { "additionalProperties": false, "description": "A consumer-selected policy blob, identified by GitHub repository, immutable\ncommit and path. The loader also uses this exact tuple as rule provenance.", diff --git a/skills/design-ruling/SKILL.md b/skills/design-ruling/SKILL.md index 7334821..e8fd21b 100644 --- a/skills/design-ruling/SKILL.md +++ b/skills/design-ruling/SKILL.md @@ -15,6 +15,9 @@ must be traceable to them. Run `uphold check --json` from the repository root and read `decide`: - `adopt` is the record ids the repository answers design questions from. + An entry may be a table `{id, why}` instead of an id. Its `why` is the + repository's reason for adopting the record: context for reading the + record's scope, never a citation, and never a ground for an answer. - `order` is one `{prefer, over, why}` per adopted pair that conflicts. If `uphold check` exits 1 with `design rulings refused ([decide])`, escalate diff --git a/src/catalog.rs b/src/catalog.rs index 6dd42c1..6774ee4 100644 --- a/src/catalog.rs +++ b/src/catalog.rs @@ -211,6 +211,12 @@ pub(crate) struct Record { pub id: String, #[serde(default)] pub status: String, + /// Read so `[decide]` can refuse an adoption `why` that copies it. + #[serde(default)] + pub summary: String, + /// As `summary`. + #[serde(default)] + pub claim: String, #[serde(default)] pub enforcement: Enforcement, /// Symmetric, as `scripts/validate.py` holds it: an adopted pair in it is diff --git a/src/check.rs b/src/check.rs index ef4bf52..c0dc4d9 100644 --- a/src/check.rs +++ b/src/check.rs @@ -31,7 +31,7 @@ use std::path::Path; use serde::{Deserialize, Serialize}; use crate::catalog; -use crate::config::{Decide, Policy}; +use crate::config::{Adopt, Decide, Policy}; use crate::error::{Exit, Fatal, Result, verdict}; const DECLARATION: &str = "policy/upheld.toml"; @@ -1090,8 +1090,11 @@ fn report_human(report: &Report) -> Result { "adopted {} record(s) for design rulings ([decide]):", report.decide.adopt.len() ); - for id in &report.decide.adopt { - println!(" {id}"); + for entry in &report.decide.adopt { + match entry.why() { + Some(why) => println!(" {}: {why}", entry.id()), + None => println!(" {}", entry.id()), + } } for order in &report.decide.order { println!( @@ -1103,9 +1106,21 @@ fn report_human(report: &Report) -> Result { Ok(report.verdict.exit()) } +/// The record field an adoption's `why` repeats verbatim, if any: the +/// catalog's text in a second place, which drifts from it and says nothing +/// about why this repository adopted the record. +fn copied_field(entry: &Adopt, record: &catalog::Record) -> Option<&'static str> { + let why = entry.why()?.trim(); + [("summary", &record.summary), ("claim", &record.claim)] + .into_iter() + .find(|(_, text)| text.trim() == why) + .map(|(field, _)| field) +} + /// Hold `[decide]` to the catalog this binary carries, into `report`. /// -/// Every id must be a live record, every adopted pair that one names in +/// Every id must be a live record, no adoption's `why` may copy the record's +/// text, every adopted pair that one names in /// `conflicts_with` must carry exactly one order, and every order must be over /// such a pair. An adopted conflict with no order is two answers to one /// question, which is the thing an order exists to remove; an order over @@ -1115,14 +1130,22 @@ fn decided(report: &mut Report, decide: &Decide) -> Result<()> { let mut failures = Vec::new(); let known = catalog::ids()?.len(); let mut adopted: BTreeMap<&str, &catalog::Record> = BTreeMap::new(); - for id in &decide.adopt { + for entry in &decide.adopt { + let id = entry.id(); match catalog::get(id)? { None => failures.push(format!("adopt: {}", catalog::unknown(id, known))), Some(record) if record.deprecated() => failures.push(format!( "adopt: {id:?} is deprecated; the catalog keeps it for redirects only" )), Some(record) => { - adopted.insert(id.as_str(), record); + if let Some(copied) = copied_field(entry, record) { + failures.push(format!( + "adopt: {id:?} has a `why` that is the record's `{copied}`. The \ + catalog already says what the record holds; `why` says why this \ + repository adopts it" + )); + } + adopted.insert(id, record); } } } diff --git a/src/config.rs b/src/config.rs index bccaec8..1286c71 100644 --- a/src/config.rs +++ b/src/config.rs @@ -731,15 +731,90 @@ impl Stop { #[derive(Debug, Clone, Default, Deserialize, Serialize, schemars::JsonSchema)] #[serde(deny_unknown_fields)] pub(crate) struct Decide { - /// Catalog record ids, each at most once. + /// Catalog record ids, each at most once. An entry is an id, or a table + /// `{ id, why }` whose `why` is this repository's reason for adopting it. #[serde(default)] - pub adopt: Vec, + pub adopt: Vec, /// `[[decide.order]]`: one per pair of adopted records that names the /// other in `conflicts_with`. #[serde(default)] pub order: Vec, } +/// One `[decide] adopt` entry: a record id, written alone or as `{ id, why }`. +/// +/// The `why` is the consumer's fact, not the catalog's: the record says what it +/// holds, and this says why this repository rules by it. It is printed by +/// `uphold check` and never parsed, so it is context for a ruling and never a +/// citation in one. It is optional, where an order's is required: an adoption +/// is reviewable from the record alone, an order is not. +/// +/// Serialized in the form it was written, so a string entry stays a string in +/// `uphold check --json`. +#[derive(Debug, Clone, Serialize, schemars::JsonSchema)] +#[serde(untagged)] +pub(crate) enum Adopt { + /// A record id. + Id(String), + /// A record id with this repository's reason for adopting it. + Reasoned(Reasoned), +} + +/// The table form of an adopt entry. +#[derive(Debug, Clone, Deserialize, Serialize, schemars::JsonSchema)] +#[serde(deny_unknown_fields)] +pub(crate) struct Reasoned { + pub id: String, + /// Why this repository adopts the record, in its own words. Refused at + /// load when blank; `uphold check` refuses one that copies the record's + /// `summary` or `claim`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub why: Option, +} + +impl Adopt { + pub(crate) fn id(&self) -> &str { + match self { + Self::Id(id) | Self::Reasoned(Reasoned { id, .. }) => id, + } + } + + pub(crate) fn why(&self) -> Option<&str> { + match self { + Self::Id(_) => None, + Self::Reasoned(reasoned) => reasoned.why.as_deref(), + } + } +} + +/// Written by hand rather than `#[serde(untagged)]`, whose only error is "did +/// not match any variant": a table with a misspelt key must be told which key, +/// as every other table in the policy is. +impl<'de> Deserialize<'de> for Adopt { + fn deserialize>( + deserializer: D, + ) -> std::result::Result { + struct Visitor; + impl<'de> serde::de::Visitor<'de> for Visitor { + type Value = Adopt; + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("a record id, or a table `{ id, why }`") + } + fn visit_str(self, id: &str) -> std::result::Result { + Ok(Adopt::Id(id.to_owned())) + } + fn visit_map>( + self, + map: A, + ) -> std::result::Result { + Reasoned::deserialize(serde::de::value::MapAccessDeserializer::new(map)) + .map(Adopt::Reasoned) + } + } + deserializer.deserialize_any(Visitor) + } +} + /// One `[[decide.order]]`: where `prefer` and `over` disagree, the answer /// is `prefer`'s, for the reason `why` gives. #[derive(Debug, Clone, Deserialize, Serialize, schemars::JsonSchema)] @@ -754,13 +829,21 @@ pub(crate) struct Order { impl Decide { fn validate(&self) -> std::result::Result<(), String> { let mut seen = BTreeSet::new(); - for id in &self.adopt { + for entry in &self.adopt { + let id = entry.id(); if id.trim().is_empty() { return Err("`[decide] adopt` holds a blank id".to_owned()); } - if !seen.insert(id.as_str()) { + if !seen.insert(id) { return Err(format!("`[decide] adopt` names {id:?} twice")); } + let Some(why) = entry.why() else { continue }; + if why.trim().is_empty() { + return Err(format!( + "`[decide] adopt` entry {id:?} has a blank `why`. Give this repository's \ + reason for adopting it, or write the id alone" + )); + } } for order in &self.order { for (field, value) in [ diff --git a/tests/check_cli.rs b/tests/check_cli.rs index 6590f7b..9a00b1b 100644 --- a/tests/check_cli.rs +++ b/tests/check_cli.rs @@ -1611,6 +1611,96 @@ fn a_malformed_decide_table_is_two() { } } +#[test] +fn an_adopt_entry_with_a_why_prints_it_beside_the_id() { + let output = check( + &decide_fixture( + "[decide]\nadopt = [\n \"economy-of-mechanism\",\n \ + { id = \"unix-composability\", why = \"every gate here is a separate command\" },\n]\n", + ), + &[], + ); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + let printed = stdout(&output); + assert!(printed.contains("adopted 2 record(s)"), "{printed}"); + assert!(printed.contains("\n economy-of-mechanism\n"), "{printed}"); + assert!( + printed.contains("\n unix-composability: every gate here is a separate command\n"), + "{printed}" + ); +} + +#[test] +fn the_json_report_keeps_each_adopt_entry_in_its_written_form() { + let output = check( + &decide_fixture( + "[decide]\nadopt = [\"economy-of-mechanism\", \ + { id = \"unix-composability\", why = \"separate commands\" }]\n", + ), + &["--json"], + ); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + let report: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!( + report.pointer("/decide/adopt/0").unwrap(), + "economy-of-mechanism" + ); + assert_eq!( + report.pointer("/decide/adopt/1/why").unwrap(), + "separate commands" + ); +} + +#[test] +fn an_adopt_entry_of_the_wrong_shape_is_two_and_named() { + for (entry, says) in [ + ( + "{ id = \"economy-of-mechanism\", why = \" \" }", + "\"economy-of-mechanism\" has a blank `why`", + ), + ( + "{ id = \"economy-of-mechanism\", because = \"small\" }", + "unknown field `because`", + ), + ("1", "expected a record id, or a table `{ id, why }`"), + ] { + let decide = format!("[decide]\nadopt = [{entry}]\n"); + let output = check(&decide_fixture(&decide), &[]); + assert_eq!(code(&output), 2, "{decide}: {}", stderr(&output)); + assert!( + stderr(&output).contains(says), + "{decide}: {}", + stderr(&output) + ); + } +} + +/// Whether a `why` copies the record is a question about the catalog this +/// binary carries, so `uphold check` answers it at exit 1, as it does an +/// unknown id; the policy still loads for every other command. +#[test] +fn an_adopt_why_that_copies_the_record_is_one() { + // `economy-of-mechanism`'s own text, copied. + let summary = "Keep a protection mechanism small and simple enough that every path through \ + it can be inspected and tested."; + let claim = "A mechanism whose errors stay invisible in normal use must be small and simple \ + enough to verify completely, because its flaws surface only under the attack or \ + accident it exists to stop."; + for (text, field) in [(summary, "summary"), (claim, "claim")] { + let decide = + format!("[decide]\nadopt = [{{ id = \"economy-of-mechanism\", why = \"{text}\" }}]\n"); + let output = check(&decide_fixture(&decide), &[]); + assert_eq!(code(&output), 1, "{decide}: {}", stderr(&output)); + assert!( + stderr(&output).contains(&format!( + "adopt: \"economy-of-mechanism\" has a `why` that is the record's `{field}`" + )), + "{decide}: {}", + stderr(&output) + ); + } +} + #[test] fn a_decide_table_in_an_inherited_file_is_two() { let root = workspace();