diff --git a/CHANGELOG.md b/CHANGELOG.md index 1efa34f9..8d1f7cd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -202,6 +202,32 @@ release notes. ### Tools +- `deed review --before --after ` gives a patch an evidence + receipt. It checks both module sets through the same import and shipped-module + path as `deed check`, then reports effect-row entries added to each stable + `module/declaration` identity and obligations whose tier regressed, such as + `proven -> guarded`. A newly added effectful function or handler counts as new authority, + imported effects retain the module that declared them in their identity, and + a newly introduced Guarded obligation is a third, separate kind of evidence. + + Human output is for review; `--format json` writes one `review_receipt` object + for an agent or CI job. Findings remain informational by default. Three + independent policies turn them into an exit-one gate: + `--deny-new-authority`, `--deny-weaker-promises` and `--deny-new-guarded`. + The receipt is still written, with a policy verdict in human or JSON form. + Compiler or manifest errors on either side also exit one but produce no + receipt, because comparing evidence the compiler could not establish would + make the report look stronger than it is. + + `deed mcp` exposes the same evidence as a seventh tool, `deed_review`. Its + `before` and `after` arguments are arrays of module source texts, so imports + resolve within each side without giving the server a filesystem. Its optional + policy object maps the three CLI gates to `denyNewAuthority`, + `denyWeakerPromises` and `denyNewGuarded`. A failed policy is a successful + tool call carrying `policy.passed: false`; malformed arguments remain + protocol errors, and a side that does not check returns `review_refused` + instead of a partial receipt. + - A list of numbers crosses a component boundary. `deed build --component` refused any export carrying a list; `List` now goes both ways, and a component runtime reads it as `list`: diff --git a/README.md b/README.md index 82721637..aa1f3ca2 100644 --- a/README.md +++ b/README.md @@ -153,7 +153,35 @@ an editor does rather than scraping them out of terminal output. It holds no capability: a program arrives as text, the answer leaves as text, and a program whose row reaches a file is refused before it runs. [how-to/let-an-agent-use-the-compiler.md](how-to/let-an-agent-use-the-compiler.md) -has the six tools and the one line worth reading. +has the tool reference and the one line worth reading. + +## Every agent patch comes with a receipt + +`deed review` compares the checked module set before a change with the one +after it. The first receipt names newly declared authority and obligations +that fell to a weaker contract tier: + +``` +$ deed review --before old/src --after src +receipt: review required +authority added (1) + + billing/transfer/settle: Audit.note +obligation tier regressions (1) + ! billing/transfer/settle: Positive proven -> guarded +guarded obligations added (0) +``` + +The paths may name files or directories, and imports are resolved independently +on each side. `--format json` emits the same receipt as one stable object for an +agent or CI job. Findings are informational unless a policy turns them into a +gate: `--deny-new-authority`, `--deny-weaker-promises` and +`--deny-new-guarded` each exit one when its own evidence is present, while still +printing the receipt and a policy verdict. A side that does not check is refused +rather than compared. + +The same evidence is available to an agent as `deed_review`. It takes the +before and after module sets as arrays of source text, resolves imports within +each array, and applies the same three policies without opening a file. ## Demo diff --git a/crates/deed-cli/src/args.rs b/crates/deed-cli/src/args.rs index b4407908..61c7c151 100644 --- a/crates/deed-cli/src/args.rs +++ b/crates/deed-cli/src/args.rs @@ -12,6 +12,7 @@ deed, a contract-first language Usage: deed new deed check [options] ... + deed review --before ... --after ... [options] deed test [options] ... deed run [options] ... [-- ...] deed build [options] ... @@ -25,6 +26,11 @@ Usage: Options: --format How to print diagnostics. Default: human. + --before A file or directory before the change. Repeatable. + --after A file or directory after the change. Repeatable. + --deny-new-authority With `review`, fail if authority was added. + --deny-weaker-promises With `review`, fail if an obligation's tier regressed. + --deny-new-guarded With `review`, fail if a new Guarded obligation appeared. --obligations Report which tier each refinement obligation landed in. --timings Report how long each pass took. --profile-runtime With `run`, report where runtime went. @@ -75,6 +81,10 @@ and a new project has none. `deed test --compiled` runs test blocks and the properties contracts generate through the compiled backend. Blocks the backend cannot compile are skipped and named, so the summary says both how much ran and what did not. +`deed review` compares two checked module sets. Its receipt names authority +added to effect rows and refinement obligations that moved to a weaker tier. +Findings are informational unless a `--deny-*` policy is enabled. Code that +does not check is refused before the two sets are compared. `deed run` calls `main`, handing it the one `System` capability there is. Everything after `--` goes to the program, which reads it with `Io.args`. Standard input is read when, and only when, `main`'s row says `Io.line`. A @@ -198,9 +208,21 @@ pub struct CheckArgs { pub locked: Option, } +#[derive(Debug)] +pub struct ReviewArgs { + pub before: Vec, + pub after: Vec, + pub format: Format, + pub deny_new_authority: bool, + pub deny_weaker_promises: bool, + pub deny_new_guarded: bool, +} + #[derive(Debug)] pub enum Command { Check(CheckArgs), + /// Compare the authority and contract evidence in two module sets. + Review(ReviewArgs), /// Write a new project into a directory of this name. New(String), /// Print the page for one diagnostic code. @@ -284,6 +306,7 @@ pub fn parse>(mut args: I) -> Result (Some(name), None) => Ok(Command::New(name)), }; } + "review" => return parse_review(args), "check" => Mode::Check, "test" => Mode::Test, "run" => Mode::Run, @@ -293,7 +316,7 @@ pub fn parse>(mut args: I) -> Result "doc" => Mode::Doc, other => { return Err(format!( - "unknown command `{other}`, the choices are `new`, `check`, `test`, `run`, `build`, `doc`, `fmt`, `fix`, `explain`, `lsp`, `debug` and `mcp`" + "unknown command `{other}`, the choices are `new`, `check`, `review`, `test`, `run`, `build`, `doc`, `fmt`, `fix`, `explain`, `lsp`, `debug` and `mcp`" )); } }; @@ -459,6 +482,70 @@ pub fn parse>(mut args: I) -> Result })) } +fn parse_review>(mut args: I) -> Result { + let mut before = Vec::new(); + let mut after = Vec::new(); + let mut format = Format::Human; + let mut deny_new_authority = false; + let mut deny_weaker_promises = false; + let mut deny_new_guarded = false; + + while let Some(arg) = args.next() { + match arg.as_str() { + "-h" | "--help" => return Ok(Command::Help), + "--deny-new-authority" => deny_new_authority = true, + "--deny-weaker-promises" => deny_weaker_promises = true, + "--deny-new-guarded" => deny_new_guarded = true, + "--before" => before.push(PathBuf::from( + args.next() + .ok_or_else(|| "`--before` needs a path".to_string())?, + )), + other if other.starts_with("--before=") => { + before.push(PathBuf::from(&other["--before=".len()..])); + } + "--after" => after.push(PathBuf::from( + args.next() + .ok_or_else(|| "`--after` needs a path".to_string())?, + )), + other if other.starts_with("--after=") => { + after.push(PathBuf::from(&other["--after=".len()..])); + } + "--format" => { + let value = args + .next() + .ok_or_else(|| "`--format` needs a value, `human` or `json`".to_string())?; + format = parse_format(&value)?; + } + other if other.starts_with("--format=") => { + format = parse_format(&other["--format=".len()..])?; + } + other if other.starts_with('-') => { + return Err(format!("unknown option `{other}`")); + } + path => { + return Err(format!( + "review path `{path}` must follow `--before` or `--after`" + )); + } + } + } + + if before.is_empty() || after.is_empty() { + return Err( + "`deed review` needs at least one `--before` and one `--after` path".to_string(), + ); + } + + Ok(Command::Review(ReviewArgs { + before, + after, + format, + deny_new_authority, + deny_weaker_promises, + deny_new_guarded, + })) +} + fn parse_format(value: &str) -> Result { match value { "human" => Ok(Format::Human), @@ -495,6 +582,53 @@ mod tests { assert_eq!(check.mode, Mode::Test); } + #[test] + fn review_has_explicit_repeatable_sides_a_format_and_policies() { + let Ok(Command::Review(review)) = parse(args(&[ + "review", + "--before", + "old/a.deed", + "--before=old/b.deed", + "--after=new", + "--format=json", + "--deny-new-authority", + "--deny-weaker-promises", + "--deny-new-guarded", + ])) else { + panic!("should parse"); + }; + assert_eq!(review.before.len(), 2); + assert_eq!(review.after, vec![std::path::PathBuf::from("new")]); + assert_eq!(review.format, Format::Json); + assert!(review.deny_new_authority); + assert!(review.deny_weaker_promises); + assert!(review.deny_new_guarded); + } + + #[test] + fn review_needs_both_sides_and_labels_every_path() { + let missing = parse(args(&["review", "--before", "old"])).unwrap_err(); + assert!(missing.contains("one `--after`"), "{missing}"); + + let unlabelled = parse(args(&["review", "old", "--after", "new"])).unwrap_err(); + assert!(unlabelled.contains("must follow"), "{unlabelled}"); + } + + #[test] + fn review_distinguishes_an_unknown_option_from_an_unlabelled_path() { + let unknown = parse(args(&[ + "review", "--before", "old", "--after", "new", "--strict", + ])) + .unwrap_err(); + assert_eq!(unknown, "unknown option `--strict`"); + + let unlabelled = parse(args(&[ + "review", "--before", "old", "--after", "new", "extra", + ])) + .unwrap_err(); + assert!(unlabelled.contains("must follow"), "{unlabelled}"); + } + #[test] fn lsp_is_a_command_and_takes_nothing_else() { assert!(matches!(parse(args(&["lsp"])), Ok(Command::Lsp))); diff --git a/crates/deed-cli/src/main.rs b/crates/deed-cli/src/main.rs index 780b13c9..d81453e6 100644 --- a/crates/deed-cli/src/main.rs +++ b/crates/deed-cli/src/main.rs @@ -12,11 +12,12 @@ use std::process::ExitCode; use deed_ast::Item; use deed_diagnostics::{Diagnostic, FileId, SourceMap, render_human}; +use deed_driver::review::{PolicyVerdict, ReviewPolicy, ReviewReceipt}; use deed_driver::{Checked, ObligationReport}; use deed_interp::{PropertyAttempt, PropertyConfig, PropertyInterpreter, RuntimeProfile}; use deed_typeck::Tier; -use crate::args::{CheckArgs, Command, Format, Mode, USAGE}; +use crate::args::{CheckArgs, Command, Format, Mode, ReviewArgs, USAGE}; /// Something went wrong with the invocation rather than with the code. const EXIT_USAGE: u8 = 2; @@ -65,12 +66,172 @@ fn run() -> ExitCode { Command::Explain(code) => run_explain(&code), Command::New(name) => run_new(&name), Command::Check(check) => run_check(check), + Command::Review(review) => run_review(review), Command::Lsp => run_lsp(), Command::Debug => run_debug(), Command::Mcp => run_mcp(), } } +fn run_review(args: ReviewArgs) -> ExitCode { + let before = match load_review_side(&args.before) { + Ok(checked) => checked, + Err(error) => { + eprintln!("error: before: {error}"); + return ExitCode::from(EXIT_USAGE); + } + }; + let after = match load_review_side(&args.after) { + Ok(checked) => checked, + Err(error) => { + eprintln!("error: after: {error}"); + return ExitCode::from(EXIT_USAGE); + } + }; + + let before_invalid = report_review_diagnostics("before", &before); + let after_invalid = report_review_diagnostics("after", &after); + if before_invalid || after_invalid { + return ExitCode::FAILURE; + } + + let receipt = ReviewReceipt::between(&before.checks, &after.checks); + let policy = ReviewPolicy { + deny_new_authority: args.deny_new_authority, + deny_weaker_promises: args.deny_weaker_promises, + deny_new_guarded: args.deny_new_guarded, + }; + let verdict = receipt.evaluate(policy); + let enforced = !policy.is_empty(); + let stdout = io::stdout(); + let mut out = stdout.lock(); + let result = match args.format { + Format::Human => report_review_human(&mut out, &receipt, enforced.then_some(&verdict)), + Format::Json if enforced => writeln!(out, "{}", receipt.to_json_with_policy(&verdict)), + Format::Json => writeln!(out, "{}", receipt.to_json()), + }; + match result { + Ok(()) if verdict.passed() => ExitCode::SUCCESS, + Ok(()) => ExitCode::FAILURE, + Err(error) => { + eprintln!("error: {error}"); + ExitCode::from(EXIT_USAGE) + } + } +} + +fn load_review_side(paths: &[PathBuf]) -> Result { + let files = collect_module_files(paths)?; + let mut resolved = resolve_module_set(files)?; + check_module_set(&mut resolved) +} + +fn report_review_diagnostics(label: &str, checked: &CheckedModuleSet) -> bool { + let manifest_has_errors = checked + .manifest_diagnostics + .iter() + .flatten() + .any(Diagnostic::is_error); + let compiler_has_errors = checked + .checks + .iter() + .flat_map(|file| &file.diagnostics) + .any(Diagnostic::is_error); + + for diagnostic in checked.manifest_diagnostics.iter().flatten() { + eprintln!("{label}: {}", render_human(&checked.sources, diagnostic)); + } + for file in &checked.checks { + if file.module.name.is_none() { + eprintln!( + "error: {label}: `{}` needs a module declaration for a stable review identity", + checked.sources.file(file.file).name() + ); + } + for diagnostic in &file.diagnostics { + eprintln!("{label}: {}", render_human(&checked.sources, diagnostic)); + } + } + manifest_has_errors || compiler_has_errors +} + +fn report_review_human( + out: &mut impl Write, + receipt: &ReviewReceipt, + verdict: Option<&PolicyVerdict>, +) -> io::Result<()> { + writeln!( + out, + "receipt: {}", + if receipt.is_clean() { + "clean" + } else { + "review required" + } + )?; + writeln!(out, "authority added ({})", receipt.authority_added.len())?; + for change in &receipt.authority_added { + writeln!( + out, + " + {}/{}: {}", + change.module, change.declaration, change.authority + )?; + } + writeln!( + out, + "obligation tier regressions ({})", + receipt.tier_regressions.len() + )?; + for change in &receipt.tier_regressions { + let occurrence = match change.occurrence { + 0 => String::new(), + at => format!(" #{}", at + 1), + }; + writeln!( + out, + " ! {}/{}: {}{} {} -> {}", + change.module, + change.declaration, + change.subject, + occurrence, + change.before.name(), + change.after.name() + )?; + } + writeln!( + out, + "guarded obligations added ({})", + receipt.guarded_added.len() + )?; + for change in &receipt.guarded_added { + let occurrence = match change.occurrence { + 0 => String::new(), + at => format!(" #{}", at + 1), + }; + writeln!( + out, + " ? {}/{}: {}{}", + change.module, change.declaration, change.subject, occurrence + )?; + } + if let Some(verdict) = verdict { + writeln!( + out, + "policy: {}", + if verdict.passed() { "passed" } else { "failed" } + )?; + for violation in &verdict.violations { + writeln!( + out, + " {}: {}", + violation.rule.name(), + plural(violation.findings, "finding") + )?; + } + } + Ok(()) +} + /// Writes a new project into a directory named after it. /// /// Refuses an existing directory rather than merging into it. "New" is the @@ -203,84 +364,66 @@ fn run_mcp() -> ExitCode { } } -fn run_check(args: CheckArgs) -> ExitCode { +fn collect_module_files(paths: &[PathBuf]) -> Result, String> { let mut files = Vec::new(); - for path in &args.paths { + for path in paths { if let Err(error) = collect(path, &mut files) { - eprintln!("error: {}: {error}", path.display()); - return ExitCode::from(EXIT_USAGE); + return Err(format!("{}: {error}", path.display())); } } if files.is_empty() { - eprintln!("error: no `.deed` files found"); - return ExitCode::from(EXIT_USAGE); + return Err("no `.deed` files found".to_string()); } // Deterministic order, so output can be diffed between runs. files.sort(); files.dedup(); + Ok(files) +} - if args.mode == Mode::Fmt { - return run_fmt(&files, args.check_only); - } - if args.mode == Mode::Fix { - return run_fix(&files, args.check_only); - } - if args.mode == Mode::Doc { - return run_doc(&files); - } - if args.runtime_profile && args.mode != Mode::Run { - eprintln!("error: `--profile-runtime` is only for `deed run`"); - return ExitCode::from(EXIT_USAGE); - } +struct ResolvedModuleSet { + files: Vec, + subject: usize, + shipped: Vec<&'static str>, + manifests: Vec<(String, String, Vec)>, +} +fn resolve_module_set(mut files: Vec) -> Result { // What was named is the subject; what an import needed is context. So the // library a program uses is compiled alongside it and checked, and its // tests and its `main` are not the ones you asked about. let subject = files.len(); - let mut shipped: Vec<&'static str> = Vec::new(); - let mut manifests: Vec<(String, String, Vec)> = Vec::new(); - if let Err(error) = resolve_imports(&mut files, &mut shipped, &mut manifests) { - eprintln!("error: {error}"); - return ExitCode::from(EXIT_USAGE); - } + let mut shipped = Vec::new(); + let mut manifests = Vec::new(); + resolve_imports(&mut files, &mut shipped, &mut manifests).map_err(|error| error.to_string())?; + Ok(ResolvedModuleSet { + files, + subject, + shipped, + manifests, + }) +} - // If --locked was given, verify every input matches the recorded hash - // before touching anything. A changed or missing file exits here rather - // than producing an artifact whose provenance is unknown. - if let Some(lock_path) = &args.locked { - match lock::read(lock_path) { - Err(e) => { - eprintln!("error: {e}"); - return ExitCode::from(EXIT_USAGE); - } - Ok(entries) => { - if let Err(e) = lock::verify(&entries) { - eprintln!("error: {e}"); - return ExitCode::FAILURE; - } - } - } - } +struct CheckedModuleSet { + sources: SourceMap, + checks: Vec, + manifest_diagnostics: Vec>, +} +fn check_module_set(resolved: &mut ResolvedModuleSet) -> Result { let mut sources = SourceMap::new(); let mut ids = Vec::new(); - for path in &files { - let text = match std::fs::read_to_string(path) { - Ok(text) => text, - Err(error) => { - eprintln!("error: {}: {error}", path.display()); - return ExitCode::from(EXIT_USAGE); - } - }; + for path in &resolved.files { + let text = std::fs::read_to_string(path) + .map_err(|error| format!("{}: {error}", path.display()))?; ids.push(sources.add(display_path(path), text)); } // Last, so that the subject is still the first `subject` of them. A module // that came out of the compiler is context by definition: nobody named it. - for module in &shipped { + for module in &resolved.shipped { let Some(text) = deed_driver::shipped_source(module) else { continue; }; @@ -289,24 +432,95 @@ fn run_check(args: CheckArgs) -> ExitCode { // Register manifest files in the source map so their text appears in // diagnostics, then re-anchor each diagnostic to the registered file id. - let manifest_diagnostics: Vec> = manifests + let manifest_diagnostics = std::mem::take(&mut resolved.manifests) .into_iter() .map(|(name, text, diagnostics)| { let file = sources.add(name, text); diagnostics .into_iter() - .map(|mut d| { - d.file = file; - d + .map(|mut diagnostic| { + diagnostic.file = file; + diagnostic }) .collect() }) .collect(); - // Every file at once, so a `use` has something to point at. Checking them - // one at a time would mean an import could never resolve, which is how it - // used to work and why nothing crossing a module boundary was checked. + // Every file at once, so a `use` has something to point at. let checks = deed_driver::check_all(&sources, &ids); + Ok(CheckedModuleSet { + sources, + checks, + manifest_diagnostics, + }) +} + +fn run_check(args: CheckArgs) -> ExitCode { + let files = match collect_module_files(&args.paths) { + Ok(files) => files, + Err(error) => { + eprintln!("error: {error}"); + return ExitCode::from(EXIT_USAGE); + } + }; + + if args.mode == Mode::Fmt { + return run_fmt(&files, args.check_only); + } + if args.mode == Mode::Fix { + return run_fix(&files, args.check_only); + } + if args.mode == Mode::Doc { + return run_doc(&files); + } + if args.runtime_profile && args.mode != Mode::Run { + eprintln!("error: `--profile-runtime` is only for `deed run`"); + return ExitCode::from(EXIT_USAGE); + } + + let mut resolved = match resolve_module_set(files) { + Ok(resolved) => resolved, + Err(error) => { + eprintln!("error: {error}"); + return ExitCode::from(EXIT_USAGE); + } + }; + + // If --locked was given, verify every input matches the recorded hash + // before touching anything. A changed or missing file exits here rather + // than producing an artifact whose provenance is unknown. + if let Some(lock_path) = &args.locked { + match lock::read(lock_path) { + Err(e) => { + eprintln!("error: {e}"); + return ExitCode::from(EXIT_USAGE); + } + Ok(entries) => { + if let Err(e) = lock::verify(&entries) { + eprintln!("error: {e}"); + return ExitCode::FAILURE; + } + } + } + } + + let CheckedModuleSet { + sources, + checks, + manifest_diagnostics, + } = match check_module_set(&mut resolved) { + Ok(checked) => checked, + Err(error) => { + eprintln!("error: {error}"); + return ExitCode::from(EXIT_USAGE); + } + }; + let ResolvedModuleSet { + files, + subject, + shipped, + .. + } = resolved; let stdout = io::stdout(); let mut out = stdout.lock(); diff --git a/crates/deed-cli/tests/cli.rs b/crates/deed-cli/tests/cli.rs index 7b68d656..92fef7c0 100644 --- a/crates/deed-cli/tests/cli.rs +++ b/crates/deed-cli/tests/cli.rs @@ -120,6 +120,326 @@ impl Drop for Scratch { } } +const REVIEW_BEFORE: &str = "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + effect Store {\n\ + fn read() -> Int\n\ + fn write(value: Int) -> ()\n\ + }\n\n\ + fn sync() -> Int uses Store.read, { Store.read() }\n\n\ + fn preserve(value: Positive) -> Positive { value + 1 }\n"; + +const REVIEW_AFTER: &str = "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + effect Store {\n\ + fn read() -> Int\n\ + fn write(value: Int) -> ()\n\ + }\n\n\ + fn sync() -> Int\n\ + uses Store.read, Store.write,\n\ + {\n\ + let value = Store.read()\n\ + Store.write(value)\n\ + value\n\ + }\n\n\ + fn preserve(value: Int) -> Positive { value + 1 }\n"; + +fn review_fixture(scratch: &Scratch) -> (PathBuf, PathBuf) { + scratch.write("before/review/sample.deed", REVIEW_BEFORE); + scratch.write("after/review/sample.deed", REVIEW_AFTER); + (scratch.path().join("before"), scratch.path().join("after")) +} + +#[test] +fn review_receipts_authority_and_a_weaker_obligation() { + let scratch = Scratch::new("review-human"); + let (before, after) = review_fixture(&scratch); + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + ]); + + assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); + let text = stdout(&output); + assert!(text.contains("receipt: review required"), "{text}"); + assert!(text.contains("+ review/sample/sync: Store.write"), "{text}"); + assert!( + text.contains("! review/sample/preserve: Positive proven -> guarded"), + "{text}" + ); +} + +#[test] +fn review_json_is_one_stable_receipt_object() { + let scratch = Scratch::new("review-json"); + let (before, after) = review_fixture(&scratch); + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + "--format=json", + ]); + + assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); + assert_eq!( + stdout(&output).trim(), + "{\"kind\":\"review_receipt\",\"clean\":false,\"authority_added\":[{\"module\":\"review/sample\",\"declaration\":\"sync\",\"authority\":\"Store.write\"}],\"tier_regressions\":[{\"module\":\"review/sample\",\"declaration\":\"preserve\",\"subject\":\"Positive\",\"occurrence\":0,\"before\":\"proven\",\"after\":\"guarded\"}],\"guarded_added\":[]}" + ); +} + +#[test] +fn an_unchanged_review_is_clean() { + let scratch = Scratch::new("review-clean"); + let (before, _) = review_fixture(&scratch); + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + before.to_str().unwrap(), + ]); + + assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); + assert_eq!( + stdout(&output), + "receipt: clean\nauthority added (0)\nobligation tier regressions (0)\nguarded obligations added (0)\n" + ); +} + +#[test] +fn review_policy_can_deny_authority_and_weaker_promises_independently() { + let scratch = Scratch::new("review-policy"); + let (before, after) = review_fixture(&scratch); + + for rule in ["--deny-new-authority", "--deny-weaker-promises"] { + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + rule, + ]); + assert_eq!(code(&output), 1, "{rule}: {}", stdout(&output)); + let text = stdout(&output); + assert!(text.contains("policy: failed"), "{rule}: {text}"); + assert!( + text.contains(&format!("{}: 1 finding", rule.trim_start_matches("--"))), + "{text}" + ); + } +} + +#[test] +fn review_policy_denies_a_new_guarded_obligation() { + let scratch = Scratch::new("review-new-guarded"); + let before = scratch.write( + "before.deed", + "module review/sample\n\ntype Positive = Int where value > 0\n", + ); + let after = scratch.write( + "after.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn accept(value: Int) -> Positive { value }\n", + ); + + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + "--deny-new-guarded", + ]); + assert_eq!(code(&output), 1, "{}{}", stdout(&output), stderr(&output)); + let text = stdout(&output); + assert!(text.contains("? review/sample/accept: Positive"), "{text}"); + assert!(text.contains("deny-new-guarded: 1 finding"), "{text}"); +} + +#[test] +fn review_policy_passes_an_unchanged_patch() { + let scratch = Scratch::new("review-policy-clean"); + let (before, _) = review_fixture(&scratch); + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + before.to_str().unwrap(), + "--deny-new-authority", + "--deny-weaker-promises", + "--deny-new-guarded", + ]); + + assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); + assert!( + stdout(&output).ends_with("policy: passed\n"), + "{}", + stdout(&output) + ); +} + +#[test] +fn review_json_carries_a_failed_policy_verdict() { + let scratch = Scratch::new("review-policy-json"); + let (before, after) = review_fixture(&scratch); + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + "--deny-weaker-promises", + "--format=json", + ]); + + assert_eq!(code(&output), 1, "{}{}", stdout(&output), stderr(&output)); + assert!( + stdout(&output).trim().ends_with( + "\"policy\":{\"passed\":false,\"violations\":[{\"rule\":\"deny-weaker-promises\",\"findings\":1}]}}" + ), + "{}", + stdout(&output) + ); +} + +#[test] +fn review_resolves_imported_effect_authority_on_each_side() { + let scratch = Scratch::new("review-import"); + let effect = "module services/audit\n\neffect Audit { fn note(value: Int) -> () }\n"; + scratch.write("before/services/audit.deed", effect); + scratch.write("after/services/audit.deed", effect); + let before = scratch.write( + "before/app/main.deed", + "module app/main\n\nuse services/audit.{Audit}\n\nfn work() -> () { () }\n", + ); + let after = scratch.write( + "after/app/main.deed", + "module app/main\n\nuse services/audit.{Audit}\n\n\ + fn work() -> () uses Audit.note, { Audit.note(1) }\n", + ); + + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + ]); + assert_eq!(code(&output), 0, "{}{}", stdout(&output), stderr(&output)); + assert!( + stdout(&output).contains("services/audit/Audit.note"), + "{}", + stdout(&output) + ); +} + +#[test] +fn review_refuses_to_compare_a_side_that_does_not_check() { + let scratch = Scratch::new("review-broken"); + let before = scratch.write( + "before.deed", + "module review/sample\n\nfn okay() -> Int { 1 }\n", + ); + let after = scratch.write( + "after.deed", + "module review/sample\n\nfn broken() -> Missing { 1 }\n", + ); + + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + ]); + assert_eq!(code(&output), 1, "{}{}", stdout(&output), stderr(&output)); + assert!(stdout(&output).is_empty(), "{}", stdout(&output)); + assert!( + stderr(&output).contains("after: error"), + "{}", + stderr(&output) + ); +} + +#[test] +fn review_reports_both_sides_when_both_do_not_check() { + let scratch = Scratch::new("review-both-broken"); + let before = scratch.write( + "before.deed", + "module review/sample\n\nfn broken() -> BeforeMissing { 1 }\n", + ); + let after = scratch.write( + "after.deed", + "module review/sample\n\nfn broken() -> AfterMissing { 1 }\n", + ); + + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + ]); + assert_eq!(code(&output), 1, "{}{}", stdout(&output), stderr(&output)); + assert!(stdout(&output).is_empty(), "{}", stdout(&output)); + let errors = stderr(&output); + assert!(errors.contains("before: error"), "{errors}"); + assert!(errors.contains("after: error"), "{errors}"); +} + +#[test] +fn review_refuses_an_anonymous_module() { + let scratch = Scratch::new("review-anonymous"); + let before = scratch.write("before.deed", "fn work() -> Int { 1 }\n"); + let after = scratch.write( + "after.deed", + "module review/sample\n\nfn work() -> Int { 1 }\n", + ); + + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + ]); + assert_eq!(code(&output), 1, "{}{}", stdout(&output), stderr(&output)); + assert!( + stderr(&output).contains("needs a module declaration"), + "{}", + stderr(&output) + ); +} + +#[test] +fn review_refuses_manifest_errors() { + let scratch = Scratch::new("review-manifest"); + let before = scratch.write("before/app.deed", "module app\n\nfn work() -> Int { 1 }\n"); + scratch.write("before/deed.manifest", "not a manifest directive\n"); + let after = scratch.write("after/app.deed", "module app\n\nfn work() -> Int { 1 }\n"); + + let output = run(&[ + "review", + "--before", + before.to_str().unwrap(), + "--after", + after.to_str().unwrap(), + ]); + assert_eq!(code(&output), 1, "{}{}", stdout(&output), stderr(&output)); + assert!(stdout(&output).is_empty(), "{}", stdout(&output)); + let errors = stderr(&output); + assert!(errors.contains("before: error"), "{errors}"); + assert!(errors.contains("DEED7001"), "{errors}"); +} + // -- the environment -------------------------------------------------------- /// A `main` that reports one variable, granted or not. @@ -636,6 +956,9 @@ fn help_and_version_succeed() { assert!(stdout(&help).contains("Usage:")); assert!(stdout(&help).contains("--obligations")); assert!(stdout(&help).contains("--profile-runtime")); + assert!(stdout(&help).contains("--deny-new-authority")); + assert!(stdout(&help).contains("--deny-weaker-promises")); + assert!(stdout(&help).contains("--deny-new-guarded")); let version = run(&["--version"]); assert_eq!(code(&version), 0); diff --git a/crates/deed-diagnostics/src/span.rs b/crates/deed-diagnostics/src/span.rs index ad863855..5c10e792 100644 --- a/crates/deed-diagnostics/src/span.rs +++ b/crates/deed-diagnostics/src/span.rs @@ -47,6 +47,10 @@ impl Span { offset >= self.start && offset < self.end } + pub fn contains_span(self, other: Span) -> bool { + other.start >= self.start && other.end <= self.end + } + pub fn as_range(self) -> std::ops::Range { self.start as usize..self.end as usize } @@ -71,6 +75,20 @@ mod tests { assert!(!s.contains(4)); } + #[test] + fn a_span_contains_itself_and_spans_strictly_inside_it() { + let outer = Span::new(2, 8); + assert!(outer.contains_span(outer)); + assert!(outer.contains_span(Span::new(3, 7))); + } + + #[test] + fn a_span_does_not_contain_one_reaching_past_either_edge() { + let outer = Span::new(2, 8); + assert!(!outer.contains_span(Span::new(1, 7))); + assert!(!outer.contains_span(Span::new(3, 9))); + } + #[test] fn end_is_exclusive() { let s = Span::new(1, 3); diff --git a/crates/deed-driver/src/lib.rs b/crates/deed-driver/src/lib.rs index f9c96e24..feca362a 100644 --- a/crates/deed-driver/src/lib.rs +++ b/crates/deed-driver/src/lib.rs @@ -26,6 +26,7 @@ mod library; pub mod manifest; pub mod program_gen; mod report; +pub mod review; mod rows; mod shipped; pub mod wit; diff --git a/crates/deed-driver/src/review.rs b/crates/deed-driver/src/review.rs new file mode 100644 index 00000000..ba59498a --- /dev/null +++ b/crates/deed-driver/src/review.rs @@ -0,0 +1,378 @@ +//! What changed in the parts of a Deed module a reviewer has to trust. + +use std::collections::{BTreeMap, BTreeSet, HashMap}; + +use deed_ast::Item; +use deed_diagnostics::json_string; +use deed_resolve::{ExportKind, Exports, RowEntry}; +use deed_typeck::Tier; + +use crate::{Checked, ObligationReport}; + +#[derive(Clone, PartialEq, Eq, Debug)] +pub struct AuthorityChange { + pub module: String, + pub declaration: String, + pub authority: String, +} + +#[derive(Clone, PartialEq, Eq, Debug)] +pub struct TierRegression { + pub module: String, + pub declaration: String, + pub subject: String, + pub occurrence: usize, + pub before: Tier, + pub after: Tier, +} + +#[derive(Clone, PartialEq, Eq, Debug)] +pub struct GuardedAddition { + pub module: String, + pub declaration: String, + pub subject: String, + pub occurrence: usize, +} + +#[derive(Clone, Copy, Default, PartialEq, Eq, Debug)] +pub struct ReviewPolicy { + pub deny_new_authority: bool, + pub deny_weaker_promises: bool, + pub deny_new_guarded: bool, +} + +impl ReviewPolicy { + pub fn is_empty(self) -> bool { + !self.deny_new_authority && !self.deny_weaker_promises && !self.deny_new_guarded + } +} + +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum PolicyRule { + NewAuthority, + WeakerPromises, + NewGuarded, +} + +impl PolicyRule { + pub fn name(self) -> &'static str { + match self { + Self::NewAuthority => "deny-new-authority", + Self::WeakerPromises => "deny-weaker-promises", + Self::NewGuarded => "deny-new-guarded", + } + } +} + +#[derive(Clone, PartialEq, Eq, Debug)] +pub struct PolicyViolation { + pub rule: PolicyRule, + pub findings: usize, +} + +#[derive(Default, PartialEq, Eq, Debug)] +pub struct PolicyVerdict { + pub violations: Vec, +} + +impl PolicyVerdict { + pub fn passed(&self) -> bool { + self.violations.is_empty() + } + + pub fn to_json(&self) -> String { + let violations = self + .violations + .iter() + .map(|violation| { + format!( + "{{\"rule\":{},\"findings\":{}}}", + json_string(violation.rule.name()), + violation.findings + ) + }) + .collect::>() + .join(","); + format!( + "{{\"passed\":{},\"violations\":[{violations}]}}", + self.passed() + ) + } +} + +#[derive(Default, Debug)] +pub struct ReviewReceipt { + pub authority_added: Vec, + pub tier_regressions: Vec, + pub guarded_added: Vec, +} + +impl ReviewReceipt { + pub fn between(before: &[Checked], after: &[Checked]) -> Self { + let old = snapshot(before); + let new = snapshot(after); + let mut receipt = Self::default(); + + for (key, function) in &new { + let previous_authority = old.get(key).map(|entry| &entry.authority); + for entry in function.authority.iter().filter(|entry| { + previous_authority + .is_none_or(|previous| !previous.iter().any(|old| covers(old, entry))) + }) { + receipt.authority_added.push(AuthorityChange { + module: key.0.clone(), + declaration: key.1.clone(), + authority: authority(entry, &key.0), + }); + } + compare_obligations(key, old.get(key), function, &mut receipt); + } + + receipt + } + + pub fn is_clean(&self) -> bool { + self.authority_added.is_empty() + && self.tier_regressions.is_empty() + && self.guarded_added.is_empty() + } + + pub fn evaluate(&self, policy: ReviewPolicy) -> PolicyVerdict { + let mut violations = Vec::new(); + if policy.deny_new_authority && !self.authority_added.is_empty() { + violations.push(PolicyViolation { + rule: PolicyRule::NewAuthority, + findings: self.authority_added.len(), + }); + } + if policy.deny_weaker_promises && !self.tier_regressions.is_empty() { + violations.push(PolicyViolation { + rule: PolicyRule::WeakerPromises, + findings: self.tier_regressions.len(), + }); + } + if policy.deny_new_guarded && !self.guarded_added.is_empty() { + violations.push(PolicyViolation { + rule: PolicyRule::NewGuarded, + findings: self.guarded_added.len(), + }); + } + PolicyVerdict { violations } + } + + pub fn to_json(&self) -> String { + format!("{{{}}}", self.json_fields()) + } + + pub fn to_json_with_policy(&self, verdict: &PolicyVerdict) -> String { + format!( + "{{{},\"policy\":{}}}", + self.json_fields(), + verdict.to_json() + ) + } + + fn json_fields(&self) -> String { + let authority = self + .authority_added + .iter() + .map(|change| { + format!( + "{{\"module\":{},\"declaration\":{},\"authority\":{}}}", + json_string(&change.module), + json_string(&change.declaration), + json_string(&change.authority) + ) + }) + .collect::>() + .join(","); + let regressions = self + .tier_regressions + .iter() + .map(|change| { + format!( + "{{\"module\":{},\"declaration\":{},\"subject\":{},\"occurrence\":{},\"before\":{},\"after\":{}}}", + json_string(&change.module), + json_string(&change.declaration), + json_string(&change.subject), + change.occurrence, + json_string(change.before.name()), + json_string(change.after.name()) + ) + }) + .collect::>() + .join(","); + let guarded = self + .guarded_added + .iter() + .map(|change| { + format!( + "{{\"module\":{},\"declaration\":{},\"subject\":{},\"occurrence\":{}}}", + json_string(&change.module), + json_string(&change.declaration), + json_string(&change.subject), + change.occurrence + ) + }) + .collect::>() + .join(","); + format!( + "\"kind\":\"review_receipt\",\"clean\":{},\"authority_added\":[{authority}],\"tier_regressions\":[{regressions}],\"guarded_added\":[{guarded}]", + self.is_clean() + ) + } +} + +fn compare_obligations( + key: &(String, String), + before: Option<&DeclarationSnapshot>, + after: &DeclarationSnapshot, + receipt: &mut ReviewReceipt, +) { + let mut by_subject: BTreeMap<&str, Vec<(usize, Tier)>> = BTreeMap::new(); + for ((subject, occurrence), tier) in &after.obligations { + by_subject + .entry(subject) + .or_default() + .push((*occurrence, *tier)); + } + + for (subject, mut after) in by_subject { + let mut before = before + .into_iter() + .flat_map(|function| &function.obligations) + .filter(|((old_subject, _), _)| old_subject == subject) + .map(|(_, tier)| *tier) + .collect::>(); + + // Written order is not identity. Match evidence that stayed at the + // same tier first, so moving two calls cannot manufacture a change. + after.retain(|(_, tier)| { + let Some(at) = before.iter().position(|old| old == tier) else { + return true; + }; + before.remove(at); + false + }); + before.sort_by_key(|tier| tier_rank(*tier)); + after.sort_by_key(|(_, tier)| tier_rank(*tier)); + + let paired = before.len().min(after.len()); + for (before, (occurrence, after)) in before.iter().zip(&after).take(paired) { + if matches!( + (*before, *after), + (Tier::Proven, Tier::Tested | Tier::Guarded) | (Tier::Tested, Tier::Guarded) + ) { + receipt.tier_regressions.push(TierRegression { + module: key.0.clone(), + declaration: key.1.clone(), + subject: subject.to_string(), + occurrence: *occurrence, + before: *before, + after: *after, + }); + } + } + for (occurrence, tier) in after.into_iter().skip(paired) { + if tier == Tier::Guarded { + receipt.guarded_added.push(GuardedAddition { + module: key.0.clone(), + declaration: key.1.clone(), + subject: subject.to_string(), + occurrence, + }); + } + } + } +} + +#[derive(Default)] +struct DeclarationSnapshot { + authority: BTreeSet, + obligations: BTreeMap<(String, usize), Tier>, +} + +fn snapshot(checks: &[Checked]) -> BTreeMap<(String, String), DeclarationSnapshot> { + let mut out: BTreeMap<(String, String), DeclarationSnapshot> = BTreeMap::new(); + for checked in checks { + let Some(module) = checked + .module + .name + .as_ref() + .map(|name| name.to_string_path()) + else { + continue; + }; + let exports = Exports::of(&checked.module); + for name in exports.names() { + let export = exports + .get(name) + .expect("an exported name should be readable"); + if !matches!(export.kind, ExportKind::Function | ExportKind::Handler) { + continue; + } + out.entry((module.clone(), name.to_string())) + .or_default() + .authority + .extend(export.row.iter().cloned()); + } + for item in &checked.module.items { + let Item::Function(function) = item else { + continue; + }; + let name = function.sig.name.name.clone(); + let snapshot = out.entry((module.clone(), name)).or_default(); + + let mut occurrences: HashMap<&str, usize> = HashMap::new(); + for ObligationReport { + tier, + span, + subject, + .. + } in &checked.obligations + { + if !function.span.contains_span(*span) { + continue; + } + let occurrence = occurrences.entry(subject).or_default(); + snapshot + .obligations + .insert((subject.clone(), *occurrence), *tier); + *occurrence += 1; + } + } + } + out +} + +fn authority(row: &RowEntry, current_module: &str) -> String { + if row.variable { + return format!("row {}", row.effect); + } + let effect = if row.module.is_empty() || row.module == current_module { + row.effect.clone() + } else { + format!("{}/{}", row.module, row.effect) + }; + match &row.operation { + Some(operation) => format!("{effect}.{operation}"), + None => effect, + } +} + +fn covers(old: &RowEntry, new: &RowEntry) -> bool { + old == new + || (!old.variable + && !new.variable + && old.module == new.module + && old.effect == new.effect + && old.operation.is_none()) +} + +fn tier_rank(tier: Tier) -> u8 { + match tier { + Tier::Proven => 0, + Tier::Tested => 1, + Tier::Guarded => 2, + } +} diff --git a/crates/deed-driver/tests/review.rs b/crates/deed-driver/tests/review.rs new file mode 100644 index 00000000..efc219b2 --- /dev/null +++ b/crates/deed-driver/tests/review.rs @@ -0,0 +1,313 @@ +//! Review receipts compare what two checked module sets ask a reviewer to trust. + +use deed_diagnostics::SourceMap; +use deed_driver::{ + Checked, check_text, + review::{PolicyRule, ReviewPolicy, ReviewReceipt}, +}; +use deed_typeck::Tier; + +fn checked(sources: &mut SourceMap, name: &str, text: &str) -> Checked { + let checked = check_text(sources, name, text); + assert!( + !checked.has_errors(), + "fixture should check: {:?}", + checked + .diagnostics + .iter() + .map(|diagnostic| &diagnostic.message) + .collect::>() + ); + checked +} + +#[test] +fn new_authority_and_a_weaker_proof_are_receipted() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + effect Store {\n\ + fn read() -> Int\n\ + fn write(value: Int) -> ()\n\ + }\n\n\ + fn sync() -> Int uses Store.read, { Store.read() }\n\n\ + fn preserve(value: Positive) -> Positive { value + 1 }\n", + ); + + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + effect Store {\n\ + fn read() -> Int\n\ + fn write(value: Int) -> ()\n\ + }\n\n\ + fn sync() -> Int\n\ + uses Store.read, Store.write,\n\ + {\n\ + let value = Store.read()\n\ + Store.write(value)\n\ + value\n\ + }\n\n\ + fn preserve(value: Int) -> Positive { value + 1 }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert_eq!(receipt.authority_added.len(), 1, "{receipt:?}"); + assert_eq!(receipt.authority_added[0].module, "review/sample"); + assert_eq!(receipt.authority_added[0].declaration, "sync"); + assert_eq!(receipt.authority_added[0].authority, "Store.write"); + + assert_eq!(receipt.tier_regressions.len(), 1, "{receipt:?}"); + let regression = &receipt.tier_regressions[0]; + assert_eq!(regression.declaration, "preserve"); + assert_eq!(regression.subject, "Positive"); + assert_eq!(regression.before, Tier::Proven); + assert_eq!(regression.after, Tier::Guarded); + assert_eq!( + receipt.to_json(), + "{\"kind\":\"review_receipt\",\"clean\":false,\"authority_added\":[{\"module\":\"review/sample\",\"declaration\":\"sync\",\"authority\":\"Store.write\"}],\"tier_regressions\":[{\"module\":\"review/sample\",\"declaration\":\"preserve\",\"subject\":\"Positive\",\"occurrence\":0,\"before\":\"proven\",\"after\":\"guarded\"}],\"guarded_added\":[]}" + ); +} + +#[test] +fn less_authority_and_stronger_proofs_need_no_warning() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn sync(value: Int) -> () uses Store.write, { Store.write(value) }\n\n\ + fn preserve(value: Int) -> Positive { value + 1 }\n", + ); + + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn sync(value: Int) -> () { () }\n\n\ + fn preserve(value: Positive) -> Positive { value + 1 }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert!(receipt.is_clean(), "{receipt:?}"); +} + +#[test] +fn authority_in_a_new_function_is_new_authority() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\neffect Store { fn write(value: Int) -> () }\n", + ); + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn save(value: Int) -> () uses Store.write, { Store.write(value) }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert_eq!(receipt.authority_added.len(), 1, "{receipt:?}"); + assert_eq!(receipt.authority_added[0].declaration, "save"); + assert_eq!(receipt.authority_added[0].authority, "Store.write"); +} + +#[test] +fn narrowing_a_whole_effect_to_one_operation_is_not_new_authority() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn save(value: Int) -> () uses Store, { Store.write(value) }\n", + ); + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn save(value: Int) -> () uses Store.write, { Store.write(value) }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert!(receipt.is_clean(), "{receipt:?}"); +} + +#[test] +fn widening_one_operation_to_the_whole_effect_is_new_authority() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn save(value: Int) -> () uses Store.write, { Store.write(value) }\n", + ); + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn save(value: Int) -> () uses Store, { Store.write(value) }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert_eq!(receipt.authority_added.len(), 1, "{receipt:?}"); + assert_eq!(receipt.authority_added[0].authority, "Store"); +} + +#[test] +fn authority_added_by_an_exported_handler_is_receipted() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\n\ + effect Tally { fn add(value: Int) -> () }\n\n\ + effect Audit { fn note(value: Int) -> () }\n\n\ + handler Summer implements Tally {\n\ + state total: Int\n\n\ + fn add(value) -> () { total = total + value }\n\ + }\n", + ); + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + effect Tally { fn add(value: Int) -> () }\n\n\ + effect Audit { fn note(value: Int) -> () }\n\n\ + handler Summer implements Tally {\n\ + state total: Int\n\n\ + fn add(value) -> ()\n\ + uses Audit.note,\n\ + {\n\ + Audit.note(value)\n\ + total = total + value\n\ + }\n\ + }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert_eq!(receipt.authority_added.len(), 1, "{receipt:?}"); + assert_eq!(receipt.authority_added[0].declaration, "Summer"); + assert_eq!(receipt.authority_added[0].authority, "Audit.note"); +} + +#[test] +fn a_new_guarded_obligation_has_its_own_policy_gate() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\ntype Positive = Int where value > 0\n", + ); + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn accept(value: Int) -> Positive { value }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert_eq!(receipt.guarded_added.len(), 1, "{receipt:?}"); + assert_eq!(receipt.guarded_added[0].declaration, "accept"); + assert_eq!(receipt.guarded_added[0].subject, "Positive"); + + assert!(receipt.evaluate(ReviewPolicy::default()).passed()); + let verdict = receipt.evaluate(ReviewPolicy { + deny_new_guarded: true, + ..ReviewPolicy::default() + }); + assert!(!verdict.passed()); + assert_eq!(verdict.violations.len(), 1); + assert_eq!(verdict.violations[0].rule, PolicyRule::NewGuarded); + assert_eq!(verdict.violations[0].findings, 1); + assert_eq!( + receipt.to_json_with_policy(&verdict), + "{\"kind\":\"review_receipt\",\"clean\":false,\"authority_added\":[],\"tier_regressions\":[],\"guarded_added\":[{\"module\":\"review/sample\",\"declaration\":\"accept\",\"subject\":\"Positive\",\"occurrence\":0}],\"policy\":{\"passed\":false,\"violations\":[{\"rule\":\"deny-new-guarded\",\"findings\":1}]}}" + ); +} + +#[test] +fn reordering_the_same_subject_does_not_invent_a_change() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn needs(value: Positive) -> Int { value }\n\n\ + fn combine(known: Positive, unknown: Int) -> Int {\n\ + let first = needs(known + 1)\n\ + let second = needs(unknown)\n\ + first + second\n\ + }\n", + ); + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn needs(value: Positive) -> Int { value }\n\n\ + fn combine(known: Positive, unknown: Int) -> Int {\n\ + let second = needs(unknown)\n\ + let first = needs(known + 1)\n\ + first + second\n\ + }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert!(receipt.is_clean(), "{receipt:?}"); +} + +#[test] +fn adding_guarded_beside_proven_does_not_weaken_the_proven_one() { + let mut before_sources = SourceMap::new(); + let before = checked( + &mut before_sources, + "before.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn needs(value: Positive) -> Int { value }\n\n\ + fn combine(known: Positive) -> Int { needs(known + 1) }\n", + ); + let mut after_sources = SourceMap::new(); + let after = checked( + &mut after_sources, + "after.deed", + "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn needs(value: Positive) -> Int { value }\n\n\ + fn combine(known: Positive, unknown: Int) -> Int {\n\ + let guarded = needs(unknown)\n\ + let proven = needs(known + 1)\n\ + guarded + proven\n\ + }\n", + ); + + let receipt = ReviewReceipt::between(&[before], &[after]); + assert!(receipt.tier_regressions.is_empty(), "{receipt:?}"); + assert_eq!(receipt.guarded_added.len(), 1, "{receipt:?}"); + assert_eq!(receipt.guarded_added[0].subject, "Positive"); +} diff --git a/crates/deed-mcp/Cargo.toml b/crates/deed-mcp/Cargo.toml index 617a9d0c..78c4d96c 100644 --- a/crates/deed-mcp/Cargo.toml +++ b/crates/deed-mcp/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "deed-mcp" -description = "The boundary an agent calls across: one program's text in, the compiler's answer out" +description = "The capability-free agent boundary: module source in, compiler evidence out" version.workspace = true edition.workspace = true license.workspace = true diff --git a/crates/deed-mcp/smoke.py b/crates/deed-mcp/smoke.py index 412b3668..16129fa1 100644 --- a/crates/deed-mcp/smoke.py +++ b/crates/deed-mcp/smoke.py @@ -55,12 +55,13 @@ """ WANTED = { - "deed_check": "source", - "deed_test": "source", - "deed_run": "source", - "deed_fmt": "source", - "deed_fix": "source", - "deed_explain": "code", + "deed_check": ["source"], + "deed_test": ["source"], + "deed_run": ["source"], + "deed_fmt": ["source"], + "deed_fix": ["source"], + "deed_explain": ["code"], + "deed_review": ["before", "after"], } @@ -97,11 +98,16 @@ async def main(binary: str) -> int: listed = await session.list_tools() offered = {t.name: t for t in listed.tools} assert set(offered) == set(WANTED), f"tools are {sorted(offered)}" - for name, argument in WANTED.items(): + for name, required in WANTED.items(): schema = field(offered[name], "input_schema", "inputSchema") or {} - assert schema.get("required") == [argument], f"{name} requires {schema.get('required')}" + assert schema.get("required") == required, f"{name} requires {schema.get('required')}" assert offered[name].description, f"{name} arrives undescribed" + review_schema = field(offered["deed_review"], "input_schema", "inputSchema") or {} + assert review_schema["properties"]["before"]["type"] == "array", review_schema + assert review_schema["properties"]["after"]["items"]["type"] == "string", review_schema + assert review_schema["properties"]["policy"]["type"] == "object", review_schema + checked = lines(await session.call_tool("deed_check", {"source": CLEAN})) assert not [x for x in checked if x["kind"] == "diagnostic"], checked tiers = {x["tier"] for x in checked if x["kind"] == "obligation"} @@ -136,6 +142,17 @@ async def main(binary: str) -> int: assert page[0]["code"] == "DEED4025", page assert page[0]["text"], "the page for a code is empty" + before = "module smoke/review\n\neffect Audit { fn note(value: Int) -> () }\n\nfn work() -> () { () }\n" + after = "module smoke/review\n\neffect Audit { fn note(value: Int) -> () }\n\nfn work() -> () uses Audit.note, { Audit.note(1) }\n" + reviewed = lines(await session.call_tool("deed_review", { + "before": [before], + "after": [after], + "policy": {"denyNewAuthority": True}, + })) + assert len(reviewed) == 1 and reviewed[0]["kind"] == "review_receipt", reviewed + assert reviewed[0]["authority_added"][0]["authority"] == "Audit.note", reviewed + assert reviewed[0]["policy"]["passed"] is False, reviewed + broken = lines(await session.call_tool("deed_check", {"source": "module x\n\nfn f() -> Int {\n"})) assert [x for x in broken if x["kind"] == "diagnostic"], "a broken module checked cleanly" diff --git a/crates/deed-mcp/src/lib.rs b/crates/deed-mcp/src/lib.rs index 7425ce22..41db80e2 100644 --- a/crates/deed-mcp/src/lib.rs +++ b/crates/deed-mcp/src/lib.rs @@ -24,16 +24,19 @@ //! writes about a Deed function. //! //! That is a real cost and it is written down rather than hidden: an agent -//! working on a module set has to send every file it wants checked together, -//! because there is no root here to resolve a second file's `use` against. The -//! shipped library is the exception, and only because it travels inside the -//! binary already. +//! working on a module set has to send every file it wants reviewed as source +//! text. `deed_review` accepts those texts as two arrays and resolves imports +//! among each array. There is still no root and no path lookup. The shipped +//! library is the exception, and only because it travels inside the binary +//! already. //! //! ## Why the answers come from `deed-wasm` //! -//! The playground asks the compiler the same five questions an agent does: -//! text in, JSON out, one file, no filesystem. Those answers live in -//! [`deed_wasm`], and this crate calls them rather than writing a second copy. +//! The one-file tools are the same questions the playground asks: text in, +//! JSON out, one file, no filesystem. Those answers live in [`deed_wasm`], and +//! this crate calls them rather than writing a second copy. Review has no page +//! equivalent: it checks two in-memory module sets and delegates the evidence +//! and policy decision to [`deed_driver::review`], the same source as the CLI. //! Two copies would be two answers, and the one an agent got would be the one //! nobody was looking at. `crates/deed-mcp/tests/agreement.rs` holds that. //! @@ -172,6 +175,10 @@ impl Server { program that works: it runs the `test` blocks and the \ properties the contracts generate, and a property is one \ nobody wrote. `deed_run` last, for what `main` prints. \ + Before finishing a patch, use `deed_review` with every \ + module before and after it; read the receipt and its \ + policy verdict rather than treating a successful tool \ + call as approval. \ `deed_explain` turns any DEED#### code into its page, and \ `deed_fmt` into the one layout this language has.", ), diff --git a/crates/deed-mcp/src/tools.rs b/crates/deed-mcp/src/tools.rs index a2836741..fb4e294a 100644 --- a/crates/deed-mcp/src/tools.rs +++ b/crates/deed-mcp/src/tools.rs @@ -1,10 +1,10 @@ -//! The five questions an agent can ask, and the one thing it can ask about a -//! diagnostic code. +//! The questions an agent can ask the compiler, including what changed +//! between two module sets. //! -//! Each tool takes a whole program's text, because that is the unit this +//! Most tools take one whole program's text, because that is the unit this //! language has: `design/refusals.md` says why there is no REPL, and the same -//! reasoning applies here. There is no expression to evaluate at a prompt, so -//! there is nothing smaller than a module to send. +//! reasoning applies here. Review takes two arrays of those units so imports +//! can resolve within each in-memory module set. //! //! Every answer is the JSON the rest of the compiler already publishes, handed //! back as text. A tool result in MCP is content, not a typed value, so the @@ -12,8 +12,10 @@ //! `deed check --format json` writes means an agent that has seen one has seen //! the other. -use deed_diagnostics::json_string; +use deed_diagnostics::{SourceMap, json_string}; use deed_driver::fix::fix; +use deed_driver::review::{ReviewPolicy, ReviewReceipt}; +use deed_driver::{Checked, check_all, json_report, shipped_for, shipped_source}; use deed_lsp::Json; use crate::{Failure, INVALID_PARAMS}; @@ -22,10 +24,59 @@ use crate::{Failure, INVALID_PARAMS}; struct Tool { name: &'static str, description: &'static str, - /// The single argument this tool reads, and what to say about it. - argument: (&'static str, &'static str), + arguments: &'static [Argument], } +#[derive(Clone, Copy)] +struct Argument { + name: &'static str, + description: &'static str, + kind: ArgumentKind, + required: bool, +} + +#[derive(Clone, Copy)] +enum ArgumentKind { + String, + Sources, + Policy, +} + +const SOURCE: &[Argument] = &[Argument { + name: "source", + description: "The whole text of one Deed module.", + kind: ArgumentKind::String, + required: true, +}]; + +const CODE: &[Argument] = &[Argument { + name: "code", + description: "A diagnostic code, like `DEED4025`.", + kind: ArgumentKind::String, + required: true, +}]; + +const REVIEW: &[Argument] = &[ + Argument { + name: "before", + description: "Every Deed module before the patch, as source text.", + kind: ArgumentKind::Sources, + required: true, + }, + Argument { + name: "after", + description: "Every Deed module after the patch, as source text.", + kind: ArgumentKind::Sources, + required: true, + }, + Argument { + name: "policy", + description: "Optional gates for new authority, weaker promises and new Guarded obligations.", + kind: ArgumentKind::Policy, + required: false, + }, +]; + /// The tools this server offers, in the order an agent would use them. const TOOLS: &[Tool] = &[ Tool { @@ -36,7 +87,7 @@ const TOOLS: &[Tool] = &[ `proven` when it was settled at compile time, `tested` when a test pins it, \ and `guarded` when it falls to a runtime check, in which case `reason` says \ what stopped it from being proven. Silence means the program is well formed.", - argument: ("source", "The whole text of one Deed module."), + arguments: SOURCE, }, Tool { name: "deed_test", @@ -49,7 +100,7 @@ const TOOLS: &[Tool] = &[ to its `ensures`, and the seed is on the line so the same run can be asked \ for again. Refuses without running when the program does not check: ask \ `deed_check` for what is wrong with it.", - argument: ("source", "The whole text of one Deed module."), + arguments: SOURCE, }, Tool { name: "deed_run", @@ -57,14 +108,14 @@ const TOOLS: &[Tool] = &[ running if the program does not check, or if `main`'s row asks for a \ capability this server does not hand out: there is no filesystem here, so \ a program that reads or writes files is refused rather than failed.", - argument: ("source", "The whole text of one Deed module."), + arguments: SOURCE, }, Tool { name: "deed_fmt", description: "Format a Deed program into the one layout the formatter chooses. Returns \ the formatted text, or the parse diagnostics when the file does not parse, \ because a file with no tree has no layout to pick.", - argument: ("source", "The whole text of one Deed module."), + arguments: SOURCE, }, Tool { name: "deed_fix", @@ -72,13 +123,18 @@ const TOOLS: &[Tool] = &[ repaired program. Only the repairs `deed fix` would apply without asking; \ a suggestion the compiler is not sure about is left for the reader and \ shows up in `deed_check` instead.", - argument: ("source", "The whole text of one Deed module."), + arguments: SOURCE, }, Tool { name: "deed_explain", description: "Explain one diagnostic code, such as DEED4025. Returns the page that code \ carries: what it means, why the rule exists, and usually an example.", - argument: ("code", "A diagnostic code, like `DEED4025`."), + arguments: CODE, + }, + Tool { + name: "deed_review", + description: "Compare the checked module set before a patch with the set after it. Returns one JSON review receipt naming authority additions, obligation tier regressions and newly introduced Guarded obligations. An optional policy object accepts `denyNewAuthority`, `denyWeakerPromises` and `denyNewGuarded`; its verdict is evidence, not a transport error. Both sides stay in memory: this tool opens no file and holds no capability.", + arguments: REVIEW, }, ]; @@ -90,7 +146,17 @@ pub fn listing() -> Json { TOOLS .iter() .map(|tool| { - let (argument, about) = tool.argument; + let properties = tool + .arguments + .iter() + .map(|argument| (argument.name, argument_schema(argument))) + .collect(); + let required = tool + .arguments + .iter() + .filter(|argument| argument.required) + .map(|argument| Json::string(argument.name)) + .collect(); Json::object(vec![ ("name", Json::string(tool.name)), ("description", Json::string(tool.description)), @@ -98,17 +164,8 @@ pub fn listing() -> Json { "inputSchema", Json::object(vec![ ("type", Json::string("object")), - ( - "properties", - Json::object(vec![( - argument, - Json::object(vec![ - ("type", Json::string("string")), - ("description", Json::string(about)), - ]), - )]), - ), - ("required", Json::Array(vec![Json::string(argument)])), + ("properties", Json::object(properties)), + ("required", Json::Array(required)), ]), ), ]) @@ -118,6 +175,43 @@ pub fn listing() -> Json { )]) } +fn argument_schema(argument: &Argument) -> Json { + let mut fields = vec![("description", Json::string(argument.description))]; + match argument.kind { + ArgumentKind::String => fields.push(("type", Json::string("string"))), + ArgumentKind::Sources => { + fields.push(("type", Json::string("array"))); + fields.push(( + "items", + Json::object(vec![("type", Json::string("string"))]), + )); + fields.push(("minItems", Json::number(1))); + } + ArgumentKind::Policy => { + fields.push(("type", Json::string("object"))); + fields.push(( + "properties", + Json::object(vec![ + ( + "denyNewAuthority", + Json::object(vec![("type", Json::string("boolean"))]), + ), + ( + "denyWeakerPromises", + Json::object(vec![("type", Json::string("boolean"))]), + ), + ( + "denyNewGuarded", + Json::object(vec![("type", Json::string("boolean"))]), + ), + ]), + )); + fields.push(("additionalProperties", Json::Bool(false))); + } + } + Json::object(fields) +} + /// Runs one tool, or says why it could not. /// /// A tool that ran and found the program wrong is a success: the answer to @@ -132,7 +226,11 @@ pub fn call(name: &str, arguments: Option<&Json>) -> Result { }); }; - let (wanted, _) = tool.argument; + if name == "deed_review" { + return review(arguments); + } + + let wanted = tool.arguments[0].name; let Some(value) = arguments .and_then(|arguments| arguments.get(wanted)) .and_then(Json::as_str) @@ -146,6 +244,148 @@ pub fn call(name: &str, arguments: Option<&Json>) -> Result { Ok(text_result(&answer(name, value))) } +fn review(arguments: Option<&Json>) -> Result { + let Some(arguments) = arguments else { + return Err(invalid( + "`deed_review` needs `before` and `after` arguments", + )); + }; + let before_sources = source_set(arguments, "before")?; + let after_sources = source_set(arguments, "after")?; + let (policy, policy_was_given) = policy_of(arguments)?; + + let before = review_side("before", &before_sources); + let after = review_side("after", &after_sources); + let before_refusal = refusal("before", &before); + let after_refusal = refusal("after", &after); + if before_refusal.is_some() || after_refusal.is_some() { + let mut text = before_refusal.unwrap_or_default(); + text.push_str(&after_refusal.unwrap_or_default()); + return Ok(text_result(&text)); + } + + let receipt = ReviewReceipt::between(&before.checks, &after.checks); + let verdict = receipt.evaluate(policy); + let mut text = if policy_was_given { + receipt.to_json_with_policy(&verdict) + } else { + receipt.to_json() + }; + text.push('\n'); + Ok(text_result(&text)) +} + +fn source_set<'a>(arguments: &'a Json, name: &str) -> Result, Failure> { + let Some(value) = arguments.get(name) else { + return Err(invalid(&format!( + "`deed_review` needs a `{name}` argument, as a non-empty array of strings" + ))); + }; + let Some(items) = value.as_array() else { + return Err(invalid(&format!( + "`deed_review` needs `{name}` as a non-empty array of strings" + ))); + }; + if items.is_empty() { + return Err(invalid(&format!( + "`deed_review` needs at least one `{name}` module" + ))); + } + items + .iter() + .enumerate() + .map(|(index, item)| { + item.as_str().ok_or_else(|| { + invalid(&format!( + "`deed_review` `{name}` module {} is not a string", + index + 1 + )) + }) + }) + .collect() +} + +fn policy_of(arguments: &Json) -> Result<(ReviewPolicy, bool), Failure> { + let Some(raw) = arguments.get("policy") else { + return Ok((ReviewPolicy::default(), false)); + }; + let Json::Object(fields) = raw else { + return Err(invalid("`deed_review` needs `policy` as an object")); + }; + + let mut policy = ReviewPolicy::default(); + for (name, value) in fields { + let Json::Bool(enabled) = value else { + return Err(invalid(&format!( + "`deed_review` policy `{name}` must be a boolean" + ))); + }; + match name.as_str() { + "denyNewAuthority" => policy.deny_new_authority = *enabled, + "denyWeakerPromises" => policy.deny_weaker_promises = *enabled, + "denyNewGuarded" => policy.deny_new_guarded = *enabled, + _ => { + return Err(invalid(&format!( + "`deed_review` policy has no `{name}` rule" + ))); + } + } + } + Ok((policy, true)) +} + +struct ReviewSide { + sources: SourceMap, + checks: Vec, + subjects: usize, +} + +fn review_side(label: &str, texts: &[&str]) -> ReviewSide { + let mut sources = SourceMap::new(); + let mut ids = texts + .iter() + .enumerate() + .map(|(index, text)| sources.add(format!("<{label}/{}.deed>", index + 1), *text)) + .collect::>(); + let subjects = ids.len(); + for module in shipped_for(texts.iter().copied()) { + let text = shipped_source(module).expect("a module that ships has a source"); + ids.push(sources.add(format!("/{module}.deed"), text)); + } + let checks = check_all(&sources, &ids); + ReviewSide { + sources, + checks, + subjects, + } +} + +fn refusal(label: &str, side: &ReviewSide) -> Option { + let errors = side.checks.iter().map(Checked::error_count).sum::(); + let unnamed = side.checks[..side.subjects] + .iter() + .filter(|checked| checked.module.name.is_none()) + .count(); + if errors == 0 && unnamed == 0 { + return None; + } + + let mut text = json_report(&side.sources, &side.checks, false); + text.push_str(&format!( + "{{\"kind\":\"review_refused\",\"side\":{},\"errors\":{errors},\"unnamed\":{unnamed},\"message\":{}}}\n", + json_string(label), + json_string("every reviewed source must check and declare a module") + )); + Some(text) +} + +fn invalid(message: &str) -> Failure { + Failure { + code: INVALID_PARAMS, + message: message.to_string(), + } +} + /// What each tool actually asks the compiler. fn answer(name: &str, value: &str) -> String { match name { diff --git a/crates/deed-mcp/tests/agreement.rs b/crates/deed-mcp/tests/agreement.rs index fc83992d..2a98a039 100644 --- a/crates/deed-mcp/tests/agreement.rs +++ b/crates/deed-mcp/tests/agreement.rs @@ -6,10 +6,11 @@ //! the one that drifts. //! //! Here the risk is specific. `deed-wasm` answers a browser and `deed-mcp` -//! answers an agent, and both were written to be "text in, JSON out". The -//! moment one of them grows a field the other does not, an agent and a person -//! are reading different compilers. +//! answers an agent for one-file tools. Review has a different pair to hold: +//! both the CLI and MCP must publish exactly what `deed-driver` decided. +use deed_diagnostics::SourceMap; +use deed_driver::review::{ReviewPolicy, ReviewReceipt}; use deed_lsp::{Json, json}; use deed_mcp::tools; @@ -28,6 +29,30 @@ fn answer(name: &str, argument: &str, value: &str) -> String { .to_string() } +fn review_answer(before: &[&str], after: &[&str], policy: Json) -> String { + let sources = + |items: &[&str]| Json::Array(items.iter().map(|source| Json::string(*source)).collect()); + let arguments = Json::object(vec![ + ("before", sources(before)), + ("after", sources(after)), + ("policy", policy), + ]); + let result = tools::call("deed_review", Some(&arguments)).expect("review arguments are valid"); + result + .at(&["content"]) + .and_then(Json::as_array) + .and_then(|items| items.first()) + .and_then(|item| item.get("text")) + .and_then(Json::as_str) + .expect("review returns text") + .to_string() +} + +fn checked(source: &str, name: &str) -> deed_driver::Checked { + let mut sources = SourceMap::new(); + deed_driver::check_text(&mut sources, name, source) +} + /// Programs chosen so each verb has something to say about them: one that /// checks clean and runs, one that does not check, one with a test, one with a /// guarded obligation, and one the formatter has work to do on. @@ -89,6 +114,40 @@ fn formatting_gives_an_agent_what_it_gives_a_page() { } } +#[test] +fn reviewing_gives_an_agent_what_the_driver_receipts() { + let before = "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn save(value: Positive) -> Positive { value + 1 }\n"; + let after = "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + effect Store { fn write(value: Int) -> () }\n\n\ + fn save(value: Int) -> Positive\n\ + uses Store.write,\n\ + {\n\ + Store.write(value)\n\ + value + 1\n\ + }\n"; + let receipt = ReviewReceipt::between( + &[checked(before, "before.deed")], + &[checked(after, "after.deed")], + ); + let policy = ReviewPolicy { + deny_new_authority: true, + deny_weaker_promises: true, + deny_new_guarded: true, + }; + let expected = receipt.to_json_with_policy(&receipt.evaluate(policy)) + "\n"; + let policy = Json::object(vec![ + ("denyNewAuthority", Json::Bool(true)), + ("denyWeakerPromises", Json::Bool(true)), + ("denyNewGuarded", Json::Bool(true)), + ]); + + assert_eq!(review_answer(&[before], &[after], policy), expected); +} + /// The corpus is not empty and the verbs are not all answering nothing. /// /// Without this the four tests above pass on a list of programs that produce diff --git a/crates/deed-mcp/tests/session.rs b/crates/deed-mcp/tests/session.rs index 6815f773..9a75af75 100644 --- a/crates/deed-mcp/tests/session.rs +++ b/crates/deed-mcp/tests/session.rs @@ -29,24 +29,38 @@ fn hello() -> &'static str { r#"{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-06-18"}}"# } -/// The tool call a client makes, with one string argument. -fn call(id: i64, name: &str, argument: &str, value: &str) -> String { - let params = Json::object(vec![ - ("name", Json::string(name)), - ( - "arguments", - Json::object(vec![(argument, Json::string(value))]), - ), - ]); +fn call_with_arguments(id: i64, name: &str, arguments: Json) -> String { Json::object(vec![ ("jsonrpc", Json::string("2.0")), ("id", Json::number(id)), ("method", Json::string("tools/call")), - ("params", params), + ( + "params", + Json::object(vec![("name", Json::string(name)), ("arguments", arguments)]), + ), ]) .to_text() } +/// The tool call a client makes, with one string argument. +fn call(id: i64, name: &str, argument: &str, value: &str) -> String { + call_with_arguments( + id, + name, + Json::object(vec![(argument, Json::string(value))]), + ) +} + +fn review_call(id: i64, before: &[&str], after: &[&str], policy: Option) -> String { + let sources = + |items: &[&str]| Json::Array(items.iter().map(|source| Json::string(*source)).collect()); + let mut arguments = vec![("before", sources(before)), ("after", sources(after))]; + if let Some(policy) = policy { + arguments.push(("policy", policy)); + } + call_with_arguments(id, "deed_review", Json::object(arguments)) +} + /// The text a tool call came back with. fn content(answer: &Json) -> &str { answer @@ -223,8 +237,7 @@ fn ping_is_answered_with_an_empty_result() { assert_eq!(answers[1].get("result"), Some(&Json::object(vec![]))); } -/// Every tool is listed with a description and a schema naming its one -/// argument. +/// Every tool is listed with a description and a schema naming its arguments. /// /// A tool with no schema is a tool an agent cannot call without guessing, and /// guessing is what this whole surface exists to remove. @@ -256,15 +269,62 @@ fn every_tool_says_what_it_takes() { .at(&["inputSchema", "required"]) .and_then(Json::as_array) .unwrap_or_else(|| panic!("`{name}` has no required arguments")); - assert_eq!(required.len(), 1, "`{name}` should take exactly one thing"); - - let argument = required[0].as_str().expect("an argument is named"); - assert!( - tool.at(&["inputSchema", "properties", argument, "type"]) - .and_then(Json::as_str) - == Some("string"), - "`{name}`'s `{argument}` is not described as a string" - ); + if name == "deed_review" { + assert_eq!( + required.iter().filter_map(Json::as_str).collect::>(), + ["before", "after"] + ); + for argument in ["before", "after"] { + assert_eq!( + tool.at(&["inputSchema", "properties", argument, "type"]) + .and_then(Json::as_str), + Some("array") + ); + assert_eq!( + tool.at(&["inputSchema", "properties", argument, "items", "type"]) + .and_then(Json::as_str), + Some("string") + ); + } + assert_eq!( + tool.at(&["inputSchema", "properties", "policy", "type"]) + .and_then(Json::as_str), + Some("object") + ); + for rule in ["denyNewAuthority", "denyWeakerPromises", "denyNewGuarded"] { + assert_eq!( + tool.at(&[ + "inputSchema", + "properties", + "policy", + "properties", + rule, + "type", + ]) + .and_then(Json::as_str), + Some("boolean"), + "`deed_review` policy has no boolean `{rule}`" + ); + } + assert_eq!( + tool.at(&[ + "inputSchema", + "properties", + "policy", + "additionalProperties", + ]), + Some(&Json::Bool(false)) + ); + } else { + assert_eq!(required.len(), 1, "`{name}` should take exactly one thing"); + let argument = required[0].as_str().expect("an argument is named"); + assert_eq!( + tool.at(&["inputSchema", "properties", argument, "type"]) + .and_then(Json::as_str), + Some("string"), + "`{name}`'s `{argument}` is not described as a string" + ); + } } } @@ -290,12 +350,207 @@ fn the_tools_on_offer_are_exactly_these() { "deed_explain", "deed_fix", "deed_fmt", + "deed_review", "deed_run", "deed_test", ] ); } +#[test] +fn the_agent_guide_names_exactly_the_tools_on_offer() { + let guide = std::fs::read_to_string( + std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../../how-to/let-an-agent-use-the-compiler.md"), + ) + .expect("the agent guide should be there"); + let mut documented = guide + .lines() + .filter_map(|line| line.strip_prefix("| `deed_")) + .filter_map(|line| line.split('`').next()) + .map(|name| format!("deed_{name}")) + .collect::>(); + documented.sort(); + + let answers = session(&[hello(), r#"{"jsonrpc":"2.0","id":2,"method":"tools/list"}"#]); + let mut offered = answers[1] + .at(&["result", "tools"]) + .and_then(Json::as_array) + .expect("tools/list returns tools") + .iter() + .filter_map(|tool| tool.get("name").and_then(Json::as_str)) + .map(str::to_string) + .collect::>(); + offered.sort(); + + assert_eq!(documented, offered); +} + +#[test] +fn reviewing_two_module_sets_returns_a_policy_receipt() { + let audit = "module services/audit\n\neffect Audit { fn note(value: Int) -> () }\n"; + let before = "module app/main\n\nuse services/audit.{Audit}\n\nfn work() -> () { () }\n"; + let after = "module app/main\n\nuse services/audit.{Audit}\n\n\ + fn work() -> () uses Audit.note, { Audit.note(1) }\n"; + let policy = Json::object(vec![("denyNewAuthority", Json::Bool(true))]); + let request = review_call(2, &[before, audit], &[after, audit], Some(policy)); + let answers = session(&[hello(), &request]); + let receipt = json::parse(content(&answers[1]).trim()).expect("the receipt is JSON"); + + assert_eq!( + receipt + .at(&["authority_added"]) + .and_then(Json::as_array) + .and_then(|items| items.first()) + .and_then(|item| item.get("authority")) + .and_then(Json::as_str), + Some("services/audit/Audit.note") + ); + assert_eq!(receipt.at(&["policy", "passed"]), Some(&Json::Bool(false))); + assert!(answers[1].get("result").is_some()); + assert!(answers[1].get("error").is_none()); +} + +#[test] +fn every_review_policy_field_enforces_its_driver_rule() { + let proven = "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn preserve(value: Positive) -> Positive { value + 1 }\n"; + let guarded = "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn preserve(value: Int) -> Positive { value + 1 }\n"; + let no_function = "module review/sample\n\ntype Positive = Int where value > 0\n"; + let new_guarded = "module review/sample\n\n\ + type Positive = Int where value > 0\n\n\ + fn accept(value: Int) -> Positive { value }\n"; + + for (field, before, after, rule) in [ + ( + "denyWeakerPromises", + proven, + guarded, + "deny-weaker-promises", + ), + ( + "denyNewGuarded", + no_function, + new_guarded, + "deny-new-guarded", + ), + ] { + let policy = Json::object(vec![(field, Json::Bool(true))]); + let request = review_call(2, &[before], &[after], Some(policy)); + let answers = session(&[hello(), &request]); + let receipt = json::parse(content(&answers[1]).trim()).expect("the receipt is JSON"); + assert_eq!( + receipt + .at(&["policy", "violations"]) + .and_then(Json::as_array) + .and_then(|items| items.first()) + .and_then(|violation| violation.get("rule")) + .and_then(Json::as_str), + Some(rule), + "{field} mapped to the wrong policy" + ); + } +} + +#[test] +fn review_resolves_the_shipped_modules_named_in_its_sources() { + let source = "module app/main\n\nuse std/list.{sum}\n\n\ + fn total(items: List) -> Int { sum(items) }\n"; + let request = review_call(2, &[source], &[source], None); + let answers = session(&[hello(), &request]); + let receipt = json::parse(content(&answers[1]).trim()).expect("the receipt is JSON"); + + assert_eq!( + receipt.get("kind").and_then(Json::as_str), + Some("review_receipt") + ); + assert_eq!(receipt.get("clean"), Some(&Json::Bool(true))); +} + +#[test] +fn review_refuses_broken_or_unnamed_sources_without_failing_the_tool_call() { + let before = "module app/main\n\nfn work() -> Int { 1 }\n"; + let broken = "module app/main\n\nfn work() -> Missing { 1 }\n"; + let unnamed = "fn work() -> Int { 1 }\n"; + + for after in [broken, unnamed] { + let request = review_call(2, &[before], &[after], None); + let answers = session(&[hello(), &request]); + let records = content(&answers[1]) + .lines() + .map(|line| json::parse(line).expect("review refusal lines are JSON")) + .collect::>(); + let kinds = records + .iter() + .filter_map(|record| record.get("kind").and_then(Json::as_str)) + .collect::>(); + + assert!(kinds.contains(&"review_refused"), "{kinds:?}"); + assert!(!kinds.contains(&"review_receipt"), "{kinds:?}"); + assert!(answers[1].get("result").is_some()); + assert!(answers[1].get("error").is_none()); + } +} + +#[test] +fn review_rejects_malformed_policy() { + let source = "module app/main\n\nfn work() -> Int { 1 }\n"; + for policy in [ + Json::string("strict"), + Json::object(vec![("unknownRule", Json::Bool(true))]), + Json::object(vec![("denyNewAuthority", Json::string("yes"))]), + ] { + let request = review_call(2, &[source], &[source], Some(policy)); + let answers = session(&[hello(), &request]); + assert_eq!( + answers[1].at(&["error", "code"]).and_then(Json::as_i64), + Some(deed_mcp::INVALID_PARAMS) + ); + assert!( + answers[1] + .at(&["error", "message"]) + .and_then(Json::as_str) + .is_some_and(|message| message.contains("policy")) + ); + } +} + +#[test] +fn review_rejects_missing_empty_or_non_string_module_sets() { + let source = Json::Array(vec![Json::string( + "module app/main\n\nfn work() -> Int { 1 }\n", + )]); + let invalid = [ + Json::object(vec![("after", source.clone())]), + Json::object(vec![ + ("before", Json::string("module app/main\n")), + ("after", source.clone()), + ]), + Json::object(vec![ + ("before", Json::Array(vec![])), + ("after", source.clone()), + ]), + Json::object(vec![ + ("before", Json::Array(vec![Json::number(1)])), + ("after", source), + ]), + ]; + + for arguments in invalid { + let request = call_with_arguments(2, "deed_review", arguments); + let answers = session(&[hello(), &request]); + assert_eq!( + answers[1].at(&["error", "code"]).and_then(Json::as_i64), + Some(deed_mcp::INVALID_PARAMS), + "{}", + answers[1].to_text() + ); + } +} + #[test] fn checking_a_good_program_says_nothing_and_checking_a_bad_one_names_the_code() { let good = "module p\n\nfn twice(n: Int) -> Int {\n n * 2\n}\n"; diff --git a/design/decisions/2026-07-31-agent-surface.md b/design/decisions/2026-07-31-agent-surface.md index 722820df..de8eaea2 100644 --- a/design/decisions/2026-07-31-agent-surface.md +++ b/design/decisions/2026-07-31-agent-surface.md @@ -23,8 +23,9 @@ machine cannot reach the compiler, the pitch is untested. ## Decision -`deed mcp` speaks the Model Context Protocol on stdin and stdout, offering six tools: -`deed_check`, `deed_test`, `deed_run`, `deed_fmt`, `deed_fix` and `deed_explain`. +`deed mcp` speaks the Model Context Protocol on stdin and stdout, offering these tools: +`deed_check`, `deed_test`, `deed_run`, `deed_fmt`, `deed_fix`, `deed_explain` and +`deed_review`. Three properties are load-bearing, and each is held by a test rather than by this document. @@ -47,9 +48,11 @@ travels inside the binary. ### 2. The answers come from the surface the playground already uses -`deed-wasm` answers the same five questions for a browser: text in, JSON out, one file, no -filesystem. `deed-mcp` calls those functions rather than writing a second copy, and -`crates/deed-mcp/tests/agreement.rs` compares the two byte for byte over a corpus. +`deed-wasm` answers the same five one-file questions for a browser: text in, JSON out, one +file, no filesystem. `deed-mcp` calls those functions rather than writing a second copy. +Review has no browser equivalent; it delegates evidence and policy to +`deed_driver::review`, the same implementation the CLI uses. `crates/deed-mcp/tests/agreement.rs` +compares each pair byte for byte. The failure this avoids is this repository's most common one: two consumers of one idea drifting apart, with the drift landing on whichever one nobody is looking at. Nobody watches @@ -65,9 +68,9 @@ read. ## Drawbacks (required) -The single-file limit is the real one. An agent refactoring across modules cannot ask about -the module set today, and the honest answer is that it has to send whole files and will get -`DEED3007` for an import this server cannot resolve. +The one-file tools retain their single-file limit. Review accepts explicit arrays of module +texts and resolves imports among them, but cannot discover a missing file from a path or +manifest. That is the capability-free trade: the caller must already hold every source. A second cost: the tool descriptions are prose, and prose an agent reads is prose that can go stale the way any other prose here can. `every_tool_says_what_it_takes` holds the shape @@ -101,8 +104,8 @@ the transport and the tool table and nothing else. ## Open Questions (required) -- Whether a module-set tool (several files in one call) is worth adding, or whether that is - really asking for the root this decision refused. +- Resolved 2026-08-12: `deed_review` accepts several source texts in one call. It does not + take a root, discover files or weaken the capability decision. - Whether agents read the `reason` on a guarded obligation at all. The handshake's `instructions` field points at it explicitly, which is a guess about what an agent needs told. Nothing measures whether it helps. diff --git a/how-to/let-an-agent-use-the-compiler.md b/how-to/let-an-agent-use-the-compiler.md index 6ab80dad..ae2659de 100644 --- a/how-to/let-an-agent-use-the-compiler.md +++ b/how-to/let-an-agent-use-the-compiler.md @@ -33,20 +33,44 @@ what each of the calls below comes back with. | `deed_fmt` | `source` | The one layout the formatter chooses, or the parse diagnostics. | | `deed_fix` | `source` | The program with every machine-applicable repair applied, and how many went in. | | `deed_explain` | `code` | The page for one diagnostic code, like `DEED4025`. | +| `deed_review` | `before`, `after`, optional `policy` | One receipt for a patch: authority additions, weaker obligation tiers, new Guarded obligations, and any policy violations. | The order matters, and the server says so in its handshake because an agent that is not told picks one. `deed_check`, then `deed_fix` for the repairs the compiler is sure about, then `deed_test`, then `deed_run`. Skipping the third step is the easy mistake: the first model to be pointed at this server made sixty-five checks across six tasks and never ran a -test, on tasks that were scored on whether their tests pass. +test, on tasks that were scored on whether their tests pass. Before finishing a patch, +`deed_review` compares every module before and after it. Checking is not passing. That is not a caveat, it is the distinction the whole language is arranged around: the check settles what the contract can settle, and `deed_test` runs what is left, which is the `test` blocks and the properties generated from the contracts. -Every tool takes a whole module, because that is the unit this language has. +The one-file tools take a whole module, because that is the unit this language has. [`design/refusals.md`](../design/refusals.md) says why there is no REPL, and the same -reasoning applies: there is no expression to evaluate on its own. +reasoning applies: there is no expression to evaluate on its own. `deed_review` takes arrays +of whole modules so each side of a patch can resolve imports entirely in memory. + +## Review the patch + +Send every module in each version, including modules needed to resolve a local `use`: + +```json +{ + "before": ["module app\n\nfn save() -> () { () }\n"], + "after": ["module app\n\neffect Audit { fn note() -> () }\n\nfn save() -> () uses Audit.note, { Audit.note() }\n"], + "policy": { + "denyNewAuthority": true, + "denyWeakerPromises": true, + "denyNewGuarded": true + } +} +``` + +The answer is one `review_receipt` JSON object. Without `policy`, findings are evidence and +nothing is denied. With it, read `policy.passed`: `false` is still a successful MCP tool +call, because the receipt is the answer rather than a transport failure. A side with +compiler errors returns its diagnostics and `review_refused`, never a partial receipt. ## The line worth reading @@ -145,9 +169,9 @@ directory *before* running it rather than part way through. That is [`design/04-capabilities.md`](../design/04-capabilities.md)'s rule applied to the compiler's own tooling. It also has a cost worth knowing before you hit it: there is no root -here, so a `use` that names another one of your files cannot be resolved and comes back as -`DEED3007`. Send the module set as one program, or check those files with `deed check` on a -real path. +here, so the one-file tools cannot resolve a `use` that names another one of your files. +`deed_review` can resolve modules explicitly included in its arrays, but it cannot discover +or fetch one. Use `deed check` on a real path when discovery is the question. The reasoning in full, including what was rejected, is in [`design/decisions/2026-07-31-agent-surface.md`](../design/decisions/2026-07-31-agent-surface.md).