From bb9b048f6db5f2eff122e26657b8792ca6a5bffc Mon Sep 17 00:00:00 2001 From: skillrig Date: Mon, 1 Jun 2026 13:01:41 +0000 Subject: [PATCH 1/4] feat(004): add `skillrig show` for a human-friendly full skill view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `skillrig show ` (alias `info`), a Query-pattern command that prints ONE origin skill's complete record — most importantly its full, untruncated description, which `search` clips to ~80 chars and which a human otherwise could only recover via `--json | jq` (issue #17). show reuses search's resolver + catalog-load + convention-gate path (AP-04) and dispatches to a single new exact-name lookup primitive, skillcore.FindSkill. A named skill the origin does not publish is exit 1 with what/why/fix navigation (distinct from search, where an empty query is exit 0). --json emits {origin, skill} with the whole catalog entry. Co-evolution: extends the consolidated skillrig skill (routing row + references/show.md + description keywords), the cli.md command list + Query pattern classification, and the root README. Quickstart contract in specledger/004-show-skill/quickstart.md maps 1:1 to TestQuickstart_Show*. Closes #17 https://claude.ai/code/session_01SAczAuBfsd2hTEGrFBsmQZ --- .agents/skills/skillrig/SKILL.md | 15 +- .agents/skills/skillrig/references/show.md | 66 ++++++ README.md | 19 ++ docs/design/cli.md | 5 +- internal/cli/output.go | 72 ++++++ internal/cli/root.go | 1 + internal/cli/show.go | 142 ++++++++++++ internal/cli/show_test.go | 135 +++++++++++ pkg/skillcore/catalog.go | 17 ++ pkg/skillcore/catalog_test.go | 54 +++++ specledger/004-show-skill/quickstart.md | 32 +++ test/show_quickstart_test.go | 251 +++++++++++++++++++++ 12 files changed, 801 insertions(+), 8 deletions(-) create mode 100644 .agents/skills/skillrig/references/show.md create mode 100644 internal/cli/show.go create mode 100644 internal/cli/show_test.go create mode 100644 specledger/004-show-skill/quickstart.md create mode 100644 test/show_quickstart_test.go diff --git a/.agents/skills/skillrig/SKILL.md b/.agents/skills/skillrig/SKILL.md index ddf6bea..9d0630f 100644 --- a/.agents/skills/skillrig/SKILL.md +++ b/.agents/skills/skillrig/SKILL.md @@ -3,10 +3,12 @@ name: skillrig description: >- Point a repository at your org's agent-skills library and manage vendored skills with the `skillrig` CLI — bind/choose the origin (`init`), search/discover skills in it (`search`), - vendor/add a skill (`add`, local or remote, with an optional immutable `--pin`), - verify/check that committed skills are exactly what was approved (`verify`), and generate - the origin's catalog (`index`). Use whenever the user wants to find, search, discover, - install, add, vendor, pull in, lock, or pin an agent skill from a skills library; filter + view one skill's full details (`show`/`info`), vendor/add a skill (`add`, local or remote, + with an optional immutable `--pin`), verify/check that committed skills are exactly what was + approved (`verify`), and generate the origin's catalog (`index`). Use whenever the user wants + to find, search, discover, install, add, vendor, pull in, lock, or pin an agent skill from a + skills library; read or see a skill's full/complete description or details (`show`/`info`) + when the search table truncated it; filter skills by topic; set/configure where skills come from or fix a "no origin configured" error; point a repo at a skills repo (OWNER/REPO[@branch]) or use SKILLRIG_ORIGIN; fetch from a private/remote origin or debug auth / unreachable / not-found / no-such-version fetch errors; @@ -46,6 +48,7 @@ unmodified (a CI gate), including debugging command output: |---|---|---| | Choose where skills come from (bind the origin) | `skillrig init` | [references/init.md](references/init.md) | | Discover skills in the origin (search/filter by topic) | `skillrig search [QUERY...]` | [references/search.md](references/search.md) | +| Read one skill's full details (untruncated description) | `skillrig show ` (alias `info`) | [references/show.md](references/show.md) | | Vendor a skill into the repo (local or remote; `--pin` a version) | `skillrig add ` | [references/add.md](references/add.md) | | Prove vendored skills match what was approved | `skillrig verify` | [references/verify.md](references/verify.md) | | **Origin-side:** generate the origin's catalog (`index.json`) | `skillrig index` | [references/index.md](references/index.md) | @@ -97,5 +100,5 @@ from our library" is `skillrig`; "what skills exist out there for X?" is `find-s Designed but **not implemented** (don't assume they exist): multi-client symlink views, a prerequisite/health `doctor` (the reserved exit `3`), `bump --pr` upgrades, and `global` -scope. The shipped surface is `init` + `search` + `add` (local **or** remote, with `--pin`) + -`verify`, plus the origin-side `index` generator. +scope. The shipped surface is `init` + `search` + `show` (alias `info`) + `add` (local **or** +remote, with `--pin`) + `verify`, plus the origin-side `index` generator. diff --git a/.agents/skills/skillrig/references/show.md b/.agents/skills/skillrig/references/show.md new file mode 100644 index 0000000..ce6a5d0 --- /dev/null +++ b/.agents/skills/skillrig/references/show.md @@ -0,0 +1,66 @@ +# `skillrig show ` — read one skill's full details (Query) + +> The **human** counterpart to [search](search.md): drill into ONE skill and print its whole +> record — most importantly the **complete, untruncated description** that the `search` table +> clips to ~80 chars. `info` is an alias. Needs an origin (run [init](init.md) first). + +`show` resolves the active origin and reads its catalog (`index.json`) through the **same** +path `search` uses, then prints a single named skill's full record: name, version, namespace, +the full description, topics, path, and backing-tool requirements. Reach for it when `search` +showed a truncated one-liner and a human wants to read the whole thing (an agent can instead +pipe `search --json` to `jq`). + +``` +skillrig show terraform-plan-review # full human-readable record +skillrig info terraform-plan-review # identical (alias) +skillrig show terraform-plan-review --json # complete record for an agent / jq +``` + +## Lookup is exact (a point lookup, not a filter) + +The skill name is matched **exactly** — the same canonical name `add` vendors by — not the +fuzzy, case-insensitive substring match `search` uses. So `show` is for a name you already +know (typically one you saw in `search`). A name the origin does not publish is an **error** +(exit 1), pointing you back at `search` — deliberately unlike `search`, where an empty result +is a clean exit 0. + +## Freshness & origin + +Like `search`, `show` fetches the origin's `index.json` **per call** (no local cache), resolves +the active origin through the shared resolver (`SKILLRIG_ORIGIN` > project `.skillrig/config.toml` +> global), and checks the origin's convention version before reading. A **remote** origin is +fetched over `git` (a private one uses the auto-resolved read-only token); a **local-path** +origin is read with no network. + +## Output + +- **Human (default)** — a labelled block: a `name version (namespace)` header, the `path`, + `topics`, and `requires` lines, then the **full description** as the body, and a footer hint + pointing at `add`. +- **`--json`** — `{ "origin": ..., "skill": { ...full catalog entry... } }`, untruncated and + pipeable: `skillrig show terraform-plan-review --json | jq '.skill.requires'`. + +| Flag | Purpose | +|------|---------| +| `--json` | Emit the complete record (origin + the whole catalog entry) on stdout | +| `--verbose` | Show the raw underlying cause behind a summary or error | + +## Exit codes + +| Code | When | +|------|------| +| `0` | Success — the named skill was found and printed | +| `1` | Usage/config: no origin configured, the named skill is **not in the origin**, unreachable/auth/incompatible-convention fetching the catalog, bad args | + +`show` never emits `2`/`3` (those are reserved for verification/prerequisite gates). + +## Error handling + +| Symptom (stderr) | Cause | Fix | +|------------------|-------|-----| +| `skill "" not found in origin ...` | no skill by that exact name in the catalog | run `skillrig search` to list the real names, then `show` one | +| `no origin configured` | no resolvable origin | `skillrig init --origin OWNER/REPO`, or set `SKILLRIG_ORIGIN` | +| `... is unreachable` / `authentication ... failed` / `... not found` / `convention version N` | catalog fetch/gate failures (shared with `search`) | see [search.md](search.md) — same typed errors and fixes | + +All failures state what/why/fix and exit `1`; `--verbose` shows the raw cause. Errors to +stderr, data to stdout (so `skillrig show --json 2>/dev/null | jq .` stays clean). diff --git a/README.md b/README.md index befc66f..e0879ab 100644 --- a/README.md +++ b/README.md @@ -125,6 +125,25 @@ skillrig search terraform plan skillrig search --topic aws --topic terraform ``` +### `show` + +Print **one** skill's full record from the configured origin — its complete, +**untruncated** description (the part `search` clips to a one-line preview), plus +its version, namespace, topics, path, and backing-tool requirements. `show` is the +human counterpart to `search`: where `search` lists many skills compactly, `show` +drills into one (an agent gets the same data from `search ... --json | jq`). +`info` is an alias. Read-only; needs a resolvable origin but no git working tree; +the skill name is matched exactly. A name the origin does not publish is exit 1 +(run `skillrig search` to list the real names). + +```sh +# Show a skill's full details (alias: skillrig info ). +skillrig show terraform-plan-review + +# The complete record as JSON, for an agent or jq. +skillrig show terraform-plan-review --json +``` + ### `add` Vendor a named skill from the configured origin into the canonical diff --git a/docs/design/cli.md b/docs/design/cli.md index 5559431..d58ba60 100644 --- a/docs/design/cli.md +++ b/docs/design/cli.md @@ -40,6 +40,7 @@ skillrig — rig up your agents with skills (git-native skill distribution) Commands: search Query the origin's index.json for skills [implemented] + show Print one skill's full record (untruncated description) [implemented] add Vendor a skill into this repo + write the lock entry [implemented] verify Offline integrity check — label-honesty (exit code; CI gate) [implemented] index Generate the origin's index.json from skill frontmatter [implemented, origin-side] @@ -319,7 +320,7 @@ Every `skillrig` subcommand MUST identify which pattern(s) it follows. This clas | Pattern | Purpose | Examples | Constraints | |---------|---------|----------|-------------| -| **Query** | Deterministic read of the discovery artifact | `search` *(implemented)* | Reads the origin's `index.json` (fetched per call — no offline cache this slice; an unreachable origin is the `UnreachableError`). Query-first: deterministic token-AND substring over `name`+`description`+`topics` + exact `--topic` filter; fixed relevance-bucket then lexicographic order — **no inference / no fuzzy ranking** (N6). Empty result = clean exit 0. Gates the origin's `skillrigConvention` before reading. | +| **Query** | Deterministic read of the discovery artifact | `search`, `show` *(implemented)* | Reads the origin's `index.json` (fetched per call — no offline cache this slice; an unreachable origin is the `UnreachableError`). `search` is query-first: deterministic token-AND substring over `name`+`description`+`topics` + exact `--topic` filter; fixed relevance-bucket then lexicographic order — **no inference / no fuzzy ranking** (N6); an empty result = clean exit 0. `show ` (alias `info`) is the **point-lookup** sibling: an EXACT-name match into the same catalog that prints ONE skill's full record — the complete, untruncated description `search` clips to ~80 chars (issue #17) — so an unknown NAMED skill is a usage error (exit 1), not the empty-set success. Both gate the origin's `skillrigConvention` before reading. | | **Vendor Mutation** | Write skill tree + lock entry | `add` *(implemented — local + remote)*, `bump --pr` | Writes lock via `skillcore` only. Serves a **local-path** origin (read a checkout) and a **remote** `OWNER/REPO` origin (fetch the subtree over `git`, token via `os.exec` of `gh`/`git`, never a write credential) — the two origin forms are classified, never "both-present". `--pin` vendors an immutable version. Supports `--dry-run`; refuses to clobber content that diverges from the locked `treeSha` without `--force`. `bump` *proposes* (opens a PR), never force-adopts (R13). MUST never silently discard local edits (R32). Vendors byte-identical + mode-preserving; the skill name MUST be a single path segment (no traversal); **path-traversal + symlink guards apply to remotely-fetched content too**. **Symlinks in a skill subtree are rejected this slice** — following them would break byte-identical / git-canonical vendoring (git records a symlink as a link, not its target); preserving symlinks faithfully is a future relaxation. | | **Verification Gate** | Offline integrity / prereq / conformance | `verify` *(implemented — integrity-only)*, `lint` | MUST be offline + deterministic. Exit-code driven. **No live/online signal in this path** (R11/N1). `verify` = consumer CI gate; `lint` = author CI gate on the origin. As implemented, `verify` is **integrity-only** (label-honesty + orphan detection, exit 2); prerequisite/eligibility checks (a missing `requires` tool → exit 3) belong to the future `doctor`, so `verify` does not emit exit 3 today. | | **Environment** | Health, auth, config, bootstrap | `doctor`, `init` | MUST be idempotent. `doctor` checks prerequisite auth (R18); works without a fully-configured project. `init` is **consumer-side only** — binds to an *existing* origin, never bootstraps one (architecture §2d). | @@ -333,7 +334,7 @@ Each pattern has a distinct failure mode expectation: | Pattern | Failure Mode | |---------|-------------| -| **Query** | MUST fail with clear error + suggested fix (no origin → run `init`; unreachable/auth/incompatible-convention fetching the catalog → the matching typed error). An **empty match set is success (exit 0)**, not a failure — it prints a footer hint, not an error. | +| **Query** | MUST fail with clear error + suggested fix (no origin → run `init`; unreachable/auth/incompatible-convention fetching the catalog → the matching typed error). For `search`, an **empty match set is success (exit 0)**, not a failure — it prints a footer hint, not an error. For `show`, a **named skill the origin does not publish is exit 1** (a point lookup of a named thing that doesn't exist), with a what/why/fix pointing at `search`. | | **Vendor Mutation** | MUST validate origin + auth before fetching. Three-way-merge conflict → non-zero exit, write git-style conflict markers, instruct resolve-and-rerun (architecture §5b). Never discard local edits. | | **Verification Gate** | MUST be deterministic pass/fail by exit code. Label-honesty mismatch = fail (exit 2); orphan = fail (exit 2); unresolved conflict markers = fail. Prereq miss (exit 3) is reserved for the future `doctor` — the implemented `verify` is integrity-only and does not emit it. | | **Environment** | MUST be idempotent and safe to retry. MUST distinguish "tool missing" from "tool exists but unauthenticated" (R18). | diff --git a/internal/cli/output.go b/internal/cli/output.go index b2d0881..47eb3ce 100644 --- a/internal/cli/output.go +++ b/internal/cli/output.go @@ -195,6 +195,78 @@ func writeAlignedColumns(w io.Writer, rows [][]string) error { return tw.Flush() } +// showResultJSON is the complete, untruncated --json view of a show: the +// resolved origin and the single matched skill with every field add needs. It +// reuses the skillcore.CatalogEntry JSON tags, so the record is byte-for-byte the +// same shape search emits per entry — one entry, named `skill`. +type showResultJSON struct { + Origin string `json:"origin"` + Skill skillcore.CatalogEntry `json:"skill"` +} + +// showFooterPrefix is the next-step footer for a human show — the skill name is +// appended so the hint is a runnable command (cli.md Principle 3). +const showFooterPrefix = "→ vendor it: skillrig add " + +// renderShowResult writes a single skill's full record to w. With jsonOut it +// emits one complete JSON object (origin + the whole catalog entry, all fields +// present); otherwise a human-friendly labelled block whose defining feature is +// the COMPLETE, untruncated description — the gap issue #17 closes, since search +// clips it to ~80 chars. The block is a fixed handful of header/field lines plus +// the description body and a footer hint. Data goes to stdout (the caller passes +// cmd.OutOrStdout()). +func renderShowResult(w io.Writer, origin string, e skillcore.CatalogEntry, jsonOut bool) error { + if jsonOut { + enc := json.NewEncoder(w) + enc.SetEscapeHTML(false) + + return enc.Encode(showResultJSON{Origin: origin, Skill: e}) + } + + var b strings.Builder + + fmt.Fprintf(&b, "%s %s (%s)\n", e.Name, e.Version, e.Namespace) + fmt.Fprintf(&b, "path: %s\n", e.Path) + + if len(e.Topics) > 0 { + fmt.Fprintf(&b, "topics: %s\n", strings.Join(e.Topics, ", ")) + } + + if len(e.Requires) > 0 { + fmt.Fprintf(&b, "requires: %s\n", joinRequires(e.Requires)) + } + + // The whole point of show: the complete description, untruncated, set off by a + // blank line so it reads as the body of the record. + if desc := strings.TrimSpace(e.Description); desc != "" { + fmt.Fprintf(&b, "\n%s\n", desc) + } + + fmt.Fprintf(&b, "\n%s%s\n", showFooterPrefix, e.Name) + + _, err := io.WriteString(w, b.String()) + + return err +} + +// joinRequires renders a skill's backing-tool requirements as a compact human +// summary — "tool (version)" joined by commas, or a bare tool when no version +// constraint is recorded — mirroring search's requires summary. +func joinRequires(reqs []skillcore.Require) string { + parts := make([]string, 0, len(reqs)) + + for _, r := range reqs { + part := r.Tool + if r.Version != "" { + part += " (" + r.Version + ")" + } + + parts = append(parts, part) + } + + return strings.Join(parts, ", ") +} + // searchEmptyFooter is the next-step hint for an empty search result (still exit 0). const searchEmptyFooter = "→ broaden the query, or run skillrig search with no filter to list all" diff --git a/internal/cli/root.go b/internal/cli/root.go index 459d53f..dd31d6f 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -128,6 +128,7 @@ func Execute() int { func registerSubcommands(root *cobra.Command, opts *globalOpts) { root.AddCommand(newInitCmd(opts)) root.AddCommand(newSearchCmd(opts)) + root.AddCommand(newShowCmd(opts)) root.AddCommand(newAddCmd(opts)) root.AddCommand(newVerifyCmd(opts)) root.AddCommand(newIndexCmd(opts)) diff --git a/internal/cli/show.go b/internal/cli/show.go new file mode 100644 index 0000000..ef1403e --- /dev/null +++ b/internal/cli/show.go @@ -0,0 +1,142 @@ +package cli + +import ( + "errors" + + "github.com/spf13/cobra" + + "github.com/skillrig/cli/internal/config" + "github.com/skillrig/cli/pkg/skillcore" +) + +// showCmd holds the show command's state and its injectable seams. Like search +// it is read-only and resolves the origin through the shared resolver; tests +// inject deterministic stubs (cwd, env). +type showCmd struct { + opts *globalOpts + skill string + + // getwd returns the working directory. Defaults to os.Getwd. + getwd func() (string, error) + // env is the environment accessor used by the origin resolver. + env config.Env +} + +// newShowCmd builds the `skillrig show ` command (Query pattern): print +// the full, human-readable record of ONE skill the resolved origin publishes — +// notably the COMPLETE, untruncated description that search abbreviates to a +// one-liner (issue #17). `info` is an alias (the second name the issue proposed, +// and the desire path an agent reaches for). Read-only, exit 0 on a found skill, +// exit 1 on a config/convention/reachability problem or an unknown skill name. +func newShowCmd(opts *globalOpts) *cobra.Command { + sc := &showCmd{ + opts: opts, + getwd: osGetwd, + env: config.OSEnv, + } + + cmd := &cobra.Command{ + Use: "show ", + Aliases: []string{"info"}, + Short: "Show one skill's full details from your configured origin", + Long: "show prints the complete, human-readable record of a single skill published by\n" + + "your configured origin: its full (untruncated) description, version, namespace,\n" + + "topics, path, and backing-tool requirements. It is the human counterpart to\n" + + "search — where search lists many skills with a one-line, truncated description,\n" + + "show drills into one and prints the whole thing (an agent gets the same data from\n" + + "search --json piped to jq).\n\n" + + "show resolves the active origin (SKILLRIG_ORIGIN > project > global) and reads the\n" + + "origin's catalog (index.json) exactly like search; the skill name is matched\n" + + "exactly (the same canonical name add vendors by). It is read-only and needs no git\n" + + "working tree — only a resolvable origin. Add --json for the complete record.", + Example: " # Show a skill's full details (alias: skillrig info )\n" + + " skillrig show terraform-plan-review\n\n" + + " # The complete record as JSON, for an agent or jq\n" + + " skillrig show terraform-plan-review --json", + // A custom validator (not cobra.ExactArgs) so a misinvocation is + // errors-as-navigation — what/why/fix + an example — instead of cobra's + // terse "accepts 1 arg(s)" dead end (cli.md Principle 1/2). + Args: func(_ *cobra.Command, args []string) error { + if len(args) != 1 { + return usageShowArgs(len(args)) + } + + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { + sc.skill = args[0] + + return sc.run(cmd) + }, + } + + return cmd +} + +// run resolves the origin, loads + gates the catalog (the same acquisition path +// search uses, AP-04), finds the named skill, and renders its full record. A +// missing origin/catalog or an incompatible convention map to the shared +// navigational errors (exit 1); an unknown skill name is its own what/why/fix +// usage error (exit 1) — distinct from search, where an empty *query* is exit 0. +func (sc *showCmd) run(cmd *cobra.Command) error { + cwd, err := sc.getwd() + if err != nil { + return &UsageError{Msg: "cannot determine working directory\nwhy: " + err.Error(), Cause: err} + } + + res, err := config.ResolveOrigin(cwd, sc.env) + if err != nil { + return &UsageError{Msg: "cannot resolve the active origin\nwhy: " + err.Error() + "\n" + missingOriginFix, Cause: err} + } + + if res.Source == config.SourceNone { + return usageNoOriginConfigured() + } + + // A git repo is OPTIONAL (as for search): it only enables the local-checkout + // fast-path for a remote origin's committed checkout. Outside a repo, repoRoot + // is empty and loadCatalog fetches the catalog directly (FIX-7). + repoRoot, err := gitToplevel(cmd.Context(), cwd) + if err != nil { + if !errors.Is(err, errNotGitRepo) { + return mapSearchError(res.Origin.String(), err) + } + + repoRoot = "" + } + + catalog, err := loadCatalog(cmd.Context(), repoRoot, res.Origin) + if err != nil { + return mapSearchError(res.Origin.String(), err) + } + + if err := skillcore.CheckConvention(catalog.SkillrigConvention); err != nil { + return mapSearchError(res.Origin.String(), err) + } + + entry, ok := skillcore.FindSkill(catalog, sc.skill) + if !ok { + return usageShowSkillNotFound(sc.skill, res.Origin.String()) + } + + return renderShowResult(cmd.OutOrStdout(), res.Origin.String(), entry, sc.opts.json) +} + +// usageShowArgs builds the navigational usage error for a wrong show argument +// count (errors-as-navigation: what / why / fix + a concrete example), replacing +// cobra's bare "accepts 1 arg(s)" message. +func usageShowArgs(got int) *UsageError { + return usageErrorf("show requires exactly one argument: the skill name\n"+ + "why: got %d argument(s)\n"+ + "fix: skillrig show (e.g. skillrig show terraform-plan-review); run skillrig search to list the origin's skills", got) +} + +// usageShowSkillNotFound builds the 3-part error for a named skill the origin's +// catalog does not publish. Unlike search (an empty *query* is a clean exit 0), +// show is a point lookup of a NAMED skill, so an unknown name is a usage error +// the agent must correct — the fix points at search to list the real names. +func usageShowSkillNotFound(skill, origin string) *UsageError { + return usageErrorf("skill %q not found in origin %s\n"+ + "why: the origin's catalog publishes no skill by that exact name\n"+ + "fix: run skillrig search to list the skills the origin publishes, then show one of those names", skill, origin) +} diff --git a/internal/cli/show_test.go b/internal/cli/show_test.go new file mode 100644 index 0000000..ab8fc37 --- /dev/null +++ b/internal/cli/show_test.go @@ -0,0 +1,135 @@ +package cli + +import ( + "bytes" + "encoding/json" + "strings" + "testing" + + "github.com/skillrig/cli/pkg/skillcore" +) + +// sampleShowEntry is a catalog entry with a long, multi-line description, topics, +// and a mix of version-constrained and bare requires — the full surface +// renderShowResult must project. +func sampleShowEntry() skillcore.CatalogEntry { + return skillcore.CatalogEntry{ + Name: "terraform-plan-review", + Version: "1.4.0", + Namespace: "my-org", + Description: "Review a terraform plan for risk before apply.\n\nIt is read-only and never mutates the plan.", + Topics: []string{"platform-team", "terraform", "aws"}, + Path: "skills/terraform-plan-review", + Requires: []skillcore.Require{ + {Tool: "oxid", Version: ">=0.4.0"}, + {Tool: "terraform"}, + }, + } +} + +// TestRenderShowResult_Human asserts the human block carries the full +// (untruncated, multi-line) description — the issue #17 contract — plus the +// labelled fields, a "tool (version)"/"tool" requires summary, and a runnable +// footer hint. +func TestRenderShowResult_Human(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + e := sampleShowEntry() + if err := renderShowResult(&buf, "my-org/my-skills", e, false); err != nil { + t.Fatalf("renderShowResult: %v", err) + } + + out := buf.String() + + // Full description, both lines, untruncated. + for _, want := range []string{ + "Review a terraform plan for risk before apply.", + "It is read-only and never mutates the plan.", + "terraform-plan-review 1.4.0 (my-org)", + "skills/terraform-plan-review", + "platform-team, terraform, aws", + } { + if !strings.Contains(out, want) { + t.Errorf("human output missing %q:\n%s", want, out) + } + } + + // requires: a version constraint is parenthesised; a bare tool stands alone. + if !strings.Contains(out, "requires: oxid (>=0.4.0), terraform") { + t.Errorf("requires summary wrong, want 'oxid (>=0.4.0), terraform':\n%s", out) + } + + // Footer hint is the runnable add command. + if !strings.Contains(out, "→ vendor it: skillrig add terraform-plan-review") { + t.Errorf("missing runnable footer hint:\n%s", out) + } +} + +// TestRenderShowResult_JSONComplete asserts --json is parseable and structurally +// complete: origin + a skill object with every field, the description untruncated. +func TestRenderShowResult_JSONComplete(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + e := sampleShowEntry() + if err := renderShowResult(&buf, "my-org/my-skills", e, true); err != nil { + t.Fatalf("renderShowResult: %v", err) + } + + var payload struct { + Origin string `json:"origin"` + Skill skillcore.CatalogEntry `json:"skill"` + } + + if err := json.Unmarshal(buf.Bytes(), &payload); err != nil { + t.Fatalf("show --json not parseable: %v\n%s", err, buf.String()) + } + + if payload.Origin != "my-org/my-skills" { + t.Errorf("origin = %q, want my-org/my-skills", payload.Origin) + } + + if payload.Skill.Name != e.Name || payload.Skill.Version != e.Version || payload.Skill.Path != e.Path { + t.Errorf("skill round-trip mismatch: %+v", payload.Skill) + } + + if payload.Skill.Description != e.Description { + t.Errorf("--json description truncated/altered:\ngot: %q\nwant: %q", payload.Skill.Description, e.Description) + } + + if len(payload.Skill.Topics) != 3 || len(payload.Skill.Requires) != 2 { + t.Errorf("topics/requires not complete: topics=%v requires=%v", payload.Skill.Topics, payload.Skill.Requires) + } +} + +// TestJoinRequires covers the requires summary forms in isolation. +func TestJoinRequires(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in []skillcore.Require + want string + }{ + {name: "empty", in: nil, want: ""}, + {name: "bare tool only", in: []skillcore.Require{{Tool: "terraform"}}, want: "terraform"}, + { + name: "version constrained and bare mixed", + in: []skillcore.Require{{Tool: "oxid", Version: ">=0.4.0"}, {Tool: "terraform"}}, + want: "oxid (>=0.4.0), terraform", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := joinRequires(tt.in); got != tt.want { + t.Errorf("joinRequires(%v) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} diff --git a/pkg/skillcore/catalog.go b/pkg/skillcore/catalog.go index 1c04c8f..15696d6 100644 --- a/pkg/skillcore/catalog.go +++ b/pkg/skillcore/catalog.go @@ -320,6 +320,23 @@ func Search(catalog Catalog, query, topics []string) []CatalogEntry { return matched } +// FindSkill returns the catalog entry whose name matches name exactly, and a +// bool reporting whether one was found. The match is exact and case-sensitive: a +// skill name is a single path segment and the canonical key add vendors by, so a +// point lookup resolves the same identity add would. It is the ONE exact-name +// lookup primitive show dispatches to (AP-04) — deliberately separate from the +// fuzzy, case-insensitive Search matcher, which is a filter, not an identity +// lookup. +func FindSkill(catalog Catalog, name string) (CatalogEntry, bool) { + for _, entry := range catalog.Skills { + if entry.Name == name { + return entry, true + } + } + + return CatalogEntry{}, false +} + // matchesEntry reports whether entry satisfies every lowercased query term // (token-AND substring over name+description+topics) and carries every requested // topic (case-insensitive exact membership). diff --git a/pkg/skillcore/catalog_test.go b/pkg/skillcore/catalog_test.go index aa8a396..bcb2416 100644 --- a/pkg/skillcore/catalog_test.go +++ b/pkg/skillcore/catalog_test.go @@ -342,6 +342,60 @@ func TestSearch_OrderingAndDeterminism(t *testing.T) { } } +// TestFindSkill_ExactMatch pins FindSkill's contract: an exact, case-sensitive +// name lookup that returns the full entry when present and (zero, false) when +// not. It is the identity lookup show dispatches to — deliberately stricter than +// the fuzzy, case-insensitive Search matcher, so a case-mismatched or partial +// name is a miss, not a loose hit. +func TestFindSkill_ExactMatch(t *testing.T) { + t.Parallel() + + catalog := Catalog{ + SkillrigConvention: 1, + Origin: "my-org/my-skills", + Skills: []CatalogEntry{ + {Name: "terraform-plan-review", Version: "1.4.0", Description: "Review a plan."}, + {Name: "aws-iam-audit", Version: "2.0.0", Description: "Audit IAM."}, + }, + } + + tests := []struct { + name string + query string + wantFound bool + wantVer string + }{ + {name: "exact hit returns the full entry", query: "terraform-plan-review", wantFound: true, wantVer: "1.4.0"}, + {name: "second exact hit", query: "aws-iam-audit", wantFound: true, wantVer: "2.0.0"}, + {name: "case mismatch is a miss (exact, not fuzzy)", query: "Terraform-Plan-Review", wantFound: false}, + {name: "partial name is a miss (lookup, not filter)", query: "terraform", wantFound: false}, + {name: "absent name is a miss", query: "kubernetes", wantFound: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got, found := FindSkill(catalog, tt.query) + if found != tt.wantFound { + t.Fatalf("FindSkill(%q) found = %v, want %v", tt.query, found, tt.wantFound) + } + + if !found { + if got.Name != "" { + t.Errorf("FindSkill(%q) miss should return the zero entry, got %+v", tt.query, got) + } + + return + } + + if got.Name != tt.query || got.Version != tt.wantVer { + t.Errorf("FindSkill(%q) = %q/%q, want %q/%q", tt.query, got.Name, got.Version, tt.query, tt.wantVer) + } + }) + } +} + // namesOf projects entries to their names, preserving order. It returns a // non-nil empty slice for an empty input so reflect.DeepEqual against a // []string{} literal (the no-match expectation) holds. diff --git a/specledger/004-show-skill/quickstart.md b/specledger/004-show-skill/quickstart.md new file mode 100644 index 0000000..9687ba8 --- /dev/null +++ b/specledger/004-show-skill/quickstart.md @@ -0,0 +1,32 @@ +# Quickstart — Acceptance Contract: `004-show-skill` + +Each scenario is an executable `TestQuickstart_*` (Constitution §II): concrete invocations, observable output, exit codes, and **output-shape** assertions (full untruncated human body; parseable + complete `--json`; 3-part errors). Resolves [issue #17](https://github.com/skillrig/cli/issues/17): humans had no way to read a skill's *full* description — `search` truncates it to ~80 chars and only an agent could recover it via `--json | jq`. + +`show` is a **Query** command (cli.md Pattern Classification): a deterministic read of the origin's `index.json` through the same resolver + catalog-load + convention-gate path `search` uses (AP-04), drilling into ONE named skill instead of listing many. `info` is an alias. + +## Test substrate +- Reuses the 003 search substrate: a git **consumer** repo whose origin (`my-org/my-skills`) ships an `index.json` read straight off disk, bound via `SKILLRIG_ORIGIN` (the `searchConsumer` helper). A dedicated `showCatalog()` carries one skill with a **long, multi-line** description (> the 80-char search truncation width) so non-truncation is observable. +- Origin/convention/reachability failures share `search`'s code path, so they are covered there; `show` adds only its point-lookup-specific scenarios below. + +--- + +## US1 — Read one skill's full record · P1 + +**`TestQuickstart_ShowFullDescription`** — `skillrig show ` on a skill with a long multi-line description prints the description **in full** (the exact tail bytes search would clip are present) + the name/version/topics/path labels + a footer hint pointing at `add`; **exit 0**. Cross-check: `skillrig search ` over the same catalog truncates the same description (ellipsis, tail absent) — proving show closes the gap. +**`TestQuickstart_ShowInfoAlias`** — `skillrig info ` is byte-identical to `skillrig show ` (the alias the issue named). +**`TestQuickstart_ShowJSONComplete`** — `--json` parses (`json.Unmarshal` ok) and carries `origin` + a `skill` object with the full field set (`name`/`version`/`namespace`/`description`/`topics`/`path`), the `description` value untruncated (field-presence + full body, not truncation-absence). +**`TestQuickstart_ShowHelpExamples`** — `show --help` shows the purpose line + ≥2 runnable `skillrig show` examples. + +## US2 — Trustworthy failures · P2 + +**`TestQuickstart_ShowSkillNotFound`** — `skillrig show no-such-skill` against a populated origin → **exit 1**, empty stdout, a 3-part error: what (the named skill is not in the origin), why (no skill by that exact name), fix (run `skillrig search`). Distinct from `search`, where an empty result is exit 0. +**`TestQuickstart_ShowNoOriginConfigured`** — `skillrig show ` with no resolvable origin → **exit 1**, the shared `no origin configured` what/why/fix (run `skillrig init`). +**`TestQuickstart_ShowMissingArg`** — `skillrig show` (no args) → **exit 1** with the navigational "requires exactly one argument" message (not cobra's bare "accepts 1 arg(s)"). + +--- + +### Traceability +| US | Scenarios | Maps to | +|---|---|---| +| US1 read | ShowFullDescription / ShowInfoAlias / ShowJSONComplete / ShowHelpExamples | issue #17 (full human-readable description) | +| US2 failures | ShowSkillNotFound / ShowNoOriginConfigured / ShowMissingArg | cli.md Principle 2 (errors-as-navigation), exit-code contract | diff --git a/test/show_quickstart_test.go b/test/show_quickstart_test.go new file mode 100644 index 0000000..c68d5e2 --- /dev/null +++ b/test/show_quickstart_test.go @@ -0,0 +1,251 @@ +// This file holds the TestQuickstart_* integration suite for feature +// 004-show-skill (the `skillrig show`/`info` command, issue #17). Each scenario +// maps 1:1 to a scenario in specledger/004-show-skill/quickstart.md. +// +// Like the 001/002/003 suites it builds the binary once (TestMain in +// quickstart_test.go) and execs it via run(). show is a Query command that reads +// the origin's index.json through the same resolver + catalog path search uses, +// so it reuses the 003 search substrate (searchConsumer / catalogFile) — a git +// consumer repo whose origin ships a hand-authored index.json read off disk and +// bound via SKILLRIG_ORIGIN. The point-lookup-specific behaviour (full +// untruncated description, info alias, skill-not-found exit 1) is what these +// scenarios pin; origin/convention/reachability failures share search's code +// path and are covered by the 003 suite. +package quickstart + +import ( + "strings" + "testing" +) + +// showDescTail is a sentinel embedded near the END of the long description below, +// past the 80-char point where search truncates. show must print it; search must +// not — the observable proof that show closes issue #17's gap. +const showDescTail = "SENTINEL-TAIL-VISIBLE-ONLY-IN-SHOW" + +// longDescription is a multi-line, well-over-80-char description: the exact shape +// search abbreviates to a single truncated line and show must render in full. +const longDescription = "Review a terraform plan for risk before apply: it inspects resource " + + "deletions, replacements, and IAM changes, flags drift against the last applied state, " + + "and summarizes the blast radius for a human approver.\n\n" + + "It is read-only and never mutates the plan — " + showDescTail + "." + +// showCatalog is a deterministic catalog whose first skill carries the long, +// multi-line description (plus topics) the show scenarios assert is rendered in +// full. A couple of extra skills keep it a realistic multi-skill origin. +func showCatalog() catalogFile { + return catalogFile{ + SkillrigConvention: 1, + Origin: originRepo, + Skills: []catalogSkill{ + { + Name: "terraform-plan-review", + Version: "1.4.0", + Namespace: "my-org", + Description: longDescription, + Topics: []string{"platform-team", "terraform", "aws"}, + Path: "skills/terraform-plan-review", + }, + { + Name: "aws-iam-audit", + Version: "2.0.0", + Namespace: "my-org", + Description: "Audit a terraform-managed AWS IAM policy set for drift.", + Topics: []string{"security", "aws"}, + Path: "skills/aws-iam-audit", + }, + }, + } +} + +// show runs `skillrig show args...` (or any alias passed as the first arg by the +// caller) in the consumer with the origin bound via SKILLRIG_ORIGIN. +func (c searchConsumer) show(t *testing.T, args ...string) runResult { + t.Helper() + + return run(t, runOpts{ + args: append([]string{"show"}, args...), + cwd: c.root, + env: map[string]string{"SKILLRIG_ORIGIN": originRepo}, + }) +} + +// --------------------------------------------------------------------------- +// US1 — Read one skill's full record +// --------------------------------------------------------------------------- + +// TestQuickstart_ShowFullDescription — show prints the COMPLETE description +// (including the tail bytes search clips) plus the labelled fields + a footer +// hint, exit 0; and search over the same catalog truncates that description — +// the observable proof show closes issue #17's gap. +func TestQuickstart_ShowFullDescription(t *testing.T) { + t.Parallel() + + c := newSearchConsumer(t, showCatalog()) + + res := c.show(t, sampleSkill) + if res.exit != 0 { + t.Fatalf("show exit = %d, want 0 (stderr: %s)", res.exit, res.stderr) + } + + if res.stderr != "" { + t.Errorf("show success must keep stderr empty, got: %q", res.stderr) + } + + // The defining contract of issue #17: the full, untruncated description body. + if !strings.Contains(res.stdout, showDescTail) { + t.Errorf("show must print the FULL description incl. the tail %q, got:\n%s", showDescTail, res.stdout) + } + + // Labelled record: name/version, topics, path, and a runnable footer hint. + for _, want := range []string{sampleSkill, sampleVersion, "terraform", "skills/terraform-plan-review", "skillrig add " + sampleSkill} { + if !strings.Contains(res.stdout, want) { + t.Errorf("show output missing %q:\n%s", want, res.stdout) + } + } + + // Cross-check: search over the SAME catalog truncates the same description — + // the tail search would clip is absent (this is the gap show fills). + sres := c.search(t, sampleSkill) + if sres.exit != 0 { + t.Fatalf("cross-check search exit = %d, want 0 (stderr: %s)", sres.exit, sres.stderr) + } + + if strings.Contains(sres.stdout, showDescTail) { + t.Errorf("search is expected to TRUNCATE the description (tail %q should be absent), got:\n%s", showDescTail, sres.stdout) + } +} + +// TestQuickstart_ShowInfoAlias — `skillrig info ` is the `show` alias the +// issue named: byte-identical output to `skillrig show `. +func TestQuickstart_ShowInfoAlias(t *testing.T) { + t.Parallel() + + c := newSearchConsumer(t, showCatalog()) + + showRes := c.show(t, sampleSkill) + if showRes.exit != 0 { + t.Fatalf("show exit = %d, want 0 (stderr: %s)", showRes.exit, showRes.stderr) + } + + infoRes := run(t, runOpts{ + args: []string{"info", sampleSkill}, + cwd: c.root, + env: map[string]string{"SKILLRIG_ORIGIN": originRepo}, + }) + + if infoRes.exit != 0 { + t.Fatalf("info alias exit = %d, want 0 (stderr: %s)", infoRes.exit, infoRes.stderr) + } + + if infoRes.stdout != showRes.stdout { + t.Errorf("info alias output differs from show:\ninfo=%s\nshow=%s", infoRes.stdout, showRes.stdout) + } +} + +// TestQuickstart_ShowJSONComplete — --json parses and carries origin + a single +// `skill` object with the full field set, the description untruncated. +func TestQuickstart_ShowJSONComplete(t *testing.T) { + t.Parallel() + + c := newSearchConsumer(t, showCatalog()) + + res := c.show(t, sampleSkill, "--json") + if res.exit != 0 { + t.Fatalf("show --json exit = %d, want 0 (stderr: %s)", res.exit, res.stderr) + } + + obj := decodeJSON(t, res.stdout) + requireKeys(t, obj, "origin", "skill") + + skill, ok := obj["skill"].(map[string]any) + if !ok { + t.Fatalf("skill is not an object: %v", obj["skill"]) + } + + requireKeys(t, skill, "name", "version", "namespace", "description", "topics", "path") + + if skill["name"] != sampleSkill { + t.Errorf("--json skill.name = %v, want %s", skill["name"], sampleSkill) + } + + // The description must be the COMPLETE value (issue #17), not a clipped copy. + desc, _ := skill["description"].(string) + if !strings.Contains(desc, showDescTail) { + t.Errorf("--json skill.description is truncated (missing tail %q): %q", showDescTail, desc) + } +} + +// TestQuickstart_ShowHelpExamples — show --help shows the purpose line + >=2 +// runnable `skillrig show` examples (bounded shape). +func TestQuickstart_ShowHelpExamples(t *testing.T) { + t.Parallel() + + res := run(t, runOpts{args: []string{"show", "--help"}}) + if res.exit != 0 { + t.Fatalf("show --help exit = %d, want 0", res.exit) + } + + if n := countExampleLines(res.stdout, "skillrig show"); n < 2 { + t.Errorf("show --help shows %d 'skillrig show' example lines, want >= 2:\n%s", n, res.stdout) + } +} + +// --------------------------------------------------------------------------- +// US2 — Trustworthy failures +// --------------------------------------------------------------------------- + +// TestQuickstart_ShowSkillNotFound — a NAMED skill the origin does not publish is +// exit 1 with a 3-part error (distinct from search, where an empty result is +// exit 0). +func TestQuickstart_ShowSkillNotFound(t *testing.T) { + t.Parallel() + + c := newSearchConsumer(t, showCatalog()) + + res := c.show(t, "no-such-skill") + if res.exit != 1 { + t.Fatalf("show of an unknown skill exit = %d, want 1 (stderr: %s)", res.exit, res.stderr) + } + + if res.stdout != "" { + t.Errorf("error path must keep stdout empty, got: %q", res.stdout) + } + + assertContains(t, "what", res.stderr, "no-such-skill") + assertContains(t, "why", res.stderr, "why:") + assertContains(t, "fix", res.stderr, "skillrig search") +} + +// TestQuickstart_ShowNoOriginConfigured — with no resolvable origin, show fails +// exit 1 with the shared no-origin what/why/fix (run init first). +func TestQuickstart_ShowNoOriginConfigured(t *testing.T) { + t.Parallel() + + // A fresh temp cwd with NO SKILLRIG_ORIGIN and no project/global config. + res := run(t, runOpts{args: []string{"show", sampleSkill}}) + if res.exit != 1 { + t.Fatalf("show with no origin exit = %d, want 1 (stderr: %s)", res.exit, res.stderr) + } + + if res.stdout != "" { + t.Errorf("error path must keep stdout empty, got: %q", res.stdout) + } + + assertContains(t, "what", res.stderr, "no origin") + assertContains(t, "fix", res.stderr, "skillrig init") +} + +// TestQuickstart_ShowMissingArg — show with no skill argument is exit 1 with the +// navigational "requires exactly one argument" message, not cobra's bare arg error. +func TestQuickstart_ShowMissingArg(t *testing.T) { + t.Parallel() + + res := run(t, runOpts{args: []string{"show"}}) + if res.exit != 1 { + t.Fatalf("show with no arg exit = %d, want 1 (stderr: %s)", res.exit, res.stderr) + } + + assertContains(t, "what", res.stderr, "requires exactly one argument") + assertContains(t, "fix", res.stderr, "skillrig show ") +} From d97495b1139f9d06d07dc69d43fec2cc4de1cfff Mon Sep 17 00:00:00 2001 From: skillrig Date: Fri, 5 Jun 2026 09:32:53 +0000 Subject: [PATCH 2/4] fix(004): address PR #19 review on skillrig show MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the actionable Qodo findings on the show command: - getwd error now carries a fix: line. Extracted a single usageCannotGetwd helper (repo.go) and routed every command's os.Getwd failure through it (show/search/add/verify/index/init), so the what/why/fix cannot drift per command (AP-06 discipline). - "search failed" misattribution: generalized mapSearchError into mapCatalogError(cmd, origin, err); search passes "search", show passes "show", so an unclassified failure names the real command. - Terminal-injection hardening: catalog text is fetched and untrusted, so human output now strips control bytes / neutralizes ANSI escapes via sanitizeTerminal (single-line fields drop newlines; show's description body keeps them). Applied to both show and search; --json is never sanitized. Added unit coverage. - show unit test now asserts output SHAPE (exact header/field lines + footer-as-final-line), not only substrings. - cli.md: scoped the 80-char/newline truncation rule to compact LIST output and documented show as the deliberate full-detail exception (issue #17) — show intentionally prints the complete description. The full-description behavior (Qodo #2) and the runnable footer (Qodo #3) are intentional: show exists to print the untruncated description, and per the agentskills.io spec a skill `name` is a lowercase alphanumeric-hyphen slug (no leading/trailing hyphen, no spaces), so the footer command is always shell/cobra-safe for conformant origins. https://claude.ai/code/session_01SAczAuBfsd2hTEGrFBsmQZ --- docs/design/cli.md | 4 +- internal/cli/add.go | 2 +- internal/cli/index.go | 2 +- internal/cli/init.go | 2 +- internal/cli/output.go | 53 ++++++++++++++++--- internal/cli/repo.go | 14 +++++ internal/cli/search.go | 29 +++++----- internal/cli/show.go | 8 +-- internal/cli/show_test.go | 108 ++++++++++++++++++++++++++++++++++---- internal/cli/verify.go | 2 +- 10 files changed, 183 insertions(+), 41 deletions(-) diff --git a/docs/design/cli.md b/docs/design/cli.md index d58ba60..0d7ca90 100644 --- a/docs/design/cli.md +++ b/docs/design/cli.md @@ -223,11 +223,13 @@ terraform-cost | v0.9.0 | my-org | Estimate the cost delta of a plan. `search` is **query-first**: `search [QUERY...]` is a case-insensitive token-AND substring match over `name` + `description` + `topics`, with `--topic` as a separate exact-string filter (repeatable). The result order is deterministic (N6) — a fixed relevance bucket (exact-name > name > topic > description match) then lexicographic by name. There is **no** fuzzy / semantic / TF-IDF ranking. An empty result is a **clean exit 0** (not an error) with a footer hint. -**Truncation rules** (human output only): +**Truncation rules** (compact *list* human output — `search`, the `verify` failure list): - Description: first 80 chars, append `...` if truncated - Newlines replaced with spaces - `requires` shown as a comma-joined tool list, not the full constraint set +These rules keep a **list** scannable, where each row must stay one line so N items render in ~N lines. They do **not** apply to a **single-item detail view**: `skillrig show ` is the deliberate drill-down that prints ONE skill's *complete, untruncated* description (multi-line preserved) — that is its entire reason to exist (issue #17). The list→detail split is the same scan→inspect workflow `--json` serves; `show` is the human inspect step. A detail view still strips terminal control bytes from the (untrusted, fetched) catalog text and bounds the surrounding structure (a fixed header block + footer), but the description body itself is intentionally full. Control bytes / ANSI escapes are stripped from all human output (list and detail) since catalog content is fetched and not trusted for terminal rendering; `--json` is never sanitized. + **Footer hints**: Every compact output MUST end with a hint line suggesting the drill-down command. This is the agent's navigation cue. ### JSON Output (`--json`): Complete and Pipeable diff --git a/internal/cli/add.go b/internal/cli/add.go index d060d1e..98295f1 100644 --- a/internal/cli/add.go +++ b/internal/cli/add.go @@ -99,7 +99,7 @@ func newAddCmd(opts *globalOpts) *cobra.Command { func (ac *addCmd) run(cmd *cobra.Command) error { cwd, err := ac.getwd() if err != nil { - return &UsageError{Msg: "cannot determine working directory\nwhy: " + err.Error(), Cause: err} + return usageCannotGetwd(err) } res, err := config.ResolveOrigin(cwd, ac.env) diff --git a/internal/cli/index.go b/internal/cli/index.go index d96e4a5..801df76 100644 --- a/internal/cli/index.go +++ b/internal/cli/index.go @@ -83,7 +83,7 @@ func newIndexCmd(opts *globalOpts) *cobra.Command { func (ic *indexCmd) run(cmd *cobra.Command) error { cwd, err := ic.getwd() if err != nil { - return &UsageError{Msg: "cannot determine working directory\nwhy: " + err.Error(), Cause: err} + return usageCannotGetwd(err) } originRoot, err := gitToplevel(cmd.Context(), cwd) diff --git a/internal/cli/init.go b/internal/cli/init.go index c57280a..4a66baa 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -144,7 +144,7 @@ func (ic *initCmd) writeTarget(cmd *cobra.Command) (path, scope string, err erro cwd, err := ic.getwd() if err != nil { - return "", "", &UsageError{Msg: "cannot determine working directory\nwhy: " + err.Error(), Cause: err} + return "", "", usageCannotGetwd(err) } path, err = config.ProjectWriteTarget(cmd.Context(), cwd) diff --git a/internal/cli/output.go b/internal/cli/output.go index 47eb3ce..3e9ad0a 100644 --- a/internal/cli/output.go +++ b/internal/cli/output.go @@ -6,6 +6,7 @@ import ( "io" "strings" "text/tabwriter" + "unicode" "github.com/skillrig/cli/pkg/skillcore" ) @@ -162,7 +163,14 @@ func renderSearchResult(w io.Writer, origin string, matches []skillcore.CatalogE // before the footer so the summary line is not drawn into the columns. rows := make([][]string, 0, len(matches)) for _, e := range matches { - rows = append(rows, []string{e.Name, e.Version, "— " + truncateDesc(e.Description)}) + // Catalog text is untrusted (fetched per call): strip terminal control + // bytes from each cell. truncateDesc already collapses newlines, so the + // single-line form is requested here too. + rows = append(rows, []string{ + sanitizeTerminal(e.Name, false), + sanitizeTerminal(e.Version, false), + "— " + sanitizeTerminal(truncateDesc(e.Description), false), + }) } if err := writeAlignedColumns(&b, rows); err != nil { @@ -225,24 +233,30 @@ func renderShowResult(w io.Writer, origin string, e skillcore.CatalogEntry, json var b strings.Builder - fmt.Fprintf(&b, "%s %s (%s)\n", e.Name, e.Version, e.Namespace) - fmt.Fprintf(&b, "path: %s\n", e.Path) + // Catalog text is untrusted (fetched per call): strip terminal control bytes + // from every field. Single-line fields drop newlines too; the description body + // keeps newlines so a genuinely multi-line description still renders. + name := sanitizeTerminal(e.Name, false) + + fmt.Fprintf(&b, "%s %s (%s)\n", name, sanitizeTerminal(e.Version, false), sanitizeTerminal(e.Namespace, false)) + fmt.Fprintf(&b, "path: %s\n", sanitizeTerminal(e.Path, false)) if len(e.Topics) > 0 { - fmt.Fprintf(&b, "topics: %s\n", strings.Join(e.Topics, ", ")) + fmt.Fprintf(&b, "topics: %s\n", sanitizeTerminal(strings.Join(e.Topics, ", "), false)) } if len(e.Requires) > 0 { - fmt.Fprintf(&b, "requires: %s\n", joinRequires(e.Requires)) + fmt.Fprintf(&b, "requires: %s\n", sanitizeTerminal(joinRequires(e.Requires), false)) } // The whole point of show: the complete description, untruncated, set off by a - // blank line so it reads as the body of the record. - if desc := strings.TrimSpace(e.Description); desc != "" { + // blank line so it reads as the body of the record (control bytes stripped, + // newlines preserved). + if desc := strings.TrimSpace(sanitizeTerminal(e.Description, true)); desc != "" { fmt.Fprintf(&b, "\n%s\n", desc) } - fmt.Fprintf(&b, "\n%s%s\n", showFooterPrefix, e.Name) + fmt.Fprintf(&b, "\n%s%s\n", showFooterPrefix, name) _, err := io.WriteString(w, b.String()) @@ -286,6 +300,29 @@ func truncateDesc(s string) string { return string(r[:searchDescWidth-1]) + "…" } +// sanitizeTerminal strips control characters from catalog-controlled text before +// it is printed to a human terminal. An origin's index.json is fetched and is NOT +// trusted to be free of ANSI escape sequences or other control bytes that could +// spoof the terminal or inject crafted log lines (PR #19 review). Every control +// rune is dropped — including the ESC that introduces an ANSI/CSI sequence, so +// the sequence is neutralized and only its harmless printable remainder (e.g. +// "[31m") survives — except newlines, which are kept when keepNewlines is set so +// show can still render a genuinely multi-line description body. --json output is +// NEVER routed through this: it must carry the exact bytes for an agent/jq. +func sanitizeTerminal(s string, keepNewlines bool) string { + return strings.Map(func(r rune) rune { + if keepNewlines && r == '\n' { + return r + } + + if unicode.IsControl(r) { + return -1 + } + + return r + }, s) +} + // indexResult is the presentation-layer view of an index generation: where the // catalog was written, how many skills it carries, and the convention it // declares. It is the single struct both renderers consume. diff --git a/internal/cli/repo.go b/internal/cli/repo.go index 92dbdbb..c764f69 100644 --- a/internal/cli/repo.go +++ b/internal/cli/repo.go @@ -50,6 +50,20 @@ func gitToplevel(ctx context.Context, cwd string) (string, error) { return root, nil } +// usageCannotGetwd builds the shared "cannot determine working directory" usage +// error (exit 1) every command returns when os.Getwd fails. It is errors-as- +// navigation like the rest: the what, the real cause (why), and an actionable +// fix — a single implementation so the fix line cannot drift per command. The +// raw cause is preserved for --verbose. +func usageCannotGetwd(cause error) *UsageError { + return &UsageError{ + Msg: "cannot determine working directory\n" + + "why: " + cause.Error() + "\n" + + "fix: re-run from an existing, readable directory (the shell's current directory may have been removed or be unreadable)", + Cause: cause, + } +} + // usageNotGitRepo builds the project-scope "not a git repository" usage error // (exit 1) shared by add and verify. why states the command-specific rationale; // the raw cause is preserved for --verbose. diff --git a/internal/cli/search.go b/internal/cli/search.go index 3707ea0..2ec92c3 100644 --- a/internal/cli/search.go +++ b/internal/cli/search.go @@ -84,7 +84,7 @@ func newSearchCmd(opts *globalOpts) *cobra.Command { func (sc *searchCmd) run(cmd *cobra.Command) error { cwd, err := sc.getwd() if err != nil { - return &UsageError{Msg: "cannot determine working directory\nwhy: " + err.Error(), Cause: err} + return usageCannotGetwd(err) } res, err := config.ResolveOrigin(cwd, sc.env) @@ -106,7 +106,7 @@ func (sc *searchCmd) run(cmd *cobra.Command) error { if !errors.Is(err, errNotGitRepo) { // An unexpected failure (e.g. context cancellation) is not a "not a // repo" precondition — surface it rather than silently proceed. - return mapSearchError(res.Origin.String(), err) + return mapCatalogError("search", res.Origin.String(), err) } repoRoot = "" @@ -114,11 +114,11 @@ func (sc *searchCmd) run(cmd *cobra.Command) error { catalog, err := loadCatalog(cmd.Context(), repoRoot, res.Origin) if err != nil { - return mapSearchError(res.Origin.String(), err) + return mapCatalogError("search", res.Origin.String(), err) } if err := skillcore.CheckConvention(catalog.SkillrigConvention); err != nil { - return mapSearchError(res.Origin.String(), err) + return mapCatalogError("search", res.Origin.String(), err) } matches := skillcore.Search(catalog, sc.query, sc.topics) @@ -192,7 +192,7 @@ func localCatalogPath(repoRoot string, origin config.Origin) (string, bool) { } // readCatalogFile reads and parses a local index.json, tagging the read and parse -// failures distinctly so mapSearchError can author the right what/why/fix. +// failures distinctly so mapCatalogError can author the right what/why/fix. func readCatalogFile(catalogPath string) (skillcore.Catalog, error) { //nolint:gosec // G304: path is the resolved origin path/checkout + a fixed file name, not attacker-controlled. data, err := os.ReadFile(catalogPath) @@ -209,7 +209,7 @@ func readCatalogFile(catalogPath string) (skillcore.Catalog, error) { } // catalogReadError marks the origin's catalog as unreadable (absent or no -// permission). It is presentation-free here only in that mapSearchError renders +// permission). It is presentation-free here only in that mapCatalogError renders // the what/why/fix; it carries the path and raw cause for --verbose. type catalogReadError struct { path string @@ -232,12 +232,15 @@ func (e *catalogParseError) Error() string { } func (e *catalogParseError) Unwrap() error { return e.cause } -// mapSearchError maps the failure classes search can surface to navigational -// *UsageError values (exit 1), authoring the what/why/fix prose while preserving -// the raw cause for --verbose. The convention mismatch, an unreachable/auth -// origin, and a malformed catalog are distinct messages so the agent debugs the -// real problem (errors-as-navigation; do not conflate look-alike classes). -func mapSearchError(origin string, err error) error { +// mapCatalogError maps the failure classes a catalog-reading Query command +// (search, show) can surface to navigational *UsageError values (exit 1), +// authoring the what/why/fix prose while preserving the raw cause for --verbose. +// The convention mismatch, an unreachable/auth origin, and a malformed catalog +// are distinct messages so the agent debugs the real problem (errors-as- +// navigation; do not conflate look-alike classes). cmd names the invoking command +// so the generic fallback reads " failed" instead of hardcoding one +// subcommand for both callers. +func mapCatalogError(cmd, origin string, err error) error { var convErr *skillcore.IncompatibleConventionError if errors.As(err, &convErr) { return mapConventionError(origin, convErr, err) @@ -290,5 +293,5 @@ func mapSearchError(origin string, err error) error { return usageErr } - return &UsageError{Msg: "search failed\nwhy: " + err.Error(), Cause: err} + return &UsageError{Msg: cmd + " failed\nwhy: " + err.Error(), Cause: err} } diff --git a/internal/cli/show.go b/internal/cli/show.go index ef1403e..cf13fa6 100644 --- a/internal/cli/show.go +++ b/internal/cli/show.go @@ -81,7 +81,7 @@ func newShowCmd(opts *globalOpts) *cobra.Command { func (sc *showCmd) run(cmd *cobra.Command) error { cwd, err := sc.getwd() if err != nil { - return &UsageError{Msg: "cannot determine working directory\nwhy: " + err.Error(), Cause: err} + return usageCannotGetwd(err) } res, err := config.ResolveOrigin(cwd, sc.env) @@ -99,7 +99,7 @@ func (sc *showCmd) run(cmd *cobra.Command) error { repoRoot, err := gitToplevel(cmd.Context(), cwd) if err != nil { if !errors.Is(err, errNotGitRepo) { - return mapSearchError(res.Origin.String(), err) + return mapCatalogError("show", res.Origin.String(), err) } repoRoot = "" @@ -107,11 +107,11 @@ func (sc *showCmd) run(cmd *cobra.Command) error { catalog, err := loadCatalog(cmd.Context(), repoRoot, res.Origin) if err != nil { - return mapSearchError(res.Origin.String(), err) + return mapCatalogError("show", res.Origin.String(), err) } if err := skillcore.CheckConvention(catalog.SkillrigConvention); err != nil { - return mapSearchError(res.Origin.String(), err) + return mapCatalogError("show", res.Origin.String(), err) } entry, ok := skillcore.FindSkill(catalog, sc.skill) diff --git a/internal/cli/show_test.go b/internal/cli/show_test.go index ab8fc37..e4d5aa2 100644 --- a/internal/cli/show_test.go +++ b/internal/cli/show_test.go @@ -43,30 +43,116 @@ func TestRenderShowResult_Human(t *testing.T) { out := buf.String() - // Full description, both lines, untruncated. + // Output SHAPE (not just substrings): a fixed header block — header line, then + // the path/topics/requires field lines in order — a blank-line-separated + // description body, and the footer as the final line. Asserting the exact lines + // and their order catches a layout regression a Contains check would miss. + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + + wantHead := []string{ + "terraform-plan-review 1.4.0 (my-org)", + "path: skills/terraform-plan-review", + "topics: platform-team, terraform, aws", + "requires: oxid (>=0.4.0), terraform", // version parenthesised; bare tool stands alone + } + for i, want := range wantHead { + if i >= len(lines) || lines[i] != want { + t.Fatalf("header line %d = %q, want %q\nfull output:\n%s", i, lineAt(lines, i), want, out) + } + } + + // The footer is the final line and is the runnable next-step command. + if got := lines[len(lines)-1]; got != "→ vendor it: skillrig add terraform-plan-review" { + t.Errorf("final line = %q, want the add footer hint\nfull output:\n%s", got, out) + } + + // Exactly one blank line precedes the footer (footer is set off from the body). + if n := len(lines); n < 2 || lines[n-2] != "" { + t.Errorf("footer is not preceded by a blank line:\n%s", out) + } + + // The full description body appears verbatim, both paragraphs, untruncated — + // the issue #17 contract. for _, want := range []string{ "Review a terraform plan for risk before apply.", "It is read-only and never mutates the plan.", - "terraform-plan-review 1.4.0 (my-org)", - "skills/terraform-plan-review", - "platform-team, terraform, aws", } { if !strings.Contains(out, want) { - t.Errorf("human output missing %q:\n%s", want, out) + t.Errorf("human output missing description line %q:\n%s", want, out) } } +} - // requires: a version constraint is parenthesised; a bare tool stands alone. - if !strings.Contains(out, "requires: oxid (>=0.4.0), terraform") { - t.Errorf("requires summary wrong, want 'oxid (>=0.4.0), terraform':\n%s", out) +// TestSanitizeTerminal asserts the untrusted-catalog guard: control bytes and +// the ESC that introduces an ANSI sequence are dropped (neutralizing the escape, +// leaving only its harmless printable remainder), while newlines are kept only +// when requested. --json is never routed through this, so it is not covered here. +func TestSanitizeTerminal(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + keepNewlines bool + want string + }{ + {name: "plain text unchanged", in: "terraform-plan-review", want: "terraform-plan-review"}, + {name: "em dash and ellipsis kept (not control)", in: "review — a plan…", want: "review — a plan…"}, + { + name: "ANSI escape neutralized (ESC dropped, remainder harmless)", + in: "\x1b[31mred\x1b[0m", + want: "[31mred[0m", + }, + {name: "carriage return and bell dropped", in: "a\rb\x07c", want: "abc"}, + {name: "newline dropped for single-line field", in: "a\nb", keepNewlines: false, want: "ab"}, + {name: "newline kept for body", in: "a\nb", keepNewlines: true, want: "a\nb"}, } - // Footer hint is the runnable add command. - if !strings.Contains(out, "→ vendor it: skillrig add terraform-plan-review") { - t.Errorf("missing runnable footer hint:\n%s", out) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := sanitizeTerminal(tt.in, tt.keepNewlines); got != tt.want { + t.Errorf("sanitizeTerminal(%q, %v) = %q, want %q", tt.in, tt.keepNewlines, got, tt.want) + } + }) } } +// TestRenderShowResult_StripsControlBytes proves a malicious catalog entry cannot +// inject terminal control sequences through show's human output: an ESC-laden +// name/description is rendered with the ESC bytes removed. +func TestRenderShowResult_StripsControlBytes(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + e := skillcore.CatalogEntry{ + Name: "evil\x1b[2J", + Version: "1.0.0", + Namespace: "x", + Description: "line1\x1b[31m\nline2", + Path: "skills/evil", + } + if err := renderShowResult(&buf, "o/r", e, false); err != nil { + t.Fatalf("renderShowResult: %v", err) + } + + if strings.ContainsRune(buf.String(), '\x1b') { + t.Errorf("human output contains a raw ESC byte (terminal injection):\n%q", buf.String()) + } +} + +// lineAt returns lines[i] or "" when i is out of range, for clearer +// shape-assertion failure messages. +func lineAt(lines []string, i int) string { + if i < 0 || i >= len(lines) { + return "" + } + + return lines[i] +} + // TestRenderShowResult_JSONComplete asserts --json is parseable and structurally // complete: origin + a skill object with every field, the description untruncated. func TestRenderShowResult_JSONComplete(t *testing.T) { diff --git a/internal/cli/verify.go b/internal/cli/verify.go index 20d03a0..0e23f75 100644 --- a/internal/cli/verify.go +++ b/internal/cli/verify.go @@ -64,7 +64,7 @@ func newVerifyCmd(opts *globalOpts) *cobra.Command { func (vc *verifyCmd) run(cmd *cobra.Command) error { cwd, err := vc.getwd() if err != nil { - return &UsageError{Msg: "cannot determine working directory\nwhy: " + err.Error(), Cause: err} + return usageCannotGetwd(err) } repoRoot, err := gitToplevel(cmd.Context(), cwd) From 5408da677323db6fdb688b1e3b6e661e1eda9452 Mon Sep 17 00:00:00 2001 From: skillrig Date: Fri, 5 Jun 2026 09:46:17 +0000 Subject: [PATCH 3/4] chore: renumber spec 004-show-skill -> 005-show-skill The 004 slot is taken by the 004-doctor branch (skillrig doctor). Move this feature's quickstart contract to specledger/005-show-skill/ and update the integration-suite references to match. No behavior change. https://claude.ai/code/session_01SAczAuBfsd2hTEGrFBsmQZ --- specledger/{004-show-skill => 005-show-skill}/quickstart.md | 2 +- test/show_quickstart_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) rename specledger/{004-show-skill => 005-show-skill}/quickstart.md (98%) diff --git a/specledger/004-show-skill/quickstart.md b/specledger/005-show-skill/quickstart.md similarity index 98% rename from specledger/004-show-skill/quickstart.md rename to specledger/005-show-skill/quickstart.md index 9687ba8..501c7a6 100644 --- a/specledger/004-show-skill/quickstart.md +++ b/specledger/005-show-skill/quickstart.md @@ -1,4 +1,4 @@ -# Quickstart — Acceptance Contract: `004-show-skill` +# Quickstart — Acceptance Contract: `005-show-skill` Each scenario is an executable `TestQuickstart_*` (Constitution §II): concrete invocations, observable output, exit codes, and **output-shape** assertions (full untruncated human body; parseable + complete `--json`; 3-part errors). Resolves [issue #17](https://github.com/skillrig/cli/issues/17): humans had no way to read a skill's *full* description — `search` truncates it to ~80 chars and only an agent could recover it via `--json | jq`. diff --git a/test/show_quickstart_test.go b/test/show_quickstart_test.go index c68d5e2..6994b26 100644 --- a/test/show_quickstart_test.go +++ b/test/show_quickstart_test.go @@ -1,6 +1,6 @@ // This file holds the TestQuickstart_* integration suite for feature -// 004-show-skill (the `skillrig show`/`info` command, issue #17). Each scenario -// maps 1:1 to a scenario in specledger/004-show-skill/quickstart.md. +// 005-show-skill (the `skillrig show`/`info` command, issue #17). Each scenario +// maps 1:1 to a scenario in specledger/005-show-skill/quickstart.md. // // Like the 001/002/003 suites it builds the binary once (TestMain in // quickstart_test.go) and execs it via run(). show is a Query command that reads From eb58ba047cbc2c520dfa394327f1a5f1a89877d4 Mon Sep 17 00:00:00 2001 From: skillrig Date: Fri, 5 Jun 2026 11:01:54 +0000 Subject: [PATCH 4/4] fix(005): shell-safe show footer + true-auth e2e coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the Hermes/sakul-learning review on PR #19: - Footer runnability (also Qodo #3): the `skillrig add ` hint now routes the name through shellSafeSkillArg — a spec-conformant agentskills.io slug is emitted bare, while a non-conformant name (leading dash, spaces, metacharacters that only a malformed origin could publish) is single-quoted and `--`-guarded so the printed command stays runnable in a shell and under cobra. Unit-tested. - True e2e coverage: TestE2E_FullWorkflow now runs init -> search -> show -> add over the real auth-gated git http-backend remote, for both the default branch and @staging. The show step asserts exit 0 / empty stderr, a parseable {origin, skill}, that origin preserves the configured reference (incl. @staging), the point-lookup name, and populated record fields. The @staging case targets the branch-only `staging-only` skill, proving show reads the @ref'd catalog. Verified green with `make test-e2e` (real git-http-backend + token gate). The full-description behavior remains intentional (issue #17). Enforcing the name slug at ingestion (index/lint) is still the right system-wide invariant and is left as a follow-up. https://claude.ai/code/session_01SAczAuBfsd2hTEGrFBsmQZ --- internal/cli/output.go | 43 ++++++++++++++++++- internal/cli/show_test.go | 57 +++++++++++++++++++++++++ test/e2e/auth_e2e_test.go | 89 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 184 insertions(+), 5 deletions(-) diff --git a/internal/cli/output.go b/internal/cli/output.go index 3e9ad0a..d1d9fcf 100644 --- a/internal/cli/output.go +++ b/internal/cli/output.go @@ -256,13 +256,54 @@ func renderShowResult(w io.Writer, origin string, e skillcore.CatalogEntry, json fmt.Fprintf(&b, "\n%s\n", desc) } - fmt.Fprintf(&b, "\n%s%s\n", showFooterPrefix, name) + fmt.Fprintf(&b, "\n%s%s\n", showFooterPrefix, shellSafeSkillArg(name)) _, err := io.WriteString(w, b.String()) return err } +// shellSafeSkillArg renders a skill name as a copy-pasteable argument for the +// `skillrig add` footer hint so the printed command is genuinely runnable (PR #19 +// review). A spec-conformant agentskills.io name (lowercase alphanumerics + +// hyphens) is a clean token and is emitted bare. A name that is NOT a clean token +// — which only a non-conformant origin could publish — is single-quoted for the +// shell and prefixed with cobra's `--` end-of-options marker, so a leading dash +// is not parsed as a flag and embedded spaces are not word-split. (Control bytes +// were already stripped by sanitizeTerminal before this point.) +func shellSafeSkillArg(name string) string { + if isPlainSkillToken(name) { + return name + } + + return "-- " + shellSingleQuote(name) +} + +// isPlainSkillToken reports whether name is a clean, copy-pasteable shell/cobra +// token: non-empty, not starting with '-', and built only from unreserved +// characters [A-Za-z0-9._-]. Such a name needs neither quoting nor a `--` guard. +func isPlainSkillToken(name string) bool { + if name == "" || name[0] == '-' { + return false + } + + for _, r := range name { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-', r == '_', r == '.': + default: + return false + } + } + + return true +} + +// shellSingleQuote wraps s in POSIX single quotes, escaping any embedded single +// quote, so it is a single shell word regardless of spaces or metacharacters. +func shellSingleQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + // joinRequires renders a skill's backing-tool requirements as a compact human // summary — "tool (version)" joined by commas, or a bare tool when no version // constraint is recorded — mirroring search's requires summary. diff --git a/internal/cli/show_test.go b/internal/cli/show_test.go index e4d5aa2..f700460 100644 --- a/internal/cli/show_test.go +++ b/internal/cli/show_test.go @@ -143,6 +143,63 @@ func TestRenderShowResult_StripsControlBytes(t *testing.T) { } } +// TestShellSafeSkillArg pins the runnable-footer contract (PR #19): a +// spec-conformant name is emitted bare, while a name that would break a copied +// command — leading dash (cobra flag), spaces (shell split), or an embedded quote +// — is `--`-guarded and single-quoted so the printed `skillrig add ...` stays +// runnable. +func TestShellSafeSkillArg(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want string + }{ + {name: "plain slug emitted bare", in: "terraform-plan-review", want: "terraform-plan-review"}, + {name: "unreserved chars stay bare", in: "Foo_bar.baz-1", want: "Foo_bar.baz-1"}, + {name: "leading dash guarded and quoted", in: "-rf", want: "-- '-rf'"}, + {name: "spaces quoted", in: "a b", want: "-- 'a b'"}, + {name: "embedded single quote escaped", in: "a'b", want: `-- 'a'\''b'`}, + {name: "shell metacharacter quoted", in: "a;b", want: "-- 'a;b'"}, + {name: "empty quoted", in: "", want: "-- ''"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := shellSafeSkillArg(tt.in); got != tt.want { + t.Errorf("shellSafeSkillArg(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +// TestRenderShowResult_FooterRunnableForHostileName proves a non-conformant +// catalog name cannot produce a non-runnable footer: a leading-dash, space-laden +// name is rendered as a `--`-guarded, single-quoted `skillrig add` argument. +func TestRenderShowResult_FooterRunnableForHostileName(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + e := skillcore.CatalogEntry{ + Name: "-rm rf", + Version: "1.0.0", + Namespace: "x", + Description: "d", + Path: "skills/x", + } + if err := renderShowResult(&buf, "o/r", e, false); err != nil { + t.Fatalf("renderShowResult: %v", err) + } + + if !strings.Contains(buf.String(), "skillrig add -- '-rm rf'") { + t.Errorf("footer is not a runnable (guarded + quoted) command:\n%s", buf.String()) + } +} + // lineAt returns lines[i] or "" when i is out of range, for clearer // shape-assertion failure messages. func lineAt(lines []string, i int) string { diff --git a/test/e2e/auth_e2e_test.go b/test/e2e/auth_e2e_test.go index 2eb0417..73f29c2 100644 --- a/test/e2e/auth_e2e_test.go +++ b/test/e2e/auth_e2e_test.go @@ -622,13 +622,85 @@ func searchSkillNames(t *testing.T, stdout string) []string { return names } +// showRecord mirrors the `skillrig show --json` payload: the resolved origin and +// the single point-lookup skill's record (the fields the workflow asserts). +type showRecord struct { + Origin string `json:"origin"` + Skill struct { + Name string `json:"name"` + Version string `json:"version"` + Namespace string `json:"namespace"` + Description string `json:"description"` + Path string `json:"path"` + } `json:"skill"` +} + +// decodeShowRecord strictly decodes `show --json` stdout into a showRecord. +func decodeShowRecord(t *testing.T, stdout string) showRecord { + t.Helper() + + var rec showRecord + if err := json.Unmarshal([]byte(stdout), &rec); err != nil { + t.Fatalf("show --json is not parseable: %v\n%s", err, stdout) + } + + return rec +} + +// runShowJSON runs `skillrig show --json` in the consumer with the +// given env, asserts a clean success (exit 0, empty stderr), and returns the +// decoded record. +func runShowJSON(t *testing.T, consumer string, env map[string]string, target string) showRecord { + t.Helper() + + shr := runSkillrig(t, []string{"show", target, "--json"}, consumer, env) + if shr.exit != 0 { + t.Fatalf("show %s exit = %d (stderr: %s)", target, shr.exit, shr.stderr) + } + + if shr.stderr != "" { + t.Errorf("show %s wrote to stderr on success: %q", target, shr.stderr) + } + + return decodeShowRecord(t, shr.stdout) +} + +// assertShowRecord checks a `show --json` record: the origin preserves the +// configured reference (including any @branch), the skill is the requested +// point-lookup target, and the core record fields are populated. +func assertShowRecord(t *testing.T, rec showRecord, wantOrigin, wantName string) { + t.Helper() + + if rec.Origin != wantOrigin { + t.Errorf("show origin = %q, want the configured origin %q (incl. any @branch)", rec.Origin, wantOrigin) + } + + if rec.Skill.Name != wantName { + t.Errorf("show skill.name = %q, want the requested target %q", rec.Skill.Name, wantName) + } + + for field, val := range map[string]string{ + "version": rec.Skill.Version, + "namespace": rec.Skill.Namespace, + "description": rec.Skill.Description, + "path": rec.Skill.Path, + } { + if strings.TrimSpace(val) == "" { + t.Errorf("show %s: skill.%s is empty, want a populated record field", wantName, field) + } + } +} + // TestE2E_FullWorkflow drives the complete consumer loop against the real // auth-gated server: `skillrig init --origin` (BOTH the default-branch and the // @staging forms, with auth), then `search` (asserting >1 result, exactly the -// branch's catalog), then `add` of the first and second skill (asserting each is -// vendored and locked). The @staging case is the end-to-end proof of the -// non-default-branch catalog fix — its search returns the staging-only skill that -// only exists on that branch, which a pre-fix build could not fetch at all. +// branch's catalog), then `show` of one skill (asserting the full point-lookup +// record over the same authenticated remote), then `add` of the first and second +// skill (asserting each is vendored and locked). The @staging case is the +// end-to-end proof of the non-default-branch catalog fix — its search returns the +// staging-only skill that only exists on that branch, which a pre-fix build could +// not fetch at all, and its `show staging-only` proves the detail view reads the +// @ref'd catalog too. func TestE2E_FullWorkflow(t *testing.T) { projectRoot := newMultiSkillOrigin(t) srv := newGitAuthServer(t, projectRoot, validToken) @@ -688,6 +760,15 @@ func TestE2E_FullWorkflow(t *testing.T) { t.Errorf("search names = %v, want %v (wrong branch's catalog)", got, tc.wantSkills) } + // show (authenticated) — point-lookup ONE skill's full record from the + // SAME resolved origin/config/env, over the real auth-gated remote. The + // target is the first skill this branch adds, so for @staging it is the + // branch-only `staging-only` — a success additionally proves show reads + // the @ref'd catalog, not just the default branch. The invocation and + // assertions live in helpers to keep this workflow readable. + showTarget := tc.add[0] + assertShowRecord(t, runShowJSON(t, consumer, env, showTarget), tc.originArg, showTarget) + // add the first and second skill — each vendored byte-present + locked. for i, skill := range tc.add { if r := runSkillrig(t, []string{"add", skill}, consumer, env); r.exit != 0 {