test(skills): the skill-body control reads the tree, so no body is left unchecked

candy/plugin-box/skill_doc_flags_test.go

@@ -4,6 +4,7 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
+	"slices"
 	"strings"
 	"testing"
 
@@ -25,6 +26,13 @@ import (
 // actually defines. Kong is the authority, not a hand-written list: the flag names and the env
 // bindings come from the grammar structs themselves, so the surface cannot drift.
 //
+// The SET of bodies it reads is read from the tree too, because a body the control does not read is
+// a body the control cannot check. A hand-kept list of them is a list that drifts the moment a body
+// is added, and the drift is invisible: the control stays green over a body it has never seen
+// (opencharly/plugin-box#67). So the TREE names the bodies and the cases name what each one
+// documents, and the control asserts BOTH directions — every body in the tree has a case, every case
+// names a body that exists — and that no body is read vacuously.
+//
 // It is a Go test rather than a prose review because a prose review cannot fail, and a control that
 // cannot fail is not coverage (R7).
 
@@ -90,6 +98,56 @@ var skillDocCases = []skillDocCase{
 	{skill: "list", command: "charly box list", grammar: nil},
 }
 
+// skillsRoot is the tree the bodies live in — the SAME path the corpus generator reads them from.
+const skillsRoot = "../../.agents/skills"
+
+// skillBodyNames returns the names of the skill bodies in the tree: every directory under root that
+// holds a SKILL.md. The set is READ, never listed: a list of bodies is a list that is one body
+// short the moment a body is added, and the control that reads a list cannot see the body it does
+// not read.
+func skillBodyNames(root string) ([]string, error) {
+	entries, err := os.ReadDir(root)
+	if err != nil {
+		return nil, err
+	}
+	var names []string
+	for _, e := range entries {
+		if !e.IsDir() {
+			continue
+		}
+		if _, err := os.Stat(filepath.Join(root, e.Name(), "SKILL.md")); err != nil {
+			continue
+		}
+		names = append(names, e.Name())
+	}
+	return names, nil
+}
+
+// skillDocCoverage compares the bodies in the tree with the cases the control reads, and returns
+// the bodies NO case covers and the cases that name NO body. Both directions are findings: an
+// uncovered body is read by nothing, and a stale case asserts about a body that is not there.
+func skillDocCoverage(root string, cases []skillDocCase) (uncovered, stale []string, err error) {
+	bodies, err := skillBodyNames(root)
+	if err != nil {
+		return nil, nil, err
+	}
+	covered := make(map[string]bool, len(cases))
+	for _, tc := range cases {
+		covered[tc.skill] = true
+	}
+	for _, name := range bodies {
+		if !covered[name] {
+			uncovered = append(uncovered, name)
+		}
+	}
+	for _, tc := range cases {
+		if !slices.Contains(bodies, tc.skill) {
+			stale = append(stale, tc.skill)
+		}
+	}
+	return uncovered, stale, nil
+}
+
 // kongSurface returns the flag names and env bindings a Kong grammar defines. It walks Kong's own
 // model, so the answer is what the parser actually accepts — not a list maintained beside it.
 func kongSurface(t *testing.T, grammar any) (flags map[string]bool, envs map[string]bool) {
@@ -242,17 +300,37 @@ func flagSectionBullets(body []string) []docClaim {
 // command must be a flag the command accepts, and every environment variable it pairs with one of
 // its flags must be one of the command's own bindings.
 func TestSkillBodiesDocumentOnlyRealFlags(t *testing.T) {
+	uncovered, stale, err := skillDocCoverage(skillsRoot, skillDocCases)
+	if err != nil {
+		t.Fatalf("read %s: %v", skillsRoot, err)
+	}
+	for _, name := range uncovered {
+		t.Errorf("the skill body %s is not covered: add its command and grammar to skillDocCases, or the control reads nothing in it", name)
+	}
+	for _, name := range stale {
+		t.Errorf("skillDocCases names %s, which is no skill body in %s", name, skillsRoot)
+	}
+
 	for _, tc := range skillDocCases {
 		t.Run(tc.skill, func(t *testing.T) {
-			path := filepath.Join("..", "..", ".agents", "skills", tc.skill, "SKILL.md")
+			path := filepath.Join(skillsRoot, tc.skill, "SKILL.md")
 			raw, err := os.ReadFile(path)
 			if err != nil {
 				t.Fatalf("read %s: %v", path, err)
 			}
 			body := strings.Split(string(raw), "\n")
 			flags, envs := kongSurface(t, tc.grammar)
 
-			for _, claim := range usageClaims(body, tc.command) {
+			claims := usageClaims(body, tc.command)
+			if len(claims) == 0 {
+				// A body that never invokes its own command is read by NOTHING here: the usage
+				// claims are what tie a flag in the prose to this command's grammar, and a body
+				// with none of them passes every assertion below without being compared to
+				// anything. A control that reads nothing is not coverage.
+				t.Errorf("%s: the body makes no usage claim for %s, so the control compares nothing in it", path, tc.command)
+			}
+
+			for _, claim := range claims {
 				for _, m := range flagToken.FindAllString(claim.text, -1) {
 					name := strings.TrimPrefix(m, "--")
 					if !flags[name] {
@@ -301,3 +379,47 @@ func TestSkillBodiesDocumentOnlyRealFlags(t *testing.T) {
 		})
 	}
 }
+
+// TestSkillDocCoverageIsEffective is the coverage check's own RED ARM: it must fail on a body no
+// case covers, and on a case that names no body. A coverage check that cannot fail proves nothing,
+// and a coverage check that cannot fail is the defect this control exists to catch wearing a
+// different hat — the control reads a set, and a set that is never compared with the tree is a set
+// that drifts.
+func TestSkillDocCoverageIsEffective(t *testing.T) {
+	root := t.TempDir()
+	body := filepath.Join(root, "extra")
+	if err := os.MkdirAll(body, 0o755); err != nil {
+		t.Fatalf("mkdir %s: %v", body, err)
+	}
+	if err := os.WriteFile(filepath.Join(body, "SKILL.md"), []byte("---\nname: extra\ndescription: a body no case covers\n---\n"), 0o644); err != nil {
+		t.Fatalf("write the body: %v", err)
+	}
+
+	cases := []skillDocCase{{skill: "extra", command: "charly box extra"}}
+	uncovered, stale, err := skillDocCoverage(root, cases)
+	if err != nil {
+		t.Fatalf("coverage: %v", err)
+	}
+	if len(uncovered) != 0 || len(stale) != 0 {
+		t.Fatalf("the covered tree reported uncovered=%v stale=%v", uncovered, stale)
+	}
+
+	// The case is dropped: the body in the tree is read by nothing, and the check must say so.
+	uncovered, stale, err = skillDocCoverage(root, nil)
+	if err != nil {
+		t.Fatalf("coverage: %v", err)
+	}
+	if !slices.Contains(uncovered, "extra") {
+		t.Fatalf("the coverage check did not report the uncovered body: %v", uncovered)
+	}
+
+	// A case names a body that is not there: the check must say that too.
+	cases = append(cases, skillDocCase{skill: "absent", command: "charly box absent"})
+	uncovered, stale, err = skillDocCoverage(root, cases)
+	if err != nil {
+		t.Fatalf("coverage: %v", err)
+	}
+	if !slices.Contains(stale, "absent") {
+		t.Fatalf("the coverage check did not report the stale case: %v", stale)
+	}
+}

charly.yml

@@ -459,7 +459,12 @@ check-box-local:
       # a project is the PROJECT ROOT — the layout every real skill owner uses (the candy
       # sits under `candy/`, its `.agents/skills/` at the repo root). The sdk's body
       # lookup resolves that root from the candy's source dir (deploykit/skills_collect.go
-      # → candyRepoRoot) and looks there; it did NOT look beside the candy.
+      # → candyRepoRoot) and looks there; it did NOT look beside the candy. MEASURED on
+      # this tree: the SAME project with the body BESIDE the candy is REFUSED — "no
+      # .agents/skills/psk1/SKILL.md exists at the repo root of the owning candy" — and
+      # with the body at the project root it validates clean. The arm at the end of this
+      # step pins that base, so the fixture's placement cannot drift back to a claim the
+      # lookup does not make (opencharly/plugin-box#67).
       # ---------------------------------------------------------------------------
       - check: >-
           an IMAGE candy owning a `skill:` is REJECTED with the ruling's message — it names
@@ -482,6 +487,12 @@ check-box-local:
             set -eu
             EV=".check/grammar-arms/box-skill-image-owner.log"
             mkdir -p "$(dirname "$EV")"
+            # The log is TRUNCATED here, so every assertion below reads THIS run's output. Appended
+            # to, the log keeps a previous run's lines, and an assertion like "exit: 0" can be
+            # satisfied by an EARLIER run's output while this run's arm fails — a control that can
+            # pass over a broken arm (measured: the arm below, with its body moved to the repo
+            # root, passed against the log the green run had left behind).
+            : > "$EV"
             D=$(mktemp -d)
             trap 'rm -rf "$D"' EXIT
             mkdir -p "$D/candy/pcandy"
@@ -593,6 +604,37 @@ check-box-local:
             if grep -q 'WARNING' "$EV" && grep 'WARNING' "$EV" | grep -qv 'dev/worktree build'; then
               echo 'FAIL: a warning appeared that is not the known benign dev/worktree notice'; cat "$EV"; exit 1
             fi
+            # THE BASE, PINNED. The SAME project with the body BESIDE the candy — the placement
+            # the lookup does NOT read — must be REFUSED, and the diagnostic must name the repo
+            # root it looked in. This arm is what keeps the fixture's placement honest: a fixture
+            # that put the body beside the candy would assert something the lookup does not do,
+            # and the bed would be green over a false claim. Its own log is separate, because the
+            # negative arm above also exits 1 and a shared log could satisfy either check.
+            EV2=".check/grammar-arms/box-skill-body-base.log"
+            mkdir -p "$(dirname "$EV2")"
+            # TRUNCATED, like EV above: the assertions below must read THIS run's arm, never a
+            # previous run's residue. Measured: appended to, the arm passed against the log the
+            # green run had left behind while the arm's own placement was wrong.
+            : > "$EV2"
+            D3=$(mktemp -d)
+            trap 'rm -rf "$D" "$D2" "$D3"' EXIT
+            mkdir -p "$D3/candy/pcandy/.agents/skills/psk1"
+            cp "$D2/charly.yml" "$D3/charly.yml"
+            cp "$D/candy/pcandy/charly.yml" "$D3/candy/pcandy/charly.yml"
+            printf -- '---\nname: psk1\ndescription: package-candy-owned skill\n---\n\nBody prose owned by the package candy.\n' \
+              > "$D3/candy/pcandy/.agents/skills/psk1/SKILL.md"
+            {
+              echo "--- arm: the same body BESIDE the candy, not at that candy's repo root ---"
+              set +e
+              BES=$("${CHARLY_BIN}" -C "$D3" box validate 2>&1)
+              BRC=$?
+              set -e
+              echo "exit: $BRC"
+              echo "$BES"
+            } >> "$EV2" 2>&1
+            grep -q '^exit: 1$' "$EV2" || { echo 'FAIL: a body BESIDE the candy validated clean, so the lookup base is not the repo root'; cat "$EV2"; exit 1; }
+            grep -q 'exists at the repo root of the owning candy' "$EV2" || { echo 'FAIL: the diagnostic does not name the repo root it looked in'; cat "$EV2"; exit 1; }
+            cat "$EV2"
             cat "$EV"
             echo BOX-SKILL-IMAGE-OWNER-OK
           in_container: false