Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 55 additions & 1 deletion docs/REFERENCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ namespace — that is what lets a claim in
- [`uphold schema` — the policy file's JSON Schema](#uphold-schema--the-policy-files-json-schema)
- [The review tier](#the-review-tier)
- [Controls: can a record produce a finding at all?](#controls-can-a-record-produce-a-finding-at-all)
- [Design rulings: `[decide]`](#design-rulings-decide)

## Rule shape

Expand Down Expand Up @@ -4126,7 +4127,9 @@ 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.
`decide` is the policy's [`[decide]`](#design-rulings-decide) as written,
`{"adopt": [...], "order": [{"prefer", "over", "why"}]}`, empty where the
policy has none. 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
Expand Down Expand Up @@ -4368,3 +4371,54 @@ is one that has not answered within `--timeout` seconds (default 300); a person
answering at a prompt passes a longer one. No
model client lives here; the reviewer command carries the model, the key and
the budget. See [DESIGN.md](DESIGN.md#a-control-is-the-probe-fixture-one-tier-up).

## Design rulings: `[decide]`

A repository's design questions are answered from the records it adopts, and
where two adopted records conflict, from the side it takes. Both are written in
the repository's own policy:

```toml
# policy/principles.toml
[decide]
adopt = ["unix-composability", "defense-in-depth", "economy-of-mechanism"]

[[decide.order]]
prefer = "defense-in-depth"
over = "economy-of-mechanism"
why = "A second control is worth the larger mechanism for anything that publishes."
Comment on lines +4384 to +4389

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Example omits a required order

The example adopts unix-composability and defense-in-depth, which also conflict, but supplies no order for that pair. Readers who copy it get a policy that makes uphold check exit 1. Add a second [[decide.order]] with a reason, and include it in the sample output.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment thread
coderabbitai[bot] marked this conversation as resolved.

[[decide.order]]
prefer = "unix-composability"
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
`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
one order covers the pair whichever end lists it.

`uphold check` holds the table to the catalog this binary carries and prints
the adopted list and its orders after the reconciled claims, so the review of
what a repository rules by is one command:

```text
adopted 3 record(s) for design rulings ([decide]):
unix-composability
defense-in-depth
economy-of-mechanism
order defense-in-depth over economy-of-mechanism: A second control is ...
order unix-composability over defense-in-depth: Each layer is a separate ...
```

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
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
set or an `inherit.paths` file, since which records a repository rules by is
that repository's own declaration. `uphold schema` describes the table.
49 changes: 49 additions & 0 deletions docs/schema/principles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,29 @@
},
"type": "object"
},
"Decide": {
"additionalProperties": false,
"description": "`[decide]` in a repository's own policy: the records its design questions\nare answered from, and which side it takes where two of them conflict.\n\nThe shape is refused here, at load. Whether each id is in the catalog and\nwhether every adopted conflict carries an order are questions about the\ncatalog this binary carries, and `uphold check` answers them at exit 1.",
"properties": {
"adopt": {
"default": [],
"description": "Catalog record ids, each at most once.",
"items": {
"type": "string"
},
"type": "array"
},
"order": {
"default": [],
"description": "`[[decide.order]]`: one per pair of adopted records that names the\nother in `conflicts_with`.",
"items": {
"$ref": "#/$defs/Order"
},
"type": "array"
}
},
"type": "object"
},
"Editor": {
"description": "When an invocation that supplied no body opens an editor for one.\n\nA closed value rather than a flag list or an expression, because the\nquestion has three answers and the commands this tool stands in front of\ngive exactly those three. The answer decides two things that used to be\none: whether the shim stands in as the editor, and whether it SAYS the\neditor is the checkpoint -- which it said for `gh pr merge --squash`, where\nno editor ever opens and the forge composed the merge message unread.",
"oneOf": [
Expand Down Expand Up @@ -175,6 +198,28 @@
},
"type": "object"
},
"Order": {
"additionalProperties": false,
"description": "One `[[decide.order]]`: where `prefer` and `over` disagree, the answer\nis `prefer`'s, for the reason `why` gives.",
"properties": {
"over": {
"type": "string"
},
"prefer": {
"type": "string"
},
"why": {
"description": "Read by whoever reviews the order; an order with no reason is refused.",
"type": "string"
}
},
"required": [
"prefer",
"over",
"why"
],
"type": "object"
},
"OverrideCommand": {
"additionalProperties": false,
"description": "The `command` keys of an [`OverrideShape`].",
Expand Down Expand Up @@ -849,6 +894,10 @@
"description": "Whether every path-baseline entry must say who excused it and why.\n\nOff by default, and the default is not neutrality -- it is what every\nexisting baseline file already is. A repository turns it on when its\nbaselines stop being one homogeneous debt with a header explaining all\nof it, and start holding entries that differ from each other. The\n`.ljust(` case is the shape: one call site should take the dependency\nand another is correct as it stands, and no pattern separates them, so\nthe entry that excuses the second has to carry the judgement or the\njudgement is nowhere.\n\nPolicy-level rather than per-rule, because \"may an exemption be\nanonymous\" is one answer a repository gives once. Per-rule it would be a\nsetting every new baseline has to remember, which is the same as not\nhaving it.",
"type": "boolean"
},
"decide": {
"$ref": "#/$defs/Decide",
"description": "`[decide]`: the records this repository's design questions are\nanswered from. See [`Decide`]."
},
"foreign_hosts": {
"default": [],
"description": "The hosts this repository's documents cite that carry no repository the\nprivate-name guards need an answer about.\n\nA policy field rather than a rule one for the reason\n`private_owners_from` is: a rule arriving from a bundled set cannot be\nhanded a parameter, and which hosts a repository's own bibliography\nnames is a property of the repository rather than of one rule in it.\n\nHost globs, matched case-insensitively -- `\"doi.org\"`, `\"*.sr.ht\"`. What\nis NOT named here and is not GitHub is could-not-look and exits 2,\nbecause the tool cannot tell whether `git.acme.example/acme/secret` is\nprivate and silence would read as clean.\n\nExtends [`FOREIGN_HOSTS_DEFAULT`] rather than replacing it: a host that\nserves no `owner/repo` path for anybody is not a fact about this\nrepository, and the fleet had said so in 84 identical lines.",
Expand Down
4 changes: 4 additions & 0 deletions src/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,10 @@ pub(crate) struct Record {
pub status: String,
#[serde(default)]
pub enforcement: Enforcement,
/// Symmetric, as `scripts/validate.py` holds it: an adopted pair in it is
/// a pair `[decide]` must order.
#[serde(default)]
pub conflicts_with: Vec<String>,
}

#[derive(Debug, Clone, Default, Deserialize)]
Expand Down
133 changes: 130 additions & 3 deletions src/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use std::path::Path;
use serde::{Deserialize, Serialize};

use crate::catalog;
use crate::config::Policy;
use crate::config::{Decide, Policy};
use crate::error::{Exit, Fatal, Result, verdict};

const DECLARATION: &str = "policy/upheld.toml";
Expand Down Expand Up @@ -732,8 +732,14 @@ pub(crate) struct Report {
violations: Vec<String>,
unavailable: Vec<String>,
notes: Vec<String>,
/// The policy's `[decide]`, as written, so the review of what a
/// repository rules by is this one report.
decide: Decide,
#[serde(skip)]
misplaced_count: usize,
/// How many of `violations`, at the end, are refusals of `[decide]`.
#[serde(skip)]
decide_count: usize,
#[serde(skip)]
fatal: Option<String>,
}
Expand Down Expand Up @@ -808,6 +814,7 @@ pub(crate) fn run(
return report_coverage(policy, &declaration, &installed, &supply);
}
let mut report = evaluate(&path, &declaration, &installed, &supply)?;
decided(&mut report, &policy.decide)?;
let measured = probe_revision
.map(|revision| revision.and_then(|revision| crate::probe::measure(root, &revision)));
for claim in &mut report.claims {
Expand Down Expand Up @@ -956,7 +963,9 @@ fn evaluate(
violations: installed.misplaced.clone(),
unavailable: installed.unreadable.clone(),
notes: installed.how.clone(),
decide: Decide::default(),
misplaced_count: installed.misplaced.len(),
decide_count: 0,
fatal: None,
};
for (index, (principle, rule)) in claims.into_iter().enumerate() {
Expand Down Expand Up @@ -1020,9 +1029,21 @@ fn report_human(report: &Report) -> Result<Exit> {
eprintln!("- {misplaced}");
}
}
if report.violations.len() > report.misplaced_count {
let claims_end = report.violations.len().saturating_sub(report.decide_count);
if claims_end > report.misplaced_count {
eprintln!("enforcement claims refused ({DECLARATION}):");
for failure in report.violations.iter().skip(report.misplaced_count) {
for failure in report
.violations
.iter()
.take(claims_end)
.skip(report.misplaced_count)
{
eprintln!("- {failure}");
}
}
if report.decide_count > 0 {
eprintln!("design rulings refused ([decide]):");
for failure in report.violations.iter().skip(claims_end) {
eprintln!("- {failure}");
}
}
Expand Down Expand Up @@ -1064,9 +1085,115 @@ fn report_human(report: &Report) -> Result<Exit> {
for note in &report.notes {
println!(" note {note}");
}
if !report.decide.adopt.is_empty() {
println!(
"adopted {} record(s) for design rulings ([decide]):",
report.decide.adopt.len()
);
for id in &report.decide.adopt {
println!(" {id}");
}
for order in &report.decide.order {
println!(
" order {} over {}: {}",
order.prefer, order.over, order.why
);
}
}
Ok(report.verdict.exit())
}

/// Hold `[decide]` to the catalog this binary carries, into `report`.
///
/// Every id must be a live record, 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
/// records that do not conflict, or that this repository does not adopt,
/// decides nothing and reads as though it did.
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 {
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);
}
}
}
let conflicts: BTreeSet<(&str, &str)> = adopted
.iter()
.flat_map(|(id, record)| {
record
.conflicts_with
.iter()
.filter(|other| adopted.contains_key(other.as_str()))
.map(move |other| pair(id, other))
})
.collect();
let mut ordered: BTreeMap<(&str, &str), usize> = BTreeMap::new();
for (index, order) in decide.order.iter().enumerate() {
let at = format!("order[{index}] ({} over {})", order.prefer, order.over);
let unadopted: Vec<&str> = [order.prefer.as_str(), order.over.as_str()]
.into_iter()
.filter(|id| !adopted.contains_key(id))
.collect();
if !unadopted.is_empty() {
failures.push(format!(
"{at}: orders {}, which this repository does not adopt",
unadopted.join(" and ")
));
continue;
}
let key = pair(&order.prefer, &order.over);
if !conflicts.contains(&key) {
failures.push(format!(
"{at}: the two records do not name each other in conflicts_with, so there \
is no conflict for an order to settle"
));
continue;
}
if let Some(first) = ordered.insert(key, index) {
failures.push(format!(
"{at}: order[{first}] already orders this pair; one pair takes one order"
));
}
}
for (left, right) in &conflicts {
if !ordered.contains_key(&(*left, *right)) {
failures.push(format!(
"{left} and {right} are both adopted and conflict, and no [[decide.order]] \
says which this repository prefers. Add one with prefer, over and why"
));
}
}
report.decide = decide.clone();
report.decide_count = failures.len();
report.violations.extend(failures);
if report.fatal.is_none() {
report.verdict = match verdict(report.violations.len(), report.unavailable.len()) {
Exit::Clean => Verdict::Clean,
Exit::Violations => Verdict::Violations,
Exit::Broken => Verdict::CouldNotLook,
};
}
Ok(())
}

/// A conflict's two ends in one order, since `conflicts_with` is symmetric.
fn pair<'a>(left: &'a str, right: &'a str) -> (&'a str, &'a str) {
if left <= right {
(left, right)
} else {
(right, left)
}
}

/// The denominator the reconcile cannot see: rules that run and claim nothing.
///
/// It reports and does not refuse. A mode that failed a build over an unclaimed
Expand Down
Loading
Loading