Skip to content

Commit d30e661

Browse files
Your Nameclaude
andcommitted
feat(authority): add "calm review" out-of-band approval channel; harden proposal/receipt/reconciliation integrity
Audit follow-up on the same-day WS3/WS2b work. Verified 5 findings + 2 gaps against live source, fixed all of them, then found (and fixed) that HIGH_RISK_REQUIRES_INDEPENDENT_REVIEW can silently dead-end even when elicitation is enabled -- verified empirically that a connected MCP client can complete the elicitation round-trip (no timeout, no capability error) without ever showing the question to an actual human. Audit fixes (CCK-29c/29d/30R/30R2/31R/05C): - MRTR/declined-cache fingerprint now SHA-256 (was DefaultHasher, a public 64-bit hash with no cryptographic contract) -- ReviewAuthority's own scope+risk-bound nature documented as a permanent, structural limit. - Human elicitation message now shows a bounded, sanitized diff plus base/proposed digests, not just path/risk/touched-symbols -- a receipt can only back "this is what the reviewer was shown" if they were. - approval_receipts.signature_provenance ("native" vs "legacy_unverified"), folded into the signed payload so it can't be silently upgraded by anyone with raw state.db write access. State.db v9->v10. - WatchSupervisor::refresh now persists the Reconciled EvidenceSnapshot AFTER the SCIP overlay pass, not before -- the old order recorded a snapshot_id whose provider-state component the overlay immediately made stale, defeating Human-tier minting on any project with scip-overlay (a default-on feature) active. - RiskVector.touches_uncovered_code wired at the one spend-time call site with both a live diff and coverage data already loaded. - RootedFilesystem::write_atomic_beneath's directory fsync return code is no longer discarded -- WriteReceipt.durability distinguishes a confirmed fsync from an unverified one instead of reporting unqualified success. calm review (new): a durable, MCP-protocol-independent second channel for independent review, alongside elicitation rather than replacing it. edit_lines_impl_gated opens a pending_reviews row (state.db v11) when refused with no working elicitation path; `calm review approve|decline` requires a real interactive TTY (refuses on non-TTY stdin) and reuses the same diff renderer the elicitation message uses; an agent's retry with a matching approved review is treated as ElicitGate::Approved, with ApprovalReceipt.mechanism honestly recording "cli_manual_review" rather than "elicitation". Does not weaken the underlying invariant: risk=="high" still always requires an independent reviewer through one of the two channels, never self-attestation alone. Full cargo test --workspace green (1232 calm-core + 395 calm-server + all integration binaries, 0 failed), clippy --workspace --all-targets clean, fmt clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent ebe4aa3 commit d30e661

12 files changed

Lines changed: 1855 additions & 97 deletions

File tree

crates/calm-cli/src/main.rs

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,21 @@ enum Commands {
315315
#[command(subcommand)]
316316
action: BundleAction,
317317
},
318+
/// "calm review" (audit 2026-08-10 follow-up): a durable,
319+
/// MCP-protocol-independent second channel for the independent review
320+
/// `HIGH_RISK_REQUIRES_INDEPENDENT_REVIEW` requires -- for the case
321+
/// (verified empirically the same day) where a connected MCP client
322+
/// completes CALM's own elicitation round-trip without ever showing
323+
/// anything to a human at all. See
324+
/// `calm_core::authority::pending_review`'s module doc comment for the
325+
/// full rationale. `approve`/`decline` require a real interactive
326+
/// terminal (refuse immediately on non-TTY stdin) -- that's the actual
327+
/// teeth against an agent with ordinary shell access just scripting
328+
/// past this the way it could a config toggle.
329+
Review {
330+
#[command(subcommand)]
331+
action: ReviewAction,
332+
},
318333
}
319334

320335
#[cfg(feature = "index-bundles")]
@@ -347,6 +362,38 @@ enum BundleAction {
347362
},
348363
}
349364

365+
#[derive(Subcommand)]
366+
enum ReviewAction {
367+
/// List reviews (pending only, unless --all)
368+
List {
369+
#[arg(long, default_value = ".")]
370+
project_root: PathBuf,
371+
/// Show every review regardless of status, not just pending ones
372+
#[arg(long)]
373+
all: bool,
374+
},
375+
/// Show one review's full diff and details
376+
Show {
377+
#[arg(long, default_value = ".")]
378+
project_root: PathBuf,
379+
review_id: String,
380+
},
381+
/// Approve one review -- prints the diff and asks for an interactive
382+
/// y/N confirmation. Requires a real TTY on stdin; refuses immediately
383+
/// otherwise (see `Commands::Review`'s own doc comment for why).
384+
Approve {
385+
#[arg(long, default_value = ".")]
386+
project_root: PathBuf,
387+
review_id: String,
388+
},
389+
/// Decline one review -- same TTY requirement as `approve`.
390+
Decline {
391+
#[arg(long, default_value = ".")]
392+
project_root: PathBuf,
393+
review_id: String,
394+
},
395+
}
396+
350397
#[tokio::main]
351398
async fn main() -> Result<()> {
352399
let cli = Cli::parse();
@@ -1208,11 +1255,168 @@ async fn main() -> Result<()> {
12081255
}
12091256
}
12101257
},
1258+
Commands::Review { action } => match action {
1259+
ReviewAction::List { project_root, all } => {
1260+
let root = std::fs::canonicalize(&project_root)?;
1261+
let state_db_path = calm_server::default_state_db_path(&root);
1262+
let conn = calm_core::db::conn::open_state_writer(&state_db_path)?;
1263+
calm_core::db::schema::init_state_db_versioned(&conn)?;
1264+
let status_filter = if all { None } else { Some("pending") };
1265+
let reviews = calm_core::authority::list_pending_reviews(&conn, status_filter)?;
1266+
if reviews.is_empty() {
1267+
println!("No {}reviews.", if all { "" } else { "pending " });
1268+
} else {
1269+
for r in &reviews {
1270+
println!(
1271+
"{} [{}] {} risk={}",
1272+
r.review_id,
1273+
r.status,
1274+
r.path,
1275+
r.risk.as_deref().unwrap_or("?"),
1276+
);
1277+
}
1278+
}
1279+
}
1280+
ReviewAction::Show {
1281+
project_root,
1282+
review_id,
1283+
} => {
1284+
let root = std::fs::canonicalize(&project_root)?;
1285+
let state_db_path = calm_server::default_state_db_path(&root);
1286+
let conn = calm_core::db::conn::open_state_writer(&state_db_path)?;
1287+
calm_core::db::schema::init_state_db_versioned(&conn)?;
1288+
match calm_core::authority::get_pending_review(&conn, &review_id)? {
1289+
Some(r) => print_pending_review(&r),
1290+
None => {
1291+
println!("No such review: {review_id}");
1292+
std::process::exit(1);
1293+
}
1294+
}
1295+
}
1296+
ReviewAction::Approve {
1297+
project_root,
1298+
review_id,
1299+
} => decide_review_interactively(&project_root, &review_id, true)?,
1300+
ReviewAction::Decline {
1301+
project_root,
1302+
review_id,
1303+
} => decide_review_interactively(&project_root, &review_id, false)?,
1304+
},
12111305
}
12121306

12131307
Ok(())
12141308
}
12151309

1310+
/// Renders one `PendingReview` for a human reading a terminal -- sanitized
1311+
/// at print time (not storage time; `pending_reviews.diff_preview`/`reason`
1312+
/// are stored raw, same split `build_hub_elicit_message` already uses for
1313+
/// its own sanitize-at-display treatment of agent-authored content crossing
1314+
/// into a human-facing surface).
1315+
fn print_pending_review(r: &calm_core::authority::PendingReview) {
1316+
println!("review_id: {}", r.review_id);
1317+
println!("tool: {}", r.tool);
1318+
println!("path: {}", r.path);
1319+
println!("risk: {}", r.risk.as_deref().unwrap_or("?"));
1320+
if let Some(hk) = &r.hub_kind {
1321+
println!("hub_kind: {hk}");
1322+
}
1323+
println!("status: {}", r.status);
1324+
println!(
1325+
"reason: {}",
1326+
calm_core::sanitize::sanitize_source_output(r.reason.as_deref().unwrap_or("(none given)"))
1327+
);
1328+
println!();
1329+
println!("Proposed diff:");
1330+
println!(
1331+
"{}",
1332+
calm_core::sanitize::sanitize_source_output(&r.diff_preview)
1333+
);
1334+
}
1335+
1336+
/// `calm review approve|decline` shared body. Requires a real interactive
1337+
/// TTY on stdin -- refuses immediately otherwise. This is the actual
1338+
/// safeguard: most agent tool-execution sandboxes (including the one this
1339+
/// exact feature was designed against) pipe non-interactive stdin, so a
1340+
/// script or agent with ordinary shell access can't satisfy this the way it
1341+
/// could a config toggle -- same class of guarantee `sudo`'s password
1342+
/// prompt or `git commit` opening `$EDITOR` already rely on, not a claim of
1343+
/// unbypassable-by-construction security (a sufficiently permissive sandbox
1344+
/// could still allocate a pty).
1345+
fn decide_review_interactively(
1346+
project_root: &std::path::Path,
1347+
review_id: &str,
1348+
approving: bool,
1349+
) -> Result<()> {
1350+
use std::io::IsTerminal;
1351+
if !std::io::stdin().is_terminal() {
1352+
anyhow::bail!(
1353+
"`calm review {}` requires a real interactive terminal (refusing on \
1354+
non-TTY stdin) -- run this command yourself, directly, in a terminal.",
1355+
if approving { "approve" } else { "decline" }
1356+
);
1357+
}
1358+
let root = std::fs::canonicalize(project_root)?;
1359+
let state_db_path = calm_server::default_state_db_path(&root);
1360+
let conn = calm_core::db::conn::open_state_writer(&state_db_path)?;
1361+
calm_core::db::schema::init_state_db_versioned(&conn)?;
1362+
let review = match calm_core::authority::get_pending_review(&conn, review_id)? {
1363+
Some(r) => r,
1364+
None => {
1365+
println!("No such review: {review_id}");
1366+
std::process::exit(1);
1367+
}
1368+
};
1369+
if review.status != "pending" {
1370+
println!(
1371+
"Review {review_id} is already {}, not pending.",
1372+
review.status
1373+
);
1374+
std::process::exit(1);
1375+
}
1376+
print_pending_review(&review);
1377+
println!();
1378+
print!(
1379+
"{} this write? [y/N] ",
1380+
if approving {
1381+
"Approve"
1382+
} else {
1383+
"Confirm declining"
1384+
}
1385+
);
1386+
use std::io::Write;
1387+
std::io::stdout().flush().ok();
1388+
let mut answer = String::new();
1389+
std::io::stdin().read_line(&mut answer)?;
1390+
let confirmed = matches!(answer.trim().to_lowercase().as_str(), "y" | "yes");
1391+
if !confirmed {
1392+
println!("Not confirmed -- no change made.");
1393+
return Ok(());
1394+
}
1395+
let decided_by = "cli_manual_review";
1396+
let ok = if approving {
1397+
calm_core::authority::approve_pending_review(&conn, review_id, decided_by)?
1398+
} else {
1399+
calm_core::authority::decline_pending_review(&conn, review_id, decided_by)?
1400+
};
1401+
if ok {
1402+
println!(
1403+
"Review {review_id} {}.",
1404+
if approving { "approved" } else { "declined" }
1405+
);
1406+
if approving {
1407+
println!("The agent's retry of the same edit should now succeed.");
1408+
}
1409+
} else {
1410+
println!(
1411+
"Could not {} {review_id} -- it may have expired or already been decided by \
1412+
someone else.",
1413+
if approving { "approve" } else { "decline" }
1414+
);
1415+
std::process::exit(1);
1416+
}
1417+
Ok(())
1418+
}
1419+
12161420
/// Builds the `{ "command", "args" }` MCP entry every client config shares,
12171421
/// so the absolute-binary form and the portable `npx` form differ only in
12181422
/// which `command`/`args` get passed in here.

crates/calm-core/src/authority/mod.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,16 @@
66
//! snapshot-bound authority object #65 asks for, built on top of both.
77
88
pub mod key;
9+
pub mod pending_review;
910
pub mod receipt;
1011
pub mod review;
1112
pub mod snapshot;
1213

14+
pub use pending_review::{
15+
NewPendingReview, PENDING_REVIEW_DEFAULT_TTL_SECS, PendingReview, approve_pending_review,
16+
decline_pending_review, find_approved_matching, get_pending_review, insert_pending_review,
17+
list_pending_reviews,
18+
};
1319
pub use receipt::{ApprovalReceipt, insert_approval_receipt};
1420
pub use review::{
1521
AUTHORITY_TTL_MAX_SECS, AuthorityError, AuthorityTtl, AuthorityTtlError, AuthorizeEditError,

0 commit comments

Comments
 (0)