diff --git a/cli/internal/gates/checks/negative_witness_test.go b/cli/internal/gates/checks/negative_witness_test.go new file mode 100644 index 000000000..270aba715 --- /dev/null +++ b/cli/internal/gates/checks/negative_witness_test.go @@ -0,0 +1,215 @@ +package checks + +import ( + "bufio" + "os" + "os/exec" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/boshu2/agentops/cli/internal/gates" +) + +// Check-liveness closure: every BLOCKING script-backed gate must have a test +// that demonstrates it FAILING on the thing it claims to detect. +// +// WHY: the 2026-07-25 review found two load-bearing rules that existed only as +// text — ADR-0016's "Python never ships in skills" with no gate, and an +// exact-identity digest contract whose test mocked Validate with the code under +// test's own digest function. Both were green while broken. Those were +// instances of one class: a rule nobody executes, and a check nobody proved +// executes. A gate verified only on a green tree is indistinguishable from +// `exit 0` — it would still pass with its detector deleted. This test is the +// standing requirement the review asked for. +// +// SHAPE: a shrink-only ratchet, the same mechanism the repo already uses for +// preamble adoption and shipped Python. The gates lacking a witness at the +// cutoff are pinned in scripts/.gate-negative-witness-grandfather and exempt; +// a NEW blocking gate must ship with a witness; a pinned gate that gains one +// must be pruned. The list cannot grow — the growth guard below rejects any +// entry not present at HEAD, so a change cannot exempt its own new gate. +// +// WHAT COUNTS: any file under tests/ that names the backing script AND asserts +// a non-zero outcome from it. Deliberately generous about form — bats +// (`[ "$status" -eq 1 ]`), shell integration tests, and Python +// (`returncode != 0`) all qualify. The bar is "somebody proved it can fail", +// not a particular framework. +const negativeWitnessGrandfather = "scripts/.gate-negative-witness-grandfather" + +// negativeAssertion matches the ways this repo's tests assert a failing run. +// Kept broad on purpose: a false NEGATIVE here (missing a real witness) would +// wrongly pin a well-tested gate, which is merely noisy; the ratchet's teeth +// come from the growth guard, not from this regex being exhaustive. +var negativeAssertion = regexp.MustCompile( + `\$status" -eq [1-9]|\$status" -ne 0|assert_failure|refute_success|returncode, 1|returncode != 0|-ne 0 \]`, +) + +// scrubbedGitEnv is the package-local twin of gitDiscoveryEnv +// (cli/cmd/ao/git_read.go): the process environment minus the four git +// discovery overrides. A hook-launched process inherits GIT_DIR/GIT_WORK_TREE, +// and under a leaked GIT_DIR even a `-C `-scoped git call resolves against +// the LEAKED repository — which is how a fixture `git init` once rewrote a +// shared .git/config and bricked every worktree +// (age-gate-scripts-worktree-gitdir-p62wo, ek8v). +func scrubbedGitEnv() []string { + const prefixes = "GIT_DIR=|GIT_WORK_TREE=|GIT_COMMON_DIR=|GIT_INDEX_FILE=" + env := make([]string, 0, len(os.Environ())) + for _, entry := range os.Environ() { + drop := false + for _, p := range strings.Split(prefixes, "|") { + if strings.HasPrefix(entry, p) { + drop = true + break + } + } + if !drop { + env = append(env, entry) + } + } + return env +} + +func loadPinnedGates(t *testing.T, root string) map[string]bool { + t.Helper() + pinned := map[string]bool{} + f, err := os.Open(filepath.Join(root, negativeWitnessGrandfather)) + if err != nil { + t.Fatalf("read %s: %v", negativeWitnessGrandfather, err) + } + defer f.Close() + scanner := bufio.NewScanner(f) + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + pinned[line] = true + } + if err := scanner.Err(); err != nil { + t.Fatalf("scan %s: %v", negativeWitnessGrandfather, err) + } + return pinned +} + +// collectTestBodies reads every file under tests/ once. +func collectTestBodies(t *testing.T, root string) map[string]string { + t.Helper() + bodies := map[string]string{} + testsDir := filepath.Join(root, "tests") + err := filepath.WalkDir(testsDir, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + body, readErr := os.ReadFile(path) + if readErr != nil { + return nil // unreadable fixture is not this test's business + } + bodies[path] = string(body) + return nil + }) + if err != nil { + t.Fatalf("walk tests/: %v", err) + } + return bodies +} + +// hasNegativeWitness reports whether some test names the backing script and +// asserts a non-zero outcome. +func hasNegativeWitness(bodies map[string]string, backing string) bool { + base := filepath.Base(backing) + for _, body := range bodies { + if strings.Contains(body, base) && negativeAssertion.MatchString(body) { + return true + } + } + return false +} + +func TestBlockingGatesHaveProvenNegativeWitness(t *testing.T) { + root := repoRootFromTest(t) + pinned := loadPinnedGates(t, root) + bodies := collectTestBodies(t, root) + + proven := map[string]bool{} + for _, c := range gates.Default.All() { + if c.Backing == "" || !c.Blocking { + continue + } + if hasNegativeWitness(bodies, c.Backing) { + proven[c.ID] = true + continue + } + if pinned[c.ID] { + continue // grandfathered: inert, but visibly and countably so + } + t.Errorf("blocking gate %q (backing %s) has no test proving it FAILS on what it detects.\n"+ + " Add a test under tests/ that names %s and asserts a non-zero outcome.\n"+ + " Pinning it in %s is NOT a repair — the growth guard rejects new entries.", + c.ID, c.Backing, filepath.Base(c.Backing), negativeWitnessGrandfather) + } + + // Shrink direction: a pinned gate that now has a witness must be pruned, so + // the count can only go down. + for id := range pinned { + if proven[id] { + t.Errorf("gate %q now has a negative witness but is still pinned in %s — remove that line (the allowlist only shrinks)", + id, negativeWitnessGrandfather) + } + } + + // Stale direction: a pinned gate that no longer exists in the registry is + // dead weight hiding the real count. + live := map[string]bool{} + for _, c := range gates.Default.All() { + live[c.ID] = true + } + for id := range pinned { + if !live[id] { + t.Errorf("gate %q is pinned in %s but is not in the registry — remove that line", id, negativeWitnessGrandfather) + } + } + + t.Logf("check-liveness: %d blocking gate(s) still lack a proven negative witness (shrink-only)", len(pinned)) +} + +// TestNegativeWitnessAllowlistOnlyShrinks is the growth guard. Without it the +// ratchet has no teeth: a change could add a new blocking gate and exempt it in +// the same diff, which is exactly the "rule nobody executes" pattern this whole +// mechanism exists to stop. HEAD is the authority — a same-diff self-allowlist +// grants no protection. +func TestNegativeWitnessAllowlistOnlyShrinks(t *testing.T) { + root := repoRootFromTest(t) + // cmd.Dir pins the repository and cmd.Env strips the git discovery overrides. + // TestMain's process-level ScrubGitDiscoveryEnv already covers this package, + // but the per-command scrub is the repo's stated contract and does not rely + // on a TestMain elsewhere in the package staying correct + // (age-gate-scripts-worktree-gitdir-p62wo; gitDiscoveryEnv in + // cli/cmd/ao/git_read.go). A hook-leaked GIT_DIR would otherwise resolve this + // read against a different repository entirely. + cmd := exec.Command("git", "show", "HEAD:"+negativeWitnessGrandfather) + cmd.Dir = root + cmd.Env = scrubbedGitEnv() + out, err := cmd.Output() + if err != nil { + t.Skip("no HEAD version of the allowlist (initial snapshot)") + } + base := map[string]bool{} + for _, line := range strings.Split(string(out), "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + base[line] = true + } + for id := range loadPinnedGates(t, root) { + if !base[id] { + t.Errorf("%s gained entry %q — the allowlist only SHRINKS. A new blocking gate ships with a negative witness or it does not ship.", + negativeWitnessGrandfather, id) + } + } +} diff --git a/scripts/.gate-negative-witness-grandfather b/scripts/.gate-negative-witness-grandfather new file mode 100644 index 000000000..9bb2e5325 --- /dev/null +++ b/scripts/.gate-negative-witness-grandfather @@ -0,0 +1,50 @@ +# scripts/.gate-negative-witness-grandfather — check-liveness ratchet allowlist. +# +# Every BLOCKING script-backed gate in the Go registry that, at the ratchet +# cutoff, has no test anywhere under tests/ demonstrating it FAILS on the thing +# it claims to detect. A gate proven only on a green tree is indistinguishable +# from `exit 0`: it would still pass with its detector removed. +# +# This is the generalization of the two inert rules the 2026-07-25 review found +# (ADR-0016 with no gate; a digest contract whose test mocked the code under +# test). Those were instances; this is the class. +# +# SHRINK-ONLY: the list only shrinks. When a gate gains a negative witness, its +# line MUST be pruned (the closure test FAILS until it is). A NEW blocking gate +# cannot be added here — it ships with a negative witness or it does not ship. +# +# A "negative witness" is any test under tests/ that names the backing script +# AND asserts a non-zero outcome from it. +# +# Enforced by: cli/internal/gates/checks/negative_witness_test.go +adapter.gc-executor +always.file-manifest-overlap +always.git-config-hygiene +always.mutation-route +always.quarantine-empty +always.regen-all +always.retrieval-manifest-paths +ci.policy-parity +contract.cathedral-cut +contract.compatibility +contract.finding-registry +contract.skill-mesh +contract.verdict-corpus +derived.changed-scope +go.cli-contract +go.cli-reference +go.cli-surface-counts +go.command-test-pair +go.complexity +go.home-isolation +go.test-home-isolation +go.test-isolation +skill.cli-snippets +skill.codex-override-coverage +skill.codex-parity-drift +skill.codex-runtime-sections +skill.heal-strict +skill.runtime-formats +skill.runtime-parity +skill.triggers +workflow.install-drift diff --git a/tests/scripts/validate-skill-schema.bats b/tests/scripts/validate-skill-schema.bats new file mode 100644 index 000000000..6b142e364 --- /dev/null +++ b/tests/scripts/validate-skill-schema.bats @@ -0,0 +1,89 @@ +#!/usr/bin/env bats +# Tests for scripts/validate-skill-schema.sh — the skill.schema blocking gate. +# +# WHY THIS EXISTS: skill.schema gates every SKILL.md in the corpus and, until +# now, had no test at all. Nothing had ever demonstrated it can FAIL. A gate +# proven only against a green tree is indistinguishable from `exit 0` — replace +# its validator with a no-op and CI stays green. This file is the negative +# witness, and it prunes skill.schema from +# scripts/.gate-negative-witness-grandfather (the check-liveness ratchet in +# cli/internal/gates/checks/negative_witness_test.go). +# +# The script anchors REPO_ROOT to `dirname $0/..`, so a fixture tree with its +# own scripts/ + schemas/ + skills/ exercises the real validator against +# controlled input rather than against the live corpus. + +setup() { + REPO_ROOT="$(cd "$BATS_TEST_DIRNAME/../.." && pwd)" + TMP_DIR="$(mktemp -d)" + mkdir -p "$TMP_DIR/scripts" "$TMP_DIR/schemas" "$TMP_DIR/skills" + cp "$REPO_ROOT/scripts/validate-skill-schema.sh" "$TMP_DIR/scripts/" + cp "$REPO_ROOT/schemas/skill-frontmatter.v1.schema.json" "$TMP_DIR/schemas/" + chmod +x "$TMP_DIR/scripts/validate-skill-schema.sh" +} + +teardown() { + rm -rf "$TMP_DIR" +} + +write_skill() { # write_skill + mkdir -p "$TMP_DIR/skills/$1" + { printf -- '---\n'; printf '%s\n' "$2"; printf -- '---\n\n# %s\n' "$1"; } \ + > "$TMP_DIR/skills/$1/SKILL.md" +} + +run_gate() { + ( cd "$TMP_DIR" && bash scripts/validate-skill-schema.sh ) +} + +# Descriptions are single-quoted: the corpus convention embeds `Triggers: "x"` +# inside the value, and unquoted that colon makes YAML read a nested mapping — +# which fails the gate for a PARSE error rather than the schema violation each +# negative below is meant to witness. +valid_frontmatter() { + printf "name: alpha\ndescription: 'A valid fixture skill. Triggers: \"alpha\".'\nskill_api_version: 1" +} + +@test "a schema-valid skill passes" { + write_skill alpha "$(valid_frontmatter)" + run run_gate + [ "$status" -eq 0 ] +} + +# --- negative witnesses: the gate must be shown to FAIL ---------------------- + +@test "NEGATIVE: a skill missing the required name field fails" { + write_skill alpha "$(valid_frontmatter)" + write_skill broken "description: 'Missing its name. Triggers: \"broken\".' +skill_api_version: 1" + run run_gate + [ "$status" -eq 1 ] + [[ "$output" == *broken* ]] +} + +@test "NEGATIVE: a skill missing the required description field fails" { + write_skill alpha "$(valid_frontmatter)" + write_skill broken 'name: broken +skill_api_version: 1' + run run_gate + [ "$status" -eq 1 ] + [[ "$output" == *broken* ]] +} + +@test "NEGATIVE: a skill missing skill_api_version fails" { + write_skill alpha "$(valid_frontmatter)" + write_skill broken "name: broken +description: 'Has no api version. Triggers: \"broken\".'" + run run_gate + [ "$status" -eq 1 ] + [[ "$output" == *broken* ]] +} + +@test "NEGATIVE: one bad skill fails the run even when others are valid" { + write_skill alpha "$(valid_frontmatter)" + write_skill beta "$(printf "name: beta\ndescription: 'Another valid one. Triggers: \"beta\".'\nskill_api_version: 1")" + write_skill broken "description: 'no name here' +skill_api_version: 1" + run run_gate + [ "$status" -eq 1 ] +}