From dd39f5b5fab1ca96231d5117e0977e5f77a7cf44 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Jul 2026 14:19:43 +0000 Subject: [PATCH 1/2] test(gates): require every blocking gate to prove it can fail The 2026-07-25 review found two load-bearing rules that were green while broken: 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. PR #995 fixed both instances. This is the class. A gate verified only against a green tree is indistinguishable from `exit 0` -- delete its detector and CI stays green. Measured across the registry: 32 of 54 BLOCKING script-backed gates had no test anywhere under tests/ demonstrating they FAIL on what they claim to detect. 17 had no test at all, including skill.schema and skill.triggers, which gate every SKILL.md in the corpus. The closure lives in Go because the registry does. An earlier bash/Python projection of seed.go missed four gates registered outside that file (always.git-config-hygiene, always.retrieval-manifest-paths, contract.verdict-corpus, workflow.install-drift) -- the authority is gates.Default.All(), and reading it any other way reproduces exactly the projection-drift ADR-0016 is named after. It mirrors the existing TestRatchetLibConsumersRouteLibEdits closure and is self-extending. Mechanism is the repo's proven shrink-only ratchet: the gates lacking a witness are pinned in scripts/.gate-negative-witness-grandfather; a NEW blocking gate must ship with a witness; a pinned gate that gains one must be pruned; a pinned gate that leaves the registry must be pruned; and the growth guard reads HEAD, so a change cannot exempt its own new gate in the same diff. The surviving count prints on every run. Ratcheted down by one immediately, to prove the mechanism moves rather than just counts: tests/scripts/validate-skill-schema.bats is a real witness for skill.schema (32 -> 31). It drives the actual validator against a fixture tree and asserts exit 1 on each missing required field. One fixture-fidelity trap worth recording: the first draft's negatives passed for the WRONG reason. Descriptions in this corpus embed `Triggers: "x"`, and unquoted that colon makes YAML read a nested mapping -- so the gate failed on a parse error, not the schema violation each test claimed to witness. Caught because the positive case failed too. Descriptions are now single-quoted and the missing-name case is confirmed to fail on "'name' is a required property". Verified: the meta-test's own three failure directions each seeded and observed (unpin a witness-less gate -> FAIL; pin a gate that gained a witness -> FAIL demanding the prune; pin a gate absent from the registry -> FAIL); 5/5 bats on the new witness; go build, go vet, full go test ./... green; the bats path-filter wiring tests green; make regen-check clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CJNsMBvvKCYktzaFo3E6Fu --- .../gates/checks/negative_witness_test.go | 185 ++++++++++++++++++ scripts/.gate-negative-witness-grandfather | 50 +++++ tests/scripts/validate-skill-schema.bats | 89 +++++++++ 3 files changed, 324 insertions(+) create mode 100644 cli/internal/gates/checks/negative_witness_test.go create mode 100644 scripts/.gate-negative-witness-grandfather create mode 100644 tests/scripts/validate-skill-schema.bats 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..b741d55e9 --- /dev/null +++ b/cli/internal/gates/checks/negative_witness_test.go @@ -0,0 +1,185 @@ +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 \]`, +) + +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) + // TestMain scrubs GIT_DIR/GIT_WORK_TREE for this package, and cmd.Dir is set + // explicitly, so this read cannot be redirected at another repository + // (.claude/rules/go.md, ek8v). + cmd := exec.Command("git", "show", "HEAD:"+negativeWitnessGrandfather) + cmd.Dir = root + 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 ] +} From e3462696d3c47275721184d02912c5c3d358e327 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 26 Jul 2026 14:26:19 +0000 Subject: [PATCH 2/2] fix(gates): scrub git discovery env per-command in the check-liveness test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit go.test-isolation caught the new test: its git-exec env-scrub ratchet rose from 3 to 4 because negative_witness_test.go execs `git show` with cmd.Dir set but no cmd.Env. Relying on TestMain's process-level ScrubGitDiscoveryEnv was not enough — the repo's contract is a per-command scrub, and it should not depend on a TestMain elsewhere in the package staying correct. Under a hook-leaked GIT_DIR even a directory-scoped git call resolves against the LEAKED repository (age-gate-scripts-worktree-gitdir-p62wo, ek8v). Adds a package-local scrubbedGitEnv twin of gitDiscoveryEnv and wires it into the one git call. Ratchet is back at baseline 3 with this file no longer listed. Worth recording for the PR's own subject matter: go.test-isolation is on the check-liveness allowlist as lacking a proven negative — and it just caught a real defect anyway. The pin measures TEST COVERAGE of a gate, not its efficacy. An unproven gate may work fine; the point is that nothing would tell us if it stopped. Verified: check-test-isolation.sh PASS (git-unscrubbed=3/3); full go test ./... green; meta-test green at 31; growth guard still fires on a seeded self-allowlist. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CJNsMBvvKCYktzaFo3E6Fu --- .../gates/checks/negative_witness_test.go | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/cli/internal/gates/checks/negative_witness_test.go b/cli/internal/gates/checks/negative_witness_test.go index b741d55e9..270aba715 100644 --- a/cli/internal/gates/checks/negative_witness_test.go +++ b/cli/internal/gates/checks/negative_witness_test.go @@ -46,6 +46,31 @@ 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{} @@ -159,11 +184,16 @@ func TestBlockingGatesHaveProvenNegativeWitness(t *testing.T) { // grants no protection. func TestNegativeWitnessAllowlistOnlyShrinks(t *testing.T) { root := repoRootFromTest(t) - // TestMain scrubs GIT_DIR/GIT_WORK_TREE for this package, and cmd.Dir is set - // explicitly, so this read cannot be redirected at another repository - // (.claude/rules/go.md, ek8v). + // 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)")