feat(skill-use): four more mechanical checks — 11 of 53 skills now scored on more than Trigger
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 <[email protected]> Claude-Session: https://claude.ai/code/session_01WZb5A2kfVfjpdwSochkuHz
This commit is contained in:
co-authored by
Claude Opus 5
parent
a50c41a9c2
commit
507d7444d1
@@ -323,6 +323,10 @@ fn check(skill: &str, ev: &Evidence<'_>) -> (Verdict, Verdict) {
|
|||||||
"workspace-repo-commit-protocol" => (Verdict::NotApplicable, workspace_boundary(ev)),
|
"workspace-repo-commit-protocol" => (Verdict::NotApplicable, workspace_boundary(ev)),
|
||||||
"small-focused-commits" => (Verdict::NotApplicable, commit_subject_boundary(ev)),
|
"small-focused-commits" => (Verdict::NotApplicable, commit_subject_boundary(ev)),
|
||||||
"web-search-triage" => (triage_compliance(ev), Verdict::NotApplicable),
|
"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
|
// 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
|
// 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
|
// separate rules would mean two rules for one procedure, which is how
|
||||||
@@ -785,6 +789,90 @@ fn dash_m_value(cmd: &str) -> Option<String> {
|
|||||||
None
|
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.
|
/// The tools whose arguments carry a URL the agent reached for.
|
||||||
///
|
///
|
||||||
/// `Agent` is here on purpose. On both `files`-arm runs the parent decomposed
|
/// `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());
|
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<ToolEvidence> = 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
|
/// 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.
|
/// exact uri back, and the scorer recovers the skill's name from it.
|
||||||
///
|
///
|
||||||
|
|||||||
Reference in New Issue
Block a user