feat(002): vendor & verify skills — skillcore SDK + add/verify#5
Conversation
Spec for feature 002: vendor (`add`) + verify skills, on a single shared integrity primitive (`skillcore`). spec.md is user-facing (4 stories: add, label-honesty verify, orphan/completeness, scriptable exit-code/JSON gate); spec-tech-spike.md holds the implementation decisions for the plan phase. Scope reshaped during clarification (recorded in spec Clarifications + spike): - Prerequisite/eligibility checking moved out of `verify` into a future `doctor` capability; `verify` is integrity-only (exit 0/1/2). Corrected docs/ARCHITECTURE-v0.md accordingly (verify/doctor split, open Q5 resolved). - `skillrig add` (local git-checkout source) pulled in as a durable capability so the add->verify round-trip is the acceptance contract. - Recorded SDK requirement: skillcore consumable as a Go SDK (no constitutional rule forces internal/); single-impl + presentation-free already satisfy it. Prior-art contrast captured in the spike (no code yet, spec phase): - skills.sh/Vercel (HTTP registry, bespoke SHA-256) and gh skill (GitHub REST, tree-SHA used online, no verify, frontmatter injection) — the latter confirms why skillrig must keep provenance lockfile-only (injection breaks tree-SHA label-honesty) and favors reimplement-core over wrapping gh skill. - Open questions logged for planning: git-origin coupling (A-1/OQ-1), auth for remote add (OQ-2), go-getter vs git wrapper for acquisition (OQ-3). Pushed for team alignment; PR not yet opened. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clarify session 2026-05-30 reshaped scope from reviewer feedback (21 open comments, all resolved on the SpecLedger backend with reasons). Decisions: - add obtains skills from the RESOLVED origin (no --from/path arg) — the single-origin init contract holds; origin value may be a local checkout; tests run `init --origin <local>` then `add`. GitHub-only is the prod lean. - add detects + refuses divergent content (--force to override); NO 3-way merge. add != bump: re-vendoring the same version has no upstream-advance axis (base/theirs/ours) to merge — that belongs to bump. - verify conflict-marker detection deferred (marker'd content already fails the tree-SHA check; no producer until bump). - multi-client symlinks deferred; add writes only canonical .agents/skills; agent-shell selection is a separate feature. - [[requires]] NOT mirrored into the lock — the vendored on-disk skill.toml is the single source of truth (diverges from arch 4.2; flagged). Corrections applied: byte-exact fingerprint (no whitespace leniency); verify aggregates ALL failing skills (never first-fail); add/verify require a git repo; record file named .skillrig/skills-lock.json in Key Entities. Spec re-validated against the quality checklist — all items still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Plan phase for 002 (skillcore + add + verify). Key decisions (research.md
D1-D13), grounded in prior-art study (gh-cli/git, gh skill, skills.sh):
- Tree-SHA by SHELLING git (`git rev-parse <ref>:<path>`), not in-process —
gh-cli reimplements nothing; add records it from the origin, verify recomputes
on the consumer's committed tree, both git-canonical so they match by
construction (AP-04 hardened). Relocation-invariance confirmed on real git.
- verify hashes the COMMITTED tree + flags dirty separately (read-only); refines
the spike's working-tree intent.
- skillcore is a PUBLIC pkg/skillcore package (SDK-1), presentation-free, never
fetches; CLI resolves the origin and passes a local path down (SDK boundary).
- lock omits [[requires]] (the vendored manifest is the source of truth).
- Test-oracle independence (D11): integration tests use raw git, never skillcore
(Constitution III, no circular validation).
- Fixture mirrors a CANONICAL design-aligned origin (D12); the existing
skillrig-origin repo is a pre-design sample, reconciled via recommendations.
- Large-monorepo perf (D13): local origin = no clone; future remote-add uses
partial-clone + sparse-checkout with ZERO change to the tree-SHA primitive.
Artifacts: plan.md, research.md, data-model.md (real git-tree-SHA ground truth),
contracts/{add,verify,skillcore-sdk}.md, quickstart.md (~16 TestQuickstart_*).
CLAUDE.md: sl-context-update tech section + the PRE-RELEASE marker reword.
Constitution Check I-IX all pass; no complexity violations.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cross-artifact verify cycle (two independent reviews merged) + doc alignment. spec/quickstart/contracts (002): - C1 (HIGH): reconcile spec to committed-tree + 'dirty' verdict (FR-009, FR-022, Key-Entities verdict, SC-001) — was the only spec<->plan semantic ambiguity. - Q1 (CRITICAL, caught by the independent review): rewrite stale US3.3 to the deferred/canonical-only behavior (aligns with FR-011 / Out-of-Scope). - Close test gaps: help-examples, verify-read-only, prereq-ignored scenarios. - verify help text now self-documents PROJECT scope (code/test readers never see the spec). Scope the git requirement to project (--global is a deferred carve-out); fix the add not-a-git-repo rationale. Record global-scope forward concerns (spike). - reviews/002-review.md: merged + resolved record of both reviews. docs: - ARCHITECTURE §9c (new): federated skill registries (skills.sh, AWS AgentRegistry) as external origin types — reconciles the integrity model (git origin -> tree-SHA; registry -> recorded content digest; verify stays offline; audit/usage advisory-only). Answers the spike's OQ-1 (git-origin coupling). - ROADMAP: 002 row -> exit 0/1/2 + includes local 'add'; 004 reframed as remote-origin 'add' fetch; lockfile commitment drops 'requires' (manifest is source of truth, D4). - constitution: lock-entry example drops 'requires' (matches D4). Experiment files (implement-workflow command, CLAUDE/AGENTS changes, vcr-cassettes) deliberately left unstaged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implement the second slice: the shared integrity primitive pkg/skillcore (public SDK, SDK-1/AP-04) plus `skillrig add` (Vendor Mutation) and `skillrig verify` (Verification Gate) — making "the skill your agent runs is exactly the version that was reviewed and approved" demonstrable offline. skillcore (pkg/skillcore, presentation-free, one shared implementation): TreeSHA via shelled `git rev-parse` (git-canonical, relocation-invariant), ParseManifest (go-toml/v2), Read/WriteLock (atomic, deterministic, no `requires`), Add (mode-preserving vendor + lock; force/dry-run/idempotent), Verify (label-honesty + orphan/missing/dirty, read-only, aggregates all), typed errors (VerifyFailure, GitError, LockError, SkillNotFound, Overwrite). CLI (internal/cli): add (resolve origin via config.ResolveOrigin -> skillcore .Add -> render), verify (skillcore.Verify -> render -> exit), exitCodeFor maps *VerifyFailure -> ExitVerification(2), output renderers (compact human + footer / complete --json), git repo-root helper. Tests: TestQuickstart_* round-trip, tamper->mismatch, dirty, orphan, missing, aggregate-all, exit matrix, json-complete, help examples, ignores non-canonical view dirs; + skillcore unit tests (ground-truth TreeSHA == raw git). RAW-git is the independent oracle (no circular validation). Docs/skills: cli.md synced (verify integrity-only; pkg/skillcore as a separate public package; exit 3 reserved for doctor); contracts/add.md + skillrig-init local-origin note; new skillrig-add-verify agent skill + eval sets (Constitution IX); checkpoint session log + post-implementation adversarial review report. Tooling: implement-workflow (+v2) and checkpoint-workflow commands (latter adds a clean-tree precondition before launching review agents); remove AGENTS.md; CLAUDE.md + tasks-template updates. Open follow-ups (see specledger/002-skillcore-verify/reviews/002-review.md): AR-1/AR-2 local-origin CWD-relative resolution + misleading "skill not found" when the origin checkout is absent; AR-3 add pkg/skillcore/verify_test.go. New public package: github.com/skillrig/cli/pkg/skillcore. No new dependencies. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the independent post-implementation review (reviews/002-review.md): correctness + harness bugs, error-navigation gaps, missing unit tiers, and the local-origin resolution footgun. Correctness / harness: - R2-H1: add resolvePlacement looked up the lock by the directory arg while writeLockEntry keys it by the manifest name — an identical re-add of a skill whose manifest name != its dir was wrongly refused (FR-003). Look up by name; regression test for name!=dir. - R2-H2: the re-exec'd git stub now sets GOCOVERDIR, so `go test -cover ./pkg/skillcore/...` no longer leaks a warning into captured git stderr. - GAP-C: `make test-unit` now runs ./internal/... AND ./pkg/... (skillcore's Constitution III tests were silently skipped by the documented unit tier). - GAP-D: ship an executable check.sh (0o755) in the sample skill so the mode-preservation assertion actually exercises the exec bit. Error navigation (cli.md Principle 1/2): - R2-M3: custom Args validators on add/verify return what/why/fix + an example instead of cobra's terse "accepts 1 arg(s)" / "unknown command". - R2-M4/AR-2: new *OriginNotFoundError distinguishes a missing local origin checkout from a wrong skill name (was conflated into "check the skill name"). Local-origin resolution (AR-1/R2-L6): - Anchor the origin source to the repo root (filepath.Join(repoRoot, originDir)), matching the destination — `add` now works from any subdirectory (was CWD-relative and failed from nested dirs). Verified live. Tests (Constitution III): - GAP-A: pkg/skillcore/verify_test.go — taxonomy (ok/mismatch/orphan/missing/ dirty), counts, aggregate-all, dirty-masks-mismatch precedence, empty-repo, unsupported lockfileVersion -> *LockError. - GAP-B: internal/cli/addverify_test.go — exitCodeFor (incl. wrapped *VerifyFailure), mapAddError classes, originDirRef, arg validators, renderers. Docs / tooling: - R2-L5: cli.md add synopsis corrected to the shipped surface; --origin/--pin marked dropped/planned. - AR-4: add --help, contracts/add.md, skillrig-init + skillrig-add-verify skills synced to repo-root resolution + the distinct missing-checkout error. - checkpoint-workflow review-agent template now loads agentic-go-cli-design + golang-code-style/testing/lint, and requires a clean tree before launching. Gate: make check (0 lint) + go test -cover ./... green (skillcore 79.5%, internal/cli 51.2%). Deferred: AR-5 (cosmetic stale data-model sample SHA). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merge the per-command skills `skillrig-init` and `skillrig-add-verify` into one
consolidated `.agents/skills/skillrig/` skill, following skill-creator's
progressive-disclosure / domain-organization pattern:
- root SKILL.md: short + grokkable — what skillrig is, when to use it (find/
install/vendor/verify skills from your origin), the origin precondition +
smoketest, the init->add->verify workflow, and a routing table to:
- references/{init,add,verify}.md: full per-activity detail (moved verbatim from
the two originals; all guidance preserved — cross-validated).
- evals/: merged 9 behavioral cases (4 init + 5 add/verify) + 23 trigger queries;
init-domain queries flip from negative→positive now that one skill owns them.
Discovery scope: "find" means find an APPROVED skill in your configured origin
(search is the next planned feature) — distinct from the generic `find-skills`
skill; recorded as a known gap in the root SKILL.md.
Guidance updated so future commands extend this one skill rather than spawning a
new per-command skill:
- Constitution IX: "one consolidated skill, not one-per-command" — a new command
adds a references/<cmd>.md + updates the root routing/description.
- CLAUDE.md: skill path -> .agents/skills/skillrig/ + the same rule.
Removed: .agents/skills/skillrig-init/, .agents/skills/skillrig-add-verify/.
No code change; make check green. Evals defined, not run.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review Summary by QodoSkillcore SDK with vendor & verify commands — integrity gates for skill provenance
WalkthroughsDescription• **Core SDK (pkg/skillcore)**: Public package implementing the shared integrity primitive with git tree-SHA computation, manifest parsing, atomic lock I/O, and Add/Verify operations — presentation-free, network-free, never fetches • **skillrig add command**: Vendors a skill from a local origin into .agents/skills//, byte-identical and mode-preserving, with idempotency guards, divergent-overwrite refusal (unless --force), and dry-run support; records lock entries keyed by manifest name • **skillrig verify command**: Offline, deterministic, read-only integrity gate that recomputes tree-SHAs (label-honesty), detects orphan/missing/dirty skills, and aggregates findings with exit codes 0/1/2 (3 reserved for future doctor) • **Comprehensive test coverage**: Integration test suite (1346 lines) mapping to quickstart scenarios with raw git as independent oracle; unit tests for Add, Verify, lock I/O, manifest parsing, tree-SHA, and CLI layer (error mapping, output rendering, argument validation) • **CLI integration**: Registers add and verify subcommands, maps typed errors to navigational messages, renders human and JSON output, resolves repo root and origin checkout anchored to repo root (works from any subdirectory) • **Documentation**: Specification, implementation plan, research decisions (D1–D13), technical spike, cross-artifact verification review with post-implementation adversarial findings and remediation, architecture clarifications on verify scope, and CLI design updates • **Scope clarification**: verify is integrity-only (label-honesty + orphan detection); prerequisite/eligibility checks deferred to future doctor command; exit 3 reserved for missing backing tools Diagramflowchart LR
origin["Local origin<br/>checkout"]
add["skillrig add<br/>Vendor Mutation"]
skills[".agents/skills/<br/>canonical store"]
lock[".skillrig/<br/>skills-lock.json"]
verify["skillrig verify<br/>Verification Gate"]
report["Integrity Report<br/>exit 0/1/2"]
origin -- "byte-identical<br/>mode-preserving" --> add
add -- "idempotent<br/>--force/--dry-run" --> skills
add -- "record version,<br/>commit, treeSha" --> lock
skills -- "recompute<br/>tree-SHA" --> verify
lock -- "label-honesty<br/>orphan/dirty" --> verify
verify --> report
File Changes1. test/skillcore_quickstart_test.go
|
Code Review by Qodo
1. verify fails on stdout
|
| // handleVerifyError classifies skillcore.Verify's error. A *VerifyFailure is a | ||
| // per-skill finding: render the report to stdout (human or --json) and return | ||
| // the failure so exitCodeFor yields exit 2. A *LockError is a config/usage | ||
| // problem (exit 1); any other error is wrapped as a usage error. | ||
| func (vc *verifyCmd) handleVerifyError(cmd *cobra.Command, err error) error { | ||
| var verifyFail *skillcore.VerifyFailure | ||
| if errors.As(err, &verifyFail) { | ||
| if renderErr := renderVerifyReport(cmd.OutOrStdout(), verifyFail.Report, vc.opts.json); renderErr != nil { | ||
| return renderErr | ||
| } | ||
|
|
||
| return verifyFail | ||
| } |
There was a problem hiding this comment.
2. verify fails on stdout 📘 Rule violation ≡ Correctness
On verification failure, the command writes the failure report to stdout and suppresses stderr output, mixing error/diagnostic text into the normal data stream. This breaks the stdout/stderr separation required for CLI tooling and scripting.
Agent Prompt
## Issue description
`skillrig verify` renders failure diagnostics to stdout (`cmd.OutOrStdout()`), and root error rendering intentionally prints nothing for `*skillcore.VerifyFailure`. This results in error text on stdout and an empty stderr on failure.
## Issue Context
Compliance requires errors/diagnostics on stderr and data on stdout.
## Fix Focus Areas
- internal/cli/verify.go[95-107]
- internal/cli/output.go[149-200]
- internal/cli/root.go[58-73]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| fmt.Fprintf(&b, "verify FAILED: %d of %d skills\n", failed, total) | ||
|
|
||
| for _, v := range r.Verdicts { | ||
| if v.Status == skillcore.StatusOK { | ||
| continue | ||
| } | ||
|
|
||
| fmt.Fprintf(&b, " ✗ %s %s\n", v.Name, verdictLine(v)) | ||
| } | ||
|
|
||
| b.WriteString(verifyFailFooter + "\n") | ||
|
|
There was a problem hiding this comment.
3. Unchecked fmt.fprintf errors 📘 Rule violation ≡ Correctness
renderVerifyFailure ignores error returns from fmt.Fprintf and strings.Builder.WriteString, violating the requirement that all error returns are checked or explicitly justified. Ignored write errors can silently drop output and make failures harder to diagnose.
Agent Prompt
## Issue description
`renderVerifyFailure` calls `fmt.Fprintf` and `b.WriteString` without checking their returned errors.
## Issue Context
The compliance checklist requires all error returns to be checked or explicitly discarded with a justified lint suppression.
## Fix Focus Areas
- internal/cli/output.go[179-199]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // This file holds the TestQuickstart_* integration suite for feature | ||
| // 002-skillcore-verify (add + verify). Per Constitution II (Quickstart as | ||
| // Contract) each scenario in specledger/002-skillcore-verify/quickstart.md maps | ||
| // 1:1 to a TestQuickstart_* test here. Like the 001 suite it builds the real | ||
| // binary once (TestMain in quickstart_test.go) and execs it via run(). | ||
| // | ||
| // Oracle independence (research D11): every expected tree-SHA is computed with | ||
| // RAW git (rawTreeSHA → `git rev-parse <ref>:<path>`), NEVER through skillcore — | ||
| // the binary under test uses skillcore internally, so routing the expected value | ||
| // through it would be circular validation (Constitution III). All git | ||
| // bootstrap/commit helpers here shell `git` directly with a pinned identity so | ||
| // the fixture commit is reproducible (D8). | ||
| package quickstart | ||
|
|
There was a problem hiding this comment.
4. Quickstart tests missing build tag 📘 Rule violation ⌂ Architecture
test/skillcore_quickstart_test.go defines multiple TestQuickstart_* integration tests but lacks the required //go:build integration tag at the top of the file. This makes integration tests run unintentionally in default go test runs.
Agent Prompt
## Issue description
Integration tests in `test/skillcore_quickstart_test.go` are missing the `//go:build integration` build tag.
## Issue Context
The checklist requires integration tests to be opt-in via the `integration` build tag.
## Fix Focus Areas
- test/skillcore_quickstart_test.go[1-14]
- test/skillcore_quickstart_test.go[302-310]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Resolve the bugs and one rule violation from the automated Qodo review on PR #5 (see specledger/002-skillcore-verify/reviews/002-review.md): - Path traversal (#5): validate the skill name as a single safe path segment before any FS op, so `add ../x` can't escape .agents/skills/ or os.RemoveAll an arbitrary dir. New *InvalidSkillNameError + tests. - Symlinks (#6): reject any symlink in the origin skill subtree (would let copy/compare follow outside it and break byte-identical/git-canonical vendoring). New *SymlinkUnsupportedError + test; policy noted in cli.md (preserve-as-symlink is a future relaxation). - Verify error class (#8): pathInHead now propagates a *GitError only when git cannot run or "not a git repository"; every other rev-parse failure (absent path, unborn HEAD) stays "not in tree" — honoring the Verify SDK contract without breaking the dirty/missing verdicts. Tests for both. - rev-parse option injection (#7): refuse a revision beginning with '-' (git rev-parse echoes --/--end-of-options, so a guard is the right fix). Test. - %q rule (#1): quote path strings in mapAddError's user-facing messages. Declined: verify-report-on-stdout (the report is data; exit code is the signal; contract requires `verify --json 2>/dev/null | jq`) and the //go:build integration tag (project separates integration by ./test/ dir) — PR replies posted. Skipped: unchecked strings.Builder writes (never error). Gate: golangci-lint 0 issues; go test -cover -count=1 ./... green (skillcore 80.7%, internal/cli 51.6%). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo review addressed (pushed in 76b560e)Thanks Qodo. Triaged all 8 findings. Fixed (4 bugs + 1 rule):
Skipped:
Declined, with rationale:
Gate after the round: |
…FIG env Qodo code review (3 bugs + 6 rule violations). Fixed the 4 real/actionable; declined 4 (documented decisions / false positive) — see PR reply. - #8 (security): token no longer passed via argv. authConfigArgs (-c http.extraHeader=…) → authConfigEnv injecting GIT_CONFIG_COUNT/KEY_0/VALUE_0 (git >=2.31) via cmd.Env, so the base64 credential is in the process environ, not ps-visible argv. Matches gh-cli's "token out of argv" posture (gh uses a credential helper; we use GIT_CONFIG env since we resolve the token ourselves). runEnv() added; Clone/fetchSparseInto/FetchFile use it. F2 tests rewritten: TestAuthConfigEnv + TestClone_TokenInjectionViaEnv (assert no token in argv, credential in GIT_CONFIG_VALUE_0 env). - #7: `search` no longer requires a git repo — repoRoot is optional; a non-git cwd / checkout-less remote falls through to FetchCatalog. New TestQuickstart_SearchRemoteFromNonGitDir (file:// origin from a plain tempdir). - #5: search declares Args: cobra.ArbitraryArgs (rule 782342). - #9: human description width 60 → 80 (rule 783450). - #4: investigated — no genuine ignored error (errcheck clean); no change. Gate: make check 0 lint; go test -count=1 all pass; new tests run+pass; only the two git-on-PATH t.Skip guards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Second slice of
skillrig: the shared integrity primitivepkg/skillcore(public SDK, SDK-1/AP-04) plus two consumer commands that make the product promise demonstrable offline — "the skill your agent runs is exactly the version that was reviewed and approved."skillrig add <skill>(Vendor Mutation): vendors a skill from the repo's resolved origin into canonical.agents/skills/<skill>/, byte-identical and mode-preserving, and records{version,commit,treeSha,path}in.skillrig/skills-lock.json. Idempotent; refuses divergent overwrite without--force;--dry-run. Local origin this slice (a checkout at<repo-root>/OWNER/REPO, resolved repo-root-relative so it works from any subdirectory).skillrig verify(Verification Gate): offline, deterministic, read-only — recomputes each locked skill's git tree-SHA onHEAD(label-honesty) + orphan/missing/dirty completeness, aggregating all findings. Exit0/1/2;3reserved for a futuredoctor(a missing backing tool is not a verify failure).pkg/skillcore: one implementation of TreeSHA (shelledgit rev-parse, git-canonical), manifest parse, atomic lock I/O,Add,Verify— presentation-free, never fetches. Consumed by both commands (no parallel copy).Built via the experimental
/specledger.implement-workflow, then hardened through two independent cold adversarial reviews (specledger/002-skillcore-verify/reviews/002-review.md):addidempotency bug (lock keyed by manifest name vs looked up by dir), ago test -coverstub failure, repo-root-relative origin resolution (addfrom any subdir), distinct "origin checkout not found" vs "skill not found" errors, navigational arg errors,make test-unitincludingpkg/, an exec-bit fixture, and new unit tests forverifyand theinternal/cliadd/verify layer..agents/skills/skillrig/(short root +references/{init,add,verify}.md), and updated Constitution IX + CLAUDE.md to mandate one consolidated skill going forward.Docs synced in-branch:
docs/design/cli.md(verify integrity-only,pkg/skillcoreas a public package, exit-3 reserved, shippedaddsurface).Test plan
make check— gofmt + go vet + golangci-lint (0 issues) +go test ./...— green.go test -cover ./...— green (skillcore 79.5%, internal/cli 51.2%).TestQuickstart_Add*/TestQuickstart_Verify*), with rawgitas the independent ground-truth oracle (no circular validation).init → add → commit → verifyexit 0; tamper → exit 2;addfrom a subdirectory; missing-origin and bad-args error paths.🤖 Generated with Claude Code