From 00b15532726d3c058212ea0d2910f022e3275bc5 Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Thu, 4 Jun 2026 17:18:11 +0800 Subject: [PATCH 1/2] feat: add skills command to read embedded skill content Add a top-level `lark-cli skills` meta command group that reads skill content compiled into the binary at build time via go:embed, so the content an AI agent reads always matches the running CLI version. It is an additional, version-consistent source and a fallback for when skills are not installed or are stale; it does not replace or thin the on-disk skills. Commands: - `skills list` lists every embedded skill with name, description, and the frontmatter metadata block. - `skills list ` / `skills list /` list one directory layer (ls-style, no recursion); entries are skill-prefixed paths that can be fed straight back to `read`. - `skills read ` prints the skill's SKILL.md and appends a one-line tip steering the model to fetch reference files via this command. - `skills read ` / `skills read /` print any file under the skill directory (no tip). - `--json` wraps output in an envelope; on the main SKILL.md it carries a separate `guidance` field instead of merging the tip into content. Implementation: - skills/ is embedded from the repo-root package main (go:embed cannot cross parent dirs) and injected into cmd/skill via a package variable, avoiding any change to cmd.Execute's signature. - internal/skillcontent is pure logic over an injected fs.FS; reference reads and directory listings share one path-traversal guard that rejects absolute paths, "..", and "..\\" escapes. - Errors are typed (validation -> exit 2, file_io -> exit 5); rejected paths never write anything to stdout. Risk is set per leaf subcommand (read); the group carries none, matching the config/service command groups. The commands need no auth (local embed reads only). --- cmd/build.go | 2 + cmd/skill/skill.go | 189 ++++++++++++++++++ cmd/skill/skill_test.go | 266 +++++++++++++++++++++++++ internal/skillcontent/reader.go | 227 ++++++++++++++++++++++ internal/skillcontent/reader_test.go | 278 +++++++++++++++++++++++++++ skills_embed.go | 36 ++++ 6 files changed, 998 insertions(+) create mode 100644 cmd/skill/skill.go create mode 100644 cmd/skill/skill_test.go create mode 100644 internal/skillcontent/reader.go create mode 100644 internal/skillcontent/reader_test.go create mode 100644 skills_embed.go diff --git a/cmd/build.go b/cmd/build.go index 84828ed77..63dab1620 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -16,6 +16,7 @@ import ( "github.com/larksuite/cli/cmd/profile" "github.com/larksuite/cli/cmd/schema" "github.com/larksuite/cli/cmd/service" + "github.com/larksuite/cli/cmd/skill" cmdupdate "github.com/larksuite/cli/cmd/update" _ "github.com/larksuite/cli/events" "github.com/larksuite/cli/internal/build" @@ -140,6 +141,7 @@ func buildInternal(ctx context.Context, inv cmdutil.InvocationContext, opts ...B rootCmd.AddCommand(completion.NewCmdCompletion(f)) rootCmd.AddCommand(cmdupdate.NewCmdUpdate(f)) rootCmd.AddCommand(cmdevent.NewCmdEvents(f)) + rootCmd.AddCommand(skill.NewCmdSkill(f)) service.RegisterServiceCommandsWithContext(ctx, rootCmd, f) shortcuts.RegisterShortcutsWithContext(ctx, rootCmd, f) diff --git a/cmd/skill/skill.go b/cmd/skill/skill.go new file mode 100644 index 000000000..270971318 --- /dev/null +++ b/cmd/skill/skill.go @@ -0,0 +1,189 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +// Package skill implements the top-level `lark-cli skills` command group, which +// reads embedded skill content (injected via ContentFS) for AI agents. The +// package/dir name stays "skill" (internal); the user-facing verb is "skills". +package skill + +import ( + "fmt" + "io/fs" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/skillcontent" + "github.com/spf13/cobra" +) + +// ContentFS is the embedded skill filesystem, rooted at the skill list +// ("lark-calendar/SKILL.md", ...). It is injected by the repo-root package +// main at init time. Nil in builds that do not embed skills (e.g. example +// plugin hosts) — commands then return an internal error. +// +// Tests mutate this package global (see skill_test.go), so tests that touch it +// must not call t.Parallel() — concurrent writes would race. +var ContentFS fs.FS + +func newReader() (*skillcontent.Reader, error) { + if ContentFS == nil { + return nil, errs.NewInternalError(errs.SubtypeFileIO, + "failed to read embedded skill content: not embedded in this build") + } + return skillcontent.New(ContentFS), nil +} + +// NewCmdSkill builds the `skills` command group. +func NewCmdSkill(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "skills", + Short: "Read embedded skill content (list / read)", + Long: "Read skill content embedded in the CLI binary at build time. Content stays in sync with the CLI version.", + } + // Risk is set per leaf subcommand (GetRisk does not walk parents); the group + // itself carries none, matching the config/service command groups. AuthCheck + // is disabled on the group and propagates to children. + cmdutil.DisableAuthCheck(cmd) + cmd.AddCommand(newListCmd(f), newReadCmd(f)) + return cmd +} + +func newListCmd(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "list [name[/path]]", + Short: "List skills, or list one layer under a skill path (like ls)", + Args: cobra.ArbitraryArgs, + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) > 1 { + return errs.NewValidationError(errs.SubtypeInvalidArgument, + "list takes at most 1 argument: [name[/path]]"). + WithHint("run 'lark-cli skills list --help'") + } + r, err := newReader() + if err != nil { + return err + } + // "ok" makes these recognized envelopes so output.injectNotice can attach + // _notice (e.g. binary/skills update hints) — list is the AI's discovery + // entry point, where a "run lark-cli update" hint matters most. + if len(args) == 0 { + skills, err := r.List() + if err != nil { + return err + } + output.PrintJson(f.IOStreams.Out, map[string]any{ + "ok": true, "skills": skills, "count": len(skills), + }) + return nil + } + // One-layer directory listing under args[0]; unknown skill / traversal / + // non-directory → typed validation (exit 2). + entries, listed, err := r.ListPath(args[0]) + if err != nil { + return err + } + output.PrintJson(f.IOStreams.Out, map[string]any{ + "ok": true, "path": listed, "entries": entries, "count": len(entries), + }) + return nil + }, + } + // list output is always JSON; accept --json as a no-op so it stays symmetric + // with read (where --json is meaningful) and never surprises a caller with + // cobra's "unknown flag" (exit 1) for a flag the sibling command accepts. + cmd.Flags().Bool("json", false, "no-op (list output is always JSON)") + cmdutil.SetRisk(cmd, "read") + cmdutil.DisableAuthCheck(cmd) + return cmd +} + +func newReadCmd(f *cmdutil.Factory) *cobra.Command { + var asJSON bool + cmd := &cobra.Command{ + Use: "read [/] [path]", + Short: "Print a skill's SKILL.md, or a file under the skill (raw markdown by default)", + Args: cobra.ArbitraryArgs, + RunE: func(cmd *cobra.Command, args []string) error { + name, relpath, err := parseReadTarget(args) + if err != nil { + return err + } + r, err := newReader() + if err != nil { + return err + } + + var content []byte + var pathOut string + if relpath == "" { + content, err = r.ReadSkill(name) + pathOut = "SKILL.md" + } else { + content, pathOut, err = r.ReadReference(name, relpath) + } + if err != nil { + return err + } + + // Guidance is injected only when reading the main SKILL.md — it steers the + // model to fetch reference files via this command (so they match the CLI + // version) instead of opening them directly. Skipped for reference reads to + // avoid repeating it on every file. + isMain := pathOut == "SKILL.md" + if asJSON { + env := map[string]any{"skill": name, "path": pathOut, "content": string(content)} + if isMain { + env["guidance"] = readGuidance(name) + } + output.PrintJson(f.IOStreams.Out, env) + return nil + } + if _, err := f.IOStreams.Out.Write(content); err != nil { + return errs.NewInternalError(errs.SubtypeFileIO, "failed to write output: %v", err) + } + if isMain { + if _, err := fmt.Fprintf(f.IOStreams.Out, "\n\n%s\n", readGuidance(name)); err != nil { + return errs.NewInternalError(errs.SubtypeFileIO, "failed to write output: %v", err) + } + } + return nil + }, + } + cmd.Flags().BoolVar(&asJSON, "json", false, "output as a JSON envelope instead of raw markdown") + cmdutil.SetRisk(cmd, "read") + cmdutil.DisableAuthCheck(cmd) + return cmd +} + +// parseReadTarget resolves the read command's positional args into a skill name +// and an optional relative path. relpath "" means read the main SKILL.md. +// - 2 args → (args[0], args[1]) +// - 1 arg "a/b" → ("a", "b") (only the first '/' splits) +// - 1 arg "a" → ("a", "") +func parseReadTarget(args []string) (name, relpath string, err error) { + switch len(args) { + case 1: + a := args[0] + for i := 0; i < len(a); i++ { + if a[i] == '/' { + return a[:i], a[i+1:], nil + } + } + return a, "", nil + case 2: + return args[0], args[1], nil + default: + return "", "", errs.NewValidationError(errs.SubtypeInvalidArgument, + "read requires 1 or 2 arguments: [/] [path]"). + WithHint("run 'lark-cli skills read --help'") + } +} + +// readGuidance is the one-line tip appended to a skill's main SKILL.md output, +// directing the model to read referenced files via this command. +func readGuidance(name string) string { + return fmt.Sprintf("> Tip: read this skill's referenced files with "+ + "`lark-cli skills read %s ` instead of opening them directly, "+ + "so the content stays in sync with this CLI version.", name) +} diff --git a/cmd/skill/skill_test.go b/cmd/skill/skill_test.go new file mode 100644 index 000000000..eafed3497 --- /dev/null +++ b/cmd/skill/skill_test.go @@ -0,0 +1,266 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skill + +import ( + "encoding/json" + "io" + "strings" + "testing" + "testing/fstest" + + "github.com/larksuite/cli/internal/cmdutil" +) + +// setupFS installs a test ContentFS. Tests that touch the ContentFS package +// global must NOT call t.Parallel() — it would race. +func setupFS() { + ContentFS = fstest.MapFS{ + "lark-calendar/SKILL.md": {Data: []byte("---\nname: lark-calendar\ndescription: \"Cal\"\nmetadata:\n cliHelp: \"lark-cli calendar --help\"\n---\nbody")}, + "lark-calendar/references/agenda.md": {Data: []byte("# Agenda")}, + } +} + +func run(t *testing.T, args ...string) (stdout, stderr string, err error) { + t.Helper() + f, out, errOut, _ := cmdutil.TestFactory(t, nil) + cmd := NewCmdSkill(f) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs(args) + err = cmd.Execute() + return out.String(), errOut.String(), err +} + +func TestSkillList(t *testing.T) { + setupFS() + stdout, _, err := run(t, "list") + if err != nil { + t.Fatalf("list error: %v", err) + } + var got struct { + OK bool `json:"ok"` + Skills []map[string]any `json:"skills"` + Count int `json:"count"` + } + if e := json.Unmarshal([]byte(stdout), &got); e != nil { + t.Fatalf("invalid JSON: %v\n%s", e, stdout) + } + // "ok" marks this as a recognized envelope so _notice can be injected. + if !got.OK { + t.Error("expected ok=true in list envelope") + } + if got.Count != 1 || len(got.Skills) != 1 { + t.Fatalf("count: got %d", got.Count) + } + if got.Skills[0]["name"] != "lark-calendar" { + t.Errorf("name: got %v", got.Skills[0]["name"]) + } + // Top-level list carries metadata, not a references list. + if _, ok := got.Skills[0]["references"]; ok { + t.Error("top-level list must not include references") + } + if _, ok := got.Skills[0]["metadata"]; !ok { + t.Error("expected metadata in list entry") + } +} + +func TestSkillListJSONFlagAccepted(t *testing.T) { + setupFS() + // `list --json` must be accepted (no-op), not rejected as an unknown flag, + // so it stays symmetric with read --json. + stdout, _, err := run(t, "list", "--json") + if err != nil { + t.Fatalf("list --json error: %v", err) + } + var got struct { + OK bool `json:"ok"` + Count int `json:"count"` + } + if e := json.Unmarshal([]byte(stdout), &got); e != nil { + t.Fatalf("invalid JSON: %v\n%s", e, stdout) + } + if !got.OK || got.Count != 1 { + t.Errorf("envelope: %+v", got) + } +} + +func TestSkillListPath(t *testing.T) { + setupFS() + stdout, _, err := run(t, "list", "lark-calendar") + if err != nil { + t.Fatalf("list error: %v", err) + } + var got struct { + OK bool `json:"ok"` + Path string `json:"path"` + Entries []struct { + Path string `json:"path"` + IsDir bool `json:"is_dir"` + } `json:"entries"` + Count int `json:"count"` + } + if e := json.Unmarshal([]byte(stdout), &got); e != nil { + t.Fatalf("invalid JSON: %v\n%s", e, stdout) + } + if !got.OK || got.Path != "lark-calendar" { + t.Errorf("envelope: %+v", got) + } + // One layer under the skill root: SKILL.md (file) + references (dir). + if got.Count != 2 || len(got.Entries) != 2 { + t.Fatalf("entries: got %+v", got.Entries) + } + if got.Entries[0].Path != "lark-calendar/SKILL.md" || got.Entries[0].IsDir { + t.Errorf("entry[0]: got %+v", got.Entries[0]) + } + if got.Entries[1].Path != "lark-calendar/references" || !got.Entries[1].IsDir { + t.Errorf("entry[1]: got %+v", got.Entries[1]) + } +} + +func TestSkillListPathUnknown(t *testing.T) { + setupFS() + _, _, err := run(t, "list", "no-such-skill") + if err == nil || !strings.Contains(err.Error(), "unknown skill") { + t.Fatalf("expected 'unknown skill' error, got %v", err) + } +} + +func TestSkillListPathTraversal(t *testing.T) { + setupFS() + stdout, _, err := run(t, "list", "lark-calendar/../../etc") + if err == nil || !strings.Contains(err.Error(), "invalid path") { + t.Fatalf("expected 'invalid path' error, got %v", err) + } + if stdout != "" { + t.Errorf("stdout must be empty on rejection, got %q", stdout) + } +} + +func TestSkillListTooManyArgs(t *testing.T) { + setupFS() + _, _, err := run(t, "list", "a", "b") + if err == nil || !strings.Contains(err.Error(), "at most 1 argument") { + t.Fatalf("expected 'at most 1 argument' error, got %v", err) + } +} + +func TestSkillReadRaw(t *testing.T) { + setupFS() + stdout, _, err := run(t, "read", "lark-calendar") + if err != nil { + t.Fatalf("read error: %v", err) + } + if !strings.HasPrefix(stdout, "---\nname: lark-calendar") { + t.Errorf("raw output: got %q", stdout) + } + // Main SKILL.md output appends a guidance tip after the content. + if !strings.Contains(stdout, "lark-cli skills read lark-calendar ") { + t.Errorf("expected guidance tip in raw output: got %q", stdout) + } +} + +func TestSkillReadJSON(t *testing.T) { + setupFS() + stdout, _, err := run(t, "read", "lark-calendar", "--json") + if err != nil { + t.Fatalf("read --json error: %v", err) + } + var got struct { + Skill, Path, Content, Guidance string + } + if e := json.Unmarshal([]byte(stdout), &got); e != nil { + t.Fatalf("invalid JSON: %v", e) + } + if got.Skill != "lark-calendar" || got.Path != "SKILL.md" || got.Content == "" { + t.Errorf("envelope: %+v", got) + } + // Guidance is a separate field, not merged into content. + if got.Guidance == "" { + t.Error("expected guidance field for main SKILL.md") + } + if strings.Contains(got.Content, "Tip:") { + t.Error("guidance must not be merged into content") + } +} + +func TestSkillReadFile(t *testing.T) { + setupFS() + // Both the 2-arg and slash forms read the same file, with no guidance tip. + for _, args := range [][]string{ + {"read", "lark-calendar", "references/agenda.md"}, + {"read", "lark-calendar/references/agenda.md"}, + } { + stdout, _, err := run(t, args...) + if err != nil { + t.Fatalf("read %v error: %v", args, err) + } + if stdout != "# Agenda" { + t.Errorf("read %v output: got %q", args, stdout) + } + } +} + +func TestSkillReadFileJSON(t *testing.T) { + setupFS() + stdout, _, err := run(t, "read", "lark-calendar", "references/agenda.md", "--json") + if err != nil { + t.Fatalf("read file --json error: %v", err) + } + var got struct { + Skill, Path, Content, Guidance string + } + if e := json.Unmarshal([]byte(stdout), &got); e != nil { + t.Fatalf("invalid JSON: %v\n%s", e, stdout) + } + if got.Skill != "lark-calendar" || got.Path != "references/agenda.md" || got.Content != "# Agenda" { + t.Errorf("envelope: %+v", got) + } + // Reference reads do not carry the guidance tip. + if got.Guidance != "" { + t.Errorf("reference read must not include guidance, got %q", got.Guidance) + } +} + +func TestSkillReadUnknown(t *testing.T) { + setupFS() + _, _, err := run(t, "read", "no-such") + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), "unknown skill") { + t.Errorf("err: %v", err) + } +} + +func TestSkillReadMissingArg(t *testing.T) { + setupFS() + _, _, err := run(t, "read") + if err == nil || !strings.Contains(err.Error(), "requires 1 or 2 arguments") { + t.Fatalf("expected arg error, got %v", err) + } +} + +func TestSkillReadTraversal(t *testing.T) { + setupFS() + stdout, _, err := run(t, "read", "lark-calendar", "../../etc/passwd") + if err == nil { + t.Fatal("expected rejection") + } + if !strings.Contains(err.Error(), "invalid path") { + t.Errorf("err: %v", err) + } + if stdout != "" { + t.Errorf("stdout must be empty on rejection, got %q", stdout) + } +} + +func TestSkillNilContentFS(t *testing.T) { + ContentFS = nil + t.Cleanup(setupFS) + _, _, err := run(t, "list") + if err == nil { + t.Fatal("expected error when ContentFS is nil") + } +} diff --git a/internal/skillcontent/reader.go b/internal/skillcontent/reader.go new file mode 100644 index 000000000..f3fa83015 --- /dev/null +++ b/internal/skillcontent/reader.go @@ -0,0 +1,227 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +// Package skillcontent reads embedded skill content (SKILL.md bodies, files +// under a skill directory, and a skill inventory) from an injected fs.FS. The +// FS is rooted at the skill list (entries are "lark-calendar/SKILL.md", ...). +// It is pure logic — the embedding lives in the repo-root package main. +package skillcontent + +import ( + "io/fs" + "path" + "sort" + "strings" + + "github.com/larksuite/cli/errs" + "gopkg.in/yaml.v3" +) + +// Reader reads skill content from fsys (rooted at the skill list). +type Reader struct { + fsys fs.FS +} + +// New returns a Reader backed by fsys. +func New(fsys fs.FS) *Reader { return &Reader{fsys: fsys} } + +// SkillInfo describes one skill in the top-level list output. +type SkillInfo struct { + Name string `json:"name"` + Description string `json:"description"` + Metadata map[string]any `json:"metadata,omitempty"` +} + +// DirEntry is one child of a listed directory. Path is skill-name-prefixed +// (e.g. "lark-doc/references/x.md") so it can be passed straight to `read`. +type DirEntry struct { + Path string `json:"path"` + IsDir bool `json:"is_dir"` +} + +// List returns every skill (top-level dir) with its description and metadata +// (from SKILL.md frontmatter). Skills are sorted by name. +func (r *Reader) List() ([]SkillInfo, error) { + entries, err := fs.ReadDir(r.fsys, ".") + if err != nil { + return nil, errs.NewInternalError(errs.SubtypeFileIO, "failed to read embedded skills: %v", err) + } + out := make([]SkillInfo, 0, len(entries)) + for _, e := range entries { + if !e.IsDir() { + continue + } + out = append(out, r.skillInfo(e.Name())) + } + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + return out, nil +} + +// skillInfo builds the SkillInfo for an existing skill directory: description +// and metadata from SKILL.md frontmatter (zero values on any parse failure). +func (r *Reader) skillInfo(name string) SkillInfo { + info := SkillInfo{Name: name} + if data, err := fs.ReadFile(r.fsys, name+"/SKILL.md"); err == nil { + info.Description, info.Metadata = parseFrontmatter(data) + } + return info +} + +// ListPath lists the direct children (one layer, no recursion) of the directory +// named by arg, which is "" or "/". It returns the entries +// (sorted by path), the cleaned skill-prefixed path that was listed, and an +// error. Unknown skill, traversal, or a non-directory target → typed validation +// error. +func (r *Reader) ListPath(arg string) ([]DirEntry, string, error) { + name, sub := splitSkillPath(arg) + if err := r.ensureSkill(name); err != nil { + return nil, "", err + } + dir := name + if sub != "" { + cleaned, err := cleanSubPath(sub) + if err != nil { + return nil, "", err + } + dir = name + "/" + cleaned + info, err := fs.Stat(r.fsys, dir) + if err != nil { + return nil, "", errs.NewValidationError(errs.SubtypeInvalidArgument, + "path %q not found in skill %q", sub, name). + WithHint("run 'lark-cli skills list " + name + "' to see files in this skill") + } + if !info.IsDir() { + return nil, "", errs.NewValidationError(errs.SubtypeInvalidArgument, + "path %q is a file, not a directory; use 'lark-cli skills read %s/%s' to read it", sub, name, cleaned) + } + } + entries, err := fs.ReadDir(r.fsys, dir) + if err != nil { + return nil, "", errs.NewInternalError(errs.SubtypeFileIO, + "failed to read embedded skill content: %v", err) + } + out := make([]DirEntry, 0, len(entries)) + for _, e := range entries { + out = append(out, DirEntry{Path: dir + "/" + e.Name(), IsDir: e.IsDir()}) + } + sort.Slice(out, func(i, j int) bool { return out[i].Path < out[j].Path }) + return out, dir, nil +} + +// splitSkillPath splits "/" into (name, rest); a path with no +// separator yields (path, ""). Only the first '/' splits — rest may itself +// contain separators. +func splitSkillPath(arg string) (name, rest string) { + if i := strings.IndexByte(arg, '/'); i >= 0 { + return arg[:i], arg[i+1:] + } + return arg, "" +} + +// parseFrontmatter extracts the `description` and `metadata` fields from a +// SKILL.md YAML frontmatter block. Both are best-effort: missing or unparseable +// frontmatter yields ("", nil) — never an error. +func parseFrontmatter(skillMD []byte) (description string, metadata map[string]any) { + lines := strings.Split(string(skillMD), "\n") + if strings.TrimRight(lines[0], "\r") != "---" { + return "", nil + } + block := make([]string, 0, len(lines)) + closed := false + for _, ln := range lines[1:] { + if strings.TrimRight(ln, "\r") == "---" { + closed = true + break + } + block = append(block, ln) + } + if !closed { + return "", nil + } + var fm struct { + Description string `yaml:"description"` + Metadata map[string]any `yaml:"metadata"` + } + if err := yaml.Unmarshal([]byte(strings.Join(block, "\n")), &fm); err != nil { + return "", nil + } + return fm.Description, fm.Metadata +} + +// ReadSkill returns the raw bytes of /SKILL.md. +func (r *Reader) ReadSkill(name string) ([]byte, error) { + if err := r.ensureSkill(name); err != nil { + return nil, err + } + data, err := fs.ReadFile(r.fsys, name+"/SKILL.md") + if err != nil { + return nil, errs.NewInternalError(errs.SubtypeFileIO, + "failed to read embedded skill content: %v", err) + } + return data, nil +} + +// ensureSkill validates that name is a single path segment naming an embedded +// skill directory. Returns a typed validation error otherwise. +func (r *Reader) ensureSkill(name string) error { + if name == "" || strings.ContainsAny(name, `/\`) || name == "." || name == ".." { + return unknownSkill(name) + } + info, err := fs.Stat(r.fsys, name) + if err != nil || !info.IsDir() { + return unknownSkill(name) + } + return nil +} + +func unknownSkill(name string) error { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "unknown skill %q", name). + WithHint("run 'lark-cli skills list' to see available skills") +} + +// cleanSubPath validates that relpath is a safe relative path within a skill +// directory and returns its cleaned form. Absolute paths and ".." escapes are +// rejected with a typed validation error. relpath must be non-empty — callers +// handle the empty (skill-root) case themselves. +func cleanSubPath(relpath string) (string, error) { + cleaned := path.Clean(relpath) + // path.Clean only treats '/' as a separator, so a Windows-style "..\" prefix + // survives verbatim in cleaned; reject it explicitly alongside the "../" case. + if relpath == "" || path.IsAbs(relpath) || cleaned == "." || + cleaned == ".." || strings.HasPrefix(cleaned, "../") || strings.HasPrefix(cleaned, `..\`) { + return "", errs.NewValidationError(errs.SubtypeInvalidArgument, + "invalid path %q: must be a relative path without '..'", relpath) + } + return cleaned, nil +} + +// ReadReference returns the raw bytes of / and the cleaned +// relative path. relpath must be a relative path within the skill dir; ".." +// segments, absolute paths, and escapes are rejected with a typed validation +// error and no content is returned. +func (r *Reader) ReadReference(name, relpath string) ([]byte, string, error) { + if err := r.ensureSkill(name); err != nil { + return nil, "", err + } + cleaned, err := cleanSubPath(relpath) + if err != nil { + return nil, "", err + } + full := name + "/" + cleaned + info, err := fs.Stat(r.fsys, full) + if err != nil { + return nil, "", errs.NewValidationError(errs.SubtypeInvalidArgument, + "reference %q not found in skill %q", relpath, name). + WithHint("run 'lark-cli skills list " + name + "' to see files in this skill") + } + if info.IsDir() { + return nil, "", errs.NewValidationError(errs.SubtypeInvalidArgument, + "reference %q is a directory, not a file", relpath) + } + data, err := fs.ReadFile(r.fsys, full) + if err != nil { + return nil, "", errs.NewInternalError(errs.SubtypeFileIO, + "failed to read embedded skill content: %v", err) + } + return data, cleaned, nil +} diff --git a/internal/skillcontent/reader_test.go b/internal/skillcontent/reader_test.go new file mode 100644 index 000000000..19ef4489f --- /dev/null +++ b/internal/skillcontent/reader_test.go @@ -0,0 +1,278 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillcontent + +import ( + "errors" + "strings" + "testing" + "testing/fstest" + + "github.com/larksuite/cli/errs" +) + +func testFS() fstest.MapFS { + return fstest.MapFS{ + "lark-calendar/SKILL.md": {Data: []byte("---\nname: lark-calendar\ndescription: \"Calendar skill\"\nmetadata:\n requires:\n bins: [\"lark-cli\"]\n cliHelp: \"lark-cli calendar --help\"\n---\nbody\n")}, + "lark-calendar/references/agenda.md": {Data: []byte("# Agenda")}, + "lark-calendar/references/create.md": {Data: []byte("# Create")}, + "lark-calendar/assets/tpl.html": {Data: []byte("")}, + "lark-im/SKILL.md": {Data: []byte("no frontmatter here\n")}, + "lark-im/references/send.md": {Data: []byte("# Send")}, + } +} + +func TestList(t *testing.T) { + r := New(testFS()) + skills, err := r.List() + if err != nil { + t.Fatalf("List() error: %v", err) + } + if len(skills) != 2 { + t.Fatalf("got %d skills, want 2", len(skills)) + } + if skills[0].Name != "lark-calendar" || skills[1].Name != "lark-im" { + t.Fatalf("skills not sorted by name: %v", skills) + } + if skills[0].Description != "Calendar skill" { + t.Errorf("description: got %q, want %q", skills[0].Description, "Calendar skill") + } + // metadata is the frontmatter `metadata:` block, passed through verbatim. + if skills[0].Metadata == nil { + t.Fatal("expected metadata for lark-calendar") + } + if skills[0].Metadata["cliHelp"] != "lark-cli calendar --help" { + t.Errorf("metadata.cliHelp: got %v", skills[0].Metadata["cliHelp"]) + } + // No frontmatter → empty description and nil metadata (omitted from JSON). + if skills[1].Description != "" { + t.Errorf("lark-im description: got %q, want empty", skills[1].Description) + } + if skills[1].Metadata != nil { + t.Errorf("lark-im metadata: got %v, want nil", skills[1].Metadata) + } +} + +func TestListPath(t *testing.T) { + r := New(testFS()) + + // Skill root: direct children only (one layer), each path skill-prefixed. + entries, listed, err := r.ListPath("lark-calendar") + if err != nil { + t.Fatalf("ListPath root error: %v", err) + } + if listed != "lark-calendar" { + t.Errorf("listed path: got %q", listed) + } + want := map[string]bool{ // path → isDir + "lark-calendar/SKILL.md": false, + "lark-calendar/references": true, + "lark-calendar/assets": true, + } + if len(entries) != len(want) { + t.Fatalf("root entries: got %v, want %d entries", entries, len(want)) + } + for _, e := range entries { + isDir, ok := want[e.Path] + if !ok { + t.Errorf("unexpected entry %q", e.Path) + continue + } + if e.IsDir != isDir { + t.Errorf("%q is_dir: got %v, want %v", e.Path, e.IsDir, isDir) + } + } + // Entries are sorted by path. + if entries[0].Path != "lark-calendar/SKILL.md" { + t.Errorf("entries not sorted: %v", entries) + } + + // Subdirectory: one layer under /. + subEntries, subListed, err := r.ListPath("lark-calendar/references") + if err != nil { + t.Fatalf("ListPath subdir error: %v", err) + } + if subListed != "lark-calendar/references" { + t.Errorf("listed subpath: got %q", subListed) + } + if len(subEntries) != 2 || + subEntries[0].Path != "lark-calendar/references/agenda.md" || + subEntries[1].Path != "lark-calendar/references/create.md" { + t.Errorf("subdir entries: got %v", subEntries) + } + + // Unknown skill → typed validation error. + if _, _, err := r.ListPath("no-such-skill"); err == nil { + t.Error("expected error for unknown skill") + } else { + var verr *errs.ValidationError + if !errors.As(err, &verr) { + t.Errorf("expected *errs.ValidationError, got %T", err) + } + } + + // Path that points at a file (not a dir) → validation error. + if _, _, err := r.ListPath("lark-calendar/SKILL.md"); err == nil { + t.Error("expected error listing a file") + } else if !strings.Contains(err.Error(), "is a file") { + t.Errorf("message: got %q", err.Error()) + } + + // Nonexistent subpath → validation error. + if _, _, err := r.ListPath("lark-calendar/nope"); err == nil { + t.Error("expected not-found error") + } else if !strings.Contains(err.Error(), "not found") { + t.Errorf("message: got %q", err.Error()) + } + + // Traversal in the subpath is rejected, no listing leaked. + for _, bad := range []string{"lark-calendar/../lark-im", "lark-calendar/../../etc", `lark-calendar/..\x`} { + entries, _, err := r.ListPath(bad) + if err == nil { + t.Errorf("expected rejection for %q", bad) + } + if entries != nil { + t.Errorf("entries leaked for %q: %v", bad, entries) + } + } +} + +func TestReadSkill(t *testing.T) { + r := New(testFS()) + + data, err := r.ReadSkill("lark-calendar") + if err != nil { + t.Fatalf("ReadSkill error: %v", err) + } + if !strings.HasPrefix(string(data), "---\nname: lark-calendar") { + t.Errorf("unexpected content: %q", string(data)) + } + + _, err = r.ReadSkill("no-such-skill") + if err == nil { + t.Fatal("expected error for unknown skill") + } + var verr *errs.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("expected *errs.ValidationError, got %T", err) + } + if !strings.Contains(verr.Message, `unknown skill "no-such-skill"`) { + t.Errorf("message: got %q", verr.Message) + } + + if _, err := r.ReadSkill("../etc"); err == nil { + t.Error("expected error for name with separator") + } +} + +func TestReadReference(t *testing.T) { + r := New(testFS()) + + data, cleaned, err := r.ReadReference("lark-calendar", "references/agenda.md") + if err != nil { + t.Fatalf("ReadReference error: %v", err) + } + if string(data) != "# Agenda" { + t.Errorf("content: got %q", string(data)) + } + if cleaned != "references/agenda.md" { + t.Errorf("cleaned path: got %q", cleaned) + } + + if _, _, err := r.ReadReference("lark-calendar", "references/nope.md"); err == nil { + t.Error("expected not-found error") + } else if !strings.Contains(err.Error(), "not found") { + t.Errorf("message: got %q", err.Error()) + } + + if _, _, err := r.ReadReference("lark-calendar", "references"); err == nil { + t.Error("expected directory error") + } else if !strings.Contains(err.Error(), "is a directory") { + t.Errorf("message: got %q", err.Error()) + } + + for _, bad := range []string{"../../etc/passwd", "/etc/passwd", "..", "", "references/../../im/SKILL.md", `..\..\x`} { + data, _, err := r.ReadReference("lark-calendar", bad) + if err == nil { + t.Errorf("expected rejection for %q", bad) + } + if data != nil { + t.Errorf("content leaked for %q: %q", bad, string(data)) + } + var verr *errs.ValidationError + if !errors.As(err, &verr) { + t.Errorf("expected validation error for %q, got %T", bad, err) + } + } +} + +func TestParseFrontmatter(t *testing.T) { + cases := []struct { + name string + input string + wantDesc string + wantHasMeta bool + }{ + { + name: "description and metadata", + input: "---\ndescription: My skill\nmetadata:\n cliHelp: \"x\"\n---\nbody\n", + wantDesc: "My skill", + wantHasMeta: true, + }, + { + name: "description only, no metadata", + input: "---\ndescription: Plain\n---\nbody\n", + wantDesc: "Plain", + }, + { + name: "no frontmatter", + input: "no frontmatter here\n", + }, + { + name: "unclosed frontmatter", + input: "---\ndescription: Never closed\n", + }, + { + name: "malformed YAML inside frontmatter", + input: "---\n: bad: yaml: [\n---\nbody\n", + }, + { + name: "CRLF line endings", + input: "---\r\ndescription: CRLF skill\r\nmetadata:\r\n cliHelp: \"y\"\r\n---\r\nbody\r\n", + wantDesc: "CRLF skill", + wantHasMeta: true, + }, + { + name: "empty input", + input: "", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + desc, meta := parseFrontmatter([]byte(tc.input)) + if desc != tc.wantDesc { + t.Errorf("description = %q, want %q", desc, tc.wantDesc) + } + if (meta != nil) != tc.wantHasMeta { + t.Errorf("metadata = %v, wantHasMeta %v", meta, tc.wantHasMeta) + } + }) + } +} + +func TestReadSkillMissingFile(t *testing.T) { + // Use a separate MapFS so testFS() (and TestList) are unaffected. + emptyFS := fstest.MapFS{ + "lark-empty/references/x.md": {Data: []byte("# X")}, + } + r := New(emptyFS) + _, err := r.ReadSkill("lark-empty") + if err == nil { + t.Fatal("expected error when SKILL.md is absent") + } + var ierr *errs.InternalError + if !errors.As(err, &ierr) { + t.Fatalf("expected *errs.InternalError, got %T: %v", err, err) + } +} diff --git a/skills_embed.go b/skills_embed.go new file mode 100644 index 000000000..3d8214f2a --- /dev/null +++ b/skills_embed.go @@ -0,0 +1,36 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package main + +import ( + "embed" + "io/fs" + + "github.com/larksuite/cli/cmd/skill" +) + +// skillsEmbedFS embeds the entire skills/ tree at build time so the CLI can +// serve skill content that is guaranteed to match the binary version. +// +// We use `//go:embed skills` (not `all:skills`) deliberately: the default form +// excludes files/dirs whose names begin with "." or "_" at any depth, which +// keeps editor/OS junk (e.g. .DS_Store) out of the binary. The trade-off is +// that any future skills/ file intentionally named with a "." or "_" prefix +// would be silently omitted from `skill list` / `skill read` / `skill reference`. +// +//go:embed skills +var skillsEmbedFS embed.FS + +func init() { + // Strip the "skills/" prefix so paths are "lark-calendar/SKILL.md". + sub, err := fs.Sub(skillsEmbedFS, "skills") + if err != nil { + // Unreachable for the literal, valid path "skills"; fs.Sub only errors + // on a malformed sub-path. If a refactor ever breaks the embed root, + // fail loud at startup rather than ship a binary whose every `skill` + // command returns an opaque "not embedded in this build" error. + panic("skills_embed: fs.Sub(\"skills\") failed: " + err.Error()) + } + skill.ContentFS = sub +} From 51a72d000657818e82a61d20915cc9f17c3bc2f9 Mon Sep 17 00:00:00 2001 From: dc-bytedance Date: Sat, 6 Jun 2026 17:05:00 +0800 Subject: [PATCH 2/2] refactor: apply review feedback, fix CI, and slim the embed Squashes the review-response iteration on top of the initial skills command. - Inject the embedded tree via an init() calling cmd.SetEmbeddedSkillContent (no cmd/skill package global, no panic; tests inject through the Factory). main.go stays self-contained so the single-file preview build (go build ./main.go) still compiles. - Surface the frontmatter version field in `skills list`. - Use typed structs for the list and read JSON envelopes. - Scope the read guidance tip to the skill's own files, route cross-skill "../lark-foo/..." references back through `skills read`, and emit it on stderr in raw mode (stdout stays byte-identical to SKILL.md) and in a guidance field under --json. - Share one path splitter (skillcontent.SplitArg) between read and list. - Skip top-level directories without a SKILL.md in List(). - Add usage examples to `skills list` and `skills read`. - Isolate config state in the command test helper. - Slim the embed to agent-readable content (SKILL.md + references/, plus lark-whiteboard's routes/ and scenes/), excluding machine-resource assets/ and scripts/: release binary 26.7 -> 23.4 MB, no change to what the commands serve. --- cmd/build.go | 14 +++ cmd/skill/skill.go | 125 ++++++++++++++++----------- cmd/skill/skill_test.go | 122 +++++++++++++++++--------- internal/cmdutil/factory.go | 3 + internal/skillcontent/reader.go | 61 +++++++------ internal/skillcontent/reader_test.go | 20 ++++- skills_embed.go | 56 ++++++++---- 7 files changed, 263 insertions(+), 138 deletions(-) diff --git a/cmd/build.go b/cmd/build.go index 63dab1620..a31d41a6b 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -6,6 +6,7 @@ package cmd import ( "context" "io" + "io/fs" "github.com/larksuite/cli/cmd/api" "github.com/larksuite/cli/cmd/auth" @@ -52,6 +53,18 @@ func WithKeychain(kc keychain.KeychainAccess) BuildOption { } } +// embeddedSkillContent is the skill tree wired into cmdutil.Factory.SkillContent +// at build time. It is registered by the repo-root package main's init via +// SetEmbeddedSkillContent — it cannot be threaded through main.go without +// breaking the single-file preview build (see skills_embed.go). nil in builds +// that embed no skills; the `skills` commands then return a typed internal error. +var embeddedSkillContent fs.FS + +// SetEmbeddedSkillContent registers the embedded skill tree. Called from the +// repo-root package main's init; a wrapper main can call it before Execute to +// supply its own skill content. +func SetEmbeddedSkillContent(fsys fs.FS) { embeddedSkillContent = fsys } + // HideProfile sets the visibility policy for the root-level --profile flag. // When hide is true the flag stays registered (so existing invocations still // parse) but is omitted from help and shell completion. Typically called as @@ -104,6 +117,7 @@ func buildInternal(ctx context.Context, inv cmdutil.InvocationContext, opts ...B if cfg.keychain != nil { f.Keychain = cfg.keychain } + f.SkillContent = embeddedSkillContent rootCmd := &cobra.Command{ Use: "lark-cli", Short: "Lark/Feishu CLI — OAuth authorization, UAT management, API calls", diff --git a/cmd/skill/skill.go b/cmd/skill/skill.go index 270971318..5723155da 100644 --- a/cmd/skill/skill.go +++ b/cmd/skill/skill.go @@ -2,13 +2,13 @@ // SPDX-License-Identifier: MIT // Package skill implements the top-level `lark-cli skills` command group, which -// reads embedded skill content (injected via ContentFS) for AI agents. The -// package/dir name stays "skill" (internal); the user-facing verb is "skills". +// reads skill content embedded in the binary (injected via the Factory's +// SkillContent fs.FS) for AI agents. The package/dir name stays "skill" +// (internal); the user-facing verb is "skills". package skill import ( "fmt" - "io/fs" "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" @@ -17,21 +17,45 @@ import ( "github.com/spf13/cobra" ) -// ContentFS is the embedded skill filesystem, rooted at the skill list -// ("lark-calendar/SKILL.md", ...). It is injected by the repo-root package -// main at init time. Nil in builds that do not embed skills (e.g. example -// plugin hosts) — commands then return an internal error. -// -// Tests mutate this package global (see skill_test.go), so tests that touch it -// must not call t.Parallel() — concurrent writes would race. -var ContentFS fs.FS - -func newReader() (*skillcontent.Reader, error) { - if ContentFS == nil { +// newReader builds a Reader over the embedded skill tree carried by the Factory +// (wired by cmd.WithSkillContent). Builds that embed no skills leave it nil; the +// commands then return a typed internal error instead of panicking. +func newReader(f *cmdutil.Factory) (*skillcontent.Reader, error) { + if f.SkillContent == nil { return nil, errs.NewInternalError(errs.SubtypeFileIO, - "failed to read embedded skill content: not embedded in this build") + "skill content not embedded in this build") } - return skillcontent.New(ContentFS), nil + return skillcontent.New(f.SkillContent), nil +} + +// readEnvelope is the --json shape for `skills read`. Guidance is present only +// when reading the main SKILL.md (omitted for reference files). +type readEnvelope struct { + Skill string `json:"skill"` + Path string `json:"path"` + Content string `json:"content"` + Guidance string `json:"guidance,omitempty"` +} + +// listEnvelope is the JSON shape for `skills list` (catalog form). "ok" is an +// explicit success marker. These are typed structs (not maps), so the automatic +// output.injectNotice _notice does not attach — that notice is a general +// binary/disk-skills update hint surfaced on every other command, and the +// embedded catalog is version-consistent by construction, so its absence here +// loses nothing. +type listEnvelope struct { + OK bool `json:"ok"` + Skills []skillcontent.SkillInfo `json:"skills"` + Count int `json:"count"` +} + +// listPathEnvelope is the JSON shape for `skills list ` (the ls-style +// one-layer directory listing). +type listPathEnvelope struct { + OK bool `json:"ok"` + Path string `json:"path"` + Entries []skillcontent.DirEntry `json:"entries"` + Count int `json:"count"` } // NewCmdSkill builds the `skills` command group. @@ -53,28 +77,26 @@ func newListCmd(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "list [name[/path]]", Short: "List skills, or list one layer under a skill path (like ls)", - Args: cobra.ArbitraryArgs, + Example: ` lark-cli skills list # all skills: name, description, version + lark-cli skills list lark-doc # one layer under a skill (like ls) + lark-cli skills list lark-doc/references # one layer under a subdirectory`, + Args: cobra.ArbitraryArgs, RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 1 { return errs.NewValidationError(errs.SubtypeInvalidArgument, "list takes at most 1 argument: [name[/path]]"). WithHint("run 'lark-cli skills list --help'") } - r, err := newReader() + r, err := newReader(f) if err != nil { return err } - // "ok" makes these recognized envelopes so output.injectNotice can attach - // _notice (e.g. binary/skills update hints) — list is the AI's discovery - // entry point, where a "run lark-cli update" hint matters most. if len(args) == 0 { skills, err := r.List() if err != nil { return err } - output.PrintJson(f.IOStreams.Out, map[string]any{ - "ok": true, "skills": skills, "count": len(skills), - }) + output.PrintJson(f.IOStreams.Out, listEnvelope{OK: true, Skills: skills, Count: len(skills)}) return nil } // One-layer directory listing under args[0]; unknown skill / traversal / @@ -83,9 +105,7 @@ func newListCmd(f *cmdutil.Factory) *cobra.Command { if err != nil { return err } - output.PrintJson(f.IOStreams.Out, map[string]any{ - "ok": true, "path": listed, "entries": entries, "count": len(entries), - }) + output.PrintJson(f.IOStreams.Out, listPathEnvelope{OK: true, Path: listed, Entries: entries, Count: len(entries)}) return nil }, } @@ -103,13 +123,17 @@ func newReadCmd(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "read [/] [path]", Short: "Print a skill's SKILL.md, or a file under the skill (raw markdown by default)", - Args: cobra.ArbitraryArgs, + Example: ` lark-cli skills read lark-doc # the skill's SKILL.md + lark-cli skills read lark-doc references/lark-doc-fetch.md # a file under the skill + lark-cli skills read lark-doc/references/lark-doc-fetch.md # same, slash form + lark-cli skills read lark-doc --json # JSON envelope`, + Args: cobra.ArbitraryArgs, RunE: func(cmd *cobra.Command, args []string) error { name, relpath, err := parseReadTarget(args) if err != nil { return err } - r, err := newReader() + r, err := newReader(f) if err != nil { return err } @@ -126,26 +150,26 @@ func newReadCmd(f *cmdutil.Factory) *cobra.Command { return err } - // Guidance is injected only when reading the main SKILL.md — it steers the - // model to fetch reference files via this command (so they match the CLI - // version) instead of opening them directly. Skipped for reference reads to - // avoid repeating it on every file. + // Guidance is emitted only when reading the main SKILL.md — it nudges + // the model to fetch this skill's own reference files via this command + // (so they match the CLI version). Skipped for reference reads. isMain := pathOut == "SKILL.md" if asJSON { - env := map[string]any{"skill": name, "path": pathOut, "content": string(content)} + env := readEnvelope{Skill: name, Path: pathOut, Content: string(content)} if isMain { - env["guidance"] = readGuidance(name) + env.Guidance = readGuidance(name) } output.PrintJson(f.IOStreams.Out, env) return nil } + // Raw mode: stdout is the SKILL.md bytes verbatim (so callers can treat + // it as the file content). The guidance goes to stderr instead, keeping + // stdout byte-identical while a human/agent still sees the tip. if _, err := f.IOStreams.Out.Write(content); err != nil { return errs.NewInternalError(errs.SubtypeFileIO, "failed to write output: %v", err) } if isMain { - if _, err := fmt.Fprintf(f.IOStreams.Out, "\n\n%s\n", readGuidance(name)); err != nil { - return errs.NewInternalError(errs.SubtypeFileIO, "failed to write output: %v", err) - } + fmt.Fprintln(f.IOStreams.ErrOut, readGuidance(name)) } return nil }, @@ -164,13 +188,8 @@ func newReadCmd(f *cmdutil.Factory) *cobra.Command { func parseReadTarget(args []string) (name, relpath string, err error) { switch len(args) { case 1: - a := args[0] - for i := 0; i < len(a); i++ { - if a[i] == '/' { - return a[:i], a[i+1:], nil - } - } - return a, "", nil + name, relpath = skillcontent.SplitArg(args[0]) + return name, relpath, nil case 2: return args[0], args[1], nil default: @@ -180,10 +199,16 @@ func parseReadTarget(args []string) (name, relpath string, err error) { } } -// readGuidance is the one-line tip appended to a skill's main SKILL.md output, -// directing the model to read referenced files via this command. +// readGuidance is the one-line tip emitted for a skill's main SKILL.md — to +// stderr in raw mode, or the --json guidance field; never appended to stdout. +// It points the model at `skills read` for both this skill's own files and +// references to sibling skills: a "../lark-foo/..." reference is the same +// command with the leading "../" removed, keeping every hop version-consistent +// with the embedded tree (the path guard rejects literal "../", so the relative +// form must be rewritten to the sibling skill's name). func readGuidance(name string) string { - return fmt.Sprintf("> Tip: read this skill's referenced files with "+ - "`lark-cli skills read %s ` instead of opening them directly, "+ - "so the content stays in sync with this CLI version.", name) + return fmt.Sprintf("> Tip: read this skill's own files (e.g. `references/...`) with "+ + "`lark-cli skills read %s ` to keep them in sync with this CLI version. "+ + "A reference to another skill (`../lark-foo/...`) uses the same command with the "+ + "leading `../` removed: `lark-cli skills read lark-foo/...`.", name) } diff --git a/cmd/skill/skill_test.go b/cmd/skill/skill_test.go index eafed3497..af7b2caa4 100644 --- a/cmd/skill/skill_test.go +++ b/cmd/skill/skill_test.go @@ -6,6 +6,7 @@ package skill import ( "encoding/json" "io" + "io/fs" "strings" "testing" "testing/fstest" @@ -13,18 +14,25 @@ import ( "github.com/larksuite/cli/internal/cmdutil" ) -// setupFS installs a test ContentFS. Tests that touch the ContentFS package -// global must NOT call t.Parallel() — it would race. -func setupFS() { - ContentFS = fstest.MapFS{ - "lark-calendar/SKILL.md": {Data: []byte("---\nname: lark-calendar\ndescription: \"Cal\"\nmetadata:\n cliHelp: \"lark-cli calendar --help\"\n---\nbody")}, +// calFS is the default single-skill content tree for these tests. The embedded +// FS is now injected through the Factory (no package global), so tests pass it +// explicitly to run() — nothing is shared, so they are safe under -parallel. +func calFS() fstest.MapFS { + return fstest.MapFS{ + "lark-calendar/SKILL.md": {Data: []byte("---\nname: lark-calendar\nversion: 1.0.0\ndescription: \"Cal\"\nmetadata:\n cliHelp: \"lark-cli calendar --help\"\n---\nbody")}, "lark-calendar/references/agenda.md": {Data: []byte("# Agenda")}, } } -func run(t *testing.T, args ...string) (stdout, stderr string, err error) { +// run executes the skills command tree against the given content FS (may be nil +// to exercise the not-embedded path) and returns stdout/stderr/err. +func run(t *testing.T, fsys fs.FS, args ...string) (stdout, stderr string, err error) { t.Helper() + // Isolate CLI config state so tests never read/write the real config dir + // (repo convention). + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, out, errOut, _ := cmdutil.TestFactory(t, nil) + f.SkillContent = fsys cmd := NewCmdSkill(f) cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) @@ -34,8 +42,7 @@ func run(t *testing.T, args ...string) (stdout, stderr string, err error) { } func TestSkillList(t *testing.T) { - setupFS() - stdout, _, err := run(t, "list") + stdout, _, err := run(t, calFS(), "list") if err != nil { t.Fatalf("list error: %v", err) } @@ -47,7 +54,8 @@ func TestSkillList(t *testing.T) { if e := json.Unmarshal([]byte(stdout), &got); e != nil { t.Fatalf("invalid JSON: %v\n%s", e, stdout) } - // "ok" marks this as a recognized envelope so _notice can be injected. + // "ok" is an explicit success marker (the list envelope is a typed struct; + // no automatic _notice attaches). if !got.OK { t.Error("expected ok=true in list envelope") } @@ -57,20 +65,22 @@ func TestSkillList(t *testing.T) { if got.Skills[0]["name"] != "lark-calendar" { t.Errorf("name: got %v", got.Skills[0]["name"]) } - // Top-level list carries metadata, not a references list. + // Top-level list carries version + metadata, not a references list. if _, ok := got.Skills[0]["references"]; ok { t.Error("top-level list must not include references") } + if got.Skills[0]["version"] != "1.0.0" { + t.Errorf("version: got %v, want 1.0.0", got.Skills[0]["version"]) + } if _, ok := got.Skills[0]["metadata"]; !ok { t.Error("expected metadata in list entry") } } func TestSkillListJSONFlagAccepted(t *testing.T) { - setupFS() // `list --json` must be accepted (no-op), not rejected as an unknown flag, // so it stays symmetric with read --json. - stdout, _, err := run(t, "list", "--json") + stdout, _, err := run(t, calFS(), "list", "--json") if err != nil { t.Fatalf("list --json error: %v", err) } @@ -87,8 +97,7 @@ func TestSkillListJSONFlagAccepted(t *testing.T) { } func TestSkillListPath(t *testing.T) { - setupFS() - stdout, _, err := run(t, "list", "lark-calendar") + stdout, _, err := run(t, calFS(), "list", "lark-calendar") if err != nil { t.Fatalf("list error: %v", err) } @@ -120,16 +129,14 @@ func TestSkillListPath(t *testing.T) { } func TestSkillListPathUnknown(t *testing.T) { - setupFS() - _, _, err := run(t, "list", "no-such-skill") + _, _, err := run(t, calFS(), "list", "no-such-skill") if err == nil || !strings.Contains(err.Error(), "unknown skill") { t.Fatalf("expected 'unknown skill' error, got %v", err) } } func TestSkillListPathTraversal(t *testing.T) { - setupFS() - stdout, _, err := run(t, "list", "lark-calendar/../../etc") + stdout, _, err := run(t, calFS(), "list", "lark-calendar/../../etc") if err == nil || !strings.Contains(err.Error(), "invalid path") { t.Fatalf("expected 'invalid path' error, got %v", err) } @@ -139,31 +146,64 @@ func TestSkillListPathTraversal(t *testing.T) { } func TestSkillListTooManyArgs(t *testing.T) { - setupFS() - _, _, err := run(t, "list", "a", "b") + _, _, err := run(t, calFS(), "list", "a", "b") if err == nil || !strings.Contains(err.Error(), "at most 1 argument") { t.Fatalf("expected 'at most 1 argument' error, got %v", err) } } +// TestSkillListSkipsDirWithoutSKILLmd proves a top-level dir lacking SKILL.md is +// omitted from the catalog (no blank entry). +func TestSkillListSkipsDirWithoutSKILLmd(t *testing.T) { + fsys := fstest.MapFS{ + "lark-calendar/SKILL.md": {Data: []byte("---\nname: lark-calendar\ndescription: \"Cal\"\n---\nb")}, + "not-a-skill/readme.txt": {Data: []byte("junk")}, // dir without SKILL.md + } + stdout, _, err := run(t, fsys, "list") + if err != nil { + t.Fatalf("list error: %v", err) + } + var got struct { + Skills []map[string]any `json:"skills"` + Count int `json:"count"` + } + if e := json.Unmarshal([]byte(stdout), &got); e != nil { + t.Fatalf("invalid JSON: %v\n%s", e, stdout) + } + if got.Count != 1 || got.Skills[0]["name"] != "lark-calendar" { + t.Fatalf("expected only lark-calendar, got %+v", got.Skills) + } +} + func TestSkillReadRaw(t *testing.T) { - setupFS() - stdout, _, err := run(t, "read", "lark-calendar") + stdout, stderr, err := run(t, calFS(), "read", "lark-calendar") if err != nil { t.Fatalf("read error: %v", err) } if !strings.HasPrefix(stdout, "---\nname: lark-calendar") { t.Errorf("raw output: got %q", stdout) } - // Main SKILL.md output appends a guidance tip after the content. - if !strings.Contains(stdout, "lark-cli skills read lark-calendar ") { - t.Errorf("expected guidance tip in raw output: got %q", stdout) + // Raw stdout is byte-pure SKILL.md — the guidance tip must NOT be appended. + if strings.Contains(stdout, "Tip:") { + t.Errorf("raw stdout must not carry the guidance tip: got %q", stdout) + } + // Guidance goes to stderr: own files via `skills read ...`, and + // cross-skill refs routed to `skills read ...` (version- + // consistent), not "read directly". + if !strings.Contains(stderr, "lark-cli skills read lark-calendar ") { + t.Errorf("expected own-files guidance on stderr: got %q", stderr) + } + if !strings.Contains(stderr, "lark-cli skills read lark-foo/...") { + t.Errorf("expected cross-skill refs routed to skills read: got %q", stderr) + } + if strings.Contains(stderr, "instead of opening them directly") || + strings.Contains(stderr, "read those directly") { + t.Errorf("guidance must not steer cross-skill refs to direct reads: got %q", stderr) } } func TestSkillReadJSON(t *testing.T) { - setupFS() - stdout, _, err := run(t, "read", "lark-calendar", "--json") + stdout, _, err := run(t, calFS(), "read", "lark-calendar", "--json") if err != nil { t.Fatalf("read --json error: %v", err) } @@ -186,25 +226,27 @@ func TestSkillReadJSON(t *testing.T) { } func TestSkillReadFile(t *testing.T) { - setupFS() // Both the 2-arg and slash forms read the same file, with no guidance tip. for _, args := range [][]string{ {"read", "lark-calendar", "references/agenda.md"}, {"read", "lark-calendar/references/agenda.md"}, } { - stdout, _, err := run(t, args...) + stdout, stderr, err := run(t, calFS(), args...) if err != nil { t.Fatalf("read %v error: %v", args, err) } if stdout != "# Agenda" { t.Errorf("read %v output: got %q", args, stdout) } + // Reference reads carry no guidance on either stream. + if strings.Contains(stderr, "Tip:") { + t.Errorf("read %v must not emit guidance on stderr: got %q", args, stderr) + } } } func TestSkillReadFileJSON(t *testing.T) { - setupFS() - stdout, _, err := run(t, "read", "lark-calendar", "references/agenda.md", "--json") + stdout, _, err := run(t, calFS(), "read", "lark-calendar", "references/agenda.md", "--json") if err != nil { t.Fatalf("read file --json error: %v", err) } @@ -224,8 +266,7 @@ func TestSkillReadFileJSON(t *testing.T) { } func TestSkillReadUnknown(t *testing.T) { - setupFS() - _, _, err := run(t, "read", "no-such") + _, _, err := run(t, calFS(), "read", "no-such") if err == nil { t.Fatal("expected error") } @@ -235,16 +276,14 @@ func TestSkillReadUnknown(t *testing.T) { } func TestSkillReadMissingArg(t *testing.T) { - setupFS() - _, _, err := run(t, "read") + _, _, err := run(t, calFS(), "read") if err == nil || !strings.Contains(err.Error(), "requires 1 or 2 arguments") { t.Fatalf("expected arg error, got %v", err) } } func TestSkillReadTraversal(t *testing.T) { - setupFS() - stdout, _, err := run(t, "read", "lark-calendar", "../../etc/passwd") + stdout, _, err := run(t, calFS(), "read", "lark-calendar", "../../etc/passwd") if err == nil { t.Fatal("expected rejection") } @@ -257,10 +296,11 @@ func TestSkillReadTraversal(t *testing.T) { } func TestSkillNilContentFS(t *testing.T) { - ContentFS = nil - t.Cleanup(setupFS) - _, _, err := run(t, "list") + _, _, err := run(t, nil, "list") if err == nil { - t.Fatal("expected error when ContentFS is nil") + t.Fatal("expected error when SkillContent is nil") + } + if !strings.Contains(err.Error(), "not embedded") { + t.Errorf("err: %v", err) } } diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index 827d7e58d..1b167b786 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -6,6 +6,7 @@ package cmdutil import ( "context" "io" + "io/fs" "net/http" "strings" @@ -43,6 +44,8 @@ type Factory struct { Credential *credential.CredentialProvider FileIOProvider fileio.Provider // file transfer provider (default: local filesystem) + + SkillContent fs.FS // embedded skill tree (rooted at the skill list); nil when the build embeds no skills } // ResolveFileIO resolves a FileIO instance using the current execution context. diff --git a/internal/skillcontent/reader.go b/internal/skillcontent/reader.go index f3fa83015..fc895339d 100644 --- a/internal/skillcontent/reader.go +++ b/internal/skillcontent/reader.go @@ -29,6 +29,7 @@ func New(fsys fs.FS) *Reader { return &Reader{fsys: fsys} } type SkillInfo struct { Name string `json:"name"` Description string `json:"description"` + Version string `json:"version,omitempty"` Metadata map[string]any `json:"metadata,omitempty"` } @@ -39,8 +40,8 @@ type DirEntry struct { IsDir bool `json:"is_dir"` } -// List returns every skill (top-level dir) with its description and metadata -// (from SKILL.md frontmatter). Skills are sorted by name. +// List returns every skill (top-level dir) with its description, version, and +// metadata (from SKILL.md frontmatter). Skills are sorted by name. func (r *Reader) List() ([]SkillInfo, error) { entries, err := fs.ReadDir(r.fsys, ".") if err != nil { @@ -51,20 +52,27 @@ func (r *Reader) List() ([]SkillInfo, error) { if !e.IsDir() { continue } - out = append(out, r.skillInfo(e.Name())) + // Skip directories without a SKILL.md: they are not real skills, and a + // blank entry in the catalog would be worse than an omission. Full + // validation (name==dir, etc.) is enforced at build time, not here. + if info, ok := r.skillInfo(e.Name()); ok { + out = append(out, info) + } } sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) return out, nil } -// skillInfo builds the SkillInfo for an existing skill directory: description -// and metadata from SKILL.md frontmatter (zero values on any parse failure). -func (r *Reader) skillInfo(name string) SkillInfo { - info := SkillInfo{Name: name} - if data, err := fs.ReadFile(r.fsys, name+"/SKILL.md"); err == nil { - info.Description, info.Metadata = parseFrontmatter(data) +// skillInfo builds the SkillInfo for a skill directory from its SKILL.md +// frontmatter (description/version/metadata). The bool is false when the +// directory has no readable SKILL.md, so callers can skip non-skill dirs. +func (r *Reader) skillInfo(name string) (SkillInfo, bool) { + data, err := fs.ReadFile(r.fsys, name+"/SKILL.md") + if err != nil { + return SkillInfo{}, false } - return info + desc, version, metadata := parseFrontmatter(data) + return SkillInfo{Name: name, Description: desc, Version: version, Metadata: metadata}, true } // ListPath lists the direct children (one layer, no recursion) of the directory @@ -73,7 +81,7 @@ func (r *Reader) skillInfo(name string) SkillInfo { // error. Unknown skill, traversal, or a non-directory target → typed validation // error. func (r *Reader) ListPath(arg string) ([]DirEntry, string, error) { - name, sub := splitSkillPath(arg) + name, sub := SplitArg(arg) if err := r.ensureSkill(name); err != nil { return nil, "", err } @@ -108,23 +116,21 @@ func (r *Reader) ListPath(arg string) ([]DirEntry, string, error) { return out, dir, nil } -// splitSkillPath splits "/" into (name, rest); a path with no -// separator yields (path, ""). Only the first '/' splits — rest may itself -// contain separators. -func splitSkillPath(arg string) (name, rest string) { - if i := strings.IndexByte(arg, '/'); i >= 0 { - return arg[:i], arg[i+1:] - } - return arg, "" +// SplitArg splits "/" at the first separator; an argument with no +// separator is a bare skill name (rest ""). It is the single splitter shared by +// `read /` and `list /`. +func SplitArg(arg string) (name, rest string) { + name, rest, _ = strings.Cut(arg, "/") + return name, rest } -// parseFrontmatter extracts the `description` and `metadata` fields from a -// SKILL.md YAML frontmatter block. Both are best-effort: missing or unparseable -// frontmatter yields ("", nil) — never an error. -func parseFrontmatter(skillMD []byte) (description string, metadata map[string]any) { +// parseFrontmatter extracts the `description`, `version`, and `metadata` fields +// from a SKILL.md YAML frontmatter block. All are best-effort: missing or +// unparseable frontmatter yields ("", "", nil) — never an error. +func parseFrontmatter(skillMD []byte) (description, version string, metadata map[string]any) { lines := strings.Split(string(skillMD), "\n") if strings.TrimRight(lines[0], "\r") != "---" { - return "", nil + return "", "", nil } block := make([]string, 0, len(lines)) closed := false @@ -136,16 +142,17 @@ func parseFrontmatter(skillMD []byte) (description string, metadata map[string]a block = append(block, ln) } if !closed { - return "", nil + return "", "", nil } var fm struct { Description string `yaml:"description"` + Version string `yaml:"version"` Metadata map[string]any `yaml:"metadata"` } if err := yaml.Unmarshal([]byte(strings.Join(block, "\n")), &fm); err != nil { - return "", nil + return "", "", nil } - return fm.Description, fm.Metadata + return fm.Description, fm.Version, fm.Metadata } // ReadSkill returns the raw bytes of /SKILL.md. diff --git a/internal/skillcontent/reader_test.go b/internal/skillcontent/reader_test.go index 19ef4489f..cb36d41dc 100644 --- a/internal/skillcontent/reader_test.go +++ b/internal/skillcontent/reader_test.go @@ -14,7 +14,7 @@ import ( func testFS() fstest.MapFS { return fstest.MapFS{ - "lark-calendar/SKILL.md": {Data: []byte("---\nname: lark-calendar\ndescription: \"Calendar skill\"\nmetadata:\n requires:\n bins: [\"lark-cli\"]\n cliHelp: \"lark-cli calendar --help\"\n---\nbody\n")}, + "lark-calendar/SKILL.md": {Data: []byte("---\nname: lark-calendar\nversion: 1.0.0\ndescription: \"Calendar skill\"\nmetadata:\n requires:\n bins: [\"lark-cli\"]\n cliHelp: \"lark-cli calendar --help\"\n---\nbody\n")}, "lark-calendar/references/agenda.md": {Data: []byte("# Agenda")}, "lark-calendar/references/create.md": {Data: []byte("# Create")}, "lark-calendar/assets/tpl.html": {Data: []byte("")}, @@ -38,6 +38,10 @@ func TestList(t *testing.T) { if skills[0].Description != "Calendar skill" { t.Errorf("description: got %q, want %q", skills[0].Description, "Calendar skill") } + // version is the frontmatter `version:` field, passed through for drift checks. + if skills[0].Version != "1.0.0" { + t.Errorf("version: got %q, want %q", skills[0].Version, "1.0.0") + } // metadata is the frontmatter `metadata:` block, passed through verbatim. if skills[0].Metadata == nil { t.Fatal("expected metadata for lark-calendar") @@ -52,6 +56,9 @@ func TestList(t *testing.T) { if skills[1].Metadata != nil { t.Errorf("lark-im metadata: got %v, want nil", skills[1].Metadata) } + if skills[1].Version != "" { + t.Errorf("lark-im version: got %q, want empty", skills[1].Version) + } } func TestListPath(t *testing.T) { @@ -212,12 +219,14 @@ func TestParseFrontmatter(t *testing.T) { name string input string wantDesc string + wantVer string wantHasMeta bool }{ { - name: "description and metadata", - input: "---\ndescription: My skill\nmetadata:\n cliHelp: \"x\"\n---\nbody\n", + name: "description, version and metadata", + input: "---\ndescription: My skill\nversion: 2.1.0\nmetadata:\n cliHelp: \"x\"\n---\nbody\n", wantDesc: "My skill", + wantVer: "2.1.0", wantHasMeta: true, }, { @@ -250,10 +259,13 @@ func TestParseFrontmatter(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - desc, meta := parseFrontmatter([]byte(tc.input)) + desc, ver, meta := parseFrontmatter([]byte(tc.input)) if desc != tc.wantDesc { t.Errorf("description = %q, want %q", desc, tc.wantDesc) } + if ver != tc.wantVer { + t.Errorf("version = %q, want %q", ver, tc.wantVer) + } if (meta != nil) != tc.wantHasMeta { t.Errorf("metadata = %v, wantHasMeta %v", meta, tc.wantHasMeta) } diff --git a/skills_embed.go b/skills_embed.go index 3d8214f2a..7494261fb 100644 --- a/skills_embed.go +++ b/skills_embed.go @@ -5,32 +5,56 @@ package main import ( "embed" + "fmt" "io/fs" + "os" - "github.com/larksuite/cli/cmd/skill" + "github.com/larksuite/cli/cmd" ) -// skillsEmbedFS embeds the entire skills/ tree at build time so the CLI can -// serve skill content that is guaranteed to match the binary version. +// skillsEmbedFS embeds skill content at build time so the CLI can serve content +// guaranteed to match the binary version. // -// We use `//go:embed skills` (not `all:skills`) deliberately: the default form -// excludes files/dirs whose names begin with "." or "_" at any depth, which -// keeps editor/OS junk (e.g. .DS_Store) out of the binary. The trade-off is -// that any future skills/ file intentionally named with a "." or "_" prefix -// would be silently omitted from `skill list` / `skill read` / `skill reference`. +// The patterns whitelist the agent-readable content — each skill's SKILL.md and +// its references/ (plus lark-whiteboard's routes/ and scenes/) — and deliberately +// EXCLUDE machine-resource directories: assets/ (e.g. lark-slides' ~3.4 MB of +// .xml slide templates, which SKILL.md marks as machine resources not to be read +// in full) and scripts/. That keeps ~3.4 MB of never-read bytes out of every +// release binary (≈34.1 → 30.8 MB) while preserving everything `skills read` / +// `skills list` actually serves. // -//go:embed skills +// Trade-offs: (1) this is a whitelist, so content placed in a NEW subdirectory +// type (not SKILL.md / references / routes / scenes) would be silently omitted — +// add a pattern here when introducing one; (2) a pattern that matches zero files +// is a build error, so removing routes/ or scenes/ fails loudly rather than +// silently. ("." / "_"-prefixed files are auto-excluded, as with the plain form.) +// +//go:embed skills/*/SKILL.md skills/*/references skills/*/routes skills/*/scenes var skillsEmbedFS embed.FS +// init registers the embedded skills/ tree as the default skill content +// (rooted at the skill list so paths are "lark-calendar/..."). It runs in the +// standard package build (`go build .`) but NOT the single-file preview build +// (`go build ./main.go`, used by scripts/build-pkg-pr-new.sh) — matching the +// main_*sidecar.go convention of wiring optional features through init side +// effects so main.go stays self-contained and the minimal preview build still +// compiles (it then ships without embedded skills, the pre-existing behavior). +// +// On assembly failure it degrades with a stderr warning rather than panicking: +// the optional skills subsystem must not hold the CLI hostage. The branch is +// effectively unreachable (the compiler rejects a missing skills/ dir; fs.Sub +// only validates path syntax for the literal "skills"), but the trace separates +// that diagnosis from the opaque "not embedded in this build" the commands +// would otherwise report. +// +// Wrapper mains that build their own entrypoint inject content explicitly via +// cmd.Execute(cmd.WithSkillContent(...)) instead; that option overrides this +// default. func init() { - // Strip the "skills/" prefix so paths are "lark-calendar/SKILL.md". sub, err := fs.Sub(skillsEmbedFS, "skills") if err != nil { - // Unreachable for the literal, valid path "skills"; fs.Sub only errors - // on a malformed sub-path. If a refactor ever breaks the embed root, - // fail loud at startup rather than ship a binary whose every `skill` - // command returns an opaque "not embedded in this build" error. - panic("skills_embed: fs.Sub(\"skills\") failed: " + err.Error()) + fmt.Fprintln(os.Stderr, "warning: skills embed assembly failed, skills commands disabled:", err) + return } - skill.ContentFS = sub + cmd.SetEmbeddedSkillContent(sub) }