diff --git a/.github/actions/ci/action.yml b/.github/actions/ci/action.yml index 9015cdc3..4acaf3e8 100644 --- a/.github/actions/ci/action.yml +++ b/.github/actions/ci/action.yml @@ -19,6 +19,7 @@ runs: ~/.dylint_drivers/ ~/.rustup/toolchains/ target/dylint/ + target/necessist-audit-cache/ key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }} - uses: taiki-e/install-action@v2.85.10 @@ -45,6 +46,24 @@ runs: if: ${{ env.TESTS != 'ci' }} uses: ./.github/actions/install-testing-tools + - name: Install LLM tools + if: ${{ (runner.os == 'Linux' || runner.os == 'macOS') && contains(env.TESTS, 'necessist_audit') }} + run: | + curl -fsSL https://claude.ai/install.sh | bash + curl -fsSL https://chatgpt.com/codex/install.sh | sh + printenv OPENAI_API_KEY | codex login --with-api-key + shell: bash + + # smoelius: Codex requires Bubblewrap. + - name: Install Bubblewrap on Ubuntu + if: ${{ runner.os == 'Linux' && contains(env.TESTS, 'necessist_audit') }} + run: | + sudo apt install bubblewrap + sudo apt install apparmor-profiles + sudo cp /usr/share/apparmor/extra-profiles/bwrap-userns-restrict /etc/apparmor.d || true + sudo systemctl reload apparmor + shell: bash + - name: Install sqlite3 on Ubuntu if: ${{ runner.os == 'Linux' }} run: | @@ -82,6 +101,11 @@ runs: run: | for TEST in $TESTS; do if [[ "$TEST" != 'other' ]]; then + if [[ "$TEST" = 'necessist_audit' && '${{ github.event_name }}' != 'schedule' && '${{ github.event_name }}' != 'workflow_dispatch' ]] && + ! git diff --name-only ${{ github.event.pull_request.base.sha }} | grep 'necessist_audit' >/dev/null + then + exit 0 + fi $CARGO_TEST -p necessist --test "$TEST" else $CARGO_TEST -p necessist --test general diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0698c9d0..223b3fc7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,7 +35,7 @@ jobs: fail-fast: ${{ github.event_name == 'merge_group' }} matrix: environment: [ubuntu-latest, macos-latest, windows-latest] - test: [third_party_0, third_party_1, trycmd, other] + test: [necessist_audit, third_party_0, third_party_1, trycmd, other] include: - environment: ubuntu-latest test: ci @@ -48,6 +48,8 @@ jobs: - uses: ./.github/actions/ci env: TESTS: ${{ matrix.test }} + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} all-checks: needs: [test] diff --git a/necessist/tests/necessist_audit.rs b/necessist/tests/necessist_audit.rs new file mode 100644 index 00000000..425370ee --- /dev/null +++ b/necessist/tests/necessist_audit.rs @@ -0,0 +1,100 @@ +//! A paper on Necessist (Test Harness Mutilation) appeared in Mutation 2024. +//! +//! This test audits one of its Go standard library findings. +//! +//! The structured-report approach is inspired by the worker finding artifacts in +//! the Trail of Bits `c-review` skill: +//! + +use assert_cmd::output::OutputError; +use necessist_core::util; +use std::{ + env::{split_paths, var_os}, + io::{Write, stderr}, + path::{Path, PathBuf}, + process::{Command, Output}, + time::Duration, +}; + +mod necessist_audit_support; +use necessist_audit_support::tool::Tool; + +const GO_REPO: &str = "https://github.com/golang/go"; +const GO_REV: &str = "9a0a82445650eebedf5633fdfe6e73b5836dc5c9"; +const JSON_REPORT: &str = "necessist-audit.json"; +const OUTPUT_POLL_INTERVAL: Duration = Duration::from_millis(100); +const TIMEOUT: Duration = Duration::from_mins(10); + +#[test] +#[cfg_attr(dylint_lib = "general", allow(non_thread_safe_call_in_test))] +fn necessist_audit_claude() { + let Some(tool_path) = command_on_path("claude") else { + #[allow(clippy::explicit_write)] + writeln!( + stderr(), + "Skipping `necessist_audit_claude` because `claude` is not on PATH" + ) + .unwrap(); + return; + }; + + Tool::claude(tool_path).run(); +} + +#[test] +#[cfg_attr(dylint_lib = "general", allow(non_thread_safe_call_in_test))] +fn necessist_audit_codex() { + let Some(tool_path) = command_on_path("codex") else { + #[allow(clippy::explicit_write)] + writeln!( + stderr(), + "Skipping `necessist_audit_codex` because `codex` is not on PATH" + ) + .unwrap(); + return; + }; + + Tool::codex(tool_path).run(); +} + +fn command_on_path(tool: &str) -> Option { + let path_var = var_os("PATH")?; + split_paths(&path_var).find_map(|dir| { + let candidate = dir.join(tool); + candidate.is_file().then_some(candidate) + }) +} + +fn command_output(command: &mut Command) -> Output { + let output = command.output().unwrap(); + assert!( + output.status.success(), + "command failed\n{}", + OutputError::new(output.clone()) + ); + output +} + +fn format_output(output: &Output) -> String { + OutputError::new(output.clone()).to_string() +} + +#[cfg_attr(dylint_lib = "supplementary", allow(abs_home_path))] +fn workspace_root() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")) + .parent() + .unwrap() + .to_owned() +} + +fn skill_dir() -> PathBuf { + workspace_root().join("skills/necessist-audit") +} + +fn cache_dir() -> PathBuf { + workspace_root().join("target/necessist-audit-cache") +} + +fn display_path(path: &Path) -> &Path { + util::strip_prefix(path, &workspace_root()).unwrap_or(path) +} diff --git a/necessist/tests/necessist_audit_support/accept.rs b/necessist/tests/necessist_audit_support/accept.rs new file mode 100644 index 00000000..4ea804f5 --- /dev/null +++ b/necessist/tests/necessist_audit_support/accept.rs @@ -0,0 +1,169 @@ +use serde_json::Value; + +type IsAcceptableResultFn = fn(&Value) -> bool; + +pub(crate) fn contains_acceptable_result(findings: &[Value], leads: &[Value]) -> bool { + let acceptable_result_fns: &[IsAcceptableResultFn] = &[ + is_acceptable_done_result, + is_acceptable_hello_result, + is_acceptable_crlf_oracle_result, + ]; + + [findings, leads].into_iter().any(|results| { + acceptable_result_fns + .iter() + .any(|&is_acceptable_result| results.iter().any(is_acceptable_result)) + }) +} + +// The paper highlighted this `<-done` removal. +fn is_acceptable_done_result(value: &Value) -> bool { + let removed_code = value + .get("removed_code") + .and_then(Value::as_str) + .unwrap_or_default(); + if !removed_code.contains("<-done") { + return false; + } + + assert_json_string_contains(value, "removed_code", "<-done"); + assert_json_string_contains(value, "removed_location", "smtp_test.go"); + assert_json_string_contains(value, "affected_location", "smtp_test.go"); + + let text = json_text(value); + assert!( + text.contains("traffic") || text.contains("SMTP") || text.contains("goroutine"), + "finding did not include SMTP traffic/goroutine details: {value:#}" + ); + true +} + +// Current LLMs may also report `Hello("customhost")` in `TestHello` case 8: +// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp_test.go#L474-L481 +// That is also a valid test-harness finding, though it was not called out in the +// original paper. +// +// The test appears intended to check `Client.Hello`'s documented ordering +// contract: `Hello` must be called before any other method, and the implementation +// rejects late calls once `didHello` is set: +// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp.go#L96-L105 +// +// ```go +// err = c.Verify("test@example.com") +// if err != nil { +// err = c.Hello("customhost") +// if err != nil { +// t.Errorf("Want error, got none") +// } +// } +// ``` +// +// In the fixture, `Verify` succeeds, so the branch is unreachable and the +// `Hello` call is never exercised. Removing it therefore leaves the test passing +// and exposes that the harness would not catch a regression where `Hello` +// incorrectly succeeds after another method. The intended shape is more like: +// +// ```go +// if err := c.Verify("test@example.com"); err != nil { +// t.Fatalf("Verify failed: %v", err) +// } +// if err := c.Hello("customhost"); err == nil { +// t.Errorf("Want error, got none") +// } +// ``` +fn is_acceptable_hello_result(value: &Value) -> bool { + if !json_text(value).contains("Hello(\"customhost\")") { + return false; + } + + assert_json_string_contains(value, "removed_code", "Hello(\"customhost\")"); + assert_json_string_contains(value, "removed_location", "smtp_test.go"); + assert_json_string_contains(value, "affected_location", "smtp_test.go"); + + let text = json_text(value); + assert!( + text.contains("Verify") && text.contains("Hello"), + "finding did not include Verify/Hello details: {value:#}" + ); + true +} + +// Current LLMs may also report the `TestBasic` CR/LF injection checks: +// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp_test.go#L197-L217 +// Those are also valid test-harness findings, though they were not called out in +// the original paper. +// +// The test appears intended to check that `Client.Verify`, `Client.Rcpt`, and +// `Client.Mail` reject CR/LF command injection before sending SMTP commands: +// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp.go#L185-L188 +// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp.go#L246-L249 +// https://github.com/golang/go/blob/9a0a82445650eebedf5633fdfe6e73b5836dc5c9/src/net/smtp/smtp.go#L266-L269 +// +// ```go +// if err := c.Verify("user2@gmail.com>\r\nDATA\r\nAnother injected message body\r\n.\r\nQUIT\r\n"); err == nil { +// t.Fatalf("VRFY should have failed due to a message injection attempt") +// } +// if err := c.Rcpt("golang-nuts@googlegroups.com>\r\nDATA\r\nInjected message body\r\n.\r\nQUIT\r\n"); err == nil { +// t.Fatalf("RCPT should have failed due to a message injection attempt") +// } +// if err := c.Mail("user@gmail.com>\r\nDATA\r\nAnother injected message body\r\n.\r\nQUIT\r\n"); err == nil { +// t.Fatalf("MAIL should have failed due to a message injection attempt") +// } +// ``` +// +// Necessist can remove the selector-call suffix from those short initializer +// expressions. The remaining initializer is the non-`nil` client `c`, so +// `err == nil` is false, the failure branch is skipped, and the malicious input +// is never passed to `Verify`, `Rcpt`, or `Mail`. The SMTP implementation still +// calls `validateLine`; the defect is that the test oracle can pass without +// exercising those security checks. +fn is_acceptable_crlf_oracle_result(value: &Value) -> bool { + let removed_code = value + .get("removed_code") + .and_then(Value::as_str) + .unwrap_or_default(); + if !["Verify", "Rcpt", "Mail"] + .iter() + .any(|method| removed_code.contains(&format!(".{method}(\""))) + { + return false; + } + + assert_json_string_contains(value, "removed_code", "\\r\\n"); + assert_json_string_contains(value, "removed_location", "smtp_test.go"); + assert_json_string_contains(value, "affected_location", "smtp_test.go"); + + let text = json_text(value); + assert!( + (text.contains("CR/LF") || text.contains("injection")) + && (text.contains("Verify") || text.contains("Rcpt") || text.contains("Mail")), + "finding did not include CR/LF injection oracle details: {value:#}" + ); + true +} + +fn assert_json_string_contains(value: &Value, key: &str, needle: &str) { + let haystack = value + .get(key) + .and_then(Value::as_str) + .unwrap_or_else(|| panic!("finding missing string field `{key}`")); + assert!( + haystack.contains(needle), + "expected `{key}` to contain `{needle}`, got `{haystack}`" + ); +} + +fn json_text(value: &Value) -> String { + match value { + Value::Null => String::new(), + Value::Bool(bool) => bool.to_string(), + Value::Number(number) => number.to_string(), + Value::String(string) => string.clone(), + Value::Array(array) => array.iter().map(json_text).collect::>().join("\n"), + Value::Object(object) => object + .values() + .map(json_text) + .collect::>() + .join("\n"), + } +} diff --git a/necessist/tests/necessist_audit_support/go_checkout.rs b/necessist/tests/necessist_audit_support/go_checkout.rs new file mode 100644 index 00000000..a87bf294 --- /dev/null +++ b/necessist/tests/necessist_audit_support/go_checkout.rs @@ -0,0 +1,215 @@ +use super::super::{GO_REPO, GO_REV, cache_dir, command_output, display_path, workspace_root}; +use std::{ + env::{join_paths, split_paths, var_os}, + ffi::OsString, + fs::{copy, create_dir_all, read_dir, remove_dir_all}, + path::{Path, PathBuf}, + process::Command, +}; + +pub(crate) struct GoCheckout { + root: PathBuf, + run_dir: PathBuf, + envs: Vec<(OsString, OsString)>, +} + +impl GoCheckout { + pub(crate) fn root(&self) -> &Path { + &self.root + } + + pub(crate) fn run_dir(&self) -> &Path { + &self.run_dir + } + + fn env_as_path(&self, key: &str) -> &Path { + self.envs + .iter() + .find_map(|(name, value)| (name == key).then(|| Path::new(value))) + .unwrap_or_else(|| panic!("missing Go environment variable {key}")) + } + + pub(crate) fn gocache(&self) -> &Path { + self.env_as_path("GOCACHE") + } + + pub(crate) fn gotmpdir(&self) -> &Path { + self.env_as_path("GOTMPDIR") + } + + pub(crate) fn path_env(&self) -> OsString { + let mut paths = vec![workspace_root().join("target/debug"), self.root.join("bin")]; + if let Some(path_var) = var_os("PATH") { + paths.extend(split_paths(&path_var)); + } + join_paths(paths).unwrap() + } + + pub(crate) fn envs(&self) -> impl Iterator { + self.envs.iter().map(|(key, value)| (key, value)) + } +} + +pub(crate) fn prepare_go_checkout_for_run(harness_name: &str) -> GoCheckout { + let base = prepare_base_go_checkout(); + let run_dir = cache_dir().join("runs").join(harness_name); + let go_root = run_dir.join(format!("go-{GO_REV}")); + + if go_root.try_exists().unwrap() { + remove_dir_all(&go_root).unwrap(); + } + create_dir_all(&run_dir).unwrap(); + copy_dir(base.root(), &go_root); + + let envs = go_envs(&run_dir); + GoCheckout { + root: go_root, + run_dir, + envs, + } +} + +fn prepare_base_go_checkout() -> GoCheckout { + let base_dir = cache_dir().join("base"); + let go_root = base_dir.join(format!("go-{GO_REV}")); + let envs = go_envs(&base_dir); + let smtp_dir = go_root.join("src/net/smtp"); + + if smtp_dir.join("necessist.db").is_file() { + eprintln!( + "Using cached necessist.db in {}", + display_path(&smtp_dir).display() + ); + return GoCheckout { + root: go_root, + run_dir: base_dir, + envs, + }; + } + + let existing_dir = cache_dir().join(format!("go-{GO_REV}")); + let existing_db = existing_dir.join("src/net/smtp/necessist.db"); + if existing_db.is_file() { + create_dir_all(&base_dir).unwrap(); + copy_dir(&existing_dir, &go_root); + eprintln!( + "Using cached necessist.db in {}", + display_path(&smtp_dir).display() + ); + return GoCheckout { + root: go_root, + run_dir: base_dir, + envs, + }; + } + + eprintln!( + "Preparing necessist.db in {}", + display_path(&smtp_dir).display() + ); + + if !go_root.join(".git").is_dir() { + create_dir_all(&base_dir).unwrap(); + command_output( + Command::new("git") + .arg("init") + .arg(&go_root) + .current_dir(&base_dir), + ); + command_output( + Command::new("git") + .args(["fetch", "--depth=1", GO_REPO, GO_REV]) + .current_dir(&go_root), + ); + command_output( + Command::new("git") + .args(["checkout", "--detach", "FETCH_HEAD"]) + .current_dir(&go_root), + ); + } + + if !go_root.join("bin/go").is_file() { + command_output( + Command::new("bash") + .arg("make.bash") + .current_dir(go_root.join("src")) + .envs(envs.iter().map(|(key, value)| (key, value))), + ); + } + + let go_checkout = GoCheckout { + root: go_root, + run_dir: base_dir, + envs, + }; + prepare_necessist_db(&go_checkout, &smtp_dir); + assert!( + smtp_dir.join("necessist.db").is_file(), + "necessist.db was not created in {}", + display_path(&smtp_dir).display() + ); + + go_checkout +} + +fn copy_dir(from: &Path, to: &Path) { + create_dir_all(to).unwrap(); + for entry in read_dir(from).unwrap() { + let entry = entry.unwrap(); + let from_path = entry.path(); + let to_path = to.join(entry.file_name()); + let file_type = entry.file_type().unwrap(); + if file_type.is_dir() { + copy_dir(&from_path, &to_path); + } else if file_type.is_file() { + copy(&from_path, &to_path).unwrap(); + } else { + panic!( + "unexpected file of type {file_type:?} at: {}", + from_path.display() + ); + } + } +} + +fn prepare_necessist_db(go_checkout: &GoCheckout, smtp_dir: &Path) { + command_output( + Command::new("cargo") + .args([ + "run", + "--package=necessist", + "--", + "--framework=go", + "--reset", + &format!("--root={}", smtp_dir.to_string_lossy()), + "--timeout=5", + "smtp_test.go", + ]) + .current_dir(workspace_root()) + .env("PATH", go_checkout.path_env()) + .env("GOROOT", go_checkout.root()) + .envs(go_checkout.envs()), + ); +} + +fn go_envs(run_dir: &Path) -> Vec<(OsString, OsString)> { + let gocache = run_dir.join("gocache"); + let gotmpdir = run_dir.join("gotmpdir"); + create_dir_all(&gocache).unwrap(); + create_dir_all(&gotmpdir).unwrap(); + + let mut envs = vec![ + (OsString::from("CGO_ENABLED"), OsString::from("0")), + (OsString::from("GOCACHE"), gocache.into_os_string()), + (OsString::from("GOTMPDIR"), gotmpdir.into_os_string()), + ]; + + if cfg!(all(target_os = "macos", target_arch = "aarch64")) { + envs.extend([ + (OsString::from("GOHOSTARCH"), OsString::from("amd64")), + (OsString::from("GOARCH"), OsString::from("amd64")), + ]); + } + + envs +} diff --git a/necessist/tests/necessist_audit_support/mod.rs b/necessist/tests/necessist_audit_support/mod.rs new file mode 100644 index 00000000..98d2c946 --- /dev/null +++ b/necessist/tests/necessist_audit_support/mod.rs @@ -0,0 +1,3 @@ +pub(crate) mod accept; +pub(crate) mod go_checkout; +pub(crate) mod tool; diff --git a/necessist/tests/necessist_audit_support/tool.rs b/necessist/tests/necessist_audit_support/tool.rs new file mode 100644 index 00000000..abbe54b8 --- /dev/null +++ b/necessist/tests/necessist_audit_support/tool.rs @@ -0,0 +1,227 @@ +use super::super::{ + GO_REV, JSON_REPORT, OUTPUT_POLL_INTERVAL, TIMEOUT, display_path, format_output, skill_dir, +}; +use super::accept::contains_acceptable_result; +use super::go_checkout::{GoCheckout, prepare_go_checkout_for_run}; +use serde_json::{Value, from_str}; +use std::{ + env::vars_os, + fs::{read_to_string, remove_file}, + path::{Path, PathBuf}, + process::{Command, Output, Stdio}, + sync::{Mutex, OnceLock, PoisonError}, + thread::sleep, + time::Instant, +}; + +pub(crate) struct Tool { + pub(crate) name: &'static str, + pub(crate) tool_path: PathBuf, + args_fn: fn(&GoCheckout, String) -> Vec, + stdin_null: bool, +} + +impl Tool { + pub(crate) fn claude(tool_path: PathBuf) -> Self { + Self { + name: "claude", + tool_path, + args_fn: |_go_checkout, prompt| { + vec![ + format!("--add-dir={}", skill_dir().display()), + "--allowedTools=Bash,Read,Write".to_owned(), + "--permission-mode=acceptEdits".to_owned(), + "--print".to_owned(), + prompt, + ] + }, + stdin_null: false, + } + } + + pub(crate) fn codex(tool_path: PathBuf) -> Self { + Self { + name: "codex", + tool_path, + args_fn: codex_args, + stdin_null: true, + } + } + + pub(crate) fn run(&self) { + let go_checkout = { + let _guard = cache_lock().lock().unwrap_or_else(PoisonError::into_inner); + + eprintln!("Preparing cached Go checkout {GO_REV}"); + prepare_go_checkout_for_run(self.name) + }; + + let smtp_dir = go_checkout.root().join("src/net/smtp"); + let report_path = smtp_dir.join(JSON_REPORT); + + drop(remove_file(&report_path)); + eprintln!( + "Running {} in {}", + self.name, + display_path(&smtp_dir).display() + ); + + run_tool_command(&go_checkout, self, &smtp_dir, &report_path).unwrap_or_else(|error| { + panic!( + "{} did not produce an acceptable report: {error}", + self.name + ); + }); + } +} + +fn codex_args(go_checkout: &GoCheckout, prompt: String) -> Vec { + let mut builder = CodexCommandBuilder::new(go_checkout); + + builder.push_config("sandbox_workspace_write.network_access", "true"); + builder.push_config("shell_environment_policy.inherit", "\"none\""); + builder.push_env_config("CGO_ENABLED", &"0"); + builder.push_env_config("GOARCH", &"amd64"); + builder.push_env_config("GOCACHE", &go_checkout.gocache().display()); + builder.push_env_config("GOHOSTARCH", &"amd64"); + builder.push_env_config("GOROOT", &go_checkout.root().display()); + builder.push_env_config("GOTMPDIR", &go_checkout.gotmpdir().display()); + builder.push_env_config("PATH", &go_checkout.path_env().to_string_lossy()); + + builder.finish(prompt) +} + +struct CodexCommandBuilder { + args: Vec, +} + +impl CodexCommandBuilder { + fn new(go_checkout: &GoCheckout) -> Self { + Self { + args: vec![ + "exec".to_owned(), + format!("--add-dir={}", go_checkout.run_dir().to_string_lossy()), + "--sandbox=workspace-write".to_owned(), + ], + } + } + + fn push_config(&mut self, key: &str, value: &str) { + self.args.push(format!("--config={key}={value}")); + } + + fn push_env_config(&mut self, key: &str, value: &dyn std::fmt::Display) { + self.push_config( + &format!("shell_environment_policy.set.{key}"), + &format!("\"{value}\""), + ); + } + + fn finish(mut self, prompt: String) -> Vec { + self.args.push(prompt); + self.args + } +} + +fn run_tool_command( + go_checkout: &GoCheckout, + tool: &Tool, + smtp_dir: &Path, + report_path: &Path, +) -> Result<(), String> { + let prompt = format!( + "Use the Necessist audit skill at {}.", + skill_dir().join("SKILL.md").display() + ); + + let mut command = Command::new(&tool.tool_path); + command + .args((tool.args_fn)(go_checkout, prompt)) + .current_dir(smtp_dir) + .env_clear() + .envs(vars_os()) + .env("PATH", go_checkout.path_env()) + .env("GOROOT", go_checkout.root()) + .envs(go_checkout.envs()); + + if tool.stdin_null { + command.stdin(Stdio::null()); + } + + let mut child = command + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + let start = Instant::now(); + + loop { + if child.try_wait().unwrap().is_some() { + let output = child.wait_with_output().unwrap(); + if !output.status.success() { + return Err(format!("skill command failed\n{}", format_output(&output))); + } + + if !report_path.is_file() { + return Err(format!( + "`{JSON_REPORT}` was not created\n{}", + format_output(&output) + )); + } + + eprintln!("Verifying {JSON_REPORT} produced by {}", tool.name); + return verify_json_report(report_path, Some(&output)); + } + + if report_path.is_file() && verify_json_report(report_path, None).is_ok() { + eprintln!("Verifying {JSON_REPORT} produced by {}", tool.name); + drop(child.kill()); + drop(child.wait()); + return Ok(()); + } + + if start.elapsed() >= TIMEOUT { + drop(child.kill()); + drop(child.wait()); + return Err(format!("skill command timed out after {TIMEOUT:?}")); + } + + sleep(OUTPUT_POLL_INTERVAL); + } +} + +fn verify_json_report(report_path: &Path, output: Option<&Output>) -> Result<(), String> { + let contents = read_to_string(report_path).unwrap(); + let value: Value = from_str(&contents).unwrap(); + let object = value.as_object().unwrap(); + + assert_eq!( + object.get("version"), + Some(&Value::String("0.1.0".to_owned())) + ); + + let findings = json_array(object.get("findings"), "findings"); + let leads = json_array(object.get("leads"), "leads"); + + if contains_acceptable_result(findings, leads) { + return Ok(()); + } + + let mut msg = format!("`findings`/`leads` do not contain an acceptable result in {contents}"); + if let Some(output) = output { + msg.push('\n'); + msg.push_str(&format_output(output)); + } + Err(msg) +} + +fn json_array<'a>(value: Option<&'a Value>, key: &str) -> &'a Vec { + value + .and_then(Value::as_array) + .unwrap_or_else(|| panic!("missing {key} array")) +} + +fn cache_lock() -> &'static Mutex<()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(Mutex::default) +} diff --git a/skills/necessist-audit/SKILL.md b/skills/necessist-audit/SKILL.md index dbbb044e..480bb750 100644 --- a/skills/necessist-audit/SKILL.md +++ b/skills/necessist-audit/SKILL.md @@ -34,13 +34,15 @@ Read passing removals with `necessist --dump`. Use read-only SQLite queries only ## Investigate removals -For each passing removal: +First scan all passing removals and prioritize removals most likely to expose a real defect, including removed assertions, error checks, synchronization, channel operations, waits, cleanup, setup, and calls whose comments or names imply verification. Investigate those before benign-looking removals such as duplicate assignments, redundant initialization, platform skips, logging, or unreachable branches. + +For each passing removal investigated: 1. Inspect the removal and the complete affected test. 2. Infer the intended behavior from tests, documentation, comments, related tests, and implementation. Determine why the test passes without the removed operation, and form a concrete bug hypothesis when the removal appears meaningful. 3. Seek supporting or refuting evidence in the affected test, implementation, callers, and focused non-mutating diagnostics. 4. Consider benign explanations, including idempotence, duplicate setup, unreachable conditions, equivalent operations, nondeterminism, and persistent state. Do not treat a single rerun as proof that a flaky result is stable. -5. Before reporting a finding or lead, confirm that its recorded source location and removed text match the current checkout. If they do not, mark the result as stale and recommend rerunning Necessist. Otherwise, cite repository-relative locations and the evidence supporting the conclusion. +5. Before reporting a finding or lead, confirm that its recorded source location and removed text match the current checkout. If they do not, mark the result as stale and recommend rerunning Necessist. Otherwise, cite locations consistently, using paths relative to the audited directory or repository root. Do not infer that a passing removal is a bug merely because Necessist reports it. @@ -55,8 +57,14 @@ Classify a result as a finding only when all of the following are established: If any required element is missing, classify the result as a lead and state what evidence is missing. When uncertain, classify the result as a lead. +Treat removed assertions, error checks, synchronization, channel operations, waits, cleanup, setup, and verification calls as leads unless the current checkout shows a clear benign explanation. A benign explanation must explain why the specific intended check still runs and is observed after the removal, not merely why the test process still completes. If focused diagnostics are unavailable or inconclusive, preserve the result as a lead rather than classifying it as no bug established. + +For synchronization, channel operations, waits, sleeps, joins, callbacks, and goroutine or task coordination, do not treat other ordering or eventual completion as a clear benign explanation unless the current checkout shows what remaining synchronization or ordering makes the intended check run and be observed. If the removed operation may be the only reason the test waits for an asynchronous check, callback, error path, or assertion to run, classify the result as a lead even if the test has other waits or protocol-completion steps. + ## Report +Report findings and leads in Markdown. After investigating each high-priority removal, immediately classify it as a finding, lead, no-bug, or stale. Write `necessist-audit.json` as soon as a credible finding is established or after a small initial batch of high-priority removals has produced only leads, then continue broader review. Write the Markdown report after the broader review is complete or the user's time or budget limit is reached. + Order findings by likely impact. For each finding, report: - removed code and source location; @@ -67,6 +75,51 @@ Order findings by likely impact. For each finding, report: - supporting evidence; - suggested fix. -List leads separately. End with counts of passing removals examined, findings, results for which no bug was established, and stale results. +List leads separately. End with explicit counts of passing removals examined, findings, leads, results for which no bug was established, and stale results. Those counts should match the number of passing removals examined; if they do not because some removals were skipped or could not be read, explain why. + +Write the machine-readable JSON report in the audited directory; this write is permitted even though source modifications are not. Always write the file, using empty arrays when no findings or leads are established. Include only findings and leads; put each result’s explanation, evidence, impact, and suggested fix in `details`. The JSON report must satisfy this JSON Schema: + +```json +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "type": "object", + "additionalProperties": false, + "required": ["version", "findings", "leads"], + "properties": { + "version": { + "type": "string", + "const": "0.1.0" + }, + "findings": { + "type": "array", + "items": { "$ref": "#/$defs/result" } + }, + "leads": { + "type": "array", + "items": { "$ref": "#/$defs/result" } + } + }, + "$defs": { + "result": { + "type": "object", + "additionalProperties": false, + "required": [ + "id", + "removed_code", + "removed_location", + "affected_location", + "details" + ], + "properties": { + "id": { "type": "string" }, + "removed_code": { "type": "string" }, + "removed_location": { "type": "string" }, + "affected_location": { "type": "string" }, + "details": { "type": "string" } + } + } + } +} +``` Use concise Markdown. Do not implement recommendations unless the user asks.