From 350f885c923742f89e9f40afdd703879ca92b160 Mon Sep 17 00:00:00 2001 From: Andreas Trawoeger Date: Wed, 28 Oct 2026 20:38:34 +0100 Subject: [PATCH] test(skills): the skill-body control reads the tree, so no body is left unchecked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The control that keeps this repo's skill bodies true compared the bodies it reads against a hand-kept list of ten names. A body added under .agents/skills/ was read by NOTHING and the control stayed green — the drift is invisible, because the hand-kept list is only ever wrong in the one direction no assertion can see (opencharly/plugin-box#67). The TREE now names the bodies: the control derives the set from .agents/skills/ and asserts BOTH directions — every body in the tree has a case, every case names a body that exists — and that every body it reads makes at least one usage claim, so a case cannot pass by comparing nothing. The bed's skill-owner arm now pins the lookup's base, MEASURED: the body lookup reads the owning candy's REPO ROOT, and the same body BESIDE the candy is refused. Both that arm's log and the step's own log are TRUNCATED, so their assertions read this run and never a previous run's residue — a control that can pass over a broken arm is not coverage. Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated) --- candy/plugin-box/skill_doc_flags_test.go | 126 ++++++++++++++++++++++- charly.yml | 44 +++++++- 2 files changed, 167 insertions(+), 3 deletions(-) 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