Skip to content

feat(002): vendor & verify skills — skillcore SDK + add/verify#5

Merged
so0k merged 8 commits into
mainfrom
002-skillcore-verify
May 30, 2026
Merged

feat(002): vendor & verify skills — skillcore SDK + add/verify#5
so0k merged 8 commits into
mainfrom
002-skillcore-verify

Conversation

@so0k
Copy link
Copy Markdown
Contributor

@so0k so0k commented May 30, 2026

Summary

Second slice of skillrig: the shared integrity primitive pkg/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 on HEAD (label-honesty) + orphan/missing/dirty completeness, aggregating all findings. Exit 0/1/2; 3 reserved for a future doctor (a missing backing tool is not a verify failure).
  • pkg/skillcore: one implementation of TreeSHA (shelled git 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):

  • fix(002) resolved review findings — an add idempotency bug (lock keyed by manifest name vs looked up by dir), a go test -cover stub failure, repo-root-relative origin resolution (add from any subdir), distinct "origin checkout not found" vs "skill not found" errors, navigational arg errors, make test-unit including pkg/, an exec-bit fixture, and new unit tests for verify and the internal/cli add/verify layer.
  • refactor(skill) consolidated the per-command agent skills into a single .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/skillcore as a public package, exit-3 reserved, shipped add surface).

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%).
  • All quickstart acceptance scenarios pass (TestQuickstart_Add* / TestQuickstart_Verify*), with raw git as the independent ground-truth oracle (no circular validation).
  • Live round-trip verified by hand: init → add → commit → verify exit 0; tamper → exit 2; add from a subdirectory; missing-origin and bad-args error paths.

🤖 Generated with Claude Code

so0k and others added 7 commits May 29, 2026 23:46
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>
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Skillcore SDK with vendor & verify commands — integrity gates for skill provenance

✨ Enhancement 🧪 Tests 📝 Documentation

Grey Divider

Walkthroughs

Description
• **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
Diagram
flowchart 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

Loading

Grey Divider

File Changes

1. test/skillcore_quickstart_test.go 🧪 Tests +1346/-0

Integration test suite for add/verify commands with oracle validation

• Comprehensive integration test suite (1346 lines) for the add and verify commands, mapping 1:1
 to quickstart scenarios per Constitution II
• Tests use raw git as independent oracle (never skillcore) to validate tree-SHA computation,
 ensuring no circular validation
• Covers happy paths (vendor, verify pass), error cases (divergent overwrite, missing origin), and
 completeness checks (orphan, missing, dirty skills)
• Validates exit codes (0/1/2), JSON output structure, human-readable output bounds, and read-only
 invariants

test/skillcore_quickstart_test.go


2. pkg/skillcore/add.go ✨ Enhancement +420/-0

Skill vendoring implementation with idempotency and divergence guards

• Implements the Add function to vendor a skill from a local origin into .agents/skills//,
 byte-identical and mode-preserving
• Handles idempotency (unchanged action), divergent-overwrite refusal (unless --force), and
 dry-run mode without writing
• Defines error types (SkillNotFoundError, OverwriteError) for navigational CLI error handling
• Records lock entries with version, commit, tree-SHA, and path; keyed by manifest name (not
 directory) to handle name/dir mismatch

pkg/skillcore/add.go


3. pkg/skillcore/verify.go ✨ Enhancement +348/-0

Skill verification with label-honesty and completeness checks

• Implements the Verify function to check vendored skills against the lock: label-honesty
 (tree-SHA recomputation), orphan/completeness, and dirty-working-tree detection
• Aggregates all findings into a Report with per-skill Verdict objects (status:
 ok/mismatch/orphan/missing/dirty)
• Distinguishes config/usage errors (LockError, GitError → exit 1) from verification failures
 (VerifyFailure → exit 2)
• Read-only, offline, deterministic; scans only the canonical .agents/skills/ root (ignores
 per-client view directories)

pkg/skillcore/verify.go


View more (53)
4. pkg/skillcore/verify_test.go 🧪 Tests +278/-0

Unit tests for verify verdict computation and error classification

• Unit tests for Verify covering clean pass, mismatch (tampered content), orphan (unlocked
 on-disk), missing (locked but absent), and dirty (uncommitted) verdicts
• Tests dirty-takes-precedence-over-mismatch logic and aggregation of multiple findings in one run
• Validates error classification: LockError for unsupported lockfileVersion (exit 1),
 VerifyFailure for per-skill findings (exit 2)
• Uses raw git as oracle to seed test repos with known tree-SHAs

pkg/skillcore/verify_test.go


5. pkg/skillcore/add_test.go 🧪 Tests +280/-0

Unit tests for add vendoring, idempotency, and error handling

• Unit tests for Add covering happy path (vendor and record lock), idempotency, and
 manifest-name-vs-directory edge case (FR-003)
• Tests error paths: origin checkout missing (distinct from skill not found), divergent-overwrite
 refusal, and skill-not-found
• Validates --force restores origin content and --dry-run writes nothing
• Uses raw git to bootstrap test origins and validate tree-SHA recording

pkg/skillcore/add_test.go


6. internal/cli/addverify_test.go 🧪 Tests +328/-0

CLI layer tests for error mapping, output rendering, and argument validation

• Tests for CLI layer: exit-code mapping (nil→0, usage→1, verify-failure→2), error message
 navigation (what/why/fix), and argument validation
• Validates human and JSON output rendering for both add and verify commands (compact shapes,
 structural completeness)
• Tests renderAddResult and renderVerifyReport with all required JSON keys and bounded line
 counts
• Covers origin resolution and per-command argument validators

internal/cli/addverify_test.go


7. internal/cli/root.go ✨ Enhancement +14/-0

Register add/verify commands and suppress verify-failure error rendering

• Imports skillcore package and registers add and verify subcommands via registerSubcommands
• Updates renderError to suppress generic error output for VerifyFailure (the per-skill report
 on stdout is the message; exit code 2 still applies)
• Preserves existing init command registration

internal/cli/root.go


8. internal/cli/add.go ✨ Enhancement +212/-0

Vendor Mutation command: add skill from origin

• Implements skillrig add  command for vendoring skills from a configured local origin into
 .agents/skills/
• Resolves origin and repo root, calls skillcore.Add() to vendor the skill, and maps typed errors
 to navigational usage messages
• Supports --dry-run and --force flags; includes custom argument validator for better error
 messaging
• Anchors local origin checkout to repo root (not CWD) so add works from any subdirectory

internal/cli/add.go


9. internal/cli/output.go ✨ Enhancement +206/-0

Presentation layer for add and verify outputs

• Adds renderAddResult() to format add outcomes (human summary or --json with all keys present)
• Adds renderVerifyReport() and renderVerifyFailure() to format verify results with per-skill
 verdicts
• Implements compact human output (bounded by finding count) and complete JSON output for both
 commands
• Includes helper functions addSummary(), verdictLine(), shortSha() for formatting

internal/cli/output.go


10. pkg/skillcore/helpers_test.go 🧪 Tests +170/-0

Test infrastructure and fixtures for skillcore

• Provides test helpers: pinnedGitEnv() for reproducible git commits, runGit() as independent
 oracle, writeFile() for fixture setup
• Implements bootstrapOrigin() to create real git repos with sample skills for testing
• Includes helper-process pattern (TestHelperProcess, stubCommandContext()) for deterministic
 git error simulation

pkg/skillcore/helpers_test.go


11. pkg/skillcore/treesha_test.go 🧪 Tests +166/-0

TreeSHA ground-truth and error-path tests

• Tests TreeSHA() against ground-truth raw git output (Constitution III oracle independence)
• Verifies relocation-invariance: same skill at different paths yields identical tree-SHA
• Tests real git error paths and stubbed git failures via the pluggable command constructor

pkg/skillcore/treesha_test.go


12. internal/cli/verify.go ✨ Enhancement +125/-0

Verification Gate command: check skill integrity

• Implements skillrig verify command for offline integrity checking (label-honesty + orphan
 detection)
• Resolves repo root, calls skillcore.Verify(), and classifies errors to navigational messages
• Maps VerifyFailure to exit 2, LockError/GitError to exit 1; includes custom argument
 validator
• Renders report to stdout (human or --json)

internal/cli/verify.go


13. pkg/skillcore/lock_test.go 🧪 Tests +131/-0

Lock file I/O and serialization tests

• Tests lock round-trip serialization (write then read back)
• Verifies on-disk shape contract: 2-space indentation, trailing newline, no requires key (D4)
• Tests absent lock handling (returns zero LockFile, not error) and atomic write discipline (no
 leftover temp files)

pkg/skillcore/lock_test.go


14. pkg/skillcore/lock.go ✨ Enhancement +114/-0

Lock file data model and I/O

• Defines LockFile and LockEntry types (version, origin, skills map with
 version/commit/treeSha/path)
• Implements ReadLock() (absent file is not an error) and WriteLock() (atomic
 temp-file-plus-rename)
• Omits requires from lock schema; manifest is single source of truth (D4)

pkg/skillcore/lock.go


15. pkg/skillcore/git.go ✨ Enhancement +85/-0

Testable git wrapper for skillcore primitives

• Implements gitClient with pluggable commandContext for testable git interaction (gh-cli
 pattern)
• Provides run(), revParse(), statusPorcelain() methods; captures stdout/stderr into buffers
• Returns GitError with exit code and trimmed stderr on failure; package-level entry points for
 production use

pkg/skillcore/git.go


16. pkg/skillcore/manifest_test.go 🧪 Tests +97/-0

Manifest parsing tests

• Tests ParseManifest() parses [[requires]] and unknown keys (forward-compat, strict mode off)
• Verifies minimal manifest (no requires) and error cases (missing file, malformed TOML)

pkg/skillcore/manifest_test.go


17. pkg/skillcore/errors.go ✨ Enhancement +66/-0

Typed error definitions for skillcore SDK

• Defines typed errors: VerifyFailure (carries full report), LockError (config/usage),
 OriginNotFoundError, GitError (exit code + stderr)
• Enables CLI to map errors to navigational messages and exit codes without losing raw cause for
 --verbose

pkg/skillcore/errors.go


18. internal/cli/exit.go ✨ Enhancement +21/-7

Exit code mapping for verification failures

• Extends exitCodeFor() to map skillcore.VerifyFailure to ExitVerification (2); other errors
 to ExitUsage (1)
• Updates ExitVerification constant comment to reflect its active use for verify failures
• Enables typed exit-code routing based on error class

internal/cli/exit.go


19. internal/cli/repo.go ✨ Enhancement +63/-0

Repo root resolution and git precondition checks

• Implements gitToplevel() to resolve repo root via git rev-parse --show-toplevel (offline,
 reads local .git)
• Provides usageNotGitRepo() for shared "not a git repository" error across add and verify
• Exports osGetwd seam for testable working-directory injection

internal/cli/repo.go


20. pkg/skillcore/manifest.go ✨ Enhancement +46/-0

Skill manifest parsing and data model

• Defines Manifest and Require types parsed from skill.toml (name, version, namespace,
 description, tags, requires array)
• Implements ParseManifest() using TOML unmarshaling with unknown-key tolerance (forward-compat)
• Manifest is read-only; Add/Verify consume it; never written back

pkg/skillcore/manifest.go


21. pkg/skillcore/treesha.go ✨ Enhancement +25/-0

Git tree-SHA computation primitive

• Implements TreeSHA() primitive: shells git rev-parse : to compute git-canonical tree-object
 SHA
• Returns relocation-invariant fingerprint (depends only on subtree contents, not path)
• Package-level documentation establishes skillcore as presentation-free, network-free,
 filesystem-only

pkg/skillcore/treesha.go


22. specledger/002-skillcore-verify/spec-tech-spike.md 📝 Documentation +192/-0

Technical spike: skillcore, add, verify design

• Comprehensive technical spike capturing design decisions for skillcore, add, and verify
 before implementation
• Clarifies scope (three deliverables: skillcore SDK, local-origin add, offline verify), resolves
 open questions (C1–C4)
• Documents primitives (TreeSHA, ParseManifest, lock I/O), add/verify patterns, edge cases,
 ground-truth anchoring, and prior-art analysis
• Identifies deferred work (doctor, bump, network add, global scope) and SDK boundary (public
 pkg/skillcore)

specledger/002-skillcore-verify/spec-tech-spike.md


23. specledger/002-skillcore-verify/research.md 📝 Documentation +123/-0

Research and design decisions for skillcore slice

• Phase 0 research documenting prior-art study (skills.sh, gh skill, gh-cli/git) and 13 key
 decisions (D1–D13)
• D1: shell git for tree-SHA (not in-process hashing); D2: verify committed tree + flag dirty
 separately; D3: public pkg/skillcore SDK
• D4: lock omits requires; D5: origin resolution in CLI; D6: byte-identical vendor copy; D7:
 testable git client; D8: tmpdir fixtures
• D9–D13: exit-code mapping, no new deps, test-oracle independence, fixture design, large-monorepo
 performance

specledger/002-skillcore-verify/research.md


24. Makefile ⚙️ Configuration changes +5/-4

Test target updates for skillcore package

• Updates test-unit target to include ./pkg/... (skillcore unit tests alongside internal/)
• Clarifies test-tier comments: unit tests cover presentation-free logic in both internal/ and
 pkg/skillcore

Makefile


25. specledger/002-skillcore-verify/spec.md 📝 Documentation +218/-0

Feature specification for vendor & verify skills with integrity gates

• Comprehensive user-facing feature specification for the skillrig add and skillrig verify
 commands, establishing the product promise of verifiable skill integrity
• Defines four prioritized user stories (P1–P3) covering skill vendoring, verification, orphan
 detection, and deterministic exit codes
• Specifies 22 functional requirements (FR-001 through FR-022) and 9 success criteria covering
 offline operation, label-honesty, content fingerprinting, and error handling
• Clarifies scope boundaries: defers prerequisite checking to doctor, network/auth to future work,
 and multi-client symlink views

specledger/002-skillcore-verify/spec.md


26. specledger/002-skillcore-verify/reviews/002-review.md 📝 Documentation +141/-0

Cross-artifact verification and post-implementation adversarial review findings

• Cross-artifact verification review resolving 22 requirements and 9 success criteria across spec,
 plan, research, data-model, contracts, and quickstart artifacts
• Documents two independent adversarial reviews (Review A and Review B) that identified and resolved
 critical findings: committed-tree vs working-tree reconciliation, per-client-view scope
 clarification, help-text coverage gaps, and read-only verification validation
• Post-implementation review (AR-1 through AR-5) flagging high-priority issues: CWD-relative origin
 resolution fragility, missing-origin error conflation, and test-tier gaps for verify logic and CLI
 presentation layer
• Remediation table documenting all fixes applied: idempotency bug (manifest-name vs directory-name
 lookup), coverage test failures, error-navigation improvements, and origin-path anchoring to repo
 root

specledger/002-skillcore-verify/reviews/002-review.md


27. specledger/002-skillcore-verify/plan.md 📝 Documentation +115/-0

Implementation plan for skillcore SDK and add/verify commands

• Implementation plan delivering pkg/skillcore (public SDK package) plus skillrig add and
 skillrig verify commands
• Specifies project structure: pkg/skillcore/ (presentation-free, never fetches) with git client,
 tree-SHA computation, manifest parsing, lock I/O, and add/verify operations
• Defines technical context: Go 1.24+, git as runtime dependency (shelled for tree-SHA),
 local-origin-only this slice, two-tier testing (unit + integration via TestQuickstart_*)
• Confirms Constitution compliance (specification-first, quickstart-as-contract, ground-truth
 anchoring, agent-first CLI design, code quality, YAGNI, shortest path to MVP, simplicity, skill–CLI
 co-evolution)

specledger/002-skillcore-verify/plan.md


28. .agents/commands/specledger.checkpoint-workflow.md 📝 Documentation +272/-0

Divergence review workflow for implementation validation

• Workflow command for critical divergence review comparing implementation against plan artifacts
• Provides structured process: gather implementation state, run project tests/checks, compare
 against spec/plan/data-model/quickstart artifacts, classify divergences by severity and type
• Includes session log format for tracking divergences, DoD bypasses, issues encountered, and action
 items; offers optional adversarial review agent launch with preconditions
• Defines behavior rules: lead with divergences, flag unchecked DoD, classify conscious vs
 oversight, require clean working tree before agent launch

.agents/commands/specledger.checkpoint-workflow.md


29. docs/ARCHITECTURE-v0.md 📝 Documentation +23/-6

Architecture clarifications on verify integrity gates and prerequisite checking

• Updates verify command description from "integrity + prereqs" to "integrity only (label-honesty
 + orphan)" reflecting the clarified scope division
• Clarifies the verify vs doctor split (§2): verify is the lean CI gate validating *content*
 (tree-SHA + orphan detection, exit 0/1/2), while doctor handles *eligibility* (prerequisite
 presence/version/auth, exit 3)
• Adds new section §9c on federated skill registries (skills.sh, AWS AgentRegistry) with integrity
 reconciliation: git origins use tree-SHA, registry sources use content digest, both verified offline
• Resolves open question #5 (prerequisite checking in verify vs doctor) by attributing all
 prerequisite/eligibility logic to doctor, making verify deterministic and offline-only

docs/ARCHITECTURE-v0.md


30. .specledger/templates/tasks-template-v2.md 📝 Documentation +287/-0

Task list template for feature implementation by user story

• Template for generating task lists from feature specifications, organized by user story to enable
 independent implementation and testing
• Defines phase structure: Setup, Foundational (blocking prerequisites), User Stories (P1–P3), and
 Polish phases with clear dependencies
• Includes format conventions ([ID] [P?] [Story] Description), path conventions, and parallel
 execution opportunities
• Provides implementation strategies (MVP-first, incremental delivery, parallel team) and emphasizes
 quickstart-as-contract (Constitution II) with integration tests written first

.specledger/templates/tasks-template-v2.md


31. docs/design/cli.md 📝 Documentation +23/-18

CLI design documentation updates for add/verify implementation and scope clarification

• Updates add and verify command descriptions to mark them as [implemented] and clarifies
 verify is "integrity only" (not including prerequisite checks)
• Corrects add command synopsis and examples: removes non-existent --origin/--pin flags,
 documents shipped surface (--dry-run, --force, --json, --verbose), notes --pin as deferred
• Clarifies origin resolution: origin is **resolved** (not passed as --from/--origin argument),
 and the resolved origin may be a local checkout this slice
• Updates Verification Gate pattern description to state verify is **integrity-only**
 (label-honesty + orphan, exit 2); prerequisite/eligibility checks (exit 3) are reserved for future
 doctor
• Corrects error example: re-vendor with --force (not --pin v1.4.0), and notes bump as planned
 for three-way merge
• Refines execution/presentation layer explanation: skillcore is a separate, importable **public
 package** (SDK-1), not inline; third-party tools can build on the same primitives

docs/design/cli.md


32. test/testdata/sample-origin/skills/terraform-plan-review/skill.toml 🧪 Tests +23/-0

Sample skill manifest with backing-tool prerequisites for testing

• Sample skill manifest for the terraform-plan-review skill in the test fixture origin
• Declares skill identity (name, version, namespace, description) and deterministic discovery tags
• Includes two [[requires]] entries (oxid and terraform) that are **intentionally absent** in the
 test environment to validate that verify is integrity-only and does not fail on missing backing
 tools (SC-006/FR-014)

test/testdata/sample-origin/skills/terraform-plan-review/skill.toml


33. .agents/commands/specledger.implement-workflow-v2.md Additional files +115/-0

...

.agents/commands/specledger.implement-workflow-v2.md


34. .agents/commands/specledger.implement-workflow.md Additional files +115/-0

...

.agents/commands/specledger.implement-workflow.md


35. .agents/skills/skillrig-init/evals/evals.json Additional files +0/-49

...

.agents/skills/skillrig-init/evals/evals.json


36. .agents/skills/skillrig-init/evals/trigger-eval-set.json Additional files +0/-13

...

.agents/skills/skillrig-init/evals/trigger-eval-set.json


37. .agents/skills/skillrig/SKILL.md Additional files +88/-0

...

.agents/skills/skillrig/SKILL.md


38. .agents/skills/skillrig/evals/evals.json Additional files +110/-0

...

.agents/skills/skillrig/evals/evals.json


39. .agents/skills/skillrig/evals/trigger-eval-set.json Additional files +25/-0

...

.agents/skills/skillrig/evals/trigger-eval-set.json


40. .agents/skills/skillrig/references/add.md Additional files +64/-0

...

.agents/skills/skillrig/references/add.md


41. .agents/skills/skillrig/references/init.md Additional files +39/-48

...

.agents/skills/skillrig/references/init.md


42. .agents/skills/skillrig/references/verify.md Additional files +66/-0

...

.agents/skills/skillrig/references/verify.md


43. .specledger/memory/constitution.md Additional files +12/-1

...

.specledger/memory/constitution.md


44. AGENTS.md Additional files +0/-95

...

AGENTS.md


45. CLAUDE.md Additional files +17/-3

...

CLAUDE.md


46. docs/ROADMAP.md Additional files +5/-4

...

docs/ROADMAP.md


47. specledger/002-skillcore-verify/checklists/requirements.md Additional files +37/-0

...

specledger/002-skillcore-verify/checklists/requirements.md


48. specledger/002-skillcore-verify/contracts/add.md Additional files +79/-0

...

specledger/002-skillcore-verify/contracts/add.md


49. specledger/002-skillcore-verify/contracts/skillcore-sdk.md Additional files +86/-0

...

specledger/002-skillcore-verify/contracts/skillcore-sdk.md


50. specledger/002-skillcore-verify/contracts/verify.md Additional files +97/-0

...

specledger/002-skillcore-verify/contracts/verify.md


51. specledger/002-skillcore-verify/data-model.md Additional files +158/-0

...

specledger/002-skillcore-verify/data-model.md


52. specledger/002-skillcore-verify/quickstart.md Additional files +125/-0

...

specledger/002-skillcore-verify/quickstart.md


53. specledger/002-skillcore-verify/sessions/002-skillcore-verify-checkpoint.md Additional files +60/-0

...

specledger/002-skillcore-verify/sessions/002-skillcore-verify-checkpoint.md


54. test/testdata/sample-origin/.skillrig-origin.toml Additional files +10/-0

...

test/testdata/sample-origin/.skillrig-origin.toml


55. test/testdata/sample-origin/skills/terraform-plan-review/SKILL.md Additional files +16/-0

...

test/testdata/sample-origin/skills/terraform-plan-review/SKILL.md


56. test/testdata/sample-origin/skills/terraform-plan-review/check.sh Additional files +6/-0

...

test/testdata/sample-origin/skills/terraform-plan-review/check.sh


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 30, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (3) 📎 Requirement gaps (0)

Grey Divider


Action required

1. verify fails on stdout 📘 Rule violation ≡ Correctness
Description
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.
Code

internal/cli/verify.go[R95-107]

Evidence
PR Compliance ID 783453 requires errors to go to stderr and data to stdout. The new verify path
renders failure output via cmd.OutOrStdout() and the new root error handler returns early for
*skillcore.VerifyFailure, leaving stderr empty while stdout contains verify FAILED ... text.

Rule 783453: Errors must go to stderr and data must go to stdout
internal/cli/verify.go[95-107]
internal/cli/output.go[176-199]
internal/cli/root.go[58-73]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


2. Unchecked fmt.Fprintf errors 📘 Rule violation ≡ Correctness
Description
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.
Code

internal/cli/output.go[R185-196]

Evidence
PR Compliance ID 782713 requires checking all error returns. In internal/cli/output.go,
fmt.Fprintf and b.WriteString return errors that are currently ignored in the newly added
renderVerifyFailure.

Rule 782713: All error returns must be checked
internal/cli/output.go[179-199]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


3. Quickstart tests missing build tag 📘 Rule violation ⌂ Architecture
Description
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.
Code

test/skillcore_quickstart_test.go[R1-14]

Evidence
PR Compliance ID 782685 requires integration test files to start with //go:build integration. The
new quickstart integration suite file has no build tag header, yet it contains TestQuickstart_*
tests.

Rule 782685: Integration tests must use //go:build integration build tag
test/skillcore_quickstart_test.go[1-14]
test/skillcore_quickstart_test.go[302-310]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## 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


View more (3)
4. Symlink target copying ✓ Resolved 🐞 Bug ⛨ Security
Description
Vendoring and divergence checks use os.Stat/os.Open/os.ReadFile, which follow symlinks, so a skill
containing symlinks can cause Add to read/copy files outside the origin skill directory and vendor
unintended content. This also breaks “byte-identical, git-canonical” semantics because git tree
objects record symlinks as symlinks, not the content of their targets.
Code

pkg/skillcore/add.go[R381-416]

Evidence
Both the “identical tree” probe and the actual copy operation use symlink-following APIs, so a
symlink in the origin can cause reads outside the intended subtree and a vendored tree that doesn’t
match git’s object model.

pkg/skillcore/add.go[288-319]
pkg/skillcore/add.go[351-376]
pkg/skillcore/add.go[381-416]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The copy and compare paths follow symlinks, which can:
- vendor data from outside the origin subtree (information disclosure / supply-chain risk)
- produce vendored trees that don’t correspond to the origin’s git tree object

### Issue Context
`filepath.WalkDir` yields symlink entries as non-dirs; the implementation then uses `os.Stat`/`os.Open`/`os.ReadFile` which follow the link.

### Fix Focus Areas
- Decide policy: **reject symlinks** (simplest/safer) or **preserve symlinks** (copy as symlink).
- Implement using `os.Lstat` and `fs.DirEntry.Type()` to detect symlinks and either:
 - return a typed error explaining symlinks are unsupported, or
 - `os.Readlink` + `os.Symlink` to preserve links
 - pkg/skillcore/add.go[254-320]
 - pkg/skillcore/add.go[351-420]
- Update `treesIdentical`/`filesEqual` to compare symlink metadata correctly (or reject symlinks consistently) so the overwrite/idempotency guard matches what `TreeSHA` represents:
 - pkg/skillcore/add.go[208-320]
- Add tests with a symlink inside an origin skill and assert safe behavior:
 - pkg/skillcore/add_test.go[1-260]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. mapAddError uses %s ✓ Resolved 📘 Rule violation ✧ Quality
Description
mapAddError formats path values with %s (e.g., originMissing.OriginDir, overwrite.Path),
which can hide whitespace/empty boundaries in user-facing errors. The checklist requires %q for
string values in error messages to make boundaries visible.
Code

internal/cli/add.go[R171-197]

Evidence
PR Compliance ID 782577 requires %q for string values in error messages. In internal/cli/add.go,
the newly added mapAddError formats originMissing.OriginDir and overwrite.Path with %s,
violating the requirement.

Rule 782577: Use %q in error messages to make string boundaries visible
internal/cli/add.go[171-178]
internal/cli/add.go[191-198]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`mapAddError` uses `%s` when interpolating path strings into user-facing error messages. This can make boundaries (whitespace/empty) ambiguous.

## Issue Context
Compliance requires `%q` for string values in error messages.

## Fix Focus Areas
- internal/cli/add.go[171-197]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Skill name traversal ✓ Resolved 🐞 Bug ⛨ Security
Description
skillcore.Add uses opts.Skill directly in filesystem path construction, so a skill name containing
path separators (e.g. "../..") can escape the intended .agents/skills/<skill> subtree and trigger
arbitrary overwrite/delete via os.RemoveAll and subsequent copying. This can corrupt or remove
unrelated files under RepoRoot (and also read arbitrary paths from the origin checkout).
Code

pkg/skillcore/add.go[R95-133]

Evidence
The PR wires the CLI arg directly into skillcore.Add, which then uses it as a filesystem path
segment for both the origin source and vendored destination (and for deletion on overwrite), with no
validation or containment check.

internal/cli/add.go[64-75]
internal/cli/add.go[116-124]
pkg/skillcore/add.go[95-113]
pkg/skillcore/add.go[129-133]
pkg/skillcore/add.go[151-159]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Add()` trusts `opts.Skill` as a path segment. When `opts.Skill` contains path traversal (`..`), separators (`/` or `\\`), or absolute paths, `filepath.Join()` can resolve outside the canonical vendoring root, and `os.RemoveAll(destDir)` can delete arbitrary directories.

### Issue Context
This affects both the public SDK (`pkg/skillcore`) and the CLI (`internal/cli/add.go`) since CLI arguments are forwarded verbatim.

### Fix Focus Areas
- Add strict skill-name validation (basename-only, no separators, not `.`/`..`, ideally a whitelist regex) in **both**:
 - pkg/skillcore/add.go[79-162]
 - internal/cli/add.go[64-75]
- Add a defensive “stay within repoRoot/.agents/skills” check after join/clean to prevent escapes even if validation is bypassed:
 - pkg/skillcore/add.go[95-144]
- Add tests for traversal attempts (e.g. `../x`, `x/../../y`, absolute paths) to ensure they fail safely before any delete/copy occurs:
 - pkg/skillcore/add_test.go[1-260]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

7. Verify hides git failures ✓ Resolved 🐞 Bug ≡ Correctness
Description
pathInHead treats any *GitError with ExitCode>0 as “path missing”, so Verify can downgrade real git
failures into StatusMissing/StatusOrphan instead of returning a *GitError as the Verify contract
describes. This breaks skillcore.Verify’s documented error-class behavior for SDK consumers that
call it without separately validating repoRoot is a working git repository.
Code

pkg/skillcore/verify.go[R292-309]

Evidence
Verify documents that git failures like “not a git repository” should return *GitError, but
pathInHead converts any *GitError with ExitCode>0 into a non-error missing-path result;
gitClient.run produces positive exit codes for any exec.ExitError, so fatal git failures are
included in that bucket.

pkg/skillcore/verify.go[59-69]
pkg/skillcore/verify.go[292-310]
pkg/skillcore/git.go[27-52]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`pathInHead()` currently assumes any positive exit code from `rev-parse` means “missing path”, but `gitClient.run()` wraps **all** non-zero exits (including fatal failures like “not a git repository”) into `*GitError` with a positive code. This can cause Verify to return a verification report instead of a configuration/usage error.

### Issue Context
The `Verify()` doc explicitly distinguishes *GitError (usage/config) from *VerifyFailure (per-skill findings).

### Fix Focus Areas
- Only treat errors as “missing path” when stderr matches the specific “path does not exist in” form (or use a git command that can distinguish missing tree from fatal failures):
 - pkg/skillcore/verify.go[292-310]
- Keep propagating other git failures as `*GitError` to honor the Verify contract:
 - pkg/skillcore/verify.go[59-69]
- Consider adding a focused unit test for calling Verify on a non-repo directory with a lock entry whose path is absent, asserting it returns `*GitError` (not `*VerifyFailure`):
 - pkg/skillcore/verify_test.go[1-278]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


8. rev-parse option injection ✓ Resolved 🐞 Bug ☼ Reliability
Description
gitClient.revParse invokes git rev-parse <rev> without an option terminator (--), and origin ref
validation allows leading '-' characters, so a ref like "-h" can be interpreted as git flags instead
of a revision. This can break TreeSHA/commit resolution for certain configured origins and is
avoidable with defensive argument construction.
Code

pkg/skillcore/git.go[R57-61]

Evidence
The origin ref regex allows '-' and the CLI forwards that ref into skillcore, while revParse does
not include -- to prevent git from parsing the rev argument as an option.

pkg/skillcore/git.go[57-61]
internal/config/origin.go[18-27]
internal/cli/add.go[139-147]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`git rev-parse` treats leading `-` tokens as options unless terminated by `--`. Since `Origin.Ref` is shape-validated (not semantics-validated) and permits `-`, certain refs can be misinterpreted.

### Issue Context
This affects Add’s `commit` resolution and TreeSHA’s `ref:path` resolution when ref comes from config/env.

### Fix Focus Areas
- Terminate options in `revParse` by inserting `--` before `rev`:
 - pkg/skillcore/git.go[57-61]
- Optionally harden validation to reject refs starting with `-` (belt-and-suspenders):
 - internal/config/origin.go[18-27]
- Add a unit test covering a leading-dash ref behavior (stubbed gitClient or integration test as appropriate):
 - pkg/skillcore/helpers_test.go[1-170]
 - pkg/skillcore/treesha_test.go[1-166]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread internal/cli/add.go
Comment thread internal/cli/verify.go
Comment on lines +95 to +107
// 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
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Comment thread internal/cli/output.go
Comment on lines +185 to +196
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")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Comment on lines +1 to +14
// 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

Comment thread pkg/skillcore/add.go
Comment thread pkg/skillcore/add.go
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>
@so0k
Copy link
Copy Markdown
Contributor Author

so0k commented May 30, 2026

Qodo review addressed (pushed in 76b560e)

Thanks Qodo. Triaged all 8 findings.

Fixed (4 bugs + 1 rule):

Skipped:

Declined, with rationale:

  • chore: sync SpecLedger templates to constitution v2.1.0 #2 verify writes the report to stdout — this is intentional and contract-mandated. For a verification gate, the per-skill verdict report is the data (like grep/diff/go test: non-zero exit, results on stdout); the exit code (2) is the error signal. The spec explicitly requires skillrig verify --json 2>/dev/null | jq . to work, so the JSON must be on stdout even on failure — moving it to stderr would break TestQuickstart_VerifyJSONComplete. Genuine usage/config errors (malformed lock, not-a-repo) do go to stderr.
  • feat(001): support optional branch/ref in origin reference (OWNER/REPO[@REF]) #4 missing //go:build integration tag — this project separates integration tests by the ./test/ directory, not a build tag (the 001 suite has no tag, and make test-integration is go test ./test/...). Adding the tag to only this file would be inconsistent; switching the whole project to build-tag separation is a separate decision, out of scope for this PR.

Gate after the round: golangci-lint 0 issues; go test -cover -count=1 ./... green (skillcore 80.7%, internal/cli 51.6%).

@so0k so0k merged commit 99eb7ee into main May 30, 2026
@so0k so0k deleted the 002-skillcore-verify branch May 30, 2026 11:52
so0k added a commit that referenced this pull request May 31, 2026
…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>
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