diff --git a/candy/plugin-box/skill_doc_flags_test.go b/candy/plugin-box/skill_doc_flags_test.go index a382f64..bb66d5a 100644 --- a/candy/plugin-box/skill_doc_flags_test.go +++ b/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,9 +300,20 @@ 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) @@ -252,7 +321,16 @@ func TestSkillBodiesDocumentOnlyRealFlags(t *testing.T) { 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) + } +} diff --git a/charly.yml b/charly.yml index 7f83472..040de08 100644 --- a/charly.yml +++ b/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