From 507d7444d163a98a2d23e7d4f1a9c77d930576b0 Mon Sep 17 00:00:00 2001 From: Omar Sobh Date: Fri, 18 Sep 2026 22:38:53 -0500 Subject: [PATCH] =?UTF-8?q?feat(skill-use):=20four=20more=20mechanical=20c?= =?UTF-8?q?hecks=20=E2=80=94=2011=20of=2053=20skills=20now=20scored=20on?= =?UTF-8?q?=20more=20than=20Trigger?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surveyed the catalogue against the module's own rule: only a procedure with a consequence visible in recorded tool arguments or delivered files gets a check; a heuristic over prose is a number that looks like a measurement and is not one. Four qualify beyond the seven that had checks. postgres-migrations-forward-only — "applied once and never rolled back; undo with a NEW migration" and "renaming a column: don't". An Edit to a path under migrations/ is by construction a change to a file that already existed; a written migration containing RENAME COLUMN is the forbidden rename. criterion-benchmarking — "a missing black_box lets the optimiser delete the work". A written benches/*.rs that mentions criterion and never black_box. secret-scanning-gitleaks and cargo-audit-workflow — the procedure IS running the tool, so a recorded `gitleaks` / `cargo audit` command is the compliance (PassWith, naming the count) and its absence is NotApplicable, never a violation: the stream is capped and the mission may not have reached the step. The written text comes from the tool arguments (Write.content, Edit's new_string), not from reading files back. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WZb5A2kfVfjpdwSochkuHz --- crates/cm-api/src/skill_use.rs | 136 +++++++++++++++++++++++++++++++++ 1 file changed, 136 insertions(+) diff --git a/crates/cm-api/src/skill_use.rs b/crates/cm-api/src/skill_use.rs index a8df422..91e613f 100644 --- a/crates/cm-api/src/skill_use.rs +++ b/crates/cm-api/src/skill_use.rs @@ -323,6 +323,10 @@ fn check(skill: &str, ev: &Evidence<'_>) -> (Verdict, Verdict) { "workspace-repo-commit-protocol" => (Verdict::NotApplicable, workspace_boundary(ev)), "small-focused-commits" => (Verdict::NotApplicable, commit_subject_boundary(ev)), "web-search-triage" => (triage_compliance(ev), Verdict::NotApplicable), + "postgres-migrations-forward-only" => (Verdict::NotApplicable, migrations_boundary(ev)), + "criterion-benchmarking" => (Verdict::NotApplicable, black_box_boundary(ev)), + "secret-scanning-gitleaks" => (ran_tool_compliance(ev, "gitleaks", "gitleaks"), Verdict::NotApplicable), + "cargo-audit-workflow" => (ran_tool_compliance(ev, "cargo audit", "cargo audit"), Verdict::NotApplicable), // One check for both: `cargo-test-driven-development` is the Rust // flavour of the same loop, and its own text says so. Scoring them by // separate rules would mean two rules for one procedure, which is how @@ -785,6 +789,90 @@ fn dash_m_value(cmd: &str) -> Option { None } +/// What a write tool put on disk, from its arguments: `Write.content`, or the +/// replacement half of an `Edit`. Neither reads the file back; the recorded +/// argument is the fact. +fn written_text(t: &crate::mission_events::ToolEvidence) -> Option<&str> { + t.input + .get("content") + .or_else(|| t.input.get("new_string")) + .and_then(serde_json::Value::as_str) +} + +/// `postgres-migrations-forward-only`: "every migration file is applied once +/// and never rolled back — if you need to undo, ship a NEW migration", and +/// "renaming a column: don't". +/// +/// Two visible violations. An `Edit` to a path under `migrations/` is a change +/// to a file that already existed (Edit cannot create), which is the one thing +/// the cardinal rule forbids. And a migration whose written text contains +/// `RENAME COLUMN` is the rename the skill says never to do. +fn migrations_boundary(ev: &Evidence<'_>) -> Verdict { + let is_migration = |p: &str| p.contains("/migrations/") && p.ends_with(".sql"); + for t in ev.writes() { + let Some(path) = t.path.as_deref().filter(|p| is_migration(p)) else { + continue; + }; + if t.tool != "Write" { + return Verdict::Fail(format!( + "{} on {path} — a migration is applied once and never edited; undo it \ + with a NEW migration", + t.tool + )); + } + if written_text(t).is_some_and(|c| c.to_ascii_uppercase().contains("RENAME COLUMN")) { + return Verdict::Fail(format!( + "{path} renames a column — the procedure is add, dual-write, backfill, \ + stop writing the old one, then drop it" + )); + } + } + Verdict::Pass +} + +/// `criterion-benchmarking`: "a missing `black_box` lets the optimiser delete +/// the work entirely". A bench file written without one measures nothing. +fn black_box_boundary(ev: &Evidence<'_>) -> Verdict { + for t in ev.writes() { + let Some(path) = t.path.as_deref() else { continue }; + if !(path.contains("/benches/") && path.ends_with(".rs")) { + continue; + } + if let Some(text) = written_text(t) { + if text.contains("criterion") && !text.contains("black_box") { + return Verdict::Fail(format!( + "{path} is a criterion bench with no black_box — the optimiser may \ + delete the work being measured" + )); + } + } + } + Verdict::Pass +} + +/// A skill whose procedure IS running a tool. Seeing the command is compliance; +/// not seeing it is not a violation — the stream is capped and the mission may +/// not have reached the step. One-sided, like the rest of the module. +fn ran_tool_compliance(ev: &Evidence<'_>, needle: &str, label: &str) -> Verdict { + if ev.tools.is_empty() { + return Verdict::NotObservable( + "no tool calls were recorded for this mission — whether the tool ran is \ + what this check reads" + .into(), + ); + } + let n = ev + .tools + .iter() + .filter(|t| t.command().is_some_and(|c| c.contains(needle))) + .count(); + if n > 0 { + Verdict::PassWith(format!("ran `{label}` {n}x")) + } else { + Verdict::NotApplicable + } +} + /// The tools whose arguments carry a URL the agent reached for. /// /// `Agent` is here on purpose. On both `files`-arm runs the parent decomposed @@ -1329,6 +1417,54 @@ mod tests { assert_eq!(Verdict::PassWith("x".into()).label(), Verdict::Pass.label()); } + fn write_to(path: &str, content: &str) -> ToolEvidence { + ToolEvidence { + tool: "Write".into(), + path: Some(path.into()), + input: json!({ "file_path": path, "content": content }), + response: serde_json::Value::Null, + } + } + fn edit_to(path: &str, new_string: &str) -> ToolEvidence { + ToolEvidence { + tool: "Edit".into(), + path: Some(path.into()), + input: json!({ "file_path": path, "old_string": "x", "new_string": new_string }), + response: serde_json::Value::Null, + } + } + + #[test] + fn editing_an_existing_migration_is_the_forbidden_thing() { + let ok = vec![write_to("/mission/repo/migrations/0090_add_col.sql", "ALTER TABLE t ADD COLUMN c INT;")]; + assert!(matches!(migrations_boundary(&Evidence::new("", &ok)), Verdict::Pass)); + let bad = vec![edit_to("/mission/repo/migrations/0085_usage_events_provider.sql", "-- tweak")]; + assert!(matches!(migrations_boundary(&Evidence::new("", &bad)), Verdict::Fail(_))); + let rename = vec![write_to("/mission/repo/migrations/0091_x.sql", "ALTER TABLE t RENAME COLUMN a TO b;")]; + assert!(matches!(migrations_boundary(&Evidence::new("", &rename)), Verdict::Fail(_))); + // Edits elsewhere are none of this skill's business. + let other = vec![edit_to("/mission/repo/src/lib.rs", "fn x() {}")]; + assert!(matches!(migrations_boundary(&Evidence::new("", &other)), Verdict::Pass)); + } + + #[test] + fn a_criterion_bench_without_black_box_measures_nothing() { + let bad = vec![write_to("/mission/repo/benches/parse.rs", "use criterion::*; fn b(c: &mut Criterion) { c.bench_function(\"p\", |b| b.iter(|| parse(\"x\"))); }")]; + assert!(matches!(black_box_boundary(&Evidence::new("", &bad)), Verdict::Fail(_))); + let ok = vec![write_to("/mission/repo/benches/parse.rs", "use criterion::{black_box, Criterion}; fn b(c: &mut Criterion) { c.bench_function(\"p\", |b| b.iter(|| parse(black_box(\"x\")))); }")]; + assert!(matches!(black_box_boundary(&Evidence::new("", &ok)), Verdict::Pass)); + } + + #[test] + fn running_the_tool_is_the_compliance_and_silence_is_not_a_violation() { + let ran = vec![bash("gitleaks detect --source . --no-banner")]; + assert!(matches!(ran_tool_compliance(&Evidence::new("", &ran), "gitleaks", "gitleaks"), Verdict::PassWith(_))); + let quiet = vec![bash("cargo test")]; + assert!(matches!(ran_tool_compliance(&Evidence::new("", &quiet), "gitleaks", "gitleaks"), Verdict::NotApplicable)); + let none: Vec = vec![]; + assert!(matches!(ran_tool_compliance(&Evidence::new("", &none), "cargo audit", "cargo audit"), Verdict::NotObservable(_))); + } + /// The whole loop, end to end: the index writes a uri, the agent reads that /// exact uri back, and the scorer recovers the skill's name from it. ///