Skip to content

fix(skills): the skill body's base is the owning candy's repo root, in the pin, the assertions and the prose - #74

Merged
github-actions[bot] merged 1 commit into
mainfrom
feat/skill-body-base-73
Oct 29, 2026
Merged

github-actions[bot] merged 1 commit into
mainfrom
feat/skill-body-base-73

Conversation

@aitrawog

Copy link
Copy Markdown
Contributor

Refs #73

Summary

Head b860c11b51e71f64b402d7f7704069722c9ad0f5 — 5 files changed, 125 insertions(+), 58 deletions(-).

This repo's own unit tests asserted a skill-body base that the org's build does not use. The base was unified onto the owning candy's REPO ROOT (sdk/deploykit/candy_repo_root.go → candyRepoRoot), and the sdk the org's build compiles resolves the body there. This repo's own go.mod still floored the sdk at a version from before that unification, and under THAT floor the body resolved at the owning candy's own directory instead.

The two bases disagree for every candy under candy/<name>/ — which is every real skill owner in this org — so the repo's own go test ./... was green over behavior the shipped binary does not have, and the repo's prose named the dead base:

  • candy/plugin-box/validate_skills.go said the body resolves at <owning candy SourceDir>/.agents/skills/<name>/SKILL.md — the OLD base.
  • charly.yml's check-box-local bed said the lookup "did NOT look beside the candy" — the ORG's answer, in a past tense that reads as the opposite of what it means, and in a file pointer (skills_collect.go) that no longer holds the base.

The charly/pr-validator review of #66 raised it as "One documentation nuance (not a functional defect)". It is a documentation nuance only because the prose picked ONE of the two answers without saying which build it held for — and the repo's own tests picked the other one.

What this change does:

file what it does
candy/plugin-box/go.mod the sdk floor is advanced to v0.2026297.1851 — the version the org's build compiles — so this repo's own build resolves bodies the way the shipped one does
candy/plugin-box/go.sum the checksums the new floor carries (74 insertions, 34 deletions)
candy/plugin-box/validate_skills.go the doc comment names the REPO ROOT base, and says why the two bases diverge
candy/plugin-box/validate_skills_test.go the three assertions that encoded the OLD base are flipped to the ONE base, and the file's header says why they are a floor guard
charly.yml the bed's prose is corrected: the past-tense negative becomes the present tense, the file pointer becomes candy_repo_root.go, and the sibling comment's stale pointer is fixed

The two prose statements and the three assertions now name ONE base, and the repo's own build proves it. A Go module pin is a FLOOR, not a ceiling: the org's build compiles this plugin against the HIGHEST sdk in the graph, so charly's own pin silently raised the floor above this repo's and moved the behavior under this repo's feet. Advancing the floor here closes that gap — and makes the flipped assertions a standing guard: lowering the pin again turns them RED.

How tested

Everything below ran on the committed tree this PR pushes (b860c11b51e71f64b402d7f7704069722c9ad0f5).

1. The repo's own gate — gofmt / go build / go vet / go test -count=1 in candy/plugin-box/.

# cd candy/plugin-box
$ gofmt -l .
GOFMT_RC=0 (empty list above = clean)
$ go build ./...
BUILD_RC=0
$ go vet ./...
VET_RC=0
$ go test -count=1 ./...
ok  	github.com/opencharly/plugin-box/candy/plugin-box	0.025s
?   	github.com/opencharly/plugin-box/candy/plugin-box/cmd/serve	[no test files]
TEST_RC=0

2. The structural check at the repo root.

$ charly box validate
charly box validate: OK — checked 1 candy, 1 deploy, 7 distros, 7 builders; 0 warnings, 0 errors
VALIDATE_RC=0

3. The R10 bed — check-box-local, this repo's live witness (a disposable: true kind:local
deploy driven by charly check run: build → artifact checks → realize → deployment checks → rebuild
fresh with charly update → checks again → remove).

$ charly check run check-box-local
charly check run check-box-local: [deploy-add] START (charly deploy add; log: .check/check-box-local/2026.302.0037/deploy-add.log)
charly check run check-box-local: [deploy-add] PASS after 1.216s (log: .check/check-box-local/2026.302.0037/deploy-add.log)
charly check run check-box-local: [bring-up-members] START
charly check run check-box-local: [bring-up-members] PASS after 0s
charly check run check-box-local: [check-live] START (charly check live; log: .check/check-box-local/2026.302.0037/check-live.log)
charly check run check-box-local: [check-live] PASS after 15.864s (log: .check/check-box-local/2026.302.0037/check-live.log)
charly check run check-box-local: [update] START (charly update; log: .check/check-box-local/2026.302.0037/update.log)
charly check run check-box-local: [update] PASS after 1.158s (log: .check/check-box-local/2026.302.0037/update.log)
charly check run check-box-local: [cleanup] START (charly deploy del; log: .check/check-box-local/2026.302.0037/cleanup.log)
charly check run check-box-local: [cleanup] PASS after 1.164s (log: .check/check-box-local/2026.302.0037/cleanup.log)
charly check run check-box-local: [cleanup-members] START
charly check run check-box-local: [cleanup-members] PASS after 0s
charly check run check-box-local: PASS (steps=6)
BED_RC=0

The bed's own arms include the base: the arm that puts the body BESIDE the candy must be REFUSED
and the diagnostic must name the repo root it looked in, and the control arm that puts it at the
candy's repo root must validate clean. Both held.

4. The changed path, executed LIVE against the sanctioned worktree-local DEV build
(bash scripts/bootstrap-charly.sh --dev-plugin plugin-box=<this checkout> → built ./bin/charly (2026.301.1936) — DEV BUILD, local plugin source; a DEV build is never installed and never
replaces the released binary). The binary really links THIS repo's source and this repo's floor:

$ go version -m bin/charly | grep -E 'opencharly/(sdk|plugin-box)'
	dep	github.com/opencharly/plugin-box/candy/plugin-box	(devel)
	dep	github.com/opencharly/sdk	v0.2026297.1851	h1:E85eZNFjzx/Fy7KqGyM4uDvW3fuJiKzkAqMXYMpuovA=

The changed behaviour is WHERE a skill body must live, so the two placements of the SAME project
are the discriminator — beside the candy is REFUSED and the diagnostic names the repo root; at the
owning candy's repo root it is CLEAN:

$ charly -C <project-with-the-body-BESIDE-the-candy> box validate
charly: error: command "validate": validation error: skills: skill "psk1": no body — the node carries no in-line `content:` and no .agents/skills/psk1/SKILL.md exists at the repo root of the owning candy "pcandy"
exit: 1
$ charly -C <project-with-the-body-AT-the-project-root> box validate
charly box validate: OK — checked 1 candy, 7 distros, 7 builders; 0 warnings, 0 errors
exit: 0

5. The control fails without the change (R7 — a coverage that cannot fail is invalid).

The same tree, with ONLY the sdk floor put back to v0.2026293.1143 — the pre-change value — and the flipped assertions left in place. Exactly the three flipped assertions go RED, and nothing else does:

--- FAIL: TestValidateSkillBodies_BesideTheCandyIsReported (0.00s)
    validate_skills_test.go:62: a body beside the candy must be reported — the loader reads the owning candy's REPO ROOT, not the candy's own directory
--- FAIL: TestValidateSkillBodies_AtTheRepoRootIsClean (0.00s)
    validate_skills_test.go:77: a body at the owning candy's repo root must be clean, got [skills: skill "review": no body — the node carries no in-line `content:` and no SKILL.md exists under .agents/skills beside the owning candy "plugin-review"]
--- FAIL: TestValidateSkills_PackageCandyOwnerStaysClean (0.00s)
    validate_skills_test.go:186: a package-candy owner with a resolvable body must be clean, got [skills: skill "review": no body — the node carries no in-line `content:` and no SKILL.md exists under .agents/skills beside the owning candy "plugin-review"]
FAIL	github.com/opencharly/plugin-box/candy/plugin-box	0.026s

The old sdk's own diagnostic — "no SKILL.md exists under .agents/skills beside the owning candy" — is the literal opposite of the new one's ("no SKILL.md exists at the repo root of the owning candy"), and the two arms above read the two of them. That is the whole defect in two sentences: this repo's tests were proving the OLD base, and the shipped binary enforces the NEW one.

6. R5's claim-keyed sweep — the stale base. The claim: no statement in this repo names the old base, and no statement points at a file that no longer holds it.

$ grep -rn "owning candy SourceDir\|GetSourceDir()/\.agents\|candy's own directory" --include='*.go' --include='*.yml' --include='*.md' . --exclude-dir=.git --exclude-dir=CHANGELOG
candy/plugin-box/validate_skills_test.go:19:// REPO ROOT (deploykit/candy_repo_root.go → candyRepoRoot), not at the candy's own directory —
candy/plugin-box/validate_skills_test.go:62:	t.Fatal("a body beside the candy must be reported — the loader reads the owning candy's REPO ROOT, not the candy's own directory")
candy/plugin-box/validate_skills.go:27:// and NOT the candy's own directory. The two coincide only for a root-level standalone candy;

Every surviving hit is this change's own statement of the NEW base — the phrase appears only in the sentences that say the candy's own directory is not where the lookup looks.

$ grep -rn "skills_collect.go" --include='*.go' --include='*.yml' --include='*.md' . --exclude-dir=.git --exclude-dir=CHANGELOG
charly.yml:563:            # repo root (sdk/deploykit/candy_repo_root.go → candyRepoRoot), and it is the layout

The one surviving pointer now names the file that HOLDS the base (candy_repo_root.go); the sibling pointer at charly.yml:461 was corrected the same way.

$ grep -rn "GetSourceDir()" --include='*.go' . --exclude-dir=.git
candy/plugin-box/validate_schema_rules.go:61:		if c == nil || c.GetSourceDir() == "" {
candy/plugin-box/validate_schema_rules.go:64:		f := filepath.Join(c.GetSourceDir(), spec.UnifiedFileName)

Both survivors are the SCHEMA-file lookup (spec.UnifiedFileName), not a skill body — a different surface, unchanged by this PR.

Rulebook compliance

Change classification

A dependency-alignment and test/prose fix in one plugin repo. No schema change, no CLI surface change, no cutover: the shipped binary's behavior is unchanged, because the org's build already resolved bodies at the repo root — this change makes THIS repo's own build and this repo's own assertions agree with it.

The tier is fully tested and validated: the change-class gate (the repo's Go gate, charly box validate, and the check-box-local R10 bed) passed on the committed tree, and the changed path was executed live against the worktree-local DEV build.

Filed issues

Agent: charly · session ses_8efb271d-f2c0-47d3-85e3-2a1e00b11ae0
Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)

…n the pin, the assertions and the prose

This repo's own `go.mod` floored the sdk at v0.2026293.1143, whose collector
resolved a skill body at the owning candy's OWN directory. The sdk the org's
build compiles (v0.2026297.1851) resolves it at the owning candy's REPO ROOT
(deploykit/candy_repo_root.go → candyRepoRoot). The two disagree for every
candy under `candy/<name>/`, so this repo's unit tests asserted the OLD base
while the shipped binary runs the NEW one.

The three assertions that encoded the old base are flipped, and the sdk pin is
advanced so the assertions hold in this repo's own build — which also makes
them a floor guard: lowering the pin turns them red again.

Closes #73

Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)
@github-actions

Copy link
Copy Markdown

Review — PASS

I re-derived every claim in the body against the current diff and the commit/comment state.

Counts & honesty. The body's "5 files, +125/-58" matches the per-file headers exactly (15+74+7+25+4 = 125 additions; 12+34+1+8+3 = 58 deletions). The go.mod floor advance (v0.2026293.1143 → v0.2026297.1851), the go.sum churn, and the spec bump are all present and coherent. No stale or contradictory numbers.

Correctness of the flip. The three assertions in validate_skills_test.go are consistently inverted: the "beside the candy" arm now requires a report plus a diagnostic-substring match, and the "at the repo root" arm now requires clean. TestValidateSkills_PackageCandyOwnerStaysClean correspondingly switches skillBodiesCfg(t, false) → (t, true), which is the correct layout under the new base. The two prose statements (validate_skills.go doc comment, charly.yml bed comments) now name the REPO ROOT base and point at deploykit/candy_repo_root.go, matching the diff.

Evidence consistency (R7). I recomputed the new-file line numbers from the hunk headers (@@ -42,23 +50,32 @@, @@ -162,7 +179,7 @@): the negative-arm t.Fatal lands at line 62, the positive-arm t.Fatalf at line 77 — exactly the lines quoted in the body's §5 RED output. That is a strong internal-consistency check that the pasted evidence was produced from this tree, not fabricated.

Tests. The change ships coverage that fails without it: the flipped assertions go RED when the sdk floor is lowered, which the body demonstrates and which the module mechanism (test compiles against the declared floor) supports. The new diagnostic-substring assertion is deliberately pinned and documented as such.

Security / scope. No secrets, injection, privilege, or dependency-risk red flags. The go.mod/go.sum churn is standard indirect-dep propagation from the sdk bump; the charly.yml edits are comments only (no functional change). No leftover legacy path: the R5 sweep is classified and the surviving GetSourceDir() hits are the unrelated schema-file lookup, correctly excluded.

Comments. Zero comments on this PR; nothing to disposition, and no maintainer sign-off is available, so the change stands on its own evidence — which it does.

Minor, non-blocking: the negative arm indexes e.msgs[0] without asserting message count, and the R7 reproduction would in practice also touch go.sum when reverting the floor; neither affects the verdict.

Verdict: PASS


ollama-cloud/deepseek-v4.1-flash — action-review.

View action run

@github-actions
github-actions Bot enabled auto-merge (squash) October 29, 2026 00:39
@github-actions
github-actions Bot merged commit ecb0980 into main Oct 29, 2026
3 checks passed
charly-auto-merge Bot added a commit that referenced this pull request Oct 29, 2026
github-actions Bot pushed a commit that referenced this pull request Oct 29, 2026
…rect the record of the floor reproduction (#75)

The charly/pr-validator raised two observations on #74, which
merged as ecb0980. A finding raised on your own PR is fixed or rebutted on the
thread; both are fixed here, in the repo, because the record is where the next
reader looks.

1. The negative arm asserted "did it report at all?" (`len(e.msgs) == 0`) and then
   indexed `e.msgs[0]`. It never asserted the COUNT, so a body walk that reported
   the same missing body twice stayed green while the author saw a duplicated
   finding. The arm now asserts EXACTLY ONE finding.

2. The record said the red arm was made "with ONLY the sdk floor put back". The
   floor and its checksums are one change: `go.mod` and `go.sum` are restored
   together, and the floor alone does not build. The sentence in
   CHANGELOG/2026.302.0039.md now says so, and the correction is recorded in the
   record itself rather than only in the pull request.

Issue: #74

Assisted-by: Claude Code minimax/minimax-m3.1-flash:cloud (fully tested and validated)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant