feat(005): add skillrig show for a human-friendly full skill view

[so0k]

Adds skillrig show <skill> (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

[qodo-code-review]

Review Summary by Qodo

Add skillrig show command for complete skill details with untruncated descriptions

✨ Enhancement

Walkthroughs

Description

• Adds skillrig show  command with info alias for reading complete skill details
• Implements exact-name lookup primitive FindSkill in skillcore for point-lookup matching
• Renders full untruncated descriptions in human-friendly format and JSON output
• Closes issue #17 by exposing complete skill records that search truncates to ~80 chars
• Extends CLI documentation, skill routing, and integration test suite with show-specific scenarios

Diagram

flowchart LR
  A["User invokes<br/>skillrig show SKILL"] --> B["Resolve origin<br/>via config/env"]
  B --> C["Load catalog<br/>index.json"]
  C --> D["FindSkill exact<br/>name lookup"]
  D --> E["Render full record"]
  E --> F["Human output<br/>with complete desc"]
  E --> G["JSON output<br/>origin + skill"]
  H["Unknown skill"] --> I["Exit 1 with<br/>what/why/fix"]

Loading

File Changes

1. internal/cli/show.go ✨ Enhancement +142/-0

New show command implementation with resolver and catalog loading

internal/cli/show.go

---

2. internal/cli/show_test.go 🧪 Tests +135/-0

Unit tests for show command rendering and error handling

internal/cli/show_test.go

---

3. internal/cli/output.go ✨ Enhancement +72/-0

Add renderShowResult and joinRequires output formatting functions

internal/cli/output.go

---

View more (9)

4. internal/cli/root.go ✨ Enhancement +1/-0

Register newShowCmd in subcommand routing

internal/cli/root.go

---

5. pkg/skillcore/catalog.go ✨ Enhancement +17/-0

Add FindSkill exact-name lookup primitive for point queries

pkg/skillcore/catalog.go

---

6. pkg/skillcore/catalog_test.go 🧪 Tests +54/-0

Test FindSkill exact matching behavior and edge cases

pkg/skillcore/catalog_test.go

---

7. test/show_quickstart_test.go 🧪 Tests +251/-0

Integration test suite for show command with full/alias/JSON/error scenarios

test/show_quickstart_test.go

---

8. .agents/skills/skillrig/SKILL.md 📝 Documentation +9/-6

Update skill description and routing table to include show command

.agents/skills/skillrig/SKILL.md

---

9. .agents/skills/skillrig/references/show.md 📝 Documentation +66/-0

New reference documentation for show command with usage and error handling

.agents/skills/skillrig/references/show.md

---

10. README.md 📝 Documentation +19/-0

Add show command section with examples and behavior description

README.md

---

11. docs/design/cli.md 📝 Documentation +3/-2

Update Query pattern classification and failure modes to include show

docs/design/cli.md

---

12. specledger/004-show-skill/quickstart.md 📝 Documentation +32/-0

Acceptance contract with test scenarios mapping to issue #17 resolution

specledger/004-show-skill/quickstart.md

---

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 Feature: Add support for skillrig show or skillrig info for a human friendly view of an origin skill description

✅ Compliance rules (platform): 152 rules

1. show getwd error lacks fix 📘 Rule violation ⚙ Maintainability

Description

The new show command returns a UsageError for working-directory failures without any remediation
hint (no fix: line), violating the requirement that new error messages include actionable next
steps. This can leave users stuck when os.Getwd fails (e.g., deleted CWD or permission issues).

Code

internal/cli/show.go[R82-85]

Evidence

PR Compliance ID 783436 requires newly added/modified error messages to include what failed, the
cause, and a remediation hint. In internal/cli/show.go, the new error message for a failing
getwd() path includes cannot determine working directory and why: <err>, but provides no
fix: next step.

Rule 783436: Error messages must include failure context, root cause, and remediation hint
internal/cli/show.go[82-85]

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 `show` command’s `cannot determine working directory` error includes `what` and `why` but no actionable `fix:` guidance, which violates the error-message compliance requirement.

## Issue Context
`internal/cli/show.go` returns `&UsageError{Msg: "cannot determine working directory\nwhy: ..."}` when `getwd()` fails.

## Fix Focus Areas
- internal/cli/show.go[82-85]

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

---

2. renderShowResult prints full description 📘 Rule violation ⚙ Maintainability

Description

The new human output path for show prints the full multi-line description without
truncation/newline collapsing. This conflicts with the compliance requirement that human output
descriptions be truncated to 80 characters and have newlines replaced with spaces.

Code

internal/cli/output.go[R211-215]

Evidence

PR Compliance ID 783450 requires human (non-JSON) output to truncate descriptions to 80 characters
and replace newlines with spaces. The new renderShowResult human path explicitly prints the full
trimmed description body as-is, including embedded newlines, with no truncation logic.

Rule 783450: Human output must truncate descriptions to 80 characters and replace newlines with spaces
internal/cli/output.go[211-215]

Agent prompt

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

## Issue description
Human output in `renderShowResult` prints `e.Description` verbatim (including newlines and unbounded length), but the checklist requires human descriptions to be truncated to 80 chars and newlines replaced with spaces.

## Issue Context
`renderShowResult` currently writes `desc := strings.TrimSpace(e.Description)` and then `fmt.Fprintf(&b, "\n%s\n", desc)`.

## Fix Focus Areas
- internal/cli/output.go[211-215]

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

---

3. Footer hint breaks for names 🐞 Bug ≡ Correctness

Description

renderShowResult prints a supposedly runnable skillrig add <name> footer using the raw catalog
skill name, but valid skill names can start with - or include spaces, making the suggested command
fail (flag parsing / shell splitting). This violates the stated contract that the footer is a
runnable next-step command.

Code

internal/cli/output.go[R179-218]

Evidence

The footer is constructed by concatenating a prefix with the skill name, but skill names are only
constrained to be a safe path segment (no / or \) and therefore can still start with - or
contain spaces—both of which break the printed command when copied into a shell/cobra CLI
invocation.

internal/cli/output.go[179-218]
pkg/skillcore/add.go[381-392]

Agent prompt

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

### Issue description
`renderShowResult` claims its footer is a runnable command, but it appends the raw skill name directly after `skillrig add `. Names are only validated as “single path segment” and may legally begin with `-` or contain spaces; in those cases the printed command is not runnable (cobra interprets `-foo` as a flag; shells split `foo bar`).

### Issue Context
- `renderShowResult` builds the footer via `showFooterPrefix + e.Name`.
- `validateSkillName` in skillcore does **not** reject leading `-` or spaces.

### Fix Focus Areas
- internal/cli/output.go[179-218]

### Implementation notes
- At minimum, make the command robust for leading-dash names by emitting `skillrig add -- <name>`.
- For full “runnable” behavior, add simple shell-quoting for names containing whitespace or other special chars (or tighten/align skill-name validation rules across the system if that’s the intended invariant).

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

---

4. Show tests only use Contains 📘 Rule violation ▣ Testability

Description

The newly added show unit tests validate human output primarily via substring checks without
asserting output shape (e.g., bounded/exact line count or structured layout). This reduces test
robustness and violates the output-shape testing requirement.

Code

internal/cli/show_test.go[R44-67]

Evidence

PR Compliance ID 783440 requires tests of human-readable text output to validate output shape (line
count or structural constraints), and flags tests that only use substring checks.
TestRenderShowResult_Human currently asserts presence of substrings but does not constrain the
output structure (e.g., expected lines/ordering).

Rule 783440: Tests must validate output shape, not just substring matches
internal/cli/show_test.go[44-67]

Agent prompt

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

## Issue description
Tests for human-readable output should assert output structure/shape (e.g., header + field lines + optional blank line + footer) and not rely only on `strings.Contains`.

## Issue Context
`TestRenderShowResult_Human` currently iterates over `want` substrings and checks `strings.Contains(out, want)`.

## Fix Focus Areas
- internal/cli/show_test.go[44-67]

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

---

5. Show may say “search failed” 🐞 Bug ⚙ Maintainability

Description

show reuses mapSearchError, whose fallback message hardcodes search failed, so skillrig show
can emit misleading command-specific diagnostics on unexpected/unclassified errors (notably from
gitToplevel). This makes debugging harder because the error claims a different subcommand failed.

Code

internal/cli/show.go[R96-115]

Evidence

show routes errors through mapSearchError, and mapSearchError’s final fallback string includes
the literal text search failed, which is incorrect when invoked from show.

internal/cli/show.go[96-115]
internal/cli/search.go[235-289]

Agent prompt

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

### Issue description
`internal/cli/show.go` uses `mapSearchError(...)` for some non-search-specific error paths. While most typed errors are mapped to good messages, the fallback path in `mapSearchError` returns a `UsageError` whose message starts with `search failed`, which can surface when `show` hits an unexpected/unclassified error (e.g., context cancellation or other unexpected failure in `gitToplevel`).

### Issue Context
- `show` calls `mapSearchError` directly.
- `mapSearchError` fallback hardcodes the subcommand name.

### Fix Focus Areas
- internal/cli/show.go[96-115]
- internal/cli/search.go[235-289]

### Implementation notes
Options:
1) Refactor `mapSearchError` into a generic `mapQueryError(cmdName, origin, err)` with a parameterized fallback prefix.
2) Keep `mapSearchError` for shared catalog-fetch typed errors, but avoid it for `gitToplevel` unexpected errors in `show` (return a `UsageError` with `show failed` there).

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

---

6. Unescaped terminal control output 🐞 Bug ⛨ Security

Description

The human output path for show prints catalog-controlled strings (including the full description)
directly to stdout without stripping ANSI escape sequences/control characters, enabling terminal
spoofing/log injection if the origin catalog is malicious or compromised. This risk exists already
in search, but show adds a new “print full body” path that increases the blast radius.

Code

internal/cli/output.go[R190-216]

Evidence

The show renderer prints the full description and other fields directly, and catalog content can
come from fetched origins; therefore these strings should be treated as untrusted for terminal
output.

internal/cli/output.go[190-216]
internal/cli/search.go[129-159]
internal/cli/output.go[138-167]

Agent prompt

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

### Issue description
`renderShowResult` prints untrusted catalog fields (e.g., `Name`, `Path`, `Description`) verbatim. If an origin’s `index.json` contains terminal control sequences (ANSI escapes, carriage returns), it can manipulate terminal output (spoofing) or pollute logs.

### Issue Context
- Origins can be remote and are fetched per-call; catalog content is not guaranteed to be safe for terminal rendering.
- This is not unique to `show` (search prints untrusted data too), but `show` newly prints the entire multi-line description.

### Fix Focus Areas
- internal/cli/output.go[190-216]
- internal/cli/output.go[138-167]

### Implementation notes
- Add a small `sanitizeForTerminal(string) string` helper for human output that strips control characters and ANSI escape sequences (while preserving `\n` where desired for `show`).
- Keep `--json` output raw (no sanitization) to preserve exact data.

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

---

[sakul-learning]

Hermes review + e2e test recommendation

I reviewed PR #19 (skillrig show / info) and agree this feature does warrant true e2e coverage, not only quickstart/integration tests.

Suggested e2e shape

Add coverage to the existing true-auth workflow in test/e2e/auth_e2e_test.go:

1. Stand up the real auth-gated git http-backend origin fixture.
2. Run skillrig init --origin ... in a real consumer repo so origin resolution comes from project config, not only SKILLRIG_ORIGIN.
3. Run skillrig search --json first to prove the catalog was fetched/authenticated and the expected branch catalog is selected.
4. Run skillrig show <skill> --json against the same consumer/config/env.
5. Assert:
   • exit 0 and empty stderr
   • JSON parses as {origin, skill}
   • origin preserves the configured origin, including @staging
   • skill.name is the requested point lookup target
   • core record fields (version, namespace, description, path) are non-empty
6. Keep the existing add assertions after that, so the full e2e flow becomes: init → search → show → add over the real auth-gated remote.

I implemented this locally as test(e2e): cover skillrig show workflow by extending TestE2E_FullWorkflow with the show --json step for both default-branch and @staging origins. I could not push it to this PR branch from the current token (git push origin HEAD:claude/load-golang-design-skills-LCApO returned 403: permission denied for sakul-learning). Patch is available from my local checkout if you want me to hand it off.

Verification run

Passed locally:

go test -tags e2e ./test/e2e/...
ok  	github.com/skillrig/cli/test/e2e	2.419s

go test ./...
?   	github.com/skillrig/cli	[no test files]
ok  	github.com/skillrig/cli/internal/cli	0.016s
ok  	github.com/skillrig/cli/internal/config	0.013s
ok  	github.com/skillrig/cli/internal/sourceguard	0.021s
ok  	github.com/skillrig/cli/pkg/skillcore	0.748s
ok  	github.com/skillrig/cli/test	3.132s
?   	github.com/skillrig/cli/test/e2e	[no test files]

go vet ./...
# no output / success

GOLANGCI_LINT_CACHE=$(mktemp -d) golangci-lint run
0 issues.

Note: plain golangci-lint run initially picked up stale cached paths under /home/ubuntu/reviews/skillrig-cli/...; rerunning with a fresh GOLANGCI_LINT_CACHE was clean.

Review finding

One correctness issue still looks worth fixing before merge: renderShowResult claims the footer is a runnable command, but it prints skillrig add <raw name>. validateSkillName only rejects empty/dot/path-separator names; it does not reject names with spaces or leading -. Those names can be present in a catalog and would make the copied footer command fail due shell splitting or Cobra flag parsing. Consider emitting a shell-safe command (for example skillrig add -- <quoted-name>) or tightening the skill-name invariant consistently across index/add/show.

[so0k]

Thanks @sakul-learning — both points addressed in eb58ba0 (no patch hand-off needed, I implemented them directly):

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, exactly as you outlined. The show <skill> --json step asserts exit 0 / empty stderr, a parseable {origin, skill}, that origin preserves the configured reference (including @staging), the point-lookup skill.name, and that version/namespace/description/path are non-empty. The @staging case targets the branch-only staging-only skill, so it doubles as proof that show reads the @ref'd catalog. Green locally:

make test-e2e
ok  github.com/skillrig/cli/test/e2e  2.49s

Footer runnability — agreed, and with two reviewers on it I made the footer shell/cobra-safe rather than only deferring to ingestion. renderShowResult now routes the name through shellSafeSkillArg: a spec-conformant agentskills.io slug is emitted bare (skillrig add terraform-plan-review), while a non-conformant name (leading -, spaces, metacharacters) is single-quoted and ---guarded (skillrig add -- '-rm rf') so a copied command stays runnable. Covered by TestShellSafeSkillArg + TestRenderShowResult_FooterRunnableForHostileName. Tightening the name slug at ingestion (index/lint) is still the right system-wide invariant — I'll track that as a separate follow-up since it's origin-side rather than consume-side.

Full suite, make test-e2e, and golangci-lint run are all green.

---

Generated by Claude Code

[sakul-learning]

Hermes follow-up review

I re-checked the full PR comment thread and the latest head commit eb58ba047cbc2c520dfa394327f1a5f1a89877d4.

My earlier concern is sufficiently addressed:

• True e2e coverage is now present in test/e2e/auth_e2e_test.go: TestE2E_FullWorkflow exercises init → search → show → add against the real auth-gated git http-backend origin, for both the default branch and @staging. The new show --json assertions cover exit 0, empty stderr, parseable {origin, skill}, origin preservation including @staging, requested point-lookup name, and populated version/namespace/description/path fields.
• Footer runnability is fixed in internal/cli/output.go: renderShowResult now routes the skill name through shellSafeSkillArg, leaving conformant slugs bare and using -- plus POSIX single-quoting for non-conformant/hostile names such as leading-dash or whitespace-containing names.
• Regression tests were added for the footer behavior: TestShellSafeSkillArg and TestRenderShowResult_FooterRunnableForHostileName.

I verified locally on the latest PR head:

go test ./internal/cli -run 'TestShellSafeSkillArg|TestRenderShowResult_FooterRunnableForHostileName|TestRenderShowResult'
ok  	github.com/skillrig/cli/internal/cli	0.005s

go test -tags e2e ./test/e2e -run TestE2E_FullWorkflow
ok  	github.com/skillrig/cli/test/e2e	2.507s

go test ./...
?   	github.com/skillrig/cli	[no test files]
ok  	github.com/skillrig/cli/internal/cli	0.020s
ok  	github.com/skillrig/cli/internal/config	0.015s
ok  	github.com/skillrig/cli/internal/sourceguard	0.019s
ok  	github.com/skillrig/cli/pkg/skillcore	0.766s
ok  	github.com/skillrig/cli/test	3.243s
?   	github.com/skillrig/cli/test/e2e	[no test files]

Only remaining note is non-blocking: enforcing the agentskills.io slug invariant at ingestion/index/lint would still be a useful follow-up, but this PR now defensively handles the consumer-side footer and covers the show path e2e.